fix(security): explicit JWT algorithm; body size limit

[conorluddy]

Summary

Stacks on #467 — merge that first.

• Medium: jwt.sign and jwt.verify now explicitly specify algorithm: 'HS256' / algorithms: ['HS256']. Prevents algorithm confusion attacks (e.g. alg: none) if the library ever changes its default or an attacker crafts a malicious token.
• Medium: express.json() now has an explicit limit: '10kb' — makes the body size cap intentional and documented rather than relying on the library default.

Test plan

• npm run lint && npm test green
• Manual: attempt a JWT with alg: none — verify rejected
• Manual: send a JSON body >10kb — verify 413 response

🤖 Generated with Claude Code

[claude]

Code Review — PR #468: explicit JWT algorithm; body size limit

Clean, focused PR. Nothing controversial here.

---

✅ What's good

generateJwt.ts — explicit algorithm: 'HS256'

Pins the signing algorithm so the library can never silently fall back to a different default. Prevents algorithm-confusion attacks if the secret is ever accidentally supplied as an asymmetric key.

jsonWebTokens.ts — algorithms: ['HS256'] on verify

Closes the alg: none attack vector: any token without a proper HS256 signature will now be rejected at the library level rather than relying on the secret check alone. The allowlist approach (algorithms: [...]) is the correct pattern.

express.json({ limit: '10kb' })

Makes the body size cap explicit and documented. Protects against body-based DoS and is consistent with the intent of helmet() and the global rate limiter already in place.

---

🟡 Minor suggestions

1. Test coverage

The test plan lists two manual checks:

• JWT with alg: none → verify rejected
• Body > 10kb → verify 413

Both would be straightforward to automate:

• Sign a token with { algorithm: 'none' } and assert the endpoint returns 401
• Send a 10kb+ JSON body to any endpoint and assert 413

Worth adding to the integration/e2e suite so these don't regress silently.

2. src/index.test.ts — removed csrfToken setup

