docs(07.4): add code review report — 3 critical, 3 warning, 2 info
This commit is contained in:
curo1305
+211
@@ -0,0 +1,211 @@
|
||||
---
|
||||
phase: 07.4-security-token-fingerprinting-token-binding-inserted
|
||||
reviewed: 2026-06-06T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 11
|
||||
files_reviewed_list:
|
||||
- backend/api/auth.py
|
||||
- backend/deps/auth.py
|
||||
- backend/main.py
|
||||
- backend/services/auth.py
|
||||
- backend/tests/conftest.py
|
||||
- backend/tests/test_auth_deps.py
|
||||
- backend/tests/test_auth_fgp.py
|
||||
- backend/tests/test_cloud.py
|
||||
- backend/tests/test_documents.py
|
||||
- backend/tests/test_security_headers.py
|
||||
- frontend/package.json
|
||||
findings:
|
||||
critical: 3
|
||||
warning: 3
|
||||
info: 2
|
||||
total: 8
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 07.4: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-06T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 11
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 7.4 introduced token fingerprinting (the `fgp` JWT claim) binding access tokens to the requesting client's `User-Agent` and `Accept-Language` headers. The core mechanism is implemented correctly — HMAC-SHA256 with `hmac.compare_digest` for constant-time comparison — but three critical defects were found: the grace-period bypass condition uses truthiness (`if fgp_claim:`) rather than key presence, which conflates a legitimately-absent `fgp` key (pre-7.4 migration) with a deliberately empty `fgp` value in a crafted token; the `_compute_fgp` function uses `settings.secret_key` whose default is the literal string `"CHANGEME"`, meaning any deployment that does not set that env var exposes a predictable HMAC key; and the `authed_client` fixture in `test_auth_api.py` was not updated to send `_TEST_USER_AGENT`, creating a latent test fragility that depends on httpx's version-specific default header value. Two additional logic warnings concern the refresh endpoint discarding `remember_me` session type on every rotation and a redundant local import of `hashlib` inside `logout`. The `_compute_fgp` function is accessed across a module boundary via the `_`-prefixed name, which is a minor encapsulation violation.
|
||||
|
||||
## Narrative Findings (AI reviewer)
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: `fgp` bypass condition uses truthiness, not key presence — empty `fgp` in payload silently skips fingerprint validation
|
||||
|
||||
**File:** `backend/deps/auth.py:92-93`
|
||||
**Issue:** The migration grace-period guard is written as:
|
||||
```python
|
||||
fgp_claim = payload.get("fgp", "")
|
||||
if fgp_claim:
|
||||
```
|
||||
`payload.get("fgp", "")` returns `""` in two distinct situations: (a) the key is absent (pre-7.4 token — intended bypass), and (b) the key is present but set to `""`. The `if fgp_claim:` truthiness test treats both cases identically and skips validation for both. A token whose payload contains `"fgp": ""` will therefore pass through the fingerprint check regardless of the request's `User-Agent`. Because the JWT is ES256-signed the attacker must already possess the private key to exploit this — but the condition is still semantically wrong: it should express "key absent", not "empty string", and will cause subtle false passes if any code path ever produces a token with `fgp=""` (e.g., if `_compute_fgp` were to return an empty string in a future edge case).
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# Correct: distinguish absent key from empty value
|
||||
fgp_claim = payload.get("fgp") # None when absent, str when present (even empty)
|
||||
if fgp_claim is not None: # pre-7.4 tokens have no fgp key — skip gracefully
|
||||
fgp_actual = auth_service._compute_fgp(
|
||||
request.headers.get("User-Agent", ""),
|
||||
request.headers.get("Accept-Language", ""),
|
||||
)
|
||||
if not hmac.compare_digest(fgp_claim, fgp_actual):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_401_UNAUTHORIZED,
|
||||
detail="Token fingerprint mismatch",
|
||||
headers={"WWW-Authenticate": "Bearer"},
|
||||
)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-02: `_compute_fgp` uses `settings.secret_key` whose default is `"CHANGEME"` — predictable HMAC key in unpatched deployments
|
||||
|
||||
**File:** `backend/services/auth.py:87-93`, `backend/config.py:31`
|
||||
**Issue:** The fingerprint HMAC key is `settings.secret_key`, which defaults to the literal string `"CHANGEME"` in `config.py`. Any deployment that does not explicitly set `SECRET_KEY` in its environment will use this known key. An attacker who knows the algorithm (`HMAC-SHA256`, `secret_key="CHANGEME"`, `user_agent + accept_lang`) can precompute valid `fgp` values for arbitrary header combinations and forge tokens that pass fingerprint validation — provided they can also forge ES256 signatures, which requires the private key. The immediate exploitability is blocked by ES256, but the defense-in-depth principle is violated: if the private key ever leaks, the predictable HMAC key adds zero friction. Additionally, `secret_key` is already used for other purposes (slowapi, unrelated HMAC operations) — the fgp HMAC key should be an independent secret or at minimum derived via HKDF with a distinct purpose label, consistent with the pattern used for cloud credentials (`cloud_creds_key` + HKDF).
|
||||
|
||||
**Fix:**
|
||||
1. Add a dedicated `fgp_hmac_key: str = ""` field to `Settings` with no default value that passes a non-empty check:
|
||||
```python
|
||||
# config.py
|
||||
fgp_hmac_key: str = "" # must be set; absence is caught at startup
|
||||
```
|
||||
2. Guard startup: if `fgp_hmac_key` is empty, raise `RuntimeError` so the app fails fast rather than silently using an insecure key.
|
||||
3. Update `_compute_fgp` to use `settings.fgp_hmac_key`.
|
||||
4. Alternatively, derive a sub-key via HKDF from `settings.secret_key` with `info=b"fgp"` — consistent with the cloud-creds pattern already in the codebase.
|
||||
|
||||
---
|
||||
|
||||
### CR-03: `authed_client` in `test_auth_api.py` sends no explicit `User-Agent` — fgp validation silently depends on httpx's version-default header
|
||||
|
||||
**File:** `backend/tests/test_auth_api.py:118`
|
||||
**Issue:** The `authed_client` fixture creates its `AsyncClient` without a `headers=` argument:
|
||||
```python
|
||||
async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as c:
|
||||
```
|
||||
httpx injects its own `User-Agent: python-httpx/X.Y.Z` header by default. The login endpoint (`POST /api/auth/login`) reads this header and embeds it in the `fgp` claim of the issued access token. Subsequent requests in the same test session re-use the same httpx default `User-Agent`, so the fgp check currently passes. However:
|
||||
|
||||
- If `httpx` is upgraded and the default UA string changes between the login call and a token-use call within a single test session (unlikely but possible with per-request header overrides), tests will start failing with 401 "Token fingerprint mismatch" with no obvious cause.
|
||||
- More practically: any test that obtains a token via login and then sets explicit headers (including an `Authorization` header without also propagating `User-Agent`) will trigger a mismatch if the per-request headers override the client-level default.
|
||||
- The `async_client`, `headers_client`, and `auth_client` fixtures were all updated in Phase 7.4 to send `_TEST_USER_AGENT`. The `authed_client` fixture was not updated, creating an inconsistency that will be difficult to diagnose when a future test adds a per-request `User-Agent` override.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# test_auth_api.py, authed_client fixture (line 118)
|
||||
async with AsyncClient(
|
||||
transport=ASGITransport(app=app),
|
||||
base_url="http://test",
|
||||
headers={"User-Agent": _TEST_USER_AGENT}, # add this line
|
||||
) as c:
|
||||
yield c
|
||||
```
|
||||
Also add the import at the top of `test_auth_api.py`:
|
||||
```python
|
||||
from tests.conftest import _TEST_USER_AGENT
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `refresh_token` endpoint always sets `remember_me=False` on cookie — 30-day sessions silently downgraded to 16 hours on first rotation
|
||||
|
||||
**File:** `backend/api/auth.py:367`
|
||||
**Issue:** On token rotation, `_set_refresh_cookie` is called without a `remember_me` argument:
|
||||
```python
|
||||
_set_refresh_cookie(response, new_raw) # remember_me defaults to False
|
||||
```
|
||||
The `remember_me` state from the original login is not stored anywhere (it is not in the `RefreshToken` DB row, not in a cookie, not in the JWT). Therefore, a user who logged in with `remember_me=True` (30-day session) will have their cookie's `Max-Age` silently reset to 16 hours on the first token rotation. The refresh token in the DB retains its original 30-day `expires_at`, so the token remains valid server-side for 30 days, but the browser will discard the cookie after 16 hours — effectively capping sessions to 16 hours regardless of the `remember_me` choice.
|
||||
|
||||
**Fix:**
|
||||
Add a `remember_me` boolean column to `RefreshToken` (default `False`) populated at creation time, then read it during rotation:
|
||||
```python
|
||||
# services/auth.py — rotate_refresh_token
|
||||
new_raw = await create_refresh_token(session, row.user_id, remember_me=row.remember_me)
|
||||
return new_raw, str(row.user_id), row.remember_me # return the flag
|
||||
|
||||
# api/auth.py — refresh_token endpoint
|
||||
new_raw, user_id_str, remember_me = await auth_service.rotate_refresh_token(session, raw_token)
|
||||
_set_refresh_cookie(response, new_raw, remember_me=remember_me)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-02: `_compute_fgp` (private by Python convention) is called across a module boundary via `auth_service._compute_fgp`
|
||||
|
||||
**File:** `backend/deps/auth.py:94`
|
||||
**Issue:** `_compute_fgp` has a leading underscore indicating it is module-private. `deps/auth.py` reaches across the module boundary to call `auth_service._compute_fgp(...)`. While Python does not enforce this at runtime, it violates the convention that `_`-prefixed names are internal implementation details. If `_compute_fgp` is renamed or its signature changes, the linter will not flag the call site in `deps/auth.py` because the name is accessed via attribute lookup on the imported module object. This also prevents type-checkers and IDEs from warning on incorrect usage.
|
||||
|
||||
**Fix:** Rename `_compute_fgp` to `compute_fgp` (drop the underscore) in `services/auth.py` and update all call sites. The function is already documented as part of the Phase 7.4 public API surface (it is cited in `D-04` and `D-05` design notes).
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `logout` endpoint contains a redundant local `import hashlib` shadowing the module-level import
|
||||
|
||||
**File:** `backend/api/auth.py:392`
|
||||
**Issue:** `hashlib` is already imported at the module level (line 22). Inside the `logout` handler, the code performs:
|
||||
```python
|
||||
import hashlib as _hashlib
|
||||
...
|
||||
token_hash = _hashlib.sha256(raw_token.encode()).hexdigest()
|
||||
```
|
||||
This local import is unnecessary and creates a confusing divergence from every other use of `hashlib` in the same file (which uses the module-level name). The local alias `_hashlib` serves no purpose since there is no name conflict.
|
||||
|
||||
**Fix:** Remove the local import and use the module-level `hashlib` directly:
|
||||
```python
|
||||
# Remove line 392: import hashlib as _hashlib
|
||||
token_hash = hashlib.sha256(raw_token.encode()).hexdigest()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `frontend/package.json` uses `^` (caret) version ranges for security-adjacent dependencies
|
||||
|
||||
**File:** `frontend/package.json`
|
||||
**Issue:** CLAUDE.md Security Protocol states: "Dependency pinning: `requirements.txt` and `package-lock.json` pin exact versions; no floating `>=` for security-critical packages." While `package-lock.json` pins exact versions at install time, the `package.json` itself uses caret ranges (`^`) for all dependencies including `vue`, `pinia`, `vue-router`, and `vite`. If `package-lock.json` is deleted and regenerated (e.g., after a `npm install --legacy-peer-deps` or CI environment without a lockfile), versions will float to the latest compatible release, potentially pulling in a dependency with a CVE. This is a minor risk in a lockfile-driven workflow but worth noting for the CLAUDE.md requirement.
|
||||
|
||||
**Fix:** Consider pinning to exact versions in `package.json` for the packages most relevant to security (at minimum `vite` and `vue`), or document that `package-lock.json` must always be committed and never regenerated without review.
|
||||
|
||||
---
|
||||
|
||||
### IN-02: No test asserts that `fgp` with an empty-string value in the payload bypasses validation (the CR-01 gap has no dedicated test)
|
||||
|
||||
**File:** `backend/tests/test_auth_fgp.py`
|
||||
**Issue:** FGP-03 tests the case where the `fgp` key is entirely absent from the payload — confirming the migration grace period works. There is no test for the case where `fgp` is present but set to `""`. After CR-01 is fixed (changing to `is not None`), this edge case should have an explicit test to prevent regression.
|
||||
|
||||
**Fix:** Add a FGP-05 test after fixing CR-01:
|
||||
```python
|
||||
@pytest.mark.asyncio
|
||||
async def test_fgp_empty_string_value_triggers_mismatch(auth_client, db_session):
|
||||
"""FGP-05: token with fgp='' (empty string) must fail validation, not bypass it."""
|
||||
user = await _create_user(db_session, role="user")
|
||||
now = datetime.now(timezone.utc)
|
||||
payload_empty_fgp = {
|
||||
"sub": str(user.id), "role": "user", "typ": "access",
|
||||
"iat": now, "exp": now + timedelta(minutes=15),
|
||||
"jti": str(_uuid.uuid4()),
|
||||
"fgp": "", # present but empty — must NOT bypass validation
|
||||
}
|
||||
private_pem = base64.b64decode(settings.jwt_private_key).decode()
|
||||
token = _jwt.encode(payload_empty_fgp, private_pem, algorithm="ES256")
|
||||
resp = await auth_client.get(
|
||||
"/test/me", headers={"Authorization": f"Bearer {token}", "User-Agent": "Mozilla/5.0"}
|
||||
)
|
||||
assert resp.status_code == 401
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-06T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user