fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

[conorluddy]

Summary

Follows #466 (already merged).

• High: POST /users/register was only protected by the loose global rate limiter (100 req/10 min). Now uses rateLimitTenPerTenMins (10/10 min), matching login, magic-login, and reset-password.
• High: RESIDENT_TOKEN cookie was exposing the user's internal DB ID on every auth response with no server-side benefit (the ID is already in the JWT). Removed from all auth flows. logout and refreshToken now resolve userId from the DB token record.

Review feedback addressed:

• Logout now degrades gracefully when refresh token is not in DB (expired/purged) — always returns 200, best-effort DB cleanup
• Fixed misleading MISSING_USER_ID → REFRESH_TOKEN_REQUIRED error message in logout
• Added explanatory comment in refreshToken on the intentional delete-before-validate ordering (token theft detection)
• Added test for logout when getToken returns null

Test plan

• npm run lint && npm test green
• Manual: hit POST /users/register 11 times from same IP in 10 min — verify 429 on 11th
• Manual: login response no longer sets a residentToken cookie
• Manual: logout succeeds even with an expired/revoked refresh token

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.78%. Comparing base (0e683e6) to head (7b1dc74).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #470      +/-   ##
==========================================
+ Coverage   83.71%   83.78%   +0.06%     
==========================================
  Files         109      109              
  Lines        1781     1776       -5     
  Branches      295      294       -1     
==========================================
- Hits         1491     1488       -3     
  Misses        190      190              
+ Partials      100       98       -2     

☔ 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 470 - rate limit register endpoint; remove RESIDENT_TOKEN cookie

This is the strongest of the three security PRs. The changes are correct, well-motivated, and the PR description explains the reasoning clearly.

Positives

• Removing RESIDENT_TOKEN is the right call. The cookie exposed internal DB IDs on every auth response with no server-side security benefit since the ID was already in the JWT. Removing it reduces attack surface and simplifies the auth surface.
• Rate limiting /register at 10/10 min matches the posture of login and magic-login. Registration is a prime target for account enumeration and credential-stuffing pre-work, so this is a necessary fix.
• Graceful logout degradation is the correct UX decision. The cookie is cleared before the DB lookup, so the user is effectively logged out regardless of DB state. Returning 200 on a missing token is right.
• The delete-before-validate comment in refreshToken.ts is excellent — it explains a non-obvious security decision that would otherwise look like a bug to a future reader.
• The new test for getToken returning null during logout is a good addition covering the graceful degradation path.

Issues

1. Two dead message constants in messages.ts

REFRESH_TOKEN_COUNTERPART_REQUIRED and TOKEN_USER_INVALID are defined in src/constants/messages.ts but are no longer referenced anywhere in the codebase after this PR. They should be removed to prevent dead code accumulating in the constants file.

2. Stale Swagger JSDoc on the refresh route

src/routes/auth/refreshToken.ts line 36 still documents a residentToken cookie in its swagger block with the description: HTTP-only cookie containing the user ID for cross checking with the token. This cookie no longer exists. The Swagger UI will advertise an auth mechanism that is not in use.

3. Logout: extra DB round-trip (noted, not blocking)

The old logout used userId from the RESIDENT_TOKEN cookie for a single deleteRefreshTokensByUserId call. The new logout calls getToken first, then deleteRefreshTokensByUserId — two DB calls. This is the correct trade-off since we should not trust a cookie for the userId, and logout is low-frequency so the cost is acceptable. Calling it out explicitly so it is a conscious decision rather than an accidental one.

4. Rate limiter is in-memory / single-instance only (pre-existing issue)

rateLimitTenPerTenMins uses the default in-memory store. The existing TODO comment in rateLimiter.ts acknowledges this. It is not a new issue introduced by this PR, but worth noting: in a distributed or multi-instance deployment the per-IP counter does not share state across nodes, making the limit bypassable at scale.

Minor

• The MISSING_USER_ID to REFRESH_TOKEN_REQUIRED message rename is a more accurate description of what is actually missing. Good fix.
• The renamed test description (Throws an error if the refresh token cookie is absent) is clearer than the old (Throws an error if missing the user data).
• Removed tests for userId cookie not found and token user does not match JWT user are correctly removed since they tested behaviour that no longer exists.

---

