fix(redaction): mask lowercase SIDs, and let the fixture scanner ask the masker - #562
Merged
Merged
Conversation
…the masker Two defects from #547, plus a third the first fix surfaced. The owner grammar's SID rule was the only pattern in the file without `(?i)`, so `s-1-5-21-...` exported verbatim while the uppercase form masked. Windows emits uppercase, but third-party logs and JSON round-trips do not, and nothing in the code or the docs records the inconsistency as deliberate. The fixture privacy scanner restated the SID shape a third time instead of asking the grammar; it now calls a shared sid_occurrences. That matters more than the duplication suggests. It is the guard that fails the build when a fixture carries an unmasked SID, so a divergence from what the masker matches is a hole in the safety net, and a hole there is invisible by construction. The issue's diagnosis of that scanner was not quite right, so recording what the divergence actually was. Its dash threshold is equivalent to the grammar's sub-authority count, and S-1-5-21-1010 was found by both. Measured, the two real holes were case, and the candidate having to end in a digit: S-1-5-21-1010- passed the scan while the masker masked it. Re-ran the full corpus against the tightened scanner. Nothing newly fails, so no committed fixture was hiding a SID behind either hole. Making the rule case-insensitive then exposed a third defect. The token is hashed from the matched text, so the two spellings of one SID produced two different tokens, defeating the correlation a stable token exists to provide: an analyst following one account across a bundle would see it as two. The hash is taken from the uppercase form. Mutation-checked: dropping (?i) fails three tests, hashing the text as written fails one. Closes #547. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe shared Windows SID matcher now handles case-insensitive identifiers and normalizes matched text before hashing. A public occurrence helper is re-exported and used by the fixture privacy scanner. Tests verify masking and scanning consistency. ChangesSID detection and privacy scanning
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
apps
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes SID redaction consistency in the shared Intune Windows redaction grammar, and tightens the test fixture privacy scanner by reusing the same SID matcher as the masker.
Changes:
- Make the SID regex case-insensitive so lowercase
s-1-...SIDs are masked. - Ensure SID stable tokens are derived from a canonical (uppercase) representation so different casings correlate to the same token.
- Update the fixture privacy scanner to delegate SID detection to the shared grammar (
sid_occurrences) instead of re-implementing SID shape logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/cmtraceopen-parser/tests/support/mod.rs | Fixture privacy scan now uses the shared SID matcher to avoid drift from the masker. |
| crates/cmtraceopen-parser/src/intune/apps/windows/common/redaction.rs | SID masking is now case-insensitive; adds sid_occurrences; canonicalizes SID hashing and adds tests. |
| crates/cmtraceopen-parser/src/intune/apps/windows/common/mod.rs | Re-exports sid_occurrences for reuse. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+364
to
+368
| // Hashed from the uppercase form. A SID is case-insensitive, so the | ||
| // two spellings name one identity and must reach one token; hashing | ||
| // the text as written would hand an analyst two tokens for the same | ||
| // account and break the correlation these tokens exist to provide. | ||
| stable_token("sid", &caps[0].to_ascii_uppercase()) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #547.
Two defects from the issue, plus a third the first fix surfaced.
The owner grammar was case-sensitive for SIDs only
sid_rewas the one pattern inredaction.rswithout(?i)— the profile-path, inline-credential, account-field, device-name and tenant-id rules all have it. Sos-1-5-21-...exported verbatim while the uppercase form masked. Windows emits uppercase, but third-party logs and JSON round-trips do not.The fixture privacy scanner restated the shape instead of asking the grammar
It now calls a shared
sid_occurrences. This matters more than the duplication suggests: it is the guard that fails the build when a fixture carries an unmasked SID, so any divergence from what the masker matches is a hole in the safety net — and a hole there is invisible by construction, because the check whose job is catching a leak is the one place a mismatch goes unnoticed.The issue's diagnosis of this one was not quite right, so recording what the divergence actually was. The dash threshold is equivalent to the grammar's sub-authority count;
S-1-5-21-1010was found by both. Measured against the grammar:S-1-5-21-1010s-1-5-21-...S-1-5-21-1010-userS-1-5-21-1010Re-ran the full corpus against the tightened scanner. Nothing newly fails, so no committed fixture was hiding a SID behind either hole — which was the check the issue asked for.
A third defect the first fix exposed
Making the rule case-insensitive made the lowercase form mask — to a different token. The token is hashed from the matched text, so two spellings of one SID produced two tokens. That defeats the correlation a stable token exists to provide: an analyst following one account across a bundle would see it as two identities. The hash is now taken from the uppercase form.
Verification
Mutation-checked rather than assumed:
(?i)→ 3 tests failParser suite, clippy
-D warnings, wasm32, and thesrc-taurisuite all clean.Note on scope
Three files, no formatting churn.
cargo fmt --allreflows ~20 unrelated files in this repo because no CI job enforces formatting, so only the edited lines are here.Next in the cluster: #549 (ESP export never calls its own projection) and #556 (DsRegCmd has no projection at all), both of which want the ADR-004 boundary ruling in #550 first.
Summary by CodeRabbit