Skip to content
This repository was archived by the owner on Mar 3, 2026. It is now read-only.

Revert "fix: Implement path containment to prevent traversal attacks" #2674

Merged
ddelgrosso1 merged 1 commit into from
Oct 30, 2025
Merged

Revert "fix: Implement path containment to prevent traversal attacks" #2674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 5 additions & 16 deletions samples/system-test/transfer-manager.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,37 +56,26 @@ describe('transfer manager', () => {
);
});

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

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} ${expectedFirstFilePath} ${downloadFilePath} ${chunkSize}`
`node downloadFileInChunksWithTransferManager.js ${bucketName} ${firstFilePath} ${downloadFilePath} ${chunkSize}`
);
assert.match(
output,
new RegExp(
`gs://${bucketName}/${expectedFirstFilePath} downloaded to ${downloadFilePath}.`
`gs://${bucketName}/${firstFilePath} downloaded to ${downloadFilePath}.`
)
);
});
Expand Down
3 changes: 0 additions & 3 deletions src/file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,9 +545,6 @@ 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).",
}

/**
Expand Down
103 changes: 14 additions & 89 deletions src/transfer-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,16 +497,9 @@ export class TransferManager {
[GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.UPLOAD_MANY,
};

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);
}
passThroughOptionsCopy.destination = options.customDestinationBuilder
? options.customDestinationBuilder(filePath, options)
: filePath.split(path.sep).join(path.posix.sep);
if (options.prefix) {
passThroughOptionsCopy.destination = path.posix.join(
...options.prefix.split(path.sep),
Expand Down Expand Up @@ -594,61 +587,27 @@ 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,
};

const destinationDir = finalPath.endsWith(path.sep)
? finalPath
: path.dirname(finalPath);

if (options.prefix || passThroughOptionsCopy.destination) {
passThroughOptionsCopy.destination = path.join(
options.prefix || '',
passThroughOptionsCopy.destination || '',
file.name
);
}
if (options.stripPrefix) {
passThroughOptionsCopy.destination = file.name.replace(regex, '');
}
if (
options.skipIfExists &&
existsSync(passThroughOptionsCopy.destination || '')
Expand All @@ -659,12 +618,8 @@ 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>;
Expand Down Expand Up @@ -912,34 +867,4 @@ 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;
}
}
98 changes: 14 additions & 84 deletions test/transfer-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ 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';
Expand Down Expand Up @@ -219,10 +218,7 @@ describe('Transfer Manager', () => {
it('sets the destination correctly when provided a prefix', async () => {
const prefix = 'test-prefix';
const filename = 'first.txt';
const expectedDestination = path.resolve(
process.cwd(),
path.join(prefix, filename)
);
const expectedDestination = path.normalize(`${prefix}/${filename}`);

const file = new File(bucket, filename);
sandbox.stub(file, 'download').callsFake(options => {
Expand All @@ -237,7 +233,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 = path.resolve(process.cwd(), 'first.txt');
const expectedDestination = 'first.txt';

const file = new File(bucket, filename);
sandbox.stub(file, 'download').callsFake(options => {
Expand Down Expand Up @@ -267,10 +263,8 @@ describe('Transfer Manager', () => {
destination: 'test-destination',
};
const filename = 'first.txt';
const expectedDestination = path.resolve(
process.cwd(),
passthroughOptions.destination,
filename
const expectedDestination = path.normalize(
`${passthroughOptions.destination}/${filename}`
);
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
if (typeof optionsOrCb === 'function') {
Expand All @@ -286,57 +280,6 @@ 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 => {
Expand All @@ -358,16 +301,14 @@ describe('Transfer Manager', () => {
await transferManager.downloadManyFiles(files, options);
});

it('sets the destination to CWD when prefix, strip prefix and passthroughOptions.destination are not provided', async () => {
it('does not set the destination 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, expectedDestination);
assert.strictEqual(optionsOrCb.destination, undefined);
}
return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
};
Expand All @@ -380,16 +321,10 @@ 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 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 file = 'first.txt';
const filesOrFolder = [folder, path.join(folder, file)];
const expectedFilePath = path.join(prefix, folder, file);
const expectedDir = path.join(prefix, folder);
const mkdirSpy = sandbox.spy(fsp, 'mkdir');
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
if (typeof optionsOrCb === 'function') {
Expand All @@ -400,21 +335,16 @@ describe('Transfer Manager', () => {
return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
};

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>;
}
sandbox.stub(bucket, 'file').callsFake(filename => {
const file = new File(bucket, filename);
file.download = download;
return file;
});
await transferManager.downloadManyFiles(filesOrFolder, {
prefix: prefix,
});
assert.strictEqual(
mkdirSpy.calledWith(expectedDir, {
mkdirSpy.calledOnceWith(expectedDir, {
recursive: true,
}),
true
Expand Down
⚡