KEMBAR78
fix: Implement path containment to prevent traversal attacks by thiyaguk09 · Pull Request #2654 · googleapis/nodejs-storage · GitHub
Skip to content

Conversation

thiyaguk09
Copy link
Contributor

Description

This PR addresses a potential Arbitrary File Write / Path Traversal vulnerability in TransferManager.downloadManyFiles() by enforcing strict path containment checks on all downloaded object names.

The entire path assembly pipeline has been refactored to prioritize security and path normalization:

  1. Sequential Name Assembly: All path modifications (stripPrefix, prefix) are applied sequentially to the relative object name before security validation.

  2. Absolute Path Rejection: The function now immediately rejects object names (even after modification) that are absolute paths (e.g., /etc/passwd).

  3. Canonical Resolution and Containment: The final relative path is resolved into an absolute path (finalPath = path.resolve(baseDir, name)). The function then verifies that finalPath strictly begins with the absolute baseDir, preventing path traversal sequences like ../../.

  4. Directory Marker Fix: A critical fix was added to explicitly check if the original GCS object name was a directory marker (ends in /). Since path.resolve strips the trailing slash, logic was added to re-add the separator to finalPath for directory markers, ensuring fsp.mkdir is called correctly.

The old, manual, and insecure logic for joining prefix and destination using path.join() was removed as it bypassed these new security checks.

Impact

The primary impact is a significant security uplift for all users utilizing downloadManyFiles().

  • Security: Prevents malicious object names from writing files outside the intended download folder (e.g., preventing overwrites of configuration files or system files).

  • Behavioral Change: If a malicious path traversal attempt is detected, the function will no longer silently fail or attempt an unsafe operation. It will now explicitly throw a SECURITY_PATH_TRAVERSAL_REJECTED error (or SECURITY_ABSOLUTE_PATH_REJECTED).

  • Internal Consistency: The destination passed to the underlying file.download method is now always a canonical, absolute path, improving consistency across all download scenarios.

Testing

Yes, unit tests were heavily modified.

  • All existing tests related to checking the destination path (prefix, stripPrefix, passthroughOptions.destination, and default download) were updated to assert against the new absolute, canonical path returned by the secure logic.

  • New test assertions were added to explicitly verify that attempts to use absolute paths or traversal sequences result in the expected security exceptions.

  • The complex test for nested directories was fully refactored and now passes, confirming that the new directory marker fix correctly triggers the recursive fsp.mkdir call.

Breaking Changes: None technically, as the previous behavior was insecure. However, consumers who rely on supplying relative paths for passthroughOptions.destination will now receive absolute paths in the underlying file.download callback options, which is a desirable side effect of the security fix.

Additional Information

The implementation detail to note is the use of path.resolve for path canonicalization. This forced the manual re-insertion of the trailing path separator (path.sep) for GCS folder markers, as path.resolve strips it, which otherwise would prevent the directory from being created recursively. This refactoring was essential to maintaining directory creation functionality while enforcing path safety.

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 #

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.
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/nodejs-storage API. labels Sep 26, 2025
@thiyaguk09 thiyaguk09 changed the title Fix/download path traversal fix: Implement path containment to prevent traversal attacks Sep 26, 2025
…y 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.
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.
@thiyaguk09 thiyaguk09 marked this pull request as ready for review September 29, 2025 13:54
@thiyaguk09 thiyaguk09 requested review from a team as code owners September 29, 2025 13:54
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.
promises.push(
limit(async () => {
const destination = passThroughOptionsCopy.destination;
if (!createdDirectories.has(destinationDir)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate variable to track which directories have been created?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mkdir call is now performed on every iteration for every file, which could introduce a slight performance overhead due to redundant fs calls, especially if many files are in the same directory. While fsp.mkdir with recursive: true handles existing directories gracefully, you could optimize this by tracking created directories in a Set to avoid repeated calls for the same directory.

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.
@thiyaguk09 thiyaguk09 requested a review from ddelgrosso1 October 3, 2025 10:07
@ddelgrosso1 ddelgrosso1 merged commit 08d7abf into main Oct 6, 2025
19 checks passed
@ddelgrosso1 ddelgrosso1 deleted the fix/download-path-traversal branch October 6, 2025 14:04
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: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants