fix(emails): record the account on rejected sends (chat#1889 row 27) - #790
Open
sweetmantech wants to merge 1 commit into
Open
fix(emails): record the account on rejected sends (chat#1889 row 27)#790sweetmantech wants to merge 1 commit into
sweetmantech wants to merge 1 commit into
Conversation
sendEmailHandler set attempt = { status: 'rejected' } with no account, so
every account-scoped email_send_log audit under-reported. In the
2026-07-27 scheduled-run incident an account-scoped query returned 2 of 8
attempts; the six rejections were findable only by filtering raw_body on
the recipient -- and recoup-internal-task-email-audit is account-scoped,
so it under-reports the same way.
validateSendEmailBody now returns the best-known account on its error
branch: the authenticated account where auth succeeded and the request
was rejected later, and the account the body claimed when auth itself
failed (the expired-credential case that motivated this). Documented as
attribution, never as evidence of authorization.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRejected email validation outcomes now preserve ChangesEmail rejection context
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
sweetmantech
changed the title
31 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/emails/validateSendEmailBody.ts`:
- Line 95: Reduce oversized domain functions by extracting cohesive helpers from
validateSendEmailBody for parsing, authentication, recipient resolution, and
recipient validation, while preserving existing behavior; also extract
send-result or audit-attempt construction from sendEmailHandler. Apply the
changes in lib/emails/validateSendEmailBody.ts at lines 95-95 and
lib/emails/sendEmailHandler.ts at lines 59-63, keeping both domain functions
under the repository’s size limits.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ff98d50-31f7-4705-b0a5-f6e7859e7851
⛔ Files ignored due to path filters (2)
lib/emails/__tests__/sendEmailHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/emails/__tests__/validateSendEmailBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (2)
lib/emails/sendEmailHandler.tslib/emails/validateSendEmailBody.ts
| const authContext = await validateAuthContext(request, { accountId: result.data.account_id }); | ||
| if (authContext instanceof NextResponse) { | ||
| return { rawBody, error: authContext }; | ||
| return { rawBody, accountId: result.data.account_id, error: authContext }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Reduce oversized domain functions.
Both changed areas are inside functions that exceed the repository’s function-size limits:
lib/emails/validateSendEmailBody.ts#L95-L95,105-105,121-121: extract parsing, authentication, recipient resolution, and recipient checks.lib/emails/sendEmailHandler.ts#L59-L63: extract send-result or audit-attempt construction.
As per coding guidelines, functions longer than 20 lines must be flagged. As per path instructions, domain functions must remain under 50 lines.
📍 Affects 2 files
lib/emails/validateSendEmailBody.ts#L95-L95(this comment)lib/emails/sendEmailHandler.ts#L59-L63
🤖 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 `@lib/emails/validateSendEmailBody.ts` at line 95, Reduce oversized domain
functions by extracting cohesive helpers from validateSendEmailBody for parsing,
authentication, recipient resolution, and recipient validation, while preserving
existing behavior; also extract send-result or audit-attempt construction from
sendEmailHandler. Apply the changes in lib/emails/validateSendEmailBody.ts at
lines 95-95 and lib/emails/sendEmailHandler.ts at lines 59-63, keeping both
domain functions under the repository’s size limits.
Sources: Coding guidelines, Path instructions
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Client
participant Handler as sendEmailHandler
participant Validator as validateSendEmailBody
participant Auth as validateAuthContext
participant RecipientCheck as assertRecipientsAllowed
participant Logger as logEmailAttempt
Note over Client,Logger: POST /api/emails — Rejection paths (NEW accountId flow)
Client->>Handler: POST /api/emails (body with account_id)
Handler->>Validator: validateSendEmailBody(request)
alt Body schema invalid (400)
Validator-->>Handler: { rawBody, error, accountId: undefined }
Note over Validator: No account resolved
else Schema valid
Validator->>Auth: validateAuthContext(request, { accountId: body.account_id })
alt Auth fails (401)
Auth-->>Validator: NextResponse error
Validator-->>Handler: { rawBody, error, accountId: body.account_id }
Note over Validator: NEW: use body's claimed account_id
else Auth passes
Auth-->>Validator: authContext
Validator->>RecipientCheck: assertRecipientsAllowed(authenticated account)
alt No recipients (400) or restricted (403)
RecipientCheck-->>Validator: { allowed: false }
Validator-->>Handler: { rawBody, error, accountId: authContext.accountId }
else Allowed
Validator-->>Handler: { rawBody, data }
end
end
end
alt Error path (rejected)
Handler->>Handler: attempt = { status: "rejected", accountId: validated.accountId }
Note over Handler: NEW: carry accountId from validation
Handler->>Logger: logEmailAttempt({ rawBody, ...attempt })
else Success path (unchanged)
Handler->>Handler: attempt = { status: "sent", ... }
Handler->>Logger: logEmailAttempt({ rawBody, ...attempt })
Handler-->>Client: 200
end
Auto-approved: Bug fix for email audit logging: records accountId on rejected sends so account-scoped queries include failures. Change is bounded (internal audit only), clearly beneficial, and the trade-off for auth failures is documented and acceptable.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Matrix row 27 in chat#1889.
Why
sendEmailHandler.tsL59 setattempt = { status: "rejected" }with no account, so every account-scopedemail_send_logaudit under-reported.Concretely, in the 2026-07-27 scheduled-run incident: 8
POST /api/emailsattempts, 2sentand 6rejected. An account-scoped query returned 2 of 8 — the six rejections were findable only by filteringraw_bodyon the recipient. Therecoup-internal-task-email-auditskill is account-scoped, so it under-reports the same way. This is why the incident went unnoticed.What changed
validateSendEmailBody's error branch now carriesaccountIdwherever one can be resolved, and the handler records it:account_id, already schema-validated as a UUID)The auth-failure row is the one that matters: the six rejections in the incident were
validateAuthContextfailures after the sandbox credential expired ~1h from kickoff, so an expired credential is precisely how a legitimate account's sends vanish from the audit.Deliberate trade-off, worth a reviewer's eye
On the auth-failed path the recorded id is the account the request claimed, not one we verified. That is the only thing knowable there, and attribution is the point of the log — but it does mean a caller could write rows referencing an account they don't hold. I judged that acceptable because the row is an internal audit record, its
statusisrejected(so it is never evidence of a successful action), andraw_bodyis stored alongside. It is documented onValidateSendEmailResultas "the best-known account, not proof of authorization".If you'd rather not record unverified ids, the alternative is to carry only the authenticated cases — but then the incident's six rejections still would not be attributable, which defeats the row. Happy to switch on your call.
Verification
TDD, red → green:
Three tests: the handler forwards a resolved account, the handler still logs when none was resolved (so the fix can't regress into dropping rejections entirely), and
validateSendEmailBodysurfaces the authenticated account on the 403 path.pnpm exec tsc --noEmit→ 236, identical tomain's baseline, none in the touched files.eslintclean.No docs change:
POST /api/emails's request/response contract is unchanged — this only affects what we write toemail_send_log.Preview verification to follow on this PR.
Tracked in chat#1889 (matrix row 27).
Summary by cubic
Record the best-known account on rejected email sends so account-scoped
email_send_logaudits include failures, addressing chat#1889 (row 26). This restores visibility for rejections, including those caused by expired credentials.validateSendEmailBodynow returnsaccountIdon error:account_id(schema-validated UUID).sendEmailHandlerrecordsaccountIdforrejectedattempts.Written for commit 4e2de5d. Summary will update on new commits.
Summary by CodeRabbit