MM-67872 Fixing detection issue in image proxy (#35669)

* Refactoring image proxy * Improving resilience of peeking mechanism * Moving should304 check * Replacing functions with text encoding library
2026-08-27 05:37:15 -05:00 · 2026-03-19 15:12:19 +02:00
parent 76b8e3f5f7
commit ad03248cd3
2 changed files with 179 additions and 10 deletions
@@ -19,12 +19,15 @@ import (
 	"strings"
 	"time"

+	"golang.org/x/text/encoding/unicode"
+	"golang.org/x/text/transform"
+
 	"github.com/mattermost/mattermost/server/public/shared/mlog"
 )

 var imageContentTypes = []string{
 	"image/bmp", "image/cgm", "image/g3fax", "image/gif", "image/ief", "image/jp2",
-	"image/jpeg", "image/jpg", "image/pict", "image/png", "image/prs.btif", "image/svg+xml",
+	"image/jpeg", "image/jpg", "image/pict", "image/png", "image/prs.btif",
 	"image/tiff", "image/vnd.adobe.photoshop", "image/vnd.djvu", "image/vnd.dwg",
 	"image/vnd.dxf", "image/vnd.fastbidsheet", "image/vnd.fpx", "image/vnd.fst",
 	"image/vnd.fujixerox.edmics-mmr", "image/vnd.fujixerox.edmics-rlc",
@@ -162,22 +165,30 @@ func (backend *LocalBackend) ServeImage(w http.ResponseWriter, req *http.Request

 	copyHeader(w.Header(), resp.Header, "Cache-Control", "Last-Modified", "Expires", "Etag", "Link")

-	if should304(req, resp) {
-		w.WriteHeader(http.StatusNotModified)
+	// Wrap the body in a bufio.Reader so we can peek at bytes for
+	// content-type detection without consuming the stream.
+	b := bufio.NewReaderSize(resp.Body, contentPeekSize)
+	resp.Body = io.NopCloser(b)
+
+	if isSVGContent(b) {
+		http.Error(w, msgNotAllowed, http.StatusForbidden)
 		return
 	}

 	contentType, _, _ := mime.ParseMediaType(resp.Header.Get("Content-Type"))
 	if contentType == "" || contentType == "application/octet-stream" || contentType == "binary/octet-stream" {
-		// try to detect content type
-		b := bufio.NewReader(resp.Body)
-		resp.Body = io.NopCloser(b)
 		contentType = peekContentType(b)
 	}
 	if resp.ContentLength != 0 && !contentTypeMatches(imageContentTypes, contentType) {
 		http.Error(w, msgNotAllowed, http.StatusForbidden)
 		return
 	}
+
+	if should304(req, resp) {
+		w.WriteHeader(http.StatusNotModified)
+		return
+	}
+
 	w.Header().Set("Content-Type", contentType)

 	copyHeader(w.Header(), resp.Header, "Content-Length")
@@ -248,6 +259,41 @@ func peekContentType(p *bufio.Reader) string {
 	return http.DetectContentType(byt)
 }

+// contentPeekSize is the number of bytes read ahead for content inspection.
+// It must match the bufio.Reader buffer size created in ServeImage.
+const contentPeekSize = 8192
+
+// isSVGContent peeks at the first contentPeekSize bytes of p and reports whether
+// they contain SVG markers. UTF-16 encoded content (identified by a BOM) is
+// decoded to ASCII before scanning.
+func isSVGContent(p *bufio.Reader) bool {
+	byt, err := p.Peek(contentPeekSize)
+	if err != nil && err != bufio.ErrBufferFull && err != io.EOF {
+		return false
+	}
+	if len(byt) == 0 {
+		return false
+	}
+
+	// UseBOM selects endianness from a BOM when present (0xFF 0xFE → LE,
+	// 0xFE 0xFF → BE), defaulting to LE otherwise.
+	enc := unicode.UTF16(unicode.LittleEndian, unicode.UseBOM)
+	if decoded, _, decodeErr := transform.Bytes(enc.NewDecoder(), byt); decodeErr == nil {
+		lower := strings.ToLower(string(decoded))
+		if strings.Contains(lower, "<svg") ||
+			(strings.Contains(lower, "<?xml") && strings.Contains(lower, "<svg")) {
+			return true
+		}
+	}
+
+	// Raw-byte scan for UTF-8 / ASCII content; interleaved-NUL patterns cover BOM-less UTF-16 BE.
+	rawLower := strings.ToLower(string(byt))
+	return strings.Contains(rawLower, "<svg") ||
+		(strings.Contains(rawLower, "<?xml") && strings.Contains(rawLower, "<svg")) ||
+		strings.Contains(rawLower, "<\x00s\x00v\x00g\x00") || // BOM-less UTF-16 LE
+		strings.Contains(rawLower, "\x00<\x00s\x00v\x00g") // BOM-less UTF-16 BE
+}
+
 // contentTypeMatches returns whether contentType matches one of the allowed patterns.
 func contentTypeMatches(patterns []string, contentType string) bool {
 	if len(patterns) == 0 {
@@ -4,6 +4,7 @@
 package imageproxy

 import (
+	"bytes"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -166,7 +167,7 @@ func TestLocalBackend_GetImage(t *testing.T) {
 		wait <- true
 	})

-	t.Run("SVG attachment", func(t *testing.T) {
+	t.Run("unsupported SVG content type", func(t *testing.T) {
 		handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			w.Header().Set("Cache-Control", "max-age=2592000, private")
 			w.Header().Set("Content-Type", "image/svg+xml")
@@ -187,11 +188,133 @@ func TestLocalBackend_GetImage(t *testing.T) {
 		proxy.GetImage(recorder, request, mock.URL+"/test.svg")
 		resp := recorder.Result()

-		assert.Equal(t, http.StatusOK, resp.StatusCode)
-		assert.Equal(t, "attachment;filename=\"test.svg\"", resp.Header.Get("Content-Disposition"))
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	})

-		_, err = io.ReadAll(resp.Body)
+	t.Run("SVG body with image/png content type", func(t *testing.T) {
+		body := []byte(`<svg xmlns="http://www.w3.org/2000/svg"><rect width="100" height="100"/></svg>`)
+		handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "image/png")
+
+			w.WriteHeader(http.StatusOK)
+			w.Write(body)
+		})
+
+		mock := httptest.NewServer(handler)
+		defer mock.Close()
+
+		proxy := makeTestLocalProxy()
+
+		recorder := httptest.NewRecorder()
+		request, err := http.NewRequest(http.MethodGet, "", nil)
 		require.NoError(t, err)
+		proxy.GetImage(recorder, request, mock.URL+"/image.png")
+		resp := recorder.Result()
+
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	})
+
+	t.Run("XML-based SVG with image/png content type", func(t *testing.T) {
+		body := []byte(`<?xml version="1.0" encoding="UTF-8"?><svg xmlns="http://www.w3.org/2000/svg"><rect/></svg>`)
+		handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "image/png")
+
+			w.WriteHeader(http.StatusOK)
+			w.Write(body)
+		})
+
+		mock := httptest.NewServer(handler)
+		defer mock.Close()
+
+		proxy := makeTestLocalProxy()
+
+		recorder := httptest.NewRecorder()
+		request, err := http.NewRequest(http.MethodGet, "", nil)
+		require.NoError(t, err)
+		proxy.GetImage(recorder, request, mock.URL+"/image.png")
+		resp := recorder.Result()
+
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	})
+
+	t.Run("UTF-16 LE BOM SVG with image/png content type", func(t *testing.T) {
+		// Build a UTF-16 LE payload with BOM: 0xFF 0xFE followed by each ASCII
+		// character of the SVG tag as a two-byte little-endian code unit.
+		svgASCII := `<svg xmlns="http://www.w3.org/2000/svg"><rect/></svg>`
+		body := []byte{0xFF, 0xFE} // UTF-16 LE BOM
+		for _, c := range svgASCII {
+			body = append(body, byte(c), 0x00)
+		}
+		handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "image/png")
+			w.WriteHeader(http.StatusOK)
+			w.Write(body)
+		})
+
+		mock := httptest.NewServer(handler)
+		defer mock.Close()
+
+		proxy := makeTestLocalProxy()
+
+		recorder := httptest.NewRecorder()
+		request, err := http.NewRequest(http.MethodGet, "", nil)
+		require.NoError(t, err)
+		proxy.GetImage(recorder, request, mock.URL+"/image.png")
+		resp := recorder.Result()
+
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	})
+
+	t.Run("UTF-16 BE BOM SVG with image/png content type", func(t *testing.T) {
+		// Build a UTF-16 BE payload with BOM: 0xFE 0xFF followed by each ASCII
+		// character as a two-byte big-endian code unit.
+		svgASCII := `<svg xmlns="http://www.w3.org/2000/svg"><rect/></svg>`
+		body := []byte{0xFE, 0xFF} // UTF-16 BE BOM
+		for _, c := range svgASCII {
+			body = append(body, 0x00, byte(c))
+		}
+		handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "image/png")
+			w.WriteHeader(http.StatusOK)
+			w.Write(body)
+		})
+
+		mock := httptest.NewServer(handler)
+		defer mock.Close()
+
+		proxy := makeTestLocalProxy()
+
+		recorder := httptest.NewRecorder()
+		request, err := http.NewRequest(http.MethodGet, "", nil)
+		require.NoError(t, err)
+		proxy.GetImage(recorder, request, mock.URL+"/image.png")
+		resp := recorder.Result()
+
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	})
+
+	t.Run("SVG body with leading whitespace prefix", func(t *testing.T) {
+		prefix := bytes.Repeat([]byte(" "), 600)
+		body := append(prefix, []byte(`<svg xmlns="http://www.w3.org/2000/svg"><rect/></svg>`)...)
+		handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Content-Type", "image/png")
+
+			w.WriteHeader(http.StatusOK)
+			w.Write(body)
+		})
+
+		mock := httptest.NewServer(handler)
+		defer mock.Close()
+
+		proxy := makeTestLocalProxy()
+
+		recorder := httptest.NewRecorder()
+		request, err := http.NewRequest(http.MethodGet, "", nil)
+		require.NoError(t, err)
+		proxy.GetImage(recorder, request, mock.URL+"/image.png")
+		resp := recorder.Result()
+
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
 	})

 	t.Run("Redirect", func(t *testing.T) {