From 7101875383b82a6016e49ac5c4aa7e9b6acac4f4 Mon Sep 17 00:00:00 2001 From: smilkuri Date: Fri, 19 Sep 2025 16:10:20 +0000 Subject: [PATCH 1/4] feat(lib-storage): add validation for partCount and contentLength for multipart upload --- lib/lib-storage/src/Upload.spec.ts | 87 ++++++++++++++++++++++++++++++ lib/lib-storage/src/Upload.ts | 44 +++++++++++++-- 2 files changed, 127 insertions(+), 4 deletions(-) diff --git a/lib/lib-storage/src/Upload.spec.ts b/lib/lib-storage/src/Upload.spec.ts index 0d36c71250d2..3679ac93bdfa 100644 --- a/lib/lib-storage/src/Upload.spec.ts +++ b/lib/lib-storage/src/Upload.spec.ts @@ -792,4 +792,91 @@ describe(Upload.name, () => { new Error("@aws-sdk/lib-storage: this instance of Upload has already executed .done(). Create a new instance.") ); }); + + describe("Upload Part and parts count validation", () => { + const MOCK_PART_SIZE = 1024 * 1024 * 5; // 5MB + + it("should throw error when uploaded parts count doesn't match expected parts count", async () => { + const largeBuffer = Buffer.from("#".repeat(MOCK_PART_SIZE * 2 + 100)); + const upload = new Upload({ + params: { ...params, Body: largeBuffer }, + client: new S3({}), + }); + + (upload as any).__doConcurrentUpload = vi.fn().mockResolvedValue(undefined); + + (upload as any).uploadedParts = [{ PartNumber: 1, ETag: "etag1" }]; + (upload as any).isMultiPart = true; + + await expect(upload.done()).rejects.toThrow("Expected 3 number of parts but uploaded only 1 part"); + }); + + it("should throw error when part size doesn't match expected size except for laast part", () => { + const upload = new Upload({ + params, + client: new S3({}), + }); + + const invalidPart = { + partNumber: 1, + data: Buffer.from("small"), + lastPart: false, + }; + + expect(() => { + (upload as any).__validateUploadPart(invalidPart, MOCK_PART_SIZE); + }).toThrow(`The Part size for part number 1, size 5 does not match expected size ${MOCK_PART_SIZE}`); + }); + + it("should allow smaller size for last part", () => { + const upload = new Upload({ + params, + client: new S3({}), + }); + + const lastPart = { + partNumber: 2, + data: Buffer.from("small"), + lastPart: true, + }; + + expect(() => { + (upload as any).__validateUploadPart(lastPart, MOCK_PART_SIZE); + }).not.toThrow(); + }); + + it("should throw error when part has zero content length", () => { + const upload = new Upload({ + params, + client: new S3({}), + }); + + const emptyPart = { + partNumber: 1, + data: Buffer.from(""), + lastPart: false, + }; + + expect(() => { + (upload as any).__validateUploadPart(emptyPart, MOCK_PART_SIZE); + }).toThrow("Content length is missing on the data for part number 1"); + }); + + it("should skip validation for single-part uploads", () => { + const upload = new Upload({ + params, + client: new S3({}), + }); + + const singlePart = { + partNumber: 1, + data: Buffer.from("small"), + lastPart: true, + }; + + expect(() => { + (upload as any).__validateUploadPart(singlePart, MOCK_PART_SIZE); + }).not.toThrow(); + }); + }); }); diff --git a/lib/lib-storage/src/Upload.ts b/lib/lib-storage/src/Upload.ts index 06de3cccdaef..7a1bd0d1019a 100644 --- a/lib/lib-storage/src/Upload.ts +++ b/lib/lib-storage/src/Upload.ts @@ -47,7 +47,7 @@ export class Upload extends EventEmitter { // Defaults. private readonly queueSize: number = 4; - private readonly partSize = Upload.MIN_PART_SIZE; + private readonly partSize: number; private readonly leavePartsOnError: boolean = false; private readonly tags: Tag[] = []; @@ -66,6 +66,7 @@ export class Upload extends EventEmitter { private uploadedParts: CompletedPart[] = []; private uploadEnqueuedPartsCount = 0; + private expectedPartsCount = 0; /** * Last UploadId if the upload was done with MultipartUpload and not PutObject. */ @@ -81,19 +82,21 @@ export class Upload extends EventEmitter { // set defaults from options. this.queueSize = options.queueSize || this.queueSize; - this.partSize = options.partSize || this.partSize; this.leavePartsOnError = options.leavePartsOnError || this.leavePartsOnError; this.tags = options.tags || this.tags; this.client = options.client; this.params = options.params; - this.__validateInput(); - // set progress defaults this.totalBytes = byteLength(this.params.Body); this.bytesUploadedSoFar = 0; this.abortController = options.abortController ?? new AbortController(); + + this.partSize = this.__calculatePartSize(this.totalBytes ?? 0, Upload.MIN_PART_SIZE); + this.expectedPartsCount = this.totalBytes ? Math.ceil(this.totalBytes / this.partSize) : 1; + + this.__validateInput(); } async abort(): Promise { @@ -234,6 +237,8 @@ export class Upload extends EventEmitter { return; } + this.__validateUploadPart(dataPart, this.partSize); + // Use put instead of multipart for one chunk uploads. if (dataPart.partNumber === 1 && dataPart.lastPart) { return await this.__uploadUsingPut(dataPart); @@ -364,6 +369,12 @@ export class Upload extends EventEmitter { let result; if (this.isMultiPart) { + if (this.totalBytes && this.uploadedParts.length !== this.expectedPartsCount) { + throw new Error( + `Expected ${this.expectedPartsCount} number of parts but uploaded only ${this.uploadedParts.length} parts` + ); + } + this.uploadedParts.sort((a, b) => a.PartNumber! - b.PartNumber!); const uploadCompleteParams = { @@ -427,6 +438,31 @@ export class Upload extends EventEmitter { }); } + private __calculatePartSize(targetPartSizeBytes: number, minPartSize: number): number { + const calculatedPartSize = Math.floor(targetPartSizeBytes / this.MAX_PARTS); + return Math.max(minPartSize, calculatedPartSize); + } + + private __validateUploadPart(dataPart: RawDataPart, partSize: number): void { + const actualPartSize = byteLength(dataPart.data) || 0; + + // Skip validation for single-part uploads (PUT operations) + if (dataPart.partNumber === 1 && dataPart.lastPart) { + return; + } + + if (actualPartSize === 0) { + throw new Error(`Content length is missing on the data for part number ${dataPart.partNumber}`); + } + + // Validate part size (last part may be smaller) + if (!dataPart.lastPart && actualPartSize !== partSize) { + throw new Error( + `The Part size for part number ${dataPart.partNumber}, size ${actualPartSize} does not match expected size ${partSize}` + ); + } + } + private __validateInput(): void { if (!this.params) { throw new Error(`InputError: Upload requires params to be passed to upload.`); From 5d8903e42002caf071beb0fc1374910972a35aee Mon Sep 17 00:00:00 2001 From: smilkuri Date: Mon, 22 Sep 2025 19:22:28 +0000 Subject: [PATCH 2/4] fix(lib-storage): addressing comments --- lib/lib-storage/src/Upload.spec.ts | 4 ++-- lib/lib-storage/src/Upload.ts | 36 +++++++++++++----------------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/lib/lib-storage/src/Upload.spec.ts b/lib/lib-storage/src/Upload.spec.ts index 3679ac93bdfa..a4a4e92cb534 100644 --- a/lib/lib-storage/src/Upload.spec.ts +++ b/lib/lib-storage/src/Upload.spec.ts @@ -808,7 +808,7 @@ describe(Upload.name, () => { (upload as any).uploadedParts = [{ PartNumber: 1, ETag: "etag1" }]; (upload as any).isMultiPart = true; - await expect(upload.done()).rejects.toThrow("Expected 3 number of parts but uploaded only 1 part"); + await expect(upload.done()).rejects.toThrow("Expected 3 part(s) but uploaded 1 part(s)."); }); it("should throw error when part size doesn't match expected size except for laast part", () => { @@ -859,7 +859,7 @@ describe(Upload.name, () => { expect(() => { (upload as any).__validateUploadPart(emptyPart, MOCK_PART_SIZE); - }).toThrow("Content length is missing on the data for part number 1"); + }).toThrow("A dataPart was generated without a measurable data chunk size for part number 1"); }); it("should skip validation for single-part uploads", () => { diff --git a/lib/lib-storage/src/Upload.ts b/lib/lib-storage/src/Upload.ts index 7a1bd0d1019a..359dc5e9e69c 100644 --- a/lib/lib-storage/src/Upload.ts +++ b/lib/lib-storage/src/Upload.ts @@ -66,7 +66,7 @@ export class Upload extends EventEmitter { private uploadedParts: CompletedPart[] = []; private uploadEnqueuedPartsCount = 0; - private expectedPartsCount = 0; + private expectedPartsCount?: number; /** * Last UploadId if the upload was done with MultipartUpload and not PutObject. */ @@ -93,8 +93,8 @@ export class Upload extends EventEmitter { this.bytesUploadedSoFar = 0; this.abortController = options.abortController ?? new AbortController(); - this.partSize = this.__calculatePartSize(this.totalBytes ?? 0, Upload.MIN_PART_SIZE); - this.expectedPartsCount = this.totalBytes ? Math.ceil(this.totalBytes / this.partSize) : 1; + this.partSize = Math.max(Upload.MIN_PART_SIZE, Math.floor((this.totalBytes || 0) / this.MAX_PARTS)); + this.expectedPartsCount = this.totalBytes !== undefined ? Math.ceil(this.totalBytes / this.partSize) : undefined; this.__validateInput(); } @@ -237,8 +237,6 @@ export class Upload extends EventEmitter { return; } - this.__validateUploadPart(dataPart, this.partSize); - // Use put instead of multipart for one chunk uploads. if (dataPart.partNumber === 1 && dataPart.lastPart) { return await this.__uploadUsingPut(dataPart); @@ -287,6 +285,8 @@ export class Upload extends EventEmitter { this.uploadEnqueuedPartsCount += 1; + this.__validateUploadPart(dataPart); + const partResult = await this.client.send( new UploadPartCommand({ ...this.params, @@ -369,10 +369,9 @@ export class Upload extends EventEmitter { let result; if (this.isMultiPart) { - if (this.totalBytes && this.uploadedParts.length !== this.expectedPartsCount) { - throw new Error( - `Expected ${this.expectedPartsCount} number of parts but uploaded only ${this.uploadedParts.length} parts` - ); + const { expectedPartsCount, uploadedParts } = this; + if (expectedPartsCount !== undefined && uploadedParts.length !== expectedPartsCount) { + throw new Error(`Expected ${expectedPartsCount} part(s) but uploaded ${uploadedParts.length} part(s).`); } this.uploadedParts.sort((a, b) => a.PartNumber! - b.PartNumber!); @@ -438,27 +437,24 @@ export class Upload extends EventEmitter { }); } - private __calculatePartSize(targetPartSizeBytes: number, minPartSize: number): number { - const calculatedPartSize = Math.floor(targetPartSizeBytes / this.MAX_PARTS); - return Math.max(minPartSize, calculatedPartSize); - } - - private __validateUploadPart(dataPart: RawDataPart, partSize: number): void { - const actualPartSize = byteLength(dataPart.data) || 0; + private __validateUploadPart(dataPart: RawDataPart): void { + const actualPartSize = byteLength(dataPart.data) || undefined; // Skip validation for single-part uploads (PUT operations) if (dataPart.partNumber === 1 && dataPart.lastPart) { return; } - if (actualPartSize === 0) { - throw new Error(`Content length is missing on the data for part number ${dataPart.partNumber}`); + if (actualPartSize === undefined) { + throw new Error( + `A dataPart was generated without a measurable data chunk size for part number ${dataPart.partNumber}` + ); } // Validate part size (last part may be smaller) - if (!dataPart.lastPart && actualPartSize !== partSize) { + if (!dataPart.lastPart && actualPartSize !== this.partSize) { throw new Error( - `The Part size for part number ${dataPart.partNumber}, size ${actualPartSize} does not match expected size ${partSize}` + `The Part size for part number ${dataPart.partNumber}, size ${actualPartSize} does not match expected size ${this.partSize}` ); } } From d4325dd56422c36a8af4503b0fbcf71d0ee73cb9 Mon Sep 17 00:00:00 2001 From: smilkuri Date: Wed, 24 Sep 2025 15:15:56 +0000 Subject: [PATCH 3/4] fix(lib-storage): add e2e test to trigger validation --- lib/lib-storage/src/Upload.spec.ts | 2 +- lib/lib-storage/src/Upload.ts | 12 +++---- lib/lib-storage/src/lib-storage.e2e.spec.ts | 37 +++++++++++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/lib/lib-storage/src/Upload.spec.ts b/lib/lib-storage/src/Upload.spec.ts index a4a4e92cb534..3d7666782d04 100644 --- a/lib/lib-storage/src/Upload.spec.ts +++ b/lib/lib-storage/src/Upload.spec.ts @@ -825,7 +825,7 @@ describe(Upload.name, () => { expect(() => { (upload as any).__validateUploadPart(invalidPart, MOCK_PART_SIZE); - }).toThrow(`The Part size for part number 1, size 5 does not match expected size ${MOCK_PART_SIZE}`); + }).toThrow(`The byte size for part number 1, size 5 does not match expected size ${MOCK_PART_SIZE}`); }); it("should allow smaller size for last part", () => { diff --git a/lib/lib-storage/src/Upload.ts b/lib/lib-storage/src/Upload.ts index 359dc5e9e69c..b6c98fdd7c05 100644 --- a/lib/lib-storage/src/Upload.ts +++ b/lib/lib-storage/src/Upload.ts @@ -440,21 +440,21 @@ export class Upload extends EventEmitter { private __validateUploadPart(dataPart: RawDataPart): void { const actualPartSize = byteLength(dataPart.data) || undefined; - // Skip validation for single-part uploads (PUT operations) - if (dataPart.partNumber === 1 && dataPart.lastPart) { - return; - } - if (actualPartSize === undefined) { throw new Error( `A dataPart was generated without a measurable data chunk size for part number ${dataPart.partNumber}` ); } + // Skip validation for single-part uploads (PUT operations) + if (dataPart.partNumber === 1 && dataPart.lastPart) { + return; + } + // Validate part size (last part may be smaller) if (!dataPart.lastPart && actualPartSize !== this.partSize) { throw new Error( - `The Part size for part number ${dataPart.partNumber}, size ${actualPartSize} does not match expected size ${this.partSize}` + `The byte size for part number ${dataPart.partNumber}, size ${actualPartSize} does not match expected size ${this.partSize}` ); } } diff --git a/lib/lib-storage/src/lib-storage.e2e.spec.ts b/lib/lib-storage/src/lib-storage.e2e.spec.ts index d3c6fb089d51..e615e615f49a 100644 --- a/lib/lib-storage/src/lib-storage.e2e.spec.ts +++ b/lib/lib-storage/src/lib-storage.e2e.spec.ts @@ -139,6 +139,43 @@ describe("@aws-sdk/lib-storage", () => { "S3Client AbortMultipartUploadCommand 204", ]); }); + + it("should validate part size constraints", () => { + const upload = new Upload({ + client, + params: { + Bucket, + Key: `validation-test-${Date.now()}`, + Body: Buffer.alloc(1024 * 1024 * 10), + }, + }); + + const invalidPart = { + partNumber: 2, + data: Buffer.alloc(1024 * 1024 * 3), // 3MB - too small for non-final part + lastPart: false, + }; + + expect(() => { + (upload as any).__validateUploadPart(invalidPart); + }).toThrow(/The byte size for part number 2, size \d+ does not match expected size \d+/); + }); + + it("should validate part count constraints", async () => { + const upload = new Upload({ + client, + params: { + Bucket, + Key: `validation-test-${Date.now()}`, + Body: Buffer.alloc(1024 * 1024 * 10), + }, + }); + + (upload as any).uploadedParts = [{ PartNumber: 1, ETag: "etag1" }]; + (upload as any).isMultiPart = true; + + await expect(upload.done()).rejects.toThrow(/Expected \d+ part\(s\) but uploaded \d+ part\(s\)\./); + }); }); } ); From ef5a56ecb2b047240d0dcf8aad31a8536a16bd88 Mon Sep 17 00:00:00 2001 From: smilkuri Date: Wed, 24 Sep 2025 16:32:36 +0000 Subject: [PATCH 4/4] fix(lib-storage): remove converting zero to undefined for zero byte length --- lib/lib-storage/src/Upload.spec.ts | 2 +- lib/lib-storage/src/Upload.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/lib-storage/src/Upload.spec.ts b/lib/lib-storage/src/Upload.spec.ts index 3d7666782d04..383549a58edf 100644 --- a/lib/lib-storage/src/Upload.spec.ts +++ b/lib/lib-storage/src/Upload.spec.ts @@ -859,7 +859,7 @@ describe(Upload.name, () => { expect(() => { (upload as any).__validateUploadPart(emptyPart, MOCK_PART_SIZE); - }).toThrow("A dataPart was generated without a measurable data chunk size for part number 1"); + }).toThrow(`The byte size for part number 1, size 0 does not match expected size ${MOCK_PART_SIZE}`); }); it("should skip validation for single-part uploads", () => { diff --git a/lib/lib-storage/src/Upload.ts b/lib/lib-storage/src/Upload.ts index b6c98fdd7c05..99098f80c8d8 100644 --- a/lib/lib-storage/src/Upload.ts +++ b/lib/lib-storage/src/Upload.ts @@ -438,7 +438,7 @@ export class Upload extends EventEmitter { } private __validateUploadPart(dataPart: RawDataPart): void { - const actualPartSize = byteLength(dataPart.data) || undefined; + const actualPartSize = byteLength(dataPart.data); if (actualPartSize === undefined) { throw new Error(