Browse Source

fix(http): sanitize filename in Content-Disposition header

Use mime.FormatMediaType to properly encode the filename parameter,
preventing header injection via unescaped double quotes in proxied
media URLs.
Frédéric Guillot 6 hours ago
parent
commit
75753ce8b3
3 changed files with 107 additions and 3 deletions
  1. 20 1
      internal/http/response/builder.go
  2. 85 0
      internal/http/response/builder_test.go
  3. 2 2
      internal/ui/proxy.go

+ 20 - 1
internal/http/response/builder.go

@@ -8,6 +8,7 @@ import (
 	"compress/gzip"
 	"io"
 	"log/slog"
+	"mime"
 	"net/http"
 	"strings"
 	"time"
@@ -64,7 +65,13 @@ func (b *Builder) WithBodyAsReader(body io.Reader) *Builder {
 
 // WithAttachment forces the document to be downloaded by the web browser.
 func (b *Builder) WithAttachment(filename string) *Builder {
-	b.headers["Content-Disposition"] = "attachment; filename=" + filename
+	b.headers["Content-Disposition"] = formatContentDisposition("attachment", filename)
+	return b
+}
+
+// WithInline suggests an inline filename for the current response.
+func (b *Builder) WithInline(filename string) *Builder {
+	b.headers["Content-Disposition"] = formatContentDisposition("inline", filename)
 	return b
 }
 
@@ -181,3 +188,15 @@ func ifNoneMatch(headerValue, etag string) bool {
 	// Weak ETag comparison: the opaque-tag (quoted string without W/ prefix) must match.
 	return strings.Contains(headerValue, strings.TrimPrefix(etag, `W/`))
 }
+
+func formatContentDisposition(dispositionType, filename string) string {
+	if filename == "" {
+		return dispositionType
+	}
+
+	if value := mime.FormatMediaType(dispositionType, map[string]string{"filename": filename}); value != "" {
+		return value
+	}
+
+	return dispositionType
+}

+ 85 - 0
internal/http/response/builder_test.go

@@ -5,6 +5,7 @@ package response // import "miniflux.app/v2/internal/http/response"
 
 import (
 	"bytes"
+	"mime"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -105,6 +106,90 @@ func TestBuildResponseWithAttachment(t *testing.T) {
 	}
 }
 
+func TestBuildResponseWithAttachmentEscapesFilename(t *testing.T) {
+	r, err := http.NewRequest("GET", "/", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	w := httptest.NewRecorder()
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		NewBuilder(w, r).WithAttachment(`a";filename="malware.exe`).Write()
+	})
+
+	handler.ServeHTTP(w, r)
+	resp := w.Result()
+
+	actual := resp.Header.Get("Content-Disposition")
+	mediaType, params, err := mime.ParseMediaType(actual)
+	if err != nil {
+		t.Fatalf("Unexpected parse error for %q: %v", actual, err)
+	}
+
+	if mediaType != "attachment" {
+		t.Fatalf(`Unexpected media type, got %q instead of %q`, mediaType, "attachment")
+	}
+
+	if params["filename"] != `a";filename="malware.exe` {
+		t.Fatalf(`Unexpected filename, got %q instead of %q`, params["filename"], `a";filename="malware.exe`)
+	}
+}
+
+func TestBuildResponseWithInlineEscapesFilename(t *testing.T) {
+	r, err := http.NewRequest("GET", "/", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	w := httptest.NewRecorder()
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		NewBuilder(w, r).WithInline(`a";filename="malware.exe`).Write()
+	})
+
+	handler.ServeHTTP(w, r)
+	resp := w.Result()
+
+	actual := resp.Header.Get("Content-Disposition")
+	mediaType, params, err := mime.ParseMediaType(actual)
+	if err != nil {
+		t.Fatalf("Unexpected parse error for %q: %v", actual, err)
+	}
+
+	if mediaType != "inline" {
+		t.Fatalf(`Unexpected media type, got %q instead of %q`, mediaType, "inline")
+	}
+
+	if params["filename"] != `a";filename="malware.exe` {
+		t.Fatalf(`Unexpected filename, got %q instead of %q`, params["filename"], `a";filename="malware.exe`)
+	}
+}
+
+func TestFormatContentDisposition(t *testing.T) {
+	tests := []struct {
+		name            string
+		dispositionType string
+		filename        string
+		expected        string
+	}{
+		{"empty filename returns bare type", "inline", "", "inline"},
+		{"simple filename", "attachment", "photo.jpg", `attachment; filename=photo.jpg`},
+		{"filename with double quote", "inline", `a";filename="malware.exe`, `inline; filename="a\";filename=\"malware.exe"`},
+		{"filename with spaces", "attachment", "my file.txt", `attachment; filename="my file.txt"`},
+		{"non-ASCII filename", "attachment", "café.png", `attachment; filename*=utf-8''caf%C3%A9.png`},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			actual := formatContentDisposition(tt.dispositionType, tt.filename)
+			if actual != tt.expected {
+				t.Fatalf(`formatContentDisposition(%q, %q) = %q, want %q`, tt.dispositionType, tt.filename, actual, tt.expected)
+			}
+		})
+	}
+}
+
 func TestBuildResponseWithByteBody(t *testing.T) {
 	r, err := http.NewRequest("GET", "/", nil)
 	if err != nil {

+ 2 - 2
internal/ui/proxy.go

@@ -149,8 +149,8 @@ func (h *handler) mediaProxy(w http.ResponseWriter, r *http.Request) {
 		b.WithHeader("Content-Security-Policy", response.ContentSecurityPolicyForUntrustedContent)
 		b.WithHeader("Content-Type", resp.Header.Get("Content-Type"))
 
-		if filename := path.Base(parsedMediaURL.Path); filename != "" {
-			b.WithHeader("Content-Disposition", `inline; filename="`+filename+`"`)
+		if filename := path.Base(parsedMediaURL.Path); filename != "" && filename != "." && filename != "/" {
+			b.WithInline(filename)
 		}
 
 		forwardedResponseHeader := [...]string{"Content-Encoding", "Content-Type", "Content-Length", "Accept-Ranges", "Content-Range"}