diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 22b7e43..b877dae 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,3 +23,7 @@ jobs: run: npm run typecheck - name: Tests run: npm test + # typecheck and tests both pass on a tree that fails to bundle, so main + # could break the deploy without CI noticing. + - name: Build + run: npm run build diff --git a/CHANGELOG.md b/CHANGELOG.md index c6a590b..3280437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,34 @@ All notable changes to this project are documented in this file. +## [Unreleased] + +### Security + +- **Refuse to start with a missing or weak `JWT_SECRET`.** An unset secret produced a zero-length signing key that `jose` would still sign *and verify* with, so a deploy that forgot to set it would have accepted forged sessions for any account, silently. A secret of at least 32 characters is now required. +- **Replaced bcryptjs with PBKDF2 (WebCrypto).** bcrypt cost-10 burns ~74ms of CPU; the Workers Free plan allows 10ms per request, so login and registration hard-failed on any fork deployed there. Legacy bcrypt hashes still verify and are re-hashed on next login. See `worker/lib/password.ts` for the iteration count and the trade-off behind it. +- **Password changes now revoke existing sessions** via `users.token_version`. Previously a session stayed valid for its full 30 days after the password changed. +- **Closed a user-enumeration timing leak on login** — an unknown email skipped hashing and answered measurably faster than a real account. +- **Rate limited the credential endpoints** (10/min/IP). +- **Rejected `javascript:` and `data:` URLs** on `job.url` and `contact.linkedin`, both of which are rendered into an ``. `contact.linkedin` had no URL validation at all. +- **Added a CSP** and security headers, on both static assets and `/api/*` responses, and self-hosted the font so the app makes no third-party request. +- **Registration no longer creates an orphan account when `JWT_SECRET` is broken.** It hashed and INSERTed the user, then threw while signing the session — leaving a row behind that turned the retry into a `409` for an account nobody could log into. Session config is now validated before any handler writes. +- **`PBKDF2_ITERATIONS` is configurable**, so a Paid deployment can run OWASP's 600k. Existing hashes upgrade on next login rather than needing a reset. + +### Fixed + +- Registration races return `409` instead of `500`. +- `/api/stats` excludes archived jobs from *every* stat; previously only the funnel filtered them, so archiving a job removed it from the funnel while it still dragged on the response rate. +- Changing status from the job drawer now reorders correctly — it went through `PATCH /jobs/:id`, which never updated `sort_order`, so the card landed at an arbitrary slot in its new column. +- Validation errors name the field that actually failed instead of always saying "Company and title are required". + +### Changed + +- The API is split into explicit public and protected routers, so a route's protection follows from where it is mounted rather than which line it sits on. +- CI now runs the build. + +> **Upgrading:** run `npm run db:migrate:remote` (adds `users.token_version`) and make sure `JWT_SECRET` is at least 32 characters before deploying. + ## [0.1.0] — 2026-07-14 ### Added diff --git a/README.md b/README.md index 3150509..1d90df7 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ npm run dev # → http://localhost:5173 ## Tech stack -React 19 · TypeScript · Vite · Tailwind CSS v4 · dnd-kit · Hono · Cloudflare Workers · D1 (SQLite) · bcryptjs · jose · Zod +React 19 · TypeScript · Vite · Tailwind CSS v4 · dnd-kit · Hono · Cloudflare Workers · D1 (SQLite) · WebCrypto PBKDF2 · jose · Zod ## License diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6dad0fe..db0ee1f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -86,9 +86,16 @@ Request → Worker (fetch handler) ### Authentication -1. **Registration/Login** — validates with Zod, hashes password (bcryptjs, cost 10), signs JWT (HS256, jose), sets httpOnly cookie `tracker_auth` -2. **Verification** — `requireAuth` middleware reads cookie, verifies JWT, sets `c.set("userId", userId)` -3. **Logout** — clears the cookie server-side +1. **Registration/Login** — validates with Zod, hashes the password (PBKDF2-HMAC-SHA256 via WebCrypto), signs a JWT (HS256, jose), sets the httpOnly cookie `tracker_auth` +2. **Verification** — `requireAuth` reads the cookie, verifies the JWT, checks the token version against the database, then sets `c.set("userId", userId)` +3. **Logout** — clears the auth cookie in the response. It does **not** revoke the JWT: the token stays valid until it expires, so logout protects the browser it ran in, not a token that has already been stolen. Revocation is what `token_version` is for. +4. **Password change** — increments `users.token_version`, which invalidates every session issued before it + +Notes: + +- **Why PBKDF2 and not bcrypt.** `bcryptjs` is pure JS and costs ~74ms of CPU per hash. The Workers **Free** plan allows 10ms per request, so login hard-failed there. PBKDF2 runs natively. See `worker/lib/password.ts` for the iteration count and the strength/cost trade-off behind it — it is a deliberate compromise, not a default. +- **Legacy hashes.** Accounts created under bcrypt still log in and are transparently re-hashed to PBKDF2 on their next successful login. +- **Rate limiting.** The credential endpoints are throttled per-IP via Cloudflare's rate-limit binding. The binding is optional: without it the endpoints still work and log a warning. ### Database diff --git a/docs/CONFIG.md b/docs/CONFIG.md index b8f60cd..74b3c20 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -6,8 +6,31 @@ These go in `.dev.vars` for local development and are set via `wrangler secret p | Variable | Required | Description | |---|---|---| -| `JWT_SECRET` | Yes | Used to sign auth cookies. Must be at least 32 characters. | +| `JWT_SECRET` | Yes | Signs the auth cookie. Must be at least 32 characters — enforced at runtime, not merely advised. Generate with `openssl rand -base64 32`. | | `ALLOW_REGISTRATION` | No | Set to `"true"` to allow new account signups. Defaults to `"false"`. | +| `PBKDF2_ITERATIONS` | No | Password-hash work factor. Defaults to `50000`, chosen to fit the Workers **Free** plan's 10ms CPU cap. Values below the default are ignored. | + +### Hardening the password hash on the Paid plan + +The default of 50,000 iterations is a deliberate compromise: the Workers Free plan allows 10ms of CPU per request, and no password hash is both OWASP-grade and that cheap — being slow is the entire point of one. The default keeps a free-tier fork deployable, at roughly 12x below OWASP's guidance for PBKDF2-SHA256. + +On the **Paid** plan that cap doesn't apply (30s default), so raise it: + +```bash +npx wrangler secret put PBKDF2_ITERATIONS # e.g. 600000 (OWASP), ~72ms per login +``` + +Existing accounts are not invalidated. Each hash records the iteration count it was created with, verification reads that value, and an account is re-hashed at the current setting on its next successful login. + +> **If `JWT_SECRET` is missing or too short the app refuses to issue a session and returns a 500.** That is deliberate: an empty key still produces JWTs that verify, so a deploy that forgot the secret would silently accept forged sessions for any account. Failing loudly is the safe behaviour. + +## Bindings + +| Binding | Required | Description | +|---|---|---| +| `DB` | Yes | D1 database. | +| `ASSETS` | Yes | Static asset fetcher for the SPA. | +| `AUTH_RATE_LIMITER` | No | Rate-limit binding declared in `wrangler.jsonc` (10 attempts/min/IP on the credential endpoints). Without it those endpoints still work but are **not** throttled, and a warning is logged. | ### `ALLOW_REGISTRATION` pattern @@ -66,7 +89,7 @@ See `migrations/0001_init.sql` and `migrations/0002_salary_currency_period.sql` | Table | Purpose | |---|---| -| `users` | Email + bcrypt password hash | +| `users` | Email + PBKDF2 password hash + token_version | | `jobs` | Job applications with status, salary, metadata | | `contacts` | Per-job contact people | | `activities` | Per-job timeline entries | diff --git a/docs/SETUP.md b/docs/SETUP.md index bda159c..56790f9 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -92,7 +92,10 @@ npm run db:migrate:remote ```bash npx wrangler secret put JWT_SECRET -# Paste your secret value when prompted (min 32 chars recommended) +# Generate one with: openssl rand -base64 32 +# At least 32 characters — this is ENFORCED, not advisory. A missing or short +# secret makes the app refuse to issue sessions (it would otherwise sign them +# with an empty key, which anyone could forge). npx wrangler secret put ALLOW_REGISTRATION # Set to "true" initially, then "false" after creating your account diff --git a/index.html b/index.html index 002636b..4fd3d1e 100644 --- a/index.html +++ b/index.html @@ -5,12 +5,8 @@ Tracker — Job Applications - - - + +
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/public/_headers b/public/_headers new file mode 100644 index 0000000..c4d4feb --- /dev/null +++ b/public/_headers @@ -0,0 +1,15 @@ +/* + Content-Security-Policy: default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; connect-src 'self'; form-action 'self'; base-uri 'none'; object-src 'none'; frame-ancestors 'none' + X-Content-Type-Options: nosniff + Referrer-Policy: strict-origin-when-cross-origin + X-Frame-Options: DENY + Strict-Transport-Security: max-age=31536000; includeSubDomains + Permissions-Policy: geolocation=(), microphone=(), camera=(), interest-cohort=() + +# style-src needs 'unsafe-inline' because dnd-kit writes drag transforms to +# inline style attributes, which CSP treats as inline styles. script-src does +# NOT get it: that is the directive that matters here, and with it in place a +# stored javascript: URL has nothing to execute with. +# +# font-src 'self' is why the Space Grotesk woff2 is served from public/fonts +# rather than fonts.gstatic.com. diff --git a/public/fonts/space-grotesk-var.woff2 b/public/fonts/space-grotesk-var.woff2 new file mode 100644 index 0000000..7b0e76a Binary files /dev/null and b/public/fonts/space-grotesk-var.woff2 differ 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..1fb9db8 100644 --- a/src/components/JobDrawer.tsx +++ b/src/components/JobDrawer.tsx @@ -23,9 +23,11 @@ import { JOB_STATUSES, PERIODS, STATUS_LABELS, + safeExternalUrl, type Activity, type Contact, type Job, + type JobStatus, type Reminder, } from "../../shared/types"; import { Button } from "./ui/button"; @@ -44,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; } @@ -63,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 () => { @@ -93,7 +106,7 @@ export default function JobDrawer({ job, onClose, onChange, onDelete }: Props) {
- {job.url && ( + {safeExternalUrl(job.url) && ( )} - {ct.linkedin && ( - + {safeExternalUrl(ct.linkedin) && ( + LinkedIn diff --git a/src/index.css b/src/index.css index 7a70d2c..e58e486 100644 --- a/src/index.css +++ b/src/index.css @@ -1,5 +1,19 @@ @import "tailwindcss"; +/* Self-hosted so the app makes no third-party request on load: the Google Fonts + leaked every visitor's IP and User-Agent to Google, blocked the first + paint on a server we don't control, and forced font-src to allow an external + origin in the CSP. This is the latin subset of the variable face (300–700). + Note the design uses font-black (900), which this face does not contain — the + browser synthesises it, exactly as it did when Google served the font. */ +@font-face { + font-family: "Space Grotesk"; + font-style: normal; + font-weight: 300 700; + font-display: swap; + src: url("/fonts/space-grotesk-var.woff2") format("woff2"); +} + @theme { --font-sans: "Space Grotesk", ui-sans-serif, system-ui, -apple-system, sans-serif; --font-head: "Space Grotesk", ui-sans-serif, system-ui, -apple-system, sans-serif; diff --git a/src/pages/Board.tsx b/src/pages/Board.tsx index 738b0e4..3b2a368 100644 --- a/src/pages/Board.tsx +++ b/src/pages/Board.tsx @@ -135,7 +135,9 @@ export default function Board() { setOpenJobId(null)} - onChange={(j) => setJobs((js) => js.map((x) => (x.id === j.id ? j : x)))} + onChange={(updated) => + setJobs((js) => js.map((x) => updated.find((u) => u.id === x.id) ?? x)) + } onDelete={(id) => { setJobs((js) => js.filter((x) => x.id !== id)); setOpenJobId(null); diff --git a/src/pages/TableView.tsx b/src/pages/TableView.tsx index a5e16ad..8ea1bba 100644 --- a/src/pages/TableView.tsx +++ b/src/pages/TableView.tsx @@ -154,7 +154,7 @@ export default function TableView() { setOpenJobId(null)} - onChange={(j) => setJobs((js) => js.map((x) => (x.id === j.id ? j : x)))} + onChange={(updated) => setJobs((js) => js.map((x) => updated.find((u) => u.id === x.id) ?? x))} onDelete={(id) => { setJobs((js) => js.filter((x) => x.id !== id)); setOpenJobId(null); }} /> )} diff --git a/worker/api.test.ts b/worker/api.test.ts index b3f97ae..2530e68 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 { readFileSync, readdirSync } from "node:fs"; +import bcrypt from "bcryptjs"; import { getPlatformProxy } from "wrangler"; import app from "./index"; import type { Job, Activity, Stats } from "../shared/types"; @@ -8,16 +9,26 @@ 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" }; + // 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; - 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); @@ -60,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( @@ -70,6 +111,148 @@ 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. + for (const secret of [undefined, "too-short"]) { + const res = await app.request( + "/api/auth/register", + { + ...json({ email: "weak@test.dev", password: "password1" }), + headers: { "Content-Type": "application/json" }, + }, + { ...env, JWT_SECRET: secret } as Env + ); + expect(res.status).toBe(500); + + // And it must fail *before* writing. Registration used to hash, INSERT, + // and only then throw when signing the session, leaving an orphan account + // that made the retry a 409 for an account nobody could log into. The + // same email is reused deliberately: it is the second iteration that + // would expose a leftover row. + const row = await env.DB.prepare("SELECT id FROM users WHERE email = ?").bind("weak@test.dev").first(); + expect(row).toBeNull(); + } + }); + + 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("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("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("upgrades an existing hash when PBKDF2_ITERATIONS is raised", async () => { + const c = client(); + await c.fetch("/api/auth/register", json({ email: "iters@test.dev", password: "password1" })); + + const iterationsOf = async () => { + const row = await env.DB.prepare("SELECT password_hash FROM users WHERE email = ?") + .bind("iters@test.dev") + .first<{ password_hash: string }>(); + return Number(row!.password_hash.split("$")[2]); + }; + expect(await iterationsOf()).toBe(50_000); // free-tier-safe default + + // A Paid deployment raises the work factor; existing accounts move up on + // their next login rather than needing a password reset. + const stronger: Env = { ...env, PBKDF2_ITERATIONS: "120000" }; + const res = await app.request( + "/api/auth/login", + { ...json({ email: "iters@test.dev", password: "password1" }), headers: { "Content-Type": "application/json" } }, + stronger + ); + expect(res.status).toBe(200); + expect(await iterationsOf()).toBe(120_000); + + // The re-hashed password still authenticates, and a wrong one still doesn't. + const again = await app.request( + "/api/auth/login", + { ...json({ email: "iters@test.dev", password: "password1" }), headers: { "Content-Type": "application/json" } }, + stronger + ); + expect(again.status).toBe(200); + const wrong = await app.request( + "/api/auth/login", + { ...json({ email: "iters@test.dev", password: "wrongpass1" }), headers: { "Content-Type": "application/json" } }, + stronger + ); + expect(wrong.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" })); @@ -119,6 +302,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" }))) @@ -134,6 +340,21 @@ describe("auth", () => { }); }); +describe("route protection", () => { + it("leaves the auth routes public and every other API route behind requireAuth", async () => { + // Pins the router split: the public endpoints must still be reachable with + // no cookie, and everything else must 401 with no cookie. + const anon = client(); + expect((await anon.fetch("/api/auth/login", json({ email: "nobody@test.dev", password: "wrongpass1" }))).status) + .toBe(401); // reached the handler (bad creds), not blocked by requireAuth + expect((await anon.fetch("/api/auth/logout", { method: "POST" })).status).toBe(200); + + for (const path of ["/api/jobs", "/api/stats", "/api/reminders/upcoming"]) { + expect((await client().fetch(path)).status).toBe(401); + } + }); +}); + describe("jobs", () => { it("full lifecycle: create, move (logs activity, stamps applied_at), stats, delete", async () => { const c = client(); @@ -172,11 +393,101 @@ describe("jobs", () => { expect((await c.fetch(`/api/jobs/${job.id}`)).status).toBe(404); }); - it("validates input", async () => { + 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" })); + + // One live application that DID get an interview, and one archived one that + // was ghosted. The responded job has to be non-zero: with no responses at + // all, the rate is 0 whether or not archived jobs sit in the denominator, + // and the test would pass without the fix. + const live = (await ( + await c.fetch("/api/jobs", json({ company: "Replied", title: "Role", status: "applied" })) + ).json()) as Job; + await c.fetch(`/api/jobs/${live.id}/activities`, json({ type: "interview", title: "Phone screen" })); + + const ghosted = (await ( + await c.fetch("/api/jobs", json({ company: "Ghosted", title: "Role", status: "applied" })) + ).json()) as Job; + await c.fetch(`/api/jobs/${ghosted.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 out of the denominator too: 1 responded / 1 applied = 1.0. + // Before the fix the ghosted job still counted, giving 1/2 = 0.5. + expect(stats.responseRate).toBe(1); + expect(stats.weekly.reduce((n, w) => n + w.count, 0)).toBe(1); + }); + + it("move places the card at the requested slot and renumbers the column", async () => { + const c = client(); + await c.fetch("/api/auth/register", json({ email: "order@test.dev", password: "password1" })); + + // Three cards sitting in "applied". + const ids: string[] = []; + for (const company of ["A", "B", "C"]) { + const j = (await (await c.fetch("/api/jobs", json({ company, title: "R", status: "applied" }))).json()) as Job; + ids.push(j.id); + } + + // A wishlist card moved to the top of "applied" (index 0) — this is what + // the drawer's status dropdown now does instead of a bare PATCH, which + // would have left sort_order pointing into the column it came from. + const moving = (await (await c.fetch("/api/jobs", json({ company: "New", title: "R" }))).json()) as Job; + const rows = (await ( + await c.fetch(`/api/jobs/${moving.id}/move`, { + method: "PATCH", + body: JSON.stringify({ status: "applied", index: 0 }), + }) + ).json()) as Job[]; + + const applied = rows.sort((a, b) => a.sort_order - b.sort_order); + expect(applied[0].id).toBe(moving.id); + expect(applied.map((j) => j.sort_order)).toEqual([0, 1, 2, 3]); + expect(applied.every((j) => j.status === "applied")).toBe(true); + expect(applied).toHaveLength(4); + expect(ids.every((id) => applied.some((j) => j.id === id))).toBe(true); + }); + + it("validates input and names the field that actually failed", async () => { const c = client(); await c.fetch("/api/auth/register", json({ email: "val@test.dev", password: "password1" })); expect((await c.fetch("/api/jobs", json({ company: "" }))).status).toBe(400); expect((await c.fetch("/api/jobs", json({ company: "X", title: "Y", status: "nope" }))).status).toBe(400); + + // Every 400 used to read "Company and title are required", so a rejected + // URL blamed the wrong field entirely. + const badUrl = await c.fetch("/api/jobs", json({ company: "X", title: "Y", url: "javascript:alert(1)" })); + expect(badUrl.status).toBe(400); + expect(((await badUrl.json()) as { error: string }).error).toBe("Job URL must start with http:// or https://"); + + const noCompany = await c.fetch("/api/jobs", json({ company: "", title: "Y" })); + expect(((await noCompany.json()) as { error: string }).error).toBe("Company is required"); + }); + + 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 () => { diff --git a/worker/env.d.ts b/worker/env.d.ts index 44ae473..a0d93cc 100644 --- a/worker/env.d.ts +++ b/worker/env.d.ts @@ -1,6 +1,15 @@ -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; + // Raises the password-hash work factor above the free-tier-safe default. + // See lib/password.ts. + PBKDF2_ITERATIONS?: string; + // Optional: not provided by vitest's platform proxy. See lib/ratelimit.ts. + AUTH_RATE_LIMITER?: RateLimiter; + } } diff --git a/worker/index.ts b/worker/index.ts index 4af1382..c70a643 100644 --- a/worker/index.ts +++ b/worker/index.ts @@ -7,16 +7,44 @@ import { requireAuth, type AppEnv } from "./lib/auth"; const app = new Hono(); -const api = new Hono(); -api.route("/auth", auth); -api.use("*", requireAuth); -api.route("/jobs", jobs); -api.route("/contacts", contacts); -api.route("/activities", activities); -api.route("/reminders", reminders); -api.route("/stats", stats); +// public/_headers only decorates static asset responses — /api/* is generated +// here and inherits none of it. JSON has no script context, so the CSP is a +// lockdown rather than the page policy; nosniff is the one that actually +// matters, stopping a JSON body from being re-interpreted as something else. +app.use("/api/*", async (c, next) => { + await next(); + c.header("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'"); + c.header("X-Content-Type-Options", "nosniff"); + c.header("Referrer-Policy", "strict-origin-when-cross-origin"); + c.header("X-Frame-Options", "DENY"); +}); + +// Two routers, so which routes are public is decided by the router a route is +// mounted on rather than by where the line happens to sit. Previously /auth was +// public only because it was written above `api.use("*", requireAuth)`, and a +// route added one line too high would have been silently unauthenticated. +// +// Adding a route to `protectedApi` puts it behind requireAuth wherever the line +// goes; making something public takes a deliberate move to `publicApi`. +// +// One ordering dependency remains: publicApi must be mounted first, because +// protectedApi's "*" middleware also matches /api/auth/*, and only stops there +// because the public handler has already returned a response. Getting that +// wrong 401s every request including login — loudly broken rather than quietly +// open, which is the failure mode we want. The test below pins it. +const publicApi = new Hono(); +publicApi.route("/auth", auth); + +const protectedApi = new Hono(); +protectedApi.use("*", requireAuth); +protectedApi.route("/jobs", jobs); +protectedApi.route("/contacts", contacts); +protectedApi.route("/activities", activities); +protectedApi.route("/reminders", reminders); +protectedApi.route("/stats", stats); -app.route("/api", api); +app.route("/api", publicApi); +app.route("/api", protectedApi); app.notFound((c) => { if (new URL(c.req.url).pathname.startsWith("/api/")) { return c.json({ error: "Not found" }, 404); diff --git a/worker/lib/auth.ts b/worker/lib/auth.ts index e3b3135..4013fdd 100644 --- a/worker/lib/auth.ts +++ b/worker/lib/auth.ts @@ -7,12 +7,24 @@ 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); } -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`) @@ -31,19 +43,49 @@ 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, secretKey(c.env.JWT_SECRET)); - return typeof payload.sub === "string" ? payload.sub : null; + const { payload } = await jwtVerify(token, key); + 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; +} + +// Fails the request before any handler runs a side effect. Without this, +// register hashed the password and INSERTed the user, and only then threw on +// the bad secret when issuing the session — leaving an orphan account behind +// and answering the retry with a 409 for an account nobody can log into. +export async function requireSessionConfig(c: Context, next: Next) { + secretKey(c.env.JWT_SECRET); // throws → 500, before the handler touches the DB + await next(); } 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/lib/password.ts b/worker/lib/password.ts new file mode 100644 index 0000000..4a5fb63 --- /dev/null +++ b/worker/lib/password.ts @@ -0,0 +1,107 @@ +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. +export const DEFAULT_ITERATIONS = 50_000; +const SALT_BYTES = 16; +const KEY_BITS = 256; + +// Raise the work factor without touching code: set PBKDF2_ITERATIONS. Worth +// doing on the Workers Paid plan, where the 10ms cap doesn't apply and OWASP's +// 600_000 costs ~72ms — comfortably inside the 30s budget there. +// +// Existing hashes keep working: each one stores the iteration count it was made +// with, verification reads that value, and an account is re-hashed at the +// current setting on its next successful login. +export function configuredIterations(env: Env): number { + const n = Number(env.PBKDF2_ITERATIONS); + // Below the default is never an improvement, so treat it as unset. + return Number.isInteger(n) && n > DEFAULT_ITERATIONS ? n : DEFAULT_ITERATIONS; +} + +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, iterations = DEFAULT_ITERATIONS): 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"); +} + +// True for a bcrypt hash, or a PBKDF2 hash weaker than the current setting. +// Lets a raised PBKDF2_ITERATIONS roll out across accounts as they log in, +// rather than only applying to new ones. +export function needsRehash(stored: string, iterations: number): boolean { + if (isLegacyHash(stored)) return true; + const stored_iters = Number(stored.split("$")[2]); + return !Number.isInteger(stored_iters) || stored_iters < iterations; +} + +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/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 724ac07..090d8fd 100644 --- a/worker/routes/auth.ts +++ b/worker/routes/auth.ts @@ -1,7 +1,8 @@ import { Hono } from "hono"; import { z } from "zod"; -import bcrypt from "bcryptjs"; -import { createSession, clearSession, verifySession, type AppEnv } from "../lib/auth"; +import { createSession, clearSession, authenticate, requireSessionConfig, type AppEnv } from "../lib/auth"; +import { hashPassword, verifyPassword, needsRehash, configuredIterations, dummyVerify } from "../lib/password"; +import { rateLimitByIp } from "../lib/ratelimit"; const credentials = z.object({ email: z.string().email().max(254), @@ -15,6 +16,17 @@ const changePasswordBody = z.object({ const auth = new Hono(); +// Before anything else: a broken JWT_SECRET must stop the request before a +// handler can write to the database. +auth.use("*", requireSessionConfig); + +// 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); @@ -23,17 +35,24 @@ 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 bcrypt.hash(password, 10); - await c.env.DB.prepare("INSERT INTO users (id, email, password_hash) VALUES (?, ?, ?)") - .bind(id, email.toLowerCase(), hash) - .run(); - await createSession(c, id); + const hash = await hashPassword(password, configuredIterations(c.env)); + + // 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); }); @@ -42,13 +61,32 @@ 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 }>(); - const ok = user && (await bcrypt.compare(password, user.password_hash)); - if (!ok) return c.json({ error: "Invalid email or password" }, 401); + .first<{ id: string; email: string; password_hash: string; token_version: number }>(); + 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 stored hash on the way through: off bcrypt, or up to a raised + // PBKDF2_ITERATIONS. Costs one extra hash on the login that does it. + const iterations = configuredIterations(c.env); + if (needsRehash(user.password_hash, iterations)) { + await c.env.DB.prepare("UPDATE users SET password_hash = ? WHERE id = ?") + .bind(await hashPassword(password, iterations), user.id) + .run(); + } - await createSession(c, user.id); + await createSession(c, user.id, user.token_version); return c.json({ id: user.id, email: user.email }); }); @@ -58,7 +96,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(); @@ -67,7 +105,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)); @@ -77,17 +115,27 @@ 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); - await c.env.DB.prepare("UPDATE users SET password_hash = ? WHERE id = ?").bind(hash, userId).run(); + // 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, configuredIterations(c.env)); + 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) diff --git a/worker/routes/items.ts b/worker/routes/items.ts index 531becc..71c2e41 100644 --- a/worker/routes/items.ts +++ b/worker/routes/items.ts @@ -12,7 +12,7 @@ function itemRoutes(table: "contacts" | "activities" | "reminders", schema: z.Zo const parsed = (schema as z.ZodObject).partial().safeParse( await c.req.json().catch(() => null) ); - if (!parsed.success) return c.json({ error: "Invalid fields" }, 400); + if (!parsed.success) return c.json({ error: parsed.error.issues[0]?.message ?? "Invalid fields" }, 400); const data = parsed.data as Record; const cols = columns.filter((col) => data[col] !== undefined); if (!cols.length) return c.json({ error: "Nothing to update" }, 400); diff --git a/worker/routes/jobs.ts b/worker/routes/jobs.ts index f6f83ab..c031e91 100644 --- a/worker/routes/jobs.ts +++ b/worker/routes/jobs.ts @@ -6,15 +6,31 @@ 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 = (label: string, max: number) => + z + .string() + .trim() + .max(max) + .refine((u) => safeExternalUrl(u) !== null, `${label} must start with http:// or https://`); + +// Surfaces the field that actually failed. Every 400 used to say "Company and +// title are required", so pasting a bad URL reported the wrong problem. +const firstIssue = (error: z.ZodError) => error.issues[0]?.message ?? "Invalid fields"; + 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)), + company: z.string().trim().min(1, "Company is required").max(200), + title: z.string().trim().min(1, "Title is required").max(200), + url: httpUrl("Job URL", 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 +53,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("LinkedIn URL", 500) + .nullish() + .or(z.literal("").transform(() => null)), notes: z.string().max(5000).nullish(), }); @@ -112,7 +130,7 @@ jobs.get("/", async (c) => { jobs.post("/", async (c) => { const parsed = jobFields.safeParse(await c.req.json().catch(() => null)); - if (!parsed.success) return c.json({ error: "Company and title are required" }, 400); + if (!parsed.success) return c.json({ error: firstIssue(parsed.error) }, 400); const f = parsed.data; const id = crypto.randomUUID(); const status: JobStatus = f.status ?? "wishlist"; @@ -166,7 +184,7 @@ jobs.patch("/:id", async (c) => { .partial() .extend({ archived: z.union([z.literal(0), z.literal(1)]).optional() }) .safeParse(await c.req.json().catch(() => null)); - if (!parsed.success) return c.json({ error: "Invalid fields" }, 400); + if (!parsed.success) return c.json({ error: firstIssue(parsed.error) }, 400); const f = parsed.data; const merged = { ...job, ...Object.fromEntries(Object.entries(f).filter(([, v]) => v !== undefined)) }; @@ -273,7 +291,7 @@ function childRoutes(opts: { const job = await ownedJob(c, c.get("userId"), c.req.param("id")); if (!job) return c.json({ error: "Not found" }, 404); const parsed = opts.schema.safeParse(await c.req.json().catch(() => null)); - if (!parsed.success) return c.json({ error: "Invalid fields" }, 400); + if (!parsed.success) return c.json({ error: firstIssue(parsed.error) }, 400); const data = parsed.data as Record; const id = crypto.randomUUID(); const cols = opts.columns.filter((col) => data[col] !== undefined); 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 }>(), 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).