test(report-sanitizer): cover sanitizeReportArtifact path-redaction branches#48
Conversation
…ranches Adds test/report-sanitizer.test.js pinning the exported sanitizeReportArtifact gate: longest-path-first ordering (prevents a nested path leaking a suffix), the no-op passthrough (returns the same report when no absolute path is found), input non-mutation, the custom openclawPathPlaceholder option, Windows drive-letter + UNC absolute-path detection, and non-string/relative searchedPaths filtering. No production code changed.
|
Codex review: needs maintainer review before merge. Reviewed July 19, 2026, 4:26 AM ET / 08:26 UTC. Summary Reproducibility: not applicable. This PR adds regression coverage for an existing sanitizer contract rather than reporting a user-facing failure to reproduce. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Land the focused direct tests after a maintainer confirms the asserted sanitizer semantics remain the intended public behavior; retain the existing offline, dependency-free test setup. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds regression coverage for an existing sanitizer contract rather than reporting a user-facing failure to reproduce. Is this the best way to solve the issue? Likely yes: direct tests of the exported sanitizer preserve the existing implementation boundary without adding config, dependencies, or alternate behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f165eafa2a74. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
What Problem This Solves
src/report-sanitizer.jsexportssanitizeReportArtifact(report, options)— the redaction gate that strips absolute OpenClaw target paths out of every emitted report artifact (JSON via--sanitize, Markdown, issues, compatibility reports; re-exported throughsrc/index.js:238,src/report.js:308,src/advanced.js, andsrc/cli.js). It shipped with no direct test. The only existing coverage exercises it indirectly through the report-writing happy path (report.test.js:267,cli.test.js:90), which always uses a single absolute path and the default placeholder — so the module's more subtle, security-relevant branches were unpinned:sensitiveOpenClawPathssorts matches by descending length before substring replacement. This is what stops a shorter path (e.g./home/user/openclaw) from partially matching inside a longer one (/home/user/openclaw/extensions/telegram) and leaving the trailing/extensions/telegramexposed in the output. With only single-path coverage, reversing this sort would leak path suffixes and no test would catch it.openclawPathPlaceholderoption — untested (existing coverage only hits the default<OPENCLAW_PATH>).isAbsolutePath'sC:\…regex and\\server\shareprefix) — untested; on POSIXpath.isAbsolutereturns false for these, so this branch is the only thing that redacts a Windows target path.searchedPathsfiltering — relative or non-string entries must pass through unredacted; untested.A regression in any of these would silently weaken path redaction (a leak) or change the passthrough contract, and would ship green.
Why This Change Was Made
Coverage-only. Adds one new file,
test/report-sanitizer.test.js(+91), pinning the module's currentmainbehavior against these branches. Tests import the real exportedsanitizeReportArtifactfrom../src/report-sanitizer.js(matching the repo's per-module unit-test layout, e.g.json-file.test.js) and drive it directly — no stubs. No production code is touched; no new config, defaults, dependencies, or report field names (per AGENTS.md's "preserve stable report field names").User Impact
No user-visible or runtime change. For maintainers, the path-redaction gate now regresses loudly: a future edit that reverses the longest-first ordering (a path-suffix leak), drops the Windows/UNC detection, breaks the placeholder option, or changes the no-op passthrough will fail this suite instead of shipping silently.
Evidence
Linux, Node 22.22, branched off current
main(2f93500), no install needed (the suite runs the source directly vianode --test).6/6 pass on clean
main:Full suite green with the new file — no regressions:
Non-vacuous — each scenario bites when its target branch is mutated (mutation applied to
src/report-sanitizer.js, suite re-run, then reverted):report-sanitizer.jsleft.length - right.length) — reverses longest-firstsensitivePaths.length === 0early returnoptions.openclawPathPlaceholder)return path.isAbsolute(value)only)Diff: +91, one new test file, no production code.
AI-assisted contribution.
Generated by Claude Code