GH#3716: Fix CRITICAL security issues from PR #1359 review#4062
GH#3716: Fix CRITICAL security issues from PR #1359 review#4062marcusquinn merged 1 commit intomainfrom
Conversation
…iage_result in migration (GH#3716) - batch.sh: validate max_retries, concurrency, max_concurrency, max_load_factor as integers before embedding in SQL INSERT statements (SQL injection prevention) - cron.sh: validate --interval is a positive integer before use in cron commands - database.sh: include triage_result column in t148 migration INSERT...SELECT to prevent data loss when migrating from schemas that have the column Closes #3716
🔍 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 10:17:29 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
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 critical security and data integrity issues identified in a previous review. It introduces robust input validation for several parameters in shell scripts to prevent SQL injection and ensures that a database migration correctly preserves all data, specifically 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 several critical security vulnerabilities by introducing input validation for shell scripts and correcting a data migration to prevent data loss. The changes are well-targeted and correctly implemented. My feedback includes suggestions to simplify and strengthen the validation logic for positive integers in batch.sh and cron.sh, aligning with best practices for defense-in-depth against potential injection attacks.
| if ! [[ "$2" =~ ^[0-9]+$ ]] || [[ "$2" -eq 0 ]]; then | ||
| log_error "--concurrency must be a positive integer, got: $2" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
To enhance defense-in-depth against potential injection attacks when parsing numeric data from untrusted sources, it's crucial to validate numeric fields. You can simplify this positive integer validation by using a single regular expression that excludes zero. This makes the check more concise, readable, and robust.
| if ! [[ "$2" =~ ^[0-9]+$ ]] || [[ "$2" -eq 0 ]]; then | |
| log_error "--concurrency must be a positive integer, got: $2" | |
| return 1 | |
| fi | |
| if ! [[ "$2" =~ ^[1-9][0-9]*$ ]]; then | |
| log_error "--concurrency must be a positive integer, got: $2" | |
| return 1 | |
| fi |
References
- When parsing delimited data from an untrusted source in a shell script, validate numeric fields before using them in calculations. This provides defense-in-depth against injection attacks that could result from delimiter shifting caused by malicious data.
| if ! [[ "$2" =~ ^[0-9]+$ ]] || [[ "$2" -eq 0 ]]; then | ||
| log_error "--interval must be a positive integer (minutes), got: $2" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
To enhance defense-in-depth against potential injection attacks when parsing numeric data from untrusted sources, it's crucial to validate numeric fields. Similar to the validation for --concurrency, you can simplify this positive integer check by using a single regular expression that matches numbers starting from 1. This improves consistency, makes the validation logic more direct, and enhances robustness.
| if ! [[ "$2" =~ ^[0-9]+$ ]] || [[ "$2" -eq 0 ]]; then | |
| log_error "--interval must be a positive integer (minutes), got: $2" | |
| return 1 | |
| fi | |
| if ! [[ "$2" =~ ^[1-9][0-9]*$ ]]; then | |
| log_error "--interval must be a positive integer (minutes), got: $2" | |
| return 1 | |
| fi |
References
- When parsing delimited data from an untrusted source in a shell script, validate numeric fields before using them in calculations. This provides defense-in-depth against injection attacks that could result from delimiter shifting caused by malicious data.
Replace two-part positive integer check (^[0-9]+$ + -eq 0) with single regex ^[1-9][0-9]*$ that inherently rejects zero and non-numeric input. Addresses PR #4062 review feedback for defense-in-depth consistency.
Replace two-part positive integer check (^[0-9]+$ + -eq 0) with single regex ^[1-9][0-9]*$ that inherently rejects zero and non-numeric input. Addresses PR #4062 review feedback for defense-in-depth consistency.
1 participant
Summary
Fixes three critical/high-severity issues identified by CodeRabbit and Gemini in PR #1359 review:
SQL injection in
batch.sh—max_retries,concurrency,max_concurrency, andmax_load_factorwere interpolated directly into SQL INSERT statements without validation. Added regex validation (^[0-9]+$) at parse time, rejecting non-integer input before it reaches any SQL query.Input validation in
cron.sh— The--intervalparameter was used directly in cron/launchd commands without validation. Added positive integer validation. (Note: the GH_TOKEN exposure issue was already fixed in t1260.)Data loss in
database.sh— The t148 migrationINSERT...SELECTomitted thetriage_resultcolumn, causing silent data loss when migrating databases that had the column. Addedtriage_resultto both the INSERT column list and SELECT clause.Files changed (3 of 5 cap)
.agents/scripts/supervisor-archived/batch.sh— numeric validation for 4 SQL-interpolated variables.agents/scripts/supervisor-archived/cron.sh— interval validation.agents/scripts/supervisor-archived/database.sh— triage_result preserved in t148 migrationVerification
^[0-9]+$— rejects any non-digit characters including SQL injection payloadsCloses #3716