From 826a8d5b7f5cfed2732969ded738beb460a89523 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 13:02:58 +0000 Subject: [PATCH 1/7] feat(privacy-filter): apply capture-group replacement in redact Redact previously replaced the whole field with a static string. When the pattern has capturing groups, treat replacement as a regex template ($1, $name) like awatcher filters. No capturing groups keeps the existing whole-field behavior. Related to ActivityWatch/aw-server-rust#659. --- aw-datastore/src/privacy_filter.rs | 133 +++++++++++++++++++++++++++-- 1 file changed, 127 insertions(+), 6 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index 634e8693..9341c854 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -29,7 +29,12 @@ pub struct PrivacyFilterRule { pub pattern: String, /// What to do when matched pub action: PrivacyFilterAction, - /// Replacement text for the redact action + /// Replacement text for the redact action. + /// + /// When `pattern` has capturing groups, this is a regex replacement + /// template (`$1`, `$2`, `$name`) applied to the matched text — the same + /// semantics as awatcher filters. Without capturing groups, the entire + /// field is replaced with this string. pub replacement: Option, /// Pre-compiled regex, populated lazily on first match. Not serialized. #[serde(skip)] @@ -79,6 +84,21 @@ impl PrivacyFilterRule { } } + /// Redact `source` using this rule's pattern and `replacement`. + /// + /// Capturing groups enable `$1`/`$name` substitution of the matched + /// text (awatcher-compatible). No capturing groups → replace the whole + /// field with the static string. + fn redact_value(&self, source: &str, replacement: &str) -> String { + let re = self + .regex_cache + .get_or_init(|| regex::Regex::new(&self.pattern).ok()); + match re.as_ref() { + Some(re) if re.captures_len() > 1 => re.replace_all(source, replacement).into_owned(), + _ => replacement.to_owned(), + } + } + /// Apply this rule's action to an event. /// Returns None if dropped, Some(event) if kept (possibly redacted). pub fn apply<'a>(&self, event: &'a mut Event) -> Option<&'a mut Event> { @@ -87,11 +107,14 @@ impl PrivacyFilterRule { PrivacyFilterAction::Redact => { if let Some(ref replacement) = self.replacement { if let Some(ref field_path) = self.field { - set_field( - &mut event.data, - field_path, - Value::String(replacement.clone()), - ); + let current = resolve_field(&event.data, field_path) + .and_then(|v| v.as_str()) + .map(str::to_owned); + let new_value = match current.as_deref() { + Some(source) => self.redact_value(source, replacement), + None => replacement.clone(), + }; + set_field(&mut event.data, field_path, Value::String(new_value)); } } Some(event) @@ -419,4 +442,102 @@ mod tests { let result = rule.apply(&mut event); assert!(result.is_none(), "Drop action should return None"); } + + fn redact_rule(pattern: &str, replacement: &str) -> PrivacyFilterRule { + PrivacyFilterRule { + enabled: true, + bucket_prefix: None, + field: Some("title".to_string()), + pattern: pattern.to_string(), + action: PrivacyFilterAction::Redact, + replacement: Some(replacement.to_string()), + regex_cache: OnceLock::new(), + } + } + + #[test] + fn test_redact_without_captures_replaces_whole_field() { + let rule = redact_rule(r"(?i).*banking.*", "REDACTED"); + let mut event = test_event("Online Banking - My Account Balance"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "REDACTED" + ); + } + + #[test] + fn test_redact_with_capture_groups_like_awatcher() { + // awatcher: match `org\.kde\.(.*)`, replace `$1` → `dolphin` + let rule = redact_rule(r"(.*) - Mozilla Firefox", "$1"); + let mut event = test_event("GitHub - Mozilla Firefox"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "GitHub" + ); + } + + #[test] + fn test_redact_capture_replaces_only_the_match() { + let rule = redact_rule(r"(secret)", "REDACTED"); + let mut event = test_event("my secret file"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "my REDACTED file" + ); + } + + #[test] + fn test_redact_strips_url_path_keep_host() { + let rule = redact_rule(r"https://([^/]+)/.*", "https://$1/"); + let mut event = test_event("https://bank.example/account?token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "https://bank.example/" + ); + } + + #[test] + fn test_redact_named_capture_like_awatcher() { + let rule = redact_rule(r"https://(?P[^/]+)/.*", "https://$host/"); + let mut event = test_event("https://bank.example/account?token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "https://bank.example/" + ); + } + + #[test] + fn test_redact_awatcher_vscode_dirty_indicator() { + // awatcher README: match-title = "● (.*)", replace-title = "$1" + let rule = redact_rule(r"● (.*)", "$1"); + let mut event = test_event("● file_config.rs - awatcher - Visual Studio Code"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "file_config.rs - awatcher - Visual Studio Code" + ); + } + + #[test] + fn test_redact_replace_all_occurrences() { + let rule = redact_rule(r"(token)", "REDACTED"); + let mut event = test_event("token=abc token=def"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "REDACTED=abc REDACTED=def" + ); + } } From 6a3888105295fbd0572d934b1a97e4cb4e7984a7 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 13:14:37 +0000 Subject: [PATCH 2/7] fix(privacy-filter): keep whole-field redact unless replacement is a capture template Capture-group replacement was keyed only on captures_len > 1, so an existing stored rule like `(token)` + `REDACTED` switched from whole-field redaction to replace_all and leaked unmatched text (`token=abc token=def` became `REDACTED=abc REDACTED=def`). Opt in only when the replacement contains a capture template (`$1`, `$name`). Static replacements keep whole-field behavior. Addresses Greptile P1 on ActivityWatch/aw-server-rust#665. --- aw-datastore/src/privacy_filter.rs | 73 ++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 15 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index 9341c854..61270d28 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -31,10 +31,10 @@ pub struct PrivacyFilterRule { pub action: PrivacyFilterAction, /// Replacement text for the redact action. /// - /// When `pattern` has capturing groups, this is a regex replacement - /// template (`$1`, `$2`, `$name`) applied to the matched text — the same - /// semantics as awatcher filters. Without capturing groups, the entire - /// field is replaced with this string. + /// Capture substitution (`$1`, `$2`, `$name`) is opt-in: it runs only + /// when `pattern` has capturing groups *and* this string contains a + /// capture template. Otherwise the entire field is replaced — so stored + /// rules like `(token)` + `REDACTED` keep whole-field redaction. pub replacement: Option, /// Pre-compiled regex, populated lazily on first match. Not serialized. #[serde(skip)] @@ -86,15 +86,19 @@ impl PrivacyFilterRule { /// Redact `source` using this rule's pattern and `replacement`. /// - /// Capturing groups enable `$1`/`$name` substitution of the matched - /// text (awatcher-compatible). No capturing groups → replace the whole - /// field with the static string. + /// `$1`/`$name` substitution (awatcher-compatible) runs only when the + /// pattern has capturing groups *and* `replacement` is a capture + /// template. A static replacement always replaces the whole field, even + /// if the pattern contains groups — otherwise existing stored rules + /// would silently leak unmatched sensitive text. fn redact_value(&self, source: &str, replacement: &str) -> String { let re = self .regex_cache .get_or_init(|| regex::Regex::new(&self.pattern).ok()); match re.as_ref() { - Some(re) if re.captures_len() > 1 => re.replace_all(source, replacement).into_owned(), + Some(re) if re.captures_len() > 1 && replacement_is_capture_template(replacement) => { + re.replace_all(source, replacement).into_owned() + } _ => replacement.to_owned(), } } @@ -227,6 +231,29 @@ impl PrivacyFilterEngine { } } +/// True when `replacement` contains a regex capture template (`$1`, `$name`, +/// `${name}`, `$0`). `$$` is an escaped dollar and does not count. +fn replacement_is_capture_template(replacement: &str) -> bool { + let bytes = replacement.as_bytes(); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'$' { + if bytes.get(i + 1) == Some(&b'$') { + i += 2; + continue; + } + if let Some(&next) = bytes.get(i + 1) { + if next.is_ascii_alphanumeric() || matches!(next, b'{' | b'_' | b'&' | b'\'' | b'`') + { + return true; + } + } + } + i += 1; + } + false +} + /// Resolve a dotted field path (e.g. "title", "data.url") from a serde_json Map. fn resolve_field<'a>(data: &'a Map, path: &str) -> Option<&'a Value> { let parts: Vec<&str> = path.split('.').collect(); @@ -481,14 +508,17 @@ mod tests { } #[test] - fn test_redact_capture_replaces_only_the_match() { - let rule = redact_rule(r"(secret)", "REDACTED"); - let mut event = test_event("my secret file"); + fn test_redact_static_replacement_with_captures_stays_whole_field() { + // Existing stored rules may use capturing groups with a static + // replacement. Partial replace_all would leak unmatched text + // (`token=abc token=def` → `REDACTED=abc REDACTED=def`). + let rule = redact_rule(r"(token)", "REDACTED"); + let mut event = test_event("token=abc token=def"); assert!(rule.matches("any-bucket", &event)); let result = rule.apply(&mut event).unwrap(); assert_eq!( result.data.get("title").unwrap().as_str().unwrap(), - "my REDACTED file" + "REDACTED" ); } @@ -530,14 +560,27 @@ mod tests { } #[test] - fn test_redact_replace_all_occurrences() { - let rule = redact_rule(r"(token)", "REDACTED"); + fn test_redact_replace_all_occurrences_with_template() { + let rule = redact_rule(r"(token)=\S+", "$1=REDACTED"); let mut event = test_event("token=abc token=def"); assert!(rule.matches("any-bucket", &event)); let result = rule.apply(&mut event).unwrap(); assert_eq!( result.data.get("title").unwrap().as_str().unwrap(), - "REDACTED=abc REDACTED=def" + "token=REDACTED token=REDACTED" ); } + + #[test] + fn test_replacement_is_capture_template() { + assert!(replacement_is_capture_template("$1")); + assert!(replacement_is_capture_template("https://$1/")); + assert!(replacement_is_capture_template("https://$host/")); + assert!(replacement_is_capture_template("${1}")); + assert!(replacement_is_capture_template("$0")); + assert!(!replacement_is_capture_template("REDACTED")); + assert!(!replacement_is_capture_template("token=REDACTED")); + assert!(!replacement_is_capture_template("$$")); + assert!(!replacement_is_capture_template("cost $$5")); + } } From 77caac2c83437738334f39ea3566594704780d35 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 13:22:42 +0000 Subject: [PATCH 3/7] fix(privacy-filter): treat dangling $N refs as whole-field redact replacement_is_capture_template now requires every $ reference to name a group that exists on the compiled regex. A replacement like `REDACTED $5` on a 1-group pattern would otherwise take replace_all, expand $5 to "", and leak unmatched field text. Addresses in-band P1 on ActivityWatch/aw-server-rust#665. --- aw-datastore/src/privacy_filter.rs | 122 +++++++++++++++++++++++------ 1 file changed, 99 insertions(+), 23 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index 61270d28..cca6ec64 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -96,7 +96,9 @@ impl PrivacyFilterRule { .regex_cache .get_or_init(|| regex::Regex::new(&self.pattern).ok()); match re.as_ref() { - Some(re) if re.captures_len() > 1 && replacement_is_capture_template(replacement) => { + Some(re) + if re.captures_len() > 1 && replacement_is_capture_template(replacement, re) => + { re.replace_all(source, replacement).into_owned() } _ => replacement.to_owned(), @@ -231,27 +233,79 @@ impl PrivacyFilterEngine { } } -/// True when `replacement` contains a regex capture template (`$1`, `$name`, -/// `${name}`, `$0`). `$$` is an escaped dollar and does not count. -fn replacement_is_capture_template(replacement: &str) -> bool { +/// True when `replacement` is a capture template whose every `$` reference +/// names a group that exists on `re`. `$$` is an escaped dollar. +/// +/// Dangling refs (`$5` on a 1-group pattern) must not opt into `replace_all`: +/// the regex crate expands unknown groups to `""`, which would leak unmatched +/// field text. Those replacements stay whole-field. +fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool { + let n_groups = re.captures_len(); + let named: Vec<&str> = re.capture_names().flatten().collect(); let bytes = replacement.as_bytes(); let mut i = 0; + let mut saw_valid_ref = false; while i < bytes.len() { - if bytes[i] == b'$' { - if bytes.get(i + 1) == Some(&b'$') { - i += 2; - continue; - } - if let Some(&next) = bytes.get(i + 1) { - if next.is_ascii_alphanumeric() || matches!(next, b'{' | b'_' | b'&' | b'\'' | b'`') - { - return true; + if bytes[i] != b'$' { + i += 1; + continue; + } + if bytes.get(i + 1) == Some(&b'$') { + i += 2; + continue; + } + let rest = &replacement[i + 1..]; + if rest.is_empty() { + break; + } + let first = rest.as_bytes()[0]; + if first == b'{' { + match rest[1..].find('}') { + Some(end) => { + let name = &rest[1..1 + end]; + if !capture_ref_exists(name, n_groups, &named) { + return false; + } + saw_valid_ref = true; + i += 2 + end + 1; // ${ name } } + None => return false, } + continue; + } + if matches!(first, b'&' | b'`' | b'\'') { + saw_valid_ref = true; + i += 2; + continue; + } + // Longest ident, matching the regex crate: `$1a` is name `1a`, not `$1` + `a`. + if first.is_ascii_alphanumeric() || first == b'_' { + let rb = rest.as_bytes(); + let mut j = 1; + while j < rb.len() && (rb[j].is_ascii_alphanumeric() || rb[j] == b'_') { + j += 1; + } + let name = &rest[..j]; + if !capture_ref_exists(name, n_groups, &named) { + return false; + } + saw_valid_ref = true; + i += 1 + j; + continue; } i += 1; } - false + saw_valid_ref +} + +fn capture_ref_exists(name: &str, n_groups: usize, named: &[&str]) -> bool { + if name.is_empty() { + return true; // `${}` / `$0` equivalent: the whole match + } + if name.bytes().all(|b| b.is_ascii_digit()) { + return name.parse::().ok().is_some_and(|n| n < n_groups); + } + named.iter().any(|n| *n == name) } /// Resolve a dotted field path (e.g. "title", "data.url") from a serde_json Map. @@ -571,16 +625,38 @@ mod tests { ); } + #[test] + fn test_redact_dangling_capture_ref_stays_whole_field() { + // `$5` is not group 1; replace_all would expand it to "" and leak `=abc`. + let rule = redact_rule(r"(token)", "REDACTED $5"); + let mut event = test_event("token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "REDACTED $5" + ); + } + #[test] fn test_replacement_is_capture_template() { - assert!(replacement_is_capture_template("$1")); - assert!(replacement_is_capture_template("https://$1/")); - assert!(replacement_is_capture_template("https://$host/")); - assert!(replacement_is_capture_template("${1}")); - assert!(replacement_is_capture_template("$0")); - assert!(!replacement_is_capture_template("REDACTED")); - assert!(!replacement_is_capture_template("token=REDACTED")); - assert!(!replacement_is_capture_template("$$")); - assert!(!replacement_is_capture_template("cost $$5")); + let one = regex::Regex::new(r"(token)").unwrap(); + let named = regex::Regex::new(r"(?P[^/]+)").unwrap(); + let two = regex::Regex::new(r"(a)(b)").unwrap(); + + assert!(replacement_is_capture_template("$1", &one)); + assert!(replacement_is_capture_template("https://$1/", &one)); + assert!(replacement_is_capture_template("${1}", &one)); + assert!(replacement_is_capture_template("$0", &one)); + assert!(replacement_is_capture_template("https://$host/", &named)); + assert!(replacement_is_capture_template("$1$2", &two)); + assert!(!replacement_is_capture_template("REDACTED", &one)); + assert!(!replacement_is_capture_template("token=REDACTED", &one)); + assert!(!replacement_is_capture_template("$$", &one)); + assert!(!replacement_is_capture_template("cost $$5", &one)); + assert!(!replacement_is_capture_template("REDACTED $5", &one)); + assert!(!replacement_is_capture_template("$2", &one)); + assert!(!replacement_is_capture_template("$host", &one)); + assert!(!replacement_is_capture_template("$1$2", &one)); } } From 5f98df47d8575fb5ccbfea388bf9e8dcb7409d47 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 13:34:51 +0000 Subject: [PATCH 4/7] fix(privacy-filter): do not treat Perl $&/$`/$' as capture templates The regex crate only interpolates $N / $name / ${name}. $& /$` /$' are Perl-only; treating them as valid refs would take replace_all and leak unmatched field text. Fall through to whole-field redaction instead. --- aw-datastore/src/privacy_filter.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index cca6ec64..8c9387dd 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -273,12 +273,8 @@ fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool } continue; } - if matches!(first, b'&' | b'`' | b'\'') { - saw_valid_ref = true; - i += 2; - continue; - } // Longest ident, matching the regex crate: `$1a` is name `1a`, not `$1` + `a`. + // `$&`/`$``/`$'` are Perl-only and are *not* interpolated by `regex`. if first.is_ascii_alphanumeric() || first == b'_' { let rb = rest.as_bytes(); let mut j = 1; @@ -658,5 +654,8 @@ mod tests { assert!(!replacement_is_capture_template("$2", &one)); assert!(!replacement_is_capture_template("$host", &one)); assert!(!replacement_is_capture_template("$1$2", &one)); + assert!(!replacement_is_capture_template("REDACTED $'", &one)); + assert!(!replacement_is_capture_template("REDACTED $&", &one)); + assert!(!replacement_is_capture_template("REDACTED $`", &one)); } } From fcab2085156ae140ee5ee4b2cc2092a6784377aa Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 13:55:07 +0000 Subject: [PATCH 5/7] fix(privacy-filter): do not treat empty ${} as a capture template `${}` is a named ref with an empty name, not `$0`. The regex crate expands it to "" and would leak unmatched field text via replace_all. Addresses Greptile P1 on ActivityWatch/aw-server-rust#665. Git-Session-Id: 589e6fad-191b-5e97-93c6-70c22c1f5d4c --- aw-datastore/src/privacy_filter.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index 8c9387dd..ac330cdf 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -236,9 +236,9 @@ impl PrivacyFilterEngine { /// True when `replacement` is a capture template whose every `$` reference /// names a group that exists on `re`. `$$` is an escaped dollar. /// -/// Dangling refs (`$5` on a 1-group pattern) must not opt into `replace_all`: -/// the regex crate expands unknown groups to `""`, which would leak unmatched -/// field text. Those replacements stay whole-field. +/// Dangling refs (`$5` on a 1-group pattern) and empty `${}` must not opt +/// into `replace_all`: the regex crate expands unknown / empty-named groups +/// to `""`, which would leak unmatched field text. Those stay whole-field. fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool { let n_groups = re.captures_len(); let named: Vec<&str> = re.capture_names().flatten().collect(); @@ -295,8 +295,10 @@ fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool } fn capture_ref_exists(name: &str, n_groups: usize, named: &[&str]) -> bool { + // `${}` is a named ref with an empty name, *not* `$0`. The regex crate + // expands it to "" (no such group), which would leak unmatched text. if name.is_empty() { - return true; // `${}` / `$0` equivalent: the whole match + return false; } if name.bytes().all(|b| b.is_ascii_digit()) { return name.parse::().ok().is_some_and(|n| n < n_groups); @@ -634,6 +636,16 @@ mod tests { ); } + #[test] + fn test_redact_empty_braced_ref_stays_whole_field() { + // `${}` names no group; replace_all would expand it to "" and leak `=abc`. + let rule = redact_rule(r"(token)", "${}"); + let mut event = test_event("token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!(result.data.get("title").unwrap().as_str().unwrap(), "${}"); + } + #[test] fn test_replacement_is_capture_template() { let one = regex::Regex::new(r"(token)").unwrap(); @@ -657,5 +669,8 @@ mod tests { assert!(!replacement_is_capture_template("REDACTED $'", &one)); assert!(!replacement_is_capture_template("REDACTED $&", &one)); assert!(!replacement_is_capture_template("REDACTED $`", &one)); + assert!(!replacement_is_capture_template("${}", &one)); + assert!(!replacement_is_capture_template("https://${}/", &one)); + assert!(replacement_is_capture_template("${0}", &one)); } } From 621008bfd88554c87350a07a30a375634c0f207b Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 14:26:21 +0000 Subject: [PATCH 6/7] fix(privacy-filter): reject $0 and unmatched capture groups $0 / ${0} is the whole match; replace_all would persist the match and unmatched field text. Alternation groups that exist on the regex but do not participate in a match expand to empty and leak leftover text. Stay whole-field unless every referenced group is present in every match. Addresses Greptile P1 on ActivityWatch/aw-server-rust#665. Git-Session-Id: d3267f9a-1817-52b6-92cd-86e006957db2 --- aw-datastore/src/privacy_filter.rs | 187 +++++++++++++++++++++-------- 1 file changed, 137 insertions(+), 50 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index ac330cdf..2dcf7e92 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -33,8 +33,10 @@ pub struct PrivacyFilterRule { /// /// Capture substitution (`$1`, `$2`, `$name`) is opt-in: it runs only /// when `pattern` has capturing groups *and* this string contains a - /// capture template. Otherwise the entire field is replaced — so stored - /// rules like `(token)` + `REDACTED` keep whole-field redaction. + /// capture template whose referenced groups participate in the match. + /// `$0` and unmatched alternation groups stay whole-field. Static + /// replacements always replace the entire field — so stored rules like + /// `(token)` + `REDACTED` do not leak unmatched text. pub replacement: Option, /// Pre-compiled regex, populated lazily on first match. Not serialized. #[serde(skip)] @@ -88,19 +90,20 @@ impl PrivacyFilterRule { /// /// `$1`/`$name` substitution (awatcher-compatible) runs only when the /// pattern has capturing groups *and* `replacement` is a capture - /// template. A static replacement always replaces the whole field, even - /// if the pattern contains groups — otherwise existing stored rules - /// would silently leak unmatched sensitive text. + /// template whose every referenced group participates in every match. + /// `$0` and unmatched alternation/optional groups stay whole-field — + /// otherwise `replace_all` would leak unmatched sensitive text. fn redact_value(&self, source: &str, replacement: &str) -> String { let re = self .regex_cache .get_or_init(|| regex::Regex::new(&self.pattern).ok()); match re.as_ref() { - Some(re) - if re.captures_len() > 1 && replacement_is_capture_template(replacement, re) => - { - re.replace_all(source, replacement).into_owned() - } + Some(re) if re.captures_len() > 1 => match capture_refs_in_template(replacement, re) { + Some(refs) if referenced_captures_present(re, source, &refs) => { + re.replace_all(source, replacement).into_owned() + } + _ => replacement.to_owned(), + }, _ => replacement.to_owned(), } } @@ -233,18 +236,26 @@ impl PrivacyFilterEngine { } } -/// True when `replacement` is a capture template whose every `$` reference -/// names a group that exists on `re`. `$$` is an escaped dollar. +/// A `$` reference in a replacement template: `$1` / `${1}` or `$name` / `${name}`. +enum CaptureRef { + Index(usize), + Name(String), +} + +/// Parse `replacement` as a capture template whose every `$` reference names +/// a group that exists on `re`. `$$` is an escaped dollar. /// -/// Dangling refs (`$5` on a 1-group pattern) and empty `${}` must not opt +/// Returns `None` (stay whole-field) for dangling refs, empty `${}`, `$0` +/// (whole-match identity), and Perl-only `$&`/`$``/`$'`. Those must not opt /// into `replace_all`: the regex crate expands unknown / empty-named groups -/// to `""`, which would leak unmatched field text. Those stay whole-field. -fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool { +/// to `""`, and `$0` substitutes the match unchanged — both leak unmatched +/// field text. +fn capture_refs_in_template(replacement: &str, re: ®ex::Regex) -> Option> { let n_groups = re.captures_len(); let named: Vec<&str> = re.capture_names().flatten().collect(); let bytes = replacement.as_bytes(); let mut i = 0; - let mut saw_valid_ref = false; + let mut refs = Vec::new(); while i < bytes.len() { if bytes[i] != b'$' { i += 1; @@ -263,13 +274,10 @@ fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool match rest[1..].find('}') { Some(end) => { let name = &rest[1..1 + end]; - if !capture_ref_exists(name, n_groups, &named) { - return false; - } - saw_valid_ref = true; + refs.push(resolve_capture_ref(name, n_groups, &named)?); i += 2 + end + 1; // ${ name } } - None => return false, + None => return None, } continue; } @@ -282,28 +290,59 @@ fn replacement_is_capture_template(replacement: &str, re: ®ex::Regex) -> bool j += 1; } let name = &rest[..j]; - if !capture_ref_exists(name, n_groups, &named) { - return false; - } - saw_valid_ref = true; + refs.push(resolve_capture_ref(name, n_groups, &named)?); i += 1 + j; continue; } i += 1; } - saw_valid_ref + if refs.is_empty() { + None + } else { + Some(refs) + } } -fn capture_ref_exists(name: &str, n_groups: usize, named: &[&str]) -> bool { +fn resolve_capture_ref(name: &str, n_groups: usize, named: &[&str]) -> Option { // `${}` is a named ref with an empty name, *not* `$0`. The regex crate // expands it to "" (no such group), which would leak unmatched text. if name.is_empty() { - return false; + return None; } if name.bytes().all(|b| b.is_ascii_digit()) { - return name.parse::().ok().is_some_and(|n| n < n_groups); + let n = name.parse::().ok()?; + // Group 0 is the whole match. `$0` / `${0}` would take replace_all + // and persist the match plus unmatched field text unchanged. + if n > 0 && n < n_groups { + return Some(CaptureRef::Index(n)); + } + return None; } - named.iter().any(|n| *n == name) + named + .contains(&name) + .then(|| CaptureRef::Name(name.to_owned())) +} + +/// True when every referenced group participates in every match. +/// +/// Alternation / optional groups exist on the regex but may be `None` for a +/// particular match (`$2` with `(token)|(secret)` matching `token=abc`). +/// `replace_all` expands those to `""` and leaks unmatched field text. +fn referenced_captures_present(re: ®ex::Regex, source: &str, refs: &[CaptureRef]) -> bool { + let mut any = false; + for caps in re.captures_iter(source) { + any = true; + for r in refs { + let present = match r { + CaptureRef::Index(n) => caps.get(*n).is_some(), + CaptureRef::Name(name) => caps.name(name).is_some(), + }; + if !present { + return false; + } + } + } + any } /// Resolve a dotted field path (e.g. "title", "data.url") from a serde_json Map. @@ -652,25 +691,73 @@ mod tests { let named = regex::Regex::new(r"(?P[^/]+)").unwrap(); let two = regex::Regex::new(r"(a)(b)").unwrap(); - assert!(replacement_is_capture_template("$1", &one)); - assert!(replacement_is_capture_template("https://$1/", &one)); - assert!(replacement_is_capture_template("${1}", &one)); - assert!(replacement_is_capture_template("$0", &one)); - assert!(replacement_is_capture_template("https://$host/", &named)); - assert!(replacement_is_capture_template("$1$2", &two)); - assert!(!replacement_is_capture_template("REDACTED", &one)); - assert!(!replacement_is_capture_template("token=REDACTED", &one)); - assert!(!replacement_is_capture_template("$$", &one)); - assert!(!replacement_is_capture_template("cost $$5", &one)); - assert!(!replacement_is_capture_template("REDACTED $5", &one)); - assert!(!replacement_is_capture_template("$2", &one)); - assert!(!replacement_is_capture_template("$host", &one)); - assert!(!replacement_is_capture_template("$1$2", &one)); - assert!(!replacement_is_capture_template("REDACTED $'", &one)); - assert!(!replacement_is_capture_template("REDACTED $&", &one)); - assert!(!replacement_is_capture_template("REDACTED $`", &one)); - assert!(!replacement_is_capture_template("${}", &one)); - assert!(!replacement_is_capture_template("https://${}/", &one)); - assert!(replacement_is_capture_template("${0}", &one)); + assert!(capture_refs_in_template("$1", &one).is_some()); + assert!(capture_refs_in_template("https://$1/", &one).is_some()); + assert!(capture_refs_in_template("${1}", &one).is_some()); + assert!(capture_refs_in_template("$0", &one).is_none()); + assert!(capture_refs_in_template("https://$host/", &named).is_some()); + assert!(capture_refs_in_template("$1$2", &two).is_some()); + assert!(capture_refs_in_template("REDACTED", &one).is_none()); + assert!(capture_refs_in_template("token=REDACTED", &one).is_none()); + assert!(capture_refs_in_template("$$", &one).is_none()); + assert!(capture_refs_in_template("cost $$5", &one).is_none()); + assert!(capture_refs_in_template("REDACTED $5", &one).is_none()); + assert!(capture_refs_in_template("$2", &one).is_none()); + assert!(capture_refs_in_template("$host", &one).is_none()); + assert!(capture_refs_in_template("$1$2", &one).is_none()); + assert!(capture_refs_in_template("REDACTED $'", &one).is_none()); + assert!(capture_refs_in_template("REDACTED $&", &one).is_none()); + assert!(capture_refs_in_template("REDACTED $`", &one).is_none()); + assert!(capture_refs_in_template("${}", &one).is_none()); + assert!(capture_refs_in_template("https://${}/", &one).is_none()); + assert!(capture_refs_in_template("${0}", &one).is_none()); + assert!(capture_refs_in_template("$1$0", &one).is_none()); + } + + #[test] + fn test_redact_group_zero_stays_whole_field() { + // `$0` is the whole match. replace_all would substitute it unchanged + // and leave unmatched sensitive text (`token=abc` stays `token=abc`). + let rule = redact_rule(r"(token)", "$0"); + let mut event = test_event("token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!(result.data.get("title").unwrap().as_str().unwrap(), "$0"); + } + + #[test] + fn test_redact_braced_group_zero_stays_whole_field() { + let rule = redact_rule(r"(token)", "${0}"); + let mut event = test_event("token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!(result.data.get("title").unwrap().as_str().unwrap(), "${0}"); + } + + #[test] + fn test_redact_unmatched_alternation_capture_stays_whole_field() { + // `$2` exists on `(token)|(secret)` but does not participate when + // the first alternative matches. replace_all expands it to "" and + // leaves `=abc`. Fail closed: whole-field redaction. + let rule = redact_rule(r"(token)|(secret)", "$2"); + let mut event = test_event("token=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!(result.data.get("title").unwrap().as_str().unwrap(), "$2"); + } + + #[test] + fn test_redact_participating_alternation_capture_still_replaces() { + // Same pattern, but `$2` *does* participate. Match span is still + // only `secret`; leftover `=abc` is the opt-in replace_all contract + // (same as `$1=REDACTED` keeping the space between tokens). + let rule = redact_rule(r"(token)|(secret)", "$2"); + let mut event = test_event("secret=abc"); + assert!(rule.matches("any-bucket", &event)); + let result = rule.apply(&mut event).unwrap(); + assert_eq!( + result.data.get("title").unwrap().as_str().unwrap(), + "secret=abc" + ); } } From 0de516420df7126cf56f78984562d01b02be8bc7 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 30 Aug 2026 14:45:40 +0000 Subject: [PATCH 7/7] fix(privacy-filter): fail closed on mixed malformed capture templates A valid $1 plus a dangling or unsupported dollar form ($1$, $1$&) used to skip the suffix and still enable replace_all, leaking unmatched field text. Any unparsed $ now keeps the rule whole-field. $1$$ (group plus literal dollar) still substitutes. Git-Session-Id: 1c0d8fb8-0b35-5596-9027-cd416fbe048a --- aw-datastore/src/privacy_filter.rs | 58 ++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/aw-datastore/src/privacy_filter.rs b/aw-datastore/src/privacy_filter.rs index 2dcf7e92..a2581bc2 100644 --- a/aw-datastore/src/privacy_filter.rs +++ b/aw-datastore/src/privacy_filter.rs @@ -246,10 +246,11 @@ enum CaptureRef { /// a group that exists on `re`. `$$` is an escaped dollar. /// /// Returns `None` (stay whole-field) for dangling refs, empty `${}`, `$0` -/// (whole-match identity), and Perl-only `$&`/`$``/`$'`. Those must not opt -/// into `replace_all`: the regex crate expands unknown / empty-named groups -/// to `""`, and `$0` substitutes the match unchanged — both leak unmatched -/// field text. +/// (whole-match identity), Perl-only `$&`/`$``/`$'`, and mixed malformed +/// templates such as `$1$` / `$1$&`. Those must not opt into `replace_all`: +/// the regex crate expands unknown / empty-named groups to `""`, `$0` +/// substitutes the match unchanged, and a skipped dangling `$` still leaves +/// a valid earlier ref — all leak unmatched field text. fn capture_refs_in_template(replacement: &str, re: ®ex::Regex) -> Option> { let n_groups = re.captures_len(); let named: Vec<&str> = re.capture_names().flatten().collect(); @@ -266,8 +267,10 @@ fn capture_refs_in_template(replacement: &str, re: ®ex::Regex) -> Option Option Option