fix: make the identity pin round-trip through the self-contained commit guard (iss-63)

.abcd/work/issues/open/iss-63-identity-gate-followups.md → .abcd/work/issues/resolved/iss-63-identity-gate-followups.md

@@ -6,6 +6,7 @@ severity: "minor"
 category: "future-work-seed"
 source: "agent-finding"
 found_during: "iss-62-security-review"
+resolution: "WritePin now round-trips the pin through the hook's naive parse: it marshals without HTML escaping so &/</> (legal in a git user.name like 'Marks & Spencer') are stored literally, and refuses the characters a parse can never read back (double-quote, backslash, control). The class fix (not just the double-quote) was chosen over delegating the gate to a possibly-stale binary, keeping the hook self-contained. Detector: TestWritePin_RejectsUnpinnableChars + TestWritePin_LegalIdentitiesRoundTrip, fail->pass (adversarial review caught the &/\\ siblings of the original quote-only fix). Item-2 (auto-pin without --yes) left advisory per the issue."
 ---
 
 iss-62 identity-gate follow-ups from security review (advisory, non-blocking): (1) the pre-commit shell guard mis-parses a pinned name containing an escaped double-quote (sed truncates it), blocking even a correctly-configured identity — fail-closed but a usability bug; WritePin also emits that escaped form. Consider delegating to 'abcd ahoy identity-check' when on PATH, or a more robust JSON parse. (2) A programmatic caller passing ApprovedCategories{ConfigChange:true} (without --yes) still auto-pins the current identity; deliberate per-category approval, flagged as a conscious choice.

CHANGELOG.md

@@ -30,6 +30,16 @@ called out in a **Breaking** section.
 
 ### Fixed
 
+- **The identity pin round-trips through the self-contained commit guard.** The
+  pin was stored with Go's default JSON encoder, which escapes `&`, `<`, `>` (and
+  always `"`, `\`, control characters); the pre-commit identity guard reads the
+  raw bytes between the quotes with a naive parse, so an escaped value never
+  matched `git config` and fail-closed a correctly configured identity (e.g. a
+  `user.name` of `Marks & Spencer`). The pin is now marshalled without HTML
+  escaping so `&`/`<`/`>` are stored literally, and the characters a parse can
+  never read back (`"`, `\`, control) are refused at pin time with a clear remedy
+  — keeping the commit guard zero-dependency rather than delegating it to a
+  possibly-stale binary.
 - **`abcd intent "<text>"` no longer files a draft from a mistyped subcommand.**
   A near-miss for an intent subverb (`intent paln`, `intent lnk itd-5`) is
   refused with a did-you-mean and writes nothing, mirroring `abcd capture`'s

internal/core/identity/identity.go

@@ -9,6 +9,7 @@
 package identity
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"os"
@@ -118,21 +119,55 @@ func WritePin(root string, p Pin) error {
 	if p.Name == "" || p.Email == "" {
 		return fmt.Errorf("identity pin requires both name and email")
 	}
+	// The self-contained pre-commit identity guard reads the pin with a naive
+	// sed that captures the raw bytes between the JSON quotes and compares them
+	// literally to `git config`, so the stored value must round-trip through that
+	// sed. Two things ensure it (iss-63): the pin is marshalled WITHOUT HTML
+	// escaping (below), so &, <, > — legal in a git user.name like
+	// "Marks & Spencer" — are stored literally rather than escaped; and the
+	// characters JSON must escape regardless (a double-quote, a backslash, or a
+	// control character), which the sed can never read back, are refused here so
+	// a pin can never hold one and fail-close a correct identity. This keeps the
+	// hook zero-dependency rather than delegating the gate to a possibly-stale
+	// binary.
+	if unpinnable(p.Name) {
+		return fmt.Errorf("identity pin name must not contain a double-quote, backslash, or control character (it breaks the self-contained pre-commit identity guard); adjust git config user.name")
+	}
+	if unpinnable(p.Email) {
+		return fmt.Errorf("identity pin email must not contain a double-quote, backslash, or control character; adjust git config user.email")
+	}
 	path := filepath.Join(root, PinRelPath)
 	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
 		return err
 	}
