🔒 fix(security): sanitize job name in file paths to prevent traversal#1192
Conversation
Mitigate path traversal vulnerability by sanitizing GITHUB_JOB before using it in file paths in job-started.sh and job-completed.sh. Introduced a sanitize_name function in utils.sh to replace unsafe characters with underscores. Updated unit tests accordingly. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
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 critical path traversal vulnerability in job lifecycle scripts by implementing robust sanitization for job names used in file paths. This enhancement prevents malicious actors from manipulating file system operations, thereby ensuring the integrity and security of the runner environment. Highlights
🧠 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
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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a path traversal vulnerability by sanitizing the GITHUB_JOB environment variable. The introduction of a sanitize_name utility function is a good approach. My review includes a couple of suggestions for improvement: one to enhance the performance and readability of the new sanitize_name function by using a shell builtin, and another to improve the robustness of the unit tests by sourcing the actual utility script instead of using a flawed mock. The latter is important to ensure the tests accurately reflect the behavior of the production code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* chore(deps): chore(deps)(deps): bump actions/cache from 4 to 5 Automatically merged Dependabot PR after CI validation. * chore(deps): chore(deps)(deps): bump trufflesecurity/trufflehog from 3.93.7 to 3.93.8 Automatically merged Dependabot PR after CI validation. * chore(deps): chore(deps)(deps): bump docker/metadata-action from 5 to 6 Automatically merged Dependabot PR after CI validation. * 🔒 Fix Personal Access Token exposure in runner-self-test.sh (#1191) * 🔒 fix: prevent Personal Access Token exposure in CLI history Modified `scripts/runner-self-test.sh` to pass the GitHub Personal Access Token (`GH_PAT`) to `curl` via stdin using the `-K-` option. This prevents the token from appearing in process listings or command history, addressing a security vulnerability. Changes: - Replaced direct `-H "Authorization: token $GH_PAT"` with a piped configuration using `printf` and `curl -K-`. - Added a security verification test in `tests/security/verify-curl-fix.sh`. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> * Update tests/security/verify-curl-fix.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * 🔒 Fix Command Injection in Deploy Script via Unescaped Hostname (#1193) * 🔒 Fix command injection vulnerability in deploy script Added container name validation and improved docker command security by using the -- delimiter to prevent option injection. - Added `validate_container_name` function to enforce Docker naming conventions. - Updated `health_check` and `show_status` functions in `scripts/deploy.sh` to validate container names before use and use `--` in Docker commands. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> * Update scripts/deploy.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update scripts/deploy.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update scripts/deploy.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * 🔒 fix(security): sanitize job name in file paths to prevent traversal (#1192) * fix(security): sanitize job name in file paths Mitigate path traversal vulnerability by sanitizing GITHUB_JOB before using it in file paths in job-started.sh and job-completed.sh. Introduced a sanitize_name function in utils.sh to replace unsafe characters with underscores. Updated unit tests accordingly. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> * Update tests/unit/test-job-started.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update docker/utils.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * feat: enforce Node.js 24 for all runners and workflows (#1190) * feat: enforce Node.js 24 for all runners via FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env var - Adds FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true to all runner Docker Compose files - Documents the variable in all example .env files - Ensures all JavaScript actions use Node.js 24 across standard, Chrome, and Chrome-Go runners See #performance, #devops, #nodejs, #ci-cd * chore: enforce Node.js 24 for all actions in ci-cd workflow via FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env var --------- Co-authored-by: rr <rr> --------- Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* chore(deps): chore(deps)(deps): bump actions/cache from 4 to 5 Automatically merged Dependabot PR after CI validation. * chore(deps): chore(deps)(deps): bump trufflesecurity/trufflehog from 3.93.7 to 3.93.8 Automatically merged Dependabot PR after CI validation. * chore(deps): chore(deps)(deps): bump docker/metadata-action from 5 to 6 Automatically merged Dependabot PR after CI validation. * 🔒 Fix Personal Access Token exposure in runner-self-test.sh (#1191) * 🔒 fix: prevent Personal Access Token exposure in CLI history Modified `scripts/runner-self-test.sh` to pass the GitHub Personal Access Token (`GH_PAT`) to `curl` via stdin using the `-K-` option. This prevents the token from appearing in process listings or command history, addressing a security vulnerability. Changes: - Replaced direct `-H "Authorization: token $GH_PAT"` with a piped configuration using `printf` and `curl -K-`. - Added a security verification test in `tests/security/verify-curl-fix.sh`. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> * Update tests/security/verify-curl-fix.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * 🔒 Fix Command Injection in Deploy Script via Unescaped Hostname (#1193) * 🔒 Fix command injection vulnerability in deploy script Added container name validation and improved docker command security by using the -- delimiter to prevent option injection. - Added `validate_container_name` function to enforce Docker naming conventions. - Updated `health_check` and `show_status` functions in `scripts/deploy.sh` to validate container names before use and use `--` in Docker commands. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> * Update scripts/deploy.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update scripts/deploy.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update scripts/deploy.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * 🔒 fix(security): sanitize job name in file paths to prevent traversal (#1192) * fix(security): sanitize job name in file paths Mitigate path traversal vulnerability by sanitizing GITHUB_JOB before using it in file paths in job-started.sh and job-completed.sh. Introduced a sanitize_name function in utils.sh to replace unsafe characters with underscores. Updated unit tests accordingly. Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> * Update tests/unit/test-job-started.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update docker/utils.sh Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * feat: enforce Node.js 24 for all runners and workflows (#1190) * feat: enforce Node.js 24 for all runners via FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env var - Adds FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true to all runner Docker Compose files - Documents the variable in all example .env files - Ensures all JavaScript actions use Node.js 24 across standard, Chrome, and Chrome-Go runners See #performance, #devops, #nodejs, #ci-cd * chore: enforce Node.js 24 for all actions in ci-cd workflow via FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env var --------- Co-authored-by: rr <rr> * chore: update Go version to 1.25.7 across Chrome-Go runner configs, Dockerfiles, compose, and docs --------- Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: GrammaTonic <8269379+GrammaTonic@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: rr <rr>
🎯 What: The vulnerability fixed
This PR fixes a path traversal vulnerability in the job lifecycle hook scripts (
job-started.shandjob-completed.sh). The vulnerability occurred because theGITHUB_JOBenvironment variable, which can be controlled by workflow authors, was used directly to construct file paths for state tracking without any sanitization.An attacker with the ability to define job names (e.g., in a malicious pull request or a compromised workflow) could use path traversal sequences (like
../../) in the job name. This could allow them to:job-completed.shattempts to clean up state files).🛡️ Solution: How the fix addresses the vulnerability
The fix introduces a new utility function
sanitize_nameindocker/utils.sh. This function:sedto replace any character that is not alphanumeric, an underscore, or a hyphen with an underscore..), preventing the use of../sequences.printfinstead ofechoto safely handle input strings that might start with hyphens.Both
job-started.shandjob-completed.shnow sourceutils.shand usesanitize_nameto process the job name before incorporating it into any file path.Existing unit tests in
tests/unit/test-job-started.shhave been updated to reflect and verify this new sanitization logic.PR created automatically by Jules for task 5016186936655225833 started by @GrammaTonic