🔒 Fix Command Injection in Deploy Script via Unescaped Hostname#1193
Conversation
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>
|
👋 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 significantly enhances the security posture of the deployment script by preventing potential command and option injection vulnerabilities. It introduces robust input validation for Docker container names and correctly utilizes command delimiters, ensuring that the script handles user-provided or environment-derived container names safely and predictably, thereby protecting against malicious manipulation. 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 command injection vulnerability in the deploy script. The use of a validation function for container names and the -- delimiter in docker commands are excellent security measures. My review includes a few suggestions to further improve the script's efficiency and robustness by using shell scripting best practices, such as using arrays for lists of arguments and preferring built-in operators over external commands.
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>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…-10708231991083344428
* 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>
This PR addresses a security vulnerability in
scripts/deploy.shwhere unescaped or maliciously crafted container names could lead to command or option injection.🎯 What: The vulnerability fixed is a potential command/option injection via the
⚠️ Risk: A malicious user or a compromised environment could potentially inject Docker options by crafting container names that start with a dash or contain other unexpected characters.
$containervariable in thehealth_checkandshow_statusfunctions.🛡️ Solution:
validate_container_namefunction that ensures container names match the expected Docker format (^[a-zA-Z0-9][a-zA-Z0-9_.-]*$).docker execanddocker statscalls to use the--delimiter, ensuring that subsequent arguments are treated as positional arguments rather than options.health_checkandshow_statusloops.These changes follow security best practices for shell scripting and provide defense-in-depth against injection attacks.
PR created automatically by Jules for task 10708231991083344428 started by @GrammaTonic