Log email change auth failures to Axiom with structured reason codes - #4268
Log email change auth failures to Axiom with structured reason codes#4268devkiran wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds shared authentication-failure logging and applies it to email-change request and confirmation flows. It records classified errors, request metadata, rate-limit details, and authorization failures before rethrowing errors. ChangesEmail-change authentication logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/lib/actions/confirm-email-change.ts`:
- Around line 78-92: Update the error handling around
assertCanConfirmEmailChange and withAuthFailureLogging so only the typed
authorization error emitted by assertCanConfirmEmailChange is logged with reason
"unauthorized"; classify Prisma or other unexpected failures as "unknown" or
send them through the existing error telemetry path. Preserve the existing user
and email context while deriving the reason from the caught error type.
In `@apps/web/lib/auth/request-email-change.ts`:
- Around line 63-65: Update the generic plus-address validation in the request
email-change flow to evaluate newEmail rather than email, while preserving the
existing isEmailDomainBlocked(newEmail) check and rejection condition. Ensure
the requested address is blocked when it is a generic plus-address, without
rejecting valid work addresses based on the current email.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 201f7305-16e7-45f8-b92c-bb9af60cb679
📒 Files selected for processing (3)
apps/web/lib/actions/confirm-email-change.tsapps/web/lib/auth/log-auth-failure.tsapps/web/lib/auth/request-email-change.ts
| await withAuthFailureLogging( | ||
| { | ||
| action: "email_change_confirm", | ||
| reason: "unauthorized", | ||
| userId: user.id, | ||
| email: user.email ?? undefined, | ||
| }, | ||
| async () => { | ||
| await assertCanConfirmEmailChange({ | ||
| userId: user.id, | ||
| tokenFound, | ||
| data, | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not classify every assertion error as unauthorized.
assertCanConfirmEmailChange performs Prisma queries. If either query fails, withAuthFailureLogging records the error as unauthorized because Line 81 forces that reason. Emit a typed authorization error from assertCanConfirmEmailChange, then assign unauthorized only for that error type. Preserve unexpected errors as unknown or route them to error telemetry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/actions/confirm-email-change.ts` around lines 78 - 92, Update
the error handling around assertCanConfirmEmailChange and withAuthFailureLogging
so only the typed authorization error emitted by assertCanConfirmEmailChange is
logged with reason "unauthorized"; classify Prisma or other unexpected failures
as "unknown" or send them through the existing error telemetry path. Preserve
the existing user and email context while deriving the reason from the caught
error type.
| const isGenericEmailWithPlus = email.includes("+") && isGenericEmail(email); | ||
| const emailDomainBlocked = await isEmailDomainBlocked(newEmail); | ||
| if (isGenericEmailWithPlus || emailDomainBlocked) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate newEmail for generic plus-addresses.
isGenericEmailWithPlus checks the current email, but this flow validates the requested newEmail. A generic plus-address can bypass this check when it is the requested address. A user with a generic plus-address can also be blocked from changing to a valid work address.
Proposed fix
- const isGenericEmailWithPlus = email.includes("+") && isGenericEmail(email);
+ const isGenericEmailWithPlus =
+ newEmail.includes("+") && isGenericEmail(newEmail);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isGenericEmailWithPlus = email.includes("+") && isGenericEmail(email); | |
| const emailDomainBlocked = await isEmailDomainBlocked(newEmail); | |
| if (isGenericEmailWithPlus || emailDomainBlocked) { | |
| const isGenericEmailWithPlus = | |
| newEmail.includes("+") && isGenericEmail(newEmail); | |
| const emailDomainBlocked = await isEmailDomainBlocked(newEmail); | |
| if (isGenericEmailWithPlus || emailDomainBlocked) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/auth/request-email-change.ts` around lines 63 - 65, Update the
generic plus-address validation in the request email-change flow to evaluate
newEmail rather than email, while preserving the existing
isEmailDomainBlocked(newEmail) check and rejection condition. Ensure the
requested address is blocked when it is a generic plus-address, without
rejecting valid work addresses based on the current email.
1 participant
Summary by CodeRabbit