diff --git a/server/channels/app/opengraph.go b/server/channels/app/opengraph.go index 769eda7109..0200c24456 100644 --- a/server/channels/app/opengraph.go +++ b/server/channels/app/opengraph.go @@ -14,6 +14,7 @@ import ( "github.com/pkg/errors" "golang.org/x/net/html/charset" + "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/v8/channels/app/oembed" ) @@ -62,6 +63,8 @@ func (a *App) parseOpenGraphMetadata(requestURL string, body io.Reader, contentT openGraphDecodeHTMLEntities(og) + og = filterSVGImagesFromOpenGraph(og) + // If image proxy enabled modify open graph data to feed though proxy if toProxyURL := a.ImageProxyAdder(); toProxyURL != nil { og = openGraphDataWithProxyAddedToImageURLs(og, toProxyURL) @@ -143,6 +146,16 @@ func openGraphDataWithProxyAddedToImageURLs(ogdata *opengraph.OpenGraph, toProxy return ogdata } +// filterSVGImagesFromOpenGraph removes SVG images from OpenGraph metadata. +func filterSVGImagesFromOpenGraph(og *opengraph.OpenGraph) *opengraph.OpenGraph { + if og == nil || len(og.Images) == 0 { + return og + } + + og.Images = model.FilterSVGImages(og.Images) + return og +} + func openGraphDecodeHTMLEntities(og *opengraph.OpenGraph) { og.Title = html.UnescapeString(og.Title) og.Description = html.UnescapeString(og.Description) @@ -169,6 +182,8 @@ func (a *App) parseOpenGraphFromOEmbed(requestURL string, body io.Reader) (*open }) } + og = filterSVGImagesFromOpenGraph(og) + if toProxyURL := a.ImageProxyAdder(); toProxyURL != nil { og = openGraphDataWithProxyAddedToImageURLs(og, toProxyURL) } diff --git a/server/channels/app/opengraph_test.go b/server/channels/app/opengraph_test.go index 2272a85680..66a49dbc5b 100644 --- a/server/channels/app/opengraph_test.go +++ b/server/channels/app/opengraph_test.go @@ -8,8 +8,11 @@ import ( "testing" "github.com/dyatlov/go-opengraph/opengraph" + ogImage "github.com/dyatlov/go-opengraph/opengraph/types/image" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/model" ) func BenchmarkForceHTMLEncodingToUTF8(b *testing.B) { @@ -139,3 +142,183 @@ func TestOpenGraphDecodeHTMLEntities(t *testing.T) { assert.Equal(t, og.Title, "Test's are the best.©") assert.Equal(t, og.Description, "Test's are the worst.©") } + +func TestIsSVGURL(t *testing.T) { + mainHelper.Parallel(t) + testCases := []struct { + name string + url string + expected bool + }{ + { + name: "empty URL", + url: "", + expected: false, + }, + { + name: "PNG image", + url: "https://example.com/image.png", + expected: false, + }, + { + name: "JPEG image", + url: "https://example.com/image.jpg", + expected: false, + }, + { + name: "SVG image lowercase", + url: "https://example.com/image.svg", + expected: true, + }, + { + name: "SVG image uppercase", + url: "https://example.com/image.SVG", + expected: true, + }, + { + name: "SVG image mixed case", + url: "https://example.com/image.Svg", + expected: true, + }, + { + name: "SVGZ compressed SVG", + url: "https://example.com/image.svgz", + expected: true, + }, + { + name: "SVG with query parameters", + url: "https://example.com/image.svg?v=123", + expected: true, + }, + { + name: "SVG with fragment", + url: "https://example.com/image.svg#section", + expected: true, + }, + { + name: "path containing svg but not extension", + url: "https://example.com/svg/image.png", + expected: false, + }, + { + name: "filename containing svg but different extension", + url: "https://example.com/mysvgfile.png", + expected: false, + }, + { + name: "relative SVG path", + url: "/images/icon.svg", + expected: true, + }, + { + name: "invalid URL", + url: "://invalid", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := model.IsSVGImageURL(tc.url) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestFilterSVGImagesFromOpenGraph(t *testing.T) { + mainHelper.Parallel(t) + + t.Run("nil OpenGraph", func(t *testing.T) { + result := filterSVGImagesFromOpenGraph(nil) + assert.Nil(t, result) + }) + + t.Run("empty images", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{} + result := filterSVGImagesFromOpenGraph(og) + assert.Empty(t, result.Images) + }) + + t.Run("filter SVG by URL extension", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{ + {URL: "https://example.com/image.png"}, + {URL: "https://example.com/icon.svg"}, + {URL: "https://example.com/photo.jpg"}, + } + result := filterSVGImagesFromOpenGraph(og) + require.Len(t, result.Images, 2) + assert.Equal(t, "https://example.com/image.png", result.Images[0].URL) + assert.Equal(t, "https://example.com/photo.jpg", result.Images[1].URL) + }) + + t.Run("filter SVG by SecureURL extension", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{ + {SecureURL: "https://example.com/banner.png"}, + {SecureURL: "https://example.com/icon.svg"}, + } + result := filterSVGImagesFromOpenGraph(og) + require.Len(t, result.Images, 1) + assert.Equal(t, "https://example.com/banner.png", result.Images[0].SecureURL) + }) + + t.Run("filter SVG by MIME type", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{ + {URL: "https://example.com/image.png", Type: "image/png"}, + {URL: "https://example.com/image", Type: "image/svg+xml"}, + } + result := filterSVGImagesFromOpenGraph(og) + require.Len(t, result.Images, 1) + assert.Equal(t, "https://example.com/image.png", result.Images[0].URL) + }) + + t.Run("filter SVGZ compressed images", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{ + {URL: "https://example.com/image.png"}, + {URL: "https://example.com/compressed.svgz"}, + } + result := filterSVGImagesFromOpenGraph(og) + require.Len(t, result.Images, 1) + assert.Equal(t, "https://example.com/image.png", result.Images[0].URL) + }) + + t.Run("filter all images when all are SVG", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{ + {URL: "https://example.com/image1.svg"}, + {URL: "https://example.com/image2.svg"}, + } + result := filterSVGImagesFromOpenGraph(og) + assert.Empty(t, result.Images) + }) + + t.Run("skip nil images in slice", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Images = []*ogImage.Image{ + {URL: "https://example.com/image.png"}, + nil, + {URL: "https://example.com/photo.jpg"}, + } + result := filterSVGImagesFromOpenGraph(og) + require.Len(t, result.Images, 2) + }) + + t.Run("preserve non-image OpenGraph fields", func(t *testing.T) { + og := opengraph.NewOpenGraph() + og.Title = "Test Title" + og.Description = "Test Description" + og.URL = "https://example.com" + og.Images = []*ogImage.Image{ + {URL: "https://example.com/icon.svg"}, + } + result := filterSVGImagesFromOpenGraph(og) + assert.Equal(t, "Test Title", result.Title) + assert.Equal(t, "Test Description", result.Description) + assert.Equal(t, "https://example.com", result.URL) + assert.Empty(t, result.Images) + }) +} diff --git a/server/channels/app/post_metadata.go b/server/channels/app/post_metadata.go index 3e49b88f2d..a494ca91e9 100644 --- a/server/channels/app/post_metadata.go +++ b/server/channels/app/post_metadata.go @@ -339,6 +339,18 @@ func (a *App) getEmbedForPost(c request.CTX, post *model.Post, firstLink string, } if image != nil { + // See MM-67372 + if image.Format == "svg" || model.IsSVGImageURL(firstLink) { + c.Logger().Debug("Skipping SVG image embed", + mlog.String("post_id", post.Id), + mlog.String("url", firstLink)) + // Return a link embed instead of an image embed + return &model.PostEmbed{ + Type: model.PostEmbedLink, + URL: firstLink, + }, nil + } + // Note that we're not passing the image info here since it'll be part of the PostMetadata.Images field return &model.PostEmbed{ Type: model.PostEmbedImage, @@ -617,6 +629,24 @@ func (a *App) containsPermalink(rctx request.CTX, post *model.Post) bool { return looksLikeAPermalink(link, a.GetSiteURL()) } +// filterSVGImage filters out SVG images (MM-67372). +// Returns nil if the image is an SVG, otherwise returns the image unchanged. +func filterSVGImage(image *model.PostImage, imageURL string) *model.PostImage { + if image == nil { + return nil + } + + if image.Format == "svg" { + return nil + } + + if model.IsSVGImageURL(imageURL) { + return nil + } + + return image +} + func (a *App) getLinkMetadata(c request.CTX, requestURL string, timestamp int64, isNewPost bool, previewedPostPropVal string) (*opengraph.OpenGraph, *model.PostImage, *model.Permalink, error) { requestURL = resolveMetadataURL(requestURL, a.GetSiteURL()) @@ -634,6 +664,8 @@ func (a *App) getLinkMetadata(c request.CTX, requestURL string, timestamp int64, } if ok && previewedPostPropVal == "" { + og = model.TruncateOpenGraph(og) + image = filterSVGImage(image, requestURL) return og, image, permalink, nil } @@ -641,6 +673,8 @@ func (a *App) getLinkMetadata(c request.CTX, requestURL string, timestamp int64, if !isNewPost { og, image, ok = a.getLinkMetadataFromDatabase(requestURL, timestamp) if ok && previewedPostPropVal == "" { + og = model.TruncateOpenGraph(og) + image = filterSVGImage(image, requestURL) cacheLinkMetadata(c, requestURL, timestamp, og, image, nil) return og, image, nil, nil } @@ -890,12 +924,11 @@ func (a *App) parseLinkMetadata(rctx request.CTX, requestURL string, body io.Rea body = bufRd } - if contentType == "image/svg+xml" { - image := &model.PostImage{ - Format: "svg", - } - - return nil, image, nil + if strings.HasPrefix(contentType, "image/svg+xml") { + // See MM-67372 + rctx.Logger().Debug("Filtering SVG image from link metadata", + mlog.String("url", requestURL)) + return nil, nil, nil } else if strings.HasPrefix(contentType, "image") { image, err := parseImages(rctx, requestURL, io.LimitReader(body, MaxMetadataImageSize)) return nil, image, err diff --git a/server/channels/app/post_metadata_test.go b/server/channels/app/post_metadata_test.go index e67fab8b47..59e50058b5 100644 --- a/server/channels/app/post_metadata_test.go +++ b/server/channels/app/post_metadata_test.go @@ -2763,11 +2763,15 @@ func TestParseLinkMetadata(t *testing.T) { t.Run("svg", func(t *testing.T) { og, dimensions, err := th.App.parseLinkMetadata(th.Context, "http://example.com/image.svg", nil, "image/svg+xml") assert.NoError(t, err) - assert.Nil(t, og) - assert.Equal(t, &model.PostImage{ - Format: "svg", - }, dimensions) + assert.Nil(t, dimensions) + }) + + t.Run("svg with charset", func(t *testing.T) { + og, dimensions, err := th.App.parseLinkMetadata(th.Context, "http://example.com/image.svg", nil, "image/svg+xml; charset=utf-8") + assert.NoError(t, err) + assert.Nil(t, og) + assert.Nil(t, dimensions) }) } diff --git a/server/public/model/link_metadata.go b/server/public/model/link_metadata.go index 4c53784e20..7c742978c8 100644 --- a/server/public/model/link_metadata.go +++ b/server/public/model/link_metadata.go @@ -9,6 +9,8 @@ import ( "fmt" "hash/fnv" "net/http" + "net/url" + "strings" "time" "unicode/utf8" @@ -78,13 +80,49 @@ func TruncateOpenGraph(ogdata *opengraph.OpenGraph) *opengraph.OpenGraph { ogdata.Determiner = empty.Determiner ogdata.Locale = empty.Locale ogdata.LocalesAlternate = empty.LocalesAlternate - ogdata.Images = firstNImages(ogdata.Images, LinkMetadataMaxImages) + ogdata.Images = FilterSVGImages(firstNImages(ogdata.Images, LinkMetadataMaxImages)) ogdata.Audios = empty.Audios ogdata.Videos = empty.Videos } return ogdata } +// FilterSVGImages removes SVG images from the provided list. See MM-67372. +func FilterSVGImages(images []*image.Image) []*image.Image { + if len(images) == 0 { + return images + } + + filtered := make([]*image.Image, 0, len(images)) + for _, img := range images { + if img == nil { + continue + } + // Filter by URL extension + if IsSVGImageURL(img.URL) || IsSVGImageURL(img.SecureURL) { + continue + } + // Filter by declared MIME type + if strings.HasPrefix(img.Type, "image/svg+xml") { + continue + } + filtered = append(filtered, img) + } + return filtered +} + +func IsSVGImageURL(imageURL string) bool { + if imageURL == "" { + return false + } + parsed, err := url.Parse(imageURL) + if err != nil { + return false + } + path := strings.ToLower(parsed.Path) + return strings.HasSuffix(path, ".svg") || strings.HasSuffix(path, ".svgz") +} + func (o *LinkMetadata) PreSave() { o.Hash = GenerateLinkMetadataHash(o.URL, o.Timestamp) } diff --git a/server/public/model/link_metadata_test.go b/server/public/model/link_metadata_test.go index af702a34a7..b4f608f71a 100644 --- a/server/public/model/link_metadata_test.go +++ b/server/public/model/link_metadata_test.go @@ -341,3 +341,149 @@ func TestTruncateOpenGraph(t *testing.T) { assert.Equal(t, utf8.RuneCountInString(result.Description), 305, "Description text is truncated") assert.Equal(t, utf8.RuneCountInString(result.SiteName), 305, "SiteName text is truncated") } + +func TestTruncateOpenGraphFiltersSVGImages(t *testing.T) { + og := opengraph.OpenGraph{ + Type: "website", + URL: "http://example.com", + Title: "Test", + Images: []*image.Image{ + sampleImage("image.png"), + sampleImage("icon.svg"), + sampleImage("photo.jpg"), + sampleImage("compressed.svgz"), + }, + } + result := TruncateOpenGraph(&og) + assert.Len(t, result.Images, 2, "SVG images should be filtered out") + assert.Equal(t, "http://example.com/image.png", result.Images[0].URL) + assert.Equal(t, "http://example.com/photo.jpg", result.Images[1].URL) +} + +func TestIsSVGImageURL(t *testing.T) { + testCases := []struct { + name string + url string + expected bool + }{ + { + name: "empty URL", + url: "", + expected: false, + }, + { + name: "PNG image", + url: "https://example.com/image.png", + expected: false, + }, + { + name: "JPEG image", + url: "https://example.com/image.jpg", + expected: false, + }, + { + name: "SVG image lowercase", + url: "https://example.com/image.svg", + expected: true, + }, + { + name: "SVG image uppercase", + url: "https://example.com/image.SVG", + expected: true, + }, + { + name: "SVGZ compressed SVG", + url: "https://example.com/image.svgz", + expected: true, + }, + { + name: "SVG with query parameters", + url: "https://example.com/image.svg?v=123", + expected: true, + }, + { + name: "path containing svg but not extension", + url: "https://example.com/svg/image.png", + expected: false, + }, + { + name: "invalid URL", + url: "://invalid", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := IsSVGImageURL(tc.url) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestFilterSVGImages(t *testing.T) { + t.Run("empty slice", func(t *testing.T) { + result := FilterSVGImages([]*image.Image{}) + assert.Empty(t, result) + }) + + t.Run("filter SVG by URL", func(t *testing.T) { + images := []*image.Image{ + sampleImage("image.png"), + sampleImage("icon.svg"), + sampleImage("photo.jpg"), + } + result := FilterSVGImages(images) + assert.Len(t, result, 2) + assert.Equal(t, "http://example.com/image.png", result[0].URL) + assert.Equal(t, "http://example.com/photo.jpg", result[1].URL) + }) + + t.Run("filter SVG by SecureURL", func(t *testing.T) { + images := []*image.Image{ + {SecureURL: "https://example.com/banner.png"}, + {SecureURL: "https://example.com/icon.svg"}, + } + result := FilterSVGImages(images) + assert.Len(t, result, 1) + assert.Equal(t, "https://example.com/banner.png", result[0].SecureURL) + }) + + t.Run("filter SVG by MIME type", func(t *testing.T) { + images := []*image.Image{ + {URL: "https://example.com/image.png", Type: "image/png"}, + {URL: "https://example.com/image", Type: "image/svg+xml"}, + } + result := FilterSVGImages(images) + assert.Len(t, result, 1) + assert.Equal(t, "https://example.com/image.png", result[0].URL) + }) + + t.Run("filter SVG by MIME type with charset", func(t *testing.T) { + images := []*image.Image{ + {URL: "https://example.com/image", Type: "image/svg+xml; charset=utf-8"}, + } + result := FilterSVGImages(images) + assert.Len(t, result, 0) + }) + + t.Run("filter SVGZ", func(t *testing.T) { + images := []*image.Image{ + sampleImage("image.png"), + sampleImage("compressed.svgz"), + } + result := FilterSVGImages(images) + assert.Len(t, result, 1) + assert.Equal(t, "http://example.com/image.png", result[0].URL) + }) + + t.Run("skip nil images", func(t *testing.T) { + images := []*image.Image{ + sampleImage("image.png"), + nil, + sampleImage("photo.jpg"), + } + result := FilterSVGImages(images) + assert.Len(t, result, 2) + }) +}