Harden Google account linking against a squatted email row

.gitignore

@@ -1,6 +1,7 @@
 node_modules
 .next
-.env*.local
+.env*
+!.env.example
 .DS_Store
 tsconfig.tsbuildinfo
 *.tsbuildinfo

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

__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,18 +62,40 @@ 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();
     expect(response.status).toBe(404);
     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();
   });
 });

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);

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<UploadRow>(
-      `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, {

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/<token> 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<NoteRow>(

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<NoteRow>(
     `SELECT * FROM notes WHERE share_token = $1 AND trashed = false LIMIT 1`,
     [token],