From a3f41fb422dee23036fd45ead569f0c278a036a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20S=C3=A1nchez=20Zapico?= Date: Wed, 23 Sep 2026 15:47:16 +0200 Subject: [PATCH] sec(server): sanitize user-controlled filenames in Content-Disposition (#5895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * sec(server): sanitize user-controlled filenames in Content-Disposition Playlist export, Subsonic download and public share download built the Content-Disposition header by interpolating a user-controlled name into a quoted-string with fmt.Sprintf. A name containing a double quote closes the string early and the rest is parsed as additional parameters, so a playlist named `party"; filename="evil.html` yielded attachment; filename="party"; filename="evil.html.m3u" letting whoever chose the name decide what the browser saves the download as. The names come from playlists, album/artist names and media file tags. Go's net/http already rewrites CR and LF in header values to spaces, so response splitting was not reachable; parameter injection was. Add str.ContentDispositionAttachment, which emits a sanitized ASCII-only quoted `filename` plus an RFC 5987 `filename*` carrying the original UTF-8 name, and use it at all four call sites. The `filename*` parameter also fixes non-ASCII names, which previously went out raw or were mangled by sanitizing. Signed-off-by: zapisanchez * fix(server): keep download names intact and sanitize filename* Rework ContentDispositionAttachment after review. Names with no ASCII letters now fall back to download. instead of a bare extension (東京.mp3 gave filename="mp3"). filename* is built from the same sanitized name as the ASCII fallback, so path separators, reserved characters, control and bidi characters, and invalid UTF-8 no longer reach it. The ASCII fallback transliterates accents and typographic punctuation (Legião -> Legiao, She’s -> She's) through the existing sanitize.Accents and str.Clear helpers, keeps leading dots, and only trims trailing ones. Names are capped at 255 bytes, keeping the extension. Pure ASCII names now get only the quoted filename parameter, so the header for them matches the previous output byte for byte. filename* is encoded with mime.FormatMediaType instead of a hand-written RFC 5987 encoder. Adds tests for the M3U export and Subsonic download headers. * fix(server): handle dot-only names and long fake extensions A name made only of dots trimmed down to an empty filename. It now falls back to download, like an empty stem does. path.Ext treats anything after the last dot as the extension, so a long suffix with no real extension was kept whole and replaced the stem with download, going past the 255-byte cap. Suffixes longer than 16 bytes are now treated as part of the stem and truncated with it. Neither case is reachable from the current call sites, which always append a short extension. --------- Signed-off-by: zapisanchez Co-authored-by: Deluan --- server/nativeapi/playlists.go | 4 +- server/nativeapi/playlists_test.go | 40 ++++++ server/public/handle_downloads.go | 7 +- server/public/handle_downloads_test.go | 21 +++ .../e2e/subsonic_media_retrieval_test.go | 4 +- server/subsonic/stream.go | 10 +- utils/str/content_disposition.go | 80 ++++++++++++ utils/str/content_disposition_test.go | 120 ++++++++++++++++++ 8 files changed, 271 insertions(+), 15 deletions(-) create mode 100644 utils/str/content_disposition.go create mode 100644 utils/str/content_disposition_test.go diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index d215eb9dd..00c1575a5 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" + "github.com/navidrome/navidrome/utils/str" ) type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc @@ -101,8 +102,7 @@ func handleExportPlaylist(pls playlists.Playlists) http.HandlerFunc { log.Debug(ctx, "Exporting playlist as M3U", "playlistId", plsId, "name", playlist.Name) w.Header().Set("Content-Type", "audio/x-mpegurl") - disposition := fmt.Sprintf("attachment; filename=\"%s.m3u\"", playlist.Name) - w.Header().Set("Content-Disposition", disposition) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(playlist.Name+".m3u")) _, err = w.Write([]byte(playlist.ToM3U8())) //nolint:gosec if err != nil { diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 2f6c578f9..349b4a662 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/deluan/rest" + "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" @@ -183,6 +184,37 @@ var _ = Describe("Playlist Tracks Endpoint", func() { }) }) +var _ = Describe("handleExportPlaylist", func() { + export := func(name string) *httptest.ResponseRecorder { + r := chi.NewRouter() + r.Get("/playlist/{playlistId}", handleExportPlaylist(&mockPlaylistsService{ + playlist: &model.Playlist{ID: "pls-1", Name: name}, + })) + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest("GET", "/playlist/pls-1", nil)) + return w + } + + It("names the download after the playlist", func() { + w := export("Road Trip") + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="Road Trip.m3u"`)) + }) + + It("does not let the playlist name inject a second filename parameter", func() { + w := export(`party"; filename="evil.html`) + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="party_; filename=_evil.html.m3u"`)) + }) + + It("keeps non-ASCII names in filename*", func() { + w := export("Кино") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="download.m3u"; filename*=utf-8''%D0%9A%D0%B8%D0%BD%D0%BE.m3u`)) + }) +}) + var _ = Describe("writePlaylistError", func() { DescribeTable("maps a service error to an HTTP status", func(err error, expected int) { @@ -231,6 +263,7 @@ func (m *mockPlaylistTrackRepo) Read(id string) (any, error) { type mockPlaylistsService struct { playlists.Playlists tracksRepo rest.Repository + playlist *model.Playlist removeImageFn func(ctx context.Context, id string) error setImageFn func(ctx context.Context, id string, reader io.Reader, ext string) error } @@ -249,6 +282,13 @@ func (m *mockPlaylistsService) SetImage(ctx context.Context, id string, reader i return model.ErrNotFound } +func (m *mockPlaylistsService) GetWithTracks(_ context.Context, _ string) (*model.Playlist, error) { + if m.playlist == nil { + return nil, model.ErrNotFound + } + return m.playlist, nil +} + func (m *mockPlaylistsService) TracksRepository(_ context.Context, _ string, _ bool) rest.Repository { return m.tracksRepo } diff --git a/server/public/handle_downloads.go b/server/public/handle_downloads.go index 0012c4b35..e50d7609a 100644 --- a/server/public/handle_downloads.go +++ b/server/public/handle_downloads.go @@ -2,9 +2,7 @@ package public import ( "cmp" - "fmt" "net/http" - "strings" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" @@ -31,9 +29,8 @@ func (pub *Router) handleDownloads(w http.ResponseWriter, r *http.Request) { return } - name := str.SanitizeFilename(cmp.Or(s.Description, s.ID)) - name = strings.ReplaceAll(name, ",", "_") - w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", name+".zip")) + name := cmp.Or(s.Description, s.ID) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(name+".zip")) w.Header().Set("Content-Type", "application/zip") err = pub.archiver.ZipShare(ctx, s, w) diff --git a/server/public/handle_downloads_test.go b/server/public/handle_downloads_test.go index 1a97f4379..d2118f47e 100644 --- a/server/public/handle_downloads_test.go +++ b/server/public/handle_downloads_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "mime" "net/http" "net/http/httptest" "time" @@ -95,6 +96,26 @@ var _ = Describe("handleDownloads", func() { Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`)) }) + It("sanitizes the UTF-8 filename* as well", func() { + shareIs(&model.Share{ID: "abc123", Description: "Sigur Rós/Live", Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="Sigur Ros_Live.zip"; filename*=utf-8''Sigur%20R%C3%B3s_Live.zip`)) + }) + + It("does not let the share description inject a second filename parameter", func() { + shareIs(&model.Share{ID: "abc123", Description: `mix"; filename="evil.html`, Downloadable: true}) + + w := makeRequest("abc123") + + disposition := w.Header().Get("Content-Disposition") + Expect(disposition).ToNot(ContainSubstring(`filename="evil.html`)) + _, params, err := mime.ParseMediaType(disposition) + Expect(err).ToNot(HaveOccurred()) + Expect(params["filename"]).To(Equal(`mix_; filename=_evil.html.zip`)) + }) + It("returns 403 without invoking the archiver when the share is not downloadable", func() { shareIs(&model.Share{ID: "abc123", Description: "No Download", Downloadable: false}) diff --git a/server/subsonic/e2e/subsonic_media_retrieval_test.go b/server/subsonic/e2e/subsonic_media_retrieval_test.go index 079b131ff..268d93b82 100644 --- a/server/subsonic/e2e/subsonic_media_retrieval_test.go +++ b/server/subsonic/e2e/subsonic_media_retrieval_test.go @@ -106,7 +106,7 @@ var _ = Describe("Media Retrieval Endpoints", Ordered, func() { }) Describe("Download", func() { - var trackID string + var trackID, trackTitle string BeforeAll(func() { // All test tracks are mp3 at 320kbps @@ -114,6 +114,7 @@ var _ = Describe("Media Retrieval Endpoints", Ordered, func() { Expect(err).ToNot(HaveOccurred()) Expect(songs).ToNot(BeEmpty()) trackID = songs[0].ID + trackTitle = songs[0].Title }) It("returns error when id parameter is missing", func() { @@ -143,6 +144,7 @@ var _ = Describe("Media Retrieval Endpoints", Ordered, func() { Expect(w.Code).To(Equal(http.StatusOK)) Expect(streamerSpy.LastRequest.Format).To(Equal("opus")) Expect(streamerSpy.LastRequest.BitRate).To(Equal(128)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="` + trackTitle + `.opus"`)) }) It("returns error when downloads are disabled", func() { diff --git a/server/subsonic/stream.go b/server/subsonic/stream.go index 28b4585f0..b4a6b821c 100644 --- a/server/subsonic/stream.go +++ b/server/subsonic/stream.go @@ -3,10 +3,8 @@ package subsonic import ( "context" "errors" - "fmt" "net/http" "strconv" - "strings" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/stream" @@ -15,6 +13,7 @@ import ( "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/utils/req" + "github.com/navidrome/navidrome/utils/str" ) func (api *Router) Stream(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) { @@ -94,9 +93,7 @@ func (api *Router) Download(w http.ResponseWriter, r *http.Request) (*responses. } setHeaders := func(name string) { - name = strings.ReplaceAll(name, ",", "_") - disposition := fmt.Sprintf("attachment; filename=\"%s.zip\"", name) - w.Header().Set("Content-Disposition", disposition) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(name+".zip")) w.Header().Set("Content-Type", "application/zip") } @@ -115,8 +112,7 @@ func (api *Router) Download(w http.ResponseWriter, r *http.Request) (*responses. } }() - disposition := fmt.Sprintf("attachment; filename=\"%s\"", stream.Name()) - w.Header().Set("Content-Disposition", disposition) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(stream.Name())) _, err = stream.Serve(ctx, w, r) return nil, err diff --git a/utils/str/content_disposition.go b/utils/str/content_disposition.go new file mode 100644 index 000000000..34ae8a5c6 --- /dev/null +++ b/utils/str/content_disposition.go @@ -0,0 +1,80 @@ +package str + +import ( + "cmp" + "fmt" + "mime" + "path" + "strings" + "unicode" + "unicode/utf8" + + "github.com/deluan/sanitize" +) + +const ( + maxFilenameBytes = 255 + maxExtensionBytes = 16 + fallbackFilename = "download" +) + +// ContentDispositionAttachment builds an RFC 6266 attachment header value for a user-controlled filename. +// Non-ASCII names also get a filename* parameter, which clients prefer over the ASCII fallback. +func ContentDispositionAttachment(filename string) string { + stem, ext := splitFilename(filename) + header := fmt.Sprintf("attachment; filename=%q", joinFilename(toASCII(stem), toASCII(ext))) + name := joinFilename(stem, ext) + if isASCII(name) { + return header + } + // FormatMediaType emits a percent-encoded filename* for non-ASCII values + extended := mime.FormatMediaType("attachment", map[string]string{"filename": name}) + return header + strings.TrimPrefix(extended, "attachment") +} + +func splitFilename(filename string) (stem, ext string) { + name := strings.Map(func(r rune) rune { + if unicode.IsControl(r) || unicode.Is(unicode.Bidi_Control, r) { + return -1 + } + return r + }, SanitizeFilename(strings.ToValidUTF8(filename, "_"))) + ext = path.Ext(name) + if len(ext) > maxExtensionBytes { + ext = "" + } + stem = strings.TrimSuffix(name, ext) + if limit := maxFilenameBytes - len(ext); len(stem) > limit { + for limit > 0 && !utf8.RuneStart(stem[limit]) { + limit-- + } + stem = stem[:limit] + } + return stem, ext +} + +func joinFilename(stem, ext string) string { + stem = cmp.Or(strings.TrimSpace(stem), fallbackFilename) + return cmp.Or(strings.TrimRight(stem+ext, " ."), fallbackFilename) +} + +func toASCII(s string) string { + return strings.Map(func(r rune) rune { + switch { + case r == ',': + return '_' + case r >= ' ' && r <= '~': + return r + } + return -1 + }, sanitize.Accents(SanitizeFilename(Clear(s)))) +} + +func isASCII(s string) bool { + for i := 0; i < len(s); i++ { + if s[i] >= utf8.RuneSelf { + return false + } + } + return true +} diff --git a/utils/str/content_disposition_test.go b/utils/str/content_disposition_test.go new file mode 100644 index 000000000..944392a4e --- /dev/null +++ b/utils/str/content_disposition_test.go @@ -0,0 +1,120 @@ +package str_test + +import ( + "mime" + "regexp" + "strings" + + "github.com/navidrome/navidrome/utils/str" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var asciiFilenameRe = regexp.MustCompile(`(?:^|; )filename="([^"]*)"`) + +// asciiName returns the quoted filename parameter, the one used by clients that ignore filename*. +func asciiName(filename string) string { + m := asciiFilenameRe.FindAllStringSubmatch(str.ContentDispositionAttachment(filename), -1) + ExpectWithOffset(1, m).To(HaveLen(1), "expected exactly one quoted filename parameter") + return m[0][1] +} + +// decodedName returns the name an RFC 6266 client picks, preferring filename* when present. +func decodedName(filename string) string { + disp, params, err := mime.ParseMediaType(str.ContentDispositionAttachment(filename)) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + ExpectWithOffset(1, disp).To(Equal("attachment")) + return params["filename"] +} + +var _ = Describe("ContentDispositionAttachment", func() { + Describe("parameter injection", func() { + const attack = `party"; filename="evil.html.m3u` + + It("does not let a quote in the name open a second parameter", func() { + Expect(str.ContentDispositionAttachment(attack)).To(Equal(`attachment; filename="party_; filename=_evil.html.m3u"`)) + }) + + It("keeps the header parseable", func() { + Expect(decodedName(attack)).To(Equal("party_; filename=_evil.html.m3u")) + }) + + It("does not let a quote in a non-ASCII name inject either", func() { + const utf8Attack = `東京"; filename="evil.html.m3u` + header := str.ContentDispositionAttachment(utf8Attack) + Expect(strings.Count(header, `"`)).To(Equal(2)) + Expect(decodedName(utf8Attack)).To(Equal("東京_; filename=_evil.html.m3u")) + }) + }) + + Describe("ASCII names", func() { + It("sends only the quoted filename, unchanged", func() { + Expect(str.ContentDispositionAttachment("My Playlist.m3u")).To(Equal(`attachment; filename="My Playlist.m3u"`)) + }) + + It("keeps leading dots", func() { + Expect(asciiName("...And Justice for All.zip")).To(Equal("...And Justice for All.zip")) + }) + + It("trims surrounding spaces and trailing dots", func() { + Expect(asciiName(" Greatest Hits .zip")).To(Equal("Greatest Hits.zip")) + Expect(asciiName("Loose End. ")).To(Equal("Loose End")) + }) + + It("falls back to a placeholder when only dots are left", func() { + Expect(str.ContentDispositionAttachment("...")).To(Equal(`attachment; filename="download"`)) + }) + + It("caps a long name whose last dot is not an extension", func() { + name := asciiName("a." + strings.Repeat("x", 300)) + Expect(len(name)).To(BeNumerically("<=", 255)) + Expect(name).To(HavePrefix("a.xxx")) + }) + + It("replaces path separators and reserved characters", func() { + Expect(asciiName("AC/DC: Live, 1979?.zip")).To(Equal("AC_DC_ Live_ 1979_.zip")) + }) + + It("drops control characters", func() { + Expect(asciiName("line\r\nbreak\x00\t.mp3")).To(Equal("linebreak.mp3")) + }) + }) + + Describe("non-ASCII names", func() { + It("transliterates accents in the ASCII fallback", func() { + Expect(asciiName("Legião Urbana.zip")).To(Equal("Legiao Urbana.zip")) + }) + + It("converts typographic punctuation in the ASCII fallback", func() { + Expect(asciiName("She’s a Woman — Live.mp3")).To(Equal("She's a Woman - Live.mp3")) + Expect(asciiName("“Heroes”.mp3")).To(Equal("_Heroes_.mp3")) + Expect(decodedName("She’s a Woman.mp3")).To(Equal("She’s a Woman.mp3")) + }) + + It("keeps the extension when no ASCII letters survive", func() { + Expect(asciiName("東京.mp3")).To(Equal("download.mp3")) + Expect(asciiName("Кино.m3u")).To(Equal("download.m3u")) + Expect(asciiName("東京")).To(Equal("download")) + }) + + DescribeTable("filename* carries the sanitized UTF-8 name", + func(filename, expected string) { + Expect(decodedName(filename)).To(Equal(expected)) + }, + Entry("accented", "Legião Urbana.zip", "Legião Urbana.zip"), + Entry("CJK", "東京.zip", "東京.zip"), + Entry("emoji", "🎵 mix.zip", "🎵 mix.zip"), + Entry("comma and percent", "Sigur Rós, 100%.zip", "Sigur Rós, 100%.zip"), + Entry("path separators", "Sigur Rós/Live: Heima.zip", "Sigur Rós_Live_ Heima.zip"), + Entry("control characters", "Sigur Rós\r\n\x00.zip", "Sigur Rós.zip"), + Entry("bidi override", "Björk\u202Eexe.mp3", "Björkexe.mp3"), + Entry("invalid UTF-8", "Bj\xf6rk Café.mp3", "Bj_rk Café.mp3"), + ) + + It("caps the name length and keeps the extension", func() { + name := decodedName(strings.Repeat("東", 300) + ".zip") + Expect(len(name)).To(BeNumerically("<=", 255)) + Expect(name).To(HaveSuffix("東.zip")) + }) + }) +})