The test that previously set a xsrfToken cookie on the login request no longer needs it (the middleware is gone from PR #466). This cleanup is correct.

---

🔴 No blockers

Both changes are minimal, targeted, and correct. Ready to merge once #466 and #467 are in.

[claude]

Code Review — PR #468: explicit JWT algorithm; body size limit

Summary

Clean, targeted hardening. Both changes are low-risk with clear security benefit.

---

✅ Explicit HS256 algorithm

// generateJwt.ts
jwt.sign(userToPublicUser(user), JWT_TOKEN_SECRET, {
  algorithm: 'HS256',
  expiresIn: ...,
})

// jsonWebTokens.ts
jwt.verify(token, secret, { algorithms: ['HS256'] }, (err, user) => { ... })

This correctly closes the alg: none / algorithm-confusion attack vector. The algorithms option on verify accepts an array — future-proofing if you ever need to support key rotation across two algorithms simultaneously. Good pattern.

One thing to confirm: any other places in the codebase that call jwt.sign or jwt.verify (e.g. for password-reset tokens, magic-login tokens, validation tokens) should receive the same treatment. This PR only touches generateJwt.ts and jsonWebTokens.ts. If those other token flows use jwt.sign/jwt.verify directly, they'd still be unprotected.

---

✅ Body size limit

app.use(express.json({ limit: '10kb' }))

Explicit, documented, and a sensible default for a user-management API. No complaints. If any endpoint ever needs to accept larger payloads (e.g. bulk imports, avatar data), it can mount its own express.json({ limit: '...' }) on that specific route.

---

Minor

• The limit: '10kb' is a string literal. Not worth over-engineering, but noting it's easily configurable via env var if different environments need different limits.
• No tests verify the 413 response. The test plan mentions manual verification — an integration test with a >10kb body would make this regression-proof.

---

Verdict: Approve. Small and correct. The one thing worth double-checking before merge is whether jwt.sign/jwt.verify calls elsewhere in the codebase (magic-login, reset-password, validation token flows) also need the explicit algorithm pinned.

🤖 Generated with Claude Code

[claude]

Code Review — PR #468: explicit JWT algorithm; body size limit

Overall

Minimal, focused security hardening. Both changes are correct and add meaningful protection with zero functional regression risk.

---

src/utils/generateJwt.ts + src/middleware/auth/jsonWebTokens.ts ✅

Pinning algorithm: 'HS256' on sign and algorithms: ['HS256'] on verify closes the algorithm-confusion vector. This is the correct approach per the jsonwebtoken docs — the allowlist on verify is the more critical of the two since it prevents a crafted token with alg: none or a switched asymmetric algorithm from being accepted.

Worth confirming: the only other jwt.sign call site was generateXsrfToken in src/middleware/util/xsrfToken.ts, which was removed in #466. No remaining unguarded call sites. ✅

---

src/index.ts — express.json({ limit: '10kb' }) ✅

10 KB is a sensible default for a JSON API. Making the cap explicit is better than relying on library defaults, which have changed between versions.

If any future endpoint is expected to receive larger payloads (e.g. a JSON-encoded blob), a per-route express.json({ limit: 'Xkb' }) override is the right pattern rather than raising the global cap.

---

Test coverage

No new tests needed — these are defensive configuration changes, not new logic paths. ✅

---

Summary

Area	Status
JWT algorithm pinning (sign)	✅ Correct
JWT algorithm pinning (verify)	✅ Correct
Body size limit	✅ Correct
No remaining unguarded jwt calls	✅ Verified
Test coverage	✅ No gaps introduced

Verdict: Approve. Clean, high-signal, low-risk changes.

🤖 Reviewed with Claude Code

[claude]

Code Review — PR #468: explicit JWT algorithm; body size limit

Smallest of the three PRs but both changes are well-targeted.

---

src/utils/generateJwt.ts + src/middleware/auth/jsonWebTokens.ts ✅

Pinning algorithm: 'HS256' on sign and algorithms: ['HS256'] on verify is the right defence against algorithm-confusion attacks. If someone crafts a token with "alg": "none" or tries an RS256/HS256 confusion attack, the library will now reject it explicitly rather than relying on the default behaviour of whichever version of jsonwebtoken happens to be installed.

One sweep worth doing: are there any other jwt.sign() calls in the codebase outside generateJwt.ts? The previous XSRF token utility (xsrfToken.ts, removed in #466) also called jwt.sign without an explicit algorithm. Now that it's gone, generateJwtFromUser appears to be the only signing site — but a quick grep -r "jwt.sign" before merge would confirm nothing was missed.

---

src/index.ts — express.json({ limit: '10kb' }) ✅

Making the body size cap intentional and visible is good. One question worth confirming: do any routes accept payloads legitimately larger than 10kb? For example, if there's an endpoint that receives a base64-encoded image, a batch of records, or rich user metadata, 10kb would silently break it. If all routes are thin JSON (credentials, profile fields, tokens), 10kb is comfortably safe and probably even generous.

If in doubt, a per-route express.json({ limit: '...' }) can override the global limit for the specific endpoints that need it, keeping the tight default everywhere else.

---

Test plan items

The manual test steps in the PR description are well-chosen. If an automated test for the 413 case is feasible to add to the integration test suite, it would prevent accidental regressions if the limit is ever changed.

---

Summary: Approve. Low-risk, high-value hardening.

[claude]

Code Review — PR #468

Overall: Approve. Both changes are small, correct security improvements with no downsides.

---

src/middleware/auth/jsonWebTokens.ts — Explicit algorithm in jwt.verify ✅

jwt.verify(token, secret, { algorithms: ['HS256'] }, (err, user) => { ... })

Using the array form algorithms: ['HS256'] is the correct API for the verify overload. This prevents:

• alg: none attacks — an attacker crafting a token with no signature.
• Algorithm confusion — e.g., an RS256 token being verified against an HS256 secret (if a public key were ever used as the secret value by mistake).

---

src/utils/generateJwt.ts — Explicit algorithm in jwt.sign ✅

jwt.sign(userToPublicUser(user), JWT_TOKEN_SECRET, { algorithm: 'HS256', expiresIn: ... })

Pinning the signing algorithm is good practice. Sign and verify now agree explicitly on HS256, so there's no ambiguity if the library default ever changes.

---

src/index.ts — Body size limit ✅

app.use(express.json({ limit: '10kb' }))

10 KB is a sensible cap for an auth/user-management API. Documenting the intent in code (rather than relying on the library default) is the right call.

Worth noting for the future: this is a global limit. If any endpoint ever needs to accept a larger payload (e.g., bulk operations, file metadata), you'd want to override it at the route level. For now, 10 KB is appropriate across the board.

---

Version bump / package-lock note (very minor)

Looking across the stack, package.json was bumped in PR #466 without a corresponding package-lock.json update. This means package-lock.json ends up one version behind package.json after all three PRs land (lock at 0.3.8, package at 0.3.9). The lock file version field is cosmetic and doesn't affect resolution, so this is harmless — but worth syncing with npm install before merging if you want them to stay consistent.

---

Test coverage

The test plan items (reject alg: none tokens, enforce 413 on large bodies) are appropriate manual verifications. No automated test changes needed for these particular hardening steps — the JWT algorithm is tested implicitly by any test that issues and verifies a token.

[claude]

Code Review - PR 468: explicit JWT algorithm; body size limit

Small, focused, and correct. Both changes address real attack surface.

Strengths

JWT algorithm pinning (src/utils/generateJwt.ts and src/middleware/auth/jsonWebTokens.ts): Explicitly specifying algorithm: 'HS256' on sign and algorithms: ['HS256'] on verify closes the alg:none confusion attack and future-proofs against any library default changes. This is the recommended practice per the jsonwebtoken docs and JWT security best practices.

express.json limit: '10kb' (src/index.ts): Making the body size cap explicit documents the intent and protects against JSON bomb / large-payload DoS regardless of future express.json default changes. 10 KB is a reasonable default for a user-management API.

Notes

The 10 KB limit applies globally. If any endpoint in this API is expected to accept larger payloads (e.g. file upload metadata, bulk operations), those routes would need a per-route override. Worth a quick scan to confirm no existing route legitimately needs more.

There is no corresponding test for the 413 behavior (the test plan mentions a manual check). A supertest case sending a body over 10 KB to any JSON endpoint would lock this in automatically.

Verdict

Both changes are correct, minimal, and well-scoped. Approved.

[claude]

Code Review: PR 468 - explicit JWT algorithm; body size limit

Medium: JWT algorithm pinning

Specifying algorithm: 'HS256' on sign and algorithms: ['HS256'] on verify is best practice - prevents alg: none attacks and protects against future jsonwebtoken library defaults changing. The change is minimal and targeted.

The test plan item 'attempt a JWT with alg: none - verify rejected' is worth running manually since this defence-in-depth change is hard to cover with unit tests alone.

Medium: Body size limit

10kb is a sensible default for a user management API. If future endpoints need larger payloads (e.g. file uploads or bulk operations), those routes can apply their own express.raw() or a route-level express.json override. Worth noting that this limit is now global, so any new endpoint exceeding it will silently return a 413 - something to keep in mind as the project grows.

Package version note

package-lock.json is bumped 0.3.7 to 0.3.8 while package.json targets 0.3.8 to 0.3.9. This is an expected artefact of stacked PRs and should resolve correctly when merged in order (#466 -> #467 -> #468). No action needed, just flagging so it does not surprise the merge reviewer.

---

Clean, focused changes. LGTM pending CI green.

[claude]

Code Review

Summary

Two small, high-value security hardening changes. Both are correct.

---

src/middleware/auth/jsonWebTokens.ts — Algorithm pinning on jwt.verify

✅ Important security fix.

Without algorithms: ['HS256'], the jsonwebtoken library uses the alg field from the token header itself to choose the verification algorithm. An attacker who can craft a token could specify "alg": "none" and bypass signature verification entirely (the classic "algorithm confusion" attack). Pinning to ['HS256'] closes this:

jwt.verify(token, secret, { algorithms: ['HS256'] }, (err, user) => { ... })

This is now consistent across the codebase — after PRs #466 and #467 remove xsrfTokens.ts, this is the only remaining jwt.verify call.

---

src/utils/generateJwt.ts — Explicit algorithm on jwt.sign

✅ Good defensive practice. Making the signing algorithm explicit prevents silent breakage if the jsonwebtoken library ever changes its default, and makes intent clear to readers:

return jwt.sign(userToPublicUser(user), JWT_TOKEN_SECRET, {
  algorithm: 'HS256',
  expiresIn: ...
})

---

src/index.ts — Body size limit

✅ Correct hardening. Relying on the library default is implicit; setting limit: '10kb' makes the cap intentional and visible in code review:

app.use(express.json({ limit: '10kb' }))

10kb is a reasonable ceiling for a user-management REST API where no endpoint is expected to receive large payloads. If a future endpoint needs to handle larger bodies (e.g., bulk imports), it can apply a route-level express.json({ limit: '1mb' }) override rather than raising the global limit.

---

Completeness check

After this PR stack (466 → 467 → 468) is merged:

• jwt.verify with algorithm pinning: ✅ jsonWebTokens.ts
• jwt.sign with explicit algorithm: ✅ generateJwt.ts
• Deleted files (xsrfTokens.ts, xsrfToken.ts) had their own jwt.verify/jwt.sign calls — those are gone

Nothing appears to be missed.

---

Overall

Approve. Both changes are minimal, targeted, and correct. No concerns.

[claude]

Code Review: fix(security): explicit JWT algorithm; body size limit

Summary

Small, focused, and correct. Both changes close real attack surface.

---

Explicit JWT Algorithm ✅

generateJwt.ts — adding algorithm: 'HS256' to jwt.sign makes the output contract explicit and prevents any future surprise if the library default changes.

jsonWebTokens.ts — adding algorithms: ['HS256'] to jwt.verify is the more important of the two. Without it, a crafted token with "alg": "none" could bypass verification in older versions of jsonwebtoken, and an attacker who also controls a public key could attempt an RS256 confusion attack. Allowlisting closes both.

These two changes should always travel together — signing with an explicit algorithm and verifying with an explicit allowlist. ✅

---

Body Size Limit ✅

express.json({ limit: '10kb' }) is a sensible default for a user-management API. Worth noting: this limit applies to JSON bodies only (not multipart/form-data). If file upload routes are added later, they will need their own middleware and this limit won't block them inadvertently.

Potential consideration: If any current or planned endpoint legitimately needs larger payloads (e.g., bulk operations, base64-encoded avatars), it would be worth making this limit an env variable (REQUEST_BODY_LIMIT_KB). For the current API surface, 10kb is fine as a hardcoded constant.

---

Test Coverage

No new tests needed — the algorithm constraint is exercised by the existing JWT middleware tests, and the body limit behaviour is validated by Express itself. A note in the test plan about manually verifying a 413 on oversized bodies is sufficient.

[claude]

Code Review: PR 468 - explicit JWT algorithm; body size limit

Overall: Approve

Clean, focused security hardening. Both changes are straightforward and correct.

---

What is good

Explicit HS256 in jwt.verify — adding { algorithms: ['HS256'] } closes the algorithm confusion attack surface. Without this, a crafted token with alg=none or alg=RS256 (using the HMAC secret as an RSA public key) could potentially bypass verification depending on the jsonwebtoken version. Explicit allowlisting is the right pattern.

Explicit HS256 in jwt.sign — HS256 was already the implicit default when signing with a string secret, but being explicit future-proofs the code against library behaviour changes and makes the intent clear to readers.

10kb body limit on express.json() — makes the cap intentional and documented rather than relying on library defaults. Reasonable for a user-management API.

---

Minor notes

• If any endpoint in the future needs to accept larger payloads (e.g. bulk operations or file metadata), the limit will need to be raised or overridden per-route. Worth a comment in the relevant route if that ever comes up.
• The test plan item 'Manual: attempt a JWT with alg=none — verify rejected' is a good addition. Consider adding an automated test for this in src/middleware/auth/jsonWebTokens.test.ts so the protection is regression-tested rather than relying on manual verification.

[claude]

Code Review

Small but impactful security hardening. Both changes are correct and the rationale is clear.

---

What is well done

Explicit JWT algorithm - Specifying algorithm: 'HS256' in jwt.sign() and algorithms: ['HS256'] in jwt.verify() closes the alg:none attack and RS256/HS256 confusion attacks. Even if the jsonwebtoken library's default behaviour never changes, explicit beats implicit for security-critical config. Good defensive move.

Body size limit - express.json({ limit: '10kb' }) makes the cap intentional and auditable rather than relying on the library's undocumented default. 10kb is sensible for a JSON API.

---

Issues / Questions

No test for body size limit

The test plan mentions manually verifying a 413 response, but there is no automated test. A simple supertest assertion would lock this in:

const response = await request(app)
  .post('/auth')
  .send({ data: 'x'.repeat(11000) })
expect(response.status).toBe(413)

Not blocking, but the manual step is easy to forget.

10kb limit and future endpoints

If any existing or planned endpoint legitimately needs a larger body (bulk user import, base64-encoded content, etc.), it will need its own express.json({ limit: '...' }) middleware applied at the route level. Worth noting in a comment or issue so it is not a surprise later.

---

Minor notes

• The package-lock.json version bump (0.3.7 -> 0.3.8) is consistent with the package.json bump in fix(security): hash password on user update; remove broken XSRF middleware #466 (0.3.6 -> 0.3.8). The version numbers across the stacked PRs are a little non-linear but that is a merge-order artefact, not a real issue.

---

Verdict: Approve. The changes are correct and well-scoped. Adding an automated test for the 413 response would be a nice follow-up.

[claude]

Code Review — PR #468: explicit JWT algorithm; body size limit

Small, focused, correct. Nothing to flag.

---

✅ Explicit algorithm: 'HS256' on jwt.sign

generateJwtFromUser now passes { algorithm: 'HS256' } to jwt.sign. This documents the intended algorithm and protects against any future change in the library's default behaviour.

✅ Explicit algorithms: ['HS256'] on jwt.verify

authenticateToken now passes { algorithms: ['HS256'] } to jwt.verify. This is the key defence: without an explicit allowlist, a malicious token crafted with alg: none would bypass signature verification in older or misconfigured versions of the library. Both sides of the JWT lifecycle are now pinned — consistent and correct.

✅ Body size limit 10kb

express.json({ limit: '10kb' }) makes the cap intentional and visible in code rather than relying on an undocumented library default. 10 KB is appropriate for a user-management API dealing with small JSON payloads.

---

Minor observations

• If any future endpoint needs larger payloads (e.g. a bulk-import route), the limit can be overridden per-router. Worth a note if that scenario is anticipated.
• The test plan mentions manual verification for the alg:none rejection and 413 response — integration tests for these edge cases would be a nice addition but are not a blocker.

Verdict: Approve — clean, well-targeted, no concerns.

[claude]

Code Review

Strengths

Explicit HS256 algorithm: Correct on both sign and verify. Preventing algorithm confusion (e.g. alg: none or RS256 with an attacker-controlled public key) is a well-known JWT hardening step. The fix is applied consistently: algorithm on jwt.sign in generateJwt.ts and algorithms array on jwt.verify in jsonWebTokens.ts.

10kb body size limit: Sensible default for an auth API. Makes the cap intentional and documented rather than relying on library defaults.

Issues

Are there other jwt.verify call sites without algorithms? The deleted xsrfTokens.ts had a bare jwt.verify call (now gone), but it is worth a quick grep across the codebase to confirm no other unguarded verify calls remain, e.g. in passport strategies or token validation utilities.

Non-blocking observations

• The body size limit applies only to express.json() (application/json). If any routes accept multipart or URL-encoded bodies, those would need their own limits — likely not a concern for this API, but worth noting.

[claude]

Code Review: fix(security): explicit JWT algorithm; body size limit

Small, targeted, and correct. No blockers.

---

Strengths

Explicit JWT algorithm

Specifying algorithm: 'HS256' on jwt.sign and algorithms: ['HS256'] on jwt.verify is the right call. The alg: none attack vector is real and this eliminates it regardless of library defaults. The API forms are correct (algorithm as string for sign, algorithms as array for verify per the jsonwebtoken API).

Body size limit

express.json({ limit: '10kb' }) makes the cap intentional and visible rather than relying on an undocumented default. Simple and effective.

---

Issues

1. No automated tests for the 413 or algorithm-rejection paths (low)

The test plan lists these as manual checks. Both are straightforward to cover with supertest and would give permanent regression protection for these security-critical paths.

2. Other jwt.sign / jwt.verify call sites (low)

generateJwt.ts is updated, but it is worth confirming no other files call jwt.sign or jwt.verify directly without the algorithm option. A quick search across the codebase would confirm full coverage.

---

Notes

• Version alignment between package.json (0.3.8) and package-lock.json (0.3.8) is correct here, resolving the mismatch introduced in fix(security): hash password on user update; remove broken XSRF middleware #466.
• Both changes are independently safe to merge with no functional side effects.

[claude]

Code Review — PR #468: explicit JWT algorithm; body size limit

Overall this is a solid, focused security hardening PR. The changes are minimal and correct.

✅ Positives

• Algorithm pinning is the right call. Explicitly specifying algorithm: 'HS256' in jwt.sign and algorithms: ['HS256'] in jwt.verify eliminates any risk from algorithm confusion attacks (e.g., alg: none or RS256-with-HMAC confusion). Depending on library defaults is always fragile across version upgrades.
• Body size limit is intentional and documented. express.json({ limit: '10kb' }) is a sensible default for this API and makes the cap explicit rather than relying on library behaviour that could change.
• Integration tests cover both hardened paths — good to have these as regression guards.

⚠️ Concerns

1. Integration test requires a live database (may fail in CI)

tests/integration/security.test.ts calls postgresDatabaseClient.connect() in beforeAll. Neither test scenario (alg: none rejection, 413 body size) actually writes or reads DB data — both fail before touching the database layer. The full DB lifecycle setup (BEGIN/ROLLBACK) is unnecessary overhead and will cause this test suite to fail in any CI environment that doesn't have a running Postgres instance. Consider either:

• Removing the DB setup/teardown entirely from this file (the tests don't need it), or
• If a DB is always available in CI, it's fine as-is — but worth confirming.

2. Stale Swagger docs in refreshToken route

src/routes/auth/refreshToken.ts line 36 still documents the residentToken cookie in its JSDoc. That cookie is removed by PR #470, which this PR depends on. The docs should be updated to remove that stale cookie description.

3. Minor: as any cast is necessary but the comment is the right call

The eslint-disable on the algorithm: 'none' cast is correct — TypeScript rightly doesn't expose 'none' as a valid Algorithm. The comment makes the intent clear. No change needed, just noting it's a known and accepted pattern.

💡 Suggestion

Since the alg: none integration test never touches the database, move the DB lifecycle hooks to a shared test setup file (if one exists) or remove them from this specific file. The test is correct and valuable as-is, but the unnecessary DB connection could cause CI flakiness.

---

Summary: Approve with the minor note about the unnecessary DB setup in the integration test. Both security fixes are correct and well-tested.