Skip to content

Commit 8b01d1c

Browse files
committed
fix(auth): address code-review findings (iter 1)
Code review (max effort, 9 angles + verify + sweep) surfaced 15 substantive findings. This commit addresses the ones with concrete failure scenarios; the cosmetic/altitude items are deferred. Correctness: `claims["sub"]` raised uncaught KeyError because `claims_options` only declared `iss` and `aud` essential and `JWTClaims.validate()` doesn't enforce `sub` by default — added `sub: {"essential": True}` so a missing sub raises JoseError as documented. `body["id_token"]` raised raw KeyError and `token_response.json()` raised raw JSONDecodeError; both bypassed the documented httpx.HTTPError/JoseError failure-mode contract — wrap with explicit ValueError messages so the route layer's 400-mapping is unambiguous, and add `Accept: application/json` on the token POST so the server can't return form-urlencoded by default. `metadata[<endpoint>]` accesses raised uncaught KeyError on a non-conformant discovery doc — pre-check with `.get()` and raise ValueError with the missing field name. JWKS cache had no invalidation on signature failure — an auth-server key rotation broke logins until the verifier process restarted; now BadSignatureError invalidates the JWKS cache once and retries decode against the refreshed JWKS, mirroring the rotation-graceful behavior the module docstring promised. `_load_metadata`/`_load_jwks` race: concurrent first-callers each passed `if cache is None` and each fetched — added `asyncio.Lock`s with double-checked locking. Authorize-URL assembly used `f"{endpoint}?{urlencode}"` which produces a malformed URL with two `?`s if the endpoint already has a query string — switched to `authlib.common.urls.add_params_to_uri`, which parses and merges correctly. JWKS trust pool included every key in the JWKS regardless of `use`/`alg` — added `_signing_keys_only` filter so an encryption key or unsupported-alg key in the same JWKS can't join the signature-verification trust pool. Data quality: `claims.get("email", "")` silently produced ReviewerSession(email="") for users without an email claim — now `email: str | None` and absent claims surface as None. `claims.get("preferred_username") or claims.get("username") or None` falsy-collapsed empty strings, masking IdP misconfig — replaced with `_first_present(...)` which distinguishes absent claim (None) from explicitly-empty claim (preserved as ""). Efficiency: TimestampSigner constructed per `encode_session`/`decode_session` call rerouted HMAC key derivation on every gated request — cached via `lru_cache` keyed on (secret, salt) so the request-gating middleware doesn't pay the HMAC-init cost per request. `JsonWebToken(["RS256","ES256"])` hoisted to module scope (`_JWT`). `JsonWebKey.generate_key("RSA", 2048, ...)` test fixtures changed from function-scoped to module-scoped (was ~1-3s of pure RSA keygen per pytest run). Reuse: hand-rolled S256 PKCE replaced with `authlib.oauth2.rfc7636.create_s256_code_challenge` (authlib is already a dep, the previous implementation was the kind of crypto code we shouldn't own). Test gaps closed: SESSION_TTL expiry branch of `decode_session` (held the wall clock 12h+60s forward); token endpoint 4xx (`httpx.HTTPStatusError`) — previously only ConnectError was covered; 2xx response missing `id_token` field → ValueError; 2xx response with non-JSON body → ValueError; id_token missing `sub` claim → JoseError; JWKS rotation: BadSignatureError triggers refetch + retry, pinned by a test that flips the stub JWKS between calls. Deferred: OIDC nonce (code+PKCE flow with a locally-consumed id_token gets equivalent replay protection from the verifier-binding; added a module-docstring section explaining why); ReviewerSession dataclass vs Pydantic divergence (style; PR #70 already wraps with VerifiedBy Pydantic at the storage boundary); env-accessor consolidation (5 near-identical helpers; cosmetic).
1 parent da40914 commit 8b01d1c

2 files changed

Lines changed: 457 additions & 77 deletions

File tree

0 commit comments

Comments
 (0)