diff --git a/nextjs_space/app/api/consultation/submit/route.ts b/nextjs_space/app/api/consultation/submit/route.ts index e764c882..9e5b5301 100644 --- a/nextjs_space/app/api/consultation/submit/route.ts +++ b/nextjs_space/app/api/consultation/submit/route.ts @@ -1,5 +1,7 @@ import { NextRequest, NextResponse } from "next/server"; import { clerkClient } from "@clerk/nextjs/server"; +import { emailsMatch } from "@/lib/security/email-ownership"; +import { getVerifiedSessionEmail } from "@/lib/security/session-email"; import { createAuditLog, AUDIT_ACTIONS, getClientInfo } from "@/lib/audit-log"; import { triggerWebhook, WEBHOOK_EVENTS } from "@/lib/integrations/webhook"; @@ -25,6 +27,16 @@ import { logger } from '@/lib/logger'; import { apiError, apiValidationError } from '@/lib/api-error'; import { checkPolicyGate } from '@/lib/legal/policy-gate'; +/** 409 for "that address already belongs to an account you have not proven you own". */ +function accountExistsResponse() { + return apiError(new Error("Account already exists for this email"), { + route: "POST /api/consultation/submit", + status: 409, + safeMessage: + "An account already exists for this email address. Please sign in and then complete your consultation. If you cannot access that account, contact support — for your protection we cannot link it from an unauthenticated form.", + }); +} + // SECURITY (C1, C13): Strict whitelist schema — no `.passthrough()`. Every // field that lands in the database or is forwarded to Dr. Green must be // declared here and length-capped. The tenant is resolved server-side from @@ -173,6 +185,51 @@ export async function POST(request: NextRequest) { }); } + // SECURITY (account takeover): this route is PUBLIC — the caller has not + // proven they own `body.email`, anyone can type anyone's address. Creating + // records for a BRAND-NEW address is fine; touching an address that + // already has an account is not. Without the ownership gate below, an + // anonymous caller could submit a victim's email, have Clerk's + // "already exists" swallowed, and reach the linking step further down that + // re-points the VICTIM's users row at a Dr Green client the ATTACKER + // controls — so once the attacker's own (genuine-looking) ID is approved, + // the victim's account inherits VERIFIED. + // + // Ownership over anything that already exists is provable ONE way: an + // authenticated session for that address. Notably NOT by Clerk accepting a + // new account below — that only proves nobody held the *Clerk* identity, + // which says nothing about a local users row that predates this request + // (legacy import, or a dropped Clerk delete-webhook). + const sessionOwnsEmail = emailsMatch(await getVerifiedSessionEmail(), body.email); + + // Check if user already exists locally (email is globally unique, don't filter by tenantId) + const existingUser = await prisma.users.findUnique({ + where: { email: body.email.toLowerCase() }, + }); + + // THE ownership gate. It runs BEFORE Clerk is touched, which matters: an + // address with a local row but no Clerk account (legacy import, dropped + // delete-webhook) is one Clerk will happily mint for ANYONE. Gating after + // that call left a two-request takeover — mint an account for the target + // address (Clerk performs no mailbox-control check; the password is the + // submitter's own), sign in with it, resubmit, and the session now + // "proves" ownership of a row the caller never owned. Refusing before the + // mint means there is no account to sign into, and no squatting on the + // address either. + // + // Consequence, deliberately accepted: the real owner of a local row with + // no Clerk account cannot self-serve through this route. Restoring that + // needs a flow that actually proves mailbox control (Clerk verification / + // password reset), never "sign up again" — which is indistinguishable + // from the attack. + if (existingUser && !sessionOwnsEmail) { + logger.warn( + "[Consultation] refused submission for an existing account by a caller not signed in as it", + { tenantId }, + ); + return accountExistsResponse(); + } + // 1. Create Clerk User (Auth) let clerkUser; try { @@ -190,10 +247,18 @@ export async function POST(request: NextRequest) { }, }); } catch (clerkError: any) { - // Ignore if user already exists in Clerk, proceed to DB/DrGreen + // The address already has an account. Continuing "to DB/DrGreen" here + // is what let an anonymous caller operate on someone else's row — + // refuse unless they are signed in as that address. if (clerkError.errors?.[0]?.code === "form_identifier_exists") { - logger.info("[Consultation] user already exists in Clerk", { tenantId }); - // Optionally fetch the user to get their ID if needed, but for now we proceed + if (!sessionOwnsEmail) { + logger.warn( + "[Consultation] refused submission for an existing address by a caller not signed in as it", + { tenantId }, + ); + return accountExistsResponse(); + } + logger.info("[Consultation] existing Clerk account, caller is signed in as it", { tenantId }); } else { throw clerkError; // Re-throw other errors (e.g., weak password) } @@ -212,11 +277,6 @@ export async function POST(request: NextRequest) { ); } - // Check if user already exists locally (email is globally unique, don't filter by tenantId) - const existingUser = await prisma.users.findUnique({ - where: { email: body.email.toLowerCase() }, - }); - let userId: string | undefined; if (existingUser) { @@ -269,12 +329,29 @@ export async function POST(request: NextRequest) { userId = newUser.id; logger.info("[Consultation] created local user mirror", { userId, tenantId }); } catch (prismaError: any) { - // Race condition: Clerk webhook may have created the user between our check and create + // Race condition: Clerk webhook may have created the user between our + // check and create. The ownership gate saw no row for this address, so + // one appeared mid-request — but "it must therefore be ours" is an + // assumption, not a guarantee (another writer could land a row for the + // same address in that window), so the adopt below re-checks rather + // than trusting it. if (prismaError.code === "P2002") { const raceUser = await prisma.users.findUnique({ where: { email: body.email.toLowerCase() }, }); if (raceUser) { + // Adopt only what this request is entitled to: the row is the + // mirror of the Clerk account we just minted (ids match, since + // the local id IS clerkUser.id), or the caller is signed in as + // the address. Anything else is a stranger's row that landed in + // the race window — refuse rather than adopt it. + if (!(clerkUser && raceUser.id === clerkUser.id) && !sessionOwnsEmail) { + logger.warn( + "[Consultation] refused a raced row that this request did not create", + { tenantId }, + ); + return accountExistsResponse(); + } userId = raceUser.id; if (!raceUser.tenantId) { await prisma.users.update({ @@ -588,8 +665,15 @@ export async function POST(request: NextRequest) { }, }); - // CRITICAL FIX: Also update the User record with the Dr. Green Client ID - // This is required for the kyc-check to work, as it looks at the User table. + // Also update the User record with the Dr. Green Client ID — required + // for the kyc-check, which reads the User table. + // + // SECURITY: the write an account-takeover targets. Safe by the ownership + // gate above — `userId` is either a row THIS request created, or a + // pre-existing row whose address the caller is signed in as. Enforced + // there and regression-tested in + // tests/unit/consultation-submit-ownership.test.ts; a re-check here + // would be unfirable by construction (the mistake the first cut made). if (userId) { await prisma.users.update({ where: { id: userId }, diff --git a/nextjs_space/lib/security/email-ownership.ts b/nextjs_space/lib/security/email-ownership.ts new file mode 100644 index 00000000..a1bf68db --- /dev/null +++ b/nextjs_space/lib/security/email-ownership.ts @@ -0,0 +1,34 @@ +/** + * Ownership of an email address on PUBLIC endpoints. + * + * A signup-style endpoint cannot assume the caller owns the address they + * typed — anyone can type anyone's. That is fine while the request only + * CREATES records for that address, and dangerous the moment it MUTATES a + * record that already existed: re-pointing an existing user's tenant binding + * or their external client id lets an attacker attach a stranger's account to + * a record the attacker controls, and that account then inherits whatever + * status the record earns (approval, verification…). + * + * The ONLY proof of ownership over a pre-existing record is an authenticated + * session for that address — hence this module exposes just the comparison. + * + * Explicitly NOT proof: "the identity provider accepted a brand-new account + * for this address in this request". An earlier version of this module + * offered that as a second route to ownership, which is wrong and was + * exploitable: the provider only vouches that nobody held the *provider's* + * identity, which says nothing about a local row that predates the request + * (a legacy import, or a dropped delete-webhook leaving an orphaned row). + * A caller who mints a fresh provider account for a stranger's address must + * still not be able to mutate that stranger's existing row. + */ + +/** Case/whitespace-insensitive address comparison. Null/empty never matches. */ +export function emailsMatch( + a: string | null | undefined, + b: string | null | undefined, +): boolean { + const left = a?.trim().toLowerCase(); + const right = b?.trim().toLowerCase(); + if (!left || !right) return false; + return left === right; +} diff --git a/nextjs_space/lib/security/session-email.ts b/nextjs_space/lib/security/session-email.ts new file mode 100644 index 00000000..e312136c --- /dev/null +++ b/nextjs_space/lib/security/session-email.ts @@ -0,0 +1,36 @@ +import { currentUser } from "@clerk/nextjs/server"; + +/** + * The signed-in caller's VERIFIED PRIMARY email address, or null. + * + * Kept apart from `./email-ownership` so that module stays pure and + * dependency-free; this one owns the Clerk read. + * + * Two deliberate choices, both because the result is used as an ownership + * claim over existing records: + * - Clerk-direct rather than `getCurrentUser()`: callers include PUBLIC + * routes that must keep working for anonymous visitors, and getCurrentUser + * additionally resolves tenants and can throw for not-yet-provisioned or + * multi-tenant accounts. All that is needed here is which address, if any, + * this caller has already authenticated as. + * - Primary AND verified: the positionally-first address is not necessarily + * the one the session is anchored to, and an unverified address proves + * nothing about who controls the mailbox. Clerk is expected to allow only + * verified addresses as primary — asserting it here means the guarantee + * does not depend on that remaining true. + */ +export async function getVerifiedSessionEmail(): Promise { + try { + const sessionUser = await currentUser(); + if (!sessionUser) return null; + const primary = sessionUser.emailAddresses?.find( + (address) => + address.id === sessionUser.primaryEmailAddressId && + address.verification?.status === "verified", + ); + return primary?.emailAddress ?? null; + } catch { + // Expired/invalid token — treat as anonymous, never as an error. + return null; + } +} diff --git a/nextjs_space/tests/unit/consultation-submit-ownership.test.ts b/nextjs_space/tests/unit/consultation-submit-ownership.test.ts new file mode 100644 index 00000000..55f9b6bf --- /dev/null +++ b/nextjs_space/tests/unit/consultation-submit-ownership.test.ts @@ -0,0 +1,280 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +/** + * Account-takeover guard on the PUBLIC consultation signup. + * + * The hole: this route is unauthenticated (it IS the signup), so the submitted + * address is unproven. It used to swallow Clerk's "email already exists" and + * carry on against the existing account, ending in + * `users.update({ drGreenClientId, tenantId })` — re-pointing a stranger's + * account at a Dr Green client the caller controls. Approving the caller's own + * genuine ID then made the stranger's account read VERIFIED, which the + * purchase gate, the tenant-admin badge and the status webhooks all trust. + * + * The first fix was itself defective: it accepted "Clerk just minted a new + * account" as proof of ownership, which is true of the CLERK identity and says + * nothing about a local row that predates the request — so the mirror-image + * case (local row exists, no Clerk account for it) walked straight through a + * guard that could never fire. These tests drive the real handler, because a + * unit test of the helper and a regex over the source both passed against that + * defective version. + * + * The second fix closed that in one request but not in two: Clerk was still + * called first, so a caller could mint an account for the target address + * (Clerk performs no mailbox-control check — the password is the submitter's + * own), sign in with it, and resubmit with a session that now "proved" + * ownership. Hence the gate runs BEFORE Clerk, and the refusal cases below + * assert `createUser` was never reached. + */ + +const clerkMock = vi.hoisted(() => ({ + currentUser: vi.fn(), + createUser: vi.fn(), + clerkClient: vi.fn(), +})); +const prismaMock = vi.hoisted(() => ({ + users: { findUnique: vi.fn(), create: vi.fn(), update: vi.fn() }, + consultation_questionnaires: { create: vi.fn(), update: vi.fn() }, +})); +const libMock = vi.hoisted(() => ({ + checkRateLimit: vi.fn(), + getTenantFromRequest: vi.fn(), + resolveTenant: vi.fn(), + checkPolicyGate: vi.fn(), + getTenantVerificationMode: vi.fn(), + isSaIdUploadEnabled: vi.fn(), + getTenantDrGreenConfig: vi.fn(), + callDrGreenAPI: vi.fn(), + createSaIdClient: vi.fn(), + uploadIdentityDocument: vi.fn(), + recordIdDocumentOutcome: vi.fn(), + createAuditLog: vi.fn(), + triggerWebhook: vi.fn(), + mapMedicalConditionsForDrGreen: vi.fn(), +})); + +vi.mock("@clerk/nextjs/server", () => ({ + currentUser: clerkMock.currentUser, + clerkClient: clerkMock.clerkClient, +})); +vi.mock("@/lib/db", () => ({ prisma: prismaMock })); +vi.mock("@/lib/security/rate-limit", () => ({ checkRateLimit: libMock.checkRateLimit })); +vi.mock("@/lib/tenant/tenant", () => ({ getTenantFromRequest: libMock.getTenantFromRequest })); +vi.mock("@/lib/tenant/tenant-resolver", () => ({ resolveTenant: libMock.resolveTenant })); +vi.mock("@/lib/legal/policy-gate", () => ({ checkPolicyGate: libMock.checkPolicyGate })); +vi.mock("@/lib/verification-mode", () => ({ + getTenantVerificationMode: libMock.getTenantVerificationMode, + isSaIdUploadEnabled: libMock.isSaIdUploadEnabled, +})); +vi.mock("@/lib/tenant/tenant-config", () => ({ + getTenantDrGreenConfig: libMock.getTenantDrGreenConfig, +})); +vi.mock("@/lib/drgreen/drgreen-api-client", () => ({ callDrGreenAPI: libMock.callDrGreenAPI })); +vi.mock("@/lib/drgreen-identity", () => ({ + createSaIdClient: libMock.createSaIdClient, + uploadIdentityDocument: libMock.uploadIdentityDocument, +})); +vi.mock("@/lib/verification/id-document-status", () => ({ + recordIdDocumentOutcome: libMock.recordIdDocumentOutcome, +})); +vi.mock("@/lib/drgreen/dr-green-mapping", () => ({ + mapMedicalConditionsForDrGreen: libMock.mapMedicalConditionsForDrGreen, +})); +vi.mock("@/lib/audit-log", () => ({ + createAuditLog: libMock.createAuditLog, + AUDIT_ACTIONS: { CONSULTATION_SUBMITTED: "consultation.submitted" }, + getClientInfo: () => ({}), +})); +vi.mock("@/lib/integrations/webhook", () => ({ + triggerWebhook: libMock.triggerWebhook, + WEBHOOK_EVENTS: { CONSULTATION_SUBMITTED: "consultation.submitted" }, +})); + +import { POST } from "@/app/api/consultation/submit/route"; + +const TENANT = { id: "tenant-1", countryCode: "ZA", settings: {} }; +const VICTIM_EMAIL = "victim@example.com"; + +/** A users row that already exists — the thing an attacker wants to re-point. */ +const existingVictimRow = { + id: "user-victim", + email: VICTIM_EMAIL, + tenantId: "tenant-victim", + drGreenClientId: "drg_victim_original", +}; + +function submission(over: Record = {}) { + return { + firstName: "Attacker", + lastName: "Person", + email: VICTIM_EMAIL, + password: "sup3rsecret!", + phoneCode: "+27", + phoneNumber: "821234567", + dateOfBirth: "1990-01-01", + gender: "Other", + // Required by consultationSchema (min 2) — everything else defaults. + countryCode: "ZA", + ...over, + }; +} + +function request(body: unknown) { + return new NextRequest("http://store.localhost/api/consultation/submit", { + method: "POST", + headers: { "content-type": "application/json", "x-forwarded-for": "203.0.113.9" }, + body: JSON.stringify(body), + }); +} + +/** Signed-in session for `email`, shaped like Clerk's verified primary. */ +function session(email: string) { + return { + primaryEmailAddressId: "idn_1", + emailAddresses: [ + { id: "idn_1", emailAddress: email, verification: { status: "verified" } }, + ], + }; +} + +beforeEach(() => { + vi.clearAllMocks(); + libMock.checkRateLimit.mockResolvedValue({ success: true }); + libMock.getTenantFromRequest.mockResolvedValue(TENANT); + libMock.checkPolicyGate.mockResolvedValue({ allowed: true }); + libMock.getTenantVerificationMode.mockReturnValue("KYC"); + libMock.isSaIdUploadEnabled.mockReturnValue(false); + libMock.getTenantDrGreenConfig.mockResolvedValue({ apiKey: "k", secretKey: "s" }); + libMock.mapMedicalConditionsForDrGreen.mockReturnValue([]); + libMock.callDrGreenAPI.mockResolvedValue({ + data: { data: { id: "drg_attacker_client", kycLink: "https://kyc.example/x" } }, + }); + libMock.createAuditLog.mockResolvedValue(undefined); + libMock.triggerWebhook.mockResolvedValue(undefined); + clerkMock.clerkClient.mockResolvedValue({ users: { createUser: clerkMock.createUser } }); + prismaMock.consultation_questionnaires.create.mockResolvedValue({ id: "q-1" }); + prismaMock.consultation_questionnaires.update.mockResolvedValue({}); + prismaMock.users.create.mockResolvedValue({ id: "user-new" }); + prismaMock.users.update.mockResolvedValue({}); +}); + +/** Nothing may be written for a submission the caller has not proven it owns. */ +function expectNoSideEffects() { + expect(prismaMock.users.update).not.toHaveBeenCalled(); + expect(prismaMock.users.create).not.toHaveBeenCalled(); + expect(prismaMock.consultation_questionnaires.create).not.toHaveBeenCalled(); + expect(libMock.callDrGreenAPI).not.toHaveBeenCalled(); + expect(libMock.createSaIdClient).not.toHaveBeenCalled(); +} + +describe("consultation submit — ownership of an existing account", () => { + it("refuses an anonymous caller when Clerk holds the address (no local row)", async () => { + clerkMock.currentUser.mockResolvedValue(null); + clerkMock.createUser.mockRejectedValue({ + errors: [{ code: "form_identifier_exists", message: "That email address is taken." }], + }); + prismaMock.users.findUnique.mockResolvedValue(null); + + const response = await POST(request(submission())); + + expect(response.status).toBe(409); + expectNoSideEffects(); + }); + + it("refuses an anonymous caller when a local row exists — without letting Clerk mint", async () => { + // The mirror-image case the first fix missed: a users row exists (legacy + // import, or a dropped Clerk delete-webhook) while the address is free in + // Clerk, so createUser would succeed for anyone. Refusing BEFORE that call + // is what also closes the mint → sign-in → resubmit chain. + clerkMock.currentUser.mockResolvedValue(null); + clerkMock.createUser.mockResolvedValue({ id: "clerk_attacker" }); + prismaMock.users.findUnique.mockResolvedValue(existingVictimRow); + + const response = await POST(request(submission())); + + expect(response.status).toBe(409); + expectNoSideEffects(); + // No account is minted for an address the caller has not proven it owns — + // otherwise the refusal just becomes step one of a two-request takeover. + expect(clerkMock.createUser).not.toHaveBeenCalled(); + // And the pre-existing row keeps pointing at its own Dr Green client. + expect(prismaMock.users.update).not.toHaveBeenCalledWith( + expect.objectContaining({ where: { id: existingVictimRow.id } }), + ); + }); + + it("refuses a caller signed in as somebody else", async () => { + clerkMock.currentUser.mockResolvedValue(session("attacker@example.com")); + clerkMock.createUser.mockResolvedValue({ id: "clerk_attacker" }); + prismaMock.users.findUnique.mockResolvedValue(existingVictimRow); + + const response = await POST(request(submission())); + + expect(response.status).toBe(409); + expectNoSideEffects(); + expect(clerkMock.createUser).not.toHaveBeenCalled(); + }); + + it("refuses when the session's primary address is unverified", async () => { + clerkMock.currentUser.mockResolvedValue({ + primaryEmailAddressId: "idn_1", + emailAddresses: [ + { id: "idn_1", emailAddress: VICTIM_EMAIL, verification: { status: "unverified" } }, + ], + }); + clerkMock.createUser.mockResolvedValue({ id: "clerk_attacker" }); + prismaMock.users.findUnique.mockResolvedValue(existingVictimRow); + + const response = await POST(request(submission())); + + expect(response.status).toBe(409); + expectNoSideEffects(); + expect(clerkMock.createUser).not.toHaveBeenCalled(); + }); + + it("refuses to adopt a raced row this request did not create", async () => { + // No row at gate time, so the handler creates one and hits P2002 — but the + // row that appeared belongs to someone else, not to the Clerk account we + // just minted. Adopting it would re-open the same takeover. + clerkMock.currentUser.mockResolvedValue(null); + clerkMock.createUser.mockResolvedValue({ id: "clerk_new_user" }); + prismaMock.users.findUnique + .mockResolvedValueOnce(null) // ownership gate: nothing there yet + .mockResolvedValueOnce({ ...existingVictimRow, id: "user-someone-else" }); // the race + prismaMock.users.create.mockRejectedValue({ code: "P2002" }); + + const response = await POST(request(submission())); + + expect(response.status).toBe(409); + expect(prismaMock.users.update).not.toHaveBeenCalled(); + expect(prismaMock.consultation_questionnaires.create).not.toHaveBeenCalled(); + }); +}); + +describe("consultation submit — legitimate flows still work", () => { + it("lets a brand-new address through the gate", async () => { + clerkMock.currentUser.mockResolvedValue(null); + clerkMock.createUser.mockResolvedValue({ id: "clerk_new_user" }); + prismaMock.users.findUnique.mockResolvedValue(null); + + const response = await POST(request(submission({ email: "brand-new@example.com" }))); + + expect(response.status).not.toBe(409); + expect(prismaMock.users.create).toHaveBeenCalled(); + expect(prismaMock.consultation_questionnaires.create).toHaveBeenCalled(); + }); + + it("lets a signed-in customer complete their own consultation", async () => { + clerkMock.currentUser.mockResolvedValue(session(VICTIM_EMAIL)); + clerkMock.createUser.mockRejectedValue({ + errors: [{ code: "form_identifier_exists", message: "That email address is taken." }], + }); + prismaMock.users.findUnique.mockResolvedValue(existingVictimRow); + + const response = await POST(request(submission())); + + expect(response.status).not.toBe(409); + expect(prismaMock.consultation_questionnaires.create).toHaveBeenCalled(); + }); +}); diff --git a/nextjs_space/tests/unit/email-ownership.test.ts b/nextjs_space/tests/unit/email-ownership.test.ts new file mode 100644 index 00000000..01844366 --- /dev/null +++ b/nextjs_space/tests/unit/email-ownership.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; + +import { emailsMatch } from "@/lib/security/email-ownership"; + +/** + * The comparison behind every ownership decision on a public endpoint. + * The decision itself (who may touch a pre-existing account) is exercised + * against the real handler in consultation-submit-ownership.test.ts — an + * earlier version of this suite tested a helper in isolation and a regex over + * the route's source, and both passed while the route was still exploitable. + */ +describe("emailsMatch", () => { + it("compares case- and whitespace-insensitively", () => { + expect(emailsMatch("Ann@Example.com", " ann@example.com ")).toBe(true); + }); + + it("never matches on a missing side", () => { + expect(emailsMatch(null, "ann@example.com")).toBe(false); + expect(emailsMatch(undefined, "ann@example.com")).toBe(false); + expect(emailsMatch("", "ann@example.com")).toBe(false); + expect(emailsMatch(" ", "ann@example.com")).toBe(false); + expect(emailsMatch("ann@example.com", null)).toBe(false); + expect(emailsMatch(null, null)).toBe(false); + }); + + it("does not treat different addresses as equal", () => { + expect(emailsMatch("ann@example.com", "ann@example.co")).toBe(false); + expect(emailsMatch("ann+tag@example.com", "ann@example.com")).toBe(false); + expect(emailsMatch("ann@example.com", "anne@example.com")).toBe(false); + }); +});