fix: security hardening — path traversal, injection, jq filter bug (batch 3)#3877
Conversation
…njection, JSON injection, jq filter bug Security fixes: - marcusquinn#3152: worker-token-helper.sh — validate --token-file path within TOKEN_DIR to prevent path traversal; validate --ttl is numeric to prevent bash arithmetic injection; remove 2>/dev/null from strategy fallback calls - marcusquinn#3179: ai-judgment-helper.sh — use awk -v for safe variable passing (prevents command injection via threshold); use jq for JSON construction (prevents JSON injection via LLM output) - marcusquinn#3240: stuck-detection-helper.sh — fix jq filter to match on both issue number AND repo slug (prevents cross-repo data corruption); use awk -v for safe variable passing; remove 2>/dev/null from gh issue edit calls
|
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 (2)
WalkthroughRefactors in three shell helper scripts: JSON construction moved to jq with typed fields, silent error redirections removed to allow error propagation, and worker-token inputs and file paths validated to prevent injection and path traversal. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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 several critical agent scripts by addressing multiple vulnerabilities. It introduces robust input validation and safer data handling practices to prevent common attack vectors such as path traversal, command injection, and JSON injection, while also rectifying a logic error that could lead to cross-repository data corruption. The changes aim to make these scripts more resilient and secure against malicious inputs and incorrect state management. 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 introduces several important security hardening fixes across three scripts, addressing vulnerabilities like path traversal, command injection, and data corruption. The changes are well-implemented, using safer alternatives like awk -v and jq --arg for passing variables and jq for robust JSON construction. I've identified a few minor issues related to duplicated code and incorrect indentation, which are detailed in the comments. Overall, this is a great set of security improvements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.agents/scripts/worker-token-helper.sh (1)
370-379: Drop the duplicated TTL guard.Line 370 through Line 379 repeat the same regex check and error path. Keeping a single validation block avoids drift the next time this rule changes.
Proposed cleanup
--ttl | -t) ttl="$2" # Validate TTL is numeric to prevent arithmetic injection if ! [[ "$ttl" =~ ^[0-9]+$ ]]; then log_token "ERROR" "TTL must be a positive integer: ${ttl}" return 1 fi - # Validate TTL is numeric to prevent arithmetic injection - if ! [[ "$ttl" =~ ^[0-9]+$ ]]; then - log_token "ERROR" "TTL must be a positive integer: ${ttl}" - return 1 - fi if ((ttl > MAX_TTL)); then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/worker-token-helper.sh around lines 370 - 379, The script contains a duplicated TTL numeric-guard block using the variable ttl and calling log_token on failure; remove the redundant second validation so only one check remains: keep the single if [[ "$ttl" =~ ^[0-9]+$ ]] { ... } block that logs via log_token "ERROR" "TTL must be a positive integer: ${ttl}" and returns 1, and delete the repeated identical lines to avoid duplication and future drift.
🤖 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/ai-judgment-helper.sh:
- Around line 908-911: The JSON output is being pretty-printed by jq -n which
breaks the JSONL one-record-per-line contract; update the jq invocations that
build result JSON (e.g., where result_json is assigned and the other jq -n usage
that emits evaluator records used by eval_dataset) to use compact mode by
changing jq -n to jq -cn so each record is emitted as a single-line JSON object,
preserving the existing use of --arg/--argjson for fields like eval_type, score,
passed and details.
In @.agents/scripts/stuck-detection-helper.sh:
- Around line 344-349: The jq deduplication is wrong and a duplicate --arg is
present: in the pipeline that builds new_state from state (using variables
issue_number, repo_slug, now) change unique_by(.issue + .repo) to
unique_by([.issue, .repo]) so uniqueness is based on the tuple rather than
string concatenation, and remove the duplicated --arg repo "$repo_slug" argument
so --arg repo is only passed once.
In @.agents/scripts/worker-token-helper.sh:
- Around line 529-538: The current realpath call on the token file can fail if
the file is already deleted; change the check to canonicalize the parent
directory instead: compute parent_dir=$(dirname "$token_file") and realpath that
(realpath "$parent_dir") and verify it starts with ${TOKEN_DIR}, and separately
validate the filename via basename to ensure it contains no directory
components; keep using log_token for errors and preserve the existing -f guards
in the cleanup logic so deletion remains safe and idempotent.
---
Nitpick comments:
In @.agents/scripts/worker-token-helper.sh:
- Around line 370-379: The script contains a duplicated TTL numeric-guard block
using the variable ttl and calling log_token on failure; remove the redundant
second validation so only one check remains: keep the single if [[ "$ttl" =~
^[0-9]+$ ]] { ... } block that logs via log_token "ERROR" "TTL must be a
positive integer: ${ttl}" and returns 1, and delete the repeated identical lines
to avoid duplication and future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2311bd43-8d71-4b3b-b53e-d499c026a7a3
📒 Files selected for processing (3)
.agents/scripts/ai-judgment-helper.sh.agents/scripts/stuck-detection-helper.sh.agents/scripts/worker-token-helper.sh
|
This PR is from an external contributor (@johnwaldo). Auto-merge is disabled for external PRs — a maintainer must review and merge manually. |
marcusquinn
added
the
external-contributor
…idempotent revoke - Remove duplicated TTL validation block in worker-token-helper.sh - Use jq -cn (compact) instead of jq -n to maintain JSONL contract - Fix indentation on fallback_json block in ai-judgment-helper.sh - Remove duplicate --arg repo in stuck-detection-helper.sh - Fix unique_by(.issue + .repo) → unique_by([.issue, .repo]) to prevent string concatenation collisions (e.g. issue 12 + repo 3/x vs issue 1 + repo 23/x) - Fix cmd_revoke realpath to resolve parent dir instead of token file, so cleanup works when only .meta remains (token already deleted)
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/stuck-detection-helper.sh (1)
269-277:⚠️ Potential issue | 🟠 MajorValidate confidence as numeric before threshold comparison to prevent string comparison semantics.
Line 271 uses
awkto compare confidence against threshold, but awk treats non-numeric strings as strings. Whenconfidenceis a non-numeric value (e.g.,"high","foo"), the comparison becomes a lexicographic comparison, causing strings that appear "greater than" the threshold to spuriously return 1 and trigger label/comment operations.The repro confirms the issue:
0.8 -> 1 ✓ correct 0.6 -> 0 ✓ correct high -> 1 ✗ WRONG (string "high" >= "0.7") foo -> 1 ✗ WRONG (string "foo" >= "0.7")Add numeric validation before the awk comparison:
Suggested fix
# Check confidence threshold local above_threshold + if ! [[ "$confidence" =~ ^([0-9]+([.][0-9]+)?|[.][0-9]+)$ ]] || \ + ! [[ "$STUCK_CONFIDENCE_THRESHOLD" =~ ^([0-9]+([.][0-9]+)?|[.][0-9]+)$ ]]; then + _sd_log_error "confidence values must be numeric (got confidence=${confidence}, threshold=${STUCK_CONFIDENCE_THRESHOLD})" + return 1 + fi above_threshold=$(awk -v c="$confidence" -v t="$STUCK_CONFIDENCE_THRESHOLD" 'BEGIN { print (c >= t) ? 1 : 0 }') || above_threshold="0"This aligns with the coding guideline requirement for automation scripts: Reliability and robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stuck-detection-helper.sh around lines 269 - 277, The confidence variable must be validated as a numeric value before running the awk threshold comparison to avoid lexicographic comparisons; modify the block around above_threshold (and the STUCK_CONFIDENCE_THRESHOLD check) to first test confidence with a numeric regex (e.g., allow integers or decimals) and if it fails, set a safe numeric default (0) or treat it as below threshold and log a warning via _sd_log_info; then run the existing awk comparison to compute above_threshold and proceed to call _sd_record_milestone/return as before.
🤖 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/worker-token-helper.sh:
- Around line 445-454: The token validation in cmd_validate resolves token_file
with realpath but compares it to the raw TOKEN_DIR string, which can fail when
TOKEN_DIR contains symlinks; update cmd_validate to canonicalize TOKEN_DIR
(e.g., compute a TOKEN_DIR_REAL via realpath or similar) and compare real_path
of token_file against that canonical TOKEN_DIR path (same approach used in
cmd_revoke), ensuring the check uses the canonicalized directory before
returning error.
---
Outside diff comments:
In @.agents/scripts/stuck-detection-helper.sh:
- Around line 269-277: The confidence variable must be validated as a numeric
value before running the awk threshold comparison to avoid lexicographic
comparisons; modify the block around above_threshold (and the
STUCK_CONFIDENCE_THRESHOLD check) to first test confidence with a numeric regex
(e.g., allow integers or decimals) and if it fails, set a safe numeric default
(0) or treat it as below threshold and log a warning via _sd_log_info; then run
the existing awk comparison to compute above_threshold and proceed to call
_sd_record_milestone/return as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edb72120-8811-4e49-8b86-c8002f879460
📒 Files selected for processing (3)
.agents/scripts/ai-judgment-helper.sh.agents/scripts/stuck-detection-helper.sh.agents/scripts/worker-token-helper.sh
…ze TOKEN_DIR in cmd_validate - stuck-detection-helper.sh: Add numeric regex validation for confidence and threshold before awk comparison to prevent string comparison semantics (e.g., "high" >= "0.7" is true lexicographically in awk) - worker-token-helper.sh: Canonicalize TOKEN_DIR via realpath in cmd_validate to match the pattern already used in cmd_revoke, preventing false rejections when TOKEN_DIR contains symlinks Addresses remaining CodeRabbit review feedback on PR marcusquinn#3877.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/stuck-detection-helper.sh (1)
264-283:⚠️ Potential issue | 🟠 MajorValidate all parameters before GitHub operations and state mutations.
The guard at line 264 checks
issue_number,milestone_min, andconfidence, but omitselapsed_min—yetelapsed_minis interpolated into the comment body at line 313. More critically,milestone_minis passed to_sd_record_milestone(line 276) which pipes it tojq --argjson ms "$milestone"at line 242; non-numeric values like"30m"causejqto exit with code 2, leaving GitHub labels applied but state unrecorded. Similarly,confidenceenters an awk comparison at line 270 without validation, risking lexicographic semantics if the variable contains non-numeric text.Add numeric validation before any side effects:
if [[ -z "$issue_number" || -z "$milestone_min" || -z "$elapsed_min" || -z "$confidence" ]]; then _sd_log_error "usage: label-stuck <issue_number> <milestone_min> <elapsed_min> <confidence> <reasoning> <suggested_actions> [--repo <slug>]" return 1 fi + if ! [[ "$milestone_min" =~ ^[0-9]+$ ]] || ! [[ "$elapsed_min" =~ ^[0-9]+$ ]]; then + _sd_log_error "milestone_min and elapsed_min must be positive integers (got milestone=${milestone_min}, elapsed=${elapsed_min})" + return 1 + fi + + if ! [[ "$confidence" =~ ^([0-9]+([.][0-9]+)?|[.][0-9]+)$ ]]; then + _sd_log_error "confidence must be numeric (got confidence=${confidence})" + return 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stuck-detection-helper.sh around lines 264 - 283, The guard currently omits validating elapsed_min and does not ensure milestone_min and confidence are numeric before performing GitHub ops or calling _sd_record_milestone; update the parameter validation at the start of label-stuck to: check that elapsed_min is provided, validate milestone_min and confidence (and STUCK_CONFIDENCE_THRESHOLD) match a numeric regex before any side effects, and return non-zero with an _sd_log_error if validation fails; ensure the check happens before any calls that mutate state (e.g., before invoking _sd_record_milestone and before any labeling/GitHub API calls) so functions like _sd_record_milestone and the awk comparison receive only numeric values.
🤖 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/stuck-detection-helper.sh:
- Around line 423-428: The jq expression that assigns new_state can fail when
.labeled_issues is missing; update the filter used in the new_state assignment
(the jq call using --arg key "$issue_key" --arg issue "$issue_number" --arg repo
"$repo_slug") to coalesce .labeled_issues to an empty array before iterating it
(for example using (.labeled_issues // []) or (.labeled_issues // [] ) ), e.g.
keep del(.milestones_checked[$key]) and set .labeled_issues = [(.labeled_issues
// [])[] | select(not (.issue == $issue and .repo == $repo))] so the jq call no
longer errors and the || true masking is unnecessary for this case.
---
Outside diff comments:
In @.agents/scripts/stuck-detection-helper.sh:
- Around line 264-283: The guard currently omits validating elapsed_min and does
not ensure milestone_min and confidence are numeric before performing GitHub ops
or calling _sd_record_milestone; update the parameter validation at the start of
label-stuck to: check that elapsed_min is provided, validate milestone_min and
confidence (and STUCK_CONFIDENCE_THRESHOLD) match a numeric regex before any
side effects, and return non-zero with an _sd_log_error if validation fails;
ensure the check happens before any calls that mutate state (e.g., before
invoking _sd_record_milestone and before any labeling/GitHub API calls) so
functions like _sd_record_milestone and the awk comparison receive only numeric
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b59465a8-23b7-4464-bc8b-0fc5a609c26f
📒 Files selected for processing (2)
.agents/scripts/stuck-detection-helper.sh.agents/scripts/worker-token-helper.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/scripts/worker-token-helper.sh
…gainst missing state Address remaining CodeRabbit CHANGES_REQUESTED review feedback on PR marcusquinn#3877: - Add elapsed_min to required parameter check in cmd_label_stuck - Validate milestone_min and elapsed_min as positive integers before any GitHub operations or state mutations (prevents jq --argjson failures and comment interpolation of non-numeric values) - Guard cmd_label_clear jq filter against missing .labeled_issues key using null coalescing (// []) to prevent 'Cannot iterate over null' errors that were silently masked by || true
- ai-judgment-helper: use jq -cn for compact one-record-per-line output - stuck-detection-helper: validate elapsed_min/milestone_min/confidence as numeric - stuck-detection-helper: fix unique_by string concat collision, use tuple key - stuck-detection-helper: handle missing .labeled_issues key in clear filter - worker-token-helper: resolve TOKEN_DIR with realpath for symlink consistency - worker-token-helper: handle missing token file gracefully in cmd_revoke
|
@coderabbitai review All review feedback addressed:
Please re-review. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
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/stuck-detection-helper.sh (1)
149-158:⚠️ Potential issue | 🟠 MajorUse a symlink-safe temp file for state writes.
"${state_file}.tmp.$$"is predictable, so a pre-created symlink inSUPERVISOR_DIRcan redirect the write into another file. Also,mv ... 2>/dev/nullstill hides the real failure reason in the atomic-write path.Suggested fix
- local tmp_file="${state_file}.tmp.$$" - if ! printf '%s\n' "$state_json" >"$tmp_file"; then + local tmp_file + tmp_file=$(mktemp "$(dirname "$state_file")/$(basename "$state_file").tmp.XXXXXX") || { + _sd_log_warn "failed to allocate temp state file" + return 1 + } + if ! printf '%s\n' "$state_json" >"$tmp_file"; then _sd_log_warn "failed to write temp state file: $tmp_file" rm -f "$tmp_file" 2>/dev/null || true return 1 fi - if ! mv -f "$tmp_file" "$state_file" 2>/dev/null; then + if ! mv -f "$tmp_file" "$state_file"; then _sd_log_warn "failed to move temp state to: $state_file" rm -f "$tmp_file" 2>/dev/null || true return 1 fiAs per coding guidelines, ".agents/scripts/*.sh: Automation scripts - focus on: Reliability and robustness" and "Clear logging and feedback".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stuck-detection-helper.sh around lines 149 - 158, Replace the predictable temp filename logic that sets tmp_file="${state_file}.tmp.$$" with a symlink-safe mktemp call that creates the temp file in the same directory as state_file (e.g., use mktemp --tmpdir="$(dirname "$state_file")" or mktemp "$(dirname "$state_file")/tmp.XXXXXX"), and ensure the created tmp_file is cleaned up on failures; also stop silencing mv errors—capture mv's stderr when moving tmp_file to state_file and pass that error text to _sd_log_warn (include the state_file and captured error), then remove tmp_file and return 1. Reference: tmp_file, state_file, and _sd_log_warn.
🧹 Nitpick comments (2)
.agents/scripts/worker-token-helper.sh (1)
370-378: Solid arithmetic injection guard.The regex validation ensures TTL contains only digits before any arithmetic evaluation, effectively blocking injection payloads like
1; rm -rf /.One edge-case nitpick: bash arithmetic interprets leading zeros as octal (e.g.,
010→ 8 decimal). If you want explicit decimal interpretation regardless of input format, you could use10#$ttlin the comparison. This is quite minor since users rarely pass octal-formatted TTLs.,
Optional: force decimal interpretation
- if ((ttl > MAX_TTL)); then + if ((10#$ttl > MAX_TTL)); then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/worker-token-helper.sh around lines 370 - 378, The comparison against MAX_TTL can misinterpret inputs with leading zeros as octal; update the arithmetic check that uses ttl (and subsequent assignment) to force decimal interpretation by applying the bash base prefix (e.g., use 10#$ttl) when evaluating ttl in the (( ttl > MAX_TTL )) test and any arithmetic context involving ttl (refer to the ttl variable and the MAX_TTL comparison in the worker-token-helper.sh snippet and keep the existing regex validation and log_token calls intact)..agents/scripts/stuck-detection-helper.sh (1)
291-295: Clarify exit code semantics: "skipped" paths should use distinct status.The
label-stuckcommand returns 0 in three distinct scenarios: confidence below threshold (line 295), GitHub skipped viaSD_SKIP_GITHUB=true(line 317), and successful labeling (line 372). This ambiguity means future callers cannot distinguish between "checked and skipped" versus "checked and labeled"—currently no active callers exist, but integrating this helper without a separate exit code for skipped scenarios will create the misreporting risk described. Consider using exit code 2 (or similar) for the "checked but skipped" paths to keep the contract unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stuck-detection-helper.sh around lines 291 - 295, The "label-stuck" script currently returns 0 for both "checked but skipped" and "checked and labeled" outcomes; change the skipped-return semantics so callers can distinguish them by returning a non-zero but distinct code (e.g., 2) for skipped paths. Specifically, update the branch that logs "confidence ... below threshold" (the block using _sd_log_info, _sd_record_milestone and currently `return 0`) to `return 2`, and likewise update the SD_SKIP_GITHUB path (the branch checking SD_SKIP_GITHUB=true) to return the same distinct code; keep the successful labeling/normal completion return as 0 so success remains unchanged. Ensure the referenced helpers (_sd_log_info, _sd_record_milestone) remain called as before and only the return values change.
🤖 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/stuck-detection-helper.sh:
- Around line 351-354: The gh issue comment failure handler currently calls
_sd_log_warn which exits 0, so failures are swallowed; update the handler after
the gh issue comment invocation to propagate a non-zero status (e.g., change the
failure branch to call _sd_log_warn and then exit 1 or return 1 depending on
whether this code runs in the main script or inside a function) so that the gh
issue comment failure is visible to the caller; target the gh issue comment
invocation and its failure block and ensure it returns/ exits non-zero instead
of relying on _sd_log_warn's current behavior.
---
Outside diff comments:
In @.agents/scripts/stuck-detection-helper.sh:
- Around line 149-158: Replace the predictable temp filename logic that sets
tmp_file="${state_file}.tmp.$$" with a symlink-safe mktemp call that creates the
temp file in the same directory as state_file (e.g., use mktemp
--tmpdir="$(dirname "$state_file")" or mktemp "$(dirname
"$state_file")/tmp.XXXXXX"), and ensure the created tmp_file is cleaned up on
failures; also stop silencing mv errors—capture mv's stderr when moving tmp_file
to state_file and pass that error text to _sd_log_warn (include the state_file
and captured error), then remove tmp_file and return 1. Reference: tmp_file,
state_file, and _sd_log_warn.
---
Nitpick comments:
In @.agents/scripts/stuck-detection-helper.sh:
- Around line 291-295: The "label-stuck" script currently returns 0 for both
"checked but skipped" and "checked and labeled" outcomes; change the
skipped-return semantics so callers can distinguish them by returning a non-zero
but distinct code (e.g., 2) for skipped paths. Specifically, update the branch
that logs "confidence ... below threshold" (the block using _sd_log_info,
_sd_record_milestone and currently `return 0`) to `return 2`, and likewise
update the SD_SKIP_GITHUB path (the branch checking SD_SKIP_GITHUB=true) to
return the same distinct code; keep the successful labeling/normal completion
return as 0 so success remains unchanged. Ensure the referenced helpers
(_sd_log_info, _sd_record_milestone) remain called as before and only the return
values change.
In @.agents/scripts/worker-token-helper.sh:
- Around line 370-378: The comparison against MAX_TTL can misinterpret inputs
with leading zeros as octal; update the arithmetic check that uses ttl (and
subsequent assignment) to force decimal interpretation by applying the bash base
prefix (e.g., use 10#$ttl) when evaluating ttl in the (( ttl > MAX_TTL )) test
and any arithmetic context involving ttl (refer to the ttl variable and the
MAX_TTL comparison in the worker-token-helper.sh snippet and keep the existing
regex validation and log_token calls intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a36c1c74-c0c7-44a6-997a-b1be80ed69b8
📒 Files selected for processing (2)
.agents/scripts/stuck-detection-helper.sh.agents/scripts/worker-token-helper.sh
Capture gh issue comment failure status and return it after state recording completes. State is always recorded regardless of comment success (since the label was already applied), preventing re-labeling on the next check cycle.
|
@coderabbitai review Addressed the remaining review comment:
All other review comments (duplicate TTL validation, indentation, duplicate --arg, jq -cn, unique_by tuple, realpath canonicalization, labeled_issues null guard) were already addressed in prior commits. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Dispatching worker to address review feedback.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Worker killed after 3h34m with 2 commits (struggle_ratio: 19996).
|
|
Dispatching fix worker to address 7 unresolved bot review suggestions.
|
Review Bot Suggestions — Resolution SummaryAll CodeRabbit and Gemini Code Assist findings have been addressed. Here's the status of each: Already Fixed (prior commits)
Intentionally Not Applied
All inline comments have been replied to individually. |
2 participants
Summary
Security hardening fixes for 3 scripts addressing path traversal, command injection, and data corruption vulnerabilities.
Issues Fixed
worker-token-helper.sh--token-filepath is withinTOKEN_DIR(prevents path traversal + arbitrary file deletion); validate--ttlis numeric (prevents bash arithmetic injection); remove2>/dev/nullfrom strategy fallback callsai-judgment-helper.shawk -vfor safe variable passing (prevents command injection viathreshold); usejqfor JSON construction (prevents JSON injection via LLM output)stuck-detection-helper.shawk -vfor safe variable passing; remove2>/dev/nullfromgh issue editcallsVulnerability Details
--token-fileaccepted any path — attacker could overwrite/delete arbitrary files viadd if=/dev/urandom+rm -f--ttlvalue used in(( ))without validation — bash evaluates expressions likea[$(id>&2)0]thresholdCLI arg interpolated into awk string —0.7; system("id")executes arbitrary commandsdetailsfield could manipulate JSON structureCloses #3152, closes #3240
Partially addresses #3179 (critical and high findings fixed; medium findings remain)
Summary by CodeRabbit