From e2ef36ab8c3d18b55d31761a2c9ad41221e8c735 Mon Sep 17 00:00:00 2001 From: Nicolas Dreno Date: Fri, 11 Sep 2026 09:50:18 +0200 Subject: [PATCH 1/2] Test inspected values in place instead of copying every one evaluate_step copied every resolved value with to_vec before testing it, even when the rule had no transformations, so the borrowing the resolver does to avoid copying was undone one line later. A rule set resolves thousands of values per request, and most operators read the value as-is. Candidates are now Cow: without transformations the value is tested in place with no copy, and a transformation that changes nothing (t:none, or t:lowercase on already-lowercase input) returns a borrow rather than allocating. Only the value that actually matches is owned, once, for MATCHED_VAR. multiMatch still materialises the value before and after each transformation, since it tests each stage. Behaviour is unchanged: the transformation-order and multiMatch tests pass as before, and the Core Rule Set still compiles to 590 rules and blocks the same traffic. --- crates/parapet/src/transaction.rs | 72 +++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 17 deletions(-) diff --git a/crates/parapet/src/transaction.rs b/crates/parapet/src/transaction.rs index 3b9db73..e6d4ada 100644 --- a/crates/parapet/src/transaction.rs +++ b/crates/parapet/src/transaction.rs @@ -4,6 +4,8 @@ //! `process_*` call runs the rules for that phase. Evaluation stops at the //! first disruptive action, as SecLang specifies. +use std::borrow::Cow; + use crate::action::{Ctl, RuleEngineMode, SetVarOp, Transformation}; use crate::collections::{BodyError, CompiledTarget, OwnedValue, Variables}; use crate::engine::{ChainLink, CompiledRule, Disruptive, RuleSet, SetVarSpec}; @@ -540,23 +542,33 @@ impl<'r> Transaction<'r> { let mut hit: Option = None; for value in values { - // `multiMatch` tests after every transformation, not only the last, - // so a payload that is detectable at an intermediate decoding - // stage is still caught. - let mut candidates: Vec> = Vec::new(); - let mut current = value.value.to_vec(); - if multi_match { - candidates.push(current.clone()); - } - for t in transformations { - current = t.apply(¤t).into_owned(); - if multi_match { - candidates.push(current.clone()); + // The candidates to test, borrowing the resolved value where no copy + // is needed. Without transformations that is the value itself, tested + // in place. `multiMatch` also tests the value before and after each + // transformation, so a payload detectable at an intermediate decoding + // stage is still caught; without it only the fully transformed value + // is tested. + let candidates: Vec> = if multi_match { + let mut cs: Vec> = Vec::with_capacity(transformations.len() + 1); + let mut current: Cow<[u8]> = Cow::Borrowed(&value.value); + cs.push(current.clone()); + for t in transformations { + current = Cow::Owned(t.apply(¤t).into_owned()); + cs.push(current.clone()); } - } - if !multi_match { - candidates.push(current); - } + cs + } else { + let mut current: Cow<[u8]> = Cow::Borrowed(&value.value); + for t in transformations { + // A transformation that changes nothing (e.g. `t:none`, or + // `t:lowercase` on already-lowercase input) returns a borrow; + // only an actual change costs an allocation. + if let Cow::Owned(changed) = t.apply(¤t) { + current = Cow::Owned(changed); + } + } + vec![current] + }; for candidate in candidates { let result = operator.evaluate(&candidate, &self.vars, capture); @@ -566,7 +578,7 @@ impl<'r> Transaction<'r> { } hit = Some(OwnedValue { name: value.name.to_string(), - value: candidate, + value: candidate.into_owned(), }); break; } @@ -1013,6 +1025,32 @@ SecRule ARGS "@rx attack" "id:3,phase:1,pass,setvar:'tx.reached=1'" assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); } + #[test] + fn a_noop_transformation_in_the_chain_does_not_break_matching() { + // `t:none` returns its input unchanged; the value still reaches the + // later `t:lowercase` and the anchored operator matches. + let rs = rules(r#"SecRule ARGS "@rx ^attack$" "id:1,phase:1,deny,t:none,t:lowercase""#); + let mut tx = get(&rs, "/?q=ATTACK"); + assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); + } + + #[test] + fn a_rule_with_no_transformation_matches_the_raw_value() { + let rs = rules(r#"SecRule ARGS "@rx attack" "id:1,phase:1,deny""#); + let mut tx = get(&rs, "/?q=attack"); + assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); + } + + #[test] + fn a_whole_collection_exclusion_does_not_catch_a_longer_collection() { + // `!ARGS` excludes the ARGS collection, not every collection whose name + // starts with "ARGS". A rule inspecting ARGS_GET with that exclusion + // must still see its members. + let rs = rules(r#"SecRule ARGS_GET|!ARGS "@rx attack" "id:1,phase:1,deny""#); + let mut tx = get(&rs, "/?q=attack"); + assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); + } + #[test] fn a_regex_selector_narrows_a_collection() { let rs = rules(r#"SecRule REQUEST_HEADERS:/^X-/ "@rx attack" "id:1,phase:1,deny""#); From 9008799f4788ae8b042c95b7f3d949141df88acf Mon Sep 17 00:00:00 2001 From: Nicolas Dreno Date: Fri, 11 Sep 2026 09:50:18 +0200 Subject: [PATCH 2/2] Match a whole-collection exclusion by collection, not string prefix A selector-less exclusion (`!ARGS`) removed every resolved value whose name began with the collection name, so it also stripped `ARGS_GET:x` and `ARGS_NAMES:x`, whose names start with "ARGS". A rule that excluded one collection while inspecting a longer-named sibling silently lost members. The exclusion now resolves the value's name into (collection, member) and keeps it only when the collection matches exactly: a scalar named `PREFIX`, or a map member named `PREFIX:member`. A name that merely starts with the prefix, like ARGS_GET against ARGS, no longer matches. Selector-bearing exclusions were already anchored by the `:` split and are unaffected; the Core Rule Set, which always writes a selector, is unchanged. --- crates/parapet/src/collections.rs | 41 +++++++++++++++++++------------ 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/crates/parapet/src/collections.rs b/crates/parapet/src/collections.rs index f5ae4de..00b4d4a 100644 --- a/crates/parapet/src/collections.rs +++ b/crates/parapet/src/collections.rs @@ -241,24 +241,33 @@ impl Variables { out.retain(|value| { !excluded.iter().any(|(collection, selector)| { let prefix = collection_name(*collection); - if !value.name.starts_with(prefix) { - return false; - } - match selector { - None => true, - Some(CompiledSelector::Name(n)) => value - .name - .strip_prefix(prefix) - .and_then(|r| r.strip_prefix(':')) - .is_some_and(|member| member.eq_ignore_ascii_case(n)), - Some(CompiledSelector::Regex(re)) => value - .name - .strip_prefix(prefix) - .and_then(|r| r.strip_prefix(':')) - .is_some_and(|member| re.is_match(member)), + // Match the collection exactly, not by string prefix: a + // value is named `PREFIX` (a scalar) or `PREFIX:member` (a + // map). A bare `starts_with(prefix)` would let `!ARGS` also + // exclude `ARGS_GET:x` and `ARGS_NAMES:x`, since those names + // begin with "ARGS". + let member = match value.name.strip_prefix(prefix) { + Some("") => None, + Some(rest) => match rest.strip_prefix(':') { + Some(member) => Some(member), + // A longer collection name that merely starts with + // `prefix`, such as ARGS_GET against ARGS. + None => return false, + }, + None => return false, + }; + match (selector, member) { + // Whole-collection exclusion: every member of it. + (None, _) => true, + // A selector cannot match a scalar, which has no member. + (Some(_), None) => false, + (Some(CompiledSelector::Name(n)), Some(member)) => { + member.eq_ignore_ascii_case(n) + } + (Some(CompiledSelector::Regex(re)), Some(member)) => re.is_match(member), // An XPath exclusion cannot be evaluated without an // XML tree, and XML is never populated yet. - Some(CompiledSelector::XPath(_)) => false, + (Some(CompiledSelector::XPath(_)), _) => false, } }) });