fix: strip developer-identity paths from the cli.Run error envelope (iss-76) - #50
Merged
Conversation
cli.Run routes every command error through the --json envelope (and the stderr text line), so absolute local paths reached machine output three ways: an *os.PathError/*os.LinkError embeds one in Error(); core fmt-formats one via %s (e.g. capture's ledger-path guards); a custom error type renders one (e.g. history's home-rooted StorePathError). A typed walk over the two os error types -- the first cut -- missed the latter two, which a security review reproduced as live leaks on the same surface. scrubPaths now redacts the two identity-bearing roots (working directory -> '.', home -> '~') wherever they appear, then reduces any remaining absolute PathError/LinkError path to its base name (covering a path argument outside both roots). Generalises the per-branch fix iss-29 made for docs-lint config. Detectors watched fail then pass: per-verb --json envelope tables for 'memory ask --page-json' (PathError) and 'capture' on a symlinked issues/open (fmt-embedded), plus a scrubPaths unit table covering PathError/LinkError/ wrapped/joined/relative/non-path and the capture (cwd) and history (home) classes. Scope is the developer-identity portion (cwd/home + PathError/LinkError), not a universal absolute-path scrub -- a verb echoing a user-supplied absolute path outside both roots (e.g. 'memory ingest /tmp/x') carries no identity and is tracked in iss-81, along with the capture success-envelope 'path' field. A regex scrub is deliberately avoided: this surface also carries URLs a path regex would mangle. Security review (HOLD -> resolved via its option 2). Assisted-by: Claude:claude-opus-4-8
iss-76's cli.Run boundary fix lands in this branch. The security re-review surfaced a narrower, non-identity residual (verb-level echo of a user-supplied absolute path outside cwd/home; plus the capture success-envelope path field) -- captured as iss-81 rather than widening iss-76. Assisted-by: Claude:claude-opus-4-8
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
1 participant
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.
What
cli.Runrenders every command error into the--jsonenvelope and the stderrline. Identity-bearing local paths reached that surface three ways — an
os.PathError/os.LinkError, a pathfmt-formatted into a core error, and acustom error type — so a verb error could print the developer's home or repo
path. iss-29 fixed one branch (docs-lint config load); this generalises it.
How
New
scrubPathsat theRun()boundary:→ .) and home (→ ~) roots wherever theyappear — catches
fmt-formatted and custom-error-type paths a typed walkcannot see;
os.PathError/os.LinkErrorpath (e.g. a path argumentoutside both roots) to its base name.
A regex scrub was deliberately avoided: this error surface also carries URLs
(fetch failures) that an absolute-path regex would mangle.
Detectors (watched fail → pass)
TestJSONErrorEnvelopeNoAbsolutePathLeak—memory ask --page-jsonon amissing absolute path (
PathError).TestCaptureSymlinkErrorNoPathLeak— realcapture --jsonagainst a symlinkedissues/open(thefmt-embedded ledger-path guard).TestScrubPaths— unit table: PathError / LinkError / wrapped / joined /relative / non-path, plus the capture (cwd) and history (home) classes.
Review
Correctness review: PROMOTE. Security review: initial HOLD reproduced two live
leaks (capture symlink, history store path) — both now closed and re-verified.
The re-review confirmed the identity invariant holds and raised a narrower,
non-identity residual (a verb echoing a user-supplied absolute path outside
cwd/home; and the capture success-envelope
pathfield). Resolved via thereview's option 2 — scope the claim honestly — and filed the residual as
iss-81 rather than widening this change (which would have overlapped iss-79).
Scope
internal/surface/cli/cli.go+ one test file + CHANGELOG + ledger (resolvesiss-76, files iss-81). Full
make preflightgreen (build, vet, gofmt, tests,-race).Part of drain #2. Chain: branched from
main; independent of iss-79/iss-73.Assisted-by: Claude:claude-opus-4-8