Merge 028515a4176b00bebbdfd424a578ac18610a650b into 3867fab4da6ea7142bfc6374c94b34a708e03b4e

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 {

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)

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() {

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

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
+}

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"),
+		)
+	})
+})