diff --git a/builtins/pdps/cedar-direct/src/factory.rs b/builtins/pdps/cedar-direct/src/factory.rs index 43393d0..e1c2719 100644 --- a/builtins/pdps/cedar-direct/src/factory.rs +++ b/builtins/pdps/cedar-direct/src/factory.rs @@ -7,12 +7,13 @@ // // ```yaml // global: -// pdp: -// - kind: cedar-direct -// dialect: cedar # optional, defaults to PdpDialect::Cedar -// policy_text: | # required (or policy_file) -// @id("owner-override") -// permit(...); +// apl: +// pdp: +// - kind: cedar-direct +// dialect: cedar # optional, defaults to PdpDialect::Cedar +// policy_text: | # required (or policy_file) +// @id("owner-override") +// permit(...); // ``` // // Hosts register an instance of this factory in `AplOptions.pdp_factories`; 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 diff --git a/builtins/pdps/cel/src/factory.rs b/builtins/pdps/cel/src/factory.rs index 12b75f1..448d013 100644 --- a/builtins/pdps/cel/src/factory.rs +++ b/builtins/pdps/cel/src/factory.rs @@ -6,9 +6,10 @@ // // ```yaml // global: -// pdp: -// - kind: cel -// on_error: deny # optional; deny | allow, default deny +// apl: +// pdp: +// - kind: cel +// on_error: deny # optional; deny | allow, default deny // ``` // // The CEL expression itself lives in each route's `cel: { expr: "..." }` diff --git a/builtins/pdps/cel/src/resolver.rs b/builtins/pdps/cel/src/resolver.rs index 9dc034c..4f66122 100644 --- a/builtins/pdps/cel/src/resolver.rs +++ b/builtins/pdps/cel/src/resolver.rs @@ -209,8 +209,8 @@ impl CelResolver { /// eager-compile knob of its own. /// # Errors /// - /// Returns `BuildError` when the block is not a mapping or a setting is out - /// of range, such as a zero cache cap. + /// Returns `BuildError` when the block is not a mapping, carries an unknown + /// key, or gives `on_error` a value other than `deny` / `allow`. pub fn from_config(value: &serde_yaml::Value) -> Result { let map = value .as_mapping() diff --git a/builtins/pdps/opa/src/input.rs b/builtins/pdps/opa/src/input.rs index ab6b343..a5b2cb7 100644 --- a/builtins/pdps/opa/src/input.rs +++ b/builtins/pdps/opa/src/input.rs @@ -130,8 +130,11 @@ fn attr_to_value(attr: &AttributeValue) -> Value { } /// Yield an `f64` as a JSON integer when it is a whole number in `i64` range, -/// otherwise a JSON float. Keeps parity with CEL's `float_to_value` so a bag -/// value populated as `Float(2.0)` reads as `2` for an author. A non-finite +/// otherwise a JSON float, so a bag value populated as `Float(2.0)` reads as +/// `2` for an author. Rego has a single unified `number` type, so narrowing a +/// whole-valued float never breaks arithmetic or comparison here the way it did +/// in CEL. CEL for that reason stopped narrowing and now keeps every float a +/// double, so the two PDPs deliberately diverge on this point. A non-finite /// float has no JSON representation and becomes `null`. #[allow( clippy::cast_possible_truncation, 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); diff --git a/builtins/plugins/delegator-oauth/src/factory.rs b/builtins/plugins/delegator-oauth/src/factory.rs index 9ae3a84..6b4c969 100644 --- a/builtins/plugins/delegator-oauth/src/factory.rs +++ b/builtins/plugins/delegator-oauth/src/factory.rs @@ -15,7 +15,7 @@ // config: // token_endpoint: https://idp.example.com/token // client_id: praxis-gateway -// client_secret_source: { kind: env, var: OAUTH_CLIENT_SECRET } +// client_secret_source: { kind: env_var, name: OAUTH_CLIENT_SECRET } // // The `kind: delegator/oauth` string is part of this crate's public // API. Hosts call diff --git a/builtins/plugins/elicitation-ciba/src/factory.rs b/builtins/plugins/elicitation-ciba/src/factory.rs index 6f72a7b..293bb84 100644 --- a/builtins/plugins/elicitation-ciba/src/factory.rs +++ b/builtins/plugins/elicitation-ciba/src/factory.rs @@ -14,7 +14,7 @@ // backchannel_endpoint: https://kc/realms/corp/protocol/openid-connect/ext/ciba/auth // token_endpoint: https://kc/realms/corp/protocol/openid-connect/token // client_id: praxis-policy-gateway -// client_secret_source: { kind: env, name: CIBA_CLIENT_SECRET } +// client_secret_source: { kind: env_var, name: CIBA_CLIENT_SECRET } // // Then policy routes name it: `require_approval(manager-approver, from: claim.manager, ...)`. // 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 { 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, diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 145df4f..22f21fb 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(), + }); + } }, } } @@ -1175,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, @@ -1201,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 (deny) 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. @@ -3120,6 +3145,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 { @@ -3793,6 +3833,100 @@ mod tests { ); } + #[tokio::test] + async fn field_op_in_do_fans_out_redact_over_result_array() { + // Regression guard for the dispatch_field_op fan-out: a do:-embedded + // `result.rows.ssn | redact` must reach EVERY row, not silently no-op + // because `rows` is an array. Only the route.rs result-section path had + // array coverage; the evaluator's FieldOp path did not. + let mut bag = AttributeBag::new(); + let rule = Rule { + condition: Expression::Always, + effects: vec![Effect::FieldOp { + path: "result.rows.ssn".into(), + stages: vec![Stage::Redact { condition: None }], + }], + source: "demo.policy[0]".into(), + }; + let steps = vec![Effect::from(rule)]; + let mut payload = crate::route::RoutePayload::with_result( + json!({}), + json!({ "rows": [ + { "ssn": "111-11-1111", "name": "a" }, + { "ssn": "222-22-2222", "name": "b" } + ]}), + ); + + let eval = evaluate_effects( + &steps, + &mut bag, + &(Arc::new(FakePdp { + decision: Decision::Allow, + }) as Arc), + &null_plugins(), + &noop_delegations(), + &noop_elicitations(), + crate::step::DispatchPhase::Post, + &mut payload, + ) + .await; + + assert_eq!(eval.decision, Decision::Allow); + assert!(eval.result_modified, "the fan-out 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]")); + assert_eq!( + result["rows"][0]["name"], + json!("a"), + "non-targeted fields untouched" + ); + } + + #[tokio::test] + async fn field_op_in_do_fans_out_omit_over_result_array() { + // Omit drives remove_dotted through the fan-out, a distinct path from + // redact's set_dotted: every row's `ssn` key must be removed. + let mut bag = AttributeBag::new(); + let rule = Rule { + condition: Expression::Always, + effects: vec![Effect::FieldOp { + path: "result.rows.ssn".into(), + stages: vec![Stage::Omit], + }], + source: "demo.policy[0]".into(), + }; + let steps = vec![Effect::from(rule)]; + let mut payload = crate::route::RoutePayload::with_result( + json!({}), + json!({ "rows": [ + { "ssn": "111-11-1111", "name": "a" }, + { "ssn": "222-22-2222", "name": "b" } + ]}), + ); + + let eval = evaluate_effects( + &steps, + &mut bag, + &(Arc::new(FakePdp { + decision: Decision::Allow, + }) as Arc), + &null_plugins(), + &noop_delegations(), + &noop_elicitations(), + crate::step::DispatchPhase::Post, + &mut payload, + ) + .await; + + assert_eq!(eval.decision, Decision::Allow); + assert!(eval.result_modified, "the fan-out omit must register"); + let result = payload.result.as_ref().unwrap(); + assert!(result["rows"][0].get("ssn").is_none(), "row 0 ssn omitted"); + assert!(result["rows"][1].get("ssn").is_none(), "row 1 ssn omitted"); + assert_eq!(result["rows"][0]["name"], json!("a")); + } + /// A plugin stage learns which field it's operating on from the /// invocation's `name`. That name is relative to the args / result /// root at every call site, so an invoker can look the field up in @@ -4063,6 +4197,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 diff --git a/crates/ppe-apl-core/src/parser.rs b/crates/ppe-apl-core/src/parser.rs index 28ca7e3..8342d90 100644 --- a/crates/ppe-apl-core/src/parser.rs +++ b/crates/ppe-apl-core/src/parser.rs @@ -1463,16 +1463,44 @@ fn parse_step_map(m: &serde_yaml::Mapping, source: &str) -> Result Result { } } +/// Deserialize a `HashMap` that rejects duplicate keys instead of +/// silently keeping the last one. +/// +/// `serde_yaml` catches duplicate keys only in `Value`/`Mapping`-typed positions +/// and in derived named struct fields; a plain `HashMap` field replays every +/// entry to `HashMap::insert`, so a repeated key is silently last-wins. On a +/// policy that means a whole route, redaction pipeline, or plugin override can +/// vanish with no diagnostic — a fail-open the engine's `deny_unknown_fields` +/// philosophy exists to prevent. This visitor errors on the first repeat. +fn deserialize_unique_string_map<'de, D, V>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, + V: serde::Deserialize<'de>, +{ + use serde::de::{Error as _, MapAccess, Visitor}; + struct UniqueMapVisitor(std::marker::PhantomData); + impl<'de, V: serde::Deserialize<'de>> Visitor<'de> for UniqueMapVisitor { + type Value = HashMap; + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a map with unique keys") + } + fn visit_map>(self, mut access: A) -> Result { + let mut map: HashMap = + HashMap::with_capacity(access.size_hint().unwrap_or(0)); + while let Some((key, value)) = access.next_entry::()? { + if map.contains_key(&key) { + return Err(A::Error::custom(format!("duplicate key `{key}`"))); + } + map.insert(key, value); + } + Ok(map) + } + } + deserializer.deserialize_map(UniqueMapVisitor(std::marker::PhantomData)) +} + /// Top-level config — only the bits the parser understands. /// /// `policy_evaluator:`, `imports:`, `global:`, `defaults:`, `tags:`, @@ -2685,7 +2749,7 @@ fn parse_taint_scope(s: &str, src: &str) -> Result { #[derive(Debug, Default, Deserialize)] pub struct ConfigYaml { /// The `routes:` block, keyed by route. - #[serde(default)] + #[serde(default, deserialize_with = "deserialize_unique_string_map")] pub routes: HashMap, /// Root `plugins:` block — full declarations. @@ -2718,17 +2782,17 @@ pub struct RouteYaml { pub authorization: Option, /// `args:` field → pipe-chain string. Compiled to per-field pipelines. - #[serde(default)] + #[serde(default, deserialize_with = "deserialize_unique_string_map")] pub args: HashMap, /// `result:` field → pipe-chain string. Compiled to per-field pipelines. - #[serde(default)] + #[serde(default, deserialize_with = "deserialize_unique_string_map")] pub result: HashMap, /// Per-route plugin overrides — only the spec-overridable keys /// (config / capabilities / `on_error`). Merged on top of the root /// `plugins:` declaration at dispatch time. - #[serde(default)] + #[serde(default, deserialize_with = "deserialize_unique_string_map")] pub plugins: HashMap, /// Anything else on the route (meta, taint, when) — stashed. Also @@ -2930,6 +2994,34 @@ 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) } @@ -3612,6 +3704,37 @@ do: )); } + #[test] + fn restrict_with_sequence_body_errors_not_misparses() { + // Regression: `- restrict: [allow]` (a sequence body under the + // `restrict` step keyword) must not be swallowed by the predicate + // shorthand as `IsTrue("restrict")` — a rule that can never fire and + // silently discards the intended constraint. It now reaches the real + // restrict handler and errors on the non-map body. + let err = parse_step_yaml("restrict:\n - allow").unwrap_err(); + // It must reach the restrict handler (which rejects a non-map body), + // not compile to a predicate rule on an attribute named `restrict`. + assert!( + format!("{err}").to_lowercase().contains("restrict"), + "expected a restrict-handler error, got {err}" + ); + } + + #[test] + fn reserved_step_keyword_as_map_key_errors() { + // `taint:` map and a lone `do:` / `when:` single-key map are author + // errors — previously they compiled to `IsTrue("taint")` etc. and + // never fired. They must now be rejected with a pointer to the right + // shape. + for yaml in ["taint:\n - deny", "do:\n - deny", "when:\n - deny"] { + let err = parse_step_yaml(yaml).unwrap_err(); + assert!( + format!("{err}").contains("not a valid step map key"), + "expected a helpful rejection for {yaml:?}, got {err}" + ); + } + } + #[test] fn shorthand_multi_effect_map_with_nested_delegate() { // Map-form effects (like `delegate:`) work inside a shorthand @@ -4189,6 +4312,77 @@ routes: ); } + #[test] + fn two_elicit_steps_in_one_phase_rejected() { + // Two elicitations in one phase share a single retry id; the second + // would resolve against the first's approval. Reject at load. + let yaml = r#" +routes: + payroll: + pre_invocation: + - "confirm(approver, from: user.sub)" + - "require_approval(approver, from: user.manager)" +"#; + let err = compile_config(yaml).unwrap_err(); + assert!( + format!("{err}").contains("at most one elicitation per phase"), + "expected 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. + let yaml = r#" +routes: + payroll: + pre_invocation: + - "require_approval(approver, from: user.manager)" + post_invocation: + - "confirm(auditor, from: user.sub)" +"#; + compile_config(yaml).expect("one elicit per phase must compile"); + } + + #[test] + fn duplicate_route_key_is_rejected_not_last_wins() { + // Two routes with the same key: serde would keep the last and silently + // drop the first's authorization block — a fail-open. It must error. + let yaml = r#" +routes: + dup: + pre_invocation: + - "require(authenticated)" + dup: + result: + ssn: "redact" +"#; + let err = compile_config(yaml).unwrap_err(); + assert!( + format!("{err}").contains("duplicate key"), + "expected duplicate-key error, got {err}" + ); + } + + #[test] + fn duplicate_field_pipeline_key_is_rejected() { + // `result:` with the same field twice would silently drop one pipeline. + // Dropping a `redact` in favor of a passthrough leaks the field. + let yaml = r#" +routes: + r: + result: + ssn: "redact" + ssn: "hash" +"#; + let err = compile_config(yaml).unwrap_err(); + assert!( + format!("{err}").contains("duplicate key"), + "expected duplicate-key error, got {err}" + ); + } + #[test] fn authorization_nested_and_flat_forms_are_equivalent() { // The nested `authorization:` block and the flat diff --git a/crates/ppe-apl-core/src/route.rs b/crates/ppe-apl-core/src/route.rs index 14dd2c1..44916ad 100644 --- a/crates/ppe-apl-core/src/route.rs +++ b/crates/ppe-apl-core/src/route.rs @@ -111,46 +111,68 @@ 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`, not 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 +222,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 arrays on the path (e.g. redact `rows.ssn` on every + // row); an object-only path expands to itself. An over-large shape + // fails closed rather than half-redacting. + 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_policy (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_policy (below); a + // result-pipeline deny short-circuits before it. + constraints: Vec::new(), + args_modified: false, + result_modified, + pending: None, + }; + }, + } } } } @@ -312,8 +355,23 @@ pub async fn evaluate_route( } } +/// Index one JSON value by a single path segment. An object is indexed by the +/// segment as a key; an array is indexed by the segment parsed as a base-10 +/// index. Anything else (or an out-of-range / non-numeric array index) is +/// `None`. Shared by the read/write/remove helpers so a numeric segment means +/// the same thing everywhere. +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, + } +} + /// 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 +380,127 @@ 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) +} + +/// 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 redaction must reach every element — not +/// silently no-op because `Value::get("rows")` on... an array of objects +/// returns the object but the *next* hop lands on an array. Each array +/// encountered before the final segment fans out into one concrete path per +/// index (`rows.0.ssn`, `rows.1.ssn`, …). Paths whose leaf is absent on a +/// given element are dropped (consistent with the missing-field skip rule). +/// +/// A terminal array is left as a single path (`result.rows | redact` replaces +/// the whole array), preserving existing whole-value pipeline semantics. +/// +/// Returns `None` when the expansion would exceed a fan-out bound (array +/// nesting deeper than [`MAX_FANOUT_DEPTH`], or more than [`MAX_EXPANDED_PATHS`] +/// leaves). Fanning out over attacker-shaped tool output (`payload.result`) is +/// otherwise unbounded in stack depth and allocation, so an over-large shape +/// fails closed: the caller denies rather than either 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 — `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 → skip (no path emitted) + }; + 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) +} + +/// 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 (or a +/// pathologically deep one right at the parse limit reached under a +/// multi-segment path) 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; + +/// 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 (a path from +/// [`expand_field_paths`] carries numeric segments for array elements). +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) } /// 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 +511,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, } } @@ -1224,4 +1399,219 @@ mod tests { "nor written through" ); } + + #[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 — whole-value pipeline 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() { + // A pathologically deep array nesting must fail closed (None), not + // recurse without bound. Build a value nested past MAX_FANOUT_DEPTH. + 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 (MAX_EXPANDED_PATHS) must fail closed too, not just + // the depth bound covered above: a single array wide enough to exceed + // the leaf cap returns None so the caller denies rather than + // half-redacting a huge result 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" + ); + } + + #[tokio::test] + async fn route_result_over_large_fanout_denies() { + // When a result field path fans out past the bound, evaluate_post must + // DENY (fail closed) rather than pass the un-redacted tail through. Use + // a shape nested past MAX_FANOUT_DEPTH (cheap) 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 route_args_over_large_fanout_denies() { + // Same fail-closed guarantee on the args (pre) phase. + let mut route = CompiledRoute::new("ping"); + route.args.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::new(json!({ "rows": nested })); + + let r = evaluate_route( + &route, + &mut bag, + &mut payload, + &pdp_arc(), + &plugins(), + &delegations(), + &elicitations(), + ) + .await; + match r.decision { + Decision::Deny { reason, .. } => { + let reason = reason.unwrap_or_default(); + assert!(reason.contains("args field"), "reason: {reason}"); + assert!( + reason.contains("too many elements to redact safely"), + "reason: {reason}" + ); + }, + other => panic!("over-large args fan-out must deny, got {other:?}"), + } + assert!( + !r.args_modified, + "nothing may be redacted on a fail-closed deny" + ); + } + + #[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() { + // The finding: `result.rows.ssn | redact` must reach every row, not + // silently no-op because `rows` is an array. Without the fan-out the + // SSNs would pass through unredacted — 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")); + } } diff --git a/crates/ppe-apl-core/src/rules.rs b/crates/ppe-apl-core/src/rules.rs index d245b6c..f4dc4ae 100644 --- a/crates/ppe-apl-core/src/rules.rs +++ b/crates/ppe-apl-core/src/rules.rs @@ -334,6 +334,49 @@ 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 retry state is carried in a *single* + /// per-request key (the agent echoes one id), so two elicit steps in one + /// phase share that key: the second step would skip its own dispatch and + /// resolve against the first step's id — a stronger `require_approval` + /// silently satisfied by a weaker `confirm`. Correct multi-elicit needs a + /// per-step id the current single-id retry protocol cannot carry, so the + /// safe posture is to reject the configuration at load rather than + /// mis-evaluate it at runtime. + /// + /// 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 — 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 (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 @@ -1039,6 +1082,43 @@ mod tests { assert!(outer.validate_parallel_purity().is_err()); } + #[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 (same evaluation walk). + assert_eq!( + Effect::When { + condition: crate::rules::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 + ); + } + #[test] fn validate_top_level_sequential_allows_mutations() { // FieldOp / Delegate are allowed under Sequential (or at top 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-apl-runtime/src/route_handler.rs b/crates/ppe-apl-runtime/src/route_handler.rs index d3d5ad4..527784d 100644 --- a/crates/ppe-apl-runtime/src/route_handler.rs +++ b/crates/ppe-apl-runtime/src/route_handler.rs @@ -740,7 +740,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 @@ -1019,6 +1035,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. /// @@ -1072,5 +1115,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" + ); } } diff --git a/crates/ppe-apl-runtime/src/visitor.rs b/crates/ppe-apl-runtime/src/visitor.rs index 8566c9d..d88ef08 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -804,6 +804,34 @@ impl ConfigVisitor for AplConfigVisitor { return Err(err_msg.into()); } + // Reject a phase that can reach more than one elicitation. Retry + // state for an elicit step is carried in a single per-request key + // (the agent echoes one id), so two elicit steps in the same phase + // share it: the second resolves against the first's id instead of + // dispatching its own — a stronger `require_approval` silently + // satisfied by a weaker `confirm`. Checked on the fully-stacked + // route so an elicit inherited from a global / tag layer plus one on + // the route also trips it. Correct multi-elicit needs a per-step id + // the current single-id protocol cannot carry, so this is rejected + // at load rather than mis-evaluated at runtime. + for (phase, effects) in [ + ("pre_invocation", &effective.policy), + ("post_invocation", &effective.post_policy), + ] { + let elicits: usize = effects + .iter() + .map(praxis_policy_apl_core::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()); + } + } + let route_arc = Arc::new(effective); // Resolve the entity-specific CMF hook pair. The visitor's diff --git a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs index c3d2eef..5ab9fa2 100644 --- a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs +++ b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs @@ -26,23 +26,9 @@ use std::sync::Arc; use praxis_policy_apl_runtime::{AplOptions, register_apl}; use praxis_policy_core::engine::PolicyEngine; -/// Load with the APL visitor installed and no factories registered, and return -/// the error text. Registering none is the point for most cases here: it is what -/// an operator hits when they name a `kind` the host never wired up. -fn load_err(yaml: &str) -> String { - let mgr = Arc::new(PolicyEngine::default()); - register_apl(&mgr, AplOptions::in_process()); - match mgr.load_config_yaml(yaml) { - Ok(()) => panic!("this config must not load"), - Err(e) => format!("{e}"), - } -} - -fn loads(yaml: &str) { - let mgr = Arc::new(PolicyEngine::default()); - register_apl(&mgr, AplOptions::in_process()); - mgr.load_config_yaml(yaml).expect("this config must load"); -} +// ----------------------------------------------------------------------------- +// Tests +// ----------------------------------------------------------------------------- /// The control. Everything below differs from this by one deliberate mistake, so /// without it a rejection could be attributable to something else in the block. @@ -51,7 +37,7 @@ fn a_config_with_no_wiring_block_loads() { loads("plugin_settings:\n routing_enabled: true\n"); } -// ---- global.apl.pdp ---------------------------------------------------- +// global.apl.pdp #[test] fn a_pdp_entry_that_is_not_a_mapping_is_rejected_with_its_index() { @@ -100,7 +86,7 @@ fn the_reported_index_identifies_which_pdp_entry_failed() { ); } -// ---- global.apl.session_store ------------------------------------------ +// global.apl.session_store #[test] fn a_session_store_that_is_not_a_mapping_is_rejected() { @@ -129,7 +115,7 @@ fn a_session_store_kind_with_no_registered_factory_is_rejected() { ); } -// ---- renamed and misplaced keys ---------------------------------------- +// renamed and misplaced keys /// `identity:` was renamed to `authentication:`, and a stale one is rejected /// rather than ignored. An unknown field is dropped silently, which would leave @@ -172,7 +158,7 @@ fn a_visitor_error_is_attributed_to_the_visitor() { ); } -// ---- global.apl.attribute_files ---------------------------------------- +// global.apl.attribute_files /// `attribute_files` supplies the static `data.*` tree a policy reads. Every way /// of getting it wrong has to fail the load. @@ -217,3 +203,55 @@ fn an_attribute_file_that_does_not_exist_is_rejected() { fn an_empty_attribute_files_list_loads() { loads("global:\n apl:\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. It must be rejected at load, not mis-evaluated. +#[test] +fn a_global_and_route_elicit_in_one_phase_is_rejected() { + let e = load_err( + "plugin_settings:\n routing_enabled: true\nglobal:\n apl:\n pre_invocation:\n - \"require_approval(manager-approver, from: user.manager)\"\nroutes:\n - tool: get_compensation\n apl:\n pre_invocation:\n - \"confirm(user-confirm, from: user.sub)\"\n", + ); + assert!( + e.contains("at most one elicitation per phase"), + "a global+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( + "plugin_settings:\n routing_enabled: true\nglobal:\n apl:\n pre_invocation:\n - \"require_approval(manager-approver, from: user.manager)\"\nroutes:\n - tool: get_compensation\n apl:\n post_invocation:\n - \"confirm(user-confirm, from: user.sub)\"\n", + ); +} + +// ----------------------------------------------------------------------------- +// Test Utilities +// ----------------------------------------------------------------------------- + +/// Load with the APL visitor installed and no factories registered, and return +/// the error text. Registering none is the point for most cases here: it is what +/// an operator hits when they name a `kind` the host never wired up. +fn load_err(yaml: &str) -> String { + let mgr = Arc::new(PolicyEngine::default()); + register_apl(&mgr, AplOptions::in_process()); + match mgr.load_config_yaml(yaml) { + Ok(()) => panic!("this config must not load"), + Err(e) => format!("{e}"), + } +} + +fn loads(yaml: &str) { + let mgr = Arc::new(PolicyEngine::default()); + register_apl(&mgr, AplOptions::in_process()); + mgr.load_config_yaml(yaml).expect("this config must load"); +} diff --git a/crates/ppe-core/src/config.rs b/crates/ppe-core/src/config.rs index 69b8cd0..263cf55 100644 --- a/crates/ppe-core/src/config.rs +++ b/crates/ppe-core/src/config.rs @@ -805,6 +805,33 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box = config.plugins.iter().map(|p| p.name.as_str()).collect(); + // Validate the `authentication:` step names the same way `plugins:` + // names are validated. An unresolvable step name is silently skipped at + // dispatch (`filter_entries_by_route` finds no matching entry and drops + // it with no error), so a typo leaves that authentication step unrun — + // the exact fail-open `parse_config` guards against for the renamed + // `identity:` key. Global, group, and route authentication all share + // one shape, so one check covers all three. + let validate_identity = |identity: &Option, + context: &str| + -> Result<(), Box> { + if let Some(identity) = identity { + for step in &identity.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_identity(&config.global.identity, "global")?; + for (i, route) in config.routes.iter().enumerate() { let count = [ route.tool.is_some(), @@ -857,6 +884,8 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box Result<(), Box, + tags: Vec, } impl Hash for RouteCacheKey { @@ -123,6 +132,10 @@ impl Hash for RouteCacheKey { self.entity_name.as_str().hash(state); self.hook_name.as_str().hash(state); self.scope.as_deref().hash(state); + self.tags.len().hash(state); + for tag in &self.tags { + tag.as_str().hash(state); + } } } @@ -132,6 +145,7 @@ impl PartialEq for RouteCacheKey { && self.entity_name == other.entity_name && self.hook_name == other.hook_name && self.scope == other.scope + && self.tags == other.tags } } @@ -1679,6 +1693,15 @@ impl PolicyEngine { let request_scope = meta.scope.as_deref(); + // Canonical (sorted, deduped) view of the request tags. Tags activate + // policy groups during resolution, so they are part of the cache + // identity; sorting makes the key independent of `HashSet` order. + let request_tags: Vec<&str> = { + let mut t: Vec<&str> = meta.tags.iter().map(String::as_str).collect(); + t.sort_unstable(); + t + }; + // Fast path: zero-allocation cache lookup with raw_entry let hash = { use std::hash::BuildHasher as _; @@ -1687,6 +1710,10 @@ impl PolicyEngine { entity_name.hash(&mut hasher); hook_name.hash(&mut hasher); request_scope.hash(&mut hasher); + request_tags.len().hash(&mut hasher); + for tag in &request_tags { + tag.hash(&mut hasher); + } hasher.finish() }; { @@ -1705,6 +1732,12 @@ impl PolicyEngine { && key.entity_name == entity_name && key.hook_name == hook_name && key.scope.as_deref() == request_scope + && key.tags.len() == request_tags.len() + && key + .tags + .iter() + .zip(request_tags.iter()) + .all(|(a, b)| a == b) }) { return Arc::clone(cached); } @@ -1767,6 +1800,7 @@ impl PolicyEngine { entity_name: entity_name.to_owned(), hook_name: hook_name.to_owned(), scope: meta.scope.clone(), + tags: request_tags.iter().map(|t| (*t).to_owned()).collect(), }; // Decide under the lock; log outside it so I/O doesn't block readers. // One warn per fill cycle — prevents log spam under DoS. @@ -2285,6 +2319,39 @@ mod tests { } } + /// A blocking plugin that halts (`continue_processing=false`) but attaches NO + /// violation. This is exactly the shape the executor must synthesize a deny + /// for, rather than letting it fall through to the allow / modification path. + struct SilentBlockPlugin { + cfg: PluginConfig, + } + + #[async_trait] + impl Plugin for SilentBlockPlugin { + fn config(&self) -> &PluginConfig { + &self.cfg + } + async fn initialize(&self) -> Result<(), Box> { + Ok(()) + } + async fn shutdown(&self) -> Result<(), Box> { + Ok(()) + } + } + + impl HookHandler for SilentBlockPlugin { + async fn handle( + &self, + _payload: &TestPayload, + _extensions: &Extensions, + _ctx: &mut PluginContext, + ) -> PluginResult { + let mut r = PluginResult::allow(); + r.continue_processing = false; // block, but attach no violation + r + } + } + /// Handler that always returns an error (for testing `on_error` behavior). struct ErrorHandler; @@ -2460,6 +2527,38 @@ mod tests { assert_eq!(result.violation.as_ref().unwrap().code, "denied"); } + #[tokio::test] + async fn test_blocking_plugin_without_violation_synthesizes_deny() { + // A sequential (blocking) plugin that returns continue_processing=false + // with NO violation must still deny, not fall through to allow. The + // executor synthesizes a `plugin_deny` violation naming the plugin. + let mgr = PolicyEngine::default(); + let config = make_config("silent-blocker", 10, PluginMode::Sequential); + let plugin = Arc::new(SilentBlockPlugin { + cfg: config.clone(), + }); + mgr.register_handler::(plugin, config).unwrap(); + mgr.initialize().await.unwrap(); + + let payload: Box = Box::new(TestPayload { + value: "test".into(), + }); + let (result, _) = mgr + .invoke_by_name("test_hook", payload, Extensions::default(), None) + .await; + + assert!( + !result.continue_processing, + "a blocking plugin without a violation must still deny" + ); + let v = result + .violation + .as_ref() + .expect("a deny must carry a synthesized violation"); + assert_eq!(v.code, "plugin_deny"); + assert_eq!(v.plugin_name.as_deref(), Some("silent-blocker")); + } + #[tokio::test] async fn test_invoke_typed() { let mgr = PolicyEngine::default(); @@ -6507,10 +6606,11 @@ routes: .await; assert!(r1.continue_processing); - // Clear cache so new tags take effect - mgr.clear_routing_cache(); - - // With urgent tag from host → denier also fires → denied + // With urgent tag from host → denier also fires → denied. No cache + // clear between the two invokes: the tag set is part of the route + // cache key, so the tagged request must not collide with the untagged + // entry the first invoke populated. A stale hit here would run the + // untagged (allow-only) lineup and fail open. let p2: Box = Box::new(TestPayload { value: "t".into() }); let (r2, _) = mgr .invoke_by_name( @@ -6521,6 +6621,18 @@ routes: ) .await; assert!(!r2.continue_processing); + + // And back to no tag → cached untagged (allow) entry still stands. + let p3: Box = Box::new(TestPayload { value: "t".into() }); + let (r3, _) = mgr + .invoke_by_name( + "test_hook", + p3, + make_meta("tool", "get_compensation", None, &[]), + None, + ) + .await; + assert!(r3.continue_processing, "untagged request stays allowed"); } #[tokio::test] @@ -6667,6 +6779,81 @@ routes: assert!(sec.has_label("PLUGIN_ADDED")); } + /// Handler holding `read_labels` that returns a NON-superset label set: it + /// drops the canonical `ORIGINAL` and substitutes `LAUNDERED`. A `read_labels` + /// plugin saw the real labels, so an absent one is a removal attempt. + struct LabelLaunderHandler; + + #[async_trait] + impl AnyHookHandler for LabelLaunderHandler { + async fn invoke( + &self, + _payload: &dyn PluginPayload, + extensions: &Extensions, + _ctx: &mut PluginContext, + ) -> Result, Box> { + let mut ext = extensions.cow_copy(); + let mut laundered = std::collections::HashSet::new(); + laundered.insert("LAUNDERED".to_owned()); + if let Some(ref mut sec) = ext.security { + sec.labels = crate::extensions::MonotonicSet::from_set(laundered); + } + let mut result: PluginResult = PluginResult::allow(); + result.modified_extensions = Some(ext); + Ok(crate::executor::erase_result(result)) + } + fn hook_type_name(&self) -> &'static str { + "test_hook" + } + } + + #[tokio::test] + async fn test_executor_drops_whole_edit_on_read_labels_laundering() { + // The commit relocated the label-laundering defense out of merge_security + // and into the executor's `labels_ok`. Pin it end-to-end: a `read_labels` + // plugin that returns a non-superset (drops ORIGINAL, adds LAUNDERED) must + // have its WHOLE edit dropped, not folded. If `labels_ok` were gone, + // merge_security's folding would still refuse the removal but would ADD + // LAUNDERED, so the discriminating assertion is that LAUNDERED never lands. + let mgr = PolicyEngine::default(); + let mut config = make_config("label-launderer", 10, PluginMode::Sequential); + config.capabilities = ["append_labels".to_owned(), "read_labels".to_owned()].into(); + let plugin = Arc::new(AllowPlugin { + cfg: config.clone(), + }); + let handler: Arc = Arc::new(LabelLaunderHandler); + mgr.register_raw::(plugin, config, handler) + .unwrap(); + mgr.initialize().await.unwrap(); + + let mut security = crate::extensions::SecurityExtension::default(); + security.add_label("ORIGINAL"); + let ext = Extensions { + security: Some(Arc::new(security)), + ..Default::default() + }; + + let payload: Box = Box::new(TestPayload { + value: "test".into(), + }); + let (result, _) = mgr.invoke_by_name("test_hook", payload, ext, None).await; + + assert!(result.continue_processing); + let modified = result + .modified_extensions + .as_ref() + .expect("accumulated extensions present"); + let sec = modified.security.as_ref().expect("security slot present"); + assert!( + sec.has_label("ORIGINAL"), + "the canonical label survives the refused removal" + ); + assert!( + !sec.has_label("LAUNDERED"), + "the whole laundering edit is dropped by labels_ok, not folded in" + ); + } + #[tokio::test] async fn test_executor_rejects_immutable_tampering() { let mgr = PolicyEngine::default(); @@ -7108,10 +7295,11 @@ plugins: assert_eq!(names, vec!["first".to_owned(), "second".to_owned()]); } - /// The route cache is keyed on all four fields. `Hash` is derived and used; - /// `PartialEq` is hand-written, so a field omitted there would make two + /// The route cache is keyed on all five fields. `Hash` and `PartialEq` are + /// both hand-written, so a field omitted from either would make two /// distinct routes collide in the cache and one would be served the other's - /// filtered entry list. + /// filtered entry list. `tags` in particular are load-bearing — they + /// activate policy groups. #[test] fn the_route_cache_key_distinguishes_every_field() { let base = RouteCacheKey { @@ -7119,6 +7307,7 @@ plugins: entity_name: "get_x".into(), hook_name: "cmf.tool_pre_invoke".into(), scope: None, + tags: vec![], }; assert_eq!(base, base.clone(), "a key equals itself"); @@ -7139,6 +7328,10 @@ plugins: scope: Some("read".into()), ..base.clone() }, + RouteCacheKey { + tags: vec!["urgent".into()], + ..base.clone() + }, ]; for v in variants { assert_ne!( diff --git a/crates/ppe-core/src/error.rs b/crates/ppe-core/src/error.rs index 1b65a4f..fb965be 100644 --- a/crates/ppe-core/src/error.rs +++ b/crates/ppe-core/src/error.rs @@ -30,7 +30,8 @@ pub enum PluginError { plugin_name: String, /// What went wrong. message: String, - /// Business-logic error code (e.g., `"invalid_token"`). + /// The underlying error that caused this failure, if any — the + /// error-chain `source` for `{:?}`/`Display` walking. #[source] source: Option>, /// Business-logic error code set by the plugin. @@ -82,7 +83,7 @@ impl PluginError { /// Box this error for use in `Result>`. /// /// Public APIs return `Result>` rather than - /// `Result>` because the enum is large (~184 bytes + /// `Result` because the enum is large (~184 bytes /// — `details: HashMap` and the `source: Box` push it /// well past clippy's `result_large_err` threshold). Boxing keeps /// `Result` pointer-sized on the success path; the diff --git a/crates/ppe-core/src/executor.rs b/crates/ppe-core/src/executor.rs index 163dcd9..f5ec44a 100644 --- a/crates/ppe-core/src/executor.rs +++ b/crates/ppe-core/src/executor.rs @@ -486,10 +486,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); } diff --git a/crates/ppe-core/src/extensions/container.rs b/crates/ppe-core/src/extensions/container.rs index 48cf707..0ed58d9 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()); } @@ -450,9 +456,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 +484,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 }) } @@ -1114,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"); @@ -1135,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" ); } @@ -1255,6 +1311,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; 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()); + } }