fix(security): hash password on user update; remove broken XSRF middleware

[conorluddy]

Summary

• Critical: updateUser service was storing passwords in plaintext when PATCH /users/:id received a password field. Now hashes via createHash() before the DB write, matching the pattern in createUser and resetPasswordWithToken.
• Critical: XSRF middleware was tautological — it read the token from cookies twice instead of checking the request header, and the cookie was httpOnly so JS couldn't read it anyway. Removed the entire XSRF system (middleware, utility, cookies set on login/refresh/magic-login/google). sameSite: strict on all auth cookies already provides CSRF protection.

Files removed

• src/middleware/auth/xsrfTokens.ts
• src/middleware/util/xsrfToken.ts
• src/middleware/auth/xsrfTokens.test.ts

Test plan

• npm run lint && npm test green
• Manual: PATCH /users/:id with a password field — verify bcrypt hash stored in DB ($2b$ prefix)
• Manual: mutation requests succeed without any XSRF cookie (middleware removed, sameSite: strict handles CSRF)

🤖 Generated with Claude Code

[claude]

Code Review — PR #466: hash password on user update; remove broken XSRF middleware

Overall this is a solid set of critical security fixes. A few observations below.

---

✅ What's good

updateUser.ts — password hashing fix

The fix is correct and the reordering is an improvement:

• Strength validation now runs before the hash is computed (avoids wasted bcrypt work on weak passwords)
• createHash() is used consistently with createUser and resetPasswordWithToken
• The conditional spread ...(passwordHash && { password: passwordHash }) is safe — null is falsy and won't clobber existing password

XSRF removal

The existing implementation was truly broken. In xsrfTokens.ts:

// Line 34 — reads from cookies
let xsrfToken: string = req.cookies['xsrfToken']

// Line 40 — reads from cookies AGAIN (should have been req.headers['xsrf-token'])
const requestHeadersXsrfToken: string = req.cookies?.['xsrfToken']

The "header check" was comparing the cookie to itself, providing zero CSRF protection. Since all auth cookies already use sameSite: strict, removal is the right call. The test, middleware, and utility files are all cleaned up.

---

🟡 Issues / Suggestions

1. AGENTS.md documentation is now stale (minor)

The Authentication Flow section still says:

3. XSRF protection for state-changing operations

This should be updated to reflect that CSRF is handled via sameSite: strict cookies, not a dedicated token, so future contributors aren't confused.

2. refreshToken.test.ts — cookie count change

The diff changes toHaveBeenCalledTimes(3) → toHaveBeenCalledTimes(2). PR #467 (which stacks on this) will change it to 1 again. Not a bug, just worth confirming the final count is correct after both PRs merge.

3. keys.ts — XSRF_TOKEN constant removed

Confirmed no remaining usages — this is clean.

---

🔴 No blockers

The critical password-hashing fix is correct and should be merged. The XSRF removal is well-reasoned and backed by test cleanup. Recommend updating AGENTS.md in a follow-up.

[claude]

Code Review — PR #466: hash password on user update; remove broken XSRF middleware

Summary

This is a solid security fix. Both changes address real vulnerabilities. Notes below are mostly minor observations and suggestions.

---

✅ updateUser.ts — Plaintext password fix

The fix is correct and follows the existing pattern in createUser. Two improvements over the original:

1. The password strength check now runs on the plaintext value before hashing — the old code checked updatedUserProperties.password which would have been a bcrypt hash if the plaintext happened to pass through, meaning the strength check was effectively meaningless.
2. The spread now uses the passwordHash result, so plaintext never touches the DB object.

One minor concern: if createHash() throws (e.g. bcrypt failure), the error will bubble up unhandled. This matches how createUser handles it, so consistency is fine — just worth being aware of.

---

✅ XSRF middleware removal

The bug in xsrfTokens.ts was genuine and severe:

// Line 34 — reads from COOKIE (not the header)
const requestHeadersXsrfToken: string = req.cookies?.['xsrfToken']

