From 136305594cc2d16aa2ae71bce8255a27b3809461 Mon Sep 17 00:00:00 2001 From: Alex Reppel <77722411+REPPL@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:20:58 +0100 Subject: [PATCH 1/6] =?UTF-8?q?Harden=20the=20capture=E2=86=92analysis=20p?= =?UTF-8?q?ipeline=20against=20untrusted=20sessions=20(bughunt-7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarially-verified fixes from the bughunt-7 security review. Each carries a regression test verified failing against the pre-fix code; the full gate (build, gofmt, go vet, go test -race, and the merge/report pipeline smoke) is green. Concurrency & durability of the verdict record (findings.jsonl): - Close the Ingest/AppendVerdict TOCTOU: Ingest now runs its verdict-guard probe, truncate, and rewrite as one step under an exclusive flock (commitFindings), matching the lock AppendVerdict holds, so a concurrent re-ingest can no longer destroy a committed verdict. - Bind each verdict to the finding the analyst judged: AppendVerdict re-verifies under its lock that the targeted id still names the same finding, so a concurrent re-ingest that reuses an id cannot misattribute the verdict. Untrusted time-magnitude class: - Bound utterance t0/t1 magnitude in timeline.checkedUtterances (the speech-side twin of the interaction range guard). - Guard report.clock and review.clock against non-finite / out-of-range seconds before the float64→int conversion, defending the display sink for a hand-authored timeline.jsonl/findings.jsonl that bypasses merge. - Replace report's finite 1e18 standalone-event flush sentinel with +Inf, which had silently dropped events at or past it. Shown-vs-validated consistency: - EmitRequest sanitises each emitted timeline line, so analyze.validate now indexes and compares in SafeText form (ids, utterance text, selectors, routes, quotes); an honest verbatim-copied answer over control/bidi-bearing text is no longer rejected as "not present". Untrusted-read bounds and disclosure: - Cap LoadManifest at 1 MiB; hold AppendVerdict to MaxJSONLLine; guard the read-side FIFO on holdsVerdicts; keep the raw session path out of report.md's findings-unavailable notice. record / transcribe: - transcribe persists an external recording's audio→session offset in an audio.offset.json sidecar, so a bare re-run recovers it instead of silently assuming 0; a malformed sidecar refuses with guidance. - record captures the avfoundation ":default" microphone (not enumerated index 0) and logs the detected input roster, so a virtual audio driver is not silently recorded in the real mic's place. (avfoundation capture is not exercised by CI; verify on macOS hardware before release.) - probeDevices surfaces ffmpeg's own diagnostic when the device listing is empty; surface a device-listing run failure rather than misreporting "no microphone found". Assisted-by: Claude:claude-fable-5 --- .abcd/work/DECISIONS.md | 58 ++++++++++++ docs/reference/session-directory.md | 10 ++ internal/analyze/analyze.go | 35 +++++-- internal/analyze/analyze_test.go | 121 ++++++++++++++++++++++++ internal/analyze/emit.go | 12 ++- internal/analyze/ingest.go | 96 +++++++++++++------ internal/analyze/validate.go | 45 ++++++--- internal/demo/demo.go | 9 +- internal/record/record.go | 15 ++- internal/record/record_test.go | 50 +++++++--- internal/record/recorders.go | 96 ++++++++++++++----- internal/report/report.go | 36 ++++++- internal/report/report_test.go | 108 +++++++++++++++++++++ internal/review/review.go | 125 +++++++++++++++++++++++-- internal/review/review_test.go | 115 ++++++++++++++++++++++- internal/session/session.go | 20 +++- internal/session/session_test.go | 44 +++++++++ internal/timeline/timeline.go | 44 ++++++++- internal/timeline/timeline_test.go | 120 ++++++++++++++++++++++++ internal/transcribe/transcribe.go | 86 +++++++++++++++++ internal/transcribe/transcribe_test.go | 92 ++++++++++++++++++ internal/transcribe/whispercpp.go | 12 ++- 22 files changed, 1245 insertions(+), 104 deletions(-) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 3aae7dd..5d54cff 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -186,3 +186,61 @@ Architecture-shaping decisions graduate to an ADR under it used the zero-value anchor and wrote a silently corrupt timeline (~55-year offsets, nonsense report duration) with exit 0. Transcript-only sessions are unaffected. +- 2026-07-22 — Close the Ingest/AppendVerdict TOCTOU on `findings.jsonl`: + `Ingest` now runs its verdict-guard probe, `O_TRUNC`, and rewrite as one step + under a `LOCK_EX` advisory lock (`commitFindings`), matching the lock + `review.AppendVerdict` already holds. Previously the probe and the truncating + write were two lock-free opens, so a concurrent `testimony review` could commit + a verdict between them and have the re-ingest destroy it — the human-decision + record the guard exists to protect. Sibling sweep: `findings.jsonl` is the only + session file with both an appender and a truncating rewriter; the other + `WriteJSONL` sites (timeline, transcript) are truncate-only, no sibling. +- 2026-07-22 — Bind each review verdict to the finding the analyst judged. + `review.AppendVerdict` now takes the shown finding and, under its existing + `LOCK_EX` on findings.jsonl, re-reads the current findings and refuses if the + targeted id is gone or now names a different finding (`verifyTarget`, + `analyze.SameIdentity`). Previously the walk validated the target only against + the in-memory snapshot from `analyze.Load`, so a concurrent `analyze -ingest` + (permitted until the first verdict exists) that restarts finding ids at F-001 + could slide a different finding under the same id and misattribute the + operator's verdict — silent corruption of the precision record. Refactored + `analyze.Load` to expose `ParseRecords(io.Reader)` so the re-check reads through + the already-locked descriptor. Sibling sweep: `AppendVerdict` is the sole + verdict writer; both production callers now bind the judged finding. +- 2026-07-22 — Guard the untrusted-time-magnitude class end to end. Utterance + t0/t1 (session-relative float64 seconds) now have a magnitude bound in + timeline.checkedUtterances (1e9 s), the speech-side twin of the interaction + t<=0 guard; report.clock and review.clock refuse non-finite / out-of-range + seconds before the float64→int conversion (rendering `--:--`), defending the + display sink for a hand-authored timeline.jsonl/findings.jsonl that bypasses + merge; and report's trailing standalone-event flush uses a +Inf sentinel + instead of a finite 1e18 that silently dropped events at/after it. Sibling + sweep: report.clock and review.clock were the two duplicated float→int time + sinks; both guarded. +- 2026-07-22 — Validate findings against the SafeText form of the timeline. + EmitRequest shows the agent each timeline line through session.SafeText, but + analyze.validate indexed and compared the raw bytes, so an id/selector/route/ + quote carrying a stripped Bidi_Control or control character could never be + matched by an honest verbatim-copied answer (fail-closed, hostile/hand-edited + transcripts and genuine RTL speech). indexTimeline now stores SafeText keys and + validate compares SafeText(quote/selector/route/id). SafeText is a no-op on + ordinary text. Resolves the selector/route/id sibling of the earlier + emit-quote asymmetry. +- 2026-07-22 — Surface ffmpeg's own diagnostic when the avfoundation device + listing is empty (record.probeDevices): an ffmpeg built without avfoundation + exits non-zero as an *exec.ExitError with the cause in its output, previously + discarded and misreported as "no microphone found". Now a bounded output tail + is appended to the error. +- 2026-07-22 — transcribe persists the audio→session offset for external + recordings in a sidecar (`audio.offset.json`) beside audio.wav. A bare re-run + (different model, reusing audio.wav) reads it back instead of silently assuming + offset 0 and shifting every utterance; a record-origin audio.wav has no sidecar + and stays offset 0; a present-but-unusable sidecar refuses with guidance rather + than guessing. New session-directory artefact, documented in + docs/reference/session-directory.md. +- 2026-07-22 — record captures the microphone via avfoundation ":default" (the + system default input) instead of enumerated index 0, so a virtual audio driver + (BlackHole/Loopback/conferencing device) that enumerates first is no longer + silently recorded in the real mic's place; startRecorders logs the detected + audio-input roster so a surprising default stays visible. NOTE: the avfoundation + capture path is not exercised by CI — verify on macOS hardware before release. diff --git a/docs/reference/session-directory.md b/docs/reference/session-directory.md index 5047dfc..d36c656 100644 --- a/docs/reference/session-directory.md +++ b/docs/reference/session-directory.md @@ -6,6 +6,7 @@ Every capture session lives in one directory (by default under `sessions/`): sessions// manifest.json # session metadata, including t0_epoch_ms (written by demo) audio.wav # 16 kHz mono ASR input (written by transcribe; local only) + audio.offset.json # audio→session offset for an external recording (written by transcribe; local only) events.rrweb.jsonl # raw rrweb stream, archival (written by demo) interactions.jsonl # normalised interaction events (written by demo) transcript.jsonl # time-aligned utterances (written by transcribe) @@ -74,6 +75,15 @@ One utterance per line. Times are session-relative seconds (audio time plus the {"id":"utt-003","t0":16.0,"t1":21.0,"speaker":"P1","text":"Now I expect this save button to confirm somehow.","words":[{"w":"Now","t":17.6},{"w":"I","t":17.92}]} ``` +## `audio.offset.json` + +Written by `transcribe` only when the audio came from an external recording (a `-audio FILE` that is not the session's own `audio.wav`), which is not captured at `t0`. It records the audio→session offset so a later bare `transcribe` (for example, a re-run with a different model that reuses `audio.wav`) recovers the same offset instead of assuming `0`. A session recorded with `testimony record` captures `audio.wav` at `t0` and has no sidecar; its offset is `0`. If the sidecar is present but unreadable or malformed, `transcribe` refuses rather than guess, and asks for an explicit `-audio` or `-offset`. + +| Field | Type | Required | Meaning | +|---|---|---|---| +| `offset_seconds` | number | yes | seconds added to every audio-clock time to place it on the session clock | +| `provenance` | string | no | how the offset was obtained, for the operator | + ## `events.rrweb.jsonl` One raw [rrweb](https://github.com/rrweb-io/rrweb) event per line, exactly as emitted by the recorder (DOM snapshots, incremental mutations, pointer movement). Archival only: nothing downstream reads it; it exists so full session replay stays possible later. diff --git a/internal/analyze/analyze.go b/internal/analyze/analyze.go index 0bdf883..d84cc11 100644 --- a/internal/analyze/analyze.go +++ b/internal/analyze/analyze.go @@ -16,7 +16,9 @@ import ( "bytes" "encoding/json" "fmt" + "io" "path/filepath" + "reflect" "regexp" "github.com/REPPL/Testimony/internal/session" @@ -92,11 +94,21 @@ func Load(dir string) ([]Finding, []Verdict, error) { return nil, nil, err } defer f.Close() + return ParseRecords(f, path) +} +// ParseRecords splits a findings.jsonl stream into finding and verdict records, +// applying the same rules Load documents: blank lines are skipped and a verdict +// carrying an out-of-enum value is ignored rather than applied. name labels +// errors. Load is ParseRecords over the on-disk file opened through the +// no-follow guard; review.AppendVerdict reuses it to re-read the current +// findings through its own already-locked descriptor, so the re-check and the +// append observe the same file under one lock. +func ParseRecords(r io.Reader, name string) ([]Finding, []Verdict, error) { var findings []Finding var verdicts []Verdict - sc := bufio.NewScanner(f) - sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) + sc := bufio.NewScanner(r) + sc.Buffer(make([]byte, 0, 64*1024), session.MaxJSONLLine) line := 0 for sc.Scan() { line++ @@ -108,12 +120,12 @@ func Load(dir string) ([]Finding, []Verdict, error) { Kind string `json:"kind"` } if err := json.Unmarshal(raw, &probe); err != nil { - return nil, nil, fmt.Errorf("%s:%d: %w", path, line, err) + return nil, nil, fmt.Errorf("%s:%d: %w", name, line, err) } if probe.Kind == "verdict" { var v Verdict if err := json.Unmarshal(raw, &v); err != nil { - return nil, nil, fmt.Errorf("%s:%d: %w", path, line, err) + return nil, nil, fmt.Errorf("%s:%d: %w", name, line, err) } // The verdict enum is closed (confirmed|rejected|duplicate). A verdict // carrying any other value — a typo, an empty string, or a foreign @@ -130,16 +142,27 @@ func Load(dir string) ([]Finding, []Verdict, error) { } var fnd Finding if err := json.Unmarshal(raw, &fnd); err != nil { - return nil, nil, fmt.Errorf("%s:%d: %w", path, line, err) + return nil, nil, fmt.Errorf("%s:%d: %w", name, line, err) } findings = append(findings, fnd) } if err := sc.Err(); err != nil { - return nil, nil, fmt.Errorf("%s: %w", path, err) + return nil, nil, fmt.Errorf("%s: %w", name, err) } return findings, verdicts, nil } +// SameIdentity reports whether a and b are the same finding — equal in every +// field a human verdict is recorded against. Status is excluded: it is the one +// field a verdict is meant to change, and Ingest launders it to "unverified" on +// every written finding regardless. review uses it under the append lock to +// confirm a verdict still targets the finding the analyst was shown, rather than +// a different finding a concurrent re-ingest slid under the same id. +func SameIdentity(a, b Finding) bool { + a.Status, b.Status = "", "" + return reflect.DeepEqual(a, b) +} + // EffectiveStatus maps each finding id to its effective status: every finding // starts "unverified"; verdict records are applied in file order and the last // one for an id wins. This single helper is used by both review (to pick the diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index 69a8fff..ed92067 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -5,7 +5,9 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" + "time" "github.com/REPPL/Testimony/internal/session" ) @@ -104,6 +106,34 @@ func TestIngestValidationFailures(t *testing.T) { } } +// TestIngestQuoteValidatesAgainstSanitisedUtterance is the shown-vs-validated +// regression. EmitRequest runs every timeline line through session.SafeText, so +// an utterance whose text carries a Bidi_Control character (here U+200F, common +// in genuine RTL speech) is shown to the agent with that character stripped. The +// agent copies the quote verbatim from the request it was given — i.e. the +// sanitised text — and cites the utterance. validate must therefore compare the +// quote against the sanitised utterance text too. Pre-fix it indexed the raw +// utterance text and the honest, verbatim-copied quote was rejected as "not a +// verbatim substring", an unrepairable failure since the agent can never see the +// stripped character. +func TestIngestQuoteValidatesAgainstSanitisedUtterance(t *testing.T) { + // The raw utterance carries U+200F (RLM) between "save" and "button". Ingest + // reads timeline.jsonl (the merged, raw form). + const tl = "{\"t\":22,\"src\":\"speech\",\"id\":\"utt-004\",\"payload\":{\"speaker\":\"P1\",\"t1\":28,\"text\":\"I clicked the save ‏button and nothing happened\"}}\n" + dir := writeSession(t, tl) + + // The agent's quote is the SafeText'd span — no RLM, because the request never + // showed it one. + answer := `{"findings":[{"id":"F-001","t":22,"type":"bug","severity":3,"quote":"the save button and nothing happened","evidence":["utt-004"]}]}` + findings, err := Ingest(dir, strings.NewReader(answer)) + if err != nil { + t.Fatalf("an honest quote copied from the sanitised request was rejected: %v", err) + } + if len(findings) != 1 { + t.Fatalf("got %d findings, want 1", len(findings)) + } +} + func TestIngestDuplicateID(t *testing.T) { dir := writeSession(t, timelineFixture) dup := `{"findings":[ @@ -545,3 +575,94 @@ func TestIngestOversizedFindingLeavesPriorFileIntact(t *testing.T) { t.Fatalf("a prior findings.jsonl was modified despite the refusal") } } + +// TestIngestGuardAndWriteAreOneLockedStep is the TOCTOU regression for the +// verdict guard. Pre-fix, Ingest probed for verdicts and then rewrote +// findings.jsonl as two separate, lock-free opens, so a concurrent `testimony +// review` could commit a verdict (under review.AppendVerdict's exclusive lock) +// in between and have the O_TRUNC rewrite destroy it — the human-decision +// record the guard exists to protect. Post-fix the probe, truncate, and write +// run under one exclusive flock on findings.jsonl, so Ingest cannot touch the +// file while a verdict writer holds the lock. The test stands in for that +// writer: it takes the lock, starts Ingest, requires Ingest NOT to complete +// while the lock is held (pre-fix it completes and truncates in milliseconds), +// then commits a verdict and releases. Ingest must then see the verdict and +// refuse, leaving it intact. +func TestIngestGuardAndWriteAreOneLockedStep(t *testing.T) { + dir := writeSession(t, timelineFixture) + path := filepath.Join(dir, session.FindingsFile) + const priorFinding = `{"kind":"finding","id":"F-001","t":22,"type":"bug","severity":3,"quote":"I clicked save and nothing happened","evidence":["utt-004"],"status":"unverified"}` + "\n" + if err := os.WriteFile(path, []byte(priorFinding), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + + // Stand-in for a `testimony review` mid-AppendVerdict: same open, same lock. + locked, err := session.OpenFileNoFollow(path, os.O_APPEND|os.O_RDWR, 0o644) + if err != nil { + t.Fatalf("open findings: %v", err) + } + if err := syscall.Flock(int(locked.Fd()), syscall.LOCK_EX); err != nil { + t.Fatalf("flock: %v", err) + } + + done := make(chan error, 1) + go func() { + _, err := Ingest(dir, strings.NewReader(goodAnswer)) + done <- err + }() + + // While the verdict writer holds the lock, Ingest must block. Pre-fix it + // ignores the lock and finishes (truncating the file) within milliseconds, + // so completing inside this grace window is the defect itself. + select { + case err := <-done: + t.Fatalf("Ingest completed while a verdict writer held the lock (err=%v)", err) + case <-time.After(500 * time.Millisecond): + } + + const verdict = `{"kind":"verdict","finding":"F-001","verdict":"confirmed","note":"seen it","at":"2026-07-22"}` + "\n" + if _, err := locked.WriteString(verdict); err != nil { + t.Fatalf("append verdict: %v", err) + } + if err := locked.Close(); err != nil { // releases the lock + t.Fatalf("close: %v", err) + } + + if err := <-done; err == nil || !strings.Contains(err.Error(), "verdict records") { + t.Fatalf("expected the verdict-guard refusal after the lock released, got %v", err) + } + b, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read findings: %v", err) + } + if !strings.Contains(string(b), `"kind":"verdict"`) { + t.Fatalf("the concurrently committed verdict was destroyed:\n%s", b) + } + if !strings.Contains(string(b), priorFinding[:40]) { + t.Fatalf("the prior finding was destroyed:\n%s", b) + } +} + +// TestEmitRequestSanitisesTimelineBidi is the Trojan-Source regression for the +// timeline block. An exchanged session's transcript text flows through merge into +// timeline.jsonl and then into the emitted request that cli prints to the +// operator's terminal. json.Marshal escapes C0 controls and ESC but passes the +// Unicode Bidi_Control set (here U+202E RLO / U+202C PDF) through as raw bytes, so +// pre-fix a right-to-left override reordered the displayed quote — the exact +// spoofing SafeText strips on the report and review paths. EmitRequest must now +// route each marshalled timeline line through SafeText too. +func TestEmitRequestSanitisesTimelineBidi(t *testing.T) { + const bidiTimeline = "{\"t\":22,\"src\":\"speech\",\"id\":\"utt-004\",\"payload\":{\"speaker\":\"P1\",\"t1\":28,\"text\":\"benign ‮gnihtemos evil‬ end\"}}\n" + dir := writeSession(t, bidiTimeline) + got, err := EmitRequest(dir) + if err != nil { + t.Fatalf("EmitRequest: %v", err) + } + if strings.ContainsRune(got, '‮') || strings.ContainsRune(got, '‬') { + t.Fatalf("emitted request carries a raw Bidi_Control character from the timeline text") + } + // The underlying words are unchanged; only the reordering controls are gone. + if !strings.Contains(got, "gnihtemos evil") { + t.Fatalf("sanitised timeline text missing from the emitted request:\n%s", got) + } +} diff --git a/internal/analyze/emit.go b/internal/analyze/emit.go index 7e46115..0fc37df 100644 --- a/internal/analyze/emit.go +++ b/internal/analyze/emit.go @@ -86,8 +86,14 @@ func EmitRequest(dir string) (string, error) { // report and review: without it an App, Participant, or task carrying ESC // drives ANSI sequences in the terminal, and one carrying a newline forges // Markdown structure (a fake "## " heading or extra rubric instructions) inside - // the request the agent is asked to obey. The timeline block below needs no - // such treatment: json.Marshal escapes control bytes on the way out. + // the request the agent is asked to obey. The timeline block below needs the + // same treatment for a narrower reason: json.Marshal escapes the C0 controls + // and ESC, but it passes the Unicode Bidi_Control set through as raw bytes, so + // an exchanged session's transcript or event text could still smuggle a + // Trojan-Source reordering (CVE-2021-42574) into the request printed to the + // operator's terminal — the exact spoofing SafeText strips on the report and + // review paths. Each marshalled line is run through SafeText below; JSON's own + // structural bytes are ASCII and pass through it unchanged. b.WriteString("## Session\n\n") fmt.Fprintf(&b, "- App: %s\n", session.SafeText(orNone(man.App))) fmt.Fprintf(&b, "- Participant: %s\n", session.SafeText(orNone(man.Participant))) @@ -110,7 +116,7 @@ func EmitRequest(dir string) (string, error) { if err != nil { return "", err } - b.Write(line) + b.WriteString(session.SafeText(string(line))) b.WriteByte('\n') } } diff --git a/internal/analyze/ingest.go b/internal/analyze/ingest.go index 8221cc9..e29a778 100644 --- a/internal/analyze/ingest.go +++ b/internal/analyze/ingest.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "syscall" "github.com/REPPL/Testimony/internal/session" "github.com/REPPL/Testimony/internal/timeline" @@ -103,16 +104,67 @@ func Ingest(dir string, r io.Reader) ([]Finding, error) { return nil, errors.Join(errs...) } - if held, err := holdsVerdicts(dir); err != nil { + if err := commitFindings(dir, findings); err != nil { return nil, err - } else if held { - return nil, fmt.Errorf("refusing to overwrite %s: it already holds verdict records (the retained precision record)", session.FindingsFile) } + return findings, nil +} - if err := session.WriteJSONL(filepath.Join(dir, session.FindingsFile), findings); err != nil { - return nil, fmt.Errorf("write findings: %w", err) +// commitFindings runs the verdict guard and the truncating write as one locked +// step. Probing with holdsVerdicts and then calling session.WriteJSONL as two +// separate opens left a TOCTOU window: a concurrent `testimony review` commits a +// verdict (under its own lock, see review.AppendVerdict) between the probe and +// the O_TRUNC open, and the rewrite destroys it — precisely the human-decision +// record the guard exists to protect. Holding one exclusive advisory lock across +// probe, truncate, and write forecloses the interleaving: AppendVerdict blocks +// until the commit completes, so a verdict is either visible to the probe (and +// the re-ingest refused) or appended after the new findings. The findings were +// already held to session.MaxJSONLLine by oversizedFindings, so writing through +// the locked descriptor keeps the read-side invariant WriteJSONL enforces. +func commitFindings(dir string, findings []Finding) error { + path := filepath.Join(dir, session.FindingsFile) + f, err := session.OpenFileNoFollow(path, os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return err } - return findings, nil + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + f.Close() + return err + } + held, err := holdsVerdicts(f, path) + if err != nil { + f.Close() + return err + } + if held { + f.Close() + return fmt.Errorf("refusing to overwrite %s: it already holds verdict records (the retained precision record)", session.FindingsFile) + } + if err := f.Truncate(0); err != nil { + f.Close() + return fmt.Errorf("write findings: %w", err) + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + f.Close() + return fmt.Errorf("write findings: %w", err) + } + w := bufio.NewWriter(f) + enc := json.NewEncoder(w) + for _, v := range findings { + if err := enc.Encode(v); err != nil { + f.Close() + return fmt.Errorf("write findings: %w", err) + } + } + if err := w.Flush(); err != nil { + f.Close() + return fmt.Errorf("write findings: %w", err) + } + // Close releases the lock with the descriptor. + if err := f.Close(); err != nil { + return fmt.Errorf("write findings: %w", err) + } + return nil } // oversizedFindings reports any finding whose findings.jsonl line — its JSON @@ -227,27 +279,17 @@ func decodeFinding(raw json.RawMessage) (Finding, error) { }, nil } -// holdsVerdicts reports whether an existing findings.jsonl already contains any -// verdict record. A missing file is not an error (false, nil). It scans for raw -// kind:"verdict" lines rather than reusing analyze.Load, whose verdict slice is -// filtered to the closed enum (confirmed|rejected|duplicate): a hand-edited or -// shared file whose only verdict lines carry a foreign or typo'd value would -// otherwise slip past the guard and have its human-decision records truncated by -// a re-ingest — exactly the precision history the guard exists to protect. -func holdsVerdicts(dir string) (bool, error) { - path := filepath.Join(dir, session.FindingsFile) - // Read-side no-follow guard rather than plain os.Open: a FIFO planted at - // findings.jsonl in an exchanged session would otherwise hang this open for - // ever. A missing file still satisfies os.ErrNotExist and is not an error here. - f, err := session.OpenFileNoFollowRead(path) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return false, nil - } - return false, err - } - defer f.Close() - +// holdsVerdicts reports whether the findings.jsonl open on f already contains +// any verdict record. It reads through the caller's descriptor — opened under +// the no-follow guard and exclusively locked by commitFindings — rather than +// opening the path itself, so the probe and the write it gates observe the same +// locked file. It scans for raw kind:"verdict" lines rather than reusing +// analyze.Load, whose verdict slice is filtered to the closed enum +// (confirmed|rejected|duplicate): a hand-edited or shared file whose only +// verdict lines carry a foreign or typo'd value would otherwise slip past the +// guard and have its human-decision records truncated by a re-ingest — exactly +// the precision history the guard exists to protect. +func holdsVerdicts(f io.Reader, path string) (bool, error) { sc := bufio.NewScanner(f) sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) for sc.Scan() { diff --git a/internal/analyze/validate.go b/internal/analyze/validate.go index 380c7e5..50441bb 100644 --- a/internal/analyze/validate.go +++ b/internal/analyze/validate.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/REPPL/Testimony/internal/session" "github.com/REPPL/Testimony/internal/timeline" ) @@ -29,6 +30,18 @@ type timelineIndex struct { } // indexTimeline builds a timelineIndex from merged timeline entries. +// +// Every key the answer is matched against is stored in its session.SafeText form +// (ids, utterance text, selectors, routes), because that is the only form of the +// timeline the answering agent is ever shown: EmitRequest routes each marshalled +// timeline line through SafeText to strip terminal-control and Trojan-Source +// bytes before printing the request. Indexing the raw bytes here while the agent +// copies the sanitised bytes would make an id, selector, route, or quote that +// carried any stripped character impossible to validate — an honest, +// verbatim-copied answer would be rejected as "not present". SafeText is a no-op +// on ordinary text, so this changes nothing for a normal session and only closes +// the shown-vs-validated gap on control-character-bearing (i.e. hostile or +// hand-edited) transcripts. func indexTimeline(entries []timeline.Entry) timelineIndex { idx := timelineIndex{ ids: map[string]bool{}, @@ -37,20 +50,20 @@ func indexTimeline(entries []timeline.Entry) timelineIndex { routes: map[string]bool{}, } for i, e := range entries { - idx.ids[e.ID] = true + idx.ids[session.SafeText(e.ID)] = true end := e.T switch e.Src { case "speech": end = timeline.SpeechEnd(e) if s, ok := e.Payload["text"].(string); ok { - idx.uttText[e.ID] = s + idx.uttText[session.SafeText(e.ID)] = session.SafeText(s) } case "event": if s, ok := e.Payload["selector"].(string); ok && s != "" { - idx.selectors[s] = true + idx.selectors[session.SafeText(s)] = true } if r, ok := e.Payload["route"].(string); ok && r != "" { - idx.routes[r] = true + idx.routes[session.SafeText(r)] = true } } // Seed idx.end on the first entry, exactly as idx.start is seeded below, so @@ -158,13 +171,17 @@ func validate(findings []positioned, idx timelineIndex) []error { errs = append(errs, fmt.Errorf("%s: evidence lists %d ids, exceeding the limit of %d", label, len(f.Evidence), maxEvidence)) } for _, id := range f.Evidence { - if !idx.ids[id] { + // Match against the SafeText form the index holds — the form the agent + // was shown (see indexTimeline). The id is normalised once here so the + // utt-* anchor test and the uttText lookup agree with the id set. + nid := session.SafeText(id) + if !idx.ids[nid] { errs = append(errs, fmt.Errorf("%s: evidence id %q not found in the timeline", label, id)) continue } - if strings.HasPrefix(id, "utt-") { + if strings.HasPrefix(nid, "utt-") { hasUtt = true - if txt, ok := idx.uttText[id]; ok { + if txt, ok := idx.uttText[nid]; ok { uttTexts = append(uttTexts, txt) } } @@ -174,19 +191,23 @@ func validate(findings []positioned, idx timelineIndex) []error { } // quote: non-empty and a verbatim substring of one cited evidence - // utterance's text (per-utterance, no normalisation). + // utterance's text. Both sides are compared in SafeText form (uttTexts is + // already sanitised via the index): the agent copies the quote from the + // sanitised request, so validating against the raw utterance would reject an + // honest verbatim copy whenever the utterance carried a stripped character. if f.Quote == "" { errs = append(errs, fmt.Errorf("%s: quote must be non-empty", label)) - } else if !containsAny(uttTexts, f.Quote) { + } else if !containsAny(uttTexts, session.SafeText(f.Quote)) { errs = append(errs, fmt.Errorf("%s: quote is not a verbatim substring of any cited evidence utterance", label)) } - // ui selector/route, when present, must name a real event. + // ui selector/route, when present, must name a real event — matched in the + // same SafeText form the index and the emitted request use. if f.UI != nil { - if f.UI.Selector != "" && !idx.selectors[f.UI.Selector] { + if f.UI.Selector != "" && !idx.selectors[session.SafeText(f.UI.Selector)] { errs = append(errs, fmt.Errorf("%s: ui.selector %q is not present on any timeline event", label, f.UI.Selector)) } - if f.UI.Route != "" && !idx.routes[f.UI.Route] { + if f.UI.Route != "" && !idx.routes[session.SafeText(f.UI.Route)] { errs = append(errs, fmt.Errorf("%s: ui.route %q is not present on any timeline event", label, f.UI.Route)) } } diff --git a/internal/demo/demo.go b/internal/demo/demo.go index e2569ee..dfa8909 100644 --- a/internal/demo/demo.go +++ b/internal/demo/demo.go @@ -252,9 +252,12 @@ func (s *server) appendLines(w http.ResponseWriter, r *http.Request, f *os.File, s.mu.Lock() defer s.mu.Unlock() if err := appendRecords(f, lines); err != nil { - // The capture was not persisted; tell the client so it does not treat a - // dropped event as recorded, rather than answering 204 as if it had - // succeeded. + // The capture was not persisted. Tell the client so it does not treat a + // dropped event as recorded (it answers the 500 rather than a 204), and log + // to the operator's terminal: the client uses sendBeacon, which cannot report + // a server status back to the page, so this line is the only signal the person + // running the session gets that their evidence stream has started dropping. + fmt.Fprintf(os.Stderr, "testimony demo: capture write failed, event(s) dropped: %v\n", err) http.Error(w, "capture write failed", http.StatusInternalServerError) return } diff --git a/internal/record/record.go b/internal/record/record.go index 84524c3..ce12763 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -136,7 +136,7 @@ func Run(opts Options) error { ctx, stop := notifyContext() defer stop() - children, err := startRecordersFn(dir, recorders) + children, err := startRecordersFn(dir, recorders, opts.Log) if err != nil { return err } @@ -280,7 +280,7 @@ func checkPlainOutput(path string) error { // ffmpeg subprocess per requested stream, each in its own process group with // captured stderr. On darwin the streams are non-empty; elsewhere they are, so // this is a no-op returning no children. -func startRecorders(dir string, streams []string) ([]*liveChild, error) { +func startRecorders(dir string, streams []string, log io.Writer) ([]*liveChild, error) { if len(streams) == 0 { return nil, nil } @@ -288,10 +288,17 @@ func startRecorders(dir string, streams []string) ([]*liveChild, error) { if err != nil { return nil, fmt.Errorf("ffmpeg not found on PATH (needed to capture audio/screen): brew install ffmpeg") } - micIndex, screenIndex, err := probeDevices(ffmpeg, contains(streams, streamScreen)) + screenIndex, mics, err := probeDevices(ffmpeg, contains(streams, streamScreen)) if err != nil { return nil, err } + // The microphone is captured via avfoundation ":default" (micArgs), so ffmpeg + // resolves the system default at capture time. Log the detected input roster + // so a virtual audio driver shadowing the real mic is visible before a session + // is recorded to silence. + if contains(streams, streamMicrophone) && len(mics) > 0 { + fmt.Fprintf(log, " audio inputs: %s\n microphone : system default (avfoundation :default)\n", strings.Join(mics, ", ")) + } var children []*liveChild for _, stream := range streams { @@ -303,7 +310,7 @@ func startRecorders(dir string, streams []string) ([]*liveChild, error) { var args []string switch stream { case streamMicrophone: - args = micArgs(micIndex, out) + args = micArgs(out) case streamScreen: args = screenArgs(screenIndex, out) } diff --git a/internal/record/record_test.go b/internal/record/record_test.go index 7513c71..4d676ed 100644 --- a/internal/record/record_test.go +++ b/internal/record/record_test.go @@ -54,10 +54,10 @@ func TestResolveVideo(t *testing.T) { // --- argv builders --- func TestMicArgs(t *testing.T) { - got := micArgs(0, "sessions/s1/audio.wav") + got := micArgs("sessions/s1/audio.wav") want := []string{ "-f", "avfoundation", - "-i", ":0", + "-i", ":default", "-ac", "1", "-ar", "16000", "-c:a", "pcm_s16le", @@ -107,16 +107,19 @@ func TestParseAVDevices(t *testing.T) { func TestSelectDevices(t *testing.T) { video, audio := parseAVDevices(sampleDevices) - mic, screen, err := selectDevices(video, audio, true) + screen, mics, err := selectDevices(video, audio, true) if err != nil { t.Fatalf("selectDevices: %v", err) } - if mic != 0 { - t.Fatalf("mic index: got %d, want 0 (system default input)", mic) - } if screen != 1 { t.Fatalf("screen index: got %d, want 1 (Capture screen)", screen) } + // The audio-input roster is returned for logging, in listing order. The mic + // itself is captured via avfoundation :default, not by this list. + wantMics := []string{"Studio Display Microphone", "USB audio CODEC"} + if !reflect.DeepEqual(mics, wantMics) { + t.Fatalf("audio roster: got %q, want %q", mics, wantMics) + } // Audio-only: screen index is not resolved and no error. if _, _, err := selectDevices(video, audio, false); err != nil { @@ -134,6 +137,31 @@ func TestSelectDevices(t *testing.T) { } } +// TestOutputTail covers the ffmpeg-diagnostic surfacing: an empty listing paired +// with real ffmpeg output must yield a bounded, trimmed tail so probeDevices can +// tell the operator the true cause (e.g. an avfoundation-less build) instead of +// misdirecting them to their microphone. +func TestOutputTail(t *testing.T) { + if got := outputTail(nil); got != "" { + t.Fatalf("empty input must yield empty tail, got %q", got) + } + if got := outputTail([]byte(" \n ")); got != "" { + t.Fatalf("whitespace-only input must trim to empty, got %q", got) + } + short := []byte("Unknown input format: 'avfoundation'") + if got := outputTail(short); got != string(short) { + t.Fatalf("short output must pass through trimmed: got %q", got) + } + long := make([]byte, 0, 1000) + for i := 0; i < 1000; i++ { + long = append(long, 'x') + } + got := outputTail(long) + if len(got) > 420 || !strings.HasPrefix(got, "...") { + t.Fatalf("long output must be bounded and elided, got len %d prefix %.3q", len(got), got) + } +} + // --- platform plan --- func TestPlan(t *testing.T) { @@ -306,7 +334,7 @@ func TestRunInstallsSignalHandlerBeforeSpawning(t *testing.T) { cancel = c return ctx, c } - startRecordersFn = func(dir string, streams []string) ([]*liveChild, error) { + startRecordersFn = func(dir string, streams []string, _ io.Writer) ([]*liveChild, error) { note("spawn") // A well-behaved recorder that finalises a real audio.wav and is reaped on // SIGINT, so the lifecycle stops cleanly. cancel is non-nil only if notify @@ -363,7 +391,7 @@ func TestRunClassifiesStartupExitDespiteSlowStop(t *testing.T) { } var stubborn *fakeProc - startRecordersFn = func(dir string, streams []string) ([]*liveChild, error) { + startRecordersFn = func(dir string, streams []string, _ io.Writer) ([]*liveChild, error) { buf := &lockedBuffer{} _, _ = buf.Write([]byte("[AVFoundation indev @ 0x0] Failed to open device\nInput/output error")) // The microphone recorder is denied by TCC and dies at once — well inside @@ -467,7 +495,7 @@ func TestStartRecordersRefusesSymlinkedOutput(t *testing.T) { t.Fatal(err) } - children, err := startRecorders(dir, []string{streamMicrophone}) + children, err := startRecorders(dir, []string{streamMicrophone}, io.Discard) if err == nil { stopAll(children) t.Fatal("startRecorders must refuse a symlinked audio output rather than spawn ffmpeg on it") @@ -537,7 +565,7 @@ func TestRunReportsRecorderStoppedWithNoOutput(t *testing.T) { cancel = c return ctx, c } - startRecordersFn = func(dir string, streams []string) ([]*liveChild, error) { + startRecordersFn = func(dir string, streams []string, _ io.Writer) ([]*liveChild, error) { buf := &lockedBuffer{} _, _ = buf.Write([]byte("[AVFoundation indev @ 0x0] Failed to open device\nInput/output error")) // A recorder that blocked on the TCC prompt all session: it never wrote @@ -640,7 +668,7 @@ func TestRunStopsDemoServerThroughBoundedShutdown(t *testing.T) { cancel = cf return ctx, cf } - startRecordersFn = func(dir string, streams []string) ([]*liveChild, error) { + startRecordersFn = func(dir string, streams []string, _ io.Writer) ([]*liveChild, error) { child := newLiveChild(streamMicrophone, newFakeProc(syscall.SIGINT), &lockedBuffer{}) if c.interrupt { // A well-behaved recorder that finalised a real audio.wav, so the diff --git a/internal/record/recorders.go b/internal/record/recorders.go index 338de70..ad7f171 100644 --- a/internal/record/recorders.go +++ b/internal/record/recorders.go @@ -1,6 +1,7 @@ package record import ( + "errors" "fmt" "os/exec" "regexp" @@ -8,15 +9,19 @@ import ( "strings" ) -// micArgs builds the ffmpeg argv that captures the default microphone to a -// canonical 16 kHz mono PCM WAV — exactly the parameters transcribe.convertAudio -// produces, so the file is canonical ASR input needing no re-conversion. Pure: -// it takes the resolved avfoundation audio-device index and the output path, so -// it is unit-testable without a device. -func micArgs(micIndex int, outPath string) []string { +// micArgs builds the ffmpeg argv that captures the system default microphone to +// a canonical 16 kHz mono PCM WAV — exactly the parameters +// transcribe.convertAudio produces, so the file is canonical ASR input needing +// no re-conversion. It captures the avfoundation ":default" audio device (the +// actual system default input macOS resolves at capture time) rather than a +// fixed index: a virtual audio driver (BlackHole, Loopback, a conferencing +// tool's device) can enumerate at index 0 and would then be recorded to silence +// in the real microphone's place. startRecorders logs the detected input roster +// so a surprising default is still visible. Pure and unit-testable. +func micArgs(outPath string) []string { return []string{ "-f", "avfoundation", - "-i", ":" + strconv.Itoa(micIndex), + "-i", ":default", "-ac", "1", "-ar", "16000", "-c:a", "pcm_s16le", @@ -82,15 +87,21 @@ func parseAVDevices(stderr string) (video, audio []avDevice) { return video, audio } -// selectDevices picks the microphone and (when video is requested) screen -// avfoundation indices from parsed device lists. The screen is the video device -// whose name contains "Capture screen"; the microphone is audio index 0 (the -// system default input). Pure, so the selection logic is tested without ffmpeg. -func selectDevices(video, audio []avDevice, wantScreen bool) (micIndex, screenIndex int, err error) { +// selectDevices validates that at least one avfoundation audio input exists and, +// when video is requested, resolves the screen index (the video device whose +// name contains "Capture screen"). It returns the detected audio-input names for +// the caller to log: the microphone itself is captured via avfoundation +// ":default" (see micArgs), not by index, so ffmpeg picks the system default at +// capture time and this list is what makes a surprising default — a virtual +// audio driver shadowing the real mic — visible to the operator. Pure, so the +// selection logic is tested without ffmpeg. +func selectDevices(video, audio []avDevice, wantScreen bool) (screenIndex int, mics []string, err error) { if len(audio) == 0 { - return 0, 0, fmt.Errorf("no avfoundation audio input devices found; connect or enable a microphone") + return 0, nil, fmt.Errorf("no avfoundation audio input devices found; connect or enable a microphone") + } + for _, d := range audio { + mics = append(mics, d.name) } - micIndex = audio[0].index screenIndex = -1 if wantScreen { @@ -101,20 +112,59 @@ func selectDevices(video, audio []avDevice, wantScreen bool) (micIndex, screenIn } } if screenIndex == -1 { - return 0, 0, fmt.Errorf("no avfoundation \"Capture screen\" device found; screen capture is unavailable") + return 0, nil, fmt.Errorf("no avfoundation \"Capture screen\" device found; screen capture is unavailable") } } - return micIndex, screenIndex, nil + return screenIndex, mics, nil } -// probeDevices runs ffmpeg's avfoundation device listing and resolves the -// microphone and (when wantScreen) screen indices. The command exits non-zero -// by design after printing the list to stderr, so a non-nil exec error is -// expected; only a failure to find the needed devices is fatal. Impure (spawns +// probeDevices runs ffmpeg's avfoundation device listing, validates an audio +// input exists, and resolves the (when wantScreen) screen index. The command +// exits non-zero by design after printing the list to stderr, so a non-nil exec +// error is expected; only a failure to find the needed devices is fatal. It +// returns the detected audio-input names for the caller to log. Impure (spawns // ffmpeg); isolated here and skipped in CI. -func probeDevices(ffmpeg string, wantScreen bool) (micIndex, screenIndex int, err error) { +func probeDevices(ffmpeg string, wantScreen bool) (screenIndex int, mics []string, err error) { cmd := exec.Command(ffmpeg, "-hide_banner", "-f", "avfoundation", "-list_devices", "true", "-i", "") - out, _ := cmd.CombinedOutput() // always exits non-zero; the listing is on stderr + out, runErr := cmd.CombinedOutput() + // ffmpeg prints the listing to stderr and then exits non-zero by design, so an + // *exec.ExitError is expected and ignored — the listing is still in out. But a + // failure to run ffmpeg at all (a corrupt or wrong-architecture binary, a + // sandbox/exec denial, a fork failure after LookPath already succeeded) is a + // different error: out is then empty, parseAVDevices finds nothing, and + // selectDevices would misreport "no microphone found", sending the operator to + // check hardware and permissions for a process that never started. Surface it. + var exitErr *exec.ExitError + if runErr != nil && !errors.As(runErr, &exitErr) { + return 0, nil, fmt.Errorf("run ffmpeg device listing: %w", runErr) + } video, audio := parseAVDevices(string(out)) - return selectDevices(video, audio, wantScreen) + screenIndex, mics, err = selectDevices(video, audio, wantScreen) + if err != nil { + // selectDevices found no usable device. The benign by-design exit prints the + // listing to stderr, but an ffmpeg that genuinely failed — built without + // avfoundation ("Unknown input format: 'avfoundation'"), or killed by a + // signal — also lands here as an *exec.ExitError with the real cause sitting + // in out, which parseAVDevices then reads as an empty listing. Without the + // tail the operator is told to check their microphone for what is actually a + // toolchain fault. Surface ffmpeg's own last words so the true cause reaches + // them. + if tail := outputTail(out); tail != "" { + return 0, nil, fmt.Errorf("%w; ffmpeg said: %s", err, tail) + } + return 0, nil, err + } + return screenIndex, mics, nil +} + +// outputTail returns a trimmed, bounded tail of ffmpeg's output for inclusion in +// an error, so a diagnostic reaches the operator without dumping the whole +// listing. Empty in, empty out. +func outputTail(out []byte) string { + const max = 400 + s := strings.TrimSpace(string(out)) + if len(s) > max { + s = "..." + s[len(s)-max:] + } + return s } diff --git a/internal/report/report.go b/internal/report/report.go index fd66f32..234c7b3 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io/fs" + "math" "path/filepath" "sort" "strings" @@ -16,6 +17,14 @@ import ( "github.com/REPPL/Testimony/internal/timeline" ) +// maxClockSeconds bounds a value clock will format. A real session stamp is +// minutes to hours; 1e9 seconds (~31 years) is far past any genuine one while +// staying well inside int64 so the float64→int conversion below can never go +// out of range. timeline.jsonl and findings.jsonl are attacker-authorable and +// reach clock without passing timeline.checkedUtterances, so this sink must +// defend itself rather than trust its input. +const maxClockSeconds = 1e9 + // Render reads manifest + timeline from dir and returns the Markdown report. // window is the utterance↔event join window in seconds. func Render(dir string, window float64) (string, error) { @@ -90,7 +99,12 @@ func Render(dir string, window float64) (string, error) { fmt.Fprintf(&b, " - [%s] %s\n", clock(e.T), eventLine(e)) } } - flushStandaloneBefore(1e18) + // Flush every remaining standalone event. The sentinel is +Inf, not a finite + // literal: an earlier 1e18 bound silently dropped any event whose t was at or + // past it (reachable from a hand-authored timeline.jsonl), omitting evidence + // from the report while merge and report both exited 0. Every JSON-decodable t + // is finite and so strictly less than +Inf, so this flushes all of them. + flushStandaloneBefore(math.Inf(1)) b.WriteString("\n## Findings\n\n") renderFindings(&b, dir) @@ -107,7 +121,14 @@ func renderFindings(b *strings.Builder, dir string) { b.WriteString("_No findings yet — run `testimony analyze` then `testimony review`._\n") return } - fmt.Fprintf(b, "_Findings unavailable: %v_\n", err) + // The raw error carries the joined filesystem path (an absolute one when the + // operator passed an absolute -session), which on macOS embeds the username, + // and it is the one string in this function not passed through SafeText. The + // report is the human-facing artefact a session directory is built to share, + // so leaking that path — or unescaped control bytes from a malformed line — + // into it is an info-disclosure the generic notice avoids; the detailed error + // still surfaces on stderr when the operator re-runs analyze or review. + b.WriteString("_Findings unavailable: findings.jsonl could not be read (run `testimony analyze`/`review` to see why)._\n") return } @@ -192,6 +213,17 @@ func end(entries []timeline.Entry) float64 { // arithmetic because %02d over a negative integer division renders garbage such // as "-1:-30". func clock(sec float64) string { + // Defend the float64→int conversion below. A non-finite or astronomically + // large sec — reachable from a hand-authored timeline.jsonl or findings.jsonl, + // which bypass timeline.checkedUtterances — makes int(sec+0.5) an out-of-range + // conversion the Go spec leaves implementation-defined: on arm64 it saturates + // to MaxInt64 and prints a nonsensical duration like "153722867280912930:07"; + // on amd64 it wraps negative, defeating the sign handling below. Such a value + // is not a time, so render a visibly-broken placeholder rather than fabricate a + // precise-looking wrong stamp in the human evidence artefact. + if math.IsNaN(sec) || math.Abs(sec) > maxClockSeconds { + return "--:--" + } neg := sec < 0 if neg { sec = -sec diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 2a68011..7bedda4 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -1,6 +1,7 @@ package report import ( + "math" "os" "path/filepath" "strings" @@ -328,3 +329,110 @@ func TestReportFindingsSanitiseIDAndVerdict(t *testing.T) { t.Fatalf("confirmed count line was altered:\n%s", md) } } + +// TestReportDoesNotLeakPathOnUnreadableFindings covers the info-disclosure on the +// findings-unavailable path. findings.jsonl exists but cannot be read — here a +// symlink, which session's no-follow guard refuses with an error naming the full +// path. That path is absolute when the operator passed an absolute -session and +// on macOS embeds the username. report.md is the artefact a session directory is +// built to share, so the raw error (the one string in renderFindings not routed +// through SafeText) must not land in it. Render still succeeds; the report shows a +// generic notice and no filesystem path. Pre-fix the whole error, path and all, +// was written into report.md. +func TestReportDoesNotLeakPathOnUnreadableFindings(t *testing.T) { + dir := setupSession(t) + findings := filepath.Join(dir, session.FindingsFile) + if err := os.Symlink(filepath.Join(dir, "elsewhere"), findings); err != nil { + t.Fatalf("symlink: %v", err) + } + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render should stay non-fatal on an unreadable findings file: %v", err) + } + if strings.Contains(md, dir) { + t.Fatalf("report leaked the session path into report.md:\n%s", md) + } + if !strings.Contains(md, "could not be read") { + t.Fatalf("expected a generic findings-unavailable notice, got:\n%s", md) + } +} + +// TestClockRefusesOutOfRangeTime is the sink half of the time-magnitude class. +// timeline.jsonl and findings.jsonl are attacker-authorable and reach clock +// without passing timeline.checkedUtterances. A finite-but-astronomical t makes +// int(sec+0.5) an out-of-range float64→int conversion (implementation-defined: +// arm64 saturates to MaxInt64 and prints "153722867280912930:07", amd64 wraps +// negative), planting a nonsensical stamp in the human evidence artefact. clock +// must render a visibly-broken placeholder instead. Pre-fix it did the raw +// conversion. +func TestClockRefusesOutOfRangeTime(t *testing.T) { + for _, tc := range []struct { + name string + sec float64 + }{ + {"huge positive", 1e300}, + {"huge negative", -1e300}, + {"positive inf", math.Inf(1)}, + {"nan", math.NaN()}, + } { + got := clock(tc.sec) + if got != "--:--" { + t.Errorf("clock(%s)=%q, want %q (out-of-range must not reach int conversion)", tc.name, got, "--:--") + } + } + // The ordinary range is unaffected. + if got := clock(125); got != "02:05" { + t.Fatalf("clock(125)=%q, want 02:05", got) + } +} + +// TestReportRendersHugeTimeAsPlaceholder is the end-to-end guard: a hand-authored +// timeline.jsonl whose speech carries t=1e300 must render a placeholder Duration, +// not a saturated-integer garbage stamp, and Render must still exit cleanly. +func TestReportRendersHugeTimeAsPlaceholder(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "x", Participant: "P1"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + const tl = `{"t":1e300,"src":"speech","id":"u1","payload":{"speaker":"P1","t1":1e300,"text":"planted"}} +` + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render: %v", err) + } + if strings.Contains(md, "153722867280912930") { + t.Fatalf("report rendered a saturated-integer garbage stamp:\n%s", md) + } + if !strings.Contains(md, "--:--") { + t.Fatalf("expected a placeholder stamp for the out-of-range time:\n%s", md) + } +} + +// TestReportFlushesEventPastLegacySentinel covers the sentinel bug: the trailing +// standalone-event flush used a finite 1e18 bound, so any event with t at or past +// it was silently omitted from the report while merge and report both exited 0. +// The flush is now +Inf-bounded, so every finite-t event appears. Pre-fix the +// event below (t=1e18) was dropped. +func TestReportFlushesEventPastLegacySentinel(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "x", Participant: "P1"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + // One ordinary utterance and a standalone event at exactly the old sentinel. + const tl = `{"t":5,"src":"speech","id":"u1","payload":{"speaker":"P1","t1":6,"text":"hello"}} +{"t":1e18,"src":"event","id":"ev-001","payload":{"kind":"click","selector":"#late","route":"#r"}} +` + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render: %v", err) + } + if !strings.Contains(md, "#late") { + t.Fatalf("standalone event at the legacy 1e18 sentinel was dropped from the report:\n%s", md) + } +} diff --git a/internal/review/review.go b/internal/review/review.go index 4098e71..f99a1ed 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -13,15 +13,23 @@ import ( "fmt" "io" "io/fs" + "math" "os" "path/filepath" "sort" "strings" + "syscall" "github.com/REPPL/Testimony/internal/analyze" "github.com/REPPL/Testimony/internal/session" ) +// maxClockSeconds bounds a value clock will format, mirroring report.maxClockSeconds: +// a real session stamp is minutes to hours, and 1e9 seconds (~31 years) stays +// well inside int64 so the float64→int conversion in clock can never go out of +// range on an attacker-authored findings.jsonl time. +const maxClockSeconds = 1e9 + // Options configures a review run. type Options struct { Dir string // session directory @@ -72,14 +80,29 @@ func single(opts Options, findings []analyze.Finding) error { if err := checkTargets(findings, opts.Finding, verdict, of); err != nil { return err } + // checkTargets passed, so the id is present in the snapshot; bind the verdict + // to that finding so AppendVerdict can confirm it is unchanged at write time. + target := findByID(findings, opts.Finding) rec := analyze.Verdict{Kind: "verdict", Finding: opts.Finding, Verdict: verdict, Of: of, At: opts.Today} - if err := AppendVerdict(opts.Dir, rec); err != nil { + if err := AppendVerdict(opts.Dir, rec, target); err != nil { return err } fmt.Fprintln(opts.Out, describe(rec)) return nil } +// findByID returns a pointer to the finding with the given id, or nil. The +// returned pointer is into a copy, safe to retain. +func findByID(findings []analyze.Finding, id string) *analyze.Finding { + for i := range findings { + if findings[i].ID == id { + f := findings[i] + return &f + } + } + return nil +} + // errPersist marks an error that arose while writing a verdict to disk, as // distinct from the validation errors the walk raises for an unrecognised // keystroke or a bad duplicate target. The walk must be able to tell them @@ -146,9 +169,9 @@ func walk(opts Options, findings []analyze.Finding, verdicts []analyze.Verdict) func applyChoice(opts Options, findings []analyze.Finding, f analyze.Finding, choice string, r *bufio.Reader) (done, quit bool, err error) { switch strings.ToLower(strings.TrimSpace(choice)) { case "c": - return true, false, record(opts, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "confirmed", At: opts.Today}) + return true, false, record(opts, f, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "confirmed", At: opts.Today}) case "r": - return true, false, record(opts, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "rejected", At: opts.Today}) + return true, false, record(opts, f, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "rejected", At: opts.Today}) case "d": fmt.Fprint(opts.Out, " duplicate of (F-NNN): ") target, rerr := readLine(r) @@ -162,7 +185,7 @@ func applyChoice(opts Options, findings []analyze.Finding, f analyze.Finding, ch if err := checkTargets(findings, f.ID, "duplicate", target); err != nil { return false, false, err } - return true, false, record(opts, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "duplicate", Of: target, At: opts.Today}) + return true, false, record(opts, f, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "duplicate", Of: target, At: opts.Today}) case "s", "": fmt.Fprintln(opts.Out, " skipped.") return true, false, nil @@ -173,8 +196,8 @@ func applyChoice(opts Options, findings []analyze.Finding, f analyze.Finding, ch } } -func record(opts Options, rec analyze.Verdict) error { - if err := AppendVerdict(opts.Dir, rec); err != nil { +func record(opts Options, judged analyze.Finding, rec analyze.Verdict) error { + if err := AppendVerdict(opts.Dir, rec, &judged); err != nil { // Wrapped so walk can distinguish a lost verdict from a mistyped // keystroke; see errPersist. return fmt.Errorf("%w: %v", errPersist, err) @@ -221,11 +244,30 @@ func ParseVerdictFlag(s string) (verdict, of string, err error) { // AppendVerdict appends one verdict record to findings.jsonl without touching // any existing line (append-only; latest verdict wins for display). -func AppendVerdict(dir string, v analyze.Verdict) error { +// +// expect, when non-nil, is the finding the analyst was shown when they made this +// decision. AppendVerdict re-reads the current findings under its lock and +// refuses if the targeted id is gone or now names a different finding — see +// verifyTarget. Callers pass nil only when there is no snapshot to bind against +// (there are none in production; the review paths always pass the judged +// finding). +func AppendVerdict(dir string, v analyze.Verdict, expect *analyze.Finding) error { b, err := json.Marshal(v) if err != nil { return err } + // Hold the verdict to MaxJSONLLine, the shared read-side invariant every other + // JSONL writer respects (session.WriteJSONL, analyze.oversizedFindings, + // demo.tooLongForJSONL). A verdict carries its finding id verbatim, and in an + // exchanged or hand-edited findings.jsonl that id can be just under the 4 MiB + // scanner cap — small enough that the finding line loads, large enough that the + // verdict's own framing tips the line over it. Appending it would durably brick + // the verdict history this package exists to protect: every later analyze.Load, + // review, report, and holdsVerdicts would fail with "token too long". + if len(b)+1 > session.MaxJSONLLine { + return fmt.Errorf("verdict for %s encodes to %d bytes, over the %d-byte %s line limit", + v.Finding, len(b)+1, session.MaxJSONLLine, session.FindingsFile) + } path := filepath.Join(dir, session.FindingsFile) // O_RDWR rather than O_WRONLY because the record cannot be framed correctly // without first reading the byte already at the end of the file. @@ -233,6 +275,34 @@ func AppendVerdict(dir string, v analyze.Verdict) error { if err != nil { return err } + // Take an exclusive advisory lock across the probe → write → rollback sequence. + // Two `testimony review` processes appending to one session's findings.jsonl + // would otherwise race: A measures the end, B appends a full verdict past it, A's + // write fails part-way (ENOSPC — the case the rollback exists for), and A's + // Truncate then cuts the file back below B's committed record, deleting it. + // writeVerdict re-measuring the end only shrinks that window; the lock closes it, + // so the length A rolls back to is the true end before A's own bytes. The lock + // releases with the descriptor on Close. + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + f.Close() + return err + } + // Under the lock, confirm the verdict still targets the finding the analyst + // judged. review.Run snapshots findings once (analyze.Load) and then blocks on + // the operator for the whole interactive walk; a concurrent `analyze -ingest` + // can truncate-and-rewrite findings.jsonl in that gap — permitted until the + // first verdict exists — and because finding ids restart at F-001 the verdict + // would otherwise attach to a different finding under the same id, silently + // misattributing the human decision this file exists to hold. The re-check runs + // under the same exclusive lock analyze.commitFindings takes, so the re-ingest + // is either already visible here (mismatch → refuse, no verdict written) or + // serialised after this append and then blocked by its own verdict-guard. + if expect != nil { + if err := verifyTarget(f, v, *expect); err != nil { + f.Close() + return err + } + } if err := writeVerdict(f, append(b, '\n')); err != nil { f.Close() return err @@ -242,6 +312,38 @@ func AppendVerdict(dir string, v analyze.Verdict) error { return f.Close() } +// verifyTarget re-reads the findings currently in f (the locked findings.jsonl +// descriptor) and confirms the verdict v still applies to the finding expect — +// the one the analyst was shown. It refuses if the id has vanished or now names +// a different finding, and for a duplicate verdict if the "of" target has +// vanished. f is read via a SectionReader over ReadAt, which leaves the file +// offset untouched, and the descriptor is O_APPEND, so the subsequent write +// still lands at the true end of file. +func verifyTarget(f *os.File, v analyze.Verdict, expect analyze.Finding) error { + info, err := f.Stat() + if err != nil { + return err + } + findings, _, err := analyze.ParseRecords(io.NewSectionReader(f, 0, info.Size()), session.FindingsFile) + if err != nil { + return err + } + cur := findByID(findings, v.Finding) + if cur == nil { + return fmt.Errorf("finding %s is no longer in %s; it changed since review started — re-run `testimony review`", + v.Finding, session.FindingsFile) + } + if !analyze.SameIdentity(*cur, expect) { + return fmt.Errorf("finding %s changed since review started (a re-analysis rewrote %s); re-run `testimony review` before recording a verdict", + v.Finding, session.FindingsFile) + } + if v.Verdict == "duplicate" && findByID(findings, v.Of) == nil { + return fmt.Errorf("duplicate target %s is no longer in %s; re-run `testimony review`", + v.Of, session.FindingsFile) + } + return nil +} + // verdictFile is the subset of *os.File that writeVerdict needs; a fake // satisfies it in tests to exercise the partial-write rollback. type verdictFile interface { @@ -365,6 +467,15 @@ func readLine(r *bufio.Reader) (string, error) { // moment on the very surface where they record the verdict. This mirrors // report.clock; see the note in review_test.go about the duplication. func clock(sec float64) string { + // Defend the float64→int conversion below against a non-finite or + // astronomically large sec from a hand-authored findings.jsonl (printFinding + // renders f.T, and analyze.Load does not bound it): int(sec+0.5) would be an + // out-of-range conversion the Go spec leaves implementation-defined, printing a + // nonsensical stamp on the surface where the analyst records a verdict. Mirrors + // report.clock's guard — the class fix for both copies of this function. + if math.IsNaN(sec) || math.Abs(sec) > maxClockSeconds { + return "--:--" + } neg := sec < 0 if neg { sec = -sec diff --git a/internal/review/review_test.go b/internal/review/review_test.go index 0bcfe36..fe75ae7 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "io" + "math" "os" "path/filepath" "strings" @@ -293,7 +294,7 @@ func TestAppendVerdictTerminatesAnUnterminatedLastLine(t *testing.T) { } rec := analyze.Verdict{Kind: "verdict", Finding: "F-003", Verdict: "confirmed", At: "2026-07-17"} - if err := AppendVerdict(dir, rec); err != nil { + if err := AppendVerdict(dir, rec, nil); err != nil { t.Fatalf("AppendVerdict: %v", err) } @@ -420,7 +421,7 @@ func TestAppendVerdictRefusesSymlink(t *testing.T) { if err := os.Symlink(outside, filepath.Join(dir, session.FindingsFile)); err != nil { t.Fatalf("symlink: %v", err) } - err := AppendVerdict(dir, analyze.Verdict{Kind: "verdict", Finding: "F-001", Verdict: "confirmed", At: "2026-07-17"}) + err := AppendVerdict(dir, analyze.Verdict{Kind: "verdict", Finding: "F-001", Verdict: "confirmed", At: "2026-07-17"}, nil) if err == nil { t.Fatal("AppendVerdict followed a symlink; want refusal") } @@ -471,9 +472,119 @@ func TestClockRoundsSymmetrically(t *testing.T) { {-61.5, "-01:02"}, {-90, "-01:30"}, {-3600, "-60:00"}, + // Out-of-range and non-finite times (from a hand-authored findings.jsonl + // f.T that analyze.Load does not bound) must render a placeholder, never an + // implementation-defined out-of-range int conversion. Sibling of + // report.TestClockRefusesOutOfRangeTime. + {1e300, "--:--"}, + {-1e300, "--:--"}, + {math.Inf(1), "--:--"}, + {math.NaN(), "--:--"}, } { if got := clock(tc.sec); got != tc.want { t.Fatalf("clock(%g) = %q, want %q", tc.sec, got, tc.want) } } } + +// TestAppendVerdictRefusesOversizedLine is the write-side twin of +// analyze.oversizedFindings. Every JSONL reader scans with a MaxJSONLLine-capped +// buffer, so a verdict line past that cap is durably unreadable and bricks the +// whole findings.jsonl — the verdict history this package exists to protect — on +// the next Load/review/report. A finding id can arrive just under the cap from an +// exchanged or hand-edited file (the finding line loads; the verdict's framing +// tips it over). AppendVerdict must refuse rather than append. Pre-fix it wrote +// the line unchecked. +func TestAppendVerdictRefusesOversizedLine(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(findingsFixture), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + hugeID := "F-" + strings.Repeat("9", session.MaxJSONLLine) + rec := analyze.Verdict{Kind: "verdict", Finding: hugeID, Verdict: "confirmed", At: "2026-07-17"} + err := AppendVerdict(dir, rec, nil) + if err == nil || !strings.Contains(err.Error(), "line limit") { + t.Fatalf("expected an over-limit refusal, got %v", err) + } + // The refusal is pre-write: the existing findings.jsonl is byte-unchanged. + b, rerr := os.ReadFile(filepath.Join(dir, session.FindingsFile)) + if rerr != nil { + t.Fatalf("read findings: %v", rerr) + } + if string(b) != findingsFixture { + t.Fatalf("findings.jsonl was modified despite the refusal") + } +} + +// TestAppendVerdictRefusesReingestedFinding is the verdict-misattribution +// regression. review.Run snapshots findings once and then blocks on the operator +// for the whole interactive walk; a concurrent `analyze -ingest` may +// truncate-and-rewrite findings.jsonl in that gap (permitted until the first +// verdict exists), and because finding ids restart at F-001 the verdict the +// analyst gives to the finding they were shown would otherwise attach to a +// different finding now holding the same id — silent corruption of the human +// decision record. AppendVerdict now re-reads under its lock and refuses when the +// targeted id no longer names the finding that was judged. Pre-fix (no re-check) +// the verdict was appended and misattributed. +func TestAppendVerdictRefusesReingestedFinding(t *testing.T) { + dir := writeSession(t) + path := filepath.Join(dir, session.FindingsFile) + + // The finding the analyst was shown (F-001 from the fixture). + shown := analyze.Finding{ + ID: "F-001", T: 22, Type: "bug", Severity: 3, + Quote: "I clicked save and nothing happened", + Evidence: []string{"utt-004", "ev-003"}, + UI: &analyze.UI{Selector: "[data-testid=save-btn]", Route: "#general"}, + Status: "unverified", + } + + // A concurrent re-ingest rewrote findings.jsonl: F-001 now names a DIFFERENT + // finding (same id, different content), exactly what commitFindings permits + // before any verdict exists. + const reingested = `{"id":"F-001","t":48,"type":"friction","severity":2,"quote":"The menu ordering confused me","evidence":["utt-009"],"status":"unverified"} +` + if err := os.WriteFile(path, []byte(reingested), 0o644); err != nil { + t.Fatalf("rewrite findings: %v", err) + } + + rec := analyze.Verdict{Kind: "verdict", Finding: "F-001", Verdict: "confirmed", At: "2026-07-17"} + err := AppendVerdict(dir, rec, &shown) + if err == nil || !strings.Contains(err.Error(), "changed since review started") { + t.Fatalf("expected a changed-finding refusal, got %v", err) + } + // No verdict was written: the re-ingested file is byte-unchanged, so the + // analyst's confirmation cannot be silently pinned to the wrong finding. + b, rerr := os.ReadFile(path) + if rerr != nil { + t.Fatalf("read findings: %v", rerr) + } + if string(b) != reingested { + t.Fatalf("findings.jsonl was modified despite the refusal:\n%s", b) + } +} + +// TestAppendVerdictAcceptsUnchangedFinding is the positive control: when the +// finding still matches what was judged, the verdict is recorded normally, so +// the new guard does not block the ordinary path. +func TestAppendVerdictAcceptsUnchangedFinding(t *testing.T) { + dir := writeSession(t) + shown := analyze.Finding{ + ID: "F-001", T: 22, Type: "bug", Severity: 3, + Quote: "I clicked save and nothing happened", + Evidence: []string{"utt-004", "ev-003"}, + UI: &analyze.UI{Selector: "[data-testid=save-btn]", Route: "#general"}, + Status: "unverified", + } + rec := analyze.Verdict{Kind: "verdict", Finding: "F-001", Verdict: "confirmed", At: "2026-07-17"} + if err := AppendVerdict(dir, rec, &shown); err != nil { + t.Fatalf("AppendVerdict on an unchanged finding: %v", err) + } + _, verdicts, err := analyze.Load(dir) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(verdicts) != 1 || verdicts[0].Finding != "F-001" || verdicts[0].Verdict != "confirmed" { + t.Fatalf("verdict not recorded as expected: %+v", verdicts) + } +} diff --git a/internal/session/session.go b/internal/session/session.go index 6dc20db..fbab0b0 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -43,6 +43,7 @@ type Manifest struct { const ( ManifestFile = "manifest.json" AudioFile = "audio.wav" + AudioOffsetFile = "audio.offset.json" ScreenFile = "screen.mp4" RawEventsFile = "events.rrweb.jsonl" InteractionsFile = "interactions.jsonl" @@ -121,6 +122,18 @@ func (m Manifest) T0() (int64, error) { return m.T0EpochMS, nil } +// 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). +const maxManifestBytes = 1 << 20 // 1 MiB + // LoadManifest reads manifest.json from dir. func LoadManifest(dir string) (Manifest, error) { var m Manifest @@ -133,10 +146,15 @@ func LoadManifest(dir string) (Manifest, error) { return m, fmt.Errorf("load manifest: %w", err) } defer f.Close() - b, err := io.ReadAll(f) + // Read one byte past the cap so an over-large manifest is refused as too big + // rather than silently truncated and then misreported as malformed JSON. + b, err := io.ReadAll(io.LimitReader(f, maxManifestBytes+1)) if err != nil { return m, fmt.Errorf("load manifest: %w", err) } + if len(b) > maxManifestBytes { + return m, fmt.Errorf("load manifest: %s exceeds %d bytes; refusing to read", ManifestFile, maxManifestBytes) + } if err := json.Unmarshal(b, &m); err != nil { return m, fmt.Errorf("parse manifest: %w", err) } diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 7703ba2..3b0b559 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -458,3 +458,47 @@ func TestSafeText(t *testing.T) { } } } + +// TestLoadManifestRefusesOversized covers the last untrusted read on the session +// surface that was unbounded. manifest.json in an exchanged session is +// attacker-controllable, so a multi-gigabyte manifest (a few KB once zipped) +// would OOM any command that loads it via io.ReadAll. LoadManifest now caps the +// read at maxManifestBytes and refuses anything larger, matching every sibling +// reader (ReadJSONL, analyze.Ingest, the demo body caps). Pre-fix this buffered +// the whole file and parsed it. +func TestLoadManifestRefusesOversized(t *testing.T) { + dir := t.TempDir() + // A JSON object padded past the cap with whitespace (valid JSON, so a pre-fix + // LoadManifest would happily parse it): {"session":"s"}. The size is a + // literal, not the maxManifestBytes constant, so this test compiles against and + // exercises the pre-fix behaviour too. + const overCap = (1 << 20) + 4096 + big := make([]byte, overCap) + for i := range big { + big[i] = ' ' + } + copy(big, []byte(`{"session":"s"`)) + big[len(big)-1] = '}' + if err := os.WriteFile(filepath.Join(dir, ManifestFile), big, 0o644); err != nil { + t.Fatalf("write manifest: %v", err) + } + if _, err := LoadManifest(dir); err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("expected an oversize refusal, got %v", err) + } +} + +// TestLoadManifestAcceptsOrdinary guards the ordinary case the cap must not +// break: a normal, small manifest still loads. +func TestLoadManifestAcceptsOrdinary(t *testing.T) { + dir := t.TempDir() + if err := SaveManifest(dir, Manifest{Session: "s", T0EpochMS: 1}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + m, err := LoadManifest(dir) + if err != nil { + t.Fatalf("LoadManifest: %v", err) + } + if m.Session != "s" { + t.Fatalf("want session s, got %q", m.Session) + } +} diff --git a/internal/timeline/timeline.go b/internal/timeline/timeline.go index f046687..71eff1e 100644 --- a/internal/timeline/timeline.go +++ b/internal/timeline/timeline.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io/fs" + "math" "path/filepath" "sort" @@ -171,6 +172,14 @@ func readOptionalJSONL[T any](path string) ([]T, error) { return out, err } +// maxUtteranceSeconds bounds the magnitude of an utterance's session-relative +// t0/t1. A real usability session is minutes to hours; 1e9 seconds (~31 years) +// is far past any genuine value while staying small enough that (t - t0) and the +// report's clock conversion never approach an integer or float hazard. It +// mirrors report.maxClockSeconds so the input boundary and the display sink +// refuse the same set of absurd times. +const maxUtteranceSeconds = 1e9 + // checkedInteractions enforces the two fields that // docs/reference/session-directory.md marks required on an interaction — t and // kind — and converts the accepted records for BuildEntries. Without the check a @@ -189,6 +198,15 @@ func checkedInteractions(path string, raw []rawInteraction) ([]Interaction, erro if r.T == nil { return nil, fmt.Errorf("%s: interaction %d is missing t; cannot place it on the session clock", path, i+1) } + // t is epoch milliseconds, so a value at or below zero anchors the event at + // or before 1 January 1970 — no capture produces that, exactly the reasoning + // session.Manifest.T0 refuses a non-positive anchor on. Refusing it here also + // forecloses the extreme: a t near math.MinInt64 makes (t - t0EpochMS) wrap on + // signed overflow in BuildEntries and plant the event millions of years after + // session start, inflating the report's span while Merge still exits 0. + if *r.T <= 0 { + return nil, fmt.Errorf("%s: interaction %d has t %d; an epoch-millisecond time must be positive", path, i+1, *r.T) + } if r.Kind == "" { return nil, fmt.Errorf("%s: interaction %d is missing kind; an event must name what happened", path, i+1) } @@ -232,8 +250,32 @@ func checkedUtterances(path string, raw []rawUtterance) ([]Utterance, error) { if r.T0 == nil { return nil, fmt.Errorf("%s: utterance %d is missing t0; cannot place it on the session clock", path, i+1) } + // A present t1 is accepted only when it does not precede t0. An explicit + // t1 < t0 is the same inverted-window hazard the nil case is defaulted away + // from: EventsNear would join over [t0-window, t1+window] with hi < lo and + // silently match no event, detaching from the utterance the very + // interactions spoken over it. Falling back to t0 reproduces the documented + // SpeechEnd/nil-t1 answer rather than contradicting it — a missing or + // backwards end can only shrink the join window, never move the utterance. + // Bound the magnitude of t0 (and t1 below). Utterance times are + // session-relative seconds where a small negative value is legitimate + // (speech captured just before t0), so unlike the interaction side's + // positive-epoch-ms rule this checks magnitude, not sign. An astronomically + // large |t0| — a hand-edited transcript's 1e300 — is not a session time: it + // overflows report's float64→int clock into a nonsensical duration and makes + // the utterance's EventsNear join window span the whole session, so it + // silently captures every event away from the utterances they were spoken + // over. This is the speech-side twin of checkedInteractions' range refusal; + // the report sink clock() defends the same class for hand-authored + // timeline.jsonl that reaches it without passing through here. + if math.Abs(*r.T0) > maxUtteranceSeconds { + return nil, fmt.Errorf("%s: utterance %d has t0 %g; a session-relative time in seconds cannot exceed %g in magnitude", path, i+1, *r.T0, maxUtteranceSeconds) + } t1 := *r.T0 - if r.T1 != nil { + if r.T1 != nil && *r.T1 >= *r.T0 { + if math.Abs(*r.T1) > maxUtteranceSeconds { + return nil, fmt.Errorf("%s: utterance %d has t1 %g; a session-relative time in seconds cannot exceed %g in magnitude", path, i+1, *r.T1, maxUtteranceSeconds) + } t1 = *r.T1 } out = append(out, Utterance{ diff --git a/internal/timeline/timeline_test.go b/internal/timeline/timeline_test.go index fc91b3d..a7b6554 100644 --- a/internal/timeline/timeline_test.go +++ b/internal/timeline/timeline_test.go @@ -272,6 +272,55 @@ func TestMergeRejectsUtteranceMissingT0(t *testing.T) { } } +// TestMergeRejectsUtteranceHugeT0 is the magnitude twin of the interaction-side +// range check. Utterance t0/t1 are session-relative seconds (negative is +// legitimate), so the guard bounds magnitude, not sign. A hand-edited transcript +// carrying t0=1e300 is not a session time: it overflows report's float64→int +// clock into a garbage duration and makes the utterance's EventsNear window span +// the whole session, silently stealing every event from the utterances they were +// spoken over. Merge must refuse it and name the line. Pre-fix it merged with +// exit 0. +func TestMergeRejectsUtteranceHugeT0(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + lines := "" + + `{"id":"utt-001","t0":8.0,"t1":12.5,"speaker":"P1","text":"ordinary"}` + "\n" + + `{"id":"utt-002","t0":1e300,"t1":1e300,"speaker":"P1","text":"planted"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TranscriptFile), []byte(lines), 0o644); err != nil { + t.Fatalf("write transcript: %v", err) + } + _, _, err := Merge(dir) + if err == nil { + t.Fatalf("expected an out-of-range t0 error, got nil") + } + if !strings.Contains(err.Error(), "utterance 2") || !strings.Contains(err.Error(), "t0") { + t.Fatalf("error should name the offending line and field, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.TimelineFile)); statErr == nil { + t.Fatalf("timeline.jsonl was written despite the out-of-range utterance") + } +} + +// TestMergeRejectsUtteranceHugeT1 covers the t1 half: a sane t0 but an +// astronomical t1 (t1 >= t0, so it passes the ordering check) must also be +// refused, since the join window's upper bound is built from t1. +func TestMergeRejectsUtteranceHugeT1(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + line := `{"id":"utt-001","t0":8.0,"t1":1e300,"speaker":"P1","text":"planted end"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TranscriptFile), []byte(line), 0o644); err != nil { + t.Fatalf("write transcript: %v", err) + } + _, _, err := Merge(dir) + if err == nil || !strings.Contains(err.Error(), "t1") { + t.Fatalf("expected an out-of-range t1 error, got %v", err) + } +} + // TestMergeAcceptsUtteranceAtT0 guards the other half of the required-field // check, mirroring TestMergeAcceptsInteractionAtT0: an utterance beginning the // moment capture starts carries t0 0, which a value-typed decode cannot @@ -366,3 +415,74 @@ func TestEventsNearWindow(t *testing.T) { t.Fatalf("ev-002 should be outside both windows") } } + +// TestMergeRejectsInteractionNonPositiveT is the range-check twin of +// TestMergeRejectsInteractionMissingT. t is epoch milliseconds, so a value at or +// below zero anchors the event at or before 1 January 1970 — no capture produces +// that, and at the extreme (a t near math.MinInt64) the (t - t0) subtraction in +// BuildEntries wraps on signed overflow and plants the event millions of years +// after the session start, inflating the report span while Merge still exits 0. +// checkedInteractions now refuses a non-positive t, naming the line, and writes +// no timeline. Pre-fix this line merged silently. +func TestMergeRejectsInteractionNonPositiveT(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + lines := "" + + `{"t":` + strconv.FormatInt(t0+9_500, 10) + `,"kind":"click","selector":"[data-testid=save-btn]"}` + "\n" + + `{"t":-9223372036854775808,"kind":"click","selector":"[data-testid=tab-appearance]"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.InteractionsFile), []byte(lines), 0o644); err != nil { + t.Fatalf("write interactions: %v", err) + } + _, _, err := Merge(dir) + if err == nil || !strings.Contains(err.Error(), "interaction 2") { + t.Fatalf("expected a non-positive-t error naming interaction 2, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.TimelineFile)); statErr == nil { + t.Fatalf("timeline.jsonl was written despite the out-of-range interaction time") + } +} + +// TestMergeClampsUtteranceInvertedSpan covers an explicit t1 < t0 in a +// transcript.jsonl. The nil-t1 default already guards a missing end, but an +// explicit backwards end recreates the same inverted join window +// [t0-window, t1+window] (hi < lo) that silently matches no event. Pre-fix the +// value passed through unvalidated; now it falls back to t0, so the utterance's +// own interactions still attach. Alice speaks over an event that must stay joined. +func TestMergeClampsUtteranceInvertedSpan(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + utt := `{"id":"utt-001","t0":22.0,"t1":0.0,"text":"I clicked save and nothing happened."}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TranscriptFile), []byte(utt), 0o644); err != nil { + t.Fatalf("write transcript: %v", err) + } + inter := `{"t":` + strconv.FormatInt(t0+23_000, 10) + `,"kind":"click","selector":"[data-testid=save-btn]"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.InteractionsFile), []byte(inter), 0o644); err != nil { + t.Fatalf("write interactions: %v", err) + } + if _, _, err := Merge(dir); err != nil { + t.Fatalf("Merge: %v", err) + } + entries, err := session.ReadJSONL[Entry](filepath.Join(dir, session.TimelineFile)) + if err != nil { + t.Fatalf("read timeline: %v", err) + } + var speech Entry + for _, e := range entries { + if e.Src == "speech" { + speech = e + } + } + if got := SpeechEnd(speech); got != 22.0 { + t.Fatalf("inverted t1 should clamp to t0 (22.0), SpeechEnd = %v", got) + } + // The event at 23.0s falls in [22-2.5, 22+2.5] once t1 is clamped up to t0, + // so it attaches; with the pre-fix inverted window [19.5, 2.5] it did not. + near := EventsNear(entries, speech, 2.5) + if len(near) != 1 || near[0] != "ev-001" { + t.Fatalf("want the event to attach to the utterance, got %v", near) + } +} diff --git a/internal/transcribe/transcribe.go b/internal/transcribe/transcribe.go index 29eb440..80e18b0 100644 --- a/internal/transcribe/transcribe.go +++ b/internal/transcribe/transcribe.go @@ -10,6 +10,8 @@ package transcribe import ( + "encoding/json" + "errors" "fmt" "io" "math" @@ -82,6 +84,17 @@ func Run(opts Options) (int, error) { if err != nil { return 0, err } + // An external recording's audio.wav is NOT captured at t0, and disk bytes + // cannot distinguish it from a record-origin audio.wav. Persist the offset + // beside audio.wav so a later bare `transcribe` (a re-run with a different + // model, reusing audio.wav) reads it back instead of silently assuming 0 and + // shifting every utterance by the forgotten offset. record-origin audio.wav + // writes no sidecar and correctly defaults to 0. + if external { + if err := writeOffsetSidecar(opts.SessionDir, offset, provenance); err != nil { + return 0, fmt.Errorf("persist audio offset: %w", err) + } + } fmt.Fprintf(opts.Log, "offset: %+.2fs (%s)\n", offset, provenance) var segs []segment @@ -156,9 +169,82 @@ func resolveOffset(opts Options, man session.Manifest, external bool) (float64, } return 0, "default 0: audio creation time unavailable", nil } + // In-place: audio.wav is reused. A prior external -audio run persists the + // offset it derived in a sidecar (see writeOffsetSidecar); honour it so the + // re-run does not silently drop the offset. A record-origin audio.wav has no + // sidecar and correctly defaults to 0 (captured at t0). + if off, provenance, ok, err := readOffsetSidecar(opts.SessionDir); err != nil { + return 0, "", err + } else if ok { + return off, provenance, nil + } return 0, "default 0: session audio.wav captured at t0", nil } +// offsetSidecar is the persisted audio→session offset written beside audio.wav +// after an external -audio conversion, so a later in-place re-run recovers it. +type offsetSidecar struct { + OffsetSeconds float64 `json:"offset_seconds"` + Provenance string `json:"provenance,omitempty"` +} + +// maxOffsetSidecarBytes caps the sidecar read: a genuine one is well under a +// kilobyte, and the session directory is attacker-authorable, so an oversized +// file is refused rather than buffered. +const maxOffsetSidecarBytes = 64 << 10 + +// writeOffsetSidecar persists offset (with its provenance, for the operator) +// beside audio.wav via the no-follow write guard, so a session's own directory +// cannot redirect the write through a planted symlink. +func writeOffsetSidecar(dir string, offset float64, provenance string) error { + b, err := json.Marshal(offsetSidecar{OffsetSeconds: offset, Provenance: provenance}) + if err != nil { + return err + } + return session.WriteFileNoFollow(filepath.Join(dir, session.AudioOffsetFile), append(b, '\n'), 0o644) +} + +// readOffsetSidecar reads the offset sidecar if present. ok is false with no +// error when the file is absent (the record-origin case). A present-but-unusable +// sidecar — unreadable, oversized, malformed, or carrying a non-finite offset — +// is a refuse-with-guidance error rather than a silent fallback to 0: the audio +// is known to be external (only convertAudio writes this file), so guessing 0 +// would reintroduce exactly the silent shift the sidecar exists to prevent. +func readOffsetSidecar(dir string) (offset float64, provenance string, ok bool, err error) { + path := filepath.Join(dir, session.AudioOffsetFile) + f, err := session.OpenFileNoFollowRead(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return 0, "", false, nil + } + return 0, "", false, err + } + defer f.Close() + + b, err := io.ReadAll(io.LimitReader(f, maxOffsetSidecarBytes+1)) + if err != nil { + return 0, "", false, offsetSidecarErr(err) + } + if len(b) > maxOffsetSidecarBytes { + return 0, "", false, offsetSidecarErr(fmt.Errorf("file exceeds %d bytes", maxOffsetSidecarBytes)) + } + var sc offsetSidecar + if err := json.Unmarshal(b, &sc); err != nil { + return 0, "", false, offsetSidecarErr(err) + } + if math.IsNaN(sc.OffsetSeconds) || math.IsInf(sc.OffsetSeconds, 0) { + return 0, "", false, offsetSidecarErr(fmt.Errorf("offset_seconds is not a finite number")) + } + return sc.OffsetSeconds, fmt.Sprintf("persisted: audio.wav converted from an external recording (%+.2fs)", sc.OffsetSeconds), true, nil +} + +// offsetSidecarErr wraps a sidecar fault in the same refuse-with-guidance +// message, telling the operator how to recover the offset explicitly. +func offsetSidecarErr(cause error) error { + return fmt.Errorf("%s is present but unusable (%v); re-run with -audio FILE to re-derive the offset, or pass -offset SECONDS", + session.AudioOffsetFile, cause) +} + // mapSegments converts engine segments to the Utterance schema of // docs/reference/session-directory.md: sequential utt-NNN IDs, offset applied, times // rounded to 2 decimal places, whitespace trimmed, empty segments skipped, diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index 77075c8..f46ea07 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -243,6 +243,58 @@ func TestResolveOffsetInPlaceNoT0(t *testing.T) { } } +// TestResolveOffsetInPlaceReadsSidecar is the re-run-offset-loss regression. A +// first `transcribe -audio rec.m4a` converts the recording into audio.wav and +// derives a nonzero offset; a later bare `transcribe` reuses audio.wav and hits +// the in-place branch. Pre-fix that branch always returned 0, silently shifting +// every utterance by the forgotten offset. The offset is now persisted in a +// sidecar beside audio.wav and read back here. write-then-read round-trips the +// value. +func TestResolveOffsetInPlaceReadsSidecar(t *testing.T) { + dir := t.TempDir() + if err := writeOffsetSidecar(dir, 12.4, "derived: audio creation_time − manifest t0"); err != nil { + t.Fatalf("writeOffsetSidecar: %v", err) + } + off, prov, err := resolveOffset(Options{SessionDir: dir}, session.Manifest{T0EpochMS: 1_700_000_000_000}, false) + if err != nil { + t.Fatalf("in-place with a sidecar must succeed: %v", err) + } + if off != 12.4 { + t.Fatalf("offset: got %v, want 12.4 (from the sidecar, not the naive 0)", off) + } + if !strings.Contains(prov, "persisted") { + t.Fatalf("provenance should name the persisted origin, got %q", prov) + } +} + +// TestResolveOffsetInPlaceRefusesBadSidecar covers the refuse-with-guidance +// fallback: a present-but-unusable sidecar (here malformed JSON) means the audio +// is known external but its offset is unrecoverable, so the run must refuse and +// tell the operator how to state the offset, never silently fall back to 0. +func TestResolveOffsetInPlaceRefusesBadSidecar(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, session.AudioOffsetFile), []byte("{not json"), 0o644); err != nil { + t.Fatalf("write sidecar: %v", err) + } + _, _, err := resolveOffset(Options{SessionDir: dir}, session.Manifest{T0EpochMS: 1}, false) + if err == nil || !strings.Contains(err.Error(), "re-run with -audio") { + t.Fatalf("a malformed sidecar must refuse with guidance, got %v", err) + } +} + +// TestResolveOffsetInPlaceNoSidecarIsRecordOrigin proves the record flow is +// untouched: audio.wav with no sidecar is captured at t0, so the offset is 0. +func TestResolveOffsetInPlaceNoSidecarIsRecordOrigin(t *testing.T) { + dir := t.TempDir() + off, prov, err := resolveOffset(Options{SessionDir: dir}, session.Manifest{T0EpochMS: 1}, false) + if err != nil { + t.Fatalf("record-origin in-place must succeed: %v", err) + } + if off != 0 || !strings.Contains(prov, "captured at t0") { + t.Fatalf("record-origin must be offset 0 captured at t0, got %v (%s)", off, prov) + } +} + // TestResolveOffsetExternalNoT0 is the silent-transcript-corruption regression: // pre-fix resolveOffset took the raw man.T0EpochMS and, on the external // recording path, handed it to deriveOffset unchecked. A received or hand-edited @@ -522,3 +574,43 @@ func TestCheckSessionAudioRefusesFIFO(t *testing.T) { t.Fatalf("a real audio.wav must be accepted, got %v", err) } } + +// TestResolveModelRefusesFIFO is the model-path twin of the FIFO refusals on the +// package's other subprocess-input sites (TestConvertAudioRefusesFIFOInput, +// TestConvertAudioRefusesFIFOOutput, TestCheckSessionAudioRefusesFIFO). The +// resolved -model path is handed to whisper-cli's -m and opened by it, so a FIFO +// there would block that open(2) for ever. resolveModel must fall through to the +// candidate search / not-found error rather than returning the FIFO. Pre-fix the +// `!fi.IsDir()` test accepted it, since a FIFO is not a directory. +func TestResolveModelRefusesFIFO(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "ggml-fifo.bin") + if err := syscall.Mkfifo(fifo, 0o644); err != nil { + t.Skipf("mkfifo unsupported: %v", err) + } + got, err := resolveModel(fifo) + if err == nil { + t.Fatalf("resolveModel returned %q for a FIFO path; want a not-found error", got) + } + if got == fifo { + t.Fatalf("resolveModel returned the FIFO path itself: %q", got) + } +} + +// TestResolveModelAcceptsRegularFile guards the ordinary case the refusal must +// not break: an existing regular ggml file (or a symlink to one, which os.Stat +// resolves) is returned as-is. +func TestResolveModelAcceptsRegularFile(t *testing.T) { + dir := t.TempDir() + model := filepath.Join(dir, "ggml-test.bin") + if err := os.WriteFile(model, []byte("ggml"), 0o644); err != nil { + t.Fatalf("write model: %v", err) + } + got, err := resolveModel(model) + if err != nil { + t.Fatalf("resolveModel(regular file): %v", err) + } + if got != model { + t.Fatalf("want %q, got %q", model, got) + } +} diff --git a/internal/transcribe/whispercpp.go b/internal/transcribe/whispercpp.go index e0e0f2d..ccd730d 100644 --- a/internal/transcribe/whispercpp.go +++ b/internal/transcribe/whispercpp.go @@ -102,8 +102,16 @@ func parseWhisperCpp(raw []byte) ([]segment, error) { // resolveModel turns the -model value into a ggml model file path: an // existing file path is used as-is; otherwise common locations under $HOME // are tried, and the miss carries download guidance. +// A candidate is accepted only when it is a regular file, not merely a +// non-directory: the resolved path is handed to whisper-cli as -m and opened by +// that subprocess, so a FIFO planted at the -model path (or at one of the +// searched candidates in a shared home) would block whisper-cli's open(2) for +// ever waiting for a writer — the same hang convertAudio, checkPlainOutput, and +// checkSessionAudio refuse at the package's other subprocess-input sites. A +// symlink to a real model still resolves, because os.Stat follows it to the +// regular file underneath. func resolveModel(model string) (string, error) { - if fi, err := os.Stat(model); err == nil && !fi.IsDir() { + if fi, err := os.Stat(model); err == nil && fi.Mode().IsRegular() { return model, nil } name := "ggml-" + model + ".bin" @@ -118,7 +126,7 @@ func resolveModel(model string) (string, error) { filepath.Join(home, "models", name), } for _, c := range candidates { - if fi, err := os.Stat(c); err == nil && !fi.IsDir() { + if fi, err := os.Stat(c); err == nil && fi.Mode().IsRegular() { return c, nil } } From 2a293d0c10b560d45715626a40ce55cf0cfe7fc8 Mon Sep 17 00:00:00 2001 From: Alex Reppel <77722411+REPPL@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:55:08 +0100 Subject: [PATCH 2/6] Close a second round of untrusted-session defects across the pipeline A multi-round review after v0.3.0 surfaced these; each fix carries a regression test verified failing against the pre-fix code. Analysis, report, and review integrity: - Roll findings.jsonl back to empty on a failed write, so a partial line no longer bricks the file against its own re-ingest recovery (holdsVerdicts could not parse it to overwrite it). - Neutralise inline Markdown in report.md so an attacker-authored quote cannot smuggle a remote-image tracking beacon into the shareable artefact. - Reject a finding quote that sanitises to empty (a verbatim-check bypass, since Contains(text, "") is always true). - Refuse duplicate finding ids on load, naming the line, instead of collapsing them under one status in the report. - Bound interaction-time magnitude at merge, matching the utterance-side twin. - Sanitise finding ids in review error strings before they reach the terminal. Untrusted-session filesystem handling: - Route analyze -ingest/-out through the no-follow guard (symlink/FIFO refusal), as every other session-artefact read and write already is. - Refuse a symlinked in-place audio.wav (read-side out-of-session exfil into the re-shareable transcript.jsonl). - Distinguish a missing audio.wav from an unreadable one. - Bound the derived and persisted audio offset at transcribe, where the bad recording metadata enters, rather than one command later at merge. Capture reliability (record/transcribe, macOS): - Time out avfoundation device enumeration so a wedged device or driver cannot hang record before it can be interrupted. - Bound retained recorder stderr so a device-stall log flood over a long session cannot exhaust memory and orphan the recorders. - Convert audio via a temp file renamed on success, so an interrupted or crashed ffmpeg never leaves a partial audio.wav a later run would transcribe as whole. - Flag a force-stopped recorder so a truncated, unplayable screen.mp4 is not reported as good by the size-only check. Behaviour change: analyze -ingest/-out now require a regular file and reject named pipes, process substitution, and /dev/stdout; use -ingest - for stdin and omit -out for stdout. Recorded in CHANGELOG. Assisted-by: Claude:claude-fable-5 --- CHANGELOG.md | 62 ++++++++++++ internal/analyze/analyze.go | 14 +++ internal/analyze/analyze_test.go | 82 +++++++++++++++ internal/analyze/ingest.go | 56 ++++++++--- internal/analyze/validate.go | 9 +- internal/cli/cli.go | 14 ++- internal/cli/cli_test.go | 71 +++++++++++++ internal/record/proc.go | 55 +++++++++-- internal/record/record.go | 17 +++- internal/record/record_test.go | 86 ++++++++++++++++ internal/record/recorders.go | 21 +++- internal/record/tcc.go | 25 +++++ internal/report/report.go | 132 ++++++++++++++++--------- internal/report/report_test.go | 84 ++++++++++++++++ internal/review/review.go | 17 ++-- internal/review/review_test.go | 31 ++++++ internal/timeline/timeline.go | 22 ++++- internal/timeline/timeline_test.go | 28 ++++++ internal/transcribe/ffmpeg.go | 36 ++++++- internal/transcribe/transcribe.go | 59 +++++++++-- internal/transcribe/transcribe_test.go | 113 +++++++++++++++++++++ 21 files changed, 944 insertions(+), 90 deletions(-) create mode 100644 internal/cli/cli_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index aaeefd3..0dfdf1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,68 @@ called out in a **Breaking** section. ## [Unreleased] +A second robustness pass over the same capture → analysis pipeline, closing the +defects a multi-round review surfaced after v0.3.0. Every fix carries a +regression test; the review finished with two consecutive passes finding only +nitpicks. + +### Fixed + +Evidence-record and validation integrity: + +- A failed write of `findings.jsonl` (a full disk part-way through) no longer + leaves a truncated fragment that bricks the file against its own recovery — + the re-ingest path could not even parse it to overwrite it. The write now rolls + back to an empty, re-ingestable file. +- `report.md`, the shareable evidence artefact, no longer passes attacker-authored + text through as live Markdown: an image or link in a transcript quote (e.g. a + remote-image tracking beacon) is neutralised so it renders as literal text. +- A finding whose verbatim quote consists only of stripped control/bidi + characters is rejected, rather than passing the quote check against an empty + string. +- A hand-edited `findings.jsonl` carrying two findings with the same `id` is + refused on load, naming the line, instead of silently collapsing them under one + status in the report. +- An interaction whose epoch-millisecond time is astronomically large is refused + at merge (matching the existing utterance-side bound), rather than producing a + session span the report can only render as `--:--`. + +Robustness against malformed or exchanged sessions: + +- `analyze -ingest FILE` and `analyze -out FILE` now go through the same + symlink/FIFO-refusing guard every other session-artefact read and write uses, so + a pointer into an untrusted session directory cannot redirect the write out of + the session or hang the read on a planted FIFO. +- The session's own `audio.wav` is refused when it is a symlink, so a shared + session cannot redirect transcription at an out-of-session file whose words would + then land in the (re-shareable) `transcript.jsonl`. +- A missing-vs-unreadable `audio.wav` is distinguished, so a permissions or + symlink-loop error is no longer misreported as "no audio, run record first". +- A finding id embedded in a `review` error message is sanitised before it reaches + the terminal, closing an ANSI-escape path through a hand-authored id. +- A derived or persisted audio offset beyond a plausible magnitude is refused at + `transcribe`, where the bad recording metadata enters, rather than one command + later at `merge`. + +Capture reliability (`record`/`transcribe`, macOS): + +- Device enumeration now runs under a timeout, so a wedged capture device or + driver no longer hangs `record` before it can be interrupted. +- A recorder's captured stderr is bounded, so a device-stall log flood over a long + session cannot exhaust memory and orphan the recorders. +- Voice-recording conversion writes to a temp file and renames on success, so an + interrupted or crashed `ffmpeg` never leaves a partial `audio.wav` a later run + would transcribe as if complete. +- A recorder that had to be force-stopped (it missed the finalisation grace) is + flagged, so a truncated, unplayable `screen.mp4` is no longer reported as good. + +### Changed + +- **Behaviour:** `analyze -ingest` and `analyze -out` now require a regular file + and reject named pipes, shell process substitution (`-ingest <(…)`), and + `/dev/stdout`. Use `-ingest -` to read the answer from stdin, and omit `-out` to + write the request to stdout — the supported equivalents. + ## [0.3.0] - 2026-07-22 A robustness release. No new commands: the pipeline gains no surface, but the diff --git a/internal/analyze/analyze.go b/internal/analyze/analyze.go index d84cc11..3cde60b 100644 --- a/internal/analyze/analyze.go +++ b/internal/analyze/analyze.go @@ -107,6 +107,16 @@ func Load(dir string) ([]Finding, []Verdict, error) { func ParseRecords(r io.Reader, name string) ([]Finding, []Verdict, error) { var findings []Finding var verdicts []Verdict + // Finding ids must be unique across the file. Ingest already rejects duplicates + // in an answer (validate), but a hand-edited or exchanged findings.jsonl can carry + // two findings sharing one id — and every id-keyed consumer then silently + // misbehaves: EffectiveStatus collapses them onto one status entry, so one verdict + // paints both findings in the report, and review's findByID only ever reaches the + // first. Refusing the file here, naming the offending line, turns that + // display-collapse into an honest error the operator can repair, and keeps the + // id-uniqueness EffectiveStatus/findByID assume true actually true on every path + // that loads findings. + seenID := map[string]bool{} sc := bufio.NewScanner(r) sc.Buffer(make([]byte, 0, 64*1024), session.MaxJSONLLine) line := 0 @@ -144,6 +154,10 @@ func ParseRecords(r io.Reader, name string) ([]Finding, []Verdict, error) { if err := json.Unmarshal(raw, &fnd); err != nil { return nil, nil, fmt.Errorf("%s:%d: %w", name, line, err) } + if seenID[fnd.ID] { + return nil, nil, fmt.Errorf("%s:%d: duplicate finding id %q; each finding must have a unique id", name, line, fnd.ID) + } + seenID[fnd.ID] = true findings = append(findings, fnd) } if err := sc.Err(); err != nil { diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index ed92067..0fde40c 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -1,6 +1,7 @@ package analyze import ( + "errors" "fmt" "os" "path/filepath" @@ -12,6 +13,65 @@ import ( "github.com/REPPL/Testimony/internal/session" ) +// failAfterWriter is a findingsFile whose Write fails, recording whether the +// caller truncated back to 0 *after* the failed write — the rollback writeFindings +// must perform so a partial write never bricks findings.jsonl against its own +// recovery. The post-write ordering matters: writeFindings also truncates to 0 +// before writing, so only a truncate that follows the write attempt proves the +// rollback ran. +type failAfterWriter struct { + wrote bool + rolledBack bool +} + +func (w *failAfterWriter) Write(p []byte) (int, error) { + w.wrote = true + return 0, errors.New("no space left on device") +} +func (w *failAfterWriter) Truncate(size int64) error { + if w.wrote && size == 0 { + w.rolledBack = true + } + return nil +} +func (w *failAfterWriter) Seek(offset int64, whence int) (int64, error) { return 0, nil } + +// TestWriteFindingsRollsBackOnWriteError is the corrupt-on-failure regression for +// commitFindings. It runs f.Truncate(0) before writing, so a short write (ENOSPC) +// used to leave a truncated JSON fragment that not only broke every reader but +// blocked the recovery path — the next analyze -ingest's holdsVerdicts errors on +// the fragment before it can rewrite. writeFindings must roll the file back to +// empty (parseable, re-ingestable) on any write error. Pre-fix (neutralise the +// `f.Truncate(0)` in writeFindings' error path to demonstrate) no truncate follows +// the failed write and this fails. +func TestWriteFindingsRollsBackOnWriteError(t *testing.T) { + w := &failAfterWriter{} + err := writeFindings(w, []Finding{{ID: "F-001", T: 1, Type: "bug", Severity: 3, Quote: "x", Evidence: []string{"utt-001"}, Status: "unverified"}}) + if err == nil { + t.Fatal("writeFindings returned nil on a failing write; want the write error") + } + if !w.rolledBack { + t.Fatal("writeFindings did not truncate back to empty after the failed write; a partial line would survive and brick re-ingest") + } +} + +// TestIngestRejectsQuoteThatSanitisesToEmpty is the verbatim-bypass regression. A +// quote of only stripped characters (a lone U+202E) is raw-non-empty but SafeText +// reduces it to "", and strings.Contains(text, "") is always true, so pre-fix the +// verbatim-substring gate passed for a quote never spoken and the finding was +// written. Ingest must now refuse it. +func TestIngestRejectsQuoteThatSanitisesToEmpty(t *testing.T) { + dir := writeSession(t, timelineFixture) + answer := "{\"findings\":[{\"id\":\"F-001\",\"t\":22,\"type\":\"bug\",\"severity\":3,\"quote\":\"‮\",\"evidence\":[\"utt-004\"]}]}" + _, err := Ingest(dir, strings.NewReader(answer)) + if err == nil || !strings.Contains(err.Error(), "quote must be non-empty") { + t.Fatalf("expected a sanitised-empty quote refusal, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.FindingsFile)); statErr == nil { + t.Fatalf("findings.jsonl was written despite a quote that sanitises to nothing") + } +} + // timelineFixture is a minimal merged timeline: one spoken utterance and two // events, enough to exercise every validation rule. sessionEnd is 28 (utt-004 // t1). @@ -146,6 +206,28 @@ func TestIngestDuplicateID(t *testing.T) { } } +// TestLoadRejectsDuplicateFindingID is the display-collapse regression on the read +// side. Ingest already blocks duplicate ids in an answer, but a hand-edited or +// exchanged findings.jsonl can carry two findings sharing one id; every id-keyed +// consumer (EffectiveStatus, review.findByID) then silently misbehaves. ParseRecords +// must refuse the file and name the offending line so the id-uniqueness those +// consumers assume is actually enforced on every load path. +func TestLoadRejectsDuplicateFindingID(t *testing.T) { + dir := t.TempDir() + dup := `{"id":"F-001","t":22,"type":"bug","severity":3,"quote":"a","evidence":["utt-004"],"status":"unverified"}` + "\n" + + `{"id":"F-001","t":23,"type":"friction","severity":2,"quote":"b","evidence":["utt-004"],"status":"unverified"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(dup), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + _, _, err := Load(dir) + if err == nil || !strings.Contains(err.Error(), "duplicate finding id") { + t.Fatalf("expected a duplicate-finding-id refusal naming line 2, got %v", err) + } + if !strings.Contains(err.Error(), ":2:") { + t.Fatalf("refusal should name the offending line, 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/analyze/ingest.go b/internal/analyze/ingest.go index e29a778..cbe55db 100644 --- a/internal/analyze/ingest.go +++ b/internal/analyze/ingest.go @@ -140,28 +140,60 @@ func commitFindings(dir string, findings []Finding) error { f.Close() return fmt.Errorf("refusing to overwrite %s: it already holds verdict records (the retained precision record)", session.FindingsFile) } - if err := f.Truncate(0); err != nil { + if err := writeFindings(f, findings); err != nil { f.Close() - return fmt.Errorf("write findings: %w", err) + return err } - if _, err := f.Seek(0, io.SeekStart); err != nil { - f.Close() + // Close releases the lock with the descriptor. + if err := f.Close(); err != nil { return fmt.Errorf("write findings: %w", err) } - w := bufio.NewWriter(f) - enc := json.NewEncoder(w) + return nil +} + +// findingsFile is the subset of *os.File writeFindings needs; a fake satisfies it +// in tests to exercise the truncate-then-write rollback. +type findingsFile interface { + io.Writer + Truncate(size int64) error + Seek(offset int64, whence int) (int64, error) +} + +// writeFindings replaces f's contents with the findings, one JSON object per line, +// rolling the file back to empty if the write only partly lands. +// +// The whole set is encoded into one buffer before f is truncated, so the truncate +// and the write are a single Write of pre-built bytes rather than a streamed series +// of encodes that a mid-way I/O error (ENOSPC) could leave half-flushed. That +// matters because commitFindings ran f.Truncate(0) first: without the rollback a +// short write left findings.jsonl holding a truncated, newline-less JSON fragment, +// which not only breaks every reader but blocks the tool's own recovery — the next +// analyze -ingest calls holdsVerdicts, which json.Unmarshals every line and errors +// on the fragment before it can conclude "no verdicts" and rewrite. On any write +// error the file is therefore truncated back to empty: an empty findings.jsonl is +// parseable (zero findings, zero verdicts) and re-ingestable, so the failure state +// no longer forecloses its own repair. This is the same partial-write rollback +// review.writeVerdict and demo.appendRecords already apply; commitFindings was the +// one writer without it. The set is bounded by oversizedFindings before this runs, +// so buffering it whole holds one bounded answer, not an unbounded stream. +func writeFindings(f findingsFile, findings []Finding) error { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) for _, v := range findings { if err := enc.Encode(v); err != nil { - f.Close() return fmt.Errorf("write findings: %w", err) } } - if err := w.Flush(); err != nil { - f.Close() + if err := f.Truncate(0); err != nil { return fmt.Errorf("write findings: %w", err) } - // Close releases the lock with the descriptor. - if err := f.Close(); err != nil { + if _, err := f.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("write findings: %w", err) + } + if _, err := f.Write(buf.Bytes()); err != nil { + // Best-effort roll back to an empty (parseable, re-ingestable) file, then + // surface the original error. + f.Truncate(0) return fmt.Errorf("write findings: %w", err) } return nil @@ -291,7 +323,7 @@ func decodeFinding(raw json.RawMessage) (Finding, error) { // the precision history the guard exists to protect. func holdsVerdicts(f io.Reader, path string) (bool, error) { sc := bufio.NewScanner(f) - sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) + sc.Buffer(make([]byte, 0, 64*1024), session.MaxJSONLLine) for sc.Scan() { raw := sc.Bytes() if len(bytes.TrimSpace(raw)) == 0 { diff --git a/internal/analyze/validate.go b/internal/analyze/validate.go index 50441bb..604e6e0 100644 --- a/internal/analyze/validate.go +++ b/internal/analyze/validate.go @@ -195,7 +195,14 @@ func validate(findings []positioned, idx timelineIndex) []error { // already sanitised via the index): the agent copies the quote from the // sanitised request, so validating against the raw utterance would reject an // honest verbatim copy whenever the utterance carried a stripped character. - if f.Quote == "" { + // The quote is compared in SafeText form, so its emptiness must be judged + // there too. A raw quote of only stripped characters (e.g. a lone U+202E) is + // non-empty and clears the first check, but SafeText reduces it to "" and + // strings.Contains(anyText, "") is always true — so the verbatim-substring + // gate would pass for a quote the participant never spoke, and the finding + // would carry a quote that sanitises to nothing. Reject the sanitised-empty + // quote explicitly, before the substring test that "" trivially satisfies. + if session.SafeText(f.Quote) == "" { errs = append(errs, fmt.Errorf("%s: quote must be non-empty", label)) } else if !containsAny(uttTexts, session.SafeText(f.Quote)) { errs = append(errs, fmt.Errorf("%s: quote is not a verbatim substring of any cited evidence utterance", label)) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index c3c2ed4..ecf9f0c 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -178,7 +178,12 @@ func Run(args []string) int { } in := os.Stdin if *ingest != "-" { - f, err := os.Open(*ingest) + // Read the answer file through the no-follow guard, like every other + // session-surface read: the operator naturally saves the model's answer + // beside the session (e.g. sessions/x/answer.json), and a session is an + // exchange unit — a received one can ship a FIFO at that name (plain + // os.Open blocks in open(2) for ever) or a symlink out of the directory. + f, err := session.OpenFileNoFollowRead(*ingest) if err != nil { return fail(err) } @@ -198,7 +203,12 @@ func Run(args []string) int { return fail(err) } if *out != "" { - if err := os.WriteFile(*out, []byte(prompt), 0o644); err != nil { + // Write through the no-follow guard, matching the report.md write above and + // every other session-surface write: the operator naturally directs -out at + // a path beside the session (e.g. sessions/x/request.md), and a received + // session can ship a symlink there that plain os.WriteFile would follow, + // truncating an arbitrary operator-writable file outside the session. + if err := session.WriteFileNoFollow(*out, []byte(prompt), 0o644); err != nil { return fail(err) } fmt.Printf("wrote %s\n", *out) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go new file mode 100644 index 0000000..37fc853 --- /dev/null +++ b/internal/cli/cli_test.go @@ -0,0 +1,71 @@ +package cli + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/REPPL/Testimony/internal/session" +) + +// miniSession writes a minimal but valid session (manifest + one timeline entry) +// so `analyze` reaches its -out write / -ingest read without failing earlier. +func miniSession(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", App: "app", Participant: "P1"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + tl := `{"t":0,"src":"speech","id":"utt-001","payload":{"speaker":"P1","t1":1,"text":"hi"}}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + return dir +} + +// TestAnalyzeOutRefusesSymlink is the F6 write-side regression: `analyze -out` used +// plain os.WriteFile, which follows a symlink planted at the output name in an +// exchanged session and truncates an arbitrary file outside it. Routed through +// session.WriteFileNoFollow, the write is refused and the outside target is untouched. +func TestAnalyzeOutRefusesSymlink(t *testing.T) { + dir := miniSession(t) + outside := filepath.Join(t.TempDir(), "victim") + if err := os.WriteFile(outside, []byte("ORIGINAL"), 0o600); err != nil { + t.Fatalf("seed victim: %v", err) + } + out := filepath.Join(dir, "request.md") + if err := os.Symlink(outside, out); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if code := Run([]string{"analyze", "-session", dir, "-out", out}); code == 0 { + t.Fatal("analyze -out followed a symlink; want a non-zero exit") + } + if b, _ := os.ReadFile(outside); string(b) != "ORIGINAL" { + t.Fatalf("out-of-session file overwritten through symlink: %q", b) + } +} + +// TestAnalyzeIngestRefusesFIFO is the F6 read-side regression: `analyze -ingest FILE` +// used plain os.Open, which blocks in open(2) for ever on a FIFO planted at the +// answer name in an exchanged session — Ingest's byte cap never helps because the +// open never returns. Routed through session.OpenFileNoFollowRead, the FIFO is +// refused at once. The test runs Run in a goroutine and fails on timeout. +func TestAnalyzeIngestRefusesFIFO(t *testing.T) { + dir := miniSession(t) + fifo := filepath.Join(dir, "answer.json") + if err := syscall.Mkfifo(fifo, 0o644); err != nil { + t.Skipf("FIFOs unavailable: %v", err) + } + done := make(chan int, 1) + go func() { done <- Run([]string{"analyze", "-session", dir, "-ingest", fifo}) }() + select { + case code := <-done: + if code == 0 { + t.Fatal("analyze -ingest of a FIFO returned success; want refusal") + } + case <-time.After(5 * time.Second): + t.Fatal("analyze -ingest blocked on a FIFO instead of refusing it") + } +} diff --git a/internal/record/proc.go b/internal/record/proc.go index 655a869..6038e76 100644 --- a/internal/record/proc.go +++ b/internal/record/proc.go @@ -1,7 +1,6 @@ package record import ( - "bytes" "os" "os/exec" "sync" @@ -35,28 +34,60 @@ func (e *execProc) Signal(sig os.Signal) error { func (e *execProc) Wait() error { return e.cmd.Wait() } -// lockedBuffer is a concurrency-safe sink for a child's stderr: os/exec copies -// stderr on its own goroutine while the lifecycle reads the tail from another. +// stderrRetain caps how much of a child's stderr lockedBuffer keeps. A record +// session is long-running by design, and avfoundation floods stderr when a device +// stalls ("frame dropped", mux-queue warnings), so an unbounded buffer could grow by +// hundreds of MB over a session and OOM the parent — which, because each recorder is +// in its own process group with the parent as the only signaller, would orphan the +// ffmpeg children still recording. Only the trailing bytes are ever read (tail() uses +// ≤1200), so retaining a bounded window loses nothing diagnostic while bounding memory. +const stderrRetain = 8 << 10 + +// lockedBuffer is a concurrency-safe, memory-bounded sink for a child's stderr: +// os/exec copies stderr on its own goroutine while the lifecycle reads the tail from +// another. It keeps only the trailing stderrRetain bytes. type lockedBuffer struct { - mu sync.Mutex - buf bytes.Buffer + mu sync.Mutex + buf []byte // trailing bytes only, len capped at stderrRetain + dropped bool // true once any earlier bytes were discarded } func (l *lockedBuffer) Write(p []byte) (int, error) { l.mu.Lock() defer l.mu.Unlock() - return l.buf.Write(p) + n := len(p) + if n >= stderrRetain { + // This write alone overflows the window; keep only its own tail, without + // growing the backing array. + l.buf = append(l.buf[:0], p[n-stderrRetain:]...) + l.dropped = true + return n, nil + } + l.buf = append(l.buf, p...) + if len(l.buf) > stderrRetain { + // Compact the trailing window to the front of the same backing array + // (overlapping forward copy, which append/copy handle), so memory stays bounded. + drop := len(l.buf) - stderrRetain + l.buf = append(l.buf[:0], l.buf[drop:]...) + l.dropped = true + } + return n, nil } -// tail returns the trailing portion of the captured stderr for error messages. +// tail returns the trailing portion of the captured stderr for error messages, +// prefixing an ellipsis whenever earlier bytes were dropped (by the retention cap or +// the tail bound), so a truncation is visible rather than silent. func (l *lockedBuffer) tail() string { l.mu.Lock() defer l.mu.Unlock() const max = 1200 - b := l.buf.Bytes() + b := l.buf if len(b) > max { return "…" + string(b[len(b)-max:]) } + if l.dropped { + return "…" + string(b) + } return string(b) } @@ -68,6 +99,7 @@ type liveChild struct { started time.Time // when watching began; used to tell a start-up failure from a mid-session stop done chan struct{} // closed once, when Wait returns err error // Wait result; read only after done is closed + killed bool // set by stopChild when the grace expired and SIGKILL was sent; read after stopAll } // watch starts the single reaper goroutine. It must be called exactly once, @@ -94,7 +126,14 @@ func stopChild(c *liveChild, grace time.Duration) { select { case <-c.done: case <-time.After(grace): + // The recorder did not finalise its container within the grace period. SIGKILL + // leaves whatever bytes were flushed — for an MP4, whose moov atom is written + // only on clean shutdown, that is very likely a truncated, unplayable file. Mark + // it so finaliseOutputs surfaces the risk instead of blessing the file by size. + // killed is written here on the caller's goroutine (stopAll, sequential) and + // read only after stopAll returns, so no synchronisation is needed. _ = c.p.Signal(syscall.SIGKILL) + c.killed = true <-c.done } } diff --git a/internal/record/record.go b/internal/record/record.go index ce12763..9dd826d 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -228,7 +228,22 @@ func stopDemo(srv *http.Server) { func finaliseOutputs(dir string, children []*liveChild) (audioReady bool, problems []string) { for _, c := range children { out := expectedOutput(dir, c.stream) - if fi, err := os.Stat(out); err == nil && fi.Size() > 0 { + fi, err := os.Stat(out) + hasData := err == nil && fi.Size() > 0 + if c.killed { + // The recorder was force-terminated because it did not finalise within the + // grace period, so even a non-empty file may be truncated/unplayable (an MP4 + // missing its moov atom especially). Surface it rather than let the size + // check bless a broken artefact. A PCM WAV survives a kill largely intact + // (only the RIFF sizes go stale), so a present audio.wav is still offered for + // transcription; a killed screen.mp4 is not consumed downstream regardless. + problems = append(problems, classifyKilledOutput(c.stream, filepath.Base(out), hasData, c.stderr.tail())) + if c.stream == streamMicrophone && hasData { + audioReady = true + } + continue + } + if hasData { if c.stream == streamMicrophone { audioReady = true } diff --git a/internal/record/record_test.go b/internal/record/record_test.go index 4d676ed..ae2f317 100644 --- a/internal/record/record_test.go +++ b/internal/record/record_test.go @@ -821,6 +821,92 @@ func TestStopChildEscalatesToSIGKILL(t *testing.T) { } } +// TestLockedBufferBoundsMemory is the OOM regression: a device-stall stderr flood +// over a long session used to grow lockedBuffer without bound, and an OOM parent +// orphans the ffmpeg children still recording. The buffer must retain only a bounded +// trailing window while still surfacing the most recent (diagnostic) bytes. +func TestLockedBufferBoundsMemory(t *testing.T) { + var b lockedBuffer + // Simulate a flood far larger than the retention window: 4 MiB in 4 KiB writes. + chunk := bytes.Repeat([]byte("frame dropped\n"), 300) // ~4 KiB + total := 0 + for i := 0; i < 1024; i++ { + n, err := b.Write(chunk) + if err != nil || n != len(chunk) { + t.Fatalf("Write returned (%d,%v), want (%d,nil)", n, err, len(chunk)) + } + total += len(chunk) + } + if total <= stderrRetain { + t.Fatalf("test wrote %d bytes, not enough to exceed the %d retention cap", total, stderrRetain) + } + // Retained bytes are bounded regardless of how much was written. + if len(b.buf) > stderrRetain { + t.Fatalf("lockedBuffer retained %d bytes, exceeding the %d cap", len(b.buf), stderrRetain) + } + // The tail is still available and marks the elision, so diagnostics survive. + tail := b.tail() + if !strings.HasPrefix(tail, "…") { + t.Fatalf("tail after a flood must mark the dropped prefix with an ellipsis, got %q…", tail[:min(20, len(tail))]) + } + if !strings.Contains(tail, "frame dropped") { + t.Fatalf("tail must retain the most recent stderr content, got %q", tail) + } + + // A small total (under the cap) is retained verbatim with no ellipsis. + var s lockedBuffer + s.Write([]byte("short output")) + if got := s.tail(); got != "short output" { + t.Fatalf("under-cap tail must be verbatim, got %q", got) + } +} + +// TestFinaliseOutputsFlagsSIGKILLedRecorder is the truncated-artefact regression: a +// recorder force-stopped after missing the finalisation grace can leave a non-empty +// but unplayable file (an MP4 with no moov atom), which the size-only check used to +// bless as good. finaliseOutputs must flag a killed recorder even when its file has +// bytes; a killed PCM audio.wav (which survives a kill) is still offered. +func TestFinaliseOutputsFlagsSIGKILLedRecorder(t *testing.T) { + dir := t.TempDir() + // Both streams left a non-empty file, but both were SIGKILLed at stop. + if err := os.WriteFile(filepath.Join(dir, session.AudioFile), []byte("RIFF....partial"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, session.ScreenFile), []byte("\x00\x00\x00\x18ftyp....truncated"), 0o644); err != nil { + t.Fatal(err) + } + mic := newLiveChild(streamMicrophone, newFakeProc(syscall.SIGKILL), &lockedBuffer{}) + scr := newLiveChild(streamScreen, newFakeProc(syscall.SIGKILL), &lockedBuffer{}) + // Force the SIGKILL escalation (short grace) so both are marked killed. + stopChild(mic, 5*time.Millisecond) + stopChild(scr, 5*time.Millisecond) + if !mic.killed || !scr.killed { + t.Fatalf("stopChild must mark a SIGKILLed recorder killed: mic=%v scr=%v", mic.killed, scr.killed) + } + + audioReady, problems := finaliseOutputs(dir, []*liveChild{mic, scr}) + // Both killed recorders are flagged despite non-empty files. + if len(problems) != 2 { + t.Fatalf("both SIGKILLed recorders must be flagged, got %d problems: %v", len(problems), problems) + } + var sawAudio, sawScreen bool + for _, p := range problems { + if strings.Contains(p, session.AudioFile) && strings.Contains(p, "force-stopped") { + sawAudio = true + } + if strings.Contains(p, session.ScreenFile) && strings.Contains(p, "truncated") { + sawScreen = true + } + } + if !sawAudio || !sawScreen { + t.Fatalf("expected force-stop warnings for both streams, got %v", problems) + } + // A killed WAV that has bytes is still offered for transcription (PCM survives a kill). + if !audioReady { + t.Fatalf("a present (if kill-truncated) audio.wav should still be offered for transcription") + } +} + func TestAnyExitReportsDeadChild(t *testing.T) { live := newLiveChild(streamMicrophone, newFakeProc(syscall.SIGINT), &lockedBuffer{}) dead := newLiveChild(streamScreen, newFakeProc(syscall.SIGINT), &lockedBuffer{}) diff --git a/internal/record/recorders.go b/internal/record/recorders.go index ad7f171..5e4e1f3 100644 --- a/internal/record/recorders.go +++ b/internal/record/recorders.go @@ -1,14 +1,25 @@ package record import ( + "context" "errors" "fmt" "os/exec" "regexp" "strconv" "strings" + "time" ) +// deviceListTimeout bounds the avfoundation device enumeration. It is a var, not a +// const, only so a test could shrink it; production never reassigns it. Enumeration +// normally returns in well under a second, but a wedged CoreAudio/camera driver or a +// stuck TCC daemon can block the AVFoundation query indefinitely, and probeDevices +// runs before the interrupt handler can stop anything — so without a deadline a single +// bad device hangs `testimony record` forever, with the session dir already created +// and nothing recording. +var deviceListTimeout = 15 * time.Second + // micArgs builds the ffmpeg argv that captures the system default microphone to // a canonical 16 kHz mono PCM WAV — exactly the parameters // transcribe.convertAudio produces, so the file is canonical ASR input needing @@ -125,8 +136,16 @@ func selectDevices(video, audio []avDevice, wantScreen bool) (screenIndex int, m // returns the detected audio-input names for the caller to log. Impure (spawns // ffmpeg); isolated here and skipped in CI. func probeDevices(ffmpeg string, wantScreen bool) (screenIndex int, mics []string, err error) { - cmd := exec.Command(ffmpeg, "-hide_banner", "-f", "avfoundation", "-list_devices", "true", "-i", "") + ctx, cancel := context.WithTimeout(context.Background(), deviceListTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, ffmpeg, "-hide_banner", "-f", "avfoundation", "-list_devices", "true", "-i", "") out, runErr := cmd.CombinedOutput() + // A wedged device/driver can make the enumeration block for ever; the context + // deadline turns that into an actionable error rather than a silent hang before + // any recorder (or the interrupt handler) is live. + if ctx.Err() == context.DeadlineExceeded { + return 0, nil, fmt.Errorf("ffmpeg avfoundation device listing timed out after %s — a capture device or driver is unresponsive; disconnect or disable it, then re-run", deviceListTimeout) + } // ffmpeg prints the listing to stderr and then exits non-zero by design, so an // *exec.ExitError is expected and ignored — the listing is still in out. But a // failure to run ffmpeg at all (a corrupt or wrong-architecture binary, a diff --git a/internal/record/tcc.go b/internal/record/tcc.go index 4cd1f9a..7c5788b 100644 --- a/internal/record/tcc.go +++ b/internal/record/tcc.go @@ -111,6 +111,31 @@ func classifyMissingOutput(stream, artefact, stderrTail string) string { return b.String() } +// classifyKilledOutput turns a recorder that had to be SIGKILLed at stop — because +// it did not finalise its container within the grace period — into an actionable +// message. hasData distinguishes the two cases: a non-empty file that is likely +// truncated/unplayable (the container trailer never got written), versus nothing at +// all. Either way the operator is told the artefact cannot be trusted, so a broken +// recording is not silently presented as complete. Pure: the caller supplies the +// stream, artefact name, whether any bytes landed, and the stderr tail. +func classifyKilledOutput(stream, artefact string, hasData bool, stderrTail string) string { + tail := strings.TrimSpace(stderrTail) + + var b strings.Builder + if hasData { + fmt.Fprintf(&b, "%s capture did not finalise %s within the shutdown grace period and was force-stopped, so the file is likely truncated or unplayable (its container trailer was never written).\n", stream, artefact) + fmt.Fprintf(&b, "Re-record if you need this stream; a longer session or a slow disk can need more time to finalise.") + } else { + fmt.Fprintf(&b, "%s capture produced no usable %s: the recorder was force-stopped before writing any output.\n", stream, artefact) + fmt.Fprintf(&b, "Re-record if you need this stream.") + } + + if tail != "" { + fmt.Fprintf(&b, "\n\nffmpeg output:\n%s", tail) + } + return b.String() +} + // writeExitCode appends the child's exit error in parentheses when present. func writeExitCode(b *strings.Builder, exitErr error) { if exitErr != nil { diff --git a/internal/report/report.go b/internal/report/report.go index 234c7b3..efb9539 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -47,45 +47,48 @@ func Render(dir string, window float64) (string, error) { } } - // Attach each event to the first utterance whose window contains it. The - // buckets are keyed by the utterance's position in speech, never by its ID: - // timeline.Merge copies a transcript's id verbatim and never validates it, so - // a transcript whose lines omit "id" gives every utterance the ID "" and an - // ID-keyed map collapses them all into one bucket — every utterance would then - // render every event attached to any of them. report.md is the human evidence - // artefact, so that silently fabricates the record of what the participant was - // doing while they spoke. Indexing by position removes the dependence on ID - // uniqueness entirely. The used[] dedup below still keys on event ids, which - // merge synthesises uniquely as ev-%03d. + // Attach each event to the first utterance whose window contains it. Both the + // buckets and the used[] dedup key on POSITION, never on ID: timeline.Merge + // copies a transcript's id and never validates it, and report reads + // timeline.jsonl directly — an exchanged or hand-edited one bypasses merge's + // ev-%03d synthesis entirely — so several utterances or several events can share + // one id. An ID-keyed utterance bucket collapses same-id utterances into one, and + // an ID-keyed event lookup (the pre-fix inner `for e := range events { if e.ID == + // id }`) attaches EVERY event sharing a matched id — including ones outside the + // window — to that utterance, then the id-keyed standalone dedup hides them. + // report.md is the human evidence artefact, so both fabricate the record of what + // the participant was doing while they spoke. Indexing by position on both sides + // removes the dependence on ID uniqueness; the window test is inlined here (it is + // timeline.EventsNear's body) so events is scanned by index rather than by id. attached := make([][]timeline.Entry, len(speech)) // utterance index → events - used := map[string]bool{} + usedEvent := make([]bool, len(events)) // event index → already attached for i, u := range speech { - for _, id := range timeline.EventsNear(entries, u, window) { - if used[id] { + lo := u.T - window + hi := timeline.SpeechEnd(u) + window + for j, e := range events { + if usedEvent[j] { continue } - used[id] = true - for _, e := range events { - if e.ID == id { - attached[i] = append(attached[i], e) - } + if e.T >= lo && e.T <= hi { + usedEvent[j] = true + attached[i] = append(attached[i], e) } } } var b strings.Builder - fmt.Fprintf(&b, "# Session report — %s\n\n", session.SafeText(man.Session)) + fmt.Fprintf(&b, "# Session report — %s\n\n", mdInline(man.Session)) fmt.Fprintf(&b, "**App:** %s · **Participant:** %s · **Duration:** %s · **Utterances:** %d · **Events:** %d\n\n", - session.SafeText(orDash(man.App)), session.SafeText(orDash(man.Participant)), clock(end(entries)), len(speech), len(events)) + mdInline(orDash(man.App)), mdInline(orDash(man.Participant)), clock(end(entries)), len(speech), len(events)) if len(man.Tasks) > 0 { - fmt.Fprintf(&b, "**Tasks:** %s\n\n", session.SafeText(strings.Join(man.Tasks, "; "))) + fmt.Fprintf(&b, "**Tasks:** %s\n\n", mdInline(strings.Join(man.Tasks, "; "))) } b.WriteString("## Timeline\n\n") ei := 0 // index into events, for standalone (unattached) ones flushStandaloneBefore := func(t float64) { for ei < len(events) && events[ei].T < t { - if !used[events[ei].ID] { + if !usedEvent[ei] { fmt.Fprintf(&b, "- [%s] %s\n", clock(events[ei].T), eventLine(events[ei])) } ei++ @@ -155,12 +158,12 @@ func renderFindings(b *strings.Builder, dir string) { } for _, f := range group { fmt.Fprintf(b, "- **%s** %s · severity %d · [%s] — “%s” — %s", - session.SafeText(f.ID), session.SafeText(f.Type), f.Severity, clock(f.T), session.SafeText(f.Quote), findingAnchor(f)) + mdInline(f.ID), mdInline(f.Type), f.Severity, clock(f.T), mdInline(f.Quote), findingAnchor(f)) if st := eff[f.ID]; st.At != "" { if st.Of != "" { - fmt.Fprintf(b, " · %s of %s (%s)", session.SafeText(st.Value), session.SafeText(st.Of), session.SafeText(st.At)) + fmt.Fprintf(b, " · %s of %s (%s)", mdInline(st.Value), mdInline(st.Of), mdInline(st.At)) } else { - fmt.Fprintf(b, " · %s (%s)", session.SafeText(st.Value), session.SafeText(st.At)) + fmt.Fprintf(b, " · %s (%s)", mdInline(st.Value), mdInline(st.At)) } } b.WriteString("\n") @@ -175,14 +178,14 @@ func findingAnchor(f analyze.Finding) string { if f.UI != nil && (f.UI.Selector != "" || f.UI.Route != "") { var parts []string if f.UI.Selector != "" { - parts = append(parts, "`"+session.SafeText(f.UI.Selector)+"`") + parts = append(parts, mdCode(f.UI.Selector)) } if f.UI.Route != "" { - parts = append(parts, session.SafeText(f.UI.Route)) + parts = append(parts, mdInline(f.UI.Route)) } return strings.Join(parts, " ") } - return "evidence " + session.SafeText(strings.Join(f.Evidence, ", ")) + return "evidence " + mdInline(strings.Join(f.Evidence, ", ")) } func end(entries []timeline.Entry) float64 { @@ -239,44 +242,81 @@ func clock(sec float64) string { return fmt.Sprintf("%s%02d:%02d", sign, s/60, s%60) } +// mdInline neutralises the inline Markdown an attacker-authored string could +// otherwise smuggle into report.md, the shareable evidence artefact. session.SafeText +// already strips the C0/C1/bidi bytes and — decisively — the newlines that could forge +// block structure (a heading, a list item), so a sanitised string can never begin a +// line and only INLINE constructs remain reachable; SafeText passes their triggers +// (`\ ` * _ [ ] ( ) ! < > ~`, backtick included) through untouched. Without this an +// event or finding text of `![x](http://host/beacon.png)` renders a live remote image +// — a tracking/exfil beacon fired the instant the shared report is opened in any +// Markdown viewer — and `[label](http://host)` an active link disguised as evidence. +// Backslash-escaping each trigger renders it as literal text in a viewer and keeps it +// readable in source. Ordinary transcript, selector, and route text carries none of +// these bytes, so the report of a normal session is byte-for-byte unchanged. +func mdInline(s string) string { + s = session.SafeText(s) + var b strings.Builder + b.Grow(len(s)) + for _, r := range s { + switch r { + case '\\', '`', '*', '_', '[', ']', '(', ')', '!', '<', '>', '~': + b.WriteByte('\\') + } + b.WriteRune(r) + } + return b.String() +} + +// mdCode renders untrusted text inside a Markdown code span, where backslash escapes +// do not apply. session.SafeText leaves the backtick that would close the span early +// and let the tail render as active markup, so backticks are stripped from the span +// content (a real CSS selector or route never carries one); everything else is literal +// inside the span and needs no escaping. +func mdCode(s string) string { + return "`" + strings.ReplaceAll(session.SafeText(s), "`", "") + "`" +} + func speaker(u timeline.Entry) string { if s, ok := u.Payload["speaker"].(string); ok && s != "" { - return session.SafeText(s) + return mdInline(s) } return "P?" } func text(u timeline.Entry) string { if s, ok := u.Payload["text"].(string); ok { - return session.SafeText(s) + return mdInline(s) } return "" } -// eventLine renders one event. Every payload string is passed through -// session.SafeText: an event's kind/selector/route come from the unauthenticated -// capture endpoint (or an attacker-authored timeline in a downloaded session), -// so stripping control bytes here stops newline-forged report structure and ANSI -// injection into report.md. +// eventLine renders one event. Every payload string is untrusted — an event's +// kind/selector/route/text/value come from the unauthenticated capture endpoint +// (or an attacker-authored timeline in a downloaded session) — so each is routed +// through mdInline (plain sinks) or mdCode (the selector code span). Both apply +// session.SafeText, stripping the control bytes that forge report structure or +// inject ANSI, and additionally neutralise the inline Markdown (an image beacon, +// an active link) that SafeText alone leaves intact. func eventLine(e timeline.Entry) string { - get := func(k string) string { + raw := func(k string) string { if s, ok := e.Payload[k].(string); ok { - return session.SafeText(s) + return s } return "" } - parts := []string{get("kind")} - if sel := get("selector"); sel != "" { - parts = append(parts, "`"+sel+"`") + parts := []string{mdInline(raw("kind"))} + if sel := raw("selector"); sel != "" { + parts = append(parts, mdCode(sel)) } - if t := get("text"); t != "" { - parts = append(parts, fmt.Sprintf("%q", t)) + if t := raw("text"); t != "" { + parts = append(parts, `"`+mdInline(t)+`"`) } - if v := get("value"); v != "" { - parts = append(parts, fmt.Sprintf("value=%q", v)) + if v := raw("value"); v != "" { + parts = append(parts, `value="`+mdInline(v)+`"`) } - if r := get("route"); r != "" { - parts = append(parts, "("+r+")") + if r := raw("route"); r != "" { + parts = append(parts, "("+mdInline(r)+")") } return strings.Join(parts, " ") } diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 7bedda4..51c1346 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -238,6 +238,90 @@ func TestReportClockRoundsSymmetrically(t *testing.T) { } } +// TestReportAttachesEventsByPositionNotDuplicateID is the event-side twin of +// TestReportAttachesEventsPerUtteranceWithoutIDs. report reads timeline.jsonl +// directly, so an exchanged or hand-edited one can carry two events sharing an id +// that merge would have made unique. Pre-fix the attach loop matched events by id, +// so an in-window event pulled its far-away same-id twin under the utterance too and +// the id-keyed standalone dedup then hid it — fabricating what the participant did +// while speaking. Attaching by position must keep the far event standalone, at its +// own time, and off the utterance. +func TestReportAttachesEventsByPositionNotDuplicateID(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "fixture", App: "app", Participant: "Alice"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + // Two events share id "ev-001": one at 10.5 s (inside the utterance window at + // 10 s), one at 9000 s (far outside). Only the near one may attach. + tl := `{"t":10,"src":"speech","id":"utt-001","payload":{"speaker":"Alice","t1":11,"text":"spoke here"}} +{"t":10.5,"src":"event","id":"ev-001","payload":{"kind":"click","selector":"near"}} +{"t":9000,"src":"event","id":"ev-001","payload":{"kind":"click","selector":"far"}} +` + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render: %v", err) + } + // The far event renders exactly once, as a standalone at its own [150:00] stamp, + // never nested under the utterance. Pre-fix it appeared under the utterance and + // was suppressed from the standalone flush. + if n := strings.Count(md, "`far`"); n != 1 { + t.Fatalf("far event rendered %d times, want exactly 1:\n%s", n, md) + } + // It renders as a top-level standalone ("- ", column 0), never nested under the + // utterance (" - ", indented) — the pre-fix bug attached it under the utterance. + if !strings.Contains(md, "\n- [150:00] click `far`") { + t.Fatalf("far same-id event was not rendered as a standalone at its own time:\n%s", md) + } + if strings.Contains(md, " - [150:00]") { + t.Fatalf("far same-id event was wrongly nested under the utterance:\n%s", md) + } + // The near one attaches under the utterance (indented list item at its own 00:11). + if !strings.Contains(md, " - [00:11] click `near`") { + t.Fatalf("near event did not attach to its utterance:\n%s", md) + } +} + +// TestReportNeutralisesInlineMarkdown is the beacon-injection regression. report.md +// is the shareable evidence artefact; an attacker-authored quote or event text of +// `![x](http://evil/beacon.png)` used to survive verbatim, so opening the report in +// any Markdown viewer fired a remote-image request — a tracking/exfil beacon. The +// active image/link markup must be neutralised (backslash-escaped) so it renders as +// literal text, while the words stay legible. +func TestReportNeutralisesInlineMarkdown(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "fixture", App: "app", Participant: "Alice"}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + // An utterance whose text is an image-beacon, and a finding whose quote is one. + tl := "{\"t\":1,\"src\":\"speech\",\"id\":\"utt-001\",\"payload\":{\"speaker\":\"Alice\",\"t1\":2,\"text\":\"![t](http://evil/a.png)\"}}\n" + if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { + t.Fatalf("write timeline: %v", err) + } + findings := "{\"id\":\"F-001\",\"t\":1,\"type\":\"bug\",\"severity\":3,\"quote\":\"![q](http://evil/b.png)\",\"evidence\":[\"utt-001\"],\"status\":\"unverified\"}\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(findings), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + md, err := Render(dir, 2.5) + if err != nil { + t.Fatalf("Render: %v", err) + } + // No active image markup survives: the `!` immediately followed by `[` and the + // `](` link opener are the beacon triggers, and neither may appear unescaped. + if strings.Contains(md, "![t](http://evil/a.png)") || strings.Contains(md, "![q](http://evil/b.png)") { + t.Fatalf("active image-beacon markdown survived into report.md:\n%s", md) + } + if strings.Contains(md, "](http://evil") { + t.Fatalf("an unescaped link/image opener survived into report.md:\n%s", md) + } + // The words are still present (escaped, not stripped), so the evidence stays legible. + if !strings.Contains(md, "evil/a.png") || !strings.Contains(md, "evil/b.png") { + t.Fatalf("neutralisation dropped the text instead of escaping it:\n%s", md) + } +} + func findingLines(t *testing.T, dir string) []string { t.Helper() b, err := os.ReadFile(filepath.Join(dir, session.FindingsFile)) diff --git a/internal/review/review.go b/internal/review/review.go index f99a1ed..edc9889 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -209,15 +209,20 @@ func record(opts Options, judged analyze.Finding, rec analyze.Verdict) error { // checkTargets validates that the finding exists and, for a duplicate, that the // target exists and differs. func checkTargets(findings []analyze.Finding, id, verdict, of string) error { + // id/of print through session.SafeText even though every current caller passes + // an operator flag or an IsFindingID-validated value: making the terminal-safety + // local here, rather than a property of caller invariants, keeps a future caller + // that passes an attacker-authored id straight from findings.jsonl from + // reintroducing the ANSI-injection this mirrors from the verdict-error paths. if !contains(findings, id) { - return fmt.Errorf("finding %s not found", id) + return fmt.Errorf("finding %s not found", session.SafeText(id)) } if verdict == "duplicate" { if of == id { return fmt.Errorf("a finding cannot be a duplicate of itself") } if !contains(findings, of) { - return fmt.Errorf("duplicate target %s not found", of) + return fmt.Errorf("duplicate target %s not found", session.SafeText(of)) } } return nil @@ -266,7 +271,7 @@ func AppendVerdict(dir string, v analyze.Verdict, expect *analyze.Finding) error // review, report, and holdsVerdicts would fail with "token too long". if len(b)+1 > session.MaxJSONLLine { return fmt.Errorf("verdict for %s encodes to %d bytes, over the %d-byte %s line limit", - v.Finding, len(b)+1, session.MaxJSONLLine, session.FindingsFile) + session.SafeText(v.Finding), len(b)+1, session.MaxJSONLLine, session.FindingsFile) } path := filepath.Join(dir, session.FindingsFile) // O_RDWR rather than O_WRONLY because the record cannot be framed correctly @@ -331,15 +336,15 @@ func verifyTarget(f *os.File, v analyze.Verdict, expect analyze.Finding) error { cur := findByID(findings, v.Finding) if cur == nil { return fmt.Errorf("finding %s is no longer in %s; it changed since review started — re-run `testimony review`", - v.Finding, session.FindingsFile) + session.SafeText(v.Finding), session.FindingsFile) } if !analyze.SameIdentity(*cur, expect) { return fmt.Errorf("finding %s changed since review started (a re-analysis rewrote %s); re-run `testimony review` before recording a verdict", - v.Finding, session.FindingsFile) + session.SafeText(v.Finding), session.FindingsFile) } if v.Verdict == "duplicate" && findByID(findings, v.Of) == nil { return fmt.Errorf("duplicate target %s is no longer in %s; re-run `testimony review`", - v.Of, session.FindingsFile) + session.SafeText(v.Of), session.FindingsFile) } return nil } diff --git a/internal/review/review_test.go b/internal/review/review_test.go index fe75ae7..e872619 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -323,6 +323,37 @@ func TestAppendVerdictTerminatesAnUnterminatedLastLine(t *testing.T) { } } +// TestVerifyTargetErrorSanitisesFindingID is the terminal-injection regression for +// verifyTarget's error strings. A finding id in a hand-authored or exchanged +// findings.jsonl is attacker-controlled; review SafeTexts it at every other terminal +// sink (printFinding, describe), but the verifyTarget/AppendVerdict error strings +// embedded it raw via %s, and cli.fail prints those to stderr — so an ESC-bearing id +// drove ANSI escape sequences into the operator's terminal. Every finding-id error +// path must route the id through session.SafeText. Here the verdict targets an id not +// present in the file (cur == nil), firing the "no longer in" error. +func TestVerifyTargetErrorSanitisesFindingID(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(findingsFixture), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + // An id carrying an ESC (0x1b) and a bell (0x07) — an ANSI/OSC sequence — that is + // not present in the file, so verifyTarget takes its cur==nil branch. + evilID := "\x1b]0;pwned\x07F-404" + rec := analyze.Verdict{Kind: "verdict", Finding: evilID, Verdict: "confirmed", At: "2026-07-17"} + expect := &analyze.Finding{ID: evilID, T: 1, Type: "bug", Severity: 3, Quote: "x", Evidence: []string{"utt-001"}} + err := AppendVerdict(dir, rec, expect) + if err == nil { + t.Fatal("AppendVerdict accepted a verdict for an absent finding; want a mismatch error") + } + if strings.ContainsRune(err.Error(), '\x1b') || strings.ContainsRune(err.Error(), '\x07') { + t.Fatalf("verifyTarget error carries raw terminal-control bytes from the finding id: %q", err.Error()) + } + // The id's printable tail still identifies the finding to the operator. + if !strings.Contains(err.Error(), "F-404") { + t.Fatalf("sanitised error dropped the finding id entirely: %q", err.Error()) + } +} + // shortWriteFile is a verdictFile whose Write persists a prefix and then errors, // standing in for a full disk (write(2) fills the remaining space, returns a // short count, and the next write returns ENOSPC — os.File.Write persists the diff --git a/internal/timeline/timeline.go b/internal/timeline/timeline.go index 71eff1e..6f815ac 100644 --- a/internal/timeline/timeline.go +++ b/internal/timeline/timeline.go @@ -141,7 +141,10 @@ func SpeechEnd(e Entry) float64 { } // EventsNear returns the IDs of event entries that fall inside the utterance -// span [u.T0-window, u.T1+window]. Used by the report's join step. +// span [u.T0-window, u.T1+window]. report inlines this same window test in its +// join step (indexing events by position, not id, so duplicate ids in an +// exchanged timeline cannot misattribute); this remains the documented statement +// of the join window. func EventsNear(entries []Entry, u Entry, window float64) []string { lo := u.T - window hi := SpeechEnd(u) + window @@ -192,7 +195,7 @@ const maxUtteranceSeconds = 1e9 // for the same reason: it would join the timeline naming no observed action. // Refusing the whole merge names the offending line so the operator repairs the // capture instead of reading a corrupted account of the session. -func checkedInteractions(path string, raw []rawInteraction) ([]Interaction, error) { +func checkedInteractions(path string, t0EpochMS int64, raw []rawInteraction) ([]Interaction, error) { out := make([]Interaction, 0, len(raw)) for i, r := range raw { if r.T == nil { @@ -207,6 +210,19 @@ func checkedInteractions(path string, raw []rawInteraction) ([]Interaction, erro if *r.T <= 0 { return nil, fmt.Errorf("%s: interaction %d has t %d; an epoch-millisecond time must be positive", path, i+1, *r.T) } + // Bound the resulting session-relative magnitude, the epoch-ms twin of + // checkedUtterances' |t0| ≤ maxUtteranceSeconds check. The sign check above + // rules out the negative extreme, but a huge positive t (up to MaxInt64) still + // yields rel = (t − t0)/1000 of ~9e15 s — no session time — which merge writes + // while exiting 0, then report's end() reports as the session span and clock() + // renders as the broken "--:--", and it also inflates analyze.indexTimeline's + // idx.end so a finding may be anchored anywhere up to it. checkedUtterances + // refuses the same absurd magnitude on the speech side; without this the + // interaction side was the asymmetric gap, admitting a session-relative time + // its own twin rejects. t0 is the manifest anchor Merge resolved for this call. + if rel := float64(*r.T-t0EpochMS) / 1000.0; math.Abs(rel) > maxUtteranceSeconds { + return nil, fmt.Errorf("%s: interaction %d has t %d, a session-relative time of %gs that exceeds %g in magnitude", path, i+1, *r.T, rel, maxUtteranceSeconds) + } if r.Kind == "" { return nil, fmt.Errorf("%s: interaction %d is missing kind; an event must name what happened", path, i+1) } @@ -330,7 +346,7 @@ func Merge(dir string) (speech, events int, err error) { } } - ints, err := checkedInteractions(intsPath, raw) + ints, err := checkedInteractions(intsPath, t0, raw) if err != nil { return 0, 0, err } diff --git a/internal/timeline/timeline_test.go b/internal/timeline/timeline_test.go index a7b6554..884685a 100644 --- a/internal/timeline/timeline_test.go +++ b/internal/timeline/timeline_test.go @@ -209,6 +209,34 @@ func TestMergeRejectsInteractionMissingKind(t *testing.T) { } } +// TestMergeRejectsInteractionHugeT is the magnitude twin the interaction side was +// missing: the sign check refuses t <= 0, but a huge positive t (here ~9e18 ms) +// passes it and yields a session-relative time of ~9e15 s — no session time. Pre-fix +// merge wrote it and exited 0, then report rendered the span as the broken "--:--" +// and analyze.indexTimeline's idx.end inflated to admit findings anchored anywhere up +// to it. checkedUtterances already bounds the same magnitude on the speech side +// (TestMergeRejectsUtteranceHugeT0); checkedInteractions must too, naming the line and +// writing no timeline. +func TestMergeRejectsInteractionHugeT(t *testing.T) { + dir := t.TempDir() + if err := session.SaveManifest(dir, session.Manifest{Session: "s", T0EpochMS: t0}); err != nil { + t.Fatalf("SaveManifest: %v", err) + } + lines := "" + + `{"t":` + strconv.FormatInt(t0+9_500, 10) + `,"kind":"click","selector":"[data-testid=save-btn]"}` + "\n" + + `{"t":9000000000000000000,"kind":"click","selector":"[data-testid=tab-appearance]"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.InteractionsFile), []byte(lines), 0o644); err != nil { + t.Fatalf("write interactions: %v", err) + } + _, _, err := Merge(dir) + if err == nil || !strings.Contains(err.Error(), "interaction 2") { + t.Fatalf("expected an out-of-range-magnitude error naming interaction 2, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.TimelineFile)); statErr == nil { + t.Fatalf("timeline.jsonl was written despite the out-of-range interaction time") + } +} + // TestMergeAcceptsInteractionAtT0 guards the other half of the required-field // check: an interaction captured at exactly t0 has a relative time of 0, which a // value-typed decode cannot distinguish from an absent "t". Alice clicking the diff --git a/internal/transcribe/ffmpeg.go b/internal/transcribe/ffmpeg.go index 3644f17..0e2dd2b 100644 --- a/internal/transcribe/ffmpeg.go +++ b/internal/transcribe/ffmpeg.go @@ -40,9 +40,39 @@ func convertAudio(in, out string) error { if err != nil { return fmt.Errorf("ffmpeg not found on PATH (needed to produce the 16 kHz mono %s): brew install ffmpeg", session.AudioFile) } - cmd := exec.Command(ffmpeg, "-y", "-i", in, "-ac", "1", "-ar", "16000", "-c:a", "pcm_s16le", out) - if raw, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("ffmpeg: %w\n%s", err, tail(raw)) + // Convert into a temp file beside out, then rename over out only on success (see + // atomicConvert), so an interrupted or crashed ffmpeg (Ctrl+C, SIGKILL, ENOSPC) + // never leaves a partial audio.wav that a later bare `transcribe` would silently + // treat as the whole recording. + return atomicConvert(out, func(tmpPath string) error { + cmd := exec.Command(ffmpeg, "-y", "-i", in, "-ac", "1", "-ar", "16000", "-c:a", "pcm_s16le", tmpPath) + if raw, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("ffmpeg: %w\n%s", err, tail(raw)) + } + return nil + }) +} + +// atomicConvert runs a producer that writes the converted audio to a temp file beside +// out, then renames it over out only if the producer succeeded. If the producer +// returns an error — including one raised after it has already written a partial temp, +// as a signalled or ENOSPC-hit ffmpeg does — out is left untouched and the temp is +// removed, so a failed conversion never leaves a truncated file that a later run would +// mistake for the whole recording. The temp shares out's directory so the rename stays +// on one filesystem and is atomic. The producer receives the temp path. +func atomicConvert(out string, produce func(tmpPath string) error) error { + tmp, err := os.CreateTemp(filepath.Dir(out), ".audio-*.wav") + if err != nil { + return fmt.Errorf("audio convert: create temp: %w", err) + } + tmpPath := tmp.Name() + tmp.Close() // the producer reopens by path; we only needed the reserved name + defer os.Remove(tmpPath) + if err := produce(tmpPath); err != nil { + return err + } + if err := os.Rename(tmpPath, out); err != nil { + return fmt.Errorf("audio convert: finalise %s: %w", out, err) } return nil } diff --git a/internal/transcribe/transcribe.go b/internal/transcribe/transcribe.go index 80e18b0..d432802 100644 --- a/internal/transcribe/transcribe.go +++ b/internal/transcribe/transcribe.go @@ -121,15 +121,36 @@ func Run(opts Options) (int, error) { // through session.OpenFileNoFollow's regular-file guard, and mere existence is // not enough to establish that reading it will terminate: in a session that was // shared or downloaded rather than recorded here, a FIFO planted at audio.wav -// satisfies os.Stat and then blocks the engine's read for ever, hanging -// `testimony transcribe` on a session the operator merely received. A symlink is -// resolved by os.Stat and needs no refusal here — a symlink redirects writes, -// and this path is only ever read. +// blocks the engine's read for ever, hanging `testimony transcribe` on a session +// the operator merely received. +// +// os.Lstat, not os.Stat, so a symlink at audio.wav is refused rather than +// resolved. The pre-fix comment reasoned "a symlink redirects writes, and this +// path is only ever read" and waived the refusal — but a symlink at the in-place +// audio.wav redirects the READ too: a received session shipping audio.wav -> +// /some/private/recording.wav makes the engine transcribe that out-of-session +// file into transcript.jsonl, which lives inside the (re-shareable) session +// directory. That is the exact read-side exfil session.OpenFileNoFollowRead +// refuses everywhere else; the in-place audio read was the one exempt path. A +// record-origin or convertAudio-produced audio.wav is always a real regular file, +// never a symlink, so nothing legitimate is refused. (An operator-named -audio +// symlink stays fine: convertAudio still os.Stat-resolves it, because that path +// the operator named themselves rather than received.) func checkSessionAudio(wav, sessionDir string) error { - fi, err := os.Stat(wav) + fi, err := os.Lstat(wav) if err != nil { - return fmt.Errorf("no %s in session %s and no -audio given: run `testimony record` first, or pass -audio FILE", - session.AudioFile, sessionDir) + // Reserve the "no audio.wav, run record first" guidance for a genuinely + // absent file. A different Stat failure — EACCES on the session directory, a + // symlink loop (ELOOP), an I/O error — must not be misreported as absence, + // which would send the operator to re-record a session whose audio exists. + if errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("no %s in session %s and no -audio given: run `testimony record` first, or pass -audio FILE", + session.AudioFile, sessionDir) + } + return fmt.Errorf("reading %s: %w", wav, err) + } + if fi.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to read %s: it is a symlink", wav) } if !fi.Mode().IsRegular() { return fmt.Errorf("refusing to read %s: it is not a regular file", wav) @@ -165,6 +186,13 @@ func resolveOffset(opts Options, man session.Manifest, external bool) (float64, return 0, "", fmt.Errorf("deriving audio offset: %w", err) } if off, ok := deriveOffset(opts.Audio, t0); ok { + // A derived offset beyond the session-time magnitude is not a real capture + // timing — it means the recording's creation_time (attacker-influenceable + // metadata) or the manifest t0 is bogus. Refuse rather than derive a + // decades-long shift that mapSegments would bake into every utterance. + if math.Abs(off) > maxOffsetSeconds { + return 0, "", fmt.Errorf("derived audio offset %+.2fs exceeds %g in magnitude; the recording's creation_time or the manifest t0 is implausible — pass -offset SECONDS to state it explicitly", off, maxOffsetSeconds) + } return off, "derived: audio creation_time − manifest t0", nil } return 0, "default 0: audio creation time unavailable", nil @@ -193,6 +221,16 @@ type offsetSidecar struct { // file is refused rather than buffered. const maxOffsetSidecarBytes = 64 << 10 +// maxOffsetSeconds bounds the audio→session offset, mirroring +// timeline.maxUtteranceSeconds (1e9 s, ~31 years). The offset is added to every +// utterance time, so an offset beyond this alone forces every merged time past the +// magnitude timeline.checkedUtterances refuses — merge would reject the whole +// transcript one command later. Bounding it here, at derivation and at sidecar read, +// fails the run where the bad value enters (a poisoned recording creation_time, or a +// hand-edited sidecar) rather than leaving a transcript.jsonl that only merge rejects, +// so the operator learns which input is wrong. +const maxOffsetSeconds = 1e9 + // writeOffsetSidecar persists offset (with its provenance, for the operator) // beside audio.wav via the no-follow write guard, so a session's own directory // cannot redirect the write through a planted symlink. @@ -235,6 +273,13 @@ func readOffsetSidecar(dir string) (offset float64, provenance string, ok bool, if math.IsNaN(sc.OffsetSeconds) || math.IsInf(sc.OffsetSeconds, 0) { return 0, "", false, offsetSidecarErr(fmt.Errorf("offset_seconds is not a finite number")) } + // Bound the persisted offset the same as the derived one: a hand-edited sidecar + // carrying an astronomical offset would otherwise shift every re-run's utterance + // past the magnitude merge accepts, and refusing here names the sidecar as the + // fault rather than letting merge reject the resulting transcript. + if math.Abs(sc.OffsetSeconds) > maxOffsetSeconds { + return 0, "", false, offsetSidecarErr(fmt.Errorf("offset_seconds %+.2fs exceeds %g in magnitude", sc.OffsetSeconds, maxOffsetSeconds)) + } return sc.OffsetSeconds, fmt.Sprintf("persisted: audio.wav converted from an external recording (%+.2fs)", sc.OffsetSeconds), true, nil } diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index f46ea07..19ae24b 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -282,6 +282,35 @@ func TestResolveOffsetInPlaceRefusesBadSidecar(t *testing.T) { } } +// TestResolveOffsetRefusesOversizedSidecar is the offset-magnitude regression: a +// hand-edited sidecar carrying an astronomical offset (finite, so it clears the +// NaN/Inf check) would shift every re-run's utterance past the magnitude merge +// accepts, so the corruption surfaces one command later at merge. Bounding the offset +// at read time refuses with guidance naming the sidecar as the fault. A large-but- +// in-bounds offset still round-trips. +func TestResolveOffsetRefusesOversizedSidecar(t *testing.T) { + dir := t.TempDir() + if err := writeOffsetSidecar(dir, 5e9, "derived: audio creation_time − manifest t0"); err != nil { + t.Fatalf("writeOffsetSidecar: %v", err) + } + _, _, err := resolveOffset(Options{SessionDir: dir}, session.Manifest{T0EpochMS: 1}, false) + if err == nil || !strings.Contains(err.Error(), "re-run with -audio") { + t.Fatalf("an oversized sidecar offset must refuse with guidance, got %v", err) + } + + // A large but in-bounds offset (10 hours) is still accepted and round-trips. + if err := writeOffsetSidecar(dir, 36000, "derived: audio creation_time − manifest t0"); err != nil { + t.Fatalf("writeOffsetSidecar: %v", err) + } + off, _, err := resolveOffset(Options{SessionDir: dir}, session.Manifest{T0EpochMS: 1}, false) + if err != nil { + t.Fatalf("an in-bounds sidecar offset must be accepted: %v", err) + } + if off != 36000 { + t.Fatalf("offset: got %v, want 36000", off) + } +} + // TestResolveOffsetInPlaceNoSidecarIsRecordOrigin proves the record flow is // untouched: audio.wav with no sidecar is captured at t0, so the offset is 0. func TestResolveOffsetInPlaceNoSidecarIsRecordOrigin(t *testing.T) { @@ -422,12 +451,59 @@ func TestConvertAudioIntegration(t *testing.T) { if err != nil || fi.Size() == 0 { t.Fatalf("expected non-empty %s: %v", session.AudioFile, err) } + // Atomicity: a successful conversion leaves no `.audio-*.wav` temp behind (it was + // renamed over out, and the deferred cleanup is a no-op). + if temps, _ := filepath.Glob(filepath.Join(dir, ".audio-*.wav")); len(temps) != 0 { + t.Fatalf("conversion left temp files behind: %v", temps) + } if err := convertAudio(filepath.Join(dir, "voice.mp3"), out); err == nil { t.Fatal("unsupported extension must error") } } +// TestAtomicConvertLeavesNoPartialOnFailure is the interrupted-conversion regression, +// hermetic (no ffmpeg): a producer that writes a partial temp and then fails — exactly +// what a Ctrl+C'd or ENOSPC-hit ffmpeg does — must leave out untouched and the temp +// removed, so a later bare `transcribe` never mistakes a truncated fragment for the +// whole recording. Pre-fix (ffmpeg wrote straight to out) the fragment survived at out. +func TestAtomicConvertLeavesNoPartialOnFailure(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, session.AudioFile) + + // Producer writes a partial WAV to the temp, then reports failure (interrupted). + err := atomicConvert(out, func(tmpPath string) error { + if werr := os.WriteFile(tmpPath, []byte("RIFF....partial fragment"), 0o644); werr != nil { + t.Fatalf("seed partial temp: %v", werr) + } + return errors.New("ffmpeg: killed by signal") + }) + if err == nil { + t.Fatal("atomicConvert must surface the producer's error") + } + // out must not exist: the partial temp was never renamed into place. + if _, statErr := os.Stat(out); !os.IsNotExist(statErr) { + t.Fatalf("a failed conversion left a partial %s behind (err=%v)", session.AudioFile, statErr) + } + // The temp must be cleaned up. + if temps, _ := filepath.Glob(filepath.Join(dir, ".audio-*.wav")); len(temps) != 0 { + t.Fatalf("a failed conversion left temp files behind: %v", temps) + } + + // A succeeding producer renames its temp into place and leaves no temp. + if err := atomicConvert(out, func(tmpPath string) error { + return os.WriteFile(tmpPath, []byte("RIFF....complete"), 0o644) + }); err != nil { + t.Fatalf("atomicConvert on success: %v", err) + } + if b, rerr := os.ReadFile(out); rerr != nil || string(b) != "RIFF....complete" { + t.Fatalf("successful conversion did not land the full output: %q (err=%v)", b, rerr) + } + if temps, _ := filepath.Glob(filepath.Join(dir, ".audio-*.wav")); len(temps) != 0 { + t.Fatalf("successful conversion left temp files behind: %v", temps) + } +} + func TestResolveVAD(t *testing.T) { cases := []struct{ pref, want string }{ {"", "silero"}, @@ -575,6 +651,43 @@ func TestCheckSessionAudioRefusesFIFO(t *testing.T) { } } +// TestCheckSessionAudioRefusesSymlink is the read-side exfil regression. A session +// is an exchange unit, so a received one can ship audio.wav as a symlink to a file +// outside the session — a private recording. Pre-fix checkSessionAudio used os.Stat, +// which resolves the link to its regular target and returns nil, so the engine +// transcribes that out-of-session file into transcript.jsonl inside the re-shareable +// session directory. os.Lstat must refuse the symlink, matching the read-side stance +// session.OpenFileNoFollowRead takes everywhere else. A symlink to a genuinely absent +// target must also be reported as a symlink, not misreported as "no audio.wav" — the +// F9 half: the file exists, so re-recording is the wrong advice. +func TestCheckSessionAudioRefusesSymlink(t *testing.T) { + dir := t.TempDir() + wav := filepath.Join(dir, session.AudioFile) + + // (a) symlink to a real regular file outside the session. + outside := filepath.Join(t.TempDir(), "private.wav") + if err := os.WriteFile(outside, []byte("RIFF private audio"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, wav); err != nil { + t.Skipf("symlinks unavailable on this platform: %v", err) + } + if err := checkSessionAudio(wav, dir); err == nil || !strings.Contains(err.Error(), "symlink") { + t.Fatalf("a symlinked audio.wav must be refused as a symlink (pre-fix it was silently followed), got %v", err) + } + + // (b) symlink to an absent target must still be named a symlink, not "no audio.wav". + if err := os.Remove(wav); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(dir, "does-not-exist.wav"), wav); err != nil { + t.Fatalf("symlink: %v", err) + } + if err := checkSessionAudio(wav, dir); err == nil || !strings.Contains(err.Error(), "symlink") { + t.Fatalf("a dangling symlink must be reported as a symlink, not as absence, got %v", err) + } +} + // TestResolveModelRefusesFIFO is the model-path twin of the FIFO refusals on the // package's other subprocess-input sites (TestConvertAudioRefusesFIFOInput, // TestConvertAudioRefusesFIFOOutput, TestCheckSessionAudioRefusesFIFO). The From d9098a779d493a84b70d202f5c3f80a832e3b570 Mon Sep 17 00:00:00 2001 From: Alex Reppel <77722411+REPPL@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:13:19 +0100 Subject: [PATCH 3/6] Close the third-round defects an adversarial review found in 2a293d0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent multi-agent adversarial review of the previous commit (13 finders, every finding put to three refuters on distinct lenses) confirmed four behavioral defects and seven coverage gaps. Each fix below carries a regression test verified failing against the neutralised pre-fix code. Behavioral fixes: - Widen SafeText from the Bidi_Control enumeration to every Unicode format character (Cf): ZWSP, word joiner, ZWNBSP/BOM, soft hyphen, and the U+E00xx tag block render as nothing while surviving in the bytes, so displayed requests/quotes could still differ invisibly from the recorded bytes (ASCII smuggling past the Trojan-Source guard). - Judge quote emptiness after trimming: a quote of "\t" sanitises to " ", which cleared the sanitised-empty check and trivially satisfied the verbatim-substring gate against any utterance containing a space. - Set a recorder's killed flag from the reaped wait status (death by the escalation SIGKILL) instead of unconditionally in the timeout branch: a child that finalised cleanly right at the grace boundary was condemned as truncated and failed the run for a complete recording. - Restructure probeDevices from CommandContext+CombinedOutput to start/reap/select: Cmd.Wait blocks in Process.Wait before consuming the context cancellation, so a child pinned in an uninterruptible kernel wait (the wedged-driver case the deadline exists for) hung record forever despite the expired deadline. The child now gets SIGKILL plus a bounded reap grace, then is abandoned with an actionable error; a listing that completed as the deadline fired is used rather than discarded. - Restore 0644 on the converted audio.wav (os.CreateTemp reserves at 0600, and the rename silently narrowed the artefact's mode). Coverage (tests that previously stayed green with their fix reverted): - Hermetic fake-ffmpeg tests for both probeDevices timeout paths and the by-design non-zero exit; the enumeration is no longer untested. - The derived-offset magnitude bound, via a stubbed derivation seam. - The missing-vs-unreadable audio.wav distinction (EACCES directory). - The mdCode backtick strip (code-span beacon escape) in report.md. - The five review error-path SafeText sites beyond the one already tested. - The convertAudio→atomicConvert call-site wiring, via a conversion-runner seam (a real ffmpeg failure aborts before creating output, so no integration case can observe the wiring), plus a real-ffmpeg failure case. Assisted-by: Claude:claude-fable-5 --- CHANGELOG.md | 26 +++++ internal/analyze/analyze_test.go | 18 +++ internal/analyze/validate.go | 11 +- internal/record/proc.go | 32 +++++- internal/record/record_test.go | 129 +++++++++++++++++++-- internal/record/recorders.go | 149 ++++++++++++++++++------- internal/report/report_test.go | 17 ++- internal/review/review_test.go | 64 +++++++++++ internal/session/session.go | 37 +++--- internal/session/session_test.go | 10 +- internal/transcribe/ffmpeg.go | 32 +++++- internal/transcribe/transcribe.go | 2 +- internal/transcribe/transcribe_test.go | 141 +++++++++++++++++++++++ 13 files changed, 593 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dfdf1d..4054ff2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,32 @@ Capture reliability (`record`/`transcribe`, macOS): - A recorder that had to be force-stopped (it missed the finalisation grace) is flagged, so a truncated, unplayable `screen.mp4` is no longer reported as good. +A third adversarial pass over that same commit closed what its own review had +missed: + +- Text sanitisation now strips every invisible Unicode format character (zero + width space, word joiner, BOM, soft hyphen, the tag block), not only the bidi + controls — closing the remaining gap between what a terminal or `report.md` + displays and the bytes actually recorded (invisible-text smuggling). +- A finding quote that sanitises to whitespace alone is rejected like one that + sanitises to nothing, closing the remaining trivially-satisfied verbatim + check. +- A recorder that finalised cleanly right at the shutdown grace boundary is no + longer misclassified as force-stopped — the flag now reflects whether the + escalation SIGKILL actually terminated it — so a complete, playable recording + is not reported as truncated with a spurious failure exit. +- Device enumeration no longer relies on the enumeration child being killable: + a child that survives SIGKILL (a wedged kernel driver) is abandoned with an + actionable error instead of hanging `record` forever, and a listing that + completed just as the deadline fired is used rather than discarded. Both + enumeration paths are now covered by hermetic tests against a fake `ffmpeg`, + as are the force-stop classification, the derived-offset bound, the + missing-vs-unreadable audio split, the report code-span escape, the review + error-path sanitisation, and the atomic-conversion call-site wiring — fixes + whose tests previously stayed green when the fix was reverted. +- A converted `audio.wav` keeps the ordinary `0644` file mode instead of + silently inheriting the temp file's `0600`. + ### Changed - **Behaviour:** `analyze -ingest` and `analyze -out` now require a regular file diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index 0fde40c..e69bd81 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -72,6 +72,24 @@ func TestIngestRejectsQuoteThatSanitisesToEmpty(t *testing.T) { } } +// TestIngestRejectsQuoteThatSanitisesToWhitespace is the whitespace twin of the +// sanitised-empty refusal: SafeText maps a tab to a space, so a quote of "\t" +// sanitises to " " — non-empty, clearing the empty check — and +// strings.Contains(text, " ") is true for any utterance containing a space, so +// the verbatim gate passed for a quote the participant never spoke. Emptiness +// must be judged after trimming. +func TestIngestRejectsQuoteThatSanitisesToWhitespace(t *testing.T) { + dir := writeSession(t, timelineFixture) + answer := "{\"findings\":[{\"id\":\"F-001\",\"t\":22,\"type\":\"bug\",\"severity\":3,\"quote\":\"\\t\",\"evidence\":[\"utt-004\"]}]}" + _, err := Ingest(dir, strings.NewReader(answer)) + if err == nil || !strings.Contains(err.Error(), "quote must be non-empty") { + t.Fatalf("expected a whitespace-only quote refusal, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(dir, session.FindingsFile)); statErr == nil { + t.Fatalf("findings.jsonl was written despite a quote that sanitises to whitespace") + } +} + // timelineFixture is a minimal merged timeline: one spoken utterance and two // events, enough to exercise every validation rule. sessionEnd is 28 (utt-004 // t1). diff --git a/internal/analyze/validate.go b/internal/analyze/validate.go index 604e6e0..9a112d3 100644 --- a/internal/analyze/validate.go +++ b/internal/analyze/validate.go @@ -200,9 +200,14 @@ func validate(findings []positioned, idx timelineIndex) []error { // non-empty and clears the first check, but SafeText reduces it to "" and // strings.Contains(anyText, "") is always true — so the verbatim-substring // gate would pass for a quote the participant never spoke, and the finding - // would carry a quote that sanitises to nothing. Reject the sanitised-empty - // quote explicitly, before the substring test that "" trivially satisfies. - if session.SafeText(f.Quote) == "" { + // would carry a quote that sanitises to nothing. The same holds one step up + // for whitespace: SafeText maps a tab to a space, so a quote of "\t" + // sanitises to " ", which is non-empty yet a trivially-satisfied substring + // of any utterance containing a space — the identical bypass wearing + // whitespace. Judge emptiness after trimming, so a quote that sanitises to + // nothing-but-whitespace is rejected before the substring test it would + // trivially satisfy. + if strings.TrimSpace(session.SafeText(f.Quote)) == "" { errs = append(errs, fmt.Errorf("%s: quote must be non-empty", label)) } else if !containsAny(uttTexts, session.SafeText(f.Quote)) { errs = append(errs, fmt.Errorf("%s: quote is not a verbatim substring of any cited evidence utterance", label)) diff --git a/internal/record/proc.go b/internal/record/proc.go index 6038e76..0d18ced 100644 --- a/internal/record/proc.go +++ b/internal/record/proc.go @@ -14,6 +14,11 @@ type proc interface { Start() error Signal(os.Signal) error Wait() error + // DiedOf reports whether the process was terminated by sig, as opposed to + // exiting on its own (cleanly or otherwise). Valid only after Wait has + // returned; the answer is what lets stopChild tell a recorder its SIGKILL + // actually cut short from one that had already finalised at the boundary. + DiedOf(sig syscall.Signal) bool } // execProc adapts an *exec.Cmd to proc. Each recorder runs in its own process @@ -34,6 +39,18 @@ func (e *execProc) Signal(sig os.Signal) error { func (e *execProc) Wait() error { return e.cmd.Wait() } +// DiedOf reads the reaped state cmd.Wait recorded. Callers hold the +// happens-before edge (they read only after the reaper's close(done), which +// follows Wait), so ProcessState is stable here. +func (e *execProc) DiedOf(sig syscall.Signal) bool { + ps := e.cmd.ProcessState + if ps == nil { + return false + } + ws, ok := ps.Sys().(syscall.WaitStatus) + return ok && ws.Signaled() && ws.Signal() == sig +} + // stderrRetain caps how much of a child's stderr lockedBuffer keeps. A record // session is long-running by design, and avfoundation floods stderr when a device // stalls ("frame dropped", mux-queue warnings), so an unbounded buffer could grow by @@ -99,7 +116,7 @@ type liveChild struct { started time.Time // when watching began; used to tell a start-up failure from a mid-session stop done chan struct{} // closed once, when Wait returns err error // Wait result; read only after done is closed - killed bool // set by stopChild when the grace expired and SIGKILL was sent; read after stopAll + killed bool // set by stopChild when its escalation SIGKILL terminated the child; read after stopAll } // watch starts the single reaper goroutine. It must be called exactly once, @@ -132,8 +149,19 @@ func stopChild(c *liveChild, grace time.Duration) { // it so finaliseOutputs surfaces the risk instead of blessing the file by size. // killed is written here on the caller's goroutine (stopAll, sequential) and // read only after stopAll returns, so no synchronisation is needed. + // + // Condemn the artefact only when the SIGKILL is what actually terminated the + // recorder. A child that finalised and exited cleanly right at the grace + // boundary can still land in this branch — the reaper closes done only after + // Wait returns (which also joins the stderr copier), so there is a scheduling + // window between a clean exit and done becoming readable, and select picks + // randomly when both cases are ready. In that window the SIGKILL hits an + // already-exited process (a no-op); marking such a child killed reported a + // complete, playable recording as "likely truncated or unplayable" and failed + // the run. The reaped wait status is ground truth for who ended the process, + // and c.err/ProcessState are stable once done is closed. _ = c.p.Signal(syscall.SIGKILL) - c.killed = true <-c.done + c.killed = c.p.DiedOf(syscall.SIGKILL) } } diff --git a/internal/record/record_test.go b/internal/record/record_test.go index ae2f317..fcc8980 100644 --- a/internal/record/record_test.go +++ b/internal/record/record_test.go @@ -162,6 +162,75 @@ func TestOutputTail(t *testing.T) { } } +// fakeFFmpeg writes an executable shell script standing in for the ffmpeg +// binary, so probeDevices' spawn-and-wait paths run hermetically on any Unix. +func fakeFFmpeg(t *testing.T, script string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "ffmpeg") + if err := os.WriteFile(path, []byte("#!/bin/sh\n"+script), 0o755); err != nil { + t.Fatalf("write fake ffmpeg: %v", err) + } + return path +} + +// TestProbeDevicesTimesOut is the wedged-enumeration regression: a hung device +// listing must surface an actionable timeout instead of hanging `testimony +// record` forever before the interrupt handler is live. `exec` replaces the +// shell with sleep so the deadline SIGKILL reaps the actual blocker. +func TestProbeDevicesTimesOut(t *testing.T) { + old := deviceListTimeout + deviceListTimeout = 50 * time.Millisecond + t.Cleanup(func() { deviceListTimeout = old }) + + start := time.Now() + _, _, err := probeDevices(fakeFFmpeg(t, "exec sleep 10\n"), false) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("a hung enumeration must surface a timeout, got %v", err) + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("the deadline did not bound the wait: took %v", elapsed) + } +} + +// TestProbeDevicesAbandonsUnreapableChild covers the residual wedged-driver +// sub-case: a child pinned where SIGKILL cannot take effect can never be reaped, +// and probeDevices must abandon it rather than wait forever. The stand-in: the +// shell (not exec'd) dies on SIGKILL but its sleep grandchild inherits the +// output pipe, so Wait cannot finish within the kill grace — the same +// wait-never-returns shape as an unkillable process. +func TestProbeDevicesAbandonsUnreapableChild(t *testing.T) { + oldList, oldKill := deviceListTimeout, probeKillGrace + deviceListTimeout = 50 * time.Millisecond + probeKillGrace = 100 * time.Millisecond + t.Cleanup(func() { deviceListTimeout, probeKillGrace = oldList, oldKill }) + + start := time.Now() + _, _, err := probeDevices(fakeFFmpeg(t, "sleep 10\n"), false) + if err == nil || !strings.Contains(err.Error(), "unresponsive") { + t.Fatalf("an unreapable enumeration must be abandoned with an actionable error, got %v", err) + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("the kill grace did not bound the wait: took %v", elapsed) + } +} + +// TestProbeDevicesToleratesByDesignExit pins the happy path end to end over a +// real child process: ffmpeg prints the listing to stderr and exits non-zero by +// design, and probeDevices must parse the devices out of that "failure". +func TestProbeDevicesToleratesByDesignExit(t *testing.T) { + script := "cat >&2 <<'EOF'\n" + sampleDevices + "\nEOF\nexit 1\n" + screen, mics, err := probeDevices(fakeFFmpeg(t, script), true) + if err != nil { + t.Fatalf("the by-design non-zero exit must be tolerated: %v", err) + } + if screen != 1 { + t.Fatalf("screen index: got %d, want 1 (Capture screen)", screen) + } + if !reflect.DeepEqual(mics, []string{"Studio Display Microphone", "USB audio CODEC"}) { + t.Fatalf("audio roster: got %q", mics) + } +} + // --- platform plan --- func TestPlan(t *testing.T) { @@ -755,13 +824,19 @@ func TestRunDegradesHonestly(t *testing.T) { // --- lifecycle state machine over a fake proc --- // fakeProc records the signals it receives and exits Wait only when it receives -// its designated exit signal. +// its designated exit signal. exitDelay, when set, holds Wait open for that long +// after the exit-triggering signal — modelling the reaper's scheduling gap +// between a child's real exit and close(done) becoming readable. fatalSig +// records which signal actually ended the proc, so DiedOf answers as a reaped +// wait status would. type fakeProc struct { - mu sync.Mutex - signals []os.Signal - exitOn os.Signal - exit chan struct{} - exitOnce sync.Once + mu sync.Mutex + signals []os.Signal + exitOn os.Signal + exitDelay time.Duration + exit chan struct{} + exitOnce sync.Once + fatalSig os.Signal } func newFakeProc(exitOn os.Signal) *fakeProc { @@ -775,7 +850,16 @@ func (f *fakeProc) Signal(s os.Signal) error { f.signals = append(f.signals, s) f.mu.Unlock() if s == f.exitOn { - f.exitOnce.Do(func() { close(f.exit) }) + f.exitOnce.Do(func() { + f.mu.Lock() + f.fatalSig = s + f.mu.Unlock() + if f.exitDelay > 0 { + time.AfterFunc(f.exitDelay, func() { close(f.exit) }) + } else { + close(f.exit) + } + }) } return nil } @@ -785,6 +869,12 @@ func (f *fakeProc) Wait() error { return nil } +func (f *fakeProc) DiedOf(sig syscall.Signal) bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.fatalSig == os.Signal(sig) +} + func (f *fakeProc) sent() []os.Signal { f.mu.Lock() defer f.mu.Unlock() @@ -819,6 +909,31 @@ func TestStopChildEscalatesToSIGKILL(t *testing.T) { if len(got) != 2 || got[0] != syscall.SIGINT || got[1] != syscall.SIGKILL { t.Fatalf("expected SIGINT then SIGKILL, got %v", got) } + if !c.killed { + t.Fatal("a recorder the escalation SIGKILL terminated must be marked killed, so finaliseOutputs distrusts its artefact") + } +} + +// TestStopChildDoesNotCondemnCleanExitAtGraceBoundary is the misclassification +// regression at the grace deadline. A recorder that finalises and exits cleanly +// right at the boundary can still land in stopChild's timeout branch — the reaper +// closes done only after Wait returns, so there is a scheduling window between the +// clean exit and done becoming readable, and select picks randomly when both cases +// are ready. Pre-fix the branch set killed unconditionally, so the no-op SIGKILL +// into that window condemned a complete, playable recording as "likely truncated +// or unplayable" and failed the run. killed must reflect who actually ended the +// process: here the child dies of SIGINT (finalised) with its Wait held open past +// the grace, and must not be marked killed. +func TestStopChildDoesNotCondemnCleanExitAtGraceBoundary(t *testing.T) { + fp := newFakeProc(syscall.SIGINT) + fp.exitDelay = 50 * time.Millisecond // exit already triggered; Wait returns after the grace + c := newLiveChild(streamScreen, fp, &lockedBuffer{}) + + stopChild(c, 5*time.Millisecond) + + if c.killed { + t.Fatal("a recorder that finalised on SIGINT was marked killed because its reap outlasted the grace; its complete artefact would be reported as truncated") + } } // TestLockedBufferBoundsMemory is the OOM regression: a device-stall stderr flood diff --git a/internal/record/recorders.go b/internal/record/recorders.go index 5e4e1f3..b1773f8 100644 --- a/internal/record/recorders.go +++ b/internal/record/recorders.go @@ -1,25 +1,33 @@ package record import ( - "context" "errors" "fmt" "os/exec" "regexp" "strconv" "strings" + "sync" "time" ) // deviceListTimeout bounds the avfoundation device enumeration. It is a var, not a -// const, only so a test could shrink it; production never reassigns it. Enumeration -// normally returns in well under a second, but a wedged CoreAudio/camera driver or a -// stuck TCC daemon can block the AVFoundation query indefinitely, and probeDevices -// runs before the interrupt handler can stop anything — so without a deadline a single -// bad device hangs `testimony record` forever, with the session dir already created -// and nothing recording. +// const, only so a test can shrink it (TestProbeDevicesTimesOut does); production +// never reassigns it. Enumeration normally returns in well under a second, but a +// wedged CoreAudio/camera driver or a stuck TCC daemon can block the AVFoundation +// query indefinitely, and probeDevices runs before the interrupt handler can stop +// anything — so without a deadline a single bad device hangs `testimony record` +// forever, with the session dir already created and nothing recording. var deviceListTimeout = 15 * time.Second +// probeKillGrace bounds how long probeDevices waits for the enumeration child to be +// reaped after the deadline SIGKILL. A var only for the same test reason as +// deviceListTimeout. If even SIGKILL does not produce an exit within this grace, the +// child is pinned in an uninterruptible kernel wait (a wedged IOKit driver call +// defers signal delivery until the kernel call returns — possibly never), and no +// amount of waiting will reap it; probeDevices abandons it instead of hanging. +var probeKillGrace = 2 * time.Second + // micArgs builds the ffmpeg argv that captures the system default microphone to // a canonical 16 kHz mono PCM WAV — exactly the parameters // transcribe.convertAudio produces, so the file is canonical ASR input needing @@ -129,51 +137,116 @@ func selectDevices(video, audio []avDevice, wantScreen bool) (screenIndex int, m return screenIndex, mics, nil } +// probeSink is a concurrency-safe, bounded sink for the enumeration child's +// output: os/exec copies each pipe on its own goroutine while probeDevices may +// return early on the abandon path, and those copiers then keep writing here. +// It keeps the leading bytes — the section headers and device rows parseAVDevices +// needs print first, so unlike lockedBuffer (diagnostic tail) the head is the +// valuable window. A device listing is a few KB; the cap only bounds a +// misbehaving binary. +type probeSink struct { + mu sync.Mutex + buf []byte +} + +// probeSinkRetain caps probeSink at far above any real device listing. +const probeSinkRetain = 1 << 20 + +func (s *probeSink) Write(p []byte) (int, error) { + s.mu.Lock() + defer s.mu.Unlock() + if room := probeSinkRetain - len(s.buf); room > 0 { + if len(p) > room { + p = p[:room] + } + s.buf = append(s.buf, p...) + } + return len(p), nil +} + +func (s *probeSink) text() string { + s.mu.Lock() + defer s.mu.Unlock() + return string(s.buf) +} + // probeDevices runs ffmpeg's avfoundation device listing, validates an audio // input exists, and resolves the (when wantScreen) screen index. The command // exits non-zero by design after printing the list to stderr, so a non-nil exec // error is expected; only a failure to find the needed devices is fatal. It // returns the detected audio-input names for the caller to log. Impure (spawns -// ffmpeg); isolated here and skipped in CI. +// a child process), but the binary path is a parameter, so both the timeout and +// the by-design-exit paths are covered hermetically by tests with a fake ffmpeg. +// +// The wait is structured as start → own reap goroutine → select against the +// deadline, rather than exec.CommandContext + CombinedOutput: Cmd.Wait blocks in +// Process.Wait before it consumes the context cancellation, so with a child +// pinned in an uninterruptible kernel wait (the wedged-driver case the deadline +// exists for) CombinedOutput would hang forever despite the expired context — +// SIGKILL delivery is deferred until the kernel call returns. Here the deadline +// fires regardless: the child is SIGKILLed, given probeKillGrace to be reaped, +// and otherwise abandoned (the reap and pipe-copy goroutines leak by design — +// nothing can reap an unkillable process, and record aborts on the error). func probeDevices(ffmpeg string, wantScreen bool) (screenIndex int, mics []string, err error) { - ctx, cancel := context.WithTimeout(context.Background(), deviceListTimeout) - defer cancel() - cmd := exec.CommandContext(ctx, ffmpeg, "-hide_banner", "-f", "avfoundation", "-list_devices", "true", "-i", "") - out, runErr := cmd.CombinedOutput() - // A wedged device/driver can make the enumeration block for ever; the context - // deadline turns that into an actionable error rather than a silent hang before - // any recorder (or the interrupt handler) is live. - if ctx.Err() == context.DeadlineExceeded { + cmd := exec.Command(ffmpeg, "-hide_banner", "-f", "avfoundation", "-list_devices", "true", "-i", "") + var sink probeSink + cmd.Stdout = &sink + cmd.Stderr = &sink + if startErr := cmd.Start(); startErr != nil { + return 0, nil, fmt.Errorf("run ffmpeg device listing: %w", startErr) + } + done := make(chan error, 1) // buffered: the reap goroutine must not leak blocked on the abandon path + go func() { done <- cmd.Wait() }() + + var runErr error + timedOut := false + select { + case runErr = <-done: + case <-time.After(deviceListTimeout): + timedOut = true + _ = cmd.Process.Kill() + select { + case runErr = <-done: + case <-time.After(probeKillGrace): + return 0, nil, fmt.Errorf("ffmpeg avfoundation device listing hung for %s and survived SIGKILL — a capture device or kernel driver is unresponsive; disconnect or disable it, then re-run", deviceListTimeout) + } + } + + video, audio := parseAVDevices(sink.text()) + screenIndex, mics, err = selectDevices(video, audio, wantScreen) + if err == nil { + // A listing that raced the deadline but parsed completely still wins: the + // select above picks randomly when the exit and the timer are both ready, + // and discarding a valid enumeration would abort a recording session with a + // misleading "device is unresponsive" for a probe that in fact succeeded. + return screenIndex, mics, nil + } + if timedOut { return 0, nil, fmt.Errorf("ffmpeg avfoundation device listing timed out after %s — a capture device or driver is unresponsive; disconnect or disable it, then re-run", deviceListTimeout) } // ffmpeg prints the listing to stderr and then exits non-zero by design, so an - // *exec.ExitError is expected and ignored — the listing is still in out. But a - // failure to run ffmpeg at all (a corrupt or wrong-architecture binary, a - // sandbox/exec denial, a fork failure after LookPath already succeeded) is a - // different error: out is then empty, parseAVDevices finds nothing, and - // selectDevices would misreport "no microphone found", sending the operator to - // check hardware and permissions for a process that never started. Surface it. + // *exec.ExitError is expected and ignored — the listing is still in the sink. + // But a Wait that failed for a different reason (an I/O error copying the + // pipes; a start-time failure is already caught above) means the listing never + // arrived, and selectDevices would misreport "no microphone found", sending the + // operator to check hardware and permissions for a process that never ran to + // completion. Surface it. var exitErr *exec.ExitError if runErr != nil && !errors.As(runErr, &exitErr) { return 0, nil, fmt.Errorf("run ffmpeg device listing: %w", runErr) } - video, audio := parseAVDevices(string(out)) - screenIndex, mics, err = selectDevices(video, audio, wantScreen) - if err != nil { - // selectDevices found no usable device. The benign by-design exit prints the - // listing to stderr, but an ffmpeg that genuinely failed — built without - // avfoundation ("Unknown input format: 'avfoundation'"), or killed by a - // signal — also lands here as an *exec.ExitError with the real cause sitting - // in out, which parseAVDevices then reads as an empty listing. Without the - // tail the operator is told to check their microphone for what is actually a - // toolchain fault. Surface ffmpeg's own last words so the true cause reaches - // them. - if tail := outputTail(out); tail != "" { - return 0, nil, fmt.Errorf("%w; ffmpeg said: %s", err, tail) - } - return 0, nil, err + // selectDevices found no usable device. The benign by-design exit prints the + // listing to stderr, but an ffmpeg that genuinely failed — built without + // avfoundation ("Unknown input format: 'avfoundation'"), or killed by a + // signal — also lands here as an *exec.ExitError with the real cause sitting + // in the sink, which parseAVDevices then reads as an empty listing. Without the + // tail the operator is told to check their microphone for what is actually a + // toolchain fault. Surface ffmpeg's own last words so the true cause reaches + // them. + if tail := outputTail([]byte(sink.text())); tail != "" { + return 0, nil, fmt.Errorf("%w; ffmpeg said: %s", err, tail) } - return screenIndex, mics, nil + return 0, nil, err } // outputTail returns a trimmed, bounded tail of ffmpeg's output for inclusion in diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 51c1346..2ebcd4f 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -295,8 +295,11 @@ func TestReportNeutralisesInlineMarkdown(t *testing.T) { if err := session.SaveManifest(dir, session.Manifest{Session: "fixture", App: "app", Participant: "Alice"}); err != nil { t.Fatalf("SaveManifest: %v", err) } - // An utterance whose text is an image-beacon, and a finding whose quote is one. - tl := "{\"t\":1,\"src\":\"speech\",\"id\":\"utt-001\",\"payload\":{\"speaker\":\"Alice\",\"t1\":2,\"text\":\"![t](http://evil/a.png)\"}}\n" + // An utterance whose text is an image-beacon, a finding whose quote is one, + // and an event whose selector smuggles one through the code span: the inner + // backticks would close the span early and let the image markup render live. + tl := "{\"t\":1,\"src\":\"speech\",\"id\":\"utt-001\",\"payload\":{\"speaker\":\"Alice\",\"t1\":2,\"text\":\"![t](http://evil/a.png)\"}}\n" + + "{\"t\":1.5,\"src\":\"event\",\"id\":\"ev-001\",\"payload\":{\"kind\":\"click\",\"selector\":\"x` ![p](http://span.example/c.png) `y\"}}\n" if err := os.WriteFile(filepath.Join(dir, session.TimelineFile), []byte(tl), 0o644); err != nil { t.Fatalf("write timeline: %v", err) } @@ -320,6 +323,16 @@ func TestReportNeutralisesInlineMarkdown(t *testing.T) { if !strings.Contains(md, "evil/a.png") || !strings.Contains(md, "evil/b.png") { t.Fatalf("neutralisation dropped the text instead of escaping it:\n%s", md) } + // The selector's inner backticks are stripped, so the whole selector stays one + // inert code span (span content is literal — inert — so the image markup may + // remain as text there). Pre-strip, "x` ![p](...) `y" split into `x` + live + // image markup + `y`, firing the beacon from inside the "code" rendering. + if !strings.Contains(md, "`x ![p](http://span.example/c.png) y`") { + t.Fatalf("selector did not survive as a single backtick-free code span:\n%s", md) + } + if strings.Contains(md, "`x` ") { + t.Fatalf("selector code span was closed early by an embedded backtick:\n%s", md) + } } func findingLines(t *testing.T, dir string) []string { diff --git a/internal/review/review_test.go b/internal/review/review_test.go index e872619..f35fe7c 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -354,6 +354,70 @@ func TestVerifyTargetErrorSanitisesFindingID(t *testing.T) { } } +// TestReviewErrorPathsSanitiseFindingIDs pins every remaining finding-id error +// sink this package sanitises — checkTargets (both ids), AppendVerdict's +// oversized-line refusal, and verifyTarget's identity-changed and +// duplicate-target-gone branches. TestVerifyTargetErrorSanitisesFindingID +// covers only the cur==nil branch; each branch here reverted to a raw %s +// independently while the whole suite stayed green, and the verifyTarget pair +// carry ids sourced straight from an attacker-authored findings.jsonl through +// review's normal flow. +func TestReviewErrorPathsSanitiseFindingIDs(t *testing.T) { + const evilPrefix = "\x1b]0;pwned\x07" + assertClean := func(t *testing.T, err error, wantSub string) { + t.Helper() + if err == nil { + t.Fatal("want an error carrying the sanitised id, got nil") + } + if strings.ContainsRune(err.Error(), '\x1b') || strings.ContainsRune(err.Error(), '\x07') { + t.Fatalf("error carries raw terminal-control bytes: %q", err.Error()) + } + if !strings.Contains(err.Error(), wantSub) { + t.Fatalf("sanitised error dropped the id's printable tail %q: %q", wantSub, err.Error()) + } + } + + t.Run("checkTargets finding not found", func(t *testing.T) { + findings := []analyze.Finding{{ID: "F-001"}} + assertClean(t, checkTargets(findings, evilPrefix+"F-404", "confirmed", ""), "F-404") + }) + + t.Run("checkTargets duplicate target not found", func(t *testing.T) { + findings := []analyze.Finding{{ID: "F-001"}} + assertClean(t, checkTargets(findings, "F-001", "duplicate", evilPrefix+"F-405"), "F-405") + }) + + t.Run("AppendVerdict oversized line", func(t *testing.T) { + v := analyze.Verdict{Kind: "verdict", Finding: evilPrefix + "F-001", Verdict: "confirmed", + At: strings.Repeat("x", session.MaxJSONLLine)} + assertClean(t, AppendVerdict(t.TempDir(), v, nil), "F-001") + }) + + t.Run("verifyTarget identity changed", func(t *testing.T) { + dir := t.TempDir() + evilID := evilPrefix + "F-001" + line := `{"id":"` + "\\u001b]0;pwned\\u0007" + `F-001","t":22,"type":"bug","severity":3,"quote":"a","evidence":["utt-004"],"status":"unverified"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(line), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + rec := analyze.Verdict{Kind: "verdict", Finding: evilID, Verdict: "confirmed", At: "2026-07-22"} + // The snapshot differs in quote: the finding was rewritten since review started. + expect := &analyze.Finding{ID: evilID, T: 22, Type: "bug", Severity: 3, Quote: "different", Evidence: []string{"utt-004"}} + assertClean(t, AppendVerdict(dir, rec, expect), "F-001") + }) + + t.Run("verifyTarget duplicate target gone", func(t *testing.T) { + dir := t.TempDir() + line := `{"id":"F-001","t":22,"type":"bug","severity":3,"quote":"a","evidence":["utt-004"],"status":"unverified"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(line), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + rec := analyze.Verdict{Kind: "verdict", Finding: "F-001", Verdict: "duplicate", Of: evilPrefix + "F-999", At: "2026-07-22"} + expect := &analyze.Finding{ID: "F-001", T: 22, Type: "bug", Severity: 3, Quote: "a", Evidence: []string{"utt-004"}} + assertClean(t, AppendVerdict(dir, rec, expect), "F-999") + }) +} + // shortWriteFile is a verdictFile whose Write persists a prefix and then errors, // standing in for a full disk (write(2) fills the remaining space, returns a // short count, and the next write returns ENOSPC — os.File.Write persists the diff --git a/internal/session/session.go b/internal/session/session.go index fbab0b0..4b2b01a 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -25,6 +25,7 @@ import ( "strings" "syscall" "time" + "unicode" ) // Manifest describes a capture session. t0_epoch_ms anchors all @@ -255,15 +256,20 @@ func WriteFileNoFollow(path string, data []byte, perm os.FileMode) error { // artefact (report.md) or a terminal line (review). It strips C0/C1 control // bytes — including the newline and carriage return that could forge report // structure or split a JSONL record, and the ESC (0x1b) that drives ANSI -// terminal sequences — turns tabs into spaces, and removes the complete Unicode -// Bidi_Control set (U+061C, U+200E, U+200F, U+202A-U+202E, U+2066-U+2069) along -// with the line and paragraph separators, the formatting controls behind -// Trojan-Source spoofing (CVE-2021-42574), so a right-to-left override or an -// Arabic letter mark cannot make a -// displayed quote or anchor differ from the bytes a verdict is recorded -// against. Attacker-authored transcript, interaction, manifest, and finding -// text therefore cannot inject headings, terminal control sequences, extra -// lines, or reordered glyphs. Ordinary text is unchanged. +// terminal sequences — turns tabs into spaces, and removes every Unicode +// format character (category Cf) along with the line and paragraph +// separators. Cf covers the complete Bidi_Control set behind Trojan-Source +// spoofing (CVE-2021-42574) — so a right-to-left override or an Arabic letter +// mark cannot make a displayed quote or anchor differ from the bytes a verdict +// is recorded against — and equally the invisible characters outside it (ZWSP +// U+200B, word joiner U+2060, ZWNBSP/BOM U+FEFF, soft hyphen U+00AD, the +// U+E00xx tag block) that render as nothing in a terminal or Markdown viewer +// while surviving in the bytes: enumerating the bidi set alone left those as an +// ASCII-smuggling residue, hiding text (for instance an instruction to the +// analysis agent) inside a request or quote the operator reads as clean. +// Attacker-authored transcript, interaction, manifest, and finding text +// therefore cannot inject headings, terminal control sequences, extra lines, +// reordered glyphs, or invisibly-smuggled text. Ordinary text is unchanged. func SafeText(s string) string { return strings.Map(func(r rune) rune { switch { @@ -271,13 +277,12 @@ func SafeText(s string) string { return ' ' case r < 0x20, r == 0x7f, r >= 0x80 && r <= 0x9f: return -1 - // The complete Unicode Bidi_Control set, plus the line and paragraph - // separators: leaving any member out would let that one character do the - // reordering the rest are stripped to prevent. - case r == 0x061c, // ALM - r == 0x200e || r == 0x200f, // LRM, RLM - r >= 0x202a && r <= 0x202e, // LRE, RLE, PDF, LRO, RLO - r >= 0x2066 && r <= 0x2069, // LRI, RLI, FSI, PDI + // Every format character, plus the line and paragraph separators (Zl/Zp, + // outside Cf): the category test subsumes the Bidi_Control enumeration and + // closes the invisible-Cf residue in one predicate, so no member can be + // left out to do the reordering or smuggling the rest are stripped to + // prevent. + case unicode.Is(unicode.Cf, r), r == 0x2028 || r == 0x2029: // line / paragraph separator return -1 default: diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 3b0b559..41f65a6 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -450,7 +450,15 @@ func TestSafeText(t *testing.T) { "line\u2028sep": "linesep", // U+2028 line separator "mark\u200e\u200fs": "marks", // U+200E/U+200F LRM/RLM "alm\u061cstrip": "almstrip", // U+061C ARABIC LETTER MARK (Bidi_Control) - "caf\u00e9": "caf\u00e9", // ordinary accented text is unchanged + // The invisible format characters (category Cf) outside the Bidi_Control + // set: each renders as nothing while surviving in the bytes, so leaving any + // one in lets displayed text differ from recorded bytes (ASCII smuggling). + "zero\u200bwidth": "zerowidth", // U+200B ZERO WIDTH SPACE + "word\u2060joiner": "wordjoiner", // U+2060 WORD JOINER + "bom\ufeffstrip": "bomstrip", // U+FEFF ZWNBSP/BOM + "soft\u00adhyphen": "softhyphen", // U+00AD SOFT HYPHEN + "tag\U000E0041\U000E0042block": "tagblock", // U+E0041/42 Unicode tag block + "caf\u00e9": "caf\u00e9", // ordinary accented text is unchanged } for in, want := range cases { if got := SafeText(in); got != want { diff --git a/internal/transcribe/ffmpeg.go b/internal/transcribe/ffmpeg.go index 0e2dd2b..685fa35 100644 --- a/internal/transcribe/ffmpeg.go +++ b/internal/transcribe/ffmpeg.go @@ -45,14 +45,24 @@ func convertAudio(in, out string) error { // never leaves a partial audio.wav that a later bare `transcribe` would silently // treat as the whole recording. return atomicConvert(out, func(tmpPath string) error { - cmd := exec.Command(ffmpeg, "-y", "-i", in, "-ac", "1", "-ar", "16000", "-c:a", "pcm_s16le", tmpPath) - if raw, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("ffmpeg: %w\n%s", err, tail(raw)) - } - return nil + return convertRunner(ffmpeg, in, tmpPath) }) } +// convertRunner runs the actual ffmpeg conversion into the temp path. A var +// only so a hermetic test can stand in for ffmpeg and pin the atomicConvert +// wiring at the call site — a real ffmpeg failure on a bad input aborts before +// it ever creates the output, so no integration case can observe whether the +// producer was pointed at the temp or at out itself. Production never +// reassigns it. +var convertRunner = func(ffmpeg, in, tmpPath string) error { + cmd := exec.Command(ffmpeg, "-y", "-i", in, "-ac", "1", "-ar", "16000", "-c:a", "pcm_s16le", tmpPath) + if raw, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("ffmpeg: %w\n%s", err, tail(raw)) + } + return nil +} + // atomicConvert runs a producer that writes the converted audio to a temp file beside // out, then renames it over out only if the producer succeeded. If the producer // returns an error — including one raised after it has already written a partial temp, @@ -71,6 +81,12 @@ func atomicConvert(out string, produce func(tmpPath string) error) error { if err := produce(tmpPath); err != nil { return err } + // os.CreateTemp reserves the name at 0600; the finished audio.wav is an + // ordinary session artefact, so restore the 0644 a direct write would have + // given it — otherwise the conversion silently narrows the file's mode. + if err := os.Chmod(tmpPath, 0o644); err != nil { + return fmt.Errorf("audio convert: finalise %s: %w", out, err) + } if err := os.Rename(tmpPath, out); err != nil { return fmt.Errorf("audio convert: finalise %s: %w", out, err) } @@ -109,6 +125,12 @@ func checkPlainOutput(path string) error { return nil } +// deriveOffsetFn is the seam resolveOffset calls for offset derivation. A var +// only so a test can stub the ffprobe-gated derivation (the deviceListTimeout +// precedent in record) and exercise resolveOffset's derived-offset magnitude +// bound hermetically; production never reassigns it. +var deriveOffsetFn = deriveOffset + // deriveOffset reads the original recording's creation time via ffprobe and // returns creation_epoch_seconds − t0_epoch_seconds. The boolean is false // whenever ffprobe or the creation_time tag is unavailable — derivation is diff --git a/internal/transcribe/transcribe.go b/internal/transcribe/transcribe.go index d432802..b0735f3 100644 --- a/internal/transcribe/transcribe.go +++ b/internal/transcribe/transcribe.go @@ -185,7 +185,7 @@ func resolveOffset(opts Options, man session.Manifest, external bool) (float64, if err != nil { return 0, "", fmt.Errorf("deriving audio offset: %w", err) } - if off, ok := deriveOffset(opts.Audio, t0); ok { + if off, ok := deriveOffsetFn(opts.Audio, t0); ok { // A derived offset beyond the session-time magnitude is not a real capture // timing — it means the recording's creation_time (attacker-influenceable // metadata) or the manifest t0 is bogus. Refuse rather than derive a diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index 19ae24b..80581ba 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -311,6 +311,34 @@ func TestResolveOffsetRefusesOversizedSidecar(t *testing.T) { } } +// TestResolveOffsetRefusesOversizedDerivedOffset is the derived half of the +// offset-magnitude bound (the sidecar half has its own test above): a poisoned +// recording creation_time (attacker-influenceable metadata) that derives an +// astronomical offset must refuse at derivation, naming the input, rather than +// bake a decades-long shift into every utterance for merge to reject one +// command later. The ffprobe-gated derivation is stubbed through the +// deriveOffsetFn seam so the bound itself is exercised hermetically. +func TestResolveOffsetRefusesOversizedDerivedOffset(t *testing.T) { + old := deriveOffsetFn + t.Cleanup(func() { deriveOffsetFn = old }) + + deriveOffsetFn = func(audio string, t0EpochMS int64) (float64, bool) { return 5e9, true } + _, _, err := resolveOffset(Options{Audio: "ext.wav"}, session.Manifest{T0EpochMS: 1}, true) + if err == nil || !strings.Contains(err.Error(), "exceeds") || !strings.Contains(err.Error(), "-offset") { + t.Fatalf("an oversized derived offset must refuse with -offset guidance, got %v", err) + } + + // A large but in-bounds derived offset (10 hours) still resolves. + deriveOffsetFn = func(audio string, t0EpochMS int64) (float64, bool) { return 36000, true } + off, provenance, err := resolveOffset(Options{Audio: "ext.wav"}, session.Manifest{T0EpochMS: 1}, true) + if err != nil { + t.Fatalf("an in-bounds derived offset must be accepted: %v", err) + } + if off != 36000 || !strings.Contains(provenance, "derived") { + t.Fatalf("offset: got %v (%q), want 36000 (derived)", off, provenance) + } +} + // TestResolveOffsetInPlaceNoSidecarIsRecordOrigin proves the record flow is // untouched: audio.wav with no sidecar is captured at t0, so the offset is 0. func TestResolveOffsetInPlaceNoSidecarIsRecordOrigin(t *testing.T) { @@ -460,6 +488,79 @@ func TestConvertAudioIntegration(t *testing.T) { if err := convertAudio(filepath.Join(dir, "voice.mp3"), out); err == nil { t.Fatal("unsupported extension must error") } + + // A conversion where ffmpeg itself fails — a corrupt recording wearing a .wav + // extension — must leave no output and no temp. This pins the convertAudio → + // atomicConvert wiring at the call site: the hermetic helper test alone stays + // green if a later edit routes ffmpeg straight at out again. + failDir := t.TempDir() + corrupt := filepath.Join(failDir, "corrupt.wav") + if err := os.WriteFile(corrupt, []byte("not audio at all"), 0o644); err != nil { + t.Fatal(err) + } + failOut := filepath.Join(failDir, session.AudioFile) + if err := convertAudio(corrupt, failOut); err == nil { + t.Fatal("a corrupt input must fail the conversion") + } + if _, statErr := os.Stat(failOut); !os.IsNotExist(statErr) { + t.Fatalf("a failed conversion left %s behind (err=%v)", session.AudioFile, statErr) + } + if temps, _ := filepath.Glob(filepath.Join(failDir, ".audio-*.wav")); len(temps) != 0 { + t.Fatalf("a failed conversion left temp files behind: %v", temps) + } +} + +// TestConvertAudioRoutesThroughTemp pins the convertAudio → atomicConvert +// wiring at the call site, hermetically (the convertRunner stub stands in for +// ffmpeg; LookPath resolves against a fake on PATH). The producer must be +// handed a temp path beside out — never out itself — and its failure must leave +// out absent and no temp behind. The helper's own unit test stays green if a +// later edit routes ffmpeg straight at out again; this one does not. +func TestConvertAudioRoutesThroughTemp(t *testing.T) { + fakeBin := t.TempDir() + if err := os.WriteFile(filepath.Join(fakeBin, "ffmpeg"), []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", fakeBin) + + dir := t.TempDir() + in := filepath.Join(dir, "voice.wav") + if err := os.WriteFile(in, []byte("RIFF input"), 0o644); err != nil { + t.Fatal(err) + } + out := filepath.Join(dir, session.AudioFile) + + oldRunner := convertRunner + t.Cleanup(func() { convertRunner = oldRunner }) + var producedAt string + convertRunner = func(ffmpeg, in, tmpPath string) error { + producedAt = tmpPath + // Exactly what an interrupted or ENOSPC-hit ffmpeg does: a partial file, + // then failure. + if werr := os.WriteFile(tmpPath, []byte("RIFF....partial"), 0o644); werr != nil { + t.Fatalf("seed partial: %v", werr) + } + return errors.New("ffmpeg: killed by signal") + } + + if err := convertAudio(in, out); err == nil { + t.Fatal("convertAudio must surface the conversion failure") + } + if producedAt == "" { + t.Fatal("convertRunner was never invoked") + } + if producedAt == out { + t.Fatal("the producer was pointed straight at out; a partial conversion would land as audio.wav") + } + if filepath.Dir(producedAt) != dir { + t.Fatalf("temp %q is not beside out (rename would cross filesystems)", producedAt) + } + if _, statErr := os.Stat(out); !os.IsNotExist(statErr) { + t.Fatalf("a failed conversion left %s behind (err=%v)", session.AudioFile, statErr) + } + if temps, _ := filepath.Glob(filepath.Join(dir, ".audio-*.wav")); len(temps) != 0 { + t.Fatalf("a failed conversion left temp files behind: %v", temps) + } } // TestAtomicConvertLeavesNoPartialOnFailure is the interrupted-conversion regression, @@ -499,6 +600,11 @@ func TestAtomicConvertLeavesNoPartialOnFailure(t *testing.T) { if b, rerr := os.ReadFile(out); rerr != nil || string(b) != "RIFF....complete" { t.Fatalf("successful conversion did not land the full output: %q (err=%v)", b, rerr) } + // The finished artefact carries an ordinary 0644, not os.CreateTemp's 0600 — + // the conversion must not silently narrow the mode a direct write gave it. + if fi, serr := os.Stat(out); serr != nil || fi.Mode().Perm() != 0o644 { + t.Fatalf("converted %s mode: got %v (err=%v), want 0644", session.AudioFile, fi.Mode().Perm(), serr) + } if temps, _ := filepath.Glob(filepath.Join(dir, ".audio-*.wav")); len(temps) != 0 { t.Fatalf("successful conversion left temp files behind: %v", temps) } @@ -688,6 +794,41 @@ func TestCheckSessionAudioRefusesSymlink(t *testing.T) { } } +// TestCheckSessionAudioDistinguishesUnreadableFromMissing pins the +// missing-vs-unreadable split: only a genuinely absent audio.wav earns the "run +// `testimony record` first" guidance. A different Lstat failure — here EACCES +// from an unsearchable directory — must surface as a read error, because +// sending the operator to re-record a session whose audio exists destroys the +// misdiagnosed session's value. +func TestCheckSessionAudioDistinguishesUnreadableFromMissing(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permission checks") + } + sealed := filepath.Join(t.TempDir(), "sealed") + if err := os.Mkdir(sealed, 0o755); err != nil { + t.Fatal(err) + } + wav := filepath.Join(sealed, session.AudioFile) + if err := os.WriteFile(wav, []byte("RIFF real audio"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(sealed, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(sealed, 0o755) }) + + err := checkSessionAudio(wav, sealed) + if err == nil { + t.Fatal("an unreadable audio.wav must error") + } + if strings.Contains(err.Error(), "testimony record") { + t.Fatalf("an unreadable audio.wav was misreported as absent (re-record advice): %v", err) + } + if !strings.Contains(err.Error(), "reading") { + t.Fatalf("the error should name the read failure, got %v", err) + } +} + // TestResolveModelRefusesFIFO is the model-path twin of the FIFO refusals on the // package's other subprocess-input sites (TestConvertAudioRefusesFIFOInput, // TestConvertAudioRefusesFIFOOutput, TestCheckSessionAudioRefusesFIFO). The From 3f9597f511fdaf7eb0ac7e2117d7ff1316da3dae Mon Sep 17 00:00:00 2001 From: Alex Reppel <77722411+REPPL@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:32:30 +0100 Subject: [PATCH 4/6] Bound stopChild's post-SIGKILL reap; honour umask on converted audio MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fourth adversarial review round (all agents on Opus, zero errors) confirmed one medium defect and flagged one consistency regression from the previous fix commit. Each carries a regression test verified failing against the neutralised pre-fix code. - stopChild's second wait on c.done, after the escalation SIGKILL, was unbounded — the exact hang d9098a7 had just bounded in probeDevices, whose own comment establishes that this wait can never complete for a child pinned in an uninterruptible kernel wait (a wedged capture driver). Because stopAll is sequential, one such recorder kept SIGINT from ever reaching the rest, so finaliseOutputs, stopDemo, and the Next commands never ran and `record` hung until killed externally. stopChild now bounds the reap with stopReapGrace, abandons the unreapable child (marking it killed so its artefact is distrusted), and returns so the shutdown completes. - The audio-conversion chmod restored a flat 0644 rather than the umask-masked mode a direct ffmpeg write produces, so under a restrictive umask the microphone recording came out wider than the operator's umask and every sibling artefact. It now applies 0644 &^ umask, matching the record-side audio.wav. (The review panel refuted the world-readable *harm* as contained by the 0700 session directory, but confirmed the mechanism; this restores consistency for the regression the prior commit introduced.) Assisted-by: Claude:claude-fable-5 --- internal/record/proc.go | 45 ++++++++++++++++++-------- internal/record/record.go | 7 ++++ internal/record/record_test.go | 30 +++++++++++++++++ internal/transcribe/ffmpeg.go | 16 ++++++--- internal/transcribe/transcribe_test.go | 27 ++++++++++++++++ 5 files changed, 108 insertions(+), 17 deletions(-) diff --git a/internal/record/proc.go b/internal/record/proc.go index 0d18ced..c90fa60 100644 --- a/internal/record/proc.go +++ b/internal/record/proc.go @@ -116,7 +116,7 @@ type liveChild struct { started time.Time // when watching began; used to tell a start-up failure from a mid-session stop done chan struct{} // closed once, when Wait returns err error // Wait result; read only after done is closed - killed bool // set by stopChild when its escalation SIGKILL terminated the child; read after stopAll + killed bool // set by stopChild when its escalation SIGKILL terminated the child, or when the child could not be reaped at all; read after stopAll } // watch starts the single reaper goroutine. It must be called exactly once, @@ -137,31 +137,50 @@ func newLiveChild(stream string, p proc, stderr *lockedBuffer) *liveChild { // stop asks one child to finalise its container: SIGINT (ffmpeg writes the // trailer/moov atom and exits), wait up to grace, and only on timeout escalate -// to SIGKILL. It returns after the child has been reaped. +// to SIGKILL. It returns after the child has been reaped, or — if even SIGKILL +// cannot reap it within stopReapGrace — after abandoning it, so the sequential +// shutdown never hangs on one wedged recorder. func stopChild(c *liveChild, grace time.Duration) { _ = c.p.Signal(syscall.SIGINT) select { case <-c.done: + return case <-time.After(grace): - // The recorder did not finalise its container within the grace period. SIGKILL - // leaves whatever bytes were flushed — for an MP4, whose moov atom is written - // only on clean shutdown, that is very likely a truncated, unplayable file. Mark - // it so finaliseOutputs surfaces the risk instead of blessing the file by size. - // killed is written here on the caller's goroutine (stopAll, sequential) and - // read only after stopAll returns, so no synchronisation is needed. - // + } + // The recorder did not finalise its container within the grace period. SIGKILL + // leaves whatever bytes were flushed — for an MP4, whose moov atom is written + // only on clean shutdown, that is very likely a truncated, unplayable file. Mark + // it so finaliseOutputs surfaces the risk instead of blessing the file by size. + // killed is written here on the caller's goroutine (stopAll, sequential) and + // read only after stopAll returns, so no synchronisation is needed. + _ = c.p.Signal(syscall.SIGKILL) + select { + case <-c.done: // Condemn the artefact only when the SIGKILL is what actually terminated the // recorder. A child that finalised and exited cleanly right at the grace - // boundary can still land in this branch — the reaper closes done only after - // Wait returns (which also joins the stderr copier), so there is a scheduling + // boundary can still land here — the reaper closes done only after Wait + // returns (which also joins the stderr copier), so there is a scheduling // window between a clean exit and done becoming readable, and select picks // randomly when both cases are ready. In that window the SIGKILL hits an // already-exited process (a no-op); marking such a child killed reported a // complete, playable recording as "likely truncated or unplayable" and failed // the run. The reaped wait status is ground truth for who ended the process, // and c.err/ProcessState are stable once done is closed. - _ = c.p.Signal(syscall.SIGKILL) - <-c.done c.killed = c.p.DiedOf(syscall.SIGKILL) + case <-time.After(stopReapGrace): + // Even SIGKILL did not produce an exit within the reap grace: the child is + // pinned in an uninterruptible kernel wait (a wedged capture driver defers + // signal delivery until the kernel call returns — possibly never), so no wait + // can reap it. Without this bound the whole shutdown hangs on the unbounded + // receive: stopAll is sequential, so one wedged recorder keeps SIGINT from + // ever reaching the rest and finaliseOutputs/stopDemo/nextCommands never run — + // the operator's only recourse being to kill record externally, which skips + // finalisation and orphans every still-live recorder. Abandon the reaper + // goroutine (it closes done if the child ever dies; nothing else can reap it — + // the same accepted leak as probeDevices' probeKillGrace path) and treat the + // artefact as untrusted so finaliseOutputs surfaces it. This mirrors the bound + // d9098a7 gave probeDevices, whose comment established that this very wait can + // never complete for a wedged child. + c.killed = true } } diff --git a/internal/record/record.go b/internal/record/record.go index 9dd826d..3722210 100644 --- a/internal/record/record.go +++ b/internal/record/record.go @@ -43,6 +43,13 @@ import ( // assertion. Production never reassigns them. var stopGrace = 5 * time.Second +// stopReapGrace bounds how long stopChild waits for the reaper after the +// escalation SIGKILL, mirroring recorders.go's probeKillGrace. A wedged capture +// driver can defer even SIGKILL delivery indefinitely (the child is pinned in an +// uninterruptible kernel wait), so an unbounded reap here would hang the whole +// sequential shutdown on one bad recorder. A var only so tests can shrink it. +var stopReapGrace = 2 * time.Second + // startupWindow bounds how soon after a recorder starts an exit is still // treated as a start-up failure (e.g. a TCC denial, which fails within a // second or two). A recorder that ran longer than this before exiting cannot diff --git a/internal/record/record_test.go b/internal/record/record_test.go index fcc8980..d7a618e 100644 --- a/internal/record/record_test.go +++ b/internal/record/record_test.go @@ -914,6 +914,36 @@ func TestStopChildEscalatesToSIGKILL(t *testing.T) { } } +// TestStopChildAbandonsUnreapableChild is the wedged-shutdown regression. A +// recorder pinned in an uninterruptible kernel wait cannot be reaped even by +// SIGKILL, so stopChild's post-SIGKILL receive on c.done never completes; pre-fix +// (an unbounded <-c.done) that hung the whole sequential shutdown on one bad +// recorder, so finaliseOutputs and the Next commands never ran and record hung +// until killed externally. stopChild must bound the reap, abandon the child, and +// mark it killed so its artefact is distrusted. The fake never exits on any signal +// stopChild sends, modelling the unreapable child (its reaper goroutine blocks in +// Wait forever, by design). +func TestStopChildAbandonsUnreapableChild(t *testing.T) { + old := stopReapGrace + stopReapGrace = 30 * time.Millisecond + t.Cleanup(func() { stopReapGrace = old }) + + fp := newFakeProc(syscall.SIGUSR1) // a signal stopChild never sends, so Wait never returns + c := newLiveChild(streamScreen, fp, &lockedBuffer{}) + + start := time.Now() + stopChild(c, 10*time.Millisecond) + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("stopChild did not bound its wait on an unreapable child: took %v", elapsed) + } + if !c.killed { + t.Fatal("an abandoned, unreapable recorder must be marked killed so finaliseOutputs distrusts its artefact") + } + if got := fp.sent(); len(got) != 2 || got[0] != syscall.SIGINT || got[1] != syscall.SIGKILL { + t.Fatalf("expected SIGINT then SIGKILL before abandoning, got %v", got) + } +} + // TestStopChildDoesNotCondemnCleanExitAtGraceBoundary is the misclassification // regression at the grace deadline. A recorder that finalises and exits cleanly // right at the boundary can still land in stopChild's timeout branch — the reaper diff --git a/internal/transcribe/ffmpeg.go b/internal/transcribe/ffmpeg.go index 685fa35..1ad3064 100644 --- a/internal/transcribe/ffmpeg.go +++ b/internal/transcribe/ffmpeg.go @@ -8,6 +8,7 @@ import ( "os/exec" "path/filepath" "strings" + "syscall" "time" "github.com/REPPL/Testimony/internal/session" @@ -81,10 +82,17 @@ func atomicConvert(out string, produce func(tmpPath string) error) error { if err := produce(tmpPath); err != nil { return err } - // os.CreateTemp reserves the name at 0600; the finished audio.wav is an - // ordinary session artefact, so restore the 0644 a direct write would have - // given it — otherwise the conversion silently narrows the file's mode. - if err := os.Chmod(tmpPath, 0o644); err != nil { + // os.CreateTemp reserves the name at 0600; a direct ffmpeg write would instead + // have created audio.wav honouring the operator's umask — 0644 for the default + // 022, but 0600 under a restrictive umask a privacy-conscious operator sets so + // the microphone recording is not group/world-readable. Restore that + // umask-masked mode, matching the record-side audio.wav and every sibling + // artefact (all created through umask-masked opens); a flat 0644 would silently + // widen this one file past the umask. The brief syscall.Umask(0) probe is safe + // here — transcribe creates no other file concurrently. + um := syscall.Umask(0) + syscall.Umask(um) + if err := os.Chmod(tmpPath, 0o644&^os.FileMode(um)); err != nil { return fmt.Errorf("audio convert: finalise %s: %w", out, err) } if err := os.Rename(tmpPath, out); err != nil { diff --git a/internal/transcribe/transcribe_test.go b/internal/transcribe/transcribe_test.go index 80581ba..b2c4bab 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -510,6 +510,33 @@ func TestConvertAudioIntegration(t *testing.T) { } } +// TestAtomicConvertHonoursUmask is the file-mode regression. os.CreateTemp +// reserves the temp at 0600, so the finished audio.wav must be widened back to +// the mode a direct ffmpeg write would have produced — which honours the +// operator's umask, exactly like the record-side audio.wav and every sibling +// artefact. A flat chmod 0644 (the pre-fix code) widened this one file past a +// restrictive umask, so a privacy-conscious operator's microphone recording came +// out group/world-readable. Under umask 077 the result must be 0600, not 0644. +func TestAtomicConvertHonoursUmask(t *testing.T) { + old := syscall.Umask(0o077) + t.Cleanup(func() { syscall.Umask(old) }) + + dir := t.TempDir() + out := filepath.Join(dir, session.AudioFile) + if err := atomicConvert(out, func(tmpPath string) error { + return os.WriteFile(tmpPath, []byte("RIFF....complete"), 0o644) + }); err != nil { + t.Fatalf("atomicConvert: %v", err) + } + fi, err := os.Stat(out) + if err != nil { + t.Fatalf("stat converted audio: %v", err) + } + if got := fi.Mode().Perm(); got != 0o600 { + t.Fatalf("under umask 077 the converted %s must honour the umask (0600), got %v — a flat 0644 widens the recording past the operator's umask", session.AudioFile, got) + } +} + // TestConvertAudioRoutesThroughTemp pins the convertAudio → atomicConvert // wiring at the call site, hermetically (the convertRunner stub stands in for // ffmpeg; LookPath resolves against a fake on PATH). The producer must be From 9af6c0ff867c2fa9e51f6551c369dd808a1445c6 Mon Sep 17 00:00:00 2001 From: Alex Reppel <77722411+REPPL@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:42:23 +0100 Subject: [PATCH 5/6] Fix probeSink.Write to honour the io.Writer full-count contract Round 3 of the adversarial review (Opus, zero errors) surfaced no defects; its sole nitpick was a genuine contract violation in the probeDevices sink added by d9098a7: on the overflow branch Write returned the truncated byte count, so a misbehaving ffmpeg flooding past the 1 MiB cap would make os/exec's copy goroutine abort the pump with ErrShortWrite mid-listing. A bounded sink must absorb-and-drop overflow while reporting the full count, as lockedBuffer already does. Latent only because a real device listing never approaches the cap. Regression test verified failing against the pre-fix short-count return. Assisted-by: Claude:claude-fable-5 --- internal/record/record_test.go | 20 ++++++++++++++++++++ internal/record/recorders.go | 7 ++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/record/record_test.go b/internal/record/record_test.go index d7a618e..3240e33 100644 --- a/internal/record/record_test.go +++ b/internal/record/record_test.go @@ -173,6 +173,26 @@ func fakeFFmpeg(t *testing.T, script string) string { return path } +// TestProbeSinkReturnsFullCount pins the io.Writer contract on the bounded probe +// sink: even a write that straddles or exceeds the retention cap must report the +// full byte count, because os/exec's copy goroutine treats a short count with a +// nil error as ErrShortWrite and aborts the pump mid-listing. Pre-fix the +// straddling write returned only the retained-room count. +func TestProbeSinkReturnsFullCount(t *testing.T) { + var s probeSink + first := make([]byte, probeSinkRetain-10) // leaves 10 bytes of room + if n, err := s.Write(first); n != len(first) || err != nil { + t.Fatalf("Write returned (%d, %v), want (%d, nil)", n, err, len(first)) + } + straddle := make([]byte, 100) // 10 bytes fit, 90 dropped — must still report 100 + if n, err := s.Write(straddle); n != len(straddle) || err != nil { + t.Fatalf("straddling Write returned (%d, %v), want (%d, nil)", n, err, len(straddle)) + } + if n, err := s.Write([]byte("more")); n != 4 || err != nil { // sink already full + t.Fatalf("full-sink Write returned (%d, %v), want (4, nil)", n, err) + } +} + // TestProbeDevicesTimesOut is the wedged-enumeration regression: a hung device // listing must surface an actionable timeout instead of hanging `testimony // record` forever before the interrupt handler is live. `exec` replaces the diff --git a/internal/record/recorders.go b/internal/record/recorders.go index b1773f8..8cbe9e1 100644 --- a/internal/record/recorders.go +++ b/internal/record/recorders.go @@ -155,13 +155,18 @@ const probeSinkRetain = 1 << 20 func (s *probeSink) Write(p []byte) (int, error) { s.mu.Lock() defer s.mu.Unlock() + // Report the full count even when the retention cap discards the tail: a + // bounded sink absorbs-and-drops overflow, and returning a short count with a + // nil error violates io.Writer and makes os/exec's copy goroutine abort the + // pump with ErrShortWrite (mid-listing, on a flood past the cap). + n := len(p) if room := probeSinkRetain - len(s.buf); room > 0 { if len(p) > room { p = p[:room] } s.buf = append(s.buf, p...) } - return len(p), nil + return n, nil } func (s *probeSink) text() string { From 939f0e2da10dea6326ba098022ed1c494267d998 Mon Sep 17 00:00:00 2001 From: Alex Reppel <77722411+REPPL@users.noreply.github.com> Date: Fri, 24 Jul 2026 17:33:42 +0100 Subject: [PATCH 6/6] Release v0.4.0: finalise CHANGELOG Close the Unreleased section as v0.4.0, fold in the stopChild reap-bound, the umask-masked audio mode, and the probeSink io.Writer fix from the later review rounds, and repair the comparison-link footer (the Unreleased link still pointed at v0.2.0 and the v0.3.0 link was missing). Assisted-by: Claude:claude-fable-5 --- CHANGELOG.md | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4054ff2..b6682cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ leading `v`. Before v1.0.0, minor releases may make breaking changes; each one is called out in a **Breaking** section. -## [Unreleased] +## [0.4.0] - 2026-07-24 A second robustness pass over the same capture → analysis pipeline, closing the defects a multi-round review surfaced after v0.3.0. Every fix carries a @@ -65,8 +65,8 @@ Capture reliability (`record`/`transcribe`, macOS): - A recorder that had to be force-stopped (it missed the finalisation grace) is flagged, so a truncated, unplayable `screen.mp4` is no longer reported as good. -A third adversarial pass over that same commit closed what its own review had -missed: +Further adversarial review passes over that same commit closed what it had +itself missed: - Text sanitisation now strips every invisible Unicode format character (zero width space, word joiner, BOM, soft hyphen, the tag block), not only the bidi @@ -88,8 +88,20 @@ missed: missing-vs-unreadable audio split, the report code-span escape, the review error-path sanitisation, and the atomic-conversion call-site wiring — fixes whose tests previously stayed green when the fix was reverted. -- A converted `audio.wav` keeps the ordinary `0644` file mode instead of - silently inheriting the temp file's `0600`. +- A converted `audio.wav` is written with the operator's umask-masked mode, like + every other session artefact and the record-side `audio.wav`, rather than the + temp file's private `0600` or a flat `0644` wider than the umask allows. +- The recorder shutdown no longer hangs on a wedged capture device: the wait + after the escalation `SIGKILL` is now bounded, so a child pinned in an + uninterruptible kernel wait is abandoned (and its artefact distrusted) instead + of stalling the whole sequential shutdown before `record` can finalise its + outputs and print the follow-up commands. +- Device enumeration survives an unresponsive child rather than hanging on it: + the wait is structured so an expired deadline always takes effect even when the + child cannot be reaped, closing a residual hang the first timeout could not. +- The bounded enumeration-output sink honours the `io.Writer` contract on + overflow, so a flood past its cap can no longer abort the capture of the + listing partway through. ### Changed @@ -219,6 +231,7 @@ Resource and process lifecycle: - A one-line installer and a checksummed release of static binaries for macOS and Linux. -[Unreleased]: https://github.com/REPPL/Testimony/compare/v0.2.0...HEAD +[0.4.0]: https://github.com/REPPL/Testimony/compare/v0.3.0...v0.4.0 +[0.3.0]: https://github.com/REPPL/Testimony/compare/v0.2.0...v0.3.0 [0.2.0]: https://github.com/REPPL/Testimony/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/REPPL/Testimony/releases/tag/v0.1.0