Audio upload hardening follow-ups the first adversarial review could not verify or did not fix #46

Open
opened 2026-10-02 14:53:02 +00:00 by mathias · 2 comments
Owner

From the adversarial review of PR #39 (skills:pr-gatekeeper, head 7029dd1, verdict BLOCK; the findings it reproduced were fixed in the same PR, see ADR-035 decision 8). These are the items it listed as not verified, plus two limits I introduced while fixing. None blocks the PR; each is a place the upload feature is weaker than its ADRs may suggest.

Not verified by the reviewer (so unknown, not safe)

  1. mov alis external references to sibling spool files. Names are random, so judged impractical; never crafted or run.
  2. ffprobe abuse: extreme stream counts and probe CPU time; there is no probe timeout beyond the job's.
  3. CSRF on POST /uploads: it follows the app's existing pattern; the cookie flags were not reviewed.
  4. alive_at vs the app clock: the heartbeat stamps with the database's NOW(), the page compares with the application clock. A skew larger than the 45 s threshold would show a healthy job as lost.
  5. A hung database blocks Tracker.save, which runs on the parent context under the tracker's mutex: a stalled DB could stall phase reporting and, through the mutex, the heartbeat.
  6. An unbounded gateway response body: llm.Client reads the whole body before looking at the status.
  7. Non-amd64: the Dockerfile's ffmpeg is amd64 only.
  8. Whether a real LiteLLM echoes prompts in its error bodies. The mechanism that could leak them is closed (errors are logged and stored as kinds), so this only matters for the narrower HTTP error text the llm client still returns to other callers.

Limits from the fixes
9. A partial recording inside the tolerance is accepted silently. A file that converts to 80% or more of the length it claimed, or less than two minutes short, is transcribed with no notice. The tolerance exists because variable-bitrate estimates are imprecise; a user-facing "transcribed X of Y minutes" notice would remove the silence.
10. Vendor the ffmpeg tarball. Upstream's releases/ path holds only the current version, so the pinned 7.0.2 moves to old-releases/ (the build tries both) or disappears; the build then fails. The durable fix is to store the verified tarball (sha256 abda8d77ce8309141f83ab8edf0596834087c52467f6badf376a6a2a4c87cf67) where the build can fetch it.
11. Image size: the two static binaries are about 80 MB each (about 160 MB added), not the 70 MB estimated in #34. Probing with ffmpeg -i instead of ffprobe would halve it; a code change.

Refs: PR #39, ADR-033, ADR-035, #34.


Created via git-mcp on behalf of @mathias

From the adversarial review of PR #39 (`skills:pr-gatekeeper`, head `7029dd1`, verdict BLOCK; the findings it reproduced were fixed in the same PR, see ADR-035 decision 8). These are the items it listed as **not verified**, plus two limits I introduced while fixing. None blocks the PR; each is a place the upload feature is weaker than its ADRs may suggest. **Not verified by the reviewer (so unknown, not safe)** 1. **mov `alis` external references** to sibling spool files. Names are random, so judged impractical; never crafted or run. 2. **ffprobe abuse**: extreme stream counts and probe CPU time; there is no probe timeout beyond the job's. 3. **CSRF** on `POST /uploads`: it follows the app's existing pattern; the cookie flags were not reviewed. 4. **`alive_at` vs the app clock**: the heartbeat stamps with the database's `NOW()`, the page compares with the application clock. A skew larger than the 45 s threshold would show a healthy job as lost. 5. **A hung database blocks `Tracker.save`**, which runs on the parent context under the tracker's mutex: a stalled DB could stall phase reporting and, through the mutex, the heartbeat. 6. **An unbounded gateway response body**: `llm.Client` reads the whole body before looking at the status. 7. **Non-amd64**: the Dockerfile's ffmpeg is amd64 only. 8. Whether a **real LiteLLM** echoes prompts in its error bodies. The mechanism that could leak them is closed (errors are logged and stored as kinds), so this only matters for the narrower HTTP error text the llm client still returns to other callers. **Limits from the fixes** 9. **A partial recording inside the tolerance is accepted silently.** A file that converts to 80% or more of the length it claimed, or less than two minutes short, is transcribed with no notice. The tolerance exists because variable-bitrate estimates are imprecise; a user-facing "transcribed X of Y minutes" notice would remove the silence. 10. **Vendor the ffmpeg tarball.** Upstream's `releases/` path holds only the current version, so the pinned 7.0.2 moves to `old-releases/` (the build tries both) or disappears; the build then fails. The durable fix is to store the verified tarball (sha256 `abda8d77ce8309141f83ab8edf0596834087c52467f6badf376a6a2a4c87cf67`) where the build can fetch it. 11. **Image size**: the two static binaries are about 80 MB each (about 160 MB added), not the 70 MB estimated in #34. Probing with `ffmpeg -i` instead of `ffprobe` would halve it; a code change. Refs: PR #39, ADR-033, ADR-035, #34. --- _Created via git-mcp on behalf of @mathias_
mathias added the risk/medium label 2026-10-02 14:53:02 +00:00
Author
Owner

