diff --git a/.abcd/work/issues/open/iss-68-spec-store-hardening.md b/.abcd/work/issues/resolved/iss-68-spec-store-hardening.md similarity index 65% rename from .abcd/work/issues/open/iss-68-spec-store-hardening.md rename to .abcd/work/issues/resolved/iss-68-spec-store-hardening.md index 7772c7f0..ed57b14b 100644 --- a/.abcd/work/issues/open/iss-68-spec-store-hardening.md +++ b/.abcd/work/issues/resolved/iss-68-spec-store-hardening.md @@ -7,6 +7,7 @@ category: "bug" source: "agent-finding" found_during: "clean-slate-sweep" found_at: "internal/core/spec/store.go" +resolution: "Spec-store hardening: open-once O_NOFOLLOW+O_NONBLOCK read (P7, closes symlink-swap TOCTOU + FIFO hang), NextID fail-closed on a numberless spec_id (P5, narrowed to agree with record-lint's prefix rule after security review caught an over-broad first cut), clearer spec-body placeholder (seed4). seed5 (ancestor symlink) + P8 (close clobber race) document-accepted under trusted-worktree. ruthless SHIP + security PASS (2 rounds; security caught + re-verified a P5 divergence BLOCK)." --- spec store hardening (itd-80): spec.Create writes a Summary/TODO body — decide the minimal native-spec body and make record-lint tolerant (spec.go:135, seed4); ensureDir/ensureRealDir only Lstat the leaf so a symlinked ancestor (specs/, intents/) is followed — literal guard or honest comment (seed5, spec+intent); readRepoFile Lstat then ReadFile re-resolves the path (symlink TOCTOU) and the size cap is not atomic — open-once O_NOFOLLOW+fstat+LimitReader (store.go:225, P7); Close Lstat-exists-then-Rename is racy and os.Rename silently overwrites — no-replace primitive (store.go:212, P8); maxIntentSpecNum silently drops a non-null unparseable spec_id, fail-open reservation scan (store.go:146, P5). Corpus: seed4, seed5, P7, P8, P5. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f2dc136..e5e1a337 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,8 @@ called out in a **Breaking** section. ### Changed +- `abcd intent plan` seeds a new native spec with a clear author-guidance + placeholder in its `## Summary`, rather than a bare `TODO` (iss-68). - `abcd spec close ` now reconciles the linked intent (itd-80): it moves the intent `planned/ → shipped/` and then closes the spec, so one command completes the lifecycle transition. It is fail-closed (a missing/empty intent @@ -152,6 +154,15 @@ called out in a **Breaking** section. ### Security +- **Spec-store hardening** (iss-68). The spec-store reader now opens a file once + with `O_NOFOLLOW`+`O_NONBLOCK` and validates the file descriptor before reading, + closing a symlink-swap window (and never blocking on a FIFO leaf). `NextID` fails + closed on an intent `spec_id` that carries no parseable reservation number (e.g. + `spc-` with no digits) instead of silently dropping it from the id-reservation + scan (which could hand out a colliding id); a `spc-N` or `spc-N-` form still + reserves N, consistent with record-lint. (The leaf-only ancestor-symlink guard + and the atomic-rename clobber check are documented as accepted under the + trusted-worktree model.) - **Release receipt-gate hardening** (iss-70). The `receipt_gate` record-lint rule now binds each semantic-pass receipt to the gate it attests: a receipt satisfies a required gate only when its `policy.detector` equals that gate name, diff --git a/internal/core/spec/spec.go b/internal/core/spec/spec.go index 1f98fc0f..9dc96753 100644 --- a/internal/core/spec/spec.go +++ b/internal/core/spec/spec.go @@ -132,6 +132,11 @@ func renderSpec(id, slug, intentID string) string { fmt.Fprintf(&b, "intent: %s\n", intentID) b.WriteString("---\n") fmt.Fprintf(&b, "# %s\n\n", slug) - b.WriteString("## Summary\n\nTODO\n") + b.WriteString("## Summary\n\n") + // A clear author-guidance placeholder, not a bare "TODO" that reads as drift: + // the spec body is the design record the intent's fidelity review audits against. + fmt.Fprintf(&b, "_Draft: describe what %s delivers for %s — scope, approach, and how "+ + "it satisfies the intent's Acceptance Criteria. This spec is the design record "+ + "the fidelity review audits against._\n", id, intentID) return b.String() } diff --git a/internal/core/spec/store.go b/internal/core/spec/store.go index 624c666b..286ed007 100644 --- a/internal/core/spec/store.go +++ b/internal/core/spec/store.go @@ -1,10 +1,13 @@ package spec import ( + "errors" "fmt" + "io" "os" "path/filepath" "strings" + "syscall" "github.com/REPPL/abcd-cli/internal/core/frontmatter" "github.com/REPPL/abcd-cli/internal/fsutil" @@ -143,6 +146,17 @@ func maxIntentSpecNum(repoRoot string) (int, error) { if frontmatter.IsNull(v) { continue } + // A non-null spec_id from which no reservation number can be parsed + // (e.g. "spc-", "spc-abc") is fail-closed, not silently dropped: + // dropping it would leave its reservation out of the max and let NextID + // mint a colliding id. A "spc-N" or "spc-N-" form is fine and + // reserves N — record-lint's planned rule (prefix ^spc-) and the + // spec_lifecycle specNum parse both tolerate the trailing slug, so this + // check must NOT reject that form or the two gates would disagree and a + // lint-green record would brick the mint path. + if !specNumRe.MatchString(v) { + return 0, fmt.Errorf("spec: intent %s has a spec_id %q with no reservable number (must be spc-N)", rel, v) + } if n := specNum(v); n > max { max = n } @@ -209,6 +223,12 @@ func Close(repoRoot, specID string) (Spec, error) { return Spec{}, err } dstRel := filepath.Join(SpecsRelDir, StatusClosed, name) + // Best-effort clobber guard: os.Rename would silently overwrite the destination, + // so refuse when it already exists. This Lstat→Rename check is racy against a + // file appearing in the window — accepted under the trusted-worktree model (only + // the developer/agent mutates the store; there is no concurrent adversary), where + // the atomic same-filesystem rename is preferred over a non-atomic no-clobber + // link+remove that a crash could leave half-done. if _, err := os.Lstat(filepath.Join(closedDir, name)); err == nil { return Spec{}, fmt.Errorf("spec: refusing to overwrite existing %s", dstRel) } @@ -220,15 +240,23 @@ func Close(repoRoot, specID string) (Spec, error) { return sp, nil } -// readRepoFile reads a repo file behind the trust-boundary guards: refuse a -// symlinked leaf, require a regular file, and cap the size. +// readRepoFile reads a repo file behind the trust-boundary guards. It opens ONCE +// with O_NOFOLLOW (refuse a symlinked leaf) and O_NONBLOCK (a FIFO/device leaf +// returns immediately instead of blocking the open), then validates the SAME file +// descriptor (regular file, size cap) before reading — so a symlink swap between +// stat and read cannot redirect it. func readRepoFile(abs, rel string) ([]byte, error) { - fi, err := os.Lstat(abs) + f, err := os.OpenFile(abs, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) if err != nil { - return nil, fmt.Errorf("spec: stat %s: %w", rel, err) + if errors.Is(err, syscall.ELOOP) { + return nil, fmt.Errorf("spec: %s is a symlink (refusing to follow)", rel) + } + return nil, fmt.Errorf("spec: opening %s: %w", rel, err) } - if fi.Mode()&os.ModeSymlink != 0 { - return nil, fmt.Errorf("spec: %s is a symlink (refusing to follow)", rel) + defer f.Close() + fi, err := f.Stat() + if err != nil { + return nil, fmt.Errorf("spec: stat %s: %w", rel, err) } if !fi.Mode().IsRegular() { return nil, fmt.Errorf("spec: %s is not a regular file", rel) @@ -236,10 +264,13 @@ func readRepoFile(abs, rel string) ([]byte, error) { if fi.Size() > maxSpecFileBytes { return nil, fmt.Errorf("spec: %s exceeds the %d-byte cap", rel, maxSpecFileBytes) } - data, err := os.ReadFile(abs) + data, err := io.ReadAll(io.LimitReader(f, maxSpecFileBytes+1)) if err != nil { return nil, fmt.Errorf("spec: reading %s: %w", rel, err) } + if int64(len(data)) > maxSpecFileBytes { + return nil, fmt.Errorf("spec: %s exceeds the %d-byte cap", rel, maxSpecFileBytes) + } return data, nil } diff --git a/internal/core/spec/store_test.go b/internal/core/spec/store_test.go index fd1679d1..ed50f2ff 100644 --- a/internal/core/spec/store_test.go +++ b/internal/core/spec/store_test.go @@ -4,7 +4,9 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" + "time" ) const ( @@ -45,7 +47,8 @@ func TestNextIDMaxAcrossSpecsAndIntents(t *testing.T) { // A spec-store file at spc-5 (higher than any intent reservation). writeFile(t, root, specsOpen+"/spc-5-existing.md", "---\nid: spc-5\nslug: existing\nintent: itd-9\n---\n# ok\n") - // An intent reserving spc-2 (lower; must not lower the max). + // An intent reserving spc-2 via the tolerated spc-N- form (lower; must + // not lower the max). record-lint accepts this shape, so the store must too. writeFile(t, root, intentsBase+"/planned/itd-20-x.md", "---\nid: itd-20\nslug: x\nspec_id: spc-2-thing\nkind: standalone\n---\n# ok\n") @@ -212,3 +215,43 @@ func TestCloseAlreadyClosedFails(t *testing.T) { t.Fatal("Close on an already-closed spec must fail") } } + +// TestNextIDRejectsUnreservableSpecID (iss-68 P5) proves a non-null spec_id with +// no parseable reservation number ("spc-oops") fails NextID closed rather than +// being silently dropped from the reservation scan (which could hand out a +// colliding id). A well-formed "spc-N" / "spc-N-" is accepted (see +// TestNextIDMaxAcrossSpecsAndIntents); only a numberless one is rejected. +func TestNextIDRejectsUnreservableSpecID(t *testing.T) { + root := t.TempDir() + writeFile(t, root, intentsBase+"/planned/itd-20-x.md", + "---\nid: itd-20\nslug: x\nspec_id: spc-oops\nkind: standalone\n---\n# ok\n") + if _, err := NextID(root); err == nil { + t.Fatal("NextID must fail closed on a spec_id with no reservable number, not silently drop it") + } +} + +// TestLoadRejectsFifoSpecFile (iss-68 P7) proves a FIFO at a spec path is rejected +// promptly, not hung on. The read opens with O_NOFOLLOW|O_NONBLOCK and validates +// the fd, so a FIFO returns a not-regular error instead of blocking os.ReadFile. +func TestLoadRejectsFifoSpecFile(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, specsOpen), 0o755); err != nil { + t.Fatal(err) + } + if err := syscall.Mkfifo(filepath.Join(root, specsOpen, "spc-1-x.md"), 0o644); err != nil { + t.Skipf("mkfifo unsupported: %v", err) + } + done := make(chan error, 1) + go func() { + _, err := Load(root) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("a FIFO spec file must be refused, not read") + } + case <-time.After(3 * time.Second): + t.Fatal("Load hung on a FIFO spec file (open must not block)") + } +}