Merge pull request #641 from abhinavxd/fix-svg-uploads

stop uploaded SVG files from running scripts

cmd/media.go

@@ -255,22 +255,19 @@ func serveMediaFile(r *fastglue.Request, app *App, uuid string, media *mmodels.M
 	consts := app.consts.Load().(*constants)
 	switch consts.UploadProvider {
 	case "fs":
-		disposition := "attachment"
-
-		// Inline images/videos/pdfs. SVG excluded.
-		if !forceDownload &&
-			media.ContentType != "image/svg+xml" &&
-			(strings.HasPrefix(media.ContentType, "image/") ||
-				strings.HasPrefix(media.ContentType, "video/") ||
-				media.ContentType == "application/pdf") {
-			disposition = "inline"
+		// Older rows hold content types exactly as the client sent them.
+		contentType := mmodels.NormalizeContentType(media.ContentType)
+		disposition := mmodels.ContentDisposition(contentType)
+		if forceDownload {
+			disposition = mmodels.DispositionAttachment
 		}
 
-		r.RequestCtx.Response.Header.Set("Content-Type", media.ContentType)
+		r.RequestCtx.Response.Header.Set("Content-Type", contentType)
 		r.RequestCtx.Response.Header.Set("Content-Disposition", mime.FormatMediaType(disposition, map[string]string{"filename": media.Filename}))
 		r.RequestCtx.Response.Header.Set("X-Content-Type-Options", "nosniff")
-		// Sandbox SVGs.
-		if media.ContentType == "image/svg+xml" {
+		// Chrome's video and PDF viewers break in a sandbox, and inline types run no script anyway.
+		// Downloads are sandboxed for webviews that render attachments inline.
+		if disposition == mmodels.DispositionAttachment {
 			r.RequestCtx.Response.Header.Set("Content-Security-Policy", "sandbox")
 		}
 		r.RequestCtx.Response.Header.Set("Cache-Control", fmt.Sprintf("%s, max-age=%d, immutable", cacheVisibility(media.Private), int(mediaCacheTTL.Seconds())))

cmd/media_test.go

@@ -3,7 +3,14 @@ package main
 import (
 	"bytes"
 	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
+
+	mmodels "github.com/abhinavxd/libredesk/internal/media/models"
+	"github.com/valyala/fasthttp"
+	"github.com/zerodha/fastglue"
 )
 
 func TestPrepareImageUploadAllowsOversizedImageWithoutThumbnail(t *testing.T) {
@@ -31,3 +38,63 @@ func TestPrepareImageUploadAllowsOversizedImageWithoutThumbnail(t *testing.T) {
 		t.Fatalf("dimensions = %dx%d, want 10000x5000", meta.Width, meta.Height)
 	}
 }
+
+func TestServeMediaFileSandboxesDownloads(t *testing.T) {
+	const uuid = "0b7a3c1e-2f4d-4b8a-9c6e-1d2f3a4b5c6d"
+	dir := t.TempDir()
+	svg := []byte(`<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>`)
+	if err := os.WriteFile(filepath.Join(dir, uuid), svg, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := ko.Set("upload.fs.upload_path", dir); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { ko.Delete("upload.fs.upload_path") })
+
+	app := &App{}
+	app.consts.Store(&constants{UploadProvider: "fs"})
+
+	tests := []struct {
+		name            string
+		contentType     string
+		download        bool
+		wantType        string
+		wantDisposition string
+		wantSandbox     bool
+	}{
+		{"svg", "image/svg+xml", false, "image/svg+xml", "attachment", true},
+		{"svg with bad param", "image/svg+xml; x", false, "image/svg+xml", "attachment", true},
+		{"other xml image", "image/x+xml", false, "image/x+xml", "attachment", true},
+		{"unparseable", "text/plain,image/svg+xml", false, "application/octet-stream", "attachment", true},
+		{"html", "text/html", false, "text/html", "attachment", true},
+		{"png", "image/png", false, "image/png", "inline", false},
+		{"png forced download", "image/png", true, "image/png", "attachment", true},
+		{"video", "video/mp4", false, "video/mp4", "inline", false},
+		{"video forced download", "video/mp4", true, "video/mp4", "attachment", true},
+		{"pdf", "application/pdf", false, "application/pdf", "inline", false},
+		{"pdf forced download", "application/pdf", true, "application/pdf", "attachment", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := &fastglue.Request{RequestCtx: &fasthttp.RequestCtx{}, Context: app}
+			if tt.download {
+				req.RequestCtx.Request.SetRequestURI("/uploads/" + uuid + "?download=1")
+			}
+			media := &mmodels.Media{ContentType: tt.contentType, Filename: "file", Private: true}
+			if err := serveMediaFile(req, app, uuid, media); err != nil {
+				t.Fatal(err)
+			}
+
+			h := &req.RequestCtx.Response.Header
+			if got := string(h.ContentType()); got != tt.wantType {
+				t.Fatalf("Content-Type = %q, want %q", got, tt.wantType)
+			}
+			if got := string(h.Peek("Content-Disposition")); !strings.HasPrefix(got, tt.wantDisposition) {
+				t.Fatalf("Content-Disposition = %q, want %s", got, tt.wantDisposition)
+			}
+			if got := string(h.Peek("Content-Security-Policy")) == "sandbox"; got != tt.wantSandbox {
+				t.Fatalf("sandboxed = %v, want %v", got, tt.wantSandbox)
+			}
+		})
+	}
+}

internal/media/media.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"mime"
 	"net/http"
 	"net/url"
 	"os"
@@ -260,19 +259,11 @@ func (m *Manager) GetBlob(name string) ([]byte, error) {
 
 // GetURL returns the URL for accessing a media file by its name.
 func (m *Manager) GetURL(uuid, contentType, fileName string) string {
-	// Keep some content types inline. SVG excluded.
-	disposition := "attachment"
-	if contentType != "image/svg+xml" &&
-		(strings.HasPrefix(contentType, "image/") ||
-			strings.HasPrefix(contentType, "video/") ||
-			contentType == "application/pdf") {
-		disposition = "inline"
-	}
-	return m.store.GetURL(uuid, disposition, fileName)
+	return m.store.GetURL(uuid, models.ContentDisposition(contentType), fileName)
 }
 
 func (m *Manager) GetURLForDownload(uuid, fileName string) string {
-	return m.store.GetURL(uuid, "attachment", fileName)
+	return m.store.GetURL(uuid, models.DispositionAttachment, fileName)
 }
 
 // GetSignedURL generates a signed URL for secure media access if the store supports it.
@@ -299,7 +290,7 @@ func (m *Manager) GetThumbnailURL(uuid string) string {
 			return u.String()
 		}
 	}
-	return m.store.GetURL(image.ThumbPrefix+uuid, "inline", "")
+	return m.store.GetURL(image.ThumbPrefix+uuid, models.DispositionInline, "" /** fileName **/)
 }
 
 // SignedURLValidator returns the store's signature validator if available.
@@ -415,18 +406,10 @@ func (m *Manager) deleteUnlinkedRows(stmt *sqlx.Stmt) error {
 // For generic types, it uses http.DetectContentType (stdlib) as a fast path,
 // falling back to mimetype library for deeper inspection using magic numbers.
 func (m *Manager) detectContentType(sourceContentType string, content io.ReadSeeker) (string, error) {
-	// Set default if empty
-	if sourceContentType == "" {
-		sourceContentType = "application/octet-stream"
-	}
-
-	// Handle "image/svg+xml; charset=utf-8", keep just the type.
-	if mediaType, _, err := mime.ParseMediaType(sourceContentType); err == nil && mediaType != "" {
-		sourceContentType = mediaType
-	}
+	sourceContentType = models.NormalizeContentType(sourceContentType)
 
 	// Trust source unless it's a generic/useless type
-	if sourceContentType != "application/octet-stream" &&
+	if sourceContentType != models.ContentTypeOctetStream &&
 		sourceContentType != "application/data" &&
 		sourceContentType != "application/binary" {
 		m.lo.Debug("detected media content type from trusted source", "detected_type", sourceContentType)
@@ -443,7 +426,7 @@ func (m *Manager) detectContentType(sourceContentType string, content io.ReadSee
 
 	// If stdlib gives a useful type, use it.
 	// stdlib defaults to application/octet-stream for unknown types.
-	if detected != "application/octet-stream" {
+	if detected != models.ContentTypeOctetStream {
 		content.Seek(0, io.SeekStart)
 		m.lo.Debug("detected media content type using stdlib", "detected_type", detected, "source_type", sourceContentType)
 		return detected, nil

internal/media/models/models.go

@@ -2,6 +2,9 @@ package models
 
 import (
 	"encoding/json"
+	"errors"
+	"mime"
+	"strings"
 	"time"
 
 	"github.com/volatiletech/null/v9"
@@ -13,7 +16,11 @@ const (
 	ModelUser         = "users"
 	ModelHelpArticles = "help_articles"
 
-	DispositionInline = "inline"
+	DispositionInline     = "inline"
+	DispositionAttachment = "attachment"
+
+	ContentTypeOctetStream = "application/octet-stream"
+	ContentTypePDF         = "application/pdf"
 )
 
 // IsPublicModel reports whether media linked to the model type is served without authentication.
@@ -42,3 +49,24 @@ type Media struct {
 	URL     string `json:"url"`
 	Content []byte `json:"-"`
 }
+
+// NormalizeContentType returns the lowercased type/subtype of a Content-Type value, or application/octet-stream if it does not parse.
+func NormalizeContentType(contentType string) string {
+	mediaType, _, err := mime.ParseMediaType(contentType)
+	if (err != nil && !errors.Is(err, mime.ErrInvalidMediaParameter)) || !strings.Contains(mediaType, "/") {
+		return ContentTypeOctetStream
+	}
+	return mediaType
+}
+
+// ContentDisposition returns inline for images, video and PDF. XML types such as SVG can run scripts and are always attachments.
+func ContentDisposition(contentType string) string {
+	contentType = NormalizeContentType(contentType)
+	if strings.HasSuffix(contentType, "+xml") {
+		return DispositionAttachment
+	}
+	if strings.HasPrefix(contentType, "image/") || strings.HasPrefix(contentType, "video/") || contentType == ContentTypePDF {
+		return DispositionInline
+	}
+	return DispositionAttachment
+}

internal/media/models/models_test.go

@@ -0,0 +1,57 @@
+package models
+
+import "testing"
+
+func TestNormalizeContentType(t *testing.T) {
+	tests := []struct {
+		in   string
+		want string
+	}{
+		{"image/png", "image/png"},
+		{"IMAGE/SVG+XML", "image/svg+xml"},
+		{"image/svg+xml; charset=utf-8", "image/svg+xml"},
+		{"image/svg+xml; x", "image/svg+xml"},
+		{"Image/Svg+Xml; x", "image/svg+xml"},
+		{`image/svg+xml ; a="b`, "image/svg+xml"},
+		{"text/plain,image/svg+xml", ContentTypeOctetStream},
+		{"image", ContentTypeOctetStream},
+		{"", ContentTypeOctetStream},
+		{"garbage", ContentTypeOctetStream},
+	}
+	for _, tt := range tests {
+		t.Run(tt.in, func(t *testing.T) {
+			if got := NormalizeContentType(tt.in); got != tt.want {
+				t.Fatalf("NormalizeContentType(%q) = %q, want %q", tt.in, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestContentDisposition(t *testing.T) {
+	tests := []struct {
+		in   string
+		want string
+	}{
+		{"image/png", DispositionInline},
+		{"video/mp4", DispositionInline},
+		{"application/pdf", DispositionInline},
+		{"Application/PDF; x", DispositionInline},
+		{"image/svg+xml", DispositionAttachment},
+		{"image/svg+xml; x", DispositionAttachment},
+		{"IMAGE/SVG+XML", DispositionAttachment},
+		{"image/x+xml", DispositionAttachment},
+		{"video/x+xml", DispositionAttachment},
+		{"application/xhtml+xml", DispositionAttachment},
+		{"text/html", DispositionAttachment},
+		{"text/xml", DispositionAttachment},
+		{"text/plain,image/svg+xml", DispositionAttachment},
+		{"", DispositionAttachment},
+	}
+	for _, tt := range tests {
+		t.Run(tt.in, func(t *testing.T) {
+			if got := ContentDisposition(tt.in); got != tt.want {
+				t.Fatalf("ContentDisposition(%q) = %q, want %q", tt.in, got, tt.want)
+			}
+		})
+	}
+}

internal/media/stores/s3/s3.go

@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/abhinavxd/libredesk/internal/media"
+	"github.com/abhinavxd/libredesk/internal/media/models"
 	"github.com/rhnvrm/simples3"
 )
 
@@ -67,11 +68,13 @@ func New(opt Opt) (media.Store, error) {
 // Put uploads a file to S3 with the specified name, content type, and file content.
 // It returns the name of the file or an error if the upload fails.
 func (c *Client) Put(name string, cType string, file io.ReadSeeker) (string, error) {
+	cType = models.NormalizeContentType(cType)
 	p := simples3.UploadInput{
-		Bucket:      c.opts.Bucket,
-		ContentType: cType,
-		FileName:    name,
-		Body:        file,
+		Bucket:             c.opts.Bucket,
+		ContentType:        cType,
+		ContentDisposition: models.ContentDisposition(cType),
+		FileName:           name,
+		Body:               file,
 		// Paths inside the bucket should not start with /.
 		ObjectKey: c.makeBucketPath(name),
 	}

internal/media/stores/s3/s3_test.go

@@ -0,0 +1,136 @@
+package s3
+
+import (
+	"io"
+	"mime"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/abhinavxd/libredesk/internal/media/models"
+	"github.com/google/uuid"
+)
+
+func TestPutStoresSafeContentHeaders(t *testing.T) {
+	tests := []struct {
+		contentType string
+		wantType    string
+		disposition string
+	}{
+		{"image/svg+xml", "image/svg+xml", "attachment"},
+		{"IMAGE/SVG+XML; x", "image/svg+xml", "attachment"},
+		{`image/svg+xml; charset="unterminated`, "image/svg+xml", "attachment"},
+		{"image/svg+xml; charset=utf-8; charset=ascii", "application/octet-stream", "attachment"},
+		{"text/plain,image/svg+xml", "application/octet-stream", "attachment"},
+		{"image/x+xml", "image/x+xml", "attachment"},
+		{"text/html", "text/html", "attachment"},
+		{"", "application/octet-stream", "attachment"},
+		{"image/png", "image/png", "inline"},
+		{"video/mp4", "video/mp4", "inline"},
+		{"application/pdf", "application/pdf", "inline"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.contentType, func(t *testing.T) {
+			body := `<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>`
+			requests := make(chan http.Header, 1)
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if r.Method != http.MethodPut || r.URL.Path != "/test-bucket/uploads/file" {
+					t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+				}
+				got, err := io.ReadAll(r.Body)
+				if err != nil || string(got) != body {
+					t.Errorf("uploaded body = %q, error = %v", got, err)
+				}
+				requests <- r.Header.Clone()
+				w.WriteHeader(http.StatusOK)
+			}))
+			defer server.Close()
+
+			store, err := New(Opt{
+				URL:        server.URL,
+				PublicURL:  "https://cdn.example.com",
+				AccessKey:  uuid.NewString(),
+				SecretKey:  uuid.NewString(),
+				Region:     "test-region",
+				Bucket:     "test-bucket",
+				BucketPath: "uploads",
+				BucketType: "private",
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if _, err := store.Put("file", tt.contentType, strings.NewReader(body)); err != nil {
+				t.Fatal(err)
+			}
+
+			headers := <-requests
+			if got := headers.Get("Content-Type"); got != tt.wantType {
+				t.Errorf("Content-Type = %q, want %q", got, tt.wantType)
+			}
+			if got := headers.Get("Content-Disposition"); got != tt.disposition {
+				t.Errorf("Content-Disposition = %q, want %q", got, tt.disposition)
+			}
+		})
+	}
+}
+
+func TestGetURLPreservesPublicURLs(t *testing.T) {
+	tests := []struct {
+		name        string
+		bucketType  string
+		publicURL   string
+		disposition string
+		wantHost    string
+		wantPath    string
+		wantSigned  bool
+	}{
+		{"private download", "private", "", models.DispositionAttachment, "storage.example.com", "/test-bucket/uploads/existing-file", true},
+		{"private inline", "private", "", models.DispositionInline, "storage.example.com", "/test-bucket/uploads/existing-file", true},
+		{"private custom download", "private", "https://cdn.example.com", models.DispositionAttachment, "cdn.example.com", "/uploads/existing-file", false},
+		{"private custom inline", "private", "https://cdn.example.com", models.DispositionInline, "cdn.example.com", "/uploads/existing-file", false},
+		{"public download", "public", "", models.DispositionAttachment, "storage.example.com", "/test-bucket/uploads/existing-file", false},
+		{"public inline", "public", "", models.DispositionInline, "storage.example.com", "/test-bucket/uploads/existing-file", false},
+		{"public custom download", "public", "https://cdn.example.com", models.DispositionAttachment, "cdn.example.com", "/uploads/existing-file", false},
+		{"public custom inline", "public", "https://cdn.example.com", models.DispositionInline, "cdn.example.com", "/uploads/existing-file", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			store, err := New(Opt{
+				URL:        "https://storage.example.com",
+				PublicURL:  tt.publicURL,
+				AccessKey:  uuid.NewString(),
+				SecretKey:  uuid.NewString(),
+				Region:     "test-region",
+				Bucket:     "test-bucket",
+				BucketPath: "uploads",
+				BucketType: tt.bucketType,
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			filename := "customer attachment.svg"
+			u, err := url.Parse(store.GetURL("existing-file", tt.disposition, filename))
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if u.Host != tt.wantHost || u.Path != tt.wantPath {
+				t.Errorf("URL target = %s%s, want %s%s", u.Host, u.Path, tt.wantHost, tt.wantPath)
+			}
+			query := u.Query()
+			if got := query.Get("X-Amz-Signature") != ""; got != tt.wantSigned {
+				t.Errorf("signed URL = %v, want %v", got, tt.wantSigned)
+			}
+			if tt.wantSigned {
+				got, params, err := mime.ParseMediaType(query.Get("response-content-disposition"))
+				if err != nil || got != tt.disposition || params["filename"] != filename {
+					t.Errorf("download disposition = %q, filename = %q, error = %v", got, params["filename"], err)
+				}
+			} else if u.RawQuery != "" {
+				t.Error("public URL has unexpected query parameters")
+			}
+		})
+	}
+}