From 0a61ffeefe9f492b52e72095624940447cacc367 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Mon, 18 Mar 2024 11:22:42 +0900 Subject: [PATCH 1/8] @uppy/aws-s3-multipart: Fix escaping issue with client signed request. fixes #5005 --- .../aws-s3-multipart/src/createSignedURL.js | 54 +++++++++++++------ .../src/createSignedURL.test.js | 37 ++++++++++++- 2 files changed, 75 insertions(+), 16 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js index 593b327b6e..c7804c1681 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js @@ -76,6 +76,30 @@ async function hash (key, data) { return subtle.sign(algorithm, await generateHmacKey(key), ec.encode(data)) } +/** + * @see + * https://github.com/smithy-lang/smithy-typescript/blob/main/packages/smithy-client/src/extended-encode-uri-component.ts + */ +function extendedEncodeURIComponent(str) { + return encodeURIComponent(str).replace(/[!'()*]/g, (c) => + `%${c.charCodeAt(0).toString(16).toUpperCase()}` + ) +} + +class Query { + #params = [] + + append(key, value) { + this.#params.push([key, value]) + } + + toString() { + return this.#params + .map(([key, value]) => `${extendedEncodeURIComponent(key)}=${extendedEncodeURIComponent(value)}`) + .join('&') + } +} + /** * @see https://docs.aws.amazon.com/IAM/latest/UserGuide/create-signed-request.html * @param {Record} param0 @@ -90,32 +114,32 @@ export default async function createSignedURL ({ }) { const Service = 's3' const host = `${bucketName}.${Service}.${Region}.amazonaws.com` - const CanonicalUri = `/${encodeURI(Key)}` + const CanonicalUri = `/${Key.split('/').map(extendedEncodeURIComponent).join('/')}` const payload = 'UNSIGNED-PAYLOAD' const requestDateTime = new Date().toISOString().replace(/[-:]|\.\d+/g, '') // YYYYMMDDTHHMMSSZ const date = requestDateTime.slice(0, 8) // YYYYMMDD const scope = `${date}/${Region}/${Service}/aws4_request` - const url = new URL(`https://${host}${CanonicalUri}`) + const query = new Query(); // N.B.: URL search params needs to be added in the ASCII order - url.searchParams.set('X-Amz-Algorithm', 'AWS4-HMAC-SHA256') - url.searchParams.set('X-Amz-Content-Sha256', payload) - url.searchParams.set('X-Amz-Credential', `${accountKey}/${scope}`) - url.searchParams.set('X-Amz-Date', requestDateTime) - url.searchParams.set('X-Amz-Expires', expires) + query.append('X-Amz-Algorithm', 'AWS4-HMAC-SHA256') + query.append('X-Amz-Content-Sha256', payload) + query.append('X-Amz-Credential', `${accountKey}/${scope}`) + query.append('X-Amz-Date', requestDateTime) + query.append('X-Amz-Expires', expires) // We are signing on the client, so we expect there's going to be a session token: - url.searchParams.set('X-Amz-Security-Token', sessionToken) - url.searchParams.set('X-Amz-SignedHeaders', 'host') + query.append('X-Amz-Security-Token', sessionToken) + query.append('X-Amz-SignedHeaders', 'host') // Those two are present only for Multipart Uploads: - if (partNumber) url.searchParams.set('partNumber', partNumber) - if (uploadId) url.searchParams.set('uploadId', uploadId) - url.searchParams.set('x-id', partNumber && uploadId ? 'UploadPart' : 'PutObject') + if (partNumber) query.append('partNumber', partNumber) + if (uploadId) query.append('uploadId', uploadId) + query.append('x-id', partNumber && uploadId ? 'UploadPart' : 'PutObject') // Step 1: Create a canonical request const canonical = createCanonicalRequest({ CanonicalUri, - CanonicalQueryString: url.search.slice(1), + CanonicalQueryString: query.toString(), SignedHeaders: { host, }, @@ -141,7 +165,7 @@ export default async function createSignedURL ({ const signature = arrayBufferToHexString(await hash(kSigning, stringToSign)) // Step 5: Add the signature to the request - url.searchParams.set('X-Amz-Signature', signature) + query.append('X-Amz-Signature', signature) - return url + return new URL(`https://${host}${CanonicalUri}?${query.toString()}`) } diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js index cfcb12742c..7687f1d24e 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js @@ -51,6 +51,8 @@ describe('createSignedURL', () => { ) }) it('should be able to sign multipart upload', async () => { + const url = new URL('https://example.com/?%21%27%28%29%2A') + console.log(url.search) const client = new S3Client(s3ClientOptions) const partNumber = 99 const uploadId = 'dummyUploadId' @@ -71,7 +73,40 @@ describe('createSignedURL', () => { UploadId: uploadId, PartNumber: partNumber, Key: 'some/key', - }, { expiresIn: 900 }))).searchParams.get('X-Amz-Signature'), + }), { expiresIn: 900 })).searchParams.get('X-Amz-Signature'), + ) + }) + it('should escape path and query as restricted to RFC 3986', async () => { + const client = new S3Client(s3ClientOptions) + const partNumber = 99 + const uploadId = 'Upload!\'()*Id' + const Key = '!\'()*/!\'()*.ext' + const implResult = + await createSignedURL({ + accountKey: s3ClientOptions.credentials.accessKeyId, + accountSecret: s3ClientOptions.credentials.secretAccessKey, + sessionToken: s3ClientOptions.credentials.sessionToken, + uploadId, + partNumber, + bucketName, + Key, + Region: s3ClientOptions.region, + expires: 900, + }) + const sdkResult = + new URL( + await getSignedUrl(client, new UploadPartCommand({ + Bucket: bucketName, + UploadId: uploadId, + PartNumber: partNumber, + Key, + }), { expiresIn: 900 } + ) ) + assert.strictEqual(implResult.pathname, sdkResult.pathname) + + const extractUploadIdQueryValue = (result) => + result.search.match(/uploadId=(.+?)(&|$)/)[1] + assert.strictEqual(extractUploadIdQueryValue(implResult), extractUploadIdQueryValue(sdkResult)) }) }) From 3ab6024bfca48658a7fea4a4e202c3033a22d5a8 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 11:15:32 +0900 Subject: [PATCH 2/8] @uppy/aws-s3-multipart: Escape more special chars in path and less chars in query on self-signing request. Thanks to review from @aduh95 --- .../aws-s3-multipart/src/createSignedURL.js | 60 +++++++------------ .../src/createSignedURL.test.js | 6 +- 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js index c7804c1681..67a57d4f57 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js @@ -76,28 +76,8 @@ async function hash (key, data) { return subtle.sign(algorithm, await generateHmacKey(key), ec.encode(data)) } -/** - * @see - * https://github.com/smithy-lang/smithy-typescript/blob/main/packages/smithy-client/src/extended-encode-uri-component.ts - */ -function extendedEncodeURIComponent(str) { - return encodeURIComponent(str).replace(/[!'()*]/g, (c) => - `%${c.charCodeAt(0).toString(16).toUpperCase()}` - ) -} - -class Query { - #params = [] - - append(key, value) { - this.#params.push([key, value]) - } - - toString() { - return this.#params - .map(([key, value]) => `${extendedEncodeURIComponent(key)}=${extendedEncodeURIComponent(value)}`) - .join('&') - } +function percentEncode(c) { + return `%${c.charCodeAt(0).toString(16).toUpperCase()}` } /** @@ -114,32 +94,38 @@ export default async function createSignedURL ({ }) { const Service = 's3' const host = `${bucketName}.${Service}.${Region}.amazonaws.com` - const CanonicalUri = `/${Key.split('/').map(extendedEncodeURIComponent).join('/')}` + /** + * List of char out of `encodeURI()` is taken from ECMAScript spec + * + * @see https://tc39.es/ecma262/#sec-encodeuri-uri + */ + const CanonicalUri = `/${encodeURI(Key).replace(/[;?:@&=+$,#!'()*]/g, percentEncode)}` + console.log(CanonicalUri) const payload = 'UNSIGNED-PAYLOAD' const requestDateTime = new Date().toISOString().replace(/[-:]|\.\d+/g, '') // YYYYMMDDTHHMMSSZ const date = requestDateTime.slice(0, 8) // YYYYMMDD const scope = `${date}/${Region}/${Service}/aws4_request` - const query = new Query(); + const url = new URL(`https://${host}${CanonicalUri}`) // N.B.: URL search params needs to be added in the ASCII order - query.append('X-Amz-Algorithm', 'AWS4-HMAC-SHA256') - query.append('X-Amz-Content-Sha256', payload) - query.append('X-Amz-Credential', `${accountKey}/${scope}`) - query.append('X-Amz-Date', requestDateTime) - query.append('X-Amz-Expires', expires) + url.searchParams.set('X-Amz-Algorithm', 'AWS4-HMAC-SHA256') + url.searchParams.set('X-Amz-Content-Sha256', payload) + url.searchParams.set('X-Amz-Credential', `${accountKey}/${scope}`) + url.searchParams.set('X-Amz-Date', requestDateTime) + url.searchParams.set('X-Amz-Expires', expires) // We are signing on the client, so we expect there's going to be a session token: - query.append('X-Amz-Security-Token', sessionToken) - query.append('X-Amz-SignedHeaders', 'host') + url.searchParams.set('X-Amz-Security-Token', sessionToken) + url.searchParams.set('X-Amz-SignedHeaders', 'host') // Those two are present only for Multipart Uploads: - if (partNumber) query.append('partNumber', partNumber) - if (uploadId) query.append('uploadId', uploadId) - query.append('x-id', partNumber && uploadId ? 'UploadPart' : 'PutObject') + if (partNumber) url.searchParams.set('partNumber', partNumber) + if (uploadId) url.searchParams.set('uploadId', uploadId) + url.searchParams.set('x-id', partNumber && uploadId ? 'UploadPart' : 'PutObject') // Step 1: Create a canonical request const canonical = createCanonicalRequest({ CanonicalUri, - CanonicalQueryString: query.toString(), + CanonicalQueryString: url.search.slice(1), SignedHeaders: { host, }, @@ -165,7 +151,7 @@ export default async function createSignedURL ({ const signature = arrayBufferToHexString(await hash(kSigning, stringToSign)) // Step 5: Add the signature to the request - query.append('X-Amz-Signature', signature) + url.searchParams.set('X-Amz-Signature', signature) - return new URL(`https://${host}${CanonicalUri}?${query.toString()}`) + return url; } diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js index 7687f1d24e..fdc5c0d375 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js @@ -79,8 +79,10 @@ describe('createSignedURL', () => { it('should escape path and query as restricted to RFC 3986', async () => { const client = new S3Client(s3ClientOptions) const partNumber = 99 - const uploadId = 'Upload!\'()*Id' - const Key = '!\'()*/!\'()*.ext' + const specialChars = ';?:@&=+$,#!\'()' + const uploadId = `Upload${specialChars}Id` + // '.*' chars of path should be encoded + const Key = `${specialChars}.*/${specialChars}.*.ext` const implResult = await createSignedURL({ accountKey: s3ClientOptions.credentials.accessKeyId, From eb5833257352d5f679a29ac7f7a7df51a016b260 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 18:51:09 +0900 Subject: [PATCH 3/8] @uppy/aws-s3-multipart: Update comment to more explicitly Co-authored-by: Antoine du Hamel --- packages/@uppy/aws-s3-multipart/src/createSignedURL.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js index 67a57d4f57..264d4b7dc6 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js @@ -95,7 +95,8 @@ export default async function createSignedURL ({ const Service = 's3' const host = `${bucketName}.${Service}.${Region}.amazonaws.com` /** - * List of char out of `encodeURI()` is taken from ECMAScript spec + * List of char out of `encodeURI()` is taken from ECMAScript spec. + * Note that the `/` character is purposefully not included in list below. * * @see https://tc39.es/ecma262/#sec-encodeuri-uri */ From 52ca44623a157173e0e060c068df3e25a5da9706 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 18:53:10 +0900 Subject: [PATCH 4/8] @uppy/aws-s3-multipart: Remove debug code --- packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js index fdc5c0d375..6866249577 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js @@ -51,8 +51,6 @@ describe('createSignedURL', () => { ) }) it('should be able to sign multipart upload', async () => { - const url = new URL('https://example.com/?%21%27%28%29%2A') - console.log(url.search) const client = new S3Client(s3ClientOptions) const partNumber = 99 const uploadId = 'dummyUploadId' From 66e7bd6fb1cc60329e8fb29c313481fc47b77e7e Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 18:56:25 +0900 Subject: [PATCH 5/8] @uppy/aws-s3-multipart: Improve test case --- .../@uppy/aws-s3-multipart/src/createSignedURL.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js index 6866249577..b3cbd88d99 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js @@ -105,8 +105,9 @@ describe('createSignedURL', () => { ) assert.strictEqual(implResult.pathname, sdkResult.pathname) - const extractUploadIdQueryValue = (result) => - result.search.match(/uploadId=(.+?)(&|$)/)[1] - assert.strictEqual(extractUploadIdQueryValue(implResult), extractUploadIdQueryValue(sdkResult)) + const extractUploadId = /([?&])uploadId=([^&]+?)(&|$)/ + const extractSignature = /([?&])X-Amz-Signature=([^&]+?)(&|$)/ + assert.strictEqual(implResult.search.match(extractUploadId)[2], sdkResult.search.match(extractUploadId)[2]) + assert.strictEqual(implResult.search.match(extractSignature)[2], sdkResult.search.match(extractSignature)[2]) }) }) From af0eaa413e9c464cee2de18f4845693621296675 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 18:59:11 +0900 Subject: [PATCH 6/8] @uppy/aws-s3-multipart: Remove trailing comma --- packages/@uppy/aws-s3-multipart/src/createSignedURL.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js index 264d4b7dc6..54d842e875 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js @@ -154,5 +154,5 @@ export default async function createSignedURL ({ // Step 5: Add the signature to the request url.searchParams.set('X-Amz-Signature', signature) - return url; + return url } From 40e77e78d67103935bbc797289e2b050e924ebb3 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 19:03:00 +0900 Subject: [PATCH 7/8] packages/@uppy/aws-s3-multipart Remove debug code --- packages/@uppy/aws-s3-multipart/src/createSignedURL.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js index 54d842e875..bef07e5b9c 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.js @@ -101,7 +101,6 @@ export default async function createSignedURL ({ * @see https://tc39.es/ecma262/#sec-encodeuri-uri */ const CanonicalUri = `/${encodeURI(Key).replace(/[;?:@&=+$,#!'()*]/g, percentEncode)}` - console.log(CanonicalUri) const payload = 'UNSIGNED-PAYLOAD' const requestDateTime = new Date().toISOString().replace(/[-:]|\.\d+/g, '') // YYYYMMDDTHHMMSSZ From 91a0bf6d16ce4b87fe67604f1e148289dba33a53 Mon Sep 17 00:00:00 2001 From: Hiroki Shimizu Date: Tue, 19 Mar 2024 19:04:22 +0900 Subject: [PATCH 8/8] @uppy/aws-s3-multipart: Test case, argument was not match place --- packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js index b3cbd88d99..bb873829eb 100644 --- a/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js +++ b/packages/@uppy/aws-s3-multipart/src/createSignedURL.test.js @@ -47,7 +47,7 @@ describe('createSignedURL', () => { Bucket: bucketName, Fields: {}, Key: 'some/key', - }, { expiresIn: 900 }))).searchParams.get('X-Amz-Signature'), + }), { expiresIn: 900 })).searchParams.get('X-Amz-Signature'), ) }) it('should be able to sign multipart upload', async () => {