Skip to content

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

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

Feat: Implement robust path validation and structured skip reporting#2707
thiyaguk09 wants to merge 5 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
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

2 participants