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

[conorluddy]

Summary

Stacks on #466 — merge that first.

• 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 instead of trusting a separate cookie.

Test plan

• npm run lint && npm test green
• Manual: hit POST /users/register 11 times from same IP in 10 min — verify 429 on the 11th
• Manual: login response no longer sets a residentToken cookie
• Manual: logout still works (session cleared via refresh token DB lookup)

🤖 Generated with Claude Code

[claude]

Code Review — PR #467: rate limit register endpoint; remove RESIDENT_TOKEN cookie

Good security improvements overall. A few things worth discussing.

---

✅ What's good

Rate limiting POST /register

Applying rateLimitTenPerTenMins is consistent with login, magic-login, and reset-password — a registration endpoint is a high-value brute-force/spam target and absolutely should have a tighter limit.

RESIDENT_TOKEN cookie removal

Exposing the raw DB user ID on every auth response was unnecessary — it's already embedded in the JWT. Removing it reduces the attack surface. The change to derive userId from the DB token record in logout and refreshToken is the right direction (trust the server-side record, not a client-supplied cookie).

---

🟡 Issues / Suggestions

1. logout.ts — misleading error message

const token = await SERVICES.getToken({ tokenId: refreshTokenId })
if (!token?.userId) {
  throw new BadRequestError(MESSAGES.MISSING_USER_ID)
}

MISSING_USER_ID is technically wrong here — the real problem is that the refresh token wasn't found in the DB (expired, already deleted, etc.). Using MESSAGES.TOKEN_NOT_FOUND or a dedicated REFRESH_TOKEN_INVALID message would be clearer and more helpful for debugging.

2. logout.ts — can't log out after token expiry

If a user's refresh token has already expired and been cleaned from the DB, getToken returns null and logout throws BadRequestError. The cookie will persist until its maxAge elapses. This is a regression from the old behaviour (which logged out unconditionally). Consider degrading gracefully — clear the cookie and return success even if the DB lookup fails:

// Always clear the cookie
res.cookie(REFRESH_TOKEN, '', { httpOnly: true, ..., expires: new Date(0) })

// Best-effort DB cleanup
const token = await SERVICES.getToken({ tokenId: refreshTokenId })
if (token?.userId) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}

handleSuccessResponse({ res, message: MESSAGES.LOGOUT_SUCCESS })

3. refreshToken.ts — delete-before-validate ordering preserved

The "destroy on detection" pattern (delete all tokens before checking validity) is carried forward, which is intentional for detecting token theft. Just flagging it explicitly: a replayed or malformed refresh token will wipe all sessions for that user. This is a known trade-off — worth a code comment if not already present.

4. logout.test.ts — missing failure path test

The happy path is covered via the mocked getToken. There's no test for the case where getToken returns null (token not found), which is now a new error path in the implementation.

5. Removed security tests — appropriate

The removed tests ('theres no UserId in the cookies' and 'the token user does not match the JWT user') mapped to checks that relied on RESIDENT_TOKEN. Their removal is correct; the underlying security property (trust the DB record) is preserved.

---

🔴 Blocker

Issue #2 (logout regression) is worth addressing before merge — a user who tries to log out after their refresh token has expired will get an error instead of a clean logout. At minimum this should fail silently rather than 400.

[claude]

Code Review — PR #467: rate limit register; remove RESIDENT_TOKEN cookie

Summary

Good follow-up to #466. Both changes are well-motivated. A few things worth considering.

---

✅ Rate-limit on /users/register

Straightforward and correct. Applying rateLimitTenPerTenMins at the route level matches the pattern used for /auth/login, /auth/magic-login, and /auth/reset-password. No issues here.

---

✅ RESIDENT_TOKEN cookie removal

Removing a cookie that exposed the internal DB user ID with no server-side enforcement was the right call — the ID was already inside the signed JWT. The structural change of resolving userId from the DB token record (instead of trusting a client cookie) is a meaningful security improvement.

---

Observations for logout.ts

// Cookie is cleared in the response first...
res.cookie(REFRESH_TOKEN, '', { ..., expires: new Date(0) })

// ...then the request cookie is read
const refreshTokenId = req.cookies?.[REFRESH_TOKEN]
if (!refreshTokenId) {
  throw new BadRequestError(MESSAGES.MISSING_USER_ID)
}

