diff --git a/cmd/media.go b/cmd/media.go index fb1234b0..b539d8c4 100644 --- a/cmd/media.go +++ b/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()))) diff --git a/cmd/media_test.go b/cmd/media_test.go index 3cc8f86a..b8234bff 100644 --- a/cmd/media_test.go +++ b/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(``) + 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) + } + }) + } +} diff --git a/internal/media/media.go b/internal/media/media.go index af1ce5b0..999d538f 100644 --- a/internal/media/media.go +++ b/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 diff --git a/internal/media/models/models.go b/internal/media/models/models.go index 4d63e4d0..e2e3ceb4 100644 --- a/internal/media/models/models.go +++ b/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 +} diff --git a/internal/media/models/models_test.go b/internal/media/models/models_test.go new file mode 100644 index 00000000..3e29626d --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/media/stores/s3/s3.go b/internal/media/stores/s3/s3.go index db8ad0f1..8a8d43d8 100644 --- a/internal/media/stores/s3/s3.go +++ b/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), } diff --git a/internal/media/stores/s3/s3_test.go b/internal/media/stores/s3/s3_test.go new file mode 100644 index 00000000..23f6e259 --- /dev/null +++ b/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 := `` + 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") + } + }) + } +}