Skip to content

Comments

Feat: Implement robust path validation and structured skip reporting#2707

Open
thiyaguk09 wants to merge 8 commits intomainfrom
fix/download-directory-path-traversal
Open

Feat: Implement robust path validation and structured skip reporting#2707
thiyaguk09 wants to merge 8 commits intomainfrom
fix/download-directory-path-traversal

Conversation

@thiyaguk09
Copy link
Contributor

@thiyaguk09 thiyaguk09 commented Jan 28, 2026

Description

This PR implements a robust security layer for the downloadManyFiles method in the TransferManager. It addresses potential path traversal vulnerabilities and introduces a structured result reporting system.

Key Changes:

  • Structured Reporting: Changed the return type from Promise<DownloadResponse[]> to Promise<DownloadManyFilesResult>. This new object includes both successful responses and a skippedFiles array containing detailed metadata (reason, message, local path).

  • Strict Security Validation: Implemented a "Strict Blocking" model for file paths. It prevents:

    • Path Traversal: Blocking dot-segments (../) that resolve outside the intended target.
    • Absolute Escape: Blocking object names that start with /, \, or Windows drive letters to prevent root-level directory escapes.
    • Filename Poisoning: Filtering illegal characters such as Null bytes (\0), control characters, and Windows volume separators (:).
  • Parity Guarantee: Updated the internal batch loop to ensure every input file is accounted for. If a file is not downloaded due to security or a network error, it is explicitly added to the skippedFiles list.

Impact

This is a BREAKING CHANGE.
Existing implementations that expect an array return from downloadManyFiles will need to be updated to destructure the new result object.

Testing

Unit Tests:

  • Added a "Parity Check" test to ensure inputCount === responses.length + skippedFiles.length.
  • Verified behavior across simulated Unix and Windows path structures.

Integration Tests:

  • Updated existing storage samples to handle the new return type and log skipped files.
  • Verified that successful downloads still function as expected with the new parallel limit logic.

Additional Information

Migration Guide for Users

Before:

const results = await transferManager.downloadManyFiles(files);
console.log(`${results.length} files downloaded.`);

After:

const { responses, skippedFiles } = await transferManager.downloadManyFiles(files);
console.log(`${responses.length} files downloaded.`);

if (skippedFiles.length > 0) {
 console.warn(`${skippedFiles.length} files were skipped for security or errors.`);
}

Checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease
  • Appropriate docs were updated
  • Appropriate comments were added, particularly in complex areas or places that require background
  • No new warnings or issues will be generated from this change

Fixes #2660

BREAKING CHANGE: downloadManyFiles now returns a DownloadManyFilesResult
object
instead of an array of DownloadResponse.

- Implements strict blocking for absolute paths (Unix and Windows
styles).
- Prevents path traversal via dot-segments (../) using path.relative
validation.
- Blocks illegal characters and poisoned paths (e.g., Windows volume
colons).
- Updates internal logic to resolve paths against a safe base directory
(CWD or prefix).
BREAKING CHANGE: downloadManyFiles now returns a DownloadManyFilesResult
object
instead of an array of DownloadResponse.

- Implements strict blocking for absolute paths (Unix and Windows
styles) and dot-segment traversal.
- Adds DownloadManyFilesResult interface with SkipReason enums for
programmatic handling of skipped files.
- Ensures input-to-output parity where every file is accounted for in
either 'responses' or 'skippedFiles'.
- Robustly handles 'unknown' catch variables by narrowing to Error
instances.
- Optimizes directory creation logic within the parallel download loop.
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Jan 28, 2026
@thiyaguk09 thiyaguk09 marked this pull request as ready for review January 28, 2026 14:22
@thiyaguk09 thiyaguk09 requested review from a team as code owners January 28, 2026 14:22
@thiyaguk09 thiyaguk09 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 29, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 29, 2026
@thiyaguk09 thiyaguk09 force-pushed the fix/download-directory-path-traversal branch from 4d80a50 to 1536b31 Compare January 29, 2026 09:28
Added protection against path traversal attacks.

Implemented validation for illegal characters (e.g., Windows drive
colons).

Coerced absolute-style GCS keys to remain within the target sandbox.

Added comprehensive test suite covering 11 path scenarios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the googleapis/nodejs-storage API. size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

downloadManyFiles can't write to tempdir outside of cwd

3 participants