Security hardening: forgeable sessions, free-tier login failure, and 9 other fixes

[Patel230]

Deep review of the full codebase, then one commit per fix. Test count goes from 10 to 20; typecheck, tests and build all pass, and the whole flow was smoke-tested against wrangler dev.

⚠️ Deploy order matters

Two changes here will take production down if shipped in the wrong order:

1. wrangler secret put JWT_SECRET — must be ≥32 chars (the current one may be shorter; the app now refuses to sign sessions below that floor)
2. npm run db:migrate:remote — adds users.token_version, which authenticate() reads on every request
3. …then npm run deploy

Migration before deploy, or every authenticated request 500s on the missing column.

The two that actually mattered

A missing JWT_SECRET let anyone forge any session. TextEncoder().encode(undefined) returns a zero-length key, and jose will happily sign and verify HS256 with it. A fork deployed without the secret set would have accepted a forged cookie for any user_id — no error, no crash, app looks fine. Now fails closed.

Login was broken on the Workers Free plan. bcryptjs is pure JS and costs ~74ms CPU; Free caps CPU at 10ms/request, so login and register hard-fail with a 1102 there. It only worked for us because our account is Paid — i.e. it was broken for exactly the students meant to fork it.

Worth stating plainly: switching to PBKDF2 does not magically fix this. No secure password hash fits in 10ms — being slow is the point of one (bcrypt-10: 74ms, PBKDF2 600k/OWASP: 72ms, PBKDF2 100k: 13ms). We deliberately chose free-tier deployability over hash strength and run 50k iterations (~6ms), ~12x below OWASP guidance. The trade-off is documented at the constant, and iterations are stored per-hash so the factor can be raised later without invalidating anything.

Everything else

Sessions survived a password change	token_version in the JWT, bumped on change, checked per request. The old test passed because it checked the old password failed, not the old session.
Login leaked which emails exist	Unknown email skipped hashing and answered ~6ms faster. Now verifies against a dummy hash.
No rate limiting on credentials	10/min/IP via CF's rate-limit binding (optional — absent binding warns, doesn't break).
javascript:/data: URLs reached an <a href>	zod's .url() only runs new URL(), which accepts both. contact.linkedin had no URL validation at all. Guarded on write and render.
Registration race → 500	Check-then-insert had a TOCTOU window. Constraint is now the source of truth → 409.
/api/stats disagreed with itself	Only the funnel filtered archived, so archiving a job removed it from the funnel while it still dragged on the response rate.
Drawer status change misordered cards	It used PATCH /jobs/:id, which never touches sort_order. Now routes through /move.
Auth boundary was positional	/auth was public only because it sat above api.use("*", requireAuth). Now split into explicit public/protected routers.
No CSP; font loaded from Google	Added CSP + headers; self-hosted the woff2 so there's no third-party request.
Misleading validation errors	Every 400 said "Company and title are required" — even for a bad URL.
CI never built	Added npm run build.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Security

  • Strengthened password protection, session invalidation, login timing safeguards, credential rate limiting, URL validation, and security headers.
  • Added enforcement for sufficiently strong session secrets.
  • Self-hosted fonts to reduce external dependencies.

• Bug Fixes

  • Corrected registration conflict responses, job ordering, statistics involving archived jobs, and validation error messages.
  • Improved job and contact link safety.

• Documentation

  • Updated setup, configuration, architecture, technology, and changelog documentation.

• Chores

  • CI now verifies production builds in addition to type checks and tests.

[coderabbitai]

Warning

Review limit reached

@Patel230, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fca6d70a-9fc3-4486-9dc5-660b04624628

📥 Commits

Reviewing files that changed from the base of the PR and between d717cba and 2e70622.

📒 Files selected for processing (9)

• CHANGELOG.md
• docs/ARCHITECTURE.md
• docs/CONFIG.md
• worker/api.test.ts
• worker/env.d.ts
• worker/index.ts
• worker/lib/auth.ts
• worker/lib/password.ts
• worker/routes/auth.ts

📝 Walkthrough

Walkthrough

Authentication is hardened with PBKDF2, token-version session invalidation, JWT secret checks, timing mitigation, and credential rate limiting. API validation, statistics, job movement, frontend links, security headers, font loading, CI builds, tests, and documentation are also updated.

Changes

Application hardening and consistency

