GH#3706: Fix critical security findings in code-audit-helper.sh#4069
GH#3706: Fix critical security findings in code-audit-helper.sh#4069marcusquinn merged 1 commit intomainfrom
Conversation
…ewlines (GH#3706) - Add numeric validation for auto-detected pr_number from gh pr view, preventing SQL injection when gh returns unexpected non-numeric output - Add newline/CR stripping in sql_escape() to prevent multi-line SQL corruption in import_coderabbit_findings line-by-line INSERT generation Closes #3706
WalkthroughThe pull request addresses SQL injection vulnerabilities in the code-audit helper script by implementing input normalization in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🔍 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: Tue Mar 10 12:00:34 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/scripts/code-audit-helper.sh (1)
260-534: Note: API collector deduplication remains as future refactor opportunity.Issue
#3706also mentioned reducing duplicated code acrosscollect_sonarcloud,collect_codacy, andcollect_codefactorby introducing a genericcollect_from_apihelper. This is correctly deferred from this security-focused PR but worth tracking.The three collectors share a common pattern:
- Token presence check → API call → jq parsing → SQL file generation → batch import
A future refactor could extract this into a reusable helper accepting service name, URL builder, token variable, and jq filter. Not blocking for this PR.
Would you like me to open a follow-up issue to track the API collector deduplication refactor?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/code-audit-helper.sh around lines 260 - 534, The collectors collect_sonarcloud, collect_codacy, and collect_codefactor duplicate the same token-check → API call → jq parsing → SQL generation → db import flow; create a follow-up issue (tracking to `#3706`) that proposes a generic collect_from_api helper to accept service name, token/env var, URL builder, request headers/body, and jq filter file to centralize token validation, curl invocation, jq-to-SQL templating, tempfile cleanup (mktemp + _save_cleanup_scope/push_cleanup), and batch db import so these three functions can be refactored to call the shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/scripts/code-audit-helper.sh:
- Around line 260-534: The collectors collect_sonarcloud, collect_codacy, and
collect_codefactor duplicate the same token-check → API call → jq parsing → SQL
generation → db import flow; create a follow-up issue (tracking to `#3706`) that
proposes a generic collect_from_api helper to accept service name, token/env
var, URL builder, request headers/body, and jq filter file to centralize token
validation, curl invocation, jq-to-SQL templating, tempfile cleanup (mktemp +
_save_cleanup_scope/push_cleanup), and batch db import so these three functions
can be refactored to call the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be52573b-8cbf-4c35-8b5e-d91aac391df2
📒 Files selected for processing (1)
.agents/scripts/code-audit-helper.sh
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 implements crucial fixes to 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 effectively addresses two security vulnerabilities in code-audit-helper.sh. The validation added for the auto-detected pull request number is a crucial fix to prevent potential SQL injection. The change to the sql_escape function to handle newlines and carriage returns hardens the script against malformed SQL generation. I have one minor suggestion to improve the handling of carriage returns for better consistency.
| # Replace newlines and carriage returns with spaces to prevent | ||
| # multi-line SQL corruption in line-by-line INSERT generation | ||
| val="${val//$'\n'/ }" | ||
| val="${val//$'\r'/}" |
There was a problem hiding this comment.
For consistency and to correctly handle all types of line endings (like classic Mac OS \r), it's better to replace carriage returns with a space, just like you do for newlines. Currently, \r is removed, which could cause words to be merged together (e.g., word1\rword2 becomes word1word2).
| val="${val//$'\r'/}" | |
| val="${val//$'\r'/ }" |
|
Completed via PR #4094. Task t1429 merged to main. |
1 participant
Summary
gh pr viewto prevent SQL injection whenghreturns unexpected non-numeric output (CodeRabbit HIGH finding from PR t1032.1: Implement code-audit-helper.sh unified audit orchestrator #1376)sql_escape()with newline/CR stripping to prevent multi-line SQL corruption inimport_coderabbit_findingsline-by-line INSERT generation (CodeRabbit nitpick from PR t1032.1: Implement code-audit-helper.sh unified audit orchestrator #1376)Details
Fix 1 — SQL injection via unvalidated auto-detected PR number (HIGH)
Line 623 calls
gh pr view --json number -q .numberand assigns the result directly topr_number. While the--prCLI argument was already validated (line 611), the auto-detected path bypassed validation entirely. Ifghreturned unexpected output (error string leaking to stdout), it would flow directly into SQL interpolation on line 638.Added numeric regex validation (
^[0-9]+$) immediately after thegh pr viewassignment, defaulting to 0 on failure.Fix 2 — SQL file corruption from multi-line descriptions (MEDIUM)
sql_escape()only escaped single quotes for SQLite. Multi-line descriptions (from API responses or code comments) containing literal newlines would split INSERT statements across multiple lines in the SQL file generated byimport_coderabbit_findings, causing parse errors.Added
$'\n'and$'\r'replacement with spaces before the quote escaping step.Verification
bash -n: PASSshellcheck -x: zero violationsCloses #3706
Summary by CodeRabbit
Release Notes