Conversation
…unt-7) Adversarially-verified fixes from the bughunt-7 security review. Each carries a regression test verified failing against the pre-fix code; the full gate (build, gofmt, go vet, go test -race, and the merge/report pipeline smoke) is green. Concurrency & durability of the verdict record (findings.jsonl): - Close the Ingest/AppendVerdict TOCTOU: Ingest now runs its verdict-guard probe, truncate, and rewrite as one step under an exclusive flock (commitFindings), matching the lock AppendVerdict holds, so a concurrent re-ingest can no longer destroy a committed verdict. - Bind each verdict to the finding the analyst judged: AppendVerdict re-verifies under its lock that the targeted id still names the same finding, so a concurrent re-ingest that reuses an id cannot misattribute the verdict. Untrusted time-magnitude class: - Bound utterance t0/t1 magnitude in timeline.checkedUtterances (the speech-side twin of the interaction range guard). - Guard report.clock and review.clock against non-finite / out-of-range seconds before the float64→int conversion, defending the display sink for a hand-authored timeline.jsonl/findings.jsonl that bypasses merge. - Replace report's finite 1e18 standalone-event flush sentinel with +Inf, which had silently dropped events at or past it. Shown-vs-validated consistency: - EmitRequest sanitises each emitted timeline line, so analyze.validate now indexes and compares in SafeText form (ids, utterance text, selectors, routes, quotes); an honest verbatim-copied answer over control/bidi-bearing text is no longer rejected as "not present". Untrusted-read bounds and disclosure: - Cap LoadManifest at 1 MiB; hold AppendVerdict to MaxJSONLLine; guard the read-side FIFO on holdsVerdicts; keep the raw session path out of report.md's findings-unavailable notice. record / transcribe: - transcribe persists an external recording's audio→session offset in an audio.offset.json sidecar, so a bare re-run recovers it instead of silently assuming 0; a malformed sidecar refuses with guidance. - record captures the avfoundation ":default" microphone (not enumerated index 0) and logs the detected input roster, so a virtual audio driver is not silently recorded in the real mic's place. (avfoundation capture is not exercised by CI; verify on macOS hardware before release.) - probeDevices surfaces ffmpeg's own diagnostic when the device listing is empty; surface a device-listing run failure rather than misreporting "no microphone found". Assisted-by: Claude:claude-fable-5
A multi-round review after v0.3.0 surfaced these; each fix carries a regression test verified failing against the pre-fix code. Analysis, report, and review integrity: - Roll findings.jsonl back to empty on a failed write, so a partial line no longer bricks the file against its own re-ingest recovery (holdsVerdicts could not parse it to overwrite it). - Neutralise inline Markdown in report.md so an attacker-authored quote cannot smuggle a remote-image tracking beacon into the shareable artefact. - Reject a finding quote that sanitises to empty (a verbatim-check bypass, since Contains(text, "") is always true). - Refuse duplicate finding ids on load, naming the line, instead of collapsing them under one status in the report. - Bound interaction-time magnitude at merge, matching the utterance-side twin. - Sanitise finding ids in review error strings before they reach the terminal. Untrusted-session filesystem handling: - Route analyze -ingest/-out through the no-follow guard (symlink/FIFO refusal), as every other session-artefact read and write already is. - Refuse a symlinked in-place audio.wav (read-side out-of-session exfil into the re-shareable transcript.jsonl). - Distinguish a missing audio.wav from an unreadable one. - Bound the derived and persisted audio offset at transcribe, where the bad recording metadata enters, rather than one command later at merge. Capture reliability (record/transcribe, macOS): - Time out avfoundation device enumeration so a wedged device or driver cannot hang record before it can be interrupted. - Bound retained recorder stderr so a device-stall log flood over a long session cannot exhaust memory and orphan the recorders. - Convert audio via a temp file renamed on success, so an interrupted or crashed ffmpeg never leaves a partial audio.wav a later run would transcribe as whole. - Flag a force-stopped recorder so a truncated, unplayable screen.mp4 is not reported as good by the size-only check. Behaviour change: analyze -ingest/-out now require a regular file and reject named pipes, process substitution, and /dev/stdout; use -ingest - for stdin and omit -out for stdout. Recorded in CHANGELOG. Assisted-by: Claude:claude-fable-5
An independent multi-agent adversarial review of the previous commit (13 finders, every finding put to three refuters on distinct lenses) confirmed four behavioral defects and seven coverage gaps. Each fix below carries a regression test verified failing against the neutralised pre-fix code. Behavioral fixes: - Widen SafeText from the Bidi_Control enumeration to every Unicode format character (Cf): ZWSP, word joiner, ZWNBSP/BOM, soft hyphen, and the U+E00xx tag block render as nothing while surviving in the bytes, so displayed requests/quotes could still differ invisibly from the recorded bytes (ASCII smuggling past the Trojan-Source guard). - Judge quote emptiness after trimming: a quote of "\t" sanitises to " ", which cleared the sanitised-empty check and trivially satisfied the verbatim-substring gate against any utterance containing a space. - Set a recorder's killed flag from the reaped wait status (death by the escalation SIGKILL) instead of unconditionally in the timeout branch: a child that finalised cleanly right at the grace boundary was condemned as truncated and failed the run for a complete recording. - Restructure probeDevices from CommandContext+CombinedOutput to start/reap/select: Cmd.Wait blocks in Process.Wait before consuming the context cancellation, so a child pinned in an uninterruptible kernel wait (the wedged-driver case the deadline exists for) hung record forever despite the expired deadline. The child now gets SIGKILL plus a bounded reap grace, then is abandoned with an actionable error; a listing that completed as the deadline fired is used rather than discarded. - Restore 0644 on the converted audio.wav (os.CreateTemp reserves at 0600, and the rename silently narrowed the artefact's mode). Coverage (tests that previously stayed green with their fix reverted): - Hermetic fake-ffmpeg tests for both probeDevices timeout paths and the by-design non-zero exit; the enumeration is no longer untested. - The derived-offset magnitude bound, via a stubbed derivation seam. - The missing-vs-unreadable audio.wav distinction (EACCES directory). - The mdCode backtick strip (code-span beacon escape) in report.md. - The five review error-path SafeText sites beyond the one already tested. - The convertAudio→atomicConvert call-site wiring, via a conversion-runner seam (a real ffmpeg failure aborts before creating output, so no integration case can observe the wiring), plus a real-ffmpeg failure case. Assisted-by: Claude:claude-fable-5
A fourth adversarial review round (all agents on Opus, zero errors) confirmed one medium defect and flagged one consistency regression from the previous fix commit. Each carries a regression test verified failing against the neutralised pre-fix code. - stopChild's second wait on c.done, after the escalation SIGKILL, was unbounded — the exact hang d9098a7 had just bounded in probeDevices, whose own comment establishes that this wait can never complete for a child pinned in an uninterruptible kernel wait (a wedged capture driver). Because stopAll is sequential, one such recorder kept SIGINT from ever reaching the rest, so finaliseOutputs, stopDemo, and the Next commands never ran and `record` hung until killed externally. stopChild now bounds the reap with stopReapGrace, abandons the unreapable child (marking it killed so its artefact is distrusted), and returns so the shutdown completes. - The audio-conversion chmod restored a flat 0644 rather than the umask-masked mode a direct ffmpeg write produces, so under a restrictive umask the microphone recording came out wider than the operator's umask and every sibling artefact. It now applies 0644 &^ umask, matching the record-side audio.wav. (The review panel refuted the world-readable *harm* as contained by the 0700 session directory, but confirmed the mechanism; this restores consistency for the regression the prior commit introduced.) Assisted-by: Claude:claude-fable-5
Round 3 of the adversarial review (Opus, zero errors) surfaced no defects; its sole nitpick was a genuine contract violation in the probeDevices sink added by d9098a7: on the overflow branch Write returned the truncated byte count, so a misbehaving ffmpeg flooding past the 1 MiB cap would make os/exec's copy goroutine abort the pump with ErrShortWrite mid-listing. A bounded sink must absorb-and-drop overflow while reporting the full count, as lockedBuffer already does. Latent only because a real device listing never approaches the cap. Regression test verified failing against the pre-fix short-count return. Assisted-by: Claude:claude-fable-5
Close the Unreleased section as v0.4.0, fold in the stopChild reap-bound, the umask-masked audio mode, and the probeSink io.Writer fix from the later review rounds, and repair the comparison-link footer (the Unreleased link still pointed at v0.2.0 and the v0.3.0 link was missing). Assisted-by: Claude:claude-fable-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A second robustness pass over the capture → analysis pipeline, closing defects an
adversarial multi-round review surfaced after v0.3.0. The base commits (
2a293d0,built on
1363055) landed the fixes; three further commits on this branch closewhat an independent four-round review of
2a293d0then found.Review method
Each round fanned out 13 file/lens finders over the diff, then put every
surviving finding to three independent adversarial refuters on distinct lenses
(reachability, already-guarded, reproduction); a majority refute killed a
finding. Rounds ran until two consecutive passes surfaced only nitpicks. Every
fix carries a regression test verified failing against the neutralised pre-fix
code; the full gate (
go build,gofmt -l,go vet,go test -race ./...)and the merge+report smoke are green.
What the review found and fixed (beyond
2a293d0)d9098a7— round 1 (6 behavioural + 5 coverage):SafeTextwidened from the bidi-control set to all of Unicode categoryCf,closing invisible-character smuggling (ZWSP, word joiner, BOM, soft hyphen, the
U+E00xx tag block) between displayed text and recorded bytes.
twin of the sanitises-to-empty bypass).
killedflag is derived from the reaped wait status, so arecorder that finalised cleanly at the grace boundary is no longer condemned.
probeDevicesdevice enumeration restructured (start → own reaper → select) soan expired deadline takes effect even against a child
Cmd.Waitcannot reap.previously stayed green when their fix was reverted (mdCode backtick escape,
five review SafeText sinks, the derived-offset bound, the missing-vs-unreadable
audio split, the
convertAudio→atomicConvertwiring).3f9597f— round 2 (1 medium + 1 consistency):stopChild's post-SIGKILL reap was unbounded — the same hangd9098a7hadjust bounded in
probeDevices. A wedged capture child stalled the wholesequential shutdown before
recordcould finalise. Now bounded + abandon.audio.wavmode honours the umask (0o644 &^ umask) rather than aflat
0644wider than the operator's umask and every sibling artefact.9af6c0f— round 3 (nitpick):probeSink.Writehonours theio.Writerfull-count contract on overflow.Rounds 3 and 4 surfaced no confirmed defects (round 4: nothing at all) → converged.
Behaviour change (breaking, called out for v0.4.0)
analyze -ingest/-outnow require a regular file and reject named pipes,process substitution, and
/dev/stdout. Use-ingest -for stdin and omit-outfor stdout. See CHANGELOG.