Layer / File(s)	Summary
Authentication foundations migrations/..., worker/lib/..., worker/env.d.ts, wrangler.jsonc	Adds token-version persistence, PBKDF2 password utilities with legacy bcrypt verification, JWT secret enforcement, and optional IP-based rate limiting.
Authentication routes and protection worker/routes/auth.ts, worker/index.ts, worker/api.test.ts	Updates registration, login, password changes, session revocation, route protection, and related authentication tests.
API validation, job movement, and statistics shared/types.ts, worker/routes/items.ts, worker/routes/jobs.ts, worker/routes/stats.ts, worker/api.test.ts	Restricts external URLs to HTTP(S), reports specific validation issues, excludes archived jobs from statistics, and tests job movement and validation behavior.
Frontend job update flow src/components/JobDrawer.tsx, src/pages/Board.tsx, src/pages/TableView.tsx	Uses the move endpoint for status changes, propagates updated job collections, merges them into view state, and sanitizes rendered links.
Security delivery and documentation .github/workflows/ci.yml, public/_headers, index.html, src/index.css, README.md, docs/*, CHANGELOG.md	Adds build verification, security headers, self-hosted fonts, and documentation for the updated authentication and deployment behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuthRoutes
  participant Password
  participant Database
  Client->>AuthRoutes: Submit login credentials
  AuthRoutes->>Database: Load password hash and token version
  AuthRoutes->>Password: Verify password or dummy credentials
  Password-->>AuthRoutes: Return verification result
  AuthRoutes->>Database: Store upgraded hash when applicable
  AuthRoutes-->>Client: Set tracker_auth JWT cookie
  Client->>AuthRoutes: Send authenticated request
  AuthRoutes->>Database: Compare JWT tv with users.token_version
  Database-->>AuthRoutes: Return session validity
  AuthRoutes-->>Client: Return protected response or 401

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is clearly related to the PR’s main theme of security hardening and auth/session fixes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch harden/security-pass

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 8

🧹 Nitpick comments (1)

src/pages/Board.tsx (1)

138-140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use filter and spread to merge jobs instead of map to avoid dropping new records.

When the backend /move endpoint returns the renumbered destination column, it may contain jobs that were added by another client and don't yet exist in the local jobs state. The current js.map(...) logic only updates existing records and will silently drop these new jobs.

Align this with the robust merge pattern already used in onDragEnd:

• src/pages/Board.tsx#L138-L140: Replace the map logic with a filter and spread approach to append any newly discovered jobs.
• src/pages/TableView.tsx#L157-L157: Apply the same filter and spread merge logic here.

♻️ Proposed refactor for merging jobs

For both files, replace the onChange logic with the following:

-          onChange={(updated) =>
-            setJobs((js) => js.map((x) => updated.find((u) => u.id === x.id) ?? x))
-          }
+          onChange={(updated) => {
+            const ids = new Set(updated.map((u) => u.id));
+            setJobs((js) => [...js.filter((j) => !ids.has(j.id)), ...updated]);
+          }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/Board.tsx` around lines 138 - 140, Update the onChange merge logic
in src/pages/Board.tsx lines 138-140 and src/pages/TableView.tsx line 157 to use
the established filter-and-spread pattern from onDragEnd: update matching
existing jobs and append returned jobs whose IDs are not already present,
preserving newly discovered records from the backend.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/ARCHITECTURE.md`:
- Line 91: Update the “Logout” step in the architecture flow to state that it
clears the auth cookie in the response, rather than implying server-side session
deletion or JWT revocation. Keep the description concise and consistent with the
behavior of clearSession.

In `@public/_headers`:
- Around line 1-7: Apply the security headers defined in public/_headers to
Worker-generated /api/* responses by adding them in the Worker response path,
reusing the same policy values for CSP and related headers. Ensure all relevant
Worker responses receive the shared headers rather than limiting the policy to
static assets.

In `@worker/api.test.ts`:
- Around line 352-371: Update the “excludes archived jobs from every stat” test
to include an active job with a response, then assert the non-zero responseRate
expected when the archived job is excluded from the denominator. Add archived
interview/offer fixtures if needed to verify the remaining statistics also
exclude archived jobs.
- Around line 114-130: Update the registration flow exercised by the weak
JWT_SECRET test to validate session configuration before inserting the user,
ensuring invalid secrets return 500 without committing an account. Extend the
test around the registration request to verify no user row remains for each
weak-secret case, while preserving the existing 500 response assertion.

In `@worker/lib/password.ts`:
- Around line 13-25: The password hashing default in ITERATIONS is too weak for
production. Change the production default to the stated secure PBKDF2 guidance,
and make the 50,000-iteration free-tier compromise an explicit deployment opt-in
through the existing configuration mechanism; ensure hashing and verification
continue using the iteration count stored in each hash.
- Around line 77-84: Update dummyVerify and the login verification flow to use
one fixed verification budget across missing users, PBKDF2 hashes, and legacy $2
bcrypt hashes. Ensure bcrypt records do not perform an additional longer
bcrypt.compare path; route them through the same established dummy verification
timing while preserving the false result for unknown users.

In `@worker/routes/auth.ts`:
- Around line 76-80: Update both password update flows, including the
legacy-login upgrade near isLegacyHash and the password-change flow, to use
compare-and-set updates: add the verified hash snapshot as an additional
password_hash predicate and bind it accordingly. Check the update result for
zero affected rows and handle that as a concurrent modification rather than
reporting success.
- Around line 34-51: Validate the JWT signing configuration at the start of the
registration handler, before hashPassword and the users INSERT, and return the
existing configuration error response when invalid. Ensure createSession
receives only validated configuration so failed registrations do not persist an
account; keep the UNIQUE constraint handling unchanged.

---

Nitpick comments:
In `@src/pages/Board.tsx`:
- Around line 138-140: Update the onChange merge logic in src/pages/Board.tsx
lines 138-140 and src/pages/TableView.tsx line 157 to use the established
filter-and-spread pattern from onDragEnd: update matching existing jobs and
append returned jobs whose IDs are not already present, preserving newly
discovered records from the backend.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 94555490-b77f-4050-9d2b-f7851c857101

📥 Commits

Reviewing files that changed from the base of the PR and between 61f6ab6 and d717cba.

⛔ Files ignored due to path filters (1)

• public/fonts/space-grotesk-var.woff2 is excluded by !**/*.woff2