The comment even said "XSRF-Token should actually be checked in the headers" but the code read the same cookie twice. With httpOnly: true, the client JS could never read the cookie to set the header, making this CSRF protection completely non-functional — it was just checking that the cookie existed.

Removing it is the right call. sameSite: strict on HttpOnly cookies gives solid same-site CSRF protection for browser clients. Worth noting in project docs that cross-origin API clients (native apps, third-party integrations) should use another mechanism if that's ever a requirement.

---

Minor observations

• src/constants/keys.ts still exports RESIDENT_TOKEN after this PR. PR fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467 removes it — that's fine as a stacking concern, just noting the intermediate state isn't clean on its own.
• src/index.test.ts removes the CSRF token setup from the login test. The simplification is correct, but there's now no integration test that hits the login path with cookies at all. Consider whether a cookie-presence assertion in an integration test would add value.
• googleCallback.ts still has the TODO comment: // This is probably redundant as the id is in the jwt anyway... Revisit. PR fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467 removes this cookie — the comment can be dropped there.

---

Test coverage

Deleted xsrfTokens.test.ts correctly mirrors the deleted xsrfTokens.ts. The updateUser service lacks a dedicated test for the password-hashing path — a test asserting that a PATCH /users/:id with a password field stores a bcrypt hash (e.g. starts with $2b$) rather than plaintext would close this gap.

---

Verdict: Approve with the suggestion to add a test for the password-hashing behaviour in updateUser. The XSRF removal is correct and well-motivated.

🤖 Generated with Claude Code

[claude]

Code Review — PR #466: hash password on user update; remove broken XSRF middleware

Overall

Good security fixes. The changes are focused, well-motivated, and the cleanup is thorough. A few things worth flagging.

---

src/services/user/updateUser.ts ✅

The password-hashing fix is correct. Moving the strength check before createHash() (rather than after the object is built) is also an improvement — it avoids an async bcrypt call when the password would be rejected anyway.

One minor note: the method now silently ignores an explicit password: "" (empty string) because password ? await createHash(password) : null short-circuits on falsy. This matches the existing behaviour for other fields (username, firstName, etc.) which also use truthy-spread, so it is consistent — just worth being conscious of.

---

Removal of the XSRF system ✅ (with a caveat)

The original implementation was genuinely broken: requestHeadersXsrfToken was reading from req.cookies a second time instead of req.headers, meaning it always compared cookie→cookie rather than cookie→header. Removing it is the right call.

Caveat: The PR notes that sameSite: strict provides equivalent CSRF protection. That is true for standard browsers, but it does not hold for:

• Native mobile apps / non-browser API clients
• Older browser versions that do not enforce SameSite
• Server-side request scenarios

If this API will ever be consumed by non-browser clients, this assumption should be documented as a known limitation (or enforced via a different mechanism such as a required Origin/Referer check).

---

Test coverage

Removal of xsrfTokens.test.ts is correct since the file it tests is deleted. All references are cleaned up consistently across controllers, routes, and integration tests. ✅

---

Minor nits

• src/routes/auth/index.ts has a blank line removed before const router = Router() that looks unintentional — no functional impact.
• RESIDENT_TOKEN constant remains in keys.ts at this point in the stack; it is cleaned up in fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467. Fine for a stacked PR.

---

Summary

Area	Status
Password hashing bug fix	✅ Correct
XSRF removal rationale	✅ Sound (caveat on non-browser clients)
Code cleanup completeness	✅ Thorough
Test coverage	✅ Updated correctly

Verdict: Approve with the caveat that sameSite: strict assumptions are documented if non-browser clients are in scope.

🤖 Reviewed with Claude Code

[claude]

Code Review — PR #466: hash password on user update; remove broken XSRF middleware

Overall this is a solid set of critical fixes. A few observations below.

---

src/services/user/updateUser.ts ✅

