diff --git a/.abcd/work/issues/resolved/iss-2608261447262355-test-suite-adopts-the-real-abcd-on-path.md b/.abcd/work/issues/resolved/iss-2608261447262355-test-suite-adopts-the-real-abcd-on-path.md new file mode 100644 index 00000000..d7b1ce6e --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2608261447262355-test-suite-adopts-the-real-abcd-on-path.md @@ -0,0 +1,26 @@ +--- +schema_version: 1 +id: "iss-2608261447262355" +slug: "test-suite-adopts-the-real-abcd-on-path" +severity: "major" +category: "bug" +source: "user-observation" +found_during: "handover review, verified against origin/main after the name-guard hardening merged" +found_at: "internal/core/ahoy/store.go" +resolution: "effectiveBinTarget honours an explicitly set ABCD_BIN_TARGET over adoption, so a caller that names a sandbox target gets it. Adoption is unchanged when the variable is unset, which is production. Two tests pin both halves, the first watched failing against the old order." +impact: internal +--- + +The test suite can adopt and overwrite a developer's real abcd on PATH, because hermeticRepo does not scrub it and effectiveBinTarget prefers an owned PATH entry over an explicitly set ABCD_BIN_TARGET. + +internal/surface/cli/cli_test.go hermeticRepo sets HOME, ABCD_PLUGIN_ROOT and ABCD_BIN_TARGET to temporary directories, and its own docstring says it "redirects HOME, the plugin root and the PATH symlink target to temp locations" — so sandboxing the install target is its stated contract. internal/core/ahoy/store.go effectiveBinTarget breaks that contract: it returns ownedPathEntry(pluginRoot) whenever one exists and only falls back to binTarget(), so the sandbox target is ignored. An ahoy install reached from such a test adopts the entry in place and rewrites it to point inside a test tempdir the test then deletes, so a plain go test ./... mutates state outside the sandbox. + +PRECONDITION, measured rather than assumed, because the naive statement of this bug overclaims. Adoption needs an OWNED entry, and classifyBinTarget calls an entry owned only when it is a symlink into the plugin root (binTargetOwnedSymlink), a dev shim (binTargetDevShim), or a regular file whose provenance is recorded at ~/.abcd/path-entry AND whose bytes still hash to the recorded sha (binTargetOwnedCopy). A plain release binary with no provenance record classifies binTargetForeign and is never adopted. So the population at risk is a dev-shim install (`abcd ahoy install --dev`) or an ahoy-installed owned copy, not every machine with abcd on PATH. + +Evidence for the precondition: on this machine the release-only account carries a real v0.6.6 binary at ~/.local/bin/abcd with no ~/.abcd/path-entry, so it classifies foreign; a peer session confirmed two full `go test ./...` runs on 2026-08-26 left it byte-identical, mtime unchanged. The same machine's development account runs a dev shim and is in the at-risk population. `abcd update` does not write the provenance record, so a release-only install does not drift into the at-risk set by updating; `abcd ahoy install` does. + +Why it is worth more than the inconvenience: this repository is worked from two accounts on purpose, one running the freshly built binary and one running only the latest cut release so the released surface can be observed honestly. Converting the release-only account's binary into a dev shim would destroy that signal with no visible failure. The account is currently outside the at-risk set by classification alone, which is luck rather than design. + +The partial fix shipped with the name-guard hardening covers hermeticEnv, which now scrubs abcd off PATH and uses Lstat so a dangling symlink is dropped too. hermeticRepo is deliberately not covered, because closing it is a design decision: should an explicitly set ABCD_BIN_TARGET suppress adoption of PATH entries outside it? That keeps dependency-gap realism, since other tools stay on PATH, while making install sandbox-safe, and it is inert in production where ABCD_BIN_TARGET is unset. + +Detector: effectiveBinTarget returns the explicit ABCD_BIN_TARGET when one is set even though an owned entry is on PATH, and still returns the owned entry when it is unset. Both watched failing and passing respectively against the current adoption order. diff --git a/internal/core/ahoy/bintarget_sandbox_test.go b/internal/core/ahoy/bintarget_sandbox_test.go new file mode 100644 index 00000000..0e0643ca --- /dev/null +++ b/internal/core/ahoy/bintarget_sandbox_test.go @@ -0,0 +1,77 @@ +package ahoy + +import ( + "os" + "path/filepath" + "testing" +) + +// TestExplicitBinTargetSuppressesAdoption pins the sandbox escape recorded as +// iss-2608261447262355. +// +// effectiveBinTarget answers one question — which PATH entry does a verb act on +// — and it used to answer it by preferring any owned entry it could find, only +// falling back to binTarget(). In production that is right: ABCD_BIN_TARGET is +// unset, an install that finds an abcd it already owns should UPDATE that one +// rather than leave a second copy somewhere else, and adoption is how `ahoy +// install` stays idempotent across the shapes a real machine presents. +// +// Under test it is destructive. A test that sets ABCD_BIN_TARGET to a temp path +// is asking for a sandbox, and the preference silently overrode the ask: the +// developer's own installed abcd is an owned entry, so `ahoy install` reached +// from a test adopted the REAL binary and rewrote it to point inside a temp dir +// that the test then deleted. The machines that dogfood the installer are +// exactly the machines that have an owned entry to find, so the blast radius +// tracked the population most likely to run the suite. +// +// So an explicitly set ABCD_BIN_TARGET now wins. It is an instruction, not a +// hint, and honouring it is inert in production where the variable is unset. +func TestExplicitBinTargetSuppressesAdoption(t *testing.T) { + pluginRoot := t.TempDir() + if err := os.WriteFile(filepath.Join(pluginRoot, binName), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + + // An OWNED entry on PATH: a symlink named abcd pointing into the plugin root. + // This is the developer's installed abcd, standing in for the thing the old + // behaviour adopted and overwrote. + pathDir := t.TempDir() + realEntry := filepath.Join(pathDir, binName) + if err := os.Symlink(filepath.Join(pluginRoot, binName), realEntry); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", pathDir) + + // The sandbox the caller asked for. + sandbox := filepath.Join(t.TempDir(), "bin", binName) + t.Setenv("ABCD_BIN_TARGET", sandbox) + + if got := effectiveBinTarget(pluginRoot); got != sandbox { + t.Errorf("effectiveBinTarget = %q, want the explicit ABCD_BIN_TARGET %q\n"+ + "an explicit target is a sandbox instruction; adopting %q instead lets a test write outside its sandbox", + got, sandbox, realEntry) + } +} + +// TestAdoptionStillWinsWhenBinTargetIsUnset is the other half, and the reason +// the fix is scoped to an EXPLICIT setting rather than removing adoption. With +// ABCD_BIN_TARGET unset — production — an owned PATH entry must still be the +// entry every verb acts on, or `ahoy install` stops being idempotent and starts +// leaving a second copy beside the one it already owns. +func TestAdoptionStillWinsWhenBinTargetIsUnset(t *testing.T) { + pluginRoot := t.TempDir() + if err := os.WriteFile(filepath.Join(pluginRoot, binName), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + pathDir := t.TempDir() + owned := filepath.Join(pathDir, binName) + if err := os.Symlink(filepath.Join(pluginRoot, binName), owned); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", pathDir) + t.Setenv("ABCD_BIN_TARGET", "") + + if got := effectiveBinTarget(pluginRoot); got != owned { + t.Errorf("effectiveBinTarget = %q, want the owned PATH entry %q — adoption must survive for the unset case", got, owned) + } +} diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 2fb5b376..5cdaa398 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -266,6 +266,22 @@ func danglingPathEntry(pluginRoot string) (pathEntry, bool) { // effectiveBinTarget is the PATH entry every verb acts on: an existing owned // install (adopted where it stands), else the default target. func effectiveBinTarget(pluginRoot string) string { + // An explicitly set ABCD_BIN_TARGET is an instruction, not a hint, and it + // wins over adoption (iss-2608261447262355). Adoption is right when the + // variable is unset — an install that finds an abcd it already owns should + // update THAT one rather than leave a second copy elsewhere, which is how + // `ahoy install` stays idempotent across the shapes a real machine presents. + // But a caller that names a target has asked for a specific file, and the + // only callers that name one are tests asking for a sandbox. Preferring an + // owned entry over their ask is how `go test ./...` came to adopt a real + // PATH entry and rewrite it into a temp dir: the machines that dogfood the + // installer are exactly the machines with an owned entry to find. + // + // Inert in production, where ABCD_BIN_TARGET is unset and this falls through + // to the adoption path unchanged. + if os.Getenv("ABCD_BIN_TARGET") != "" { + return binTarget() + } if e, ok := ownedPathEntry(pluginRoot); ok { return e.path }