docs(07.2): add code review report
This commit is contained in:
curo1305
+151
@@ -0,0 +1,151 @@
|
||||
---
|
||||
phase: 07.2-security-jti-claim-redis-access-token-revocation-inserted
|
||||
reviewed: 2026-06-06T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 8
|
||||
files_reviewed_list:
|
||||
- backend/api/admin.py
|
||||
- backend/api/auth.py
|
||||
- backend/deps/auth.py
|
||||
- backend/services/auth.py
|
||||
- backend/tests/test_admin_api.py
|
||||
- backend/tests/test_auth_api.py
|
||||
- backend/tests/test_auth_deps.py
|
||||
- backend/tests/test_task2_auth_service.py
|
||||
findings:
|
||||
critical: 2
|
||||
warning: 4
|
||||
info: 2
|
||||
total: 8
|
||||
status: 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()`:
|
||||
|
||||
```python
|
||||
# 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:
|
||||
```python
|
||||
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:
|
||||
```python
|
||||
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_
|
||||
Reference in New Issue
Block a user