-
Notifications
You must be signed in to change notification settings - Fork 390
fix: Implement path containment to prevent traversal attacks #2654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
…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.
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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Description
Impact
Testing
Additional Information
Checklist
Fixes #