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

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");
+  });
+});