From 74363e6655d5be2b26a012e4d997c0babc712479 Mon Sep 17 00:00:00 2001 From: aviavissar Date: Tue, 1 Sep 2026 10:13:28 +0300 Subject: [PATCH 1/2] test(cel): pin stdlib-vs-custom and namespace-vs-scalar collision precedence Signed-off-by: aviavissar --- builtins/pdps/cel/src/activation.rs | 80 ++++++++++++++++++++++++++-- builtins/pdps/cel/src/resolver.rs | 81 +++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 5 deletions(-) diff --git a/builtins/pdps/cel/src/activation.rs b/builtins/pdps/cel/src/activation.rs index 8551550b..108c557d 100644 --- a/builtins/pdps/cel/src/activation.rs +++ b/builtins/pdps/cel/src/activation.rs @@ -16,11 +16,16 @@ // String → Value::String // StringSet → Value::List(of String) (so `"x" in session.labels` works) // -// Collision rule: if a key is both a leaf and a namespace prefix -// (`delegation` AND `delegation.depth`), the namespace (map) wins and the -// scalar leaf is dropped with a `tracing::warn!`. In practice the cmf -// BagBuilder never emits both, but the bag is an open namespace so we -// resolve it deterministically rather than panic. +// Collision rule: the same name cannot be both a single value and a +// map. If the bag has `delegation` and `delegation.depth`, we keep +// the map, drop the single value, and log a warning. +// +// That case is real. CMF copies each role name into `role. = +// true` (and the same for permissions and teams) without stripping +// dots. A subject with roles `admin` and `admin.readonly` therefore +// gets both `role.admin` and `role.admin.readonly`, and +// `role.admin == true` evaluates false. We pick a winner rather than +// panic, because anyone can put any key in the bag. use std::collections::{BTreeMap, HashMap}; @@ -232,6 +237,14 @@ mod tests { matches!(run_cel(expr, &ctx), Ok(Value::Bool(true))) } + /// Same split `build_tree` uses, so the test cannot drift from + /// production by hand-writing a segments slice that disagrees with + /// the dotted key. + fn insert_key(root: &mut BTreeMap, key: &str, leaf: Value) { + let segments: Vec<&str> = key.split('.').collect(); + insert(root, key, &segments, leaf); + } + #[test] fn dotted_keys_become_nested_maps() { let mut bag = AttributeBag::new(); @@ -381,4 +394,61 @@ mod tests { bag.set("delegation.depth", 3_i64); assert!(truthy("delegation.depth == 3", &bag)); } + + /// The sibling of `namespace_wins_on_leaf_collision`: a scalar + /// arrives *after* the namespace already exists. `insert` keeps the + /// branch and drops the scalar (the warning at the terminal-segment + /// arm). Driven through `insert` directly because `AttributeBag` is + /// a `HashMap` — `set` order is not `iter` order, so a bag cannot + /// pin this arm. + #[test] + fn namespace_wins_when_scalar_arrives_after_branch() { + let mut root = BTreeMap::new(); + insert_key(&mut root, "delegation.depth", Value::from(3_i64)); + insert_key( + &mut root, + "delegation", + Value::from("scalar-value".to_owned()), + ); + + assert!( + matches!( + root.get("delegation"), + Some(Node::Branch(children)) + if matches!( + children.get("depth"), + Some(Node::Leaf(Value::Int(3))) + ) + ), + "namespace must win when a scalar arrives after the branch exists; \ + depth must stay Int(3), not the colliding string", + ); + } + + /// Mirror of `namespace_wins_when_scalar_arrives_after_branch`: + /// `delegation` first, then `delegation.depth`. `insert` turns the + /// single value into a map and keeps `depth`. Direct `insert` + /// so a `HashMap` cannot skip this path. + #[test] + fn namespace_wins_when_branch_arrives_after_scalar() { + let mut root = BTreeMap::new(); + insert_key( + &mut root, + "delegation", + Value::from("scalar-value".to_owned()), + ); + insert_key(&mut root, "delegation.depth", Value::from(3_i64)); + + assert!( + matches!( + root.get("delegation"), + Some(Node::Branch(children)) + if matches!( + children.get("depth"), + Some(Node::Leaf(Value::Int(3))) + ) + ), + "a later dotted key must promote the scalar to a map and keep depth as Int(3)", + ); + } } diff --git a/builtins/pdps/cel/src/resolver.rs b/builtins/pdps/cel/src/resolver.rs index 9dc034c2..03dc5630 100644 --- a/builtins/pdps/cel/src/resolver.rs +++ b/builtins/pdps/cel/src/resolver.rs @@ -145,6 +145,17 @@ impl CelResolver { /// /// Composes: calling `with_functions` more than once stacks the /// callbacks. Each runs in registration order on every context. + /// If two custom setups register the same name, the later one wins. + /// + /// Prefer names that are not already in the CEL standard library + /// (`size`, `matches`, `double`, …). `with_functions` does not + /// reject a colliding name. Which body runs is decided by CEL, not + /// by this crate: today CEL tries a matching built-in first + /// (`size("hello")` stays the standard `size`) and calls the + /// custom function only when the built-in has no matching form + /// (`size(42)` can run a custom `size` that takes an int). That + /// dispatch is not part of this resolver's contract. `has(...)` is + /// rewritten by the parser, so a custom `has` never runs. /// /// # Example /// @@ -671,6 +682,76 @@ mod tests { ); } + /// A custom `size(int) -> 777` must not replace stdlib + /// `size("hello")` (still 5), but must run for `size(42)` because + /// the stdlib has no `size` that takes an int. + #[tokio::test] + async fn custom_size_stdlib_overload_then_int_fallback() { + let r = CelResolver::new().with_functions(|ctx| { + ctx.add_function("size", |_n: i64| -> i64 { 777 }); + }); + let bag = bag_with(&[("subject.id", "alice")]); + + let stdlib = r + .evaluate(&cel_call("size('hello') == 5"), &bag) + .await + .unwrap(); + assert_eq!( + stdlib.decision, + Decision::Allow, + "stdlib size('hello') is 5; a matching built-in must win", + ); + + let fallback = r + .evaluate(&cel_call("size(42) == 777"), &bag) + .await + .unwrap(); + assert_eq!( + fallback.decision, + Decision::Allow, + "no stdlib size(int); the custom function must run as fallback", + ); + } + + /// Two custom setups that register the same name and signature: + /// the later registration's body must win. Pins the doc-comment + /// claim on `with_functions` ("If two custom setups register the + /// same name, the later one wins") so a silent semantics change + /// in the CEL crate breaks loudly here. + #[tokio::test] + async fn later_custom_function_overwrites_earlier_one() { + let r = CelResolver::new() + .with_functions(|ctx| { + ctx.add_function("magic", |n: i64| -> i64 { n + 100 }); + }) + .with_functions(|ctx| { + // Same name, same signature, different body. + ctx.add_function("magic", |n: i64| -> i64 { n + 999 }); + }); + let bag = bag_with(&[("subject.id", "alice")]); + + // The later registration (n + 999) must be the one that runs. + let out = r + .evaluate(&cel_call("magic(1) == 1000"), &bag) + .await + .unwrap(); + assert_eq!( + out.decision, + Decision::Allow, + "the later custom function body must win; expected magic(1) == 1000", + ); + + // Confirm the earlier body (n + 100) is NOT the one running. + let out = r + .evaluate(&cel_call("magic(1) == 101"), &bag) + .await + .unwrap(); + assert!( + matches!(out.decision, Decision::Deny { .. }), + "the earlier custom function body (n + 100) must be shadowed", + ); + } + /// The `regex` cel-feature is explicitly enabled in our Cargo.toml. /// Pin that `matches(s, pattern)` actually works through the /// resolver so a future feature-set churn breaks loudly here. From 4f231f625eda6b386e5df5a38e0890b99499afb0 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Tue, 22 Sep 2026 12:24:21 -0400 Subject: [PATCH 2/2] test(cel): trim collision test commentary Signed-off-by: Frederico Araujo --- builtins/pdps/cel/src/activation.rs | 30 +++--------------- builtins/pdps/cel/src/resolver.rs | 47 ++++------------------------- 2 files changed, 10 insertions(+), 67 deletions(-) diff --git a/builtins/pdps/cel/src/activation.rs b/builtins/pdps/cel/src/activation.rs index 108c557d..58d95e4b 100644 --- a/builtins/pdps/cel/src/activation.rs +++ b/builtins/pdps/cel/src/activation.rs @@ -16,16 +16,8 @@ // String → Value::String // StringSet → Value::List(of String) (so `"x" in session.labels` works) // -// Collision rule: the same name cannot be both a single value and a -// map. If the bag has `delegation` and `delegation.depth`, we keep -// the map, drop the single value, and log a warning. -// -// That case is real. CMF copies each role name into `role. = -// true` (and the same for permissions and teams) without stripping -// dots. A subject with roles `admin` and `admin.readonly` therefore -// gets both `role.admin` and `role.admin.readonly`, and -// `role.admin == true` evaluates false. We pick a winner rather than -// panic, because anyone can put any key in the bag. +// If a key is both a leaf and a namespace prefix, the namespace wins +// and the scalar is dropped with a warning. use std::collections::{BTreeMap, HashMap}; @@ -237,9 +229,6 @@ mod tests { matches!(run_cel(expr, &ctx), Ok(Value::Bool(true))) } - /// Same split `build_tree` uses, so the test cannot drift from - /// production by hand-writing a segments slice that disagrees with - /// the dotted key. fn insert_key(root: &mut BTreeMap, key: &str, leaf: Value) { let segments: Vec<&str> = key.split('.').collect(); insert(root, key, &segments, leaf); @@ -395,12 +384,6 @@ mod tests { assert!(truthy("delegation.depth == 3", &bag)); } - /// The sibling of `namespace_wins_on_leaf_collision`: a scalar - /// arrives *after* the namespace already exists. `insert` keeps the - /// branch and drops the scalar (the warning at the terminal-segment - /// arm). Driven through `insert` directly because `AttributeBag` is - /// a `HashMap` — `set` order is not `iter` order, so a bag cannot - /// pin this arm. #[test] fn namespace_wins_when_scalar_arrives_after_branch() { let mut root = BTreeMap::new(); @@ -420,15 +403,10 @@ mod tests { Some(Node::Leaf(Value::Int(3))) ) ), - "namespace must win when a scalar arrives after the branch exists; \ - depth must stay Int(3), not the colliding string", + "namespace must retain its child", ); } - /// Mirror of `namespace_wins_when_scalar_arrives_after_branch`: - /// `delegation` first, then `delegation.depth`. `insert` turns the - /// single value into a map and keeps `depth`. Direct `insert` - /// so a `HashMap` cannot skip this path. #[test] fn namespace_wins_when_branch_arrives_after_scalar() { let mut root = BTreeMap::new(); @@ -448,7 +426,7 @@ mod tests { Some(Node::Leaf(Value::Int(3))) ) ), - "a later dotted key must promote the scalar to a map and keep depth as Int(3)", + "namespace must replace the scalar", ); } } diff --git a/builtins/pdps/cel/src/resolver.rs b/builtins/pdps/cel/src/resolver.rs index 03dc5630..920f3a49 100644 --- a/builtins/pdps/cel/src/resolver.rs +++ b/builtins/pdps/cel/src/resolver.rs @@ -145,17 +145,8 @@ impl CelResolver { /// /// Composes: calling `with_functions` more than once stacks the /// callbacks. Each runs in registration order on every context. - /// If two custom setups register the same name, the later one wins. - /// - /// Prefer names that are not already in the CEL standard library - /// (`size`, `matches`, `double`, …). `with_functions` does not - /// reject a colliding name. Which body runs is decided by CEL, not - /// by this crate: today CEL tries a matching built-in first - /// (`size("hello")` stays the standard `size`) and calls the - /// custom function only when the built-in has no matching form - /// (`size(42)` can run a custom `size` that takes an int). That - /// dispatch is not part of this resolver's contract. `has(...)` is - /// rewritten by the parser, so a custom `has` never runs. + /// Later registrations replace earlier custom functions with the same name. + /// Standard-library overloads take precedence; `has` is parser-reserved. /// /// # Example /// @@ -682,9 +673,6 @@ mod tests { ); } - /// A custom `size(int) -> 777` must not replace stdlib - /// `size("hello")` (still 5), but must run for `size(42)` because - /// the stdlib has no `size` that takes an int. #[tokio::test] async fn custom_size_stdlib_overload_then_int_fallback() { let r = CelResolver::new().with_functions(|ctx| { @@ -696,28 +684,15 @@ mod tests { .evaluate(&cel_call("size('hello') == 5"), &bag) .await .unwrap(); - assert_eq!( - stdlib.decision, - Decision::Allow, - "stdlib size('hello') is 5; a matching built-in must win", - ); + assert_eq!(stdlib.decision, Decision::Allow); let fallback = r .evaluate(&cel_call("size(42) == 777"), &bag) .await .unwrap(); - assert_eq!( - fallback.decision, - Decision::Allow, - "no stdlib size(int); the custom function must run as fallback", - ); + assert_eq!(fallback.decision, Decision::Allow); } - /// Two custom setups that register the same name and signature: - /// the later registration's body must win. Pins the doc-comment - /// claim on `with_functions` ("If two custom setups register the - /// same name, the later one wins") so a silent semantics change - /// in the CEL crate breaks loudly here. #[tokio::test] async fn later_custom_function_overwrites_earlier_one() { let r = CelResolver::new() @@ -725,31 +700,21 @@ mod tests { ctx.add_function("magic", |n: i64| -> i64 { n + 100 }); }) .with_functions(|ctx| { - // Same name, same signature, different body. ctx.add_function("magic", |n: i64| -> i64 { n + 999 }); }); let bag = bag_with(&[("subject.id", "alice")]); - // The later registration (n + 999) must be the one that runs. let out = r .evaluate(&cel_call("magic(1) == 1000"), &bag) .await .unwrap(); - assert_eq!( - out.decision, - Decision::Allow, - "the later custom function body must win; expected magic(1) == 1000", - ); + assert_eq!(out.decision, Decision::Allow); - // Confirm the earlier body (n + 100) is NOT the one running. let out = r .evaluate(&cel_call("magic(1) == 101"), &bag) .await .unwrap(); - assert!( - matches!(out.decision, Decision::Deny { .. }), - "the earlier custom function body (n + 100) must be shadowed", - ); + assert!(matches!(out.decision, Decision::Deny { .. })); } /// The `regex` cel-feature is explicitly enabled in our Cargo.toml.