Critical fix confirmed. The plaintext password storage was a severe vulnerability; the createHash() call before the DB write is the right pattern, consistent with createUser and resetPasswordWithToken.

The reordering is an improvement: password-strength validation now runs on the plaintext input before hashing rather than on the already-absent hash value. One minor note — if both an invalid email and a weak password are supplied, the caller now gets the email error first (email validation runs at line ~38, password at ~42). That order seems fine.

// ✅ Good: validate plain text, then hash
if (password && !isStrongPassword(password, PASSWORD_STRENGTH_CONFIG)) {
  throw new PasswordStrengthError()
}
const passwordHash = password ? await createHash(password) : null

---

XSRF middleware removal ✅

The bug in xsrfTokens.ts was real — requestHeadersXsrfToken was reading from req.cookies a second time instead of req.headers:

// Was (broken):
const requestHeadersXsrfToken: string = req.cookies?.['xsrfToken']

// Should have been:
const requestHeadersXsrfToken: string = req.headers?.['xsrf-token']

And since the cookie was httpOnly, JS couldn't ever write it to the request header — making the double-submit pattern impossible to fulfil. Removing the middleware is the right call.

Point worth documenting (README / CLAUDE.md / ADR): the CSRF defence model is now solely sameSite: strict on all auth cookies. That's valid for browser-first flows, but worth an explicit comment for anyone considering API / mobile clients that bypass sameSite semantics.

---

src/controllers/auth/logout.ts (in this PR)

The logout handler still reads userId from the RESIDENT_TOKEN cookie to call deleteRefreshTokensByUserId. Since #467 stacks on this and removes that dependency, this is a known transient state — just flagging it so reviewers are aware the full cleanup lands in the next PR.

---

Tests ✅

All test changes accurately mirror the production code removals. Deleting xsrfTokens.test.ts is correct — there is nothing left to test.

---

Env / config impact

EXPIRATION_XSRF_TOKEN is now removed from requiredEnvVars. Deployments that have it set will continue to work (extra env vars are harmless), but it would be worth a note in the changelog so teams can clean up their .env files.

---

Summary: Approve with the minor documentation suggestion above.

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.71%. Comparing base (e6c314b) to head (2a57134).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/services/user/updateUser.ts	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
+ Coverage   83.34%   83.71%   +0.37%     
==========================================
  Files         111      109       -2     
  Lines        1837     1781      -56     
  Branches      305      295      -10     
==========================================
- Hits         1531     1491      -40     
+ Misses        198      190       -8     
+ Partials      108      100       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code Review — PR #466

Overall: Approve with minor notes. Both fixes are correct and well-reasoned. The XSRF removal is particularly well-justified.

---

src/services/user/updateUser.ts — Password hashing fix ✅

The fix is correct and the refactored order of operations is an improvement:

1. Validate email
2. Validate password strength (fail fast, before the expensive bcrypt call)
3. Hash the password
4. Build updatedUserProperties
5. Check at least one property is present
6. Write to DB

One minor efficiency note: createHash(password) is called before the AT_LEAST_ONE_PROPERTY_REQUIRED guard. In practice this can't actually cause a wasted hash — if a valid password is passed, passwordHash will always be truthy and end up in updatedUserProperties. But the ordering might read as slightly surprising on first glance. No change needed; just worth being aware of if this function grows.

---

src/middleware/auth/xsrfTokens.ts / src/middleware/util/xsrfToken.ts — XSRF removal ✅

The removal is well-justified. The middleware had a clear implementation bug:

// Bug: reads from cookies TWICE — never checks the request header
let xsrfToken: string = req.cookies['xsrfToken']          // token source
const requestHeadersXsrfToken: string = req.cookies?.['xsrfToken']  // should be req.headers['x-csrf-token']

Since the cookie was httpOnly: true, JavaScript couldn't read it to forward it in a header, making the double-submit pattern impossible to implement correctly in the first place. sameSite: strict on all auth cookies does cover the same-origin CSRF threat model.

