From 6d1d688e27ee3f647ffac9596dcb85a75ae7cca7 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 27 Jan 2026 11:40:22 +0000 Subject: [PATCH 1/7] feat: implement robust path validation and structured skip reporting BREAKING CHANGE: downloadManyFiles now returns a DownloadManyFilesResult object instead of an array of DownloadResponse. - Implements strict blocking for absolute paths (Unix and Windows styles). - Prevents path traversal via dot-segments (../) using path.relative validation. - Blocks illegal characters and poisoned paths (e.g., Windows volume colons). - Updates internal logic to resolve paths against a safe base directory (CWD or prefix). --- .../downloadManyFilesWithTransferManager.js | 18 ++- samples/system-test/transfer-manager.test.js | 2 +- src/file.ts | 13 ++ src/transfer-manager.ts | 39 +++++- test/transfer-manager.ts | 127 ++++++++++++++++++ 5 files changed, 192 insertions(+), 7 deletions(-) diff --git a/samples/downloadManyFilesWithTransferManager.js b/samples/downloadManyFilesWithTransferManager.js index 7a464ad4c..2324f822f 100644 --- a/samples/downloadManyFilesWithTransferManager.js +++ b/samples/downloadManyFilesWithTransferManager.js @@ -49,10 +49,24 @@ function main( async function downloadManyFilesWithTransferManager() { // Downloads the files - await transferManager.downloadManyFiles([firstFileName, secondFileName]); + const {skippedFiles} = await transferManager.downloadManyFiles([ + firstFileName, + secondFileName, + ]); for (const fileName of [firstFileName, secondFileName]) { - console.log(`gs://${bucketName}/${fileName} downloaded to ${fileName}.`); + const isSkipped = skippedFiles.some(f => f.fileName === fileName); + if (!isSkipped) { + console.log( + `gs://${bucketName}/${fileName} downloaded to ${fileName}.` + ); + } + } + + if (skippedFiles.length > 0) { + for (const skipped of skippedFiles) { + console.warn(`Skipped ${skipped.fileName}: ${skipped.reason}`); + } } } diff --git a/samples/system-test/transfer-manager.test.js b/samples/system-test/transfer-manager.test.js index f6180bed3..9d9295001 100644 --- a/samples/system-test/transfer-manager.test.js +++ b/samples/system-test/transfer-manager.test.js @@ -56,7 +56,7 @@ describe('transfer manager', () => { ); }); - it('should download mulitple files', async () => { + it('should download multiple files', async () => { const output = execSync( `node downloadManyFilesWithTransferManager.js ${bucketName} ${firstFilePath} ${secondFilePath}` ); diff --git a/src/file.ts b/src/file.ts index 5c6f4e681..6dd84945e 100644 --- a/src/file.ts +++ b/src/file.ts @@ -396,6 +396,19 @@ export interface CopyCallback { export type DownloadResponse = [Buffer]; +export interface DownloadManyFilesResult { + responses: DownloadResponse[]; + skippedFiles: SkippedFileInfo[]; +} + +export interface SkippedFileInfo { + fileName: string; + localPath: string; + reason: string; + message?: string; + error?: Error; +} + export type DownloadCallback = ( err: RequestError | null, contents: Buffer diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index dd4e41eeb..a2e63edca 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -16,6 +16,7 @@ import {Bucket, UploadOptions, UploadResponse} from './bucket.js'; import { + DownloadManyFilesResult, DownloadOptions, DownloadResponse, File, @@ -566,13 +567,22 @@ export class TransferManager { async downloadManyFiles( filesOrFolder: File[] | string[] | string, options: DownloadManyFilesOptions = {} - ): Promise { + ): Promise { const limit = pLimit( options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT ); const promises: Promise[] = []; + const skippedFiles: Array<{ + fileName: string; + localPath: string; + reason: string; + }> = []; let files: File[] = []; + const baseDestination = path.resolve( + options.prefix || options.passthroughOptions?.destination || '.' + ); + if (!Array.isArray(filesOrFolder)) { const directoryFiles = await this.bucket.getFiles({ prefix: filesOrFolder, @@ -598,16 +608,33 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; + let dest: string | undefined; if (options.prefix || passThroughOptionsCopy.destination) { - passThroughOptionsCopy.destination = path.join( + dest = path.join( options.prefix || '', passThroughOptionsCopy.destination || '', file.name ); } if (options.stripPrefix) { - passThroughOptionsCopy.destination = file.name.replace(regex, ''); + dest = file.name.replace(regex, ''); + } + + const resolvedPath = path.resolve(dest || file.name); + const relativePath = path.relative(baseDestination, resolvedPath); + const isOutside = + relativePath.startsWith('..') || path.isAbsolute(relativePath); + + if (isOutside || file.name.includes(':')) { + skippedFiles.push({ + fileName: file.name, + localPath: resolvedPath, + reason: 'Path Traversal Detected', + }); + continue; } + passThroughOptionsCopy.destination = dest; + if ( options.skipIfExists && existsSync(passThroughOptionsCopy.destination || '') @@ -629,8 +656,12 @@ export class TransferManager { }) ); } + const results = await Promise.all(promises); - return Promise.all(promises); + return { + responses: results, + skippedFiles: skippedFiles, + }; } /** diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 2582782fa..729840561 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -41,6 +41,7 @@ import fs from 'fs'; import {promises as fsp, Stats} from 'fs'; import * as sinon from 'sinon'; +import {DownloadManyFilesResult} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -350,6 +351,132 @@ describe('Transfer Manager', () => { true ); }); + + it('skips files that attempt path traversal via dot-segments (../) and returns them in skippedFiles', async () => { + const prefix = 'safe-directory'; + const maliciousFilename = '../../etc/passwd'; + const validFilename = 'valid.txt'; + + const maliciousFile = new File(bucket, maliciousFilename); + const validFile = new File(bucket, validFilename); + + const downloadStub = sandbox + .stub(validFile, 'download') + .resolves([Buffer.alloc(0)]); + const maliciousDownloadStub = sandbox.stub(maliciousFile, 'download'); + + const result = await transferManager.downloadManyFiles( + [maliciousFile, validFile], + {prefix} + ); + + assert.strictEqual(maliciousDownloadStub.called, false); + assert.strictEqual(downloadStub.calledOnce, true); + + const results = result as DownloadManyFilesResult; + assert.strictEqual(results.skippedFiles.length, 1); + assert.strictEqual(results.skippedFiles[0].fileName, maliciousFilename); + assert.strictEqual( + results.skippedFiles[0].reason, + 'Path Traversal Detected' + ); + }); + + it('allows files with relative segments that resolve within the target directory', async () => { + const prefix = 'safe-directory'; + const filename = './subdir/../subdir/file.txt'; + const file = new File(bucket, filename); + + const downloadStub = sandbox + .stub(file, 'download') + .resolves([Buffer.alloc(0)]); + + await transferManager.downloadManyFiles([file], {prefix}); + + assert.strictEqual(downloadStub.calledOnce, true); + }); + + it('prevents traversal when no prefix is provided', async () => { + const maliciousFilename = '../../../traversal.txt'; + const file = new File(bucket, maliciousFilename); + const downloadStub = sandbox.stub(file, 'download'); + + const result = await transferManager.downloadManyFiles([file]); + + assert.strictEqual(downloadStub.called, false); + const results = result as DownloadManyFilesResult; + assert.strictEqual(results.skippedFiles.length, 1); + }); + + it('jails absolute-looking paths with nested segments into the target directory', async () => { + const prefix = './downloads'; + const filename = '/tmp/shady.txt'; + const file = new File(bucket, filename); + const expectedDestination = path.join(prefix, filename); + + const downloadStub = sandbox + .stub(file, 'download') + .resolves([Buffer.alloc(0)]); + + const result = await transferManager.downloadManyFiles([file], {prefix}); + + assert.strictEqual(downloadStub.called, true); + const options = downloadStub.firstCall.args[0] as DownloadOptions; + assert.strictEqual(options.destination, expectedDestination); + + const results = result as DownloadManyFilesResult; + assert.strictEqual(results.skippedFiles.length, 0); + }); + + it('jails absolute-looking Unix paths (e.g. /etc/passwd) into the target directory instead of skipping', async () => { + const prefix = 'downloads'; + const filename = '/etc/passwd'; + const expectedDestination = path.join(prefix, filename); + + const file = new File(bucket, filename); + const downloadStub = sandbox + .stub(file, 'download') + .resolves([Buffer.alloc(0)]); + + await transferManager.downloadManyFiles([file], {prefix}); + + assert.strictEqual(downloadStub.calledOnce, true); + const options = downloadStub.firstCall.args[0] as DownloadOptions; + assert.strictEqual(options.destination, expectedDestination); + }); + + it('correctly handles stripPrefix and verifies the resulting path is still safe', async () => { + const options = { + stripPrefix: 'secret/', + prefix: 'local-folder', + }; + const filename = 'secret/../../escape.txt'; + const file = new File(bucket, filename); + + const downloadStub = sandbox.stub(file, 'download'); + + const result = await transferManager.downloadManyFiles([file], options); + + assert.strictEqual(downloadStub.called, false); + const results = result as DownloadManyFilesResult; + assert.strictEqual(results.skippedFiles.length, 1); + }); + + it('should skips files containing Windows volume separators (:) to prevent drive-injection attacks', async () => { + const prefix = 'C:\\local\\target'; + const maliciousFile = new File(bucket, 'C:\\system\\win32'); + + const result = await transferManager.downloadManyFiles([maliciousFile], { + prefix, + }); + + const results = result as DownloadManyFilesResult; + assert.strictEqual(results.skippedFiles.length, 1); + assert.strictEqual( + results.skippedFiles[0].reason, + 'Path Traversal Detected' + ); + }); }); describe('downloadFileInChunks', () => { From d0d7e4257adfc5bedced3e75a88ab9ac661f2eaa Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 27 Jan 2026 14:38:44 +0000 Subject: [PATCH 2/7] feat: implement robust path validation and structured skip reporting BREAKING CHANGE: downloadManyFiles now returns a DownloadManyFilesResult object instead of an array of DownloadResponse. - Implements strict blocking for absolute paths (Unix and Windows styles) and dot-segment traversal. - Adds DownloadManyFilesResult interface with SkipReason enums for programmatic handling of skipped files. - Ensures input-to-output parity where every file is accounted for in either 'responses' or 'skippedFiles'. - Robustly handles 'unknown' catch variables by narrowing to Error instances. - Optimizes directory creation logic within the parallel download loop. --- .../downloadManyFilesWithTransferManager.js | 3 +- src/file.ts | 10 +++- src/transfer-manager.ts | 52 +++++++++++++------ test/transfer-manager.ts | 37 +++++++++++-- 4 files changed, 80 insertions(+), 22 deletions(-) diff --git a/samples/downloadManyFilesWithTransferManager.js b/samples/downloadManyFilesWithTransferManager.js index 2324f822f..817253207 100644 --- a/samples/downloadManyFilesWithTransferManager.js +++ b/samples/downloadManyFilesWithTransferManager.js @@ -54,8 +54,9 @@ function main( secondFileName, ]); + const skippedFileNames = new Set(skippedFiles.map(f => f.fileName)); for (const fileName of [firstFileName, secondFileName]) { - const isSkipped = skippedFiles.some(f => f.fileName === fileName); + const isSkipped = skippedFileNames.has(fileName); if (!isSkipped) { console.log( `gs://${bucketName}/${fileName} downloaded to ${fileName}.` diff --git a/src/file.ts b/src/file.ts index 6dd84945e..6cdbdbe69 100644 --- a/src/file.ts +++ b/src/file.ts @@ -401,11 +401,19 @@ export interface DownloadManyFilesResult { skippedFiles: SkippedFileInfo[]; } +export enum SkipReason { + PATH_TRAVERSAL = 'PATH_TRAVERSAL', + ILLEGAL_CHARACTER = 'ILLEGAL_CHARACTER', + ABSOLUTE_PATH_BLOCKED = 'ABSOLUTE_PATH_BLOCKED', + ALREADY_EXISTS = 'ALREADY_EXISTS', + DOWNLOAD_ERROR = 'DOWNLOAD_ERROR', +} + export interface SkippedFileInfo { fileName: string; localPath: string; reason: string; - message?: string; + message: string; error?: Error; } diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index a2e63edca..0d786d0dc 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -22,6 +22,8 @@ import { File, FileExceptionMessages, RequestError, + SkippedFileInfo, + SkipReason, } from './file.js'; import pLimit from 'p-limit'; import * as path from 'path'; @@ -572,11 +574,7 @@ export class TransferManager { options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT ); const promises: Promise[] = []; - const skippedFiles: Array<{ - fileName: string; - localPath: string; - reason: string; - }> = []; + const skippedFiles: SkippedFileInfo[] = []; let files: File[] = []; const baseDestination = path.resolve( @@ -622,14 +620,19 @@ export class TransferManager { const resolvedPath = path.resolve(dest || file.name); const relativePath = path.relative(baseDestination, resolvedPath); - const isOutside = - relativePath.startsWith('..') || path.isAbsolute(relativePath); + const isOutside = relativePath.startsWith('..'); + const isAbsoluteAttempt = path.isAbsolute(relativePath); + + if (isOutside || isAbsoluteAttempt || file.name.includes(':')) { + let reason: SkipReason = SkipReason.ILLEGAL_CHARACTER; + if (isOutside) reason = SkipReason.PATH_TRAVERSAL; + else if (isAbsoluteAttempt) reason = SkipReason.ABSOLUTE_PATH_BLOCKED; - if (isOutside || file.name.includes(':')) { skippedFiles.push({ fileName: file.name, localPath: resolvedPath, - reason: 'Path Traversal Detected', + reason: reason, + message: `File ${file.name} was skipped due to path validation failure.`, }); continue; } @@ -644,15 +647,30 @@ export class TransferManager { promises.push( limit(async () => { - const destination = passThroughOptionsCopy.destination; - if (destination && destination.endsWith(path.sep)) { - await fsp.mkdir(destination, {recursive: true}); - return Promise.resolve([ - Buffer.alloc(0), - ]) as Promise; + try { + const destination = passThroughOptionsCopy.destination; + if ( + destination && + (destination.endsWith(path.sep) || destination.endsWith('/')) + ) { + await fsp.mkdir(destination, {recursive: true}); + return [Buffer.alloc(0)] as DownloadResponse; + } + + return file.download(passThroughOptionsCopy); + } catch (err) { + const error = err instanceof Error ? err : new Error(String(err)); + skippedFiles.push({ + fileName: file.name, + localPath: path.resolve( + passThroughOptionsCopy.destination || file.name + ), + reason: SkipReason.DOWNLOAD_ERROR, + message: error.message, + error: error as Error, + }); + return [Buffer.alloc(0)] as DownloadResponse; } - - return file.download(passThroughOptionsCopy); }) ); } diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 729840561..945ce7925 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -41,7 +41,7 @@ import fs from 'fs'; import {promises as fsp, Stats} from 'fs'; import * as sinon from 'sinon'; -import {DownloadManyFilesResult} from '../src/file.js'; +import {DownloadManyFilesResult, SkipReason} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -378,7 +378,7 @@ describe('Transfer Manager', () => { assert.strictEqual(results.skippedFiles[0].fileName, maliciousFilename); assert.strictEqual( results.skippedFiles[0].reason, - 'Path Traversal Detected' + SkipReason.PATH_TRAVERSAL ); }); @@ -474,9 +474,40 @@ describe('Transfer Manager', () => { assert.strictEqual(results.skippedFiles.length, 1); assert.strictEqual( results.skippedFiles[0].reason, - 'Path Traversal Detected' + SkipReason.ILLEGAL_CHARACTER ); }); + + it('should account for every input file (Parity Check)', async () => { + const prefix = './downloads'; + const fileNames = [ + 'valid1.txt', + '../../traversal.txt', // Should be skipped + '/absolute/blocked.txt', + 'c:/absolute/blocked.txt', // Should be skipped + 'absolute/../valid2.txt', + ]; + + const files = fileNames.map(name => bucket.file(name)); + + sandbox.stub(File.prototype, 'download').resolves([Buffer.alloc(0)]); + + const result = (await transferManager.downloadManyFiles(files, { + prefix, + })) as DownloadManyFilesResult; + + const totalProcessed = + result.responses.length + result.skippedFiles.length; + + assert.strictEqual( + totalProcessed, + fileNames.length, + `Parity Failure: Processed ${totalProcessed} files but input had ${fileNames.length}` + ); + + assert.strictEqual(result.responses.length, 3); + assert.strictEqual(result.skippedFiles.length, 2); + }); }); describe('downloadFileInChunks', () => { From ef7475bccdc8ab14e38d636dcbd3c0dc1bdce442 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 27 Jan 2026 14:44:52 +0000 Subject: [PATCH 3/7] fix windows error --- src/transfer-manager.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 0d786d0dc..26068a3eb 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -624,8 +624,9 @@ export class TransferManager { const isAbsoluteAttempt = path.isAbsolute(relativePath); if (isOutside || isAbsoluteAttempt || file.name.includes(':')) { - let reason: SkipReason = SkipReason.ILLEGAL_CHARACTER; + let reason: SkipReason = SkipReason.DOWNLOAD_ERROR; if (isOutside) reason = SkipReason.PATH_TRAVERSAL; + else if (file.name.includes(':')) reason = SkipReason.ILLEGAL_CHARACTER; else if (isAbsoluteAttempt) reason = SkipReason.ABSOLUTE_PATH_BLOCKED; skippedFiles.push({ From 1536b31bbca78ceb6d0d620a4828f01620665db5 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 29 Jan 2026 09:08:14 +0000 Subject: [PATCH 4/7] chore: skip public access tests due to PAP (b/457800112) --- samples/system-test/buckets.test.js | 8 +++- samples/system-test/files.test.js | 8 +++- system-test/storage.ts | 72 +++++++++++++++++++++++++---- 3 files changed, 77 insertions(+), 11 deletions(-) diff --git a/samples/system-test/buckets.test.js b/samples/system-test/buckets.test.js index f8f8d70cb..de4e6068f 100644 --- a/samples/system-test/buckets.test.js +++ b/samples/system-test/buckets.test.js @@ -366,7 +366,13 @@ it("should add a bucket's website configuration", async () => { }); }); -it('should make bucket publicly readable', async () => { +/** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ +it.skip('should make bucket publicly readable', async () => { const output = execSync(`node makeBucketPublic.js ${bucketName}`); assert.match( output, diff --git a/samples/system-test/files.test.js b/samples/system-test/files.test.js index 1bd20623f..54a7bc507 100644 --- a/samples/system-test/files.test.js +++ b/samples/system-test/files.test.js @@ -334,7 +334,13 @@ describe('file', () => { await bucket.file(publicFileName).delete(); }); - it('should make a file public', () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make a file public', () => { const output = execSync( `node makePublic.js ${bucketName} ${publicFileName}` ); diff --git a/system-test/storage.ts b/system-test/storage.ts index 769a21bdf..937031954 100644 --- a/system-test/storage.ts +++ b/system-test/storage.ts @@ -289,7 +289,13 @@ describe('storage', function () { await bucket.acl.delete({entity: USER_ACCOUNT}); }); - it('should make a bucket public', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make a bucket public', async () => { await bucket.makePublic(); const [aclObject] = await bucket.acl.get({entity: 'allUsers'}); assert.deepStrictEqual(aclObject, { @@ -302,7 +308,13 @@ describe('storage', function () { await bucket.acl.delete({entity: 'allUsers'}); }); - it('should make files public', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make files public', async () => { await Promise.all( ['a', 'b', 'c'].map(text => createFileWithContentPromise(text)) ); @@ -319,7 +331,13 @@ describe('storage', function () { ]); }); - it('should make a bucket private', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make a bucket private', async () => { try { await bucket.makePublic(); await new Promise(resolve => @@ -404,7 +422,13 @@ describe('storage', function () { await file.acl.delete({entity: USER_ACCOUNT}); }); - it('should make a file public', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make a file public', async () => { await file.makePublic(); const [aclObject] = await file.acl.get({entity: 'allUsers'}); assert.deepStrictEqual(aclObject, { @@ -452,7 +476,13 @@ describe('storage', function () { assert.strictEqual(encryptionAlgorithm, 'AES256'); }); - it('should make a file public during the upload', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make a file public during the upload', async () => { const [file] = await bucket.upload(FILES.big.path, { resumable: false, public: true, @@ -465,7 +495,13 @@ describe('storage', function () { }); }); - it('should make a file public from a resumable upload', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should make a file public from a resumable upload', async () => { const [file] = await bucket.upload(FILES.big.path, { resumable: true, public: true, @@ -529,7 +565,13 @@ describe('storage', function () { ]); }); - it('should set a policy', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should set a policy', async () => { const [policy] = await bucket.iam.getPolicy(); policy!.bindings.push({ role: 'roles/storage.legacyBucketReader', @@ -2305,7 +2347,13 @@ describe('storage', function () { }); }); - it('iam#setPolicy', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('iam#setPolicy', async () => { await requesterPaysDoubleTest(async options => { const [policy] = await bucket.iam.getPolicy(); @@ -3004,7 +3052,13 @@ describe('storage', function () { await Promise.all([file.delete, copiedFile.delete()]); }); - it('should respect predefined Acl at file#copy', async () => { + /** + * TODO: (b/457800112)Re-enable once Org Policy allows PAP overrides on test buckets. + * REASON: Organization Policy "Public Access Prevention" (PAP) is enabled. + * This prevents 'allUsers' or 'allAuthenticatedUsers' from being added to + * IAM/ACL policies, causing 403 errors in system tests. + */ + it.skip('should respect predefined Acl at file#copy', async () => { const opts = {destination: 'CloudLogo'}; const [file] = await bucket.upload(FILES.logo.path, opts); const copyOpts = {predefinedAcl: 'publicRead'}; From 29d32dde1aa484b0a91b70d2ebaa7d6bdf88d8df Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 19 Feb 2026 07:27:37 +0000 Subject: [PATCH 5/7] feat: implement robust path validation for multi-file downloads Added protection against path traversal attacks. Implemented validation for illegal characters (e.g., Windows drive colons). Coerced absolute-style GCS keys to remain within the target sandbox. Added comprehensive test suite covering 11 path scenarios. --- src/transfer-manager.ts | 7 +++--- test/transfer-manager.ts | 50 ++++++++++++++++++++++++++++++++-------- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 26068a3eb..5edb0eb5b 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -621,13 +621,12 @@ export class TransferManager { const resolvedPath = path.resolve(dest || file.name); const relativePath = path.relative(baseDestination, resolvedPath); const isOutside = relativePath.startsWith('..'); - const isAbsoluteAttempt = path.isAbsolute(relativePath); + const hasIllegalChars = (dest || file.name).includes(':'); - if (isOutside || isAbsoluteAttempt || file.name.includes(':')) { + if (isOutside || hasIllegalChars) { let reason: SkipReason = SkipReason.DOWNLOAD_ERROR; if (isOutside) reason = SkipReason.PATH_TRAVERSAL; - else if (file.name.includes(':')) reason = SkipReason.ILLEGAL_CHARACTER; - else if (isAbsoluteAttempt) reason = SkipReason.ABSOLUTE_PATH_BLOCKED; + else if (hasIllegalChars) reason = SkipReason.ILLEGAL_CHARACTER; skippedFiles.push({ fileName: file.name, diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 945ce7925..41f57756b 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -32,6 +32,7 @@ import { DownloadManyFilesOptions, } from '../src/index.js'; import assert from 'assert'; +import {describe, it, beforeEach, before, afterEach, after} from 'mocha'; import * as path from 'path'; import {GaxiosOptions, GaxiosResponse} from 'gaxios'; import {GCCL_GCS_CMD_KEY} from '../src/nodejs-common/util.js'; @@ -353,7 +354,7 @@ describe('Transfer Manager', () => { }); it('skips files that attempt path traversal via dot-segments (../) and returns them in skippedFiles', async () => { - const prefix = 'safe-directory'; + const prefix = 'download-directory'; const maliciousFilename = '../../etc/passwd'; const validFilename = 'valid.txt'; @@ -462,7 +463,7 @@ describe('Transfer Manager', () => { assert.strictEqual(results.skippedFiles.length, 1); }); - it('should skips files containing Windows volume separators (:) to prevent drive-injection attacks', async () => { + it('should skip files containing Windows volume separators (:) to prevent drive-injection attacks', async () => { const prefix = 'C:\\local\\target'; const maliciousFile = new File(bucket, 'C:\\system\\win32'); @@ -479,18 +480,25 @@ describe('Transfer Manager', () => { }); it('should account for every input file (Parity Check)', async () => { - const prefix = './downloads'; + const prefix = '/local/target'; const fileNames = [ - 'valid1.txt', - '../../traversal.txt', // Should be skipped - '/absolute/blocked.txt', - 'c:/absolute/blocked.txt', // Should be skipped - 'absolute/../valid2.txt', + 'data/file.txt', // Normal (Download) + 'data/../sibling.txt', // Internal Traversal (Download) + '../escape.txt', // External Traversal (Skip - Path Traversal '..') + '/etc/passwd', // Leading Slash (Download) + '/local/usr/a.txt', // Path matches prefix (Download) + 'dir/./file.txt', // Dot segment (Download) + 'windows\\file.txt', // Windows separator (Download) + 'data\\..\\sibling.txt', // Windows traversal (Download) + '..\\escape.txt', // Windows escape (Skip - Path Traversal '..') + 'C:\\system\\win32', // Windows Drive (Skip - Illegal Char ':') + 'C:\\local\\target\\a.txt', // Windows Absolute (Skip - Illegal Char ':') ]; const files = fileNames.map(name => bucket.file(name)); sandbox.stub(File.prototype, 'download').resolves([Buffer.alloc(0)]); + sandbox.stub(fsp, 'mkdir').resolves(); const result = (await transferManager.downloadManyFiles(files, { prefix, @@ -505,8 +513,30 @@ describe('Transfer Manager', () => { `Parity Failure: Processed ${totalProcessed} files but input had ${fileNames.length}` ); - assert.strictEqual(result.responses.length, 3); - assert.strictEqual(result.skippedFiles.length, 2); + const expectedDownloads = 7; + const expectedSkips = 4; + + assert.strictEqual( + result.responses.length, + expectedDownloads, + `Expected ${expectedDownloads} downloads but got ${result.responses.length}` + ); + + assert.strictEqual( + result.skippedFiles.length, + expectedSkips, + `Expected ${expectedSkips} skips but got ${result.skippedFiles.length}` + ); + + const traversalSkips = result.skippedFiles.filter( + f => f.reason === SkipReason.PATH_TRAVERSAL + ); + assert.strictEqual(traversalSkips.length, 2); + + const illegalCharSkips = result.skippedFiles.filter( + f => f.reason === SkipReason.ILLEGAL_CHARACTER + ); + assert.strictEqual(illegalCharSkips.length, 2); }); }); From 3b88a31eb66f11c6676038381eaec3fd8baae57c Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 19 Feb 2026 07:54:55 +0000 Subject: [PATCH 6/7] code improvement --- src/file.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/file.ts b/src/file.ts index 6cdbdbe69..067203617 100644 --- a/src/file.ts +++ b/src/file.ts @@ -404,7 +404,6 @@ export interface DownloadManyFilesResult { export enum SkipReason { PATH_TRAVERSAL = 'PATH_TRAVERSAL', ILLEGAL_CHARACTER = 'ILLEGAL_CHARACTER', - ABSOLUTE_PATH_BLOCKED = 'ABSOLUTE_PATH_BLOCKED', ALREADY_EXISTS = 'ALREADY_EXISTS', DOWNLOAD_ERROR = 'DOWNLOAD_ERROR', } @@ -412,7 +411,7 @@ export enum SkipReason { export interface SkippedFileInfo { fileName: string; localPath: string; - reason: string; + reason: SkipReason; message: string; error?: Error; } From d2279ac6a263b8399ade19b75ac3ec9dee105f3c Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Mon, 2 Mar 2026 11:47:53 +0000 Subject: [PATCH 7/7] feat: implement secure path validation for downloadManyFiles - Adds protection against path traversal (../) using normalized path resolution. - Prevents Windows-style drive letter injection while allowing GCS timestamps. - Implements directory jail logic to ensure absolute-style paths are relative to destination. - Preserves backward compatibility by returning an augmented DownloadResponse array. - Automates recursive directory creation for validated nested files. - Adds comprehensive 13-scenario test suite for edge-case parity. --- .../downloadManyFilesWithTransferManager.js | 23 +++-- src/file.ts | 20 ++-- src/transfer-manager.ts | 76 ++++++++------- test/transfer-manager.ts | 92 ++++++++++--------- 4 files changed, 110 insertions(+), 101 deletions(-) diff --git a/samples/downloadManyFilesWithTransferManager.js b/samples/downloadManyFilesWithTransferManager.js index 817253207..2d68bcd17 100644 --- a/samples/downloadManyFilesWithTransferManager.js +++ b/samples/downloadManyFilesWithTransferManager.js @@ -48,27 +48,26 @@ function main( const transferManager = new TransferManager(storage.bucket(bucketName)); async function downloadManyFilesWithTransferManager() { - // Downloads the files - const {skippedFiles} = await transferManager.downloadManyFiles([ + // Downloads the files. The result is an array of DownloadResponses + // augmented with 'skipped' and 'reason' properties. + const results = await transferManager.downloadManyFiles([ firstFileName, secondFileName, ]); - const skippedFileNames = new Set(skippedFiles.map(f => f.fileName)); - for (const fileName of [firstFileName, secondFileName]) { - const isSkipped = skippedFileNames.has(fileName); - if (!isSkipped) { + for (let i = 0; i < results.length; i++) { + const result = results[i]; + // Each result is a DownloadResponse [Buffer] + // We check our custom properties to see if it was blocked by validation + const fileName = result.fileName || [firstFileName, secondFileName][i]; + if (result.skipped) { + console.warn(`Skipped ${fileName}: ${result.reason}`); + } else { console.log( `gs://${bucketName}/${fileName} downloaded to ${fileName}.` ); } } - - if (skippedFiles.length > 0) { - for (const skipped of skippedFiles) { - console.warn(`Skipped ${skipped.fileName}: ${skipped.reason}`); - } - } } downloadManyFilesWithTransferManager().catch(console.error); diff --git a/src/file.ts b/src/file.ts index 067203617..bf4601591 100644 --- a/src/file.ts +++ b/src/file.ts @@ -396,10 +396,14 @@ export interface CopyCallback { export type DownloadResponse = [Buffer]; -export interface DownloadManyFilesResult { - responses: DownloadResponse[]; - skippedFiles: SkippedFileInfo[]; -} +export type DownloadResponseWithStatus = [Buffer] & { + skipped?: boolean; + reason?: SkipReason; + fileName?: string; + localPath?: string; + message?: string; + error?: Error; +}; export enum SkipReason { PATH_TRAVERSAL = 'PATH_TRAVERSAL', @@ -408,14 +412,6 @@ export enum SkipReason { DOWNLOAD_ERROR = 'DOWNLOAD_ERROR', } -export interface SkippedFileInfo { - fileName: string; - localPath: string; - reason: SkipReason; - message: string; - error?: Error; -} - export type DownloadCallback = ( err: RequestError | null, contents: Buffer diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 5edb0eb5b..e01335ea5 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -16,13 +16,12 @@ import {Bucket, UploadOptions, UploadResponse} from './bucket.js'; import { - DownloadManyFilesResult, DownloadOptions, DownloadResponse, + DownloadResponseWithStatus, File, FileExceptionMessages, RequestError, - SkippedFileInfo, SkipReason, } from './file.js'; import pLimit from 'p-limit'; @@ -569,12 +568,12 @@ export class TransferManager { async downloadManyFiles( filesOrFolder: File[] | string[] | string, options: DownloadManyFilesOptions = {} - ): Promise { + ): Promise { const limit = pLimit( options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT ); const promises: Promise[] = []; - const skippedFiles: SkippedFileInfo[] = []; + const skippedFiles: DownloadResponseWithStatus[] = []; let files: File[] = []; const baseDestination = path.resolve( @@ -606,34 +605,46 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; - let dest: string | undefined; + const normalizedGcsName = file.name + .replace(/\\/g, path.sep) + .replace(/\//g, path.sep); + + let dest: string; if (options.prefix || passThroughOptionsCopy.destination) { dest = path.join( options.prefix || '', passThroughOptionsCopy.destination || '', - file.name + normalizedGcsName ); } if (options.stripPrefix) { - dest = file.name.replace(regex, ''); + dest = normalizedGcsName.replace(regex, ''); + } else { + dest = path.join( + options.prefix || '', + passThroughOptionsCopy.destination || '', + normalizedGcsName + ); } - const resolvedPath = path.resolve(dest || file.name); + const resolvedPath = path.resolve(baseDestination, dest); const relativePath = path.relative(baseDestination, resolvedPath); - const isOutside = relativePath.startsWith('..'); - const hasIllegalChars = (dest || file.name).includes(':'); + const isOutside = relativePath.split(path.sep).includes('..'); + const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name); - if (isOutside || hasIllegalChars) { + if (isOutside || hasIllegalDrive) { let reason: SkipReason = SkipReason.DOWNLOAD_ERROR; if (isOutside) reason = SkipReason.PATH_TRAVERSAL; - else if (hasIllegalChars) reason = SkipReason.ILLEGAL_CHARACTER; + else if (hasIllegalDrive) reason = SkipReason.ILLEGAL_CHARACTER; - skippedFiles.push({ - fileName: file.name, - localPath: resolvedPath, - reason: reason, - message: `File ${file.name} was skipped due to path validation failure.`, - }); + const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus; + skippedResult.skipped = true; + skippedResult.reason = reason; + skippedResult.fileName = file.name; + skippedResult.localPath = resolvedPath; + skippedResult.message = `File ${file.name} was skipped due to path validation failure.`; + + skippedFiles.push(skippedResult); continue; } passThroughOptionsCopy.destination = dest; @@ -656,30 +667,25 @@ export class TransferManager { await fsp.mkdir(destination, {recursive: true}); return [Buffer.alloc(0)] as DownloadResponse; } - - return file.download(passThroughOptionsCopy); + const resp = (await file.download( + passThroughOptionsCopy + )) as DownloadResponseWithStatus; + resp.skipped = false; + resp.fileName = file.name; + return resp; } catch (err) { - const error = err instanceof Error ? err : new Error(String(err)); - skippedFiles.push({ - fileName: file.name, - localPath: path.resolve( - passThroughOptionsCopy.destination || file.name - ), - reason: SkipReason.DOWNLOAD_ERROR, - message: error.message, - error: error as Error, - }); - return [Buffer.alloc(0)] as DownloadResponse; + const errorResp = [Buffer.alloc(0)] as DownloadResponseWithStatus; + errorResp.skipped = true; + errorResp.reason = SkipReason.DOWNLOAD_ERROR; + errorResp.error = err as Error; + return errorResp; } }) ); } const results = await Promise.all(promises); - return { - responses: results, - skippedFiles: skippedFiles, - }; + return [...skippedFiles, ...results]; } /** diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 41f57756b..68510e156 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -40,9 +40,8 @@ import {AuthClient, GoogleAuth} from 'google-auth-library'; import {tmpdir} from 'os'; import fs from 'fs'; import {promises as fsp, Stats} from 'fs'; - import * as sinon from 'sinon'; -import {DownloadManyFilesResult, SkipReason} from '../src/file.js'; +import {DownloadResponseWithStatus, SkipReason} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -366,21 +365,18 @@ describe('Transfer Manager', () => { .resolves([Buffer.alloc(0)]); const maliciousDownloadStub = sandbox.stub(maliciousFile, 'download'); - const result = await transferManager.downloadManyFiles( + const result = (await transferManager.downloadManyFiles( [maliciousFile, validFile], {prefix} - ); + )) as DownloadResponseWithStatus[]; assert.strictEqual(maliciousDownloadStub.called, false); assert.strictEqual(downloadStub.calledOnce, true); - const results = result as DownloadManyFilesResult; - assert.strictEqual(results.skippedFiles.length, 1); - assert.strictEqual(results.skippedFiles[0].fileName, maliciousFilename); - assert.strictEqual( - results.skippedFiles[0].reason, - SkipReason.PATH_TRAVERSAL - ); + const skipped = result.find(r => r.fileName === maliciousFilename); + assert.ok(skipped); + assert.strictEqual(skipped!.skipped, true); + assert.strictEqual(skipped!.reason, SkipReason.PATH_TRAVERSAL); }); it('allows files with relative segments that resolve within the target directory', async () => { @@ -402,11 +398,13 @@ describe('Transfer Manager', () => { const file = new File(bucket, maliciousFilename); const downloadStub = sandbox.stub(file, 'download'); - const result = await transferManager.downloadManyFiles([file]); + const result = (await transferManager.downloadManyFiles([ + file, + ])) as DownloadResponseWithStatus[]; assert.strictEqual(downloadStub.called, false); - const results = result as DownloadManyFilesResult; - assert.strictEqual(results.skippedFiles.length, 1); + assert.strictEqual(result[0].skipped, true); + assert.strictEqual(result[0].reason, SkipReason.PATH_TRAVERSAL); }); it('jails absolute-looking paths with nested segments into the target directory', async () => { @@ -419,14 +417,16 @@ describe('Transfer Manager', () => { .stub(file, 'download') .resolves([Buffer.alloc(0)]); - const result = await transferManager.downloadManyFiles([file], {prefix}); + const result = (await transferManager.downloadManyFiles([file], { + prefix, + })) as DownloadResponseWithStatus[]; assert.strictEqual(downloadStub.called, true); const options = downloadStub.firstCall.args[0] as DownloadOptions; assert.strictEqual(options.destination, expectedDestination); - const results = result as DownloadManyFilesResult; - assert.strictEqual(results.skippedFiles.length, 0); + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].skipped, false); }); it('jails absolute-looking Unix paths (e.g. /etc/passwd) into the target directory instead of skipping', async () => { @@ -439,11 +439,14 @@ describe('Transfer Manager', () => { .stub(file, 'download') .resolves([Buffer.alloc(0)]); - await transferManager.downloadManyFiles([file], {prefix}); + const result = (await transferManager.downloadManyFiles([file], { + prefix, + })) as DownloadResponseWithStatus[]; assert.strictEqual(downloadStub.calledOnce, true); const options = downloadStub.firstCall.args[0] as DownloadOptions; assert.strictEqual(options.destination, expectedDestination); + assert.strictEqual(result[0].skipped, false); }); it('correctly handles stripPrefix and verifies the resulting path is still safe', async () => { @@ -456,27 +459,30 @@ describe('Transfer Manager', () => { const downloadStub = sandbox.stub(file, 'download'); - const result = await transferManager.downloadManyFiles([file], options); + const result = (await transferManager.downloadManyFiles( + [file], + options + )) as DownloadResponseWithStatus[]; assert.strictEqual(downloadStub.called, false); - const results = result as DownloadManyFilesResult; - assert.strictEqual(results.skippedFiles.length, 1); + assert.strictEqual(result[0].skipped, true); + assert.strictEqual(result[0].reason, SkipReason.PATH_TRAVERSAL); }); it('should skip files containing Windows volume separators (:) to prevent drive-injection attacks', async () => { const prefix = 'C:\\local\\target'; const maliciousFile = new File(bucket, 'C:\\system\\win32'); - const result = await transferManager.downloadManyFiles([maliciousFile], { + const result = (await transferManager.downloadManyFiles([maliciousFile], { prefix, - }); + })) as DownloadResponseWithStatus[]; - const results = result as DownloadManyFilesResult; - assert.strictEqual(results.skippedFiles.length, 1); - assert.strictEqual( - results.skippedFiles[0].reason, - SkipReason.ILLEGAL_CHARACTER - ); + assert.strictEqual(result.length, 1); + + const response = result[0]; + assert.strictEqual(response.skipped, true); + assert.strictEqual(response.reason, SkipReason.ILLEGAL_CHARACTER); + assert.strictEqual(response.fileName, 'C:\\system\\win32'); }); it('should account for every input file (Parity Check)', async () => { @@ -493,6 +499,8 @@ describe('Transfer Manager', () => { '..\\escape.txt', // Windows escape (Skip - Path Traversal '..') 'C:\\system\\win32', // Windows Drive (Skip - Illegal Char ':') 'C:\\local\\target\\a.txt', // Windows Absolute (Skip - Illegal Char ':') + '..temp.txt', // Leading dots in filename (Download - Not a traversal) + 'test-2026:01:01.txt', // GCS Timestamps (Download - Colon is middle, not drive) ]; const files = fileNames.map(name => bucket.file(name)); @@ -502,38 +510,38 @@ describe('Transfer Manager', () => { const result = (await transferManager.downloadManyFiles(files, { prefix, - })) as DownloadManyFilesResult; - - const totalProcessed = - result.responses.length + result.skippedFiles.length; + })) as DownloadResponseWithStatus[]; assert.strictEqual( - totalProcessed, + result.length, fileNames.length, - `Parity Failure: Processed ${totalProcessed} files but input had ${fileNames.length}` + `Parity Failure: Processed ${result.length} files but input had ${fileNames.length}` ); - const expectedDownloads = 7; + const downloads = result.filter(r => !r.skipped); + const skips = result.filter(r => r.skipped); + + const expectedDownloads = 9; const expectedSkips = 4; assert.strictEqual( - result.responses.length, + downloads.length, expectedDownloads, - `Expected ${expectedDownloads} downloads but got ${result.responses.length}` + `Expected ${expectedDownloads} downloads but got ${downloads.length}` ); assert.strictEqual( - result.skippedFiles.length, + skips.length, expectedSkips, - `Expected ${expectedSkips} skips but got ${result.skippedFiles.length}` + `Expected ${expectedSkips} skips but got ${skips.length}` ); - const traversalSkips = result.skippedFiles.filter( + const traversalSkips = skips.filter( f => f.reason === SkipReason.PATH_TRAVERSAL ); assert.strictEqual(traversalSkips.length, 2); - const illegalCharSkips = result.skippedFiles.filter( + const illegalCharSkips = skips.filter( f => f.reason === SkipReason.ILLEGAL_CHARACTER ); assert.strictEqual(illegalCharSkips.length, 2);