fix(stack): close codex follow-up — auth races, CORS gaps, missing email-link routes (#77)

* fix(stack): close codex follow-up — auth races, CORS gaps, missing email-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

* fix(auth): keep "Email already verified" branch after atomic claim

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.

Author: agjs

apps/api/src/api/accounts/ownership-transfers.service.ts

@@ -261,47 +261,65 @@ export class OwnershipTransfersService {
   ): Promise<IOwnershipTransfer> {
     const tokenHash = hashOpaqueToken(token);
 
-    const [transfer] = await db
-      .select()
-      .from(accountOwnershipTransfers)
-      .where(
-        and(
-          eq(accountOwnershipTransfers.tokenHash, tokenHash),
-          isNull(accountOwnershipTransfers.acceptedAt),
-          isNull(accountOwnershipTransfers.declinedAt),
-          isNull(accountOwnershipTransfers.cancelledAt)
+    return db.transaction(async (tx) => {
+      /*
+       * Mirrors `accept()`'s `FOR UPDATE`. Without the row lock, an
+       * accept running in parallel commits its UPDATE between this
+       * SELECT and a naive UPDATE-by-id, and both `acceptedAt` and
+       * `declinedAt` end up set. The row lock + a WHERE that repeats
+       * the `acceptedAt IS NULL` predicates on the UPDATE forces the
+       * loser to fall through to a clean 404.
+       */
+      const [transfer] = await tx
+        .select()
+        .from(accountOwnershipTransfers)
+        .where(
+          and(
+            eq(accountOwnershipTransfers.tokenHash, tokenHash),
+            isNull(accountOwnershipTransfers.acceptedAt),
+            isNull(accountOwnershipTransfers.declinedAt),
+            isNull(accountOwnershipTransfers.cancelledAt)
+          )
         )
-      )
-      .limit(1);
+        .limit(1)
+        .for("update");
 
-    if (!transfer) {
-      throw ApiErrors.notFound("Ownership transfer");
-    }
+      if (!transfer) {
+        throw ApiErrors.notFound("Ownership transfer");
+      }
 
-    if (transfer.toUserId !== decliningUserId) {
-      throw ApiErrors.forbidden(
-        "Only the named recipient can decline this transfer"
-      );
-    }
+      if (transfer.toUserId !== decliningUserId) {
+        throw ApiErrors.forbidden(
+          "Only the named recipient can decline this transfer"
+        );
+      }
 
-    const [declined] = await db
-      .update(accountOwnershipTransfers)
-      .set({ declinedAt: now(), updatedAt: now() })
-      .where(eq(accountOwnershipTransfers.id, transfer.id))
-      .returning();
+      const [declined] = await tx
+        .update(accountOwnershipTransfers)
+        .set({ declinedAt: now(), updatedAt: now() })
+        .where(
+          and(
+            eq(accountOwnershipTransfers.id, transfer.id),
+            isNull(accountOwnershipTransfers.acceptedAt),
+            isNull(accountOwnershipTransfers.declinedAt),
+            isNull(accountOwnershipTransfers.cancelledAt)
+          )
+        )
+        .returning();
 
-    if (!declined) {
-      throw ApiErrors.database("Failed to mark transfer declined");
-    }
+      if (!declined) {
+        throw ApiErrors.notFound("Ownership transfer");
+      }
 
-    void auditLogService.record({
-      userId: decliningUserId,
-      action: AUDIT_ACTIONS.ACCOUNT_OWNERSHIP_TRANSFER_DECLINED,
-      resource: `account:${transfer.accountId}`,
-      metadata: { transferId: transfer.id },
-    });
+      void auditLogService.record({
+        userId: decliningUserId,
+        action: AUDIT_ACTIONS.ACCOUNT_OWNERSHIP_TRANSFER_DECLINED,
+        resource: `account:${transfer.accountId}`,
+        metadata: { transferId: transfer.id },
+      });
 
-    return toOwnershipTransfer(declined);
+      return toOwnershipTransfer(declined);
+    });
   }
 
   async cancel(

apps/api/src/api/auth/services/email-verification.service.ts

@@ -37,44 +37,67 @@ export class EmailVerificationService {
    */
   async verify(token: string): Promise<IAuthenticatedResult> {
     const tokenHash = hashOpaqueToken(token);
-    const record = await db.query.emailVerificationTokens.findFirst({
-      where: and(
-        eq(emailVerificationTokens.tokenHash, tokenHash),
-        gt(emailVerificationTokens.expiresAt, now())
-      ),
-    });
-
-    if (!record) {
-      throw ApiErrors.invalidInput("Invalid or expired verification token");
-    }
+    const verifiedAt = now();
 
-    const user = await db.query.users.findFirst({
-      where: eq(users.id, record.userId),
-    });
+    const { user, provisioned } = await db.transaction(async (tx) => {
+      /*
+       * Atomic token claim. Postgres serializes concurrent DELETEs on
+       * the same row: the first call returns the deleted row, every
+       * later call sees an empty RETURNING and falls through to the
+       * "invalid or expired" branch. Without this, two parallel verify
+       * calls both pass a pre-tx existence check, both call
+       * `provisionAfterVerification`, and the user ends up with two
+       * personal accounts because that path also does select-then-
+       * insert under no unique constraint.
+       */
+      const [claimed] = await tx
+        .delete(emailVerificationTokens)
+        .where(
+          and(
+            eq(emailVerificationTokens.tokenHash, tokenHash),
+            gt(emailVerificationTokens.expiresAt, now())
+          )
+        )
+        .returning();
+
+      if (!claimed) {
+        throw ApiErrors.invalidInput("Invalid or expired verification token");
+      }
 
-    if (!user) {
-      throw ApiErrors.notFound("User");
-    }
+      const [claimedUser] = await tx
+        .select()
+        .from(users)
+        .where(eq(users.id, claimed.userId))
+        .limit(1)
+        .for("update");
 
-    if (user.emailVerifiedAt !== null) {
-      throw ApiErrors.invalidInput("Email already verified");
-    }
+      if (!claimedUser) {
+        throw ApiErrors.notFound("User");
+      }
 
-    const verifiedAt = now();
+      /*
+       * Resend can leave a fresh token on a user that's already been
+       * verified through a different link. Reject explicitly instead of
+       * silently re-running the provisioning path — the user-facing
+       * message ("Email already verified") is more informative than
+       * "Invalid or expired" for the legitimate "I clicked the older
+       * email" case.
+       */
+      if (claimedUser.emailVerifiedAt !== null) {
+        throw ApiErrors.invalidInput("Email already verified");
+      }
 
-    const provisioned = await db.transaction(async (tx) => {
       await tx
         .update(users)
         .set({ emailVerifiedAt: verifiedAt, updatedAt: verifiedAt })
-        .where(eq(users.id, user.id));
-      await tx
-        .delete(emailVerificationTokens)
-        .where(eq(emailVerificationTokens.tokenHash, tokenHash));
+        .where(eq(users.id, claimedUser.id));
 
-      return accountsService.provisionAfterVerification(
-        { userId: user.id },
+      const account = await accountsService.provisionAfterVerification(
+        { userId: claimedUser.id },
         tx
       );
+
+      return { user: claimedUser, provisioned: account };
     });
 
     /*

apps/api/src/api/auth/services/password-reset.service.ts

@@ -136,26 +136,38 @@ export class PasswordResetService {
 
   async complete(token: string, newPassword: string): Promise<IMessageResult> {
     const tokenHash = hashOpaqueToken(token);
-    const record = await db.query.passwordResetTokens.findFirst({
-      where: and(
-        eq(passwordResetTokens.tokenHash, tokenHash),
-        gt(passwordResetTokens.expiresAt, now())
-      ),
-    });
+    const passwordHash = await passwordService.hash(newPassword);
 
-    if (!record) {
-      throw ApiErrors.invalidInput("Invalid or expired reset token");
-    }
+    const record = await db.transaction(async (tx) => {
+      /*
+       * Atomic token claim — see email-verification.service.ts for the
+       * full rationale. The pre-hash bcrypt cost runs outside the tx so
+       * a duplicate submission still pays it, but only one call gets
+       * past the DELETE...RETURNING. The loser sees an empty result
+       * and surfaces "Invalid or expired" — the user already-completed
+       * state stays consistent because nothing past the claim runs
+       * twice.
+       */
+      const [claimed] = await tx
+        .delete(passwordResetTokens)
+        .where(
+          and(
+            eq(passwordResetTokens.tokenHash, tokenHash),
+            gt(passwordResetTokens.expiresAt, now())
+          )
+        )
+        .returning();
 
-    const passwordHash = await passwordService.hash(newPassword);
+      if (!claimed) {
+        throw ApiErrors.invalidInput("Invalid or expired reset token");
+      }
 
-    await db.transaction(async (tx) => {
       const updatedProviders = await tx
         .update(userAuthProviders)
         .set({ passwordHash })
         .where(
           and(
-            eq(userAuthProviders.userId, record.userId),
+            eq(userAuthProviders.userId, claimed.userId),
             eq(userAuthProviders.provider, EMAIL_PROVIDER_KEY)
           )
         )
@@ -165,9 +177,7 @@ export class PasswordResetService {
         throw ApiErrors.invalidInput("Password login is not enabled");
       }
 
-      await tx
-        .delete(passwordResetTokens)
-        .where(eq(passwordResetTokens.userId, record.userId));
+      return claimed;
     });
 
     /*

apps/api/src/api/billing/billing.service.ts

@@ -1,4 +1,4 @@
-import { and, eq, isNull } from "drizzle-orm";
+import { and, eq, isNull, or } from "drizzle-orm";
 import Stripe from "stripe";
 
 import { db } from "../../clients/postgres";
@@ -169,17 +169,46 @@ export class BillingService {
     let stripeCustomerId = account.stripeCustomerId;
 
     if (stripeCustomerId === null || stripeCustomerId === "") {
-      const customer = await this.stripe.customers.create({
-        name: account.name,
-        metadata: { accountId: account.id },
-      });
+      /*
+       * Idempotency key keyed on the account id makes Stripe collapse a
+       * double-click into a single customer record. Without it, two
+       * parallel checkout requests each see `stripeCustomerId === null`,
+       * each `customers.create()` returns a *different* id, and the
+       * second DB write wins — the orphaned customer's future webhook
+       * deliveries don't resolve the account and the subscription
+       * silently lands on the wrong tenant. Stripe holds the key for
+       * 24h, which covers any plausible double-submit window.
+       */
+      const customer = await this.stripe.customers.create(
+        {
+          name: account.name,
+          metadata: { accountId: account.id },
+        },
+        { idempotencyKey: `account-customer:${account.id}` }
+      );
 
       stripeCustomerId = customer.id;
 
+      /*
+       * Conditional update — only write the customer id when the column
+       * is still empty. A concurrent request that beat us to the Stripe
+       * API may have already filled it with the same value (idempotency
+       * key collapse) or, in theory, raced past our row in a different
+       * order; either way, leaving the existing value alone keeps the
+       * write idempotent.
+       */
       await db
         .update(accounts)
         .set({ stripeCustomerId })
-        .where(eq(accounts.id, accountId));
+        .where(
+          and(
+            eq(accounts.id, accountId),
+            or(
+              isNull(accounts.stripeCustomerId),
+              eq(accounts.stripeCustomerId, "")
+            )
+          )
+        );
     }
 
     const session = await this.stripe.checkout.sessions.create({

apps/api/src/config/security/security.constants.ts

@@ -21,7 +21,26 @@ export const CORS_ALLOWED_HEADERS = [
   "Content-Type",
   "Authorization",
   "X-Requested-With",
+  /*
+   * Sentry browser SDK writes both the Sentry-native and W3C trace
+   * headers on outbound fetches when `browserTracingIntegration` is on
+   * (see apps/ui/src/app/main.tsx). Without these in the allowlist the
+   * cross-origin preflight rejects the request before it ever reaches
+   * the API, and the SPA degrades to untraced calls.
+   */
+  "sentry-trace",
+  "baggage",
+  "traceparent",
 ];
 
+/**
+ * Response headers the browser is allowed to expose to JS via
+ * `response.headers.get(...)`. Same-origin reads them unconditionally;
+ * cross-origin needs an explicit allowlist. `x-request-id` is the
+ * forensic id our error toasts surface — without it, "ask support for
+ * request id X" breaks the moment the API runs on a different host.
+ */
+export const CORS_EXPOSED_HEADERS = ["x-request-id"];
+
 /** Browser preflight cache TTL in seconds (24h). */
 export const CORS_MAX_AGE_SECONDS = 86_400;

apps/api/src/config/security/security.ts

@@ -4,6 +4,7 @@ import { env } from "../env";
 import { ValkeyRateLimitContext } from "../../lib/rate-limit/valkey-context";
 import {
   CORS_ALLOWED_HEADERS,
+  CORS_EXPOSED_HEADERS,
   CORS_MAX_AGE_SECONDS,
   CORS_METHODS,
 } from "./security.constants";
@@ -25,6 +26,7 @@ export const buildCors = () => {
     credentials: true,
     methods: CORS_METHODS,
     allowedHeaders: CORS_ALLOWED_HEADERS,
+    exposeHeaders: CORS_EXPOSED_HEADERS,
     maxAge: CORS_MAX_AGE_SECONDS,
     aot: true,
   });

apps/api/tests/config/security/security.constants.test.ts

@@ -0,0 +1,29 @@
+import { describe, expect, it } from "bun:test";
+
+import {
+  CORS_ALLOWED_HEADERS,
+  CORS_EXPOSED_HEADERS,
+} from "../../../src/config/security/security.constants";
+
+describe("CORS allowlist", () => {
+  it("allows the Sentry browser SDK trace propagation headers", () => {
+    /*
+     * `browserTracingIntegration` writes both the Sentry-native
+     * `sentry-trace` + `baggage` headers and the W3C `traceparent`
+     * header on outbound /api/* fetches. Cross-origin preflight has
+     * to allow them all or the API never sees the call.
+     */
+    expect(CORS_ALLOWED_HEADERS).toContain("sentry-trace");
+    expect(CORS_ALLOWED_HEADERS).toContain("baggage");
+    expect(CORS_ALLOWED_HEADERS).toContain("traceparent");
+  });
+
+  it("exposes x-request-id to the browser", () => {
+    /*
+     * Error toasts read `response.headers.get("x-request-id")` and
+     * show it for support. Cross-origin reads need an explicit
+     * exposed-headers allowlist.
+     */
+    expect(CORS_EXPOSED_HEADERS).toContain("x-request-id");
+  });
+});