From e7227482f36cc6965d6a288eabf9e97719691de0 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 29 Sep 2023 12:20:08 +0100 Subject: [PATCH] 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 | 3 ++ internal/data/matrix-logo.svg | 18 +++++++++ tests/media_filename_test.go | 76 ++++++++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 18 deletions(-) create mode 100644 internal/data/matrix-logo.svg diff --git a/internal/data/data.go b/internal/data/data.go index c4455d2c..586c5c49 100644 --- a/internal/data/data.go +++ b/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 diff --git a/internal/data/matrix-logo.svg b/internal/data/matrix-logo.svg new file mode 100644 index 00000000..900a5aa0 --- /dev/null +++ b/internal/data/matrix-logo.svg @@ -0,0 +1,18 @@ + + + + matrix logo white + Created with Sketch. + + + + + + + + + + + + + \ No newline at end of file diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 637c996f..c1138a79 100644 --- a/tests/media_filename_test.go +++ b/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 }