From 5ae7e9406ed549b89b41e05e5f10ab814003846b Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Fri, 6 Mar 2026 13:38:38 +0000 Subject: [PATCH 1/8] 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. --- handwritten/storage/src/file.ts | 16 ++ handwritten/storage/src/transfer-manager.ts | 82 ++++++-- handwritten/storage/test/transfer-manager.ts | 198 ++++++++++++++++++- 3 files changed, 281 insertions(+), 15 deletions(-) diff --git a/handwritten/storage/src/file.ts b/handwritten/storage/src/file.ts index 5c51e63dfb0..28a2ec57f47 100644 --- a/handwritten/storage/src/file.ts +++ b/handwritten/storage/src/file.ts @@ -396,6 +396,22 @@ export interface CopyCallback { export type DownloadResponse = [Buffer]; +export type DownloadResponseWithStatus = [Buffer] & { + skipped?: boolean; + reason?: SkipReason; + fileName?: string; + localPath?: string; + message?: string; + error?: Error; +}; + +export enum SkipReason { + PATH_TRAVERSAL = 'PATH_TRAVERSAL', + ILLEGAL_CHARACTER = 'ILLEGAL_CHARACTER', + ALREADY_EXISTS = 'ALREADY_EXISTS', + DOWNLOAD_ERROR = 'DOWNLOAD_ERROR', +} + export type DownloadCallback = ( err: RequestError | null, contents: Buffer, diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index be34c76f08e..25c30aaa2bd 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -18,9 +18,11 @@ import {Bucket, UploadOptions, UploadResponse} from './bucket.js'; import { DownloadOptions, DownloadResponse, + DownloadResponseWithStatus, File, FileExceptionMessages, RequestError, + SkipReason, } from './file.js'; import pLimit from 'p-limit'; import * as path from 'path'; @@ -571,8 +573,13 @@ export class TransferManager { options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT, ); const promises: Promise[] = []; + const skippedFiles: DownloadResponseWithStatus[] = []; 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 +605,50 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; + const normalizedGcsName = file.name + .replace(/\\/g, path.sep) + .replace(/\//g, path.sep); + + let dest: string; if (options.prefix || passThroughOptionsCopy.destination) { - passThroughOptionsCopy.destination = path.join( + dest = path.join( options.prefix || '', passThroughOptionsCopy.destination || '', - file.name, + normalizedGcsName ); } if (options.stripPrefix) { - passThroughOptionsCopy.destination = file.name.replace(regex, ''); + dest = normalizedGcsName.replace(regex, ''); + } else { + dest = path.join( + options.prefix || '', + passThroughOptionsCopy.destination || '', + normalizedGcsName + ); } + + const resolvedPath = path.resolve(baseDestination, dest); + const relativePath = path.relative(baseDestination, resolvedPath); + const isOutside = relativePath.split(path.sep).includes('..'); + const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name); + + if (isOutside || hasIllegalDrive) { + let reason: SkipReason = SkipReason.DOWNLOAD_ERROR; + if (isOutside) reason = SkipReason.PATH_TRAVERSAL; + else if (hasIllegalDrive) reason = SkipReason.ILLEGAL_CHARACTER; + + 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; + if ( options.skipIfExists && existsSync(passThroughOptionsCopy.destination || '') @@ -617,20 +658,33 @@ 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; + } + const resp = (await file.download( + passThroughOptionsCopy + )) as DownloadResponseWithStatus; + resp.skipped = false; + resp.fileName = file.name; + return resp; + } catch (err) { + const errorResp = [Buffer.alloc(0)] as DownloadResponseWithStatus; + errorResp.skipped = true; + errorResp.reason = SkipReason.DOWNLOAD_ERROR; + errorResp.error = err as Error; + return errorResp; } - - return file.download(passThroughOptionsCopy); - }), + }) ); } - - return Promise.all(promises); + const results = await Promise.all(promises); + return [...skippedFiles, ...results]; } /** diff --git a/handwritten/storage/test/transfer-manager.ts b/handwritten/storage/test/transfer-manager.ts index 2582782fa7a..68510e15633 100644 --- a/handwritten/storage/test/transfer-manager.ts +++ b/handwritten/storage/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'; @@ -39,8 +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 {DownloadResponseWithStatus, SkipReason} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -350,6 +351,201 @@ describe('Transfer Manager', () => { true ); }); + + it('skips files that attempt path traversal via dot-segments (../) and returns them in skippedFiles', async () => { + const prefix = 'download-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} + )) as DownloadResponseWithStatus[]; + + assert.strictEqual(maliciousDownloadStub.called, false); + assert.strictEqual(downloadStub.calledOnce, true); + + 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 () => { + 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, + ])) as DownloadResponseWithStatus[]; + + assert.strictEqual(downloadStub.called, false); + 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 () => { + 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, + })) as DownloadResponseWithStatus[]; + + assert.strictEqual(downloadStub.called, true); + const options = downloadStub.firstCall.args[0] as DownloadOptions; + assert.strictEqual(options.destination, expectedDestination); + + 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 () => { + 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)]); + + 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 () => { + 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 + )) as DownloadResponseWithStatus[]; + + assert.strictEqual(downloadStub.called, false); + 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], { + prefix, + })) as DownloadResponseWithStatus[]; + + 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 () => { + const prefix = '/local/target'; + const fileNames = [ + '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 ':') + '..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)); + + sandbox.stub(File.prototype, 'download').resolves([Buffer.alloc(0)]); + sandbox.stub(fsp, 'mkdir').resolves(); + + const result = (await transferManager.downloadManyFiles(files, { + prefix, + })) as DownloadResponseWithStatus[]; + + assert.strictEqual( + result.length, + fileNames.length, + `Parity Failure: Processed ${result.length} files but input had ${fileNames.length}` + ); + + const downloads = result.filter(r => !r.skipped); + const skips = result.filter(r => r.skipped); + + const expectedDownloads = 9; + const expectedSkips = 4; + + assert.strictEqual( + downloads.length, + expectedDownloads, + `Expected ${expectedDownloads} downloads but got ${downloads.length}` + ); + + assert.strictEqual( + skips.length, + expectedSkips, + `Expected ${expectedSkips} skips but got ${skips.length}` + ); + + const traversalSkips = skips.filter( + f => f.reason === SkipReason.PATH_TRAVERSAL + ); + assert.strictEqual(traversalSkips.length, 2); + + const illegalCharSkips = skips.filter( + f => f.reason === SkipReason.ILLEGAL_CHARACTER + ); + assert.strictEqual(illegalCharSkips.length, 2); + }); }); describe('downloadFileInChunks', () => { From 83c28c92e8cb9cfc7abe59189a04fc7998aee816 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 10 Mar 2026 08:08:37 +0000 Subject: [PATCH 2/8] fix double-assignment --- handwritten/storage/src/transfer-manager.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index 25c30aaa2bd..60e5f26c516 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -610,13 +610,6 @@ export class TransferManager { .replace(/\//g, path.sep); let dest: string; - if (options.prefix || passThroughOptionsCopy.destination) { - dest = path.join( - options.prefix || '', - passThroughOptionsCopy.destination || '', - normalizedGcsName - ); - } if (options.stripPrefix) { dest = normalizedGcsName.replace(regex, ''); } else { @@ -633,9 +626,9 @@ export class TransferManager { const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name); if (isOutside || hasIllegalDrive) { - let reason: SkipReason = SkipReason.DOWNLOAD_ERROR; - if (isOutside) reason = SkipReason.PATH_TRAVERSAL; - else if (hasIllegalDrive) reason = SkipReason.ILLEGAL_CHARACTER; + const reason = isOutside + ? SkipReason.PATH_TRAVERSAL + : SkipReason.ILLEGAL_CHARACTER; const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus; skippedResult.skipped = true; From 57686a9c1b0ab299a9c2f4601e9b1f95aff8d673 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Mon, 16 Mar 2026 13:39:02 +0000 Subject: [PATCH 3/8] feat(storage): secure path resolution and preserve result parity - Implemented "jail" logic using path.resolve to prevent traversal. - Neutralized leading slashes in GCS object names. - Pre-allocated results array to maintain 1:1 input/output index parity. - Added automatic recursive directory creation for nested local paths. - Fixed prioritization of destination options in downloadManyFiles. --- handwritten/storage/src/transfer-manager.ts | 76 +++++++++++--------- handwritten/storage/test/transfer-manager.ts | 16 +++-- 2 files changed, 52 insertions(+), 40 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index 60e5f26c516..ae71f6f0bf3 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -572,12 +572,11 @@ export class TransferManager { const limit = pLimit( options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT, ); - const promises: Promise[] = []; - const skippedFiles: DownloadResponseWithStatus[] = []; + const promises: Promise[] = []; let files: File[] = []; const baseDestination = path.resolve( - options.prefix || options.passthroughOptions?.destination || '.' + options.passthroughOptions?.destination || options.prefix || '.' ); if (!Array.isArray(filesOrFolder)) { @@ -599,30 +598,28 @@ export class TransferManager { : EMPTY_REGEX; const regex = new RegExp(stripRegexString, 'g'); - for (const file of files) { - const passThroughOptionsCopy = { - ...options.passthroughOptions, - [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, - }; + const finalResults: DownloadResponseWithStatus[] = new Array(files.length); + + for (let i = 0; i < files.length; i++) { + const file = files[i]; const normalizedGcsName = file.name - .replace(/\\/g, path.sep) - .replace(/\//g, path.sep); + .replace(/\\/g, '/') + .replace(/^\/+/, ''); let dest: string; if (options.stripPrefix) { dest = normalizedGcsName.replace(regex, ''); } else { - dest = path.join( - options.prefix || '', - passThroughOptionsCopy.destination || '', - normalizedGcsName - ); + dest = normalizedGcsName; } const resolvedPath = path.resolve(baseDestination, dest); - const relativePath = path.relative(baseDestination, resolvedPath); - const isOutside = relativePath.split(path.sep).includes('..'); + const relativeFromBase = path.relative(baseDestination, resolvedPath); + + const isOutside = + path.isAbsolute(relativeFromBase) || + relativeFromBase.split(path.sep).includes('..'); const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name); if (isOutside || hasIllegalDrive) { @@ -635,49 +632,60 @@ export class TransferManager { 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); + finalResults[i] = skippedResult; continue; } - passThroughOptionsCopy.destination = dest; - if ( - options.skipIfExists && - existsSync(passThroughOptionsCopy.destination || '') - ) { + if (options.skipIfExists && existsSync(resolvedPath)) { continue; } + const passThroughOptionsCopy = { + ...options.passthroughOptions, + destination: resolvedPath, + [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, + }; + promises.push( limit(async () => { try { - const destination = passThroughOptionsCopy.destination; - if ( - destination && - (destination.endsWith(path.sep) || destination.endsWith('/')) - ) { + const destination = passThroughOptionsCopy.destination!; + + if (destination.endsWith(path.sep) || destination.endsWith('/')) { await fsp.mkdir(destination, {recursive: true}); - return [Buffer.alloc(0)] as DownloadResponse; + const dirResp = [Buffer.alloc(0)] as DownloadResponseWithStatus; + dirResp.skipped = false; + dirResp.fileName = file.name; + dirResp.localPath = destination; + finalResults[i] = dirResp; + return; } + + await fsp.mkdir(path.dirname(destination), {recursive: true}); + const resp = (await file.download( passThroughOptionsCopy )) as DownloadResponseWithStatus; resp.skipped = false; resp.fileName = file.name; - return resp; + resp.localPath = destination; + finalResults[i] = resp; } catch (err) { const errorResp = [Buffer.alloc(0)] as DownloadResponseWithStatus; errorResp.skipped = true; errorResp.reason = SkipReason.DOWNLOAD_ERROR; + errorResp.fileName = file.name; + errorResp.localPath = resolvedPath; errorResp.error = err as Error; - return errorResp; + finalResults[i] = errorResp; } }) ); } - const results = await Promise.all(promises); - return [...skippedFiles, ...results]; + + await Promise.all(promises); + return finalResults; } /** diff --git a/handwritten/storage/test/transfer-manager.ts b/handwritten/storage/test/transfer-manager.ts index 68510e15633..96306ddf053 100644 --- a/handwritten/storage/test/transfer-manager.ts +++ b/handwritten/storage/test/transfer-manager.ts @@ -325,7 +325,7 @@ describe('Transfer Manager', () => { const file = 'first.txt'; const filesOrFolder = [folder, path.join(folder, file)]; const expectedFilePath = path.join(prefix, folder, file); - const expectedDir = path.join(prefix, folder); + const expectedDir = path.resolve(prefix, folder); const mkdirSpy = sandbox.spy(fsp, 'mkdir'); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { @@ -345,9 +345,7 @@ describe('Transfer Manager', () => { prefix: prefix, }); assert.strictEqual( - mkdirSpy.calledOnceWith(expectedDir, { - recursive: true, - }), + mkdirSpy.calledWith(expectedDir, {recursive: true}), true ); }); @@ -411,7 +409,10 @@ describe('Transfer Manager', () => { const prefix = './downloads'; const filename = '/tmp/shady.txt'; const file = new File(bucket, filename); - const expectedDestination = path.join(prefix, filename); + const expectedDestination = path.resolve( + prefix, + filename.replace(/^\/+/, '') + ); const downloadStub = sandbox .stub(file, 'download') @@ -432,7 +433,10 @@ describe('Transfer Manager', () => { 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 expectedDestination = path.resolve( + prefix, + filename.replace(/^\/+/, '') + ); const file = new File(bucket, filename); const downloadStub = sandbox From 11ef1aa1e23d53642f2fce64a59771be4d008694 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Mon, 16 Mar 2026 14:07:26 +0000 Subject: [PATCH 4/8] feat(storage): prioritize drive check for Windows compatibility - Reordered security checks to catch illegal drive letters before path resolution. - Fixed SkipReason mismatch (Illegal Character vs Path Traversal) on Windows. - Ensured absolute Windows paths are neutralized before traversal validation. --- handwritten/storage/src/transfer-manager.ts | 29 +++++++++++---------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index ae71f6f0bf3..3f4a3f2a236 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -603,16 +603,23 @@ export class TransferManager { for (let i = 0; i < files.length; i++) { const file = files[i]; + const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name); + if (hasIllegalDrive) { + const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus; + skippedResult.skipped = true; + skippedResult.reason = SkipReason.ILLEGAL_CHARACTER; + skippedResult.fileName = file.name; + finalResults[i] = skippedResult; + continue; + } + const normalizedGcsName = file.name .replace(/\\/g, '/') .replace(/^\/+/, ''); - let dest: string; - if (options.stripPrefix) { - dest = normalizedGcsName.replace(regex, ''); - } else { - dest = normalizedGcsName; - } + const dest = options.stripPrefix + ? normalizedGcsName.replace(regex, '') + : normalizedGcsName; const resolvedPath = path.resolve(baseDestination, dest); const relativeFromBase = path.relative(baseDestination, resolvedPath); @@ -620,19 +627,13 @@ export class TransferManager { const isOutside = path.isAbsolute(relativeFromBase) || relativeFromBase.split(path.sep).includes('..'); - const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name); - - if (isOutside || hasIllegalDrive) { - const reason = isOutside - ? SkipReason.PATH_TRAVERSAL - : SkipReason.ILLEGAL_CHARACTER; + if (isOutside) { const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus; skippedResult.skipped = true; - skippedResult.reason = reason; + skippedResult.reason = SkipReason.PATH_TRAVERSAL; skippedResult.fileName = file.name; skippedResult.localPath = resolvedPath; - finalResults[i] = skippedResult; continue; } From c38262e35b8ceff422d162acfb62ecf0443b6b65 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 17 Mar 2026 11:40:42 +0000 Subject: [PATCH 5/8] fix(storage): storage downloadManyFiles scoping and path traversal safety Isolated async loop variables to fix path leakage, decoupled prefix from destination logic, and added cross-platform traversal checks for both forward and backslashes. --- handwritten/storage/src/transfer-manager.ts | 31 ++++++++------- handwritten/storage/test/transfer-manager.ts | 42 +++++++++++--------- 2 files changed, 40 insertions(+), 33 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index 3f4a3f2a236..326573e475f 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -576,7 +576,7 @@ export class TransferManager { let files: File[] = []; const baseDestination = path.resolve( - options.passthroughOptions?.destination || options.prefix || '.' + options.passthroughOptions?.destination || '.' ); if (!Array.isArray(filesOrFolder)) { @@ -613,9 +613,7 @@ export class TransferManager { continue; } - const normalizedGcsName = file.name - .replace(/\\/g, '/') - .replace(/^\/+/, ''); + const normalizedGcsName = file.name.replace(/^[\\/]+/, ''); const dest = options.stripPrefix ? normalizedGcsName.replace(regex, '') @@ -626,7 +624,7 @@ export class TransferManager { const isOutside = path.isAbsolute(relativeFromBase) || - relativeFromBase.split(path.sep).includes('..'); + relativeFromBase.split(/[\\/]/).includes('..'); if (isOutside) { const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus; @@ -642,14 +640,14 @@ export class TransferManager { continue; } - const passThroughOptionsCopy = { - ...options.passthroughOptions, - destination: resolvedPath, - [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, - }; - promises.push( limit(async () => { + const passThroughOptionsCopy = { + ...options.passthroughOptions, + destination: resolvedPath, + [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, + }; + try { const destination = passThroughOptionsCopy.destination!; @@ -668,10 +666,13 @@ export class TransferManager { const resp = (await file.download( passThroughOptionsCopy )) as DownloadResponseWithStatus; - resp.skipped = false; - resp.fileName = file.name; - resp.localPath = destination; - finalResults[i] = resp; + + finalResults[i] = { + ...resp, + skipped: false, + fileName: file.name, + localPath: destination, + } as DownloadResponse; } catch (err) { const errorResp = [Buffer.alloc(0)] as DownloadResponseWithStatus; errorResp.skipped = true; diff --git a/handwritten/storage/test/transfer-manager.ts b/handwritten/storage/test/transfer-manager.ts index 96306ddf053..7433f05462e 100644 --- a/handwritten/storage/test/transfer-manager.ts +++ b/handwritten/storage/test/transfer-manager.ts @@ -342,7 +342,7 @@ describe('Transfer Manager', () => { return file; }); await transferManager.downloadManyFiles(filesOrFolder, { - prefix: prefix, + passthroughOptions: {destination: prefix}, }); assert.strictEqual( mkdirSpy.calledWith(expectedDir, {recursive: true}), @@ -351,7 +351,7 @@ describe('Transfer Manager', () => { }); it('skips files that attempt path traversal via dot-segments (../) and returns them in skippedFiles', async () => { - const prefix = 'download-directory'; + const destination = 'download-directory'; const maliciousFilename = '../../etc/passwd'; const validFilename = 'valid.txt'; @@ -365,7 +365,7 @@ describe('Transfer Manager', () => { const result = (await transferManager.downloadManyFiles( [maliciousFile, validFile], - {prefix} + {passthroughOptions: {destination: destination}} )) as DownloadResponseWithStatus[]; assert.strictEqual(maliciousDownloadStub.called, false); @@ -378,7 +378,7 @@ describe('Transfer Manager', () => { }); it('allows files with relative segments that resolve within the target directory', async () => { - const prefix = 'safe-directory'; + const destination = 'safe-directory'; const filename = './subdir/../subdir/file.txt'; const file = new File(bucket, filename); @@ -386,7 +386,9 @@ describe('Transfer Manager', () => { .stub(file, 'download') .resolves([Buffer.alloc(0)]); - await transferManager.downloadManyFiles([file], {prefix}); + await transferManager.downloadManyFiles([file], { + passthroughOptions: {destination: destination}, + }); assert.strictEqual(downloadStub.calledOnce, true); }); @@ -406,11 +408,11 @@ describe('Transfer Manager', () => { }); it('jails absolute-looking paths with nested segments into the target directory', async () => { - const prefix = './downloads'; + const destination = './downloads'; const filename = '/tmp/shady.txt'; const file = new File(bucket, filename); const expectedDestination = path.resolve( - prefix, + destination, filename.replace(/^\/+/, '') ); @@ -419,7 +421,7 @@ describe('Transfer Manager', () => { .resolves([Buffer.alloc(0)]); const result = (await transferManager.downloadManyFiles([file], { - prefix, + passthroughOptions: {destination: destination}, })) as DownloadResponseWithStatus[]; assert.strictEqual(downloadStub.called, true); @@ -431,10 +433,10 @@ describe('Transfer Manager', () => { }); it('jails absolute-looking Unix paths (e.g. /etc/passwd) into the target directory instead of skipping', async () => { - const prefix = 'downloads'; + const destination = 'downloads'; const filename = '/etc/passwd'; const expectedDestination = path.resolve( - prefix, + destination, filename.replace(/^\/+/, '') ); @@ -444,7 +446,7 @@ describe('Transfer Manager', () => { .resolves([Buffer.alloc(0)]); const result = (await transferManager.downloadManyFiles([file], { - prefix, + passthroughOptions: {destination: destination}, })) as DownloadResponseWithStatus[]; assert.strictEqual(downloadStub.calledOnce, true); @@ -474,11 +476,11 @@ describe('Transfer Manager', () => { }); it('should skip files containing Windows volume separators (:) to prevent drive-injection attacks', async () => { - const prefix = 'C:\\local\\target'; + const destination = 'C:\\local\\target'; const maliciousFile = new File(bucket, 'C:\\system\\win32'); const result = (await transferManager.downloadManyFiles([maliciousFile], { - prefix, + passthroughOptions: {destination: destination}, })) as DownloadResponseWithStatus[]; assert.strictEqual(result.length, 1); @@ -490,7 +492,7 @@ describe('Transfer Manager', () => { }); it('should account for every input file (Parity Check)', async () => { - const prefix = '/local/target'; + const destination = '/local/target'; const fileNames = [ 'data/file.txt', // Normal (Download) 'data/../sibling.txt', // Internal Traversal (Download) @@ -505,6 +507,10 @@ describe('Transfer Manager', () => { '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) + '\\a\\b\\c.txt', // Leading backslash (Should stay in base) + '/abs/path/file.txt', // Leading forward slash (Should stay in base) + '\\\\network\\share', // UNC-style leading double backslash + '//multiple//slashes', // Multiple leading forward slashes ]; const files = fileNames.map(name => bucket.file(name)); @@ -513,7 +519,7 @@ describe('Transfer Manager', () => { sandbox.stub(fsp, 'mkdir').resolves(); const result = (await transferManager.downloadManyFiles(files, { - prefix, + passthroughOptions: {destination: destination}, })) as DownloadResponseWithStatus[]; assert.strictEqual( @@ -525,8 +531,8 @@ describe('Transfer Manager', () => { const downloads = result.filter(r => !r.skipped); const skips = result.filter(r => r.skipped); - const expectedDownloads = 9; - const expectedSkips = 4; + const expectedDownloads = 12; + const expectedSkips = 5; assert.strictEqual( downloads.length, @@ -543,7 +549,7 @@ describe('Transfer Manager', () => { const traversalSkips = skips.filter( f => f.reason === SkipReason.PATH_TRAVERSAL ); - assert.strictEqual(traversalSkips.length, 2); + assert.strictEqual(traversalSkips.length, 3); const illegalCharSkips = skips.filter( f => f.reason === SkipReason.ILLEGAL_CHARACTER From 283eadf141d6d66ed2b79f54a5c1247371bc8506 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 17 Mar 2026 12:08:48 +0000 Subject: [PATCH 6/8] fix(storage): ensure unique path resolution and cross-platform safety Updated Parity Check tests with platform-conditional logic to handle OS-specific backslash resolution. --- handwritten/storage/test/transfer-manager.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/handwritten/storage/test/transfer-manager.ts b/handwritten/storage/test/transfer-manager.ts index 7433f05462e..c6f5c7effd6 100644 --- a/handwritten/storage/test/transfer-manager.ts +++ b/handwritten/storage/test/transfer-manager.ts @@ -492,6 +492,7 @@ describe('Transfer Manager', () => { }); it('should account for every input file (Parity Check)', async () => { + const isWindows = process.platform === 'win32'; const destination = '/local/target'; const fileNames = [ 'data/file.txt', // Normal (Download) @@ -501,7 +502,7 @@ describe('Transfer Manager', () => { '/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) + 'data\\..\\sibling.txt', // Windows traversal (Download) | LINUX: Skip (traversal) '..\\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 ':') @@ -531,8 +532,9 @@ describe('Transfer Manager', () => { const downloads = result.filter(r => !r.skipped); const skips = result.filter(r => r.skipped); - const expectedDownloads = 12; - const expectedSkips = 5; + const expectedDownloads = isWindows ? 13 : 12; + const expectedSkips = isWindows ? 4 : 5; + const expectedTraversalSkips = isWindows ? 2 : 3; assert.strictEqual( downloads.length, @@ -549,7 +551,7 @@ describe('Transfer Manager', () => { const traversalSkips = skips.filter( f => f.reason === SkipReason.PATH_TRAVERSAL ); - assert.strictEqual(traversalSkips.length, 3); + assert.strictEqual(traversalSkips.length, expectedTraversalSkips); const illegalCharSkips = skips.filter( f => f.reason === SkipReason.ILLEGAL_CHARACTER From 35ead710591208b8dcbcb4cb9606d2031bb0955f Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Tue, 17 Mar 2026 14:10:04 +0000 Subject: [PATCH 7/8] fix(storage): address logical escapes in stripPrefix --- handwritten/storage/src/transfer-manager.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index 326573e475f..0fec8ffb4dd 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -613,11 +613,11 @@ export class TransferManager { continue; } - const normalizedGcsName = file.name.replace(/^[\\/]+/, ''); + const fileName = options.stripPrefix + ? file.name.replace(regex, '') + : file.name; - const dest = options.stripPrefix - ? normalizedGcsName.replace(regex, '') - : normalizedGcsName; + const dest = fileName.replace(/^[\\/]+/, ''); const resolvedPath = path.resolve(baseDestination, dest); const relativeFromBase = path.relative(baseDestination, resolvedPath); From 3355bb0271fa6973ca8dea074164d1db7bdc92bd Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Fri, 20 Mar 2026 08:38:11 +0000 Subject: [PATCH 8/8] fix: clarify downloadManyFiles docs --- handwritten/storage/src/transfer-manager.ts | 39 ++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/handwritten/storage/src/transfer-manager.ts b/handwritten/storage/src/transfer-manager.ts index 0fec8ffb4dd..556056b9ad6 100644 --- a/handwritten/storage/src/transfer-manager.ts +++ b/handwritten/storage/src/transfer-manager.ts @@ -542,6 +542,34 @@ export class TransferManager { * instead of being returned as a buffer. * @returns {Promise} * + * @behavior + * **Return shape change (breaking/observable behavior):** + * - Previously, the returned array only contained entries for files that were successfully downloaded. + * - This meant the response length could be smaller than the number of requested input files. + * - Now, the returned array always has the same length and ordering as the input file list. + * - Each index in the response corresponds directly to the same index in the input. + * - Files that are skipped or fail will still have an entry in the result with: + * - `skipped = true` + * - `reason` populated with a {@link SkipReason} + * + * **New guarantees:** + * - Response length === number of requested files + * - Stable positional mapping between input and output + * - All outcomes (success, skipped, error) are explicitly represented + * + * @security + * **Path traversal protection (new):** + * - File paths are resolved relative to the configured destination directory. + * - Any file whose resolved path escapes the base destination directory is rejected. + * - This prevents directory traversal attacks (e.g. `../../etc/passwd`). + * - Such files are not downloaded and instead return: + * - `skipped = true` + * - `reason = SkipReason.PATH_TRAVERSAL` + * + * **Additional validation:** + * - File names containing illegal drive prefixes (e.g. `C:\`) are skipped + * to prevent unintended writes on host systems. + * * @example * ``` * const {Storage} = require('@google-cloud/storage'); @@ -556,12 +584,15 @@ export class TransferManager { * // The following files have been downloaded: * // - "file1.txt" (with the contents from my-bucket.file1.txt) * // - "file2.txt" (with the contents from my-bucket.file2.txt) + * // response.length === 2 (always matches input length) + * // Each entry corresponds to the respective input file * const response = await transferManager.downloadManyFiles([bucket.File('file1.txt'), bucket.File('file2.txt')]); * // The following files have been downloaded: * // - "file1.txt" (with the contents from my-bucket.file1.txt) * // - "file2.txt" (with the contents from my-bucket.file2.txt) * const response = await transferManager.downloadManyFiles('test-folder'); - * // All files with GCS prefix of 'test-folder' have been downloaded. + * // All files with GCS prefix of 'test-folder' have been processed. + * // Skipped or failed files are still included in the response. * ``` * */ @@ -637,6 +668,12 @@ export class TransferManager { } if (options.skipIfExists && existsSync(resolvedPath)) { + const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus; + skippedResult.skipped = true; + skippedResult.reason = SkipReason.ALREADY_EXISTS; + skippedResult.fileName = file.name; + skippedResult.localPath = resolvedPath; + finalResults[i] = skippedResult; continue; }