fix: URL encoding of file location returned by createFile

index.js

@@ -179,11 +179,19 @@ class S3Adapter {
     return params;
   }
 
+  // An S3 key is raw, a url is not, so each segment is percent-encoded while
+  // the separators stay separators. Applied to the whole key, bucket prefix
+  // included, so every url naming an object agrees.
+  _encodeKey(key) {
+    return key.split('/').map(encodeURIComponent).join('/');
+  }
+
   // For a given config object, filename, and data, store a file in S3
   // Returns a promise containing the S3 object creation response
   async createFile(filename, data, contentType, options = {}) {
     const params = this._buildCreateFileParams(filename, data, contentType, options);
     const endpoint = this._endpoint || `https://${this._bucket}.s3.${this._region}.amazonaws.com`;
+    const location = `${endpoint}/${this._encodeKey(params.Key)}`;
 
     // Streaming upload path
     if (typeof data?.pipe === 'function') {
@@ -196,7 +204,7 @@ class S3Adapter {
         this.createBucket()
           .then(() => upload.done())
           .then(
-            (response) => resolve(Object.assign(response || {}, { Location: `${endpoint}/${params.Key}` })),
+            (response) => resolve(Object.assign(response || {}, { Location: location })),
             reject
           );
       });
@@ -206,7 +214,7 @@ class S3Adapter {
     await this.createBucket();
     const command = new PutObjectCommand(params);
     const response = await this._s3Client.send(command);
-    return Object.assign(response || {}, { Location: `${endpoint}/${params.Key}` });
+    return Object.assign(response || {}, { Location: location });
   }
 
   async deleteFile(filename) {
@@ -247,12 +255,16 @@ class S3Adapter {
   // The location is the direct S3 link if the option is set,
   // otherwise we serve the file through parse-server
   async getFileLocation(config, filename) {
-    const fileName = filename.split('/').map(encodeURIComponent).join('/');
+    const fileName = this._encodeKey(filename);
     if (!this._directAccess) {
       return `${config.mount}/files/${config.applicationId}/${fileName}`;
     }
 
     const fileKey = `${this._bucketPrefix}${fileName}`;
+    // The prefix belongs to the url too, so it is encoded with the filename
+    // rather than concatenated raw. Otherwise a prefix containing a space
+    // produces a different url here than the location createFile reports.
+    const encodedFileKey = this._encodeKey(`${this._bucketPrefix}${filename}`);
 
     let presignedUrl = '';
     if (this._presignedUrl) {
@@ -268,10 +280,10 @@ class S3Adapter {
     }
 
     if (!this._baseUrl) {
-      return `https://${this._bucket}.s3.amazonaws.com/${fileKey}`;
+      return `https://${this._bucket}.s3.amazonaws.com/${encodedFileKey}`;
     }
 
-    const baseUrlFileKey = this._baseUrlDirect ? fileName : fileKey;
+    const baseUrlFileKey = this._baseUrlDirect ? fileName : encodedFileKey;
     return await buildDirectAccessUrl(this._baseUrl, baseUrlFileKey, presignedUrl, config, filename);
   }
 

spec/test.spec.js

@@ -905,6 +905,60 @@ describe('S3Adapter tests', () => {
       expect(s3ClientMock.send).toHaveBeenCalledWith(jasmine.any(PutObjectCommand));
     });
 
+    it('should url encode the returned location', async () => {
+      const s3 = new S3Adapter(options);
+      s3._s3Client = s3ClientMock;
+
+      const { Location } = await s3.createFile('my file (1).txt', 'hello world', 'text/utf8', {});
+
+      // The key keeps its raw form for S3, only the URL is encoded, and the
+      // prefix separator stays a separator.
+      expect(Location).toContain('/test/my%20file%20(1).txt');
+      expect(Location).not.toContain('my file (1).txt');
+      expect(Location).not.toContain('test%2Fmy');
+      expect(new URL(Location).pathname).toBe('/test/my%20file%20(1).txt');
+    });
+
+    it('should encode the bucket prefix in both urls that name the object', async () => {
+      const s3 = new S3Adapter({
+        bucket: 'bucket-1',
+        bucketPrefix: 'my folder/',
+        directAccess: true,
+      });
+      s3._s3Client = s3ClientMock;
+
+      const { Location } = await s3.createFile('a b.txt', 'hello world', 'text/utf8', {});
+      const url = await s3.getFileLocation(
+        { mount: 'http://my.server.com/parse', applicationId: 'xxxx' },
+        'a b.txt'
+      );
+
+      // Previously getFileLocation left the prefix raw, so the two disagreed.
+      expect(Location).toContain('/my%20folder/a%20b.txt');
+      expect(url).toContain('/my%20folder/a%20b.txt');
+    });
+
+    it('should url encode the returned location for a stream', async () => {
+      const rewiredModule = rewire('../index');
+      rewiredModule.__set__('Upload', function () {
+        this.done = () => Promise.resolve();
+        this.abort = () => Promise.resolve();
+      });
+      const RewiredS3Adapter = rewiredModule;
+      const s3 = new RewiredS3Adapter(options);
+      s3._s3Client = s3ClientMock;
+      s3._hasBucket = true;
+
+      const stream = new Readable();
+      stream.push('hello world');
+      stream.push(null);
+
+      const { Location } = await s3.createFile('my file (1).txt', stream, 'text/plain', {});
+
+      expect(Location).toContain('/test/my%20file%20(1).txt');
+      expect(Location).not.toContain('my file (1).txt');
+    });
+
     it('should save a stream with metadata added', async () => {
       const rewiredModule = rewire('../index');
       let uploadParams;