diff --git a/index.js b/index.js index 51864bb..2b0f70d 100644 --- a/index.js +++ b/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); } diff --git a/spec/test.spec.js b/spec/test.spec.js index 2fed658..0639172 100644 --- a/spec/test.spec.js +++ b/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;