diff --git a/samples/downloadManyFilesWithTransferManager.js b/samples/downloadManyFilesWithTransferManager.js index 7a464ad4c..817253207 100644 --- a/samples/downloadManyFilesWithTransferManager.js +++ b/samples/downloadManyFilesWithTransferManager.js @@ -49,10 +49,25 @@ function main( async function downloadManyFilesWithTransferManager() { // Downloads the files - await transferManager.downloadManyFiles([firstFileName, secondFileName]); + const {skippedFiles} = await transferManager.downloadManyFiles([ + firstFileName, + secondFileName, + ]); + const skippedFileNames = new Set(skippedFiles.map(f => f.fileName)); for (const fileName of [firstFileName, secondFileName]) { - console.log(`gs://${bucketName}/${fileName} downloaded to ${fileName}.`); + const isSkipped = skippedFileNames.has(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..6cdbdbe69 100644 --- a/src/file.ts +++ b/src/file.ts @@ -396,6 +396,27 @@ export interface CopyCallback { export type DownloadResponse = [Buffer]; +export interface DownloadManyFilesResult { + responses: DownloadResponse[]; + 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; + error?: Error; +} + export type DownloadCallback = ( err: RequestError | null, contents: Buffer diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index dd4e41eeb..26068a3eb 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -16,11 +16,14 @@ import {Bucket, UploadOptions, UploadResponse} from './bucket.js'; import { + DownloadManyFilesResult, DownloadOptions, DownloadResponse, File, FileExceptionMessages, RequestError, + SkippedFileInfo, + SkipReason, } from './file.js'; import pLimit from 'p-limit'; import * as path from 'path'; @@ -566,13 +569,18 @@ 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[] = []; 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 +606,39 @@ 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('..'); + const isAbsoluteAttempt = path.isAbsolute(relativePath); + + if (isOutside || isAbsoluteAttempt || file.name.includes(':')) { + 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({ + fileName: file.name, + localPath: resolvedPath, + reason: reason, + message: `File ${file.name} was skipped due to path validation failure.`, + }); + continue; } + passThroughOptionsCopy.destination = dest; + if ( options.skipIfExists && existsSync(passThroughOptionsCopy.destination || '') @@ -617,20 +648,39 @@ 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); }) ); } + 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..945ce7925 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, SkipReason} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -350,6 +351,163 @@ 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, + 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]); + + 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, + 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', () => {