From aa5b6faa2ae69dca56d66a34b1d6082d97a51a60 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 6 Aug 2026 06:14:29 -0500 Subject: [PATCH] fix(auth): audit two malformed-IdP claims.py crash paths instead of 500ing (BACKLOG #1016) --- docs/BACKLOG.md | 2 +- messagefoundry/auth/oidc/claims.py | 26 ++++++++++++++++-- tests/test_auth_oidc.py | 44 ++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 23bd8f58..522098e8 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4695,7 +4695,7 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre ## 1016. claims.py 500s on two malformed-IdP shapes with no closed-set audit row -> 🔢 **Filed 2026-08-04 — not started.** Value **5/10** · Difficulty **2/10** · _fill-in_. Two narrow, attacker-influenceable inputs raise **past** the `ClaimsError` contract, so the response is a 500 with no closed-set audit row instead of a named claim rejection. +> ✅ **Fixed 2026-08-06.** Value 5/10 · Difficulty 2/10. Both malformed-IdP shapes — a non-ASCII nonce and a list `aud` carrying an unhashable element — now reject as named, audited ClaimsErrors (nonce_mismatch / claim_aud); on first deployment either would otherwise have surfaced as a 500 with no closed-set audit row. **Cluster:** Security / authentication robustness. **Priority:** P2. **Verdict:** build (small). **Severity:** low — availability and audit completeness, not an auth bypass. Neither path admits a bad principal; both turn a rejectable token into an unclassified 500. diff --git a/messagefoundry/auth/oidc/claims.py b/messagefoundry/auth/oidc/claims.py index a9b31180..3e195652 100644 --- a/messagefoundry/auth/oidc/claims.py +++ b/messagefoundry/auth/oidc/claims.py @@ -181,6 +181,20 @@ def _verify_signature(id_token: str, key: object, policy: OidcClaimPolicy) -> Ma raise ClaimsError("malformed_payload", str(exc)) from exc +def _nonce_matches(received: str, expected: str) -> bool: + """Constant-time nonce comparison — mirrors ``flow.state_matches`` at the encoding boundary. + + ``hmac.compare_digest`` raises ``TypeError`` on a ``str`` carrying non-ASCII, so comparing the + token nonce directly turns a hostile ``nonce`` like ``n-éé`` into an unhandled 500 that skips the + audited ``nonce_mismatch`` branch. The flow nonce we minted is base64url, so a non-ASCII token + nonce cannot match anyway — this is a plain non-match, not a special case. + """ + try: + return hmac.compare_digest(received.encode("ascii"), expected.encode("ascii")) + except UnicodeEncodeError: + return False + + def _check_core_claims( claims: Mapping[str, object], policy: OidcClaimPolicy, now: float ) -> tuple[float, str]: @@ -205,7 +219,15 @@ def _check_core_claims( raise ClaimsError("claim_iss", "iss does not match the pinned issuer") aud = claims.get("aud") - audiences = {aud} if isinstance(aud, str) else set(aud) if isinstance(aud, list) else set() + # A ``list`` aud whose elements are unhashable (a list-of-lists, or a list carrying a dict) + # raises ``TypeError`` from ``set(aud)`` — the one malformed shape that escapes past the fall- + # through to an empty set. Convert it into the same audited ``claim_aud`` rejection the + # membership check below emits, rather than a 500 with no closed-set audit row. The body is kept + # to the single set-building expression so the only TypeError caught is the one from that call. + try: + audiences = {aud} if isinstance(aud, str) else set(aud) if isinstance(aud, list) else set() + except TypeError as exc: + raise ClaimsError("claim_aud", "aud is malformed") from exc if policy.client_id not in audiences: raise ClaimsError("claim_aud", "aud does not contain the client_id") # With multiple audiences, azp MUST be present and equal to our client_id (OIDC core 3.1.3.7/2). @@ -229,7 +251,7 @@ def _check_core_claims( raise ClaimsError("not_yet_valid", "id_token nbf is in the future") token_nonce = claims.get("nonce") - if not isinstance(token_nonce, str) or not hmac.compare_digest(token_nonce, policy.nonce): + if not isinstance(token_nonce, str) or not _nonce_matches(token_nonce, policy.nonce): raise ClaimsError("nonce_mismatch", "id_token nonce does not match the flow nonce") # ``sub`` is REQUIRED of an id_token by OIDC Core 2 and is the only stable identifier the diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py index 4447d313..bcef34b0 100644 --- a/tests/test_auth_oidc.py +++ b/tests/test_auth_oidc.py @@ -309,6 +309,50 @@ def test_multi_aud_with_matching_azp_accepted(rsa_key: rsa.RSAPrivateKey) -> Non assert principal.username == "jdoe" +# --- claims: two malformed-IdP shapes reject as audited ClaimsErrors, not 500s (BACKLOG #1016) ----- + + +def test_a_non_ascii_nonce_is_a_mismatch_not_a_crash(rsa_key: rsa.RSAPrivateKey) -> None: + """A non-ASCII token nonce must reject as an audited ``nonce_mismatch``, not an unhandled 500. + ``hmac.compare_digest`` raises ``TypeError`` on a str carrying non-ASCII, so without the encoding + guard a hostile ``nonce`` would, on first deployment, surface as a 500 that skips the closed-set + audit row every other claim rejection emits. The policy nonce is ASCII ``n-123``.""" + jws = _mint(rsa_key, "k1", _good_claims(nonce="n-éé")) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == "nonce_mismatch" + assert exc.value.reason in oidc.REASONS + + +@pytest.mark.parametrize("aud", [[["mefor-console"]], [{"a": 1}]]) +def test_an_unhashable_aud_element_is_claim_aud_not_a_crash( + rsa_key: rsa.RSAPrivateKey, aud: Any +) -> None: + """A ``list`` aud carrying an unhashable element (a nested list, or a dict) raises ``TypeError`` + from ``set(aud)`` — the one malformed shape that escapes the fall-through to an empty set. It + must reject as an audited ``claim_aud``, not a 500. A top-level dict/int/None already falls + through cleanly, so reaching the guard proves it fired: pre-fix the path raises before the + membership check below it could run.""" + jws = _mint(rsa_key, "k1", _good_claims(aud=aud)) + with pytest.raises(oidc.ClaimsError) as exc: + oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert exc.value.reason == "claim_aud" + assert exc.value.reason in oidc.REASONS + + +def test_the_malformed_shape_guards_do_not_over_reject_a_valid_token( + rsa_key: rsa.RSAPrivateKey, +) -> None: + """Negative control (guard-the-guard): the two malformed-shape guards must not reject an ordinary + valid token — an ASCII nonce equal to the policy nonce and a plain str aud validate byte- + identically. The existing ``{"nonce": "wrong"}`` rung already proves an ASCII wrong-nonce still + routes to ``nonce_mismatch``, so the encoding guard does not swallow real mismatches.""" + jws = _mint(rsa_key, "k1", _good_claims()) + principal = oidc.validate_id_token(jws, _policy(), _cache_for(rsa_key), clock=lambda: 1_000_100) + assert principal.username == "jdoe" + assert principal.subject == "S-1-5-21-abc" + + def test_mfa_gate_off_accepts_a_password_only_token(rsa_key: rsa.RSAPrivateKey) -> None: jws = _mint(rsa_key, "k1", _good_claims(amr=["pwd"])) principal = oidc.validate_id_token(