Files
xarmian d83632e258 feat(attachments): extension trust — the audio/video split, the text family, the CFB office trio and RTF (TASK-2976 / BUG-2963 PR B) (#1309)
* test(attachments): record the Void-beyond-window limit, and correct three comments that claimed more than their evidence (TASK-2976)

The first commit of BUG-2963 PR B, before any extension-trust work: round
6's three items, deliberately kept out of PR A so its verified tip would
not move, plus the nix terminator-comment correction folded in from
BUG-2974's trail.

1. TestMatroskaDocTypeBeyondTheWindowIsWebM records a LIMITATION. A Void
   element is legal anywhere in an EBML header and may be any size; make
   one larger than the 512 bytes this door reads and the DocType behind
   it is not in the input at all, so the parse finds nothing and the
   stdlib's video/webm stands. Nothing got worse — the same file was
   video/webm before the DocType read existed, and video/webm's own
   allowlist entry permits inline serving — and no larger window fixes
   it, since Void may be larger still. The fixture is the ordinary
   FFmpeg Matroska with a 560-byte Void spliced into its header and the
   header size widened to match, the same construction as
   matroska-void-padded.head512; the complete file reads as
   matroska,webm under ffprobe, and what is committed is its first 512
   bytes, so the DocType is absent by construction rather than by
   truncation accident.

2. TestTarWinsAPrefixCollision no longer calls the collision
   "asymmetric". That was the round-4 premise round 5 refuted with
   flac-ustar-in-comment.head512 — a FLAC's Vorbis COMMENT tags are
   arbitrary UTF-8, so real audio carries "ustar" at offset 257 as
   readily as a real tar carries an audio marker at offset zero. The
   implementation and the fixtures already said so; the stale word
   survived in the place a reader looks first. The comment now gives the
   real reason tar leads the default order, which is weaker: its magic
   sits at a fixed offset rather than at a prefix.

3. The seven-byte textual-AAC input's comment says which of the two
   things it is. It is a valid ADTS sync and layer signature that the
   stdlib reads as text — not a decodable AAC, and nothing in the test
   establishes that it is. What review established is the case it stands
   in for, and the comment now separates the two.

4. The nix loop-terminator comments named the wrong mechanism
   (BUG-2974, observed day 62). The heal push cannot loop because a push
   made with the default GITHUB_TOKEN creates no workflow runs at all —
   `gh run list --branch main` spans cbfc073e -> 31d11e76 -> 0b16be49
   and skips the heal commit 249a8f87 entirely. The corrected-tree
   argument the comments gave is true and UNEXERCISED; it is now written
   as the backstop, with the tripwire that it becomes load-bearing the
   moment that push uses a PAT or app token. The per-commit query is not
   the instrument for this and the comment says so: it also returned
   nothing for 0b16be49, which has runs.

Also swept: the fixture README's "the two EBML entries" was already
false at four, and nix.yml's "THAT DISTINCTION" lost its referent once
paragraphs landed above it. The new fixture joins the FuzzSniffMIME
seeds.

go test ./internal/attachments/ green; nix/heal-vendor-hash_test.sh 20
passed, 0 failed.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* feat(attachments): the ISO-BMFF audio/video split — .mov and .m4a stop being refused (TASK-2976, BUG-2963 F4)

Both iPhone-default containers were refused at upload while their types
sat on the allowlist. Measured on 06ccabdd before the change:

  clip.mov  (qt   brand) -> sniff application/octet-stream -> mime_not_allowed
  clip.m4a  (isom brand) -> sniff video/mp4 -> mime_extension_mismatch
  clip.m4a  (M4A  brand) -> sniff video/mp4 -> mime_extension_mismatch

Three legs, per the day-62 per-family ruling on BUG-2963:

1. Major brand "qt  " -> video/quicktime. The stdlib's mp4 matcher wants
   a brand beginning "mp4" and a QuickTime file carries none, so it had
   no opinion at all. This ADDS a detection, exactly as the still-image
   brands do.

2. Major brand "M4A " -> audio/mp4. This is the one place this package
   OVERRIDES a type the standard library named: an .m4a's compatible
   brands routinely include "mp41", so the stdlib answers video/mp4 from
   a compatible brand, having no way to weigh it against the major one.
   The major brand is the file's own statement about itself. The
   override is bounded — both types are allowlisted, both render inline,
   and the only thing that moves is the CATEGORY, which decides whether
   the UI offers an audio player or a video one.

   It is read from the MAJOR brand alone and BEFORE the mp4-yield, which
   is the whole mechanism: the yield would otherwise return "" on that
   same mp41 brand before the check could speak. Both facts are stated
   in the code.

3. isom-branded bytes named .m4a -> audio/mp4, in ValidateUpload beside
   the zip+document branch and as the same kind of trust one family
   over: the BYTES establish the container, the FILENAME chooses only
   the spelling within it. No track reads, per the ruling.

sniffISOBMFFImage is now sniffISOBMFF. It answers for audio and video
now, and a name saying Image while it returns audio/mp4 is the exact
defect class this bug keeps producing.

The controls are the test. An accept-only test is satisfied by a rule
that trusts .m4a outright, which is a much larger claim than the one
granted, so each leg carries what the STDLIB said about the same bytes
(the thing separating "adds a detection" from "overrides one"), and the
extension leg carries four: the same bytes named .mp4 stay video, the
same bytes with no extension stay video, a WebM named .m4a is still
refused (the trust does not reach outside the MP4 family), and PNG bytes
named .m4a are still refused (the extension introduces no type). A
synthetic ftyp box with major isom and "M4A " among its COMPATIBLE
brands is refused too — reading a category out of the compatible list is
a wider rule than the one granted.

Fixtures: quicktime.head512, m4a-brand.head512, m4a-isom.head512 — real
FFmpeg n7.1 output, provenance in the fixture README. Swept prose that
said this pre-check is still-images-only (SniffMIME's doc comment, the
mime_isobmff.go header).

go test ./internal/attachments/ green. Mutation matrix next, then F5.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* refactor(attachments): the .m4a branch's category guard was dead — assert the invariant it stood for instead (TASK-2976)

A mutation run says the guard survives removal, and it is right: the
branch answers for exactly .m4a, so extEntry.Category == CategoryAudio
can never differ from the ext test beside it. (The zip branch's
identical-looking guard IS load-bearing, because that branch answers for
many extensions at once.)

What the branch actually depends on is that .m4a maps to an allowlisted
audio entry, since it returns that entry. That is now asserted directly
in TestBUG2963F4M4AExtensionTrust, so a remap of .m4a fails a test
instead of quietly retyping MP4 bytes. Dead code replaced by a tested
invariant, rather than kept as defence in depth it cannot provide.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* feat(attachments): the text family — the extension chooses which text, category unchanged (TASK-2976, BUG-2963 F5)

text/plain is the stdlib saying "these bytes are text" and nothing more;
it has no signature separating Markdown from YAML from JavaScript
because at the byte level there is none to have. So when the sniff is
text/plain and the extension maps to an allowlisted TEXT entry, that
entry is what gets stored. Per the day-62 ruling.

Category-preserving is the whole safety argument: nothing becomes an
image, an archive or a document by being renamed, and a mapping that is
not an allowlisted text entry does not fire — .svg still reaches
extension_blocked rather than being quietly stored as text.

TestTextFamilyStillStoresAsPlain is now TestBUG2963F5TextFamily. It
recorded this boundary and said in its own failure message that F5
landing should move it; it kept the same three inputs and changed the
expectation, which is what "move it" meant.

THE SERVING BUCKET MOVES FOR EVERY TYPE F5 TOUCHES, NOT ONLY .js.
text/plain is the only member of the text family in inlineSafe, so
choosing any other text spelling takes the file out of inline serving
and into Content-Disposition: attachment. The ruling weighed that for
.js, where the move to RenderForceDownload is the point. It applies
equally to .md, .csv, .json, .yaml and .toml, where it is a side effect
— and it turns out to be the honest one: the client's own DR-5 already
says every text/* subtype but plain text "is downloaded or rendered
inconsistently across browsers", and those types only reached the
browser-preview affordance because the server was storing them as
text/plain. Whether any of the five belongs in inlineSafe is an
allowlist decision and is not taken here.

The in-app markdown preview is unaffected: it fetch()es the bytes and
renders them itself, which an attachment disposition does not impede,
and it now matches on the MIME rather than falling back to the
extension. Its prose in web/src/lib/attachments/display.ts said an
uploaded .md "is stored as text/plain" as a present-tense measured
fact; that is now history, and the fallback it justifies is what
PRE-EXISTING rows still depend on, so the comment says that instead
(CONVE-23, and its mirror in display.test.ts).

Controls, since an accept-only test is satisfied by "trust text
extensions outright": .txt / an unmapped extension / no extension at all
stay text/plain; PNG bytes named .md are still a mismatch; text bytes
named .mp3 are still a mismatch (an allowlisted mapping in another
category is the dangerous shape and the one the category guard is for);
.svg still fails extension_blocked; and the .js leg asserts RenderMode
== RenderForceDownload against the same bytes named .txt, not merely
that the type changed.

go test ./internal/attachments/ green. Mutation matrix next.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* feat(attachments): the legacy Office trio and RTF — two more families the door refused (TASK-2976, BUG-2963)

CFB. The container legacy Office wrote .doc/.xls/.ppt into has no stdlib
signature, so every one of those files sniffed application/octet-stream
and was refused while all three types sat on the allowlist. The branch
reads eight fixed bytes and says only "this is a CFB container", because
that is all eight bytes can say: .msi installers and Visio files wear
the same header, and telling them apart is a directory-stream walk this
door does not do. So the extension chooses which of the three REVIEWED
types is stored, exactly as the zip+document branch does one container
family over.

The extension list is written out rather than derived from extMIMEMap.
The question is not "does this extension map to a document" — .vsd and
.msi are CFB too — it is "is this one of the three the allowlist
reviewed", and that is a list, not a predicate. .docx is the sharp
control: a document extension whose type is allowlisted but whose
container is zip, refused on a CFB header.

RTF. Printable ASCII, so the stdlib answers text/plain — an OPINION, not
the absence of one — and a .rtf was refused as a mismatch against its
own type. This one needs no extension: five fixed bytes at offset zero,
one of them a version digit the spec fixes, is a stronger signature than
the three-byte "ID3" the stdlib uses for audio/mpeg. So it is a
refinement in SniffMIME rather than extension trust in ValidateUpload,
and the tests assert it under three filenames including .bin, which is
what says so.

text/plain therefore joins the refinement switch, and the comment there
says what makes that case narrower than the octet-stream one: it admits
exactly one signature, and the bar for a second is the argument at
validRTFStream rather than the list's existence.

One behaviour change beyond the refusals: an EXTENSIONLESS RTF used to
store as text/plain and serve inline; it now stores as application/rtf
and serves as an attachment. More conservative, and named here because
it is the one leg that changes a file that was previously accepted.

CONVE-23 sweep, three sites, all of them prose this change made false:
SniffMIME's "two refinements", and the F6 comment claiming
text/javascript "is not reachable EITHER" — F5 made it reachable, which
is the whole point of including .js.

The CFB fixture is hand-built and the test says why that is sound here
and not elsewhere: the branch reads eight fixed bytes, so eight bytes
exercise all of it, unlike the ISO-BMFF and EBML fixtures where the code
walks structure an encoder produces.

go test ./internal/attachments/ green.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn

* fix(attachments): gate the CFB branch on the stdlib verdict, not the refined one (TASK-2976, BUG-2963)

A surviving mutant found this. Dropping the octet-stream gate changed
no test, which said the gate was either dead or untested; it is
untested, and it was also gating on the wrong value.

A CFB file's 512-byte header is sector data, so one can carry "ustar" at
offset 257 and be refined to application/x-tar by this package's own
magic table. Gated on the REFINED verdict, that file is refused under
its own .doc name — the false refusal BUG-2963 exists to remove. Gated
on the stdlib's, the question is whether anything IDENTIFIED the bytes,
and nothing did.

This is the reasoning the .aac branch already carries, in the same
function, for the same collision from the other side. The test is a CFB
header with ustar spliced into its sector data, and it asserts the
premise (these bytes really do refine to x-tar) before asserting the
outcome, so it cannot pass by the collision quietly failing to happen.

Claude-Session: https://claude.ai/code/session_01GqaEDuCtRiSJfa7eppWecn
2026-09-09 19:23:18 -04:00

240 lines
12 KiB
YAML

name: Nix
on:
push:
branches: [main]
pull_request:
branches: [main]
concurrency:
# PR runs share a group per ref and supersede each other — a newer push to the
# same PR makes the older run's answer worthless.
#
# PUSH runs get a group of their OWN, one per commit, and this is deliberate
# (TASK-2954). A run on `main` may owe main a vendorHash heal, and an evicted
# run never performs it. `cancel-in-progress: false` is not enough on its own —
# GitHub holds only ONE pending run per group, so a third push evicts the
# queued second, and the eviction looks exactly like a run that decided there
# was nothing to do. A per-commit group cannot be evicted by anything.
#
# What an evicted heal costs is now DELAY, not the fix. Since BUG-2974 the
# heal is gated on the committed hash rather than on what the push touched, so
# the next push to main meets the same stale hash and heals it from its own
# run. The per-commit group is what makes the heal land on the merge that
# caused it instead of waiting for whatever lands next.
#
# What that trades for: two main runs can reach the push step at once, and the
# loser gets a non-fast-forward rejection and goes red. That is the right
# direction — a red job is read, and the next push to main heals anyway.
group: nix-${{ github.event_name == 'push' && github.sha || github.ref }}
cancel-in-progress: ${{ github.event_name == 'pull_request' }}
permissions:
contents: read
# All third-party Actions are pinned to a 40-char commit SHA with a trailing
# '# vX.Y.Z' comment so a compromised maintainer or moved tag cannot silently
# execute attacker code in CI. Bump the SHA + comment together when updating.
jobs:
nix:
name: Nix build & check
runs-on: ubuntu-latest
# NO write permission here, deliberately — see `push-vendor-hash` below.
# This job builds; it never pushes.
outputs:
vendor_hash_bumped: ${{ steps.vendorhash.outputs.bumped }}
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: DeterminateSystems/nix-installer-action@ef8a148080ab6020fd15196c2084a2eea5ff2d25 # v22
# THE RED-BASELINE FIX (TASK-2954). `vendorHash` pins the Go module set by
# content hash; Dependabot updates go.mod/go.sum and has no idea this file
# exists, so EVERY Go bump PR failed the steps below on a hash mismatch,
# structurally and forever. A check that is always red is not a check —
# a bump that genuinely breaks the build looked identical, at a glance, to
# one that only moved the hash.
#
# This recomputes the hash in the WORKING TREE, so the steps below judge
# the build with this ref's actual module set. Nothing is pushed from here
# and no token is used: a Dependabot PR run gets a read-only GITHUB_TOKEN
# (it runs as if from a fork), and the `permissions` key does NOT lift that
# — only a repository-wide setting does, which would hand fork PRs write
# tokens too. The push therefore happens on `push: main` instead, in the
# job below, where the run is not Dependabot's.
#
# It runs on every run, not just Dependabot's: a human's own go.mod change
# moves the hash the same way, and a check that is honest only for one
# author is the shape this item exists to remove.
#
# It runs BEFORE `nix flake check`, which builds the package too and would
# hit the same mismatch first. It is deliberately not a gate: if the build
# fails for any reason other than a go-modules hash mismatch, the script
# exits non-zero, this step still succeeds, and the real steps below report
# the real failure. The gate stays exactly where it was.
- name: Recompute vendorHash before judging the build
id: vendorhash
run: |
set -o pipefail
if nix build .#default --print-build-logs 2>&1 | tee /tmp/nix-build.log; then
echo "Build is green; vendorHash is current."
exit 0
fi
if ! new_hash="$(nix/bump-vendor-hash.sh /tmp/nix-build.log)"; then
echo "Not a go-modules hash mismatch — leaving it to the build steps below."
exit 0
fi
echo "vendorHash -> $new_hash"
echo "bumped=true" >> "$GITHUB_OUTPUT"
- name: Carry the recomputed package.nix to the push job
if: steps.vendorhash.outputs.bumped == 'true'
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
with:
name: recomputed-package-nix
path: nix/package.nix
retention-days: 1
- name: nix flake check
run: nix flake check --print-build-logs
- name: nix build
run: nix build .#default --print-build-logs
- name: Smoke test binary
run: |
./result/bin/pad --version
./result/bin/pad --help
# The Go toolchain here exists only to install govulncheck for the
# scan step below — the pad binary under test was built by Nix above.
- uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
with:
go-version: "1.26"
- name: Scan Nix-built binary with govulncheck
# CI's main govulncheck job scans a `go build` binary, which honours
# go.mod's `toolchain go1.26.6` line — but nixpkgs pins
# GOTOOLCHAIN=local, so the Nix artifact is built with whatever Go
# patch the nixos-26.05 channel ships and can carry stdlib advisories
# the main gate can no longer see (BUG-2567). This scans the artifact
# that actually ships via Nix, against nix/accepted-advisories.txt:
# known-accepted advisories stay green and visible, any NEW advisory
# fails the job, and a warning fires when an accepted advisory clears
# (the signal to prune the list and eventually close BUG-2567).
#
# govulncheck pinned to the same release as ci.yml — bump the two
# together. GOTOOLCHAIN=auto on the install mirrors ci.yml's
# rationale: setup-go exports GOTOOLCHAIN=local, and if govulncheck's
# own go.mod ever requires a patch newer than setup-go's resolved
# one, `auto` lets Go fetch it instead of failing the install.
run: |
GOTOOLCHAIN=auto go install golang.org/x/vuln/cmd/govulncheck@v1.2.0
GOVULNCHECK="$(go env GOPATH)/bin/govulncheck" nix/vulnscan.sh result/bin/pad
# THE ONLY JOB WITH A WRITE TOKEN, and it exists as a separate job precisely so
# that the token is never minted in a context an untrusted change could reach.
#
# An `if:` on a STEP is not a security boundary — the job would still hold a
# write-capable GITHUB_TOKEN, and `actions/checkout` persists it in the
# workspace for every later command, so any change to this file inside a PR
# could use it. An `if:` on a JOB is different in kind: the job does not start,
# and no write token is issued for that run at all.
#
# WHY ON `push: main` AND NOT ON THE PR (ruled on TASK-2954's trail). A
# Dependabot PR run gets a read-only token whatever `permissions` says. The
# merge, though, is authored by a human, so the `push` run that follows is an
# ordinary run with an ordinary token. The PR's own check was already honest,
# because the job above recomputes in the working tree; `main` is what needs
# the committed fix, and it gets it from the run of the merge itself — or,
# when that run loses the push race, from the run of the next push to main.
# The two rejected alternatives were a PAT in the Dependabot secret store (a
# standing credential for a once-a-week fix) and enabling write tokens for
# pull-request workflows (which would give them to FORK PRs on a public repo —
# the surface CONVE-2438 exists to keep closed).
#
# THE LOOP TERMINATES BECAUSE THIS PUSH TRIGGERS NOTHING. A push made with
# the default `GITHUB_TOKEN` creates no workflow runs, so the commit this job
# pushes gets no run at all. Observed on the first heal this fix produced
# (BUG-2974, day 62): `gh run list --branch main` spans cbfc073e ->
# 31d11e76 -> 0b16be49 and skips the heal commit 249a8f87 entirely. Read the
# BRANCH listing, not `gh run list --commit` — that query also returned
# nothing for 0b16be49, which demonstrably has runs, so it cannot tell
# "no runs" from "no answer". The loop is closed by construction rather than
# by being green.
#
# THE CORRECTED-TREE TERMINATOR IS THE BACKSTOP, AND IT HAS NEVER RUN. If
# this push did trigger a run, the build job above would find the hash
# correct, print "Build is green; vendorHash is current.", never set
# `bumped`, and this job would not start — the FIX's own effect on the build,
# holding regardless of what any push touched. That is an argument, not a
# measurement: nothing has ever exercised it. It becomes load-bearing the
# moment this push uses a PAT or a GitHub App token, which is the obvious
# edit for anyone who wants CI to validate the healed tree. Making that
# change inherits an untested terminator; exercise it before relying on it.
#
# A CONSEQUENCE OF THE SAME MECHANISM, not a defect: the healed tip carries
# no CI of its own. Its tree is the merge commit plus the one hash line that
# merge's own Nix run computed and built against, and the next push to main
# builds on it normally.
#
# THE RANGE-VS-STATE DISTINCTION IS LOAD-BEARING, AND IT WAS GOT WRONG ONCE
# (BUG-2974). This
# comment used to name the loop guard as a gate asking whether the push had
# touched `go.mod`/`go.sum`, and that gate existed as a step below. It is the
# right question for loop prevention and the WRONG one for recovery: after a
# lost push race the tree that needs healing was written by an EARLIER push,
# so every later merge that did not itself move the module set refused to fix
# it, exited GREEN, and left main carrying a hash a clean nix build rejects.
# The gate is gone. The state comparison below is the only gate, and being a
# question about state it is equally true for a hash this push broke and one
# an earlier push broke and could not land.
#
# If another merge lands while this job runs, the push is REJECTED as a
# non-fast-forward and this job goes RED rather than overwriting anything —
# and the heal is genuinely not lost now, because the next push to main meets
# the same stale hash, recomputes it, and heals it from its own run.
#
# `needs: nix` without `if: always()` is the second half: this runs only when
# the build job SUCCEEDED, so a module set whose corrected tree still fails
# `nix flake check`, the smoke test or the vulnerability scan is never
# committed to main.
push-vendor-hash:
name: Heal main's vendorHash
needs: nix
if: >-
github.event_name == 'push' &&
github.ref == 'refs/heads/main' &&
needs.nix.outputs.vendor_hash_bumped == 'true'
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
# Nothing here reads history any more — BUG-2974 removed the gate that
# did. The depth stays full rather than dropping to the default shallow
# clone: this is the clone the heal commit is pushed from, full depth is
# what every run of this job has ever used, and a shallow push is not a
# variable worth introducing on a job that runs only when a Go bump
# actually moves the module set.
fetch-depth: 0
- uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
with:
name: recomputed-package-nix
path: nix/
# THE GATE, and the only one, lives in the script: does what main COMMITTED
# differ from what the build job RECOMPUTED? The artifact exists only
# because the build job found a mismatch, but comparing there keeps the job
# honest about the tree it is actually looking at rather than about what
# another job reported. It is a file rather than a `run:` block so that
# nix/heal-vendor-hash_test.sh can drive it against a real git remote —
# BUG-2974 shipped green for a day because nothing could exercise it.
- name: Commit and push the recomputed hash
run: nix/heal-vendor-hash.sh
env:
SHA: ${{ github.sha }}