Summary: Two action items before merge: (1) remove the two dead message constants REFRESH_TOKEN_COUNTERPART_REQUIRED and TOKEN_USER_INVALID from src/constants/messages.ts, and (2) update the Swagger JSDoc on src/routes/auth/refreshToken.ts to remove the stale residentToken cookie documentation. Everything else is correct and well-implemented.

[claude]

Code Review: fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

Overall this is a solid, well-scoped security hardening PR. The changes are correct and the rationale is clear. A few items worth discussing:

---

Strengths

• Removing RESIDENT_TOKEN — the right call. Leaking an internal DB ID to every client with no server-side benefit was unnecessary exposure.
• JWT algorithm pinning (HS256 in both sign and verify) — closes the alg: none vulnerability properly. The integration test for this is a great addition.
• express.json({ limit: '10kb' }) — good defense-in-depth, and the accompanying 413 integration test covers it cleanly.
• Graceful logout degradation — handling the expired/purged token case with a best-effort DB cleanup and always returning 200 is the right UX/security trade-off.
• Test coverage — the new logout null-token test and updated refreshToken tests reflect the changed behaviour accurately.

---

Issues

1. Version number skips 0.3.9
package.json jumps from 0.3.8 → 0.3.10, skipping 0.3.9 (which PR #469 uses). These two PRs will conflict on the version bump when one merges before the other.

2. Delete-before-validate DoS surface in refreshToken.ts
The comment does a good job explaining the intent, but the full attack surface is worth calling out explicitly:

Any client holding a currently-valid (non-used, non-expired) refresh token for a victim can call the refresh endpoint and wipe all of that victim's active sessions — before the caller even proves they can complete a valid refresh.

The attacker ends up with a new valid token; the victim loses all other sessions. If an attacker already holds a valid refresh token, they've already achieved session access, so this may be an acceptable trade-off for theft detection. But it's worth documenting this as a known consequence, not just the "replayed expired token" case mentioned in the comment.

3. Orphaned MESSAGES constants
After removing the cookie-based userId check, these two messages in messages.ts are no longer referenced anywhere:

• REFRESH_TOKEN_COUNTERPART_REQUIRED (line 121)
• TOKEN_USER_INVALID (line 136)

Minor cleanup, but dead constants in a messages file tend to stick around indefinitely.

4. In-memory rate limiter won't survive horizontal scaling
rateLimitTenPerTenMins uses the default in-memory store. If the app ever runs behind a load balancer with multiple processes, each process has independent counters, making the effective limit 10 × N. The // store: ... // Redis, Memcached comment in rateLimiter.ts already calls this out — just flagging it's now relevant for the registration endpoint too since that's the attack surface being hardened here.

5. Stale comment in rateLimiter.ts:12
// Limit each IP to 100 requests per window (here, per 15 minutes).

The window is TIMESPAN.MINUTE * 10. Pre-existing, but since this PR already touches auth rate limiting it's a good time to fix.

6. Integration test requires a live DB
tests/integration/security.test.ts calls postgresDatabaseClient.connect() and postgresDatabaseClient.query('BEGIN') in beforeAll. If CI doesn't have a running postgres instance at test time, this suite will error out at setup rather than skip gracefully. Are integration tests in a separate test script / CI job from unit tests in this project?

---

Summary

The core security changes (removing RESIDENT_TOKEN, algorithm pinning, body size limit, tighter rate limiting on registration) are all correct and well-implemented. The main actionable items before merge are: resolve the version conflict with PR #469, and consider clarifying the delete-before-validate trade-off comment. Everything else is minor.

🤖 Generated with Claude Code

[claude]

PR Review: fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

Overall this is a solid, well-motivated security hardening PR. The changes are cohesive, the comments explain why not what, and the new integration tests are a welcome addition. A few things worth addressing before merge:

---

Must Fix

package.json / package-lock.json version mismatch
package.json is bumped to 0.3.10 but package-lock.json only reflects 0.3.8. They should be in sync. Run npm install (no extra args) to regenerate the lock file after the version bump.

Dead code in messages.ts
After removing the RESIDENT_TOKEN-vs-JWT cross-validation from refreshToken.ts, the following constants in src/constants/messages.ts have no remaining consumers:

• REFRESH_TOKEN_COUNTERPART_REQUIRED
• TOKEN_USER_INVALID

They should be removed from messages.ts to avoid misleading future readers (and to keep the linter/type-coverage baseline honest).

---

Worth Discussing

refreshToken.ts: delete-before-validate trade-off
The comment explains the intent well, but the consequence is broader than it might look:

// Deletes ALL sessions BEFORE checking token.used
await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
if (token.used) { throw new ForbiddenError(MESSAGES.TOKEN_USED) }

If an attacker obtains any refresh token ID for a user (even one already marked used), presenting it forces a full session wipe for that user before the used check fires — effectively a targeted logout DoS. Token IDs are CUID2s so brute force is infeasible, but consider: does the system ever expose token IDs in logs, URLs, or error responses? If not, the risk is low and the theft-detection benefit is worth it. Just worth making explicit in a comment that token IDs must remain confidential.

Skipped route test
src/routes/auth/logout.test.ts is updated in this PR but the wrapping describe.skip means it never runs. If there's a known blocker for enabling it, it's worth a // TODO: comment; otherwise the test provides false assurance.

Integration test cleanup strategy
The beforeAll/afterAll in tests/integration/security.test.ts wraps a single raw postgresDatabaseClient connection in BEGIN/ROLLBACK. The two current tests don't mutate data (the 401 test reads; the 413 test is rejected before the DB), so it's harmless today. But if future tests in this file create rows via the app (which uses a separate Drizzle/pool connection), the rollback won't clean them up. Worth a comment flagging this.

---

Positives

• Removing RESIDENT_TOKEN — clean elimination of a cookie that exposed internal IDs with no server-side benefit; userId is now sourced authoritatively from the DB token record.
• JWT algorithm pinning (HS256 on both sign and verify) — correctly closes the alg:none confusion attack vector.
• Rate limiting /register at 10/10 min — consistent with login, magic-login, and reset-password; this was an obvious gap.
• express.json({ limit: '10kb' }) — simple, effective defence against payload bloat.
• Graceful logout degradation — always returning 200 when the token is already gone is the right UX pattern; session is cleared from the client side regardless.
• Security integration tests — the alg:none and 413 tests directly validate the hardening in the same PR. Good discipline.
• Comment quality — the refreshToken.ts comment on delete-before-validate ordering is exactly the kind of "why" documentation the codebase calls for in CODESTYLE.md.

---

Summary: Two blocking items (lock file sync + dead messages). One design conversation worth having (delete-before-validate scope). Everything else is clean. Happy to approve once those are addressed.

[claude]

PR Review: fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

Note: previous review comments from earlier sessions exist on this PR. This is a consolidated review superseding those.

This is a well-scoped, well-motivated security hardening PR. The changes are correct and the PR description explains the rationale clearly. Three items need addressing before merge.

---

Must Fix

1. package.json / package-lock.json version out of sync
package.json is at 0.3.13 but package-lock.json still reflects 0.3.8. Run npm install (no extra args) to regenerate the lock file after the version bump, then commit the updated lock file.

2. Dead constants in src/constants/messages.ts
After removing the RESIDENT_TOKEN-vs-JWT cross-validation from refreshToken.ts, these two constants have no remaining consumers:

• REFRESH_TOKEN_COUNTERPART_REQUIRED (line 121)
• TOKEN_USER_INVALID (line 136)

Remove them to avoid misleading future readers and to keep the constants file honest.

3. Stale Swagger JSDoc on the refresh route
src/routes/auth/refreshToken.ts:36 still documents a residentToken cookie. This cookie no longer exists. The Swagger UI will advertise an auth mechanism that is not in use.

---

Worth Discussing (not blocking)

refreshToken.ts: delete-before-validate scope
The comment explains the intent for expired/replayed tokens well. One edge case worth adding: if an attacker obtains a currently-valid refresh token ID for a victim and calls the refresh endpoint, deleteRefreshTokensByUserId fires before the used check — wiping all of that user's active sessions. Since the attacker already has a valid token, session access is already compromised, so this is an acceptable trade-off for theft detection. Token IDs are CUID2s so enumeration is infeasible, but an extra sentence making this explicit would help future readers.

Skipped route test in src/routes/auth/logout.test.ts
The test is updated in this PR but the describe.skip wrapper means it never runs. If there is a known blocker, a // TODO: comment would clarify intent.

---

Positives

• Removing RESIDENT_TOKEN — clean elimination of a cookie that exposed internal DB IDs with no server-side security benefit.
• JWT algorithm pinning (HS256) on both sign and verify correctly closes the alg:none confusion attack.
• Rate limiting /register at 10/10 min — closes an obvious gap; now consistent with login, magic-login, and reset-password.
• express.json({ limit: '10kb' }) — simple, effective defence against payload bloat with a clean 413 handler.
• Graceful logout degradation — clearing the cookie before the DB lookup and always returning 200 on a missing token is the right UX/security trade-off.
• refreshToken.ts delete-before-validate comment — exactly the kind of "why not what" documentation that prevents a future reader from treating it as a bug.
• New integration tests — the alg:none and 413 tests directly validate the hardening introduced in the same PR.

---

Summary: Three action items before merge: sync the lock file, remove two dead message constants, update the stale Swagger JSDoc. Core security changes are correct and well-implemented.

🤖 Generated with Claude Code

[claude]

PR Review: fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

Overall this is a solid, well-scoped security hardening PR. The changes are coherent, the intent is clearly documented, and test coverage is updated throughout. A few items worth addressing before merge:

---

Bugs / Issues

1. type HttpError placed between import statements (errorHandler.ts:4)

The type declaration is sandwiched between two import blocks. TypeScript hoists type declarations so this compiles, but it violates import ordering conventions and will likely confuse linters. The type should be placed after all imports.

---

2. Hardcoded string in errorHandler.ts (inconsistent with codebase style)

Every other branch in this handler uses MESSAGES.XXX. A MESSAGES.PAYLOAD_TOO_LARGE constant should be added to keep this consistent and avoid magic strings scattered across the codebase.

---

3. package.json / package-lock.json version mismatch

• package.json: "version": "0.3.14"
• package-lock.json: "version": "0.3.8"

These should match. If the version was bumped in package.json, running npm install (or at minimum npm version) is needed to sync the lock file.

---

Security Notes (informational, not blocking)

4. refreshToken.ts — delete-before-validate ordering trade-off

The comment explains the theft-detection rationale well. One trade-off worth being explicit about: a stolen (but expired or already-used) token can wipe all of the legitimate owner's sessions without the attacker gaining access. Depending on threat model this is fine, but worth a conscious sign-off that the UX cost (forced re-auth for all devices) is preferred over the alternative.

5. logout.ts — extra DB round-trip per logout

Previously logout used the RESIDENT_TOKEN cookie for a DB-free userId lookup. Now it fetches the token record to extract userId. This is the right call for correctness and security, just worth noting for performance-sensitive deployments.

---

Test Coverage

6. Integration test transaction isolation (tests/integration/security.test.ts)

The test wraps in BEGIN/ROLLBACK using postgresDatabaseClient, but the app likely uses its own connection pool internally. The ROLLBACK in afterAll would only roll back queries made through this specific client — data written by the app during the test may not be rolled back. Worth verifying no residual test data leaks into a shared DB, or confirming tests run against an ephemeral DB.

7. Skipped route test TODO

The describe.skip with a // TODO comment in src/routes/auth/logout.test.ts is acknowledged test debt. Worth tracking in an issue so it does not get forgotten.

---

Positives

• JWT algorithm pinning (HS256 on both sign and verify) is a meaningful fix against the alg:none attack, and the integration test for it is a great addition.
• Removing RESIDENT_TOKEN is the right call — exposing an internal DB ID in a cookie with no server-side verification value was pure surface area with no benefit.
• Rate limiting POST /users/register was a genuine gap — good catch.
• The express.json({ limit: '10kb' }) body-size cap is low-cost, high-value hardening.
• Graceful logout degradation (token not in DB → still 200) is well-reasoned and well-tested.
• The theft-detection comment in refreshToken.ts is clear and the ordering is correct.

---

Summary: Two mechanical fixes needed (type placement, MESSAGES constant for the 413 handler), plus a lock file sync. Everything else is informational. Nice work on a clean, security-focused PR.

[claude]

Code Review -- PR 470: rate limit register; remove RESIDENT_TOKEN cookie

Overall this is a solid set of security improvements. The RESIDENT_TOKEN removal and JWT algorithm pinning are both correct and meaningful fixes. A few items worth addressing before merge.

---

Positives

• Removing RESIDENT_TOKEN is the right call. Trusting a user-supplied cookie for userId instead of the DB-backed token record was a latent privilege-escalation risk.
• algorithms: ['HS256'] in jwt.verify closes the alg:none confusion attack. The matching integration test makes this verifiable.
• express.json({ limit: '10kb' }) plus the 413 error handler and integration test is a clean, complete change unit.
• rateLimitTenPerTenMins on /register correctly matches the convention already applied to login, magic-login, and reset-password.
• Graceful logout degradation (always 200, best-effort DB cleanup) is good UX and the new test covers the null-token path.
• REFRESH_TOKEN_REQUIRED rename in logout is more accurate than the previous MISSING_USER_ID.

---

Issues / Concerns

1. refreshToken.ts -- session wipe fires before used-token check

The comment explains this is intentional theft detection: a replayed token triggers a full session wipe. That is a reasonable design, but a legitimate client that suffers a network timeout during refresh (server processed it, client never got the response and retries) will also get all sessions wiped and be forced to re-authenticate. This is worth a follow-up issue so the product impact is tracked, even if the behaviour stays as-is.

2. errorHandler.ts -- type declaration placed between imports

HttpError is declared between two import statements. Move it after all imports -- this is valid TypeScript but non-standard and can confuse tooling/linters.

3. errorHandler.ts -- unused type field on HttpError

type?: string is declared on HttpError but never referenced in the handler body. Remove it or use it.

4. src/routes/auth/logout.test.ts -- lingering describe.skip

The suite is now skipped and has a fresh TODO comment. Either fix it or delete the file -- skipped tests quietly rot and give false confidence in coverage.

5. tests/integration/security.test.ts -- 413 test missing body assertion

The test only asserts status 413. Consider also asserting the response body shape (e.g. { message: 'Payload too large.' }) to verify the error handler returns the right payload, not just the right status code.

6. package.json / package-lock.json version mismatch

package.json ends at 0.3.14 but package-lock.json ends at 0.3.8. These should match. Running npm install after bumping package.json will sync the lock file.

---

Relationship with PR #471

PR #471 (fix/remove-legacy-babel-v6) appears to share the same auth-file changes as this PR. Both target main from separate branches, so merging one will create conflicts in the other. Coordinate the merge order or rebase #471 on top of this branch once it is merged.

---

Summary

Area	Status
Security -- RESIDENT_TOKEN removal	Correct
Security -- JWT alg pinning	Correct
Security -- rate limit + body limit	Correct
Logout graceful degradation	Good
refreshToken session-wipe ordering	Intentional but needs documentation/tracking
errorHandler type placement	Minor style nit
Skipped test in logout route	Tech debt -- fix or delete
Package version mismatch	Fix before merge
Integration test body assertion	Optional improvement

Reviewed with Claude Code

[claude]

Code Review — PR #470 (follow-up, 2026-06-13)

This is a follow-up review noting what has been resolved since earlier review rounds and flagging remaining items.

---

Resolved since prior reviews ✓

• Dead constants REFRESH_TOKEN_COUNTERPART_REQUIRED and TOKEN_USER_INVALID are removed from messages.ts in the current diff.
• Stale residentToken / xsrfToken Swagger JSDoc removed from src/routes/auth/refreshToken.ts.
• Graceful logout degradation added and tested.
• MISSING_USER_ID → REFRESH_TOKEN_REQUIRED rename in logout is now accurate.
• delete-before-validate comment in refreshToken.ts explains the theft-detection intent.

---

Remaining items before merge

1. Branch is out of date with main (blocks clean merge)

main is now at 0.3.11. The PR has package.json at 0.3.14 and package-lock.json at 0.3.8 — both different from main and from each other. A rebase is needed before this can merge cleanly. After rebasing, run npm install (no extra args) to sync the lock file, then confirm package.json and package-lock.json share the same version string.

2. HttpError type declaration placed between import statements (src/middleware/util/errorHandler.ts:4)

import { NextFunction, Request, Response, ErrorRequestHandler } from 'express'
import { HTTP_CLIENT_ERROR, HTTP_SERVER_ERROR } from '../../constants/http'

type HttpError = Error & { status?: number; type?: string }  // ← between imports
import { logger } from '../../utils/logger'

TypeScript hoists type declarations so this compiles, but it breaks conventional import grouping and can confuse linters. Move type HttpError after all import statements.

3. type?: string field on HttpError is declared but never read

The err.type field is accessed nowhere in the handler body. Remove it or use it.

4. Skipped route test (src/routes/auth/logout.test.ts)

describe.skip remains with a // TODO comment calling out the wrong HTTP method and stale expected response. A skipped test that flags its own incorrectness provides no safety net and gives false coverage confidence. Either fix it or delete the file — the unit-level logout.test.ts already covers the controller behaviour.

---

Informational (not blocking)

refreshToken.ts network-timeout edge case: The delete-before-validate approach is correctly documented for the stolen-token scenario, but the same wipe fires if a legitimate client retries a refresh after a network timeout (server processed the request, client never received the response). Worth a follow-up issue to track the UX cost — forced re-auth across all devices on retry — even if the behaviour stays as-is.

Integration test isolation: tests/integration/security.test.ts wraps in BEGIN/ROLLBACK via postgresDatabaseClient, but the app uses its own connection pool. The rollback won't clean up rows written through the app's pool. The two current tests don't write data so this is harmless today; worth a comment flagging it before new data-mutating tests are added to the file.

---

Summary

Item	Status
Rebase + lock file sync	Required
HttpError placement	Minor fix
Unused type? field	Minor fix
Skipped logout route test	Fix or delete
Core security changes	Correct ✓
Test coverage for new paths	Adequate ✓

Core security work is solid. Needs a rebase on main and the three minor fixes before merge.

🤖 Generated with Claude Code

[claude]

Code Review: PR 470 - fix(security): rate limit register; remove RESIDENT_TOKEN cookie

Overview: A solid security hardening PR. Each decision is well-reasoned and clearly explained in the PR body.

---

Strengths

• Rate-limiting /register at 10/10 min matches login/magic-login/reset-password, closing a gap that allowed enumeration/brute-force at 10x the login rate.
• Removing RESIDENT_TOKEN eliminates an unnecessary cookie exposing the user's internal DB ID — it was already in the JWT and provided no server-side value.
• Pinning algorithm to HS256 in both jwt.sign and jwt.verify closes the alg:none / algorithm-confusion attack vector. The new integration test for this is excellent.
• express.json({ limit: '10kb' }) plus the 413 handler is a lightweight and correct DoS mitigation.
• Graceful logout degradation (always 200, best-effort DB cleanup) is the right UX: the cookie is cleared regardless, so the client is logged out.
• The delete-before-validate comment in refreshToken.ts correctly explains the token theft detection mechanism.

---

Issues and suggestions

1. Version jump in package.json (medium)
package.json jumps from 0.3.9 to 0.3.14 while package-lock.json moves from 0.3.7 to 0.3.8. These do not agree, and a 5-version jump for a single PR is surprising. This matches the husky hook bug noted in PR 501 (hook bumps package.json but does not stage the lockfile). Both files should agree on a single-increment version before merging.

2. HttpError type declared between import statements (style)
In src/middleware/util/errorHandler.ts the type HttpError declaration appears between two import statements. By convention, all imports should come first. A one-line move resolves this.

3. Skipped route-level logout test (low)
src/routes/auth/logout.test.ts is in a describe.skip block with a TODO about a wrong HTTP method and a stale expected response. The logout route currently has no route-level test coverage. Worth fixing here or in a fast follow.

4. Integration tests require a live database
tests/integration/security.test.ts calls postgresDatabaseClient.connect() and needs a running Postgres instance. The BEGIN/ROLLBACK isolation is correct, but CI must provision a Postgres service or these tests will silently pass without actually connecting. Please confirm the CI workflow covers this.

5. Removed TOKEN_USER_INVALID cross-check (worth a comment)
The old refreshToken validated that token.userId matched the RESIDENT_TOKEN cookie userId. That check is removed here because RESIDENT_TOKEN is gone — and the new flow is actually more secure (userId comes entirely from the DB token, zero client input trusted). A one-line comment in refreshToken.ts noting this would help future reviewers who notice the deleted test.

6. Unused field on HttpError (minor)
type HttpError includes type?: string but that field is never read in the handler. Remove it or use it.

---

Verdict: The security direction is correct and the implementation is sound. The main items before merging are the version mismatch in package.json/package-lock.json and confirming CI provisions a Postgres service for the integration suite. Everything else is minor polish.

[claude]

Code Review: fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

Overview

Substantial security hardening PR. Covers four distinct concerns: register rate limiting, RESIDENT_TOKEN cookie removal, JWT algorithm pinning, and request body size limiting. The PR description is clear and the review-feedback section shows this has already gone through at least one iteration.

---

Code Quality & Best Practices ✅

• RESIDENT_TOKEN removal is correct. Exposing the internal DB user ID in a cookie with no server-side benefit is unnecessary information leakage. Deriving userId from the DB token record in logout and refreshToken is strictly better — it removes a client-controlled input from a security-sensitive lookup.
• logout.ts graceful degradation is a good UX improvement. Clearing the cookie first and doing best-effort DB cleanup is the right contract — the user is effectively logged out client-side regardless of DB state.
• refreshToken.ts delete-before-validate ordering is now documented with a comment explaining the theft-detection intent. The comment is helpful but note the security implication below.
• JWT algorithm pinning (HS256 in both generateJwt and authenticateToken) closes the "alg: none" attack vector cleanly.
• 10kb body limit + 413 handler is a low-cost, high-value DoS mitigation.
• errorHandler.ts has a minor style issue: the HttpError type is declared between import groups rather than at the top of the type block. Minor nit.

---

Potential Bugs / Issues

1. refreshToken.ts: Delete-before-validate wipes sessions on any invalid token

The sequence is: fetch token → delete ALL user sessions → check used → check expiry → check type → issue new token.

If the token is used or expired, all the user's other sessions are wiped and no new token is issued — the user must log in again everywhere. The comment frames this as theft-detection, which is intentional, but it's worth confirming this is the desired behaviour for the expiry case too (not just the used case). A user with an expired refresh token on one device will silently log themselves out of all other devices.

2. tests/integration/security.test.ts: DB dependency for a test that doesn't need it

The 413 test does not touch the database, but beforeAll/afterAll call postgresDatabaseClient.connect() and end(). This means the entire suite fails in any environment without a running Postgres instance — including local unit test runs. Consider splitting the 413/401 tests into a unit/supertest suite that doesn't require a DB connection, or wrapping the DB lifecycle in a try/catch with a beforeAll skip guard.

3. src/routes/auth/logout.test.ts: Skipped test with TODO

// TODO: route test needs updating — wrong method (GET→POST) and stale expected response
describe.skip('GET /logout', () => {

Skipping with a TODO is fine as tracked debt, but it means the route has no integration-level test coverage. Worth filing as a follow-up issue if not addressed in this PR.

4. package.json vs package-lock.json version mismatch

package.json shows 0.3.9 → 0.3.14, package-lock.json shows 0.3.7 → 0.3.8. These are inconsistent — this is the exact bug being fixed in PR #501. It won't cause a functional regression, but it's a sign the fix in #501 is needed before merging this.

---

Security Considerations

• Register rate limit (10/10 min): Consistent with login and reset-password. Good.
• Logout no longer trusts a client cookie for userId: Material improvement. Previously, a malicious client could supply an arbitrary RESIDENT_TOKEN to target another user's token deletion. Now the userId is looked up from the DB token record.
• refreshToken user-ID sourced from DB, not cookie: Same class of improvement — removes a client-controlled input from an auth-critical path.
• none algorithm test: The integration test correctly verifies this is rejected. The as any cast for 'none' is appropriate in a test context and the eslint-disable comment is already in place.

---

Test Coverage

• New logout graceful-degradation test: good addition.
• Removed tests for RESIDENT_TOKEN cookie assertions: correct cleanup.
• Removed TOKEN_USER_INVALID and REFRESH_TOKEN_COUNTERPART_REQUIRED error tests: appropriate, those paths no longer exist.
• The integration security test covers the two new hardening changes (413, alg:none). The DB dependency concern above should be addressed.

---

Summary

Verdict: Approve with minor notes.

The security improvements are real and well-executed. The main items worth a follow-up:

1. Confirm the intended behaviour when an expired (not stolen/replayed) refresh token triggers full session wipe.
2. Decouple the integration security tests from the Postgres dependency, or document that they are intentionally DB-integration tests.
3. Fix the skipped logout route test (or file a tracking issue).
4. Merge after PR fix(db): remove broken migrate/generate scripts in favor of push workflow #501 lands to resolve the lockfile version mismatch.

🤖 Reviewed with Claude Code

[claude]

Code Review — PR #470: fix(security): rate limit register endpoint; remove RESIDENT_TOKEN cookie

Overview

A solid, well-motivated security hardening PR. The changes address real attack surface: redundant cookie exposure, missing rate limit on a public mutation endpoint, and implicit JWT algorithm trust. The refactoring to derive userId from the DB token (rather than a cookie) is the right call — cookies are controllable by clients, DB records are not.

---

Security Fixes — Assessment

Rate limiting /register ✅
Correct fix. An unbounded public registration endpoint is a standard abuse vector for spam/enumeration. Matching it to rateLimitTenPerTenMins (consistent with login, magic-login, reset-password) is the right choice.

Removing RESIDENT_TOKEN cookie ✅
The cookie was exposing the internal DB ID on every auth response with no server-side enforcement value (ID was already in the JWT). Clean removal across all auth flows with tests updated accordingly.

Explicit algorithm: 'HS256' in JWT sign/verify ✅ (Critical)
This closes the alg: none bypass attack where a crafted token with "alg": "none" could skip signature verification in some libraries. The integration test in security.test.ts validates this is actually blocked. Well done.

JSON body size limit (express.json({ limit: '10kb' })) ✅
Good defensive measure against large payload DoS. Pairing it with a PAYLOAD_TOO_LARGE handler in errorHandler.ts is correct — without it, body-parser's 413 errors bypass the typed error pipeline.

refreshToken.ts — userId from DB token, not cookie ✅
Stronger security posture. The delete-before-validate ordering (theft detection via full session wipe on replayed tokens) is a legitimate pattern and the comment explaining the reasoning is helpful. Removing the now-obsolete cross-cookie user validation check (TOKEN_USER_INVALID) simplifies the code without sacrificing correctness.

logout.ts — graceful degradation when token not in DB ✅
Always returning 200 when the cookie is already cleared is correct UX — the user is logged out regardless of DB state. The test for the null-token path is a welcome addition.

---

Issues & Suggestions

1. errorHandler.ts — type definition placement

+type HttpError = Error & { status?: number; type?: string }
 import { logger } from '../../utils/logger'

The HttpError type is declared between import blocks, which is unconventional and will confuse linters/formatters. Move it above or below the import block.

2. Skipped test in src/routes/auth/logout.test.ts

// TODO: route test needs updating — wrong method (GET→POST) and stale expected response
describe.skip('GET /logout', () => {

The TODO is accurate but this test has been skipped for some time. If it's not being fixed in this PR, consider filing a follow-up issue rather than leaving dead code in the suite with a comment — skipped tests don't fail CI and tend to rot indefinitely.

3. refreshToken.ts — error messaging after full session wipe
When token.used is true (replay detected), the code throws TOKEN_USED after already deleting all sessions. This is intentional for theft detection, but the error is somewhat cryptic to a legitimate user who may have had their session invalidated after a prior attack. Consider logging the event (e.g. logger.warn('Possible token replay/theft detected for userId X')) so this is visible in ops monitoring, even if the client message stays generic.

4. Type coverage delta
The type-coverage numbers dropped slightly (6652 → 6599 total count, 98.55% → 98.53%). This is likely just churn from deleted code paths, not a regression, and stays well above the 90% floor. No action needed, just noting it for awareness.

---

Test Coverage

• Unit tests updated for logout, refreshToken, magicLoginWithToken — all cookie-removal changes are tested.
• New tests/integration/security.test.ts validates the alg: none bypass and the 10kb body limit — these are exactly the right integration tests for these changes.
• The graceful-degradation path in logout (null token from DB) is now covered.

---

Summary

The changes are correct, well-tested, and improve the security posture meaningfully. Two minor issues to address before merge:

1. Move the HttpError type declaration out of the middle of the import block in errorHandler.ts.
2. Decide on the skipped logout route test — fix it or track it in an issue and delete the dead test.

Everything else looks good to merge. 🔒