fix(probe): run the diagnose egress leak self-check; ship payload schema, not values (#199)#214
Conversation
…ema, not values (#199) probe-adapter's report is documented as sanitized/shareable but had neither of diagnose's two egress protections: no assert_no_leak backstop over the rendered bytes, and no Pseudonymizer, so identifier-shaped values passed the collection scrubbers verbatim. - promote the guard loop (_guard/_repair_candidates/_assert_clean/ LeakDetected) from diagnostics.py into sanitize.py as public guard/assert_clean/LeakDetected — one audited implementation; diagnostics delegates and re-exports LeakDetected - reduce EventCapture.payload to payload_schema (key-path:type via the existing _walk_paths; no payload value of any kind ships) and identifier-gate payload_keys; --json schema_version 1 -> 2 - guard both probe renderers on their exact emitted bytes (ensure_ascii=False); cmd_probe registers the project dir name as a project-ns alias (collision-skipped vs CLI/binary names) and fails closed per the #195 contract: stderr message, empty stdout, exit != 0, no --out - collection hardening the guard surfaced live: path components embedding the current username are redacted (pytest-of-<user> shipped verbatim before), home-rooted --binary hints render ~-relative, credential-shaped dict keys collapse to <key> in key paths, --keep-temp prints a ~-relative tmpdir Closes #199
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughProbe reports now emit captured payload schemas instead of values, apply alias-aware redaction, and pass final Markdown or JSON through a shared fail-closed egress guard. CLI output handling, diagnostics integration, documentation, changelog entries, and security tests are updated accordingly. ChangesProbe egress safety and schema hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant probe
participant sanitize.guard
participant Output
CLI->>probe: run scan or probe with pseudonymizer
probe-->>CLI: schema-based finding
CLI->>sanitize.guard: validate rendered report
sanitize.guard-->>CLI: clean output or LeakDetected
CLI->>Output: emit validated bytes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
`_redact_location` filtered only "/" out of `path.parts`, but on Windows a rooted path's anchor is "\\". It survived the filter, reached the identifier gate, and was struck as if it were a component -- prepending a phantom "<redacted>" to every non-home absolute path in a probe report. The root separator is structure, not a component, so it is dropped on both flavours. A drive or UNC anchor does carry content, so it still goes through the gate. Pinned by a test driven through PureWindowsPath, so the Linux legs catch this class of bug instead of only the Windows ones.
Closes #199.
probe-adapter's report is documented as sanitized and shareable (the adapter-authoring guide's whole point is pasting it into an issue/PR), but it had neither of the two egress protectionsdiagnoserelies on for the same expectation: noassert_no_leakbackstop over the rendered bytes, and noPseudonymizer— identifier-shaped values passed the collection scrubbers verbatim by design. probe.py's own docstring tracked the gap and cited #199.What changed
One audited guard. The guard loop (
_guard/_repair_candidates/_assert_clean/LeakDetected) moved fromdiagnostics.pyintosanitize.pyas publicguard/assert_clean/LeakDetected. Diagnostics delegates and re-exportsLeakDetected(noqa-pinned; an identity test catches the ruff-F401-autofix regression). The diagnostics canary/zero-repair/encoding suites pass unchanged — the proof the move preserved semantics byte-for-byte.Payloads ship as shape, never values.
EventCapture.payload(a whole scrubbed hook-payload dict — the widest identifier-shaped surface) is replaced bypayload_schema: dotted key paths + leaf types via the existing_walk_paths(tool_input.command:str), which is the part profile authoring actually needs.payload_keysstays but is identifier-gated.--jsonschema_versionbumps 1 → 2 (captured_events[].payloadremoved — a field removal, per the machine.py additive-only contract).Guard on the exact emitted bytes. Both probe renderers run
sanitize.guardbefore returning;render_jsonnow dumpsensure_ascii=False(same safety requirement as diagnose — the guard must see the string as itself, andmachine.emit/write_documentemit it verbatim).cmd_proberegisters the project directory name as aproject-ns alias (skipped if it collides with the CLI/binary name), routes it into location redaction at collection time (zero repairs on the canonical run), and on a hit fails closed per the #195 contract: message → stderr, empty stdout, exit ≠ 0, no partial--outfile. A stray alias original is repaired and disclosed (backstop_repairs+ stderr warning), exactly like diagnose.Collection hardening the guard surfaced immediately. Running the new guard against the existing test suite caught a live leak: transcript-location components that embed the current username shipped verbatim (
pytest-of-<user>in every CI run of the PII tests; any user-named$TMPDIRin production). Path components embedding the username are now redacted; also hardened: home-rooted--binaryhints render~-relative in the summary and the not-found warning, credential-shaped dict keys (ghp_…as a map key is identifier-shaped) collapse to<key>in key paths, and--keep-tempprints a~-relative tmpdir.Verification
trunk check --no-fix --allclean.src/stashed to the pre-fix tree, the 13 new/extended egress tests all fail.--outabsence),_collect_capturesschema-not-values, end-to-end egress through the real capture-hook subprocess seam, location aliasing zero-repair invariant, backstop repair of a stray project name in key paths, non-ASCII guard fidelity, guard mechanics ported totest_sanitize.py.schema_version: 2, zero occurrences of the username or project name, zero repairs;--jsonparses whole.Out of scope
#202 (diagnose
_scrub_policydict-key gap — sibling class, untouched), #212 (documents.py rename),machine.pybehavior, the capture hook script (raw retention inside the transient tmpdir is correct by design),_walk_pathsdepth-guarding (pre-existing).Summary by CodeRabbit
Breaking Changes
captured_events[].payloadentries are replaced bycaptured_events[].payload_schema.Security Enhancements
Documentation