From d048631abc4fea416d08045fb2167db07c22b307 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 22 Sep 2026 20:54:46 -0400 Subject: [PATCH] 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.svg" or `x.s"vg` passed the blocklist (".svg" 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 | 107 +++++++ internal/attachments/filename_test.go | 120 ++++++++ internal/attachments/mime.go | 22 +- internal/server/handlers_attachments.go | 100 ++----- .../handlers_attachments_bug2818_test.go | 268 ++++++++++++++++++ internal/server/handlers_import_bundle.go | 10 +- 6 files changed, 541 insertions(+), 86 deletions(-) create mode 100644 internal/attachments/filename.go create mode 100644 internal/attachments/filename_test.go create mode 100644 internal/server/handlers_attachments_bug2818_test.go diff --git a/internal/attachments/filename.go b/internal/attachments/filename.go new file mode 100644 index 000000000..65b6d6ae9 --- /dev/null +++ b/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.svg" or `x.s"vg` passed the blocklist (".svg" +// 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 +} diff --git a/internal/attachments/filename_test.go b/internal/attachments/filename_test.go new file mode 100644 index 000000000..f00a45deb --- /dev/null +++ b/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(``) + +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") + } +} diff --git a/internal/attachments/mime.go b/internal/attachments/mime.go index 0657bdb48..163b16da3 100644 --- a/internal/attachments/mime.go +++ b/internal/attachments/mime.go @@ -639,9 +639,11 @@ func buildCanonicalExtForMIME() map[string]string { // // That divergence — storable here, stripped by Content-Disposition sanitising, // so ".svg" 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", diff --git a/internal/server/handlers_attachments.go b/internal/server/handlers_attachments.go index e948a2530..9f546ed48 100644 --- a/internal/server/handlers_attachments.go +++ b/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 "shot.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 ".svg" 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) } } diff --git a/internal/server/handlers_attachments_bug2818_test.go b/internal/server/handlers_attachments_bug2818_test.go new file mode 100644 index 000000000..e2195352b --- /dev/null +++ b/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(``) + +// 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)) + } + } +} diff --git a/internal/server/handlers_import_bundle.go b/internal/server/handlers_import_bundle.go index 3dd748567..ce0f57aa3 100644 --- a/internal/server/handlers_import_bundle.go +++ b/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, }