feat(privacy): Privacy & Secret Protection Engine - #19
Conversation
Modular deterministic detectors for AU identifiers, financial data, credentials and crypto material with configurable policy actions and privacy metadata. Extends Bower.Redaction; no AI at runtime.
📝 WalkthroughWalkthroughAdds a deterministic, policy-driven privacy engine with modular detectors for regulated identifiers, financial data, identity fields, secrets, crypto material, and security markings. JSON/text redaction, privacy metadata, compatibility facades, tests, documentation, and benchmarks are included. ChangesPrivacy engine and detector stack
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/Bower.Redaction/Detectors/RegexDetectorBase.cs (1)
29-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant string re-materialization per detector.
The caller (
PrivacyEngine.ApplyDetectorsToValue) already holds the originalstringand only converts it to aReadOnlySpan<char>before callingDetect. Here,text.ToString()immediately reallocates an identical string — repeated once per detector (~30 detectors) for every scanned field.Regex.EnumerateMatches(ReadOnlySpan<char>)(available since .NET 7; this repo already relies on .NET 9 APIs elsewhere, e.g.Convert.ToHexStringLowerinPolicyApplicator.cs) can iterate matches directly over the span without allocating, usingValueMatch.Index/.Lengthinstead ofMatch.Index/.Length.♻️ Proposed refactor using EnumerateMatches
public virtual void Detect(ReadOnlySpan<char> text, string path, ICollection<DetectionMatch> matches) { - // Regex operates on string; single allocation only when content present. if (text.IsEmpty) { return; } - string s = text.ToString(); - foreach (Match match in Pattern.Matches(s)) + foreach (ValueMatch match in Pattern.EnumerateMatches(text)) { - if (!match.Success) - { - continue; - } - - ReadOnlySpan<char> span = s.AsSpan(match.Index, match.Length); + ReadOnlySpan<char> span = text.Slice(match.Index, match.Length); bool validated = !RequiresValidation || Validate(span); if (RequiresValidation && !validated) { continue; } matches.Add(new DetectionMatch( Id, Category, match.Index, match.Length, validated, SubKind(span))); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bower.Redaction/Detectors/RegexDetectorBase.cs` around lines 29 - 30, Update the detector loop in RegexDetectorBase.Detect to call Pattern.EnumerateMatches directly with the existing ReadOnlySpan<char> instead of materializing text.ToString(). Replace Match usage with the returned ValueMatch type and preserve the current index and length handling through ValueMatch.Index and ValueMatch.Length.src/Bower.Redaction/SensitiveDataDetector.cs (1)
26-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting that
maskInPlaceis now ignored.The parameter is discarded (
_ = maskInPlace;) purely for backward compatibility. A one-line XML doc note on the parameter would help existing callers understand redaction is now always policy-driven regardless of this flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bower.Redaction/SensitiveDataDetector.cs` around lines 26 - 39, Update the XML documentation for SensitiveScanResult.ScanAndRedact to note that the maskInPlace parameter is retained for backward compatibility but ignored, since redaction is always policy-driven. Keep the existing _ = maskInPlace; behavior unchanged.src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs (1)
6-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd match timeouts to
[GeneratedRegex]detectors that process untrusted payloads.ApiKeyDetectorinSecretDetectors.csalready sets aTimeSpan.FromMilliseconds(50)timeout on its hand-rolledRegexinstances, showing the team is aware regex on untrusted event content needs a timeout guard — but every[GeneratedRegex]-based detector across the catalog omitsmatchTimeoutMilliseconds.GeneratedRegexAttributesupports a(pattern, options, matchTimeoutMilliseconds)constructor overload, so this is a low-effort, drop-in fix.
src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs#L6-L234: addmatchTimeoutMillisecondstoAwsRegex,AzureRegex,EntraRegex,GcpRegex,JwtRegex,OAuthRegex,K8sRegex,DockerRegex,DbRegex(line 219 combines[^;]+;with unbounded.*, the riskiest pattern in this file), andEnvRegex.src/Bower.Redaction/Detectors/Financial/FinancialDetectors.cs#L101-L102: add a timeout toCardRegex, which repeats(?:\d[ -]*?){13,19}over attacker-controlled text.src/Bower.Redaction/Detectors/Crypto/CryptographicMaterialDetector.cs#L53-L56: add a timeout toPemRegex, whose lazy[\s\S]+?spans across 4 alternatives on unbounded input.♻️ Example fix pattern
- [GeneratedRegex( - @"(?:Server|Data Source|Host)=[^;]+;.*(?:Password|Pwd)=[^;]+|jdbc:[a-z0-9]+:\/\/[^\s]+|postgres(?:ql)?:\/\/[^\s]+|mysql:\/\/[^\s]+|mongodb(?:\+srv)?:\/\/[^\s]+|Driver=\{[^}]+\}.*Pwd=", - RegexOptions.IgnoreCase | RegexOptions.CultureInvariant)] + [GeneratedRegex( + @"(?:Server|Data Source|Host)=[^;]+;.*(?:Password|Pwd)=[^;]+|jdbc:[a-z0-9]+:\/\/[^\s]+|postgres(?:ql)?:\/\/[^\s]+|mysql:\/\/[^\s]+|mongodb(?:\+srv)?:\/\/[^\s]+|Driver=\{[^}]+\}.*Pwd=", + RegexOptions.IgnoreCase | RegexOptions.CultureInvariant, matchTimeoutMilliseconds: 100)] private static partial Regex DbRegex();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs` around lines 6 - 234, Add a consistent finite match timeout to every affected generated regex: AwsRegex, AzureRegex, EntraRegex, GcpRegex, JwtRegex, OAuthRegex, K8sRegex, DockerRegex, DbRegex, and EnvRegex in src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs (lines 6-234), CardRegex in src/Bower.Redaction/Detectors/Financial/FinancialDetectors.cs (lines 101-102), and PemRegex in src/Bower.Redaction/Detectors/Crypto/CryptographicMaterialDetector.cs (lines 53-56). Use the GeneratedRegexAttribute overload with matchTimeoutMilliseconds, preserving each existing pattern and options.src/Bower.Redaction/Detectors/Australian/AustralianIdentifierDetectors.cs (1)
7-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
TfnDetectorinvoke the overridden validation hook consistently.
RegexDetectorBase.Detect()callsValidate()whenRequiresValidationis true, butTfnDetectorreplaces that default path with its ownDetect(), creating a separate duplicate checksum check and leavingValidate()unreachable. Either keep the base scanning path and return the captured digit span for validation, or remove theValidate()override entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bower.Redaction/Detectors/Australian/AustralianIdentifierDetectors.cs` around lines 7 - 57, Update TfnDetector to use RegexDetectorBase.Detect() so its RequiresValidation path invokes the existing Validate() override, removing the duplicate custom Detect() checksum logic. Ensure the detector still validates the captured digit group rather than the optional context text, using the base class’s supported capture-span behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/privacy/privacy-secret-engine.md`:
- Around line 3-14: Establish a single redaction boundary across both
documentation sites: in docs/privacy/privacy-secret-engine.md lines 3-14, revise
the lifecycle text and diagram to place the Privacy & Secret Engine before typed
parsing; in docs/architecture/overview.md lines 10-13, clarify that typed
parsing creates typed event contracts only after raw JSON redaction.
- Around line 137-140: Update the detector extension steps to require every new
detector to implement ISensitiveDetector; describe IFieldNameDetector only as an
optional additional interface for detectors needing field-name matching.
Preserve the existing registration options through
DetectorCatalog.CreateDefaultValueDetectors() or PrivacyEngine constructor
injection.
In `@src/Bower.Redaction/Detectors/Australian/AustralianIdentifierDetectors.cs`:
- Around line 59-89: Update CrnDetector.Detect so CRN regex matches are not
marked Validated: true, since no checksum validation is performed; set the match
result to the unvalidated state while preserving the existing format matching,
span selection, and early-return behavior.
In `@src/Bower.Redaction/Detectors/RegexDetectorBase.cs`:
- Around line 21-52: The Detect method’s Pattern.Matches(s) call allows
unbounded regex execution on event text. Update the regex matching used by
RegexDetectorBase to enforce a bounded per-detector or shared timeout, or apply
RegexOptions.NonBacktracking where supported, while preserving existing match
validation and DetectionMatch creation behavior.
In `@src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs`:
- Around line 47-57: Resolve the generic JWT overlap between EntraTokenDetector
and JwtDetector without relying on detector array order. Update the detector
matching or PrivacyEngine overlap resolution to apply a stable detector/policy
priority, or make EntraRegex exclusive to Entra-specific tokens while preserving
device_code detection. Ensure identical start and length matches always produce
deterministic actions.
In `@src/Bower.Redaction/Privacy/PrivacyEngine.cs`:
- Around line 122-147: Update the field-name match branch in WalkObject so Allow
and AlertOnly findings continue into ApplyDetectorsToValue, WalkObject, or
WalkArray for nested scanning instead of unconditionally continuing. For other
actions, route the property value through the existing PolicyApplicator
transformation flow used by normal value matches, including string values,
rather than always removing the property, while preserving accurate action
metadata and existing traversal behavior.
- Around line 171-191: Update WalkArray to detect JsonArray elements and
recursively call WalkArray with the nested array, its itemPath, and the existing
findings collection before processing object or scalar values. Add an
adversarial test covering sensitive data inside nested arrays, including nested
objects where applicable, and verify all detectors redact the values before
persistence.
In `@src/Bower.Redaction/Privacy/PrivacyPolicy.cs`:
- Line 80: Update the default policy mapping for DetectorIds.Tfn to use
PrivacyAction.Mask or Remove instead of PrivacyAction.Sha256, while preserving
HMAC only for configurations with a mandatory managed key. Add coverage
verifying CreateDefault() never produces a TFN SHA-256 digest, and ensure
redaction failures are treated as security failures.
- Around line 14-35: Update the PrivacyPolicy collection properties, including
DetectorActions, DisabledDetectors, OptInDetectors, and EnabledOptInDetectors,
to defensively copy incoming dictionaries/sets into policy-owned immutable or
read-only collections using ordinal comparison. Ensure later mutations to
caller-owned collections cannot affect detector evaluation or actions, and add
an adversarial test that mutates each source collection after policy/engine
construction and verifies scan behavior remains unchanged.
---
Nitpick comments:
In `@src/Bower.Redaction/Detectors/Australian/AustralianIdentifierDetectors.cs`:
- Around line 7-57: Update TfnDetector to use RegexDetectorBase.Detect() so its
RequiresValidation path invokes the existing Validate() override, removing the
duplicate custom Detect() checksum logic. Ensure the detector still validates
the captured digit group rather than the optional context text, using the base
class’s supported capture-span behavior.
In `@src/Bower.Redaction/Detectors/RegexDetectorBase.cs`:
- Around line 29-30: Update the detector loop in RegexDetectorBase.Detect to
call Pattern.EnumerateMatches directly with the existing ReadOnlySpan<char>
instead of materializing text.ToString(). Replace Match usage with the returned
ValueMatch type and preserve the current index and length handling through
ValueMatch.Index and ValueMatch.Length.
In `@src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs`:
- Around line 6-234: Add a consistent finite match timeout to every affected
generated regex: AwsRegex, AzureRegex, EntraRegex, GcpRegex, JwtRegex,
OAuthRegex, K8sRegex, DockerRegex, DbRegex, and EnvRegex in
src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs (lines 6-234),
CardRegex in src/Bower.Redaction/Detectors/Financial/FinancialDetectors.cs
(lines 101-102), and PemRegex in
src/Bower.Redaction/Detectors/Crypto/CryptographicMaterialDetector.cs (lines
53-56). Use the GeneratedRegexAttribute overload with matchTimeoutMilliseconds,
preserving each existing pattern and options.
In `@src/Bower.Redaction/SensitiveDataDetector.cs`:
- Around line 26-39: Update the XML documentation for
SensitiveScanResult.ScanAndRedact to note that the maskInPlace parameter is
retained for backward compatibility but ignored, since redaction is always
policy-driven. Keep the existing _ = maskInPlace; behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83c179ad-5835-4d55-9cf2-eea199858640
📒 Files selected for processing (32)
AGENTS.mdBower.slndocs/architecture/overview.mddocs/privacy/privacy-secret-engine.mdsrc/Bower.Collector/Program.cssrc/Bower.Contracts/SecurityEventEnvelope.cssrc/Bower.Redaction/AGENTS.mdsrc/Bower.Redaction/Detectors/Australian/AustralianIdentifierDetectors.cssrc/Bower.Redaction/Detectors/Classification/SecurityMarkingDetector.cssrc/Bower.Redaction/Detectors/Crypto/CryptographicMaterialDetector.cssrc/Bower.Redaction/Detectors/FieldNameSecretDetector.cssrc/Bower.Redaction/Detectors/Financial/FinancialDetectors.cssrc/Bower.Redaction/Detectors/Identity/IdentityDetectors.cssrc/Bower.Redaction/Detectors/RegexDetectorBase.cssrc/Bower.Redaction/Detectors/Secrets/SecretDetectors.cssrc/Bower.Redaction/JsonEventRedactor.cssrc/Bower.Redaction/Privacy/DetectionMatch.cssrc/Bower.Redaction/Privacy/DetectorCatalog.cssrc/Bower.Redaction/Privacy/DetectorIds.cssrc/Bower.Redaction/Privacy/ISensitiveDetector.cssrc/Bower.Redaction/Privacy/PolicyApplicator.cssrc/Bower.Redaction/Privacy/PrivacyAction.cssrc/Bower.Redaction/Privacy/PrivacyEngine.cssrc/Bower.Redaction/Privacy/PrivacyMetadata.cssrc/Bower.Redaction/Privacy/PrivacyPolicy.cssrc/Bower.Redaction/SensitiveDataDetector.cssrc/Bower.Redaction/Validation/ChecksumAlgorithms.cstests/Bower.Benchmarks/Bower.Benchmarks.csprojtests/Bower.Benchmarks/Program.cstests/Bower.Benchmarks/packages.lock.jsontests/Bower.UnitTests/PrivacyEngineTests.cstests/Bower.UnitTests/SensitiveDataDetectorTests.cs
| Bower's privacy engine inspects every event **after parse and before | ||
| persistence / normalisation**. It is a core control for preventing accidental | ||
| leakage of regulated Australian identifiers, credentials and cryptographic | ||
| material into downstream SIEM platforms. | ||
|
|
||
| ``` | ||
| Raw Event → Parser → Privacy & Secret Engine → Normalisation → Output | ||
| ├── Detect | ||
| ├── Validate | ||
| ├── Classify | ||
| └── Apply Policy | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Define one redaction boundary across the architecture docs.
The privacy guide places the engine after parsing, while the architecture guide places it before typed parsing. This ambiguity can lead integrations to parse sensitive raw data before the control runs.
docs/privacy/privacy-secret-engine.md#L3-L14: change the lifecycle text and diagram to place redaction before typed parsing.docs/architecture/overview.md#L10-L13: clarify that “typed parsing” means creation of typed event contracts after raw JSON redaction.
Based on learnings, behavior changes require documentation updates.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
docs/privacy/privacy-secret-engine.md#L3-L14(this comment)docs/architecture/overview.md#L10-L13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/privacy/privacy-secret-engine.md` around lines 3 - 14, Establish a
single redaction boundary across both documentation sites: in
docs/privacy/privacy-secret-engine.md lines 3-14, revise the lifecycle text and
diagram to place the Privacy & Secret Engine before typed parsing; in
docs/architecture/overview.md lines 10-13, clarify that typed parsing creates
typed event contracts only after raw JSON redaction.
Source: Learnings
| 1. Implement `ISensitiveDetector` (value scan) and/or `IFieldNameDetector`. | ||
| 2. Pass detectors into `PrivacyEngine` constructor **or** extend | ||
| `DetectorCatalog.CreateDefaultValueDetectors()`. | ||
| 3. Set per-id actions / disable flags on `PrivacyPolicy`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Require ISensitiveDetector for every detector extension.
and/or IFieldNameDetector permits a field-name-only implementation. Require ISensitiveDetector, with IFieldNameDetector as an additional interface where needed.
Proposed documentation fix
-1. Implement `ISensitiveDetector` (value scan) and/or `IFieldNameDetector`.
+1. Implement `ISensitiveDetector`; additionally implement `IFieldNameDetector`
+ when the detector needs field-name matching.As per coding guidelines, “New detectors must implement ISensitiveDetector and register through DetectorCatalog or constructor injection.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. Implement `ISensitiveDetector` (value scan) and/or `IFieldNameDetector`. | |
| 2. Pass detectors into `PrivacyEngine` constructor **or** extend | |
| `DetectorCatalog.CreateDefaultValueDetectors()`. | |
| 3. Set per-id actions / disable flags on `PrivacyPolicy`. | |
| 1. Implement `ISensitiveDetector`; additionally implement `IFieldNameDetector` | |
| when the detector needs field-name matching. | |
| 2. Pass detectors into `PrivacyEngine` constructor **or** extend | |
| `DetectorCatalog.CreateDefaultValueDetectors()`. | |
| 3. Set per-id actions / disable flags on `PrivacyPolicy`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/privacy/privacy-secret-engine.md` around lines 137 - 140, Update the
detector extension steps to require every new detector to implement
ISensitiveDetector; describe IFieldNameDetector only as an optional additional
interface for detectors needing field-name matching. Preserve the existing
registration options through DetectorCatalog.CreateDefaultValueDetectors() or
PrivacyEngine constructor injection.
Source: Coding guidelines
| public sealed partial class CrnDetector : RegexDetectorBase | ||
| { | ||
| public override string Id => DetectorIds.Crn; | ||
| public override string Category => DetectorCategories.Australian; | ||
| protected override Regex Pattern => CrnRegex(); | ||
|
|
||
| [GeneratedRegex( | ||
| @"\b(?:CRN|Customer\s*Reference\s*Number|Centrelink)?[:\s#-]*(\d{3}[\s-]?\d{3}[\s-]?\d{3}[A-Za-z])\b", | ||
| RegexOptions.IgnoreCase | RegexOptions.CultureInvariant)] | ||
| private static partial Regex CrnRegex(); | ||
|
|
||
| public override void Detect(ReadOnlySpan<char> text, string path, ICollection<DetectionMatch> matches) | ||
| { | ||
| if (text.IsEmpty) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string s = text.ToString(); | ||
| foreach (Match match in Pattern.Matches(s)) | ||
| { | ||
| Group g = match.Groups.Count > 1 ? match.Groups[1] : match.Groups[0]; | ||
| if (!g.Success) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| matches.Add(new DetectionMatch(Id, Category, g.Index, g.Length, Validated: true)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CrnDetector marks matches Validated: true with no checksum check.
Unlike TfnDetector, MedicareDetector, AbnDetector, AcnDetector, and IhiDetector in this same file — which all gate Validated: true behind a ChecksumAlgorithms.IsValidXxx(...) call — CrnDetector.Detect() unconditionally sets Validated: true (line 86) purely from a regex format match. The CRN's trailing letter is a checksum, but per Services Australia/Centrelink documentation it's generated by an internal, undocumented algorithm, so it's plausible no IsValidCrn exists — meaning this "validated" match is actually just a format guess. Downstream consumers of DetectionMatch.Validated (confidence scoring, policy decisions) will be misled into treating an unverified heuristic match as checksum-confirmed.
🐛 Proposed fix
- matches.Add(new DetectionMatch(Id, Category, g.Index, g.Length, Validated: true));
+ matches.Add(new DetectionMatch(Id, Category, g.Index, g.Length, Validated: false));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public sealed partial class CrnDetector : RegexDetectorBase | |
| { | |
| public override string Id => DetectorIds.Crn; | |
| public override string Category => DetectorCategories.Australian; | |
| protected override Regex Pattern => CrnRegex(); | |
| [GeneratedRegex( | |
| @"\b(?:CRN|Customer\s*Reference\s*Number|Centrelink)?[:\s#-]*(\d{3}[\s-]?\d{3}[\s-]?\d{3}[A-Za-z])\b", | |
| RegexOptions.IgnoreCase | RegexOptions.CultureInvariant)] | |
| private static partial Regex CrnRegex(); | |
| public override void Detect(ReadOnlySpan<char> text, string path, ICollection<DetectionMatch> matches) | |
| { | |
| if (text.IsEmpty) | |
| { | |
| return; | |
| } | |
| string s = text.ToString(); | |
| foreach (Match match in Pattern.Matches(s)) | |
| { | |
| Group g = match.Groups.Count > 1 ? match.Groups[1] : match.Groups[0]; | |
| if (!g.Success) | |
| { | |
| continue; | |
| } | |
| matches.Add(new DetectionMatch(Id, Category, g.Index, g.Length, Validated: true)); | |
| } | |
| } | |
| } | |
| public sealed partial class CrnDetector : RegexDetectorBase | |
| { | |
| public override string Id => DetectorIds.Crn; | |
| public override string Category => DetectorCategories.Australian; | |
| protected override Regex Pattern => CrnRegex(); | |
| [GeneratedRegex( | |
| @"\b(?:CRN|Customer\s*Reference\s*Number|Centrelink)?[:\s#-]*(\d{3}[\s-]?\d{3}[\s-]?\d{3}[A-Za-z])\b", | |
| RegexOptions.IgnoreCase | RegexOptions.CultureInvariant)] | |
| private static partial Regex CrnRegex(); | |
| public override void Detect(ReadOnlySpan<char> text, string path, ICollection<DetectionMatch> matches) | |
| { | |
| if (text.IsEmpty) | |
| { | |
| return; | |
| } | |
| string s = text.ToString(); | |
| foreach (Match match in Pattern.Matches(s)) | |
| { | |
| Group g = match.Groups.Count > 1 ? match.Groups[1] : match.Groups[0]; | |
| if (!g.Success) | |
| { | |
| continue; | |
| } | |
| matches.Add(new DetectionMatch(Id, Category, g.Index, g.Length, Validated: false)); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Detectors/Australian/AustralianIdentifierDetectors.cs`
around lines 59 - 89, Update CrnDetector.Detect so CRN regex matches are not
marked Validated: true, since no checksum validation is performed; set the match
result to the unvalidated state while preserving the existing format matching,
span selection, and early-return behavior.
| public virtual void Detect(ReadOnlySpan<char> text, string path, ICollection<DetectionMatch> matches) | ||
| { | ||
| // Regex operates on string; single allocation only when content present. | ||
| if (text.IsEmpty) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string s = text.ToString(); | ||
| foreach (Match match in Pattern.Matches(s)) | ||
| { | ||
| if (!match.Success) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| ReadOnlySpan<char> span = s.AsSpan(match.Index, match.Length); | ||
| bool validated = !RequiresValidation || Validate(span); | ||
| if (RequiresValidation && !validated) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| matches.Add(new DetectionMatch( | ||
| Id, | ||
| Category, | ||
| match.Index, | ||
| match.Length, | ||
| validated, | ||
| SubKind(span))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)(Program\.cs|PolicyApplicator\.cs|RegexDetectorBase\.cs|.*Detector.*\.cs|.*Redaction.*\.cs|).*\.(cs|csproj|json|yaml|yml)$' | sed -n '1,200p'
echo
echo "== RegexDetectorBase.cs =="
if [ -f src/Bower.Redaction/Detectors/RegexDetectorBase.cs ]; then
cat -n src/Bower.Redaction/Detectors/RegexDetectorBase.cs
fi
echo
echo "== related base/detector constructors and Pattern declaration =="
rg -n "class RegexDetectorBase|Record|Record<|protected .*Pattern|Regex\(|Pattern\." src/Bower.Redaction/Detectors -g '*.cs' || true
echo
echo "== Program.cs relevant HTTP endpoint snippets =="
for f in $(git ls-files | rg 'Program\.cs$'); do
echo "--- $f"
cat -n "$f" | sed -n '1,240p'
done
echo
echo "== payload cap references =="
rg -n "1048576|1 MB|content-length|Length|Max|events|v1/events|Json" src -g '*.cs' | sed -n '1,240p'Repository: jusso-dev/Bower
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files =="
git ls-files | sed -n '1,220p'
echo
echo "== RegexDetectorBase if present =="
if git ls-files | grep -qx 'src/Bower.Redaction/Detectors/RegexDetectorBase.cs'; then
wc -l src/Bower.Redaction/Detectors/RegexDetectorBase.cs
cat -n src/Bower.Redaction/Detectors/RegexDetectorBase.cs
fiRepository: jusso-dev/Bower
Length of output: 10986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== global.json target SDK =="
cat -n global.json | sed -n '1,120p'
echo
echo "== Bower.Redaction project SDK packages =="
cat -n src/Bower.Redaction/Bower.Redaction.csproj | sed -n '1,160p'
echo
echo "== JsonEventRedactor payload cap implementation =="
if [ -f src/Bower.Redaction/JsonEventRedactor.cs ]; then
ast-grep outline src/Bower.Redaction/JsonEventRedactor.cs --view expanded
cat -n src/Bower.Redaction/JsonEventRedactor.cs | sed -n '1,260p'
fi
echo
echo "== Redaction scanning entry paths =="
rg -n "JsonEventRedactor|MaximumPayloadBytes|SetJsonOptions|Detect\\(|Redact|DetectAllMatch|PrivacyEngine" src/Bower.Redaction tests/Bower.UnitTests -g '*.cs' | sed -n '1,240p'Repository: jusso-dev/Bower
Length of output: 20623
🌐 Web query:
.NET Regex Matching Timeout Regular expression matching times out Regex.IsMatch overload matchTimeout documentation
💡 Result:
In.NET, the matchTimeout parameter in Regex methods is a critical tool used to prevent "catastrophic backtracking," a performance issue where complex regular expressions take an excessive amount of time to process specific input strings [1][2]. When a matching operation exceeds the specified timeout interval, the engine throws a RegexMatchTimeoutException [1][3]. Recommended Overload The recommended static method for performing a pattern match with a timeout is Regex.IsMatch(String, String, RegexOptions, TimeSpan) [1][4]. This overload allows you to explicitly define a TimeSpan limit for the operation [1][5]. How Timeouts Work 1. Implementation: The matchTimeout parameter overrides any default timeout values defined for the application domain [1][6]. 2. Default Behavior: If no timeout is explicitly set, the default is Regex.InfiniteMatchTimeout, meaning the operation will not time out [5][7]. 3. Global Setting: You can set a default timeout for all Regex operations within an application domain by using AppDomain.SetData with the key "REGEX_DEFAULT_MATCH_TIMEOUT" and providing a TimeSpan value [5][7]. 4. Mechanics: The regex engine performs periodic checks to see if the timeout interval has elapsed. If it has, it throws a RegexMatchTimeoutException during the next check [5][7]. This ensures the engine does not spend more than a predictable amount of time (often O(n) relative to input length) on any single operation [2]. Best Practices * Use Timeouts: Always set a timeout when processing untrusted input to mitigate potential Regex Denial-of-Service (ReDoS) attacks [8][2]. * Handle Exceptions: If a RegexMatchTimeoutException occurs, it often indicates either excessive backtracking or a timeout value that is too low for the current system load [3][9]. The engine maintains its state after the exception, so you may choose to retry or abandon the operation [3][9]. * Non-Backtracking: In modern.NET (especially.NET 7 and later), consider using the RegexOptions.NonBacktracking flag, which uses an automata-based engine that guarantees linear-time performance and avoids backtracking issues entirely [8][2]. For further details, refer to the official documentation on the Regex.IsMatch method and Best Practices for Regular Expressions [1][8].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.ismatch?view=net-10.0
- 2: https://devblogs.microsoft.com/dotnet/regular-expression-improvements-in-dotnet-7/
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regexmatchtimeoutexception?view=net-10.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.ismatch?view=net-9.0
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.matchtimeout?view=net-10.0
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.ismatch?view=netframework-4.8.1
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.matchtimeout?view=net-9.0
- 8: https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-regex
- 9: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regexmatchtimeoutexception?view=net-9.0
Avoid unbounded regex execution on event text.
Pattern.Matches(s) runs with no matchTimeout, and event payload text is scanned before redaction in /v1/events. A crafted backtrack-heavy value can hang a request-processing thread even under the 1 MB payload cap; add per-detector or shared bounded timeouts, e.g. matchTimeoutMilliseconds, EnumerateMatches(..., timeout), or RegexOptions.NonBacktracking where supported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Detectors/RegexDetectorBase.cs` around lines 21 - 52, The
Detect method’s Pattern.Matches(s) call allows unbounded regex execution on
event text. Update the regex matching used by RegexDetectorBase to enforce a
bounded per-detector or shared timeout, or apply RegexOptions.NonBacktracking
where supported, while preserving existing match validation and DetectionMatch
creation behavior.
Source: Path instructions
| public sealed partial class EntraTokenDetector : RegexDetectorBase | ||
| { | ||
| public override string Id => DetectorIds.Entra; | ||
| public override string Category => DetectorCategories.Secrets; | ||
| protected override Regex Pattern => EntraRegex(); | ||
|
|
||
| [GeneratedRegex( | ||
| @"\b(?:eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b|device_code\s*[:=]\s*[A-Za-z0-9\-_]{10,}", | ||
| RegexOptions.IgnoreCase | RegexOptions.CultureInvariant)] | ||
| private static partial Regex EntraRegex(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how PrivacyEngine resolves overlapping matches from different detector IDs
# at the same span, and whether outcome is deterministic regardless of detector order.
rg -n "Overlap|Resolve|OrderBy" src/Bower.Redaction/Privacy/PrivacyEngine.csRepository: jusso-dev/Bower
Length of output: 665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SecretDetectors relevant regions =="
sed -n '1,130p' src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs | cat -n
echo
echo "== PrivacyEngine overlap resolution region =="
sed -n '180,260p' src/Bower.Redaction/Privacy/PrivacyEngine.cs | cat -n
echo
echo "== Deterministic match-selection probe from source snippets =="
python3 - <<'PY'
import re
from pathlib import Path
secret = Path('src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs').read_text()
ent = re.search(r'EntraRegex\(\)\)\s*\n\s*private static partial Regex EntraRegex\(\)', secret).group(0)
jwt = re.search(r'JwtRegex\(\)\)\s*\n\s*private static partial Regex JwtRegex\(\)', secret).group(0)
def extract(s):
m=re.search(r'GeneratedRegex\(\s*@?"([^"\n]+)"', s|'.')
return re.sub(r'\s*\\n[^\n,]+', '', m.group(1)) if m else None
print('Entra pattern:', extract(ent))
print('Jwt pattern:', extract(jwt))
# Simple static regex containment check: a token matching A+B should match both if each pattern contains the other.
patterns = {
'EntraTokenDetector': extract(ent),
'JwtDetector': extract(jwt)
}
for name_a, pat_a in patterns.items():
for name_b, pat_b in patterns.items():
if name_a==name_b: continue
# If the token payload of pattern B is captured by A's regex, same span occurs.
try:
rgx=re.compile(pat_a, re.IGNORECASE|re.CULTUREINVARIANT|re.ASCII)
# Extract JWT token string from B pattern if it appears as fixed substring or simple class concat.
print(f'Contains {name_b} literal string in {name_a}: {pat_b in pat_a}')
except Exception as e:
print(f'Compiled check failed for {name_a}: {type(e).__name__}: {e}')
PYRepository: jusso-dev/Bower
Length of output: 9006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PrivacyEngine overlap selection tie-break and action mapping =="
sed -n '200,260p' src/Bower.Redaction/Privacy/PrivacyEngine.cs | cat -n
echo
echo "== detector order construction =="
rg -n "new \[\]|Add\\(|FindAll|GetAll|IEnumerable<ISensitiveDetector>|detectors\\s*:" src/Bower.Redaction src -g '*.cs' | head -80
echo
echo "== pattern overlap simulation =="
python3 - <<'PY'
import re
patterns = {
'EntraTokenDetector': r'\b(?:eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b|device_code\s*[:=]\s*[A-Za-z0-9\-_]{10,}',
'JwtDetector': r'\beyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\b'
}
samples = [
'eyJpayload.header.sig',
'eyJeyJeyJ.header.sig',
'x device_code abcabcabc1234567890'
]
for s in samples:
print(s)
for name, pat in patterns.items():
m = re.search(pat, s, flags=re.I|re.CIL|re.CULTUREINVARIANT)
print(' ', name, s[m.start():m.end()] if m else 'no match')
# Same-length token ordering examples
token = 'eyJeyJeyJ.eyJeyJeyJ.eyJeyJeyJ'
print('same_len_token', re.search(r'\beyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\b', token).start(),
re.search(r'\b(?:eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b|device_code\s*[:=]\s*[A-Za-z0-9\-_]{10,}', token).start())
PYRepository: jusso-dev/Bower
Length of output: 12644
Resolve the JWT overlap deterministically
EntraTokenDetector and JwtDetector both match generic eyJ...\.eyJ...\.<signature> tokens, and PrivacyEngine only breaks same-length same-start overlaps by detector array order. Use a stable tie-breaker such as detector/policy priority, or make one detector’s pattern exclusive to avoid order-dependent actions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Detectors/Secrets/SecretDetectors.cs` around lines 47 -
57, Resolve the generic JWT overlap between EntraTokenDetector and JwtDetector
without relying on detector array order. Update the detector matching or
PrivacyEngine overlap resolution to apply a stable detector/policy priority, or
make EntraRegex exclusive to Entra-specific tokens while preserving device_code
detection. Ensure identical start and length matches always produce
deterministic actions.
Source: Coding guidelines
| private void WalkObject(JsonObject value, string parentPath, List<AppliedFinding> findings) | ||
| { | ||
| foreach ((string propertyName, JsonNode? child) in value.ToArray()) | ||
| { | ||
| string path = $"{parentPath}.{propertyName}"; | ||
| string normalized = NormalizeFieldName(propertyName); | ||
|
|
||
| if (policy.IsDetectorEnabled(fieldNameDetector.Id) && | ||
| fieldNameDetector.MatchesFieldName(normalized)) | ||
| { | ||
| PrivacyAction action = policy.ResolveAction(fieldNameDetector.Id); | ||
| findings.Add(new AppliedFinding( | ||
| fieldNameDetector.Id, | ||
| fieldNameDetector.Category, | ||
| path, | ||
| action, | ||
| Validated: true, | ||
| SubKind: null)); | ||
|
|
||
| if (action is not (PrivacyAction.Allow or PrivacyAction.AlertOnly)) | ||
| { | ||
| value.Remove(propertyName); | ||
| } | ||
|
|
||
| continue; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Field-name-secret match unconditionally skips value scanning, and collapses every non-allow action to full removal.
Two related problems in this block:
continueat Line 146 fires regardless of the resolvedaction. WhenactionisAlloworAlertOnly(field-name match kept, e.g. an admin override for a name-matched-but-benign field), the code never callsApplyDetectorsToValue/WalkObject/WalkArrayon that field's value. Any real credit card, TFN, JWT, etc. nested inside an allow-listed field name is never independently scanned by the other detectors, even though those detectors are otherwise supposed to be "independently enableable" per policy.- When
actionis anything other thanAllow/AlertOnly(Replace, Mask, Sha256, Hmac, Encrypt), Line 143 always fully deletes the property instead of routing throughPolicyApplicator(as normal value matches do at Line 248). The metadata built from this finding (viaPolicyApplicator.ActionLabel) will report e.g."Masked"or"SHA256"even though the actual effect was a full removal — the privacy metadata no longer accurately reflects what happened to the data.
At minimum, for string-valued fields, the applicator's actual transformation should be applied instead of an unconditional remove; and for Allow/AlertOnly, the value should still be recursed into so other detectors can flag/act on nested sensitive content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Privacy/PrivacyEngine.cs` around lines 122 - 147, Update
the field-name match branch in WalkObject so Allow and AlertOnly findings
continue into ApplyDetectorsToValue, WalkObject, or WalkArray for nested
scanning instead of unconditionally continuing. For other actions, route the
property value through the existing PolicyApplicator transformation flow used by
normal value matches, including string values, rather than always removing the
property, while preserving accurate action metadata and existing traversal
behavior.
Source: Path instructions
| private void WalkArray(JsonArray array, string path, List<AppliedFinding> findings) | ||
| { | ||
| for (int index = 0; index < array.Count; index++) | ||
| { | ||
| string itemPath = $"{path}[{index}]"; | ||
| if (array[index] is JsonObject nested) | ||
| { | ||
| WalkObject(nested, itemPath, findings); | ||
| } | ||
| else if (array[index] is JsonValue arrayValue && | ||
| arrayValue.TryGetValue(out string? arrayText) && | ||
| arrayText is not null) | ||
| { | ||
| string redacted = ApplyDetectorsToValue(arrayText, itemPath, findings); | ||
| if (redacted != arrayText) | ||
| { | ||
| array[index] = redacted; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Nested JSON arrays are never traversed — sensitive data inside them escapes redaction.
WalkArray handles JsonObject and string JsonValue elements, but has no branch for array[index] is JsonArray. A payload like {"a": [["user@example.com"]]} or {"a": [[{"cardNumber": "4111..."}]]} will silently skip all detectors for the inner array, so credit card numbers, TFNs, JWTs, etc. nested one level deeper than expected are persisted unredacted. Given this engine's stated contract ("Operates after parse / before persistence") and the guideline that redaction failures are security failures, this is a real leak path, not just an edge case.
🔒 Proposed fix: recurse into nested arrays
private void WalkArray(JsonArray array, string path, List<AppliedFinding> findings)
{
for (int index = 0; index < array.Count; index++)
{
string itemPath = $"{path}[{index}]";
if (array[index] is JsonObject nested)
{
WalkObject(nested, itemPath, findings);
}
+ else if (array[index] is JsonArray nestedArray)
+ {
+ WalkArray(nestedArray, itemPath, findings);
+ }
else if (array[index] is JsonValue arrayValue &&
arrayValue.TryGetValue(out string? arrayText) &&
arrayText is not null)
{
string redacted = ApplyDetectorsToValue(arrayText, itemPath, findings);
if (redacted != arrayText)
{
array[index] = redacted;
}
}
}
}Given the path-instruction that queue/redaction changes require crash/recovery or adversarial tests, please add an adversarial test with nested arrays to guard against regressions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void WalkArray(JsonArray array, string path, List<AppliedFinding> findings) | |
| { | |
| for (int index = 0; index < array.Count; index++) | |
| { | |
| string itemPath = $"{path}[{index}]"; | |
| if (array[index] is JsonObject nested) | |
| { | |
| WalkObject(nested, itemPath, findings); | |
| } | |
| else if (array[index] is JsonValue arrayValue && | |
| arrayValue.TryGetValue(out string? arrayText) && | |
| arrayText is not null) | |
| { | |
| string redacted = ApplyDetectorsToValue(arrayText, itemPath, findings); | |
| if (redacted != arrayText) | |
| { | |
| array[index] = redacted; | |
| } | |
| } | |
| } | |
| } | |
| private void WalkArray(JsonArray array, string path, List<AppliedFinding> findings) | |
| { | |
| for (int index = 0; index < array.Count; index++) | |
| { | |
| string itemPath = $"{path}[{index}]"; | |
| if (array[index] is JsonObject nested) | |
| { | |
| WalkObject(nested, itemPath, findings); | |
| } | |
| else if (array[index] is JsonArray nestedArray) | |
| { | |
| WalkArray(nestedArray, itemPath, findings); | |
| } | |
| else if (array[index] is JsonValue arrayValue && | |
| arrayValue.TryGetValue(out string? arrayText) && | |
| arrayText is not null) | |
| { | |
| string redacted = ApplyDetectorsToValue(arrayText, itemPath, findings); | |
| if (redacted != arrayText) | |
| { | |
| array[index] = redacted; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Privacy/PrivacyEngine.cs` around lines 171 - 191, Update
WalkArray to detect JsonArray elements and recursively call WalkArray with the
nested array, its itemPath, and the existing findings collection before
processing object or scalar values. Add an adversarial test covering sensitive
data inside nested arrays, including nested objects where applicable, and verify
all detectors redact the values before persistence.
Source: Path instructions
| public IReadOnlyDictionary<string, PrivacyAction> DetectorActions { get; init; } = | ||
| new Dictionary<string, PrivacyAction>(StringComparer.Ordinal); | ||
|
|
||
| /// <summary>Disabled detector ids (case-sensitive stable ids).</summary> | ||
| public IReadOnlySet<string> DisabledDetectors { get; init; } = | ||
| new HashSet<string>(StringComparer.Ordinal); | ||
|
|
||
| /// <summary>Optional detectors that are off unless explicitly enabled.</summary> | ||
| public IReadOnlySet<string> OptInDetectors { get; init; } = | ||
| new HashSet<string>(StringComparer.Ordinal) | ||
| { | ||
| DetectorIds.IpAddress, | ||
| DetectorIds.Hostname, | ||
| DetectorIds.Username, | ||
| DetectorIds.Address, | ||
| DetectorIds.Gps, | ||
| DetectorIds.SecurityMarking | ||
| }; | ||
|
|
||
| /// <summary>Explicitly enable opt-in detectors.</summary> | ||
| public IReadOnlySet<string> EnabledOptInDetectors { get; init; } = | ||
| new HashSet<string>(StringComparer.Ordinal); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Defensively copy policy collections.
These IReadOnly* properties retain caller-owned mutable dictionaries/sets. Mutating a source collection after engine construction can enable a detector or change a secret action to Allow during later scans. Copy inputs into policy-owned immutable/read-only backing collections, and add an adversarial mutation test.
As per coding guidelines, “Keep evaluation deterministic” and “Redact sensitive data before persistence; treat any redaction failure as a security failure.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Privacy/PrivacyPolicy.cs` around lines 14 - 35, Update
the PrivacyPolicy collection properties, including DetectorActions,
DisabledDetectors, OptInDetectors, and EnabledOptInDetectors, to defensively
copy incoming dictionaries/sets into policy-owned immutable or read-only
collections using ordinal comparison. Ensure later mutations to caller-owned
collections cannot affect detector evaluation or actions, and add an adversarial
test that mutates each source collection after policy/engine construction and
verifies scan behavior remains unchanged.
Source: Coding guidelines
| DetectorActions = new Dictionary<string, PrivacyAction>(StringComparer.Ordinal) | ||
| { | ||
| [DetectorIds.FieldNameSecret] = PrivacyAction.Remove, | ||
| [DetectorIds.Tfn] = PrivacyAction.Sha256, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not SHA-256 TFNs by default.
A TFN has a small, structured input space, so an unkeyed sha256: value persisted in telemetry is practical to enumerate offline. Default to Mask or Remove; use HMAC only when a mandatory managed key is available. Add a test that CreateDefault() never emits a TFN SHA-256 digest.
As per coding guidelines, “Redact sensitive data before persistence; treat any redaction failure as a security failure.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bower.Redaction/Privacy/PrivacyPolicy.cs` at line 80, Update the default
policy mapping for DetectorIds.Tfn to use PrivacyAction.Mask or Remove instead
of PrivacyAction.Sha256, while preserving HMAC only for configurations with a
mandatory managed key. Add coverage verifying CreateDefault() never produces a
TFN SHA-256 digest, and ensure redaction failures are treated as security
failures.
Source: Coding guidelines
1 participant
Summary
Bower.Redaction(PrivacyEngine,ISensitiveDetector, policy actions, metadata).privacymetadata (detected kinds + actions only; never original values).docs/privacy/privacy-secret-engine.md).Test plan
dotnet restore Bower.sln --locked-modedotnet build Bower.sln --configuration Releasedotnet test Bower.sln --configuration Release(88 tests)dotnet format Bower.sln --verify-no-changesdotnet run --project tests/Bower.Benchmarks -c ReleaseSummary by CodeRabbit