---
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_