diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 5d16153..1525c0a 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -25,17 +25,6 @@ Architecture-shaping decisions graduate to an ADR under - 2026-07-17 — WhisperX VAD defaults to silero (`-vad` overrides): pyannote's checkpoint trips newer torch's `weights_only` load and aborts every run; found in the first live end-to-end session on the target Mac. -- 2026-07-18 — Sanitise the finding `id` and verdict fields (`value`/`of`/`at`) - through `SafeText` when rendered to `report.md` and the review terminal: a - shared session's `findings.jsonl` is not revalidated by `analyze.Load`, so - those channels could still inject forged report structure / ANSI. Residual of - the earlier control-byte hardening, caught by a confirmation hunt. -- 2026-07-18 — Third hardening pass (confirmation hunt): `review.describe`'s - verdict echo now `SafeText`s the id/verdict fields (the sibling of the fix - above, on the record path); `SafeText` also strips the Unicode BiDi/isolate - and line-separator controls (Trojan-Source, CVE-2021-42574); and `validate` - caps a finding's evidence at 64 ids, so a hostile answer cannot write a single - findings.jsonl line larger than the JSONL reader's buffer and brick the file. - 2026-07-17 — `record` uses ffmpeg avfoundation for both screen and microphone capture, not `screencapture -v`: ffmpeg is already a hard dependency (mic + transcribe), its SIGINT→finalise-container behaviour is battle-tested and @@ -73,6 +62,17 @@ Architecture-shaping decisions graduate to an ADR under (confirmed, unverified, duplicate, rejected). Flagged divergences from the note: task-boundary chunking is deferred behind a seam (timeline carries no task markers), and keyframe extraction (AC3) is deferred to a later intent. +- 2026-07-18 — Sanitise the finding `id` and verdict fields (`value`/`of`/`at`) + through `SafeText` when rendered to `report.md` and the review terminal: a + shared session's `findings.jsonl` is not revalidated by `analyze.Load`, so + those channels could still inject forged report structure / ANSI. Residual of + the earlier control-byte hardening, caught by a confirmation hunt. +- 2026-07-18 — Third hardening pass (confirmation hunt): `review.describe`'s + verdict echo now `SafeText`s the id/verdict fields (the sibling of the fix + above, on the record path); `SafeText` also strips the Unicode BiDi/isolate + and line-separator controls (Trojan-Source, CVE-2021-42574); and `validate` + caps a finding's evidence at 64 ids, so a hostile answer cannot write a single + findings.jsonl line larger than the JSONL reader's buffer and brick the file. - 2026-07-18 — Security hardening (harden branch). Demo capture server: binds loopback by default (a bare `:port` normalises to `127.0.0.1:port`, opt into a wider bind with an explicit host); the write endpoints now require a loopback @@ -850,3 +850,39 @@ Architecture-shaping decisions graduate to an ADR under new regression test. Both reviewers' verdicts were BLOCK, so per the loop's merge gate the PR (#48) stays open for the human rather than auto-merging, even with the fix pushed and CI green. +- 2026-08-07 — Bug-hunt round 33: one confirmed defect (two readers), two + nitpicks. `session.ReadJSONL` capped a single line at `MaxJSONLLine` but + never the whole file, contradicting the comment on `maxManifestBytes` that + listed it as an already-bounded sibling reader; a file built from many + small, individually-legal lines defeated the per-line cap and OOM'd + `merge`, `report`, and `analyze`. New `session.MaxJSONLBytes` (16 MiB, + matching `analyze.Ingest`'s existing cap) bounds the running total in + `ReadJSONL`, with a matching `WriteJSONL` pre-flight check for its two + actual callers (transcript.jsonl, timeline.jsonl). Post-hoc adversarial + review of the round's own PR (correctness; docs accuracy) independently + caught that `analyze.ParseRecords` — findings.jsonl's own scanner, never + routed through `ReadJSONL` — carried the identical gap and was missed by + the round's own comment claiming every sibling reader was already bounded; + `ParseRecords` now enforces `MaxJSONLBytes` too, and the comments on + `maxManifestBytes`/`WriteJSONL` were corrected to name the readers and + writers accurately (`WriteJSONL` never writes findings.jsonl; + `analyze.commitFindings`/`review.AppendVerdict` do, through their own + locked descriptors). Nitpicks fixed: `AGENTS.md` claimed CI runs both the + plain and race-enabled `go test` lines, but CI only runs the race-enabled + one (no test differs between them); two 2026-07-18 `DECISIONS.md` entries + had been spliced ahead of a run of 2026-07-17 entries they were committed + after, breaking the file's own "newest last" rule by both date and commit + order, and were moved back. Reverted before merge: `report.eventLine`'s + `orDash` wrapper was initially removed as unreachable dead code, but round + 28 (2026-08-05, above) had already considered and explicitly rejected this + exact claim as a deliberate locally-redundant guard against a future + caller invariant change — the same rationale `review.go`'s `checkTargets` + states for its own SafeText calls. An adversarial PR reviewer caught the + reintroduction before merge; the wrapper stays. Refuted: unbounded + error-accumulation in `analyze.Ingest`/`errors.Join` "defeating" + `maxAnswerBytes` (one refuter showed the same OOM reproduces from + `json.Unmarshal` alone before a single error accumulates, and a realistic + degenerate answer stays around 255 MB, well within bounds — split verdict, + discarded); a dangling-link nitpick in `AGENTS.md`'s abcd-managed fence + (split verdict on whether `.abcd/rules.json` gives an indirect fix path; + discarded as out of scope). diff --git a/AGENTS.md b/AGENTS.md index 45c0f83..6dd6d64 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,8 +79,9 @@ sh -n install.sh && bash -n install.sh # installer syntax ``` CI (`.github/workflows/ci.yml`) runs every gate above (the single-test example -line is illustrative, not a gate) on every push and pull request, plus checks -with no local command above: installer flag-handling +line is illustrative, not a gate; the race-enabled test line supersedes the +plain one, so CI runs only that one) on every push and pull request, plus +checks with no local command above: installer flag-handling tests (`--help`/`--dir`/`--version`/`--bogus`), a compile-only cross-check for the other release platforms, a version-stamp ldflags check, a full-history `gitleaks` secret scan, and a `zizmor` workflow-security audit. diff --git a/internal/analyze/analyze.go b/internal/analyze/analyze.go index bdd9dc4..75a7cec 100644 --- a/internal/analyze/analyze.go +++ b/internal/analyze/analyze.go @@ -124,9 +124,20 @@ func ParseRecords(r io.Reader, name string) ([]Finding, []Verdict, error) { sc := bufio.NewScanner(r) sc.Buffer(make([]byte, 0, 64*1024), session.MaxJSONLLine) line := 0 + var total int64 for sc.Scan() { line++ raw := sc.Bytes() + // A per-line cap alone leaves the file's total size unbounded: a + // hand-edited or exchanged findings.jsonl built from many small, + // individually-legal lines would otherwise drive this loop's per-line + // allocation (findings and verdicts both accumulate into slices) well + // past the bytes on disk, mirroring the amplification session.ReadJSONL + // guards against for its own callers. + total += int64(len(raw)) + 1 + if total > session.MaxJSONLBytes { + return nil, nil, fmt.Errorf("%s: exceeds %d bytes across %d lines; refusing to read", name, session.MaxJSONLBytes, line) + } if len(bytes.TrimSpace(raw)) == 0 { continue } diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index 6e62e7d..65907d0 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -1,6 +1,7 @@ package analyze import ( + "bytes" "errors" "fmt" "os" @@ -372,6 +373,32 @@ func TestLoadRejectsNullLine(t *testing.T) { } } +// TestLoadRejectsOversizedTotal covers the file-size hole a per-line cap alone +// leaves open: ParseRecords scans findings.jsonl (attacker-controllable once a +// session is exchanged) with a bufio.Scanner capped at session.MaxJSONLLine +// per line, but nothing previously bounded how many lines the file may hold. +// A file built from many small, individually-legal lines used to buffer +// without limit into the findings/verdicts slices, mirroring the +// amplification session.ReadJSONL guards against for its own callers. +// ParseRecords now caps the running total at session.MaxJSONLBytes, matching +// ReadJSONL's stance. +func TestLoadRejectsOversizedTotal(t *testing.T) { + dir := t.TempDir() + // {"kind":"verdict"}\n is 19 bytes; comfortably over MaxJSONLBytes in total. + line := []byte(`{"kind":"verdict"}` + "\n") + var buf bytes.Buffer + for buf.Len() <= session.MaxJSONLBytes { + buf.Write(line) + } + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), buf.Bytes(), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + _, _, err := Load(dir) + if err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("expected an oversize-total refusal, got %v", err) + } +} + func TestIngestUnknownRubric(t *testing.T) { dir := writeSession(t, timelineFixture) _, err := Ingest(dir, strings.NewReader(`{"rubric":"testimony-analysis/v99","findings":[]}`)) diff --git a/internal/session/session.go b/internal/session/session.go index 1be6c93..4e8fd8c 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -131,13 +131,16 @@ func (m Manifest) T0() (int64, error) { // maxManifestBytes caps LoadManifest's read of manifest.json. A genuine manifest // is a few hundred bytes; 1 MiB is generous for one carrying long notes or a big // task list. The cap matters because manifest.json in an exchanged session is -// attacker-controllable (see Manifest.T0's threat note), and it is the last -// untrusted read on the session surface that was still unbounded: an attacker -// ships a multi-gigabyte manifest (a few KB once zipped) and any command that -// loads it — merge, report, analyze, transcribe — would otherwise buffer the -// whole file into memory before parsing and drive the process into OOM. Every -// sibling reader of untrusted session files is already bounded (ReadJSONL caps a -// line at MaxJSONLLine, analyze.Ingest at maxAnswerBytes, the demo body caps). +// attacker-controllable (see Manifest.T0's threat note), and an attacker ships +// a multi-gigabyte manifest (a few KB once zipped) that any command loading it +// — merge, report, analyze, transcribe — would otherwise buffer into memory +// before parsing and drive the process into OOM. Every sibling reader of an +// untrusted session's JSONL files is bounded the same way: ReadJSONL caps both +// a line (MaxJSONLLine) and the whole file (MaxJSONLBytes), and +// analyze.ParseRecords — findings.jsonl's own scanner, not routed through +// ReadJSONL — carries the same pair of caps. analyze.Ingest caps the untrusted +// answer it validates at maxAnswerBytes, and the demo body caps what it +// accepts at capture time; neither reads a session's own JSONL files back. const maxManifestBytes = 1 << 20 // 1 MiB // LoadManifest reads manifest.json from dir. @@ -409,11 +412,26 @@ func SafeTextLines(s string) string { // MaxJSONLLine is the largest single JSONL record the readers accept. It is the // shared read-side invariant every writer must respect: a record persisted above -// this size is durably unreadable, breaking merge, report, and analyze for the +// this size is durably unreadable, breaking merge, report, or analyze for the // whole session, so the capture endpoints reject anything larger rather than // accept a line no reader can take back. const MaxJSONLLine = 4 << 20 // 4 MiB +// MaxJSONLBytes caps a JSONL reader's total read across every line in a file, +// the counterpart to MaxJSONLLine bounding a single one. A per-line cap alone +// leaves total file size unbounded: a session's JSONL artefacts are +// attacker-controllable when exchanged (see ReadJSONL's no-follow comment), +// and a file built from many small, well-formed lines defeats MaxJSONLLine +// while still driving json.Unmarshal's per-line allocation into hundreds of +// megabytes for a file only tens of megabytes on disk. Both ReadJSONL and +// analyze.ParseRecords (findings.jsonl's own scanner) enforce it. 16 MiB +// matches the cap analyze.Ingest already applies to untrusted input at the +// same scale; a genuine session's timeline, interactions, transcript, or +// findings file is a small fraction of that. events.rrweb.jsonl is archival +// only — no command reads it back through either scanner, so this cap does +// not bound it. +const MaxJSONLBytes = 16 << 20 // 16 MiB + // jsonlEncoder returns a json.Encoder configured exactly as WriteJSONL's own // encoders are, so a size measured against it predicts what WriteJSONL will // later check and write. HTML escaping is disabled: JSONL artefacts are never @@ -455,12 +473,22 @@ func ReadJSONL[T any](path string) ([]T, error) { defer f.Close() var out []T + var total int64 sc := bufio.NewScanner(f) sc.Buffer(make([]byte, 0, 64*1024), MaxJSONLLine) line := 0 for sc.Scan() { line++ raw := sc.Bytes() + // Counted before the blank-line skip so a file padded with blank lines + // past the cap is refused rather than scanned past forever. bufio.ScanLines + // strips a trailing \r along with the \n, so a CRLF file is undercounted by + // one byte per line against what is actually on disk — benign here, since + // the extra byte is never itself decoded or retained. + total += int64(len(raw)) + 1 + if total > MaxJSONLBytes { + return nil, fmt.Errorf("%s: exceeds %d bytes across %d lines; refusing to read", path, MaxJSONLBytes, line) + } // Skip blank lines, including whitespace-only ones (as may appear in a // hand-edited or exchanged session), matching analyze.Load so the two // JSONL readers agree on what counts as blank. @@ -484,21 +512,28 @@ func ReadJSONL[T any](path string) ([]T, error) { // from an untrusted, downloaded session directory — cannot be redirected to an // arbitrary file outside the session. // -// It also holds the writers to MaxJSONLLine, the read-side invariant: without -// the check merge could persist a timeline.jsonl (or analyze a findings.jsonl) -// carrying a record longer than ReadJSONL can scan back, report success, and -// leave the operator with an artefact its own reader — and every later merge, -// report, and analyze run over that session — refuses whole. The whole set is -// measured before the file is opened, so a refusal neither truncates an -// existing artefact nor leaves a prefix of the new one behind, matching the -// all-or-nothing stance of analyze.Ingest and demo.appendRecords. That costs a -// second encoding pass over records that are small structs; a durably -// unreadable session is the worse trade. +// It also holds its two callers — transcribe (transcript.jsonl) and merge +// (timeline.jsonl) — to the read-side invariants ReadJSONL enforces: without +// the checks below, either could persist a record longer than MaxJSONLLine, or +// a set totalling more than MaxJSONLBytes, that ReadJSONL can never scan back, +// report success, and leave the operator with an artefact its own reader — and +// every later merge, report, and analyze run over that session — refuses +// whole. The whole set is measured before the file is opened, so a refusal +// neither truncates an existing artefact nor leaves a prefix of the new one +// behind, matching the all-or-nothing stance of analyze.Ingest and +// demo.appendRecords. That costs a second encoding pass over records that are +// small structs; a durably unreadable session is the worse trade. +// +// findings.jsonl never passes through here: analyze.commitFindings and +// review.AppendVerdict write it through their own locked descriptors, so +// ParseRecords (its read side) enforces MaxJSONLLine/MaxJSONLBytes without a +// matching write-side pre-flight check. func WriteJSONL[T any](path string, values []T) error { // Encode into one reusable buffer so the pre-flight pass holds a single // record, not the whole file, in memory. var buf bytes.Buffer check := jsonlEncoder(&buf) + var total int64 for i, v := range values { buf.Reset() if err := check.Encode(v); err != nil { @@ -521,6 +556,14 @@ func WriteJSONL[T any](path string, values []T) error { // no line of any file, and no line of the source transcript either. return fmt.Errorf("%s: line %d of the output encodes to %d bytes, over the %d-byte JSONL line limit", path, i+1, buf.Len(), MaxJSONLLine) } + total += int64(buf.Len()) + if total > MaxJSONLBytes { + // Same write-before-read stance as MaxJSONLLine's check above and + // SaveManifest's own cap: refuse before opening the file rather than + // persist a timeline.jsonl or transcript.jsonl that ReadJSONL's matching + // total-size cap would then refuse to read back. + return fmt.Errorf("%s: output would be %d bytes across %d lines, over the %d-byte JSONL file limit ReadJSONL enforces; refusing to write a session no command could read back", path, total, i+1, MaxJSONLBytes) + } } f, err := OpenFileNoFollow(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o644) diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 8951716..0fa0408 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -1,6 +1,7 @@ package session import ( + "bufio" "bytes" "errors" "os" @@ -347,6 +348,45 @@ func TestWriteJSONLRefusalLeavesNoPartialFile(t *testing.T) { } } +// TestWriteJSONLRefusesOversizedTotal is the write-side half of +// TestReadJSONLRefusesOversizedTotal: a set of records whose total would +// exceed MaxJSONLBytes must be refused before the file is opened, matching +// SaveManifest's write-before-read stance — a session whose timeline.jsonl or +// transcript.jsonl exceeds ReadJSONL's total-size cap can never be read back +// by merge, report, or analyze. Records are padded well under MaxJSONLLine +// rather than minimal, so the set crosses the total-size cap in a few +// thousand records instead of millions. +func TestWriteJSONLRefusesOversizedTotal(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + if err := WriteJSONL(path, []map[string]string{{"actor": "Alice"}}); err != nil { + t.Fatalf("seed write: %v", err) + } + + // {"v":"<4000 x's>"}\n is ~4010 bytes; enough records push the total past + // MaxJSONLBytes. + pad := strings.Repeat("x", 4000) + values := make([]map[string]string, MaxJSONLBytes/4010+10) + for i := range values { + values[i] = map[string]string{"v": pad} + } + err := WriteJSONL(path, values) + if err == nil { + t.Fatal("WriteJSONL persisted a set over MaxJSONLBytes in total; want refusal") + } + if !strings.Contains(err.Error(), "JSONL file limit") { + t.Errorf("error does not name the file-size limit: %v", err) + } + + // The earlier artefact is intact: the refused write never opened the file. + got, err := ReadJSONL[map[string]string](path) + if err != nil { + t.Fatalf("ReadJSONL after refusal: %v", err) + } + if len(got) != 1 || got[0]["actor"] != "Alice" { + t.Fatalf("refused write disturbed the existing artefact: %v", got) + } +} + // TestWriteJSONLDoesNotEscapeHTML pins WriteJSONL's encoder to the same // non-escaping behaviour compactLine (demo's capture-side line canonicaliser) // already has: JSONL artefacts are never embedded in HTML, so escaping <, >, @@ -508,6 +548,64 @@ func TestReadJSONLPlainFileStillWorks(t *testing.T) { } } +// TestReadJSONLRefusesOversizedTotal covers the file-size hole MaxJSONLLine +// left open: a per-line cap bounds one record but not how many a file may +// hold. A session's JSONL artefacts are attacker-controllable when exchanged, +// so a file built from many small, individually-legal lines used to buffer +// without limit into out, driving json.Unmarshal's per-line allocation well +// past the bytes on disk. ReadJSONL now caps the running total at +// MaxJSONLBytes and refuses anything larger, matching LoadManifest's stance +// on manifest.json. Lines are padded well under MaxJSONLLine rather than +// minimal, so the file crosses the total-size cap in a few thousand lines +// instead of millions — the cap counts bytes, not lines, so this exercises +// the same running-total check with far less test overhead. +func TestReadJSONLRefusesOversizedTotal(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + f, err := os.Create(path) + if err != nil { + t.Fatalf("create: %v", err) + } + w := bufio.NewWriter(f) + line := []byte(`{"a":"` + strings.Repeat("x", 4000) + `"}` + "\n") + for total := 0; total <= MaxJSONLBytes; total += len(line) { + if _, err := w.Write(line); err != nil { + t.Fatalf("write: %v", err) + } + } + if err := w.Flush(); err != nil { + t.Fatalf("flush: %v", err) + } + if err := f.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + _, err = ReadJSONL[map[string]any](path) + if err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("expected an oversize-total refusal, got %v", err) + } +} + +// TestReadJSONLAcceptsOrdinaryTotal guards the ordinary case +// TestReadJSONLRefusesOversizedTotal's cap must not break: a normal, +// many-but-small-line file still reads back in full. +func TestReadJSONLAcceptsOrdinaryTotal(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + values := make([]map[string]int, 1000) + for i := range values { + values[i] = map[string]int{"n": i} + } + if err := WriteJSONL(path, values); err != nil { + t.Fatalf("WriteJSONL: %v", err) + } + got, err := ReadJSONL[map[string]int](path) + if err != nil { + t.Fatalf("ReadJSONL: %v", err) + } + if len(got) != len(values) { + t.Fatalf("got %d entries, want %d", len(got), len(values)) + } +} + // TestLoadManifestPlainFileStillWorks confirms the manifest read guard leaves an // ordinary regular-file manifest.json readable. func TestLoadManifestPlainFileStillWorks(t *testing.T) {