Feat: Implement robust path validation and structured skip reporting

samples/downloadManyFilesWithTransferManager.js

@@ -48,11 +48,25 @@ function main(
   const transferManager = new TransferManager(storage.bucket(bucketName));
 
   async function downloadManyFilesWithTransferManager() {
-    // Downloads the files
-    await transferManager.downloadManyFiles([firstFileName, secondFileName]);
+    // Downloads the files. The result is an array of DownloadResponses
+    // augmented with 'skipped' and 'reason' properties.
+    const results = await transferManager.downloadManyFiles([
+      firstFileName,
+      secondFileName,
+    ]);
 
-    for (const fileName of [firstFileName, secondFileName]) {
-      console.log(`gs://${bucketName}/${fileName} downloaded to ${fileName}.`);
+    for (let i = 0; i < results.length; i++) {
+      const result = results[i];
+      // Each result is a DownloadResponse [Buffer]
+      // We check our custom properties to see if it was blocked by validation
+      const fileName = result.fileName || [firstFileName, secondFileName][i];
+      if (result.skipped) {
+        console.warn(`Skipped ${fileName}: ${result.reason}`);
+      } else {
+        console.log(
+          `gs://${bucketName}/${fileName} downloaded to ${fileName}.`
+        );
+      }
     }
   }
 

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,22 @@ export interface CopyCallback {
 
 export type DownloadResponse = [Buffer];
 
+export type DownloadResponseWithStatus = [Buffer] & {
+  skipped?: boolean;
+  reason?: SkipReason;
+  fileName?: string;
+  localPath?: string;
+  message?: string;
+  error?: Error;
+};
+
+export enum SkipReason {
+  PATH_TRAVERSAL = 'PATH_TRAVERSAL',
+  ILLEGAL_CHARACTER = 'ILLEGAL_CHARACTER',
+  ALREADY_EXISTS = 'ALREADY_EXISTS',
+  DOWNLOAD_ERROR = 'DOWNLOAD_ERROR',
+}
+
 export type DownloadCallback = (
   err: RequestError | null,
   contents: Buffer

src/transfer-manager.ts

@@ -18,9 +18,11 @@ import {Bucket, UploadOptions, UploadResponse} from './bucket.js';
 import {
   DownloadOptions,
   DownloadResponse,
+  DownloadResponseWithStatus,
   File,
   FileExceptionMessages,
   RequestError,
+  SkipReason,
 } from './file.js';
 import pLimit from 'p-limit';
 import * as path from 'path';
@@ -571,8 +573,13 @@ export class TransferManager {
       options.concurrencyLimit || DEFAULT_PARALLEL_DOWNLOAD_LIMIT
     );
     const promises: Promise<DownloadResponse>[] = [];
+    const skippedFiles: DownloadResponseWithStatus[] = [];
     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 +605,50 @@ export class TransferManager {
         [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY,
       };
 
+      const normalizedGcsName = file.name
+        .replace(/\\/g, path.sep)
+        .replace(/\//g, path.sep);
+
+      let dest: string;
       if (options.prefix || passThroughOptionsCopy.destination) {
-        passThroughOptionsCopy.destination = path.join(
+        dest = path.join(
           options.prefix || '',
           passThroughOptionsCopy.destination || '',
-          file.name
+          normalizedGcsName
         );
       }
       if (options.stripPrefix) {
-        passThroughOptionsCopy.destination = file.name.replace(regex, '');
+        dest = normalizedGcsName.replace(regex, '');
+      } else {
+        dest = path.join(
+          options.prefix || '',
+          passThroughOptionsCopy.destination || '',
+          normalizedGcsName
+        );
       }
+
+      const resolvedPath = path.resolve(baseDestination, dest);
+      const relativePath = path.relative(baseDestination, resolvedPath);
+      const isOutside = relativePath.split(path.sep).includes('..');
+      const hasIllegalDrive = /^[a-zA-Z]:/.test(file.name);
+
+      if (isOutside || hasIllegalDrive) {
+        let reason: SkipReason = SkipReason.DOWNLOAD_ERROR;
+        if (isOutside) reason = SkipReason.PATH_TRAVERSAL;
+        else if (hasIllegalDrive) reason = SkipReason.ILLEGAL_CHARACTER;
+
+        const skippedResult = [Buffer.alloc(0)] as DownloadResponseWithStatus;
+        skippedResult.skipped = true;
+        skippedResult.reason = reason;
+        skippedResult.fileName = file.name;
+        skippedResult.localPath = resolvedPath;
+        skippedResult.message = `File ${file.name} was skipped due to path validation failure.`;
+
+        skippedFiles.push(skippedResult);
+        continue;
+      }
+      passThroughOptionsCopy.destination = dest;
+
       if (
         options.skipIfExists &&
         existsSync(passThroughOptionsCopy.destination || '')
@@ -617,20 +658,34 @@ 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;
+            }
+            const resp = (await file.download(
+              passThroughOptionsCopy
+            )) as DownloadResponseWithStatus;
+            resp.skipped = false;
+            resp.fileName = file.name;
+            return resp;
+          } catch (err) {
+            const errorResp = [Buffer.alloc(0)] as DownloadResponseWithStatus;
+            errorResp.skipped = true;
+            errorResp.reason = SkipReason.DOWNLOAD_ERROR;
+            errorResp.error = err as Error;
+            return errorResp;
           }
-
-          return file.download(passThroughOptionsCopy);
         })
       );
     }
+    const results = await Promise.all(promises);
 
-    return Promise.all(promises);
+    return [...skippedFiles, ...results];
   }
 
   /**