diff --git a/client/client.go b/client/client.go index 55e95430..d98bb6e1 100644 --- a/client/client.go +++ b/client/client.go @@ -101,6 +101,19 @@ func (c *CSAPI) DownloadContent(t ct.TestLike, mxcUri string) ([]byte, string) { return b, contentType } +// DownloadContentAuthenticated downloads media from _matrix/client/v1/media resource, returning the raw bytes and the Content-Type. Fails the test on error. +func (c *CSAPI) DownloadContentAuthenticated(t ct.TestLike, mxcUri string) ([]byte, string) { + t.Helper() + origin, mediaId := SplitMxc(mxcUri) + res := c.MustDo(t, "GET", []string{"_matrix", "client", "v1", "media", "download", origin, mediaId}) + contentType := res.Header.Get("Content-Type") + b, err := io.ReadAll(res.Body) + if err != nil { + ct.Errorf(t, err.Error()) + } + return b, contentType +} + // MustCreateRoom creates a room with an optional HTTP request body. Fails the test on error. Returns the room ID. func (c *CSAPI) MustCreateRoom(t ct.TestLike, reqBody map[string]interface{}) string { t.Helper() diff --git a/tests/csapi/apidoc_content_test.go b/tests/csapi/apidoc_content_test.go index a28236c6..be8724c1 100644 --- a/tests/csapi/apidoc_content_test.go +++ b/tests/csapi/apidoc_content_test.go @@ -2,6 +2,7 @@ package csapi_tests import ( "bytes" + "net/http" "testing" "github.com/matrix-org/complement" @@ -27,3 +28,29 @@ func TestContent(t *testing.T) { t.Fatalf("expected contentType to be %s, got %s", wantContentType, contentType) } } + +// same as above but testing _matrix/client/v1/media/download +func TestContentCSAPIMediaV1(t *testing.T) { + deployment := complement.Deploy(t, 1) + defer deployment.Destroy(t) + + alice := deployment.Register(t, "hs1", helpers.RegistrationOpts{}) + + wantContentType := "img/png" + mxcUri := alice.UploadContent(t, data.MatrixPng, "test.png", wantContentType) + + content, contentType := alice.DownloadContentAuthenticated(t, mxcUri) + if !bytes.Equal(data.MatrixPng, content) { + t.Fatalf("uploaded and downloaded content doesn't match: want %v\ngot\n%v", data.MatrixPng, content) + } + if contentType != wantContentType { + t.Fatalf("expected contentType to be \n %s, got \n %s", wantContentType, contentType) + } + + // Remove the AccessToken and try again, this should now return a 401. + alice.AccessToken = "" + res := alice.Do(t, "GET", []string{"_matrix", "client", "v1", "media", "download", "hs1", mxcUri}) + if res.StatusCode != http.StatusUnauthorized { + t.Fatalf("expected HTTP status: %d, got %d", http.StatusUnauthorized, res.StatusCode) + } +} diff --git a/tests/csapi/media_async_uploads_test.go b/tests/csapi/media_async_uploads_test.go index 901caa45..598f3121 100644 --- a/tests/csapi/media_async_uploads_test.go +++ b/tests/csapi/media_async_uploads_test.go @@ -70,4 +70,14 @@ func TestAsyncUpload(t *testing.T) { t.Fatalf("expected contentType to be %s, got %s", wantContentType, contentType) } }) + + t.Run("Download media over _matrix/client/v1/media/download", func(t *testing.T) { + content, contentType := alice.DownloadContentAuthenticated(t, mxcURI) + if !bytes.Equal(data.MatrixPng, content) { + t.Fatalf("uploaded and downloaded content doesn't match: want %v\ngot\n%v", data.MatrixPng, content) + } + if contentType != wantContentType { + t.Fatalf("expected contentType to be %s, got %s", wantContentType, contentType) + } + }) } diff --git a/tests/federation_media_content_test.go b/tests/federation_media_content_test.go new file mode 100644 index 00000000..c2cfd16e --- /dev/null +++ b/tests/federation_media_content_test.go @@ -0,0 +1,28 @@ +package tests + +import ( + "bytes" + "github.com/matrix-org/complement" + "github.com/matrix-org/complement/helpers" + "github.com/matrix-org/complement/internal/data" + "testing" +) + +func TestContentMediaV1(t *testing.T) { + deployment := complement.Deploy(t, 2) + defer deployment.Destroy(t) + + hs1 := deployment.Register(t, "hs1", helpers.RegistrationOpts{}) + hs2 := deployment.Register(t, "hs2", helpers.RegistrationOpts{}) + + wantContentType := "img/png" + mxcUri := hs1.UploadContent(t, data.MatrixPng, "test.png", wantContentType) + + content, contentType := hs2.DownloadContentAuthenticated(t, mxcUri) + if !bytes.Equal(data.MatrixPng, content) { + t.Fatalf("uploaded and downloaded content doesn't match: want %v\ngot\n%v", data.MatrixPng, content) + } + if contentType != wantContentType { + t.Fatalf("expected contentType to be \n %s, got \n %s", wantContentType, contentType) + } +} diff --git a/tests/media_filename_test.go b/tests/media_filename_test.go index bff7d8f6..709174f2 100644 --- a/tests/media_filename_test.go +++ b/tests/media_filename_test.go @@ -45,7 +45,20 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, filename, "image/png") - name, _ := downloadForFilename(t, alice, mxcUri, "") + name, _ := downloadForFilename(t, alice, mxcUri, "", false) + + // 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) + } + }) + + t.Run(fmt.Sprintf("Can download file '%s' over /_matrix/client/v1/media/download", filename), func(t *testing.T) { + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixPng, filename, "image/png") + + name, _ := downloadForFilename(t, alice, mxcUri, "", true) // filename is not required, but if it's an attachment then check it matches if name != filename { @@ -61,12 +74,25 @@ func TestMediaFilenames(t *testing.T) { 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, false) if filename != altName { t.Fatalf("filename did not match, expected '%s', got '%s'", altName, filename) } }) + t.Run("Can download specifying a different ASCII file name over _matrix/client/v1/media/download", func(t *testing.T) { + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixPng, asciiFileName, "image/png") + + const altName = "file.png" + filename, _ := downloadForFilename(t, alice, mxcUri, altName, true) + + if filename != altName { + t.Fatalf("filename did not match, expected '%s', got '%s'", altName, filename) + } + }) + }) t.Run("Unicode", func(t *testing.T) { @@ -87,7 +113,21 @@ func TestMediaFilenames(t *testing.T) { const diffUnicodeFilename = "\u2615" // coffee emoji - filename, _ := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename) + filename, _ := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename, false) + + if filename != diffUnicodeFilename { + t.Fatalf("filename did not match, expected '%s', got '%s'", diffUnicodeFilename, filename) + } + }) + + t.Run("Can download specifying a different Unicode file name over _matrix/client/v1/media/download", func(t *testing.T) { + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") + + const diffUnicodeFilename = "\u2615" // coffee emoji + + filename, _ := downloadForFilename(t, alice, mxcUri, diffUnicodeFilename, true) if filename != diffUnicodeFilename { t.Fatalf("filename did not match, expected '%s', got '%s'", diffUnicodeFilename, filename) @@ -100,7 +140,19 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") - filename, _ := downloadForFilename(t, alice, mxcUri, "") + filename, _ := downloadForFilename(t, alice, mxcUri, "", false) + + if filename != unicodeFileName { + t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) + } + }) + + t.Run("Can download with Unicode file name locally over _matrix/client/v1/media/download", func(t *testing.T) { + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") + + filename, _ := downloadForFilename(t, alice, mxcUri, "", true) if filename != unicodeFileName { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) @@ -113,7 +165,19 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") - filename, _ := downloadForFilename(t, bob, mxcUri, "") + filename, _ := downloadForFilename(t, bob, mxcUri, "", false) + + if filename != unicodeFileName { + t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) + } + }) + + t.Run("Can download with Unicode file name over federation via _matrix/client/v1/media/download", func(t *testing.T) { + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixPng, unicodeFileName, "image/png") + + filename, _ := downloadForFilename(t, bob, mxcUri, "", true) if filename != unicodeFileName { t.Fatalf("filename did not match, expected '%s', got '%s'", unicodeFileName, filename) @@ -131,7 +195,25 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixPng, "", "image/png") - _, isAttachment := downloadForFilename(t, bob, mxcUri, "") + _, isAttachment := downloadForFilename(t, bob, mxcUri, "", false) + + if isAttachment { + t.Fatal("Expected file to be served as inline") + } + }) + + t.Run("Will serve safe media types as inline via _matrix/client/v1/media/download", func(t *testing.T) { + if runtime.Homeserver != runtime.Synapse && runtime.Homeserver != runtime.Conduwuit { + // We need to check that this security behaviour is being correctly run in + // Synapse or conduwuit, 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 and non-conduwuit homeserver") + } + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixPng, "", "image/png") + + _, isAttachment := downloadForFilename(t, bob, mxcUri, "", true) if isAttachment { t.Fatal("Expected file to be served as inline") @@ -150,7 +232,26 @@ func TestMediaFilenames(t *testing.T) { // Add parameters and upper-case, which should be parsed as text/plain. mxcUri := alice.UploadContent(t, data.MatrixPng, "", "Text/Plain; charset=utf-8") - _, isAttachment := downloadForFilename(t, bob, mxcUri, "") + _, isAttachment := downloadForFilename(t, bob, mxcUri, "", false) + + if isAttachment { + t.Fatal("Expected file to be served as inline") + } + }) + + t.Run("Will serve safe media types with parameters as inline via _matrix/client/v1/media/download", func(t *testing.T) { + if runtime.Homeserver != runtime.Synapse && runtime.Homeserver != runtime.Conduwuit { + // We need to check that this security behaviour is being correctly run in + // Synapse or conduwuit, 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 and non-conduwuit homeserver") + } + t.Parallel() + + // Add parameters and upper-case, which should be parsed as text/plain. + mxcUri := alice.UploadContent(t, data.MatrixPng, "", "Text/Plain; charset=utf-8") + + _, isAttachment := downloadForFilename(t, bob, mxcUri, "", true) if isAttachment { t.Fatal("Expected file to be served as inline") @@ -168,7 +269,25 @@ func TestMediaFilenames(t *testing.T) { mxcUri := alice.UploadContent(t, data.MatrixSvg, "", "image/svg") - _, isAttachment := downloadForFilename(t, bob, mxcUri, "") + _, isAttachment := downloadForFilename(t, bob, mxcUri, "", false) + + if !isAttachment { + t.Fatal("Expected file to be served as an attachment") + } + }) + + t.Run("Will serve unsafe media types as attachments via _matrix/client/v1/media/download", func(t *testing.T) { + if runtime.Homeserver != runtime.Synapse && runtime.Homeserver != runtime.Conduwuit { + // We need to check that this security behaviour is being correctly run in + // Synapse or conduwuit, 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 and non-conduwuit homeserver") + } + t.Parallel() + + mxcUri := alice.UploadContent(t, data.MatrixSvg, "", "image/svg") + + _, isAttachment := downloadForFilename(t, bob, mxcUri, "", true) if !isAttachment { t.Fatal("Expected file to be served as an attachment") @@ -179,7 +298,7 @@ func TestMediaFilenames(t *testing.T) { } // Returns content disposition information like (filename, isAttachment) -func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName string) (filename string, isAttachment bool) { +func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName string, authenticatedEndpoint bool) (filename string, isAttachment bool) { t.Helper() origin, mediaId := client.SplitMxc(mxcUri) @@ -188,8 +307,16 @@ func downloadForFilename(t *testing.T, c *client.CSAPI, mxcUri string, diffName if diffName != "" { path = []string{"_matrix", "media", "v3", "download", origin, mediaId, diffName} + + if authenticatedEndpoint { + path = []string{"_matrix", "client", "v1", "media", "download", origin, mediaId, diffName} + } } else { path = []string{"_matrix", "media", "v3", "download", origin, mediaId} + + if authenticatedEndpoint { + path = []string{"_matrix", "client", "v1", "media", "download", origin, mediaId} + } } res := c.MustDo(t, "GET", path) diff --git a/tests/media_nofilename_test.go b/tests/media_nofilename_test.go index 0415d744..e7042bae 100644 --- a/tests/media_nofilename_test.go +++ b/tests/media_nofilename_test.go @@ -7,8 +7,8 @@ import ( "testing" "github.com/matrix-org/complement" - "github.com/matrix-org/complement/helpers" "github.com/matrix-org/complement/federation" + "github.com/matrix-org/complement/helpers" "github.com/matrix-org/complement/must" ) @@ -96,3 +96,77 @@ func TestMediaWithoutFileName(t *testing.T) { }) }) } + +// same test as above, but for the new _matrix/client/v1/media endpoint +func TestMediaWithoutFileNameCSMediaV1(t *testing.T) { + deployment := complement.Deploy(t, 2) + defer deployment.Destroy(t) + + hs1 := deployment.Register(t, "hs1", helpers.RegistrationOpts{}) + hs2 := deployment.Register(t, "hs2", helpers.RegistrationOpts{}) + + file := []byte("Hello World!") + fileName := "" + contentType := "text/plain" + + t.Run("parallel", func(t *testing.T) { + // sytest: Can upload without a file name + t.Run("Can upload without a file name", func(t *testing.T) { + t.Parallel() + mxc := hs1.UploadContent(t, file, fileName, contentType) + must.NotEqual(t, mxc, "", "did not return an MXC URI") + must.StartWithStr(t, mxc, "mxc://", "returned invalid MXC URI") + }) + // sytest: Can download without a file name locally + t.Run("Can download without a file name locally", func(t *testing.T) { + t.Parallel() + mxc := hs1.UploadContent(t, file, fileName, contentType) + must.NotEqual(t, mxc, "", "did not return an MXC URI") + must.StartWithStr(t, mxc, "mxc://", "returned invalid MXC URI") + + b, ct := hs1.DownloadContentAuthenticated(t, mxc) + + // Check the Content-Type response header. + // NOTSPEC: There is ambiguity over whether the homeserver is allowed to change the + // Content-Type header here from what is sent by the client during upload. Synapse does + // so, and there are benefits to doing so, but the spec needs to pick a side. + // For now, we're operating under the assumption that homeservers are free to add other + // directives. All we're going to check is the mime-type. + mimeType := strings.Split(ct, ";")[0] + must.Equal( + t, mimeType, contentType, + fmt.Sprintf( + "Wrong mime-type returned in Content-Type returned. got Content-Type '%s', extracted mime-type '%s', expected mime-type: '%s'", + ct, mimeType, contentType, + ), + ) + must.Equal(t, string(b), string(file), "wrong file content returned") + }) + // sytest: Can download without a file name over federation + t.Run("Can download without a file name over federation", func(t *testing.T) { + t.Parallel() + + mxc := hs1.UploadContent(t, file, fileName, contentType) + must.NotEqual(t, mxc, "", "did not return an MXC URI") + must.StartWithStr(t, mxc, "mxc://", "returned invalid MXC URI") + + b, ct := hs2.DownloadContentAuthenticated(t, mxc) + + // Check the Content-Type response header. + // NOTSPEC: There is ambiguity over whether the homeserver is allowed to change the + // Content-Type header here from what is sent by the client during upload. Synapse does + // so, and there are benefits to doing so, but the spec needs to pick a side. + // For now, we're operating under the assumption that homeservers are free to add other + // directives. All we're going to check is the mime-type. + mimeType := strings.Split(ct, ";")[0] + must.Equal( + t, mimeType, contentType, + fmt.Sprintf( + "Wrong mime-type returned in Content-Type returned. got Content-Type '%s', extracted mime-type '%s', expected mime-type: '%s'", + ct, mimeType, contentType, + ), + ) + must.Equal(t, string(b), string(file), "wrong file content returned") + }) + }) +}