From 459e576acfb631c45a20f59a7ed9ca4f66051e87 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:20:45 +0100 Subject: [PATCH 1/2] fix: strip local-identity paths from the cli.Run error envelope (iss-76) 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 --- CHANGELOG.md | 11 + internal/surface/cli/cli.go | 80 +++++++- .../cli/error_pathleak_surface_test.go | 188 ++++++++++++++++++ 3 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 internal/surface/cli/error_pathleak_surface_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 07f399ac..a7aa3bca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,17 @@ called out in a **Breaking** section. ### Fixed +- **`--json` and stderr command errors no longer leak the developer's home or + working-directory paths.** `cli.Run` routes every command error through the + machine envelope, so identity-bearing paths reached it three ways: an + `os.PathError`/`os.LinkError` (e.g. `memory ask --page-json` on a missing file), + a path `fmt`-formatted into a core error (e.g. `capture` on a symlinked ledger + dir), and a custom error type (e.g. history's home-rooted store path). The + `Run()` boundary now redacts the working-directory and home roots (to `.` and + `~`) and reduces any remaining `PathError`/`LinkError` path to its base name. + Generalises the per-branch fix made in `iss-29` (iss-76). A verb echoing a + user-supplied absolute path outside both roots is out of scope, tracked in + `iss-81`. - The `intent_lifecycle` record-lint rule now **blocks duplicate intent ids**. Id allocators are branch-local — parallel agents on separate branches each scan for `max + 1` and mint the same id — so two intents both claimed diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index c3b6bb48..0d260c3f 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -1564,7 +1564,7 @@ func Run(args []string, stdout, stderr io.Writer) int { if errors.As(err, &coded) { code = coded.ExitCode() } - if msg := err.Error(); msg != "" { + if msg := scrubPaths(err); msg != "" { // Honour --json for the error surface too: a caller that asked for // machine output must get a JSON envelope, never raw Go text (iss-29). if asJSON, _ := root.PersistentFlags().GetBool("json"); asJSON { @@ -1584,6 +1584,84 @@ type errorEnvelope struct { Error string `json:"error"` } +// scrubPaths renders err for machine/stderr output with the DEVELOPER-IDENTITY +// portion of any local path removed. cli.Run routes every command error through +// the --json envelope and the stderr line, and an identity-bearing path reaches +// that surface 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). All three are +// handled (iss-76 — the identity-scrub generalisation of the one branch iss-29 +// fixed): +// +// - the two roots that carry developer identity — the working directory and the +// home directory — are redacted to "." and "~" wherever they appear, catching +// fmt-formatted and custom-error-type paths a typed walk cannot see; +// - any remaining absolute path embedded by os.PathError/os.LinkError (e.g. a +// path argument outside both roots) is reduced to its base name. +// +// This is NOT a universal absolute-path scrub: a verb that echoes a user-supplied +// absolute path lying outside both roots (e.g. `memory ingest /tmp/x`) still +// surfaces it — that path carries no developer identity, and sanitising such +// verb-level echoes is tracked separately (iss-81). Scrubbing here rather than by +// regex is deliberate: this error surface also carries URLs (fetch failures) that +// an absolute-path regex would mangle. +func scrubPaths(err error) string { + msg := err.Error() + if cwd, e := os.Getwd(); e == nil { + msg = redactRoot(msg, cwd, ".") + } + if home, e := os.UserHomeDir(); e == nil { + msg = redactRoot(msg, home, "~") + } + for _, p := range embeddedPaths(err) { + if filepath.IsAbs(p) { + msg = strings.ReplaceAll(msg, p, filepath.Base(p)) + } + } + return msg +} + +// redactRoot replaces every occurrence of the absolute directory root (followed +// by a path separator) in s with repl. The filesystem root ("/") and empty or +// relative roots are skipped so a message is never mangled into meaninglessness. +func redactRoot(s, root, repl string) string { + if len(root) <= 1 || !filepath.IsAbs(root) { + return s + } + sep := string(os.PathSeparator) + return strings.ReplaceAll(s, root+sep, repl+sep) +} + +// embeddedPaths collects the filesystem paths carried by os.PathError/os.LinkError +// anywhere in err's Unwrap chain, including errors.Join fan-out. +func embeddedPaths(err error) []string { + var paths []string + var walk func(error) + walk = func(e error) { + for e != nil { + switch t := e.(type) { + case *os.PathError: + paths = append(paths, t.Path) + case *os.LinkError: + paths = append(paths, t.Old, t.New) + } + switch u := e.(type) { + case interface{ Unwrap() error }: + e = u.Unwrap() + case interface{ Unwrap() []error }: + for _, sub := range u.Unwrap() { + walk(sub) + } + return + default: + return + } + } + } + walk(err) + return paths +} + // render writes v as indented JSON when asJSON is set, otherwise delegates to // the text renderer. Keeping this one helper is what makes every command's // --json behaviour uniform. diff --git a/internal/surface/cli/error_pathleak_surface_test.go b/internal/surface/cli/error_pathleak_surface_test.go new file mode 100644 index 00000000..28fe35f4 --- /dev/null +++ b/internal/surface/cli/error_pathleak_surface_test.go @@ -0,0 +1,188 @@ +package cli + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestJSONErrorEnvelopeNoAbsolutePathLeak is the iss-76 detector: cli.Run routes +// every command error through the --json envelope (and the stderr text line), so +// any verb whose error chain carries an *os.PathError / *os.LinkError would leak +// its absolute local path into machine output. The sanitisation belongs at the +// Run() boundary — iss-29 fixed only the docs-lint config-load branch. This is a +// per-verb table: each row drives a real verb into a filesystem error and asserts +// the envelope carries no absolute path but keeps the file's base name for context. +func TestJSONErrorEnvelopeNoAbsolutePathLeak(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + + // An absolute path guaranteed to fail os.ReadFile with a *PathError. + absMissing := filepath.Join(repo, "no-such-dir", "page.json") + + cases := []struct { + name string + args []string + base string // basename the sanitised message should retain + }{ + { + name: "memory ask --page-json missing file", + args: []string{"memory", "ask", "q", "--file-back", "--page-json", absMissing, "--json"}, + base: "page.json", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := Run(tc.args, &stdout, &stderr) + if code == 0 { + t.Fatalf("expected a non-zero exit; stdout=%q stderr=%q", stdout.String(), stderr.String()) + } + var env struct { + Error string `json:"error"` + } + if err := json.Unmarshal(stderr.Bytes(), &env); err != nil { + t.Fatalf("--json error not JSON-shaped: %v\nstderr: %q", err, stderr.String()) + } + if strings.Contains(env.Error, repo) { + t.Fatalf("envelope leaked the absolute path %q:\n%s", repo, env.Error) + } + if !strings.Contains(env.Error, tc.base) { + t.Fatalf("envelope dropped all file context (want basename %q):\n%s", tc.base, env.Error) + } + }) + } +} + +// TestCaptureSymlinkErrorNoPathLeak reproduces the security-review finding that +// the fix must also cover: capture's allocator embeds an ABSOLUTE ledger path via +// fmt.Errorf("%w … %s") — not an os.PathError — so a typed walk misses it. A +// symlinked issues/open dir trips the guard and its error reaches the --json +// envelope; it must not leak the developer's absolute repo path. +func TestCaptureSymlinkErrorNoPathLeak(t *testing.T) { + repo := t.TempDir() + t.Chdir(repo) + issues := filepath.Join(repo, ".abcd", "work", "issues") + if err := os.MkdirAll(issues, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(t.TempDir(), filepath.Join(issues, "open")); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := Run([]string{"capture", "a defect", "--json"}, &stdout, &stderr) + if code == 0 { + t.Fatalf("expected a non-zero exit for a symlinked issues/open; stdout=%q stderr=%q", stdout.String(), stderr.String()) + } + var env struct { + Error string `json:"error"` + } + if err := json.Unmarshal(stderr.Bytes(), &env); err != nil { + t.Fatalf("--json error not JSON-shaped: %v\nstderr: %q", err, stderr.String()) + } + if strings.Contains(env.Error, repo) { + t.Fatalf("capture envelope leaked the absolute repo path %q:\n%s", repo, env.Error) + } +} + +// homeRootedErr is a custom error type (like history.StorePathError) that renders +// a home-dir-rooted absolute path — the class a typed PathError walk cannot see. +type homeRootedErr struct{ path string } + +func (e homeRootedErr) Error() string { return "history: store unreadable (" + e.path + ")" } + +// TestScrubPaths is the unit-level detector for the Run()-boundary sanitiser. It +// removes local-identity paths reaching machine output three ways — os.PathError/ +// os.LinkError (→ base name), and cwd- or home-rooted paths embedded by fmt or a +// custom error type (→ "." / "~") — while leaving relative and non-path errors +// untouched. +func TestScrubPaths(t *testing.T) { + abs := filepath.Join(os.TempDir(), "secret", "config.json") + rel := filepath.Join("rel", "config.json") + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + home, err := os.UserHomeDir() + if err != nil { + t.Fatal(err) + } + + cases := []struct { + name string + err error + wantAbsent string // must NOT appear in the scrubbed message + wantPresent []string + }{ + { + name: "fmt-embedded cwd path (capture class)", + err: fmt.Errorf("path unsafe: allocator lock path is a symlink: %s", filepath.Join(cwd, ".abcd", "work", "issues", ".iss-alloc.lock")), + wantAbsent: cwd, + wantPresent: []string{".iss-alloc.lock"}, + }, + { + name: "custom-type home path (history class)", + err: homeRootedErr{path: filepath.Join(home, ".abcd", "history", "x")}, + wantAbsent: home, + wantPresent: []string{".abcd", "history: store unreadable"}, + }, + { + name: "bare PathError", + err: &os.PathError{Op: "open", Path: abs, Err: fs.ErrNotExist}, + wantAbsent: abs, + wantPresent: []string{"open", "config.json", fs.ErrNotExist.Error()}, + }, + { + name: "wrapped PathError keeps context", + err: fmt.Errorf("cannot read --page-json: %w", &os.PathError{Op: "open", Path: abs, Err: fs.ErrPermission}), + wantAbsent: abs, + wantPresent: []string{"cannot read --page-json", "config.json"}, + }, + { + name: "LinkError both paths", + err: &os.LinkError{Op: "rename", Old: abs, New: abs + ".tmp", Err: fs.ErrExist}, + wantAbsent: abs, + wantPresent: []string{"rename", "config.json"}, + }, + { + name: "joined errors both scrubbed", + err: errors.Join(&os.PathError{Op: "open", Path: abs, Err: fs.ErrNotExist}, fmt.Errorf("and: %w", &os.PathError{Op: "stat", Path: abs, Err: fs.ErrPermission})), + wantAbsent: abs, + wantPresent: []string{"config.json"}, + }, + { + name: "relative PathError left intact", + err: &os.PathError{Op: "open", Path: rel, Err: fs.ErrNotExist}, + wantAbsent: "\x00-never-", // sentinel: nothing to strip + wantPresent: []string{rel}, + }, + { + name: "non-path error untouched", + err: errors.New("plain failure"), + wantAbsent: "\x00-never-", + wantPresent: []string{"plain failure"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := scrubPaths(tc.err) + if strings.Contains(got, tc.wantAbsent) { + t.Fatalf("scrubPaths kept the absolute path %q: %s", tc.wantAbsent, got) + } + for _, want := range tc.wantPresent { + if !strings.Contains(got, want) { + t.Fatalf("scrubPaths dropped %q: %s", want, got) + } + } + }) + } +} From 62ba1d0ef54f693fa7b06f718c481d1c59d79385 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 22:43:31 +0100 Subject: [PATCH 2/2] chore(ledger): resolve iss-76; file iss-81 (review residual) 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 --- ...s-81-machine-output-abspath-beyond-error-scrub.md | 12 ++++++++++++ .../iss-76-json-error-abspath-leak.md | 1 + 2 files changed, 13 insertions(+) create mode 100644 .abcd/work/issues/open/iss-81-machine-output-abspath-beyond-error-scrub.md rename .abcd/work/issues/{open => resolved}/iss-76-json-error-abspath-leak.md (84%) diff --git a/.abcd/work/issues/open/iss-81-machine-output-abspath-beyond-error-scrub.md b/.abcd/work/issues/open/iss-81-machine-output-abspath-beyond-error-scrub.md new file mode 100644 index 00000000..557cbd1f --- /dev/null +++ b/.abcd/work/issues/open/iss-81-machine-output-abspath-beyond-error-scrub.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-81" +slug: "machine-output-abspath-beyond-error-scrub" +severity: "minor" +category: "bug" +source: "user-observation" +found_during: "2026-07-12 iss-76 security re-review" +found_at: "internal/surface/cli/cli.go" +--- + +Machine output surfaces absolute local paths OUTSIDE the cli.Run error-identity scrub added in iss-76. Two instances found in the iss-76 security re-review: (1) 'memory ingest ' — materialFromLocal EvalSymlinks-resolves the user's source argument and embeds the resolved absolute path in a custom IngestError (internal/core/memory/ingest.go:655,658,663,667). It lies outside cwd/home, so scrubPaths' root-redaction and PathError base-name reduction both miss it. Low severity: it is the user's own argument and carries no developer identity for out-of-home paths (a ~/x argument still redacts to ~/x). (2) STRONGER, separate surface: the capture SUCCESS envelope emits an absolute 'path' field, e.g. {"path":"/.abcd/work/issues/open/iss-N-...md"} — a full home/cwd-rooted path emitted unbidden on the SUCCESS path, which the error-only scrubPaths never sees. Detector (per unrecognized-input-never-writes / no-absolute-paths-in-machine-output): extend the per-verb --json table to assert no absolute path in BOTH the success and error envelopes for verbs echoing store/source paths; fix by rendering repo-relative or base-name paths at those sites. Acceptance corpus: the ingest error sites and the capture success 'path' field above. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-76-json-error-abspath-leak.md b/.abcd/work/issues/resolved/iss-76-json-error-abspath-leak.md similarity index 84% rename from .abcd/work/issues/open/iss-76-json-error-abspath-leak.md rename to .abcd/work/issues/resolved/iss-76-json-error-abspath-leak.md index 5022d24a..6b84ab15 100644 --- a/.abcd/work/issues/open/iss-76-json-error-abspath-leak.md +++ b/.abcd/work/issues/resolved/iss-76-json-error-abspath-leak.md @@ -6,6 +6,7 @@ severity: "minor" category: "bug" source: "agent-finding" found_during: "2026-07-12 /abcd:run iss-29 security review" +resolution: "cli.Run boundary now redacts cwd/home identity roots + base-names PathError/LinkError; residual verb-level abspath echoes tracked in iss-81" --- cli.Run now routes all command errors through the --json envelope, so any verb that returns a bare *fs.PathError (os.ReadFile/os.Open failures not wrapped by core) emits the absolute local path into machine JSON output, violating no-absolute-paths-in-machine-output. iss-29 sanitised the docs-lint config-load branch specifically; the systemic fix is to sanitise PathError-bearing errors at the Run() boundary (or audit which errors reach the envelope). Detector: a table test that runs each verb's known filesystem-error path under --json and asserts the envelope carries no absolute path. Pre-existing as stderr text; newly widened to --json by cli.Run. \ No newline at end of file