From fb5c38a3259233433001de2c48968ff956bd9d92 Mon Sep 17 00:00:00 2001 From: Frank Barrett Date: Wed, 5 Aug 2026 18:19:59 -0700 Subject: [PATCH 1/2] fix: stop a stranger's registration from locking an address out of google sign-in /api/auth/register writes the users row before the verification email is answered, so anyone can plant a row for an address they don't own. The Google signIn upsert arbitrated on the primary key, but the unique constraint that row trips is users_email_idx on lower(email), so the arbiter never matched it and the insert raised 23505. Nothing catches that, so the victim's Google sign-in failed and would have kept failing forever. The mirror upsert now looks first and, when a different row already holds the address, hands that row to the Google account id instead of inserting beside it. Its notes and uploads move with it because jwt() keys the session on the Google sub, so a row that kept its old id would leave its content unreachable from either sign-in path. security_events keeps its original user_id: it is an audit trail of what happened under that id, not state to migrate. Password material on an unverified row is dropped in the same statement. Left in place it would turn this fix into something worse than the lockout it repairs -- the squatter's password would open the linked account the moment the real owner clicked the verification mail they were sent at squat time. A verified row is the same person arriving by a second door, so its password survives. Two smaller boundaries in the same pass: /api/native/exchange was the only endpoint the proxy allowlists without a session and without a rate limit, despite a DB write behind it, so it now takes the same per-IP cap as /api/auth/verify. GET /api/uploads/:id fetched the row on id alone and judged it afterwards. That was correct, but it was the one query in the app not carrying its owner inline, and the ordering is what kept it correct. The owner check moves into the WHERE clause; the share arm stays as the one legitimate non-owner read and still requires a live shared note, owned by the upload's owner, that actually embeds this file. Its test now models that predicate in the fake pool, and covers a signed-in non-owner and a wrong share token as well. Also carries the .gitignore fix already on this branch: .env* was only ignored in its .local form. Co-Authored-By: Claude Fable 5 --- .gitignore | 3 +- __tests__/uploadReadRoute.test.ts | 42 ++++++++++++++--- app/api/native/exchange/route.ts | 19 ++++++++ app/api/uploads/[id]/route.ts | 48 +++++++++++--------- app/p/[token]/page.tsx | 5 +++ app/p/[token]/raw.txt/route.ts | 3 ++ auth.ts | 75 +++++++++++++++++++++++++++---- 7 files changed, 159 insertions(+), 36 deletions(-) diff --git a/.gitignore b/.gitignore index c56e778..dd84749 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ node_modules .next -.env*.local +.env* +!.env.example .DS_Store tsconfig.tsbuildinfo *.tsbuildinfo diff --git a/__tests__/uploadReadRoute.test.ts b/__tests__/uploadReadRoute.test.ts index c4e35fb..00808b7 100644 --- a/__tests__/uploadReadRoute.test.ts +++ b/__tests__/uploadReadRoute.test.ts @@ -19,8 +19,8 @@ vi.mock("@/lib/storage", () => ({ import { GET } from "@/app/api/uploads/[id]/route"; const id = "a".repeat(32); +const shareToken = "public-share-token"; const upload = { - user_id: "owner", storage_key: `keep/owner/${id}.png`, content_type: "image/png", }; @@ -36,7 +36,17 @@ beforeEach(() => { mocks.auth.mockReset(); mocks.query.mockReset(); mocks.getPrivateFile.mockReset(); - mocks.query.mockResolvedValueOnce({ rows: [upload] }); + // The route asks Postgres for a row the caller is entitled to rather than + // fetching one and deciding afterwards, so the fake pool stands in for that + // predicate: a row comes back only when the bound owner id or share token + // would have satisfied it. + mocks.query.mockImplementation(async (_sql: string, params: unknown[]) => { + const [rowId, ownerId, share, reference] = params as + [string, string | null, string | null, string]; + const entitled = + ownerId === "owner" || (share === shareToken && reference === `/api/uploads/${id}`); + return { rows: rowId === id && entitled ? [upload] : [] }; + }); mocks.getPrivateFile.mockResolvedValue({ body: new Uint8Array([1, 2, 3]).buffer, contentType: "image/png", @@ -52,6 +62,15 @@ describe("GET /api/uploads/:id", () => { expect(mocks.getPrivateFile).toHaveBeenCalledWith(upload.storage_key); }); + it("constrains the lookup on the caller rather than on the id alone", async () => { + mocks.auth.mockResolvedValue({ user: { id: "owner" } }); + await request(); + const [sql, params] = mocks.query.mock.calls[0]; + expect(sql).toMatch(/u\.user_id = \$2/); + expect(sql).toMatch(/n\.share_token = \$3/); + expect(params).toEqual([id, "owner", null, `/api/uploads/${id}`]); + }); + it("hides an upload from an unauthenticated caller", async () => { mocks.auth.mockResolvedValue(null); const response = await request(); @@ -59,11 +78,24 @@ describe("GET /api/uploads/:id", () => { expect(mocks.getPrivateFile).not.toHaveBeenCalled(); }); + it("hides an upload from a signed-in non-owner", async () => { + mocks.auth.mockResolvedValue({ user: { id: "someone-else" } }); + const response = await request(); + expect(response.status).toBe(404); + expect(mocks.getPrivateFile).not.toHaveBeenCalled(); + }); + it("serves only uploads referenced by the shared note", async () => { mocks.auth.mockResolvedValue(null); - mocks.query.mockResolvedValueOnce({ rows: [{ allowed: 1 }] }); - const response = await request("?share=public-share-token"); + const response = await request(`?share=${shareToken}`); expect(response.status).toBe(200); - expect(mocks.query).toHaveBeenCalledTimes(2); + expect(mocks.query).toHaveBeenCalledTimes(1); + }); + + it("rejects a share token that does not open this upload", async () => { + mocks.auth.mockResolvedValue(null); + const response = await request("?share=some-other-token"); + expect(response.status).toBe(404); + expect(mocks.getPrivateFile).not.toHaveBeenCalled(); }); }); diff --git a/app/api/native/exchange/route.ts b/app/api/native/exchange/route.ts index ecf4155..00057af 100644 --- a/app/api/native/exchange/route.ts +++ b/app/api/native/exchange/route.ts @@ -1,10 +1,21 @@ import { NextResponse } from "next/server"; import { pool, ready } from "@/lib/db"; +import { createTokenBucketRateLimiter } from "@/lib/rateLimit"; +import { enforceIpRateLimit } from "@/lib/rateLimitGuard"; import { readJsonBody, requestBodyError } from "@/lib/requestBody"; export const runtime = "nodejs"; export const dynamic = "force-dynamic"; +// Codes are 256-bit and single-use, but this is the one endpoint the proxy lets +// through unauthenticated with a DB write behind it, so cap per-IP the way +// /api/auth/verify does — it can't be hammered as a redemption oracle or to +// load the database. +const exchangeRateLimit = createTokenBucketRateLimiter({ + limit: 20, + windowMs: 60_000, +}); + // Matches Keep's seven-day session lifetime so the cookie persists in the // app's cookie store across launches (a bare session cookie would be dropped on // quit). The JWT's own exp still governs validity — an expired token 401s and @@ -17,6 +28,14 @@ const MAX_EXCHANGE_BODY = 2 * 1024; // session exists — see the allowlist in proxy.ts); the high-entropy, // single-use code is the credential, so there is nothing to leak without it. export async function POST(req: Request) { + const limited = enforceIpRateLimit( + exchangeRateLimit, + req.headers, + "native-exchange", + "Too many attempts. Try again shortly.", + ); + if (limited) return limited; + let body: unknown; try { body = await readJsonBody(req, MAX_EXCHANGE_BODY); diff --git a/app/api/uploads/[id]/route.ts b/app/api/uploads/[id]/route.ts index 5187ef4..307ebba 100644 --- a/app/api/uploads/[id]/route.ts +++ b/app/api/uploads/[id]/route.ts @@ -7,7 +7,6 @@ export const runtime = "nodejs"; export const dynamic = "force-dynamic"; type UploadRow = { - user_id: string; storage_key: string; content_type: string; }; @@ -19,33 +18,40 @@ export async function GET( const { id } = await params; if (!/^[0-9a-f]{32}$/.test(id)) return new Response("Not found", { status: 404 }); + const session = await auth(); + const raw = new URL(req.url).searchParams.get("share") ?? ""; + const share = /^[A-Za-z0-9_-]{3,40}$/.test(raw) ? raw : null; + try { await ready(); + // Authorization is part of the lookup, like every other query here, so the + // row is never in hand before something has justified reading it. The share + // arm is the one legitimate non-owner read: it matches only a live shared + // note owned by the same account that actually embeds this upload, so a + // token can't be aimed at another account's attachments. Both misses return + // no row, and the caller can't tell a private upload from a missing one. const { rows } = await pool().query( - `SELECT user_id, storage_key, content_type FROM uploads WHERE id = $1`, - [id], + `SELECT u.storage_key, u.content_type + FROM uploads u + WHERE u.id = $1 + AND ( + u.user_id = $2 + OR ( + $3::text IS NOT NULL + AND EXISTS ( + SELECT 1 FROM notes n + WHERE n.user_id = u.user_id + AND n.share_token = $3 + AND n.trashed = false + AND position($4 in n.body) > 0 + ) + ) + )`, + [id, session?.user?.id ?? null, share, `/api/uploads/${id}`], ); const upload = rows[0]; if (!upload) return new Response("Not found", { status: 404 }); - const session = await auth(); - let allowed = session?.user?.id === upload.user_id; - if (!allowed) { - const token = new URL(req.url).searchParams.get("share") ?? ""; - if (/^[A-Za-z0-9_-]{3,40}$/.test(token)) { - const reference = `/api/uploads/${id}`; - const shared = await pool().query( - `SELECT 1 FROM notes - WHERE user_id = $1 AND share_token = $2 AND trashed = false - AND position($3 in body) > 0 - LIMIT 1`, - [upload.user_id, token, reference], - ); - allowed = Boolean(shared.rows[0]); - } - } - if (!allowed) return new Response("Not found", { status: 404 }); - const file = await getPrivateFile(upload.storage_key); if (!file) return new Response("Not found", { status: 404 }); return new Response(file.body, { diff --git a/app/p/[token]/page.tsx b/app/p/[token]/page.tsx index ffd16d1..36a180b 100644 --- a/app/p/[token]/page.tsx +++ b/app/p/[token]/page.tsx @@ -12,6 +12,11 @@ import type { ComponentProps } from "react"; export const runtime = "nodejs"; export const dynamic = "force-dynamic"; +// Trashing a note revokes its share link; archiving does not. An archived note +// is still readable at /p/ by anyone holding it. That may well be the +// intent — archive is a "get it out of my list" gesture, not "unpublish" — but +// nothing states it, so it is recorded here rather than silently changed. +// Same filter in ./raw.txt/route.ts; the two have to agree. async function loadShared(token: string) { await ready(); const { rows } = await pool().query( diff --git a/app/p/[token]/raw.txt/route.ts b/app/p/[token]/raw.txt/route.ts index ace39b5..298bf6a 100644 --- a/app/p/[token]/raw.txt/route.ts +++ b/app/p/[token]/raw.txt/route.ts @@ -14,6 +14,9 @@ export async function GET( ) { const { token } = await params; await ready(); + // Trashed notes drop off the share link; archived ones stay readable. Whether + // archiving should also revoke the link is an open question, not a settled + // one — see the note in ../page.tsx, which filters identically. const { rows } = await pool().query( `SELECT * FROM notes WHERE share_token = $1 AND trashed = false LIMIT 1`, [token], diff --git a/auth.ts b/auth.ts index f8df597..a521465 100644 --- a/auth.ts +++ b/auth.ts @@ -125,20 +125,77 @@ export const { handlers, auth, signIn, signOut } = NextAuth({ }, // Mirror Google users into our `users` table so email/password sign-in // resolves back to the same account. + // + // The unique constraint on that table is users_email_idx — UNIQUE on + // lower(email) (lib/db.ts) — not the primary key. So an `ON CONFLICT (id)` + // arbiter alone never matches a row that already holds this address, and + // the insert raises 23505 instead. Anyone can create such a row for an + // address they don't own: /api/auth/register writes the user before the + // verification email is answered. Left unhandled that throw is permanent, + // and the victim can never sign in with Google at all. + // + // So when another row holds the address, hand that row to the Google + // account id rather than inserting a second one, and carry its notes and + // uploads across — jwt() keys the session on the Google sub, so a row that + // kept its old id would leave its content stranded. Password material on an + // unverified row is discarded with it: nobody proved they own the mailbox, + // and keeping it would let the squatter's password open the linked account + // the moment the real owner clicked the verification mail. A verified row is + // the same person arriving by a second door, so its password still works. async signIn({ user, account }) { if (account?.provider !== "google") return true; const id = account.providerAccountId ?? user.id; if (!id) return true; + const email = user.email ?? null; + const name = user.name ?? null; + const now = Date.now(); await ready(); - await pool().query( - `INSERT INTO users (id, email, name, updated_at) - VALUES ($1, $2, $3, $4) - ON CONFLICT (id) DO UPDATE - SET email = EXCLUDED.email, - name = EXCLUDED.name, - updated_at = EXCLUDED.updated_at`, - [id, user.email ?? null, user.name ?? null, Date.now()], - ); + + const client = await pool().connect(); + try { + await client.query("BEGIN"); + const { rows } = await client.query<{ id: string }>( + `SELECT id FROM users + WHERE id = $1 OR ($2::text IS NOT NULL AND lower(email) = lower($2)) + FOR UPDATE`, + [id, email], + ); + const squatterId = rows.find((row) => row.id !== id)?.id; + const hasOwnRow = rows.some((row) => row.id === id); + + if (squatterId && !hasOwnRow) { + await client.query( + `UPDATE users + SET id = $1, + name = $2, + updated_at = $3, + password_hash = CASE WHEN email_verified IS NULL THEN NULL ELSE password_hash END, + verify_token = CASE WHEN email_verified IS NULL THEN NULL ELSE verify_token END, + verify_token_expires = + CASE WHEN email_verified IS NULL THEN NULL ELSE verify_token_expires END + WHERE id = $4`, + [id, name, now, squatterId], + ); + await client.query(`UPDATE notes SET user_id = $1 WHERE user_id = $2`, [id, squatterId]); + await client.query(`UPDATE uploads SET user_id = $1 WHERE user_id = $2`, [id, squatterId]); + } else { + await client.query( + `INSERT INTO users (id, email, name, updated_at) + VALUES ($1, $2, $3, $4) + ON CONFLICT (id) DO UPDATE + SET email = EXCLUDED.email, + name = EXCLUDED.name, + updated_at = EXCLUDED.updated_at`, + [id, email, name, now], + ); + } + await client.query("COMMIT"); + } catch (err) { + await client.query("ROLLBACK").catch(() => {}); + throw err; + } finally { + client.release(); + } return true; }, }, From 1af6241037d4acc44f1794383aa9d0d3c1d43829 Mon Sep 17 00:00:00 2001 From: Frank Barrett Date: Thu, 6 Aug 2026 17:57:19 -0700 Subject: [PATCH 2/2] fix: drop an adopted row's password when a google sign-in claims its address 01ff46b taught the Google signIn callback to adopt a `users` row that already held the incoming address instead of raising 23505 beside it. It kept that row's password_hash whenever email_verified was set, reasoning that a verified row is the same person arriving by a second door. That reads the flag for more than it says. email_verified records that someone opened the mailbox. It says nothing about who chose the password sitting in the row, and /api/auth/register stores password_hash before anyone answers the verification mail. So: a stranger registers your address, you get a real verification mail from the real domain and click it, and your first Google sign-in then adopts that row with the stranger's password still on it. They sign in through the credentials provider, land on your Google sub, and read every note you own. Before the adoption existed that sequence only locked you out; keeping the hash turned the repair into a takeover. The adopted row's password material now always goes. Someone who really did own both doors keeps the Google one and loses the password, which is a door fewer rather than a lockout. Adoption is also gated on Google's email_verified claim now. Matching on lower(email) is what decides whose notes move, so an unverified claim must not be able to aim at another account's row. An unverified address still gets a row, just without the address on it, and COALESCE on the insert stops a missing email from erasing one already stored. The transaction moves to lib/googleAccountLink.ts so the rules are testable without standing up NextAuth, and a successful adoption records an account.link security event: it is the one thing in that table that changes who can reach an account. Co-Authored-By: Claude Opus 5 (1M context) --- __tests__/googleAccountLink.test.ts | 143 ++++++++++++++++++++++++++++ auth.ts | 89 ++++------------- lib/audit.ts | 4 + lib/googleAccountLink.ts | 104 ++++++++++++++++++++ 4 files changed, 272 insertions(+), 68 deletions(-) create mode 100644 __tests__/googleAccountLink.test.ts create mode 100644 lib/googleAccountLink.ts diff --git a/__tests__/googleAccountLink.test.ts b/__tests__/googleAccountLink.test.ts new file mode 100644 index 0000000..e969db0 --- /dev/null +++ b/__tests__/googleAccountLink.test.ts @@ -0,0 +1,143 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +type UserRow = { id: string; email: string | null; email_verified: string | null }; + +const mocks = vi.hoisted(() => ({ query: vi.fn(), release: vi.fn() })); + +vi.mock("@/lib/db", () => ({ + ready: vi.fn(async () => {}), + pool: () => ({ + connect: async () => ({ query: mocks.query, release: mocks.release }), + }), +})); + +import { linkGoogleAccount } from "@/lib/googleAccountLink"; + +const GOOGLE_SUB = "104857392017465920371"; +let users: UserRow[] = []; + +// Stands in for the transaction: only the SELECT needs to answer from data, and +// it answers with the same predicate Postgres would apply. +function fakePool(sql: string, params: unknown[] = []) { + if (/^\s*SELECT id, email_verified FROM users/.test(sql)) { + const [id, email] = params as [string, string | null]; + const rows = users.filter( + (u) => + u.id === id || + (email !== null && u.email !== null && u.email.toLowerCase() === email.toLowerCase()), + ); + return { rows: rows.map(({ id: rowId, email_verified }) => ({ id: rowId, email_verified })) }; + } + return { rows: [] }; +} + +const statements = () => mocks.query.mock.calls.map(([sql]) => sql as string); +const find = (pattern: RegExp) => statements().find((sql) => pattern.test(sql)); + +function link(email: string | null, emailVerified = true) { + return linkGoogleAccount({ + id: GOOGLE_SUB, + email, + emailVerified, + name: "Ada", + now: 1_700_000_000_000, + }); +} + +beforeEach(() => { + mocks.query.mockReset(); + mocks.release.mockReset(); + mocks.query.mockImplementation(async (sql: string, params: unknown[]) => fakePool(sql, params)); + users = []; +}); + +describe("linkGoogleAccount", () => { + it("inserts a fresh row when nothing holds the address", async () => { + await expect(link("ada@example.com")).resolves.toEqual({ linked: false }); + expect(find(/INSERT INTO users/)).toBeTruthy(); + expect(find(/UPDATE users/)).toBeUndefined(); + }); + + it("adopts a squatted row rather than raising 23505 beside it", async () => { + users = [{ id: "squatter", email: "ada@example.com", email_verified: null }]; + + await expect(link("ada@example.com")).resolves.toEqual({ + linked: true, + adoptedFrom: "squatter", + wasVerified: false, + }); + expect(find(/UPDATE users/)).toBeTruthy(); + expect(find(/INSERT INTO users/)).toBeUndefined(); + }); + + it("carries the adopted row's notes and uploads to the Google sub", async () => { + users = [{ id: "squatter", email: "ada@example.com", email_verified: null }]; + await link("ada@example.com"); + + for (const table of ["notes", "uploads"]) { + const call = mocks.query.mock.calls.find(([sql]) => + new RegExp(`UPDATE ${table} SET user_id`).test(sql as string), + ); + expect(call?.[1]).toEqual([GOOGLE_SUB, "squatter"]); + } + }); + + // The takeover this guards: /api/auth/register stores a password_hash before + // anyone answers the verification mail, so a stranger can seed a row for an + // address they don't own. email_verified only says the real owner later clicked + // that (genuine) link. Keeping the hash past adoption would leave the stranger + // with a working password on the victim's Google account. + it("drops the squatter's password even when the row was verified", async () => { + users = [{ id: "squatter", email: "ada@example.com", email_verified: "1699999999999" }]; + + const outcome = await link("ada@example.com"); + expect(outcome).toEqual({ linked: true, adoptedFrom: "squatter", wasVerified: true }); + + const update = find(/UPDATE users/) ?? ""; + expect(update).toMatch(/password_hash = NULL/); + expect(update).toMatch(/verify_token = NULL/); + expect(update).toMatch(/verify_token_expires = NULL/); + // Nothing may make the clearing conditional on email_verified again. + expect(update).not.toMatch(/CASE/i); + expect(update).not.toMatch(/email_verified/); + }); + + it("ignores an address Google has not verified", async () => { + users = [{ id: "squatter", email: "ada@example.com", email_verified: null }]; + + await expect(link("ada@example.com", false)).resolves.toEqual({ linked: false }); + expect(find(/UPDATE users/)).toBeUndefined(); + const insert = mocks.query.mock.calls.find(([sql]) => /INSERT INTO users/.test(sql as string)); + expect(insert?.[1]).toEqual([GOOGLE_SUB, null, "Ada", 1_700_000_000_000]); + }); + + it("keeps a stored address when Google returns none", async () => { + users = [{ id: GOOGLE_SUB, email: "ada@example.com", email_verified: "1699999999999" }]; + + await link(null); + expect(find(/INSERT INTO users/)).toMatch(/COALESCE\(EXCLUDED\.email, users\.email\)/); + }); + + it("updates its own row in place instead of adopting a second one", async () => { + users = [ + { id: GOOGLE_SUB, email: "ada@example.com", email_verified: null }, + { id: "other", email: "ada@example.com", email_verified: null }, + ]; + + await expect(link("ada@example.com")).resolves.toEqual({ linked: false }); + expect(find(/UPDATE users/)).toBeUndefined(); + }); + + it("rolls back and releases the client when a statement fails", async () => { + users = [{ id: "squatter", email: "ada@example.com", email_verified: null }]; + mocks.query.mockImplementation(async (sql: string, params: unknown[]) => { + if (/UPDATE notes/.test(sql)) throw new Error("boom"); + return fakePool(sql, params); + }); + + await expect(link("ada@example.com")).rejects.toThrow("boom"); + expect(statements()).toContain("ROLLBACK"); + expect(statements()).not.toContain("COMMIT"); + expect(mocks.release).toHaveBeenCalled(); + }); +}); diff --git a/auth.ts b/auth.ts index a521465..32f40a1 100644 --- a/auth.ts +++ b/auth.ts @@ -4,6 +4,7 @@ import Credentials from "next-auth/providers/credentials"; import { pool, ready } from "@/lib/db"; import { hashPassword, passwordNeedsRehash, verifyPassword } from "@/lib/password"; import { recordSecurityEvent } from "@/lib/audit"; +import { linkGoogleAccount } from "@/lib/googleAccountLink"; import { maskEmail } from "@/lib/logger"; import { clientIpFromHeaders } from "@/lib/rateLimit"; import { checkLoginThrottle } from "@/lib/loginThrottle"; @@ -124,77 +125,29 @@ export const { handlers, auth, signIn, signOut } = NextAuth({ return session; }, // Mirror Google users into our `users` table so email/password sign-in - // resolves back to the same account. - // - // The unique constraint on that table is users_email_idx — UNIQUE on - // lower(email) (lib/db.ts) — not the primary key. So an `ON CONFLICT (id)` - // arbiter alone never matches a row that already holds this address, and - // the insert raises 23505 instead. Anyone can create such a row for an - // address they don't own: /api/auth/register writes the user before the - // verification email is answered. Left unhandled that throw is permanent, - // and the victim can never sign in with Google at all. - // - // So when another row holds the address, hand that row to the Google - // account id rather than inserting a second one, and carry its notes and - // uploads across — jwt() keys the session on the Google sub, so a row that - // kept its old id would leave its content stranded. Password material on an - // unverified row is discarded with it: nobody proved they own the mailbox, - // and keeping it would let the squatter's password open the linked account - // the moment the real owner clicked the verification mail. A verified row is - // the same person arriving by a second door, so its password still works. - async signIn({ user, account }) { + // resolves back to the same account. Adopting a row that already holds the + // address is a transfer of that row's notes, so the rules governing it live + // in lib/googleAccountLink.ts with the reasoning behind them. + async signIn({ user, account, profile }) { if (account?.provider !== "google") return true; const id = account.providerAccountId ?? user.id; if (!id) return true; - const email = user.email ?? null; - const name = user.name ?? null; - const now = Date.now(); - await ready(); - - const client = await pool().connect(); - try { - await client.query("BEGIN"); - const { rows } = await client.query<{ id: string }>( - `SELECT id FROM users - WHERE id = $1 OR ($2::text IS NOT NULL AND lower(email) = lower($2)) - FOR UPDATE`, - [id, email], - ); - const squatterId = rows.find((row) => row.id !== id)?.id; - const hasOwnRow = rows.some((row) => row.id === id); - - if (squatterId && !hasOwnRow) { - await client.query( - `UPDATE users - SET id = $1, - name = $2, - updated_at = $3, - password_hash = CASE WHEN email_verified IS NULL THEN NULL ELSE password_hash END, - verify_token = CASE WHEN email_verified IS NULL THEN NULL ELSE verify_token END, - verify_token_expires = - CASE WHEN email_verified IS NULL THEN NULL ELSE verify_token_expires END - WHERE id = $4`, - [id, name, now, squatterId], - ); - await client.query(`UPDATE notes SET user_id = $1 WHERE user_id = $2`, [id, squatterId]); - await client.query(`UPDATE uploads SET user_id = $1 WHERE user_id = $2`, [id, squatterId]); - } else { - await client.query( - `INSERT INTO users (id, email, name, updated_at) - VALUES ($1, $2, $3, $4) - ON CONFLICT (id) DO UPDATE - SET email = EXCLUDED.email, - name = EXCLUDED.name, - updated_at = EXCLUDED.updated_at`, - [id, email, name, now], - ); - } - await client.query("COMMIT"); - } catch (err) { - await client.query("ROLLBACK").catch(() => {}); - throw err; - } finally { - client.release(); + const outcome = await linkGoogleAccount({ + id, + email: user.email ?? null, + emailVerified: profile?.email_verified === true, + name: user.name ?? null, + now: Date.now(), + }); + if (outcome.linked) { + void recordSecurityEvent("account.link", { + userId: id, + meta: { + provider: "google", + adoptedFrom: outcome.adoptedFrom, + wasVerified: outcome.wasVerified, + }, + }); } return true; }, diff --git a/lib/audit.ts b/lib/audit.ts index 85ee741..4a5b2a0 100644 --- a/lib/audit.ts +++ b/lib/audit.ts @@ -15,6 +15,10 @@ export type SecurityEvent = | "login.success" | "login.failure" | "register" + // A Google sign-in took over a pre-existing row that held the same address, + // moving its notes and dropping its password. Rare, and the one event here + // that changes who can reach an account, so it is worth a durable row. + | "account.link" | "session.revoke" | "share.create" | "share.revoke"; diff --git a/lib/googleAccountLink.ts b/lib/googleAccountLink.ts new file mode 100644 index 0000000..ade9f9e --- /dev/null +++ b/lib/googleAccountLink.ts @@ -0,0 +1,104 @@ +import { pool, ready } from "@/lib/db"; + +export type GoogleIdentity = { + /** Google's stable `sub`. Every session for this account is keyed on it. */ + id: string; + email: string | null; + /** Google's `email_verified` claim, passed through untouched. */ + emailVerified: boolean; + name: string | null; + now: number; +}; + +export type LinkOutcome = + | { linked: false } + | { linked: true; adoptedFrom: string; wasVerified: boolean }; + +/** + * Mirror a Google user into `users` so email/password sign-in resolves back to + * the same account. + * + * The unique constraint on that table is users_email_idx, UNIQUE on lower(email) + * (lib/db.ts), rather than the primary key. An `ON CONFLICT (id)` arbiter alone + * therefore never matches a row that already holds this address, and the insert + * raises 23505 instead. Anyone can plant such a row: /api/auth/register writes + * the user before the verification email is answered. Unhandled, that throw is + * permanent, and the victim can never sign in with Google at all. + * + * So when another row holds the address, hand that row to the Google account id + * instead of inserting beside it, and carry its notes and uploads across. jwt() + * keys the session on the Google sub, so a row that kept its old id would leave + * its content unreachable from either sign-in path. + * + * Two rules keep that hand-off from becoming a takeover. + * + * The address has to be one Google asserts as verified. Matching on lower(email) + * is what decides whose notes move, so an unverified claim must never be able to + * aim at another account's row. + * + * The adopted row's password material always goes. email_verified records that + * someone opened the mailbox, never that they chose the password sitting in the + * row: register stores password_hash before anyone answers the mail, so a + * stranger's password plus the owner's click on that genuine link would leave + * the stranger holding a working credential on the linked account. Someone who + * really did own both doors keeps the Google one and loses the password. + */ +export async function linkGoogleAccount(identity: GoogleIdentity): Promise { + const { id, name, now } = identity; + const email = identity.emailVerified ? identity.email : null; + await ready(); + + const client = await pool().connect(); + try { + await client.query("BEGIN"); + const { rows } = await client.query<{ id: string; email_verified: string | null }>( + `SELECT id, email_verified FROM users + WHERE id = $1 OR ($2::text IS NOT NULL AND lower(email) = lower($2)) + FOR UPDATE`, + [id, email], + ); + const squatter = rows.find((row) => row.id !== id); + const hasOwnRow = rows.some((row) => row.id === id); + + let outcome: LinkOutcome = { linked: false }; + if (squatter && !hasOwnRow) { + await client.query( + `UPDATE users + SET id = $1, + name = $2, + updated_at = $3, + password_hash = NULL, + verify_token = NULL, + verify_token_expires = NULL + WHERE id = $4`, + [id, name, now, squatter.id], + ); + // security_events keeps its original user_id: it is an audit trail of what + // happened under that id, not state to migrate. + await client.query(`UPDATE notes SET user_id = $1 WHERE user_id = $2`, [id, squatter.id]); + await client.query(`UPDATE uploads SET user_id = $1 WHERE user_id = $2`, [id, squatter.id]); + outcome = { + linked: true, + adoptedFrom: squatter.id, + wasVerified: squatter.email_verified !== null, + }; + } else { + await client.query( + `INSERT INTO users (id, email, name, updated_at) + VALUES ($1, $2, $3, $4) + ON CONFLICT (id) DO UPDATE + SET email = COALESCE(EXCLUDED.email, users.email), + name = EXCLUDED.name, + updated_at = EXCLUDED.updated_at`, + [id, email, name, now], + ); + } + await client.query("COMMIT"); + return outcome; + } catch (err) { + await client.query("ROLLBACK").catch(() => {}); + throw err; + } finally { + client.release(); + } +}