fix: rules-loader guarded read + private session-state dir (iss-66)

.abcd/work/DECISIONS.md

@@ -228,3 +228,18 @@ parallel-agent merge contention bites.
   sign-off before landing (a STOP-adjacent design surface). Ledger triage committed
   to `main` as a `chore:` record commit (matches prior record-to-main practice;
   keeps the fix branches clean); each code fix lands on its own `auto/*` branch + PR.
+- 2026-07-12 — iss-66 rules-loader trust boundary. Fixed the two mechanical items:
+  the Load Lstat→ReadFile TOCTOU on `.abcd/rules.json` (now open-once O_NOFOLLOW +
+  fstat, C19) and the session-state dir moved off the world-writable shared /tmp to
+  the per-user cache dir (P14). **P15 document-accepted, NOT changed:** a per-repo
+  `.abcd/rules.json` can set a default domain dormant and flip the global kill
+  switch (Merge is intentionally per-field + sticky-kill-switch). Rationale: rules
+  are an *opt-in, opinionated-but-overridable* config layer; `.abcd/rules.json` is a
+  committed file (editing it needs repo write access, like any committed guardrail),
+  and the real enforcement of dangerous actions is harness-level (git-guardrails
+  hooks, the iss-62 identity gate, pre-commit), not the injected advisory prose.
+  Silencing a domain removes prose, not a hard gate. **Deferred design alternative
+  (surfaced, not taken):** introduce a protected "guardrail" domain class that a
+  per-repo override cannot set dormant and that the kill switch cannot silence —
+  this adds a new protected-domain concept to the rules contract, a maintainer
+  decision, not an autonomous change.

.abcd/work/issues/open/iss-66-rules-loader-trust-boundary.md → .abcd/work/issues/resolved/iss-66-rules-loader-trust-boundary.md

@@ -7,6 +7,7 @@ category: "bug"
 source: "agent-finding"
 found_during: "clean-slate-sweep"
 found_at: "internal/core/rules/rules.go"
+resolution: "Rules loader hardened: guarded read (open-once O_NOFOLLOW+O_NONBLOCK+fstat, shared by Load/LoadState/LoadBackstop) closes the rules.json symlink-swap TOCTOU (C19) and a FIFO-leaf hang the first cut introduced (F1, caught by security review); session-state moved off shared /tmp to the per-user cache dir (P14). P15 (override can disable guardrail domains/kill switch) document-accepted with rationale + surfaced protected-domain alternative in DECISIONS. ruthless SHIP + security PASS (2 rounds; security found + re-verified the FIFO BLOCK)."
 ---
 
 rules loader trust-boundary (itd-3): an untrusted per-repo .abcd/rules.json can silence default guardrail domains — Merge() ORs the global kill switch (rules.go:156) and mergeDomain lets an override set any default domain dormant, so a cloned repo suppresses all rule injection (P15); session-state lives in a predictable shared /tmp path (os.TempDir/abcd-rules-state, sha256(session).json, session defaults to constant default) letting a local co-tenant suppress injection fail-open (inject.go:99, P14); Load() Lstats rules.json then os.ReadFile re-resolves by name, a symlink-swap TOCTOU (rules.go:132, C19). Detector: override cannot disable a default guardrail/kill-switch; state under UserCacheDir+ownership check; open-once O_NOFOLLOW+fstat. Corpus: P15, P14, C19.

CHANGELOG.md

@@ -158,6 +158,13 @@ called out in a **Breaking** section.
   status directories at once. A migrator-supplied `ForceID` is validated against
   the `iss-N` shape before any path is built, so a traversal id cannot touch the
   filesystem outside the ledger.
+- **Rules-loader trust hardening** (iss-66). The per-repo `.abcd/rules.json` is now
+  opened once with `O_NOFOLLOW` and validated on that file descriptor, closing a
+  Lstat-then-read window where the file could be swapped for a symlink. The
+  prompt-router's per-session dedup state moved off the world-writable shared temp
+  dir to the per-user cache dir (`ABCD_RULES_STATE_DIR` still overrides), so a local
+  co-tenant can no longer pre-create the predictable state path to suppress rule
+  injection.
 
 ## [v0.1.0] - 2026-07-07
 

internal/core/rules/inject.go

@@ -4,6 +4,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"sort"
@@ -91,12 +92,22 @@ func Inject(rs RuleSet, prompt string, prev SessionState, backstop int) InjectRe
 
 // stateDir is the machine-local directory holding per-session ledgers. It is
 // overridable via ABCD_RULES_STATE_DIR (used by tests and by operators who want