-	data, err := json.MarshalIndent(p, "", "  ")
-	if err != nil {
+	// Marshal WITHOUT HTML escaping (so &, <, > survive literally) and route the
+	// bytes through the canonical atomic primitive (temp + fchmod + fsync +
+	// rename + parent-dir fsync): a plain in-place os.WriteFile truncates the pin
+	// before rewriting it — a crash mid-write leaves a corrupt or empty
+	// identity.json — and it follows a symlink at path. json.Encoder appends the
+	// trailing newline.
+	var buf bytes.Buffer
+	enc := json.NewEncoder(&buf)
+	enc.SetEscapeHTML(false)
+	enc.SetIndent("", "  ")
+	if err := enc.Encode(p); err != nil {
 		return err
 	}
-	// Route through the canonical atomic primitive (temp + fchmod + fsync + rename
-	// + parent-dir fsync): a plain in-place os.WriteFile truncates the pin before
-	// rewriting it — a crash mid-write leaves a corrupt or empty identity.json, and
-	// it follows a symlink at path. This is the fifth writer the iss-32
-	// consolidation missed (the guard only flags divergent atomic primitives, not a
-	// non-atomic write).
-	return fsutil.WriteFileAtomic(path, append(data, '\n'), 0o644)
+	return fsutil.WriteFileAtomic(path, buf.Bytes(), 0o644)
+}
+
+// unpinnable reports whether s holds a character the identity pin cannot safely
+// carry: a double-quote or backslash (which JSON must always escape) or a
+// control character — none of which the self-contained pre-commit hook's sed can
+// read back to compare against `git config` (iss-63). Characters like &, <, >
+// are pinnable because WritePin marshals without HTML escaping.
+func unpinnable(s string) bool {
+	for _, r := range s {
+		if r == '"' || r == '\\' || r < 0x20 {
+			return true
+		}
+	}
+	return false
 }
 
 // EffectiveIdentity returns the author identity git would stamp on a commit in

internal/core/identity/identity_test.go

@@ -4,6 +4,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -200,6 +201,54 @@ func TestWritePin_RequiresBothFields(t *testing.T) {
 	}
 }
 
+// The self-contained pre-commit hook reads the pin with a naive sed, so the
+// stored value must round-trip through it (iss-63). WritePin refuses the
+// characters JSON must always escape (double-quote, backslash, control), which
+// the sed cannot read back, and stores &, <, > literally (no HTML escaping) so
+// a legal git identity like "Marks & Spencer" is not fail-closed.
+func TestWritePin_RejectsUnpinnableChars(t *testing.T) {
+	dir := t.TempDir()
+	for _, p := range []Pin{
+		{Name: `Alex "the dev"`, Email: "alex@example.com"}, // double-quote
+		{Name: `back\slash`, Email: "alex@example.com"},     // backslash
+		{Name: "line\nbreak", Email: "alex@example.com"},    // control char
+		{Name: "Alex", Email: `a"x@example.com`},            // quote in email
+	} {
+		if err := WritePin(dir, p); err == nil {
+			t.Fatalf("want error for unpinnable pin %+v", p)
+		}
+	}
+}
+
+func TestWritePin_LegalIdentitiesRoundTrip(t *testing.T) {
+	for _, want := range []Pin{
+		{Name: "Marks & Spencer", Email: "ci@m-and-s.example"}, // & is legal, must not HTML-escape
+		{Name: "O'Brien", Email: "obrien@example.com"},         // apostrophe
+		{Name: "José <team>", Email: "jose@example.com"},       // unicode + < >
+	} {
+		dir := t.TempDir()
+		if err := WritePin(dir, want); err != nil {
+			t.Fatalf("legal identity %+v must write: %v", want, err)
+		}
+		got, ok, err := LoadPin(dir)
+		if err != nil || !ok || got != want {
+			t.Fatalf("round-trip %+v: got %+v ok=%v err=%v", want, got, ok, err)
+		}
+		// With HTML escaping off, the raw pin carries &, <, > literally; the bug
+		// (escaping on) would store the \uXXXX form instead, so the literal char
+		// would be ABSENT from the file — which the hook's sed cannot read back.
+		raw, err := os.ReadFile(filepath.Join(dir, PinRelPath))
+		if err != nil {
+			t.Fatal(err)
+		}
+		for _, r := range []rune{'&', '<', '>'} {
+			if strings.ContainsRune(want.Name, r) && !strings.ContainsRune(string(raw), r) {
+				t.Fatalf("pin did not store %q literally (HTML-escaped?); the hook sed cannot read it back:\n%s", r, raw)
+			}
+		}
+	}
+}
+
 // Blocks reports whether a pre-commit hook should refuse the commit: a mismatch
 // or an unset identity blocks; OK and NoPin (opted-out) do not.
 func TestBlocks(t *testing.T) {