From af5155e585f05af6aab8d97cf74c28385c3b7f5b Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 9 Aug 2023 00:29:25 +0100 Subject: [PATCH 1/9] Drop attachment requirements for media download Fixes #645 For https://github.com/matrix-org/synapse/pull/15988 --- tests/media_filename_test.go | 44 ++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 637c996f..6734e37e 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -43,9 +43,10 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, filename, "image/png") - name := downloadForFilename(t, alice, mxcUri, "") + name, isAttachment := downloadForFilename(t, alice, mxcUri, "") - if name != filename { + // filename is not required, but if it's an attachment then check it matches + if isAttachment && name != filename { t.Fatalf("Incorrect filename '%s', expected '%s'", name, filename) } }) @@ -58,8 +59,9 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, "test.png", "image/png") const altName = "file.png" - filename := downloadForFilename(t, alice, mxcUri, altName) - if filename != altName { + filename, isAttachment := downloadForFilename(t, alice, mxcUri, altName) + + if isAttachment && filename != altName { t.Fatalf("filename did not match, expected '%s', got '%s'", altName, filename) } }) @@ -83,8 +85,9 @@ func TestMediaFilenames(t *testing.T) { const diffUnicodeFilename = "\u2615" // coffee emoji - filename := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename) - if filename != diffUnicodeFilename { + filename, isAttachment := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename) + + if isAttachment && filename != diffUnicodeFilename { t.Fatalf("filename did not match, expected '%s', got '%s'", diffUnicodeFilename, filename) } }) @@ -95,9 +98,9 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") - filename := downloadForFilename(t, alice, mxcUri, "") + filename, isAttachment := downloadForFilename(t, alice, mxcUri, "") - if filename != unicodeFileName { + if isAttachment && filename != unicodeFileName { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) } }) @@ -108,9 +111,9 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") - filename := downloadForFilename(t, bob, mxcUri, "") + filename, isAttachment := downloadForFilename(t, bob, mxcUri, "") - if filename != unicodeFileName { + if isAttachment && filename != unicodeFileName { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) } }) @@ -119,7 +122,7 @@ func TestMediaFilenames(t *testing.T) { } // Returns content disposition information like (mediatype, filename) -func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName string) string { +func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName string) (filename string, isAttachment bool) { t.Helper() origin, mediaId := client.SplitMxc(mxcUri) @@ -139,15 +142,16 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName t.Fatalf("Got err when parsing content disposition: %s", err) } - if mediaType != "attachment" { - t.Fatalf("Found unexpected mediatype %s, expected attachment", mediaType) + if mediaType = "attachment" || { + if filename, ok := params["filename"]; ok { + return filename, true + } else { + t.Fatalf("Content Disposition did not have filename") + 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", mediaType) } + return "", false } From d65fa149070c55f10b3e1d726fcae2e3242ed733 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 9 Aug 2023 01:27:10 +0100 Subject: [PATCH 2/9] Update media_filename_test.go Fix operand --- tests/media_filename_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 6734e37e..4f116875 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -142,7 +142,7 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName t.Fatalf("Got err when parsing content disposition: %s", err) } - if mediaType = "attachment" || { + if mediaType = "attachment" { if filename, ok := params["filename"]; ok { return filename, true } else { From 953a65a2845639864a34054c423eb27bd1856d1a Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 14 Aug 2023 09:21:38 +0100 Subject: [PATCH 3/9] == --- tests/media_filename_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 4f116875..0ddf213f 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -142,7 +142,7 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName t.Fatalf("Got err when parsing content disposition: %s", err) } - if mediaType = "attachment" { + if mediaType == "attachment" { if filename, ok := params["filename"]; ok { return filename, true } else { From 00698c87796047569839923a16dbdf539f770999 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 21 Aug 2023 17:50:22 +0100 Subject: [PATCH 4/9] Add test --- internal/data/data.go | 3 +++ internal/data/matrix-logo.svg | 18 ++++++++++++++++++ tests/media_filename_test.go | 27 +++++++++++++++++++++++---- 3 files changed, 44 insertions(+), 4 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 0ddf213f..0e74f484 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" @@ -117,11 +118,29 @@ func TestMediaFilenames(t *testing.T) { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) } }) + + 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, asciiFileName, "image/svg") + + _, isAttachment := downloadForFilename(t, bob, mxcUri, "") + + if !isAttachment { + t.Fatal("Expected file type to be an attachment but ") + } + }) }) }) } -// Returns content disposition information like (mediatype, filename) +// 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() @@ -141,9 +160,9 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName if err != nil { t.Fatalf("Got err when parsing content disposition: %s", err) } - + filename, hasFilename := params["filename"] if mediaType == "attachment" { - if filename, ok := params["filename"]; ok { + if !hasFilename { return filename, true } else { t.Fatalf("Content Disposition did not have filename") @@ -153,5 +172,5 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName if mediaType != "inline" { t.Fatalf("Found unexpected mediatype %s, expected attachment", mediaType) } - return "", false + return filename, false } From 6e60beaba43d9c585a92325467e53922a645cc1a Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 21 Aug 2023 18:24:50 +0100 Subject: [PATCH 5/9] Whoops --- tests/media_filename_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 0e74f484..a7024bcd 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -162,7 +162,7 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName } filename, hasFilename := params["filename"] if mediaType == "attachment" { - if !hasFilename { + if hasFilename { return filename, true } else { t.Fatalf("Content Disposition did not have filename") From c68cee031ba9041a11cab2f3f81fc843a273fb8a Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 21 Aug 2023 22:40:35 +0100 Subject: [PATCH 6/9] Update tests/media_filename_test.go Co-authored-by: Erik Johnston --- tests/media_filename_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index a7024bcd..9d8ee488 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -170,7 +170,7 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName } } if mediaType != "inline" { - t.Fatalf("Found unexpected mediatype %s, expected attachment", mediaType) + t.Fatalf("Found unexpected mediatype %s, expected 'attachment' or 'inline'", mediaType) } return filename, false } From 89ca28ad9c319b51c78e3e3a3a1f60c054830673 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 22 Sep 2023 13:21:13 -0400 Subject: [PATCH 7/9] Review comments. --- tests/media_filename_test.go | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 9d8ee488..71b8bec0 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -44,10 +44,10 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, filename, "image/png") - name, isAttachment := downloadForFilename(t, alice, mxcUri, "") + name, _ := downloadForFilename(t, alice, mxcUri, "") // filename is not required, but if it's an attachment then check it matches - if isAttachment && name != filename { + if name != filename { t.Fatalf("Incorrect filename '%s', expected '%s'", name, filename) } }) @@ -57,12 +57,12 @@ 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, isAttachment := downloadForFilename(t, alice, mxcUri, altName) + filename, _ := downloadForFilename(t, alice, mxcUri, altName) - if isAttachment && filename != altName { + if filename != altName { t.Fatalf("filename did not match, expected '%s', got '%s'", altName, filename) } }) @@ -86,9 +86,9 @@ func TestMediaFilenames(t *testing.T) { const diffUnicodeFilename = "\u2615" // coffee emoji - filename, isAttachment := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename) + filename, _ := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename) - if isAttachment && filename != diffUnicodeFilename { + if filename != diffUnicodeFilename { t.Fatalf("filename did not match, expected '%s', got '%s'", diffUnicodeFilename, filename) } }) @@ -99,9 +99,9 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") - filename, isAttachment := downloadForFilename(t, alice, mxcUri, "") + filename, _ := downloadForFilename(t, alice, mxcUri, "") - if isAttachment && filename != unicodeFileName { + if filename != unicodeFileName { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) } }) @@ -112,9 +112,9 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") - filename, isAttachment := downloadForFilename(t, bob, mxcUri, "") + filename, _ := downloadForFilename(t, bob, mxcUri, "") - if isAttachment && filename != unicodeFileName { + if filename != unicodeFileName { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) } }) @@ -166,7 +166,6 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName return filename, true } else { t.Fatalf("Content Disposition did not have filename") - return "", true } } if mediaType != "inline" { From 6f8d3869bb7644296773a3c1d3e71ff7bd859549 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 22 Sep 2023 13:21:29 -0400 Subject: [PATCH 8/9] Add an additional test. --- tests/media_filename_test.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index 71b8bec0..cd4ee669 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -119,6 +119,24 @@ func TestMediaFilenames(t *testing.T) { } }) + 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 @@ -128,12 +146,12 @@ func TestMediaFilenames(t *testing.T) { } t.Parallel() - mxcUri := alice.UploadContent(t, data.MatrixSvg, asciiFileName, "image/svg") + mxcUri := alice.UploadContent(t, data.MatrixSvg, "", "image/svg") _, isAttachment := downloadForFilename(t, bob, mxcUri, "") if !isAttachment { - t.Fatal("Expected file type to be an attachment but ") + t.Fatal("Expected file to be served as an attachment") } }) }) From 9c42ad5682ff881ecc54ebaa945e38440c5e4473 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 22 Sep 2023 13:53:57 -0400 Subject: [PATCH 9/9] Fix-up attachment w/o filename. --- tests/media_filename_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index cd4ee669..c1138a79 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -183,7 +183,7 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName if hasFilename { return filename, true } else { - t.Fatalf("Content Disposition did not have filename") + return "", true } } if mediaType != "inline" {