fix(attachments): the blocklist judges the same filename the download header serves (BUG-2818) (#1445)

A stored attachment name had three readers that disagreed. The extension
blocklist looked up filepath.Ext of the name as stored; the
Content-Disposition sanitiser dropped control characters, '"' and '\'
before serving it. So "x.s<VT>vg" or `x.s"vg` passed the blocklist
(".s<VT>vg" is no known extension) and was downloaded as "x.svg". For SVG,
whose bytes sniff as allowed text/xml, the extension is the whole defence.

The fix makes them read one string:

- DroppedFilenameRune (internal/attachments) is the ONE character rule:
  Unicode control characters (C0, DEL, C1), '"' and '\'. The header
  sanitiser uses it instead of its own switch.
- NormalizeFilename runs at BOTH ingest doors before ValidateUpload: the
  upload handler, and bundle import, which stored the manifest's name
  verbatim with none of the upload door's rules. It keeps the upload
  door's leaf reduction and BUG-2803's fallback, drops the predicate's
  characters, and trims trailing dots and spaces (which a download save
  strips) unless that would leave nothing.
- ServedFilename covers legacy rows stored before this: a name whose
  normalised extension is blocked is served as stem + ".bin".

Normalise rather than refuse: the attack still ends in the ordinary 415
extension_blocked, because its normalised name carries the extension it
was hiding, while a benign name with a stray tab is stored without it.

Visible change: stored attachment names lose control characters, quotes
and trailing dots/spaces.

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

internal/attachments/filename.go

@@ -0,0 +1,107 @@
+package attachments
+
+import (
+	"path/filepath"
+	"strings"
+	"unicode"
+	"unicode/utf8"
+)
+
+// BUG-2818: one filename, three readers.
+//
+// A stored attachment name is read by the extension blocklist (ValidateUpload),
+// by the Content-Disposition header that names the download, and by whatever
+// saves that download to disk. They used to disagree: the header dropped
+// control bytes, quotes and backslashes that the blocklist had counted as part
+// of the extension, so "x.s<VT>vg" or `x.s"vg` passed the blocklist (".s<VT>vg"
+// is no known extension) and reached the client as "x.svg". The blocklist is the
+// whole defence for SVG, whose bytes sniff as allowed text/xml.
+//
+// The fix is that all three read ONE string. NormalizeFilename runs at every
+// ingest door BEFORE ValidateUpload, and the header drops characters with the
+// same predicate, DroppedFilenameRune, so the name the blocklist evaluates is by
+// construction the name the header serves.
+
+// DroppedFilenameRune reports whether r is removed from an attachment filename.
+// It is the ONE character rule shared by NormalizeFilename (ingest) and the
+// Content-Disposition sanitiser (serve); a second copy of it anywhere is the
+// defect this exists to prevent.
+//
+//   - Unicode control characters (C0, DEL and C1): no legitimate use in a name,
+//     and the header cannot carry them.
+//   - '"' and '\': they break the header's quoted-string, so the header has to
+//     drop them, and anything it drops the blocklist must not have seen.
+func DroppedFilenameRune(r rune) bool {
+	return unicode.IsControl(r) || r == '"' || r == '\\'
+}
+
+// fallbackFilename is the name for an upload whose own name leaves nothing.
+const fallbackFilename = "upload.bin"
+
+// NormalizeFilename is the stored form of a caller-supplied attachment name.
+// Every door that writes attachments.filename calls it, before ValidateUpload
+// sees the name. It normalises rather than refuses: the bytes of the upload are
+// fine and only the label is at issue, and an attack name still ends in a
+// refusal, because its normalised form carries the extension it was hiding
+// (checkpoint 1 on BUG-2818 has the full reasoning).
+//
+// In order:
+//  1. Reduce to a leaf under BOTH separator conventions. filepath.Base is
+//     platform-specific and leaves a backslash alone on Unix, while the stored
+//     name is consumed cross-platform, where "..\evil" is a traversal.
+//  2. A name that is not valid UTF-8, or that carries a NUL, cannot be stored
+//     (the server's bindableText rule; BUG-2803's fallback). It falls back to
+//     "upload" plus its extension only when that extension is known and allowed
+//     (SafeFallbackExtension), so the fallback cannot carry a blocked suffix.
+//  3. Drop every rune DroppedFilenameRune names.
+//  4. Trim trailing dots and spaces. Windows and browser download sanitisers
+//     strip them when saving, so "x.svg." lands on disk as "x.svg" while
+//     filepath.Ext reads ".". Skipped when it would leave nothing.
+//  5. A name left empty, or that is only a path component ("." or ".."),
+//     becomes "upload.bin".
+func NormalizeFilename(raw string) string {
+	name := filepath.Base(raw)
+	if i := strings.LastIndexByte(name, '\\'); i >= 0 {
+		name = name[i+1:]
+	}
+	if !utf8.ValidString(name) || strings.ContainsRune(name, 0) {
+		if ext := filepath.Ext(name); SafeFallbackExtension(ext) {
+			return "upload" + strings.ToLower(ext)
+		}
+		return "upload"
+	}
+	name = strings.Map(func(r rune) rune {
+		if DroppedFilenameRune(r) {
+			return -1
+		}
+		return r
+	}, name)
+	// Only when something is left: a name that is ALL dots and spaces has no
+	// extension to expose, and "..." is an ordinary POSIX name (BUG-2803,
+	// codex round 27), while "." and ".." are caught below as path components.
+	if trimmed := strings.TrimRight(name, ". "); trimmed != "" {
+		name = trimmed
+	}
+	if name == "" || name == "." || name == ".." || name == "/" {
+		return fallbackFilename
+	}
+	return name
+}
+
+// ServedFilename is the name an attachment is offered under, derived from its
+// STORED name. For a row stored after BUG-2818 it equals the stored name.
+// For a LEGACY row, stored before ingest normalised, it applies the same
+// normalisation, and when that uncovers an extension the blocklist refuses
+// (the row that was never evaluated as one), it replaces the extension with
+// ".bin" so the download cannot land as the blocked type.
+func ServedFilename(stored string) string {
+	name := NormalizeFilename(stored)
+	if ext := filepath.Ext(name); BlockedExtension(ext) {
+		stem := strings.TrimSuffix(name, ext)
+		if stem == "" {
+			stem = "attachment"
+		}
+		return stem + ".bin"
+	}
+	return name
+}

internal/attachments/filename_test.go

@@ -0,0 +1,120 @@
+package attachments
+
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// svgHead is an SVG document with a script, which http.DetectContentType
+// sniffs as text/xml: an ALLOWED type. The extension is therefore the only
+// thing that refuses it, which is what makes an extension bypass a bypass.
+var svgHead = []byte(`<?xml version="1.0"?><svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>`)
+
+func TestNormalizeFilename(t *testing.T) {
+	cases := []struct{ in, want string }{
+		// Unchanged: ordinary names in any script.
+		{"shot.png", "shot.png"},
+		{"日本語のメモ.txt", "日本語のメモ.txt"},
+		{"a b.pdf", "a b.pdf"},
+		{"...hidden", "...hidden"},
+		// BUG-2818: every character the header drops is dropped HERE, so the
+		// extension the blocklist judges is the one the header serves.
+		{"x.s\vvg", "x.svg"},
+		{"x.s\x08vg", "x.svg"},
+		{"x.s\u0085vg", "x.svg"}, // C1: a control character too
+		{"x.s\x7fvg", "x.svg"},
+		{`x.s"vg`, "x.svg"},
+		// Trailing dots and spaces, which a download save strips (Q1).
+		{"x.svg.", "x.svg"},
+		{"x.svg ", "x.svg"},
+		{"x.svg . .", "x.svg"},
+		{"x.svg\t.", "x.svg"},
+		// Leaf reduction under both separators, as the upload door always did.
+		{"a/b/c.png", "c.png"},
+		{`..\..\evil.png`, "evil.png"},
+		// A backslash cut is a SEPARATOR first: `x.s\vg` is the leaf "vg",
+		// never "x.svg".
+		{`x.s\vg`, "vg"},
+		// Nothing left: the generic name.
+		{"", "upload.bin"},
+		{".", "upload.bin"},
+		{"..", "upload.bin"},
+		{"/", "upload.bin"},
+		{"\v\v", "upload.bin"},
+		// All dots is a POSIX name, not a path component (BUG-2803 round 27).
+		{"...", "..."},
+		{". .", ". ."},
+		// Unstorable text keeps BUG-2803's fallback, extension only if allowed.
+		{"sh\x00ot.png", "upload.png"},
+		{"sh\x00ot.svg", "upload"},
+		{"bad\xffname.png", "upload.png"},
+	}
+	for _, c := range cases {
+		if got := NormalizeFilename(c.in); got != c.want {
+			t.Errorf("NormalizeFilename(%q) = %q, want %q", c.in, got, c.want)
+		}
+	}
+}
+
+// The adversarial property, per attack name. The raw name PASSES the blocklist
+// (the counterfactual: this is the bypass, measured rather than asserted), and
+// the normalised name is refused as extension_blocked.
+func TestNormalizedAttackNamesAreRefusedByTheBlocklist(t *testing.T) {
+	attacks := []string{
+		"x.s\vvg", "x.s\x08vg", "x.s\u0085vg", "x.s\x7fvg", `x.s"vg`,
+		"x.svg.", "x.svg ", "x.svg\t.", "x.e\vxe", "x.b\"at",
+	}
+	for _, raw := range attacks {
+		if _, code, err := ValidateUpload(svgHead, raw); err != nil {
+			t.Errorf("counterfactual %q: the raw name was already refused (%s), so this case proves nothing", raw, code)
+		}
+		name := NormalizeFilename(raw)
+		if _, code, err := ValidateUpload(svgHead, name); err == nil || code != "extension_blocked" {
+			t.Errorf("%q normalised to %q, which ValidateUpload answered %q (%v); want extension_blocked", raw, name, code, err)
+		}
+	}
+}
+
+func TestServedFilename(t *testing.T) {
+	cases := []struct{ stored, want string }{
+		{"shot.png", "shot.png"},
+		{"report.pdf", "report.pdf"},
+		// Legacy rows stored before BUG-2818 whose normalised extension is
+		// blocked: served as .bin, never as the blocked type.
+		{"x.s\vvg", "x.bin"},
+		{`x.s"vg`, "x.bin"},
+		{"setup.e\vxe", "setup.bin"},
+		{"x.svg.", "x.bin"},
+		{".s\vvg", "attachment.bin"},
+		// A legacy bundle-import name carrying a backslash reduces to its leaf.
+		{`x.s\vg`, "vg"},
+	}
+	for _, c := range cases {
+		got := ServedFilename(c.stored)
+		if got != c.want {
+			t.Errorf("ServedFilename(%q) = %q, want %q", c.stored, got, c.want)
+		}
+		if BlockedExtension(filepath.Ext(got)) {
+			t.Errorf("ServedFilename(%q) = %q serves a blocked extension", c.stored, got)
+		}
+	}
+}
+
+func TestBlockedExtensionAgreesWithValidateUpload(t *testing.T) {
+	// Every mapped extension: blocked iff ValidateUpload refuses it as
+	// extension_blocked on bytes it would otherwise accept. Two readings of
+	// one map, checked against each other rather than against a list.
+	for ext := range extMIMEMap {
+		_, code, _ := ValidateUpload(svgHead, "f"+ext)
+		if got, want := BlockedExtension(ext), code == "extension_blocked"; got != want {
+			t.Errorf("BlockedExtension(%q) = %v, but ValidateUpload answered %q", ext, got, code)
+		}
+		if BlockedExtension(ext) != BlockedExtension(strings.ToUpper(ext)) {
+			t.Errorf("BlockedExtension(%q) depends on case", ext)
+		}
+	}
+	if BlockedExtension(".nosuchext") || BlockedExtension("") {
+		t.Error("an unknown or empty extension is not blocked")
+	}
+}

internal/attachments/mime.go

@@ -639,9 +639,11 @@ func buildCanonicalExtForMIME() map[string]string {
 //
 // That divergence — storable here, stripped by Content-Disposition sanitising,
 // so ".s<VT>vg" reaches the client as ".svg" past a blocklist that never
-// evaluated it — is a pre-existing hazard on the ORDINARY upload path, where
-// the name is not synthesised at all. Tracked as BUG-2818; this predicate only
-// refuses to add a second door to it.
+// evaluated it — was also open on the ORDINARY upload path, where the name is
+// not synthesised at all. BUG-2818 closed it there: NormalizeFilename drops
+// those characters before ValidateUpload judges the name (filename.go). This
+// predicate still refuses such a suffix on its own terms, and is reached only
+// for a name that cannot be stored at all.
 //
 // Anything else is dropped and the fallback stays extensionless.
 func SafeFallbackExtension(ext string) bool {
@@ -656,6 +658,20 @@ func SafeFallbackExtension(ext string) bool {
 	return allowed
 }
 
+// BlockedExtension reports whether ext (with its leading dot, any case) is one
+// ValidateUpload refuses as extension_blocked: a known extension whose type is
+// not on the allowlist. It is the same map and the same allowlist lookup, so
+// ServedFilename (BUG-2818) cannot disagree with the upload gate about which
+// suffixes are blocked.
+func BlockedExtension(ext string) bool {
+	mimeStr, ok := extMIMEMap[strings.ToLower(ext)]
+	if !ok {
+		return false
+	}
+	_, allowed := allowed[NormalizeMIME(mimeStr)]
+	return !allowed
+}
+
 var extMIMEMap = map[string]string{
 	".png":  "image/png",
 	".jpg":  "image/jpeg",

internal/server/handlers_attachments.go

@@ -10,7 +10,6 @@ import (
 	"log/slog"
 	"net/http"
 	"os"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -322,75 +321,14 @@ func (s *Server) handleUploadAttachment(w http.ResponseWriter, r *http.Request)
 		itemIDPtr = &canonicalItemID
 	}
 
-	// Sanitize the filename: strip path components so a client can't
-	// sneak directory traversal through the display name. We don't
-	// store this in the storage backend — only in the DB row for UI.
-	// The uploaded filename is caller-supplied text bound for a text column,
-	// and a multipart header can carry a NUL through the RFC 5987 encoded
-	// form (filename*=UTF-8\'\'a%00.png). Same predicate as the path and
-	// query rules; falling back to a generic name rather than refusing the
-	// upload, because the bytes are fine and only the label is unusable
-	// (codex round 4, BUG-2803).
-	// Reduce to a leaf under BOTH separator conventions before anything else.
-	// filepath.Base is platform-specific, so on Unix it leaves a backslash
-	// alone — and the stored name is consumed cross-platform (a Windows client
-	// joining it onto a directory reads "..\\evil" as a traversal). Splitting
-	// on the backslash too makes the stored value a safe path component
-	// everywhere rather than only on the server's own OS (codex round 28).
-	//
-	// This NORMALISES rather than refuses: a legitimate Unix name containing a
-	// backslash keeps its last segment instead of being replaced wholesale,
-	// which is less lossy than the alternative and still portable.
-	filename := filepath.Base(header.Filename)
-	if i := strings.LastIndexByte(filename, '\\'); i >= 0 {
-		filename = filename[i+1:]
-	}
-	if !bindableText(filename) {
-		// Keep the EXTENSION when it is itself storable. The unusable part
-		// of a name like "sh<NUL>ot.png" is the stem; ".png" is ordinary
-		// text, and it is what every downstream consumer dispatches on —
-		// Content-Disposition, the web download anchor, bundle export naming,
-		// and `pad attachment view`, whose whole contract is handing a path to
-		// something that opens files by extension.
-		//
-		// Dropping it made this fallback lossier than the empty-name one two
-		// lines below, which has always produced "upload.bin" (codex round
-		// 24). Bounded to a short extension so a hostile name cannot smuggle
-		// a long tail through the fallback.
-		// bindableText is NOT the bar here (codex round 25). It permits
-		// control characters — they are valid UTF-8 and not NUL — and a
-		// control character survives storage but is STRIPPED when the name
-		// is written into Content-Disposition. So ".s<VT>vg" passes the
-		// extension blocklist, which sees no known extension, and reappears
-		// as ".svg" at the client. attachments.SafeFallbackExtension requires
-		// a KNOWN, ALLOWED, plain-alphanumeric extension instead, so a
-		// synthesised name can only carry a suffix the product already
-		// accepts on the ordinary path.
-		if ext := filepath.Ext(filename); attachments.SafeFallbackExtension(ext) {
-			filename = "upload" + strings.ToLower(ext)
-		} else {
-			filename = "upload"
-		}
-	}
-	// A name that is only dots or separators is not a filename, it is a PATH
-	// COMPONENT, and consumers join it onto a directory. ".." was missing
-	// here: it survives bindableText, and filepath.Ext("..") is "." — non-empty
-	// — so even an extension check passes it through, while filepath.Join on
-	// the client side resolves it to the PARENT directory (codex round 26).
-	//
-	// Only "." and ".." are path components; "..." and longer runs are
-	// ordinary POSIX filenames and are kept (codex round 27 — the trimmed-form
-	// check I used first refused those too, which is over-refusal for no
-	// gain). A backslash is likewise a legal character in a Unix filename, and
-	// filepath.Base above has already reduced any "a/b" to "b", so a separator
-	// test here is dead on this platform and was removed rather than left to
-	// look load-bearing.
-	//
-	// What remains is the real hazard: a name a consumer joins onto a
-	// directory and lands somewhere else.
-	if filename == "" || filename == "." || filename == ".." || filename == "/" {
-		filename = "upload.bin"
-	}
+	// The stored name, and the name ValidateUpload judges below: one string,
+	// normalised by the same function every ingest door calls (BUG-2818). It
+	// reduces the name to a leaf under both separator conventions, falls back
+	// to a generic name when the text cannot be stored at all (BUG-2803), drops
+	// the characters the Content-Disposition header drops, and trims the
+	// trailing dots and spaces a download would lose. See
+	// attachments.NormalizeFilename for each rule's reason.
+	filename := attachments.NormalizeFilename(header.Filename)
 
 	// Stream into a temp file under the OS temp dir. We copy in 32KiB
 	// chunks via io.Copy and tee through a sha256 hasher.
@@ -904,7 +842,7 @@ func (s *Server) handleGetAttachment(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("X-Pad-Attachment-Derived", derived)
 	}
 	w.Header().Set("Content-Disposition",
-		fmt.Sprintf(`%s; filename=%q`, disposition, sanitizeHeaderFilename(att.Filename)))
+		fmt.Sprintf(`%s; filename=%q`, disposition, sanitizeHeaderFilename(attachments.ServedFilename(att.Filename))))
 
 	// http.ServeContent gives Range, conditional GETs, and 206
 	// responses for free — but it requires an io.ReadSeeker. FSStore
@@ -956,10 +894,12 @@ func isKnownVariant(v string) bool {
 }
 
 // sanitizeHeaderFilename strips characters that can't safely appear in
-// a Content-Disposition filename header — quotes, CR, LF, and any
-// control byte. The Filename column was already basenamed at upload
-// time so path separators are not a concern; we only need to keep the
-// header value parseable.
+// a Content-Disposition filename header: quotes, backslashes and control
+// characters, by attachments.DroppedFilenameRune, which is also the rule
+// ingest normalises with (BUG-2818). The caller passes the SERVED name
+// (attachments.ServedFilename), so for any row stored since that fix this
+// drops nothing; it still runs because a header value must be parseable
+// whatever reaches it.
 func sanitizeHeaderFilename(name string) string {
 	if name == "" {
 		return "attachment"
@@ -967,12 +907,10 @@ func sanitizeHeaderFilename(name string) string {
 	var b strings.Builder
 	b.Grow(len(name))
 	for _, r := range name {
-		switch {
-		case r == '"' || r == '\\':
-			// Drop quotes and backslashes — they break the quoted-string syntax.
-		case r < 0x20 || r == 0x7f:
-			// Drop control bytes — protect against header injection.
-		default:
+		// The SAME predicate ingest normalises with (BUG-2818): a character
+		// dropped here and kept there would let the blocklist judge a
+		// different extension from the one this header serves.
+		if !attachments.DroppedFilenameRune(r) {
 			b.WriteRune(r)
 		}
 	}

internal/server/handlers_attachments_bug2818_test.go

@@ -0,0 +1,268 @@
+package server
+
+import (
+	"archive/tar"
+	"bytes"
+	"compress/gzip"
+	"encoding/json"
+	"io"
+	"mime"
+	"mime/multipart"
+	"net/http"
+	"net/http/httptest"
+	"net/textproto"
+	"path/filepath"
+	"strings"
+	"testing"
+	"unicode/utf8"
+
+	"github.com/PerpetualSoftware/pad/internal/attachments"
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// BUG-2818: a name the blocklist passed was served as a blocked extension,
+// because the Content-Disposition sanitiser dropped characters (control
+// bytes, '"', '\') that the blocklist had counted as part of the extension.
+
+// svgScript sniffs as text/xml, an ALLOWED type, so only the extension refuses it.
+var svgScript = []byte(`<?xml version="1.0"?><svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>`)
+
+// uploadEncodedName uploads body under an RFC 5987 encoded filename. That is
+// the reachable spelling for these bytes: Go's multipart reader refuses a raw
+// control byte in a header line before any handler sees it.
+func uploadEncodedName(t *testing.T, srv *Server, slug, encoded string, body []byte) *httptest.ResponseRecorder {
+	t.Helper()
+	var buf bytes.Buffer
+	mw := multipart.NewWriter(&buf)
+	h := make(textproto.MIMEHeader)
+	h.Set("Content-Disposition", `form-data; name="file"; filename*=UTF-8''`+encoded)
+	h.Set("Content-Type", "application/octet-stream")
+	part, err := mw.CreatePart(h)
+	if err != nil {
+		t.Fatalf("create part: %v", err)
+	}
+	part.Write(body)
+	mw.Close()
+	req := httptest.NewRequest("POST", "/api/v1/workspaces/"+slug+"/attachments", &buf)
+	req.Header.Set("Content-Type", mw.FormDataContentType())
+	req.RemoteAddr = "127.0.0.1:1234"
+	rr := httptest.NewRecorder()
+	srv.ServeHTTP(rr, req)
+	return rr
+}
+
+// The attack at the upload door: every spelling that used to store and serve
+// as ".svg" is now refused as extension_blocked, because the blocklist judges
+// the normalised name.
+func TestUpload_BUG2818_HiddenBlockedExtensionIsRefused(t *testing.T) {
+	srv, slug := testServerWithAttachments(t)
+	for _, encoded := range []string{
+		"x.s%0Bvg",    // vertical tab (C0)
+		"x.s%08vg",    // backspace
+		"x.s%7Fvg",    // DEL
+		"x.s%C2%85vg", // NEL (C1)
+		"x.s%22vg",    // double quote
+		"x.svg.",      // trailing dot
+		"x.svg%20",    // trailing space
+	} {
+		rr := uploadEncodedName(t, srv, slug, encoded, svgScript)
+		if rr.Code != http.StatusUnsupportedMediaType || !strings.Contains(rr.Body.String(), "extension_blocked") {
+			t.Errorf("%s: status=%d body=%s; want 415 extension_blocked", encoded, rr.Code, rr.Body.String())
+		}
+	}
+	// Control: the same bytes under an honest, allowed name are accepted, so
+	// the refusals above are about the name.
+	if rr := uploadEncodedName(t, srv, slug, "x.xml", svgScript); rr.Code != http.StatusCreated {
+		t.Fatalf("control upload x.xml: status=%d body=%s", rr.Code, rr.Body.String())
+	}
+}
+
+// A benign name carrying a stray control byte is stored without it, not refused.
+func TestUpload_BUG2818_BenignControlByteIsNormalisedNotRefused(t *testing.T) {
+	srv, slug := testServerWithAttachments(t)
+	rr := uploadEncodedName(t, srv, slug, "my%09notes.txt", []byte("plain text\n"))
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	var got struct {
+		Filename string `json:"filename"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &got); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if got.Filename != "mynotes.txt" {
+		t.Errorf("stored filename = %q, want %q", got.Filename, "mynotes.txt")
+	}
+}
+
+// A LEGACY row, stored before ingest normalised, is served under a name that
+// cannot be the blocked type, whatever its stored bytes say.
+func TestDownload_BUG2818_LegacyRowIsNotServedAsABlockedExtension(t *testing.T) {
+	srv, slug := testServerWithAttachments(t)
+	for _, legacy := range []string{"x.s\vvg", `x.s"vg`, "x.svg.", "setup.e\x08xe"} {
+		id, _, urlPath := uploadHelper(t, srv, slug, "legacy.xml", svgScript)
+		if _, err := srv.store.DB().Exec(`UPDATE attachments SET filename = ? WHERE id = ?`, legacy, id); err != nil {
+			t.Fatalf("plant legacy name %q: %v", legacy, err)
+		}
+		req := httptest.NewRequest("GET", urlPath, nil)
+		req.RemoteAddr = "127.0.0.1:1234"
+		rr := httptest.NewRecorder()
+		srv.ServeHTTP(rr, req)
+		if rr.Code != http.StatusOK {
+			t.Fatalf("%q: status=%d body=%s", legacy, rr.Code, rr.Body.String())
+		}
+		served := dispositionFilename(t, rr.Header().Get("Content-Disposition"))
+		if attachments.BlockedExtension(filepath.Ext(served)) {
+			t.Errorf("legacy %q served as %q: a blocked extension the blocklist never judged", legacy, served)
+		}
+		if !strings.HasSuffix(served, ".bin") {
+			t.Errorf("legacy %q served as %q, want the extension neutralised to .bin", legacy, served)
+		}
+	}
+}
+
+// dispositionFilename reads the filename parameter the way a client does.
+func dispositionFilename(t *testing.T, header string) string {
+	t.Helper()
+	_, params, err := mime.ParseMediaType(header)
+	if err != nil {
+		t.Fatalf("parse Content-Disposition %q: %v", header, err)
+	}
+	return params["filename"]
+}
+
+// The SECOND door: bundle import stored the manifest's name verbatim. A bundle
+// whose entry hides .svg is now judged on the normalised name, and refused
+// (the importer skips a refused attachment), while an entry whose name only
+// needs cleaning is imported cleaned.
+func TestImportBundle_BUG2818_ManifestNamesAreNormalisedBeforeTheBlocklist(t *testing.T) {
+	src, srcSlug := testServerWithAttachments(t)
+	uploadHelper(t, src, srcSlug, "attack.xml", svgScript)
+	uploadHelper(t, src, srcSlug, "benign.txt", []byte("plain text\n"))
+	rr := doRequest(src, "GET", "/api/v1/workspaces/"+srcSlug+"/export?format=tar", nil)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("export: %d %s", rr.Code, rr.Body.String())
+	}
+	bundle := rewriteManifestNames(t, rr.Body.Bytes(), map[string]string{
+		"attack.xml": "x.s\vvg",
+		"benign.txt": `..\no` + "\t" + `tes.txt`,
+	})
+
+	dest, _ := testServerWithAttachments(t)
+	req := httptest.NewRequest("POST", "/api/v1/workspaces/import?name=Imported", bytes.NewReader(bundle))
+	req.Header.Set("Content-Type", "application/gzip")
+	req.RemoteAddr = "127.0.0.1:1234"
+	rr = httptest.NewRecorder()
+	dest.ServeHTTP(rr, req)
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("import: status=%d body=%s", rr.Code, rr.Body.String())
+	}
+	var ws models.Workspace
+	if err := json.Unmarshal(rr.Body.Bytes(), &ws); err != nil {
+		t.Fatalf("decode ws: %v", err)
+	}
+	rr = doRequest(dest, "GET", "/api/v1/workspaces/"+ws.Slug+"/attachments", nil)
+	var list struct {
+		Attachments []struct {
+			Filename string `json:"filename"`
+		} `json:"attachments"`
+	}
+	if err := json.Unmarshal(rr.Body.Bytes(), &list); err != nil {
+		t.Fatalf("decode list: %v (%s)", err, rr.Body.String())
+	}
+	var names []string
+	for _, a := range list.Attachments {
+		names = append(names, a.Filename)
+	}
+	if len(names) != 1 || names[0] != "notes.txt" {
+		t.Errorf("imported attachment names = %q, want only [\"notes.txt\"] (the hidden .svg refused, the benign one cleaned)", names)
+	}
+}
+
+// rewriteManifestNames re-emits a bundle with manifest entries renamed, and
+// renames each one's BLOB entry to match. The importer pairs a blob with its
+// manifest entry by bundleAttachmentPath(id, filename), whose suffix comes from
+// the filename, so renaming only the manifest makes a changed-extension entry
+// match no blob and be skipped before it reaches the door under test (the
+// first version of this fixture did exactly that, and a mutant of the door
+// survived it). A crafted bundle names its blobs consistently.
+func rewriteManifestNames(t *testing.T, bundle []byte, rename map[string]string) []byte {
+	t.Helper()
+	gz, err := gzip.NewReader(bytes.NewReader(bundle))
+	if err != nil {
+		t.Fatalf("gzip: %v", err)
+	}
+	tr := tar.NewReader(gz)
+	var out bytes.Buffer
+	ogz := gzip.NewWriter(&out)
+	otw := tar.NewWriter(ogz)
+	renamed, reblobbed := 0, 0
+	blobRename := map[string]string{}
+	for {
+		hdr, err := tr.Next()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			t.Fatalf("tar: %v", err)
+		}
+		body, err := io.ReadAll(tr)
+		if err != nil {
+			t.Fatalf("read: %v", err)
+		}
+		if hdr.Name == "attachments/manifest.json" {
+			var m map[string]any
+			if err := json.Unmarshal(body, &m); err != nil {
+				t.Fatalf("manifest: %v", err)
+			}
+			entries, _ := m["entries"].([]any)
+			for _, e := range entries {
+				em := e.(map[string]any)
+				if to, ok := rename[em["filename"].(string)]; ok {
+					id := em["id"].(string)
+					blobRename[bundleAttachmentPath(id, em["filename"].(string))] = bundleAttachmentPath(id, to)
+					em["filename"] = to
+					renamed++
+				}
+			}
+			if body, err = json.Marshal(m); err != nil {
+				t.Fatalf("remarshal: %v", err)
+			}
+		}
+		name := hdr.Name
+		if to, ok := blobRename[name]; ok {
+			name = to
+			reblobbed++
+		}
+		if err := otw.WriteHeader(&tar.Header{Name: name, Mode: 0o644, Size: int64(len(body))}); err != nil {
+			t.Fatalf("header: %v", err)
+		}
+		if _, err := otw.Write(body); err != nil {
+			t.Fatalf("write: %v", err)
+		}
+	}
+	otw.Close()
+	ogz.Close()
+	if renamed != len(rename) || reblobbed != len(rename) {
+		t.Fatalf("renamed %d manifest entries and %d blobs, want %d of each: the fixture did not reach the door",
+			renamed, reblobbed, len(rename))
+	}
+	return out.Bytes()
+}
+
+// The header and ingest drop EXACTLY the same characters, over every rune.
+// This is the lead's ruled invariant for BUG-2818 as a test: a sanitiser with
+// its own character switch (the pre-fix shape) differs on C1 and fails here.
+func TestSanitizeHeaderFilename_DropsExactlyTheIngestPredicate(t *testing.T) {
+	for r := rune(0); r <= utf8.MaxRune; r++ {
+		if !utf8.ValidRune(r) {
+			continue
+		}
+		name := "a" + string(r) + "b"
+		dropped := sanitizeHeaderFilename(name) == "ab"
+		if dropped != attachments.DroppedFilenameRune(r) {
+			t.Fatalf("U+%04X: header drops=%v, ingest predicate drops=%v; the two must share one rule",
+				r, dropped, attachments.DroppedFilenameRune(r))
+		}
+	}
+}

internal/server/handlers_import_bundle.go

@@ -614,7 +614,13 @@ func (s *Server) rehydrateAttachment(
 	if len(head) > 512 {
 		head = head[:512]
 	}
-	allowed, code, vErr := attachments.ValidateUpload(head, entry.Filename)
+	// The manifest's name is caller-supplied, exactly like an upload's, and
+	// was stored VERBATIM before BUG-2818: no leaf reduction, no fallback, and
+	// the characters the download header drops still inside the extension the
+	// blocklist judged. It goes through the same normaliser as the upload door,
+	// BEFORE validation, so the name judged is the name stored and served.
+	filename := attachments.NormalizeFilename(entry.Filename)
+	allowed, code, vErr := attachments.ValidateUpload(head, filename)
 	if vErr != nil {
 		return "", fmt.Errorf("mime validation (%s): %w", code, vErr)
 	}
@@ -670,7 +676,7 @@ func (s *Server) rehydrateAttachment(
 		ContentHash: hash,
 		MimeType:    allowed.MIME,
 		SizeBytes:   int64(len(blob)),
-		Filename:    entry.Filename,
+		Filename:    filename,
 		Width:       entry.Width,
 		Height:      entry.Height,
 	}