Skip to content

Commit 254b6b2

Browse files
thiyaguk09ddelgrosso1
thiyaguk09
and
ddelgrosso1
authored
Revert "fix: Implement path containment to prevent traversal attacks (#2654)" (#2674)
This reverts commit 08d7abf. Co-authored-by: Denis DelGrosso <[email protected]>
1 parent f572456 commit 254b6b2

File tree

4 files changed

+33
-192
lines changed

4 files changed

+33
-192
lines changed

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

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

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;
59+
it('should download mulitple files', async () => {
6760
const output = execSync(
68-
`node downloadManyFilesWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${expectedSecondFilePath}`
61+
`node downloadManyFilesWithTransferManager.js ${bucketName} ${firstFilePath} ${secondFilePath}`
6962
);
7063
assert.match(
7164
output,
7265
new RegExp(
73-
`gs://${bucketName}/${expectedFirstFilePath} downloaded to ${expectedFirstFilePath}.\ngs://${bucketName}/${expectedSecondFilePath} downloaded to ${expectedSecondFilePath}.`
66+
`gs://${bucketName}/${firstFilePath} downloaded to ${firstFilePath}.\ngs://${bucketName}/${secondFilePath} downloaded to ${secondFilePath}.`
7467
)
7568
);
7669
});
7770

7871
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;
8372
const output = execSync(
84-
`node downloadFileInChunksWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${downloadFilePath} ${chunkSize}`
73+
`node downloadFileInChunksWithTransferManager.js ${bucketName} ${firstFilePath} ${downloadFilePath} ${chunkSize}`
8574
);
8675
assert.match(
8776
output,
8877
new RegExp(
89-
`gs://${bucketName}/${expectedFirstFilePath} downloaded to ${downloadFilePath}.`
78+
`gs://${bucketName}/${firstFilePath} downloaded to ${downloadFilePath}.`
9079
)
9180
);
9281
});

‎src/file.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -545,9 +545,6 @@ 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).",
551548
}
552549

553550
/**

‎src/transfer-manager.ts‎

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

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-
}
500+
passThroughOptionsCopy.destination = options.customDestinationBuilder
501+
? options.customDestinationBuilder(filePath, options)
502+
: filePath.split(path.sep).join(path.posix.sep);
510503
if (options.prefix) {
511504
passThroughOptionsCopy.destination = path.posix.join(
512505
...options.prefix.split(path.sep),
@@ -594,61 +587,27 @@ export class TransferManager {
594587
});
595588
}
596589

597-
const baseDir = this._resolveAndValidateBaseDir(options);
598-
599590
const stripRegexString = options.stripPrefix
600591
? `^${options.stripPrefix}`
601592
: EMPTY_REGEX;
602593
const regex = new RegExp(stripRegexString, 'g');
603594

604-
const createdDirectories = new Set<string>();
605595
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-
642596
const passThroughOptionsCopy = {
643597
...options.passthroughOptions,
644-
destination: finalPath,
645598
[GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY,
646599
};
647600

648-
const destinationDir = finalPath.endsWith(path.sep)
649-
? finalPath
650-
: path.dirname(finalPath);
651-
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+
}
652611
if (
653612
options.skipIfExists &&
654613
existsSync(passThroughOptionsCopy.destination || '')
@@ -659,12 +618,8 @@ export class TransferManager {
659618
promises.push(
660619
limit(async () => {
661620
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-
}
667621
if (destination && destination.endsWith(path.sep)) {
622+
await fsp.mkdir(destination, {recursive: true});
668623
return Promise.resolve([
669624
Buffer.alloc(0),
670625
]) as Promise<DownloadResponse>;
@@ -912,34 +867,4 @@ export class TransferManager {
912867
: yield fullPath;
913868
}
914869
}
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-
}
945870
}

‎test/transfer-manager.ts‎

Lines changed: 14 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ 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';
4544

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

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

242238
const file = new File(bucket, filename);
243239
sandbox.stub(file, 'download').callsFake(options => {
@@ -267,10 +263,8 @@ describe('Transfer Manager', () => {
267263
destination: 'test-destination',
268264
};
269265
const filename = 'first.txt';
270-
const expectedDestination = path.resolve(
271-
process.cwd(),
272-
passthroughOptions.destination,
273-
filename
266+
const expectedDestination = path.normalize(
267+
`${passthroughOptions.destination}/${filename}`
274268
);
275269
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
276270
if (typeof optionsOrCb === 'function') {
@@ -286,57 +280,6 @@ describe('Transfer Manager', () => {
286280
await transferManager.downloadManyFiles([file], {passthroughOptions});
287281
});
288282

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-
340283
it('does not download files that already exist locally when skipIfExists is true', async () => {
341284
const firstFile = new File(bucket, 'first.txt');
342285
sandbox.stub(firstFile, 'download').callsFake(options => {
@@ -358,16 +301,14 @@ describe('Transfer Manager', () => {
358301
await transferManager.downloadManyFiles(files, options);
359302
});
360303

361-
it('sets the destination to CWD when prefix, strip prefix and passthroughOptions.destination are not provided', async () => {
304+
it('does not set the destination when prefix, strip prefix and passthroughOptions.destination are not provided', async () => {
362305
const options = {};
363306
const filename = 'first.txt';
364-
const expectedDestination = path.resolve(process.cwd(), filename);
365-
366307
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
367308
if (typeof optionsOrCb === 'function') {
368309
optionsOrCb(null, Buffer.alloc(0));
369310
} else if (optionsOrCb) {
370-
assert.strictEqual(optionsOrCb.destination, expectedDestination);
311+
assert.strictEqual(optionsOrCb.destination, undefined);
371312
}
372313
return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
373314
};
@@ -380,16 +321,10 @@ describe('Transfer Manager', () => {
380321
it('should recursively create directory and write file contents if destination path is nested', async () => {
381322
const prefix = 'text-prefix';
382323
const folder = 'nestedFolder/';
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-
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);
393328
const mkdirSpy = sandbox.spy(fsp, 'mkdir');
394329
const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => {
395330
if (typeof optionsOrCb === 'function') {
@@ -400,21 +335,16 @@ describe('Transfer Manager', () => {
400335
return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
401336
};
402337

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-
}
338+
sandbox.stub(bucket, 'file').callsFake(filename => {
339+
const file = new File(bucket, filename);
340+
file.download = download;
411341
return file;
412342
});
413343
await transferManager.downloadManyFiles(filesOrFolder, {
414344
prefix: prefix,
415345
});
416346
assert.strictEqual(
417-
mkdirSpy.calledWith(expectedDir, {
347+
mkdirSpy.calledOnceWith(expectedDir, {
418348
recursive: true,
419349
}),
420350
true

0 commit comments

Comments
 (0)