feat(storage): Implement robust path validation and structured skip reporting

handwritten/storage/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,

handwritten/storage/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,43 @@ export class TransferManager {
         [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY,
       };
 
-      if (options.prefix || passThroughOptionsCopy.destination) {
-        passThroughOptionsCopy.destination = path.join(
+      const normalizedGcsName = file.name
+        .replace(/\\/g, path.sep)
+        .replace(/\//g, path.sep);
+
+      let dest: string;
+      if (options.stripPrefix) {
+        dest = normalizedGcsName.replace(regex, '');
+      } else {
+        dest = path.join(
           options.prefix || '',
           passThroughOptionsCopy.destination || '',
-          file.name,
+          normalizedGcsName
         );
       }
-      if (options.stripPrefix) {
-        passThroughOptionsCopy.destination = file.name.replace(regex, '');
+
+      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) {
+        const reason = isOutside
+          ? SkipReason.PATH_TRAVERSAL
+          : 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 +651,33 @@ 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);
-        }),
+        })
       );
     }
-
-    return Promise.all(promises);
+    const results = await Promise.all(promises);
+    return [...skippedFiles, ...results];
   }
 
   /**

handwritten/storage/test/transfer-manager.ts

@@ -32,15 +32,16 @@ import {
   DownloadManyFilesOptions,
 } from '../src/index.js';
 import assert from 'assert';
+import {describe, it, beforeEach, before, afterEach, after} from 'mocha';
 import * as path from 'path';
 import {GaxiosOptions, GaxiosResponse} from 'gaxios';
 import {GCCL_GCS_CMD_KEY} from '../src/nodejs-common/util.js';
 import {AuthClient, GoogleAuth} from 'google-auth-library';
 import {tmpdir} from 'os';
 import fs from 'fs';
 import {promises as fsp, Stats} from 'fs';
-
 import * as sinon from 'sinon';
+import {DownloadResponseWithStatus, SkipReason} from '../src/file.js';
 
 describe('Transfer Manager', () => {
   const BUCKET_NAME = 'test-bucket';
@@ -350,6 +351,201 @@ describe('Transfer Manager', () => {
         true
       );
     });
+
+    it('skips files that attempt path traversal via dot-segments (../) and returns them in skippedFiles', async () => {
+      const prefix = 'download-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}
+      )) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(maliciousDownloadStub.called, false);
+      assert.strictEqual(downloadStub.calledOnce, true);
+
+      const skipped = result.find(r => r.fileName === maliciousFilename);
+      assert.ok(skipped);
+      assert.strictEqual(skipped!.skipped, true);
+      assert.strictEqual(skipped!.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,
+      ])) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(downloadStub.called, false);
+      assert.strictEqual(result[0].skipped, true);
+      assert.strictEqual(result[0].reason, SkipReason.PATH_TRAVERSAL);
+    });
+
+    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,
+      })) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(downloadStub.called, true);
+      const options = downloadStub.firstCall.args[0] as DownloadOptions;
+      assert.strictEqual(options.destination, expectedDestination);
+
+      assert.strictEqual(result.length, 1);
+      assert.strictEqual(result[0].skipped, false);
+    });
+
+    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)]);
+
+      const result = (await transferManager.downloadManyFiles([file], {
+        prefix,
+      })) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(downloadStub.calledOnce, true);
+      const options = downloadStub.firstCall.args[0] as DownloadOptions;
+      assert.strictEqual(options.destination, expectedDestination);
+      assert.strictEqual(result[0].skipped, false);
+    });
+
+    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
+      )) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(downloadStub.called, false);
+      assert.strictEqual(result[0].skipped, true);
+      assert.strictEqual(result[0].reason, SkipReason.PATH_TRAVERSAL);
+    });
+
+    it('should skip 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,
+      })) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(result.length, 1);
+
+      const response = result[0];
+      assert.strictEqual(response.skipped, true);
+      assert.strictEqual(response.reason, SkipReason.ILLEGAL_CHARACTER);
+      assert.strictEqual(response.fileName, 'C:\\system\\win32');
+    });
+
+    it('should account for every input file (Parity Check)', async () => {
+      const prefix = '/local/target';
+      const fileNames = [
+        'data/file.txt', // Normal (Download)
+        'data/../sibling.txt', // Internal Traversal (Download)
+        '../escape.txt', // External Traversal (Skip - Path Traversal '..')
+        '/etc/passwd', // Leading Slash (Download)
+        '/local/usr/a.txt', // Path matches prefix (Download)
+        'dir/./file.txt', // Dot segment (Download)
+        'windows\\file.txt', // Windows separator (Download)
+        'data\\..\\sibling.txt', // Windows traversal (Download)
+        '..\\escape.txt', // Windows escape (Skip - Path Traversal '..')
+        'C:\\system\\win32', // Windows Drive (Skip - Illegal Char ':')
+        'C:\\local\\target\\a.txt', // Windows Absolute (Skip - Illegal Char ':')
+        '..temp.txt', // Leading dots in filename (Download - Not a traversal)
+        'test-2026:01:01.txt', // GCS Timestamps (Download - Colon is middle, not drive)
+      ];
+
+      const files = fileNames.map(name => bucket.file(name));
+
+      sandbox.stub(File.prototype, 'download').resolves([Buffer.alloc(0)]);
+      sandbox.stub(fsp, 'mkdir').resolves();
+
+      const result = (await transferManager.downloadManyFiles(files, {
+        prefix,
+      })) as DownloadResponseWithStatus[];
+
+      assert.strictEqual(
+        result.length,
+        fileNames.length,
+        `Parity Failure: Processed ${result.length} files but input had ${fileNames.length}`
+      );
+
+      const downloads = result.filter(r => !r.skipped);
+      const skips = result.filter(r => r.skipped);
+
+      const expectedDownloads = 9;
+      const expectedSkips = 4;
+
+      assert.strictEqual(
+        downloads.length,
+        expectedDownloads,
+        `Expected ${expectedDownloads} downloads but got ${downloads.length}`
+      );
+
+      assert.strictEqual(
+        skips.length,
+        expectedSkips,
+        `Expected ${expectedSkips} skips but got ${skips.length}`
+      );
+
+      const traversalSkips = skips.filter(
+        f => f.reason === SkipReason.PATH_TRAVERSAL
+      );
+      assert.strictEqual(traversalSkips.length, 2);
+
+      const illegalCharSkips = skips.filter(
+        f => f.reason === SkipReason.ILLEGAL_CHARACTER
+      );
+      assert.strictEqual(illegalCharSkips.length, 2);
+    });
   });
 
   describe('downloadFileInChunks', () => {