fix(S3 Node): Percent-encode the copy source path (#40282)

packages/nodes-base/credentials/common/aws/utils.test.ts

@@ -9,6 +9,7 @@ import {
 	AWS_REGION_SHAPE_PATTERN,
 	awsGetSignInOptionsAndUpdateRequest,
 	parseAwsUrl,
+	uriEncodeS3CopySource,
 	uriEncodeS3Pathname,
 	validateBedrockEndpointOverride,
 } from './utils';
@@ -1891,3 +1892,37 @@ describe('awsGetSignInOptionsAndUpdateRequest — S3 object-path canonicalizatio
 		);
 	});
 });
+
+describe('uriEncodeS3CopySource', () => {
+	// CopyObject reads an optional `?versionId=<id>` selector after the key. The
+	// key is encoded; the selector must stay readable or S3 reads it as key text.
+	it.each([
+		['/bucket/résumé.pdf', '/bucket/r%C3%A9sum%C3%A9.pdf'],
+		['/bucket/résumé.pdf?versionId=3HL4kqt', '/bucket/r%C3%A9sum%C3%A9.pdf?versionId=3HL4kqt'],
+		['/bucket/my report.pdf?versionId=3HL4kqt', '/bucket/my%20report.pdf?versionId=3HL4kqt'],
+		['/bucket/key.txt?versionId=null', '/bucket/key.txt?versionId=null'],
+	])('encodes %s as %s', (copySource, expected) => {
+		expect(uriEncodeS3CopySource(copySource)).toBe(expected);
+	});
+
+	it('encodes a key holding a question mark that is not a version selector', () => {
+		expect(uriEncodeS3CopySource('/bucket/what?.pdf')).toBe('/bucket/what%3F.pdf');
+	});
+
+	it('splits on the last selector, so an earlier one stays part of the key', () => {
+		expect(uriEncodeS3CopySource('/bucket/a?versionId=x?versionId=y')).toBe(
+			'/bucket/a%3FversionId%3Dx?versionId=y',
+		);
+	});
+
+	it('is idempotent', () => {
+		for (const copySource of [
+			'/bucket/résumé.pdf?versionId=3HL4kqt',
+			'/bucket/plain.txt',
+			'/bucket/my report.pdf',
+		]) {
+			const once = uriEncodeS3CopySource(copySource);
+			expect(uriEncodeS3CopySource(once)).toBe(once);
+		}
+	});
+});

packages/nodes-base/credentials/common/aws/utils.ts

@@ -450,6 +450,21 @@ export function uriEncodeS3Pathname(
 	return options.preserveEncodedSlashes ? encodedPathname : encodedPathname.replace(/%2F/g, '/');
 }
 
