MM-67372: Improve link preview metadata handling and filtering (#35178) (#35222)

Automatic Merge

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

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

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" {
+	if strings.HasPrefix(contentType, "image/svg+xml") {
-		image := &model.PostImage{
+		// See MM-67372
-			Format: "svg",
+		rctx.Logger().Debug("Filtering SVG image from link metadata",
-		}
+			mlog.String("url", requestURL))
-
+		return nil, nil, nil
-		return nil, image, nil
 	} else if strings.HasPrefix(contentType, "image") {
 		image, err := parseImages(rctx, requestURL, io.LimitReader(body, MaxMetadataImageSize))
 		return nil, image, err

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{
+		assert.Nil(t, dimensions)
-			Format: "svg",
+	})
-		}, 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)
 	})
 }
 

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

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