fix(approval): redact non-canonically-cased home paths in scrub_paths#5042
fix(approval): redact non-canonically-cased home paths in scrub_paths#5042mysma-9403 wants to merge 2 commits into
Conversation
scrub_paths guards its redaction walk with a fast-path bailout that returned verbatim when the input contained neither "Users" nor "home". That check was case-sensitive while the matcher it guards (match_home_prefix) is case-insensitive (eq_ignore_ascii_case), so paths like c:\users\alice\work or /HOME/alice/work short-circuited unredacted, leaking the OS username into the durable pending_approvals audit row and the ApprovalRequested event — a surface the module docstring promises is username-free. cwd/path/dir/command/file are not in SENSITIVE_KEYS, so scrub_paths is their only redaction boundary, and the existing tests only covered canonical C:\Users\ / /Users/ / /home/ casings. Lower-case the input once and test the guard against it, so the fast-path is exactly as permissive as match_home_prefix (which only matches strings holding 'users'/'home' case-insensitively). Canonical casings redact as before; unrelated strings still return verbatim. Tests: lowercase_windows / uppercase_linux / mixed_case home paths now redact to <HOME>; a non-path string still returns unchanged through the guard.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRedaction casing coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/openhuman/approval/redact.rs`:
- Around line 134-135: Replace the unconditional to_ascii_lowercase allocation
in the approval redaction check with a zero-allocation, case-insensitive
sliding-window scan over input’s bytes, exiting as soon as “users” or “home” is
found. Preserve the existing bailout condition while avoiding a full lowercase
copy for large strings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fd49f3e-42e0-4e01-acef-14ec34bbc0fd
📒 Files selected for processing (1)
src/openhuman/approval/redact.rs
… a lowercase copy The case-insensitive fast-path guard allocated a full lowercase copy of the input via to_ascii_lowercase() on every call. Tool arguments can be large (source or file contents), so that is an O(N) allocation on every string field processed, even for the common no-path case that bails immediately. Replace it with a zero-allocation sliding-window scan over the bytes (windows(n).any(|w| w.eq_ignore_ascii_case(..))). windows(n) yields nothing when the input is shorter than the needle, so short strings bail without a panic. Case-insensitivity — and the redaction behaviour the tests lock in — is unchanged. Per CodeRabbit review on tinyhumansai#5042.
Summary
scrub_pathsreturned non-canonically-cased home paths (e.g.c:\users\alice\…,/HOME/alice/…) verbatim, leaking the OS username into the durable approval audit surface.match_home_prefixmatcher it guards.Problem
scrub_pathsredacts absolute home paths to<HOME>before an approval summary is persisted into the durablepending_approvalstable and broadcast on the event bus (DomainEvent::ApprovalRequested). The module docstring promises this output cannot leak the user's username on multi-tenant log shipping.It opens with a fast-path bailout:
This guard is case-sensitive, but the matcher it guards (
match_home_prefix) is deliberately case-insensitive (eq_ignore_ascii_case). So a path likec:\users\alice\work(lowercaseusers) or/HOME/alice/work(uppercase) contains neither"Users"nor"home"literally, hits the early return, and is emitted unredacted — leaking the username into a surface the module guarantees is username-free. Path-bearing keys (cwd/path/dir/command/file) are not inSENSITIVE_KEYS, soscrub_pathsis their only redaction boundary. The existing tests only cover canonicalC:\Users\//Users///home/, so the gap was untested.Solution
Lower-case the input once and test the guard against it, so the fast-path can only skip strings the matcher genuinely could not redact:
match_home_prefixonly ever matches strings containingusersorhome(case-insensitively), so this guard is now exactly as permissive as the matcher — no false bailout, and unrelated strings still return verbatim (one allocation on the common no-path path). Canonical casings that already redacted are unaffected.Submission Checklist
lowercase_windows_home_path_is_scrubbed,uppercase_linux_home_path_is_scrubbed,mixed_case_home_path_is_scrubbed(the previously-leaking cases), andnon_path_string_without_home_marker_is_unchanged(guard still bails correctly).cargo test -p openhuman --lib approval::redactpasses.N/A: security bug fix, no user-visible feature change.## Related—N/A.N/A: does not touch a release-cut surface.Closes #NNN— no existing issue; found by inspection.Impact
pending_approvalsaudit row and theApprovalRequestedevent for non-canonically-cased home paths. No behaviour change for canonical paths or non-path strings; no schema/migration.Related
AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/approval-redact-case-insensitive-homea0ea9cb4cValidation Run
pnpm --filter openhuman-app format:check— N/A (no frontend change)pnpm typecheck— N/A (no frontend change)cargo test -p openhuman --lib approval::redactcargo fmtBehavior Changes
<HOME>like their canonical forms.Parity Contract
/Users/,/home/,C:\Users\paths redact exactly as before; non-path strings still return verbatim via the (now case-insensitive) fast-path guard.match_home_prefixexactly.Summary by CodeRabbit