From 5aef60bee2eacd9ae62815ff285437a12d95c399 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 14:32:39 -0400 Subject: [PATCH 01/20] fix(cel): keep whole-valued floats as doubles Narrowing a whole-valued double to an int broke float arithmetic: a confidence of exactly 1.0 became int 1, so `confidence * 100.0` failed with "no such overload" and denied the maximum-confidence case while allowing lower ones. CEL already compares an int literal against a double operand, so the narrowing bought nothing. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- builtins/pdps/cel/src/activation.rs | 116 +++++++++++++++++----------- 1 file changed, 73 insertions(+), 43 deletions(-) diff --git a/builtins/pdps/cel/src/activation.rs b/builtins/pdps/cel/src/activation.rs index 9812e54..490050a 100644 --- a/builtins/pdps/cel/src/activation.rs +++ b/builtins/pdps/cel/src/activation.rs @@ -161,18 +161,21 @@ fn node_to_value(node: Node) -> Value { /// Convert one `AttributeValue` to a `cel::Value`. /// -/// CEL's type model distinguishes `int` and `double` strictly: -/// `delegation.depth <= 2` errors if `delegation.depth` is a double -/// and `2` is an int (the literal). To shield authors from that -/// asymmetry, an `f64` whose value is a whole number and fits a `i64` -/// is yielded as `Value::Int`. The same logic applies to the -/// author-supplied yaml args (see `yaml_to_value`) — both surfaces -/// now agree. +/// An `f64` is yielded as `Value::Float`, never silently narrowed to an int. +/// CEL's `==` / `<=` / `<` and friends already compare an int literal against +/// a double operand (verified against the pinned `cel` version's ordering +/// impls and pinned by test), so `delegation.depth <= 2` works with a +/// double-valued `depth`. Narrowing a whole-valued double to an int used to be +/// done "to help literal comparison", but it broke float *arithmetic*: a +/// `confidence` of exactly `1.0` became `int 1`, and `confidence * 100.0` then +/// errored with "no such overload" (int × double) — so the maximum confidence +/// was denied while a lower one was allowed, an outcome inversion driven purely +/// by whether the value happened to be integral. fn attr_to_value(attr: &AttributeValue) -> Value { match attr { AttributeValue::Bool(b) => Value::from(*b), AttributeValue::Int(i) => Value::from(*i), - AttributeValue::Float(f) => float_to_value(*f), + AttributeValue::Float(f) => Value::from(*f), AttributeValue::String(s) => Value::from(s.clone()), // StringSet → list(string). Sort before yielding so authors // who reach for `session.labels[0]` (or any other @@ -189,34 +192,11 @@ fn attr_to_value(attr: &AttributeValue) -> Value { } } -/// Yield an `f64` as `Value::Int` when it represents a whole number -/// in `i64` range, otherwise `Value::Float`. Used by both -/// `attr_to_value` (bag scalars) and `yaml_to_value` (author args) so -/// `delegation.depth: 2` works against the literal `2` regardless of -/// whether the bag populated it as `Int(2)` or `Float(2.0)`. -#[allow( - clippy::cast_possible_truncation, - clippy::cast_precision_loss, - reason = "the conversion is guarded to finite, integral, in-range values; the \ - bound casts are deliberate and explained below" -)] -fn float_to_value(f: f64) -> Value { - // The upper bound is strict on purpose. `i64::MAX as f64` cannot represent - // 2^63 - 1 and rounds up to exactly 2^63, so `<=` against it would admit - // 2^63, which is one past the last i64 and saturates on conversion. `<` is - // then exactly the right test. `i64::MIN as f64` is exact at -2^63, so the - // lower bound stays inclusive. - if f.is_finite() && f.fract() == 0.0 && f >= i64::MIN as f64 && f < i64::MAX as f64 { - Value::from(f as i64) - } else { - Value::from(f) - } -} - /// Convert a `serde_yaml::Value` (author-supplied `cel:` args) to a -/// `cel::Value`. Numbers without a fractional part map to `Int`, otherwise -/// `Float`. Non-string mapping keys are skipped (CEL map keys here are -/// always strings for author ergonomics). +/// `cel::Value`. An integer literal maps to `Int`, a fractional one to +/// `Float`; a float is never narrowed to an int (same reasoning as +/// `attr_to_value`). Non-string mapping keys are skipped (CEL map keys here +/// are always strings for author ergonomics). fn yaml_to_value(v: &serde_yaml::Value) -> Value { match v { serde_yaml::Value::Null => Value::Null, @@ -225,7 +205,7 @@ fn yaml_to_value(v: &serde_yaml::Value) -> Value { if let Some(i) = n.as_i64() { Value::from(i) } else { - float_to_value(n.as_f64().unwrap_or(f64::NAN)) + Value::from(n.as_f64().unwrap_or(f64::NAN)) } }, serde_yaml::Value::String(s) => Value::from(s.clone()), @@ -304,23 +284,73 @@ mod tests { assert!(truthy("session.labels.exists(l, l == 'PII')", &bag)); } - /// An `f64` whose value is a whole number is yielded as an int so - /// authors can compare against integer literals without CEL's - /// strict int-vs-double type rules blowing up. A genuinely - /// fractional `f64` still arrives as a float (so `confidence > 0.9` - /// behaves correctly). + /// A double-valued bag scalar compares correctly against an integer + /// literal without being narrowed to an int — CEL's ordering handles the + /// mixed comparison. Narrowing is deliberately not done because it breaks + /// float arithmetic (see `whole_valued_float_keeps_arithmetic`). #[test] - fn whole_number_float_arrives_as_int_for_literal_compare() { + fn double_scalar_compares_against_int_literal() { let mut bag = AttributeBag::new(); bag.set("delegation.depth", 2.0_f64); bag.set("intent.confidence", 0.92_f64); - // Compare-with-int-literal: requires the bag value to be int. assert!(truthy("delegation.depth == 2", &bag)); assert!(truthy("delegation.depth <= 2", &bag)); // Genuine doubles still compare to double literals. assert!(truthy("intent.confidence > 0.9", &bag)); } + /// Regression: a whole-valued double (`1.0`) must stay a double so that + /// float arithmetic on it still resolves. Narrowing it to `int 1` made + /// `confidence * 100.0` fail with "no such overload" and denied the + /// maximum-confidence case while allowing lower ones. + #[test] + fn whole_valued_float_keeps_arithmetic() { + let mut bag = AttributeBag::new(); + bag.set("intent.confidence", 1.0_f64); + assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag)); + // And the sub-1.0 case is unchanged. + bag.set("intent.confidence", 0.92_f64); + assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag)); + } + + /// The float-stays-float change relies on CEL comparing an int literal + /// against a double operand in *either* operand order. The existing tests + /// only cover `depth 2`; pin the reversed `2 depth` form too, so a + /// one-sided comparison impl can't silently break the half of author + /// policies that write the literal on the left. + #[test] + fn mixed_int_float_comparison_is_order_independent() { + let mut bag = AttributeBag::new(); + bag.set("delegation.depth", 2.0_f64); + assert!(truthy("2 == delegation.depth", &bag)); + assert!(truthy("2 <= delegation.depth", &bag)); + assert!(truthy("2 >= delegation.depth", &bag)); + assert!(truthy("3 > delegation.depth", &bag)); + assert!(truthy("1 < delegation.depth", &bag)); + // A true inequality must resolve to a value, not a "no such overload" + // error; asserted via its positive form so an error can't pass as false. + assert!(truthy("3 != delegation.depth", &bag)); + } + + #[test] + fn whole_valued_float_in_int_list_matches() { + // Guard the float-stays-float change against the `in` operator: a + // whole-valued double must still be found in an int list (CEL's `in` + // uses cross-type equality), so membership doesn't invert on int-vs- + // double the way the old narrowing avoided for `==` but broke for `*`. + let mut bag = AttributeBag::new(); + bag.set("delegation.depth", 2.0_f64); + assert!( + truthy("delegation.depth in [1, 2, 3]", &bag), + "float 2.0 in int list" + ); + bag.set("intent.confidence", 1.0_f64); + assert!( + truthy("intent.confidence in [0.5, 1.0]", &bag), + "float in float list" + ); + } + /// `StringSet` is yielded in sorted order so indexing returns a /// stable value across runs. `"compensation" < "PII"` (ASCII; /// uppercase letters sort before lowercase, but both labels here From 57857fc78bf0c23cd907135b3df7df159a791695 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 14:44:55 -0400 Subject: [PATCH 02/20] fix(opa): reject inline modules sharing a global package subtree The collision check was exact-match, so an inline `package authz.exceptions` slipped past a global `package authz` and still fed the `data.authz.*` subtree that the global rule reads. The check is now prefix-aware on a path boundary, so `data.authz` still does not collide with `data.authznext`. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- builtins/pdps/opa/src/resolver.rs | 94 ++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/builtins/pdps/opa/src/resolver.rs b/builtins/pdps/opa/src/resolver.rs index 1fa27db..239f2e5 100644 --- a/builtins/pdps/opa/src/resolver.rs +++ b/builtins/pdps/opa/src/resolver.rs @@ -313,7 +313,19 @@ impl OpaResolver { // Reject an inline module that lands in a global module's package — it // would merge into (and could override) operator policy. Fail-closed: // inline modules may add new packages, never redefine a global one. - if self.global_packages.contains(&package) { + // + // The check is prefix-aware, not exact-match: package paths are dotted + // (`data.authz`), and a global rule reads whole subtrees, so an inline + // module in a *sub-package* of a global one (`data.authz.exceptions` + // under `data.authz`) still feeds `data.authz.*` that a global + // `authz` rule can consume — an override by the back door. Reject when + // the inline package equals, is nested under, or contains any global + // package, so the two never share a `data` subtree. + if self + .global_packages + .iter() + .any(|g| packages_share_subtree(&package, g)) + { return Err(EngineError::PackageCollision(package)); } @@ -385,6 +397,17 @@ impl OpaResolver { } } +/// True when two dotted Rego package paths occupy the same `data` subtree — +/// they are equal, or one is nested under the other (`data.authz` and +/// `data.authz.exceptions`). A bare prefix comparison is wrong: `data.authz` +/// must not be judged to contain `data.authznext`, so the boundary is only a +/// match when the next character is a path separator. +fn packages_share_subtree(a: &str, b: &str) -> bool { + a == b + || a.strip_prefix(b).is_some_and(|rest| rest.starts_with('.')) + || b.strip_prefix(a).is_some_and(|rest| rest.starts_with('.')) +} + /// Internal — failure shapes from preparing a per-step engine. All three /// always deny regardless of `on_error`: a compile error is an author bug, a /// package collision is a trust-boundary violation, and a cache-full condition @@ -965,6 +988,30 @@ msg := "not a decision" } } + /// An inline module in a *sub-package* of a global package is rejected + /// fail-closed. A global `authz` rule reads whole `data.authz.*` subtrees, + /// so an inline `package authz.exceptions` still feeds operator policy even + /// though it never names `package authz` directly — the back-door override + /// the exact-match check used to miss. + #[tokio::test] + async fn inline_module_cannot_override_global_subpackage() { + // Global policy allows only when the caller is listed under + // `data.authz.exceptions` — a subtree an inline module must not reach. + let global = "package authz\ndefault allow := false\nallow if data.authz.exceptions[input.subject.id]\n"; + let r = resolver(&[global], OnError::Allow); + let inline = "package authz.exceptions\nexceptions := {\"eve\": true}\n"; + let out = r + .evaluate(&call("data.authz.allow", Some(inline)), &bag("eve")) + .await + .unwrap(); + match out.decision { + Decision::Deny { reason, .. } => { + assert!(reason.unwrap_or_default().contains("collides")); + }, + other => panic!("sub-package collision must deny, got {other:?}"), + } + } + /// An inline module in a fresh package (no global collision) is accepted and /// evaluates — inline modules remain a usable feature for additive policy. #[tokio::test] @@ -978,6 +1025,51 @@ msg := "not a decision" assert_eq!(out.decision, Decision::Allow); } + /// The collision check must be prefix-*boundary* aware, not a bare string + /// prefix: a global `data.authz` must not be judged to contain + /// `data.authznext` just because one string-prefixes the other. A + /// sibling-prefix package is a genuinely separate `data` subtree and stays + /// allowed; a naive `starts_with` would wrongly reject it (fail-closed, but + /// a spurious denial of a legitimate inline module). + #[tokio::test] + async fn inline_module_in_prefix_sibling_package_is_allowed() { + let global = "package authz\nallow if input.subject.id == \"alice\"\n"; + let r = resolver(&[global], OnError::Deny); + // `authznext` shares the `authz` string prefix but is a different subtree. + let inline = "package authznext\nallow if input.subject.id == \"alice\"\n"; + let out = r + .evaluate(&call("data.authznext.allow", Some(inline)), &bag("alice")) + .await + .unwrap(); + assert_eq!( + out.decision, + Decision::Allow, + "a prefix-sibling package must not collide with a global package" + ); + } + + /// The symmetric case: an inline module whose package is a *parent* of a + /// global package (inline `data.authz`, global `data.authz.sub`) still feeds + /// a `data` subtree the operator policy reads, so it is rejected fail-closed. + /// This exercises the `b.strip_prefix(a)` arm of `packages_share_subtree`, + /// which the sub-package test (child-of-global) does not. + #[tokio::test] + async fn inline_module_that_is_parent_of_global_collides() { + let global = "package authz.sub\nallow if input.subject.id == \"alice\"\n"; + let r = resolver(&[global], OnError::Allow); + let inline = "package authz\nallow := true\n"; + let out = r + .evaluate(&call("data.authz.allow", Some(inline)), &bag("alice")) + .await + .unwrap(); + match out.decision { + Decision::Deny { reason, .. } => { + assert!(reason.unwrap_or_default().contains("collides")); + }, + other => panic!("parent-package collision must deny, got {other:?}"), + } + } + #[tokio::test] async fn missing_query_is_dispatch_error() { let r = resolver(&[ALLOW_WITH_DEFAULT], OnError::Deny); From 3b3fee38bac0df180a9b67aed1fc9953044ad411 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 14:51:22 -0400 Subject: [PATCH 03/20] fix(identity-jwt): redact secrets in Debug, accept any Bearer casing The derived Debug reached `DecodingKeySource::Secret`, so a `{:?}` of the resolver printed HMAC secrets in plaintext. For HS* those are signing keys, which is token-forgery material. KeyStore and TrustedIssuer already redact theirs; the resolver now matches. The auth scheme is also case-insensitive per RFC 9110, so a `bearer ` header no longer falls through to the parser as a malformed token. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- builtins/plugins/identity-jwt/src/resolver.rs | 58 ++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/builtins/plugins/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index a3e9daf..14ddcc0 100644 --- a/builtins/plugins/identity-jwt/src/resolver.rs +++ b/builtins/plugins/identity-jwt/src/resolver.rs @@ -100,7 +100,6 @@ const DEFAULT_LEEWAY_SECONDS: u64 = 60; /// `PluginFactory::create` trait surface across the workspace) while /// putting the network I/O on the natural async hook the host /// already drives via `PolicyEngine::initialize().await`. -#[derive(Debug)] pub struct JwtIdentityResolver { cfg: PluginConfig, /// Each issuer behind its own `Arc` so the verify path can clone @@ -130,6 +129,26 @@ pub struct JwtIdentityResolver { header: String, } +// Manual `Debug` rather than derived: `cfg` retains the raw config JSON and +// `pending_jwks` holds `DecodingKeySource` values, both of which carry HMAC +// secrets and inline PEM key material. For HS* those secrets are *signing* +// keys — token-forgery material — so a `{:?}` of the resolver must never +// print them. Only the non-secret operational fields are shown; the +// secret-carrying fields are elided. This mirrors the redacting `Debug` on +// `KeyStore` / `TrustedIssuer` and the sibling auth plugins. +impl std::fmt::Debug for JwtIdentityResolver { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("JwtIdentityResolver") + .field("name", &self.cfg.name) + .field("role", &self.role) + .field("header", &self.header) + .field("pending_jwks_count", &self.pending_jwks.len()) + .field("cfg", &"") + .field("pending_jwks", &"") + .finish_non_exhaustive() + } +} + impl JwtIdentityResolver { /// Build a resolver from a `PluginConfig`. Reads `cfg.config` /// (the plugin-specific config field — `Option`), @@ -677,7 +696,7 @@ impl HookHandler for JwtIdentityResolver { let header_lc = self.header.to_ascii_lowercase(); let header_value = payload.headers().get(header_lc.as_str()); let raw_token: String = match header_value { - Some(v) => v.strip_prefix("Bearer ").unwrap_or(v).to_owned(), + Some(v) => strip_bearer_prefix(v).to_owned(), None if !payload.raw_token().is_empty() => payload.raw_token().to_owned(), None => { return PluginResult::deny(PluginViolation::new( @@ -934,6 +953,24 @@ fn peek_issuer(token: &str) -> Option { value.get("iss")?.as_str().map(String::from) } +/// Strip a leading `Bearer` auth-scheme from a header value, if present. +/// +/// The `Bearer` scheme name is case-insensitive per RFC 9110 §11.1 (`bearer`, +/// `BEARER`, `Bearer` are all the same scheme), and the scheme is followed by +/// one or more spaces before the token. A value that does not carry the scheme +/// (a bare token) is returned unchanged, so hosts that pre-strip still work. +fn strip_bearer_prefix(value: &str) -> &str { + // Split on the first space: the scheme is everything before it. Requiring + // a space means a bare token (no scheme, e.g. host pre-stripped) and a + // token that merely starts with "bearer" both fall through untouched. + match value.split_once(' ') { + Some((scheme, after)) if scheme.eq_ignore_ascii_case("bearer") => { + after.trim_start_matches(' ') + }, + _ => value, + } +} + /// Reason `validate_token` couldn't verify the JWT. Wraps the /// usual `jsonwebtoken::errors::Error` plus the kid-selection /// and JWKS-availability cases. @@ -1124,6 +1161,23 @@ mod tests { ); } + #[test] + fn strip_bearer_prefix_is_case_insensitive() { + // Canonical casing. + assert_eq!(strip_bearer_prefix("Bearer abc.def.ghi"), "abc.def.ghi"); + // Lower / upper / mixed — RFC 9110 scheme is case-insensitive. + assert_eq!(strip_bearer_prefix("bearer abc.def.ghi"), "abc.def.ghi"); + assert_eq!(strip_bearer_prefix("BEARER abc.def.ghi"), "abc.def.ghi"); + assert_eq!(strip_bearer_prefix("BeArEr abc.def.ghi"), "abc.def.ghi"); + // Multiple spaces between scheme and token collapse away. + assert_eq!(strip_bearer_prefix("bearer abc.def.ghi"), "abc.def.ghi"); + // A bare token (host pre-stripped) passes through untouched. + assert_eq!(strip_bearer_prefix("abc.def.ghi"), "abc.def.ghi"); + // A token that merely starts with "bearer" but has no space is not + // the scheme and must not be truncated. + assert_eq!(strip_bearer_prefix("bearerish"), "bearerish"); + } + #[test] fn new_rejects_missing_config_block() { let cfg = PluginConfig { From b7ad8ff6c8ae6c33da827964b65421b1ed34052f Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:50:49 -0400 Subject: [PATCH 04/20] fix(valkey): alarm on a timed-out TTL refresh A timeout mapped to Ok(false), so the refresh-failed alarm never fired for the most likely failure under an overloaded backend. A persistently timing out EXPIRE would silently stop the sliding TTL and let session taint expire mid-session with no signal. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- builtins/session/valkey/src/store.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/builtins/session/valkey/src/store.rs b/builtins/session/valkey/src/store.rs index 962aedc..387155e 100644 --- a/builtins/session/valkey/src/store.rs +++ b/builtins/session/valkey/src/store.rs @@ -128,16 +128,21 @@ impl SessionStore for ValkeySessionStore { // closed. A persistently-failing refresh risks silent key // expiry across requests — see the operator runbook. if let Some(ttl) = self.ttl_seconds { - let refresh: Result = match tokio::time::timeout( - self.command_timeout, - conn.expire(&key, ttl_for_expire(ttl)), - ) - .await - { - Ok(res) => res, - Err(_) => Ok(false), // treat timeout as a failed refresh + // A timeout is a refresh failure too — in fact the most likely one + // under an overloaded backend — so it must raise the same alarm as + // a backend error, not be swallowed. Both map to a warn carrying + // the `session_store_ttl_refresh_failed` alarm; otherwise a + // persistently-timing-out EXPIRE would silently stop the sliding + // TTL and let session taint expire mid-session with no signal. + let refresh: Result, _> = + tokio::time::timeout(self.command_timeout, conn.expire(&key, ttl_for_expire(ttl))) + .await; + let refresh_error: Option = match refresh { + Ok(Ok(_)) => None, + Ok(Err(e)) => Some(e.to_string()), + Err(_) => Some("EXPIRE timed out".to_owned()), }; - if let Err(e) = refresh { + if let Some(e) = refresh_error { tracing::warn!( alarm = "session_store_ttl_refresh_failed", error = %e, From fc5bba0920134582e94556fdd6549ea1f84e0fb4 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:50:49 -0400 Subject: [PATCH 05/20] fix(core): compare every authority field in chain_extends The append-only check ignored authorization_details, ttl_seconds, and timestamp, so an existing hop could have its RFC 9396 grant rewritten wider or its lifetime extended and still pass as an append. DelegationHop already documents that each hop's details must be structurally narrowed. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-core/src/extensions/container.rs | 67 ++++++++++++++++++++- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/crates/ppe-core/src/extensions/container.rs b/crates/ppe-core/src/extensions/container.rs index 2e26652..8512127 100644 --- a/crates/ppe-core/src/extensions/container.rs +++ b/crates/ppe-core/src/extensions/container.rs @@ -450,9 +450,14 @@ impl Extensions { /// True when `returned` is `canonical` plus zero or more appended hops. /// -/// Hops are compared on the fields that carry authority — subject, audience, -/// granted scopes, and strategy. A rewrite of any of those on an existing hop is -/// not an append, so the whole edit is refused. +/// Every existing hop must be unchanged. The comparison covers all the fields +/// that carry authority or bound it: subject, audience, granted scopes, +/// strategy, the RFC 9396 `authorization_details` (documented as "must be +/// structurally narrowed" — a rewrite widens the grant), the `ttl_seconds` +/// lifetime (extending it is a privilege escalation), and the `timestamp` +/// (which drives age/expiry checks). A rewrite of any of these on an existing +/// hop is not an append, so the whole edit is refused. `from_cache` is merge +/// bookkeeping, not authority, so it is not compared. /// /// Public so out-of-process hosts can apply the same validation to a chain /// arriving over the wire before it reaches the merge, instead of reimplementing @@ -473,6 +478,9 @@ pub fn chain_extends( && before.audience == after.audience && before.scopes_granted == after.scopes_granted && before.strategy == after.strategy + && before.authorization_details == after.authorization_details + && before.ttl_seconds == after.ttl_seconds + && before.timestamp == after.timestamp }) } @@ -1290,6 +1298,59 @@ mod tests { ); } + #[test] + fn test_delegation_hop_authorization_details_cannot_be_widened() { + use crate::extensions::authorization::AuthorizationDetail; + use crate::extensions::delegation::DelegationHop; + + let narrow = AuthorizationDetail { + detail_type: "payment".into(), + actions: Some(vec!["initiate".into()]), + ..Default::default() + }; + let mut delegation = DelegationExtension::default(); + delegation.append_hop(DelegationHop { + subject_id: "user-1".into(), + scopes_granted: vec!["pay".into()], + authorization_details: vec![narrow.clone()], + ttl_seconds: Some(60), + ..Default::default() + }); + delegation.origin_subject_id = Some("user-1".into()); + + let mut ext = Extensions { + delegation: Some(Arc::new(delegation)), + ..Default::default() + }; + ext.delegation_write_token = Some(WriteToken::new()); + + // Rewrite the existing hop's RFC 9396 authorization_details to add + // actions (a widening) and extend its ttl — neither is an append. + let mut cow = ext.cow_copy(); + { + let hop = &mut cow.delegation.as_mut().unwrap().chain[0]; + hop.authorization_details = vec![AuthorizationDetail { + detail_type: "payment".into(), + actions: Some(vec!["initiate".into(), "refund".into(), "admin".into()]), + ..Default::default() + }]; + hop.ttl_seconds = Some(999_999); + } + + ext.merge_owned(cow); + let merged_hop = &ext.delegation.as_ref().unwrap().chain[0]; + assert_eq!( + merged_hop.authorization_details, + vec![narrow], + "an existing hop's authorization_details cannot be rewritten wider" + ); + assert_eq!( + merged_hop.ttl_seconds, + Some(60), + "an existing hop's ttl cannot be extended" + ); + } + #[test] fn test_delegation_append_is_honored_and_depth_recomputed() { use crate::extensions::delegation::DelegationHop; From bca0e8a9d843727921895ed8ce37a0bf56bfbdd6 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:50:49 -0400 Subject: [PATCH 06/20] fix(core): synthesize a deny when a blocking plugin attaches no violation The sequential phase halted only when a violation was present, so a blocking plugin that set continue_processing=false without one fell through to the modification path. The concurrent phase already synthesizes concurrent_deny for the same case. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-core/src/executor.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/crates/ppe-core/src/executor.rs b/crates/ppe-core/src/executor.rs index f5e0470..4bbd0cc 100644 --- a/crates/ppe-core/src/executor.rs +++ b/crates/ppe-core/src/executor.rs @@ -491,10 +491,21 @@ impl Executor { match result { Ok(Ok(result_box)) => { if let Some(erased) = extract_erased(result_box) { - if !erased.continue_processing - && can_block - && let Some(mut v) = erased.violation - { + if !erased.continue_processing && can_block { + // A blocking plugin that signals "do not continue" + // halts the pipeline whether or not it attached a + // violation. A missing violation is synthesized + // rather than treated as an allow — the concurrent + // phase does the same (`concurrent_deny` below), and + // letting a deny-without-reason fall through to the + // modification path would be a fail-open in the + // phase whose whole job is enforcement. + let mut v = erased.violation.unwrap_or_else(|| { + crate::error::PluginViolation::new( + "plugin_deny", + format!("Plugin '{plugin_name}' denied"), + ) + }); v.plugin_name = Some(plugin_name.to_owned()); return Some(v); } From 1589b1b93e58be87f44c7586ea36151f09b2d735 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:50:49 -0400 Subject: [PATCH 07/20] fix(apl-runtime): track http and custom writes in extensions_changed The change gate compared only security, delegation, and raw_credentials, so a route whose sole mutation was a header rewrite or a custom-extension write reported unchanged and the edit was dropped before the merge. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-apl-runtime/src/route_handler.rs | 86 ++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/crates/ppe-apl-runtime/src/route_handler.rs b/crates/ppe-apl-runtime/src/route_handler.rs index 477fb40..93393cb 100644 --- a/crates/ppe-apl-runtime/src/route_handler.rs +++ b/crates/ppe-apl-runtime/src/route_handler.rs @@ -844,7 +844,23 @@ fn extensions_changed(before: &Extensions, after: &Extensions) -> bool { (None, None) => false, _ => true, }; - security_changed || delegation_changed || raw_creds_changed + // `http` and `custom` are mutable slots too: a route plugin holding + // `write_headers` rewrites request/response headers, and a plugin may + // stash data in `custom`. Omitting them here left `modified_extensions` + // None when a plugin's *only* edit was a header write, so the executor + // never merged it and the upstream call never saw the change. (The + // `candidate_constraint` slot is handled by the force-`Some` above.) + let http_changed = match (before.http.as_ref(), after.http.as_ref()) { + (Some(a), Some(b)) => !Arc::ptr_eq(a, b), + (None, None) => false, + _ => true, + }; + let custom_changed = match (before.custom.as_ref(), after.custom.as_ref()) { + (Some(a), Some(b)) => !Arc::ptr_eq(a, b), + (None, None) => false, + _ => true, + }; + security_changed || delegation_changed || raw_creds_changed || http_changed || custom_changed } /// Extract the elicitation id an agent echoes on retry from the @@ -1157,6 +1173,33 @@ mod tests { assert!(extensions_changed(&before, &after)); } + /// A route plugin whose only edit is an HTTP header write (via + /// `write_headers`) must be detected — otherwise the header rewrite is + /// dropped at this boundary and never reaches the upstream request. + #[test] + fn an_http_change_alone_is_detected() { + use praxis_policy_core::extensions::HttpExtension; + let before = Extensions::default(); + let after = Extensions { + http: Some(Arc::new(HttpExtension::default())), + ..Extensions::default() + }; + assert!( + extensions_changed(&before, &after), + "a header write must not be mistaken for no change" + ); + } + + #[test] + fn a_custom_change_alone_is_detected() { + let before = Extensions::default(); + let after = Extensions { + custom: Some(Arc::new(std::collections::HashMap::new())), + ..Extensions::default() + }; + assert!(extensions_changed(&before, &after)); + } + /// The arm that actually runs in production, for both slots that a route can /// mutate without touching security. /// @@ -1210,5 +1253,46 @@ mod tests { ), "a replaced delegation chain must be detected" ); + + // The production arm for http/custom is `Some(a) -> Some(b)` with a + // replaced Arc (a plugin rewriting an already-present slot). The + // slot-appears tests above only hit the `None -> Some` arm; cover the + // replaced-Arc arm too so a regression there (e.g. value-equality) is + // caught. + use praxis_policy_core::extensions::HttpExtension; + let http = Arc::new(HttpExtension::default()); + let with_http = |c: &Arc| Extensions { + http: Some(Arc::clone(c)), + ..Extensions::default() + }; + assert!( + !extensions_changed(&with_http(&http), &with_http(&http)), + "the same http Arc on both sides is not a change" + ); + assert!( + extensions_changed( + &with_http(&http), + &with_http(&Arc::new(HttpExtension::default())) + ), + "a replaced http slot (a header rewrite) must be detected" + ); + + type CustomMap = std::collections::HashMap; + let custom: Arc = Arc::new(CustomMap::new()); + let with_custom = |c: &Arc| Extensions { + custom: Some(Arc::clone(c)), + ..Extensions::default() + }; + assert!( + !extensions_changed(&with_custom(&custom), &with_custom(&custom)), + "the same custom Arc on both sides is not a change" + ); + assert!( + extensions_changed( + &with_custom(&custom), + &with_custom(&Arc::new(CustomMap::new())) + ), + "a replaced custom slot must be detected" + ); } } From 205469bc7a9fb1ecda0d277732b7d9067295c6e0 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:50:49 -0400 Subject: [PATCH 08/20] fix(core): reject unknown keys in attenuation config AttenuationConfig accepted unknown fields silently, so a typo in an attenuation: block dropped that constraint and minted a broader token than the route author wrote. The invoker comment claiming unknown keys flow through to the plugin was wrong and is corrected. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- .../ppe-apl-runtime/src/delegation_invoker.rs | 79 +++++++++++++++++-- crates/ppe-core/src/delegation/payload.rs | 4 + 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/crates/ppe-apl-runtime/src/delegation_invoker.rs b/crates/ppe-apl-runtime/src/delegation_invoker.rs index 2db2487..58e108d 100644 --- a/crates/ppe-apl-runtime/src/delegation_invoker.rs +++ b/crates/ppe-apl-runtime/src/delegation_invoker.rs @@ -42,7 +42,7 @@ use chrono::SecondsFormat; use tokio::sync::Mutex; use praxis_policy_core::delegation::{ - DelegationPayload, DelegationSubject, TokenDelegateHook, + AttenuationConfig, DelegationPayload, DelegationSubject, TokenDelegateHook, payload::{AuthEnforcedBy, TargetType}, }; use praxis_policy_core::engine::PolicyEngine; @@ -113,17 +113,22 @@ impl DelegationInvoker for DelegationPluginInvoker { // Read step args first — the subject / actor role selection below // reads from them. Step `config_override` is a yaml map per the IR; - // extract a few well-known keys onto the typed DelegationPayload - // builders. Unknown keys still flow through to the plugin via the - // per-call config-override pathway (plugins consume them from their - // `cfg.config`). Recognized keys: `target` (required), `subject`, - // `actor`, `audience`, `permissions`, `target_type`, - // `auth_enforced_by`; everything else stays opaque. + // each recognized key is lifted onto the typed DelegationPayload. + // Recognized keys: `target` (required), `subject`, `actor`, + // `audience`, `permissions`, `target_type`, `auth_enforced_by`, and + // `attenuation`. Any other key is intentionally dropped — the payload + // has no untyped input channel, so an unrecognized key does not reach + // the plugin (an earlier comment here claimed otherwise; it did not). // // There is deliberately no `mode` key: the delegation mode is // *derived* from `subject` by the handler rather than declared, so a // route can't claim on-behalf-of-user while handing over a workload // SVID. + // + // `attenuation` is load-bearing: it *narrows* the minted credential's + // scope, so silently dropping it would mint a broader token than the + // author asked for — a fail-open. A malformed `attenuation:` errors + // (fail closed) rather than being ignored. let cfg = step.config_override.as_ref().and_then(|v| v.as_mapping()); // Resolve who the exchange is *for*. Defaults to the user @@ -214,6 +219,9 @@ impl DelegationInvoker for DelegationPluginInvoker { { payload = payload.with_auth_enforced_by(auth_enforced_by_from_str(enforcer)); } + if let Some(attenuation) = attenuation_from_cfg(cfg)? { + payload = payload.with_route_attenuation(attenuation); + } // Dispatch. The plan's pre-resolved entry already has any // per-route config override merged into the plugin's @@ -378,6 +386,23 @@ fn target_type_from_str(s: &str) -> TargetType { } } +/// Parse the optional `attenuation:` block into a typed `AttenuationConfig`. +/// +/// Attenuation *narrows* the minted credential's scope, so a present-but- +/// malformed block fails closed (returns `InvalidConfig`) rather than being +/// silently ignored, which would mint a broader token than the author asked +/// for. An absent block yields `None` (no narrowing requested). +fn attenuation_from_cfg( + cfg: Option<&serde_yaml::Mapping>, +) -> Result, DelegationError> { + let Some(att) = cfg.and_then(|m| m.get(serde_yaml::Value::String("attenuation".into()))) else { + return Ok(None); + }; + serde_yaml::from_value(att.clone()) + .map(Some) + .map_err(|e| DelegationError::InvalidConfig(format!("invalid `attenuation:` block: {e}"))) +} + fn auth_enforced_by_from_str(s: &str) -> AuthEnforcedBy { match s.to_ascii_lowercase().as_str() { "caller" => AuthEnforcedBy::Caller, @@ -425,6 +450,46 @@ mod tests { assert_eq!(s("subject: this_workload"), DelegationSubject::ThisWorkload); } + #[test] + fn attenuation_block_parses_onto_typed_config() { + let att = attenuation_from_cfg(Some(&cfg( + "attenuation:\n actions: [read]\n ttl_seconds: 300\n capabilities: [comp.read]", + ))) + .expect("valid attenuation parses") + .expect("attenuation present"); + assert_eq!(att.actions, vec!["read".to_owned()]); + assert_eq!(att.ttl_seconds, Some(300)); + assert_eq!(att.capabilities, vec!["comp.read".to_owned()]); + } + + #[test] + fn attenuation_absent_is_none() { + assert!( + attenuation_from_cfg(Some(&cfg("target: hr-service"))) + .unwrap() + .is_none() + ); + assert!(attenuation_from_cfg(None).unwrap().is_none()); + } + + #[test] + fn malformed_attenuation_fails_closed() { + // A present-but-malformed block must error rather than be dropped: + // dropping it would mint a broader token than the author asked for. + let err = attenuation_from_cfg(Some(&cfg("attenuation:\n ttl_seconds: not-a-number"))) + .expect_err("malformed attenuation must fail closed"); + assert!(matches!(err, DelegationError::InvalidConfig(_))); + } + + #[test] + fn attenuation_typo_key_fails_closed() { + // A misspelled key must not deserialize into an empty (no-op) config + // that silently widens the token — it is a hard error. + let err = attenuation_from_cfg(Some(&cfg("attenuation:\n actionss: [read]"))) + .expect_err("unknown attenuation key must fail closed"); + assert!(matches!(err, DelegationError::InvalidConfig(_))); + } + #[test] fn subject_absent_defaults_to_user() { // An absent `subject:` is the documented default (on-behalf-of user). diff --git a/crates/ppe-core/src/delegation/payload.rs b/crates/ppe-core/src/delegation/payload.rs index d160903..b095aa4 100644 --- a/crates/ppe-core/src/delegation/payload.rs +++ b/crates/ppe-core/src/delegation/payload.rs @@ -194,7 +194,11 @@ pub enum AuthEnforcedBy { /// minted token's scope claim. v0 doesn't include a template /// renderer — handlers receive the raw template string and render /// themselves; a framework-side renderer can come later. +// `deny_unknown_fields`: attenuation *narrows* a credential, so a misspelled +// key (`actionss:`) must not deserialize into an all-empty, no-op config that +// silently widens the minted token. An unknown key is a hard error instead. #[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct AttenuationConfig { /// Specific capabilities the route author wants granted. #[serde(default, skip_serializing_if = "Vec::is_empty")] From c9894417de6cdd06795d433e8b1de67a2010dccc Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:50:50 -0400 Subject: [PATCH 09/20] fix(core): serialize delegated_tokens with a non-string key DelegationKey is a struct, so the delegated_tokens map could not round trip through serde_json. Any serialization carrying a minted token failed. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- .../src/extensions/raw_credentials.rs | 130 +++++++++++++++++- 1 file changed, 129 insertions(+), 1 deletion(-) diff --git a/crates/ppe-core/src/extensions/raw_credentials.rs b/crates/ppe-core/src/extensions/raw_credentials.rs index 8115230..3423880 100644 --- a/crates/ppe-core/src/extensions/raw_credentials.rs +++ b/crates/ppe-core/src/extensions/raw_credentials.rs @@ -434,10 +434,81 @@ pub struct RawCredentialsExtension { /// handlers and cached for re-use. Read with /// `read_delegated_tokens`; write with `write_delegated_tokens` /// (`TokenDelegate` handlers only). - #[serde(default)] + /// + /// Serialized as a sequence of `[key, value]` pairs, not a JSON object: + /// the key is the `DelegationKey` struct, and JSON object keys must be + /// strings, so a plain map serialization errors at runtime the moment a + /// token is minted — which would break the documented `extensions` wire + /// channel, audit dumps, and hot-reload snapshots (the very paths the + /// module's serialization-safety contract promises work). The pair-seq + /// form serializes cleanly and round-trips; the token bytes inside each + /// value stay `#[serde(skip)]`. + #[serde(default, with = "delegated_tokens_as_pairs")] pub delegated_tokens: HashMap, } +/// serde adapter: represent `delegated_tokens` as a sequence of +/// `(DelegationKey, RawDelegatedToken)` pairs so a non-string map key +/// serializes to JSON. See the field doc for why a plain map cannot. +/// +/// Deserialization is tolerant of *both* the pair-sequence form and a plain +/// map. Before this adapter, only an empty `delegated_tokens` ever serialized, +/// and it did so as a JSON object (`{}`); a snapshot or wire message written by +/// that older code must still load, so a map input is accepted too (its entries, +/// if any, having string-shaped keys from a hand-built or empty document). This +/// keeps mixed-version rollouts and old persisted snapshots readable in both +/// directions. +mod delegated_tokens_as_pairs { + use super::{DelegationKey, RawDelegatedToken}; + use serde::de::{MapAccess, SeqAccess, Visitor}; + use serde::{Deserializer, Serialize as _, Serializer}; + use std::collections::HashMap; + + pub(super) fn serialize( + map: &HashMap, + serializer: S, + ) -> Result { + let pairs: Vec<(&DelegationKey, &RawDelegatedToken)> = map.iter().collect(); + pairs.serialize(serializer) + } + + struct MapOrPairs; + + impl<'de> Visitor<'de> for MapOrPairs { + type Value = HashMap; + + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a sequence of [key, value] pairs or a map") + } + + fn visit_seq>(self, mut seq: A) -> Result { + let mut map = HashMap::with_capacity(seq.size_hint().unwrap_or(0)); + while let Some((k, v)) = seq.next_element::<(DelegationKey, RawDelegatedToken)>()? { + map.insert(k, v); + } + Ok(map) + } + + fn visit_map>(self, mut access: A) -> Result { + // Legacy shape: a JSON object. Only the empty case was ever + // emitted (a non-empty struct-keyed map could not serialize), so + // this is normally `{}`; still, drain any entries a hand-built + // document carries. + let mut map = HashMap::with_capacity(access.size_hint().unwrap_or(0)); + while let Some((k, v)) = access.next_entry::()? { + map.insert(k, v); + } + Ok(map) + } + } + + pub(super) fn deserialize<'de, D: Deserializer<'de>>( + deserializer: D, + ) -> Result, D::Error> { + deserializer.deserialize_any(MapOrPairs) + } +} + #[cfg(test)] #[allow( clippy::expect_used, @@ -610,4 +681,61 @@ mod tests { assert_eq!(&*restored_tok.token, ""); assert_eq!(restored_tok.source_header, "X-User-Token"); } + + #[test] + fn delegated_tokens_serialize_without_error_and_round_trip() { + // Regression: `delegated_tokens` is keyed by the `DelegationKey` + // struct, which cannot be a JSON object key. A plain map serialization + // errored the moment a token was minted, breaking the documented wire + // channel. The pair-seq form must serialize cleanly, drop the token + // bytes, and round-trip the key + metadata. + let mut ext = RawCredentialsExtension::default(); + let key = DelegationKey::new( + DelegationMode::OnBehalfOfUser, + "workday-api", + vec!["read:comp".to_owned()], + ) + .with_subject_id("user-1"); + ext.delegated_tokens.insert( + key.clone(), + RawDelegatedToken::new( + "minted-secret", + "Authorization", + "workday-api", + vec!["read:comp".to_owned()], + chrono::Utc::now(), + ), + ); + + // The whole point: this used to return Err("key must be a string"). + let json = serde_json::to_string(&ext).expect("delegated_tokens must serialize"); + assert!( + !json.contains("minted-secret"), + "token bytes must be dropped" + ); + + let restored: RawCredentialsExtension = serde_json::from_str(&json).unwrap(); + let restored_tok = restored + .delegated_tokens + .get(&key) + .expect("the key must round-trip"); + assert_eq!(&*restored_tok.token, "", "token stays skipped"); + assert_eq!(restored_tok.audience, "workday-api"); + } + + #[test] + fn legacy_empty_map_delegated_tokens_still_deserializes() { + // Before the pair-seq adapter, only an empty delegated_tokens ever + // serialized, and it did so as a JSON object `{}`. A snapshot written + // by that older code must still load — the deserializer accepts a map + // as well as the new sequence form. + let legacy = r#"{"inbound_tokens":{},"delegated_tokens":{}}"#; + let restored: RawCredentialsExtension = serde_json::from_str(legacy).unwrap(); + assert!(restored.delegated_tokens.is_empty()); + + // And the new sequence form loads too. + let modern = r#"{"inbound_tokens":{},"delegated_tokens":[]}"#; + let restored: RawCredentialsExtension = serde_json::from_str(modern).unwrap(); + assert!(restored.delegated_tokens.is_empty()); + } } From 9a9c3ca7fdbb38a09d67e0fcfbcddf4bcef6825e Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:52:24 -0400 Subject: [PATCH 10/20] fix(core): stop merge_security dropping an append-only plugin's labels A plugin holding append_labels but not read_labels sees an empty filtered label set, so its returned set is never a superset of canonical and the whole edit was discarded, silently disabling write-only tainting and DLP plugins. Monotonicity is already structural here because the returned labels are folded in rather than assigned, and the laundering check that needs capability context lives in the executor's labels_ok. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-core/src/extensions/container.rs | 74 +++++++++++++++++---- 1 file changed, 61 insertions(+), 13 deletions(-) diff --git a/crates/ppe-core/src/extensions/container.rs b/crates/ppe-core/src/extensions/container.rs index 8512127..c898063 100644 --- a/crates/ppe-core/src/extensions/container.rs +++ b/crates/ppe-core/src/extensions/container.rs @@ -364,10 +364,21 @@ impl Extensions { /// Merge the `security` slot — labels only, and only as an append. /// /// Labels are the Monotonic tier: `append_labels` permits growing the set - /// and nothing else. A returned set that is not a superset of canonical is a - /// removal attempt and is dropped whole rather than partially applied — a - /// laundered declassification is the exact attack this gate exists for, and - /// removal requires a `DeclassifierToken` no plugin can construct. + /// and nothing else. Monotonicity is guaranteed *structurally* here — the + /// returned labels are folded into the canonical set, never assigned over + /// it, so a canonical label the plugin could not see (or deliberately + /// dropped) is re-added by construction and can never be removed by this + /// path. Removal requires a `DeclassifierToken` no plugin can construct. + /// + /// This does not itself judge a shrunk returned set to be a laundering + /// *attempt* and reject the whole edit: that requires knowing whether the + /// plugin was shown the canonical labels (a `read_labels` holder) or an + /// empty filtered view (an append-only plugin, whose additions are + /// legitimate even though they are not a superset of canonical). That + /// capability context lives in the executor, which drops the whole edit + /// when a `read_labels` plugin returns a non-superset set (`labels_ok`), + /// before this merge is ever reached. Re-checking it here without that + /// context silently discarded an append-only plugin's labels. /// /// Every other field on the slot is Immutable in the tier model with /// `write_cap: None` — `subject`, `auth_method`, `client`, `caller_workload`, @@ -389,11 +400,6 @@ impl Extensions { // Monotonic: fold in the additions, never assign the returned set. // Assignment would drop any canonical label the plugin could not see, // and folding makes a filtered-away label unremovable by construction. - if !owned.labels.is_superset(&merged.labels) { - // Not a superset of what the plugin was shown either — an explicit - // removal attempt. Drop the whole edit; the canonical set stands. - return; - } for label in owned.labels.iter() { merged.labels.add_label(label.clone()); } @@ -1122,7 +1128,13 @@ mod tests { } #[test] - fn test_label_removal_is_dropped_whole() { + fn test_label_removal_is_refused_by_folding() { + // A returned set that drops a canonical label cannot remove it: the + // merge folds the returned labels into the canonical set, so the + // dropped label survives (monotonicity is structural here). The + // *drop-whole* punishment for a `read_labels` laundering attempt lives + // in the executor's `labels_ok`, which has the capability context this + // low-level merge does not — see `merge_security`'s doc. let mut security = SecurityExtension::default(); security.add_label("PII"); security.add_label("HIPAA"); @@ -1143,10 +1155,46 @@ mod tests { ext.merge_owned(cow); let merged = ext.security.as_ref().unwrap(); - assert!(merged.has_label("HIPAA"), "the removal is refused"); assert!( - !merged.has_label("CLEAN"), - "and the edit is dropped whole, not partially applied" + merged.has_label("HIPAA"), + "the removal is refused by folding" + ); + assert!(merged.has_label("PII")); + } + + #[test] + fn test_append_only_plugin_labels_survive_nonempty_canonical() { + // Regression: a plugin holding `append_labels` but not `read_labels` + // sees an empty filtered label set, so its returned set contains only + // its additions — not a superset of the canonical set. The old + // superset gate in `merge_security` dropped those additions whenever + // canonical was non-empty, silently disabling a write-only tainting / + // DLP plugin. Folding must now land the addition. + let mut security = SecurityExtension::default(); + security.add_label("EXISTING"); + + let mut ext = Extensions { + security: Some(Arc::new(security)), + ..Default::default() + }; + ext.labels_write_token = Some(WriteToken::new()); + + let mut cow = ext.cow_copy(); + // Filtered view for a no-read plugin is empty; it appends its label. + let mut added = std::collections::HashSet::new(); + added.insert("PII".to_owned()); + cow.security.as_mut().unwrap().labels = crate::extensions::MonotonicSet::from_set(added); + + ext.merge_owned(cow); + + let merged = ext.security.as_ref().unwrap(); + assert!( + merged.has_label("PII"), + "the append-only plugin's label must land" + ); + assert!( + merged.has_label("EXISTING"), + "and canonical labels are preserved" ); } From f6e06e974ebd94d3fc862e6622c89c01b2ad5935 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Tue, 1 Sep 2026 15:54:58 -0400 Subject: [PATCH 11/20] fix(apl-core): fail closed when a parallel branch panics A panicking branch contributed no taints and no Halt, so a branch that would have denied was silently dropped and the parallel block continued. In a policy engine that is a fail-open, so a panic now halts the block. This reverses the earlier choice to keep sibling branches running, and drops the comment that claimed the panic was logged when the crate has no tracing dep. Ported from #54. Signed-off-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-apl-core/src/evaluator.rs | 92 ++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 13 deletions(-) diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index a5b5fbb..ec577f4 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -1093,11 +1093,15 @@ fn dispatch_parallel<'a>( // Aggregate in input order: append every branch's taints; pick // the first Halt (by branch index, not wall-clock order) as the - // overall result. Aborted / panicked branches contribute no - // taints — they didn't run to completion. A panicked branch is - // *not* converted into a Halt; we log via `tracing::warn!` and - // continue. (A misbehaving plugin shouldn't take down the - // parallel block any more than it would the host process.) + // overall result. Aborted branches contribute no taints — they + // were short-circuit cancelled. A *panicked* branch is converted + // into a fail-closed Halt: we cannot know whether the branch it + // ran would have denied, and the engine's contract everywhere + // else is that an effect which cannot complete denies rather than + // permits (a plugin `Err` halts at line 497, and the concurrent + // executor phase halts on a panicking branch under `on_error: + // fail`). Swallowing the panic here would make a guard branch's + // deny vanish — fail-open in the one phase whose job is to block. let mut first_halt: Option = None; for (idx, outcome) in outcomes.into_iter().enumerate() { match outcome { @@ -1124,14 +1128,19 @@ fn dispatch_parallel<'a>( // post-config-extension. }, BranchOutcome::Panicked(msg) => { - // A panicking branch is a misbehaving plugin/effect; - // dropping its output (no Halt, no taints) keeps the - // parallel block's other branches intact rather than - // taking the whole block down. praxis-policy-apl-core has no - // tracing dep — host integrations that care can - // surface the panic via praxis-policy-core's plugin error - // path. `idx`/`msg` are eaten here. - let _ = (idx, msg); + // A panicking branch is a misbehaving plugin/effect. It + // fails closed: a panic is a more severe failure than a + // plugin `Err` (which already halts), so it must not be + // treated more leniently. The first panic, in branch + // index order, stands in as the phase's deny if no + // earlier branch already produced a Halt. `msg` carries + // the panic payload into the audit reason. + if first_halt.is_none() { + first_halt = Some(Decision::Deny { + reason: Some(format!("parallel branch {idx} panicked: {msg}")), + rule_source: "parallel.branch_panic".to_owned(), + }); + } }, } } @@ -3120,6 +3129,21 @@ mod tests { } } + /// Invoker that panics on any plugin call — models a misbehaving plugin + /// unwinding inside a `parallel:` branch. + struct PanicPlugins; + #[async_trait] + impl PluginInvoker for PanicPlugins { + async fn invoke( + &self, + _name: &str, + _bag: &AttributeBag, + _invocation: PluginInvocation<'_>, + ) -> Result { + panic!("plugin blew up mid-branch"); + } + } + fn pdp_step(decision_diagnostic_label: &str) -> Effect { Effect::Pdp { call: PdpCall { @@ -4063,6 +4087,48 @@ mod tests { } } + #[tokio::test] + async fn parallel_panicked_branch_fails_closed() { + // A branch that panics (misbehaving plugin) must halt the phase, not + // be silently dropped — a swallowed panic would let a guard branch's + // deny vanish (fail-open). The surviving Allow branch must not rescue + // the request. + let mut bag = AttributeBag::new(); + let mut payload = crate::route::RoutePayload::new(json!({})); + + let rule = Rule { + condition: Expression::Always, + effects: vec![Effect::Parallel(vec![ + Effect::Plugin { + name: "guard".into(), + }, + Effect::Allow, + ])], + source: "test.policy[0]".into(), + }; + + let eval = evaluate_effects( + &vec![Effect::from(rule)], + &mut bag, + &(Arc::new(FakePdp { + decision: Decision::Allow, + }) as Arc), + &(Arc::new(PanicPlugins) as Arc), + &noop_delegations(), + &noop_elicitations(), + crate::step::DispatchPhase::Pre, + &mut payload, + ) + .await; + + match eval.decision { + Decision::Deny { rule_source, .. } => { + assert_eq!(rule_source, "parallel.branch_panic"); + }, + other => panic!("panicked branch must deny, got {other:?}"), + } + } + #[tokio::test] async fn parallel_picks_first_index_halt_not_first_to_complete() { // When two branches both deny, the one with the lower index From 74f9f4dcebc88335e857778995734112c7d7b086 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 15:58:11 -0400 Subject: [PATCH 12/20] fix(core): validate authentication step names at load RouteIdentityStep documents that a step name must match a top-level plugins: entry registered under identity.resolve, but nothing enforced it. An unresolvable name found no entry at dispatch and was dropped with no error, so the route ran with that authentication step missing. Global, group, and route authentication share one shape, so one check covers all three. Reworked from #54 for the renamed authentication and bundles fields. Co-authored-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-core/src/config.rs | 106 ++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/crates/ppe-core/src/config.rs b/crates/ppe-core/src/config.rs index 12f4af9..4be39b1 100644 --- a/crates/ppe-core/src/config.rs +++ b/crates/ppe-core/src/config.rs @@ -2261,6 +2261,35 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box = config.plugins.iter().map(|p| p.name.as_str()).collect(); + // Validate `authentication:` step names the same way `plugins:` names + // are. `RouteIdentityStep` documents that a step name must match a + // top-level `plugins:` entry registered under `identity.resolve`, but + // nothing enforced it: an unresolvable name finds no entry at dispatch + // and is dropped with no error, leaving that authentication step + // unrun. Global, group, and route authentication share one shape, so + // one check covers all three. + let validate_authentication = + |authentication: &Option, + context: &str| + -> Result<(), Box> { + let Some(authentication) = authentication else { + return Ok(()); + }; + for step in &authentication.steps { + if !plugin_names.contains(step.name.as_str()) { + return Err(Box::new(PluginError::Config { + message: format!( + "{context} authentication references unknown plugin '{}'", + step.name + ), + })); + } + } + Ok(()) + }; + + validate_authentication(&config.global.authentication, "global")?; + // A `global.defaults` key that names no entity type never applies to // anything, so a typo there would be silently inert rather than wrong. for entity_type in config.global.defaults.keys() { @@ -2379,6 +2408,8 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box Result<(), Box Date: Tue, 1 Sep 2026 16:04:55 -0400 Subject: [PATCH 13/20] test(apl-core): pin duplicate field-pipeline key rejection A `result:` naming the same field twice must not keep one pipeline silently, since dropping a redact for a passthrough leaks the field. Every route reaches RouteYaml through a serde_yaml::Value and serde_yaml rejects a repeated key there, so the property already holds; #54 proposed a custom deserializer for it, which the current parse path makes unnecessary. Pinned because a refactor deserializing RouteYaml from a string would be last-wins. Co-authored-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-apl-core/src/parser.rs | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/crates/ppe-apl-core/src/parser.rs b/crates/ppe-apl-core/src/parser.rs index 5b89074..2037a3e 100644 --- a/crates/ppe-apl-core/src/parser.rs +++ b/crates/ppe-apl-core/src/parser.rs @@ -4717,6 +4717,41 @@ route: assert!(msg.contains("result.x"), "expected result.x in: {msg}"); } + #[test] + fn duplicate_field_pipeline_key_is_rejected_not_last_wins() { + // A `result:` naming the same field twice must not silently keep one + // pipeline: dropping a `redact` in favour of a passthrough leaks the + // field. Every route reaches `RouteYaml` through a `serde_yaml::Value`, + // and serde_yaml rejects a repeated key in a mapping position, so the + // property holds for free today. Pinned because it would not survive a + // refactor that deserialized `RouteYaml` straight from a string: a + // plain `HashMap` field replays entries into `insert` and is last-wins. + let yaml = r#" +route: + result: + ssn: "redact" + ssn: "hash" +"#; + let err = compile_test_policy("r", yaml).expect_err("a repeated field key is not legal"); + assert!( + format!("{err}").contains("duplicate entry with key"), + "expected a duplicate-key error, got {err}" + ); + } + + #[test] + fn distinct_field_pipeline_keys_still_load() { + // The guard above must reject only repeats, not ordinary multi-field + // blocks. + let yaml = r#" +route: + result: + ssn: "redact" + email: "hash" +"#; + compile_test_policy("r", yaml).expect("distinct field keys are legal"); + } + #[test] fn removed_policy_field_names_are_rejected() { // The removed authorization-phase keys must fail loudly, never be From cc68b662022923653e75fa0ede5f8012b7e642c9 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 16:13:39 -0400 Subject: [PATCH 14/20] fix(apl-core): fan field pipelines out over arrays on the path `result.rows.ssn | redact` silently redacted nothing when `rows` was an array: the dotted helpers required every segment to be an object, so the walk fell off at the array and the pipeline never ran. A rule that reads as redacting every row's SSN passed them all through, which is a PII fail-open. Paths now expand to one concrete path per array element before the pipeline runs, and the dotted read, write, and remove helpers follow numeric segments so the expansion resolves. A terminal array is still one path, so whole-value pipelines keep their semantics. Expansion is bounded in depth and width, and an over-large shape denies rather than half-redacting the head and passing the tail through. The same fan-out applies to `do:`-embedded field ops. Reworked from #54 against the current route and evaluator shape. Co-authored-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-apl-core/src/evaluator.rs | 70 ++-- crates/ppe-apl-core/src/route.rs | 566 +++++++++++++++++++++------ 2 files changed, 497 insertions(+), 139 deletions(-) diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index ec577f4..20649e1 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -1184,6 +1184,7 @@ async fn dispatch_field_op( // Pick the right side of the payload based on the path prefix. // Out-of-phase ops drop silently (see the doc comment). + #[derive(Clone, Copy)] enum Side { Args, Result, @@ -1210,43 +1211,58 @@ async fn dispatch_field_op( }); }; - let Some(current) = get_dotted(root, subpath).cloned() else { - return EffectOutcome::Continue; // missing field → silent no-op + // Fan out over arrays on the path so a `do:`-embedded field op like + // `result.rows.ssn | redact` reaches every element's leaf instead of + // silently no-op'ing on the array. An object-only path expands to itself. + // An over-large shape fails closed rather than half-redacting. + let Some(paths) = crate::route::expand_field_paths(root, subpath) else { + return EffectOutcome::Halt(Decision::Deny { + reason: Some(format!( + "FieldOp path `{path}` expands to too many elements to redact safely" + )), + rule_source: fallback_source.to_owned(), + }); }; let pipeline = crate::pipeline::Pipeline { stages: stages.to_vec(), }; - // `subpath`, not `path`: the field name a pipeline reports to a - // plugin is relative to the args / result root, matching what the - // `args:` / `result:` section pipelines pass. The prefixed `path` - // stays in use for deny messages, where the reader wants the side - // spelled out. - let eval = evaluate_pipeline(&pipeline, ¤t, bag, plugins, subpath, phase).await; - taints.extend(eval.taints); let mark_modified = |side: Side, args: &mut bool, result: &mut bool| match side { Side::Args => *args = true, Side::Result => *result = true, }; - match eval.outcome { - FieldOutcome::Pass => EffectOutcome::Continue, - FieldOutcome::Replace(new_val) => { - if set_dotted(root, subpath, new_val) { - mark_modified(side, args_modified, result_modified); - } - EffectOutcome::Continue - }, - FieldOutcome::Omit => { - if remove_dotted(root, subpath) { - mark_modified(side, args_modified, result_modified); - } - EffectOutcome::Continue - }, - FieldOutcome::Deny { reason, .. } => EffectOutcome::Halt(Decision::Deny { - reason: Some(reason), - rule_source: fallback_source.to_owned(), - }), + for concrete in paths { + let Some(current) = get_dotted(root, &concrete).cloned() else { + continue; // missing field on this element → silent no-op + }; + // `subpath`, not `path`: the field name a pipeline reports to a + // plugin is relative to the args / result root, matching what the + // `args:` / `result:` section pipelines pass. The prefixed `path` + // stays in use for deny messages, where the reader wants the side + // spelled out. + let eval = evaluate_pipeline(&pipeline, ¤t, bag, plugins, subpath, phase).await; + taints.extend(eval.taints); + match eval.outcome { + FieldOutcome::Pass => {}, + FieldOutcome::Replace(new_val) => { + if set_dotted(root, &concrete, new_val) { + mark_modified(side, args_modified, result_modified); + } + }, + FieldOutcome::Omit => { + if remove_dotted(root, &concrete) { + mark_modified(side, args_modified, result_modified); + } + }, + FieldOutcome::Deny { reason, .. } => { + return EffectOutcome::Halt(Decision::Deny { + reason: Some(reason), + rule_source: fallback_source.to_owned(), + }); + }, + } } + EffectOutcome::Continue } /// Result of running a pipeline against one field's value. diff --git a/crates/ppe-apl-core/src/route.rs b/crates/ppe-apl-core/src/route.rs index 8413801..cac243e 100644 --- a/crates/ppe-apl-core/src/route.rs +++ b/crates/ppe-apl-core/src/route.rs @@ -111,46 +111,69 @@ pub async fn evaluate_pre( let mut args_modified = false; for rule in &route.args { - let Some(current) = get_dotted(&payload.args, &rule.field).cloned() else { - continue; // missing field → no pipeline to run + // Fan out over any array on the way to the leaf: `args.rows.ssn` + // redacts the `ssn` of every element of `rows` rather than nothing. A + // plain object path expands to itself, so non-array fields are + // unchanged. An over-large shape fails closed rather than + // half-redacting. + let Some(paths) = expand_field_paths(&payload.args, &rule.field) else { + return RouteDecision { + decision: Decision::Deny { + reason: Some(format!( + "args field `{}` expands to too many elements to redact safely", + rule.field + )), + rule_source: rule.source.clone(), + }, + taints, + constraints: Vec::new(), + args_modified, + result_modified: false, + pending: None, + }; }; - let eval = evaluate_pipeline( - &rule.pipeline, - ¤t, - bag, - plugins, - &rule.field, - DispatchPhase::Pre, - ) - .await; - taints.extend(eval.taints); - match eval.outcome { - FieldOutcome::Pass => {}, - FieldOutcome::Replace(new_val) => { - if set_dotted(&mut payload.args, &rule.field, new_val) { - args_modified = true; - } - }, - FieldOutcome::Omit => { - if remove_dotted(&mut payload.args, &rule.field) { - args_modified = true; - } - }, - FieldOutcome::Deny { reason, .. } => { - return RouteDecision { - decision: Decision::Deny { - reason: Some(reason), - rule_source: rule.source.clone(), - }, - taints, - // `restrict` only fires in the policy phase (below); - // an args-pipeline deny short-circuits before it. - constraints: Vec::new(), - args_modified, - result_modified: false, - pending: None, - }; - }, + for path in paths { + let Some(current) = get_dotted(&payload.args, &path).cloned() else { + continue; // missing field on this element → no pipeline to run + }; + let eval = evaluate_pipeline( + &rule.pipeline, + ¤t, + bag, + plugins, + &rule.field, + DispatchPhase::Pre, + ) + .await; + taints.extend(eval.taints); + match eval.outcome { + FieldOutcome::Pass => {}, + FieldOutcome::Replace(new_val) => { + if set_dotted(&mut payload.args, &path, new_val) { + args_modified = true; + } + }, + FieldOutcome::Omit => { + if remove_dotted(&mut payload.args, &path) { + args_modified = true; + } + }, + FieldOutcome::Deny { reason, .. } => { + return RouteDecision { + decision: Decision::Deny { + reason: Some(reason), + rule_source: rule.source.clone(), + }, + taints, + // `restrict` only fires in the policy phase (below); + // an args-pipeline deny short-circuits before it. + constraints: Vec::new(), + args_modified, + result_modified: false, + pending: None, + }; + }, + } } } @@ -200,46 +223,67 @@ pub async fn evaluate_post( if let Some(result) = payload.result.as_mut() { for rule in &route.result { - let Some(current) = get_dotted(result, &rule.field).cloned() else { - continue; + // Fan out over any array on the way to the leaf, so + // `result.rows.ssn | redact` reaches every row instead of silently + // no-opping. See `expand_field_paths`. + let Some(paths) = expand_field_paths(result, &rule.field) else { + return RouteDecision { + decision: Decision::Deny { + reason: Some(format!( + "result field `{}` expands to too many elements to redact safely", + rule.field + )), + rule_source: rule.source.clone(), + }, + taints, + constraints: Vec::new(), + args_modified: false, + result_modified, + pending: None, + }; }; - let eval = evaluate_pipeline( - &rule.pipeline, - ¤t, - bag, - plugins, - &rule.field, - DispatchPhase::Post, - ) - .await; - taints.extend(eval.taints); - match eval.outcome { - FieldOutcome::Pass => {}, - FieldOutcome::Replace(new_val) => { - if set_dotted(result, &rule.field, new_val) { - result_modified = true; - } - }, - FieldOutcome::Omit => { - if remove_dotted(result, &rule.field) { - result_modified = true; - } - }, - FieldOutcome::Deny { reason, .. } => { - return RouteDecision { - decision: Decision::Deny { - reason: Some(reason), - rule_source: rule.source.clone(), - }, - taints, - // `restrict` fires in post_invocation (below); a - // result-pipeline deny short-circuits before it. - constraints: Vec::new(), - args_modified: false, - result_modified, - pending: None, - }; - }, + for path in paths { + let Some(current) = get_dotted(result, &path).cloned() else { + continue; + }; + let eval = evaluate_pipeline( + &rule.pipeline, + ¤t, + bag, + plugins, + &rule.field, + DispatchPhase::Post, + ) + .await; + taints.extend(eval.taints); + match eval.outcome { + FieldOutcome::Pass => {}, + FieldOutcome::Replace(new_val) => { + if set_dotted(result, &path, new_val) { + result_modified = true; + } + }, + FieldOutcome::Omit => { + if remove_dotted(result, &path) { + result_modified = true; + } + }, + FieldOutcome::Deny { reason, .. } => { + return RouteDecision { + decision: Decision::Deny { + reason: Some(reason), + rule_source: rule.source.clone(), + }, + taints, + // `restrict` fires in post_invocation (below); a + // result-pipeline deny short-circuits before it. + constraints: Vec::new(), + args_modified: false, + result_modified, + pending: None, + }; + }, + } } } } @@ -312,8 +356,130 @@ pub async fn evaluate_route( } } +/// Maximum recursion depth [`expand_field_paths`] will descend before failing +/// closed. The walk increments depth on every hop, so this bounds the sum of a +/// path's segment count and the array-nesting levels fanned through, not array +/// nesting alone. 128 is comfortably above any real (path, tool-result) shape +/// and sits near `serde_json`'s own default parse-recursion limit, so in +/// practice only a host-constructed payload that bypasses the parser can trip +/// it; when it does, the caller denies rather than recursing without bound. +const MAX_FANOUT_DEPTH: usize = 128; + +/// Maximum number of concrete leaf paths one field rule may fan out into +/// before failing closed. High enough for legitimate bulk results, low enough +/// to bound the work and allocation an adversarial result shape can force. +const MAX_EXPANDED_PATHS: usize = 100_000; + +/// Resolve one path segment against a value. Object segments index by key; a +/// numeric segment indexes into an array, so a path produced by +/// [`expand_field_paths`] resolves. +fn segment_get<'a>(value: &'a serde_json::Value, seg: &str) -> Option<&'a serde_json::Value> { + match value { + serde_json::Value::Object(_) => value.get(seg), + serde_json::Value::Array(items) => seg.parse::().ok().and_then(|i| items.get(i)), + _ => None, + } +} + +/// Expand a dotted field path into the concrete paths it names once arrays +/// along the way are accounted for. +/// +/// A field pipeline (`result.rows.ssn | redact`) targets a leaf. When an +/// *intermediate* segment is an array the leaf exists once per element, so the +/// redaction has to reach every element rather than silently no-op. Each array +/// encountered before the final segment fans out into one concrete path per +/// index (`rows.0.ssn`, `rows.1.ssn`, and so on). Paths whose leaf is absent on +/// a given element are dropped, matching the missing-field skip rule. +/// +/// A terminal array is left as a single path, so `result.rows | redact` still +/// replaces the whole array and existing whole-value semantics are preserved. +/// +/// Returns `None` when the expansion would exceed a fan-out bound, either array +/// nesting deeper than [`MAX_FANOUT_DEPTH`] or more than [`MAX_EXPANDED_PATHS`] +/// leaves. Fanning out over attacker-shaped tool output is otherwise unbounded +/// in stack depth and allocation, so an over-large shape fails closed: the +/// caller denies rather than recursing without limit or half-redacting and +/// passing the tail through unredacted. +pub(crate) fn expand_field_paths(root: &serde_json::Value, path: &str) -> Option> { + fn join(prefix: &str, seg: &str) -> String { + if prefix.is_empty() { + seg.to_owned() + } else { + format!("{prefix}.{seg}") + } + } + + // Returns false once a bound is hit; callers stop and fail closed. + fn walk( + value: &serde_json::Value, + segs: &[&str], + prefix: &str, + depth: usize, + out: &mut Vec, + ) -> bool { + if depth > MAX_FANOUT_DEPTH || out.len() > MAX_EXPANDED_PATHS { + return false; + } + // All segments consumed, so `prefix` is a concrete path to a leaf. + let Some((seg, rest)) = segs.split_first() else { + out.push(prefix.to_owned()); + return true; + }; + if let serde_json::Value::Array(items) = value { + if let Ok(idx) = seg.parse::() { + // An explicit numeric segment indexes this array element. + if let Some(child) = items.get(idx) { + return walk(child, rest, &join(prefix, seg), depth + 1, out); + } + } else { + // A non-numeric segment against an array means the array is an + // intermediate hop the path didn't name: apply the remaining + // segments (starting at `seg`) to every element. + for (i, item) in items.iter().enumerate() { + if !walk(item, segs, &join(prefix, &i.to_string()), depth + 1, out) { + return false; + } + } + } + return true; + } + let Some(child) = segment_get(value, seg) else { + return true; // missing segment on this branch, so emit no path + }; + walk(child, rest, &join(prefix, seg), depth + 1, out) + } + + let segs: Vec<&str> = path.split('.').collect(); + let mut out = Vec::new(); + walk(root, &segs, "", 0, &mut out).then_some(out) +} + +/// Descend to the parent value of a dotted path, following object keys and +/// numeric array indices. Returns `None` if any parent segment is missing or +/// crosses a scalar. Shared by `set_dotted` / `remove_dotted` so both write +/// through arrays the same way `get_dotted` reads through them. +fn parent_mut<'a>( + root: &'a mut serde_json::Value, + parents: &[&str], +) -> Option<&'a mut serde_json::Value> { + let mut cur = root; + for seg in parents { + cur = match cur { + serde_json::Value::Object(map) => map.get_mut(*seg)?, + serde_json::Value::Array(items) => seg + .parse::() + .ok() + .and_then(move |i| items.get_mut(i))?, + _ => return None, + }; + } + Some(cur) +} + /// Read `root.a.b.c` from a JSON value via dot-separated path. Returns -/// `None` if any segment is missing or the path crosses a non-object. +/// `None` if any segment is missing. Object segments index by key; a numeric +/// segment indexes into an array, so a path expanded by +/// [`expand_field_paths`] resolves. /// /// Public because host bridges read fields back out of their own payload /// projections — a plugin dispatched from a pipeline stage reports a new @@ -322,14 +488,15 @@ pub async fn evaluate_route( pub fn get_dotted<'a>(root: &'a serde_json::Value, path: &str) -> Option<&'a serde_json::Value> { let mut cur = root; for seg in path.split('.') { - cur = cur.get(seg)?; + cur = segment_get(cur, seg)?; } Some(cur) } /// Write to `root.a.b.c` via dot-separated path. Returns true on success; -/// false if the parent path doesn't exist or doesn't resolve to an object. -/// Does not create missing parent objects — that'd hide schema bugs. +/// false if the parent path doesn't exist or the leaf's parent is a scalar. +/// Does not create missing parent objects — that'd hide schema bugs. A numeric +/// leaf segment overwrites that array element in place. pub(crate) fn set_dotted( root: &mut serde_json::Value, path: &str, @@ -340,45 +507,49 @@ pub(crate) fn set_dotted( Some(x) => x, None => return false, }; - let mut cur = root; - for seg in parents { - let Some(next) = cur.get_mut(*seg) else { - return false; - }; - if !next.is_object() { - return false; - } - cur = next; - } - if let serde_json::Value::Object(map) = cur { - map.insert((*leaf).to_owned(), value); - true - } else { - false + let Some(cur) = parent_mut(root, parents) else { + return false; + }; + match cur { + serde_json::Value::Object(map) => { + map.insert((*leaf).to_owned(), value); + true + }, + serde_json::Value::Array(items) => match leaf.parse::().ok() { + Some(i) => match items.get_mut(i) { + Some(slot) => { + *slot = value; + true + }, + None => false, + }, + None => false, + }, + _ => false, } } -/// Remove `root.a.b.c` from a JSON value. Returns true if removal happened. +/// Remove `root.a.b.c` from a JSON value. Returns true if removal happened. A +/// numeric leaf segment removes that array element. pub(crate) fn remove_dotted(root: &mut serde_json::Value, path: &str) -> bool { let parts: Vec<&str> = path.split('.').collect(); let (leaf, parents) = match parts.split_last() { Some(x) => x, None => return false, }; - let mut cur = root; - for seg in parents { - let Some(next) = cur.get_mut(*seg) else { - return false; - }; - if !next.is_object() { - return false; - } - cur = next; - } - if let serde_json::Value::Object(map) = cur { - map.remove(*leaf).is_some() - } else { - false + let Some(cur) = parent_mut(root, parents) else { + return false; + }; + match cur { + serde_json::Value::Object(map) => map.remove(*leaf).is_some(), + serde_json::Value::Array(items) => match leaf.parse::().ok() { + Some(i) if i < items.len() => { + items.remove(i); + true + }, + _ => false, + }, + _ => false, } } @@ -859,6 +1030,177 @@ mod tests { assert!(!r.args_modified); } + #[test] + fn expand_field_paths_fans_out_over_intermediate_arrays() { + let v = json!({ + "rows": [ { "ssn": "a" }, { "ssn": "b" }, { "other": 1 } ] + }); + // The intermediate `rows` array fans out; the element missing `ssn` + // is dropped (its path never resolves), matching missing-field skip. + let mut paths = expand_field_paths(&v, "rows.ssn").expect("within bounds"); + paths.sort(); + assert_eq!( + paths, + vec!["rows.0.ssn".to_owned(), "rows.1.ssn".to_owned()] + ); + + // A terminal array is one path, preserving whole-value semantics. + assert_eq!( + expand_field_paths(&v, "rows"), + Some(vec!["rows".to_owned()]) + ); + + // A plain object path expands to itself. + let flat = json!({ "a": { "b": 1 } }); + assert_eq!( + expand_field_paths(&flat, "a.b"), + Some(vec!["a.b".to_owned()]) + ); + + // An explicit numeric segment indexes rather than fanning out. + assert_eq!( + expand_field_paths(&v, "rows.0.ssn"), + Some(vec!["rows.0.ssn".to_owned()]) + ); + } + + #[test] + fn expand_field_paths_fails_closed_on_deep_nesting() { + // Pathologically deep array nesting must fail closed rather than + // recurse without bound. + let mut v = json!({ "leaf": 1 }); + for _ in 0..(super::MAX_FANOUT_DEPTH + 5) { + v = serde_json::Value::Array(vec![v]); + } + let root = json!({ "rows": v }); + assert_eq!( + expand_field_paths(&root, "rows.leaf"), + None, + "deep nesting must fail closed" + ); + } + + #[test] + fn expand_field_paths_fails_closed_on_wide_array() { + // The width bound must fail closed too, not just the depth bound: a + // single array past the leaf cap returns None so the caller denies + // rather than half-redacting and passing the tail through. + let mut rows = Vec::with_capacity(super::MAX_EXPANDED_PATHS + 2); + for i in 0..(super::MAX_EXPANDED_PATHS + 2) { + rows.push(json!({ "ssn": i })); + } + let root = json!({ "rows": serde_json::Value::Array(rows) }); + assert_eq!( + expand_field_paths(&root, "rows.ssn"), + None, + "a wide array past the leaf cap must fail closed" + ); + } + + #[test] + fn dotted_helpers_index_into_arrays() { + let mut v = json!({ "rows": [ { "ssn": "a" }, { "ssn": "b" } ] }); + // Read through an array index. + assert_eq!(get_dotted(&v, "rows.1.ssn"), Some(&json!("b"))); + // Write through it. + assert!(set_dotted(&mut v, "rows.0.ssn", json!("[REDACTED]"))); + assert_eq!(v["rows"][0]["ssn"], json!("[REDACTED]")); + // Remove a field from an array element. + assert!(remove_dotted(&mut v, "rows.1.ssn")); + assert!(v["rows"][1].get("ssn").is_none()); + } + + #[tokio::test] + async fn result_pipeline_redacts_every_array_element() { + // `result.rows.ssn | redact` must reach every row rather than silently + // no-opping because `rows` is an array. Without the fan-out the SSNs + // pass through unredacted, which is a PII fail-open. + let mut route = CompiledRoute::new("test"); + route.result.push(field_rule( + "rows.ssn", + vec![Stage::Redact { condition: None }], + )); + + let mut bag = AttributeBag::new(); + let mut payload = RoutePayload::with_result( + json!({}), + json!({ + "rows": [ + { "ssn": "111-11-1111", "name": "a" }, + { "ssn": "222-22-2222", "name": "b" } + ] + }), + ); + + let r = evaluate_route( + &route, + &mut bag, + &mut payload, + &pdp_arc(), + &plugins(), + &delegations(), + &elicitations(), + ) + .await; + + assert_eq!(r.decision, Decision::Allow); + assert!(r.result_modified, "the redaction must register"); + let result = payload.result.as_ref().unwrap(); + assert_eq!(result["rows"][0]["ssn"], json!("[REDACTED]")); + assert_eq!(result["rows"][1]["ssn"], json!("[REDACTED]")); + // Non-targeted fields untouched. + assert_eq!(result["rows"][0]["name"], json!("a")); + } + + #[tokio::test] + async fn route_result_over_large_fanout_denies() { + // When a result field path fans out past the bound, evaluate_post must + // deny rather than pass the un-redacted tail through. A shape nested + // past MAX_FANOUT_DEPTH is the cheap way to trip the bound. + let mut route = CompiledRoute::new("ping"); + route.result.push(field_rule( + "rows.leaf", + vec![Stage::Redact { condition: None }], + )); + + let mut nested = json!({ "leaf": "secret" }); + for _ in 0..(super::MAX_FANOUT_DEPTH + 5) { + nested = serde_json::Value::Array(vec![nested]); + } + let mut bag = AttributeBag::new(); + let mut payload = RoutePayload::with_result(json!({}), json!({ "rows": nested })); + + let r = evaluate_route( + &route, + &mut bag, + &mut payload, + &pdp_arc(), + &plugins(), + &delegations(), + &elicitations(), + ) + .await; + match r.decision { + Decision::Deny { + reason, + rule_source, + } => { + let reason = reason.unwrap_or_default(); + assert!(reason.contains("result field"), "reason: {reason}"); + assert!( + reason.contains("too many elements to redact safely"), + "reason: {reason}" + ); + assert_eq!(rule_source, "test.rows.leaf"); + }, + other => panic!("over-large result fan-out must deny, got {other:?}"), + } + assert!( + !r.result_modified, + "nothing may be redacted on a fail-closed deny" + ); + } + #[tokio::test] async fn post_invocation_runs_after_result() { let mut route = CompiledRoute::new("ping"); From 22de41fbf79de98bb5a1e3d463749dbba5c6f8bb Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 17:36:22 -0400 Subject: [PATCH 15/20] fix(apl): reject more than one elicitation per phase The elicitation id is a single flat bag key and a present id means "already dispatched, poll it", so two elicit steps in one phase collide inside a single request: the first dispatches and writes the key, the second reads it, skips its own dispatch, and adopts the first's verdict. A `require_approval` written after a `confirm` never reaches an approver, and which step wins is only evaluation order. The config expresses two independent gates the engine can enforce as one, so it is rejected at load. The parser checks each block and the visitor rechecks the fully-stacked route, which is where a global or group elicit combined with a route one first becomes visible. Reworked from #54 for the renamed phase fields. Co-authored-by: Shane Utt Signed-off-by: Frederico Araujo --- crates/ppe-apl-core/src/parser.rs | 63 +++++++++++ crates/ppe-apl-core/src/rules.rs | 100 ++++++++++++++++++ crates/ppe-apl-runtime/src/visitor.rs | 26 +++++ .../tests/visitor_config_errors.rs | 50 +++++++++ 4 files changed, 239 insertions(+) diff --git a/crates/ppe-apl-core/src/parser.rs b/crates/ppe-apl-core/src/parser.rs index 2037a3e..9d0da57 100644 --- a/crates/ppe-apl-core/src/parser.rs +++ b/crates/ppe-apl-core/src/parser.rs @@ -3222,6 +3222,35 @@ fn compile_apl_blocks(source: &str, raw: RouteYaml) -> Result 1 { + return Err(ParseError::Rule { + rule: format!("{source}.{phase}"), + msg: format!( + "{phase} reaches {elicits} elicitation steps; at most one elicitation per \ + phase is supported (they would share one retry id and resolve against \ + each other)" + ), + }); + } + } + Ok(route) } @@ -4717,6 +4746,40 @@ route: assert!(msg.contains("result.x"), "expected result.x in: {msg}"); } + #[test] + fn two_elicit_steps_in_one_phase_rejected() { + // Two elicitations in one phase share a single retry id, so the second + // would resolve against the first's approval. Reject at load. + let yaml = r#" +route: + authorization: + pre_invocation: + - "confirm(approver, from: user.sub)" + - "require_approval(approver, from: user.manager)" +"#; + let err = compile_test_policy("payroll", yaml) + .expect_err("two elicits in one phase must be rejected"); + assert!( + format!("{err}").contains("at most one elicitation per phase"), + "expected a multi-elicit rejection, got {err}" + ); + } + + #[test] + fn single_elicit_per_phase_compiles() { + // One elicit in pre and one in post is fine: separate phases, separate + // evaluation walks, so no shared retry id. + let yaml = r#" +route: + authorization: + pre_invocation: + - "require_approval(approver, from: user.manager)" + post_invocation: + - "confirm(auditor, from: user.sub)" +"#; + compile_test_policy("payroll", yaml).expect("one elicit per phase must compile"); + } + #[test] fn duplicate_field_pipeline_key_is_rejected_not_last_wins() { // A `result:` naming the same field twice must not silently keep one diff --git a/crates/ppe-apl-core/src/rules.rs b/crates/ppe-apl-core/src/rules.rs index 6cccdf6..38ffced 100644 --- a/crates/ppe-apl-core/src/rules.rs +++ b/crates/ppe-apl-core/src/rules.rs @@ -334,6 +334,54 @@ impl Effect { } } + /// Count the `Elicit` nodes reachable in this effect subtree. + /// + /// Used by the config-load validator to reject a phase that can reach more + /// than one elicitation. `elicitation.id` is a single flat bag key, not a + /// per-step one, and `dispatch_elicitation` treats it as present to mean + /// "already dispatched, poll it". So within *one* request, with no retry + /// involved: the first elicit dispatches and writes the shared key, and the + /// second reads it, skips its own dispatch entirely, and adopts the first's + /// verdict. A `require_approval` written after a `confirm` never reaches an + /// approver, it is rubber-stamped by whoever answered the confirm. The + /// resolved bundle (status, outcome, approver, intent id) is shared the same + /// way, and which step wins is just evaluation order. + /// + /// Correct multi-elicit needs a per-step id the current single-id protocol + /// cannot carry, so the safe posture is to reject a config that expresses + /// two independent gates the engine can only enforce as one. + /// + /// A PDP's `on_allow` / `on_deny` arms are counted *together* even though + /// only one runs per evaluation: the PDP verdict can flip between the + /// initial request and the retry (its inputs change), so across the two + /// round trips both arms can fire and collide on the shared retry id, which + /// is the same hazard. Summing them is the deliberate conservative choice, + /// so a policy with an elicit in each PDP arm is rejected rather than + /// silently exposed to a verdict flip. `When` arms are likewise summed, + /// since the compiler cannot prove two conditions disjoint. + pub fn count_elicits(&self) -> usize { + match self { + Effect::Elicit(_) => 1, + Effect::Sequential(effects) | Effect::Parallel(effects) => { + effects.iter().map(Effect::count_elicits).sum() + }, + Effect::When { body, .. } => body.iter().map(Effect::count_elicits).sum(), + Effect::Pdp { + on_allow, on_deny, .. + } => { + on_allow.iter().map(Effect::count_elicits).sum::() + + on_deny.iter().map(Effect::count_elicits).sum::() + }, + Effect::Allow + | Effect::Deny { .. } + | Effect::Plugin { .. } + | Effect::Taint { .. } + | Effect::Restrict { .. } + | Effect::FieldOp { .. } + | Effect::Delegate(_) => 0, + } + } + /// Walk the effect tree rejecting any `FieldOp` / `Delegate` that /// lives directly or transitively under a `Parallel` node. Returns /// the path string of the first violation found (or `Ok(())` if @@ -976,6 +1024,58 @@ mod tests { ); } + #[test] + fn count_elicits_sums_through_control_flow() { + let elicit = |name: &str| { + Effect::Elicit(crate::step::ElicitStep { + kind: crate::step::ElicitKind::Approval, + plugin_name: name.into(), + channel: None, + from: "user.manager".into(), + purpose: None, + scope: None, + timeout: None, + config_override: None, + on_error: None, + source: "test".into(), + }) + }; + // No elicit. + assert_eq!(Effect::Allow.count_elicits(), 0); + // One at top level. + assert_eq!(elicit("a").count_elicits(), 1); + // Two under a When body count as two: the compiler cannot prove two + // conditions disjoint, so they are summed. + assert_eq!( + Effect::When { + condition: Expression::Always, + body: vec![elicit("a"), elicit("b")], + source: "test".into(), + } + .count_elicits(), + 2 + ); + // And nested through Sequential. + assert_eq!( + Effect::Sequential(vec![elicit("a"), Effect::Allow, elicit("b")]).count_elicits(), + 2 + ); + // A PDP's two arms are summed, since a verdict flip between the initial + // request and the retry can fire both across the two round trips. + assert_eq!( + Effect::Pdp { + call: crate::step::PdpCall { + dialect: crate::step::PdpDialect::Cedar, + args: serde_yaml::Value::Null, + }, + on_allow: vec![elicit("a")], + on_deny: vec![elicit("b")], + } + .count_elicits(), + 2 + ); + } + #[test] fn validate_parallel_pure_block_passes() { // A parallel block of read-only effects validates clean. diff --git a/crates/ppe-apl-runtime/src/visitor.rs b/crates/ppe-apl-runtime/src/visitor.rs index 65c9193..bdc632c 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -1084,6 +1084,32 @@ impl ConfigVisitor for AplConfigVisitor { return Err(err_msg.into()); } + // Reject a phase that can reach more than one elicitation. The + // elicitation id is one flat bag key, so in a single request the + // second elicit skips its own dispatch and adopts the first's + // verdict, leaving a `require_approval` rubber-stamped by whoever + // answered an earlier `confirm`. See `Effect::count_elicits`. + // Checked here on the fully-stacked route so an elicit inherited + // from a global or group layer plus one on the route also trips it, + // where the parser's per-block check sees only one layer. + for (phase, effects) in [ + ("pre_invocation", &effective.pre_invocation), + ("post_invocation", &effective.post_invocation), + ] { + let elicits: usize = effects + .iter() + .map(praxis_policy_apl_core::rules::Effect::count_elicits) + .sum(); + if elicits > 1 { + let err_msg = format!( + "route '{route_key}': {phase} reaches {elicits} elicitation steps; at \ + most one elicitation per phase is supported (they would share one retry \ + id and resolve against each other)" + ); + return Err(err_msg.into()); + } + } + // Each half installs only when the effective route declares steps // for it, the way the global catch-all already decides. let installs_pre = declares_pre_phase(&effective); diff --git a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs index 7180d8d..dcca00d 100644 --- a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs +++ b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs @@ -223,3 +223,53 @@ fn an_attribute_file_that_does_not_exist_is_rejected() { fn an_empty_attribute_files_list_loads() { loads("global:\n attribute_files: []\n"); } + +// ----------------------------------------------------------------------------- +// Cross-layer elicitation stacking +// ----------------------------------------------------------------------------- + +/// The parser rejects two elicits written in a single route block; the visitor +/// adds the case the parser cannot see, because it only exists once layers are +/// stacked: an elicit inherited from the `global` layer plus one on the route, +/// landing in the same phase. Both would share the one per-request elicitation +/// id, so the second (a weaker `confirm`) would resolve against the first's +/// (`require_approval`) approval. Rejected at load, not mis-evaluated. +#[test] +fn a_global_and_route_elicit_in_one_phase_is_rejected() { + let e = load_err( + r#"global: + authorization: + pre_invocation: + - "require_approval(manager-approver, from: user.manager)" +routes: + - tool: get_compensation + authorization: + pre_invocation: + - "confirm(user-confirm, from: user.sub)" +"#, + ); + assert!( + e.contains("at most one elicitation per phase"), + "a global plus route elicit stacked into one phase must be rejected: {e}" + ); +} + +/// The control: the same two elicits split across the pre and post phases are +/// separate evaluation walks with separate ids, so the config loads. Without it, +/// the rejection above could be coming from having two elicits at all rather +/// than two in one phase. +#[test] +fn a_global_pre_elicit_and_route_post_elicit_load() { + loads( + r#"global: + authorization: + pre_invocation: + - "require_approval(manager-approver, from: user.manager)" +routes: + - tool: get_compensation + authorization: + post_invocation: + - "confirm(user-confirm, from: user.sub)" +"#, + ); +} From 74d41c1a8bd4a73d5b99638c3c2b878e3d99fc39 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 20:32:50 -0400 Subject: [PATCH 16/20] docs: tighten comments added by the #54 port Drop em dashes and trim the wordier doc blocks across the ported commits. Comment-only, except for one real fix: `get_dotted` is public and its doc linked `expand_field_paths`, which is `pub(crate)`, so rustdoc under `-D warnings` failed. That link is now plain code formatting. Signed-off-by: Frederico Araujo --- builtins/pdps/cel/src/activation.rs | 18 ++++------ builtins/pdps/opa/src/resolver.rs | 10 +++--- builtins/plugins/identity-jwt/src/resolver.rs | 12 +++---- builtins/session/valkey/src/store.rs | 11 +++--- crates/ppe-apl-core/src/evaluator.rs | 8 ++--- crates/ppe-apl-core/src/route.rs | 14 ++++---- crates/ppe-apl-core/src/rules.rs | 35 +++++++------------ .../ppe-apl-runtime/src/delegation_invoker.rs | 10 +++--- crates/ppe-apl-runtime/src/route_handler.rs | 2 +- crates/ppe-core/src/config.rs | 10 +++--- crates/ppe-core/src/executor.rs | 12 +++---- crates/ppe-core/src/extensions/container.rs | 25 ++++++------- .../src/extensions/raw_credentials.rs | 25 ++++++------- 13 files changed, 82 insertions(+), 110 deletions(-) diff --git a/builtins/pdps/cel/src/activation.rs b/builtins/pdps/cel/src/activation.rs index 490050a..c521c1d 100644 --- a/builtins/pdps/cel/src/activation.rs +++ b/builtins/pdps/cel/src/activation.rs @@ -161,16 +161,12 @@ fn node_to_value(node: Node) -> Value { /// Convert one `AttributeValue` to a `cel::Value`. /// -/// An `f64` is yielded as `Value::Float`, never silently narrowed to an int. -/// CEL's `==` / `<=` / `<` and friends already compare an int literal against -/// a double operand (verified against the pinned `cel` version's ordering -/// impls and pinned by test), so `delegation.depth <= 2` works with a -/// double-valued `depth`. Narrowing a whole-valued double to an int used to be -/// done "to help literal comparison", but it broke float *arithmetic*: a -/// `confidence` of exactly `1.0` became `int 1`, and `confidence * 100.0` then -/// errored with "no such overload" (int × double) — so the maximum confidence -/// was denied while a lower one was allowed, an outcome inversion driven purely -/// by whether the value happened to be integral. +/// An `f64` stays `Value::Float`, never narrowed to an int. CEL already +/// compares an int literal against a double operand (pinned by test), so +/// `delegation.depth <= 2` works on a double. Narrowing whole-valued doubles +/// broke arithmetic instead: `1.0` became `int 1`, so `confidence * 100.0` +/// failed with "no such overload" and denied the maximum-confidence case while +/// allowing lower ones. fn attr_to_value(attr: &AttributeValue) -> Value { match attr { AttributeValue::Bool(b) => Value::from(*b), @@ -285,7 +281,7 @@ mod tests { } /// A double-valued bag scalar compares correctly against an integer - /// literal without being narrowed to an int — CEL's ordering handles the + /// literal without being narrowed to an int: CEL's ordering handles the /// mixed comparison. Narrowing is deliberately not done because it breaks /// float arithmetic (see `whole_valued_float_keeps_arithmetic`). #[test] diff --git a/builtins/pdps/opa/src/resolver.rs b/builtins/pdps/opa/src/resolver.rs index 239f2e5..82eab42 100644 --- a/builtins/pdps/opa/src/resolver.rs +++ b/builtins/pdps/opa/src/resolver.rs @@ -318,7 +318,7 @@ impl OpaResolver { // (`data.authz`), and a global rule reads whole subtrees, so an inline // module in a *sub-package* of a global one (`data.authz.exceptions` // under `data.authz`) still feeds `data.authz.*` that a global - // `authz` rule can consume — an override by the back door. Reject when + // `authz` rule can consume, an override by the back door. Reject when // the inline package equals, is nested under, or contains any global // package, so the two never share a `data` subtree. if self @@ -397,7 +397,7 @@ impl OpaResolver { } } -/// True when two dotted Rego package paths occupy the same `data` subtree — +/// True when two dotted Rego package paths occupy the same `data` subtree: /// they are equal, or one is nested under the other (`data.authz` and /// `data.authz.exceptions`). A bare prefix comparison is wrong: `data.authz` /// must not be judged to contain `data.authznext`, so the boundary is only a @@ -991,12 +991,12 @@ msg := "not a decision" /// An inline module in a *sub-package* of a global package is rejected /// fail-closed. A global `authz` rule reads whole `data.authz.*` subtrees, /// so an inline `package authz.exceptions` still feeds operator policy even - /// though it never names `package authz` directly — the back-door override - /// the exact-match check used to miss. + /// though it never names `package authz` directly. That is the back-door + /// override the exact-match check missed. #[tokio::test] async fn inline_module_cannot_override_global_subpackage() { // Global policy allows only when the caller is listed under - // `data.authz.exceptions` — a subtree an inline module must not reach. + // `data.authz.exceptions`, a subtree an inline module must not reach. let global = "package authz\ndefault allow := false\nallow if data.authz.exceptions[input.subject.id]\n"; let r = resolver(&[global], OnError::Allow); let inline = "package authz.exceptions\nexceptions := {\"eve\": true}\n"; diff --git a/builtins/plugins/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index 14ddcc0..6f09e4f 100644 --- a/builtins/plugins/identity-jwt/src/resolver.rs +++ b/builtins/plugins/identity-jwt/src/resolver.rs @@ -130,12 +130,10 @@ pub struct JwtIdentityResolver { } // Manual `Debug` rather than derived: `cfg` retains the raw config JSON and -// `pending_jwks` holds `DecodingKeySource` values, both of which carry HMAC -// secrets and inline PEM key material. For HS* those secrets are *signing* -// keys — token-forgery material — so a `{:?}` of the resolver must never -// print them. Only the non-secret operational fields are shown; the -// secret-carrying fields are elided. This mirrors the redacting `Debug` on -// `KeyStore` / `TrustedIssuer` and the sibling auth plugins. +// `pending_jwks` holds `DecodingKeySource` values, both carrying HMAC secrets +// and inline PEM key material. For HS* those secrets are *signing* keys, so a +// `{:?}` of the resolver must never print them. Only the non-secret fields are +// shown, mirroring the redacting `Debug` on `KeyStore` / `TrustedIssuer`. impl std::fmt::Debug for JwtIdentityResolver { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("JwtIdentityResolver") @@ -1165,7 +1163,7 @@ mod tests { fn strip_bearer_prefix_is_case_insensitive() { // Canonical casing. assert_eq!(strip_bearer_prefix("Bearer abc.def.ghi"), "abc.def.ghi"); - // Lower / upper / mixed — RFC 9110 scheme is case-insensitive. + // Lower / upper / mixed: RFC 9110 scheme is case-insensitive. assert_eq!(strip_bearer_prefix("bearer abc.def.ghi"), "abc.def.ghi"); assert_eq!(strip_bearer_prefix("BEARER abc.def.ghi"), "abc.def.ghi"); assert_eq!(strip_bearer_prefix("BeArEr abc.def.ghi"), "abc.def.ghi"); diff --git a/builtins/session/valkey/src/store.rs b/builtins/session/valkey/src/store.rs index 387155e..b9fff9f 100644 --- a/builtins/session/valkey/src/store.rs +++ b/builtins/session/valkey/src/store.rs @@ -128,12 +128,11 @@ impl SessionStore for ValkeySessionStore { // closed. A persistently-failing refresh risks silent key // expiry across requests — see the operator runbook. if let Some(ttl) = self.ttl_seconds { - // A timeout is a refresh failure too — in fact the most likely one - // under an overloaded backend — so it must raise the same alarm as - // a backend error, not be swallowed. Both map to a warn carrying - // the `session_store_ttl_refresh_failed` alarm; otherwise a - // persistently-timing-out EXPIRE would silently stop the sliding - // TTL and let session taint expire mid-session with no signal. + // A timeout is a refresh failure too, and the likeliest one under + // an overloaded backend, so it raises the same alarm as a backend + // error instead of being swallowed. Otherwise a persistently + // timing-out EXPIRE would stop the sliding TTL and let session + // taint expire mid-session with no signal. let refresh: Result, _> = tokio::time::timeout(self.command_timeout, conn.expire(&key, ttl_for_expire(ttl))) .await; diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 20649e1..366a06e 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -1093,7 +1093,7 @@ fn dispatch_parallel<'a>( // Aggregate in input order: append every branch's taints; pick // the first Halt (by branch index, not wall-clock order) as the - // overall result. Aborted branches contribute no taints — they + // overall result. Aborted branches contribute no taints: they // were short-circuit cancelled. A *panicked* branch is converted // into a fail-closed Halt: we cannot know whether the branch it // ran would have denied, and the engine's contract everywhere @@ -1101,7 +1101,7 @@ fn dispatch_parallel<'a>( // permits (a plugin `Err` halts at line 497, and the concurrent // executor phase halts on a panicking branch under `on_error: // fail`). Swallowing the panic here would make a guard branch's - // deny vanish — fail-open in the one phase whose job is to block. + // deny vanish, a fail-open in the one phase whose job is to block. let mut first_halt: Option = None; for (idx, outcome) in outcomes.into_iter().enumerate() { match outcome { @@ -3145,7 +3145,7 @@ mod tests { } } - /// Invoker that panics on any plugin call — models a misbehaving plugin + /// Invoker that panics on any plugin call, modelling a misbehaving plugin /// unwinding inside a `parallel:` branch. struct PanicPlugins; #[async_trait] @@ -4106,7 +4106,7 @@ mod tests { #[tokio::test] async fn parallel_panicked_branch_fails_closed() { // A branch that panics (misbehaving plugin) must halt the phase, not - // be silently dropped — a swallowed panic would let a guard branch's + // be silently dropped: a swallowed panic would let a guard branch's // deny vanish (fail-open). The surviving Allow branch must not rescue // the request. let mut bag = AttributeBag::new(); diff --git a/crates/ppe-apl-core/src/route.rs b/crates/ppe-apl-core/src/route.rs index cac243e..2da76c9 100644 --- a/crates/ppe-apl-core/src/route.rs +++ b/crates/ppe-apl-core/src/route.rs @@ -357,12 +357,10 @@ pub async fn evaluate_route( } /// Maximum recursion depth [`expand_field_paths`] will descend before failing -/// closed. The walk increments depth on every hop, so this bounds the sum of a -/// path's segment count and the array-nesting levels fanned through, not array -/// nesting alone. 128 is comfortably above any real (path, tool-result) shape -/// and sits near `serde_json`'s own default parse-recursion limit, so in -/// practice only a host-constructed payload that bypasses the parser can trip -/// it; when it does, the caller denies rather than recursing without bound. +/// closed. Depth increments on every hop, so this bounds a path's segment count +/// plus the array-nesting levels fanned through, not nesting alone. 128 sits +/// above any real shape and near `serde_json`'s own parse-recursion limit, so +/// in practice only a payload that bypasses the parser trips it. const MAX_FANOUT_DEPTH: usize = 128; /// Maximum number of concrete leaf paths one field rule may fan out into @@ -479,7 +477,7 @@ fn parent_mut<'a>( /// Read `root.a.b.c` from a JSON value via dot-separated path. Returns /// `None` if any segment is missing. Object segments index by key; a numeric /// segment indexes into an array, so a path expanded by -/// [`expand_field_paths`] resolves. +/// `expand_field_paths` resolves. /// /// Public because host bridges read fields back out of their own payload /// projections — a plugin dispatched from a pipeline stage reports a new @@ -495,7 +493,7 @@ pub fn get_dotted<'a>(root: &'a serde_json::Value, path: &str) -> Option<&'a ser /// Write to `root.a.b.c` via dot-separated path. Returns true on success; /// false if the parent path doesn't exist or the leaf's parent is a scalar. -/// Does not create missing parent objects — that'd hide schema bugs. A numeric +/// Does not create missing parent objects, which would hide schema bugs. A numeric /// leaf segment overwrites that array element in place. pub(crate) fn set_dotted( root: &mut serde_json::Value, diff --git a/crates/ppe-apl-core/src/rules.rs b/crates/ppe-apl-core/src/rules.rs index 38ffced..d1026fd 100644 --- a/crates/ppe-apl-core/src/rules.rs +++ b/crates/ppe-apl-core/src/rules.rs @@ -336,29 +336,20 @@ impl Effect { /// Count the `Elicit` nodes reachable in this effect subtree. /// - /// Used by the config-load validator to reject a phase that can reach more - /// than one elicitation. `elicitation.id` is a single flat bag key, not a - /// per-step one, and `dispatch_elicitation` treats it as present to mean - /// "already dispatched, poll it". So within *one* request, with no retry - /// involved: the first elicit dispatches and writes the shared key, and the - /// second reads it, skips its own dispatch entirely, and adopts the first's - /// verdict. A `require_approval` written after a `confirm` never reaches an - /// approver, it is rubber-stamped by whoever answered the confirm. The - /// resolved bundle (status, outcome, approver, intent id) is shared the same - /// way, and which step wins is just evaluation order. + /// Used by the config-load validator to reject a phase reaching more than + /// one elicitation. `elicitation.id` is a single flat bag key, and + /// `dispatch_elicitation` reads a present id as "already dispatched, poll + /// it". So within one request the first elicit dispatches and writes the + /// key, and the second skips its own dispatch and adopts the first's + /// verdict: a `require_approval` after a `confirm` never reaches an + /// approver. Supporting this properly needs a per-step id the current + /// protocol cannot carry, so the config is rejected instead. /// - /// Correct multi-elicit needs a per-step id the current single-id protocol - /// cannot carry, so the safe posture is to reject a config that expresses - /// two independent gates the engine can only enforce as one. - /// - /// A PDP's `on_allow` / `on_deny` arms are counted *together* even though - /// only one runs per evaluation: the PDP verdict can flip between the - /// initial request and the retry (its inputs change), so across the two - /// round trips both arms can fire and collide on the shared retry id, which - /// is the same hazard. Summing them is the deliberate conservative choice, - /// so a policy with an elicit in each PDP arm is rejected rather than - /// silently exposed to a verdict flip. `When` arms are likewise summed, - /// since the compiler cannot prove two conditions disjoint. + /// A PDP's `on_allow` / `on_deny` arms are summed even though only one runs + /// per evaluation, because the verdict can flip between the initial request + /// and the retry, letting both fire across the two round trips. `When` arms + /// are summed for the same reason: the compiler cannot prove two conditions + /// disjoint. pub fn count_elicits(&self) -> usize { match self { Effect::Elicit(_) => 1, diff --git a/crates/ppe-apl-runtime/src/delegation_invoker.rs b/crates/ppe-apl-runtime/src/delegation_invoker.rs index 58e108d..5a09168 100644 --- a/crates/ppe-apl-runtime/src/delegation_invoker.rs +++ b/crates/ppe-apl-runtime/src/delegation_invoker.rs @@ -116,9 +116,9 @@ impl DelegationInvoker for DelegationPluginInvoker { // each recognized key is lifted onto the typed DelegationPayload. // Recognized keys: `target` (required), `subject`, `actor`, // `audience`, `permissions`, `target_type`, `auth_enforced_by`, and - // `attenuation`. Any other key is intentionally dropped — the payload - // has no untyped input channel, so an unrecognized key does not reach - // the plugin (an earlier comment here claimed otherwise; it did not). + // `attenuation`. Any other key is intentionally dropped: the payload + // has no untyped input channel, so an unrecognized key never reaches + // the plugin. // // There is deliberately no `mode` key: the delegation mode is // *derived* from `subject` by the handler rather than declared, so a @@ -127,7 +127,7 @@ impl DelegationInvoker for DelegationPluginInvoker { // // `attenuation` is load-bearing: it *narrows* the minted credential's // scope, so silently dropping it would mint a broader token than the - // author asked for — a fail-open. A malformed `attenuation:` errors + // author asked for, which is a fail-open. A malformed `attenuation:` errors // (fail closed) rather than being ignored. let cfg = step.config_override.as_ref().and_then(|v| v.as_mapping()); @@ -484,7 +484,7 @@ mod tests { #[test] fn attenuation_typo_key_fails_closed() { // A misspelled key must not deserialize into an empty (no-op) config - // that silently widens the token — it is a hard error. + // that silently widens the token. It is a hard error. let err = attenuation_from_cfg(Some(&cfg("attenuation:\n actionss: [read]"))) .expect_err("unknown attenuation key must fail closed"); assert!(matches!(err, DelegationError::InvalidConfig(_))); diff --git a/crates/ppe-apl-runtime/src/route_handler.rs b/crates/ppe-apl-runtime/src/route_handler.rs index 93393cb..206b178 100644 --- a/crates/ppe-apl-runtime/src/route_handler.rs +++ b/crates/ppe-apl-runtime/src/route_handler.rs @@ -1174,7 +1174,7 @@ mod tests { } /// A route plugin whose only edit is an HTTP header write (via - /// `write_headers`) must be detected — otherwise the header rewrite is + /// `write_headers`) must be detected, or the header rewrite is /// dropped at this boundary and never reaches the upstream request. #[test] fn an_http_change_alone_is_detected() { diff --git a/crates/ppe-core/src/config.rs b/crates/ppe-core/src/config.rs index 4be39b1..89eec86 100644 --- a/crates/ppe-core/src/config.rs +++ b/crates/ppe-core/src/config.rs @@ -2262,12 +2262,10 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box = config.plugins.iter().map(|p| p.name.as_str()).collect(); // Validate `authentication:` step names the same way `plugins:` names - // are. `RouteIdentityStep` documents that a step name must match a - // top-level `plugins:` entry registered under `identity.resolve`, but - // nothing enforced it: an unresolvable name finds no entry at dispatch - // and is dropped with no error, leaving that authentication step - // unrun. Global, group, and route authentication share one shape, so - // one check covers all three. + // are. `RouteIdentityStep` requires a step to name a top-level + // `plugins:` entry, but nothing enforced it: an unresolvable name finds + // no entry at dispatch and is dropped silently, leaving that step + // unrun. Global, group, and route authentication share one shape. let validate_authentication = |authentication: &Option, context: &str| diff --git a/crates/ppe-core/src/executor.rs b/crates/ppe-core/src/executor.rs index 4bbd0cc..7bbc594 100644 --- a/crates/ppe-core/src/executor.rs +++ b/crates/ppe-core/src/executor.rs @@ -494,12 +494,12 @@ impl Executor { if !erased.continue_processing && can_block { // A blocking plugin that signals "do not continue" // halts the pipeline whether or not it attached a - // violation. A missing violation is synthesized - // rather than treated as an allow — the concurrent - // phase does the same (`concurrent_deny` below), and - // letting a deny-without-reason fall through to the - // modification path would be a fail-open in the - // phase whose whole job is enforcement. + // violation. A missing one is synthesized rather + // than treated as an allow, as the concurrent phase + // already does (`concurrent_deny` below). Letting a + // reasonless deny fall through to the modification + // path would be a fail-open in the phase whose job + // is enforcement. let mut v = erased.violation.unwrap_or_else(|| { crate::error::PluginViolation::new( "plugin_deny", diff --git a/crates/ppe-core/src/extensions/container.rs b/crates/ppe-core/src/extensions/container.rs index c898063..8d0ff35 100644 --- a/crates/ppe-core/src/extensions/container.rs +++ b/crates/ppe-core/src/extensions/container.rs @@ -364,21 +364,18 @@ impl Extensions { /// Merge the `security` slot — labels only, and only as an append. /// /// Labels are the Monotonic tier: `append_labels` permits growing the set - /// and nothing else. Monotonicity is guaranteed *structurally* here — the + /// and nothing else. Monotonicity is guaranteed *structurally* here: the /// returned labels are folded into the canonical set, never assigned over /// it, so a canonical label the plugin could not see (or deliberately /// dropped) is re-added by construction and can never be removed by this /// path. Removal requires a `DeclassifierToken` no plugin can construct. /// - /// This does not itself judge a shrunk returned set to be a laundering - /// *attempt* and reject the whole edit: that requires knowing whether the - /// plugin was shown the canonical labels (a `read_labels` holder) or an - /// empty filtered view (an append-only plugin, whose additions are - /// legitimate even though they are not a superset of canonical). That - /// capability context lives in the executor, which drops the whole edit - /// when a `read_labels` plugin returns a non-superset set (`labels_ok`), - /// before this merge is ever reached. Re-checking it here without that - /// context silently discarded an append-only plugin's labels. + /// It does not also judge a shrunk set to be a laundering *attempt* and + /// reject the whole edit. That needs to know whether the plugin saw the + /// canonical labels or an empty filtered view, and that capability context + /// lives in the executor's `labels_ok`, which runs before this merge. + /// Re-checking it here without the context discarded an append-only + /// plugin's labels. /// /// Every other field on the slot is Immutable in the tier model with /// `write_cap: None` — `subject`, `auth_method`, `client`, `caller_workload`, @@ -459,7 +456,7 @@ impl Extensions { /// Every existing hop must be unchanged. The comparison covers all the fields /// that carry authority or bound it: subject, audience, granted scopes, /// strategy, the RFC 9396 `authorization_details` (documented as "must be -/// structurally narrowed" — a rewrite widens the grant), the `ttl_seconds` +/// structurally narrowed", since a rewrite widens the grant), the `ttl_seconds` /// lifetime (extending it is a privilege escalation), and the `timestamp` /// (which drives age/expiry checks). A rewrite of any of these on an existing /// hop is not an append, so the whole edit is refused. `from_cache` is merge @@ -1134,7 +1131,7 @@ mod tests { // dropped label survives (monotonicity is structural here). The // *drop-whole* punishment for a `read_labels` laundering attempt lives // in the executor's `labels_ok`, which has the capability context this - // low-level merge does not — see `merge_security`'s doc. + // low-level merge does not. See `merge_security`'s doc. let mut security = SecurityExtension::default(); security.add_label("PII"); security.add_label("HIPAA"); @@ -1166,7 +1163,7 @@ mod tests { fn test_append_only_plugin_labels_survive_nonempty_canonical() { // Regression: a plugin holding `append_labels` but not `read_labels` // sees an empty filtered label set, so its returned set contains only - // its additions — not a superset of the canonical set. The old + // its additions, not a superset of the canonical set. The old // superset gate in `merge_security` dropped those additions whenever // canonical was non-empty, silently disabling a write-only tainting / // DLP plugin. Folding must now land the addition. @@ -1373,7 +1370,7 @@ mod tests { ext.delegation_write_token = Some(WriteToken::new()); // Rewrite the existing hop's RFC 9396 authorization_details to add - // actions (a widening) and extend its ttl — neither is an append. + // actions (a widening) and extend its ttl. Neither is an append. let mut cow = ext.cow_copy(); { let hop = &mut cow.delegation.as_mut().unwrap().chain[0]; diff --git a/crates/ppe-core/src/extensions/raw_credentials.rs b/crates/ppe-core/src/extensions/raw_credentials.rs index 3423880..f65dba8 100644 --- a/crates/ppe-core/src/extensions/raw_credentials.rs +++ b/crates/ppe-core/src/extensions/raw_credentials.rs @@ -436,13 +436,11 @@ pub struct RawCredentialsExtension { /// (`TokenDelegate` handlers only). /// /// Serialized as a sequence of `[key, value]` pairs, not a JSON object: - /// the key is the `DelegationKey` struct, and JSON object keys must be - /// strings, so a plain map serialization errors at runtime the moment a - /// token is minted — which would break the documented `extensions` wire - /// channel, audit dumps, and hot-reload snapshots (the very paths the - /// module's serialization-safety contract promises work). The pair-seq - /// form serializes cleanly and round-trips; the token bytes inside each - /// value stay `#[serde(skip)]`. + /// the key is a `DelegationKey` struct and JSON object keys must be + /// strings, so a plain map errors at runtime the moment a token is minted, + /// breaking the `extensions` wire channel, audit dumps, and hot-reload + /// snapshots. The pair form round-trips, and token bytes inside each value + /// stay `#[serde(skip)]`. #[serde(default, with = "delegated_tokens_as_pairs")] pub delegated_tokens: HashMap, } @@ -451,13 +449,10 @@ pub struct RawCredentialsExtension { /// `(DelegationKey, RawDelegatedToken)` pairs so a non-string map key /// serializes to JSON. See the field doc for why a plain map cannot. /// -/// Deserialization is tolerant of *both* the pair-sequence form and a plain -/// map. Before this adapter, only an empty `delegated_tokens` ever serialized, -/// and it did so as a JSON object (`{}`); a snapshot or wire message written by -/// that older code must still load, so a map input is accepted too (its entries, -/// if any, having string-shaped keys from a hand-built or empty document). This -/// keeps mixed-version rollouts and old persisted snapshots readable in both -/// directions. +/// Deserialization accepts both the pair form and a plain map. Before this +/// adapter only an empty `delegated_tokens` ever serialized, and it did so as +/// `{}`, so snapshots and wire messages written by that older code must still +/// load. This keeps mixed-version rollouts readable. mod delegated_tokens_as_pairs { use super::{DelegationKey, RawDelegatedToken}; use serde::de::{MapAccess, SeqAccess, Visitor}; @@ -727,7 +722,7 @@ mod tests { fn legacy_empty_map_delegated_tokens_still_deserializes() { // Before the pair-seq adapter, only an empty delegated_tokens ever // serialized, and it did so as a JSON object `{}`. A snapshot written - // by that older code must still load — the deserializer accepts a map + // by that older code must still load, so the deserializer accepts a map // as well as the new sequence form. let legacy = r#"{"inbound_tokens":{},"delegated_tokens":{}}"#; let restored: RawCredentialsExtension = serde_json::from_str(legacy).unwrap(); From a9aee3bbb3c8d8e22ca624fa6699bf1644468578 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 20:57:27 -0400 Subject: [PATCH 17/20] test: declare the plugins that authentication steps name Two fixtures named an `authentication:` step whose plugin was absent from `plugins:`, which the new load-time check refuses. Both were under-specified rather than wrong to reject: engine.rs resolves an authentication step by finding a matching entry and drops it with no else branch, so such a route would resolve no identity plugin and authenticate with nothing. delegation_identity_warning also needed an inert factory, since a declared plugin's `kind:` must resolve to one. Signed-off-by: Frederico Araujo --- .../tests/delegation_identity_warning.rs | 36 +++++++++++++++++-- crates/ppe-core/tests/config_key_sets.rs | 17 +++++---- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs b/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs index 8baea3a..85949c4 100644 --- a/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs +++ b/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs @@ -26,12 +26,38 @@ use std::sync::{Arc, Mutex}; use praxis_policy_core::engine::PolicyEngine; +use praxis_policy_core::error::PluginError; +use praxis_policy_core::factory::{PluginFactory, PluginInstance}; +use praxis_policy_core::plugin::{Plugin, PluginConfig}; use tracing::field::{Field, Visit}; use tracing::span::{Attributes, Id, Record}; use tracing::{Event, Metadata, Subscriber}; const ALARM: &str = "delegation_without_identity_resolution"; +/// A plugin registering no handler. These cases are decided during the config +/// load, so what it would do on a request is beside the point; it exists so the +/// `authentication:` step in `WITH_IDENTITY` names a declared plugin whose +/// `kind:` resolves to a factory. +struct Inert(PluginConfig); + +impl Plugin for Inert { + fn config(&self) -> &PluginConfig { + &self.0 + } +} + +struct InertFactory; + +impl PluginFactory for InertFactory { + fn create(&self, config: &PluginConfig) -> Result> { + Ok(PluginInstance { + plugin: Arc::new(Inert(config.clone())), + handlers: Vec::new(), + }) + } +} + /// A route delegating the caller's credential, with nothing configured /// to validate it. const NO_IDENTITY: &str = r#" @@ -46,11 +72,16 @@ routes: "#; /// The same route with an `authentication:` block, which is what -/// identity resolution keys on. +/// identity resolution keys on. The step's plugin is declared because an +/// `authentication:` name that matches no `plugins:` entry is refused at load: +/// it would resolve to nothing at dispatch and leave the route unauthenticated. const WITH_IDENTITY: &str = r#" engine_settings: dispatch: policy -plugins: [] +plugins: + - name: corp-jwt + kind: builtin + hooks: [identity.resolve] routes: - tool: get_compensation authentication: @@ -136,6 +167,7 @@ fn alarms_raised_by_loading(yaml: &str) -> Vec { }; let mgr = Arc::new(PolicyEngine::default()); + mgr.register_factory("builtin", Box::new(InertFactory)); praxis_policy_apl_runtime::register_apl( &mgr, praxis_policy_apl_runtime::AplOptions::in_process(), diff --git a/crates/ppe-core/tests/config_key_sets.rs b/crates/ppe-core/tests/config_key_sets.rs index d649062..674a694 100644 --- a/crates/ppe-core/tests/config_key_sets.rs +++ b/crates/ppe-core/tests/config_key_sets.rs @@ -639,15 +639,20 @@ fn a_misspelled_authentication_object_key_is_rejected() { } /// Both accepted shapes still load, and the flag still reads through. +/// +/// Each declares the `jwt` plugin the step names: an `authentication:` step +/// matching no `plugins:` entry is refused at load, since it would resolve to +/// nothing at dispatch and leave the route unauthenticated. #[test] fn the_authentication_object_shapes_still_load() { - let additive = praxis_policy_core::config::parse_config( - "routes:\n - tool: get_weather\n authentication: [jwt]\n", - ) + const PLUGINS: &str = "plugins:\n - name: jwt\n kind: builtin\n hooks: [identity.resolve]\n"; + let additive = praxis_policy_core::config::parse_config(&format!( + "{PLUGINS}routes:\n - tool: get_weather\n authentication: [jwt]\n" + )) .expect("the list form is additive"); - let replacing = praxis_policy_core::config::parse_config( - "routes:\n - tool: get_weather\n authentication:\n replace_inherited: true\n steps: [jwt]\n", - ) + let replacing = praxis_policy_core::config::parse_config(&format!( + "{PLUGINS}routes:\n - tool: get_weather\n authentication:\n replace_inherited: true\n steps: [jwt]\n" + )) .expect("the object form loads"); for (label, cfg, expected) in [("list", additive, false), ("object", replacing, true)] { let identity = cfg.routes[0] From c760cc6e741201322321a3fe0e4d96732609d3a1 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 21:03:42 -0400 Subject: [PATCH 18/20] style: wrap an over-width const in the authentication fixture Missed running fmt after the previous commit. No behavior change. Signed-off-by: Frederico Araujo --- crates/ppe-core/tests/config_key_sets.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ppe-core/tests/config_key_sets.rs b/crates/ppe-core/tests/config_key_sets.rs index 674a694..4af21f2 100644 --- a/crates/ppe-core/tests/config_key_sets.rs +++ b/crates/ppe-core/tests/config_key_sets.rs @@ -645,7 +645,8 @@ fn a_misspelled_authentication_object_key_is_rejected() { /// nothing at dispatch and leave the route unauthenticated. #[test] fn the_authentication_object_shapes_still_load() { - const PLUGINS: &str = "plugins:\n - name: jwt\n kind: builtin\n hooks: [identity.resolve]\n"; + const PLUGINS: &str = + "plugins:\n - name: jwt\n kind: builtin\n hooks: [identity.resolve]\n"; let additive = praxis_policy_core::config::parse_config(&format!( "{PLUGINS}routes:\n - tool: get_weather\n authentication: [jwt]\n" )) From 688ca15e9c18740907546afbe61eb78b1b6152a2 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 21:53:42 -0400 Subject: [PATCH 19/20] docs: condense comments across the #54 port Comment-only: no code or doctest lines change. Signed-off-by: Frederico Araujo --- builtins/pdps/cel/src/activation.rs | 35 ++-------- builtins/pdps/opa/src/resolver.rs | 39 ++--------- builtins/plugins/identity-jwt/src/resolver.rs | 22 ++---- builtins/session/valkey/src/store.rs | 6 +- crates/ppe-apl-core/src/evaluator.rs | 40 ++--------- crates/ppe-apl-core/src/parser.rs | 24 +------ crates/ppe-apl-core/src/route.rs | 69 +++---------------- crates/ppe-apl-core/src/rules.rs | 24 +------ .../ppe-apl-runtime/src/delegation_invoker.rs | 14 +--- crates/ppe-apl-runtime/src/route_handler.rs | 16 +---- crates/ppe-apl-runtime/src/visitor.rs | 10 +-- .../tests/delegation_identity_warning.rs | 9 +-- .../tests/visitor_config_errors.rs | 16 +---- crates/ppe-core/src/config.rs | 12 +--- crates/ppe-core/src/delegation/payload.rs | 4 +- crates/ppe-core/src/executor.rs | 10 +-- crates/ppe-core/src/extensions/container.rs | 44 ++---------- .../src/extensions/raw_credentials.rs | 34 ++------- crates/ppe-core/tests/config_key_sets.rs | 4 -- 19 files changed, 63 insertions(+), 369 deletions(-) diff --git a/builtins/pdps/cel/src/activation.rs b/builtins/pdps/cel/src/activation.rs index c521c1d..8551550 100644 --- a/builtins/pdps/cel/src/activation.rs +++ b/builtins/pdps/cel/src/activation.rs @@ -161,12 +161,8 @@ fn node_to_value(node: Node) -> Value { /// Convert one `AttributeValue` to a `cel::Value`. /// -/// An `f64` stays `Value::Float`, never narrowed to an int. CEL already -/// compares an int literal against a double operand (pinned by test), so -/// `delegation.depth <= 2` works on a double. Narrowing whole-valued doubles -/// broke arithmetic instead: `1.0` became `int 1`, so `confidence * 100.0` -/// failed with "no such overload" and denied the maximum-confidence case while -/// allowing lower ones. +/// An `f64` stays `Value::Float`; narrowing whole-valued floats breaks CEL +/// arithmetic such as `confidence * 100.0`. fn attr_to_value(attr: &AttributeValue) -> Value { match attr { AttributeValue::Bool(b) => Value::from(*b), @@ -188,11 +184,8 @@ fn attr_to_value(attr: &AttributeValue) -> Value { } } -/// Convert a `serde_yaml::Value` (author-supplied `cel:` args) to a -/// `cel::Value`. An integer literal maps to `Int`, a fractional one to -/// `Float`; a float is never narrowed to an int (same reasoning as -/// `attr_to_value`). Non-string mapping keys are skipped (CEL map keys here -/// are always strings for author ergonomics). +/// Convert an author-supplied `cel:` argument to a `cel::Value`. Integers map +/// to `Int`, floats to `Float`, and non-string mapping keys are skipped. fn yaml_to_value(v: &serde_yaml::Value) -> Value { match v { serde_yaml::Value::Null => Value::Null, @@ -280,10 +273,6 @@ mod tests { assert!(truthy("session.labels.exists(l, l == 'PII')", &bag)); } - /// A double-valued bag scalar compares correctly against an integer - /// literal without being narrowed to an int: CEL's ordering handles the - /// mixed comparison. Narrowing is deliberately not done because it breaks - /// float arithmetic (see `whole_valued_float_keeps_arithmetic`). #[test] fn double_scalar_compares_against_int_literal() { let mut bag = AttributeBag::new(); @@ -295,25 +284,15 @@ mod tests { assert!(truthy("intent.confidence > 0.9", &bag)); } - /// Regression: a whole-valued double (`1.0`) must stay a double so that - /// float arithmetic on it still resolves. Narrowing it to `int 1` made - /// `confidence * 100.0` fail with "no such overload" and denied the - /// maximum-confidence case while allowing lower ones. #[test] fn whole_valued_float_keeps_arithmetic() { let mut bag = AttributeBag::new(); bag.set("intent.confidence", 1.0_f64); assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag)); - // And the sub-1.0 case is unchanged. bag.set("intent.confidence", 0.92_f64); assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag)); } - /// The float-stays-float change relies on CEL comparing an int literal - /// against a double operand in *either* operand order. The existing tests - /// only cover `depth 2`; pin the reversed `2 depth` form too, so a - /// one-sided comparison impl can't silently break the half of author - /// policies that write the literal on the left. #[test] fn mixed_int_float_comparison_is_order_independent() { let mut bag = AttributeBag::new(); @@ -323,17 +302,11 @@ mod tests { assert!(truthy("2 >= delegation.depth", &bag)); assert!(truthy("3 > delegation.depth", &bag)); assert!(truthy("1 < delegation.depth", &bag)); - // A true inequality must resolve to a value, not a "no such overload" - // error; asserted via its positive form so an error can't pass as false. assert!(truthy("3 != delegation.depth", &bag)); } #[test] fn whole_valued_float_in_int_list_matches() { - // Guard the float-stays-float change against the `in` operator: a - // whole-valued double must still be found in an int list (CEL's `in` - // uses cross-type equality), so membership doesn't invert on int-vs- - // double the way the old narrowing avoided for `==` but broke for `*`. let mut bag = AttributeBag::new(); bag.set("delegation.depth", 2.0_f64); assert!( diff --git a/builtins/pdps/opa/src/resolver.rs b/builtins/pdps/opa/src/resolver.rs index 82eab42..17c7e44 100644 --- a/builtins/pdps/opa/src/resolver.rs +++ b/builtins/pdps/opa/src/resolver.rs @@ -310,17 +310,7 @@ impl OpaResolver { .add_policy(INLINE_MODULE_NAME.to_owned(), src.to_owned()) .map_err(|e| EngineError::Compile(e.to_string()))?; - // Reject an inline module that lands in a global module's package — it - // would merge into (and could override) operator policy. Fail-closed: - // inline modules may add new packages, never redefine a global one. - // - // The check is prefix-aware, not exact-match: package paths are dotted - // (`data.authz`), and a global rule reads whole subtrees, so an inline - // module in a *sub-package* of a global one (`data.authz.exceptions` - // under `data.authz`) still feeds `data.authz.*` that a global - // `authz` rule can consume, an override by the back door. Reject when - // the inline package equals, is nested under, or contains any global - // package, so the two never share a `data` subtree. + // Inline modules may add packages but may not share a global package subtree. if self .global_packages .iter() @@ -397,11 +387,9 @@ impl OpaResolver { } } -/// True when two dotted Rego package paths occupy the same `data` subtree: -/// they are equal, or one is nested under the other (`data.authz` and -/// `data.authz.exceptions`). A bare prefix comparison is wrong: `data.authz` -/// must not be judged to contain `data.authznext`, so the boundary is only a -/// match when the next character is a path separator. +/// Whether two dotted Rego package paths are equal or one contains the other. +/// Path-separator boundaries keep siblings such as `data.authz` and +/// `data.authznext` distinct. fn packages_share_subtree(a: &str, b: &str) -> bool { a == b || a.strip_prefix(b).is_some_and(|rest| rest.starts_with('.')) @@ -988,15 +976,8 @@ msg := "not a decision" } } - /// An inline module in a *sub-package* of a global package is rejected - /// fail-closed. A global `authz` rule reads whole `data.authz.*` subtrees, - /// so an inline `package authz.exceptions` still feeds operator policy even - /// though it never names `package authz` directly. That is the back-door - /// override the exact-match check missed. #[tokio::test] async fn inline_module_cannot_override_global_subpackage() { - // Global policy allows only when the caller is listed under - // `data.authz.exceptions`, a subtree an inline module must not reach. let global = "package authz\ndefault allow := false\nallow if data.authz.exceptions[input.subject.id]\n"; let r = resolver(&[global], OnError::Allow); let inline = "package authz.exceptions\nexceptions := {\"eve\": true}\n"; @@ -1025,17 +1006,10 @@ msg := "not a decision" assert_eq!(out.decision, Decision::Allow); } - /// The collision check must be prefix-*boundary* aware, not a bare string - /// prefix: a global `data.authz` must not be judged to contain - /// `data.authznext` just because one string-prefixes the other. A - /// sibling-prefix package is a genuinely separate `data` subtree and stays - /// allowed; a naive `starts_with` would wrongly reject it (fail-closed, but - /// a spurious denial of a legitimate inline module). #[tokio::test] async fn inline_module_in_prefix_sibling_package_is_allowed() { let global = "package authz\nallow if input.subject.id == \"alice\"\n"; let r = resolver(&[global], OnError::Deny); - // `authznext` shares the `authz` string prefix but is a different subtree. let inline = "package authznext\nallow if input.subject.id == \"alice\"\n"; let out = r .evaluate(&call("data.authznext.allow", Some(inline)), &bag("alice")) @@ -1048,11 +1022,6 @@ msg := "not a decision" ); } - /// The symmetric case: an inline module whose package is a *parent* of a - /// global package (inline `data.authz`, global `data.authz.sub`) still feeds - /// a `data` subtree the operator policy reads, so it is rejected fail-closed. - /// This exercises the `b.strip_prefix(a)` arm of `packages_share_subtree`, - /// which the sub-package test (child-of-global) does not. #[tokio::test] async fn inline_module_that_is_parent_of_global_collides() { let global = "package authz.sub\nallow if input.subject.id == \"alice\"\n"; diff --git a/builtins/plugins/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index 6f09e4f..89e4d50 100644 --- a/builtins/plugins/identity-jwt/src/resolver.rs +++ b/builtins/plugins/identity-jwt/src/resolver.rs @@ -129,11 +129,8 @@ pub struct JwtIdentityResolver { header: String, } -// Manual `Debug` rather than derived: `cfg` retains the raw config JSON and -// `pending_jwks` holds `DecodingKeySource` values, both carrying HMAC secrets -// and inline PEM key material. For HS* those secrets are *signing* keys, so a -// `{:?}` of the resolver must never print them. Only the non-secret fields are -// shown, mirroring the redacting `Debug` on `KeyStore` / `TrustedIssuer`. +// Implement `Debug` manually because `cfg` and `pending_jwks` may contain HMAC +// signing secrets or inline PEM keys. impl std::fmt::Debug for JwtIdentityResolver { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("JwtIdentityResolver") @@ -953,14 +950,9 @@ fn peek_issuer(token: &str) -> Option { /// Strip a leading `Bearer` auth-scheme from a header value, if present. /// -/// The `Bearer` scheme name is case-insensitive per RFC 9110 §11.1 (`bearer`, -/// `BEARER`, `Bearer` are all the same scheme), and the scheme is followed by -/// one or more spaces before the token. A value that does not carry the scheme -/// (a bare token) is returned unchanged, so hosts that pre-strip still work. +/// The scheme is case-insensitive per RFC 9110 §11.1. Bare tokens are returned +/// unchanged for hosts that strip the scheme themselves. fn strip_bearer_prefix(value: &str) -> &str { - // Split on the first space: the scheme is everything before it. Requiring - // a space means a bare token (no scheme, e.g. host pre-stripped) and a - // token that merely starts with "bearer" both fall through untouched. match value.split_once(' ') { Some((scheme, after)) if scheme.eq_ignore_ascii_case("bearer") => { after.trim_start_matches(' ') @@ -1161,18 +1153,12 @@ mod tests { #[test] fn strip_bearer_prefix_is_case_insensitive() { - // Canonical casing. assert_eq!(strip_bearer_prefix("Bearer abc.def.ghi"), "abc.def.ghi"); - // Lower / upper / mixed: RFC 9110 scheme is case-insensitive. assert_eq!(strip_bearer_prefix("bearer abc.def.ghi"), "abc.def.ghi"); assert_eq!(strip_bearer_prefix("BEARER abc.def.ghi"), "abc.def.ghi"); assert_eq!(strip_bearer_prefix("BeArEr abc.def.ghi"), "abc.def.ghi"); - // Multiple spaces between scheme and token collapse away. assert_eq!(strip_bearer_prefix("bearer abc.def.ghi"), "abc.def.ghi"); - // A bare token (host pre-stripped) passes through untouched. assert_eq!(strip_bearer_prefix("abc.def.ghi"), "abc.def.ghi"); - // A token that merely starts with "bearer" but has no space is not - // the scheme and must not be truncated. assert_eq!(strip_bearer_prefix("bearerish"), "bearerish"); } diff --git a/builtins/session/valkey/src/store.rs b/builtins/session/valkey/src/store.rs index b9fff9f..7884c2b 100644 --- a/builtins/session/valkey/src/store.rs +++ b/builtins/session/valkey/src/store.rs @@ -128,11 +128,7 @@ impl SessionStore for ValkeySessionStore { // closed. A persistently-failing refresh risks silent key // expiry across requests — see the operator runbook. if let Some(ttl) = self.ttl_seconds { - // A timeout is a refresh failure too, and the likeliest one under - // an overloaded backend, so it raises the same alarm as a backend - // error instead of being swallowed. Otherwise a persistently - // timing-out EXPIRE would stop the sliding TTL and let session - // taint expire mid-session with no signal. + // Timeouts raise the same refresh-failure alarm as backend errors. let refresh: Result, _> = tokio::time::timeout(self.command_timeout, conn.expire(&key, ttl_for_expire(ttl))) .await; diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 366a06e..0ca3d1b 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -1091,17 +1091,8 @@ fn dispatch_parallel<'a>( }) .await; - // Aggregate in input order: append every branch's taints; pick - // the first Halt (by branch index, not wall-clock order) as the - // overall result. Aborted branches contribute no taints: they - // were short-circuit cancelled. A *panicked* branch is converted - // into a fail-closed Halt: we cannot know whether the branch it - // ran would have denied, and the engine's contract everywhere - // else is that an effect which cannot complete denies rather than - // permits (a plugin `Err` halts at line 497, and the concurrent - // executor phase halts on a panicking branch under `on_error: - // fail`). Swallowing the panic here would make a guard branch's - // deny vanish, a fail-open in the one phase whose job is to block. + // Aggregate in branch order, appending taints and keeping the first Halt. + // Cancelled branches contribute nothing; panicked branches fail closed. let mut first_halt: Option = None; for (idx, outcome) in outcomes.into_iter().enumerate() { match outcome { @@ -1128,13 +1119,7 @@ fn dispatch_parallel<'a>( // post-config-extension. }, BranchOutcome::Panicked(msg) => { - // A panicking branch is a misbehaving plugin/effect. It - // fails closed: a panic is a more severe failure than a - // plugin `Err` (which already halts), so it must not be - // treated more leniently. The first panic, in branch - // index order, stands in as the phase's deny if no - // earlier branch already produced a Halt. `msg` carries - // the panic payload into the audit reason. + // Use the panic as this branch's synthetic Halt and audit reason. if first_halt.is_none() { first_halt = Some(Decision::Deny { reason: Some(format!("parallel branch {idx} panicked: {msg}")), @@ -1211,10 +1196,7 @@ async fn dispatch_field_op( }); }; - // Fan out over arrays on the path so a `do:`-embedded field op like - // `result.rows.ssn | redact` reaches every element's leaf instead of - // silently no-op'ing on the array. An object-only path expands to itself. - // An over-large shape fails closed rather than half-redacting. + // Expand intermediate arrays; excessive fan-out fails closed. let Some(paths) = crate::route::expand_field_paths(root, subpath) else { return EffectOutcome::Halt(Decision::Deny { reason: Some(format!( @@ -1235,11 +1217,8 @@ async fn dispatch_field_op( let Some(current) = get_dotted(root, &concrete).cloned() else { continue; // missing field on this element → silent no-op }; - // `subpath`, not `path`: the field name a pipeline reports to a - // plugin is relative to the args / result root, matching what the - // `args:` / `result:` section pipelines pass. The prefixed `path` - // stays in use for deny messages, where the reader wants the side - // spelled out. + // Plugins receive a root-relative field name; deny messages use the + // prefixed path to identify the args or result side. let eval = evaluate_pipeline(&pipeline, ¤t, bag, plugins, subpath, phase).await; taints.extend(eval.taints); match eval.outcome { @@ -3145,8 +3124,7 @@ mod tests { } } - /// Invoker that panics on any plugin call, modelling a misbehaving plugin - /// unwinding inside a `parallel:` branch. + /// Invoker that panics on every plugin call. struct PanicPlugins; #[async_trait] impl PluginInvoker for PanicPlugins { @@ -4105,10 +4083,6 @@ mod tests { #[tokio::test] async fn parallel_panicked_branch_fails_closed() { - // A branch that panics (misbehaving plugin) must halt the phase, not - // be silently dropped: a swallowed panic would let a guard branch's - // deny vanish (fail-open). The surviving Allow branch must not rescue - // the request. let mut bag = AttributeBag::new(); let mut payload = crate::route::RoutePayload::new(json!({})); diff --git a/crates/ppe-apl-core/src/parser.rs b/crates/ppe-apl-core/src/parser.rs index 9d0da57..ef424c6 100644 --- a/crates/ppe-apl-core/src/parser.rs +++ b/crates/ppe-apl-core/src/parser.rs @@ -3223,14 +3223,8 @@ fn compile_apl_blocks(source: &str, raw: RouteYaml) -> Result(value: &'a serde_json::Value, seg: &str) -> Option<&'a serde_ /// Expand a dotted field path into the concrete paths it names once arrays /// along the way are accounted for. /// -/// A field pipeline (`result.rows.ssn | redact`) targets a leaf. When an -/// *intermediate* segment is an array the leaf exists once per element, so the -/// redaction has to reach every element rather than silently no-op. Each array -/// encountered before the final segment fans out into one concrete path per -/// index (`rows.0.ssn`, `rows.1.ssn`, and so on). Paths whose leaf is absent on -/// a given element are dropped, matching the missing-field skip rule. -/// -/// A terminal array is left as a single path, so `result.rows | redact` still -/// replaces the whole array and existing whole-value semantics are preserved. -/// -/// Returns `None` when the expansion would exceed a fan-out bound, either array -/// nesting deeper than [`MAX_FANOUT_DEPTH`] or more than [`MAX_EXPANDED_PATHS`] -/// leaves. Fanning out over attacker-shaped tool output is otherwise unbounded -/// in stack depth and allocation, so an over-large shape fails closed: the -/// caller denies rather than recursing without limit or half-redacting and -/// passing the tail through unredacted. +/// Intermediate arrays fan out into indexed paths; missing leaves are skipped. +/// A terminal array remains one path so whole-value operations still apply. +/// Returns `None` when depth or leaf-count bounds are exceeded, allowing callers +/// to fail closed rather than apply a partial transformation. pub(crate) fn expand_field_paths(root: &serde_json::Value, path: &str) -> Option> { fn join(prefix: &str, seg: &str) -> String { if prefix.is_empty() { @@ -407,7 +384,6 @@ pub(crate) fn expand_field_paths(root: &serde_json::Value, path: &str) -> Option } } - // Returns false once a bound is hit; callers stop and fail closed. fn walk( value: &serde_json::Value, segs: &[&str], @@ -418,21 +394,17 @@ pub(crate) fn expand_field_paths(root: &serde_json::Value, path: &str) -> Option if depth > MAX_FANOUT_DEPTH || out.len() > MAX_EXPANDED_PATHS { return false; } - // All segments consumed, so `prefix` is a concrete path to a leaf. let Some((seg, rest)) = segs.split_first() else { out.push(prefix.to_owned()); return true; }; if let serde_json::Value::Array(items) = value { if let Ok(idx) = seg.parse::() { - // An explicit numeric segment indexes this array element. if let Some(child) = items.get(idx) { return walk(child, rest, &join(prefix, seg), depth + 1, out); } } else { - // A non-numeric segment against an array means the array is an - // intermediate hop the path didn't name: apply the remaining - // segments (starting at `seg`) to every element. + // Apply an unnamed intermediate array to every element. for (i, item) in items.iter().enumerate() { if !walk(item, segs, &join(prefix, &i.to_string()), depth + 1, out) { return false; @@ -493,8 +465,7 @@ pub fn get_dotted<'a>(root: &'a serde_json::Value, path: &str) -> Option<&'a ser /// Write to `root.a.b.c` via dot-separated path. Returns true on success; /// false if the parent path doesn't exist or the leaf's parent is a scalar. -/// Does not create missing parent objects, which would hide schema bugs. A numeric -/// leaf segment overwrites that array element in place. +/// Does not create missing parents. A numeric leaf overwrites an array element. pub(crate) fn set_dotted( root: &mut serde_json::Value, path: &str, @@ -1033,8 +1004,6 @@ mod tests { let v = json!({ "rows": [ { "ssn": "a" }, { "ssn": "b" }, { "other": 1 } ] }); - // The intermediate `rows` array fans out; the element missing `ssn` - // is dropped (its path never resolves), matching missing-field skip. let mut paths = expand_field_paths(&v, "rows.ssn").expect("within bounds"); paths.sort(); assert_eq!( @@ -1042,20 +1011,17 @@ mod tests { vec!["rows.0.ssn".to_owned(), "rows.1.ssn".to_owned()] ); - // A terminal array is one path, preserving whole-value semantics. assert_eq!( expand_field_paths(&v, "rows"), Some(vec!["rows".to_owned()]) ); - // A plain object path expands to itself. let flat = json!({ "a": { "b": 1 } }); assert_eq!( expand_field_paths(&flat, "a.b"), Some(vec!["a.b".to_owned()]) ); - // An explicit numeric segment indexes rather than fanning out. assert_eq!( expand_field_paths(&v, "rows.0.ssn"), Some(vec!["rows.0.ssn".to_owned()]) @@ -1064,8 +1030,6 @@ mod tests { #[test] fn expand_field_paths_fails_closed_on_deep_nesting() { - // Pathologically deep array nesting must fail closed rather than - // recurse without bound. let mut v = json!({ "leaf": 1 }); for _ in 0..(super::MAX_FANOUT_DEPTH + 5) { v = serde_json::Value::Array(vec![v]); @@ -1080,9 +1044,6 @@ mod tests { #[test] fn expand_field_paths_fails_closed_on_wide_array() { - // The width bound must fail closed too, not just the depth bound: a - // single array past the leaf cap returns None so the caller denies - // rather than half-redacting and passing the tail through. let mut rows = Vec::with_capacity(super::MAX_EXPANDED_PATHS + 2); for i in 0..(super::MAX_EXPANDED_PATHS + 2) { rows.push(json!({ "ssn": i })); @@ -1098,21 +1059,15 @@ mod tests { #[test] fn dotted_helpers_index_into_arrays() { let mut v = json!({ "rows": [ { "ssn": "a" }, { "ssn": "b" } ] }); - // Read through an array index. assert_eq!(get_dotted(&v, "rows.1.ssn"), Some(&json!("b"))); - // Write through it. assert!(set_dotted(&mut v, "rows.0.ssn", json!("[REDACTED]"))); assert_eq!(v["rows"][0]["ssn"], json!("[REDACTED]")); - // Remove a field from an array element. assert!(remove_dotted(&mut v, "rows.1.ssn")); assert!(v["rows"][1].get("ssn").is_none()); } #[tokio::test] async fn result_pipeline_redacts_every_array_element() { - // `result.rows.ssn | redact` must reach every row rather than silently - // no-opping because `rows` is an array. Without the fan-out the SSNs - // pass through unredacted, which is a PII fail-open. let mut route = CompiledRoute::new("test"); route.result.push(field_rule( "rows.ssn", @@ -1146,15 +1101,11 @@ mod tests { let result = payload.result.as_ref().unwrap(); assert_eq!(result["rows"][0]["ssn"], json!("[REDACTED]")); assert_eq!(result["rows"][1]["ssn"], json!("[REDACTED]")); - // Non-targeted fields untouched. assert_eq!(result["rows"][0]["name"], json!("a")); } #[tokio::test] async fn route_result_over_large_fanout_denies() { - // When a result field path fans out past the bound, evaluate_post must - // deny rather than pass the un-redacted tail through. A shape nested - // past MAX_FANOUT_DEPTH is the cheap way to trip the bound. let mut route = CompiledRoute::new("ping"); route.result.push(field_rule( "rows.leaf", diff --git a/crates/ppe-apl-core/src/rules.rs b/crates/ppe-apl-core/src/rules.rs index d1026fd..33e91eb 100644 --- a/crates/ppe-apl-core/src/rules.rs +++ b/crates/ppe-apl-core/src/rules.rs @@ -336,20 +336,9 @@ impl Effect { /// Count the `Elicit` nodes reachable in this effect subtree. /// - /// Used by the config-load validator to reject a phase reaching more than - /// one elicitation. `elicitation.id` is a single flat bag key, and - /// `dispatch_elicitation` reads a present id as "already dispatched, poll - /// it". So within one request the first elicit dispatches and writes the - /// key, and the second skips its own dispatch and adopts the first's - /// verdict: a `require_approval` after a `confirm` never reaches an - /// approver. Supporting this properly needs a per-step id the current - /// protocol cannot carry, so the config is rejected instead. - /// - /// A PDP's `on_allow` / `on_deny` arms are summed even though only one runs - /// per evaluation, because the verdict can flip between the initial request - /// and the retry, letting both fire across the two round trips. `When` arms - /// are summed for the same reason: the compiler cannot prove two conditions - /// disjoint. + /// Validators reject multiple elicitations per phase because the protocol + /// has one shared elicitation id. Conditional and PDP arms are summed + /// conservatively because different arms may run across retries. pub fn count_elicits(&self) -> usize { match self { Effect::Elicit(_) => 1, @@ -1031,12 +1020,8 @@ mod tests { source: "test".into(), }) }; - // No elicit. assert_eq!(Effect::Allow.count_elicits(), 0); - // One at top level. assert_eq!(elicit("a").count_elicits(), 1); - // Two under a When body count as two: the compiler cannot prove two - // conditions disjoint, so they are summed. assert_eq!( Effect::When { condition: Expression::Always, @@ -1046,13 +1031,10 @@ mod tests { .count_elicits(), 2 ); - // And nested through Sequential. assert_eq!( Effect::Sequential(vec![elicit("a"), Effect::Allow, elicit("b")]).count_elicits(), 2 ); - // A PDP's two arms are summed, since a verdict flip between the initial - // request and the retry can fire both across the two round trips. assert_eq!( Effect::Pdp { call: crate::step::PdpCall { diff --git a/crates/ppe-apl-runtime/src/delegation_invoker.rs b/crates/ppe-apl-runtime/src/delegation_invoker.rs index 5a09168..fa331ac 100644 --- a/crates/ppe-apl-runtime/src/delegation_invoker.rs +++ b/crates/ppe-apl-runtime/src/delegation_invoker.rs @@ -125,10 +125,7 @@ impl DelegationInvoker for DelegationPluginInvoker { // route can't claim on-behalf-of-user while handing over a workload // SVID. // - // `attenuation` is load-bearing: it *narrows* the minted credential's - // scope, so silently dropping it would mint a broader token than the - // author asked for, which is a fail-open. A malformed `attenuation:` errors - // (fail closed) rather than being ignored. + // Malformed attenuation fails closed rather than widening the credential. let cfg = step.config_override.as_ref().and_then(|v| v.as_mapping()); // Resolve who the exchange is *for*. Defaults to the user @@ -388,10 +385,7 @@ fn target_type_from_str(s: &str) -> TargetType { /// Parse the optional `attenuation:` block into a typed `AttenuationConfig`. /// -/// Attenuation *narrows* the minted credential's scope, so a present-but- -/// malformed block fails closed (returns `InvalidConfig`) rather than being -/// silently ignored, which would mint a broader token than the author asked -/// for. An absent block yields `None` (no narrowing requested). +/// A malformed block returns `InvalidConfig`; an absent block returns `None`. fn attenuation_from_cfg( cfg: Option<&serde_yaml::Mapping>, ) -> Result, DelegationError> { @@ -474,8 +468,6 @@ mod tests { #[test] fn malformed_attenuation_fails_closed() { - // A present-but-malformed block must error rather than be dropped: - // dropping it would mint a broader token than the author asked for. let err = attenuation_from_cfg(Some(&cfg("attenuation:\n ttl_seconds: not-a-number"))) .expect_err("malformed attenuation must fail closed"); assert!(matches!(err, DelegationError::InvalidConfig(_))); @@ -483,8 +475,6 @@ mod tests { #[test] fn attenuation_typo_key_fails_closed() { - // A misspelled key must not deserialize into an empty (no-op) config - // that silently widens the token. It is a hard error. let err = attenuation_from_cfg(Some(&cfg("attenuation:\n actionss: [read]"))) .expect_err("unknown attenuation key must fail closed"); assert!(matches!(err, DelegationError::InvalidConfig(_))); diff --git a/crates/ppe-apl-runtime/src/route_handler.rs b/crates/ppe-apl-runtime/src/route_handler.rs index 206b178..b8eddcb 100644 --- a/crates/ppe-apl-runtime/src/route_handler.rs +++ b/crates/ppe-apl-runtime/src/route_handler.rs @@ -844,12 +844,7 @@ fn extensions_changed(before: &Extensions, after: &Extensions) -> bool { (None, None) => false, _ => true, }; - // `http` and `custom` are mutable slots too: a route plugin holding - // `write_headers` rewrites request/response headers, and a plugin may - // stash data in `custom`. Omitting them here left `modified_extensions` - // None when a plugin's *only* edit was a header write, so the executor - // never merged it and the upstream call never saw the change. (The - // `candidate_constraint` slot is handled by the force-`Some` above.) + // `http` and `custom` are mutable slots; `candidate_constraint` is handled above. let http_changed = match (before.http.as_ref(), after.http.as_ref()) { (Some(a), Some(b)) => !Arc::ptr_eq(a, b), (None, None) => false, @@ -1173,9 +1168,6 @@ mod tests { assert!(extensions_changed(&before, &after)); } - /// A route plugin whose only edit is an HTTP header write (via - /// `write_headers`) must be detected, or the header rewrite is - /// dropped at this boundary and never reaches the upstream request. #[test] fn an_http_change_alone_is_detected() { use praxis_policy_core::extensions::HttpExtension; @@ -1254,11 +1246,7 @@ mod tests { "a replaced delegation chain must be detected" ); - // The production arm for http/custom is `Some(a) -> Some(b)` with a - // replaced Arc (a plugin rewriting an already-present slot). The - // slot-appears tests above only hit the `None -> Some` arm; cover the - // replaced-Arc arm too so a regression there (e.g. value-equality) is - // caught. + // Cover replacement of an existing mutable slot, not only `None -> Some`. use praxis_policy_core::extensions::HttpExtension; let http = Arc::new(HttpExtension::default()); let with_http = |c: &Arc| Extensions { diff --git a/crates/ppe-apl-runtime/src/visitor.rs b/crates/ppe-apl-runtime/src/visitor.rs index bdc632c..4c545b3 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -1084,14 +1084,8 @@ impl ConfigVisitor for AplConfigVisitor { return Err(err_msg.into()); } - // Reject a phase that can reach more than one elicitation. The - // elicitation id is one flat bag key, so in a single request the - // second elicit skips its own dispatch and adopts the first's - // verdict, leaving a `require_approval` rubber-stamped by whoever - // answered an earlier `confirm`. See `Effect::count_elicits`. - // Checked here on the fully-stacked route so an elicit inherited - // from a global or group layer plus one on the route also trips it, - // where the parser's per-block check sees only one layer. + // Repeat elicitation validation after stacking to catch duplicates + // introduced across global, group, and route layers. for (phase, effects) in [ ("pre_invocation", &effective.pre_invocation), ("post_invocation", &effective.post_invocation), diff --git a/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs b/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs index 85949c4..b6b5ff3 100644 --- a/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs +++ b/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs @@ -35,10 +35,7 @@ use tracing::{Event, Metadata, Subscriber}; const ALARM: &str = "delegation_without_identity_resolution"; -/// A plugin registering no handler. These cases are decided during the config -/// load, so what it would do on a request is beside the point; it exists so the -/// `authentication:` step in `WITH_IDENTITY` names a declared plugin whose -/// `kind:` resolves to a factory. +/// Declares the identity plugin used by the configuration fixture. struct Inert(PluginConfig); impl Plugin for Inert { @@ -72,9 +69,7 @@ routes: "#; /// The same route with an `authentication:` block, which is what -/// identity resolution keys on. The step's plugin is declared because an -/// `authentication:` name that matches no `plugins:` entry is refused at load: -/// it would resolve to nothing at dispatch and leave the route unauthenticated. +/// identity resolution keys on. const WITH_IDENTITY: &str = r#" engine_settings: dispatch: policy diff --git a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs index dcca00d..9be331f 100644 --- a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs +++ b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs @@ -224,16 +224,7 @@ fn an_empty_attribute_files_list_loads() { loads("global:\n attribute_files: []\n"); } -// ----------------------------------------------------------------------------- -// Cross-layer elicitation stacking -// ----------------------------------------------------------------------------- - -/// The parser rejects two elicits written in a single route block; the visitor -/// adds the case the parser cannot see, because it only exists once layers are -/// stacked: an elicit inherited from the `global` layer plus one on the route, -/// landing in the same phase. Both would share the one per-request elicitation -/// id, so the second (a weaker `confirm`) would resolve against the first's -/// (`require_approval`) approval. Rejected at load, not mis-evaluated. +/// Cross-layer elicitations in one phase are rejected after route stacking. #[test] fn a_global_and_route_elicit_in_one_phase_is_rejected() { let e = load_err( @@ -254,10 +245,7 @@ routes: ); } -/// The control: the same two elicits split across the pre and post phases are -/// separate evaluation walks with separate ids, so the config loads. Without it, -/// the rejection above could be coming from having two elicits at all rather -/// than two in one phase. +/// Elicitations in separate phases remain valid. #[test] fn a_global_pre_elicit_and_route_post_elicit_load() { loads( diff --git a/crates/ppe-core/src/config.rs b/crates/ppe-core/src/config.rs index 89eec86..568dcac 100644 --- a/crates/ppe-core/src/config.rs +++ b/crates/ppe-core/src/config.rs @@ -2261,11 +2261,7 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box = config.plugins.iter().map(|p| p.name.as_str()).collect(); - // Validate `authentication:` step names the same way `plugins:` names - // are. `RouteIdentityStep` requires a step to name a top-level - // `plugins:` entry, but nothing enforced it: an unresolvable name finds - // no entry at dispatch and is dropped silently, leaving that step - // unrun. Global, group, and route authentication share one shape. + // Authentication steps must name a declared plugin. let validate_authentication = |authentication: &Option, context: &str| @@ -4112,9 +4108,6 @@ routes: [] ); } - /// A typo'd `authentication:` step name must be rejected at load. At - /// dispatch it resolves to no entry and is dropped silently, so the route - /// runs with that authentication step missing, which is a fail-open. #[test] fn test_route_unknown_authentication_step_rejected() { let err = parse_config( @@ -4139,7 +4132,6 @@ routes: ); } - /// The same check on the global block. #[test] fn test_global_unknown_authentication_step_rejected() { let err = parse_config( @@ -4161,8 +4153,6 @@ routes: [] ); } - /// A step naming a declared plugin still loads, so the check does not - /// reject legitimate configs. #[test] fn test_known_authentication_step_is_accepted() { parse_config( diff --git a/crates/ppe-core/src/delegation/payload.rs b/crates/ppe-core/src/delegation/payload.rs index b095aa4..7eb036b 100644 --- a/crates/ppe-core/src/delegation/payload.rs +++ b/crates/ppe-core/src/delegation/payload.rs @@ -194,9 +194,7 @@ pub enum AuthEnforcedBy { /// minted token's scope claim. v0 doesn't include a template /// renderer — handlers receive the raw template string and render /// themselves; a framework-side renderer can come later. -// `deny_unknown_fields`: attenuation *narrows* a credential, so a misspelled -// key (`actionss:`) must not deserialize into an all-empty, no-op config that -// silently widens the minted token. An unknown key is a hard error instead. +// Unknown fields fail closed because silently dropping attenuation can widen a credential. #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct AttenuationConfig { diff --git a/crates/ppe-core/src/executor.rs b/crates/ppe-core/src/executor.rs index 7bbc594..074e15d 100644 --- a/crates/ppe-core/src/executor.rs +++ b/crates/ppe-core/src/executor.rs @@ -492,14 +492,8 @@ impl Executor { Ok(Ok(result_box)) => { if let Some(erased) = extract_erased(result_box) { if !erased.continue_processing && can_block { - // A blocking plugin that signals "do not continue" - // halts the pipeline whether or not it attached a - // violation. A missing one is synthesized rather - // than treated as an allow, as the concurrent phase - // already does (`concurrent_deny` below). Letting a - // reasonless deny fall through to the modification - // path would be a fail-open in the phase whose job - // is enforcement. + // A blocking result always halts; synthesize a violation + // when the plugin did not provide one. let mut v = erased.violation.unwrap_or_else(|| { crate::error::PluginViolation::new( "plugin_deny", diff --git a/crates/ppe-core/src/extensions/container.rs b/crates/ppe-core/src/extensions/container.rs index 8d0ff35..dc0b09d 100644 --- a/crates/ppe-core/src/extensions/container.rs +++ b/crates/ppe-core/src/extensions/container.rs @@ -364,18 +364,11 @@ impl Extensions { /// Merge the `security` slot — labels only, and only as an append. /// /// Labels are the Monotonic tier: `append_labels` permits growing the set - /// and nothing else. Monotonicity is guaranteed *structurally* here: the - /// returned labels are folded into the canonical set, never assigned over - /// it, so a canonical label the plugin could not see (or deliberately - /// dropped) is re-added by construction and can never be removed by this - /// path. Removal requires a `DeclassifierToken` no plugin can construct. + /// and nothing else. Folding returned labels into the canonical set makes + /// removal structurally impossible. /// - /// It does not also judge a shrunk set to be a laundering *attempt* and - /// reject the whole edit. That needs to know whether the plugin saw the - /// canonical labels or an empty filtered view, and that capability context - /// lives in the executor's `labels_ok`, which runs before this merge. - /// Re-checking it here without the context discarded an append-only - /// plugin's labels. + /// Capability-aware laundering detection happens earlier in `labels_ok`; + /// this layer cannot distinguish a filtered view from a removal attempt. /// /// Every other field on the slot is Immutable in the tier model with /// `write_cap: None` — `subject`, `auth_method`, `client`, `caller_workload`, @@ -394,9 +387,7 @@ impl Extensions { None => SecurityExtension::default(), }; - // Monotonic: fold in the additions, never assign the returned set. - // Assignment would drop any canonical label the plugin could not see, - // and folding makes a filtered-away label unremovable by construction. + // Fold additions into canonical labels; never replace the set. for label in owned.labels.iter() { merged.labels.add_label(label.clone()); } @@ -453,14 +444,8 @@ impl Extensions { /// True when `returned` is `canonical` plus zero or more appended hops. /// -/// Every existing hop must be unchanged. The comparison covers all the fields -/// that carry authority or bound it: subject, audience, granted scopes, -/// strategy, the RFC 9396 `authorization_details` (documented as "must be -/// structurally narrowed", since a rewrite widens the grant), the `ttl_seconds` -/// lifetime (extending it is a privilege escalation), and the `timestamp` -/// (which drives age/expiry checks). A rewrite of any of these on an existing -/// hop is not an append, so the whole edit is refused. `from_cache` is merge -/// bookkeeping, not authority, so it is not compared. +/// Existing hops must match on every authority-bearing or bounding field. +/// `from_cache` is merge bookkeeping and is not compared. /// /// Public so out-of-process hosts can apply the same validation to a chain /// arriving over the wire before it reaches the merge, instead of reimplementing @@ -1126,12 +1111,6 @@ mod tests { #[test] fn test_label_removal_is_refused_by_folding() { - // A returned set that drops a canonical label cannot remove it: the - // merge folds the returned labels into the canonical set, so the - // dropped label survives (monotonicity is structural here). The - // *drop-whole* punishment for a `read_labels` laundering attempt lives - // in the executor's `labels_ok`, which has the capability context this - // low-level merge does not. See `merge_security`'s doc. let mut security = SecurityExtension::default(); security.add_label("PII"); security.add_label("HIPAA"); @@ -1161,12 +1140,6 @@ mod tests { #[test] fn test_append_only_plugin_labels_survive_nonempty_canonical() { - // Regression: a plugin holding `append_labels` but not `read_labels` - // sees an empty filtered label set, so its returned set contains only - // its additions, not a superset of the canonical set. The old - // superset gate in `merge_security` dropped those additions whenever - // canonical was non-empty, silently disabling a write-only tainting / - // DLP plugin. Folding must now land the addition. let mut security = SecurityExtension::default(); security.add_label("EXISTING"); @@ -1177,7 +1150,6 @@ mod tests { ext.labels_write_token = Some(WriteToken::new()); let mut cow = ext.cow_copy(); - // Filtered view for a no-read plugin is empty; it appends its label. let mut added = std::collections::HashSet::new(); added.insert("PII".to_owned()); cow.security.as_mut().unwrap().labels = crate::extensions::MonotonicSet::from_set(added); @@ -1369,8 +1341,6 @@ mod tests { }; ext.delegation_write_token = Some(WriteToken::new()); - // Rewrite the existing hop's RFC 9396 authorization_details to add - // actions (a widening) and extend its ttl. Neither is an append. let mut cow = ext.cow_copy(); { let hop = &mut cow.delegation.as_mut().unwrap().chain[0]; diff --git a/crates/ppe-core/src/extensions/raw_credentials.rs b/crates/ppe-core/src/extensions/raw_credentials.rs index f65dba8..9a10f87 100644 --- a/crates/ppe-core/src/extensions/raw_credentials.rs +++ b/crates/ppe-core/src/extensions/raw_credentials.rs @@ -435,24 +435,16 @@ pub struct RawCredentialsExtension { /// `read_delegated_tokens`; write with `write_delegated_tokens` /// (`TokenDelegate` handlers only). /// - /// Serialized as a sequence of `[key, value]` pairs, not a JSON object: - /// the key is a `DelegationKey` struct and JSON object keys must be - /// strings, so a plain map errors at runtime the moment a token is minted, - /// breaking the `extensions` wire channel, audit dumps, and hot-reload - /// snapshots. The pair form round-trips, and token bytes inside each value - /// stay `#[serde(skip)]`. + /// Serialized as `[key, value]` pairs because JSON object keys must be + /// strings. Token bytes remain excluded by `#[serde(skip)]`. #[serde(default, with = "delegated_tokens_as_pairs")] pub delegated_tokens: HashMap, } -/// serde adapter: represent `delegated_tokens` as a sequence of -/// `(DelegationKey, RawDelegatedToken)` pairs so a non-string map key -/// serializes to JSON. See the field doc for why a plain map cannot. +/// Serialize `delegated_tokens` as pairs so its structured keys work in JSON. /// -/// Deserialization accepts both the pair form and a plain map. Before this -/// adapter only an empty `delegated_tokens` ever serialized, and it did so as -/// `{}`, so snapshots and wire messages written by that older code must still -/// load. This keeps mixed-version rollouts readable. +/// Deserialization also accepts the legacy map form; older code could emit an +/// empty map even though non-empty structured-key maps failed to serialize. mod delegated_tokens_as_pairs { use super::{DelegationKey, RawDelegatedToken}; use serde::de::{MapAccess, SeqAccess, Visitor}; @@ -485,10 +477,7 @@ mod delegated_tokens_as_pairs { } fn visit_map>(self, mut access: A) -> Result { - // Legacy shape: a JSON object. Only the empty case was ever - // emitted (a non-empty struct-keyed map could not serialize), so - // this is normally `{}`; still, drain any entries a hand-built - // document carries. + // Accept legacy JSON objects, normally `{}`. let mut map = HashMap::with_capacity(access.size_hint().unwrap_or(0)); while let Some((k, v)) = access.next_entry::()? { map.insert(k, v); @@ -679,11 +668,6 @@ mod tests { #[test] fn delegated_tokens_serialize_without_error_and_round_trip() { - // Regression: `delegated_tokens` is keyed by the `DelegationKey` - // struct, which cannot be a JSON object key. A plain map serialization - // errored the moment a token was minted, breaking the documented wire - // channel. The pair-seq form must serialize cleanly, drop the token - // bytes, and round-trip the key + metadata. let mut ext = RawCredentialsExtension::default(); let key = DelegationKey::new( DelegationMode::OnBehalfOfUser, @@ -702,7 +686,6 @@ mod tests { ), ); - // The whole point: this used to return Err("key must be a string"). let json = serde_json::to_string(&ext).expect("delegated_tokens must serialize"); assert!( !json.contains("minted-secret"), @@ -720,15 +703,10 @@ mod tests { #[test] fn legacy_empty_map_delegated_tokens_still_deserializes() { - // Before the pair-seq adapter, only an empty delegated_tokens ever - // serialized, and it did so as a JSON object `{}`. A snapshot written - // by that older code must still load, so the deserializer accepts a map - // as well as the new sequence form. let legacy = r#"{"inbound_tokens":{},"delegated_tokens":{}}"#; let restored: RawCredentialsExtension = serde_json::from_str(legacy).unwrap(); assert!(restored.delegated_tokens.is_empty()); - // And the new sequence form loads too. let modern = r#"{"inbound_tokens":{},"delegated_tokens":[]}"#; let restored: RawCredentialsExtension = serde_json::from_str(modern).unwrap(); assert!(restored.delegated_tokens.is_empty()); diff --git a/crates/ppe-core/tests/config_key_sets.rs b/crates/ppe-core/tests/config_key_sets.rs index 4af21f2..86bec0b 100644 --- a/crates/ppe-core/tests/config_key_sets.rs +++ b/crates/ppe-core/tests/config_key_sets.rs @@ -639,10 +639,6 @@ fn a_misspelled_authentication_object_key_is_rejected() { } /// Both accepted shapes still load, and the flag still reads through. -/// -/// Each declares the `jwt` plugin the step names: an `authentication:` step -/// matching no `plugins:` entry is refused at load, since it would resolve to -/// nothing at dispatch and leave the route unauthenticated. #[test] fn the_authentication_object_shapes_still_load() { const PLUGINS: &str = From 08b76e6820ec18be64af53279b5b34c4a70bf5df Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 1 Sep 2026 21:57:10 -0400 Subject: [PATCH 20/20] docs: keep the invariants the condensing dropped Name the fields `chain_extends` compares, since the bug it fixed was three of them missing and an unnamed set invites dropping one again. Restore the `DeclassifierToken` requirement and a line on each label test saying what regressed, so neither reads as redundant later. Signed-off-by: Frederico Araujo --- crates/ppe-core/src/extensions/container.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/ppe-core/src/extensions/container.rs b/crates/ppe-core/src/extensions/container.rs index dc0b09d..5d9d213 100644 --- a/crates/ppe-core/src/extensions/container.rs +++ b/crates/ppe-core/src/extensions/container.rs @@ -365,7 +365,8 @@ impl Extensions { /// /// Labels are the Monotonic tier: `append_labels` permits growing the set /// and nothing else. Folding returned labels into the canonical set makes - /// removal structurally impossible. + /// removal structurally impossible; it requires a `DeclassifierToken` no + /// plugin can construct. /// /// Capability-aware laundering detection happens earlier in `labels_ok`; /// this layer cannot distinguish a filtered view from a removal attempt. @@ -444,8 +445,10 @@ impl Extensions { /// True when `returned` is `canonical` plus zero or more appended hops. /// -/// Existing hops must match on every authority-bearing or bounding field. -/// `from_cache` is merge bookkeeping and is not compared. +/// Existing hops must match on every authority-bearing or bounding field: +/// subject, audience, granted scopes, strategy, `authorization_details`, +/// `ttl_seconds`, and `timestamp`. Dropping any of them reopens a widening +/// path. `from_cache` is merge bookkeeping and is not compared. /// /// Public so out-of-process hosts can apply the same validation to a chain /// arriving over the wire before it reaches the merge, instead of reimplementing @@ -1111,6 +1114,7 @@ mod tests { #[test] fn test_label_removal_is_refused_by_folding() { + // Folding, not a superset gate, is what refuses the removal. let mut security = SecurityExtension::default(); security.add_label("PII"); security.add_label("HIPAA"); @@ -1140,6 +1144,9 @@ mod tests { #[test] fn test_append_only_plugin_labels_survive_nonempty_canonical() { + // Regression: a superset gate here discarded the labels of a plugin + // holding `append_labels` but not `read_labels`, which sees an empty + // filtered set and so never returns a superset of canonical. let mut security = SecurityExtension::default(); security.add_label("EXISTING");