📒 Files selected for processing (25)

• .github/workflows/ci.yml
• CHANGELOG.md
• README.md
• docs/ARCHITECTURE.md
• docs/CONFIG.md
• docs/SETUP.md
• index.html
• migrations/0003_token_version.sql
• public/_headers
• shared/types.ts
• src/components/JobDrawer.tsx
• src/index.css
• src/pages/Board.tsx
• src/pages/TableView.tsx
• worker/api.test.ts
• worker/env.d.ts
• worker/index.ts
• worker/lib/auth.ts
• worker/lib/password.ts
• worker/lib/ratelimit.ts
• worker/routes/auth.ts
• worker/routes/items.ts
• worker/routes/jobs.ts
• worker/routes/stats.ts
• wrangler.jsonc

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Apply these security headers to Worker-generated responses too.

public/_headers only covers static asset responses; /api/* is handled by Worker code, so those responses won't inherit this CSP/header policy. Add the shared headers in the Worker response path as well, or document that this policy is static-assets only.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@public/_headers` around lines 1 - 7, Apply the security headers defined in
public/_headers to Worker-generated /api/* responses by adding them in the
Worker response path, reusing the same policy values for CSP and related
headers. Ensure all relevant Worker responses receive the shared headers rather
than limiting the policy to static assets.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Do not make the below-guidance work factor the production default.

At 50,000 iterations, a stolen password database is approximately 12× cheaper to attack than the stated guidance. Rate limiting cannot mitigate offline cracking. Use a secure default and make the free-tier compromise an explicit deployment opt-in.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@worker/lib/password.ts` around lines 13 - 25, The password hashing default in
ITERATIONS is too weak for production. Change the production default to the
stated secure PBKDF2 guidance, and make the 50,000-iteration free-tier
compromise an explicit deployment opt-in through the existing configuration
mechanism; ensure hashing and verification continue using the iteration count
stored in each hash.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- worker/lib/password.ts (outline) ---'
ast-grep outline worker/lib/password.ts --view expanded || true

echo
echo '--- worker/lib/password.ts lines 1-220 ---'
cat -n worker/lib/password.ts | sed -n '1,220p'

echo
echo '--- search for bcrypt/legacy password references ---'
rg -n --hidden -S 'bcrypt|legacy|dummyVerify|verifyPassword|hashPassword|PBKDF2|PBKDF2|pbkdf2' worker lib . || true

Repository: Patel230/tracker

Length of output: 14135

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

cat -n worker/lib/password.ts | sed -n '1,180p'

Repository: Patel230/tracker

Length of output: 4483

---

Equalize the bcrypt fallback path too. Missing users already hit the PBKDF2 dummy hash, but accounts that still have $2 hashes spend far longer in bcrypt.compare, so login timing can reveal which users have not been migrated. Use one fixed verification budget for missing, PBKDF2, and bcrypt records.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@worker/lib/password.ts` around lines 77 - 84, Update dummyVerify and the
login verification flow to use one fixed verification budget across missing
users, PBKDF2 hashes, and legacy $2 bcrypt hashes. Ensure bcrypt records do not
perform an additional longer bcrypt.compare path; route them through the same
established dummy verification timing while preserving the false result for
unknown users.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Make password writes conditional on the hash that was verified.

Both flows verify a snapshot and then update unconditionally. A legacy login can restore an old password over a concurrent password change, while two password-change requests can both succeed. Add AND password_hash = ? to each update and handle a missing returned row as a concurrent modification.

Proposed compare-and-set updates

-UPDATE users SET password_hash = ? WHERE id = ?
+UPDATE users SET password_hash = ? WHERE id = ? AND password_hash = ?

-UPDATE users SET password_hash = ?, token_version = token_version + 1 WHERE id = ? RETURNING token_version
+UPDATE users
+SET password_hash = ?, token_version = token_version + 1
+WHERE id = ? AND password_hash = ?
+RETURNING token_version

Also applies to: 109-127

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@worker/routes/auth.ts` around lines 76 - 80, Update both password update
flows, including the legacy-login upgrade near isLegacyHash and the
password-change flow, to use compare-and-set updates: add the verified hash
snapshot as an additional password_hash predicate and bind it accordingly. Check
the update result for zero affected rows and handle that as a concurrent
modification rather than reporting success.