fix(oidc): validate ID tokens + PKCE/nonce, stop email auto-link, tokens off URL (OC1/OC2/OH4/OH5)#57
Open
PenguinzTech wants to merge 2 commits into
Open
fix(oidc): validate ID tokens + PKCE/nonce, stop email auto-link, tokens off URL (OC1/OC2/OH4/OH5)#57PenguinzTech wants to merge 2 commits into
PenguinzTech wants to merge 2 commits into
Conversation
…tokens off URL Fixes account-takeover and token-leak findings in the OIDC/SSO consumer: - OC1: callback now validates the id_token (JWKS signature, iss/aud/exp/nonce) instead of blindly trusting the userinfo response; identity derives from validated claims. Adds PKCE (S256) + nonce to authorize/callback. - OC2: auto-link/auto-provision gated on email_verified (+ OIDC_AUTO_LINK_ VERIFIED_EMAIL flag, default off); no more silent email-based account linking. - OH4: access/refresh tokens returned as httpOnly Secure SameSite cookies, not in the redirect URL query string. - OH5: identity linking only via authenticated OIDC round-trip; removed body-supplied external_sub/external_email linking. Adds nullable columns oidc_providers.issuer, user_oidc_links.email_verified. _encrypt_secret fail-open and update_provider gating deferred to A5. Full backend suite: 403 passed, 1 skipped, 90.16% coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideHardens the OIDC flow by adding PKCE + nonce, full ID token validation via JWKS, secure cookie delivery of tokens, and a proper authenticated OIDC-based identity linking flow, along with minimal schema/config wiring to support issuer and email_verified tracking. Sequence diagram for hardened OIDC authorize/callback and link flowsequenceDiagram
actor User
participant Browser
participant oidc_bp
participant OIDCProvider
participant JWKS_Endpoint
User ->> Browser: Open /oidc/link/{provider_slug} (authenticated)
Browser ->> oidc_bp: GET /oidc/link/{provider_slug}
oidc_bp ->> oidc_bp: initiate_link_identity
oidc_bp ->> oidc_bp: _generate_pkce_pair
oidc_bp ->> oidc_bp: _generate_nonce
oidc_bp ->> Browser: redirect to OIDC authorization_endpoint
User ->> Browser: Approve OIDC login
OIDCProvider ->> Browser: redirect to /oidc/{slug}/callback?code&state
Browser ->> oidc_bp: GET /oidc/{slug}/callback
oidc_bp ->> oidc_bp: callback
oidc_bp ->> oidc_bp: validate state
oidc_bp ->> oidc_bp: read oidc_code_verifier, oidc_nonce
oidc_bp ->> OIDCProvider: AsyncOAuth2Client.fetch_token
oidc_bp ->> OIDCProvider: GET discovery_url
OIDCProvider -->> oidc_bp: token_endpoint, jwks_uri, issuer
oidc_bp ->> OIDCProvider: fetch_token (code, code_verifier)
OIDCProvider -->> oidc_bp: id_token
oidc_bp ->> JWKS_Endpoint: _fetch_jwks(jwks_uri)
JWKS_Endpoint -->> oidc_bp: JWKS keys
oidc_bp ->> oidc_bp: _validate_id_token(id_token, keys, client_id, issuer, nonce)
alt link_mode set in session
oidc_bp ->> oidc_bp: create user_oidc_links row (email_verified)
oidc_bp ->> Browser: redirect /account?linked=ok
else standard login
oidc_bp ->> oidc_bp: find user_oidc_links or get_user_by_email
oidc_bp ->> oidc_bp: create_user if needed and email_verified
oidc_bp ->> oidc_bp: create_access_token
oidc_bp ->> oidc_bp: create_refresh_token
oidc_bp ->> Browser: redirect /login?sso=ok with httpOnly cookies
end
Entity relationship diagram for new OIDC issuer and email_verified fieldserDiagram
USERS {
int id
}
OIDC_PROVIDERS {
int id
string issuer
}
USER_OIDC_LINKS {
int id
int user_id
int provider_id
boolean email_verified
}
USERS ||--o{ USER_OIDC_LINKS : has
OIDC_PROVIDERS ||--o{ USER_OIDC_LINKS : links
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
_fetch_jwksreturn type annotation (dict[str, Any]) doesn’t match its actual behavior (it consistently returns a list of key dicts), which can confuse callers and type checkers; consider updating the annotation and docstring tolist[dict[str, Any]](and removing the unused global_jwks_cachetype mention ofkeysas a dict). - In
_validate_id_token, the base64 padding logic for the header (header_b64 += '=' * (4 - len(header_b64) % 4)) will incorrectly add four '=' characters when the length is already a multiple of 4; guard this with a conditional so padding is only added whenlen(header_b64) % 4 != 0. - The new
set_cookie(..., secure=True, ...)on the OIDC callback response may break local HTTP development; consider deriving thesecureflag from configuration or environment (e.g., only enabling it when running behind HTTPS in production).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_fetch_jwks` return type annotation (`dict[str, Any]`) doesn’t match its actual behavior (it consistently returns a list of key dicts), which can confuse callers and type checkers; consider updating the annotation and docstring to `list[dict[str, Any]]` (and removing the unused global `_jwks_cache` type mention of `keys` as a dict).
- In `_validate_id_token`, the base64 padding logic for the header (`header_b64 += '=' * (4 - len(header_b64) % 4)`) will incorrectly add four '=' characters when the length is already a multiple of 4; guard this with a conditional so padding is only added when `len(header_b64) % 4 != 0`.
- The new `set_cookie(..., secure=True, ...)` on the OIDC callback response may break local HTTP development; consider deriving the `secure` flag from configuration or environment (e.g., only enabling it when running behind HTTPS in production).
## Individual Comments
### Comment 1
<location path="services/flask-backend/app/oidc.py" line_range="214-223" />
<code_context>
+# --- JWKS & ID Token Validation ---
+
+
+async def _fetch_jwks(provider_id: int, jwks_uri: str) -> dict[str, Any]:
+ """
+ Fetch and cache JWKS from provider.
+
+ Returns: {"keys": [...]}
+ """
+ global _jwks_cache
+
+ # Check cache
+ if provider_id in _jwks_cache:
+ cached = _jwks_cache[provider_id]
+ if time.time() - cached["fetched_at"] < _JWKS_TTL_SECONDS:
+ return cached["keys"]
+
+ # Fetch fresh
+ import httpx
+
+ try:
+ async with httpx.AsyncClient() as http:
+ resp = await http.get(jwks_uri, timeout=10)
+ data = resp.json()
+ _jwks_cache[provider_id] = {
+ "fetched_at": time.time(),
+ "keys": data.get("keys", []),
+ }
+ return data.get("keys", [])
+ except Exception as e:
+ logger.error(f"JWKS fetch failed for provider {provider_id}: {e}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Return typing/docstring for _fetch_jwks don’t match actual behaviour and HTTP errors aren’t checked
The function returns a list (`data.get("keys", [])`), but the annotation/docstring claim it returns a dict. Since callers treat `jwks_keys` as a list, update the return type and docstring to `list[dict[str, Any]]` for consistency.
We also call `resp.json()` without checking `resp.status_code` or handling invalid JSON. This can produce confusing errors and cache bad responses. Please check for a successful status (e.g. `2xx`), handle JSON errors, and avoid updating the cache on failure.
</issue_to_address>
### Comment 2
<location path="services/flask-backend/app/oidc.py" line_range="269" />
<code_context>
+ import json
+
+ header_b64 = id_token.split(".")[0]
+ header_b64 += "=" * (4 - len(header_b64) % 4)
+ header = json.loads(base64.urlsafe_b64decode(header_b64))
+ except Exception as e:
</code_context>
<issue_to_address>
**issue (bug_risk):** Base64 padding logic can over-pad when the length is already aligned
If `len(header_b64) % 4 == 0`, this will still append four `=` characters and produce an invalid base64 string. Instead, compute the required padding with:
```python
pad_len = (-len(header_b64)) % 4
header_b64 += "=" * pad_len
```
so that already correctly padded values are left unchanged.
</issue_to_address>
### Comment 3
<location path="services/flask-backend/app/oidc.py" line_range="290-293" />
<code_context>
+
+ # Verify signature and decode
+ try:
+ jws = JsonWebSignature()
+ jwk = JsonWebKey.import_key(jwks_key_data)
+ claims = jws.deserialize_compact(id_token, jwk)
+ claims_dict = json.loads(claims)
+ except Exception as e:
+ raise ValueError(f"ID token signature verification failed: {e}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Authlib JWS deserialization result is likely misused, leading to incorrect claim parsing
`JsonWebSignature.deserialize_compact` returns a dict like `{"header": ..., "payload": b"..."}`, not a JSON string. Using that value directly in `json.loads(claims)` is incorrect and will error or mis-parse. Instead, extract the payload and decode it:
```python
result = jws.deserialize_compact(id_token, jwk)
payload = result["payload"] # bytes
claims_dict = json.loads(payload)
```
Alternatively, consider using an authlib JWT helper that handles both verification and decoding in one step.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+214
to
+223
| async def _fetch_jwks(provider_id: int, jwks_uri: str) -> dict[str, Any]: | ||
| """ | ||
| Fetch and cache JWKS from provider. | ||
|
|
||
| Returns: {"keys": [...]} | ||
| """ | ||
| global _jwks_cache | ||
|
|
||
| # Check cache | ||
| if provider_id in _jwks_cache: |
There was a problem hiding this comment.
issue (bug_risk): Return typing/docstring for _fetch_jwks don’t match actual behaviour and HTTP errors aren’t checked
The function returns a list (data.get("keys", [])), but the annotation/docstring claim it returns a dict. Since callers treat jwks_keys as a list, update the return type and docstring to list[dict[str, Any]] for consistency.
We also call resp.json() without checking resp.status_code or handling invalid JSON. This can produce confusing errors and cache bad responses. Please check for a successful status (e.g. 2xx), handle JSON errors, and avoid updating the cache on failure.
| import json | ||
|
|
||
| header_b64 = id_token.split(".")[0] | ||
| header_b64 += "=" * (4 - len(header_b64) % 4) |
There was a problem hiding this comment.
issue (bug_risk): Base64 padding logic can over-pad when the length is already aligned
If len(header_b64) % 4 == 0, this will still append four = characters and produce an invalid base64 string. Instead, compute the required padding with:
pad_len = (-len(header_b64)) % 4
header_b64 += "=" * pad_lenso that already correctly padded values are left unchanged.
Comment on lines
+290
to
+293
| jws = JsonWebSignature() | ||
| jwk = JsonWebKey.import_key(jwks_key_data) | ||
| claims = jws.deserialize_compact(id_token, jwk) | ||
| claims_dict = json.loads(claims) |
There was a problem hiding this comment.
issue (bug_risk): Authlib JWS deserialization result is likely misused, leading to incorrect claim parsing
JsonWebSignature.deserialize_compact returns a dict like {"header": ..., "payload": b"..."}, not a JSON string. Using that value directly in json.loads(claims) is incorrect and will error or mis-parse. Instead, extract the payload and decode it:
result = jws.deserialize_compact(id_token, jwk)
payload = result["payload"] # bytes
claims_dict = json.loads(payload)Alternatively, consider using an authlib JWT helper that handles both verification and decoding in one step.
Follow-up hardening on the id_token validator: JsonWebSignature was constructed with no algorithm allowlist, so it honored whatever alg the token header claimed. An attacker could forge a token with alg:HS256 and sign it using the provider's PUBLIC JWKS key as the HMAC secret (or alg:none) and have it accepted. - Restrict to asymmetric algs only: RS256/384/512, ES256/384/512, PS256/384/512. - Defensively reject the alg in the token header before verifying (blocks HS*/none). - Reject symmetric (oct) JWKS keys so a public key can't be used as an HMAC secret. - authlib 1.3.0: JsonWebSignature(algorithms=[...]); payload via JWSObject.payload. Regression tests (8/8): forged HS256-with-public-key rejected, alg:none rejected, oct key rejected, valid RS256 accepted, plus iss/exp/kid/nonce checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security fixes (Track A2) — OIDC/SSO hardening
Stacked on #56 (base =
fix/auth-secrets-config); merge that first. Ultimately targetsrelease/v0.1.x.Fixes account-takeover-class findings in
services/flask-backend/app/oidc.py:sub/email. Now validates the id_token via cached JWKS: signature,iss,aud,exp,nonce. Adds PKCE (S256) + nonce to the authorize/callback flow.email_verified(+OIDC_AUTO_LINK_VERIFIED_EMAILflag, default off). No more silent email-based linking to existing accounts.external_sub/external_email.Schema: adds nullable
oidc_providers.issuer,user_oidc_links.email_verified.Deferred to A5 (by design):
_encrypt_secretfail-open,update_providerpremium gating.Tests
Full backend suite: 403 passed, 1 skipped, 90.16% coverage. mypy --strict clean.
🤖 Generated with Claude Code
Summary by Sourcery
Harden OIDC authentication and linking flows with ID token validation, PKCE/nonce, safer auto-linking, and secure token delivery.
New Features:
Bug Fixes:
Enhancements: