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/CHANGELOG.md b/CHANGELOG.md index aaeefd3..b6682cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,107 @@ 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 +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. + +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 + 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` 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 + +- **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 @@ -131,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 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..3cde60b 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,31 @@ 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) + // 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 for sc.Scan() { line++ @@ -108,12 +130,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 +152,31 @@ 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) + } + 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 { - 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..e69bd81 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -1,15 +1,95 @@ package analyze import ( + "errors" "fmt" "os" "path/filepath" "strings" + "syscall" "testing" + "time" "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") + } +} + +// 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). @@ -104,6 +184,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":[ @@ -116,6 +224,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":[]}`)) @@ -545,3 +675,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..cbe55db 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,99 @@ 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 := writeFindings(f, findings); err != nil { + f.Close() + return err + } + // Close releases the lock with the descriptor. + if err := f.Close(); err != nil { + return fmt.Errorf("write findings: %w", err) + } + 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 { + return fmt.Errorf("write findings: %w", err) + } + } + if err := f.Truncate(0); err != nil { + return fmt.Errorf("write findings: %w", err) + } + 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 } // oversizedFindings reports any finding whose findings.jsonl line — its JSON @@ -227,29 +311,19 @@ 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) + 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 380c7e5..9a112d3 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,35 @@ 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). - if f.Quote == "" { + // 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. + // 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. 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, 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/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/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/proc.go b/internal/record/proc.go index 655a869..c90fa60 100644 --- a/internal/record/proc.go +++ b/internal/record/proc.go @@ -1,7 +1,6 @@ package record import ( - "bytes" "os" "os/exec" "sync" @@ -15,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 @@ -35,28 +39,72 @@ 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. +// 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 +// 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 +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, 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, @@ -88,13 +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): - _ = c.p.Signal(syscall.SIGKILL) - <-c.done + } + // 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 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.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 84524c3..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 @@ -136,7 +143,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 } @@ -228,7 +235,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 } @@ -280,7 +302,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 +310,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 +332,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..3240e33 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,120 @@ 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) + } +} + +// 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 +} + +// 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 +// 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) { @@ -306,7 +423,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 +480,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 +584,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 +654,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 +757,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 @@ -727,13 +844,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 { @@ -747,7 +870,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 } @@ -757,6 +889,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() @@ -791,6 +929,147 @@ 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") + } +} + +// 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 +// 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 +// 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) { diff --git a/internal/record/recorders.go b/internal/record/recorders.go index 338de70..8cbe9e1 100644 --- a/internal/record/recorders.go +++ b/internal/record/recorders.go @@ -1,22 +1,46 @@ package record import ( + "errors" "fmt" "os/exec" "regexp" "strconv" "strings" + "sync" + "time" ) -// 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 { +// deviceListTimeout bounds the avfoundation device enumeration. It is a var, not a +// 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 +// 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 +106,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 +131,137 @@ 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 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() + // 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 micIndex, screenIndex, nil + return n, nil +} + +func (s *probeSink) text() string { + s.mu.Lock() + defer s.mu.Unlock() + return string(s.buf) } -// 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 -// ffmpeg); isolated here and skipped in CI. -func probeDevices(ffmpeg string, wantScreen bool) (micIndex, screenIndex int, err error) { +// 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 +// 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) { cmd := exec.Command(ffmpeg, "-hide_banner", "-f", "avfoundation", "-list_devices", "true", "-i", "") - out, _ := cmd.CombinedOutput() // always exits non-zero; the listing is on stderr - video, audio := parseAVDevices(string(out)) - return selectDevices(video, audio, wantScreen) + 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 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) + } + // 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 0, nil, err +} + +// 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/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 fd66f32..efb9539 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) { @@ -38,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++ @@ -90,7 +102,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 +124,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 } @@ -134,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") @@ -154,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 { @@ -192,6 +216,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 @@ -207,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 2a68011..2ebcd4f 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" @@ -237,6 +238,103 @@ 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, 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) + } + 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) + } + // 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 { t.Helper() b, err := os.ReadFile(filepath.Join(dir, session.FindingsFile)) @@ -328,3 +426,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..edc9889 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) @@ -186,15 +209,20 @@ func record(opts Options, 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 @@ -221,11 +249,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", + 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 // without first reading the byte already at the end of the file. @@ -233,6 +280,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 +317,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`", + 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", + 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`", + session.SafeText(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 +472,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..f35fe7c 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) } @@ -322,6 +323,101 @@ 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()) + } +} + +// 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 @@ -420,7 +516,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 +567,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..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 @@ -43,6 +44,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 +123,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 +147,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) } @@ -237,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 { @@ -253,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 7703ba2..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 { @@ -458,3 +466,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..6f815ac 100644 --- a/internal/timeline/timeline.go +++ b/internal/timeline/timeline.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io/fs" + "math" "path/filepath" "sort" @@ -140,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 @@ -171,6 +175,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 @@ -183,12 +195,34 @@ func readOptionalJSONL[T any](path string) ([]T, error) { // 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 { 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) + } + // 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) } @@ -232,8 +266,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{ @@ -288,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 fc91b3d..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 @@ -272,6 +300,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 +443,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/ffmpeg.go b/internal/transcribe/ffmpeg.go index 3644f17..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" @@ -40,13 +41,66 @@ 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) + // 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 { + 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, +// 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 + } + // 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 { + return fmt.Errorf("audio convert: finalise %s: %w", out, err) + } + return nil +} + // checkPlainOutput refuses an ffmpeg output path that already exists as anything // other than a regular file. ffmpeg is handed the path as a string and told to // overwrite it with -y, so this write cannot go through @@ -79,6 +133,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 29eb440..b0735f3 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 @@ -108,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) @@ -151,14 +185,111 @@ 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 + // 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 } + // 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 + +// 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. +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")) + } + // 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 +} + +// 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..b2c4bab 100644 --- a/internal/transcribe/transcribe_test.go +++ b/internal/transcribe/transcribe_test.go @@ -243,6 +243,115 @@ 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) + } +} + +// 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) + } +} + +// 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) { + 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 @@ -370,10 +479,162 @@ 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") } + + // 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) + } +} + +// 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 +// 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, +// 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) + } + // 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) + } } func TestResolveVAD(t *testing.T) { @@ -522,3 +783,115 @@ func TestCheckSessionAudioRefusesFIFO(t *testing.T) { t.Fatalf("a real audio.wav must be accepted, got %v", err) } } + +// 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) + } +} + +// 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 +// 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 } }