Drop attachment requirements for media download (#646)

Chooses whether to use an inline or attachment Content-Disposition
header based on the type of media being downloaded.

internal/data/data.go

@@ -7,3 +7,6 @@ var MatrixPng []byte
 
 //go:embed large.png
 var LargePng []byte
+
+//go:embed matrix-logo.svg
+var MatrixSvg []byte

tests/media_filename_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/matrix-org/complement/internal/b"
 	"github.com/matrix-org/complement/internal/client"
 	"github.com/matrix-org/complement/internal/data"
+	"github.com/matrix-org/complement/runtime"
 )
 
 const asciiFileName = "ascii"
@@ -43,8 +44,9 @@ func TestMediaFilenames(t *testing.T) {
 
 					mxcUri := alice.UploadContent(t, data.MatrixPng, filename, "image/png")
 
-					name := downloadForFilename(t, alice, mxcUri, "")
+					name, _ := downloadForFilename(t, alice, mxcUri, "")
 
+					// filename is not required, but if it's an attachment then check it matches
 					if name != filename {
 						t.Fatalf("Incorrect filename '%s', expected '%s'", name, filename)
 					}
@@ -55,10 +57,11 @@ func TestMediaFilenames(t *testing.T) {
 			t.Run("Can download specifying a different ASCII file name", func(t *testing.T) {
 				t.Parallel()
 
-				mxcUri := alice.UploadContent(t, data.MatrixPng, "test.png", "image/png")
+				mxcUri := alice.UploadContent(t, data.MatrixPng, asciiFileName, "image/png")
 
 				const altName = "file.png"
-				filename := downloadForFilename(t, alice, mxcUri, altName)
+				filename, _ := downloadForFilename(t, alice, mxcUri, altName)
+
 				if filename != altName {
 					t.Fatalf("filename did not match, expected '%s', got '%s'", altName, filename)
 				}
@@ -83,7 +86,8 @@ func TestMediaFilenames(t *testing.T) {
 
 				const diffUnicodeFilename = "\u2615" // coffee emoji
 
-				filename := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename)
+				filename, _ := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename)
+
 				if filename != diffUnicodeFilename {
 					t.Fatalf("filename did not match, expected '%s', got '%s'", diffUnicodeFilename, filename)
 				}
@@ -95,7 +99,7 @@ func TestMediaFilenames(t *testing.T) {
 
 				mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png")
 
-				filename := downloadForFilename(t, alice, mxcUri, "")
+				filename, _ := downloadForFilename(t, alice, mxcUri, "")
 
 				if filename != unicodeFileName {
 					t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename)
@@ -108,18 +112,54 @@ func TestMediaFilenames(t *testing.T) {
 
 				mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png")
 
-				filename := downloadForFilename(t, bob, mxcUri, "")
+				filename, _ := downloadForFilename(t, bob, mxcUri, "")
 
 				if filename != unicodeFileName {
 					t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename)
 				}
 			})
+
+			t.Run("Will serve safe media types as inline", func(t *testing.T) {
+				if runtime.Homeserver != runtime.Synapse {
+					// We need to check that this security behaviour is being correctly run in
+					// Synapse, but since this is not part of the Matrix spec we do not assume
+					// other homeservers are doing so.
+					t.Skip("Skipping test of Content-Disposition header requirements on non-Synapse homeserver")
+				}
+				t.Parallel()
+
+				mxcUri := alice.UploadContent(t, data.MatrixPng, "", "image/png")
+
+				_, isAttachment := downloadForFilename(t, bob, mxcUri, "")
+
+				if isAttachment {
+					t.Fatal("Expected file to be served as inline")
+				}
+			})
+
+			t.Run("Will serve unsafe media types as attachments", func(t *testing.T) {
+				if runtime.Homeserver != runtime.Synapse {
+					// We need to check that this security behaviour is being correctly run in
+					// Synapse, but since this is not part of the Matrix spec we do not assume
+					// other homeservers are doing so.
+					t.Skip("Skipping test of Content-Disposition header requirements on non-Synapse homeserver")
+				}
+				t.Parallel()
+
+				mxcUri := alice.UploadContent(t, data.MatrixSvg, "", "image/svg")
+
+				_, isAttachment := downloadForFilename(t, bob, mxcUri, "")
+
+				if !isAttachment {
+					t.Fatal("Expected file to be served as an attachment")
+				}
+			})
 		})
 	})
 }
 
-// Returns content disposition information like (mediatype, filename)
-func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName string) string {
+// Returns content disposition information like (filename, isAttachment)
+func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName string) (filename string, isAttachment bool) {
 	t.Helper()
 
 	origin, mediaId := client.SplitMxc(mxcUri)
@@ -138,16 +178,16 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName
 	if err != nil {
 		t.Fatalf("Got err when parsing content disposition: %s", err)
 	}
-
-	if mediaType != "attachment" {
-		t.Fatalf("Found unexpected mediatype %s, expected attachment", mediaType)
+	filename, hasFilename := params["filename"]
+	if mediaType == "attachment" {
+		if hasFilename {
+			return filename, true
+		} else {
+			return "", true
+		}
 	}
-
-	if filename, ok := params["filename"]; ok {
-		return filename
-	} else {
-		t.Fatalf("Content Disposition did not have filename")
-
-		return ""
+	if mediaType != "inline" {
+		t.Fatalf("Found unexpected mediatype %s, expected 'attachment' or 'inline'", mediaType)
 	}
+	return filename, false
 }