12 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 07.2-security-jti-claim-redis-access-token-revocation-inserted | 2026-06-06T00:00:00Z | standard | 8 |
|
|
issues_found |
Phase 07.2: Code Review Report
Reviewed: 2026-06-06 Depth: standard Files Reviewed: 8 Status: issues_found
Summary
Phase 7.2 implements three things: a jti UUID claim on access tokens, a user_nbf Redis check in get_current_user to invalidate pre-event tokens, and user_nbf writes in the four security-event handlers. The core security structure is sound — the except HTTPException: raise guard precedes the broad except Exception, all four write sites use settings.access_token_expire_minutes * 60 (not the banned literal 900), and the NBF write is correctly gated on if not body.is_active only.
Two critical issues exist: the JWT algorithm is still HS256 despite CLAUDE.md mandating ES256, and the three NBF-check tests in test_auth_deps.py were promoted to full assertions without @pytest.mark.xfail — but more importantly were implemented as Wave 1 complete tests when the plan intended them to start as stubs and be promoted. As implemented they are passing real assertions, which is actually the correct end state, so the critical issue is the algorithm mismatch, not the xfail promotion.
The second critical issue is a race in password_reset_confirm (auth.py): a user's pre-reset access tokens are not invalidated with a user_nbf write, leaving a 15-minute window open after a self-service password reset via the reset link — the same gap this entire phase was designed to close.
Critical Issues
CR-01: JWT algorithm is HS256, not ES256 as mandated by CLAUDE.md
File: backend/services/auth.py:100 and backend/services/auth.py:110
Issue: create_access_token signs with algorithm="HS256" (symmetric HMAC-SHA256). CLAUDE.md Security Protocol states unambiguously: "Algorithm: ES256 (ECDSA P-256) — asymmetric; the private key signs, the public key verifies; a leaked public key cannot forge tokens." All four jwt.encode / jwt.decode call sites use HS256, meaning a single leaked secret_key allows unlimited token forgery. Phase 7.3 exists precisely to upgrade to ES256, but it has not been executed; meanwhile the CLAUDE.md requirement is active and the code ships HS256 tokens.
Fix: Implement ES256 properly — generate or load an ECDSA P-256 key pair from env vars, use algorithm="ES256" at encode and decode sites. Until Phase 7.3 ships, this finding must remain open; CLAUDE.md's "Login token hardening (state of the art)" section must be updated to note the gap if HS256 is intentionally deferred.
CR-02: password_reset_confirm does not write user_nbf — pre-reset access tokens remain valid for up to 15 minutes
File: backend/api/auth.py:700-746
Issue: The POST /api/auth/password-reset/confirm endpoint changes the user's password and revokes all refresh tokens (line 742) but never writes user_nbf:{user_id} to Redis. Every other security-event handler in this phase (change-password, enable-totp, disable-totp, admin-deactivation) correctly writes the NBF key. A user who received a password-reset email — including one sent by an attacker who compromised the email inbox — can use the reset link, change the victim's password, and the victim's live access token continues to be accepted for up to 15 minutes. This is the exact threat T-7.2-01 this phase is meant to close.
Fix: Add the user_nbf write to password_reset_confirm immediately after revoking refresh tokens and before await session.commit():
# Revoke any pre-reset access tokens still within their TTL window (T-7.2-01)
await request.app.state.redis.set(
f"user_nbf:{user.id}",
int(time.time()),
ex=settings.access_token_expire_minutes * 60,
)
Also add import time (already imported in auth.py at line 22) and inject request: Request into the function signature. A corresponding test parallel to test_change_password_writes_user_nbf_to_redis must be added.
Warnings
WR-01: Three NBF-check tests in test_auth_deps.py have no @pytest.mark.xfail decorator but were listed as Wave-0 stubs — they are real assertions that may hide promotion errors
File: backend/tests/test_auth_deps.py:189-250
Issue: The plan (07.2-01-PLAN.md) specifies Wave-0 stubs decorated with @pytest.mark.xfail(strict=False). The implemented tests (test_get_current_user_rejects_token_when_iat_before_user_nbf, test_get_current_user_allows_token_when_iat_after_user_nbf, test_get_current_user_failopen_on_redis_error) contain fully-implemented assertions with no xfail decorator. This means they pass now because Wave 1 (Plan 02) was already implemented, which is the correct end state — but the VALIDATION.md status table still shows them as pending and the plan acceptance criteria checking for XFAIL counts will fail their grep assertions. The actual concern is that the comment block at line 184 says "xfailed with strict=False" but the tests are not, creating a documentation lie that will confuse the next developer.
Fix: Update the comment at line 184 to say "promoted to full assertions (Wave 1 complete)". Update VALIDATION.md status column for nbf-check-reject, nbf-check-allow, and nbf-fail-open from pending to green.
WR-02: Three NBF-write tests in test_auth_api.py have no @pytest.mark.xfail decorator but the comment block says they do
File: backend/tests/test_auth_api.py:606-697
Issue: The comment at line 606-608 reads: "These three tests are xfailed with strict=False. When Wave 2 (Plan 03) adds user_nbf writes to the handlers, these stubs are promoted to real assertions by replacing pytest.xfail(...) with actual Redis key assertions." However, the three tests (test_change_password_writes_user_nbf_to_redis, test_enable_totp_writes_user_nbf_to_redis, test_disable_totp_writes_user_nbf_to_redis) contain real assertions and no xfail decorator. Since Wave 2 writes were already implemented, these tests are correctly passing — but the comment is wrong and refers to a stub pattern that was never applied. If anyone reads this and tries to find the pytest.xfail(...) call referenced in the comment, they won't find it.
Fix: Update the comment block to say "promoted to full assertions (Wave 2 complete)" and remove references to placeholder bodies.
WR-03: admin_client fixture clears app.state.redis after yield but test_activate_user_does_not_write_user_nbf accesses it via app.state.redis before teardown — FakeRedis instance identity is fragile
File: backend/tests/test_admin_api.py:473-503
Issue: The test at line 473 manually pokes fake_redis._store.pop(...) (line 492) using a direct dict access on the _store attribute of FakeRedis. This couples the test to the internal implementation detail of FakeRedis. If FakeRedis._store is renamed or the implementation changes, this test silently stops clearing the key — the pop on a missing key returns None without error, so the test would pass vacuously if the store key format changed. The test's correctness depends on _store being a dict with exact string keys matching f"user_nbf:{target.id}", but the FakeRedis set method stores (value, deadline) tuples, and the store key is the raw string key. This is actually fine structurally, but the .pop bypass skips the TTL semantics.
Fix: Instead of poking _store directly, use the public set interface to overwrite and then delete via a new delete method, or simply restructure the test to activate a user that was never previously deactivated (avoiding the need to clear a prior NBF write entirely).
WR-04: FakeRedis.get() returns the stored value directly (not bytes) when non-bytes values are stored, but test_deactivate_user_writes_user_nbf_to_redis assumes .decode() is available
File: backend/tests/test_admin_api.py:469
Issue: The assertion at line 469 is:
assert int(nbf_bytes.decode() if isinstance(nbf_bytes, (bytes, bytearray)) else nbf_bytes) > 0
This is correct and tolerates both types. However, admin.py writes int(time.time()) — a Python int — as the Redis value (line 359). FakeRedis.set() stores the value as-is (no bytes conversion), so FakeRedis.get() returns an int, not bytes. The isinstance(nbf_bytes, (bytes, bytearray)) branch is False, the else nbf_bytes branch returns the raw int, and int(int_value) > 0 evaluates correctly. This works by accident. In production, aioredis stores bytes and returns bytes, so int(time.time()) would need .to_bytes() or str(...) encoding to be round-trippable. The production redis.set(key, int(time.time())) will serialize the integer as a string bytes representation (Redis accepts integers), and redis.get() returns b"1749123456", so the decode path works. But the FakeRedis path silently diverges from production semantics.
Fix: In all write sites, store the value as str(int(time.time())) rather than bare int(time.time()) to make FakeRedis and production Redis behavior identical. Example in admin.py line 359:
await request.app.state.redis.set(
f"user_nbf:{user.id}",
str(int(time.time())), # str not int: consistent with aioredis bytes return
ex=settings.access_token_expire_minutes * 60,
)
Apply the same change to all four write sites in auth.py (lines 510, 609, 655).
Info
IN-01: test_admin_api.py two-step "activate after deactivate" test accesses main.app directly rather than the client's transport app
File: backend/tests/test_admin_api.py:479-503
Issue: The test imports from main import app at line 479 and uses app.state.redis at line 491 and 502. The admin_client fixture also imports and configures from main import app and sets app.state.redis = FakeRedis(). Since these are the same module-level singleton, this works — but it is a fragile pattern. If the test infrastructure is ever refactored to use sub-apps or the fixture creates a different app instance, this test will silently access the wrong Redis. The companion test in test_auth_deps.py correctly uses auth_client._transport.app.state.redis (line 200) which follows the actual request path.
Fix: Use client._transport.app.state.redis consistently instead of importing app directly.
IN-02: services/auth.py — verify_backup_code does not early-exit after finding a match; it continues iterating all remaining rows
File: backend/services/auth.py:363-368
Issue: The comment at line 364 says "Always call verify_password for ALL rows (constant-time: no early exit)" — this is intentional to prevent timing-based enumeration. This is architecturally correct for security, not a bug. However, the comment justification is incomplete: after matched_row is set, subsequent verify_password calls still iterate through remaining rows, but the matched_row is overwritten if a second code also matches (which should be impossible given unique hashed codes). The comment should explicitly state that matched_row is intentionally overwritten rather than accumulated, so a future developer doesn't "optimize" this into an early return.
Fix: Add a clarifying comment: # matched_row intentionally overwritten if somehow two hashes collide — last match wins; does not affect security since we need only one valid code.
Reviewed: 2026-06-06 Reviewer: Claude (gsd-code-reviewer) Depth: standard