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_test.go b/server/public/handle_downloads_test.go index a0e3048b9..d2118f47e 100644 --- a/server/public/handle_downloads_test.go +++ b/server/public/handle_downloads_test.go @@ -74,7 +74,7 @@ var _ = Describe("handleDownloads", func() { w := makeRequest("abc123") Expect(w.Code).To(Equal(http.StatusOK)) - Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"; filename*=UTF-8''My%20Mixtape.zip`)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"`)) Expect(w.Header().Get("Content-Type")).To(Equal("application/zip")) Expect(archiver.called).To(BeTrue()) Expect(w.Body.String()).To(Equal("zip-contents")) @@ -85,7 +85,7 @@ var _ = Describe("handleDownloads", func() { w := makeRequest("abc123") - Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"; filename*=UTF-8''abc123.zip`)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"`)) }) It("sanitizes characters that are unsafe in a filename", func() { @@ -93,7 +93,15 @@ var _ = Describe("handleDownloads", func() { w := makeRequest("abc123") - Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"; filename*=UTF-8''AC%2FDC%3A%20Live%2C%201979.zip`)) + 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() { @@ -105,7 +113,7 @@ var _ = Describe("handleDownloads", func() { 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`)) + Expect(params["filename"]).To(Equal(`mix_; filename=_evil.html.zip`)) }) It("returns 403 without invoking the archiver when the share is not downloadable", func() { 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/utils/str/content_disposition.go b/utils/str/content_disposition.go index c45d6c084..22a28fdbd 100644 --- a/utils/str/content_disposition.go +++ b/utils/str/content_disposition.go @@ -1,88 +1,73 @@ package str import ( + "cmp" "fmt" + "mime" + "path" "strings" + "unicode" + "unicode/utf8" + + "github.com/deluan/sanitize" ) -// fallbackFilename is used when sanitizing leaves nothing usable (e.g. a name made -// entirely of non-ASCII runes). Clients that understand filename* still get the real -// name; this is only the ASCII fallback. -const fallbackFilename = "download" +const maxFilenameBytes = 255 -// ContentDispositionAttachment builds a Content-Disposition header value (RFC 6266) -// for an attachment download named filename. -// -// Filenames here are derived from user-controlled data — playlist names, album and -// artist names, and titles read from media file tags — so they must never be -// interpolated into the header as-is. A name containing a double quote closes the -// quoted-string early and the remainder is parsed as additional parameters, so a -// playlist named -// -// party"; filename="evil.html -// -// would produce `attachment; filename="party"; filename="evil.html.m3u"`, letting -// whoever chose the name control what the browser saves the download as. Go's -// net/http rewrites CR and LF in header values to spaces, so response splitting is -// not reachable this way, but parameter injection is. -// -// The `filename` parameter is reduced to a quoted, ASCII-only token and the original -// UTF-8 name is preserved in an RFC 5987 `filename*` parameter, which RFC 6266 §4.3 -// requires clients to prefer when both are present. Without `filename*`, sanitizing -// to ASCII would mangle every non-Latin name. +// 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 { - return fmt.Sprintf("attachment; filename=%q; filename*=UTF-8''%s", - asciiFilename(filename), encodeRFC5987(filename)) + 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") } -// asciiFilename reduces filename to a token that is safe inside a quoted-string: the -// characters SanitizeFilename already strips (including the double quote), plus -// commas, control characters and everything outside printable ASCII. Leading and -// trailing spaces and dots are trimmed as well, since they are not valid in Windows -// filenames. -func asciiFilename(filename string) string { - var sb strings.Builder - sb.Grow(len(filename)) - for _, r := range SanitizeFilename(filename) { +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) + stem = strings.TrimSuffix(name, ext) + if limit := max(maxFilenameBytes-len(ext), 0); 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), "download") + return strings.TrimRight(stem+ext, " .") +} + +func toASCII(s string) string { + return strings.Map(func(r rune) rune { switch { case r == ',': - // Some clients split the header value on commas - sb.WriteByte('_') + return '_' case r >= ' ' && r <= '~': - sb.WriteRune(r) + return r } - } - name := strings.Trim(sb.String(), " .") - if name == "" { - return fallbackFilename - } - return name + return -1 + }, sanitize.Accents(SanitizeFilename(Clear(s)))) } -// encodeRFC5987 percent-encodes filename for use as an ext-value, escaping every byte -// outside the attr-char set defined in RFC 5987 §3.2.1. -func encodeRFC5987(filename string) string { - const upperhex = "0123456789ABCDEF" - var sb strings.Builder - sb.Grow(len(filename)) - for i := 0; i < len(filename); i++ { - if c := filename[i]; isAttrChar(c) { - sb.WriteByte(c) - } else { - sb.WriteByte('%') - sb.WriteByte(upperhex[c>>4]) - sb.WriteByte(upperhex[c&0x0F]) +func isASCII(s string) bool { + for i := 0; i < len(s); i++ { + if s[i] >= utf8.RuneSelf { + return false } } - return sb.String() -} - -// isAttrChar reports whether c is an attr-char: RFC 5987 §3.2.1 defines it as -// ALPHA / DIGIT / "!" / "#" / "$" / "&" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" -func isAttrChar(c byte) bool { - switch { - case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': - return true - } - return strings.IndexByte("!#$&+-.^_`|~", c) >= 0 + return true } diff --git a/utils/str/content_disposition_test.go b/utils/str/content_disposition_test.go index 28cf9ba49..2a768a1ca 100644 --- a/utils/str/content_disposition_test.go +++ b/utils/str/content_disposition_test.go @@ -1,10 +1,7 @@ package str_test import ( - "bufio" "mime" - "net/http" - "net/http/httptest" "regexp" "strings" @@ -15,16 +12,14 @@ import ( var asciiFilenameRe = regexp.MustCompile(`(?:^|; )filename="([^"]*)"`) -// asciiName returns the plain (quoted, ASCII) filename parameter, the one used by -// clients that don't understand 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 what a client sees after parsing the header, which per -// RFC 6266 §4.3 means the filename* value wins when both are present. +// 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()) @@ -34,75 +29,82 @@ func decodedName(filename string) string { var _ = Describe("ContentDispositionAttachment", func() { Describe("parameter injection", func() { - // Without escaping, this name yields: - // attachment; filename="party"; filename="evil.html.m3u" const attack = `party"; filename="evil.html.m3u` It("does not let a quote in the name open a second parameter", func() { - Expect(asciiName(attack)).To(Equal("party_; filename=_evil.html.m3u")) + Expect(str.ContentDispositionAttachment(attack)).To(Equal(`attachment; filename="party_; filename=_evil.html.m3u"`)) }) - It("keeps the header parseable, with the attack contained in the value", func() { - Expect(decodedName(attack)).To(Equal(attack)) + It("keeps the header parseable", func() { + Expect(decodedName(attack)).To(Equal("party_; filename=_evil.html.m3u")) }) - It("emits no quotes beyond the ones delimiting the ASCII name", func() { - Expect(strings.Count(str.ContentDispositionAttachment(attack), `"`)).To(Equal(2)) - }) - - It("survives a round trip through net/http", func() { - rec := httptest.NewRecorder() - rec.Header().Set("Content-Disposition", str.ContentDispositionAttachment(`x"; filename="y.exe`)) - rec.WriteHeader(http.StatusOK) - - var sb strings.Builder - w := bufio.NewWriter(&sb) - Expect(rec.Result().Write(w)).To(Succeed()) - Expect(w.Flush()).To(Succeed()) - - Expect(sb.String()).ToNot(ContainSubstring(`filename="y.exe"`)) + 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 filename parameter", func() { - It("leaves a name that is already safe untouched", func() { - Expect(asciiName("My Playlist.m3u")).To(Equal("My Playlist.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("strips path separators and control characters", func() { - Expect(asciiName("../../etc/passwd\x00\t")).To(Equal("_.._etc_passwd")) + It("keeps leading dots", func() { + Expect(asciiName("...And Justice for All.zip")).To(Equal("...And Justice for All.zip")) }) - It("replaces commas, which some clients treat as value separators", func() { - Expect(asciiName("Bowie, David.zip")).To(Equal("Bowie_ David.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("drops non-ASCII runes", func() { - Expect(asciiName("Legião Urbana.zip")).To(Equal("Legio Urbana.zip")) + It("replaces path separators and reserved characters", func() { + Expect(asciiName("AC/DC: Live, 1979?.zip")).To(Equal("AC_DC_ Live_ 1979_.zip")) }) - It("falls back to a placeholder when nothing ASCII survives", func() { + 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")) }) - It("trims leading and trailing dots and spaces", func() { - Expect(asciiName(" ..hidden.. ")).To(Equal("hidden")) + 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")) }) }) - - Describe("filename* parameter", func() { - DescribeTable("round-trips the original name", - func(filename string) { - Expect(decodedName(filename)).To(Equal(filename)) - }, - Entry("plain ASCII", "My Playlist.m3u"), - Entry("accented", "Legião Urbana.zip"), - Entry("CJK", "東京.zip"), - Entry("emoji", "🎵 mix.zip"), - Entry("comma", "Bowie, David.zip"), - Entry("quote", `12" Mixes.zip`), - Entry("percent", "100% Hits.zip"), - Entry("semicolon", "a;b.zip"), - ) - }) })