fix: Implement path containment to prevent traversal attacks (#2654)

thiyaguk09 · web-flow · commit 08d7abf32dd3 · 2025-10-06T10:04:28.000-04:00
* fix: Implement path containment to prevent traversal attacks

This patch introduces strict path validation in TransferManager.downloadManyFiles to mitigate Arbitrary File Write and Path Traversal vulnerabilities. The fix includes two layers of defense: 1. Rejects Absolute Paths: Immediately throws an error if the object name is an absolute path (e.g., /etc/passwd). 2. Containment Check: Uses path.resolve to normalize the destination path and verify it remains strictly within the intended baseDir, preventing traversal using ../ sequences. SECURITY NOTE: This changes behavior by actively rejecting files with malicious path segments that were previously susceptible to writing outside the target directory.

* fix: Use path.relative for robust path traversal check

* fix: Enforce GCS standard '/' for directory marker detection

* fix: Secure destination path against traversal

* add error message

* fix: Correct download destination logic and ensure recursive directory creation

This commit resolves several critical issues in the `downloadManyFiles` logic related to path handling, destination assignment, and concurrent directory creation, enabling proper execution of bulk downloads and passing relevant tests.

* fix: Optimize fsp.mkdir calls using a Set in downloadManyFiles

Avoids redundant file system calls (fsp.mkdir) when downloading multiple files within the same directory. The  call, while idempotent, was being performed for every file download, leading to unnecessary I/O overhead. This commit introduces a  to track directories that have already been created within a single  call, ensuring that  is executed only once per unique destination directory path.

* refactor: Extract base directory initialization/validation

Moves the logic for resolving and validating the base download directory
(`baseDir`, including initial path traversal checks) out of
`downloadManyFiles` and into the private helper
`_resolveAndValidateBaseDir`.

This change cleans up the primary download execution path, making the
file-by-file iteration loop more focused and readable.

* fix

* refactor: Remove explicit .code assignment from RequestError

Removes the 'SECURITY_ABSOLUTE_PATH_REJECTED' &amp;
'SECURITY_PATH_TRAVERSAL_REJECTED' code assignment from the thrown
RequestError. The corresponding test assertion is updated to check the
error message and type instead of the removed .code property.
diff --git a/samples/system-test/transfer-manager.test.js b/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}.`
       )
     );
   });
diff --git a/src/file.ts b/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).",
 }
 
 /**
diff --git a/src/transfer-manager.ts b/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;
+  }
 }
diff --git 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 {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
