diff --git a/internal/attachments/mime.go b/internal/attachments/mime.go
index cc8d5294..d943eff2 100644
--- a/internal/attachments/mime.go
+++ b/internal/attachments/mime.go
@@ -109,8 +109,8 @@ var allowed = func() map[string]MIMEEntry {
// --- Text & data (chip with download) ---
for _, t := range []string{
"text/plain", "text/markdown", "text/csv", "text/tab-separated-values",
- "application/json", "application/xml", "text/xml",
- "application/yaml", "text/yaml", "application/toml",
+ "application/json", "text/xml",
+ "application/yaml", "application/toml",
} {
add(t, RenderChip, CategoryText)
}
@@ -124,8 +124,15 @@ var allowed = func() map[string]MIMEEntry {
}
// --- Forced-download text payloads — would XSS if served inline ---
+ // application/javascript was removed here (BUG-2963 F6): no extension in
+ // extMIMEMap reaches that spelling and SniffMIME cannot emit it, so the
+ // entry could never be the type an upload was stored under. text/javascript
+ // stays because .js maps to it — but note it is not reachable EITHER: a .js
+ // upload sniffs text/plain and is stored as that. The difference is that
+ // text/javascript has a route to become reachable (the F5 extension-trust
+ // work) and application/javascript has none, since nothing names it.
for _, t := range []string{
- "text/html", "text/javascript", "application/javascript",
+ "text/html", "text/javascript",
} {
add(t, RenderForceDownload, CategoryText)
}
@@ -156,8 +163,10 @@ func LookupMIME(mime string) (MIMEEntry, bool) {
// /), plus PDF (sandboxed viewer) and plain text. This is
// the server mirror of the client's VIEWER_MIMES + BROWSER_PREVIEW_MIMES
// (web/src/lib/attachments/display.ts). Notably absent: image/svg+xml and
-// application/xhtml+xml (active), text/xml + application/xml (SVG/XHTML wear
-// these after an extensionless sniff), and the whole RenderForceDownload bucket.
+// application/xhtml+xml (active), text/xml (SVG and XHTML wear it after an
+// extensionless sniff), and the whole RenderForceDownload bucket. The
+// application/xml spelling used to be named here alongside text/xml; it left
+// the allowlist entirely in BUG-2963 F6, so there is no entry left to exclude.
var inlineSafe = map[string]struct{}{
// Raster images.
"image/png": {}, "image/jpeg": {}, "image/gif": {}, "image/webp": {},
@@ -206,6 +215,12 @@ func NormalizeMIME(mime string) string {
var sniffAliases = map[string]string{
"audio/wave": "audio/wav", // .wav
"application/x-gzip": "application/gzip", // .gz / .tar.gz
+ "video/avi": "video/x-msvideo", // .avi — pure spelling difference
+ // application/ogg is deliberately NOT here, and there is no per-codec
+ // refinement either — both were written and both were removed. An alias
+ // table is for two names of ONE thing, and Ogg is one name for several:
+ // see mime_magic.go for why a container name cannot be resolved to an
+ // audio type from the head of the file.
}
// SniffMIME detects the MIME type from the leading bytes of a payload
@@ -229,7 +244,28 @@ func SniffMIME(head []byte) string {
}
got := NormalizeMIME(http.DetectContentType(head))
if alias, ok := sniffAliases[got]; ok {
- return alias
+ got = alias
+ }
+ // Two BUG-2963 refinements. (Ogg was a third and was removed; see
+ // mime_magic.go for why a container name cannot be aliased to an audio
+ // type.) Each is keyed on what the stdlib already said, so none can retype
+ // a file the standard library identified. Neither VALIDATES the format —
+ // see mime_magic.go's header for the three review rounds that settled why
+ // recognition here is by magic:
+ //
+ // - video/webm is refined, because the mimesniff table answers it from
+ // the bare EBML magic and cannot tell Matroska from WebM;
+ // - application/octet-stream is the stdlib having NO opinion, which is
+ // the only case where recognising more formats adds anything.
+ switch got {
+ case "video/webm":
+ if mime := sniffEBMLDocType(head); mime != "" {
+ return mime
+ }
+ case "application/octet-stream":
+ if mime := sniffOpaqueMagic(head); mime != "" {
+ return mime
+ }
}
return got
}
@@ -252,7 +288,58 @@ func SniffMIME(head []byte) string {
// extOverride lets callers (the multipart handler) pass the original
// filename so we can compare extensions; pass empty string to skip.
func ValidateUpload(head []byte, filename string) (entry MIMEEntry, code string, err error) {
+ stdlib := NormalizeMIME(http.DetectContentType(head))
+ if alias, ok := sniffAliases[stdlib]; ok {
+ stdlib = alias
+ }
sniffed := SniffMIME(head)
+ ext := strings.ToLower(filepath.Ext(filename))
+
+ // More than one magic can match the same bytes — a tar whose first member
+ // is named "fLaC.txt", a FLAC whose COMMENT tag contains "ustar". When the
+ // extension names one of the matching candidates, it breaks the tie. It
+ // cannot introduce a type: every candidate is one the bytes matched, and a
+ // name for a type whose magic is absent never appears in the list.
+ if stdlib == "application/octet-stream" {
+ if alt := preferCandidateForExt(sniffOpaqueCandidates(head), ext); alt != "" {
+ sniffed = alt
+ }
+ }
+
+ // Raw AAC is the one BUG-2963 format whose structure is too small to act
+ // on from the bytes alone, so it is resolved here — where the filename is
+ // known — rather than inside SniffMIME, which deliberately never sees a
+ // filename. Both halves are required, and neither is sufficient: bytes
+ // without the extension name nothing, and the extension without a valid
+ // ADTS header names nothing either. The extension does not supply
+ // evidence; it decides whether a weak structure may speak.
+ //
+ // What it does NOT do is decide the outcome for a .aac generally. A .aac
+ // whose bytes are some other allowlisted audio type is still stored as
+ // that type by the ordinary rules — the categories agree, so nothing here
+ // refuses it. This branch adds one reading; it removes none.
+ // The gate is on THE STANDARD LIBRARY'S verdict, not on the refined one,
+ // and the difference is not academic: a real AAC frame whose ancillary
+ // payload contains "ustar" at offset 257 is refined to application/x-tar
+ // by this package, and gating on the refined value refused it under its
+ // own .aac name. The stdlib said octet-stream about that file — nothing
+ // identified it — which is the condition under which a weak signature may
+ // speak.
+ //
+ // Both no-opinion verdicts count. text/plain is the second: an AAC frame
+ // whose ancillary payload is printable makes the leading bytes look
+ // textual, and leaving that verdict out refused real files too.
+ //
+ // A file the stdlib DOES identify is untouched. That the gate can fire at
+ // all is a property worth keeping rather than a formality: no signature in
+ // the mimesniff table begins with 0xFF today, so nothing the stdlib names
+ // can pass validADTSHeader — but if one ever does, this gate is what stops
+ // a fourteen-bit match from overriding it.
+ if (stdlib == "application/octet-stream" || stdlib == "text/plain") &&
+ validADTSHeader(head) && ext == ".aac" {
+ sniffed = "audio/aac"
+ }
+
e, ok := LookupMIME(sniffed)
if !ok {
return MIMEEntry{}, "mime_not_allowed", &uploadError{msg: "MIME type not allowed: " + sniffed}
@@ -327,8 +414,10 @@ var canonicalExtForMIME = buildCanonicalExtForMIME()
//
// Every key MUST be a value that extMIMEMap actually uses, or the entry is a
// line that cannot fire. One of them was exactly that on first writing —
-// "text/yaml", where this map says application/yaml — so the preference never
-// applied and .yaml won on length. The test asserts the property rather than
+// "text/yaml", where the forward map says application/yaml — so the preference
+// never applied and .yaml won on length. (text/yaml is no longer even on the
+// allowlist; BUG-2963 F6 removed it as unreachable. The lesson it taught this
+// map is why the note survives it.) The test asserts the property rather than
// trusting the next reader to notice.
func preferredExtensions() map[string]string {
return map[string]string{
@@ -345,11 +434,19 @@ func preferredExtensions() map[string]string {
// aliasExtensions covers allowed MIME spellings that NO extension in
// extMIMEMap maps to, so reversing the forward table alone leaves them
// without an extension: the forward table picks one spelling per extension
-// (.xml says application/xml, .js says text/javascript, .webm says
-// video/webm), while the allowlist accepts the alias spellings too. An
-// attachment stored under an alias type with an unstorable filename came out
-// of `pad attachment view` extensionless — the exact failure the delegation
-// to this package was built to end (codex closing round).
+// (.webm says video/webm), while the allowlist accepts the alias spellings
+// too. An attachment stored under an alias type with an unstorable filename
+// came out of `pad attachment view` extensionless — the exact failure the
+// delegation to this package was built to end (codex closing round).
+//
+// This table held four entries until BUG-2963 F6. Three stopped being
+// alias-shaped for two different reasons, and the distinction is the thing to
+// keep: text/yaml and application/javascript were REMOVED from the allowlist
+// as unreachable spellings, so an alias for them would name a refused type;
+// text/xml is still very much allowed, but the forward map now spells .xml
+// with it, so the reverse mapping is derived and an alias entry here would be
+// a duplicate the hygiene assertion below rejects. An entry leaving this
+// table therefore says nothing on its own about whether the type survived.
//
// Every key MUST be an allowed type with no forward-derived reverse mapping,
// and every value MUST be an extension the forward map sends to an ALLOWED
@@ -359,10 +456,7 @@ func preferredExtensions() map[string]string {
// every allowed MIME type has a reverse extension.
func aliasExtensions() map[string]string {
return map[string]string{
- "text/xml": ".xml", // forward map spells it application/xml
- "text/yaml": ".yml", // forward map spells it application/yaml; .yml matches its preference
- "application/javascript": ".js", // forward map spells it text/javascript
- "audio/webm": ".webm", // forward map spells it video/webm; the container is the same
+ "audio/webm": ".webm", // forward map spells it video/webm; the container is the same
}
}
@@ -486,7 +580,12 @@ var extMIMEMap = map[string]string{
".csv": "text/csv",
".tsv": "text/tab-separated-values",
".json": "application/json",
- ".xml": "application/xml",
+ // text/xml, not application/xml: the latter left the allowlist in
+ // BUG-2963 F6, and this map's values are looked up in `allowed` — an
+ // extension pointing at a removed spelling would make every .xml upload
+ // fail extension_blocked, which is the same map's mechanism for refusing
+ // .svg and .exe.
+ ".xml": "text/xml",
".yaml": "application/yaml",
".yml": "application/yaml",
".toml": "application/toml",
diff --git a/internal/attachments/mime_fuzz_test.go b/internal/attachments/mime_fuzz_test.go
new file mode 100644
index 00000000..bb3f6111
--- /dev/null
+++ b/internal/attachments/mime_fuzz_test.go
@@ -0,0 +1,57 @@
+package attachments
+
+import "testing"
+
+// FuzzSniffMIME exercises the whole sniff path — the stdlib call, both
+// refinements, and the magic recognisers — against arbitrary bytes.
+//
+// It exists because BUG-2963 added a PARSER. Every other check in this package
+// reads fixed offsets and is bounded by construction; sniffEBMLDocType walks
+// caller-supplied length fields, which is the one shape here that can index
+// out of range or fail to advance. The properties asserted are the two a
+// sniffer owes its caller: it returns, and it does not panic. What it returns
+// for nonsense is not asserted — that is the table tests' job.
+//
+// Seeds are the real fixtures plus the shapes a walk is most likely to break
+// on: truncation at every element boundary, declared sizes larger than the
+// data, and the reserved all-ones VINT that means "unknown size".
+func FuzzSniffMIME(f *testing.F) {
+ for _, name := range []string{
+ "matroska.head512", "webm.head512", "webm-void-says-matroska.head512",
+ "matroska-void-padded.head512", "tar.head512", "tar-bmp-firstmember.head512",
+ "sevenzip.head512", "flac.head512", "bzip2.head512", "aac-adts.head512",
+ "ogg-opus.head512", "ogg-vp8-video.head512", "avi.head512",
+ "elf-with-ustar-magic.head512",
+ } {
+ b, err := fixtureBytes(name)
+ if err != nil {
+ f.Fatalf("seed %s: %v", name, err)
+ }
+ f.Add(b)
+ // Truncations: the walk must survive running out mid-element.
+ for _, n := range []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 12, 24, 40, 64, 100} {
+ if n < len(b) {
+ f.Add(b[:n])
+ }
+ }
+ }
+ // EBML magic followed by hostile length fields.
+ f.Add([]byte{0x1A, 0x45, 0xDF, 0xA3, 0xFF}) // unknown-size header
+ f.Add([]byte{0x1A, 0x45, 0xDF, 0xA3, 0xFE, 0x42, 0x82, 0xFF}) // unknown-size child
+ f.Add([]byte{0x1A, 0x45, 0xDF, 0xA3, 0xA3, 0x42, 0x82, 0x88, 'm', 'a', 't', 'r'}) // child longer than the data
+ f.Add([]byte{0x1A, 0x45, 0xDF, 0xA3, 0xA3, 0x00, 0x00}) // invalid all-zero VINT marker
+ f.Add([]byte{0x1A, 0x45, 0xDF, 0xA3, 0xA3, 0x80, 0x80}) // zero-length child, must still advance
+ f.Add([]byte("OggS\x00\x02")) // truncated Ogg page header
+
+ f.Fuzz(func(t *testing.T, head []byte) {
+ // Each is called directly as well as through SniffMIME, so a panic is
+ // attributed to the check that owns it rather than to the dispatcher.
+ sniffOpaqueMagic(head)
+ sniffEBMLDocType(head)
+ validADTSHeader(head)
+ SniffMIME(head)
+ ValidateUpload(head, "fuzz.aac")
+ ValidateUpload(head, "fuzz.mkv")
+ ValidateUpload(head, "fuzz.tar")
+ })
+}
diff --git a/internal/attachments/mime_magic.go b/internal/attachments/mime_magic.go
new file mode 100644
index 00000000..8b1c2db1
--- /dev/null
+++ b/internal/attachments/mime_magic.go
@@ -0,0 +1,372 @@
+package attachments
+
+import (
+ "bytes"
+ "strings"
+)
+
+// This file closes the second half of the gap BUG-2961 opened: the upload
+// allowlist names types that SniffMIME cannot produce, so the door refuses —
+// or silently retypes — files the allowlist says are supported. A measurement
+// over real files (BUG-2963) found 25 of 48 allowlist entries unreachable as a
+// stored type.
+//
+// WHAT THESE CHECKS DO. They RECOGNISE a format from its defining magic, to
+// the same standard the standard library's own detector uses. They do not
+// prove the bytes are that format, and three review rounds established that
+// nothing available here could:
+//
+// - Round 1 defeated magic-only matching with a real, executing ELF binary
+// carrying "ustar" at offset 257, stored as application/x-tar.
+// - Round 2 defeated the checksum that was round 1's answer, with an ELF
+// carrying a CORRECT one — and archive/tar's own Reader.Next accepts that
+// file too. A 512-byte tar header is exactly those fields, so the two are
+// not distinguishable at this size by anything in the standard library.
+// - Round 3 found the accumulated validation refusing REAL files: PAX and
+// long-name GNU tars, legal randomized bzip2 blocks, FLAC declaring the
+// zero sample rate RFC 9639 permits. That is this bug's own defect —
+// refusing files people legitimately have — reintroduced by the fix.
+//
+// So the structural validators are gone and recognition is by magic. The
+// settling fact is that this is the door's EXISTING standard, not a relaxation
+// of it: http.DetectContentType recognises audio/mpeg from the three bytes
+// "ID3" (net/http/sniff.go), audio/mpeg is on the allowlist, and this door
+// already serves it inline. Every signature here is at least as wide.
+//
+// THE SAFETY PROPERTY, written from what the code does — two earlier versions
+// of this paragraph were checked and found factually wrong:
+//
+// - Nothing here is executed, and nothing is decompressed. An earlier
+// version decoded bzip2 while sniffing; that is gone.
+// - Serving is per allowlist entry, not uniform. Archives are RenderChip and
+// absent from inlineSafe, so they download. audio/flac and audio/aac are
+// inlineSafe like every other allowlisted audio type and play through an
+// element, which renders without executing embedded script.
+// - nosniff is set, so a browser will not re-interpret the bytes as
+// something more dangerous than the type stored, and that type is on a
+// reviewed allowlist. Recognising a format wrongly moves a file between
+// reviewed types; it cannot move it outside them.
+//
+// No filename is trusted to introduce a type here. The one place an extension
+// participates is documented at validADTSHeader, and it only DISAMBIGUATES a
+// structure the bytes must already carry.
+
+// sniffOpaqueMagic recognises formats the WHATWG mimesniff table has no
+// signature for, so http.DetectContentType answers application/octet-stream
+// for a real file of them. It returns the DEFAULT candidate; see
+// sniffOpaqueCandidates for why there can be more than one.
+//
+// Called ONLY when the stdlib returned application/octet-stream, so it cannot
+// override a type the standard library identified.
+func sniffOpaqueMagic(head []byte) string {
+ if c := sniffOpaqueCandidates(head); len(c) > 0 {
+ return c[0]
+ }
+ return ""
+}
+
+// sniffOpaqueCandidates returns EVERY type whose magic matches, because more
+// than one can, and which is right is not always decidable from the bytes.
+//
+// The collision is real in both directions and neither side is exotic:
+//
+// - A tar header's first 100 bytes are its member's FILENAME, so an ordinary
+// archive whose first member is called "fLaC.txt" carries the FLAC marker
+// at offset zero.
+// - A FLAC file's Vorbis COMMENT tags are arbitrary UTF-8 (RFC 9639 §8.6),
+// and an AAC frame's ancillary payload is arbitrary bytes, so either can
+// contain "ustar" at offset 257.
+//
+// An earlier version picked a winner by ordering and argued the collision was
+// asymmetric — that real audio could not plausibly carry "ustar" at a fixed
+// offset. Review refuted that with complete, decodable FLAC and AAC files
+// carrying it in ordinary metadata. Any total order refuses somebody: tar
+// first refuses those, prefixes first refuse the archive.
+//
+// So the order here is only a DEFAULT. ValidateUpload, which knows the
+// filename, may pick a different candidate from this list — see
+// preferCandidateForExt, and note what that is and is not: the extension
+// cannot introduce a type, only choose among readings the bytes themselves
+// support.
+func sniffOpaqueCandidates(head []byte) []string {
+ var out []string
+ // tar leads the default order because its magic sits at a fixed offset
+ // rather than at a prefix, so it is the one least likely to be an
+ // accident of some other format's leading bytes.
+ if validTarHeader(head) {
+ out = append(out, "application/x-tar")
+ }
+ if validSevenZipHeader(head) {
+ out = append(out, "application/x-7z-compressed")
+ }
+ if validFLACStream(head) {
+ out = append(out, "audio/flac")
+ }
+ if validBzip2Stream(head) {
+ out = append(out, "application/x-bzip2")
+ }
+ return out
+}
+
+// preferCandidateForExt returns the candidate that the filename's extension
+// names, or "" when the extension names none of them.
+//
+// This is the ONLY place a filename influences which type is chosen, and the
+// influence is deliberately weak: every candidate is a type the BYTES already
+// matched, so the extension breaks a tie rather than casting a vote. A name
+// that matches nothing in the list changes nothing, and a name for a type
+// whose magic is absent can never appear in the list at all.
+func preferCandidateForExt(candidates []string, ext string) string {
+ if len(candidates) < 2 || ext == "" {
+ return ""
+ }
+ want, ok := extMIMEMap[strings.ToLower(ext)]
+ if !ok {
+ return ""
+ }
+ want = NormalizeMIME(want)
+ for _, c := range candidates {
+ if c == want {
+ return c
+ }
+ }
+ return ""
+}
+
+// validSevenZipHeader recognises a 7z archive by its six-byte signature.
+//
+// There was a start-header CRC check here. It is gone with the rest of the
+// structural validation (see this file's header): it could not narrow what the
+// door accepts, and its neighbours had begun refusing real archives.
+func validSevenZipHeader(head []byte) bool {
+ return bytes.HasPrefix(head, []byte{0x37, 0x7A, 0xBC, 0xAF, 0x27, 0x1C})
+}
+
+// validFLACStream recognises a native FLAC stream by its four-byte "fLaC"
+// marker (RFC 9639 §8.1).
+//
+// Four bytes is a wider signature than the stdlib uses for the audio type that
+// is already on this allowlist and already served inline: audio/mpeg is
+// recognised from the three bytes "ID3". Recognising FLAC the same way is this
+// door's existing standard, not a departure from it.
+func validFLACStream(head []byte) bool {
+ return bytes.HasPrefix(head, []byte("fLaC"))
+}
+
+// validBzip2Stream recognises a bzip2 stream by "BZh" and its block-size digit.
+//
+// It does NOT decompress. An earlier version decoded through the standard
+// library to reach the stream CRC, which refused legal randomized blocks Go's
+// decoder does not implement — a real .bz2 rejected for a decoder's missing
+// feature. Nothing in this door expands an upload now.
+func validBzip2Stream(head []byte) bool {
+ return len(head) >= 4 && bytes.HasPrefix(head, []byte("BZh")) &&
+ head[3] >= '1' && head[3] <= '9'
+}
+
+// validTarHeader recognises a POSIX ustar archive by the magic at offset 257.
+//
+// TWO KINDS OF FILE ARE DELIBERATELY NOT RECOGNISED, and both are refusals
+// rather than oversights:
+//
+// - V7 tar, which predates the magic and has no signature anywhere. There is
+// nothing at a fixed offset to key on, so it cannot be recognised by this
+// kind of check at all; it stays refused, as it was before BUG-2963.
+// - Nothing else. PAX and long-name GNU archives lead with a metadata block
+// rather than a file header, and they ARE recognised, because the magic is
+// in that block too. A full PARSE would need more blocks than this door
+// ever reads, which is one of the reasons the parse is gone.
+//
+// The checksum that used to be verified here is gone. It never distinguished a
+// crafted header from a real one — archive/tar accepts an ELF carrying a
+// correct one, which ships as a fixture — and verifying it required a full
+// 512-byte block, which is why PAX and long-name GNU archives were refused.
+func validTarHeader(head []byte) bool {
+ const magicEnd = 262
+ return len(head) >= magicEnd && bytes.Equal(head[257:magicEnd], []byte("ustar"))
+}
+
+// sniffEBMLDocType returns the MIME for an EBML file by READING ITS DOCTYPE
+// ELEMENT, not by searching for a string.
+//
+// The mimesniff table maps the bare EBML magic (1A 45 DF A3) to video/webm
+// with no DocType check, so a Matroska file — on the allowlist under its own
+// name — is accepted and stored as WebM. That is not a refusal, which is why
+// it survived a pass looking for 415s: the upload succeeds and the stored type
+// is simply false.
+//
+// This walks the EBML header's child elements. The first version searched the
+// leading 64 bytes for the two strings instead, and an adversarial round broke
+// it in BOTH directions with files that ffprobe accepts: a WebM carrying
+// "matroska" inside a Void element was stored as Matroska, and a Matroska with
+// forty bytes of legal Void padding — which pushes DocType past 64 — was
+// stored as WebM, leaving the very defect this fixes live for any file a muxer
+// chose to pad. Void is legal anywhere and its contents are meaningless, so
+// only a parse can tell payload from padding.
+//
+// Called only when the stdlib returned video/webm, so it can move a file
+// between the two EBML types and nowhere else. An unreadable or absent DocType
+// returns "" and the stdlib verdict stands, which is the behaviour that
+// shipped before this existed.
+func sniffEBMLDocType(head []byte) string {
+ const ebmlHeaderID = 0x1A45DFA3
+ const docTypeID = 0x4282
+
+ id, rest, ok := readEBMLID(head)
+ if !ok || id != ebmlHeaderID {
+ return ""
+ }
+ size, rest, ok := readEBMLSize(rest)
+ if !ok {
+ return ""
+ }
+ // The header may be longer than the bytes we were given; walking what we
+ // have is correct, and running out simply means no DocType was found.
+ if size < uint64(len(rest)) {
+ rest = rest[:size]
+ }
+
+ for len(rest) > 0 {
+ childID, after, ok := readEBMLID(rest)
+ if !ok {
+ return ""
+ }
+ childSize, after, ok := readEBMLSize(after)
+ if !ok {
+ return ""
+ }
+ if childSize > uint64(len(after)) {
+ // The declared payload runs past what we hold. For DocType that
+ // is still answerable when the VALUE is complete in hand: it ends
+ // at its first NUL, and a header padding DocType out to 600 bytes
+ // puts "matroska\x00" in the first dozen. Anything else, we stop.
+ if childID == docTypeID {
+ if mime := docTypeMIME(after); mime != "" {
+ return mime
+ }
+ }
+ return ""
+ }
+ if childID == docTypeID {
+ // DocType is an ASCII string whose value ENDS at the first NUL;
+ // anything after that is padding and is not part of the value
+ // (RFC 8794 §13). TrimRight was wrong here — a round-2 finding
+ // produced an ffprobe-readable Matroska whose DocType payload was
+ // "matroska\x00junk", which trimming left intact and so failed to
+ // match, storing a real Matroska as WebM.
+ return docTypeMIME(after[:childSize])
+ }
+ rest = after[childSize:]
+ }
+ return ""
+}
+
+// readEBMLID reads an EBML element ID. IDs keep their length marker as part of
+// the value (RFC 8794 §5), so DocType is the four-hex-digit 0x4282 that the
+// specification names, and the marker byte tells us how many bytes to take.
+func readEBMLID(b []byte) (id uint32, rest []byte, ok bool) {
+ if len(b) == 0 {
+ return 0, nil, false
+ }
+ n := ebmlLength(b[0])
+ if n == 0 || n > 4 || len(b) < n {
+ return 0, nil, false
+ }
+ for i := 0; i < n; i++ {
+ id = id<<8 | uint32(b[i])
+ }
+ // An ID whose value bits are all ones is RESERVED and not a valid element
+ // ID (RFC 8794 §5). Accepting one let malformed bytes act as a
+ // zero-length child and carry the walk onward to a DocType that follows
+ // them, which a round-2 finding used to have 1a45dfa3 8d ff80 ... parse
+ // as Matroska.
+ if id == allOnesEBMLID[n] {
+ return 0, nil, false
+ }
+ return id, b[n:], true
+}
+
+// allOnesEBMLID is the reserved value at each ID width: the marker bit plus
+// every value bit set.
+var allOnesEBMLID = [5]uint32{0, 0xFF, 0x7FFF, 0x3FFFFF, 0x1FFFFFFF}
+
+// readEBMLSize reads an EBML data size, whose length marker is REMOVED from
+// the value (unlike an ID). An all-ones value means "unknown size", which this
+// treats as unreadable — a header of unknown length is not something to walk.
+func readEBMLSize(b []byte) (size uint64, rest []byte, ok bool) {
+ if len(b) == 0 {
+ return 0, nil, false
+ }
+ n := ebmlLength(b[0])
+ if n == 0 || n > 8 || len(b) < n {
+ return 0, nil, false
+ }
+ size = uint64(b[0]) & (1<<(8-uint(n)) - 1)
+ allOnes := size == 1<<(8-uint(n))-1
+ for i := 1; i < n; i++ {
+ size = size<<8 | uint64(b[i])
+ allOnes = allOnes && b[i] == 0xFF
+ }
+ if allOnes {
+ return 0, nil, false
+ }
+ return size, b[n:], true
+}
+
+// ebmlLength returns how many bytes a VINT starting with this byte occupies —
+// one plus the number of leading zero bits — or 0 for the invalid all-zero
+// marker byte.
+func ebmlLength(first byte) int {
+ for i := 0; i < 8; i++ {
+ if first&(0x80>>uint(i)) != 0 {
+ return i + 1
+ }
+ }
+ return 0
+}
+
+// validADTSHeader recognises the ADTS framing raw .aac files use: the 12-bit
+// syncword plus the two layer bits ADTS requires to be zero.
+//
+// THIS SIGNATURE IS WEAKER THAN THE REST, and that is why its caller treats it
+// differently. Fourteen bits at offset zero is less than the three bytes the
+// stdlib uses for audio/mpeg, and MPEG audio shares the syncword's shape. So
+// ValidateUpload also requires the .aac EXTENSION — not as evidence, but to
+// decide whether a signature this weak may speak at all.
+//
+// The frame-length, sampling-index and protection arithmetic that used to be
+// here is gone with the other structural validation. Review found it wrong in
+// both directions — accepting frames whose declared length could not hold
+// their own header, and rejecting legal MPEG-2 profiles — which is the tail
+// this file's header describes.
+//
+// Layout (ISO/IEC 13818-7 §6.2, adts_fixed_header):
+//
+// byte 0 syncword high 8 bits
+// byte 1 sync low 4 | MPEG version | layer (2 bits) | protection absent
+func validADTSHeader(head []byte) bool {
+ if len(head) < 2 {
+ return false
+ }
+ if head[0] != 0xFF || head[1]&0xF0 != 0xF0 {
+ return false
+ }
+ return head[1]>>1&0x03 == 0 // layer MUST be 00 for ADTS
+}
+
+// docTypeMIME maps a DocType payload to a MIME type. The value ENDS at its
+// first NUL and anything after is padding (RFC 8794 §13); a payload with no
+// NUL at all is the whole slice. A DocType this does not recognise returns ""
+// — not ours to name, and the stdlib's answer is no worse than a guess.
+func docTypeMIME(payload []byte) string {
+ if i := bytes.IndexByte(payload, 0); i >= 0 {
+ payload = payload[:i]
+ }
+ switch string(payload) {
+ case "matroska":
+ return "video/x-matroska"
+ case "webm":
+ return "video/webm"
+ }
+ return ""
+}
diff --git a/internal/attachments/mime_magic_test.go b/internal/attachments/mime_magic_test.go
new file mode 100644
index 00000000..929af657
--- /dev/null
+++ b/internal/attachments/mime_magic_test.go
@@ -0,0 +1,673 @@
+package attachments
+
+import (
+ "bytes"
+ "os"
+ "path/filepath"
+ "testing"
+)
+
+// fixtureBytes is the testing-free half of this package's fixture loading, so
+// a *testing.F can build a fuzz seed corpus from the same files that
+// readFixture (mime_isobmff_test.go) hands the table tests.
+func fixtureBytes(name string) ([]byte, error) {
+ return os.ReadFile(filepath.Join("testdata", name))
+}
+
+// TestBUG2963FormatsReachTheAllowlist is the headline property: for each
+// format BUG-2963 measured as unreachable, a real file of it now sniffs as the
+// spelling the allowlist uses and is ACCEPTED and stored under that spelling.
+//
+// Both halves are asserted deliberately. The sniff alone is not the claim —
+// the claim is what the door does — and the two can diverge, which is exactly
+// how .mkv hid: it sniffed to something the allowlist accepted, so the upload
+// succeeded while the stored type was false.
+func TestBUG2963FormatsReachTheAllowlist(t *testing.T) {
+ cases := []struct {
+ fixture string
+ filename string
+ want string // what the door stores
+ // sniffWant is what SniffMIME alone returns, when that differs from
+ // what the door stores. It differs for exactly one format: raw AAC is
+ // resolved in ValidateUpload, because its structure is too small to
+ // act on without the filename and SniffMIME never sees one.
+ sniffWant string
+ cat Category
+ why string
+ }{
+ {"tar.head512", "archive.tar", "application/x-tar", "", CategoryArchive, "ustar at offset 257"},
+ {"bzip2.head512", "notes.txt.bz2", "application/x-bzip2", "", CategoryArchive, "BZh plus the block-size digit"},
+ {"sevenzip.head512", "archive.7z", "application/x-7z-compressed", "", CategoryArchive, "the six-byte signature"},
+ {"flac.head512", "track.flac", "audio/flac", "", CategoryAudio, "fLaC plus a 34-byte STREAMINFO block"},
+ {"aac-adts.head512", "track.aac", "audio/aac", "application/octet-stream", CategoryAudio, "valid ADTS header, gated on the extension"},
+ {"avi.head512", "clip.avi", "video/x-msvideo", "", CategoryVideo, "video/avi alias"},
+ {"matroska.head512", "clip.mkv", "video/x-matroska", "", CategoryVideo, "EBML DocType matroska"},
+ }
+ for _, tc := range cases {
+ t.Run(tc.fixture, func(t *testing.T) {
+ head := readFixture(t, tc.fixture)
+ wantSniff := tc.sniffWant
+ if wantSniff == "" {
+ wantSniff = tc.want
+ }
+ if got := SniffMIME(head); got != wantSniff {
+ t.Errorf("SniffMIME = %q, want %q (%s)", got, wantSniff, tc.why)
+ }
+ entry, code, err := ValidateUpload(head, tc.filename)
+ if err != nil {
+ t.Fatalf("ValidateUpload(%s) rejected: code=%s err=%v", tc.filename, code, err)
+ }
+ if entry.MIME != tc.want {
+ t.Errorf("stored as %q, want %q", entry.MIME, tc.want)
+ }
+ if entry.Category != tc.cat {
+ t.Errorf("category %q, want %q", entry.Category, tc.cat)
+ }
+ })
+ }
+}
+
+// TestEBMLDocTypeIsParsedNotSearched pins the DocType read in all four
+// directions. The first two are the ordinary files. The last two are the pair
+// an adversarial round used to break a substring search, and they broke it
+// BOTH ways — the reason this is a parse now.
+//
+// Both adversarial fixtures were built by inserting a legal Void element (ID
+// 0xEC, contents meaningless by specification) into the ordinary fixtures and
+// widening the header's size field to match. The COMPLETE files ffprobe reads;
+// what is committed here is their first 512 bytes, like every fixture in this
+// package, because that is the whole input domain of SniffMIME. Running
+// ffprobe on the committed prefix fails with a premature EOF, which says
+// nothing about the file it came from — an earlier version of this comment
+// claimed the committed files were themselves readable, and they are not.
+func TestEBMLDocTypeIsParsedNotSearched(t *testing.T) {
+ cases := []struct {
+ fixture string
+ want string
+ why string
+ }{
+ {"matroska.head512", "video/x-matroska", "the ordinary Matroska file"},
+ {"webm.head512", "video/webm", "the ordinary WebM file — a read that always said Matroska would break this"},
+ {"webm-void-says-matroska.head512", "video/webm",
+ "a WebM whose Void padding contains the string \"matroska\"; its real DocType is webm"},
+ {"matroska-void-padded.head512", "video/x-matroska",
+ "a Matroska whose Void padding pushes DocType to offset 66 — past any fixed leading window"},
+ }
+ for _, tc := range cases {
+ t.Run(tc.fixture, func(t *testing.T) {
+ head := readFixture(t, tc.fixture)
+ // sniffEBMLDocType directly, not through SniffMIME. The WebM legs
+ // are the reason: SniffMIME falls back to the stdlib, which also
+ // answers video/webm, so a test routed through it stayed green
+ // with the explicit webm mapping deleted. Asking the parser is
+ // what makes the parser the thing under test.
+ if got := sniffEBMLDocType(head); got != tc.want {
+ t.Errorf("sniffEBMLDocType = %q, want %q — %s", got, tc.want, tc.why)
+ }
+ if got := SniffMIME(head); got != tc.want {
+ t.Errorf("SniffMIME = %q, want %q — %s", got, tc.want, tc.why)
+ }
+ })
+ }
+
+ // EBML magic, then bytes that parse to no DocType at all. The fallback is
+ // the stdlib's answer, so an unreadable header degrades to the behaviour
+ // that shipped before this check existed rather than to a refusal.
+ blank := append([]byte{0x1A, 0x45, 0xDF, 0xA3}, make([]byte, 120)...)
+ if got := SniffMIME(blank); got != "video/webm" {
+ t.Errorf("EBML header with no DocType sniffed %q, want the stdlib's video/webm", got)
+ }
+}
+
+// TestOggStaysRefused records a decision, not a mechanism: Ogg is NOT
+// recognised, and both fixtures are kept so the next person to reach for an
+// application/ogg alias meets the evidence first.
+//
+// An alias was written, ruled in, and then removed after review showed the
+// question it has to answer — is this container AUDIO — cannot be answered
+// from the head of the file. Ogg multiplexes, so a second video stream's pages
+// come later than any sniff can see. Both files below are refused today
+// exactly as they were before BUG-2963, which is the point: no regression, and
+// no acceptance of a type the allowlist never reviewed.
+func TestOggStaysRefused(t *testing.T) {
+ for _, f := range []string{"ogg-opus.head512", "ogg-vp8-video.head512"} {
+ t.Run(f, func(t *testing.T) {
+ if got := SniffMIME(readFixture(t, f)); got != "application/ogg" {
+ t.Errorf("SniffMIME = %q, want the unaliased application/ogg", got)
+ }
+ if entry, _, err := ValidateUpload(readFixture(t, f), "track.ogg"); err == nil {
+ t.Errorf("accepted and stored as %q; audio/ogg stays unreachable until "+
+ "either video/ogg is a reviewed allowlist entry or something "+
+ "demuxes far enough to enumerate the streams", entry.MIME)
+ }
+ })
+ }
+}
+
+// TestADTSGate covers the one place a filename participates, in both
+// directions. The gate is on the stdlib having no SPECIFIC format opinion —
+// application/octet-stream or text/plain — and on the .aac extension. Neither
+// half is sufficient and the pair is not a formality: review found real,
+// ffmpeg-decodable AAC files being refused because only the first verdict was
+// accepted.
+func TestADTSGate(t *testing.T) {
+ adts := readFixture(t, "aac-adts.head512")
+
+ if _, _, err := ValidateUpload(adts, "track.aac"); err != nil {
+ t.Fatalf("premise failed: the real ADTS fixture named .aac is rejected: %v", err)
+ }
+
+ // Structure without the extension: nothing asked for this reading.
+ if _, code, err := ValidateUpload(adts, "track.bin"); err == nil {
+ t.Error("ADTS bytes named .bin were accepted; the extension gate is not applied")
+ } else if code != "mime_not_allowed" {
+ t.Errorf("code = %q, want mime_not_allowed", code)
+ }
+
+ // Extension without the structure.
+ if _, code, err := ValidateUpload(make([]byte, 512), "track.aac"); err == nil {
+ t.Error("zero bytes named .aac were accepted; the extension is being trusted alone")
+ } else if code != "mime_not_allowed" {
+ t.Errorf("code = %q, want mime_not_allowed", code)
+ }
+
+ // A REAL AAC file can look textual. These seven bytes are a valid ADTS
+ // header whose every byte the stdlib reads as text, so it answers
+ // text/plain — the shape of an AAC frame whose ancillary payload is
+ // printable. Review confirmed ffmpeg decodes such a file. It must be
+ // ACCEPTED: refusing it was a real file of a listed type turned away.
+ textual := []byte{0xFF, 0xF1, 0x40, 0x41, 0x41, 0x41, 0x41}
+ if !validADTSHeader(textual) {
+ t.Fatal("premise failed: the input must be a valid ADTS header")
+ }
+ if got := SniffMIME(textual); got != "text/plain" {
+ t.Fatalf("premise failed: the stdlib called it %q, not text/plain — this case "+
+ "exists to cover the text/plain half of the gate", got)
+ }
+ entry, _, err := ValidateUpload(textual, "textual.aac")
+ if err != nil {
+ t.Errorf("a valid ADTS header the stdlib reads as text was refused: %v", err)
+ } else if entry.MIME != "audio/aac" {
+ t.Errorf("stored as %q, want audio/aac", entry.MIME)
+ }
+
+ // A type that IS identified is untouched by the .aac name. PNG bytes stay
+ // an image — but note what that leg does and does not establish: PNG fails
+ // validADTSHeader on its first byte, so it would be refused with the
+ // verdict gate removed too. It is a sanity case, not a control.
+ if _, code, _ := ValidateUpload([]byte("\x89PNG\r\n\x1a\n"), "sneaky.aac"); code != "mime_extension_mismatch" {
+ t.Errorf("PNG bytes named .aac gave code %q, want mime_extension_mismatch", code)
+ }
+
+ // An AAC whose payload contains "ustar" at offset 257. THIS PACKAGE
+ // refines such bytes to application/x-tar, and gating the AAC branch on
+ // that refined value refused the file under its own .aac name — a review
+ // round built a complete, ffmpeg-decodable AAC of exactly this shape. The
+ // gate reads the STANDARD LIBRARY's verdict instead, which for these bytes
+ // is "nothing identifies this", so the branch runs and the file is
+ // accepted.
+ //
+ // The buffer here is synthetic: it reproduces the CONDITION (a valid ADTS
+ // header, plus our own recogniser answering something else) without
+ // claiming to be decodable audio, since overwriting a real frame's bytes
+ // produces a file ffmpeg rejects. The real-file half of this class is
+ // carried by flac-ustar-in-comment.head512, which does decode.
+ collide := make([]byte, 512)
+ copy(collide, adts[:8])
+ copy(collide[257:], []byte("ustar"))
+ if !validADTSHeader(collide) {
+ t.Fatal("premise failed: the collision buffer must be a valid ADTS header")
+ }
+ if got := SniffMIME(collide); got != "application/x-tar" {
+ t.Fatalf("premise failed: SniffMIME said %q, want application/x-tar — this case "+
+ "exists because our own refinement disagrees with the stdlib here", got)
+ }
+ if entry, _, err := ValidateUpload(collide, "track.aac"); err != nil {
+ t.Errorf("refused (%v); the gate must read the stdlib's verdict, not ours", err)
+ } else if entry.MIME != "audio/aac" {
+ t.Errorf("stored as %q, want audio/aac", entry.MIME)
+ }
+
+ // The signature, byte by byte. Each leg below fails for exactly one
+ // reason, because a single example leaves most of the check untested —
+ // a review round confirmed that dropping the first-byte test, or checking
+ // only one of the two layer bits, left the suite green.
+ for _, tc := range []struct {
+ name string
+ mutate func([]byte)
+ why string
+ }{
+ {"first sync byte wrong", func(b []byte) { b[0] = 0xFE }, "the syncword is 12 bits across two bytes"},
+ {"second sync nibble wrong", func(b []byte) { b[1] &^= 0x10 }, "the high nibble of byte 1 completes the sync"},
+ {"layer bit 1 set", func(b []byte) { b[1] = b[1]&0xF9 | 0x02 }, "ADTS requires layer 00"},
+ {"layer bit 2 set", func(b []byte) { b[1] = b[1]&0xF9 | 0x04 }, "both layer bits are checked, not one"},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ b := append([]byte(nil), adts...)
+ tc.mutate(b)
+ if _, _, err := ValidateUpload(b, "track.aac"); err == nil {
+ t.Errorf("accepted — %s", tc.why)
+ }
+ })
+ }
+}
+
+// TestTarWinsAPrefixCollision covers a real archive refused because another
+// format's magic appeared in its member's FILENAME. A tar header's first 100
+// bytes are user-chosen text, so any prefix recogniser can collide with an
+// ordinary archive; the collision is asymmetric, which is why tar is tested
+// first.
+func TestTarWinsAPrefixCollision(t *testing.T) {
+ b := readFixture(t, "tar-flac-named-member.head512")
+ if string(b[:4]) != "fLaC" {
+ t.Fatal("premise failed: the fixture's first member must be named fLaC.txt, " +
+ "or there is no collision to arbitrate")
+ }
+ if !bytes.Equal(b[257:262], []byte("ustar")) {
+ t.Fatal("premise failed: the fixture must be a real ustar archive")
+ }
+ entry, code, err := ValidateUpload(b, "archive.tar")
+ if err != nil {
+ t.Fatalf("a real tar whose first member is named fLaC.txt was refused (code=%s)", code)
+ }
+ if entry.MIME != "application/x-tar" {
+ t.Errorf("stored as %q, want application/x-tar", entry.MIME)
+ }
+}
+
+// TestOpaqueMagicLosesToTheStdlib pins the ordering: the magic table is
+// consulted only where the stdlib had no opinion, so it can add a detection
+// and never replace one.
+//
+// The fixture is a REAL tar archive whose first member is named BM.txt, which
+// makes the stdlib answer image/bmp. That is a genuine limitation of this
+// design and is asserted rather than hidden: such a tar is refused today and
+// still is. An earlier version of this test used a spliced PNG and claimed no
+// real file exercised competing detections, which was false.
+//
+// The mutation that kills this test is hoisting sniffOpaqueMagic ABOVE the
+// stdlib call. Deleting the application/octet-stream case does NOT — that is
+// caught by the positive tests instead. The distinction is recorded because
+// the comment here previously named the wrong mutation, contradicting the
+// project's own matrix.
+func TestOpaqueMagicLosesToTheStdlib(t *testing.T) {
+ tarBMP := readFixture(t, "tar-bmp-firstmember.head512")
+
+ if !validTarHeader(tarBMP) {
+ t.Fatal("premise failed: the fixture must be recognised as a tar, " +
+ "or this asserts nothing about which check wins")
+ }
+ if got := SniffMIME(tarBMP); got != "image/bmp" {
+ t.Errorf("SniffMIME = %q, want image/bmp — a recognised tar must not "+
+ "override a type the stdlib identified", got)
+ }
+ if _, code, err := ValidateUpload(tarBMP, "archive.tar"); err == nil {
+ t.Error("accepted; a tar the stdlib reads as an image is refused, as it was before this change")
+ } else if code == "" {
+ t.Errorf("rejected with an empty code")
+ }
+}
+
+// TestBUG2963F6RemovedSpellings covers the deletions and, more importantly,
+// the thing deleting them nearly broke. extMIMEMap's values are looked up in
+// `allowed`, and an extension whose value is NOT allowed is the mechanism that
+// refuses .svg and .exe — so removing application/xml while .xml still named
+// it would have turned every XML upload into extension_blocked. The upload leg
+// is the regression guard; the lookup legs are the deletion itself.
+func TestBUG2963F6RemovedSpellings(t *testing.T) {
+ for _, m := range []string{"application/javascript", "text/yaml", "application/xml"} {
+ if _, ok := LookupMIME(m); ok {
+ t.Errorf("%q is still on the allowlist; BUG-2963 F6 removed it as unreachable", m)
+ }
+ }
+
+ xml := []byte(`