fix(security): harden runtime path validation for upload/output#363
fix(security): harden runtime path validation for upload/output#363abhinavkale-dev wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 442a4d7 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 |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
googleworkspace-bot
added
area: skills
area: http
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 strict validation for file paths provided via 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 important security hardening for file path validation. The new validation logic in src/validate.rs is comprehensive and covers various attack vectors like absolute paths, path traversal, and symlink escapes. However, there is a critical issue in src/executor.rs where the validated, canonical paths are discarded, and the original, potentially unsafe paths are used for file operations. This undermines the security fix and leaves a TOCTOU (Time-of-check to time-of-use) vulnerability. My review includes a critical comment with a suggestion to fix this by propagating and using the safe canonical paths.
| fn validate_execution_paths( | ||
| output_path: Option<&str>, | ||
| upload_path: Option<&str>, | ||
| ) -> Result<(), GwsError> { | ||
| if let Some(path) = output_path { | ||
| crate::validate::validate_safe_output_file_path(path)?; | ||
| } | ||
| if let Some(path) = upload_path { | ||
| crate::validate::validate_safe_input_file_path(path)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This validation function is a great addition for security. However, it currently has a critical flaw: it performs the validation but discards the resulting canonical (safe) path. The rest of the code continues to use the original, potentially unsafe output_path and upload_path strings. This creates a Time-of-check to time-of-use (TOCTOU) vulnerability, as the file system could be manipulated between the check and the actual file operation.
To fix this, validate_execution_paths should return the canonical PathBufs. Then, execute_method must use these canonical paths for all subsequent file operations.
After applying the suggestion below, you will also need to update execute_method to capture and use these canonical paths, passing them down to build_http_request and handle_binary_response. Those functions will need to be adjusted to accept &Path instead of &str for file paths.
| fn validate_execution_paths( | |
| output_path: Option<&str>, | |
| upload_path: Option<&str>, | |
| ) -> Result<(), GwsError> { | |
| if let Some(path) = output_path { | |
| crate::validate::validate_safe_output_file_path(path)?; | |
| } | |
| if let Some(path) = upload_path { | |
| crate::validate::validate_safe_input_file_path(path)?; | |
| } | |
| Ok(()) | |
| } | |
| fn validate_execution_paths( | |
| output_path: Option<&str>, | |
| upload_path: Option<&str>, | |
| ) -> Result<(Option<PathBuf>, Option<PathBuf>), GwsError> { | |
| let canonical_output_path = output_path | |
| .map(crate::validate::validate_safe_output_file_path) | |
| .transpose()?; | |
| let canonical_upload_path = upload_path | |
| .map(crate::validate::validate_safe_input_file_path) | |
| .transpose()?; | |
| Ok((canonical_output_path, canonical_upload_path)) | |
| } |
abhinavkale-dev
force-pushed
the
path-hardening-upload-output
branch
from
60013c2 to
93f78b0
Compare
|
/gemini review |
|
@jpoehnelt, Gemini’s canonical-path finding is fixed in The remaining extra commit on this PR,
is unrelated and is not part of the security fix. It is being added by my fork workflow and is causing the CLA check to fail again by adding The intended fix commits are:
If easier, please review/cherry-pick only those two commits. |
abhinavkale-dev
force-pushed
the
path-hardening-upload-output
branch
from
2194edb to
93f78b0
Compare
|
/gemini review |
2 participants
Refs #251
Description
This PR hardens runtime file path handling for dynamic
--uploadand--output.It adds shared path validators for safe input/output file paths and applies them at the execution boundary so unsafe absolute paths, traversal (
..), and escapes outside the current working directory are rejected before dry-run or execution.This fixes the issue where commands like these were previously accepted:
After this change, those inputs fail with validation errors instead of proceeding.
Dry Run Output:
{ "error": { "code": 400, "message": "--output must be a relative path, got absolute path '/tmp/gws-proof.bin'", "reason": "validationError" } }Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.