From 547527d7dca2ec92f4351ae43329f4f188bcf108 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 26 Sep 2026 09:47:51 -0400 Subject: [PATCH] fix(server): Content-Disposition carries a non-ASCII or unquotable filename in filename* with an ASCII fallback, so NBSP and zero-width names no longer reach clients as Go escape text (BUG-3190) (#1569) * fix(server): Content-Disposition carries a non-ASCII or unquotable filename in filename* with an ASCII fallback, so NBSP and zero-width names no longer reach clients as Go escape text (BUG-3190) * test(server): the Content-Disposition population carries characters the extended form must encode, and the doc comment survives gofmt (BUG-3190) --- cmd/pad/bug3190_filename_star_test.go | 20 ++++++ internal/cli/bug3190_filename_star_test.go | 20 ++++++ internal/mcp/bug3190_filename_star_test.go | 20 ++++++ internal/server/content_disposition.go | 80 +++++++++++++++++++++ internal/server/content_disposition_test.go | 77 ++++++++++++++++++++ internal/server/handlers_artifact_export.go | 3 +- internal/server/handlers_attachments.go | 2 +- internal/server/handlers_export_bundle.go | 3 +- internal/server/handlers_workspaces.go | 2 +- 9 files changed, 221 insertions(+), 6 deletions(-) create mode 100644 cmd/pad/bug3190_filename_star_test.go create mode 100644 internal/cli/bug3190_filename_star_test.go create mode 100644 internal/mcp/bug3190_filename_star_test.go create mode 100644 internal/server/content_disposition.go create mode 100644 internal/server/content_disposition_test.go diff --git a/cmd/pad/bug3190_filename_star_test.go b/cmd/pad/bug3190_filename_star_test.go new file mode 100644 index 000000000..2debbdf2c --- /dev/null +++ b/cmd/pad/bug3190_filename_star_test.go @@ -0,0 +1,20 @@ +package main + +import "testing" + +// BUG-3190: the server sends a non-ASCII (or otherwise unquotable) name as an +// ASCII fallback in filename plus the exact name in filename*. This parser +// must return the exact name. These are the headers the server emits for +// those names, byte for byte (internal/server content_disposition_test.go +// pins the same form). +func TestBUG3190_parseAttachmentFilename_PrefersFilenameStar(t *testing.T) { + for header, want := range map[string]string{ + `inline; filename="a_b.txt"; filename*=UTF-8''a%C2%A0b.txt`: "a b.txt", + `attachment; filename="__.pdf"; filename*=UTF-8''%E4%BC%9A%E8%AD%B0.pdf`: "会議.pdf", + `inline; filename="plain.txt"`: "plain.txt", + } { + if got := parseAttachmentFilename(header); got != want { + t.Errorf("%s: got %q, want %q", header, got, want) + } + } +} diff --git a/internal/cli/bug3190_filename_star_test.go b/internal/cli/bug3190_filename_star_test.go new file mode 100644 index 000000000..3412d4cb5 --- /dev/null +++ b/internal/cli/bug3190_filename_star_test.go @@ -0,0 +1,20 @@ +package cli + +import "testing" + +// BUG-3190: the server sends a non-ASCII (or otherwise unquotable) name as an +// ASCII fallback in filename plus the exact name in filename*. This parser +// must return the exact name. These are the headers the server emits for +// those names, byte for byte (internal/server content_disposition_test.go +// pins the same form). +func TestBUG3190_filenameFromContentDisposition_PrefersFilenameStar(t *testing.T) { + for header, want := range map[string]string{ + `inline; filename="a_b.txt"; filename*=UTF-8''a%C2%A0b.txt`: "a b.txt", + `attachment; filename="__.pdf"; filename*=UTF-8''%E4%BC%9A%E8%AD%B0.pdf`: "会議.pdf", + `inline; filename="plain.txt"`: "plain.txt", + } { + if got := filenameFromContentDisposition(header); got != want { + t.Errorf("%s: got %q, want %q", header, got, want) + } + } +} diff --git a/internal/mcp/bug3190_filename_star_test.go b/internal/mcp/bug3190_filename_star_test.go new file mode 100644 index 000000000..6e0be94d7 --- /dev/null +++ b/internal/mcp/bug3190_filename_star_test.go @@ -0,0 +1,20 @@ +package mcp + +import "testing" + +// BUG-3190: the server sends a non-ASCII (or otherwise unquotable) name as an +// ASCII fallback in filename plus the exact name in filename*. This parser +// must return the exact name. These are the headers the server emits for +// those names, byte for byte (internal/server content_disposition_test.go +// pins the same form). +func TestBUG3190_parseAttachmentFilename_PrefersFilenameStar(t *testing.T) { + for header, want := range map[string]string{ + `inline; filename="a_b.txt"; filename*=UTF-8''a%C2%A0b.txt`: "a b.txt", + `attachment; filename="__.pdf"; filename*=UTF-8''%E4%BC%9A%E8%AD%B0.pdf`: "会議.pdf", + `inline; filename="plain.txt"`: "plain.txt", + } { + if got := parseAttachmentFilename(header); got != want { + t.Errorf("%s: got %q, want %q", header, got, want) + } + } +} diff --git a/internal/server/content_disposition.go b/internal/server/content_disposition.go new file mode 100644 index 000000000..e0d998211 --- /dev/null +++ b/internal/server/content_disposition.go @@ -0,0 +1,80 @@ +package server + +import ( + "strings" +) + +// contentDisposition builds a Content-Disposition header value that carries +// name faithfully (BUG-3190). +// +// A name of printable ASCII with no quote or backslash is written as +// `filename="name"` alone: byte-identical to the `%q` form every site used, +// which is the same string for such a name. Any other name gets RFC 6266's +// two parameters: an ASCII fallback in `filename`, for clients that do not +// read the extended form, and the exact name in `filename*` (RFC 8187: the +// UTF-8 charset, an empty language tag, the percent-encoded bytes). A client +// that reads both +// prefers `filename*`; Go's mime.ParseMediaType, which the CLI and both MCP +// transports use, does exactly that. +// +// What this replaces was `filename=%q`, which put raw UTF-8 in a parameter +// defined as ISO-8859-1 and, worse, wrote Go escape TEXT for any rune +// strconv.IsPrint rejects: a stored "ab.txt" reached every Go client +// as the characters `a b.txt`, a different name carrying a backslash, +// which a Windows client reads as a path separator. +// +// The caller passes the name it means to serve; sanitising it (the attachment +// download drops control, quote and backslash runes) stays the caller's job. +func contentDisposition(disposition, name string) string { + if plainHeaderFilename(name) { + return disposition + `; filename="` + name + `"` + } + return disposition + `; filename="` + asciiFilenameFallback(name) + `"; filename*=UTF-8''` + encodeRFC8187(name) +} + +// plainHeaderFilename reports whether name can travel as a quoted-string +// with no escaping and no loss: printable ASCII, no quote, no backslash. +func plainHeaderFilename(name string) bool { + for i := 0; i < len(name); i++ { + c := name[i] + if c < 0x20 || c > 0x7e || c == '"' || c == '\\' { + return false + } + } + return true +} + +// asciiFilenameFallback replaces every rune plainHeaderFilename would reject +// with '_', so an ASCII extension survives ("会議.pdf" → "__.pdf"). +func asciiFilenameFallback(name string) string { + var b strings.Builder + b.Grow(len(name)) + for _, r := range name { + if r >= 0x20 && r <= 0x7e && r != '"' && r != '\\' { + b.WriteRune(r) + } else { + b.WriteByte('_') + } + } + return b.String() +} + +// encodeRFC8187 percent-encodes every byte of s that is not an RFC 8187 +// attr-char (ALPHA / DIGIT / "!#$&+-.^_`|~"). +func encodeRFC8187(s string) string { + const hex = "0123456789ABCDEF" + var b strings.Builder + b.Grow(len(s) * 3) + for i := 0; i < len(s); i++ { + c := s[i] + if ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || + strings.IndexByte("!#$&+-.^_`|~", c) >= 0 { + b.WriteByte(c) + continue + } + b.WriteByte('%') + b.WriteByte(hex[c>>4]) + b.WriteByte(hex[c&0x0f]) + } + return b.String() +} diff --git a/internal/server/content_disposition_test.go b/internal/server/content_disposition_test.go new file mode 100644 index 000000000..bc7f423df --- /dev/null +++ b/internal/server/content_disposition_test.go @@ -0,0 +1,77 @@ +package server + +import ( + "encoding/json" + "fmt" + "mime" + "net/http/httptest" + "testing" +) + +// The population: every class a stored name can be in. ASCII that travels +// plain; ASCII the quoted-string form cannot carry bare; runes strconv.IsPrint +// rejects (the `%q` corruption); multi-byte scripts; combining marks; emoji; +// and the characters RFC 8187 encoding and parameter parsing treat specially. +var contentDispositionNames = []string{ + "plain.txt", "with space.txt", "semi;colon.txt", "per%cent.txt", "star*.txt", "tick'.txt", + `quo"te.txt`, `back\slash.txt`, + "a b.txt", "a​b.txt", "a b.txt", + "résumé.pdf", "会議.pdf", "é.txt", "📎 clip.png", + // Characters the extended form must itself percent-encode, beside a rune + // that forces that form: a bare '%' there would decode as an escape. + "50% résumé.txt", "a'b é.txt", "é;x=y.txt", +} + +func TestContentDispositionRoundTripsEveryName(t *testing.T) { + for _, name := range contentDispositionNames { + h := contentDisposition("attachment", name) + disp, params, err := mime.ParseMediaType(h) + if err != nil || disp != "attachment" || params["filename"] != name { + t.Errorf("%q: header %q parsed to (%q, %q, %v)", name, h, disp, params["filename"], err) + } + for i := 0; i < len(h); i++ { + if h[i] < 0x20 || h[i] > 0x7e { + t.Errorf("%q: header %q carries a non-ASCII byte", name, h) + break + } + } + } +} + +// A name that travels plain is byte-identical to the `%q` form every site +// used, so this change is invisible for every ASCII name in the product. +func TestContentDispositionPlainNameUnchanged(t *testing.T) { + for _, name := range []string{"plain.txt", "with space.txt", "my-item.pad.md", "ws-export.tar.gz"} { + if got, was := contentDisposition("inline", name), fmt.Sprintf(`inline; filename=%q`, name); got != was { + t.Errorf("%q: %q, was %q", name, got, was) + } + } + if got := contentDisposition("attachment", "会議.pdf"); got != `attachment; filename="__.pdf"; filename*=UTF-8''%E4%BC%9A%E8%AD%B0.pdf` { + t.Errorf("CJK: %q", got) + } +} + +// Through the real upload and download doors: the name a Go client parses out +// of the download header is the name the server stored. NBSP and ZWSP were +// the measured corruption (BUG-3190 checkpoint 1). +func TestAttachmentDownloadCarriesTheStoredName(t *testing.T) { + srv, slug := testServerWithAttachments(t) + for _, name := range []string{"plain.txt", "résumé.txt", "会議.txt", "a b.txt", "a​b.txt", "é.txt", "per%cent.txt"} { + rr := doMultipartUpload(srv, slug, name, []byte("hello")) + var up struct { + ID string `json:"id"` + Filename string `json:"filename"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &up); err != nil || up.ID == "" { + t.Fatalf("%q: upload %d %s", name, rr.Code, rr.Body.String()) + } + req := httptest.NewRequest("GET", "/api/v1/workspaces/"+slug+"/attachments/"+up.ID, nil) + req.RemoteAddr = "127.0.0.1:1" + dr := httptest.NewRecorder() + srv.ServeHTTP(dr, req) + _, params, err := mime.ParseMediaType(dr.Header().Get("Content-Disposition")) + if err != nil || params["filename"] != up.Filename || up.Filename != name { + t.Errorf("sent %q, stored %q, header %q parsed to %q (%v)", name, up.Filename, dr.Header().Get("Content-Disposition"), params["filename"], err) + } + } +} diff --git a/internal/server/handlers_artifact_export.go b/internal/server/handlers_artifact_export.go index c3fece841..8df57f17e 100644 --- a/internal/server/handlers_artifact_export.go +++ b/internal/server/handlers_artifact_export.go @@ -139,8 +139,7 @@ func (s *Server) handleExportItemArtifact(w http.ResponseWriter, r *http.Request } w.Header().Set("Content-Type", "text/markdown; charset=utf-8") - w.Header().Set("Content-Disposition", - fmt.Sprintf("attachment; filename=%q", artifactExportFilename(item))) + w.Header().Set("Content-Disposition", contentDisposition("attachment", artifactExportFilename(item))) w.WriteHeader(http.StatusOK) _, _ = w.Write(out) } diff --git a/internal/server/handlers_attachments.go b/internal/server/handlers_attachments.go index ac87118a7..4eb2acad9 100644 --- a/internal/server/handlers_attachments.go +++ b/internal/server/handlers_attachments.go @@ -852,7 +852,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(attachments.ServedFilename(att.Filename)))) + contentDisposition(disposition, sanitizeHeaderFilename(attachments.ServedFilename(att.Filename)))) // http.ServeContent gives Range, conditional GETs, and 206 // responses for free — but it requires an io.ReadSeeker. FSStore diff --git a/internal/server/handlers_export_bundle.go b/internal/server/handlers_export_bundle.go index 3176eedf3..a6269204f 100644 --- a/internal/server/handlers_export_bundle.go +++ b/internal/server/handlers_export_bundle.go @@ -141,8 +141,7 @@ func (s *Server) handleExportWorkspaceBundle(w http.ResponseWriter, r *http.Requ } w.Header().Set("Content-Type", "application/gzip") - w.Header().Set("Content-Disposition", - fmt.Sprintf(`attachment; filename="%s-export.tar.gz"`, ws.Slug)) + w.Header().Set("Content-Disposition", contentDisposition("attachment", ws.Slug+"-export.tar.gz")) // Bundles are streamed; we don't know the final size up front. No // Content-Length header — http.Server falls through to chunked // transfer-encoding, which the gzip+tar pair handles fine. diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 57ba4b761..4cba157fb 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -796,7 +796,7 @@ func (s *Server) handleExportWorkspace(w http.ResponseWriter, r *http.Request) { return } - w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s-export.json"`, ws.Slug)) + w.Header().Set("Content-Disposition", contentDisposition("attachment", ws.Slug+"-export.json")) writeJSON(w, http.StatusOK, export) }