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..60e5f26c516 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,43 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; - if (options.prefix || passThroughOptionsCopy.destination) { - passThroughOptionsCopy.destination = path.join( + const normalizedGcsName = file.name + .replace(/\\/g, path.sep) + .replace(/\//g, path.sep); + + let dest: string; + if (options.stripPrefix) { + dest = normalizedGcsName.replace(regex, ''); + } else { + dest = path.join( options.prefix || '', passThroughOptionsCopy.destination || '', - file.name, + normalizedGcsName ); } - if (options.stripPrefix) { - passThroughOptionsCopy.destination = file.name.replace(regex, ''); + + 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) { + const reason = isOutside + ? SkipReason.PATH_TRAVERSAL + : 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 +651,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', () => {