kite/.planning/phases/07.4-security-token-fingerprinting-token-binding-inserted/07.4-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
07.4-security-token-fingerprinting-token-binding-inserted	2026-06-06T00:00:00Z	standard	11	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	critical warning info total 3 3 2 8	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:

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:

# 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:

# 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:

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:

# 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:

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:

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

# 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:

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:

# 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:

@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