From ab4b6052a72f938cebeca6485fcc98086c88e048 Mon Sep 17 00:00:00 2001 From: curo1305 Date: Sat, 6 Jun 2026 22:23:41 +0200 Subject: [PATCH] =?UTF-8?q?docs(07.4):=20add=20code=20review=20report=20?= =?UTF-8?q?=E2=80=94=203=20critical,=203=20warning,=202=20info?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../07.4-REVIEW.md | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 .planning/phases/07.4-security-token-fingerprinting-token-binding-inserted/07.4-REVIEW.md diff --git a/.planning/phases/07.4-security-token-fingerprinting-token-binding-inserted/07.4-REVIEW.md b/.planning/phases/07.4-security-token-fingerprinting-token-binding-inserted/07.4-REVIEW.md new file mode 100644 index 0000000..b461530 --- /dev/null +++ b/.planning/phases/07.4-security-token-fingerprinting-token-binding-inserted/07.4-REVIEW.md @@ -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_