Revert "S3 pre-signed URL: error with unicode filename #455"

catalyst · Jul 1, 2022 · 3080976 · 3080976
1 parent 123ca7e
commit 3080976
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 59 deletions.
diff --git a/classes/local/store/s3/client.php b/classes/local/store/s3/client.php
@@ -486,10 +486,9 @@ public function generate_presigned_url($contenthash, $headers = array()) {
     /**
      * @param string $contenthash
      * @param array $headers
-     * @param bool $nicefilename
      * @return signed_url
      */
-    private function generate_presigned_url_s3($contenthash, array $headers = [], $nicefilename = true) {
+    private function generate_presigned_url_s3($contenthash, $headers) {
         $contentdisposition = manager::get_header($headers, 'Content-Disposition');
         if ($contentdisposition !== '') {
             $params['ResponseContentDisposition'] = $contentdisposition;
@@ -504,12 +503,14 @@ private function generate_presigned_url_s3($contenthash, array $headers = [], $n
         $params['Bucket'] = $this->bucket;
         $params['Key'] = $this->bucketkeyprefix . $key;
 
-        if ($nicefilename) {
-            $result = $this->get_nice_filename($headers);
-            if (!empty($result)) {
-                $params['ResponseContentDisposition'] = $result['Content-Disposition'] . '; ' . $result['filename'];
-                $params['ResponseContentType'] = $result['Content-Type'];
-            }
+        $contentdisposition = manager::get_header($headers, 'Content-Disposition');
+        if ($contentdisposition !== '') {
+            $params['ResponseContentDisposition'] = $contentdisposition;
+        }
+
+        $contenttype = manager::get_header($headers, 'Content-Type');
+        if ($contenttype !== '') {
+            $params['ResponseContentType'] = $contenttype;
         }
 
         $command = $this->client->getCommand('GetObject', $params);
@@ -533,14 +534,7 @@ private function generate_presigned_url_cloudfront($contenthash, array $headers
         $expires = $this->get_expiration_time(time(), manager::get_header($headers, 'Expires'));
 
         if ($nicefilename) {
-            $result = $this->get_nice_filename($headers);
-            if (!empty($result)) {
-                $key .= '?response-content-disposition=' .
-                    rawurlencode($result['Content-Disposition']) . ';' . $result['filename'] .
-                    '&response-content-type=' . rawurlencode($result['Content-Type']);
-            } else {
-                $key .= '';
-            }
+            $key .= $this->get_nice_filename($headers);
         }
         $resource = $this->config->cloudfrontresourcedomain . '/' . $key;
         // This is the id of the Cloudfront key pair you generated.
@@ -578,12 +572,11 @@ private function generate_presigned_url_cloudfront($contenthash, array $headers
 
     /**
      * @param $headers
-     * @return array
+     * @return string
      */
     private function get_nice_filename($headers) {
         // We are trying to deliver original filename rather than hash filename to client.
         $originalfilename = '';
-        $result = [];
         $contentdisposition = trim(manager::get_header($headers, 'Content-Disposition'));
         $originalcontenttype = trim(manager::get_header($headers, 'Content-Type'));
 
@@ -603,12 +596,12 @@ private function get_nice_filename($headers) {
             }
 
             if (!empty($originalfilename)) {
-                $result['Content-Disposition'] = $contentdisposition;
-                $result['filename'] = "filename*=utf-8''" . rawurlencode($originalfilename);
-                $result['Content-Type'] = $originalcontenttype;
+                return '?response-content-disposition=' .
+                    rawurlencode($contentdisposition . ';filename="' . utf8_encode($originalfilename) . '"') .
+                    '&response-content-type=' . rawurlencode($originalcontenttype);
             }
         }
-        return $result;
+        return '';
     }
 
     /**

diff --git a/tests/fixtures/😀.txt b/tests/fixtures/😀.txt
diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php
@@ -585,39 +585,6 @@ public function test_can_generate_signed_url_by_hash_if_object_is_external() {
         }
     }
 
-    public function test_can_generate_signed_url_with_headers() {
-        $this->filesystem = new test_file_system();
-        $file = $this->create_remote_file();
-        $filehash = $file->get_contenthash();
-        try {
-            $headers = [
-                'Content-Disposition' => 'attachment; filename="filename.txt"',
-                'Content-Type' => 'text/plain',
-            ];
-            $signedurl = $this->filesystem->externalclient->generate_presigned_url($filehash, $headers);
-            $this->assertTrue($this->is_externally_readable_by_url($signedurl));
-        } catch (\coding_exception $e) {
-            $this->assertEquals($e->a, 'Pre-signed URLs not supported');
-        }
-    }
-
-    public function test_can_generate_signed_url_with_unicode_filename() {
-        $this->filesystem = new test_file_system();
-        $file = $this->create_remote_file();
-        $filehash = $file->get_contenthash();
-        try {
-            $headers = [
-                    'Content-Disposition' => 'attachment; filename="😀.txt"',
-                    'Content-Type' => 'text/plain',
-            ];
-            $signedurl = $this->filesystem->externalclient->generate_presigned_url($filehash, $headers);
-            $this->assertTrue($this->is_externally_readable_by_url($signedurl));
-
-        } catch (\coding_exception $e) {
-            $this->assertEquals($e->a, 'Pre-signed URLs not supported');
-        }
-    }
-
     public function test_presigned_url_configured_method_returns_false_if_not_configured() {
         $this->filesystem = new test_file_system();
         $this->assertFalse($this->filesystem->presigned_url_configured());

diff --git a/tests/tool_objectfs_testcase.php b/tests/tool_objectfs_testcase.php
@@ -25,7 +25,6 @@
 use tool_objectfs\local\manager;
 use tool_objectfs\local\object_manipulator\candidates\candidates_finder;
 use tool_objectfs\local\store\object_file_system;
-use tool_objectfs\local\store\signed_url;
 
 require_once(__DIR__ . '/classes/test_client.php');
 require_once(__DIR__ . '/classes/test_file_system.php');
@@ -199,9 +198,9 @@ protected function delete_draft_files($contenthash) {
         $DB->delete_records('files', array('contenthash' => $contenthash));
     }
 
-    protected function is_externally_readable_by_url(signed_url $signedurl) {
+    protected function is_externally_readable_by_url($url) {
         try {
-            $file = fopen($signedurl->url->out(false), 'r');
+            $file = fopen($url, 'r');
             if ($file === false) {
                 $result = false;
             } else {