Added after the second and third adversarial passes on PR #39 (verdicts and fixes are on the PR):

  1. io.Copy into the spool file: a write fault is not distinguishable from a read fault in a test. Accept now marks only errors from reading the client's body as ErrClientRead (a mutant that marks every receive error survives, because Accept writes to a concrete *os.File and a disk-full cannot be injected without refactoring the spool behind an interface). The guard is cr.err != nil && errors.Is(err, cr.err).
  2. A stopped worker answers "busy, try again in a few minutes" (a 429) although it will not come back. Better than the previous 500, but the message misleads; a 503 with its own wording would be honest.
  3. The video chat's switcher lists the upload chat model too while uploads are on (local only). Recorded in ADR-035; a per-surface model list would restore "video chat unchanged".
  4. Invisible characters that do not reorder text survive titles (U+FEFF, tag characters U+E0000-E007F). Cosmetic.
  5. apt-get install xz-utils in the ffmpeg build stage is an unpinned fetch (verified by apt's own signatures; the golang tag is unpinned too). It runs after the ffmpeg checksum and affects the build stage only. Pinning the base image by digest would close it.

Created via git-mcp on behalf of @mathias

Added after the second and third adversarial passes on PR #39 (verdicts and fixes are on the PR): 12. **`io.Copy` into the spool file: a write fault is not distinguishable from a read fault in a test.** `Accept` now marks only errors from reading the client's body as `ErrClientRead` (a mutant that marks every receive error survives, because `Accept` writes to a concrete `*os.File` and a disk-full cannot be injected without refactoring the spool behind an interface). The guard is `cr.err != nil && errors.Is(err, cr.err)`. 13. **A stopped worker answers "busy, try again in a few minutes"** (a 429) although it will not come back. Better than the previous 500, but the message misleads; a 503 with its own wording would be honest. 14. **The video chat's switcher lists the upload chat model too** while uploads are on (local only). Recorded in ADR-035; a per-surface model list would restore "video chat unchanged". 15. **Invisible characters that do not reorder text survive titles** (U+FEFF, tag characters U+E0000-E007F). Cosmetic. 16. **`apt-get install xz-utils` in the ffmpeg build stage is an unpinned fetch** (verified by apt's own signatures; the golang tag is unpinned too). It runs after the ffmpeg checksum and affects the build stage only. Pinning the base image by digest would close it. --- _Created via git-mcp on behalf of @mathias_
mathias added this to the v0.32.0 - audio upload follow-ups milestone 2026-10-03 18:50:00 +00:00
Author
Owner

Sprint triage (phase 1). Split out: item 2 (ffprobe memory) -> #49; items 4+5 (tracker hung save, clock) -> #51; item 6 (LLM unbounded read) -> #53; cross-origin POST (found while checking item 3) -> #48. Item 1 (alis/dref external reference) checked: not a problem under the application's flags (ffmpeg skips the external track, 78-byte WAV). One correction to this issue: Tracker.Beat does not take the tracker mutex; only logStill does. What remains here is the rest of the list; ffmpeg tarball location is D7 (do nothing, file an issue when it bites). Milestone v0.32.0. Plan: docs/sprints/audio-upload-followups-2026-10.md


Created via git-mcp on behalf of @mathias

Sprint triage (phase 1). Split out: item 2 (ffprobe memory) -> #49; items 4+5 (tracker hung save, clock) -> #51; item 6 (LLM unbounded read) -> #53; cross-origin POST (found while checking item 3) -> #48. Item 1 (`alis`/`dref` external reference) checked: not a problem under the application's flags (ffmpeg skips the external track, 78-byte WAV). One correction to this issue: `Tracker.Beat` does not take the tracker mutex; only `logStill` does. What remains here is the rest of the list; ffmpeg tarball location is D7 (do nothing, file an issue when it bites). Milestone v0.32.0. Plan: docs/sprints/audio-upload-followups-2026-10.md --- _Created via git-mcp on behalf of @mathias_
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/tapir#46