From 035b4074bc79250aeb3353c89d2bfbb391b3f3d5 Mon Sep 17 00:00:00 2001 From: Adrian Curtin <48138055+AdrianCurtin@users.noreply.github.com> Date: Wed, 13 Aug 2025 13:23:23 -0400 Subject: [PATCH] feat: built-in s3 key sanitizer (when creating files) optionally enabled, should have no effect on valid filenames --- index.js | 83 ++++++++++++++++++++++++++++++++++++++++++++--- spec/test.spec.js | 67 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index 9b94ddf..ff1ed46 100644 --- a/index.js +++ b/index.js @@ -47,6 +47,70 @@ async function buildDirectAccessUrl(baseUrl, baseUrlFileKey, presignedUrl, confi return directAccessUrl; } +function cleanS3Key(key) { + // Single-pass character replacement using a lookup map for better performance + // See: https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines + + const charMap = { + '\\': '-', + '{': '-', '}': '-', + '^': '-', '`': '-', '[': '-', ']': '-', '"': '-', '<': '-', '>': '-', + '~': '-', '#': '-', '|': '-', '%': '-', + '&': '-and-', + '$': '-dollar-', + '@': '-at-', + '=': '-equals-', + ';': '-', ':': '-', '+': '-plus-', ',': '-', '?': '-' + }; + + let result = ''; + let lastWasHyphen = false; + + for (let i = 0; i < key.length; i++) { + const char = key[i]; + const charCode = char.charCodeAt(0); + + // Skip non-printable ASCII and extended ASCII + if ((charCode >= 0 && charCode <= 31) || charCode === 127 || charCode >= 128) { + continue; + } + + // Handle whitespace - convert to single hyphen + if (/\s/.test(char)) { + if (!lastWasHyphen) { + result += '-'; + lastWasHyphen = true; + } + continue; + } + + // Replace problematic characters + if (charMap[char]) { + const replacement = charMap[char]; + if (replacement === '-') { + if (!lastWasHyphen) { + result += '-'; + lastWasHyphen = true; + } + } else { + result += replacement; + lastWasHyphen = false; + } + continue; + } + + // Keep safe characters + result += char; + lastWasHyphen = false; + } + + // Remove leading/trailing hyphens and periods + result = result.replace(/^[-\.]+|[-\.]+$/g, ''); + + // Ensure we don't end up with an empty string + return result || `file-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; +} + function responseToBuffer(response) { return new Promise((resolve, reject) => { const chunks = []; @@ -75,6 +139,7 @@ class S3Adapter { this._presignedUrlExpires = parseInt(options.presignedUrlExpires, 10); this._encryption = options.ServerSideEncryption; this._generateKey = options.generateKey; + this._cleanKey = options.cleanKey; this._endpoint = options.s3overrides?.endpoint; // Optional FilesAdaptor method this.validateFilename = options.validateFilename; @@ -141,15 +206,23 @@ class S3Adapter { // 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 = {}) { + let finalKey = filename; + + // Apply generateKey if provided + if (this._generateKey instanceof Function) { + finalKey = this._generateKey(filename, contentType, options); + } + + // Apply key cleaning if enabled (after generateKey) + if (this._cleanKey) { + finalKey = cleanS3Key(finalKey); + } + const params = { Bucket: this._bucket, - Key: this._bucketPrefix + filename, + Key: this._bucketPrefix + finalKey, Body: data, }; - - if (this._generateKey instanceof Function) { - params.Key = this._bucketPrefix + this._generateKey(filename); - } if (this._fileAcl) { if (this._fileAcl === 'none') { delete params.ACL; diff --git a/spec/test.spec.js b/spec/test.spec.js index 5402460..0334c98 100644 --- a/spec/test.spec.js +++ b/spec/test.spec.js @@ -867,6 +867,73 @@ describe('S3Adapter tests', () => { expect(commandArg).toBeInstanceOf(PutObjectCommand); expect(commandArg.input.ACL).toBeUndefined(); }); + + it('should clean S3 keys when cleanKey option is enabled', async () => { + const options = { + bucket: 'bucket-1', + cleanKey: true + }; + const s3 = new S3Adapter(options); + s3ClientMock.send.and.returnValue(Promise.resolve({})); + s3._s3Client = s3ClientMock; + + // Test filename with problematic characters + const problematicFilename = 'test file{with}[bad]chars<>&%?.txt'; + await s3.createFile(problematicFilename, 'hello world', 'text/utf8', {}); + + expect(s3ClientMock.send).toHaveBeenCalledTimes(2); + const commands = s3ClientMock.send.calls.all(); + const commandArg = commands[1].args[0]; + expect(commandArg).toBeInstanceOf(PutObjectCommand); + + // Should have cleaned the filename + const expectedCleanKey = 'test-file-with-chars-and-.txt'; + expect(commandArg.input.Key).toBe(expectedCleanKey); + }); + + it('should clean generated keys when cleanKey option is enabled', async () => { + const options = { + bucket: 'bucket-1', + cleanKey: true, + generateKey: (filename) => `generated/${filename.toUpperCase()}` + }; + const s3 = new S3Adapter(options); + s3ClientMock.send.and.returnValue(Promise.resolve({})); + s3._s3Client = s3ClientMock; + + const problematicFilename = 'test{bad}.txt'; + await s3.createFile(problematicFilename, 'hello world', 'text/utf8', {}); + + expect(s3ClientMock.send).toHaveBeenCalledTimes(2); + const commands = s3ClientMock.send.calls.all(); + const commandArg = commands[1].args[0]; + expect(commandArg).toBeInstanceOf(PutObjectCommand); + + // Should have cleaned the generated key (not the original filename) + const expectedCleanKey = 'generated/TEST-BAD-.TXT'; + expect(commandArg.input.Key).toBe(expectedCleanKey); + }); + + it('should not clean keys when cleanKey option is disabled', async () => { + const options = { + bucket: 'bucket-1', + cleanKey: false // explicitly disabled + }; + const s3 = new S3Adapter(options); + s3ClientMock.send.and.returnValue(Promise.resolve({})); + s3._s3Client = s3ClientMock; + + const problematicFilename = 'test file{with}[bad]chars.txt'; + await s3.createFile(problematicFilename, 'hello world', 'text/utf8', {}); + + expect(s3ClientMock.send).toHaveBeenCalledTimes(2); + const commands = s3ClientMock.send.calls.all(); + const commandArg = commands[1].args[0]; + expect(commandArg).toBeInstanceOf(PutObjectCommand); + + // Should preserve original filename + expect(commandArg.input.Key).toBe(problematicFilename); + }); }); describe('handleFileStream', () => {