fix(stack): close codex follow-up — auth races, CORS gaps, missing email-link routes - #77
Merged
Merged
Conversation
…ail-link routes
Second Codex pass uncovered six gaps. This PR ships fixes for all six, each
with its own guardrail or sibling test.
## Atomicity in single-use token flows
* Email verification: token lookup moves INSIDE the transaction as an atomic
`DELETE … RETURNING`. The user row is then locked `FOR UPDATE`. Two parallel
verify calls used to both pass a pre-tx existence check and both call
`provisionAfterVerification` (which itself does select-then-insert against
no unique constraint) — the user ended up with two personal accounts. The
atomic claim collapses the race to one winner.
* Password reset: same shape — atomic `DELETE … RETURNING` inside the tx.
The bcrypt hash still runs outside the tx so it doesn't pin a connection,
but only the call that wins the DELETE proceeds to UPDATE the password.
* Ownership transfer decline: now transactional with the same `FOR UPDATE`
lock + repeated `acceptedAt/declinedAt IS NULL` predicates on the UPDATE
that accept() uses. Without it, an accept/decline race could leave BOTH
timestamps set.
## Billing customer creation
* Stripe `customers.create` now passes `idempotencyKey: account-customer:{id}`
so a double-click collapses to one customer record. The DB write-back is
conditional (`stripeCustomerId IS NULL OR ''`) so a concurrent caller
that beat us to the API doesn't get overwritten. Stripe holds idempotency
keys for 24h — well beyond any plausible double-submit window.
## CORS for cross-origin Sentry tracing
* Added `sentry-trace`, `baggage`, `traceparent` to `CORS_ALLOWED_HEADERS`.
The Sentry browser SDK writes these on every outbound /api/* fetch when
`browserTracingIntegration` is loaded; without the allowlist the cross-
origin preflight rejects every traced call.
* Added `exposeHeaders: ['x-request-id']` so the SPA's error toasts can
still surface the request id when the API runs on a different host.
* Sibling test in `security.constants.test.ts` locks both behaviours.
## Three missing email-link landing routes
* `/invitations/accept` — auto-fires the accept call (mirrors VerifyEmailPage
shape). ProtectedRoute-wrapped so anonymous clicks land on /login first
and come back with the `?token=` preserved.
* `/account/ownership-transfer/accept` — renders Accept and Decline buttons,
NEVER auto-fires. Accepting transfers a whole account — a stray click on
the email shouldn't perform it. The API also enforces recipient-JWT match.
* `/account/requests` — reviewer-side inbox for domain-join requests with
approve/deny actions. Lives inside AppShell. The API enforces role on
every mutation; we don't add a second gate.
Each new page ships with `.hooks.ts`, `.utils.ts`, `.types.ts`,
`.constants.ts`, `.stories.tsx`, `.test.tsx`, `.utils.test.ts`, plus
matching sibling `.hooks.test.tsx`. i18n keys added to both en and de
common.json bundles. Log event names registered in `logger.events.ts`.
## Out of scope (P2/P3 from the Codex pass)
* P2: OpenAPI fidelity — `success: boolean` → `t.Literal(true)`, dropping
the multipart/text-plain default. Touches every route in the API and is
pure schema hygiene; deferred to its own PR.
* P3: UI/API authorization rule duplication — would need a generated rule
matrix or parity test, neither of which fits a follow-up.
## Gates
* API: 1033/1035 (2 DB-only skipped — local pg unavailable)
* UI: 572/572
* Both apps: lint + lint-meta + typecheck + knip clean
Codex follow-up tightened email verify to use atomic DELETE...RETURNING on the token row, which collapsed the duplicate-account race. But it also removed the explicit "already verified" check — a second click on an older verification link (still a valid token row from a prior resend) would now silently re-run the provisioning idempotency path instead of telling the user the email is already verified. Reinstates the check inside the transaction, after the user row is locked FOR UPDATE. Both contracts hold: races are atomic, and a stale verification email surfaces the user-friendly error string the integration test still asserts.
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
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.
Summary
Second Codex review uncovered six findings. This PR ships fixes for all of them.
Race fixes (API)
DELETE … RETURNING. The user row is then lockedFOR UPDATE. Two parallel verify calls used to both pass a pre-tx existence check and both callprovisionAfterVerification(which itself does select-then-insert against no unique constraint) — the user ended up with two personal accounts. The atomic claim collapses the race to one winner. The "Email already verified" branch is preserved inside the tx so a click on an older resend link still gets the friendly error string.DELETE … RETURNINGinside the tx. The bcrypt hash runs outside the tx so it doesn't pin a connection, but only the call that wins the DELETE proceeds to UPDATE the password.FOR UPDATElock + repeatedacceptedAt/declinedAt IS NULLpredicates on the UPDATE that accept() already uses. Without it, an accept/decline race could leave BOTH timestamps set.Billing customer creation (API)
stripe.customers.createnow passesidempotencyKey: account-customer:{id}so a double-click collapses to one customer record. The DB write-back is conditional (stripeCustomerId IS NULL OR '') so a concurrent caller that beat us to the API doesn't get overwritten.CORS for cross-origin Sentry tracing (API)
sentry-trace,baggage,traceparenttoCORS_ALLOWED_HEADERS. The Sentry browser SDK writes these on every outbound/api/*fetch whenbrowserTracingIntegrationis loaded — without the allowlist, cross-origin preflight rejects every traced call.exposeHeaders: ['x-request-id']so the SPA's error toasts can still surface the request id when the API runs on a different host.Three missing email-link landing routes (UI)
/invitations/accept— auto-fires the accept call (mirrors VerifyEmailPage). ProtectedRoute-wrapped so anonymous clicks land on/loginfirst and come back with the?token=preserved./account/ownership-transfer/accept— renders Accept and Decline buttons, NEVER auto-fires (a stray click on the email shouldn't transfer a whole account). API enforces recipient-JWT match./account/requests— reviewer-side inbox for domain-join requests with approve/deny actions. Lives inside AppShell.Each new page ships with
.hooks.ts,.utils.ts,.types.ts,.constants.ts,.stories.tsx,.test.tsx,.utils.test.ts, plus matching sibling.hooks.test.tsx. i18n keys added to bothenanddecommon.json. Log event names registered inlogger.events.ts.Out of scope (P2/P3 from the Codex pass)
success: boolean→t.Literal(true), dropping the multipart/text-plain content-type default. Touches every route in the API and is pure schema hygiene; deferred to its own PR.Test plan
EmailVerificationService.verifyintegration test: "rejects a second verification attempt for an already-verified user" still passes after the atomic refactor/account/invitations/account/requests