Feat: Implement robust path validation and structured skip reporting

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}`);
+      }
     }
   }
 

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}`
     );

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

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<void | DownloadResponse[]> {
+  ): Promise<DownloadManyFilesResult | DownloadResponse[]> {
     const limit = pLimit(
       options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT
     );
     const promises: Promise<DownloadResponse>[] = [];
+    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<DownloadResponse>;
+          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,
+    };
   }
 
   /**

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', () => {