The ordering is safe (req.cookies reads the request, res.cookie sets the *response`), but:

1. Misleading error message: MESSAGES.MISSING_USER_ID is thrown when the refresh token cookie is absent. The actual missing thing is the refresh token, not a user ID. MESSAGES.REFRESH_TOKEN_REQUIRED would be more accurate.
2. Partial failure: If SERVICES.getToken() returns null or token.userId is falsy after the cookie is already cleared in the response, the user is effectively logged out (cookie gone) but the DB tokens are not cleaned up. This is an acceptable security trade-off — a failed logout that leaves an orphaned DB token is better than a failed logout that keeps the cookie — but it's worth a comment in the code explaining the intent.

---

Removed test: 'the token user does not match the JWT user'

The removed TOKEN_USER_INVALID check was a cross-validation between the cookie-supplied userId and the DB token's userId. Since we no longer trust a cookie for userId, removing the check is correct — the JWT-derived identity is now fully driven by the DB record. Good.

---

refreshToken.ts — deletion ordering

const token = await SERVICES.getToken({ tokenId: refreshTokenId })

if (token) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}

if (!token) {
  throw new ForbiddenError(MESSAGES.TOKEN_NOT_FOUND)
}
if (token.used) {
  throw new ForbiddenError(MESSAGES.TOKEN_USED)  // all tokens already deleted
}

All refresh tokens for the user are deleted before checking whether the token is used or expired. This is intentional refresh-token rotation / breach detection behaviour (a reused token invalidates all sessions), but it's not obvious from the code. A brief comment explaining the deliberate ordering would help future readers.

---

Test coverage

• logout.test.ts does not cover the case where SERVICES.getToken returns null (should throw BadRequestError).
• logout.test.ts does not cover the case where SERVICES.getToken returns a record with no userId (same throw path).

Both are small additions that would give confidence the error branches work.

---

Verdict: Approve. The misleading error message in logout.ts and the missing test cases are worth addressing, but neither is blocking.

🤖 Generated with Claude Code

[claude]

Code Review — PR #467: rate limit register endpoint; remove RESIDENT_TOKEN cookie

Overall

The rate-limiting addition is straightforward and correct. The RESIDENT_TOKEN removal and the cascading refactor of logout and refreshToken to trust the DB over a cookie is a solid improvement. One logic concern and one UX/debugging issue worth raising.

---

src/routes/users/createUser.ts ✅

Correct — rateLimitTenPerTenMins (10/10 min) matches login, magic-login, and reset-password. Consistent.

---

src/controllers/auth/refreshToken.ts — Logic concern ⚠️

// Regardless of validity, clear tokens once we've fetched the record
if (token) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}

if (!token) { throw ... }
if (token.used) { throw ... }   // <-- all sessions already gone
if (token.expiresAt < new Date()) { throw ... }  // <-- all sessions already gone

All of a user's refresh tokens are deleted before validating whether the presented token is valid (not used, not expired). This means:

• A user with multiple active sessions who replays an old/used token gets all sessions invalidated.
• An attacker who obtains an expired token ID can trigger a full logout for that user at will.

The token-reuse detection intent is good, but the "nuke all sessions" scope covers paths that are more availability impact than security signal. A comment explaining the deliberate trade-off would help future maintainers from inadvertently narrowing the delete scope without understanding the security reasoning.

---

src/controllers/auth/logout.ts — Misleading error message ⚠️

const refreshTokenId = req.cookies?.[REFRESH_TOKEN]
if (!refreshTokenId) {
  throw new BadRequestError(MESSAGES.MISSING_USER_ID)  // wrong message
}

The missing thing here is a refresh token, not a user ID. MESSAGES.REFRESH_TOKEN_REQUIRED (or a dedicated message) would be more accurate and easier to debug.

---

src/controllers/auth/logout.ts — Cookie cleared before DB error ℹ️

The REFRESH_TOKEN cookie is cleared unconditionally before the DB lookup. This is intentional (the client loses the cookie regardless of outcome), but a comment would prevent a future maintainer from moving the clear to after the DB call as an apparent cleanup.

---

Removed test coverage

Tests for TOKEN_USER_INVALID and REFRESH_TOKEN_COUNTERPART_REQUIRED are correctly removed since those code paths no longer exist. ✅

The new getToken mock in logout.test.ts is correct. ✅

---

Summary

Area	Status
Rate limit on /register	✅ Correct
RESIDENT_TOKEN removal	✅ Good cleanup
refreshToken userId source	✅ DB is authoritative
Session wipe on invalid token	⚠️ Intentional but undocumented trade-off
Logout error message	⚠️ Misleading — should use REFRESH_TOKEN_REQUIRED
Test coverage	✅ Updated correctly

Verdict: Approve pending the error message fix in logout.ts.

🤖 Reviewed with Claude Code

[claude]

Code Review — PR #467: rate limit register endpoint; remove RESIDENT_TOKEN cookie

Good security improvements. Two items worth discussing.

---

src/routes/users/createUser.ts ✅

Using rateLimitTenPerTenMins on POST /users/register is the right call — it matches the posture on login, magic-login, and reset-password. Previously the endpoint only had the loose global limiter (100/10 min), making it an easy target for account enumeration.

---

src/controllers/auth/refreshToken.ts ✅

The cleanup here is good. Deriving userId from the DB token record rather than a cookie removes an attack surface, and the token.userId !== userId check was only defending against the cookie we just removed, so deleting it makes sense.

One observation on the token-deletion ordering:

// Tokens deleted before all validity checks run
if (token) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}
if (token.used) {
  throw new ForbiddenError(MESSAGES.TOKEN_USED)  // already deleted above
}

If an attacker repeatedly submits a used or expired refresh token belonging to a valid user, each request will nuke all of that user's sessions (DoS on sessions). The original code had the same behaviour, so this isn't a regression — but it's a known trade-off of the "delete on first touch" pattern. Worth a comment in the code noting this is intentional.

---

src/controllers/auth/logout.ts ⚠️

The new implementation requires a live DB record for the refresh token in order to resolve userId. If the token has already expired or been revoked (e.g. admin purged sessions), logout will throw a BadRequestError instead of completing cleanly:

const token = await SERVICES.getToken({ tokenId: refreshTokenId })
if (!token?.userId) {
  throw new BadRequestError(MESSAGES.MISSING_USER_ID)  // user gets a 400
}

A user who is already effectively logged out shouldn't receive an error when they explicitly ask to log out. Consider:

// Graceful degradation — clear cookie regardless, only skip DB call if token is missing
const token = await SERVICES.getToken({ tokenId: refreshTokenId }).catch(() => null)
if (token?.userId) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}
handleSuccessResponse({ res, message: MESSAGES.LOGOUT_SUCCESS })

This keeps cookies cleared and returns 200 even when the DB record is gone.

---

Tests

• Removing the RESIDENT_TOKEN cookie from test mocks is correct.
• The removed test cases ("theres no UserId in the cookies", "the token user does not match the JWT user") are correctly deleted since those code paths no longer exist.
• Consider adding a test case for logout when getToken returns null (the scenario described above).

---

Summary: Approve with the logout-graceful-degradation issue flagged for consideration before merge.

[claude]

Code Review — PR #467

Overall: Approve with one functional concern to discuss. The rate-limit fix and cookie cleanup are both straightforward improvements. One edge case in the new logout flow is worth examining.

---

src/routes/users/createUser.ts — Rate limit on /register ✅

Clean, correct fix. Applying rateLimitTenPerTenMins at the route level (rather than relying solely on the global limiter) matches the pattern already used on login, magic-login, and reset-password. No issues.

---

RESIDENT_TOKEN cookie removal ✅

The cookie was redundant (the user's ID is already in the JWT payload) and represented unnecessary surface area. The cleanup across all auth controllers is thorough and consistent. Test updates are correct.

---

src/controllers/auth/logout.ts — New flow: concern with error when token not in DB

The new implementation looks up userId from the DB token record rather than from a cookie, which is more trustworthy. However, there's a subtle behaviour change:

Before: Logout always succeeded (cookies were cleared and DB tokens deleted using the cookie value).

After: If the refresh token doesn't exist in the DB (already expired, already rotated, or the user has already logged out), the call to SERVICES.getToken() would return null, token?.userId would be falsy, and logout throws BadRequestError(MESSAGES.MISSING_USER_ID).

The client's refresh token cookie is cleared (the res.cookie(..., expires: new Date(0)) fires before the DB lookup), so the user is effectively logged out client-side. But the API returns a 400 rather than 200, which could break clients that check the response status to confirm logout.

Worth considering: should logout return 200 (idempotent success) when the token simply isn't in the DB? Something like:

const token = await SERVICES.getToken({ tokenId: refreshTokenId })
if (token?.userId) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}
// No error thrown — logout is idempotent
handleSuccessResponse({ res, message: MESSAGES.LOGOUT_SUCCESS })

---

src/controllers/auth/logout.ts — Misleading error message (minor)

When the refresh token cookie is absent, the code throws BadRequestError(MESSAGES.MISSING_USER_ID) ('User ID is missing.'). The actual missing thing here is the refresh token. MESSAGES.REFRESH_TOKEN_REQUIRED is a more accurate message and already exists in the constants. Low priority but easy to fix.

---

src/controllers/auth/refreshToken.ts — Removed cross-validation ✅

The removal of token?.userId !== userId (which compared DB token user against the RESIDENT_TOKEN cookie) is justified: the cookie was user-controlled and the DB is the authoritative source. Relying solely on the DB is more correct.

Conditional token deletion is also correct: deleting tokens only when a valid DB record exists (and thus a real userId is known) prevents accidentally mass-deleting tokens for a userId we don't actually know.

---

Test coverage

The test removals accurately reflect the removed code paths (no RESIDENT_TOKEN cookie, no user-mismatch check). The adjusted mock sequences correctly model the new call order. ✅

[claude]

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

This PR correctly addresses two distinct high-severity issues. Review below.

Strengths

Rate limiting on /register: Applying rateLimitTenPerTenMins to POST /users/register is the right call. The loose global limiter (100/10 min) was trivially bypassable for account enumeration and bulk registration attacks. Matching the same limit used by login, magic-login, and reset-password gives consistent protection.

RESIDENT_TOKEN removal: Eliminating this cookie removes a low-value information leak (the internal DB user ID) with no loss of functionality since the ID is already inside the JWT. The refactors to resolve userId from the DB token record in refreshToken.ts and logout.ts are the correct approach.

refreshToken.ts - userId is now DB-authoritative: Sourcing userId from the stored token record rather than from a client-supplied cookie is strictly more secure. The cross-validation check (token.userId !== userId) was only meaningful when userId came from a cookie; removing it is correct.

Token invalidation order: deleteRefreshTokensByUserId is now correctly guarded inside if (token), avoiding a null-dereference on the pre-existing path. Invalidating all tokens for a user when any refresh token is presented (valid or not) is intentional token-theft detection behavior and is preserved.

Issues / Notes

logout.ts - wrong error message for token-not-found case

When SERVICES.getToken returns null, the code throws BadRequestError(MESSAGES.MISSING_USER_ID). But the actual problem is that the refresh token ID was not found in the database - MESSAGES.TOKEN_NOT_FOUND would be more accurate and easier to debug. MISSING_USER_ID suggests a malformed request rather than an unknown token.

logout.ts - existing residentToken cookies not cleared

Clients that authenticated before this change will still carry the residentToken cookie until it expires. The logout handler no longer clears it. This is a minor issue since the cookie carries no server-side authority, but adding a cookie clear with expires: new Date(0) in the logout response would clean it up for existing sessions without any downside.

refreshToken.ts - deleteRefreshTokensByUserId before validity checks

If a refresh token is used or expired, all tokens for that user are still deleted before the error is thrown. This is intentional (token reuse = likely theft, so nuke all sessions) but it means a leaked or replayed old token can log out a legitimate user. This is pre-existing behavior that the PR does not worsen; just worth documenting the trade-off.

routes/auth/logout.test.ts - still skipped

The route-level logout test remains in describe.skip. Given the significant logic changes in this PR (DB lookup added, cookie dependency removed), it would be valuable to have this test running.

Verdict

Both fixes are correct and improve the security posture. The TOKEN_NOT_FOUND message issue in logout.ts is worth a quick fix. Everything else is minor.

[claude]

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

High: Register rate limiting

Applying rateLimitTenPerTenMins directly on the route is consistent with how login, magic-login, and reset-password are configured. Good.

High: RESIDENT_TOKEN removal

Removing the redundant user-ID cookie simplifies the auth cookie surface area. The ID is already carried in the JWT payload, so this was dead data being serialised on every auth response.

logout.ts - minor concern

The error thrown when the refresh token cookie is absent uses MESSAGES.MISSING_USER_ID. At that point no DB lookup has happened - the missing thing is actually the refresh token cookie, not a user ID. MESSAGES.REFRESH_TOKEN_REQUIRED (already defined) would be more accurate and easier to trace in logs.

Also consider adding a test for the new missing-refresh-token path:

it('throws if refresh token cookie is absent', async () => {
  delete mockRequest.cookies[REFRESH_TOKEN]
  await expect(logout(mockRequest, mockResponse)).rejects.toThrow(MESSAGES.REFRESH_TOKEN_REQUIRED)
})

refreshToken.ts - quiet but meaningful DoS improvement

The old code called deleteRefreshTokensByUserId unconditionally using the cookie value. An attacker who guessed a valid user ID could submit random refresh token IDs to force-logout arbitrary users. The new code only deletes tokens when the submitted ID actually exists in the DB, which closes that vector. Well spotted.

Removed test coverage

• 'theres no UserId in the cookies' test correctly removed - that code path no longer exists.
• 'the token user does not match the JWT user' test correctly removed - the cross-cookie check was replaced by trusting the DB record.

---

LGTM on the overall approach. One action item: update the error message on the missing-refresh-token path in logout.ts.

[claude]

Code Review

Summary

Both changes are good security improvements. One logic simplification is worth addressing before merging.

---

src/routes/users/createUser.ts — Rate limiting /register

✅ Correct. Applying rateLimitTenPerTenMins to /register closes a gap where account enumeration and credential stuffing attacks could target this endpoint at a much higher rate than login. This now matches the protection level on /auth, /magic-login, and /reset-password.

---

RESIDENT_TOKEN cookie removal

✅ Good cleanup. The RESIDENT_TOKEN cookie was exposing the user's internal DB primary key on every auth response — information that was already in the JWT and served no server-side purpose. Removing it reduces the attack surface and information leakage.

---

src/controllers/auth/refreshToken.ts — Logic simplification opportunity

⚠️ Minor issue: The if (token) / if (!token) double-check is logically redundant and slightly confusing to read:

// Current:
if (token) {
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
}
if (!token) {
  throw new ForbiddenError(MESSAGES.TOKEN_NOT_FOUND)
}

The early-return / guard-clause pattern (which CODESTYLE.md endorses) reads more clearly and removes the redundancy:

// Suggested:
if (!token) {
  throw new ForbiddenError(MESSAGES.TOKEN_NOT_FOUND)
}
await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })

This is also safer — the current code accesses token.userId inside if (token) but TypeScript can't narrow the type when the two checks are split into separate if blocks, depending on strictness settings.

---

src/controllers/auth/logout.ts — userId from DB record

✅ Solid improvement. Looking up userId from the refresh token DB record rather than trusting a client-supplied cookie is correct. This also eliminates the implicit trust relationship that existed between two cookies.

One note: SERVICES.getToken presumably queries by token ID regardless of token state (used/expired). For logout, that's fine — we want to be able to clear sessions even with stale tokens. Just worth confirming that getToken doesn't filter out expired records in a way that would break logout for expired sessions.

---

Removed security checks in refreshToken.ts

✅ Correct removals:

• TOKEN_USER_INVALID check removed — userId now comes from the DB record itself, so a mismatch is impossible
• REFRESH_TOKEN_COUNTERPART_REQUIRED check removed — the RESIDENT_TOKEN cookie no longer exists

The corresponding test cases for these are also correctly deleted.

---

Test coverage

Updated tests accurately reflect the new behaviour. The describe.skip('GET /logout', ...) in src/routes/auth/logout.test.ts is pre-existing and not introduced here, but it's worth tracking as a TODO to re-enable when feasible.

---

Overall

Approve with suggestion. The rate-limiting and cookie removal are clean improvements. Please consider applying the guard-clause simplification in refreshToken.ts before merging — it's a small change that improves readability and avoids the subtle double-check smell.

[claude]

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

Summary

Good security hardening overall. The RESIDENT_TOKEN removal is the right call. There is one subtle ordering issue in refreshToken.ts worth discussing.

---

Rate Limiting /register ✅

Correct placement — the limiter is applied per-route rather than relying solely on the global limiter. Consistent with /auth, /magic-login, and /reset-password.

---

RESIDENT_TOKEN Removal ✅

The cookie was exposing the internal DB user ID on every auth response with no security benefit (the ID is already in the JWT payload). Removing it is correct.

The logout refactor — resolving userId via getToken from the DB instead of trusting a separate cookie — is the right approach and removes a class of trust-boundary issues.

The graceful degradation test (Still succeeds if the refresh token is not found in the DB) is a good addition; it confirms the user is logged out at the cookie level even when the DB record is already gone.

---

refreshToken.ts — Session Nuke Ordering ⚠️

The comment acknowledges the trade-off, but it's worth spelling out the consequence more explicitly for reviewers:

// Current order:
const token = await SERVICES.getToken({ tokenId: refreshTokenId })
if (!token) throw new ForbiddenError(MESSAGES.TOKEN_NOT_FOUND)

await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })  // ← nukes ALL sessions

if (token.used) throw new ForbiddenError(MESSAGES.TOKEN_USED)

If an attacker obtains a stale or previously-used refresh token ID for a victim, they can submit it to log out all of the victim's active sessions. The attacker gains nothing (they can't produce a valid access token), but the victim experiences a forced logout — a low-cost DoS on session continuity.

This is a known trade-off in refresh token rotation: the RFC 6819 / OAuth BCP recommends nuking all tokens on reuse detection, and the comment accurately describes the intent. Just ensure this risk is consciously accepted for the app's threat model.

Redundant call: After deleteRefreshTokensByUserId removes all tokens for the user, the later await SERVICES.deleteToken({ tokenId: token.id }) is a no-op (the row is already gone). It won't cause an error assuming the service handles missing IDs gracefully, but it's dead work that could be removed or replaced with a comment explaining why it's skipped.

---

Removed Test Cases

Two tests were removed:

• 'theres no UserId in the cookies' — correctly removed; the check it covered no longer exists.
• 'the token user does not match the JWT user' — correctly removed; userId is now authoritative from the DB record, so there is no cross-reference to validate.

No coverage gaps as a result of these removals.

---

Minor

The logout.test.ts .skip on the route-level test remains — not introduced by this PR, but worth tracking.

[claude]

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

Overall: Approve with one design note

---

What is good

Rate limiting on POST /users/register — the endpoint was only protected by the loose global limiter (100 req/10 min). Moving it to rateLimitTenPerTenMins (10/10 min) matches the protection already in place on login, magic-login, and reset-password. Correct fix.

RESIDENT_TOKEN cookie removal — exposing the raw DB user ID on every auth response with no server-side benefit was unnecessary. The ID is already inside the signed JWT, so removing this extra cookie is a clean security improvement.

Logout refactoring — resolving userId from the DB token record instead of trusting a separate cookie is the right approach. The graceful-degradation test (succeeds even when token is not in DB) is a good addition.

refreshToken refactoring — userId is now authoritative from the DB record, not from a client-supplied cookie. This is strictly more secure.

---

Design note: session-nuke order in refreshToken.ts

deleteRefreshTokensByUserId is called before token.used is checked:

const token = await SERVICES.getToken({ tokenId: refreshTokenId })
if (!token) throw ...
await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })  // happens first
if (token.used) throw ...                                              // checked after

The intent (token reuse = possible theft = nuke all sessions) is documented in the code comment, and the design is a valid security tradeoff. However, this means a legitimate user experiencing a network retry on a just-used token silently loses all their other active sessions. Worth considering whether a dedicated test covers this scenario and whether the error surfaced to the client gives enough signal that they need to re-authenticate everywhere.

---

Issues to fix before merging

1. Dead message constants

MESSAGES.TOKEN_USER_INVALID and MESSAGES.REFRESH_TOKEN_COUNTERPART_REQUIRED are now unreferenced — the checks that used them were removed in this PR and PR 466 respectively. Remove them here or in PR 466 to avoid dead code.

2. Skipped logout.test.ts scenario for missing refresh token

The test description was updated from 'Throws an error if missing the user data' to 'Throws an error if the refresh token cookie is absent', which correctly reflects the new logic. Good. One scenario worth adding: what happens when req.cookies is present but the refreshToken key is an empty string vs undefined — both should be handled by the existing guard, but a test would confirm.

[claude]

Code Review

Good security hardening - the rate limit fix is straightforward and the RESIDENT_TOKEN removal is the right call. A few things worth discussing.

---

What is well done

Rate limiting on /register - Correctly applies rateLimitTenPerTenMins (matching login, magic-login, reset-password), closing an obvious enumeration/spam vector. Placement before VALIDATE.email in the middleware chain is correct.

RESIDENT_TOKEN cookie removal - The user ID was already encoded in the JWT; exposing it as a separate cookie added surface area with no benefit. Good removal.

Logout via refresh token DB lookup - logout.ts now uses SERVICES.getToken to resolve userId from the refresh token rather than trusting a separate cookie. This is strictly more secure and the graceful-degradation path (token already expired/purged) is well-handled and tested.

RefreshToken: userId from DB - token.userId is now the authoritative source rather than a separate cookie value. The old cross-validation check (token.userId !== userId) becomes redundant once both sides come from the same DB record, so removing it is correct.

---

Issues

Redundant deleteToken call in refreshToken.ts (line 69)

The sequence is:

1. Fetch token from DB (line 26)
2. deleteRefreshTokensByUserId({ userId: token.userId }) - deletes ALL sessions including this token (line 36)
3. Create a fresh refresh token (line 45)
4. deleteToken({ tokenId: token.id }) - tries to delete the already-deleted token (line 69)

Step 4 is a no-op since the token was already removed in step 2. This adds an unnecessary DB round-trip on every successful token refresh. The deleteToken call should be removed.

Aggressive session invalidation trade-off (refreshToken.ts lines 32-39)

The comment documents the intent well, but the current order means a legitimate expired token triggers deletion of all active sessions:

1. Token found in DB
2. ALL sessions deleted
3. Token checked for .used flag or expiry -> throw if invalid

For an already-expired token that has not been rotated (e.g., user was simply inactive), this nukes all their other active devices. This is the documented trade-off but it is worth confirming this is the desired UX. An alternative is to validate .used and expiresAt before deleting other sessions, and only nuke-all on .used (which indicates token reuse/theft), not on natural expiry.

---

Minor notes

• src/routes/auth/logout.test.ts remains .skip - this was already skipped before this PR, but now the test body is updated. Consider un-skipping or filing a follow-up to write a proper integration test.
• The getToken service call in logout.ts adds one extra DB round-trip per logout. Acceptable cost for correctness.

---

Test coverage

New tests added in logout.test.ts are good - graceful degradation on missing DB token is an important case to cover. The refreshToken.test.ts mock cleanup is clean.

Verdict: Mostly approve. Remove the redundant deleteToken call (step 4 above). Consider the expiry-vs-reuse distinction in session invalidation if multi-device support matters.

[claude]

Code Review — PR #467: rate limit register endpoint; remove RESIDENT_TOKEN cookie

Stacks cleanly on #466. Both changes are good security improvements. A couple of things worth discussing.

---

✅ Rate limit on /register

Simple and correct. POST /register is now on rateLimitTenPerTenMins, matching login, magic-login, and reset-password. Without this, an attacker could enumerate or spam registrations at 100 req/10 min.

---

✅ Removal of RESIDENT_TOKEN cookie

Removing the cookie that exposed the internal DB user ID on every auth response is the right call — it was redundant (the ID is in the JWT payload) and unnecessarily leaked an internal identifier.

---

⚠️ refreshToken.ts — session wipe before token validation

The current order is:

1. Fetch token from DB
2. Delete ALL refresh sessions for that user
3. Check token.used
4. Check token.expiresAt

The comment explains the rationale (token reuse = likely theft = nuke all sessions), and it is a legitimate security strategy. However, the trade-off is real: any possession of a valid refresh token ID — even an expired or previously-used one — is enough to log out all active sessions for that user.

This becomes relevant if a refresh token ID leaks via logs, browser history, or a monitoring tool, or if an admin has already expired a user's sessions and an old cookie is replayed.

One option worth considering: check token.used and token.expiresAt first, throw on invalid state without deleting sessions, and only nuke all sessions on the token.used path (the genuine reuse-detection case). That would preserve the security intent while limiting the blast radius for non-malicious replays. This is a design discussion, not a blocker.

---

⚠️ getToken has no token-type filter

SERVICES.getToken({ tokenId }) fetches any token by ID regardless of type. In refreshToken.ts and logout.ts, the caller passes the value from the refreshToken cookie — but there's no assertion that the returned token is actually TOKEN_TYPE.REFRESH.

The risk is low in practice (CUID2 IDs are unguessable, and exploiting this would require knowing a valid token ID of a different type belonging to the target user). However, an explicit token.type === TOKEN_TYPE.REFRESH guard in the caller — or a dedicated getRefreshToken service — would make the intent explicit and remove the ambiguity entirely.

---

✅ logout.ts — userId now from DB, not cookie

Deriving userId from the DB token record rather than a separate RESIDENT_TOKEN cookie is strictly better: it removes trust in a user-supplied value and simplifies the cookie surface. The graceful degradation (clear cookie even if the token is not in the DB) is well-handled and tested.

---

Test coverage

New test cases added for the changed logout behaviour (missing refresh token cookie, graceful degradation when token not found). refreshToken tests updated to remove the now-removed RESIDENT_TOKEN dependency. Coverage looks solid.

Verdict: Approve with discussion — the changes are correct and the security improvements are real. The session-wipe ordering and missing token-type guard are worth a brief discussion before merge.

[claude]

Code Review

Strengths

Rate-limiting the register endpoint: Correct fix. Without this, account-creation brute force / enumeration was uncapped. Matching the 10/10-min rate used by login and magic-login is the right choice.

RESIDENT_TOKEN removal: Clean and correct. The userId was redundant (available in the JWT payload); the cookie exposed the internal DB ID to the browser with no server-side benefit.

Logout via refresh-token DB lookup: The new pattern (look up userId from the DB record rather than a separate cookie) is more secure — it removes a client-supplied value from the trust chain. The graceful-degradation path (cookie already cleared even if the DB lookup returns null) is a good UX decision and is now tested.

refreshToken — userId now authoritative from DB: Removing the RESIDENT_TOKEN cross-check is valid now that userId is derived solely from the DB token record. No tamper vector remains at this layer.

Issues

Aggressive session invalidation on any invalid token (refreshToken.ts): deleteRefreshTokensByUserId is called after finding the token in the DB but before validating whether it is used or expired. This means a legitimately expired token (e.g. user waited too long before refreshing) will silently nuke all of that user's active sessions. The comment acknowledges the trade-off, but the UX impact is significant — consider logging a warning when this path is taken so the behaviour is observable, or limiting the nuke to the token-reuse (used === true) case only and returning a 401 for the expiry case.

Missing test coverage for the session-nuke path: The deleted test ('the token user does not match the JWT user') tested cross-user validation, which is now gone. There is no new test asserting that presenting a used or expired token triggers deleteRefreshTokensByUserId. This is an important security behaviour that should be tested.

Non-blocking observations

• MESSAGES.REFRESH_TOKEN_REQUIRED is now used in logout.ts — confirm this key already exists in messages.ts (not added in this PR).
• The skip on the logout route test (describe.skip in routes/auth/logout.test.ts) predates this PR but is worth tracking as a follow-up.

[claude]

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

Solid direction overall. One security trade-off in refreshToken.ts deserves explicit discussion.

---

Strengths

• Rate limiting /register: Correct fix. Using the same rateLimitTenPerTenMins limiter as login is consistent.
• RESIDENT_TOKEN removal: Exposing the internal DB ID in a cookie with no server-side benefit was unnecessary information disclosure. Clean removal across all auth flows.
• Logout using DB lookup: Deriving userId from the refresh token record rather than trusting a separate cookie is more secure and architecturally cleaner.
• Graceful logout degradation: The new test "Still succeeds if the refresh token is not found in the DB" is a good edge-case addition.

---

Issues

1. All sessions are nuked before the token is fully validated (medium)

In refreshToken.ts, the current order is:

1. Fetch token from DB
2. Immediately delete ALL sessions for that user (deleteRefreshTokensByUserId)
3. Then check token.used, token.type, etc.

The comment says this is intentional ("token theft" signal), but the trade-off is a meaningful DoS vector: any attacker who obtains a stale (but still DB-present) token ID can present it and wipe all active sessions for that user, even if the subsequent checks would reject it. The token is deleted on a normal flow (deleteToken at the bottom), but deleteRefreshTokensByUserId is broader — it kills everything.

Consider moving the broad session nuke to after token.used is confirmed, where reuse is definitively proven:

if (token.used) {
  // Detected reuse — THIS is the clear signal for token theft
  await SERVICES.deleteRefreshTokensByUserId({ userId: token.userId })
  throw new ForbiddenError(MESSAGES.TOKEN_USED)
}

2. Removed test: "the token user does not match the JWT user" (low)

This check was removed because userId no longer comes from a cookie (correct). But there is now no test verifying that the access token returned to the client encodes the user from the DB record. A test asserting the returned JWT contains the correct userId would close this gap.

3. logout.test.ts — getToken error path not tested (low)

The new test covers getToken returning null, but not getToken throwing (e.g., DB unavailable). Given logout is best-effort, propagating the error is probably fine, but a test clarifying the expected behaviour would be useful.

---

Notes

• The rateLimitTenPerTenMins export in rateLimiter.ts line 43 is already present — the import in createUser.ts is correct.
• The skip on the integration test in routes/auth/logout.test.ts predates this PR; worth unblocking in a follow-up.