-// the state off the default temp dir).
+// the state elsewhere). The default is the per-user cache dir, NOT the
+// world-writable shared temp dir: a predictable path under a shared /tmp lets a
+// local co-tenant pre-create or poison the session-state file and suppress rule
+// injection fail-open. The uid-qualified temp fallback (only if the user cache
+// dir is unavailable — a degraded env with no HOME/XDG_CACHE_HOME) still lives
+// under the shared temp dir and is best-effort only: it avoids a cross-user
+// collision, not co-tenant pre-creation. State is fail-open advisory dedup, so a
+// poisoned fallback at worst re-injects; it never blocks or corrupts.
 func stateDir() string {
 	if d := os.Getenv("ABCD_RULES_STATE_DIR"); d != "" {
 		return d
 	}
-	return filepath.Join(os.TempDir(), "abcd-rules-state")
+	if cache, err := os.UserCacheDir(); err == nil {
+		return filepath.Join(cache, "abcd-rules-state")
+	}
+	return filepath.Join(os.TempDir(), fmt.Sprintf("abcd-rules-state-%d", os.Getuid()))
 }
 
 // sessionFile maps a session id to a state file. The id is hashed, so an
@@ -110,12 +121,7 @@ func sessionFile(session string) string {
 // file yields the zero state (a fresh session), never an error — dedup is a
 // best-effort optimisation, not a correctness gate.
 func LoadState(session string) SessionState {
-	path := sessionFile(session)
-	fi, err := os.Lstat(path)
-	if err != nil || !fi.Mode().IsRegular() || fi.Size() > maxStateFileBytes {
-		return SessionState{}
-	}
-	data, err := os.ReadFile(path)
+	data, err := readGuarded(sessionFile(session), maxStateFileBytes)
 	if err != nil {
 		return SessionState{}
 	}
@@ -167,11 +173,7 @@ func SaveState(session string, st SessionState) error {
 // non-positive value falls back to DefaultRefreshBackstop.
 func LoadBackstop(repoRoot string) int {
 	path := filepath.Join(repoRoot, ".abcd", "config.json")
-	fi, err := os.Lstat(path)
-	if err != nil || !fi.Mode().IsRegular() || fi.Size() > maxRulesFileBytes {
-		return DefaultRefreshBackstop
-	}
-	data, err := os.ReadFile(path)
+	data, err := readGuarded(path, maxRulesFileBytes)
 	if err != nil {
 		return DefaultRefreshBackstop
 	}

internal/core/rules/inject_test.go

@@ -8,6 +8,25 @@ import (
 	"time"
 )
 
+// TestStateDirNotSharedTemp (iss-66 P14) proves the default session-state dir is
+// the per-user cache dir, not the world-writable shared temp dir where a local
+// co-tenant could pre-create the predictable session-state path and suppress
+// rule injection fail-open.
+func TestStateDirNotSharedTemp(t *testing.T) {
+	t.Setenv("ABCD_RULES_STATE_DIR", "") // exercise the default
+	cache, err := os.UserCacheDir()
+	if err != nil {
+		t.Skip("user cache dir unavailable; only the degraded uid-qualified temp fallback applies")
+	}
+	dir := stateDir()
+	if !strings.HasPrefix(dir, cache) {
+		t.Fatalf("default state dir should live under the user cache dir %s, got %s", cache, dir)
+	}
+	if strings.HasPrefix(dir, os.TempDir()) {
+		t.Fatalf("default state dir must not live under the shared temp dir, got %s", dir)
+	}
+}
+
 func TestInjectFirstTurnRenders(t *testing.T) {
 	rs := Defaults()
 	res := Inject(rs, "commit and push", SessionState{}, 0)

internal/core/rules/rules.go

@@ -20,13 +20,16 @@ package rules
 import (
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"hash/fnv"
+	"io"
 	"os"
 	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
+	"syscall"
 )
 
 // RepoRelPath is the per-repo override file, relative to the repo worktree.
@@ -100,6 +103,46 @@ func mustParseDefaults() RuleSet {
 // the caller to mutate.
 func Defaults() RuleSet { return cloneRuleSet(defaultRuleSet) }
 
+var (
+	errNotRegular = errors.New("not a regular file")
+	errTooBig     = errors.New("exceeds size cap")
+)
+
+// readGuarded opens path once, read-only, with O_NOFOLLOW (refuse a symlinked
+// leaf) and O_NONBLOCK (a FIFO/device leaf returns immediately instead of
+// blocking the open forever), then validates on the SAME file descriptor that it
+// is a regular file within limit bytes before reading through a LimitReader — so
+// no symlink swap between stat and read, no non-regular leaf, and no size overrun
+// can reach the caller. The raw open error is returned so callers can test
+// os.IsNotExist / syscall.ELOOP; a non-regular or oversize file returns the
+// errNotRegular / errTooBig sentinel.
+func readGuarded(path string, limit int64) ([]byte, error) {
+	f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+	fi, err := f.Stat()
+	if err != nil {
+		return nil, err
+	}
+	if !fi.Mode().IsRegular() {
+		return nil, errNotRegular
+	}
+	if fi.Size() > limit {
+		return nil, errTooBig
+	}
+	data, err := io.ReadAll(io.LimitReader(f, limit+1))
+	if err != nil {
+		return nil, err
+	}
+	if int64(len(data)) > limit {
+		// The file grew past the cap between fstat and read (a size TOCTOU).
+		return nil, errTooBig
+	}
+	return data, nil
+}
+
 // Load returns the defaults merged with <repoRoot>/.abcd/rules.json when that
 // file exists. An absent file yields the defaults unchanged; a present file
 // that cannot be parsed or fails validation is a fail-closed error.
@@ -110,28 +153,20 @@ func Load(repoRoot string) (RuleSet, error) {
 		return RuleSet{}, fmt.Errorf("rules: .abcd is a symlink (refusing to follow)")
 	}
 	path := filepath.Join(repoRoot, ".abcd", "rules.json")
-	fi, err := os.Lstat(path)
-	if os.IsNotExist(err) {
-		return Defaults(), nil
-	}
+	data, err := readGuarded(path, maxRulesFileBytes)
 	if err != nil {
-		return RuleSet{}, fmt.Errorf("rules: stat %s: %w", RepoRelPath, err)
-	}
-	// Trust-boundary guards (mirror the ahoy hook-manifest verifier): refuse a
-	// symlinked leaf and cap the file size so a hostile rules.json cannot force a
-	// symlink-follow or a memory blow-up.
-	if fi.Mode()&os.ModeSymlink != 0 {
-		return RuleSet{}, fmt.Errorf("rules: %s is a symlink (refusing to follow)", RepoRelPath)
-	}
-	if !fi.Mode().IsRegular() {
-		return RuleSet{}, fmt.Errorf("rules: %s is not a regular file", RepoRelPath)
-	}
-	if fi.Size() > maxRulesFileBytes {
-		return RuleSet{}, fmt.Errorf("rules: %s exceeds the %d-byte cap", RepoRelPath, maxRulesFileBytes)
-	}
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return RuleSet{}, fmt.Errorf("rules: reading %s: %w", RepoRelPath, err)
+		switch {
+		case os.IsNotExist(err):
+			return Defaults(), nil
+		case errors.Is(err, syscall.ELOOP):
+			return RuleSet{}, fmt.Errorf("rules: %s is a symlink (refusing to follow)", RepoRelPath)
+		case errors.Is(err, errNotRegular):
+			return RuleSet{}, fmt.Errorf("rules: %s is not a regular file", RepoRelPath)
+		case errors.Is(err, errTooBig):
+			return RuleSet{}, fmt.Errorf("rules: %s exceeds the %d-byte cap", RepoRelPath, maxRulesFileBytes)
+		default:
+			return RuleSet{}, fmt.Errorf("rules: reading %s: %w", RepoRelPath, err)
+		}
 	}
 	var over RuleSet
 	if err := json.Unmarshal(data, &over); err != nil {

internal/core/rules/rules_test.go

@@ -4,9 +4,61 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 	"testing"
+	"time"
 )
 
+// TestLoadRefusesSymlinkedRulesFile (iss-66 C19) proves a symlinked rules.json
+// LEAF is refused via O_NOFOLLOW. The directory-symlink test covers only the
+// ancestor guard; reverting O_NOFOLLOW makes Load follow the leaf link and read
+// an arbitrary target, so this test fails on that revert.
+func TestLoadRefusesSymlinkedRulesFile(t *testing.T) {
+	dir := t.TempDir()
+	abcd := filepath.Join(dir, ".abcd")
+	if err := os.MkdirAll(abcd, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	target := filepath.Join(dir, "real-rules.json")
+	if err := os.WriteFile(target, []byte(`{"schema_version":1}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(target, filepath.Join(abcd, "rules.json")); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := Load(dir); err == nil {
+		t.Fatal("a symlinked rules.json leaf must be refused, not followed")
+	}
+}
+
+// TestLoadRefusesFifoRulesFile (iss-66 F1) proves a FIFO leaf is rejected
+// promptly rather than hung on. O_NONBLOCK lets the open return so the
+// regular-file check fails closed; without it, open(FIFO, O_RDONLY) blocks
+// forever and wedges the (non-blocking-by-contract) prompt-router hook.
+func TestLoadRefusesFifoRulesFile(t *testing.T) {
+	dir := t.TempDir()
+	abcd := filepath.Join(dir, ".abcd")
+	if err := os.MkdirAll(abcd, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := syscall.Mkfifo(filepath.Join(abcd, "rules.json"), 0o644); err != nil {
+		t.Skipf("mkfifo unsupported: %v", err)
+	}
+	done := make(chan error, 1)
+	go func() {
+		_, err := Load(dir)
+		done <- err
+	}()
+	select {
+	case err := <-done:
+		if err == nil {
+			t.Fatal("a FIFO rules.json must be refused, not read")
+		}
+	case <-time.After(3 * time.Second):
+		t.Fatal("Load hung on a FIFO rules.json leaf (must never block the prompt-router hook)")
+	}
+}
+
 func names(ds []ResolvedDomain) []string {
 	out := make([]string, len(ds))
 	for i, d := range ds {