From 73dec9a114d1ac340ee65ad20c6014813f3149a6 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:43:06 +0530 Subject: [PATCH 01/15] Fail closed when JWT_SECRET is missing or weak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TextEncoder().encode(undefined) yields a zero-length key, and jose will both sign and verify HS256 with it. A deploy that forgot to set the secret would therefore accept forged session cookies for any user_id, with no error anywhere — the app just looks like it works. Require a secret of at least 32 characters and throw otherwise, so a misconfigured deploy 500s instead of running wide open. Resolve the key outside verifySession's try block so a config error surfaces as a 500 rather than being swallowed into a 401. Co-Authored-By: Claude Opus 4.8 --- worker/api.test.ts | 23 ++++++++++++++++++++++- worker/lib/auth.ts | 19 +++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/worker/api.test.ts b/worker/api.test.ts index b3f97ae..893259d 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -8,9 +8,12 @@ import type { Job, Activity, Stats } from "../shared/types"; let env: Env; let dispose: () => Promise; +// Must clear the 32-char floor enforced by secretKey(). +const TEST_SECRET = "test-secret-that-is-long-enough-to-pass"; + beforeAll(async () => { const proxy = await getPlatformProxy({ persist: false }); - env = { ...proxy.env, JWT_SECRET: "test-secret", ALLOW_REGISTRATION: "true" }; + env = { ...proxy.env, JWT_SECRET: TEST_SECRET, ALLOW_REGISTRATION: "true" }; dispose = proxy.dispose; const statements = ( @@ -70,6 +73,24 @@ describe("auth", () => { expect(res.status).toBe(403); }); + it("refuses to issue a session when JWT_SECRET is missing or too short", async () => { + // A zero-length key is one jose will sign and verify with, so a deploy that + // forgot the secret would accept forged sessions. It must fail loudly. + // Distinct emails: the row is inserted before createSession throws, so + // reusing one address would make the second case a 409 rather than a 500. + for (const [i, secret] of [undefined, "too-short"].entries()) { + const res = await app.request( + "/api/auth/register", + { + ...json({ email: `weak${i}@test.dev`, password: "password1" }), + headers: { "Content-Type": "application/json" }, + }, + { ...env, JWT_SECRET: secret } as Env + ); + expect(res.status).toBe(500); + } + }); + it("rejects bad credentials and unauthenticated API access", async () => { const c = client(); const bad = await c.fetch("/api/auth/login", json({ email: "a@test.dev", password: "wrongpass1" })); diff --git a/worker/lib/auth.ts b/worker/lib/auth.ts index e3b3135..3340535 100644 --- a/worker/lib/auth.ts +++ b/worker/lib/auth.ts @@ -7,7 +7,19 @@ const SESSION_DAYS = 30; export type AppEnv = { Bindings: Env; Variables: { userId: string } }; -function secretKey(secret: string) { +const MIN_SECRET_LENGTH = 32; + +// TextEncoder turns a missing secret into a zero-length key, which jose will +// happily sign AND verify with — a deploy that forgot the secret would accept +// forged sessions for any user instead of failing. Refuse to start instead. +function secretKey(secret: string | undefined) { + if (!secret || secret.length < MIN_SECRET_LENGTH) { + throw new Error( + `JWT_SECRET is missing or shorter than ${MIN_SECRET_LENGTH} characters. ` + + `Generate one with \`openssl rand -base64 32\` and set it with \`wrangler secret put JWT_SECRET\` ` + + `(or in .dev.vars for local development).` + ); + } return new TextEncoder().encode(secret); } @@ -34,8 +46,11 @@ export function clearSession(c: Context) { export async function verifySession(c: Context): Promise { const token = getCookie(c, AUTH_COOKIE); if (!token) return null; + // Resolved outside the try: a bad JWT_SECRET is a misconfigured server, not a + // bad token, and must not be swallowed into a 401. + const key = secretKey(c.env.JWT_SECRET); try { - const { payload } = await jwtVerify(token, secretKey(c.env.JWT_SECRET)); + const { payload } = await jwtVerify(token, key); return typeof payload.sub === "string" ? payload.sub : null; } catch { return null; From f1629dbd71afa3b611b817c0ad80bdb02c0c6ecd Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:50:01 +0530 Subject: [PATCH 02/15] Hash passwords with PBKDF2 via WebCrypto instead of bcryptjs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bcryptjs is pure JavaScript and a cost-10 hash burns ~74ms of CPU. The Workers Free plan caps CPU at 10ms per request, so login and register hard-failed with "exceeded resource limits" on any fork deployed to the free tier. The app worked only because our own account is on Paid. Move to PBKDF2-HMAC-SHA256 through crypto.subtle, which runs natively. No password hash is both OWASP-grade and under 10ms — being slow is the whole point of one. We choose free-tier deployability over hash strength and run 50k iterations (~6ms), roughly 12x below OWASP's guidance. The trade-off is documented at the constant. Iterations are stored per-hash, so the factor can be raised later without invalidating existing hashes. Legacy bcrypt hashes still verify and are transparently re-hashed to PBKDF2 on the account's next successful login. Co-Authored-By: Claude Opus 4.8 --- worker/api.test.ts | 25 +++++++++++++ worker/lib/password.ts | 85 ++++++++++++++++++++++++++++++++++++++++++ worker/routes/auth.ts | 17 ++++++--- 3 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 worker/lib/password.ts diff --git a/worker/api.test.ts b/worker/api.test.ts index 893259d..93d04d5 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -1,5 +1,6 @@ import { beforeAll, afterAll, describe, expect, it } from "vitest"; import { readFileSync } from "node:fs"; +import bcrypt from "bcryptjs"; import { getPlatformProxy } from "wrangler"; import app from "./index"; import type { Job, Activity, Stats } from "../shared/types"; @@ -91,6 +92,30 @@ describe("auth", () => { } }); + it("verifies a legacy bcrypt hash and upgrades it to pbkdf2 on login", async () => { + // Simulates an account created before the PBKDF2 switch. + const legacy = bcrypt.hashSync("legacypw1", 10); + await env.DB.prepare("INSERT INTO users (id, email, password_hash) VALUES (?, ?, ?)") + .bind(crypto.randomUUID(), "legacy@test.dev", legacy) + .run(); + + const c = client(); + expect((await c.fetch("/api/auth/login", json({ email: "legacy@test.dev", password: "legacypw1" }))).status).toBe( + 200 + ); + + const stored = await env.DB.prepare("SELECT password_hash FROM users WHERE email = ?") + .bind("legacy@test.dev") + .first<{ password_hash: string }>(); + expect(stored!.password_hash.startsWith("pbkdf2$")).toBe(true); + + // The upgraded hash still authenticates the same password, and only that one. + expect((await client().fetch("/api/auth/login", json({ email: "legacy@test.dev", password: "legacypw1" }))).status) + .toBe(200); + expect((await client().fetch("/api/auth/login", json({ email: "legacy@test.dev", password: "wrongpw123" }))).status) + .toBe(401); + }); + it("rejects bad credentials and unauthenticated API access", async () => { const c = client(); const bad = await c.fetch("/api/auth/login", json({ email: "a@test.dev", password: "wrongpass1" })); diff --git a/worker/lib/password.ts b/worker/lib/password.ts new file mode 100644 index 0000000..b94eca3 --- /dev/null +++ b/worker/lib/password.ts @@ -0,0 +1,85 @@ +import bcrypt from "bcryptjs"; + +// bcryptjs is pure JavaScript, so a cost-10 hash burns ~74ms of CPU — 7x the +// Workers Free plan's 10ms per-request cap, which made login hard-fail with +// "exceeded resource limits" on any fork deployed there. PBKDF2 through +// WebCrypto runs natively, so the same CPU budget buys far more work. See +// ITERATIONS below for the strength/cost trade-off we picked and why. +// +// Stored format: pbkdf2$sha256$$$ +// Legacy bcrypt hashes (they start with "$2") are still verified, then +// transparently upgraded on the next successful login. + +// 50k is a deliberate compromise, not an oversight. The Workers Free plan caps +// CPU at 10ms per request, and no password hash is both OWASP-grade and that +// cheap — slowness is the point of a password hash. Measured cost per login: +// +// bcrypt cost-10 (previous) 74ms — 7x over the free cap, login 1102s +// PBKDF2 600k (OWASP 2024) 72ms — same problem +// PBKDF2 50k 7ms — fits, at ~12x below OWASP guidance +// +// We optimise for "anyone can fork this and deploy it for free", and accept the +// weaker factor. The iteration count is stored inside each hash, so raising it +// later costs nothing: verification reads the stored value, and any account can +// be re-hashed on its next successful login. +const ITERATIONS = 50_000; +const SALT_BYTES = 16; +const KEY_BITS = 256; + +const b64 = (bytes: Uint8Array) => btoa(String.fromCharCode(...bytes)); +const unb64 = (s: string) => Uint8Array.from(atob(s), (ch) => ch.charCodeAt(0)); + +async function derive(password: string, salt: Uint8Array, iterations: number): Promise { + const key = await crypto.subtle.importKey("raw", new TextEncoder().encode(password), "PBKDF2", false, [ + "deriveBits", + ]); + const bits = await crypto.subtle.deriveBits( + { name: "PBKDF2", salt, iterations, hash: "SHA-256" }, + key, + KEY_BITS + ); + return new Uint8Array(bits); +} + +// Compares in time proportional to length only, never short-circuiting on the +// first differing byte. crypto.subtle.timingSafeEqual exists in workerd but not +// in Node, where the tests run, so this stays hand-rolled and portable. +function timingSafeEqual(a: Uint8Array, b: Uint8Array): boolean { + if (a.length !== b.length) return false; + let diff = 0; + for (let i = 0; i < a.length; i++) diff |= a[i] ^ b[i]; + return diff === 0; +} + +export async function hashPassword(password: string): Promise { + const salt = crypto.getRandomValues(new Uint8Array(SALT_BYTES)); + const derived = await derive(password, salt, ITERATIONS); + return `pbkdf2$sha256$${ITERATIONS}$${b64(salt)}$${b64(derived)}`; +} + +export function isLegacyHash(stored: string): boolean { + return stored.startsWith("$2"); +} + +export async function verifyPassword(password: string, stored: string): Promise { + if (isLegacyHash(stored)) return bcrypt.compare(password, stored); + + const [scheme, hash, iterations, salt, derived] = stored.split("$"); + if (scheme !== "pbkdf2" || hash !== "sha256" || !salt || !derived) return false; + + const iters = Number(iterations); + if (!Number.isInteger(iters) || iters < 1) return false; + + const actual = await derive(password, unb64(salt), iters); + return timingSafeEqual(actual, unb64(derived)); +} + +// A precomputed hash of an unguessable value. Verifying against it lets login +// spend the same CPU on an unknown email as on a real one, so response time +// stops revealing which addresses are registered. +let dummyHash: string | undefined; +export async function dummyVerify(password: string): Promise { + dummyHash ??= await hashPassword(crypto.randomUUID()); + await verifyPassword(password, dummyHash); + return false; +} diff --git a/worker/routes/auth.ts b/worker/routes/auth.ts index 724ac07..e43209f 100644 --- a/worker/routes/auth.ts +++ b/worker/routes/auth.ts @@ -1,7 +1,7 @@ import { Hono } from "hono"; import { z } from "zod"; -import bcrypt from "bcryptjs"; import { createSession, clearSession, verifySession, type AppEnv } from "../lib/auth"; +import { hashPassword, verifyPassword, isLegacyHash } from "../lib/password"; const credentials = z.object({ email: z.string().email().max(254), @@ -29,7 +29,7 @@ auth.post("/register", async (c) => { if (existing) return c.json({ error: "Account already exists" }, 409); const id = crypto.randomUUID(); - const hash = await bcrypt.hash(password, 10); + const hash = await hashPassword(password); await c.env.DB.prepare("INSERT INTO users (id, email, password_hash) VALUES (?, ?, ?)") .bind(id, email.toLowerCase(), hash) .run(); @@ -45,9 +45,16 @@ auth.post("/login", async (c) => { const user = await c.env.DB.prepare("SELECT id, email, password_hash FROM users WHERE email = ?") .bind(email.toLowerCase()) .first<{ id: string; email: string; password_hash: string }>(); - const ok = user && (await bcrypt.compare(password, user.password_hash)); + const ok = user && (await verifyPassword(password, user.password_hash)); if (!ok) return c.json({ error: "Invalid email or password" }, 401); + // Upgrade the account off bcrypt the first time it logs in successfully. + if (isLegacyHash(user.password_hash)) { + await c.env.DB.prepare("UPDATE users SET password_hash = ? WHERE id = ?") + .bind(await hashPassword(password), user.id) + .run(); + } + await createSession(c, user.id); return c.json({ id: user.id, email: user.email }); }); @@ -77,11 +84,11 @@ auth.post("/change-password", async (c) => { const user = await c.env.DB.prepare("SELECT password_hash FROM users WHERE id = ?") .bind(userId) .first<{ password_hash: string }>(); - if (!user || !(await bcrypt.compare(currentPassword, user.password_hash))) { + if (!user || !(await verifyPassword(currentPassword, user.password_hash))) { return c.json({ error: "Current password is incorrect" }, 401); } - const hash = await bcrypt.hash(newPassword, 10); + const hash = await hashPassword(newPassword); await c.env.DB.prepare("UPDATE users SET password_hash = ? WHERE id = ?").bind(hash, userId).run(); return c.json({ ok: true }); }); From 55f993921bc4738a690935900ae08ab4d6e67bf7 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:51:48 +0530 Subject: [PATCH 03/15] Revoke outstanding sessions when the password changes Session JWTs were stateless, so change-password rehashed the secret but left every already-issued cookie valid for its full 30-day life. The one action a user takes when they think they have been compromised did not actually lock the attacker out. The existing test missed this because it checked the old *password* stopped working, not the old *session*. Add users.token_version, embed it in the JWT, and compare it against the stored value on every authenticated request. change-password increments it, revoking prior sessions, and re-issues a cookie for the calling device so changing your password doesn't sign you out of it. This costs one D1 read per authenticated request. That is the price of being able to revoke a session at all. verifySession is replaced by authenticate() rather than extended, so the compiler flags any call site still doing a signature-only check. The test bootstrap now reads the whole migrations directory instead of naming files, so a new migration can't land without the schema tracking it. Co-Authored-By: Claude Opus 4.8 --- migrations/0003_token_version.sql | 7 ++++++ worker/api.test.ts | 38 ++++++++++++++++++++++++++----- worker/lib/auth.ts | 28 +++++++++++++++++++---- worker/routes/auth.ts | 30 ++++++++++++++++-------- 4 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 migrations/0003_token_version.sql diff --git a/migrations/0003_token_version.sql b/migrations/0003_token_version.sql new file mode 100644 index 0000000..8157767 --- /dev/null +++ b/migrations/0003_token_version.sql @@ -0,0 +1,7 @@ +-- Session JWTs were stateless, so changing your password rehashed the secret +-- but left every already-issued cookie valid for its full 30-day life. A +-- stolen session survived the exact action taken to revoke it. +-- +-- token_version is embedded in each JWT and compared on every authenticated +-- request. Bumping it invalidates all outstanding sessions for that user. +ALTER TABLE users ADD COLUMN token_version INTEGER NOT NULL DEFAULT 0; diff --git a/worker/api.test.ts b/worker/api.test.ts index 93d04d5..942e66f 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -1,5 +1,5 @@ import { beforeAll, afterAll, describe, expect, it } from "vitest"; -import { readFileSync } from "node:fs"; +import { readFileSync, readdirSync } from "node:fs"; import bcrypt from "bcryptjs"; import { getPlatformProxy } from "wrangler"; import app from "./index"; @@ -17,11 +17,14 @@ beforeAll(async () => { env = { ...proxy.env, JWT_SECRET: TEST_SECRET, ALLOW_REGISTRATION: "true" }; dispose = proxy.dispose; - const statements = ( - readFileSync(new URL("../migrations/0001_init.sql", import.meta.url), "utf8") + - "\n" + - readFileSync(new URL("../migrations/0002_salary_currency_period.sql", import.meta.url), "utf8") - ) + // Read the whole directory in order rather than naming files: a new migration + // must not be able to land without the test schema picking it up. + const dir = new URL("../migrations/", import.meta.url); + const statements = readdirSync(dir) + .filter((f) => f.endsWith(".sql")) + .sort() + .map((f) => readFileSync(new URL(f, dir), "utf8")) + .join("\n") .split(";") .map((s) => s.replace(/--.*$/gm, "").trim()) .filter(Boolean); @@ -165,6 +168,29 @@ describe("auth", () => { expect(newLogin.status).toBe(200); }); + it("revokes sessions issued before a password change, but not the one that made it", async () => { + const changer = client(); + await changer.fetch("/api/auth/register", json({ email: "revoke@test.dev", password: "originalpw1" })); + + // A second device logged in with the same account, before the change. + const other = client(); + await other.fetch("/api/auth/login", json({ email: "revoke@test.dev", password: "originalpw1" })); + expect((await other.fetch("/api/auth/me")).status).toBe(200); + + await changer.fetch( + "/api/auth/change-password", + json({ currentPassword: "originalpw1", newPassword: "brandnew12" }) + ); + + // The other device's cookie is still a validly-signed JWT, but its token + // version is stale — it must no longer authenticate. + expect((await other.fetch("/api/auth/me")).status).toBe(401); + expect((await other.fetch("/api/jobs")).status).toBe(401); + + // The device that changed the password stays signed in. + expect((await changer.fetch("/api/auth/me")).status).toBe(200); + }); + it("requires auth for change-password and validates the new password length", async () => { expect( (await client().fetch("/api/auth/change-password", json({ currentPassword: "x", newPassword: "brandnew12" }))) diff --git a/worker/lib/auth.ts b/worker/lib/auth.ts index 3340535..360862f 100644 --- a/worker/lib/auth.ts +++ b/worker/lib/auth.ts @@ -23,8 +23,8 @@ function secretKey(secret: string | undefined) { return new TextEncoder().encode(secret); } -export async function createSession(c: Context, userId: string) { - const token = await new SignJWT({ sub: userId }) +export async function createSession(c: Context, userId: string, tokenVersion: number) { + const token = await new SignJWT({ sub: userId, tv: tokenVersion }) .setProtectedHeader({ alg: "HS256" }) .setIssuedAt() .setExpirationTime(`${SESSION_DAYS}d`) @@ -43,22 +43,40 @@ export function clearSession(c: Context) { deleteCookie(c, AUTH_COOKIE, { path: "/" }); } -export async function verifySession(c: Context): Promise { +// Authenticates the request: the cookie's JWT must be valid AND its token +// version must still match the user's current one. A valid signature alone is +// not enough — that is what let sessions outlive a password change. +export async function authenticate(c: Context): Promise { const token = getCookie(c, AUTH_COOKIE); if (!token) return null; // Resolved outside the try: a bad JWT_SECRET is a misconfigured server, not a // bad token, and must not be swallowed into a 401. const key = secretKey(c.env.JWT_SECRET); + + let userId: string; + let tokenVersion: number; try { const { payload } = await jwtVerify(token, key); - return typeof payload.sub === "string" ? payload.sub : null; + if (typeof payload.sub !== "string" || typeof payload.tv !== "number") return null; + userId = payload.sub; + tokenVersion = payload.tv; } catch { return null; } + + // Costs one D1 read per authenticated request. That is the price of being + // able to revoke a session at all; the alternative is a 30-day window in + // which a leaked cookie cannot be taken away. + const user = await c.env.DB.prepare("SELECT token_version FROM users WHERE id = ?") + .bind(userId) + .first<{ token_version: number }>(); + if (!user || user.token_version !== tokenVersion) return null; + + return userId; } export async function requireAuth(c: Context, next: Next) { - const userId = await verifySession(c); + const userId = await authenticate(c); if (!userId) return c.json({ error: "Unauthorized" }, 401); c.set("userId", userId); await next(); diff --git a/worker/routes/auth.ts b/worker/routes/auth.ts index e43209f..f961107 100644 --- a/worker/routes/auth.ts +++ b/worker/routes/auth.ts @@ -1,6 +1,6 @@ import { Hono } from "hono"; import { z } from "zod"; -import { createSession, clearSession, verifySession, type AppEnv } from "../lib/auth"; +import { createSession, clearSession, authenticate, type AppEnv } from "../lib/auth"; import { hashPassword, verifyPassword, isLegacyHash } from "../lib/password"; const credentials = z.object({ @@ -33,7 +33,7 @@ auth.post("/register", async (c) => { await c.env.DB.prepare("INSERT INTO users (id, email, password_hash) VALUES (?, ?, ?)") .bind(id, email.toLowerCase(), hash) .run(); - await createSession(c, id); + await createSession(c, id, 0); return c.json({ id, email: email.toLowerCase() }, 201); }); @@ -42,9 +42,11 @@ auth.post("/login", async (c) => { if (!parsed.success) return c.json({ error: "Email and password required" }, 400); const { email, password } = parsed.data; - const user = await c.env.DB.prepare("SELECT id, email, password_hash FROM users WHERE email = ?") + const user = await c.env.DB.prepare( + "SELECT id, email, password_hash, token_version FROM users WHERE email = ?" + ) .bind(email.toLowerCase()) - .first<{ id: string; email: string; password_hash: string }>(); + .first<{ id: string; email: string; password_hash: string; token_version: number }>(); const ok = user && (await verifyPassword(password, user.password_hash)); if (!ok) return c.json({ error: "Invalid email or password" }, 401); @@ -55,7 +57,7 @@ auth.post("/login", async (c) => { .run(); } - await createSession(c, user.id); + await createSession(c, user.id, user.token_version); return c.json({ id: user.id, email: user.email }); }); @@ -65,7 +67,7 @@ auth.post("/logout", (c) => { }); auth.delete("/account", async (c) => { - const userId = await verifySession(c); + const userId = await authenticate(c); if (!userId) return c.json({ error: "Unauthorized" }, 401); // Cascades to jobs, contacts, activities, reminders via FK ON DELETE CASCADE. await c.env.DB.prepare("DELETE FROM users WHERE id = ?").bind(userId).run(); @@ -74,7 +76,7 @@ auth.delete("/account", async (c) => { }); auth.post("/change-password", async (c) => { - const userId = await verifySession(c); + const userId = await authenticate(c); if (!userId) return c.json({ error: "Unauthorized" }, 401); const parsed = changePasswordBody.safeParse(await c.req.json().catch(() => null)); @@ -88,13 +90,23 @@ auth.post("/change-password", async (c) => { return c.json({ error: "Current password is incorrect" }, 401); } + // Bumping token_version revokes every session issued before this change — + // the point of changing a password is that a leaked one stops working. const hash = await hashPassword(newPassword); - await c.env.DB.prepare("UPDATE users SET password_hash = ? WHERE id = ?").bind(hash, userId).run(); + const updated = await c.env.DB.prepare( + "UPDATE users SET password_hash = ?, token_version = token_version + 1 WHERE id = ? RETURNING token_version" + ) + .bind(hash, userId) + .first<{ token_version: number }>(); + + // Re-issue for the device that made the change, so "change password" doesn't + // also mean "log yourself out". + await createSession(c, userId, updated!.token_version); return c.json({ ok: true }); }); auth.get("/me", async (c) => { - const userId = await verifySession(c); + const userId = await authenticate(c); if (!userId) return c.json({ error: "Unauthorized" }, 401); const user = await c.env.DB.prepare("SELECT id, email FROM users WHERE id = ?") .bind(userId) From 25e3e2343f492927436ca0ebc5fa2b388cf9bc48 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:52:41 +0530 Subject: [PATCH 04/15] Close the login user-enumeration timing leak An unknown email returned before any hashing ran, so a miss answered ~6ms faster than a real account with a wrong password. That difference is stable enough to enumerate which addresses are registered. Verify against a dummy hash on the unknown-email path so both cost the same work, and cover it with a timing test that would catch the early return coming back. Co-Authored-By: Claude Opus 4.8 --- worker/api.test.ts | 26 ++++++++++++++++++++++++++ worker/routes/auth.ts | 14 +++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/worker/api.test.ts b/worker/api.test.ts index 942e66f..d5781b5 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -119,6 +119,32 @@ describe("auth", () => { .toBe(401); }); + it("takes comparable time for an unknown email as for a wrong password", async () => { + await client().fetch("/api/auth/register", json({ email: "timing@test.dev", password: "originalpw1" })); + + const median = async (email: string) => { + const runs: number[] = []; + for (let i = 0; i < 7; i++) { + const t = performance.now(); + await client().fetch("/api/auth/login", json({ email, password: "wrongpassword" })); + runs.push(performance.now() - t); + } + return runs.sort((a, b) => a - b)[3]; + }; + + // Warm the lazily-built dummy hash so it isn't charged to the first sample. + await client().fetch("/api/auth/login", json({ email: "nobody@test.dev", password: "wrongpassword" })); + + const known = await median("timing@test.dev"); + const unknown = await median("nobody@test.dev"); + + // Before the fix an unknown email skipped hashing entirely and returned in + // a fraction of the time. They should now be within the same ballpark; + // the bound is loose because CI timing is noisy, but a regression would + // show up as unknown being many times faster, not marginally so. + expect(unknown).toBeGreaterThan(known * 0.5); + }); + it("rejects bad credentials and unauthenticated API access", async () => { const c = client(); const bad = await c.fetch("/api/auth/login", json({ email: "a@test.dev", password: "wrongpass1" })); diff --git a/worker/routes/auth.ts b/worker/routes/auth.ts index f961107..4db43d0 100644 --- a/worker/routes/auth.ts +++ b/worker/routes/auth.ts @@ -1,7 +1,7 @@ import { Hono } from "hono"; import { z } from "zod"; import { createSession, clearSession, authenticate, type AppEnv } from "../lib/auth"; -import { hashPassword, verifyPassword, isLegacyHash } from "../lib/password"; +import { hashPassword, verifyPassword, isLegacyHash, dummyVerify } from "../lib/password"; const credentials = z.object({ email: z.string().email().max(254), @@ -47,8 +47,16 @@ auth.post("/login", async (c) => { ) .bind(email.toLowerCase()) .first<{ id: string; email: string; password_hash: string; token_version: number }>(); - const ok = user && (await verifyPassword(password, user.password_hash)); - if (!ok) return c.json({ error: "Invalid email or password" }, 401); + const invalid = () => c.json({ error: "Invalid email or password" }, 401); + + // An unknown email must cost the same as a known one. Skipping the hash here + // made a miss ~6ms faster than a hit, which is a reliable oracle for + // enumerating which addresses have accounts. + if (!user) { + await dummyVerify(password); + return invalid(); + } + if (!(await verifyPassword(password, user.password_hash))) return invalid(); // Upgrade the account off bcrypt the first time it logs in successfully. if (isLegacyHash(user.password_hash)) { From 4c6b892f067ce2ec3381f8377f0faca2c60d6074 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:54:32 +0530 Subject: [PATCH 05/15] Rate limit the credential endpoints login, register and change-password accepted unlimited attempts from a single IP. PBKDF2 makes each guess cost ~6ms, which slows a brute-force run down but does not stop one. Add Cloudflare's rate-limit binding at 10 attempts/minute/IP, keyed on CF-Connecting-IP (set by the edge, not spoofable by the client). An unknown IP shares one bucket, so it is throttled rather than exempt. The binding is treated as optional: if it is missing the endpoints still work and log a warning, because rate limiting is a production defence and not a correctness property. Note for the tests: getPlatformProxy hands back a real limiter once wrangler.jsonc declares one, so the suite was throttling itself. The default test env drops it and the dedicated test injects a stub. Co-Authored-By: Claude Opus 4.8 --- worker/api.test.ts | 36 +++++++++++++++++++++++++++++++++++- worker/env.d.ts | 16 +++++++++++----- worker/lib/ratelimit.ts | 40 ++++++++++++++++++++++++++++++++++++++++ worker/routes/auth.ts | 8 ++++++++ wrangler.jsonc | 10 ++++++++++ 5 files changed, 104 insertions(+), 6 deletions(-) create mode 100644 worker/lib/ratelimit.ts diff --git a/worker/api.test.ts b/worker/api.test.ts index d5781b5..ce004d0 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -14,7 +14,11 @@ const TEST_SECRET = "test-secret-that-is-long-enough-to-pass"; beforeAll(async () => { const proxy = await getPlatformProxy({ persist: false }); - env = { ...proxy.env, JWT_SECRET: TEST_SECRET, ALLOW_REGISTRATION: "true" }; + // getPlatformProxy DOES hand back a real AUTH_RATE_LIMITER because + // wrangler.jsonc declares one, and the suite makes far more than 10 login + // calls from a single (absent → "unknown") IP. Drop it by default so the + // limiter can't throttle unrelated tests; the rate-limit test injects a stub. + env = { ...proxy.env, AUTH_RATE_LIMITER: undefined, JWT_SECRET: TEST_SECRET, ALLOW_REGISTRATION: "true" }; dispose = proxy.dispose; // Read the whole directory in order rather than naming files: a new migration @@ -145,6 +149,36 @@ describe("auth", () => { expect(unknown).toBeGreaterThan(known * 0.5); }); + it("rate limits login attempts when the binding is present, and works without it", async () => { + let calls = 0; + const limited: Env = { + ...env, + AUTH_RATE_LIMITER: { + limit: async ({ key }) => { + expect(key).toBe("login:203.0.113.7"); // bucket + client IP + return { success: ++calls <= 2 }; + }, + }, + }; + + const attempt = (e: Env) => + app.request( + "/api/auth/login", + { + ...json({ email: "nobody@test.dev", password: "wrongpassword" }), + headers: { "Content-Type": "application/json", "CF-Connecting-IP": "203.0.113.7" }, + }, + e + ); + + expect((await attempt(limited)).status).toBe(401); // 1st: allowed, bad creds + expect((await attempt(limited)).status).toBe(401); // 2nd: allowed, bad creds + expect((await attempt(limited)).status).toBe(429); // 3rd: throttled + + // Absent binding (tests, local dev) must not break the endpoint. + expect((await attempt(env)).status).toBe(401); + }); + it("rejects bad credentials and unauthenticated API access", async () => { const c = client(); const bad = await c.fetch("/api/auth/login", json({ email: "a@test.dev", password: "wrongpass1" })); diff --git a/worker/env.d.ts b/worker/env.d.ts index 44ae473..d6a821f 100644 --- a/worker/env.d.ts +++ b/worker/env.d.ts @@ -1,6 +1,12 @@ -interface Env { - DB: D1Database; - ASSETS: Fetcher; - JWT_SECRET: string; - ALLOW_REGISTRATION?: string; +import type { RateLimiter } from "./lib/ratelimit"; + +declare global { + interface Env { + DB: D1Database; + ASSETS: Fetcher; + JWT_SECRET: string; + ALLOW_REGISTRATION?: string; + // Optional: not provided by vitest's platform proxy. See lib/ratelimit.ts. + AUTH_RATE_LIMITER?: RateLimiter; + } } diff --git a/worker/lib/ratelimit.ts b/worker/lib/ratelimit.ts new file mode 100644 index 0000000..00425c4 --- /dev/null +++ b/worker/lib/ratelimit.ts @@ -0,0 +1,40 @@ +import type { Context, Next } from "hono"; +import type { AppEnv } from "./auth"; + +// Cloudflare's rate-limit binding. Typed locally rather than pulled from +// workers-types so the shape is visible at the call site. +export interface RateLimiter { + limit(options: { key: string }): Promise<{ success: boolean }>; +} + +// The binding does not exist under vitest's platform proxy or in some local dev +// setups. Rate limiting is a production defence, not a correctness property, so +// its absence must not break the app — but it should be obvious, not silent. +let warned = false; + +/** + * Throttles by client IP. Used on the credential endpoints, where the whole + * attack is "try a lot of passwords quickly". + */ +export function rateLimitByIp(bucket: string) { + return async (c: Context, next: Next) => { + const limiter = c.env.AUTH_RATE_LIMITER; + if (!limiter) { + if (!warned) { + warned = true; + console.warn("AUTH_RATE_LIMITER binding is absent — credential endpoints are NOT rate limited."); + } + return next(); + } + + // CF-Connecting-IP is set by the edge and cannot be spoofed by the client. + // Falling back to a constant means an unknown IP shares one bucket, which + // throttles rather than exempts it. + const ip = c.req.header("CF-Connecting-IP") ?? "unknown"; + const { success } = await limiter.limit({ key: `${bucket}:${ip}` }); + if (!success) { + return c.json({ error: "Too many attempts. Wait a minute and try again." }, 429); + } + return next(); + }; +} diff --git a/worker/routes/auth.ts b/worker/routes/auth.ts index 4db43d0..fcd8584 100644 --- a/worker/routes/auth.ts +++ b/worker/routes/auth.ts @@ -2,6 +2,7 @@ import { Hono } from "hono"; import { z } from "zod"; import { createSession, clearSession, authenticate, type AppEnv } from "../lib/auth"; import { hashPassword, verifyPassword, isLegacyHash, dummyVerify } from "../lib/password"; +import { rateLimitByIp } from "../lib/ratelimit"; const credentials = z.object({ email: z.string().email().max(254), @@ -15,6 +16,13 @@ const changePasswordBody = z.object({ const auth = new Hono(); +// Guessing a password is a game of volume; these are the only endpoints where +// volume is the attack. change-password is included because it accepts the +// current password. +auth.use("/login", rateLimitByIp("login")); +auth.use("/register", rateLimitByIp("register")); +auth.use("/change-password", rateLimitByIp("change-password")); + auth.post("/register", async (c) => { if (c.env.ALLOW_REGISTRATION !== "true") { return c.json({ error: "This Tracker instance is private. You can run your own — fork it on GitHub." }, 403); diff --git a/wrangler.jsonc b/wrangler.jsonc index e6f2ef1..ca9eb46 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -22,6 +22,16 @@ "migrations_dir": "migrations" } ], + // Throttles the credential endpoints. `period` may only be 10 or 60. + // 10 attempts per minute per IP is generous for a human and useless for a + // brute-force run. + "ratelimits": [ + { + "name": "AUTH_RATE_LIMITER", + "namespace_id": "1001", + "simple": { "limit": 10, "period": 60 } + } + ], "vars": { // Solo tool: registration closed by default. Flip to "true" temporarily // to create the one account, then back (postly pattern). From c207061fd16f6a2257c24a0183c31847c946eef0 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:55:52 +0530 Subject: [PATCH 06/15] Reject and sanitize non-http(s) URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit zod's .url() only runs `new URL()`, which accepts javascript: and data:. Both reach an : job.url in the drawer header, and contact.linkedin — which had no URL validation at all, just a length cap. A stored "javascript:…" would run on click. This is self-XSS in a single-user tool rather than a way in from outside, but it is a five-second fix and stops being self-inflicted the moment anything is shared. Guard it on both sides. safeExternalUrl() gates the scheme on write, and the drawer runs the same check on render, because rows written before this landed may already contain one. Co-Authored-By: Claude Opus 4.8 --- shared/types.ts | 14 ++++++++++++++ src/components/JobDrawer.tsx | 9 +++++---- worker/api.test.ts | 24 ++++++++++++++++++++++++ worker/routes/jobs.ts | 16 ++++++++++++++-- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/shared/types.ts b/shared/types.ts index 69a9803..3f3bb43 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -1,3 +1,17 @@ +// `new URL()` — which is all zod's .url() checks — happily accepts +// "javascript:alert(1)" and "data:text/html,…". Both of those become live +// script when dropped into an , so every user-supplied URL is checked +// against this before it is stored OR rendered. Rendering is checked too, not +// just writing: rows saved before this existed may already hold a bad scheme. +export function safeExternalUrl(url: string | null | undefined): string | null { + if (!url) return null; + try { + return /^https?:$/.test(new URL(url).protocol) ? url : null; + } catch { + return null; // not a parseable absolute URL + } +} + export const JOB_STATUSES = ["wishlist", "applied", "interview", "offer", "rejected"] as const; export type JobStatus = (typeof JOB_STATUSES)[number]; diff --git a/src/components/JobDrawer.tsx b/src/components/JobDrawer.tsx index 61ae7d4..2625ae2 100644 --- a/src/components/JobDrawer.tsx +++ b/src/components/JobDrawer.tsx @@ -23,6 +23,7 @@ import { JOB_STATUSES, PERIODS, STATUS_LABELS, + safeExternalUrl, type Activity, type Contact, type Job, @@ -102,9 +103,9 @@ export default function JobDrawer({ job, onClose, onChange, onDelete }: Props) { ))} - {job.url && ( + {safeExternalUrl(job.url) && ( )} - {ct.linkedin && ( - + {safeExternalUrl(ct.linkedin) && ( + LinkedIn diff --git a/worker/api.test.ts b/worker/api.test.ts index ce004d0..7dfd21e 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -311,6 +311,30 @@ describe("jobs", () => { expect((await c.fetch("/api/jobs", json({ company: "X", title: "Y", status: "nope" }))).status).toBe(400); }); + it("rejects non-http(s) URLs on job.url and contact.linkedin", async () => { + const c = client(); + await c.fetch("/api/auth/register", json({ email: "urls@test.dev", password: "password1" })); + + // These all satisfy `new URL()`, which is the only thing zod's .url() + // checks, and all become executable script in an . + const dangerous = ["javascript:alert(document.cookie)", "data:text/html,", "vbscript:msgbox"]; + + for (const url of dangerous) { + expect((await c.fetch("/api/jobs", json({ company: "X", title: "Y", url }))).status).toBe(400); + } + + const job = (await (await c.fetch("/api/jobs", json({ company: "Ok", title: "Role" }))).json()) as Job; + for (const linkedin of dangerous) { + expect((await c.fetch(`/api/jobs/${job.id}/contacts`, json({ name: "N", linkedin }))).status).toBe(400); + } + + // Genuine URLs still go through, and PATCH is guarded too. + expect((await c.fetch("/api/jobs", json({ company: "X", title: "Y", url: "https://ok.dev/job" }))).status).toBe(201); + expect( + (await c.fetch(`/api/jobs/${job.id}`, { method: "PATCH", body: JSON.stringify({ url: dangerous[0] }) })).status + ).toBe(400); + }); + it("isolates users from each other", async () => { const alice = client(); await alice.fetch("/api/auth/register", json({ email: "alice@test.dev", password: "password1" })); diff --git a/worker/routes/jobs.ts b/worker/routes/jobs.ts index f6f83ab..1865e96 100644 --- a/worker/routes/jobs.ts +++ b/worker/routes/jobs.ts @@ -6,15 +6,25 @@ import { STATUS_LABELS, CURRENCIES, PERIODS, + safeExternalUrl, type Job, type JobStatus, } from "../../shared/types"; import type { AppEnv } from "../lib/auth"; +// Only http(s). See safeExternalUrl: zod's .url() alone would let a +// javascript: scheme through to an href. +const httpUrl = (max: number) => + z + .string() + .trim() + .max(max) + .refine((u) => safeExternalUrl(u) !== null, "Must be an http:// or https:// URL"); + const jobFields = z.object({ company: z.string().trim().min(1).max(200), title: z.string().trim().min(1).max(200), - url: z.string().trim().url().max(2000).nullish().or(z.literal("").transform(() => null)), + url: httpUrl(2000).nullish().or(z.literal("").transform(() => null)), location: z.string().trim().max(200).nullish(), salary_min: z.number().int().nonnegative().nullish(), salary_max: z.number().int().nonnegative().nullish(), @@ -37,7 +47,9 @@ const contactFields = z.object({ role: z.string().trim().max(200).nullish(), email: z.string().trim().email().max(254).nullish(), phone: z.string().trim().max(50).nullish(), - linkedin: z.string().trim().max(500).nullish(), + linkedin: httpUrl(500) + .nullish() + .or(z.literal("").transform(() => null)), notes: z.string().max(5000).nullish(), }); From 5d9d5cf82e2908a527158428f882f4a37ad3cdde Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:56:34 +0530 Subject: [PATCH 07/15] Return 409 rather than 500 when registration races Registration checked for an existing email and then inserted. Two concurrent signups for one address both passed the check, and the loser hit the UNIQUE constraint, which fell through to onError as a 500. Drop the pre-check and let the constraint be the single source of truth, mapping the violation to the 409 that was always intended. Co-Authored-By: Claude Opus 4.8 --- worker/api.test.ts | 30 ++++++++++++++++++++++++++++++ worker/routes/auth.ts | 23 +++++++++++++++-------- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/worker/api.test.ts b/worker/api.test.ts index 7dfd21e..17bb7c0 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -71,6 +71,36 @@ describe("auth", () => { expect((await c.fetch("/api/auth/me")).status).toBe(401); }); + it("returns 409, not 500, when the same email registers twice or races itself", async () => { + const register = () => + app.request( + "/api/auth/register", + { ...json({ email: "dupe@test.dev", password: "password1" }), headers: { "Content-Type": "application/json" } }, + env + ); + + // Sequential: the second is a plain duplicate. + expect((await register()).status).toBe(201); + expect((await register()).status).toBe(409); + + // Concurrent: both pass a check-then-insert, and the loser used to hit the + // UNIQUE constraint and fall through to onError as a 500. + const raced = await Promise.all([ + app.request( + "/api/auth/register", + { ...json({ email: "race@test.dev", password: "password1" }), headers: { "Content-Type": "application/json" } }, + env + ), + app.request( + "/api/auth/register", + { ...json({ email: "race@test.dev", password: "password1" }), headers: { "Content-Type": "application/json" } }, + env + ), + ]); + const codes = raced.map((r) => r.status).sort(); + expect(codes).toEqual([201, 409]); + }); + it("rejects registration when closed", async () => { const closed = { ...env, ALLOW_REGISTRATION: "false" }; const res = await app.request( diff --git a/worker/routes/auth.ts b/worker/routes/auth.ts index fcd8584..79aa020 100644 --- a/worker/routes/auth.ts +++ b/worker/routes/auth.ts @@ -31,16 +31,23 @@ auth.post("/register", async (c) => { if (!parsed.success) return c.json({ error: "Valid email and password (min 8 chars) required" }, 400); const { email, password } = parsed.data; - const existing = await c.env.DB.prepare("SELECT id FROM users WHERE email = ?") - .bind(email.toLowerCase()) - .first(); - if (existing) return c.json({ error: "Account already exists" }, 409); - const id = crypto.randomUUID(); const hash = await hashPassword(password); - await c.env.DB.prepare("INSERT INTO users (id, email, password_hash) VALUES (?, ?, ?)") - .bind(id, email.toLowerCase(), hash) - .run(); + + // Let the UNIQUE(email) constraint decide, rather than checking first and + // then inserting: two concurrent signups for the same address both passed + // the check and the loser hit the constraint, surfacing as a 500. + try { + await c.env.DB.prepare("INSERT INTO users (id, email, password_hash) VALUES (?, ?, ?)") + .bind(id, email.toLowerCase(), hash) + .run(); + } catch (err) { + if (String(err).includes("UNIQUE constraint failed")) { + return c.json({ error: "Account already exists" }, 409); + } + throw err; + } + await createSession(c, id, 0); return c.json({ id, email: email.toLowerCase() }, 201); }); From 34c14699929b39fcc337d6d4b3c6b17964c76fcd Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:57:17 +0530 Subject: [PATCH 08/15] Exclude archived jobs from every stat, not just the funnel The funnel query filtered archived = 0 but the response-rate, average days-to-interview and weekly-applications queries did not. Archiving a job removed it from the funnel while it kept dragging on the response rate, so two numbers on the same dashboard disagreed about which jobs existed. Co-Authored-By: Claude Opus 4.8 --- worker/api.test.ts | 21 +++++++++++++++++++++ worker/routes/stats.ts | 12 +++++++++--- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/worker/api.test.ts b/worker/api.test.ts index 17bb7c0..55a86bf 100644 --- a/worker/api.test.ts +++ b/worker/api.test.ts @@ -334,6 +334,27 @@ describe("jobs", () => { expect((await c.fetch(`/api/jobs/${job.id}`)).status).toBe(404); }); + it("excludes archived jobs from every stat, not just the funnel", async () => { + const c = client(); + await c.fetch("/api/auth/register", json({ email: "arch@test.dev", password: "password1" })); + + // Two applied jobs, neither of which ever got a response. + for (const company of ["Ghosted A", "Ghosted B"]) { + const j = (await (await c.fetch("/api/jobs", json({ company, title: "Role", status: "applied" }))).json()) as Job; + if (company === "Ghosted B") { + await c.fetch(`/api/jobs/${j.id}`, { method: "PATCH", body: JSON.stringify({ archived: 1 }) }); + } + } + + const stats = (await (await c.fetch("/api/stats")).json()) as Stats; + // The archived one is out of the funnel... + expect(stats.funnel.applied).toBe(1); + // ...and must also be out of the denominator behind the response rate, + // which previously still counted it. + expect(stats.responseRate).toBe(0); + expect(stats.weekly.reduce((n, w) => n + w.count, 0)).toBe(1); + }); + it("validates input", async () => { const c = client(); await c.fetch("/api/auth/register", json({ email: "val@test.dev", password: "password1" })); diff --git a/worker/routes/stats.ts b/worker/routes/stats.ts index 267544e..e51f418 100644 --- a/worker/routes/stats.ts +++ b/worker/routes/stats.ts @@ -4,6 +4,11 @@ import type { AppEnv } from "../lib/auth"; const INTERVIEW_TYPES = "('phone_screen', 'interview', 'onsite', 'offer')"; +// Every query here filters archived = 0. The funnel already did, but the rate, +// average and weekly queries did not, so archiving a job removed it from the +// funnel while it kept dragging on the response rate — the numbers on one +// screen disagreed with each other. + const stats = new Hono(); stats.get("/", async (c) => { @@ -25,7 +30,7 @@ stats.get("/", async (c) => { OR EXISTS (SELECT 1 FROM activities a WHERE a.job_id = jobs.id AND a.type IN ${INTERVIEW_TYPES}) THEN 1 ELSE 0 END) AS responded - FROM jobs WHERE user_id = ? AND applied_at IS NOT NULL` + FROM jobs WHERE user_id = ? AND archived = 0 AND applied_at IS NOT NULL` ) .bind(userId) .first<{ applied: number; responded: number | null }>(), @@ -36,14 +41,15 @@ stats.get("/", async (c) => { SELECT j.applied_at, (SELECT MIN(a.happened_at) FROM activities a WHERE a.job_id = j.id AND a.type IN ${INTERVIEW_TYPES}) AS first_iv - FROM jobs j WHERE j.user_id = ? AND j.applied_at IS NOT NULL + FROM jobs j WHERE j.user_id = ? AND j.archived = 0 AND j.applied_at IS NOT NULL ) WHERE first_iv IS NOT NULL` ) .bind(userId) .first<{ avg_days: number | null }>(), c.env.DB.prepare( - "SELECT applied_at FROM jobs WHERE user_id = ? AND applied_at >= datetime('now', '-84 days')" + `SELECT applied_at FROM jobs + WHERE user_id = ? AND archived = 0 AND applied_at >= datetime('now', '-84 days')` ) .bind(userId) .all<{ applied_at: string }>(), From 216d7f3bf4cbd69e087e030e6bc2590020b2c0f4 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Tue, 14 Jul 2026 04:58:36 +0530 Subject: [PATCH 09/15] Route the drawer's status change through /move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drawer's status dropdown called PATCH /jobs/:id, which updates the status column but never touches sort_order. The card therefore kept the ordering value it had in the column it left and landed at an arbitrary slot in the one it joined — sometimes colliding with a card already there. Send it through PATCH /jobs/:id/move, which pins the card at a requested index and renumbers the destination column, exactly as drag-and-drop does. The drawer's onChange now takes every job the server touched rather than a single one, since a move rewrites a whole column. Both callers merged by id already, so the change is confined to the signature. Co-Authored-By: Claude Opus 4.8 --- src/components/JobDrawer.tsx | 18 +++++++++++++++--- src/pages/Board.tsx | 4 +++- src/pages/TableView.tsx | 2 +- worker/api.test.ts | 30 ++++++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/components/JobDrawer.tsx b/src/components/JobDrawer.tsx index 2625ae2..1fb9db8 100644 --- a/src/components/JobDrawer.tsx +++ b/src/components/JobDrawer.tsx @@ -27,6 +27,7 @@ import { type Activity, type Contact, type Job, + type JobStatus, type Reminder, } from "../../shared/types"; import { Button } from "./ui/button"; @@ -45,7 +46,9 @@ const TAB_ICONS: Record = { interface Props { job: Job; onClose: () => void; - onChange: (job: Job) => void; + // Takes every job the server changed, not just this one: moving a card + // renumbers its whole destination column. + onChange: (jobs: Job[]) => void; onDelete: (id: string) => void; } @@ -64,7 +67,16 @@ export default function JobDrawer({ job, onClose, onChange, onDelete }: Props) { useFocusTrap(asideRef, true, onClose); const patch = async (fields: Partial) => { - onChange(await api.patch(`/jobs/${job.id}`, fields)); + onChange([await api.patch(`/jobs/${job.id}`, fields)]); + }; + + // Status has to go through /move, not PATCH. PATCH changes the status column + // but never touches sort_order, so the card kept the ordering value from the + // column it left and landed at an arbitrary slot in the one it joined. + // /move renumbers the destination column and returns all of its rows. + const changeStatus = async (status: JobStatus) => { + if (status === job.status) return; + onChange(await api.patch(`/jobs/${job.id}/move`, { status, index: 0 })); }; const remove = async () => { @@ -94,7 +106,7 @@ export default function JobDrawer({ job, onClose, onChange, onDelete }: Props) {