From 7c9b4b65637142a1e4a09b0a8e979601af32b1c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 14 May 2026 22:30:02 +0200 Subject: [PATCH] fix(onboarding): conditional step write + 409 on concurrent advance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two parallel tabs both reading `onboardingStep = 2` and both POSTing `step: 3` would both succeed because the second write didn't re-check the precondition — the wizard could double-advance and skip a step. Move the write onto `updateMany` with the precondition embedded in the WHERE clause: exactly one update lands, the loser sees count = 0 and returns 409 so the client can refresh and retry. The WHERE clause covers `onboardingCompletedAt: null` so the same guard catches a race with `POST /api/onboarding/complete`. The `current === 0` branch widens the match to `step IS NULL OR step = 0` so legacy pre-W14b accounts still advance from step 1. Fix-M / code-M7. --- src/app/api/onboarding/__tests__/step.test.ts | 255 ++++++++++++++++++ src/app/api/onboarding/step/route.ts | 40 ++- 2 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 src/app/api/onboarding/__tests__/step.test.ts diff --git a/src/app/api/onboarding/__tests__/step.test.ts b/src/app/api/onboarding/__tests__/step.test.ts new file mode 100644 index 000000000..e35459068 --- /dev/null +++ b/src/app/api/onboarding/__tests__/step.test.ts @@ -0,0 +1,255 @@ +/** + * v1.4.25 W21 Fix-M — concurrency regression for + * `POST /api/onboarding/step`. + * + * The step write was fetch-then-update: two parallel tabs both + * reading `onboardingStep = 2` and both POSTing `step: 3` would both + * succeed because the second write didn't re-check the precondition. + * The route now claims the row via `updateMany` with the precondition + * embedded in the WHERE clause, so exactly one update lands and the + * loser sees `count = 0` and returns 409. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/db", () => ({ + prisma: { + user: { + findUnique: vi.fn(), + findUniqueOrThrow: vi.fn(), + update: vi.fn(), + updateMany: vi.fn(), + }, + }, +})); + +vi.mock("@/lib/auth/session", () => ({ + getSession: vi.fn(), + setOnboardingPendingCookie: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn().mockResolvedValue({ + allowed: true, + remaining: 29, + resetAt: Date.now() + 60_000, + }), + rateLimitHeaders: vi.fn(() => ({})), +})); + +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { POST } from "../step/route"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; +import { checkRateLimit } from "@/lib/rate-limit"; + +const SESSION_OK = { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "user-1", username: "marc", role: "USER" as const }, +}; + +function req(body: unknown): NextRequest { + return new NextRequest("http://localhost/api/onboarding/step", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + remaining: 29, + resetAt: Date.now() + 60_000, + }); +}); + +describe("POST /api/onboarding/step — happy path", () => { + it("advances 0 → 1 on the first wizard submission", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: 0, + onboardingCompletedAt: null, + } as never); + vi.mocked(prisma.user.updateMany).mockResolvedValue({ count: 1 } as never); + vi.mocked(prisma.user.findUniqueOrThrow).mockResolvedValue({ + onboardingStep: 1, + onboardingCompletedAt: null, + } as never); + + const res = await POST(req({ step: 1 })); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { step: number } }; + expect(body.data.step).toBe(1); + }); + + it("marks completion on step 4 (sets onboardingCompletedAt)", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: 3, + onboardingCompletedAt: null, + } as never); + vi.mocked(prisma.user.updateMany).mockResolvedValue({ count: 1 } as never); + vi.mocked(prisma.user.findUniqueOrThrow).mockResolvedValue({ + onboardingStep: 4, + onboardingCompletedAt: new Date("2026-05-14T12:00:00Z"), + } as never); + + const res = await POST(req({ step: 4 })); + expect(res.status).toBe(200); + const updateCall = vi.mocked(prisma.user.updateMany).mock.calls[0][0]; + expect(updateCall.data).toMatchObject({ + onboardingStep: 4, + }); + expect(updateCall.data).toHaveProperty("onboardingCompletedAt"); + }); +}); + +describe("POST /api/onboarding/step — guard rails", () => { + it("returns 401 when unauthenticated", async () => { + vi.mocked(getSession).mockResolvedValue(null); + const res = await POST(req({ step: 1 })); + expect(res.status).toBe(401); + }); + + it("returns 422 for an out-of-range step value", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + const res = await POST(req({ step: 99 })); + expect(res.status).toBe(422); + }); + + it("returns 409 when the user has already completed onboarding", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: 4, + onboardingCompletedAt: new Date("2026-05-01T12:00:00Z"), + } as never); + + const res = await POST(req({ step: 1 })); + expect(res.status).toBe(409); + expect(prisma.user.updateMany).not.toHaveBeenCalled(); + }); + + it("returns 409 when the requested step is out of order", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: 1, + onboardingCompletedAt: null, + } as never); + + // The route sees current = 1; submitting step 3 (skipping 2) must + // be rejected before the write. + const res = await POST(req({ step: 3 })); + expect(res.status).toBe(409); + expect(prisma.user.updateMany).not.toHaveBeenCalled(); + }); +}); + +describe("POST /api/onboarding/step — concurrent write race", () => { + it("returns 409 when the conditional update affects zero rows", async () => { + // Regression for Fix-M / code-M7: two parallel tabs both read + // `onboardingStep = 2`, both POST `step: 3`. The first lands; + // when the second runs the conditional updateMany, the WHERE + // clause no longer matches (the column moved to 3) and Prisma + // returns `count = 0`. The route must surface 409 so the client + // can refresh and retry instead of double-advancing the wizard. + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: 2, + onboardingCompletedAt: null, + } as never); + vi.mocked(prisma.user.updateMany).mockResolvedValue({ count: 0 } as never); + + const res = await POST(req({ step: 3 })); + expect(res.status).toBe(409); + // findUniqueOrThrow must NOT be called when the claim fails — + // the route returns before re-reading the row. + expect(prisma.user.findUniqueOrThrow).not.toHaveBeenCalled(); + }); + + it("includes the precondition in the updateMany WHERE clause", async () => { + // The whole point of the conditional update is that the WHERE + // clause re-asserts the precondition we read. Pin the contract + // so a future refactor can't quietly drop the guard. + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: 2, + onboardingCompletedAt: null, + } as never); + vi.mocked(prisma.user.updateMany).mockResolvedValue({ count: 1 } as never); + vi.mocked(prisma.user.findUniqueOrThrow).mockResolvedValue({ + onboardingStep: 3, + onboardingCompletedAt: null, + } as never); + + await POST(req({ step: 3 })); + const whereArg = vi.mocked(prisma.user.updateMany).mock.calls[0][0] + .where as Record; + expect(whereArg).toMatchObject({ + id: "user-1", + onboardingCompletedAt: null, + }); + // Either the literal { onboardingStep: 2 } form (current > 0) or + // the legacy null-OR-zero form when current === 0. Here current + // is 2 so we expect the strict-equality form. + expect(whereArg.onboardingStep).toEqual({ in: [2] }); + }); + + it("widens the precondition to null OR 0 when the row is a legacy pre-W14b user", async () => { + // Legacy accounts may have `onboarding_step IS NULL`. The + // conditional update must still claim them on step 1. + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + onboardingStep: null, + onboardingCompletedAt: null, + } as never); + vi.mocked(prisma.user.updateMany).mockResolvedValue({ count: 1 } as never); + vi.mocked(prisma.user.findUniqueOrThrow).mockResolvedValue({ + onboardingStep: 1, + onboardingCompletedAt: null, + } as never); + + const res = await POST(req({ step: 1 })); + expect(res.status).toBe(200); + const whereArg = vi.mocked(prisma.user.updateMany).mock.calls[0][0] + .where as Record; + expect(whereArg.OR).toEqual( + expect.arrayContaining([ + { onboardingStep: 0 }, + { onboardingStep: null }, + ]), + ); + }); +}); + +describe("POST /api/onboarding/step — rate limit", () => { + it("returns 429 when the per-user rate limit is exceeded", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(checkRateLimit).mockResolvedValueOnce({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + }); + const res = await POST(req({ step: 1 })); + expect(res.status).toBe(429); + }); +}); diff --git a/src/app/api/onboarding/step/route.ts b/src/app/api/onboarding/step/route.ts index e8e44800e..d1ce29e3e 100644 --- a/src/app/api/onboarding/step/route.ts +++ b/src/app/api/onboarding/step/route.ts @@ -109,12 +109,48 @@ export const POST = apiHandler(async (request: NextRequest) => { } const completing = step === 4; - const updated = await prisma.user.update({ - where: { id: user.id }, + + // Conditional update — the WHERE clause re-asserts the + // precondition we read above so the write can only land on a row + // whose state still matches what we validated. Two parallel tabs + // both reading `onboardingStep = 2` and both POSTing `step: 3` + // would otherwise both succeed (the read-then-write race); with + // this guard exactly one update sees `count = 1`, the second sees + // `count = 0` and returns 409. Legacy pre-W14b accounts may have + // `onboarding_step IS NULL`, so the `current === 0` branch widens + // the match to null OR 0 — the wizard treats both as "not + // started". `onboardingCompletedAt: null` also catches a race with + // `POST /api/onboarding/complete`. + const claimed = await prisma.user.updateMany({ + where: + current === 0 + ? { + id: user.id, + onboardingCompletedAt: null, + OR: [{ onboardingStep: 0 }, { onboardingStep: null }], + } + : { + id: user.id, + onboardingCompletedAt: null, + onboardingStep: { in: [current] }, + }, data: { onboardingStep: step, ...(completing ? { onboardingCompletedAt: new Date() } : {}), }, + }); + if (claimed.count !== 1) { + annotate({ + action: { name: "onboarding.step" }, + meta: { outcome: "concurrent_write", current, requested: step }, + }); + return apiError( + "Onboarding step changed concurrently, refresh and retry", + 409, + ); + } + const updated = await prisma.user.findUniqueOrThrow({ + where: { id: user.id }, select: { onboardingStep: true, onboardingCompletedAt: true,