fix(transfer_manager): Prevent path traversal in download_many_to_path#1768
fix(transfer_manager): Prevent path traversal in download_many_to_path#1768chandra-siri wants to merge 13 commits intomainfrom
download_many_to_path#1768Conversation
user provided destination_dir
product-auto-label
bot
added
size: m
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a security issue in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a Path object and a _resolve_path function to resolve the destination path for downloaded blobs in the download_many_to_path function, preventing path traversal vulnerabilities. The tests were updated to reflect the change in destination path. Review feedback indicates that the implementation of _resolve_path is incorrect for relative paths and will raise a ValueError when blob_path is a relative path. Also, the pathlib.Path.is_relative_to() method was introduced in Python 3.9, and since this library supports Python 3.7+, using this method will cause an AttributeError on Python versions 3.7 and 3.8. The traceback module is imported but never used in the file. The TODO comment should be replaced with a concrete example before merging. The docstring for destination_directory contains a TODO comment, a reference to the old os.path.join implementation, and a redundant parenthetical note. The expression Path(destination_directory).resolve() is evaluated on every iteration of the loop.
product-auto-label
bot
added
size: l
chandra-siri
changed the title
chandra-siri
changed the title
download_many_to_path
| a `blob_name` "/etc/passwd" will be downloaded into | ||
| "destination_directory/etc/passwd" instead of "/etc/passwd" | ||
| Similarly, | ||
| "/tmp/my_fav_blob" downloads to "destination_directory/tmp/my_fav_blob" |
There was a problem hiding this comment.
I think this is a redundant example.
There was a problem hiding this comment.
hmm.. I gave one more example to be clear.
There was a problem hiding this comment.
I'll remove it if you insist :)
| "blobname", | ||
| [ | ||
| "../../local/target", | ||
| "../mypath", |
There was a problem hiding this comment.
@krishnamd-jkp edge case we discussed yesterday
There was a problem hiding this comment.
Download directory should be /local/target but I think you are mentioning it as mypath/ below
There was a problem hiding this comment.
it works for mypath/ as well.
2 participants
fix(transfer_manager): Prevent path traversal in download_many_to_path
This PR addresses a security vulnerability where
download_many_to_pathcould be exploited to write files outside the intended destination directory.The fix ensures that the resolved path for each blob download remains within the bounds of the user-provided
destination_directory. If a blob name would result in a path outside this directory (e.g., by using../), a warning is issued, and that specific blob download is skipped. This prevents directory traversal attacks.Absolute paths in blob names (e.g.,
/etc/passwd) are now treated as relative to thedestination_directory, so/etc/passwdwill be downloaded todestination_directory/etc/passwd.See b/449616593 for more details.
BREAKING CHANGE: Blobs that would resolve to a path outside the
destination_directoryare no longer downloaded. While this is a security fix, users relying on the previous behavior to write files outside the target directory will see a change.