Merge pull request #493 from marxjohnson/s3-pre-signed-url

S3 pre-signed URL: error with unicode filename #455
catalyst · Jun 27, 2022 · 123ca7e · 123ca7e
2 parents a277b86 + 8e107e6
commit 123ca7e
Show file tree

Hide file tree

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

diff --git a/tests/fixtures/😀.txt b/tests/fixtures/😀.txt
@@ -0,0 +1 @@
+This is a test text file with a unicode character in the file name
diff --git a/tests/object_file_system_test.php b/tests/object_file_system_test.php
@@ -585,6 +585,39 @@ 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,6 +25,7 @@
 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');
@@ -198,9 +199,9 @@ protected function delete_draft_files($contenthash) {
         $DB->delete_records('files', array('contenthash' => $contenthash));
     }
 
-    protected function is_externally_readable_by_url($url) {
+    protected function is_externally_readable_by_url(signed_url $signedurl) {
         try {
-            $file = fopen($url, 'r');
+            $file = fopen($signedurl->url->out(false), 'r');
             if ($file === false) {
                 $result = false;
             } else {