fix(security): validate --upload and --output paths against traversal#447
fix(security): validate --upload and --output paths against traversal#447anshul-garg27 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 4844e8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
the
area: core
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 significantly enhances the security of the CLI by introducing robust path validation for 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
This pull request introduces a security fix to prevent path traversal vulnerabilities for --upload and --output command-line arguments. A new validate_safe_file_path function has been added to src/validate.rs which rejects paths that attempt to escape the current working directory via .. traversal, contain control characters, or follow symlinks to external locations. This validation is integrated into src/main.rs to be performed before any file I/O operations. Comprehensive unit tests have also been added to src/validate.rs to cover various path validation scenarios.
The --upload and --output flags accepted arbitrary file paths without validation, allowing path traversal (../../.ssh/id_rsa) and symlink escape attacks. This is especially dangerous when the CLI is invoked by an AI agent that may receive adversarial input. Add validate_safe_file_path() to validate.rs and call it in main.rs before any file I/O. Rejects paths that resolve outside CWD, contain control characters, or follow symlinks to external locations.
anshul-garg27
force-pushed
the
fix/validate-upload-output-paths
branch
from
cc5a973 to
4844e8e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix a path traversal security vulnerability by validating --upload and --output paths. The implementation in src/validate.rs is comprehensive, covering control characters, path traversal, and symlink escapes. However, there is a critical Time-of-check to time-of-use (TOCTOU) vulnerability in src/main.rs. The result of the path validation is discarded, and the original, un-sanitized path is used for file operations, which defeats the purpose of the validation. This must be fixed by using the canonicalized path returned by the validation function.
| if let Some(p) = upload_path { | ||
| crate::validate::validate_safe_file_path(p, "--upload")?; | ||
| } | ||
| if let Some(p) = output_path { | ||
| crate::validate::validate_safe_file_path(p, "--output")?; | ||
| } |
There was a problem hiding this comment.
This validation introduces a Time-of-check to time-of-use (TOCTOU) vulnerability. The validate_safe_file_path function is called, and it correctly canonicalizes the path to prevent traversal. However, the returned safe PathBuf is discarded. The original, potentially unsafe path string is then used later for file I/O in executor::execute_method. An attacker could modify the file system between the check and the use (e.g., by replacing a file with a symlink to a sensitive location) to bypass this validation.
To fix this, you must use the canonicalized path returned from the validation function for all subsequent operations. This will likely require refactoring how upload_path and output_path are defined and passed to executor::execute_method to ensure the sanitized path is used.
There was a problem hiding this comment.
yeah TOCTOU is inherent to filesystem checks, but this still blocks the most common attack vector (user-supplied paths with ../ in them). a full fix would need O_NOFOLLOW or similar which is a bigger change. this raises the bar significantly vs no validation at all
…alidate --upload/--output paths - Cap Retry-After header at 60s to prevent hostile servers from hanging the CLI - Extract compute_retry_delay() with saturating_pow for safe exponential backoff - Sanitize mimeType by stripping control characters to prevent MIME header injection - Add validate_safe_file_path() for --upload and --output path validation - Gate --upload/--output through path validation in main.rs before any I/O Consolidates security fixes from PRs #448 and #447.
…alidate --upload/--output paths (#523) * fix(security): cap Retry-After sleep, sanitize upload mimeType, and validate --upload/--output paths - Cap Retry-After header at 60s to prevent hostile servers from hanging the CLI - Extract compute_retry_delay() with saturating_pow for safe exponential backoff - Sanitize mimeType by stripping control characters to prevent MIME header injection - Add validate_safe_file_path() for --upload and --output path validation - Gate --upload/--output through path validation in main.rs before any I/O Consolidates security fixes from PRs #448 and #447. * chore: regenerate skills [skip ci] * fix: resolve mimeType sanitization bypass and document TOCTOU caveat - Restructure resolve_upload_mime() using or_else chain so all code paths go through control-char stripping (early returns were bypassing it) - Document TOCTOU limitation in validate_safe_file_path() as known caveat * fix: use canonicalized paths for I/O and normalize .. in non-existent suffix Address review comments: - main.rs: use canonicalized path from validate_safe_file_path for I/O instead of discarding it (closes TOCTOU gap) - validate.rs: add normalize_dotdot() to resolve .. components in non-existent suffix (prevents traversal via doesnt_exist/../../etc/passwd) - Add regression test for non-existent prefix traversal bypass --------- Co-authored-by: jpoehnelt-bot <jpoehnelt-bot@users.noreply.github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@users.noreply.github.com>
3 participants
Summary
validate_safe_file_path()tosrc/validate.rsthat rejects paths escaping CWDsrc/main.rsfor both--uploadand--outputflags before any file I/O--upload ../../.ssh/id_rsa) especially dangerous when CLI is invoked by AI agentsDetails
Previously,
--uploadand--outputaccepted arbitrary file paths without validation. The path was passed directly totokio::fs::read()(upload) ortokio::fs::File::create()(output) with no checks.An AI agent receiving adversarial input could be tricked into:
--upload ../../.ssh/id_rsa--output ../../.bashrcThe new
validate_safe_file_path()function:Test plan
../../etc/passwdtraversal is rejected/tmpis rejected (Unix only)