+const COPY_SOURCE_VERSION_SELECTOR = '?versionId=';
+
+/**
+ * Encodes the value of the S3 `x-amz-copy-source` header. CopyObject reads an
+ * optional `?versionId=<id>` selector after the key, so the key is encoded as a
+ * pathname and the selector is kept as written. Encoding the selector too would
+ * make S3 read it as the last characters of the key.
+ */
+export function uriEncodeS3CopySource(copySource: string): string {
+	const selectorIndex = copySource.lastIndexOf(COPY_SOURCE_VERSION_SELECTOR);
+	if (selectorIndex === -1) return uriEncodeS3Pathname(copySource);
+
+	return uriEncodeS3Pathname(copySource.slice(0, selectorIndex)) + copySource.slice(selectorIndex);
+}
+
 /**
  * Prepares AWS request options for signing by constructing the proper endpoint URL,
  * handling query parameters, and setting up the request body for AWS4 signature.

packages/nodes-base/nodes/Aws/S3/V1/AwsS3V1.node.ts

@@ -11,6 +11,7 @@ import type {
 import { NodeConnectionTypes, NodeOperationError } from 'n8n-workflow';
 import { Builder } from 'xml2js';
 
+import { uriEncodeS3CopySource } from '../../../../credentials/common/aws/utils';
 import { bucketFields, bucketOperations } from './BucketDescription';
 import { fileFields, fileOperations } from './FileDescription';
 import { folderFields, folderOperations } from './FolderDescription';
@@ -477,7 +478,10 @@ export class AwsS3V1 implements INodeType {
 						const destinationPath = this.getNodeParameter('destinationPath', i) as string;
 						const additionalFields = this.getNodeParameter('additionalFields', i);
 
-						headers['x-amz-copy-source'] = sourcePath;
+						// The signature covers this header, but an HTTP header cannot carry a
+						// non-ASCII byte, so the client rewrites the value after signing and S3
+						// rejects the mismatch. Encode it the same way the signed path is encoded.
+						headers['x-amz-copy-source'] = uriEncodeS3CopySource(sourcePath);
 
 						if (additionalFields.requesterPays) {
 							headers['x-amz-request-payer'] = 'requester';

packages/nodes-base/nodes/Aws/S3/V2/AwsS3V2.node.ts

@@ -12,6 +12,7 @@ import { NodeConnectionTypes, NodeOperationError } from 'n8n-workflow';
 import type { Readable } from 'stream';
 import { Builder } from 'xml2js';
 
+import { uriEncodeS3CopySource } from '../../../../credentials/common/aws/utils';
 import { bucketFields, bucketOperations } from './BucketDescription';
 import { fileFields, fileOperations } from './FileDescription';
 import { folderFields, folderOperations } from './FolderDescription';
@@ -497,7 +498,10 @@ export class AwsS3V2 implements INodeType {
 						const destinationPath = this.getNodeParameter('destinationPath', i) as string;
 						const additionalFields = this.getNodeParameter('additionalFields', i);
 
-						headers['x-amz-copy-source'] = sourcePath;
+						// The signature covers this header, but an HTTP header cannot carry a
+						// non-ASCII byte, so the client rewrites the value after signing and S3
+						// rejects the mismatch. Encode it the same way the signed path is encoded.
+						headers['x-amz-copy-source'] = uriEncodeS3CopySource(sourcePath);
 
 						if (additionalFields.requesterPays) {
 							headers['x-amz-request-payer'] = 'requester';

packages/nodes-base/nodes/Aws/S3/test/V1/AwsS3.node.operation.test.ts

@@ -0,0 +1,110 @@
+import { mockDeep } from 'vitest-mock-extended';
+import type { IExecuteFunctions, INode } from 'n8n-workflow';
+
+import { AwsS3V1 } from '../../V1/AwsS3V1.node';
+import * as GenericFunctions from '../../V1/GenericFunctions';
+import type { MockInstance } from 'vitest';
+
+describe('AWS S3 V1 Node - File Copy', () => {
+	const executeFunctionsMock = mockDeep<IExecuteFunctions>();
+	let awsApiRequestSOAPSpy: MockInstance;
+	let node: AwsS3V1;
+
+	const copyWithSource = (sourcePath: string) => {
+		executeFunctionsMock.getNodeParameter.mockImplementation((paramName) => {
+			switch (paramName) {
+				case 'resource':
+					return 'file';
+				case 'operation':
+					return 'copy';
+				case 'sourcePath':
+					return sourcePath;
+				case 'destinationPath':
+					return '/test-bucket/backup/index.txt';
+				case 'additionalFields':
+					return {};
+				default:
+					return undefined;
+			}
+		});
+	};
+
+	const copySourceHeader = () =>
+		awsApiRequestSOAPSpy.mock.calls[1][5]['x-amz-copy-source'] as string;
+
+	beforeEach(() => {
+		vi.resetAllMocks();
+		awsApiRequestSOAPSpy = vi.spyOn(GenericFunctions, 'awsApiRequestSOAP');
+		node = new AwsS3V1({
+			displayName: 'AWS S3',
+			name: 'awsS3',
+			icon: 'file:s3.svg',
+			group: ['output'],
+			description: 'Sends data to AWS S3',
+		});
+
+		executeFunctionsMock.getCredentials.mockResolvedValue({
+			accessKeyId: 'test-key',
+			secretAccessKey: 'test-secret',
+			region: 'eu-central-1',
+		});
+		executeFunctionsMock.getNode.mockReturnValue({ typeVersion: 1 } as INode);
+		executeFunctionsMock.getInputData.mockReturnValue([{ json: {} }]);
+		executeFunctionsMock.continueOnFail.mockReturnValue(false);
+		executeFunctionsMock.helpers.returnJsonArray.mockImplementation((data) =>
+			Array.isArray(data) ? data.map((item) => ({ json: item })) : [{ json: data }],
+		);
+		executeFunctionsMock.helpers.constructExecutionMetaData.mockImplementation(
+			(data) => data as any,
+		);
+
+		awsApiRequestSOAPSpy
+			.mockResolvedValueOnce({ LocationConstraint: { _: 'eu-central-1' } })
+			.mockResolvedValueOnce({ CopyObjectResult: {} });
+	});
+
+	// An HTTP header cannot carry a non-ASCII byte. The signature is computed over
+	// the value we set, so if the client rewrites it on the way out, S3 sees a
+	// different path and answers SignatureDoesNotMatch.
+	it.each([
+		[
+			'Chinese characters',
+			'/test-bucket/workflow/同程商旅/index.txt',
+			'/test-bucket/workflow/%E5%90%8C%E7%A8%8B%E5%95%86%E6%97%85/index.txt',
+		],
+		['an accented name', '/test-bucket/résumé.pdf', '/test-bucket/r%C3%A9sum%C3%A9.pdf'],
+		['a space', '/test-bucket/my report.pdf', '/test-bucket/my%20report.pdf'],
+	])('percent-encodes the copy source for %s', async (_name, sourcePath, expected) => {
+		copyWithSource(sourcePath);
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe(expected);
+	});
+
+	it('leaves an all-ASCII source path byte-identical', async () => {
+		copyWithSource('/test-bucket/workflow/index.txt');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/workflow/index.txt');
+	});
+
+	it('does not double-encode a source path the user already encoded', async () => {
+		copyWithSource('/test-bucket/my%20report.pdf');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/my%20report.pdf');
+	});
+
+	// CopyObject reads an optional `?versionId=` selector after the key. Encoding
+	// the `?` would make S3 read the selector as the end of the key.
+	it('keeps the version selector readable while encoding the key', async () => {
+		copyWithSource('/test-bucket/r\u00e9sum\u00e9.pdf?versionId=3HL4kqt');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/r%C3%A9sum%C3%A9.pdf?versionId=3HL4kqt');
+	});
+});

packages/nodes-base/nodes/Aws/S3/test/V2/AwsS3.node.operation.test.ts

@@ -524,3 +524,105 @@ describe('AWS S3 V2 Node - Bucket Search', () => {
 		expect(result[0][0].json).toEqual(singleItem);
 	});
 });
+
+describe('AWS S3 V2 Node - File Copy', () => {
+	const executeFunctionsMock = mockDeep<IExecuteFunctions>();
+	let awsApiRequestRESTSpy: MockInstance;
+	let node: AwsS3V2;
+
+	const copyWithSource = (sourcePath: string) => {
+		executeFunctionsMock.getNodeParameter.mockImplementation((paramName) => {
+			switch (paramName) {
+				case 'resource':
+					return 'file';
+				case 'operation':
+					return 'copy';
+				case 'sourcePath':
+					return sourcePath;
+				case 'destinationPath':
+					return '/test-bucket/backup/index.txt';
+				case 'additionalFields':
+					return {};
+				default:
+					return undefined;
+			}
+		});
+	};
+
+	const copySourceHeader = () =>
+		awsApiRequestRESTSpy.mock.calls[1][5]['x-amz-copy-source'] as string;
+
+	beforeEach(() => {
+		vi.resetAllMocks();
+		awsApiRequestRESTSpy = vi.spyOn(GenericFunctions, 'awsApiRequestREST');
+		node = new AwsS3V2({
+			displayName: 'AWS S3',
+			name: 'awsS3',
+			icon: 'file:s3.svg',
+			group: ['output'],
+			description: 'Sends data to AWS S3',
+		});
+
+		executeFunctionsMock.getCredentials.mockResolvedValue({
+			accessKeyId: 'test-key',
+			secretAccessKey: 'test-secret',
+			region: 'eu-central-1',
+		});
+		executeFunctionsMock.getNode.mockReturnValue({ typeVersion: 2 } as INode);
+		executeFunctionsMock.getInputData.mockReturnValue([{ json: { test: 'data' } }]);
+		executeFunctionsMock.continueOnFail.mockReturnValue(false);
+		executeFunctionsMock.helpers.returnJsonArray.mockImplementation((data) =>
+			Array.isArray(data) ? data.map((item) => ({ json: item })) : [{ json: data }],
+		);
+		executeFunctionsMock.helpers.constructExecutionMetaData.mockImplementation(
+			(data) => data as any,
+		);
+
+		awsApiRequestRESTSpy.mockResolvedValueOnce(mockLocationResponse).mockResolvedValueOnce({});
+	});
+
+	// An HTTP header cannot carry a non-ASCII byte. The signature is computed over
+	// the value we set, so if the client rewrites it on the way out, S3 sees a
+	// different path and answers SignatureDoesNotMatch.
+	it.each([
+		[
+			'Chinese characters',
+			'/test-bucket/workflow/\u540c\u7a0b\u5546\u65c5/index.txt',
+			'/test-bucket/workflow/%E5%90%8C%E7%A8%8B%E5%95%86%E6%97%85/index.txt',
+		],
+		['an accented name', '/test-bucket/r\u00e9sum\u00e9.pdf', '/test-bucket/r%C3%A9sum%C3%A9.pdf'],
+		['a space', '/test-bucket/my report.pdf', '/test-bucket/my%20report.pdf'],
+	])('percent-encodes the copy source for %s', async (_name, sourcePath, expected) => {
+		copyWithSource(sourcePath);
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe(expected);
+	});
+
+	it('leaves an all-ASCII source path byte-identical', async () => {
+		copyWithSource('/test-bucket/workflow/index.txt');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/workflow/index.txt');
+	});
+
+	it('does not double-encode a source path the user already encoded', async () => {
+		copyWithSource('/test-bucket/my%20report.pdf');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/my%20report.pdf');
+	});
+
+	// CopyObject reads an optional `?versionId=` selector after the key. Encoding
+	// the `?` would make S3 read the selector as the end of the key.
+	it('keeps the version selector readable while encoding the key', async () => {
+		copyWithSource('/test-bucket/r\u00e9sum\u00e9.pdf?versionId=3HL4kqt');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/r%C3%A9sum%C3%A9.pdf?versionId=3HL4kqt');
+	});
+});

packages/nodes-base/nodes/S3/S3.node.ts

@@ -12,6 +12,7 @@ import { NodeApiError, NodeConnectionTypes, NodeOperationError } from 'n8n-workf
 import { Builder } from 'xml2js';
 
 import { s3ApiRequestREST, s3ApiRequestSOAP, s3ApiRequestSOAPAllItems } from './GenericFunctions';
+import { uriEncodeS3CopySource } from '../../credentials/common/aws/utils';
 import { bucketFields, bucketOperations } from '../Aws/S3/V1/BucketDescription';
 import { fileFields, fileOperations } from '../Aws/S3/V1/FileDescription';
 import { folderFields, folderOperations } from '../Aws/S3/V1/FolderDescription';
@@ -486,7 +487,10 @@ export class S3 implements INodeType {
 						const destinationPath = this.getNodeParameter('destinationPath', i) as string;
 						const additionalFields = this.getNodeParameter('additionalFields', i);
 
-						headers['x-amz-copy-source'] = sourcePath;
+						// The signature covers this header, but an HTTP header cannot carry a
+						// non-ASCII byte, so the client rewrites the value after signing and S3
+						// rejects the mismatch. Encode it the same way the signed path is encoded.
+						headers['x-amz-copy-source'] = uriEncodeS3CopySource(sourcePath);
 
 						if (additionalFields.requesterPays) {
 							headers['x-amz-request-payer'] = 'requester';

packages/nodes-base/nodes/S3/__tests__/S3.node.operation.test.ts

@@ -99,3 +99,102 @@ describe('S3 Node - Bucket Delete', () => {
 		expect(result).toEqual([[{ json: { error: 'BucketNotEmpty' } }]]);
 	});
 });
+
+describe('S3 Node - File Copy', () => {
+	const executeFunctionsMock = mockDeep<IExecuteFunctions>();
+	let s3ApiRequestSOAPSpy: MockInstance;
+	let node: S3;
+
+	const copyWithSource = (sourcePath: string) => {
+		executeFunctionsMock.getNodeParameter.mockImplementation((paramName) => {
+			switch (paramName) {
+				case 'resource':
+					return 'file';
+				case 'operation':
+					return 'copy';
+				case 'sourcePath':
+					return sourcePath;
+				case 'destinationPath':
+					return '/test-bucket/backup/index.txt';
+				case 'additionalFields':
+					return {};
+				default:
+					return undefined;
+			}
+		});
+	};
+
+	const copySourceHeader = () =>
+		s3ApiRequestSOAPSpy.mock.calls[1][5]['x-amz-copy-source'] as string;
+
+	beforeEach(() => {
+		vi.resetAllMocks();
+		s3ApiRequestSOAPSpy = vi.spyOn(GenericFunctions, 's3ApiRequestSOAP');
+		node = new S3();
+
+		executeFunctionsMock.getCredentials.mockResolvedValue({
+			endpoint: 'https://s3.amazonaws.com',
+			accessKeyId: 'test-key',
+			secretAccessKey: 'test-secret',
+			region: 'us-east-1',
+		});
+		executeFunctionsMock.getNode.mockReturnValue({ typeVersion: 1 } as INode);
+		executeFunctionsMock.getInputData.mockReturnValue([{ json: {} }]);
+		executeFunctionsMock.continueOnFail.mockReturnValue(false);
+		executeFunctionsMock.helpers.returnJsonArray.mockImplementation((data) =>
+			Array.isArray(data) ? data.map((item) => ({ json: item })) : [{ json: data }],
+		);
+		executeFunctionsMock.helpers.constructExecutionMetaData.mockImplementation(
+			(data) => data as any,
+		);
+
+		s3ApiRequestSOAPSpy
+			.mockResolvedValueOnce({ LocationConstraint: { _: 'us-east-1' } })
+			.mockResolvedValueOnce({ CopyObjectResult: {} });
+	});
+
+	// An HTTP header cannot carry a non-ASCII byte. The signature is computed over
+	// the value we set, so if the client rewrites it on the way out, S3 sees a
+	// different path and answers SignatureDoesNotMatch.
+	it.each([
+		[
+			'Chinese characters',
+			'/test-bucket/workflow/同程商旅/index.txt',
+			'/test-bucket/workflow/%E5%90%8C%E7%A8%8B%E5%95%86%E6%97%85/index.txt',
+		],
+		['an accented name', '/test-bucket/résumé.pdf', '/test-bucket/r%C3%A9sum%C3%A9.pdf'],
+		['a space', '/test-bucket/my report.pdf', '/test-bucket/my%20report.pdf'],
+	])('percent-encodes the copy source for %s', async (_name, sourcePath, expected) => {
+		copyWithSource(sourcePath);
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe(expected);
+	});
+
+	it('leaves an all-ASCII source path byte-identical', async () => {
+		copyWithSource('/test-bucket/workflow/index.txt');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/workflow/index.txt');
+	});
+
+	it('does not double-encode a source path the user already encoded', async () => {
+		copyWithSource('/test-bucket/my%20report.pdf');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/my%20report.pdf');
+	});
+
+	// CopyObject reads an optional `?versionId=` selector after the key. Encoding
+	// the `?` would make S3 read the selector as the end of the key.
+	it('keeps the version selector readable while encoding the key', async () => {
+		copyWithSource('/test-bucket/r\u00e9sum\u00e9.pdf?versionId=3HL4kqt');
+
+		await node.execute.call(executeFunctionsMock);
+
+		expect(copySourceHeader()).toBe('/test-bucket/r%C3%A9sum%C3%A9.pdf?versionId=3HL4kqt');
+	});
+});