From b5555cb2de95b4018a7f38a4679c9364afbd1935 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sat, 1 Apr 2017 20:43:34 +0100 Subject: [PATCH 1/6] fix tests for https://github.com/matrix-org/synapse/pull/2090 --- tests/51media/03ascii.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/51media/03ascii.pl b/tests/51media/03ascii.pl index fb961486e..e4ff16964 100644 --- a/tests/51media/03ascii.pl +++ b/tests/51media/03ascii.pl @@ -48,7 +48,7 @@ sub test_using_client my ( $body, $response ) = @_; my $disposition = $response->header( "Content-Disposition" ); - $disposition eq "inline; filename=ascii" or + $disposition eq "inline; filename*=utf-8''ascii" or die "Expected a UTF-8 filename parameter"; Future->done(1); @@ -90,7 +90,7 @@ sub test_using_client my ( $body, $response ) = @_; my $disposition = $response->header( "Content-Disposition" ); - $disposition eq "inline; filename=also_ascii" or + $disposition eq "inline; filename*=utf-8''also_ascii" or die "Expected a UTF-8 filename parameter"; Future->done(1); From 7c78d24942a5de5d03124352e2caeb58865a6606 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 27 Feb 2019 22:21:55 +0000 Subject: [PATCH 2/6] revert earlier changes on this branch --- tests/51media/03ascii.pl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/51media/03ascii.pl b/tests/51media/03ascii.pl index e4ff16964..7a73f0c8a 100644 --- a/tests/51media/03ascii.pl +++ b/tests/51media/03ascii.pl @@ -48,8 +48,8 @@ sub test_using_client my ( $body, $response ) = @_; my $disposition = $response->header( "Content-Disposition" ); - $disposition eq "inline; filename*=utf-8''ascii" or - die "Expected a UTF-8 filename parameter"; + $disposition eq "inline; filename=ascii" or + die "Expected an ASCII filename parameter"; Future->done(1); }); @@ -90,8 +90,8 @@ sub test_using_client my ( $body, $response ) = @_; my $disposition = $response->header( "Content-Disposition" ); - $disposition eq "inline; filename*=utf-8''also_ascii" or - die "Expected a UTF-8 filename parameter"; + $disposition eq "inline; filename=also_ascii" or + die "Expected an ASCII filename parameter"; Future->done(1); }); From 9452b01f971b34809b2e70e06b877509fa0b1407 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 27 Feb 2019 22:19:46 +0000 Subject: [PATCH 3/6] Factor out common code Not sure why we need to have this twice. --- tests/51media/01unicode.pl | 104 ++++++++++++++++++++++++---------- tests/51media/02nofilename.pl | 34 ++--------- tests/51media/03ascii.pl | 40 +++---------- 3 files changed, 87 insertions(+), 91 deletions(-) diff --git a/tests/51media/01unicode.pl b/tests/51media/01unicode.pl index ce8f392c4..137c15a4f 100644 --- a/tests/51media/01unicode.pl +++ b/tests/51media/01unicode.pl @@ -40,36 +40,85 @@ ); -test "Can upload with Unicode file name", - requires => [ $main::API_CLIENTS[0], local_user_fixture(), - qw( can_upload_media )], +=head2 upload_test_content - proves => [qw( can_upload_media_unicode )], + my ( $content_id, $content_uri ) = upload_test_content( + $user, filename => "filename", + ) -> get; - do => sub { - my ( $http, $user ) = @_; +Uploads some test content with the given filename. - $http->do_request( - method => "POST", - full_uri => "/_matrix/media/r0/upload", - content => "Test media file", - content_type => "text/plain", - - params => { - access_token => $user->access_token, - filename => $FILENAME, - } - )->then( sub { - my ( $body ) = @_; +Returns the content id of the uploaded content. + +=cut +sub upload_test_content +{ + my ( $user, %params ) = @_; + + $user->http->do_request( + method => "POST", + full_uri => "/_matrix/media/r0/upload", + content => "Test media file", + content_type => "text/plain", + + params => { + access_token => $user->access_token, + %params, + }, + )->then( sub { + my ( $body ) = @_; + + assert_json_keys( $body, qw( content_uri )); + + my $content_uri = $body->{content_uri}; + + my $parsed_uri = URI->new( $body->{content_uri} ); + my $server = $parsed_uri->authority; + my $path = $parsed_uri->path; + + my $content_id = "$server$path"; + + Future->done( $content_id, $content_uri ); + }); +} +push our @EXPORT, qw( upload_test_content ); + + +=head2 get_media + + my ( $content_dispostion, $content ) = get_media( $http, $content_id ) -> get; + +Fetches a piece of media from the server. + +=cut +sub get_media +{ + my ( $http, $content_id ) = @_; + + $http->do_request( + method => "GET", + full_uri => "/_matrix/media/r0/download/$content_id", + )->then( sub { + my ( $body, $response ) = @_; + + my $disposition = $response->header( "Content-Disposition" ); + Future->done( $disposition, $body ); + }); +} +push @EXPORT, qw( get_media ); - assert_json_keys( $body, qw( content_uri )); - my $content_uri = URI->new( $body->{content_uri} ); - my $server = $content_uri->authority; - my $path = $content_uri->path; +test "Can upload with Unicode file name", + requires => [ local_user_fixture(), + qw( can_upload_media )], - $content_id = "$server$path"; + proves => [qw( can_upload_media_unicode )], + + do => sub { + my ( $user ) = @_; + upload_test_content( $user, filename=>$FILENAME )->then( sub { + ( $content_id ) = @_; Future->done(1) }); }; @@ -85,13 +134,8 @@ sub test_using_client $content = $content_id; } - $client->do_request( - method => "GET", - full_uri => "/_matrix/media/r0/download/$content", - )->then( sub { - my ( $body, $response ) = @_; - - my $disposition = $response->header( "Content-Disposition" ); + get_media( $client, $content )->then( sub { + my ( $disposition ) = @_; uc $disposition eq uc "inline; filename*=utf-8''$FILENAME_ENCODED" or die "Expected a UTF-8 filename parameter"; diff --git a/tests/51media/02nofilename.pl b/tests/51media/02nofilename.pl index 251fdf94f..c27bebf36 100644 --- a/tests/51media/02nofilename.pl +++ b/tests/51media/02nofilename.pl @@ -1,31 +1,13 @@ my $content_id; test "Can upload without a file name", - requires => [ $main::API_CLIENTS[0], local_user_fixture() ], + requires => [ local_user_fixture() ], do => sub { - my ( $http, $user ) = @_; - - $http->do_request( - method => "POST", - full_uri => "/_matrix/media/r0/upload", - content => "Test media file", - content_type => "text/plain", - - params => { - access_token => $user->access_token, - } - )->then(sub { - my ( $body ) = @_; - - assert_json_keys( $body, qw( content_uri )); - - my $content_uri = URI->new( $body->{content_uri} ); - my $server = $content_uri->authority; - my $path = $content_uri->path; - - $content_id = "$server$path"; + my ( $user ) = @_; + upload_test_content( $user, )->then( sub { + ( $content_id ) = @_; Future->done(1) }); }; @@ -37,13 +19,9 @@ sub test_using_client { my ( $client ) = @_; - $client->do_request( - method => "GET", - full_uri => "/_matrix/media/r0/download/$content_id", - )->then( sub { - my ( $body, $response ) = @_; + get_media( $client, $content_id )->then( sub { + my ( $disposition ) = @_; - my $disposition = $response->header( "Content-Disposition" ); defined $disposition and die "Unexpected Content-Disposition header"; diff --git a/tests/51media/03ascii.pl b/tests/51media/03ascii.pl index 7a73f0c8a..842e9d4b6 100644 --- a/tests/51media/03ascii.pl +++ b/tests/51media/03ascii.pl @@ -2,35 +2,13 @@ my $content_uri; test "Can upload with ASCII file name", - requires => [ $main::API_CLIENTS[0], local_user_fixture() ], + requires => [ local_user_fixture() ], do => sub { - my ( $http, $user ) = @_; - - $http->do_request( - method => "POST", - full_uri => "/_matrix/media/r0/upload", - content => "Test media file", - content_type => "text/plain", - - params => { - access_token => $user->access_token, - filename => "ascii", - } - )->then( sub { - my ( $body ) = @_; - - assert_json_keys( $body, qw( content_uri )); - - $content_uri = $body->{content_uri}; - - my $parsed_uri = URI->new( $body->{content_uri} ); - my $server = $parsed_uri->authority; - my $path = $parsed_uri->path; - - $content_id = "$server$path"; - - Future->done(1) + my ( $user ) = @_; + upload_test_content( $user, filename=>"ascii" )->then( sub { + ( $content_id, $content_uri ) = @_; + Future->done(1); }); }; @@ -41,13 +19,9 @@ sub test_using_client { my ( $client ) = @_; - $client->do_request( - method => "GET", - full_uri => "/_matrix/media/r0/download/$content_id", - )->then( sub { - my ( $body, $response ) = @_; + get_media( $client, $content_id )->then( sub { + my ( $disposition ) = @_; - my $disposition = $response->header( "Content-Disposition" ); $disposition eq "inline; filename=ascii" or die "Expected an ASCII filename parameter"; From 5c1f1e7dd7c210a42a3a4daef4259409a055a33e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 27 Feb 2019 23:06:02 +0000 Subject: [PATCH 4/6] Add tests for filenames with spaces and semicolons --- tests/51media/03ascii.pl | 94 ++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 48 deletions(-) diff --git a/tests/51media/03ascii.pl b/tests/51media/03ascii.pl index 842e9d4b6..fd8099070 100644 --- a/tests/51media/03ascii.pl +++ b/tests/51media/03ascii.pl @@ -12,44 +12,48 @@ }); }; -# These next two tests do the same thing with two different HTTP clients, to -# test locally and via federation - -sub test_using_client -{ - my ( $client ) = @_; - - get_media( $client, $content_id )->then( sub { - my ( $disposition ) = @_; - - $disposition eq "inline; filename=ascii" or - die "Expected an ASCII filename parameter"; - - Future->done(1); - }); +# we only need one user for these tests +my $user_fixture = local_user_fixture(); + +my %TEST_CASES = ( + "ascii" => "inline; filename=ascii", + + # synapse uses filename* encoding for this, though regular filename encoding + # with a quoted string would be valid too + "name with spaces" => "inline; filename*=utf-8''name%20with%20spaces", + + "name;with;semicolons" => "inline; filename*=utf-8''name%3Bwith%3Bsemicolons", + ); + +while ( my ( $filename, $expected_disposition ) = each %TEST_CASES ) { + test "Can download file '$filename'", + requires => [ + $user_fixture, $main::API_CLIENTS[1] + ], + + check => sub { + my ( $user, $federation_client ) = @_; + + my $content_id; + + # first upload the content with the given filename + upload_test_content( $user, $filename )->then( sub { + ( $content_id ) = @_; + + # try and fetch it as a local user + get_media( $user->http, $content_id ); + })->then( sub { + assert_eq( $_[0], $expected_disposition ); + + # do the same over federation + get_media( $federation_client, $content_id ); + })->then( sub { + assert_eq( $_[0], $expected_disposition ); + Future->done(1); + }); + }; } -test "Can download with ASCII file name locally", - requires => [ $main::API_CLIENTS[0] ], - - check => sub { - my ( $http ) = @_; - test_using_client( $http ) - ->then( sub { - test_using_client( $http ) - }); - }; - -test "Can download with ASCII file name over federation", - requires => [ $main::API_CLIENTS[1] ], - - check => sub { - my ( $http ) = @_; - test_using_client( $http ) - ->then( sub { - test_using_client( $http ) - }); - }; test "Can download specifying a different ASCII file name", requires => [ $main::API_CLIENTS[0] ], @@ -76,12 +80,9 @@ sub test_using_client check => sub { my ( $http, $user, $room_id ) = @_; - test_using_client( $http ) - ->then( sub { - matrix_send_room_message( $user, $room_id, - content => { msgtype => "m.file", body => "test.txt", url => $content_uri } - ) - }); + matrix_send_room_message( $user, $room_id, + content => { msgtype => "m.file", body => "test.txt", url => $content_uri } + ); }; test "Can fetch images in room", @@ -89,12 +90,9 @@ sub test_using_client check => sub { my ( $http, $user, $room_id ) = @_; - test_using_client( $http ) - ->then( sub { - matrix_send_room_message_synced( $user, $room_id, - content => { msgtype => "m.text", body => "test" } - ) - })->then( sub { + matrix_send_room_message_synced( $user, $room_id, + content => { msgtype => "m.text", body => "test" } + )->then( sub { matrix_send_room_message_synced( $user, $room_id, content => { msgtype => "m.file", body => "test.txt", url => $content_uri } ) From f6de195053ed4c1554db01e1310d99355c368a4f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 28 Feb 2019 11:58:56 +0000 Subject: [PATCH 5/6] fix failing test --- tests/51media/03ascii.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/51media/03ascii.pl b/tests/51media/03ascii.pl index fd8099070..a34fed864 100644 --- a/tests/51media/03ascii.pl +++ b/tests/51media/03ascii.pl @@ -37,7 +37,7 @@ my $content_id; # first upload the content with the given filename - upload_test_content( $user, $filename )->then( sub { + upload_test_content( $user, filename=>$filename )->then( sub { ( $content_id ) = @_; # try and fetch it as a local user From 7c3710e906d743e90b7a389ac001a0a9fb765aa6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 7 Mar 2019 22:58:41 +0000 Subject: [PATCH 6/6] Improve Content-Disposition handling Actually parse the header properly rather than checking it matches a hardcoded string. --- tests/51media/01unicode.pl | 41 ++++++++++++++++++++++++++++++++------ tests/51media/03ascii.pl | 33 +++++++++++++++++++----------- 2 files changed, 56 insertions(+), 18 deletions(-) diff --git a/tests/51media/01unicode.pl b/tests/51media/01unicode.pl index 137c15a4f..f18c70e93 100644 --- a/tests/51media/01unicode.pl +++ b/tests/51media/01unicode.pl @@ -1,3 +1,4 @@ +use HTTP::Headers::Util qw( split_header_words ); use URI::Escape qw( uri_escape ); use SyTest::TCPProxy; @@ -86,7 +87,7 @@ sub upload_test_content =head2 get_media - my ( $content_dispostion, $content ) = get_media( $http, $content_id ) -> get; + my ( $content_disposition_params, $content ) = get_media( $http, $content_id ) -> get; Fetches a piece of media from the server. @@ -102,11 +103,41 @@ sub get_media my ( $body, $response ) = @_; my $disposition = $response->header( "Content-Disposition" ); - Future->done( $disposition, $body ); + + my $cd_params; + if ( defined $disposition ) { + $cd_params = parse_content_disposition_params( $disposition ); + } + Future->done( $cd_params, $body ); }); } push @EXPORT, qw( get_media ); +sub parse_content_disposition_params { + my ( $disposition ) = @_; + my @parts = split_header_words( $disposition ); + + # should be only one list of words + assert_eq( scalar @parts, 1, "number of content-dispostion header lists" ); + @parts = @{$parts[0]}; + + # the first part must be 'inline' + my $k = shift @parts; + my $v = shift @parts; + assert_eq( $k, "inline", "content-disposition" ); + die "invalid CD" if defined $v; + + my %params; + while (@parts) { + my $k = shift @parts; + my $v = shift @parts; + die "multiple $k params" if exists $params{$k}; + die "unknown param $k" unless ( $k eq 'filename' || $k eq 'filename*' ); + $params{$k} = $v; + } + return \%params; +} + test "Can upload with Unicode file name", requires => [ local_user_fixture(), @@ -135,10 +166,8 @@ sub test_using_client } get_media( $client, $content )->then( sub { - my ( $disposition ) = @_; - uc $disposition eq uc "inline; filename*=utf-8''$FILENAME_ENCODED" or - die "Expected a UTF-8 filename parameter"; - + my ( $cd_params ) = @_; + assert_eq( $cd_params->{'filename*'}, "utf-8''$FILENAME_ENCODED", "filename*" ); Future->done(1); }); } diff --git a/tests/51media/03ascii.pl b/tests/51media/03ascii.pl index a34fed864..239ecf1a2 100644 --- a/tests/51media/03ascii.pl +++ b/tests/51media/03ascii.pl @@ -15,17 +15,26 @@ # we only need one user for these tests my $user_fixture = local_user_fixture(); -my %TEST_CASES = ( - "ascii" => "inline; filename=ascii", - - # synapse uses filename* encoding for this, though regular filename encoding - # with a quoted string would be valid too - "name with spaces" => "inline; filename*=utf-8''name%20with%20spaces", - - "name;with;semicolons" => "inline; filename*=utf-8''name%3Bwith%3Bsemicolons", - ); +sub assert_cd_params_match_filename { + my ( $filename, $cd_params ) = @_; + + # either we need a valid "filename*" param + if ( $cd_params->{"filename*"} ) { + my $f = $cd_params->{"filename*"}; + $f =~ s/%(..)/chr hex $1/eg; + assert_eq( $f, "utf-8''$filename", "filename*" ); + + # there might also be a 'filename', but it doesn't really matter what it + # is. + return; + } + + # or we need a valid filename + my $f = $cd_params->{"filename"}; + assert_eq( $f, $filename, "filename" ); +} -while ( my ( $filename, $expected_disposition ) = each %TEST_CASES ) { +foreach my $filename ( "ascii", "name with spaces", "name;with;semicolons" ) { test "Can download file '$filename'", requires => [ $user_fixture, $main::API_CLIENTS[1] @@ -43,12 +52,12 @@ # try and fetch it as a local user get_media( $user->http, $content_id ); })->then( sub { - assert_eq( $_[0], $expected_disposition ); + assert_cd_params_match_filename( $filename, $_[0] ); # do the same over federation get_media( $federation_client, $content_id ); })->then( sub { - assert_eq( $_[0], $expected_disposition ); + assert_cd_params_match_filename( $filename, $_[0] ); Future->done(1); }); };