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