GH#4150, GH#3149, GH#4155: fix test harness, security posture output, and GraphQL injection#4165
Conversation
- assert_line_exists(): return 0 on failure (consistent with assert_contains) so the test harness runs all checks before reporting, instead of exiting on the first failure under set -e. The set +e wrapper in run_checks() mitigated this, but the function contract was still wrong. Closes marcusquinn#4150 - aidevops.sh: remove 2>/dev/null from security-posture-helper.sh invocation so users see detailed security findings during 'aidevops init' instead of only the pass/fail summary. Closes marcusquinn#3149
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 enhances the robustness of the test harness by standardizing error handling in assertion functions and improves the user experience during 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 two beneficial changes. First, it corrects the return code in the assert_line_exists test helper to ensure consistent behavior and prevent premature test suite termination. Second, it removes stderr suppression from the security posture assessment script, which will now correctly display detailed findings to the user. Both changes are well-reasoned and improve the tool's robustness and user experience. The implementation is correct, and I have no further suggestions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTest helpers now record failures without causing early exits; init reveals security-posture stderr; stats scripts gain repo-slug validation, parameterized GraphQL calls, redirected logs, and wording updates from "pulse" to "stats". Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Script (.agents/scripts/stats-functions.sh)
participant GH as gh CLI
participant GitHub as GitHub GraphQL API
participant Log as LOGFILE
rect rgba(0,128,255,0.5)
Script->>GH: call `gh api` with -F owner/-F name (parameterized query)
end
GH->>GitHub: send GraphQL request
GitHub-->>GH: return JSON response
GH->>Log: redirect stdout/stderr to LOGFILE
Script->>Script: call `_validate_repo_slug()` and sanitize fields
Script->>Log: write status / update health issue body (stats context)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…ions.sh - Add _validate_repo_slug() to reject path traversal and injection characters in repo slugs before they reach API paths - Parameterize GraphQL queries in _cleanup_stale_pinned_issues() using -F owner/name variables instead of string interpolation (prevents injection via double quotes in repo slug) - Parameterize GraphQL search queries for quality-debt counts using -F searchQuery variable instead of inline interpolation - Add slug validation gate in _get_runner_role() before API path construction - Redirect GraphQL stderr to LOGFILE instead of /dev/null for debuggability - Update auto-generated message references from pulse-wrapper.sh to stats-wrapper.sh to reflect the t1431 extraction Closes marcusquinn#4155
johnwaldo
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/scripts/stats-functions.sh (1)
657-662: Consider parameterizing the remaining GraphQL mutations for consistency.While the read queries are now parameterized, the mutation at lines 657-662 still uses string interpolation for
node_id:gh api graphql -f query=" mutation { unpinIssue(input: {issueId: \"${node_id}\"}) {The
node_idoriginates from GitHub's API response (line 651), so it's from a trusted source. However, for consistency with the new parameterized pattern and defense-in-depth, this could be refactored:♻️ Optional: Parameterize the mutation
- gh api graphql -f query=" - mutation { - unpinIssue(input: {issueId: \"${node_id}\"}) { - issue { number } - } - }" >/dev/null 2>&1 || true + gh api graphql -F nodeId="$node_id" -f query=' + mutation($nodeId: ID!) { + unpinIssue(input: {issueId: $nodeId}) { + issue { number } + } + }' >/dev/null 2>&1 || trueSimilar patterns exist at lines 301-306, 321-326, 685-690, and 987-992. This is a good-to-have refinement for a follow-up, not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stats-functions.sh around lines 657 - 662, The GraphQL mutation call using gh api graphql that invokes unpinIssue with inline string interpolation of "${node_id}" should be converted to a parameterized query like the other read queries: pass a separate variable (e.g., issueId) via -f variables='{"issueId":"'"$node_id"'"}' and update the GraphQL payload to use the $issueId variable in the mutation, modifying the gh api graphql invocation that currently contains unpinIssue(input: {issueId: \"${node_id}\"}) to accept and reference $issueId; apply the same parameterization pattern to the other similar mutation calls (the other gh api graphql invocations that inline node_id) for consistency and defense-in-depth.
🤖 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/stats-functions.sh:
- Around line 657-662: The GraphQL mutation call using gh api graphql that
invokes unpinIssue with inline string interpolation of "${node_id}" should be
converted to a parameterized query like the other read queries: pass a separate
variable (e.g., issueId) via -f variables='{"issueId":"'"$node_id"'"}' and
update the GraphQL payload to use the $issueId variable in the mutation,
modifying the gh api graphql invocation that currently contains
unpinIssue(input: {issueId: \"${node_id}\"}) to accept and reference $issueId;
apply the same parameterization pattern to the other similar mutation calls (the
other gh api graphql invocations that inline node_id) for consistency and
defense-in-depth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec01e302-3714-4d3f-9e1a-e33f73961f37
📒 Files selected for processing (1)
.agents/scripts/stats-functions.sh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.agents/scripts/stats-functions.sh (2)
114-118: Good fail-closed pattern for slug validation.Returning
"contributor"on invalid slugs correctly denies elevated permissions. However, the calling function_update_health_issue_for_repo()continues executing with the samerepo_slugin subsequentghcommands (lines 225, 238, 264, etc.) without knowing validation failed here.Since slugs originate from
repos.json(trusted configuration), the practical risk is minimal — invalid slugs would just produce harmless API 404s. For stronger defense-in-depth, consider validating at the entry point of_update_health_issue_for_repo()and returning early on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stats-functions.sh around lines 114 - 118, The _validate_repo_slug() check currently returns "contributor" on invalid slugs but allows callers like _update_health_issue_for_repo() to continue using the same repo_slug; add an early validation at the start of _update_health_issue_for_repo() that calls _validate_repo_slug "$repo_slug" and returns immediately (or exits with the same safe fallback) if validation fails so subsequent gh API calls are not attempted with an invalid slug; reference the existing _validate_repo_slug and the repo_slug parameter in your change.
686-691: Consider parameterizing remaining GraphQL mutations for consistency.Several GraphQL mutations still use string interpolation for
issueId(lines 301-306, 321-326, 658-663, 686-691, 988-993). While thesenode_idvalues originate from GitHub API responses rather than user input, parameterizing them would complete the security hardening effort and provide defense-in-depth.Example refactor for the mutation at line 686:
♻️ Optional: Parameterize unpinIssue mutation
- gh api graphql -f query=" - mutation { - unpinIssue(input: {issueId: \"${issue_node_id}\"}) { - issue { number } - } - }" >/dev/null 2>&1 || true + gh api graphql -F nodeId="$issue_node_id" -f query=' + mutation($nodeId: ID!) { + unpinIssue(input: {issueId: $nodeId}) { + issue { number } + } + }' >/dev/null 2>&1 || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/stats-functions.sh around lines 686 - 691, The gh api graphql call that embeds "${issue_node_id}" directly in the mutation (the unpinIssue mutation using the gh api graphql command) should be changed to a parameterized GraphQL query: move the issueId into a GraphQL variable (e.g., $issueId) in the mutation string and pass the value via gh's -f flag (e.g., -f issueId="$issue_node_id") instead of interpolating the variable into the query; update the mutation string used in the unpinIssue call and apply the same pattern to the other occurrences that interpolate node IDs to complete the hardening.
🤖 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/stats-functions.sh:
- Around line 114-118: The _validate_repo_slug() check currently returns
"contributor" on invalid slugs but allows callers like
_update_health_issue_for_repo() to continue using the same repo_slug; add an
early validation at the start of _update_health_issue_for_repo() that calls
_validate_repo_slug "$repo_slug" and returns immediately (or exits with the same
safe fallback) if validation fails so subsequent gh API calls are not attempted
with an invalid slug; reference the existing _validate_repo_slug and the
repo_slug parameter in your change.
- Around line 686-691: The gh api graphql call that embeds "${issue_node_id}"
directly in the mutation (the unpinIssue mutation using the gh api graphql
command) should be changed to a parameterized GraphQL query: move the issueId
into a GraphQL variable (e.g., $issueId) in the mutation string and pass the
value via gh's -f flag (e.g., -f issueId="$issue_node_id") instead of
interpolating the variable into the query; update the mutation string used in
the unpinIssue call and apply the same pattern to the other occurrences that
interpolate node IDs to complete the hardening.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f7ba11d-0a84-4ed1-8b9f-f1450c74678e
📒 Files selected for processing (1)
.agents/scripts/stats-functions.sh
2 participants
Summary
assert_line_exists()returning 1 on failure, causing early exit underset -e— now returns 0 consistent withassert_contains()2>/dev/nullfromsecurity-posture-helper.shinvocation inaidevops initso users see detailed security findingsstats-functions.shvia parameterized queries and slug validationDetails
Test harness (GH#4150):
assert_line_exists()returned 1 on failure whileassert_contains()returned 0. Both usefail()which returns 0 — the intent is to collect all failures and report at the end. Fixed to return 0 matchingassert_contains().Security posture (GH#3149):
bash "$security_posture_script" store "$project_root" 2>/dev/nullsuppressed all stderr, hiding detailed security findings duringaidevops init. The exit code is already checked — removing2>/dev/nulllets the detailed report through.GraphQL injection (GH#4155, 5 findings):
_validate_repo_slug()— rejects path traversal, quotes, and injection characters before API use_cleanup_stale_pinned_issues()GraphQL query using-F owner/-F namevariables instead of string interpolation-F searchQueryvariable_get_runner_role()before API path construction$LOGFILEinstead of/dev/nullfor debuggabilitypulse-wrapper.shtostats-wrapper.sh(t1431 extraction)Closes #4150
Closes #3149
Closes #4155
Summary by CodeRabbit
Tests
Chores