From 04525d70b83c77737779ff7f93faab037b851777 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:04:31 +0000 Subject: [PATCH 1/8] fix: bound ReadJSONL/WriteJSONL total file size, not just per-line ReadJSONL capped a single line at MaxJSONLLine but the whole file was unbounded, contradicting the comment on maxManifestBytes that lists it as an already-bounded sibling reader. A session's JSONL artefacts are attacker-controllable when exchanged, and a file built from many small, individually-legal lines defeated the per-line cap while driving json.Unmarshal's per-line allocation well past the bytes on disk, OOMing merge, report, and analyze. Add maxJSONLBytes (16 MiB, matching analyze.Ingest's existing cap for untrusted input at the same scale) as a running-total check in ReadJSONL, and a matching pre-flight check in WriteJSONL so a set of records that ReadJSONL could not read back is refused before the file is opened, preserving the existing write-before-read invariant. Assisted-by: Claude:claude-sonnet-5 --- internal/session/session.go | 42 ++++++++++++--- internal/session/session_test.go | 87 ++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 7 deletions(-) diff --git a/internal/session/session.go b/internal/session/session.go index 1be6c93..200a37a 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -131,13 +131,13 @@ 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 +// untrusted session files is bounded the same way: ReadJSONL caps both a line +// (MaxJSONLLine) and the whole file (maxJSONLBytes), analyze.Ingest caps at +// maxAnswerBytes, the demo body caps. const maxManifestBytes = 1 << 20 // 1 MiB // LoadManifest reads manifest.json from dir. @@ -414,6 +414,18 @@ func SafeTextLines(s string) string { // accept a line no reader can take back. const MaxJSONLLine = 4 << 20 // 4 MiB +// maxJSONLBytes caps ReadJSONL'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. 16 MiB matches the cap +// analyze.Ingest already applies to untrusted input at the same scale; a +// genuine session's merged timeline or findings file is a small fraction of +// that. +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 +467,19 @@ 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. + 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. @@ -499,6 +518,7 @@ func WriteJSONL[T any](path string, values []T) error { // 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 +541,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 findings.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..c24dd50 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -347,6 +347,41 @@ func TestWriteJSONLRefusalLeavesNoPartialFile(t *testing.T) { } } +// TestWriteJSONLRefusesOversizedTotal is the write-side half of +// TestReadJSONLRefusesOversizedTotal: a set of individually-small 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 findings.jsonl exceeds ReadJSONL's total-size cap can +// never be read back by merge, report, or analyze. +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) + } + + // {"n":0}\n is 8 bytes; enough records push the total past maxJSONLBytes. + values := make([]map[string]int, maxJSONLBytes/8+1000) + for i := range values { + values[i] = map[string]int{"n": 0} + } + 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 +543,58 @@ 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. +func TestReadJSONLRefusesOversizedTotal(t *testing.T) { + path := filepath.Join(t.TempDir(), TimelineFile) + f, err := os.Create(path) + if err != nil { + t.Fatalf("create: %v", err) + } + // {"a":1}\n is 8 bytes; comfortably over maxJSONLBytes in total. + line := []byte("{\"a\":1}\n") + for total := 0; total <= maxJSONLBytes; total += len(line) { + if _, err := f.Write(line); err != nil { + t.Fatalf("write: %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) { From ceca72f08953441b14a29280271dc2dc7c57860d Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:04:35 +0000 Subject: [PATCH 2/8] refactor: remove dead orDash wrapper from report.eventLine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit eventLine's parts always starts with mdOrDash(raw("kind")), which is at minimum the "—" placeholder, so the join it feeds into orDash can never be empty and the wrapper's fallback branch is unreachable (66.7% coverage on a 3-statement function). orDash had no other call site. Assisted-by: Claude:claude-sonnet-5 --- internal/report/report.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/internal/report/report.go b/internal/report/report.go index ae670bc..12da0a7 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -426,14 +426,9 @@ func eventLine(e timeline.Entry) string { if r := raw("route"); !inlineRendersEmpty(r) { parts = append(parts, "("+mdInline(r)+")") } - return orDash(strings.Join(parts, " ")) -} - -func orDash(s string) string { - if s == "" { - return "—" - } - return s + // parts always holds at least mdOrDash(raw("kind")), which is "—" at + // minimum, so the join is never empty and needs no further dash fallback. + return strings.Join(parts, " ") } // mdOrDash renders s inline, falling back to "—" when it is empty or renders From 0d57e9d5b671d5e898717f4de288b07b01602070 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:04:39 +0000 Subject: [PATCH 3/8] docs: note CI runs only the race-enabled test line AGENTS.md's "Build, test, and checks" section claims CI runs "every gate above" except the illustrative single-test example, but .github/workflows/ci.yml only runs `go test -race ./...`, superseding the plain `go test ./...` line listed just above it. No test is race-conditional or skipped between the two, so this is a wording fix, not a coverage gap. CLAUDE.md is a symlink to this file. Assisted-by: Claude:claude-sonnet-5 --- AGENTS.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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. From f5419f7ad61c206d4b142b1e2cca440d53ad920c Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:04:45 +0000 Subject: [PATCH 4/8] docs: restore append order in DECISIONS.md Two 2026-07-18 entries (finding-field sanitisation, the confirmation- hunt hardening pass) were spliced in ahead of a run of 2026-07-17 entries they were committed after, violating the file's own "newest last" rule by both date and commit order. Move them back after the 2026-07-17 block they were inserted into. Assisted-by: Claude:claude-sonnet-5 --- .abcd/work/DECISIONS.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 5d16153..3fe2726 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 From aba4eb5b39d00d5cc7a01a66c8f538e8b326e1e2 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:05:21 +0000 Subject: [PATCH 5/8] docs: record round 33 in DECISIONS.md Assisted-by: Claude:claude-sonnet-5 --- .abcd/work/DECISIONS.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 3fe2726..0ade814 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -850,3 +850,27 @@ 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, three nitpicks. + `session.ReadJSONL` capped a single line at `MaxJSONLLine` but never the + whole file, contradicting the comment on `maxManifestBytes` that lists 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 `maxJSONLBytes` (16 MiB, matching + `analyze.Ingest`'s existing cap) bounds the running total in `ReadJSONL`, + with a matching `WriteJSONL` pre-flight check preserving the + write-before-read invariant. Nitpicks fixed: `report.eventLine`'s + `orDash` wrapper was unreachable (its input always starts with + `mdOrDash`'s own "—" fallback) and had no other call site, so it was + removed; `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. 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). From 0ec4cbadfab7a0172e1f088e4336c8dc75d63777 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:18:35 +0000 Subject: [PATCH 6/8] revert: keep the orDash wrapper in report.eventLine Round 28 already considered and explicitly rejected removing this wrapper for the same reachability argument this round re-raised: it is a deliberate, locally-redundant guard against a future caller invariant change, the same rationale review.go's checkTargets states for its own SafeText calls (recorded in .abcd/work/DECISIONS.md, 2026-08-05). An adversarial PR reviewer caught the reintroduction before merge. Assisted-by: Claude:claude-sonnet-5 --- internal/report/report.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/report/report.go b/internal/report/report.go index 12da0a7..ae670bc 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -426,9 +426,14 @@ func eventLine(e timeline.Entry) string { if r := raw("route"); !inlineRendersEmpty(r) { parts = append(parts, "("+mdInline(r)+")") } - // parts always holds at least mdOrDash(raw("kind")), which is "—" at - // minimum, so the join is never empty and needs no further dash fallback. - return strings.Join(parts, " ") + return orDash(strings.Join(parts, " ")) +} + +func orDash(s string) string { + if s == "" { + return "—" + } + return s } // mdOrDash renders s inline, falling back to "—" when it is empty or renders From a7003c8949d0a82124e61733c518e83a6ef0d923 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:18:50 +0000 Subject: [PATCH 7/8] fix: bound analyze.ParseRecords' total findings.jsonl read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial PR review (both correctness and docs-accuracy lenses) caught that the prior commit's own comment overclaimed: findings.jsonl is read by analyze.ParseRecords, a separate scanner from session.ReadJSONL, and it carried no total-size bound — the same unbounded-total-file class of defect this round set out to close, just in a second reader. Export session.MaxJSONLBytes (was maxJSONLBytes) so ParseRecords can enforce the same running-total cap ReadJSONL now does. Also corrects three now-inaccurate comments the reviewers flagged: maxManifestBytes's sibling-reader claim named session.ReadJSONL alone as bounding every untrusted JSONL file, when findings.jsonl never passes through it; WriteJSONL's invariant comment named findings.jsonl as one of its callers, but WriteJSONL has exactly two call sites (transcript.jsonl, timeline.jsonl) — findings.jsonl is written by analyze.commitFindings and review.AppendVerdict through their own locked descriptors; and a CRLF file undercounts ReadJSONL's running total by one byte per line (benign — the extra byte is never decoded or retained — but the comment claimed exact counting). Also speeds up the two new session tests introduced for the total-size cap: padding lines/records to a few KB each cuts the oversized-total fixtures from ~2.1M elements to a few thousand, an 8x race-mode speedup on the package (measured 25.7s -> ~4s). Assisted-by: Claude:claude-sonnet-5 --- internal/analyze/analyze.go | 11 +++++ internal/analyze/analyze_test.go | 27 ++++++++++++ internal/session/session.go | 71 +++++++++++++++++++------------- internal/session/session_test.go | 41 +++++++++++------- 4 files changed, 107 insertions(+), 43 deletions(-) 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 200a37a..4e8fd8c 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -134,10 +134,13 @@ func (m Manifest) T0() (int64, error) { // 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 -// untrusted session files is bounded the same way: ReadJSONL caps both a line -// (MaxJSONLLine) and the whole file (maxJSONLBytes), analyze.Ingest caps at -// maxAnswerBytes, the demo body caps. +// 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,22 +412,25 @@ 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 ReadJSONL's total read across every line in a file, the -// counterpart to MaxJSONLLine bounding a single one. A per-line cap alone +// 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. 16 MiB matches the cap -// analyze.Ingest already applies to untrusted input at the same scale; a -// genuine session's merged timeline or findings file is a small fraction of -// that. -const maxJSONLBytes = 16 << 20 // 16 MiB +// 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 @@ -475,10 +481,13 @@ func ReadJSONL[T any](path string) ([]T, error) { 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. + // 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) + 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 @@ -503,16 +512,22 @@ 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. @@ -542,12 +557,12 @@ func WriteJSONL[T any](path string, values []T) error { 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 { + 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 findings.jsonl that ReadJSONL's matching + // 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) + 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) } } diff --git a/internal/session/session_test.go b/internal/session/session_test.go index c24dd50..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" @@ -348,25 +349,29 @@ func TestWriteJSONLRefusalLeavesNoPartialFile(t *testing.T) { } // TestWriteJSONLRefusesOversizedTotal is the write-side half of -// TestReadJSONLRefusesOversizedTotal: a set of individually-small 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 findings.jsonl exceeds ReadJSONL's total-size cap can -// never be read back by merge, report, or analyze. +// 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) } - // {"n":0}\n is 8 bytes; enough records push the total past maxJSONLBytes. - values := make([]map[string]int, maxJSONLBytes/8+1000) + // {"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]int{"n": 0} + 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") + 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) @@ -549,21 +554,27 @@ func TestReadJSONLPlainFileStillWorks(t *testing.T) { // 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. +// 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) } - // {"a":1}\n is 8 bytes; comfortably over maxJSONLBytes in total. - line := []byte("{\"a\":1}\n") - for total := 0; total <= maxJSONLBytes; total += len(line) { - if _, err := f.Write(line); err != nil { + 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) } From 0ee4a1cf95eee6144ade52a3a965dd035f330cfd Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:19:23 +0000 Subject: [PATCH 8/8] docs: correct round 33's DECISIONS.md entry to match the merged diff Assisted-by: Claude:claude-sonnet-5 --- .abcd/work/DECISIONS.md | 60 ++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 0ade814..1525c0a 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -850,27 +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, three nitpicks. - `session.ReadJSONL` capped a single line at `MaxJSONLLine` but never the - whole file, contradicting the comment on `maxManifestBytes` that lists 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 `maxJSONLBytes` (16 MiB, matching - `analyze.Ingest`'s existing cap) bounds the running total in `ReadJSONL`, - with a matching `WriteJSONL` pre-flight check preserving the - write-before-read invariant. Nitpicks fixed: `report.eventLine`'s - `orDash` wrapper was unreachable (its input always starts with - `mdOrDash`'s own "—" fallback) and had no other call site, so it was - removed; `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. 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). +- 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).