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

Commit 08d7abf

Browse files
thiyaguk09thiyaguk09
authored
fix: Implement path containment to prevent traversal attacks (#2654)
* 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' & '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.
1 parent b38b5d2 commit 08d7abf

File tree

4 files changed

+192
-33
lines changed

4 files changed

+192
-33
lines changed

‎samples/system-test/transfer-manager.test.js‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,26 +56,37 @@ describe('transfer manager', () => {
5656
);
5757
});
5858

59-
it('should download mulitple files', async () => {
59+
it('should download multiple files', async () => {
60+
// Remove absolute path marker to prepare for joining/validation.
61+
const expectedFirstFilePath = firstFilePath.startsWith('/')
62+
? firstFilePath.slice(1)
63+
: firstFilePath;
64+
const expectedSecondFilePath = secondFilePath.startsWith('/')
65+
? secondFilePath.slice(1)
66+
: secondFilePath;
6067
const output = execSync(
61-
`node downloadManyFilesWithTransferManager.js ${bucketName} ${firstFilePath} ${secondFilePath}`
68+
`node downloadManyFilesWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${expectedSecondFilePath}`
6269
);
6370
assert.match(
6471
output,
6572
new RegExp(
66-
`gs://${bucketName}/${firstFilePath} downloaded to ${firstFilePath}.\ngs://${bucketName}/${secondFilePath} downloaded to ${secondFilePath}.`
73+
`gs://${bucketName}/${expectedFirstFilePath} downloaded to ${expectedFirstFilePath}.\ngs://${bucketName}/${expectedSecondFilePath} downloaded to ${expectedSecondFilePath}.`
6774
)
6875
);
6976
});
7077

7178
it('should download a file utilizing chunked download', async () => {
79+
// Remove absolute path marker to prepare for joining/validation.
80+
const expectedFirstFilePath = firstFilePath.startsWith('/')
81+
? firstFilePath.slice(1)
82+
: firstFilePath;
7283
const output = execSync(
73-
`node downloadFileInChunksWithTransferManager.js ${bucketName} ${firstFilePath} ${downloadFilePath} ${chunkSize}`
84+
`node downloadFileInChunksWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${downloadFilePath} ${chunkSize}`
7485
);
7586
assert.match(
7687
output,
7788
new RegExp(
78-
`gs://${bucketName}/${firstFilePath} downloaded to ${downloadFilePath}.`
89+
`gs://${bucketName}/${expectedFirstFilePath} downloaded to ${downloadFilePath}.`
7990
)
8091
);
8192
});

‎src/file.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,9 @@ export enum FileExceptionMessages {
545545
To be sure the content is the same, you should try uploading the file again.`,
546546
MD5_RESUMED_UPLOAD = 'MD5 cannot be used with a continued resumable upload as MD5 cannot be extended from an existing value',
547547
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`.',
548+
ABSOLUTE_FILE_NAME = 'Object name is an absolute path. Security block to prevent arbitrary file writes.',
549+
TRAVERSAL_OUTSIDE_BASE = 'Path traversal detected. Security block to prevent writing outside the base directory.',
550+
TRAVERSAL_OUTSIDE_BASE_DESTINATION = "The provided destination path is unsafe and attempts to traverse outside the application's base directory (current working directory).",
548551
}
549552

550553
/**

‎src/transfer-manager.ts‎

Lines changed: 89 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -497,9 +497,16 @@ export class TransferManager {
497497
[GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.UPLOAD_MANY,
498498
};
499499

500-
passThroughOptionsCopy.destination = options.customDestinationBuilder
501-
? options.customDestinationBuilder(filePath, options)
502-
: filePath.split(path.sep).join(path.posix.sep);
500+
if (options.customDestinationBuilder) {
501+
passThroughOptionsCopy.destination = options.customDestinationBuilder(
502+
filePath,
503+
options
504+
);
505+
} else {
506+
let segments = filePath.split(path.sep);
507+
segments = segments.filter(s => s !== '');
508+
passThroughOptionsCopy.destination = path.posix.join(...segments);
509+
}
503510
if (options.prefix) {
504511
passThroughOptionsCopy.destination = path.posix.join(
505512
...options.prefix.split(path.sep),
@@ -587,27 +594,61 @@ export class TransferManager {
587594
});
588595
}
589596

597+
const baseDir = this._resolveAndValidateBaseDir(options);
598+
590599
const stripRegexString = options.stripPrefix
591600
? `^${options.stripPrefix}`
592601
: EMPTY_REGEX;
593602
const regex = new RegExp(stripRegexString, 'g');
594603

604+
const createdDirectories = new Set<string>();
595605
for (const file of files) {
606+
let name = file.name;
607+
608+
// Apply stripPrefix first if requested
609+
if (options.stripPrefix) {
610+
name = name.replace(regex, '');
611+
}
612+
613+
// This ensures the full intended relative path is validated.
614+
if (options.prefix) {
615+
name = path.join(options.prefix, name);
616+
}
617+
618+
// Reject absolute paths and traversal sequences
619+
if (path.isAbsolute(name)) {
620+
const absolutePathError = new RequestError(
621+
FileExceptionMessages.ABSOLUTE_FILE_NAME
622+
);
623+
throw absolutePathError;
624+
}
625+
626+
// Resolve the final path and perform the containment check
627+
let finalPath = path.resolve(baseDir, name);
628+
const normalizedBaseDir = baseDir.endsWith(path.sep)
629+
? baseDir
630+
: baseDir + path.sep;
631+
if (finalPath !== baseDir && !finalPath.startsWith(normalizedBaseDir)) {
632+
const traversalError = new RequestError(
633+
FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE
634+
);
635+
throw traversalError;
636+
}
637+
638+
if (file.name.endsWith('/') && !finalPath.endsWith(path.sep)) {
639+
finalPath = finalPath + path.sep;
640+
}
641+
596642
const passThroughOptionsCopy = {
597643
...options.passthroughOptions,
644+
destination: finalPath,
598645
[GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY,
599646
};
600647

601-
if (options.prefix || passThroughOptionsCopy.destination) {
602-
passThroughOptionsCopy.destination = path.join(
603-
options.prefix || '',
604-
passThroughOptionsCopy.destination || '',
605-
file.name
606-
);
607-
}
608-
if (options.stripPrefix) {
609-
passThroughOptionsCopy.destination = file.name.replace(regex, '');
610-
}
648+
const destinationDir = finalPath.endsWith(path.sep)
649+
? finalPath
650+
: path.dirname(finalPath);
651+
611652
if (
612653
options.skipIfExists &&
613654
existsSync(passThroughOptionsCopy.destination || '')
@@ -618,8 +659,12 @@ export class TransferManager {
618659
promises.push(
619660
limit(async () => {
620661
const destination = passThroughOptionsCopy.destination;
662+
if (!createdDirectories.has(destinationDir)) {
663+
// If not, create it and add it to the set for tracking
664+
await fsp.mkdir(destinationDir, {recursive: true});
665+
createdDirectories.add(destinationDir);
666+
}
621667
if (destination && destination.endsWith(path.sep)) {
622-
await fsp.mkdir(destination, {recursive: true});
623668
return Promise.resolve([
624669
Buffer.alloc(0),
625670
]) as Promise<DownloadResponse>;
@@ -867,4 +912,34 @@ export class TransferManager {
867912
: yield fullPath;
868913
}
869914
}
915+
916+
/**
917+
* Resolves the absolute base directory for downloads and validates it against
918+
* the current working directory (CWD) to prevent path traversal outside the base destination.
919+
* @param options The download options, potentially containing passthroughOptions.destination.
920+
* @returns The absolute, validated base directory path (baseDir).
921+
*/
922+
private _resolveAndValidateBaseDir(
923+
options: DownloadManyFilesOptions
924+
): string {
925+
const cwd = process.cwd();
926+
927+
// Resolve baseDir, defaulting to CWD if no destination is provided
928+
const baseDir = path.resolve(
929+
options.passthroughOptions?.destination ?? cwd
930+
);
931+
932+
// Check for path traversal: baseDir must be equal to or contained within cwd.
933+
const relativeBaseDir = path.relative(cwd, baseDir);
934+
935+
// The condition checks for traversal ('..') or cross-drive traversal (absolute path on Windows)
936+
if (relativeBaseDir.startsWith('..') || path.isAbsolute(relativeBaseDir)) {
937+
const traversalError = new RequestError(
938+
FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE_DESTINATION
939+
);
940+
throw traversalError;
941+
}
942+
943+
return baseDir;
944+
}
870945
}

‎test/transfer-manager.ts‎

Lines changed: 84 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import fs from 'fs';
4141
import {promises as fsp, Stats} from 'fs';
4242

4343
import * as sinon from 'sinon';
44+
import {FileExceptionMessages, RequestError} from '../src/file.js';
4445

4546
describe('Transfer Manager', () => {
4647
const BUCKET_NAME = 'test-bucket';
@@ -218,7 +219,10 @@ describe('Transfer Manager', () => {
218219
it('sets the destination correctly when provided a prefix', async () => {
219220
const prefix = 'test-prefix';
220221
const filename = 'first.txt';
221-
const expectedDestination = path.normalize(`${prefix}/${filename}`);
222+
const expectedDestination = path.resolve(
223+
process.cwd(),
224+
path.join(prefix, filename)
225+
);
222226

223227
const file = new File(bucket, filename);
224228
sandbox.stub(file, 'download').callsFake(options => {
@@ -233,7 +237,7 @@ describe('Transfer Manager', () => {
233237
it('sets the destination correctly when provided a strip prefix', async () => {
234238
const stripPrefix = 'should-be-removed/';
235239
const filename = 'should-be-removed/first.txt';
236-
const expectedDestination = 'first.txt';
240+
const expectedDestination = path.resolve(process.cwd(), 'first.txt');
237241

238242
const file = new File(bucket, filename);
239243
sandbox.stub(file, 'download').callsFake(options => {
@@ -263,8 +267,10 @@ describe('Transfer Manager', () => {
263267
destination: 'test-destination',
264268
};
265269
const filename = 'first.txt';
266-
const expectedDestination = path.normalize(
267-
`${passthroughOptions.destination}/${filename}`
270+
const expectedDestination = path.resolve(
271+
process.cwd(),
272+
passthroughOptions.destination,
273+
filename
268274
);
269275
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
270276
if (typeof optionsOrCb === 'function') {
@@ -280,6 +286,57 @@ describe('Transfer Manager', () => {
280286
await transferManager.downloadManyFiles([file], {passthroughOptions});
281287
});
282288

289+
it('should throws an error for absolute file names', async () => {
290+
const expectedErr = new RequestError(
291+
FileExceptionMessages.ABSOLUTE_FILE_NAME
292+
);
293+
const maliciousFilename = '/etc/passwd';
294+
const file = new File(bucket, maliciousFilename);
295+
296+
await assert.rejects(
297+
transferManager.downloadManyFiles([file]),
298+
expectedErr
299+
);
300+
});
301+
302+
it('should throw an error for path traversal in destination', async () => {
303+
const expectedErr = new RequestError(
304+
FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE_DESTINATION
305+
);
306+
const passthroughOptions = {
307+
destination: '../traversal-destination',
308+
};
309+
const file = new File(bucket, 'first.txt');
310+
await assert.rejects(
311+
transferManager.downloadManyFiles([file], {passthroughOptions}),
312+
expectedErr
313+
);
314+
});
315+
316+
it('should throw an error for path traversal in file name', async () => {
317+
const expectedErr = new RequestError(
318+
FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE
319+
);
320+
const file = new File(bucket, '../traversal-filename.txt');
321+
await assert.rejects(
322+
transferManager.downloadManyFiles([file]),
323+
expectedErr
324+
);
325+
});
326+
327+
it('should throw an error for path traversal using prefix', async () => {
328+
const expectedErr = new RequestError(
329+
FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE
330+
);
331+
const file = new File(bucket, 'first.txt');
332+
await assert.rejects(
333+
transferManager.downloadManyFiles([file], {
334+
prefix: '../traversal-prefix',
335+
}),
336+
expectedErr
337+
);
338+
});
339+
283340
it('does not download files that already exist locally when skipIfExists is true', async () => {
284341
const firstFile = new File(bucket, 'first.txt');
285342
sandbox.stub(firstFile, 'download').callsFake(options => {
@@ -301,14 +358,16 @@ describe('Transfer Manager', () => {
301358
await transferManager.downloadManyFiles(files, options);
302359
});
303360

304-
it('does not set the destination when prefix, strip prefix and passthroughOptions.destination are not provided', async () => {
361+
it('sets the destination to CWD when prefix, strip prefix and passthroughOptions.destination are not provided', async () => {
305362
const options = {};
306363
const filename = 'first.txt';
364+
const expectedDestination = path.resolve(process.cwd(), filename);
365+
307366
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
308367
if (typeof optionsOrCb === 'function') {
309368
optionsOrCb(null, Buffer.alloc(0));
310369
} else if (optionsOrCb) {
311-
assert.strictEqual(optionsOrCb.destination, undefined);
370+
assert.strictEqual(optionsOrCb.destination, expectedDestination);
312371
}
313372
return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
314373
};
@@ -321,10 +380,16 @@ describe('Transfer Manager', () => {
321380
it('should recursively create directory and write file contents if destination path is nested', async () => {
322381
const prefix = 'text-prefix';
323382
const folder = 'nestedFolder/';
324-
const file = 'first.txt';
325-
const filesOrFolder = [folder, path.join(folder, file)];
326-
const expectedFilePath = path.join(prefix, folder, file);
327-
const expectedDir = path.join(prefix, folder);
383+
const filename = 'first.txt';
384+
const filesOrFolder = [folder, path.join(folder, filename)];
385+
const dirNameWithPrefix = path.join(prefix, folder);
386+
const normalizedDir = path.resolve(process.cwd(), dirNameWithPrefix);
387+
const expectedDir = normalizedDir + path.sep;
388+
const expectedFilePath = path.resolve(
389+
process.cwd(),
390+
path.join(prefix, folder, filename)
391+
);
392+
328393
const mkdirSpy = sandbox.spy(fsp, 'mkdir');
329394
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
330395
if (typeof optionsOrCb === 'function') {
@@ -335,16 +400,21 @@ describe('Transfer Manager', () => {
335400
return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
336401
};
337402

338-
sandbox.stub(bucket, 'file').callsFake(filename => {
339-
const file = new File(bucket, filename);
340-
file.download = download;
403+
sandbox.stub(bucket, 'file').callsFake(objectName => {
404+
const file = new File(bucket, objectName);
405+
if (objectName === path.join(folder, filename)) {
406+
file.download = download;
407+
} else {
408+
file.download = () =>
409+
Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
410+
}
341411
return file;
342412
});
343413
await transferManager.downloadManyFiles(filesOrFolder, {
344414
prefix: prefix,
345415
});
346416
assert.strictEqual(
347-
mkdirSpy.calledOnceWith(expectedDir, {
417+
mkdirSpy.calledWith(expectedDir, {
348418
recursive: true,
349419
}),
350420
true

0 commit comments

Comments
 (0)