Skip to content

feat: implement secure path validation for downloadManyFiles#2

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

feat: implement secure path validation for downloadManyFiles#2
thiyaguk09 wants to merge 2 commits intomainfrom
fix/download-directory-path-traversal

Conversation

@thiyaguk09
Copy link
Owner

  • Adds protection against path traversal (../) using normalized path resolution.
  • Prevents Windows-style drive letter injection while allowing GCS timestamps.
  • Implements directory jail logic to ensure absolute-style paths are relative to destination.
  • Preserves backward compatibility by returning an augmented DownloadResponse array.
  • Automates recursive directory creation for validated nested files.
  • Adds comprehensive 13-scenario test suite for edge-case parity.

- Adds protection against path traversal (../) using normalized path
resolution.
- Prevents Windows-style drive letter injection while allowing GCS
timestamps.
- Implements directory jail logic to ensure absolute-style paths are
relative to destination.
- Preserves backward compatibility by returning an augmented
DownloadResponse array.
- Automates recursive directory creation for validated nested files.
- Adds comprehensive 13-scenario test suite for edge-case parity.
@gemini-code-assist
Copy link

Summary of Changes

Hello, 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 significantly enhances the security posture of the downloadManyFiles function by introducing sophisticated path validation mechanisms. The changes prevent common vulnerabilities like path traversal and drive letter injection, ensuring that downloaded files are always placed within their intended target directories. The update also provides more informative responses for files that are skipped due to security concerns, improving the overall reliability and safety of bulk download operations.

Highlights

  • Enhanced Security for downloadManyFiles: Implemented robust path validation to prevent path traversal attacks (e.g., ../) and Windows-style drive letter injection during file downloads.
  • Directory Jailing Logic: Introduced logic to ensure that absolute-style paths are correctly jailed within the specified destination directory, rather than allowing them to escape.
  • Augmented Download Response: Modified the downloadManyFiles return type to DownloadResponseWithStatus, providing additional details for skipped files, including the reason for skipping (e.g., PATH_TRAVERSAL, ILLEGAL_CHARACTER).
  • Automated Recursive Directory Creation: Ensured that necessary nested directories are automatically created when downloading files to a specified destination.
  • Comprehensive Test Coverage: Added an extensive suite of 13 new test scenarios to validate the secure path handling and edge cases, including various forms of malicious paths and valid GCS timestamps.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • handwritten/storage/src/file.ts
    • Added DownloadResponseWithStatus type to include metadata about skipped files and download errors.
    • Introduced SkipReason enum to categorize why a file might be skipped during download.
  • handwritten/storage/src/transfer-manager.ts
    • Imported new types DownloadResponseWithStatus and SkipReason for use in downloadManyFiles.
    • Implemented path normalization and validation checks for each file's destination path to prevent traversal and illegal characters.
    • Modified downloadManyFiles to collect and return DownloadResponseWithStatus objects, indicating skipped files and their reasons.
    • Ensured recursive directory creation for download destinations and handled potential download errors by marking files as skipped.
  • handwritten/storage/test/transfer-manager.ts
    • Imported DownloadResponseWithStatus and SkipReason for testing the new functionality.
    • Added numerous test cases to verify path traversal prevention, drive letter injection prevention, and correct directory jailing behavior.
    • Included a comprehensive 'Parity Check' test to ensure all input files are accounted for and correctly categorized as downloaded or skipped based on security rules.
Activity
  • No human activity has been recorded on this pull request yet.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces crucial security enhancements to downloadManyFiles by implementing robust path validation to prevent path traversal attacks. While the changes are well-structured and include comprehensive tests, a critical vulnerability has been identified: the current implementation contains a flaw on Windows where absolute paths (e.g., via drive letter injection or UNC/NT paths) can bypass the jailbreak check. A remediation has been provided to ensure the validation is comprehensive across all platforms. Additionally, there are a couple of suggestions to improve code clarity and maintainability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant