fix: close fail-open paths found by the security analysis

CHANGELOG.md

@@ -115,7 +115,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 - **Retries are keyed to whether a repeat is safe.** `RetryPolicy` distinguishes an operation that can be repeated from one that cannot, and `HttpTransportError::may_have_reached_peer` answers the question a caller actually needs. A JWKS `GET` retries freely; a token exchange and a CIBA dispatch retry only failures that provably never reached the peer, because a timeout cannot tell "never arrived" from "the reply was lost" and repeating either would mint a second credential or ask a human twice.
 
-- **`delegation.egress_denied` / `elicitation.egress_denied`.** New deny codes for the case where the host refuses a call before it leaves the process — an egress policy, an SSRF guard, an open circuit. Kept distinct from `idp_unreachable` on purpose: "we declined to try" and "we tried and failed" send an operator to different places, and collapsing them turns a blocked destination into a phantom network problem. No behaviour changes until a host transport produces the refusal; the bundled hyper transport never does.
+- **`delegation.egress_denied` / `elicitation.egress_denied`.** New deny codes for the case where the host refuses a call before it leaves the process — an egress policy, an SSRF guard, an open circuit. Kept distinct from `idp_unreachable` on purpose: "we declined to try" and "we tried and failed" send an operator to different places, and collapsing them turns a blocked destination into a phantom network problem. The bundled hyper transport produces the refusal for destinations in the shared address table.
 
 - **A shared table of addresses an outbound call must not reach.** `praxis_policy_core::http_addr` covers loopback, RFC 1918, link-local (the cloud-metadata range), CGNAT `100.64/10`, the IPv6 equivalents, and the embedded-IPv4 forms including NAT64. The table only; `praxis-policy-core` opens no sockets, so a transport enforces it where it dials. Sharing it stops three transports each writing a range list that drifts, and these are exactly the ranges that look finished while missing an entry.
 
@@ -596,6 +596,22 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 - **Unused items fail the build.** `dead_code` is denied workspace-wide, so a function with no caller is a compile error rather than something coverage work has to find by hand. Public host-facing API with no in-tree caller stays, marked with a reason naming who calls it from outside. ([#13](https://github.com/praxis-proxy/policy/issues/13))
 
+### Security
+
+- **A panicking `parallel:` branch is a Deny.** Dropping it let a sibling Allow stand in for a gate that never finished, the same fail-open shape as pairing a Deny with Aborted. The Deny reason says fail-closed and names the panic. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
+- **The bundled hyper transport enforces the shared address table.** Loopback, RFC 1918, link-local (including cloud metadata), and CGNAT are refused at the address that would be dialled, including IP literals that never hit DNS and hostnames that resolve only to those addresses. The refusal is `HttpTransportError::Rejected` (not a connect failure), so a token-endpoint hostname on `10.x` reports `delegation.egress_denied` rather than `idp_unreachable` and is not retried. `with_allow_private_destinations` is the hatch for a local `IdP`. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
+- **Leg-2 token-exchange denials no longer forward `error_description` or the raw body.** Leg 1 already dropped those because an `IdP` may echo the submitted credential; leg 2 submits the caller's bearer as `subject_token` and had the same leak. The violation now carries the OAuth `error` code or the HTTP status. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
+- **An omitted JWT `audiences` list is refused at load.** An empty list used to disable `aud` checking, so a token minted for another app was accepted if the signature and issuer matched. A configured list also requires the token to carry an `aud` claim; omitting the claim used to pass. The hatch is `skip_audience_validation: true`, which still accepts any `aud` or none. **Breaking** for a config that listed no audiences. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
+- **`!=` on a missing attribute is true.** `subject.role != "admin": deny` did not fire when `role` was absent, because every missing comparison returned false, including `NotEq` — so it did not match `!(subject.role == "admin")` and an unauthenticated request fell through to Allow. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
+- **A non-numeric or non-finite amount fails an order comparison closed.** `"NaN"`, `"Infinity"`, and `"lots"` made `args.amount > 10000: deny` Allow, because a failed comparison was `false` and skipped the rule. IEEE is the same trap for `NaN` and `-inf`; `inf > 10000` is true, so classifying non-finite as non-numeric also stopped `"Infinity"` from matching. The phase now Denies, and `!` cannot invert that into Allow. Mixed int/float order on an integer whose magnitude exceeds 2^53 is the same Deny — including a string-encoded amount — because coercing through `f64` would collapse distinct values and skip a max-amount rule. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
+- **A handler result that cannot be read is an execution error.** Downcast failure used to be treated as Allow in both the serial and concurrent executors, so a deny the framework could not decode was dropped. `on_error: fail` now halts. ([#16](https://github.com/praxis-proxy/policy/issues/16))
+
 ### Removed
 
 - **`compile_config`, and the second `routes:` shape it defined.** Along with it

builtins/plugins/delegator-oauth/src/delegator.rs

@@ -389,11 +389,11 @@ struct TokenExchangeResponse {
 
 /// Subset of the standard OAuth error response — `error` is the
 /// machine-readable code (`invalid_grant`, `invalid_scope`, …).
+/// `error_description` is not modelled: an `IdP` may echo the submitted
+/// credential there, and serde drops unmodelled fields.
 #[derive(Debug, Deserialize)]
 struct TokenErrorResponse {
     error: String,
-    #[serde(default)]
-    error_description: Option<String>,
 }
 
 #[async_trait]
@@ -519,24 +519,16 @@ impl OAuthDelegator {
 
         let status = response.status;
         if !response.is_success() {
-            // Try to surface the standard `error` / `error_description`
-            // fields from the IdP. Fall back to status code.
             let body = String::from_utf8_lossy(&response.body).into_owned();
-            let (code, reason) = match serde_json::from_str::<TokenErrorResponse>(&body) {
-                Ok(err) => {
-                    let mut reason = err.error.clone();
-                    if let Some(desc) = err.error_description {
-                        reason.push_str(": ");
-                        reason.push_str(&desc);
-                    }
-                    ("delegation.idp_rejected", reason)
-                },
-                Err(_) => (
-                    "delegation.idp_rejected",
-                    format!("IdP returned {status}: {body}"),
-                ),
+            // Same sanitization as leg 1: the OAuth `error` CODE only,
+            // never `error_description` or the raw body. Leg 2 submits
+            // the caller's bearer as `subject_token`, and an IdP may
+            // echo that credential back in those fields.
+            let reason = match serde_json::from_str::<TokenErrorResponse>(&body) {
+                Ok(err) => format!("token exchange rejected: {}", err.error),
+                Err(_) => format!("token exchange rejected (HTTP {status})"),
             };
-            return Err(PluginViolation::new(code, reason));
+            return Err(PluginViolation::new("delegation.idp_rejected", reason));
         }
 
         let parsed = match serde_json::from_slice::<TokenExchangeResponse>(&response.body) {

builtins/plugins/delegator-oauth/tests/oauth_e2e.rs

@@ -298,7 +298,8 @@ async fn happy_path_mints_delegated_token() {
 
 /// `IdP` returns a 400 with the standard `error` / `error_description`
 /// shape — delegator surfaces `delegation.idp_rejected` carrying the
-/// `IdP`'s machine-readable code.
+/// `IdP`'s machine-readable code. The free-text description is not
+/// forwarded: an `IdP` may echo the submitted `subject_token` there.
 #[tokio::test]
 async fn idp_rejection_surfaces_error_code() {
     let http = idp(
@@ -323,8 +324,8 @@ async fn idp_rejection_surfaces_error_code() {
         violation.reason,
     );
     assert!(
-        violation.reason.contains("not active"),
-        "reason should include the error_description; got: {}",
+        !violation.reason.contains("not active"),
+        "error_description is free text and is not forwarded: {}",
         violation.reason,
     );
 }
@@ -877,20 +878,17 @@ async fn a_leg1_success_that_is_not_a_token_response_denies() {
 // Leg-2 error shapes
 // =====================================================================
 
-/// A leg-2 rejection carrying `error_description` surfaces both the code and
-/// the description.
-///
-/// Note the asymmetry with leg 1, which deliberately drops the description
-/// because an `IdP` may echo the submitted credential back in it. Leg 2 submits
-/// the caller's bearer token as `subject_token`, so the same echo is possible
-/// here. This test records what the code does today rather than endorsing it.
+/// A leg-2 rejection carrying `error_description` must not surface the
+/// description. Leg 2 submits the caller's bearer as `subject_token`, and
+/// an `IdP` may echo that credential back in the description — the same
+/// reason leg 1 drops it.
 #[tokio::test]
-async fn a_leg2_rejection_surfaces_the_error_description() {
+async fn a_leg2_rejection_does_not_leak_error_description() {
     let http = idp(
         400,
         &json!({
             "error": "invalid_scope",
-            "error_description": "read:compensation is not granted to this client",
+            "error_description": "subject_token eyJhbGciOiJnone.echoed.token is not granted",
         })
         .to_string(),
     );
@@ -911,17 +909,24 @@ async fn a_leg2_rejection_surfaces_the_error_description() {
         violation.reason
     );
     assert!(
-        violation.reason.contains("not granted to this client"),
-        "and today the description is appended to it: {}",
+        !violation.reason.contains("eyJhbGciOiJnone"),
+        "an IdP that echoes the subject_token in error_description must \
+         not put it on the violation: {}",
+        violation.reason
+    );
+    assert!(
+        !violation.reason.contains("not granted"),
+        "error_description is free text and is not forwarded: {}",
         violation.reason
     );
 }
 
 /// A leg-2 rejection whose body is not OAuth error JSON falls back to the
-/// status. Without the fallback the violation would carry an empty reason.
+/// status. The raw body is not forwarded: it can echo the `subject_token`
+/// the same way `error_description` can.
 #[tokio::test]
 async fn a_leg2_rejection_with_an_unparseable_body_falls_back_to_the_status() {
-    let http = idp(500, "upstream exploded");
+    let http = idp(500, "upstream exploded; subject_token=eyJhbGciOiJnone");
 
     let violation = violation_for(
         build_payload("get_compensation", "https://hr.example.com", &[]),
@@ -934,6 +939,16 @@ async fn a_leg2_rejection_with_an_unparseable_body_falls_back_to_the_status() {
         "the status must appear when nothing else is parseable: {}",
         violation.reason
     );
+    assert!(
+        !violation.reason.contains("upstream exploded"),
+        "the raw body is not forwarded: {}",
+        violation.reason
+    );
+    assert!(
+        !violation.reason.contains("eyJhbGciOiJnone"),
+        "a body that echoes the subject_token must not land on the violation: {}",
+        violation.reason
+    );
 }
 
 /// Leg 2 answering 200 with a body that carries no `access_token`. There is no

builtins/plugins/identity-jwt/src/config.rs

@@ -149,10 +149,20 @@ pub struct TrustedIssuerConfig {
     /// Expected `iss` claim value.
     pub issuer: String,
 
-    /// Expected audience(s). Empty list disables `aud` validation.
+    /// Expected audience(s). Tokens must carry at least one matching
+    /// `aud` value. An empty list disables `aud` validation only when
+    /// [`skip_audience_validation`](Self::skip_audience_validation) is
+    /// set; omitting both is refused at load, the same class as an
+    /// empty algorithm list.
     #[serde(default)]
     pub audiences: Vec<String>,
 
+    /// Opt out of `aud` checking. Default is to require
+    /// [`audiences`](Self::audiences). Setting this together with a
+    /// non-empty audience list is refused: the two readings conflict.
+    #[serde(default)]
+    pub skip_audience_validation: bool,
+
     /// Algorithms accepted for signature verification (e.g.,
     /// `RS256`, `ES256`). At least one required.
     pub algorithms: Vec<Algorithm>,
@@ -735,15 +745,18 @@ impl std::fmt::Display for KeySourceError {
 impl std::error::Error for KeySourceError {}
 
 impl TrustedIssuerConfig {
-    /// Validate shape (non-empty issuer, at least one algorithm)
+    /// Validate shape (non-empty issuer, at least one algorithm,
+    /// audiences configured or explicitly skipped)
     /// without resolving the key. Used at construction time as a
     /// fast-fail gate so misshapen YAML is rejected before any
     /// network I/O is attempted.
     /// # Errors
     ///
-    /// Returns a message when `issuer` is empty or `algorithms` is empty. An
-    /// issuer with no accepted algorithm can verify nothing, so it is rejected
-    /// here rather than failing every token later.
+    /// Returns a message when `issuer` is empty, `algorithms` is empty, or
+    /// audience checking is neither configured nor explicitly skipped. An
+    /// issuer with no accepted algorithm can verify nothing; an issuer
+    /// with no audiences and no skip flag accepts a token minted for
+    /// any app.
     pub fn validate(&self) -> Result<(), String> {
         if self.issuer.trim().is_empty() {
             return Err("trusted_issuer.issuer must be non-empty".into());
@@ -754,6 +767,20 @@ impl TrustedIssuerConfig {
                 self.issuer
             ));
         }
+        if !self.skip_audience_validation && self.audiences.is_empty() {
+            return Err(format!(
+                "trusted_issuer '{}' must list at least one audience \
+                 (or set skip_audience_validation: true)",
+                self.issuer
+            ));
+        }
+        if self.skip_audience_validation && !self.audiences.is_empty() {
+            return Err(format!(
+                "trusted_issuer '{}' sets skip_audience_validation together \
+                 with audiences; pick one",
+                self.issuer
+            ));
+        }
         Ok(())
     }
 
@@ -781,6 +808,7 @@ impl TrustedIssuerConfig {
         Ok(TrustedIssuer {
             issuer: self.issuer,
             audiences: self.audiences,
+            skip_audience_validation: self.skip_audience_validation,
             keys: std::sync::Arc::new(std::sync::RwLock::new(keys)),
             algorithms: self.algorithms,
             leeway_seconds: self.leeway_seconds,
@@ -829,6 +857,7 @@ impl TrustedIssuerConfig {
         Ok(TrustedIssuer {
             issuer: self.issuer,
             audiences: self.audiences,
+            skip_audience_validation: self.skip_audience_validation,
             keys: std::sync::Arc::new(std::sync::RwLock::new(keys)),
             algorithms: self.algorithms,
             leeway_seconds: self.leeway_seconds,
@@ -1175,6 +1204,7 @@ mod tests {
         let cfg = TrustedIssuerConfig {
             issuer: String::new(),
             audiences: vec!["a".to_owned()],
+            skip_audience_validation: false,
             algorithms: vec![Algorithm::HS256],
             decoding_key: DecodingKeySource::Secret { secret: "s".into() },
             leeway_seconds: 0,
@@ -1184,4 +1214,42 @@ mod tests {
         };
         assert!(e.contains("issuer"), "{e}");
     }
+
+    fn issuer_for_audience_checks(audiences: Vec<String>, skip: bool) -> TrustedIssuerConfig {
+        TrustedIssuerConfig {
+            issuer: "https://idp.example".into(),
+            audiences,
+            skip_audience_validation: skip,
+            algorithms: vec![Algorithm::HS256],
+            decoding_key: DecodingKeySource::Secret { secret: "s".into() },
+            leeway_seconds: 0,
+        }
+    }
+
+    /// Omitted and explicitly empty `audiences` are the same fault when skip
+    /// is off: the issuer would accept a token minted for any app.
+    #[test]
+    fn omitted_and_empty_audiences_share_the_same_load_error() {
+        let omitted = issuer_for_audience_checks(Vec::new(), false);
+        let explicit = issuer_for_audience_checks(vec![], false);
+        let omitted_err = omitted.validate().expect_err("omitted audiences must fail");
+        let explicit_err = explicit.validate().expect_err("empty audiences must fail");
+        assert!(
+            omitted_err.contains("at least one audience"),
+            "{omitted_err}"
+        );
+        assert_eq!(
+            omitted_err, explicit_err,
+            "omitted and `audiences: []` must not produce different messages"
+        );
+    }
+
+    #[test]
+    fn skip_with_a_list_is_still_a_conflict() {
+        let err = issuer_for_audience_checks(vec!["app".into()], true)
+            .validate()
+            .expect_err("skip plus a list must fail");
+        assert!(err.contains("skip_audience_validation"), "{err}");
+        assert!(err.contains("pick one"), "{err}");
+    }
 }