fix(new-task): harden against shell injection and prompt injection#4388
fix(new-task): harden against shell injection and prompt injection#4388alex-solovyev merged 1 commit intomainfrom
Conversation
- Wrap $ARGUMENTS in <user_input> XML tags with explicit instruction to treat content as untrusted data, mitigating prompt injection (GH#3494 MEDIUM) - Replace bare placeholder strings in shell command examples with variable-assignment pattern (TASK_TITLE="..."; --title "$TASK_TITLE") to prevent shell metacharacter injection (GH#3494 HIGH x2) - Replace echo|grep|cut pipeline with single-pass while/case read loop for output parsing — more efficient and consistent with planning-commit-helper.sh (GH#3494 MEDIUM) - Add inline security notes explaining the injection risks and safe patterns Closes #3494
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 and efficiency of the Highlights
Changelog
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
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Fri Mar 13 07:46:50 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
There was a problem hiding this comment.
Code Review
This pull request effectively hardens the new-task command documentation against prompt and shell injection vulnerabilities, and improves parsing efficiency. The changes are well-implemented, using XML tags to guard user input in prompts and promoting safer variable-based patterns for shell commands. The refactoring from echo|grep|cut to a while read loop is a good performance enhancement. I have one minor suggestion to improve the clarity of a shell command example, but overall, these are excellent improvements.
| output=$(TASK_TITLE="$TASK_TITLE" ~/.aidevops/agents/scripts/planning-commit-helper.sh next-id --title "$TASK_TITLE") | ||
|
|
||
| # Or directly via claim-task-id.sh | ||
| output=$(~/.aidevops/agents/scripts/claim-task-id.sh --title "Task title here" --repo-path "$(git rev-parse --show-toplevel)") | ||
| output=$(TASK_TITLE="$TASK_TITLE" ~/.aidevops/agents/scripts/claim-task-id.sh --title "$TASK_TITLE" --repo-path "$(git rev-parse --show-toplevel)") |
There was a problem hiding this comment.
The VAR="$VAR" command pattern, while valid, is a bit redundant here and could be confusing. The primary security benefit comes from assigning user input to a variable and then passing that quoted variable as an argument to the command. The environment variable prefix isn't strictly necessary for the shell injection protection described and makes the example slightly harder to read. Simplifying the example to just pass the quoted variable as an argument would be clearer and equally secure.
| output=$(TASK_TITLE="$TASK_TITLE" ~/.aidevops/agents/scripts/planning-commit-helper.sh next-id --title "$TASK_TITLE") | |
| # Or directly via claim-task-id.sh | |
| output=$(~/.aidevops/agents/scripts/claim-task-id.sh --title "Task title here" --repo-path "$(git rev-parse --show-toplevel)") | |
| output=$(TASK_TITLE="$TASK_TITLE" ~/.aidevops/agents/scripts/claim-task-id.sh --title "$TASK_TITLE" --repo-path "$(git rev-parse --show-toplevel)") | |
| output=$(~/.aidevops/agents/scripts/planning-commit-helper.sh next-id --title "$TASK_TITLE") | |
| # Or directly via claim-task-id.sh | |
| output=$(~/.aidevops/agents/scripts/claim-task-id.sh --title "$TASK_TITLE" --repo-path "$(git rev-parse --show-toplevel)") |
There was a problem hiding this comment.
Good catch — the TASK_TITLE="$TASK_TITLE" prefix was indeed redundant since the variable is already in scope after the shell assignment. Applied your suggestion in PR #4407: the env-var prefix has been removed from both command examples, keeping the security protection (assign to variable first, pass quoted "$TASK_TITLE" as argument) while improving readability.
…mples The TASK_TITLE="$TASK_TITLE" prefix before the command was redundant — the variable is already in scope after the assignment. Simplify to pass "$TASK_TITLE" directly as the quoted argument, which is equally secure and clearer to read. Addresses Gemini inline suggestion on PR #4388.
…quality-debt resolved (#4413) * chore(new-task): remove redundant env-var prefix in shell command examples The TASK_TITLE="$TASK_TITLE" prefix before the command was redundant — the variable is already in scope after the assignment. Simplify to pass "$TASK_TITLE" directly as the quoted argument, which is equally secure and clearer to read. Addresses Gemini inline suggestion on PR #4388. * fix(business): correct dispatch CLI from Claude to opencode in sensitive ops example (GH#3457) All critical PR #2054 review findings were already addressed in subsequent commits: - CRITICAL: general/explore non-existent subagents removed (fixed in PR #2054 second commit) - MEDIUM: company-runners added to subagents list (fixed in PR #2054 second commit) - HIGH: duplicate monthly-close convoy name resolved (coordinator section removed in t1335/t1336) This commit fixes the one remaining inconsistency: the Sensitive Operations example used 'Claude run' while the rest of the file correctly uses 'opencode run'. Closes #3457
…tinct paragraphs (#4411) * chore(new-task): remove redundant env-var prefix in shell command examples The TASK_TITLE="$TASK_TITLE" prefix before the command was redundant — the variable is already in scope after the assignment. Simplify to pass "$TASK_TITLE" directly as the quoted argument, which is equally secure and clearer to read. Addresses Gemini inline suggestion on PR #4388. * fix(speech-to-speech): separate security blockquote into distinct paragraphs Address gemini-code-assist inline suggestion on PR #4397: add blank lines between the three parts of the security blockquote so each key point (storage instructions, hardcoding warning, reference link) stands out as a distinct paragraph for improved readability. Closes #3554
…jection example (#4522) * chore(new-task): remove redundant env-var prefix in shell command examples The TASK_TITLE="$TASK_TITLE" prefix before the command was redundant — the variable is already in scope after the assignment. Simplify to pass "$TASK_TITLE" directly as the quoted argument, which is equally secure and clearer to read. Addresses Gemini inline suggestion on PR #4388. * fix(docs): use mktemp for secure temp file in build-mcp key injection example Replace hardcoded /tmp/oc.json with mktemp to eliminate race condition and path-hijacking risk. Also fix the broken command chain where a trailing backslash caused && to be passed as an argument to jq instead of chaining the mv command. Addresses Gemini review feedback from PR #4247. Closes #4267



Summary
Fixes all 4 findings from the Gemini code review on PR #1259, closing #3494.
HIGH: Shell injection via unquoted user input in command templates (lines 26 & 76)
Before: Shell command examples used bare placeholder strings inside double quotes:
output=$(~/.aidevops/agents/scripts/planning-commit-helper.sh next-id --title "Task title here")An agent runner performing direct string substitution with user-supplied input could allow shell metacharacters (
$(...),`...`,;) to execute arbitrary commands.After: Examples now use the variable-assignment pattern with explicit security guidance:
Inline
> **Security note**blocks explain the risk and safe pattern.MEDIUM: Prompt injection via bare
$ARGUMENTS(line 9)Before:
$ARGUMENTSwas concatenated directly into the prompt with no delimiter, allowing a malicious task title to inject instructions.After:
$ARGUMENTSis wrapped in<user_input>XML tags with an explicit instruction to treat the content as untrusted data:MEDIUM: Inefficient
echo|grep|cutparsing (line 45)Before: Three separate process spawns per variable:
task_id=$(echo "$output" | grep '^TASK_ID=' | cut -d= -f2)After: Single-pass
while readloop, consistent withplanning-commit-helper.sh:Closes #3494