Merge pull request #891 from ikedas/escapeurlized_filename by ikedas

URLize: Use filesystem-independent escaping for names of files stored

src/cgi/wwsympa.fcgi.in

@@ -15297,6 +15297,9 @@ sub do_set_lang {
 sub do_attach {
     wwslog('info', '(%s, %s)', $in{'dir'}, $in{'file'});
+    # Avoid directory traversal.
+    return undef if 0 <= index $in{'dir'}, '/' or 0 <= index $in{'file'}, '/';
+
     ### Useful variables
     # current list / current shared directory

src/lib/Sympa/Message.pm

@@ -2205,7 +2205,8 @@ sub _urlize_one_part {
             || 'bin';
         $filename = sprintf 'msg.%d.%s', $i, $fileExt;
     }
-    my $file = "$expl/$dir/$filename";
+    my $safe_filename = Sympa::Tools::Text::encode_filesystem_safe($filename);
+    my $file = sprintf '%s/%s/%s', $expl, $dir, $safe_filename;
 
     # Create the linked file
     # Store body in file
@@ -2239,11 +2240,9 @@ sub _urlize_one_part {
         return undef;
     }
 
-    (my $file_name = $filename) =~ s/\./\_/g;
     # Do NOT escape '/' chars separating path components.
     my $file_url =
-        Sympa::get_url($list, 'attach',
-        paths => [$dir, Sympa::Tools::Text::escape_chars($filename)]);
+        Sympa::get_url($list, 'attach', paths => [$dir, $safe_filename]);
 
     my $parser = MIME::Parser->new;
     $parser->output_to_core(1);
@@ -2252,7 +2251,7 @@ sub _urlize_one_part {
 
     my $charset = Conf::lang2charset($language->get_lang);
     my $data    = {
-        file_name => $file_name,
+        file_name => $filename,
         file_url  => $file_url,
         file_size => $size,
         charset   => $charset,    # compat. <= 6.1.

t/Message_urlize.t

@@ -85,7 +85,8 @@ my @to_urlize = (
         filename    => 't/samples/urlize-encoding.eml',
         attachments => [
             {   name => 'ございます.pdf',
-                escaped_name => '%25e3%2581%2594%25e3%2581%259',
+                escaped_name =>
+                    '_e3_81_94_e3_81_96_e3_81_84_e3_81_be_e3_81_99.pdf',
             },
         ],
         dirname         => 'globuz_24_3c_3e_25@example.com',
@@ -95,7 +96,7 @@ my @to_urlize = (
         filename    => 't/samples/urlize-nested-mixed.eml',
         attachments => [
             {   name         => 'Würzburg.txt',
-                escaped_name => 'W%25c3%25bcrzburg.txt',
+                escaped_name => 'W_c3_bcrzburg.txt',
             },
         ],
         dirname         => '3_24@domain.tld',
@@ -115,7 +116,7 @@ my @to_urlize = (
         filename    => 't/samples/urlize-deep-nested-mixed.eml',
         attachments => [
             {   name         => 'Würzburg.txt',
-                escaped_name => 'W%25c3%25bcrzburg.txt',
+                escaped_name => 'W_c3_bcrzburg.txt',
             },
             {   name         => 'msg.3.bin',
                 escaped_name => 'msg.3.bin',
@@ -194,24 +195,45 @@ foreach my $test_file (@to_urlize) {
 
     my @expected_files;
     foreach my $file (@{$test_file->{attachments}}) {
-        ok( -f "$home_dir/$test_list_name/urlized/$urlized_directory/$file->{name}",
-            'Test case: '
-                . $test_file->{test_case}
-                . ' - The attachment '
-                . $file->{name}
-                . ' has been stored on the filesystem.'
+        my $safe_filename =
+            Sympa::Tools::Text::encode_filesystem_safe($file->{name});
+        ok( -f sprintf(
+                '%s/%s/urlized/%s/%s',
+                $home_dir,          $test_list_name,
+                $urlized_directory, $safe_filename
+            ),
+            sprintf(
+                'Test case: %s - The attachment %s has been stored on the filesystem.',
+                $test_file->{test_case},
+                $file->{name}
+            )
         );
-        if (-f "$home_dir/$test_list_name/urlized/$urlized_directory/$file->{name}"
+        if (-f sprintf(
+                '%s/%s/urlized/%s/%s',
+                $home_dir,          $test_list_name,
+                $urlized_directory, $safe_filename
+            )
         ) {
             push @expected_files, $file->{name};
         }
         my $found_url_to_attachment = 0;
-        foreach my $line (split '\n', $new_entity->as_string()) {
-            my $line_to_match =
-                  $root_url
-                . $test_file->{escaped_dirname} . '/'
-                . $file->{escaped_name};
-            if ($line =~ m{$line_to_match}) {
+        my $line_to_match = sprintf '%s%s/%s', $root_url,
+            $test_file->{escaped_dirname},
+            $file->{escaped_name};
+        foreach my $line (
+            map {
+                my $bodyh = $_->bodyhandle;
+                if ($bodyh) {
+                    split '\n', $bodyh->as_string;
+                } else {
+                    ();
+                }
+            }
+            grep {
+                lc($_->effective_type // 'text/plain') eq 'text/plain'
+            } $new_entity->parts_DFS
+        ) {
+            if (0 <= index $line, $line_to_match) {
                 $found_url_to_attachment = 1;
                 last;
             }