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/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..a0e3048b9 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" @@ -73,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"`)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"; filename*=UTF-8''My%20Mixtape.zip`)) Expect(w.Header().Get("Content-Type")).To(Equal("application/zip")) Expect(archiver.called).To(BeTrue()) Expect(w.Body.String()).To(Equal("zip-contents")) @@ -84,7 +85,7 @@ var _ = Describe("handleDownloads", func() { w := makeRequest("abc123") - Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"`)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"; filename*=UTF-8''abc123.zip`)) }) It("sanitizes characters that are unsafe in a filename", func() { @@ -92,7 +93,19 @@ var _ = Describe("handleDownloads", func() { w := makeRequest("abc123") - Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`)) + 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`)) + }) + + 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() { 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..c45d6c084 --- /dev/null +++ b/utils/str/content_disposition.go @@ -0,0 +1,88 @@ +package str + +import ( + "fmt" + "strings" +) + +// 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" + +// 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. +func ContentDispositionAttachment(filename string) string { + return fmt.Sprintf("attachment; filename=%q; filename*=UTF-8''%s", + asciiFilename(filename), encodeRFC5987(filename)) +} + +// 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) { + switch { + case r == ',': + // Some clients split the header value on commas + sb.WriteByte('_') + case r >= ' ' && r <= '~': + sb.WriteRune(r) + } + } + name := strings.Trim(sb.String(), " .") + if name == "" { + return fallbackFilename + } + return name +} + +// 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]) + } + } + 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 +} diff --git a/utils/str/content_disposition_test.go b/utils/str/content_disposition_test.go new file mode 100644 index 000000000..28cf9ba49 --- /dev/null +++ b/utils/str/content_disposition_test.go @@ -0,0 +1,108 @@ +package str_test + +import ( + "bufio" + "mime" + "net/http" + "net/http/httptest" + "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 plain (quoted, ASCII) filename parameter, the one used by +// clients that don't understand 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. +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() { + // 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")) + }) + + It("keeps the header parseable, with the attack contained in the value", func() { + Expect(decodedName(attack)).To(Equal(attack)) + }) + + 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"`)) + }) + }) + + 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")) + }) + + It("strips path separators and control characters", func() { + Expect(asciiName("../../etc/passwd\x00\t")).To(Equal("_.._etc_passwd")) + }) + + It("replaces commas, which some clients treat as value separators", func() { + Expect(asciiName("Bowie, David.zip")).To(Equal("Bowie_ David.zip")) + }) + + It("drops non-ASCII runes", func() { + Expect(asciiName("Legião Urbana.zip")).To(Equal("Legio Urbana.zip")) + }) + + It("falls back to a placeholder when nothing ASCII survives", func() { + Expect(asciiName("東京")).To(Equal("download")) + }) + + It("trims leading and trailing dots and spaces", func() { + Expect(asciiName(" ..hidden.. ")).To(Equal("hidden")) + }) + }) + + 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"), + ) + }) +})