fix: spec-store open-once read + fail-closed spec_id scan (iss-68)

.abcd/work/issues/open/iss-68-spec-store-hardening.md → .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.

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 <spc-N>` 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-<slug>` 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,

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()
 }

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-<slug>" 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,26 +240,37 @@ 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)
 	}
 	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
 }
 

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-<slug> 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-<slug>" 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)")
+	}
+}