One thing worth documenting (even just a comment in src/routes/auth/index.ts or src/routes/users/index.ts) explaining why there's no CSRF middleware and that sameSite: strict is the chosen mitigation. Future contributors might wonder why it's absent.

---

Test coverage

Deleting xsrfTokens.test.ts and magicLoginWithToken XSRF assertions is correct — they tested the now-removed code. The remaining tests correctly reflect the new behaviour. ✅

---

EXPIRATION_XSRF_TOKEN env var

Removing it from requiredEnvVars means existing deployments with that env var set won't break (the var is simply ignored). Good, non-breaking cleanup. ✅

[claude]

Code Review - PR 466: hash password on user update; remove broken XSRF middleware

Overall this is a well-targeted critical fix.

Strengths

src/services/user/updateUser.ts

• The critical bug (plaintext password storage) is correctly fixed.
• Moving the isStrongPassword check to run before createHash is a nice efficiency win. No point spending bcrypt cycles on a password that will be rejected anyway.
• The passwordHash null guard cleanly handles the optional-password case.

XSRF removal

• The removal is well-justified. The original xsrfTokens.ts had a tautological check: it read from req.cookies['xsrfToken'] twice (lines 32 and 39 of the deleted file) instead of checking the request header as the comment described. The httpOnly flag made it impossible for client-side JS to read the cookie anyway, so the double-submit pattern was never functional.
• sameSite: strict on all auth cookies provides correct CSRF protection for browser-based flows. For non-browser API clients, CSRF is not applicable at all.
• The cleanup is thorough: config, constants, routes, tests, and env vars all updated consistently.

Notes

Empty-string password edge case (updateUser.ts): The password param is typed as string or null or undefined. An empty string is falsy so it will not be hashed and the spread skips it. Worth confirming that PATCH /users/:id with password set to empty string returns AT_LEAST_ONE_PROPERTY_REQUIRED (or that the field is stripped upstream). The safe outcome here is the existing password remains unchanged.

Skipped logout route test: describe.skip was already there before this PR, but with logout logic changing substantially across this stack it would be good to track un-skipping it as a follow-up.

EXPIRATION_XSRF_TOKEN env var: Removing from requiredEnvVars is correct. A brief release note so existing users know they can remove it from their .env would be helpful.

Verdict

The critical password-hashing fix is correct and the XSRF removal is well-reasoned. Approved with the minor notes above for awareness.

[claude]

Code Review: PR 466 - hash password on update; remove broken XSRF middleware

Critical: Password hashing fix

The fix is correct and the order of operations is right. Moving isStrongPassword validation before createHash() is the proper sequence - validate first, hash only on success. The spread pattern correctly skips the password field when none was provided. Edge case working as intended: an empty-string password skips both validation and hashing so the field is omitted - no silent plaintext write.

Critical: XSRF removal

Well-justified. The middleware had two compounding bugs: (1) requestHeadersXsrfToken read from req.cookies again instead of req.headers, making it a cookie-vs-cookie comparison rather than a double-submit check; (2) the XSRF cookie was httpOnly: true, so JavaScript could never read it to inject it into a request header - making a correct double-submit flow literally impossible. Relying on sameSite: strict is a valid standard alternative for CSRF defence. Worth a brief docs note that cookie-based auth only works correctly from same-site browser contexts.

Test hygiene

Test files removed alongside the implementation - no orphaned tests. src/index.test.ts cleaned up to remove the redundant CSRF header injection. The magicLoginWithToken and logout test expectations updated in sync with the implementation.

Minor nit

The old code ran the strong-password check after building updatedUserProperties. The new code validates before hashing - correct reorder.

---

Overall: both fixes are high-quality and well-reasoned. LGTM pending CI green.

[claude]

Code Review -- PR 466, 467, 468 (stacked)

Reviewed all three PRs together. Overall a solid batch of security fixes. A few things worth addressing before merge.

---

