diff --git a/builtins/pdps/cel/src/activation.rs b/builtins/pdps/cel/src/activation.rs index 9812e54..8551550 100644 --- a/builtins/pdps/cel/src/activation.rs +++ b/builtins/pdps/cel/src/activation.rs @@ -161,18 +161,13 @@ 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` stays `Value::Float`; narrowing whole-valued floats breaks CEL +/// arithmetic such as `confidence * 100.0`. fn attr_to_value(attr: &AttributeValue) -> Value { match attr { AttributeValue::Bool(b) => Value::from(*b), 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 +184,8 @@ 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). +/// Convert an author-supplied `cel:` argument to a `cel::Value`. Integers map +/// to `Int`, floats to `Float`, and non-string mapping keys are skipped. fn yaml_to_value(v: &serde_yaml::Value) -> Value { match v { serde_yaml::Value::Null => Value::Null, @@ -225,7 +194,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 +273,53 @@ 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). #[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)); } + #[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)); + bag.set("intent.confidence", 0.92_f64); + assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag)); + } + + #[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)); + assert!(truthy("3 != delegation.depth", &bag)); + } + + #[test] + fn whole_valued_float_in_int_list_matches() { + 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/opa/src/resolver.rs b/builtins/pdps/opa/src/resolver.rs index 1fa27db..17c7e44 100644 --- a/builtins/pdps/opa/src/resolver.rs +++ b/builtins/pdps/opa/src/resolver.rs @@ -310,10 +310,12 @@ impl OpaResolver { .add_policy(INLINE_MODULE_NAME.to_owned(), src.to_owned()) .map_err(|e| EngineError::Compile(e.to_string()))?; - // Reject an inline module that lands in a global module's package — it - // would merge into (and could override) operator policy. Fail-closed: - // inline modules may add new packages, never redefine a global one. - if self.global_packages.contains(&package) { + // Inline modules may add packages but may not share a global package subtree. + if self + .global_packages + .iter() + .any(|g| packages_share_subtree(&package, g)) + { return Err(EngineError::PackageCollision(package)); } @@ -385,6 +387,15 @@ impl OpaResolver { } } +/// Whether two dotted Rego package paths are equal or one contains the other. +/// Path-separator boundaries keep siblings such as `data.authz` and +/// `data.authznext` distinct. +fn packages_share_subtree(a: &str, b: &str) -> bool { + a == b + || a.strip_prefix(b).is_some_and(|rest| rest.starts_with('.')) + || 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 +976,23 @@ msg := "not a decision" } } + #[tokio::test] + async fn inline_module_cannot_override_global_subpackage() { + 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 +1006,39 @@ msg := "not a decision" assert_eq!(out.decision, Decision::Allow); } + #[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); + 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" + ); + } + + #[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/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index a3e9daf..89e4d50 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,21 @@ pub struct JwtIdentityResolver { header: String, } +// Implement `Debug` manually because `cfg` and `pending_jwks` may contain HMAC +// signing secrets or inline PEM keys. +impl std::fmt::Debug for JwtIdentityResolver { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("JwtIdentityResolver") + .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 +691,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 +948,19 @@ 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 scheme is case-insensitive per RFC 9110 §11.1. Bare tokens are returned +/// unchanged for hosts that strip the scheme themselves. +fn strip_bearer_prefix(value: &str) -> &str { + 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 +1151,17 @@ mod tests { ); } + #[test] + fn strip_bearer_prefix_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"); + 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("abc.def.ghi"), "abc.def.ghi"); + 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..7884c2b 100644 --- a/builtins/session/valkey/src/store.rs +++ b/builtins/session/valkey/src/store.rs @@ -128,16 +128,16 @@ 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 + // Timeouts raise the same refresh-failure alarm as backend errors. + let refresh: Result, _> = + tokio::time::timeout(self.command_timeout, conn.expire(&key, ttl_for_expire(ttl))) + .await; + 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 a5b5fbb..0ca3d1b 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -1091,13 +1091,8 @@ fn dispatch_parallel<'a>( }) .await; - // Aggregate in input order: append every branch's taints; pick - // the first Halt (by branch index, not wall-clock order) as the - // overall result. Aborted / 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.) + // Aggregate in branch order, appending taints and keeping the first Halt. + // Cancelled branches contribute nothing; panicked branches fail closed. let mut first_halt: Option = None; for (idx, outcome) in outcomes.into_iter().enumerate() { match outcome { @@ -1124,14 +1119,13 @@ 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); + // Use the panic as this branch's synthetic Halt and audit reason. + if first_halt.is_none() { + first_halt = Some(Decision::Deny { + reason: Some(format!("parallel branch {idx} panicked: {msg}")), + rule_source: "parallel.branch_panic".to_owned(), + }); + } }, } } @@ -1175,6 +1169,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 +1196,52 @@ async fn dispatch_field_op( }); }; - let Some(current) = get_dotted(root, subpath).cloned() else { - return EffectOutcome::Continue; // missing field → silent no-op + // Expand intermediate arrays; excessive fan-out fails closed. + let Some(paths) = crate::route::expand_field_paths(root, subpath) else { + return EffectOutcome::Halt(Decision::Deny { + reason: Some(format!( + "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 + }; + // Plugins receive a root-relative field name; deny messages use the + // prefixed path to identify the args or result side. + let eval = evaluate_pipeline(&pipeline, ¤t, bag, plugins, subpath, phase).await; + taints.extend(eval.taints); + match eval.outcome { + 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 +3124,20 @@ mod tests { } } + /// Invoker that panics on every plugin call. + struct PanicPlugins; + #[async_trait] + impl PluginInvoker for PanicPlugins { + async fn invoke( + &self, + _name: &str, + _bag: &AttributeBag, + _invocation: PluginInvocation<'_>, + ) -> Result { + panic!("plugin blew up mid-branch"); + } + } + fn pdp_step(decision_diagnostic_label: &str) -> Effect { Effect::Pdp { call: PdpCall { @@ -4063,6 +4081,44 @@ mod tests { } } + #[tokio::test] + async fn parallel_panicked_branch_fails_closed() { + 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 5b89074..ef424c6 100644 --- a/crates/ppe-apl-core/src/parser.rs +++ b/crates/ppe-apl-core/src/parser.rs @@ -3222,6 +3222,29 @@ fn compile_apl_blocks(source: &str, raw: RouteYaml) -> Result 1 { + return Err(ParseError::Rule { + rule: format!("{source}.{phase}"), + msg: format!( + "{phase} reaches {elicits} elicitation steps; at most one elicitation per \ + phase is supported (they would share one retry id and resolve against \ + each other)" + ), + }); + } + } + Ok(route) } @@ -4717,6 +4740,63 @@ route: assert!(msg.contains("result.x"), "expected result.x in: {msg}"); } + #[test] + fn two_elicit_steps_in_one_phase_rejected() { + let yaml = r#" +route: + authorization: + pre_invocation: + - "confirm(approver, from: user.sub)" + - "require_approval(approver, from: user.manager)" +"#; + let err = compile_test_policy("payroll", yaml) + .expect_err("two elicits in one phase must be rejected"); + assert!( + format!("{err}").contains("at most one elicitation per phase"), + "expected a multi-elicit rejection, got {err}" + ); + } + + #[test] + fn single_elicit_per_phase_compiles() { + let yaml = r#" +route: + authorization: + pre_invocation: + - "require_approval(approver, from: user.manager)" + post_invocation: + - "confirm(auditor, from: user.sub)" +"#; + compile_test_policy("payroll", yaml).expect("one elicit per phase must compile"); + } + + #[test] + fn duplicate_field_pipeline_key_is_rejected_not_last_wins() { + // Pin duplicate-key rejection: last-wins parsing could drop a redaction. + let yaml = r#" +route: + result: + ssn: "redact" + ssn: "hash" +"#; + let err = compile_test_policy("r", yaml).expect_err("a repeated field key is not legal"); + assert!( + format!("{err}").contains("duplicate entry with key"), + "expected a duplicate-key error, got {err}" + ); + } + + #[test] + fn distinct_field_pipeline_keys_still_load() { + let yaml = r#" +route: + result: + ssn: "redact" + email: "hash" +"#; + compile_test_policy("r", yaml).expect("distinct field keys are legal"); + } + #[test] fn removed_policy_field_names_are_rejected() { // The removed authorization-phase keys must fail loudly, never be diff --git a/crates/ppe-apl-core/src/route.rs b/crates/ppe-apl-core/src/route.rs index 8413801..b7d0b37 100644 --- a/crates/ppe-apl-core/src/route.rs +++ b/crates/ppe-apl-core/src/route.rs @@ -111,46 +111,65 @@ 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 + // Expand intermediate arrays; excessive fan-out fails closed. + 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 +219,65 @@ 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; + // Expand intermediate arrays; excessive fan-out fails closed. + let Some(paths) = expand_field_paths(result, &rule.field) else { + return RouteDecision { + decision: Decision::Deny { + reason: Some(format!( + "result field `{}` expands to too many elements to redact safely", + rule.field + )), + rule_source: rule.source.clone(), + }, + taints, + constraints: Vec::new(), + args_modified: false, + result_modified, + pending: None, + }; }; - let eval = evaluate_pipeline( - &rule.pipeline, - ¤t, - bag, - plugins, - &rule.field, - DispatchPhase::Post, - ) - .await; - taints.extend(eval.taints); - match eval.outcome { - FieldOutcome::Pass => {}, - FieldOutcome::Replace(new_val) => { - if set_dotted(result, &rule.field, new_val) { - result_modified = true; - } - }, - FieldOutcome::Omit => { - if remove_dotted(result, &rule.field) { - result_modified = true; - } - }, - FieldOutcome::Deny { reason, .. } => { - return RouteDecision { - decision: Decision::Deny { - reason: Some(reason), - rule_source: rule.source.clone(), - }, - taints, - // `restrict` fires in post_invocation (below); a - // result-pipeline deny short-circuits before it. - constraints: Vec::new(), - args_modified: false, - result_modified, - pending: None, - }; - }, + for path in paths { + let Some(current) = get_dotted(result, &path).cloned() else { + continue; + }; + let eval = evaluate_pipeline( + &rule.pipeline, + ¤t, + bag, + plugins, + &rule.field, + DispatchPhase::Post, + ) + .await; + taints.extend(eval.taints); + match eval.outcome { + FieldOutcome::Pass => {}, + FieldOutcome::Replace(new_val) => { + if set_dotted(result, &path, new_val) { + result_modified = true; + } + }, + FieldOutcome::Omit => { + if remove_dotted(result, &path) { + result_modified = true; + } + }, + FieldOutcome::Deny { reason, .. } => { + return RouteDecision { + decision: Decision::Deny { + reason: Some(reason), + rule_source: rule.source.clone(), + }, + taints, + // `restrict` fires in post_invocation (below); a + // result-pipeline deny short-circuits before it. + constraints: Vec::new(), + args_modified: false, + result_modified, + pending: None, + }; + }, + } } } } @@ -312,8 +350,106 @@ pub async fn evaluate_route( } } +/// Maximum traversal depth, counting path segments and implicit array fan-out. +const MAX_FANOUT_DEPTH: usize = 128; + +/// Maximum number of concrete leaf paths one field rule may fan out into +/// before failing closed. +const MAX_EXPANDED_PATHS: usize = 100_000; + +/// Resolve one path segment against a value. Object segments index by key; a +/// numeric segment indexes into an array, so a path produced by +/// [`expand_field_paths`] resolves. +fn segment_get<'a>(value: &'a serde_json::Value, seg: &str) -> Option<&'a serde_json::Value> { + match value { + serde_json::Value::Object(_) => value.get(seg), + serde_json::Value::Array(items) => seg.parse::().ok().and_then(|i| items.get(i)), + _ => None, + } +} + +/// Expand a dotted field path into the concrete paths it names once arrays +/// along the way are accounted for. +/// +/// Intermediate arrays fan out into indexed paths; missing leaves are skipped. +/// A terminal array remains one path so whole-value operations still apply. +/// Returns `None` when depth or leaf-count bounds are exceeded, allowing callers +/// to fail closed rather than apply a partial transformation. +pub(crate) fn expand_field_paths(root: &serde_json::Value, path: &str) -> Option> { + fn join(prefix: &str, seg: &str) -> String { + if prefix.is_empty() { + seg.to_owned() + } else { + format!("{prefix}.{seg}") + } + } + + 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; + } + 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::() { + if let Some(child) = items.get(idx) { + return walk(child, rest, &join(prefix, seg), depth + 1, out); + } + } else { + // Apply an unnamed intermediate array to every element. + for (i, item) in items.iter().enumerate() { + if !walk(item, segs, &join(prefix, &i.to_string()), depth + 1, out) { + return false; + } + } + } + return true; + } + let Some(child) = segment_get(value, seg) else { + return true; // missing segment on this branch, so emit no path + }; + walk(child, rest, &join(prefix, seg), depth + 1, out) + } + + let segs: Vec<&str> = path.split('.').collect(); + let mut out = Vec::new(); + walk(root, &segs, "", 0, &mut out).then_some(out) +} + +/// Descend to the parent value of a dotted path, following object keys and +/// numeric array indices. Returns `None` if any parent segment is missing or +/// crosses a scalar. Shared by `set_dotted` / `remove_dotted` so both write +/// through arrays the same way `get_dotted` reads through them. +fn parent_mut<'a>( + root: &'a mut serde_json::Value, + parents: &[&str], +) -> Option<&'a mut serde_json::Value> { + let mut cur = root; + for seg in parents { + cur = match cur { + serde_json::Value::Object(map) => map.get_mut(*seg)?, + serde_json::Value::Array(items) => seg + .parse::() + .ok() + .and_then(move |i| items.get_mut(i))?, + _ => return None, + }; + } + Some(cur) +} + /// Read `root.a.b.c` from a JSON value via dot-separated path. Returns -/// `None` if any segment is missing or the path crosses a non-object. +/// `None` if any segment is missing. Object segments index by key; a numeric +/// segment indexes into an array, so a path expanded by +/// `expand_field_paths` resolves. /// /// Public because host bridges read fields back out of their own payload /// projections — a plugin dispatched from a pipeline stage reports a new @@ -322,14 +458,14 @@ pub async fn evaluate_route( pub fn get_dotted<'a>(root: &'a serde_json::Value, path: &str) -> Option<&'a serde_json::Value> { let mut cur = root; for seg in path.split('.') { - cur = cur.get(seg)?; + cur = segment_get(cur, seg)?; } Some(cur) } /// Write to `root.a.b.c` via dot-separated path. Returns true on success; -/// false if the parent path doesn't exist or doesn't resolve to an object. -/// Does not create missing parent objects — that'd hide schema bugs. +/// false if the parent path doesn't exist or the leaf's parent is a scalar. +/// Does not create missing parents. A numeric leaf overwrites an array element. pub(crate) fn set_dotted( root: &mut serde_json::Value, path: &str, @@ -340,45 +476,49 @@ pub(crate) fn set_dotted( Some(x) => x, None => return false, }; - let mut cur = root; - for seg in parents { - let Some(next) = cur.get_mut(*seg) else { - return false; - }; - if !next.is_object() { - return false; - } - cur = next; - } - if let serde_json::Value::Object(map) = cur { - map.insert((*leaf).to_owned(), value); - true - } else { - false + let Some(cur) = parent_mut(root, parents) else { + return false; + }; + match cur { + serde_json::Value::Object(map) => { + map.insert((*leaf).to_owned(), value); + true + }, + serde_json::Value::Array(items) => match leaf.parse::().ok() { + Some(i) => match items.get_mut(i) { + Some(slot) => { + *slot = value; + true + }, + None => false, + }, + None => false, + }, + _ => false, } } -/// Remove `root.a.b.c` from a JSON value. Returns true if removal happened. +/// Remove `root.a.b.c` from a JSON value. Returns true if removal happened. A +/// numeric leaf segment removes that array element. pub(crate) fn remove_dotted(root: &mut serde_json::Value, path: &str) -> bool { let parts: Vec<&str> = path.split('.').collect(); let (leaf, parents) = match parts.split_last() { Some(x) => x, None => return false, }; - let mut cur = root; - for seg in parents { - let Some(next) = cur.get_mut(*seg) else { - return false; - }; - if !next.is_object() { - return false; - } - cur = next; - } - if let serde_json::Value::Object(map) = cur { - map.remove(*leaf).is_some() - } else { - false + let Some(cur) = parent_mut(root, parents) else { + return false; + }; + match cur { + serde_json::Value::Object(map) => map.remove(*leaf).is_some(), + serde_json::Value::Array(items) => match leaf.parse::().ok() { + Some(i) if i < items.len() => { + items.remove(i); + true + }, + _ => false, + }, + _ => false, } } @@ -859,6 +999,157 @@ mod tests { assert!(!r.args_modified); } + #[test] + fn expand_field_paths_fans_out_over_intermediate_arrays() { + let v = json!({ + "rows": [ { "ssn": "a" }, { "ssn": "b" }, { "other": 1 } ] + }); + 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()] + ); + + assert_eq!( + expand_field_paths(&v, "rows"), + Some(vec!["rows".to_owned()]) + ); + + let flat = json!({ "a": { "b": 1 } }); + assert_eq!( + expand_field_paths(&flat, "a.b"), + Some(vec!["a.b".to_owned()]) + ); + + 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() { + 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() { + let mut rows = Vec::with_capacity(super::MAX_EXPANDED_PATHS + 2); + for i in 0..(super::MAX_EXPANDED_PATHS + 2) { + rows.push(json!({ "ssn": i })); + } + let root = json!({ "rows": serde_json::Value::Array(rows) }); + assert_eq!( + expand_field_paths(&root, "rows.ssn"), + None, + "a wide array past the leaf cap must fail closed" + ); + } + + #[test] + fn dotted_helpers_index_into_arrays() { + let mut v = json!({ "rows": [ { "ssn": "a" }, { "ssn": "b" } ] }); + assert_eq!(get_dotted(&v, "rows.1.ssn"), Some(&json!("b"))); + assert!(set_dotted(&mut v, "rows.0.ssn", json!("[REDACTED]"))); + assert_eq!(v["rows"][0]["ssn"], json!("[REDACTED]")); + 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() { + 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]")); + assert_eq!(result["rows"][0]["name"], json!("a")); + } + + #[tokio::test] + async fn route_result_over_large_fanout_denies() { + let mut route = CompiledRoute::new("ping"); + route.result.push(field_rule( + "rows.leaf", + vec![Stage::Redact { condition: None }], + )); + + let mut nested = json!({ "leaf": "secret" }); + for _ in 0..(super::MAX_FANOUT_DEPTH + 5) { + nested = serde_json::Value::Array(vec![nested]); + } + let mut bag = AttributeBag::new(); + let mut payload = RoutePayload::with_result(json!({}), json!({ "rows": nested })); + + let r = evaluate_route( + &route, + &mut bag, + &mut payload, + &pdp_arc(), + &plugins(), + &delegations(), + &elicitations(), + ) + .await; + match r.decision { + Decision::Deny { + reason, + rule_source, + } => { + let reason = reason.unwrap_or_default(); + assert!(reason.contains("result field"), "reason: {reason}"); + assert!( + reason.contains("too many elements to redact safely"), + "reason: {reason}" + ); + assert_eq!(rule_source, "test.rows.leaf"); + }, + other => panic!("over-large result fan-out must deny, got {other:?}"), + } + assert!( + !r.result_modified, + "nothing may be redacted on a fail-closed deny" + ); + } + #[tokio::test] async fn post_invocation_runs_after_result() { let mut route = CompiledRoute::new("ping"); diff --git a/crates/ppe-apl-core/src/rules.rs b/crates/ppe-apl-core/src/rules.rs index 6cccdf6..33e91eb 100644 --- a/crates/ppe-apl-core/src/rules.rs +++ b/crates/ppe-apl-core/src/rules.rs @@ -334,6 +334,34 @@ impl Effect { } } + /// Count the `Elicit` nodes reachable in this effect subtree. + /// + /// Validators reject multiple elicitations per phase because the protocol + /// has one shared elicitation id. Conditional and PDP arms are summed + /// conservatively because different arms may run across retries. + pub fn count_elicits(&self) -> usize { + match self { + Effect::Elicit(_) => 1, + Effect::Sequential(effects) | Effect::Parallel(effects) => { + effects.iter().map(Effect::count_elicits).sum() + }, + Effect::When { body, .. } => body.iter().map(Effect::count_elicits).sum(), + Effect::Pdp { + on_allow, on_deny, .. + } => { + on_allow.iter().map(Effect::count_elicits).sum::() + + on_deny.iter().map(Effect::count_elicits).sum::() + }, + Effect::Allow + | Effect::Deny { .. } + | Effect::Plugin { .. } + | Effect::Taint { .. } + | Effect::Restrict { .. } + | Effect::FieldOp { .. } + | Effect::Delegate(_) => 0, + } + } + /// Walk the effect tree rejecting any `FieldOp` / `Delegate` that /// lives directly or transitively under a `Parallel` node. Returns /// the path string of the first violation found (or `Ok(())` if @@ -976,6 +1004,51 @@ mod tests { ); } + #[test] + fn count_elicits_sums_through_control_flow() { + let elicit = |name: &str| { + Effect::Elicit(crate::step::ElicitStep { + kind: crate::step::ElicitKind::Approval, + plugin_name: name.into(), + channel: None, + from: "user.manager".into(), + purpose: None, + scope: None, + timeout: None, + config_override: None, + on_error: None, + source: "test".into(), + }) + }; + assert_eq!(Effect::Allow.count_elicits(), 0); + assert_eq!(elicit("a").count_elicits(), 1); + assert_eq!( + Effect::When { + condition: Expression::Always, + body: vec![elicit("a"), elicit("b")], + source: "test".into(), + } + .count_elicits(), + 2 + ); + assert_eq!( + Effect::Sequential(vec![elicit("a"), Effect::Allow, elicit("b")]).count_elicits(), + 2 + ); + assert_eq!( + Effect::Pdp { + call: crate::step::PdpCall { + dialect: crate::step::PdpDialect::Cedar, + args: serde_yaml::Value::Null, + }, + on_allow: vec![elicit("a")], + on_deny: vec![elicit("b")], + } + .count_elicits(), + 2 + ); + } + #[test] fn validate_parallel_pure_block_passes() { // A parallel block of read-only effects validates clean. diff --git a/crates/ppe-apl-runtime/src/delegation_invoker.rs b/crates/ppe-apl-runtime/src/delegation_invoker.rs index 2db2487..fa331ac 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,19 @@ 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 never reaches + // the plugin. // // There is deliberately no `mode` key: the delegation mode is // *derived* from `subject` by the handler rather than declared, so a // route can't claim on-behalf-of-user while handing over a workload // SVID. + // + // Malformed attenuation fails closed rather than widening the credential. let cfg = step.config_override.as_ref().and_then(|v| v.as_mapping()); // Resolve who the exchange is *for*. Defaults to the user @@ -214,6 +216,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 +383,20 @@ fn target_type_from_str(s: &str) -> TargetType { } } +/// Parse the optional `attenuation:` block into a typed `AttenuationConfig`. +/// +/// A malformed block returns `InvalidConfig`; an absent block returns `None`. +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 +444,42 @@ 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() { + 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() { + 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 477fb40..b8eddcb 100644 --- a/crates/ppe-apl-runtime/src/route_handler.rs +++ b/crates/ppe-apl-runtime/src/route_handler.rs @@ -844,7 +844,18 @@ 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; `candidate_constraint` is handled above. + let http_changed = match (before.http.as_ref(), after.http.as_ref()) { + (Some(a), Some(b)) => !Arc::ptr_eq(a, b), + (None, None) => false, + _ => true, + }; + let custom_changed = match (before.custom.as_ref(), after.custom.as_ref()) { + (Some(a), Some(b)) => !Arc::ptr_eq(a, b), + (None, None) => false, + _ => true, + }; + security_changed || delegation_changed || raw_creds_changed || http_changed || custom_changed } /// Extract the elicitation id an agent echoes on retry from the @@ -1157,6 +1168,30 @@ mod tests { assert!(extensions_changed(&before, &after)); } + #[test] + fn an_http_change_alone_is_detected() { + use praxis_policy_core::extensions::HttpExtension; + let before = Extensions::default(); + let after = Extensions { + http: Some(Arc::new(HttpExtension::default())), + ..Extensions::default() + }; + assert!( + extensions_changed(&before, &after), + "a header write must not be mistaken for no change" + ); + } + + #[test] + fn a_custom_change_alone_is_detected() { + let before = Extensions::default(); + let after = Extensions { + custom: Some(Arc::new(std::collections::HashMap::new())), + ..Extensions::default() + }; + assert!(extensions_changed(&before, &after)); + } + /// The arm that actually runs in production, for both slots that a route can /// mutate without touching security. /// @@ -1210,5 +1245,42 @@ mod tests { ), "a replaced delegation chain must be detected" ); + + // Cover replacement of an existing mutable slot, not only `None -> Some`. + use praxis_policy_core::extensions::HttpExtension; + let http = Arc::new(HttpExtension::default()); + let with_http = |c: &Arc| Extensions { + 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 65c9193..4c545b3 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -1084,6 +1084,26 @@ impl ConfigVisitor for AplConfigVisitor { return Err(err_msg.into()); } + // Repeat elicitation validation after stacking to catch duplicates + // introduced across global, group, and route layers. + for (phase, effects) in [ + ("pre_invocation", &effective.pre_invocation), + ("post_invocation", &effective.post_invocation), + ] { + let elicits: usize = effects + .iter() + .map(praxis_policy_apl_core::rules::Effect::count_elicits) + .sum(); + if elicits > 1 { + let err_msg = format!( + "route '{route_key}': {phase} reaches {elicits} elicitation steps; at \ + most one elicitation per phase is supported (they would share one retry \ + id and resolve against each other)" + ); + return Err(err_msg.into()); + } + } + // Each half installs only when the effective route declares steps // for it, the way the global catch-all already decides. let installs_pre = declares_pre_phase(&effective); diff --git a/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs b/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs index 8baea3a..b6b5ff3 100644 --- a/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs +++ b/crates/ppe-apl-runtime/tests/delegation_identity_warning.rs @@ -26,12 +26,35 @@ use std::sync::{Arc, Mutex}; use praxis_policy_core::engine::PolicyEngine; +use praxis_policy_core::error::PluginError; +use praxis_policy_core::factory::{PluginFactory, PluginInstance}; +use praxis_policy_core::plugin::{Plugin, PluginConfig}; use tracing::field::{Field, Visit}; use tracing::span::{Attributes, Id, Record}; use tracing::{Event, Metadata, Subscriber}; const ALARM: &str = "delegation_without_identity_resolution"; +/// Declares the identity plugin used by the configuration fixture. +struct Inert(PluginConfig); + +impl Plugin for Inert { + fn config(&self) -> &PluginConfig { + &self.0 + } +} + +struct InertFactory; + +impl PluginFactory for InertFactory { + fn create(&self, config: &PluginConfig) -> Result> { + Ok(PluginInstance { + plugin: Arc::new(Inert(config.clone())), + handlers: Vec::new(), + }) + } +} + /// A route delegating the caller's credential, with nothing configured /// to validate it. const NO_IDENTITY: &str = r#" @@ -50,7 +73,10 @@ routes: const WITH_IDENTITY: &str = r#" engine_settings: dispatch: policy -plugins: [] +plugins: + - name: corp-jwt + kind: builtin + hooks: [identity.resolve] routes: - tool: get_compensation authentication: @@ -136,6 +162,7 @@ fn alarms_raised_by_loading(yaml: &str) -> Vec { }; let mgr = Arc::new(PolicyEngine::default()); + mgr.register_factory("builtin", Box::new(InertFactory)); praxis_policy_apl_runtime::register_apl( &mgr, praxis_policy_apl_runtime::AplOptions::in_process(), diff --git a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs index 7180d8d..9be331f 100644 --- a/crates/ppe-apl-runtime/tests/visitor_config_errors.rs +++ b/crates/ppe-apl-runtime/tests/visitor_config_errors.rs @@ -223,3 +223,41 @@ fn an_attribute_file_that_does_not_exist_is_rejected() { fn an_empty_attribute_files_list_loads() { loads("global:\n attribute_files: []\n"); } + +/// Cross-layer elicitations in one phase are rejected after route stacking. +#[test] +fn a_global_and_route_elicit_in_one_phase_is_rejected() { + let e = load_err( + r#"global: + authorization: + pre_invocation: + - "require_approval(manager-approver, from: user.manager)" +routes: + - tool: get_compensation + authorization: + pre_invocation: + - "confirm(user-confirm, from: user.sub)" +"#, + ); + assert!( + e.contains("at most one elicitation per phase"), + "a global plus route elicit stacked into one phase must be rejected: {e}" + ); +} + +/// Elicitations in separate phases remain valid. +#[test] +fn a_global_pre_elicit_and_route_post_elicit_load() { + loads( + r#"global: + authorization: + pre_invocation: + - "require_approval(manager-approver, from: user.manager)" +routes: + - tool: get_compensation + authorization: + post_invocation: + - "confirm(user-confirm, from: user.sub)" +"#, + ); +} diff --git a/crates/ppe-core/src/config.rs b/crates/ppe-core/src/config.rs index 12f4af9..568dcac 100644 --- a/crates/ppe-core/src/config.rs +++ b/crates/ppe-core/src/config.rs @@ -2261,6 +2261,29 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box = config.plugins.iter().map(|p| p.name.as_str()).collect(); + // Authentication steps must name a declared plugin. + let validate_authentication = + |authentication: &Option, + context: &str| + -> Result<(), Box> { + let Some(authentication) = authentication else { + return Ok(()); + }; + for step in &authentication.steps { + if !plugin_names.contains(step.name.as_str()) { + return Err(Box::new(PluginError::Config { + message: format!( + "{context} authentication references unknown plugin '{}'", + step.name + ), + })); + } + } + Ok(()) + }; + + validate_authentication(&config.global.authentication, "global")?; + // A `global.defaults` key that names no entity type never applies to // anything, so a typo there would be silently inert rather than wrong. for entity_type in config.global.defaults.keys() { @@ -2379,6 +2402,8 @@ pub(crate) fn validate_config(config: &PolicyConfig) -> Result<(), Box 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 result always halts; synthesize a violation + // when the plugin did not provide one. + let mut v = erased.violation.unwrap_or_else(|| { + crate::error::PluginViolation::new( + "plugin_deny", + 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 2e26652..5d9d213 100644 --- a/crates/ppe-core/src/extensions/container.rs +++ b/crates/ppe-core/src/extensions/container.rs @@ -364,10 +364,12 @@ 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. Folding returned labels into the canonical set makes + /// removal structurally impossible; it requires a `DeclassifierToken` no + /// plugin can construct. + /// + /// Capability-aware laundering detection happens earlier in `labels_ok`; + /// this layer cannot distinguish a filtered view from a removal attempt. /// /// Every other field on the slot is Immutable in the tier model with /// `write_cap: None` — `subject`, `auth_method`, `client`, `caller_workload`, @@ -386,14 +388,7 @@ impl Extensions { None => SecurityExtension::default(), }; - // Monotonic: fold in the additions, never assign the returned set. - // Assignment would drop any canonical label the plugin could not see, - // and folding makes a filtered-away label unremovable by construction. - 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; - } + // Fold additions into canonical labels; never replace the set. for label in owned.labels.iter() { merged.labels.add_label(label.clone()); } @@ -450,9 +445,10 @@ 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. +/// Existing hops must match on every authority-bearing or bounding field: +/// subject, audience, granted scopes, strategy, `authorization_details`, +/// `ttl_seconds`, and `timestamp`. Dropping any of them reopens a widening +/// path. `from_cache` is merge bookkeeping and is not compared. /// /// Public so out-of-process hosts can apply the same validation to a chain /// arriving over the wire before it reaches the merge, instead of reimplementing @@ -473,6 +469,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 +1113,8 @@ mod tests { } #[test] - fn test_label_removal_is_dropped_whole() { + fn test_label_removal_is_refused_by_folding() { + // Folding, not a superset gate, is what refuses the removal. let mut security = SecurityExtension::default(); security.add_label("PII"); security.add_label("HIPAA"); @@ -1135,10 +1135,42 @@ 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 superset gate here discarded the labels of a plugin + // holding `append_labels` but not `read_labels`, which sees an empty + // filtered set and so never returns a superset of canonical. + let mut security = SecurityExtension::default(); + security.add_label("EXISTING"); + + let mut ext = Extensions { + security: Some(Arc::new(security)), + ..Default::default() + }; + ext.labels_write_token = Some(WriteToken::new()); + + let mut cow = ext.cow_copy(); + 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" ); } @@ -1290,6 +1322,57 @@ 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()); + + 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..9a10f87 100644 --- a/crates/ppe-core/src/extensions/raw_credentials.rs +++ b/crates/ppe-core/src/extensions/raw_credentials.rs @@ -434,10 +434,65 @@ 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 `[key, value]` pairs because JSON object keys must be + /// strings. Token bytes remain excluded by `#[serde(skip)]`. + #[serde(default, with = "delegated_tokens_as_pairs")] pub delegated_tokens: HashMap, } +/// Serialize `delegated_tokens` as pairs so its structured keys work in JSON. +/// +/// Deserialization also accepts the legacy map form; older code could emit an +/// empty map even though non-empty structured-key maps failed to serialize. +mod delegated_tokens_as_pairs { + use super::{DelegationKey, RawDelegatedToken}; + use serde::de::{MapAccess, SeqAccess, Visitor}; + 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 { + // Accept legacy JSON objects, normally `{}`. + let mut map = HashMap::with_capacity(access.size_hint().unwrap_or(0)); + while let Some((k, v)) = access.next_entry::()? { + map.insert(k, v); + } + 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 +665,50 @@ 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() { + 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(), + ), + ); + + 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() { + let legacy = r#"{"inbound_tokens":{},"delegated_tokens":{}}"#; + let restored: RawCredentialsExtension = serde_json::from_str(legacy).unwrap(); + assert!(restored.delegated_tokens.is_empty()); + + let modern = r#"{"inbound_tokens":{},"delegated_tokens":[]}"#; + let restored: RawCredentialsExtension = serde_json::from_str(modern).unwrap(); + assert!(restored.delegated_tokens.is_empty()); + } } diff --git a/crates/ppe-core/tests/config_key_sets.rs b/crates/ppe-core/tests/config_key_sets.rs index d649062..86bec0b 100644 --- a/crates/ppe-core/tests/config_key_sets.rs +++ b/crates/ppe-core/tests/config_key_sets.rs @@ -641,13 +641,15 @@ fn a_misspelled_authentication_object_key_is_rejected() { /// Both accepted shapes still load, and the flag still reads through. #[test] fn the_authentication_object_shapes_still_load() { - let additive = praxis_policy_core::config::parse_config( - "routes:\n - tool: get_weather\n authentication: [jwt]\n", - ) + const PLUGINS: &str = + "plugins:\n - name: jwt\n kind: builtin\n hooks: [identity.resolve]\n"; + let additive = praxis_policy_core::config::parse_config(&format!( + "{PLUGINS}routes:\n - tool: get_weather\n authentication: [jwt]\n" + )) .expect("the list form is additive"); - let replacing = praxis_policy_core::config::parse_config( - "routes:\n - tool: get_weather\n authentication:\n replace_inherited: true\n steps: [jwt]\n", - ) + let replacing = praxis_policy_core::config::parse_config(&format!( + "{PLUGINS}routes:\n - tool: get_weather\n authentication:\n replace_inherited: true\n steps: [jwt]\n" + )) .expect("the object form loads"); for (label, cfg, expected) in [("list", additive, false), ("object", replacing, true)] { let identity = cfg.routes[0]