fix: Implement path containment to prevent traversal attacks

samples/system-test/transfer-manager.test.js

@@ -56,26 +56,37 @@ describe('transfer manager', () => {
     );
   });
 
-  it('should download mulitple files', async () => {
+  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;
     const output = execSync(
-      `node downloadManyFilesWithTransferManager.js ${bucketName} ${firstFilePath} ${secondFilePath}`
+      `node downloadManyFilesWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${expectedSecondFilePath}`
     );
     assert.match(
       output,
       new RegExp(
-        `gs://${bucketName}/${firstFilePath} downloaded to ${firstFilePath}.\ngs://${bucketName}/${secondFilePath} downloaded to ${secondFilePath}.`
+        `gs://${bucketName}/${expectedFirstFilePath} downloaded to ${expectedFirstFilePath}.\ngs://${bucketName}/${expectedSecondFilePath} downloaded to ${expectedSecondFilePath}.`
       )
     );
   });
 
   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} ${firstFilePath} ${downloadFilePath} ${chunkSize}`
+      `node downloadFileInChunksWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${downloadFilePath} ${chunkSize}`
     );
     assert.match(
       output,
       new RegExp(
-        `gs://${bucketName}/${firstFilePath} downloaded to ${downloadFilePath}.`
+        `gs://${bucketName}/${expectedFirstFilePath} downloaded to ${downloadFilePath}.`
       )
     );
   });

src/file.ts

@@ -545,6 +545,9 @@ 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).",
 }
 
 /**

src/transfer-manager.ts

@@ -497,9 +497,16 @@ export class TransferManager {
         [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.UPLOAD_MANY,
       };
 
-      passThroughOptionsCopy.destination = options.customDestinationBuilder
-        ? options.customDestinationBuilder(filePath, options)
-        : filePath.split(path.sep).join(path.posix.sep);
+      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);
+      }
       if (options.prefix) {
         passThroughOptionsCopy.destination = path.posix.join(
           ...options.prefix.split(path.sep),
@@ -587,27 +594,61 @@ 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<string>();
     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,
       };
 
-      if (options.prefix || passThroughOptionsCopy.destination) {
-        passThroughOptionsCopy.destination = path.join(
-          options.prefix || '',
-          passThroughOptionsCopy.destination || '',
-          file.name
-        );
-      }
-      if (options.stripPrefix) {
-        passThroughOptionsCopy.destination = file.name.replace(regex, '');
-      }
+      const destinationDir = finalPath.endsWith(path.sep)
+        ? finalPath
+        : path.dirname(finalPath);
+
       if (
         options.skipIfExists &&
         existsSync(passThroughOptionsCopy.destination || '')
@@ -618,8 +659,12 @@ 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<DownloadResponse>;
@@ -867,4 +912,34 @@ 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;
+  }
 }

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 {FileExceptionMessages, RequestError} from '../src/file.js';
 
 describe('Transfer Manager', () => {
   const BUCKET_NAME = 'test-bucket';
@@ -218,7 +219,10 @@ describe('Transfer Manager', () => {
     it('sets the destination correctly when provided a prefix', async () => {
       const prefix = 'test-prefix';
       const filename = 'first.txt';
-      const expectedDestination = path.normalize(`${prefix}/${filename}`);
+      const expectedDestination = path.resolve(
+        process.cwd(),
+        path.join(prefix, filename)
+      );
 
       const file = new File(bucket, filename);
       sandbox.stub(file, 'download').callsFake(options => {
@@ -233,7 +237,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 = 'first.txt';
+      const expectedDestination = path.resolve(process.cwd(), 'first.txt');
 
       const file = new File(bucket, filename);
       sandbox.stub(file, 'download').callsFake(options => {
@@ -263,8 +267,10 @@ describe('Transfer Manager', () => {
         destination: 'test-destination',
       };
       const filename = 'first.txt';
-      const expectedDestination = path.normalize(
-        `${passthroughOptions.destination}/${filename}`
+      const expectedDestination = path.resolve(
+        process.cwd(),
+        passthroughOptions.destination,
+        filename
       );
       const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
         if (typeof optionsOrCb === 'function') {
@@ -280,6 +286,57 @@ 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 => {
@@ -301,14 +358,16 @@ describe('Transfer Manager', () => {
       await transferManager.downloadManyFiles(files, options);
     });
 
-    it('does not set the destination when prefix, strip prefix and passthroughOptions.destination are not provided', async () => {
+    it('sets the destination to CWD 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, undefined);
+          assert.strictEqual(optionsOrCb.destination, expectedDestination);
         }
         return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
       };
@@ -321,10 +380,16 @@ 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 file = 'first.txt';
-      const filesOrFolder = [folder, path.join(folder, file)];
-      const expectedFilePath = path.join(prefix, folder, file);
-      const expectedDir = path.join(prefix, folder);
+      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 mkdirSpy = sandbox.spy(fsp, 'mkdir');
       const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
         if (typeof optionsOrCb === 'function') {
@@ -335,16 +400,21 @@ describe('Transfer Manager', () => {
         return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
       };
 
-      sandbox.stub(bucket, 'file').callsFake(filename => {
-        const file = new File(bucket, filename);
-        file.download = download;
+      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<DownloadResponse>;
+        }
         return file;
       });
       await transferManager.downloadManyFiles(filesOrFolder, {
         prefix: prefix,
       });
       assert.strictEqual(
-        mkdirSpy.calledOnceWith(expectedDir, {
+        mkdirSpy.calledWith(expectedDir, {
           recursive: true,
         }),
         true