PR 466 -- hash password on update; remove XSRF middleware

Password hashing in updateUser

Fix is correct and the ordering is right: validate raw password strength first, then hash, then conditionally spread into the update object. Matches the pattern in createUser and resetPasswordWithToken.

Minor nit: using null as the no-password sentinel means the spread relies on truthiness of the hash. Switching to undefined and checking passwordHash !== undefined would be more explicit and would make silent no-ops impossible. Not a blocker.

XSRF middleware removal

The bug is real. The middleware read req.cookies['xsrfToken'] twice -- once to obtain the token, then again as the so-called header check -- comparing the cookie value to itself. Since the cookie was httpOnly, client JS could never have put it in a request header anyway. Tautological check correctly removed.

env.example needs updating

EXPIRATION_XSRF_TOKEN=1w is still present in .env.example. config.ts no longer reads or exports this variable so it is now dead configuration. Should be removed to avoid confusion when bootstrapping a new environment.

---

PR 467 -- rate limit register; remove RESIDENT_TOKEN cookie

Register rate limiting

Correct fix. Using rateLimitTenPerTenMins (10 req / 10 min) matches login, magic-login, and reset-password. Good consistency.

logout.ts -- error message accuracy

The error thrown when refreshTokenId is missing is MESSAGES.MISSING_USER_ID ('User ID is missing.'), but what is actually missing at that point is the refresh token, not the user ID. MESSAGES.REFRESH_TOKEN_REQUIRED would be more accurate for the first guard.

logout.ts -- test gap

The updated logout.test.ts mocks getToken to always return { userId: '123' }, but there is no test for the case where getToken returns null or returns a record with a falsy userId. Both branches now exist in production code and should be covered.

refreshToken.ts -- conditional delete is a net improvement

The old code deleted all tokens unconditionally using userId from the RESIDENT_TOKEN cookie. An attacker with a valid RESIDENT_TOKEN but a crafted refreshToken could have wiped all of a user's sessions. The new code derives userId from the DB record, preventing that. Good change.

Removed cross-check token.userId !== userId

This check was only meaningful when userId came from a separate client-supplied cookie. Since userId is now derived from the DB record the check was redundant and is correctly removed.

---

PR 468 -- explicit JWT algorithm; body size limit

JWT algorithm specification

Specifying algorithm HS256 on sign and algorithms ['HS256'] on verify closes the algorithm confusion attack surface including alg:none. Clean focused change.

Body size limit

express.json limit 10kb is a sensible default for an auth API. Routes that need larger payloads should use a route-scoped parser with a higher limit.

---

Summary

Issue	Severity	Status
Password plaintext storage in updateUser	Critical	Fixed
Broken XSRF middleware removed	Critical	Fixed
Register endpoint rate limiting	High	Fixed
RESIDENT_TOKEN cookie exposing internal DB ID	High	Fixed
JWT algorithm confusion prevention	Medium	Fixed
JSON body size limit	Medium	Fixed
.env.example still references XSRF var	Low	Needs cleanup
Logout error message accuracy	Low	Needs fix
Logout test gaps (null token and missing userId paths)	Low	Needs coverage

The critical and high items are handled well. The three low-severity items are small changes worth addressing before merge.

[claude]

Code Review

Summary

This is a solid security fix. Both changes are well-motivated and correctly implemented.

---

src/services/user/updateUser.ts — Password hashing

✅ Critical fix, correctly implemented.

The old code was passing password directly into the DB update object — a critical plaintext storage bug. The new approach is correct:

// Good: validate strength before doing expensive bcrypt work
if (password && !isStrongPassword(password, PASSWORD_STRENGTH_CONFIG)) {
  throw new PasswordStrengthError()
}
const passwordHash = password ? await createHash(password) : null

Reordering the strength check to happen before hashing is also a nice improvement — no point running bcrypt on a password that will be rejected anyway.

