t2813: Fix security vulnerabilities in pulse-wrapper.sh#2823
t2813: Fix security vulnerabilities in pulse-wrapper.sh#2823alex-solovyev merged 4 commits intomainfrom
Conversation
…njection - Add _validate_int() helper and validate all numeric env vars before arithmetic expansion — prevents RCE via crafted values like a[cmd] - Validate integers from file reads (sweep timestamp), ps output (etime components, RSS), vm_stat (page counts), and API responses (issue counts) before arithmetic or -gt comparisons - Add _sanitize_markdown() helper to strip @ mentions and backtick sequences from API data before embedding in GitHub issue comments - Consolidate 3 separate jq calls on scan_output into single pass - Consolidate 3 separate jq calls on sonar_issues into single pass - Consolidate 2 separate jq calls on sonar_status into single pass - Remove error-hiding 2>/dev/null on curl calls where it suppresses diagnostic information about network failures - Validate sys_load_1m/sys_cpu_cores before passing to awk (prevents awk injection) Closes #2813
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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIntroduces centralized integer validation and Markdown sanitization in Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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 significant security hardening for pulse-wrapper.sh. It successfully implements _validate_int() to prevent command injection via arithmetic expansion and _sanitize_markdown() for some API response data, applying these across various inputs. The consolidation of multiple jq calls also improves efficiency. However, critical command injection vulnerabilities persist in curl calls to the SonarCloud API due to unsanitized project_key interpolation; robust URL encoding, perhaps using python3, is recommended. Furthermore, some SonarCloud issue breakdown fields lack markdown sanitization, potentially causing notification spam or UI disruption. A minor suggestion is to remove a remaining stderr suppression to improve debuggability.
.agents/scripts/pulse-wrapper.sh
Outdated
| # SonarCloud public API — quality gate status | ||
| local sonar_status | ||
| sonar_status=$(curl -s "https://sonarcloud.io/api/qualitygates/project_status?projectKey=${project_key}" 2>/dev/null || echo "") | ||
| sonar_status=$(curl -s "https://sonarcloud.io/api/qualitygates/project_status?projectKey=${project_key}" || echo "") |
There was a problem hiding this comment.
The project_key variable is interpolated directly into the curl URL string without sanitization. If an attacker can control the project_key (e.g., via a modified .aidevops.json in a pull request), they can inject double quotes to terminate the URL string and add arbitrary curl options, such as -o to write to arbitrary files, which can lead to Remote Code Execution (RCE). Consider using python3 for robust URL encoding.
References
- Prefer using 'python3' (e.g., 'urllib.parse.quote') for robust URL encoding in shell scripts to handle unicode correctly and avoid potential 'jq' version compatibility issues.
.agents/scripts/pulse-wrapper.sh
Outdated
| # Fetch open issues summary | ||
| local sonar_issues | ||
| sonar_issues=$(curl -s "https://sonarcloud.io/api/issues/search?componentKeys=${project_key}&statuses=OPEN,CONFIRMED,REOPENED&ps=1&facets=severities,types" 2>/dev/null || echo "") | ||
| sonar_issues=$(curl -s "https://sonarcloud.io/api/issues/search?componentKeys=${project_key}&statuses=OPEN,CONFIRMED,REOPENED&ps=1&facets=severities,types" || echo "") |
There was a problem hiding this comment.
The project_key variable is interpolated directly into the curl URL string without sanitization. If an attacker can control the project_key (e.g., via a modified .aidevops.json in a pull request), they can inject double quotes to terminate the URL string and add arbitrary curl options, such as -o to write to arbitrary files, which can lead to Remote Code Execution (RCE). Consider using python3 for robust URL encoding.
References
- Prefer using 'python3' (e.g., 'urllib.parse.quote') for robust URL encoding in shell scripts to handle unicode correctly and avoid potential 'jq' version compatibility issues.
.agents/scripts/pulse-wrapper.sh
Outdated
| sys_load_ratio=$(awk "BEGIN {printf \"%d\", (${sys_load_1m} / ${sys_cpu_cores}) * 100}" 2>/dev/null || echo "?") | ||
| # Validate numeric before passing to awk (prevents awk injection) | ||
| if [[ "$sys_load_1m" =~ ^[0-9]+\.?[0-9]*$ ]] && [[ "$sys_cpu_cores" =~ ^[0-9]+$ ]]; then | ||
| sys_load_ratio=$(awk "BEGIN {printf \"%d\", (${sys_load_1m} / ${sys_cpu_cores}) * 100}" 2>/dev/null || echo "?") |
There was a problem hiding this comment.
While the new validation on line 1093 prevents awk injection, this 2>/dev/null still suppresses stderr, which could hide other issues (e.g., if the awk command is not found or has other unexpected errors). The PR description mentions removing error-hiding 2>/dev/null, and several general rules for this repository advise against it for better debugging. Since you have a fallback with || echo "?", it's safe to let potential errors be printed to stderr.
| sys_load_ratio=$(awk "BEGIN {printf \"%d\", (${sys_load_1m} / ${sys_cpu_cores}) * 100}" 2>/dev/null || echo "?") | |
| sys_load_ratio=$(awk "BEGIN {printf \"%d\", (${sys_load_1m} / ${sys_cpu_cores}) * 100}" || echo "?") |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/scripts/pulse-wrapper.sh (1)
1569-1595:⚠️ Potential issue | 🟠 MajorAdd error handling and timeouts to SonarCloud curl calls
The current
curl -sinvocations at lines 1569 and 1594 suppress all error output and lack timeout protections, risking silent failures and unbounded waits in automation workflows. Use-Sto show errors despite silent mode,--failto exit on HTTP errors, and timeout flags to prevent hanging:Fix
- sonar_status=$(curl -s "https://sonarcloud.io/api/qualitygates/project_status?projectKey=${project_key}" || echo "") + sonar_status=$(curl -sS --fail --connect-timeout 5 --max-time 20 \ + "https://sonarcloud.io/api/qualitygates/project_status?projectKey=${project_key}" || echo "") @@ - sonar_issues=$(curl -s "https://sonarcloud.io/api/issues/search?componentKeys=${project_key}&statuses=OPEN,CONFIRMED,REOPENED&ps=1&facets=severities,types" || echo "") + sonar_issues=$(curl -sS --fail --connect-timeout 5 --max-time 20 \ + "https://sonarcloud.io/api/issues/search?componentKeys=${project_key}&statuses=OPEN,CONFIRMED,REOPENED&ps=1&facets=severities,types" || echo "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/pulse-wrapper.sh around lines 1569 - 1595, The SonarCloud curl calls (producing sonar_status and sonar_issues and feeding gate_data/sonar_section) currently use silent curl which hides errors and have no timeouts; update both invocations that set sonar_status and sonar_issues to include error reporting and timeouts (e.g., add -S --fail and sensible --connect-timeout/--max-time flags) so failures return non-empty error output or fail fast, and ensure the existing gate_data parsing and _sanitize_markdown/sonar_section logic still handles an empty or failed response gracefully (e.g., fall back to "UNKNOWN" or an empty conditions string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/pulse-wrapper.sh:
- Around line 46-61: The _validate_int function currently returns the raw
numeric string (which may have leading zeros) causing bash to interpret values
as octal in subsequent arithmetic; update _validate_int to canonicalize
validated integers to base-10 before returning (e.g., convert to a decimal
integer using an explicit base-10 conversion or strip leading zeros) so any
input like "08" or "01024" becomes a safe decimal; ensure the function still
rejects non-digits, logs the same error message, and returns the normalized
decimal for the variables PULSE_STALE_THRESHOLD, ORPHAN_MAX_AGE,
RAM_PER_WORKER_MB, RAM_RESERVE_MB, MAX_WORKERS_CAP, and QUALITY_SWEEP_INTERVAL.
---
Outside diff comments:
In @.agents/scripts/pulse-wrapper.sh:
- Around line 1569-1595: The SonarCloud curl calls (producing sonar_status and
sonar_issues and feeding gate_data/sonar_section) currently use silent curl
which hides errors and have no timeouts; update both invocations that set
sonar_status and sonar_issues to include error reporting and timeouts (e.g., add
-S --fail and sensible --connect-timeout/--max-time flags) so failures return
non-empty error output or fail fast, and ensure the existing gate_data parsing
and _sanitize_markdown/sonar_section logic still handles an empty or failed
response gracefully (e.g., fall back to "UNKNOWN" or an empty conditions
string).
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e87d191-b86c-4950-b747-3b976bc41538
📒 Files selected for processing (1)
.agents/scripts/pulse-wrapper.sh
- _validate_int(): canonicalize to base-10 via printf '%d' "$((10#$value))" to prevent bash octal interpretation of leading-zero inputs (CodeRabbit) - URL-encode project_key via python3 urllib.parse.quote before interpolation into SonarCloud curl URLs to prevent command injection (Gemini) - Add -sS --fail --connect-timeout 5 --max-time 20 to SonarCloud curl calls with graceful fallback to empty string on failure (CodeRabbit) - Sanitize severity_breakdown and type_breakdown API fields via _sanitize_markdown() before embedding in GitHub comments (Gemini) - Remove 2>/dev/null from awk sys_load_ratio for better debuggability (Gemini)
…Rabbit trigger Merge main into bugfix/t2813-pulse-wrapper-security, resolving conflicts in pulse-wrapper.sh SonarCloud section: - Keep consolidated single-jq-pass approach from security branch (efficiency) - Extend issues jq query to also extract high/critical count in same pass - Wire sweep_gate_status, sweep_total_issues, sweep_high_critical into the CodeRabbit conditional trigger logic added in main (t1390) - Preserve all security hardening: URL-encoded project_key, curl timeouts, _sanitize_markdown on API data, numeric validation Verification: shellcheck zero violations, bash -n syntax valid
Merge conflict resolution + review feedbackMerge conflicts resolvedMerged Resolution strategy: Keep both — the security hardening from this branch AND the sweep state variables from main:
Review feedback statusAll review feedback from the previous round was already addressed in commit 30fc755:
Verification
|
🔍 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: Wed Mar 4 13:36:30 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/pulse-wrapper.sh:
- Around line 1831-1839: After calling _load_sweep_state() ensure the returned
prev_issues and prev_high_critical are normalized to numeric values before any
delta arithmetic: check each (prev_issues, prev_high_critical) with a numeric
regex (e.g. ^[0-9]+$) or arithmetic-safe test and set to 0 when invalid or empty
so downstream calculations that use prev_issues and prev_high_critical won't
fail; add this validation immediately after the _load_sweep_state() call and
before the code that computes deltas from sweep_total_issues and
sweep_high_critical.
- Around line 46-63: The _validate_int function currently allows zero which
makes RAM_PER_WORKER_MB=0 cause a divide-by-zero later; update validation so
divisor-backed settings cannot be zero by either (a) extending _validate_int to
accept an optional minimum parameter and return the default if value < min, or
(b) after calling _validate_int for RAM_PER_WORKER_MB check the returned value
and if it is less than 1 print an error and fall back to the default (1024);
specifically update the call that sets RAM_PER_WORKER_MB and the _validate_int
logic to enforce a minimum of 1 for that variable (reference symbols:
_validate_int and RAM_PER_WORKER_MB) so division operations using
RAM_PER_WORKER_MB never receive zero.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 150c3abb-f3e8-49cd-97d9-2d8e27256e86
📒 Files selected for processing (1)
.agents/scripts/pulse-wrapper.sh
- _validate_int: add optional min parameter; RAM_PER_WORKER_MB uses min=1 to prevent divide-by-zero in calculate_max_workers - Post _load_sweep_state: validate prev_issues and prev_high_critical are numeric before delta arithmetic; fall back to 0 on corrupt/missing values Addresses CodeRabbit CHANGES_REQUESTED on PR #2823 (latest review 2026-03-04T13:42)
CodeRabbit CHANGES_REQUESTED addressed (2026-03-04T13:42 review)Both actionable findings from the latest CodeRabbit review are now fixed in commit a3d2381: 1. 2. ShellCheck: zero violations. |
🔍 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: Wed Mar 4 14:10:04 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
|
Dispatching a worker to address CodeRabbit's CHANGES_REQUESTED review feedback. Issues flagged:
Resolves #2813 |
1 participant
Summary
Addresses critical security vulnerabilities identified by Gemini Code Assist review on PR #2808.
Security fixes (CRITICAL)
Command injection via arithmetic expansion: Added
_validate_int()helper that validates all values are^[0-9]+$before they enter$(( ))expansion. Bash arithmetic evaluates variable contents as expressions, so unsanitised strings likea[$(cmd)]execute arbitrary commands. Applied to:PULSE_STALE_THRESHOLD,ORPHAN_MAX_AGE,RAM_PER_WORKER_MB,RAM_RESERVE_MB,MAX_WORKERS_CAP,QUALITY_SWEEP_INTERVAL)QUALITY_SWEEP_LAST_RUN)psetime components, RSS values)vm_statpage counts,sysctlpage size)_compute_struggle_ratio(STRUGGLE_RATIO_THRESHOLD,STRUGGLE_MIN_ELAPSED_MINUTES)Markdown/mention injection: Added
_sanitize_markdown()helper that strips@mentions and backtick sequences from API response data before embedding in GitHub issue comments. Applied to SonarCloudgate_statusandconditions.Awk injection: Validate
sys_load_1mandsys_cpu_coresare numeric before interpolating into awk expressions.Efficiency improvements (MEDIUM)
Consolidated jq calls: Reduced from 3 separate
jqinvocations to 1 for:sonar_status(gate status + conditions in single pass)sonar_issues(total + severity + type breakdown in single pass)scan_output(scanned + findings + issues_created in single pass)Removed error-hiding
2>/dev/null: Removed stderr suppression oncurlcalls for SonarCloud API where it hid diagnostic information about network failures. Kept2>/dev/nullonly where it's intentional (e.g.,gopass showfor optional credentials,killfor already-dead processes).Verification
shellcheck— zero violationsbash -n— syntax validCloses #2813
Summary by CodeRabbit