diff --git a/samples/system-test/transfer-manager.test.js b/samples/system-test/transfer-manager.test.js index 9f51c3598..f6180bed3 100644 --- a/samples/system-test/transfer-manager.test.js +++ b/samples/system-test/transfer-manager.test.js @@ -56,37 +56,26 @@ describe('transfer manager', () => { ); }); - it('should download multiple files', async () => { - // Remove absolute path marker to prepare for joining/validation. - const expectedFirstFilePath = firstFilePath.startsWith('/') - ? firstFilePath.slice(1) - : firstFilePath; - const expectedSecondFilePath = secondFilePath.startsWith('/') - ? secondFilePath.slice(1) - : secondFilePath; + it('should download mulitple files', async () => { const output = execSync( - `node downloadManyFilesWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${expectedSecondFilePath}` + `node downloadManyFilesWithTransferManager.js ${bucketName} ${firstFilePath} ${secondFilePath}` ); assert.match( output, new RegExp( - `gs://${bucketName}/${expectedFirstFilePath} downloaded to ${expectedFirstFilePath}.\ngs://${bucketName}/${expectedSecondFilePath} downloaded to ${expectedSecondFilePath}.` + `gs://${bucketName}/${firstFilePath} downloaded to ${firstFilePath}.\ngs://${bucketName}/${secondFilePath} downloaded to ${secondFilePath}.` ) ); }); it('should download a file utilizing chunked download', async () => { - // Remove absolute path marker to prepare for joining/validation. - const expectedFirstFilePath = firstFilePath.startsWith('/') - ? firstFilePath.slice(1) - : firstFilePath; const output = execSync( - `node downloadFileInChunksWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${downloadFilePath} ${chunkSize}` + `node downloadFileInChunksWithTransferManager.js ${bucketName} ${firstFilePath} ${downloadFilePath} ${chunkSize}` ); assert.match( output, new RegExp( - `gs://${bucketName}/${expectedFirstFilePath} downloaded to ${downloadFilePath}.` + `gs://${bucketName}/${firstFilePath} downloaded to ${downloadFilePath}.` ) ); }); diff --git a/src/file.ts b/src/file.ts index 2aa4ec7e1..0d18458bb 100644 --- a/src/file.ts +++ b/src/file.ts @@ -545,9 +545,6 @@ export enum FileExceptionMessages { To be sure the content is the same, you should try uploading the file again.`, MD5_RESUMED_UPLOAD = 'MD5 cannot be used with a continued resumable upload as MD5 cannot be extended from an existing value', MISSING_RESUME_CRC32C_FINAL_UPLOAD = 'The CRC32C is missing for the final portion of a resumed upload, which is required for validation. Please provide `resumeCRC32C` if validation is required, or disable `validation`.', - ABSOLUTE_FILE_NAME = 'Object name is an absolute path. Security block to prevent arbitrary file writes.', - TRAVERSAL_OUTSIDE_BASE = 'Path traversal detected. Security block to prevent writing outside the base directory.', - TRAVERSAL_OUTSIDE_BASE_DESTINATION = "The provided destination path is unsafe and attempts to traverse outside the application's base directory (current working directory).", } /** diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index db7dd4fc6..dd4e41eeb 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -497,16 +497,9 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.UPLOAD_MANY, }; - if (options.customDestinationBuilder) { - passThroughOptionsCopy.destination = options.customDestinationBuilder( - filePath, - options - ); - } else { - let segments = filePath.split(path.sep); - segments = segments.filter(s => s !== ''); - passThroughOptionsCopy.destination = path.posix.join(...segments); - } + passThroughOptionsCopy.destination = options.customDestinationBuilder + ? options.customDestinationBuilder(filePath, options) + : filePath.split(path.sep).join(path.posix.sep); if (options.prefix) { passThroughOptionsCopy.destination = path.posix.join( ...options.prefix.split(path.sep), @@ -594,61 +587,27 @@ export class TransferManager { }); } - const baseDir = this._resolveAndValidateBaseDir(options); - const stripRegexString = options.stripPrefix ? `^${options.stripPrefix}` : EMPTY_REGEX; const regex = new RegExp(stripRegexString, 'g'); - const createdDirectories = new Set(); for (const file of files) { - let name = file.name; - - // Apply stripPrefix first if requested - if (options.stripPrefix) { - name = name.replace(regex, ''); - } - - // This ensures the full intended relative path is validated. - if (options.prefix) { - name = path.join(options.prefix, name); - } - - // Reject absolute paths and traversal sequences - if (path.isAbsolute(name)) { - const absolutePathError = new RequestError( - FileExceptionMessages.ABSOLUTE_FILE_NAME - ); - throw absolutePathError; - } - - // Resolve the final path and perform the containment check - let finalPath = path.resolve(baseDir, name); - const normalizedBaseDir = baseDir.endsWith(path.sep) - ? baseDir - : baseDir + path.sep; - if (finalPath !== baseDir && !finalPath.startsWith(normalizedBaseDir)) { - const traversalError = new RequestError( - FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE - ); - throw traversalError; - } - - if (file.name.endsWith('/') && !finalPath.endsWith(path.sep)) { - finalPath = finalPath + path.sep; - } - const passThroughOptionsCopy = { ...options.passthroughOptions, - destination: finalPath, [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; - const destinationDir = finalPath.endsWith(path.sep) - ? finalPath - : path.dirname(finalPath); - + if (options.prefix || passThroughOptionsCopy.destination) { + passThroughOptionsCopy.destination = path.join( + options.prefix || '', + passThroughOptionsCopy.destination || '', + file.name + ); + } + if (options.stripPrefix) { + passThroughOptionsCopy.destination = file.name.replace(regex, ''); + } if ( options.skipIfExists && existsSync(passThroughOptionsCopy.destination || '') @@ -659,12 +618,8 @@ export class TransferManager { promises.push( limit(async () => { const destination = passThroughOptionsCopy.destination; - if (!createdDirectories.has(destinationDir)) { - // If not, create it and add it to the set for tracking - await fsp.mkdir(destinationDir, {recursive: true}); - createdDirectories.add(destinationDir); - } if (destination && destination.endsWith(path.sep)) { + await fsp.mkdir(destination, {recursive: true}); return Promise.resolve([ Buffer.alloc(0), ]) as Promise; @@ -912,34 +867,4 @@ export class TransferManager { : yield fullPath; } } - - /** - * Resolves the absolute base directory for downloads and validates it against - * the current working directory (CWD) to prevent path traversal outside the base destination. - * @param options The download options, potentially containing passthroughOptions.destination. - * @returns The absolute, validated base directory path (baseDir). - */ - private _resolveAndValidateBaseDir( - options: DownloadManyFilesOptions - ): string { - const cwd = process.cwd(); - - // Resolve baseDir, defaulting to CWD if no destination is provided - const baseDir = path.resolve( - options.passthroughOptions?.destination ?? cwd - ); - - // Check for path traversal: baseDir must be equal to or contained within cwd. - const relativeBaseDir = path.relative(cwd, baseDir); - - // The condition checks for traversal ('..') or cross-drive traversal (absolute path on Windows) - if (relativeBaseDir.startsWith('..') || path.isAbsolute(relativeBaseDir)) { - const traversalError = new RequestError( - FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE_DESTINATION - ); - throw traversalError; - } - - return baseDir; - } } diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 2ab91ec1f..2582782fa 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -41,7 +41,6 @@ import fs from 'fs'; import {promises as fsp, Stats} from 'fs'; import * as sinon from 'sinon'; -import {FileExceptionMessages, RequestError} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -219,10 +218,7 @@ describe('Transfer Manager', () => { it('sets the destination correctly when provided a prefix', async () => { const prefix = 'test-prefix'; const filename = 'first.txt'; - const expectedDestination = path.resolve( - process.cwd(), - path.join(prefix, filename) - ); + const expectedDestination = path.normalize(`${prefix}/${filename}`); const file = new File(bucket, filename); sandbox.stub(file, 'download').callsFake(options => { @@ -237,7 +233,7 @@ describe('Transfer Manager', () => { it('sets the destination correctly when provided a strip prefix', async () => { const stripPrefix = 'should-be-removed/'; const filename = 'should-be-removed/first.txt'; - const expectedDestination = path.resolve(process.cwd(), 'first.txt'); + const expectedDestination = 'first.txt'; const file = new File(bucket, filename); sandbox.stub(file, 'download').callsFake(options => { @@ -267,10 +263,8 @@ describe('Transfer Manager', () => { destination: 'test-destination', }; const filename = 'first.txt'; - const expectedDestination = path.resolve( - process.cwd(), - passthroughOptions.destination, - filename + const expectedDestination = path.normalize( + `${passthroughOptions.destination}/${filename}` ); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { @@ -286,57 +280,6 @@ describe('Transfer Manager', () => { await transferManager.downloadManyFiles([file], {passthroughOptions}); }); - it('should throws an error for absolute file names', async () => { - const expectedErr = new RequestError( - FileExceptionMessages.ABSOLUTE_FILE_NAME - ); - const maliciousFilename = '/etc/passwd'; - const file = new File(bucket, maliciousFilename); - - await assert.rejects( - transferManager.downloadManyFiles([file]), - expectedErr - ); - }); - - it('should throw an error for path traversal in destination', async () => { - const expectedErr = new RequestError( - FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE_DESTINATION - ); - const passthroughOptions = { - destination: '../traversal-destination', - }; - const file = new File(bucket, 'first.txt'); - await assert.rejects( - transferManager.downloadManyFiles([file], {passthroughOptions}), - expectedErr - ); - }); - - it('should throw an error for path traversal in file name', async () => { - const expectedErr = new RequestError( - FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE - ); - const file = new File(bucket, '../traversal-filename.txt'); - await assert.rejects( - transferManager.downloadManyFiles([file]), - expectedErr - ); - }); - - it('should throw an error for path traversal using prefix', async () => { - const expectedErr = new RequestError( - FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE - ); - const file = new File(bucket, 'first.txt'); - await assert.rejects( - transferManager.downloadManyFiles([file], { - prefix: '../traversal-prefix', - }), - expectedErr - ); - }); - it('does not download files that already exist locally when skipIfExists is true', async () => { const firstFile = new File(bucket, 'first.txt'); sandbox.stub(firstFile, 'download').callsFake(options => { @@ -358,16 +301,14 @@ describe('Transfer Manager', () => { await transferManager.downloadManyFiles(files, options); }); - it('sets the destination to CWD when prefix, strip prefix and passthroughOptions.destination are not provided', async () => { + it('does not set the destination when prefix, strip prefix and passthroughOptions.destination are not provided', async () => { const options = {}; const filename = 'first.txt'; - const expectedDestination = path.resolve(process.cwd(), filename); - const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { optionsOrCb(null, Buffer.alloc(0)); } else if (optionsOrCb) { - assert.strictEqual(optionsOrCb.destination, expectedDestination); + assert.strictEqual(optionsOrCb.destination, undefined); } return Promise.resolve([Buffer.alloc(0)]) as Promise; }; @@ -380,16 +321,10 @@ describe('Transfer Manager', () => { it('should recursively create directory and write file contents if destination path is nested', async () => { const prefix = 'text-prefix'; const folder = 'nestedFolder/'; - const filename = 'first.txt'; - const filesOrFolder = [folder, path.join(folder, filename)]; - const dirNameWithPrefix = path.join(prefix, folder); - const normalizedDir = path.resolve(process.cwd(), dirNameWithPrefix); - const expectedDir = normalizedDir + path.sep; - const expectedFilePath = path.resolve( - process.cwd(), - path.join(prefix, folder, filename) - ); - + 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 mkdirSpy = sandbox.spy(fsp, 'mkdir'); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { @@ -400,21 +335,16 @@ describe('Transfer Manager', () => { return Promise.resolve([Buffer.alloc(0)]) as Promise; }; - sandbox.stub(bucket, 'file').callsFake(objectName => { - const file = new File(bucket, objectName); - if (objectName === path.join(folder, filename)) { - file.download = download; - } else { - file.download = () => - Promise.resolve([Buffer.alloc(0)]) as Promise; - } + sandbox.stub(bucket, 'file').callsFake(filename => { + const file = new File(bucket, filename); + file.download = download; return file; }); await transferManager.downloadManyFiles(filesOrFolder, { prefix: prefix, }); assert.strictEqual( - mkdirSpy.calledWith(expectedDir, { + mkdirSpy.calledOnceWith(expectedDir, { recursive: true, }), true