Minor nit: passwordHash is null when no password is provided. Prefer undefined here to be idiomatic TypeScript (and consistent with how other optional fields are handled in the spread). Both work fine with the falsy spread, but undefined is the convention for "not provided":

const passwordHash = password ? await createHash(password) : undefined

---

XSRF middleware removal

✅ Correct removal of broken security theatre.

The existing implementation had two fatal flaws that made it tautological:

1. Line 34 vs line 40 in xsrfTokens.ts: The variable named requestHeadersXsrfToken was actually reading from req.cookies (not req.headers), so it was comparing the cookie to itself — always passing.
2. httpOnly: true on the XSRF cookie: The JavaScript-inaccessible cookie meant clients could never extract it to send as a header in the first place.

The removal is justified. sameSite: strict on all auth cookies already prevents cross-site request forgery for modern browsers. This is widely accepted as equivalent CSRF protection for cookie-based auth flows.

Suggestion: Add a brief comment near the auth cookie declarations documenting that CSRF protection is provided by sameSite: strict, so future contributors don't try to re-add a CSRF token system:

// CSRF protection is handled by sameSite: 'strict' on all auth cookies.
// A separate CSRF token is not needed for this architecture.

---

Test coverage

Tests are properly updated — removed XSRF-related assertions, and the XSRF test files are cleanly deleted. The index.test.ts cleanup (removing the csrf cookie setup) is correct.

---

Overall

Approve. The password hashing fix is critical and correct. The XSRF removal eliminates broken code rather than leaving false security in place. The optional comment suggestion above is minor polish.

[claude]

Code Review: fix(security): hash password on user update; remove broken XSRF middleware

Summary

Both fixes in this PR are correct and well-motivated. The changes are clean and appropriately scoped.

---

Password Hashing Fix (updateUser.ts) ✅

The reordering is correct and improves the logic:

1. Validation before hashing — isStrongPassword now runs on the plaintext password, not on updatedUserProperties.password. The old code would have checked the plaintext anyway (it hadn't been hashed yet), but the new ordering is clearer and closer to the validation boundary.
2. The passwordHash && ... spread correctly avoids writing undefined to the DB when no password is provided.

One note on null vs undefined: The password parameter type is string | null. With const passwordHash = password ? await createHash(password) : undefined, a null password is treated the same as an absent one (no update), which is the right behaviour. Worth a comment if this is intentional (it is).

---

XSRF Removal ✅

The middleware was genuinely broken in two distinct ways:

1. It read from req.cookies['xsrfToken'] on line ~34 and then req.cookies?.['xsrfToken'] again on line ~41 — the header was never checked despite the comment saying it should be.
2. The cookie was set httpOnly: true, so JS clients couldn't read it to copy into a header even if they wanted to.

Removing it and relying on sameSite: strict is correct. This is standard CSRF defence for cookie-based auth without cross-origin requirements.

---

Minor Issues

Package version mismatch: package.json is bumped to 0.3.8 but package-lock.json only goes to 0.3.7. These should be in sync. Looks like a double-bump in package.json.

EXPIRATION_XSRF_TOKEN removal from requiredEnvVars: Good — this was blocking startup if the env var was absent. Existing deployments that already have this variable set will just have a harmless unused env var, so no migration concern.

---

Test Coverage ✅

Cleanup of XSRF-related test assertions is complete. The test file deletion is correct — keeping dead tests around is a maintenance burden.

[claude]

Code Review: PR 466 - hash password on update + remove broken XSRF middleware

Overall: Approve

Two solid security fixes. See notes below.

---

What is good

Password hashing in updateUser is the critical fix. Plaintext password storage on PATCH /users/:id was a serious vulnerability. The change correctly applies createHash() and mirrors the pattern in createUser and resetPasswordWithToken.

Validation-before-hashing order — moving isStrongPassword above the createHash call is correct: fail fast before the expensive bcrypt operation.

XSRF removal is well-justified. The middleware had a fundamental logical defect: it compared req.cookies['xsrfToken'] to itself (read twice from cookies) instead of comparing the cookie value to the request header. Because the cookie was httpOnly, client JS could never read and re-send it, making the double-submit pattern impossible. sameSite=strict on all auth cookies already provides CSRF protection for same-origin APIs, so removing this broken layer is the right call. The comment added to login.ts explaining the decision is a good addition.

---

Issues to fix before merging

1. .env.example still has EXPIRATION_XSRF_TOKEN=1w

This will confuse anyone bootstrapping the project after this merge. Please remove the line in this PR.

2. Dead message constants

MESSAGES.TOKEN_USER_INVALID and MESSAGES.REFRESH_TOKEN_COUNTERPART_REQUIRED remain in src/constants/messages.ts and will be unreferenced once PR 467 is merged. One of the two stacked PRs should clean them up.

---

Minor notes

• Version jumps from 0.3.6 to 0.3.8, skipping 0.3.7. Not a correctness issue but worth aligning with the other stacked PRs.
• XSRF_TOKEN is removed from constants/keys.ts here; RESIDENT_TOKEN is removed in the stacking PR 467. That split is fine given the explicit ordering in the PR descriptions.

[claude]

Code Review

Overall this is an excellent and necessary security fix. The critical issues are addressed cleanly.

What is well done

Password hashing fix (updateUser.ts) - The critical bug (plaintext password storage) is correctly resolved. The password strength validation is now done before hashing, which is the right order: fail fast on weak passwords before spending bcrypt cycles.

XSRF removal - The broken middleware was genuinely tautological: it read req.cookies['xsrfToken'] twice instead of checking the request header, and the cookie was httpOnly: true so client-side JS could never read it to set the header anyway. Documenting the sameSite: strict guarantee in the login comment is a good replacement callout.

Test cleanup - All XSRF-related assertions and imports are cleanly removed with no orphaned mocks or dead test code.

---

Bugs / Issues

.env.example still references EXPIRATION_XSRF_TOKEN (line 22)

This env var is no longer read by config.ts, but anyone setting up a fresh environment from .env.example will still see it. Should be removed to avoid confusion.

---

Minor notes

• RESIDENT_TOKEN constant remains in src/constants/keys.ts after this PR - correctly removed in the stacked fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467.
• src/controllers/auth/googleCallback.ts still imports RESIDENT_TOKEN after this PR (the cookie is retained here, cleaned up in fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467).

---

Test coverage

The existing XSRF test file is fully deleted rather than updated - appropriate since the middleware is gone. The updateUser password hashing fix would benefit from a unit test asserting the stored value starts with the bcrypt prefix ($2b$), but the happy-path flow is covered implicitly.

Verdict: Approve once .env.example is updated to remove EXPIRATION_XSRF_TOKEN. The core security fix is solid.

[claude]

Code Review — PR #466: hash password on user update; remove broken XSRF middleware

Overall this is a solid, well-targeted fix. Both changes address real bugs and are implemented cleanly.

---

✅ updateUser — password hashing (Critical fix)

The fix is correct and the refactor is an improvement. The reordering (validate → hash → spread) is the right sequence: fail fast on a weak password before doing the expensive bcrypt operation. This matches the pattern already used in createUser and resetPasswordWithToken.

Before: Password was spread into the update object in plaintext, with the strength check happening after construction — wasteful.

After:

• isStrongPassword throws early, before any hashing work
• createHash runs on the validated raw value
• Only the hash reaches the DB update

---

✅ Removal of the XSRF middleware

The original xsrfTokens.ts had two fatal bugs:

1. The cookie was set with httpOnly: true, so client JS could never read it to put it in a header — making the double-submit pattern impossible by design.
2. It read from req.cookies['xsrfToken'] twice (once for the stored token, and once where it should have been req.headers['xsrf-token']), so the comparison was always tautological.

sameSite: strict is the correct CSRF defence for a cookie-based API consumed by first-party clients. Removing the broken middleware (rather than patching it) is the right call.

---

Minor observations

• RESIDENT_TOKEN is still exported from keys.ts at this point — it gets removed in fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467, so merge order matters.
• The version in package.json jumps to 0.3.8 while package-lock.json says 0.3.7. Looks like a stacking artefact — worth verifying the final published version is consistent.
• The type-coverage count drops by ~186 nodes. Expected given the deleted files, but worth a sanity-check that no accidental any crept in elsewhere.

---

Test coverage

Tests updated correctly throughout — XSRF assertions removed from logout, magicLoginWithToken, refreshToken, and index tests. The config.test.ts cleanup is tidy.

Verdict: Approve — fixes are correct, minimal, and well-tested.

[claude]

Code Review: fix(security): hash password on user update; remove broken XSRF middleware

Good security fixes overall. Notes below.

---

Strengths

Password hashing fix (updateUser.ts)

The critical plaintext-password bug is correctly fixed. Moving the isStrongPassword check before createHash() is the right order — no point hashing a weak password only to reject it.

XSRF removal

The middleware was genuinely broken — it read from req.cookies['xsrfToken'] twice instead of checking req.headers['xsrf-token'], and because the cookie was httpOnly the client could not read it to forward it anyway. The comment added to login.ts clearly explains why sameSite: strict replaces it.

---

Issues

1. Test does not verify hashing actually happens (medium)

updateUser.test.ts already mocks ../../utils/crypt, but the "should update the database if everything is valid" test does not assert that createHash was called with the plaintext password. For a critical security fix, the test should confirm the hash is what reaches the DB — not just that db.update was called once.

Suggested addition in the passing test:

import { createHash } from '../../utils/crypt'
// inside the test:
expect(createHash).toHaveBeenCalledWith('StrongP@ssw0rd')

2. Version mismatch between package.json and package-lock.json (low)

package.json is bumped to 0.3.8 but package-lock.json lands on 0.3.7. These should match. Resolved in #468 once merged in order, but worth noting.

3. EXPIRATION_XSRF_TOKEN in .env / .env.example (low)

The env var is removed from config.ts. If it still appears in any .env.example or deployment docs, that is a stale entry worth cleaning up.

---

Notes

• sameSite: strict is solid CSRF protection for an API-only backend. The approach is well-reasoned and the comment in login.ts is a good explicit record of the decision.
• RESIDENT_TOKEN is intentionally still in keys.ts here since it is removed in the stacked fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467.

[claude]

Code Review

Strengths

Password hashing fix (updateUser.ts): Correct and well-executed. The reordering — validate strength, hash, then store — is cleaner than before. Previously the plaintext password was spread into updatedUserProperties and written directly to the DB; the strength check ran before the write but the hash step was entirely absent. Using the existing createHash() utility keeps this consistent with createUser and resetPasswordWithToken.

XSRF middleware removal: The original xsrfTokens.ts was definitively broken — requestHeadersXsrfToken read from req.cookies rather than req.headers, so it compared the cookie to itself and never validated an incoming CSRF header token. Removing it is the right call. sameSite: strict on HttpOnly cookies is a well-accepted CSRF mitigation for same-site browser clients, and AGENTS.md is correctly updated to reflect this. All test files are updated consistently.

Issues

Version mismatch — package.json vs package-lock.json: package.json is bumped to 0.3.8 but package-lock.json is bumped to 0.3.7. These must match. Run npm install (without adding packages) to sync the lock file, or use npm version patch which updates both atomically.

Non-blocking observations

• The 'This is probably redundant... Revisit' comment on RESIDENT_TOKEN cookie in login.ts is still present — addressed in follow-up fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie #467, no action needed here.
• Consider adding a targeted unit test for the updateUser password-hashing path (e.g. asserting the stored value is a bcrypt hash, not the raw input) to guard against regression of this exact bug.

Overall: solid critical security fix. The plaintext password storage bug was high severity. Approved pending the version bump fix.