fix: address Gemini code review feedback on profile README generation#3963
fix: address Gemini code review feedback on profile README generation#3963marcusquinn merged 2 commits intomainfrom
Conversation
- Consolidate 4 separate jq calls for user profile into single pass with tab-delimited output, remove stderr suppression (Gemini #6) - Consolidate own repos loop into single jq pass, eliminating per-row jq invocations (Gemini #2) - Replace sequential N+1 gh api calls for fork parent URLs with parallel xargs -P 6 batch fetch (Gemini #1) - Add _sanitize_md() and _sanitize_url() helpers to sanitize user-controlled fields (display_name, bio, blog, twitter) before embedding in markdown, preventing markdown injection and javascript: URI attacks (Gemini #4, #5) Ref: PR #3962 review comments from gemini-code-assist
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 feedback from a Gemini Code Assist review, focusing on enhancing 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
|
|
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 (1)
WalkthroughThe script now sanitizes markdown and URL inputs to prevent breaking characters, consolidates multiple jq invocations into single batch operations for efficiency, and implements parallel API calls (up to 6 concurrent) when processing fork contributions. Repository data gathering is refactored from per-item loops to bulk jq processing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: Mon Mar 9 05:45:39 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![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 significantly improves the performance and security of the profile README generation script by consolidating jq calls, parallel fetching of API data, and introducing sanitization functions for user-controlled input. However, the current sanitization logic has gaps, including a loose URL validation regex that allows for markdown injection, vulnerability to field-shifting attacks due to tab delimiters for user-controlled data, and unsanitized embedding of user-controlled fields like repository names and descriptions in the markdown output. Additionally, issues were identified concerning error suppression, which has been clarified with relevant rules, and a small formatting bug.
| fork_details=$(echo "$fork_names" | xargs -P 6 -I{} gh api "repos/${gh_user}/{}" --jq ' | ||
| "\(.name)\t\(.description // "No description")\t\(.parent.html_url // .html_url)" | ||
| ' 2>/dev/null || true) |
There was a problem hiding this comment.
While the parallel fetching of fork details is a great performance improvement, redirecting stderr to /dev/null is generally discouraged as it can hide important errors. For instance, if gh api fails due to authentication issues, the error message will be suppressed, making it difficult to debug why the contributions section might be empty. It's better to allow these errors to be visible for debugging purposes. The || true already prevents the script from exiting on failure if set -e is enabled.
| fork_details=$(echo "$fork_names" | xargs -P 6 -I{} gh api "repos/${gh_user}/{}" --jq ' | |
| "\(.name)\t\(.description // "No description")\t\(.parent.html_url // .html_url)" | |
| ' 2>/dev/null || true) | |
| fork_details=$(echo "$fork_names" | xargs -P 6 -I{} gh api "repos/${gh_user}/{}" --jq ' | |
| "\(.name)\t\(.description // "No description")\t\(.parent.html_url // .html_url)" | |
| ' || true) |
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.
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| _sanitize_url() { | ||
| local url="$1" | ||
| # Only allow http:// and https:// schemes | ||
| if [[ "$url" =~ ^https?:// ]]; then |
There was a problem hiding this comment.
The _sanitize_url function uses a loose regex ^https?:// to validate URLs. It only checks if the string starts with http:// or https://, but does not validate the rest of the string. An attacker can append markdown-breaking characters (like )) followed by malicious markdown or javascript: URIs. For example, a blog URL like https://example.com) [click me](javascript:alert(1)) would pass this check and result in an injected link in the generated README.
| if [[ "$url" =~ ^https?:// ]]; then | |
| if [[ "$url" =~ ^https?://[^[:space:]\(\)\[\]]+$ ]]; then |
| IFS=$'\t' read -r display_name bio blog twitter < <( | ||
| echo "$user_json" | jq -r '[ | ||
| (.name // ""), | ||
| (.bio // ""), | ||
| (if .blog != null and .blog != "" then .blog else "" end), | ||
| (if .twitter_username != null and .twitter_username != "" then .twitter_username else "" end) | ||
| ] | join("\t")' || printf '\t\t\t\n' | ||
| ) |
There was a problem hiding this comment.
The script uses tab characters as delimiters when processing data from the GitHub API. Since fields like bio and description are user-controlled and can contain tabs, an attacker can inject tabs to shift fields and control variables like blog or rurl. This can be used to bypass sanitization or inject malicious URLs into the markdown output. Consider sanitizing user-controlled fields for the delimiter character (tab) before joining them in jq.
| local own_repos | ||
| own_repos=$(echo "$repos_json" | jq -r --arg user "$gh_user" ' | ||
| [.[] | select(.fork == false and .name != $user)] | | ||
| map("- **[\(.name)](\(.html_url))** -- \(.description // "No description")") | |
There was a problem hiding this comment.
Repository names and descriptions are not sanitized before being embedded in the markdown output. While the PR adds sanitization for display_name and bio, it misses these other user-controlled fields, allowing for markdown injection. An attacker could name a repository or set its description to include markdown that alters the layout of the profile or includes malicious links.
| map("- **[\(.name)](\(.html_url))** -- \(.description // "No description")") | | |
| map("- **[\(.name | gsub(\"[\\[\\]()]\"; \"\"))](\(.html_url))** -- \(.description // \"No description\" | gsub(\"[\\[\\]()]\"; \"\"))") | |
References
- Prefer using URLs provided directly in API responses (e.g., 'html_url', 'web_url') instead of manually constructing them via string concatenation to ensure robustness.
| ' 2>/dev/null || true) | ||
| while IFS=$'\t' read -r rname rdesc rurl; do | ||
| [[ -z "$rname" ]] && continue | ||
| contrib_repos="${contrib_repos}- **[${rname}](${rurl})** -- ${rdesc}"$'\n' |
There was a problem hiding this comment.
Repository names and descriptions for forks are not sanitized before being embedded in the markdown output. This allows for markdown injection from user-controlled repository data.
| contrib_repos="${contrib_repos}- **[${rname}](${rurl})** -- ${rdesc}"$'\n' | |
| contrib_repos="${contrib_repos}- **[$(_sanitize_md "${rname}")](${rurl})** -- $(_sanitize_md "${rdesc}")"$'\n' |
References
- Prefer using URLs provided directly in API responses (e.g., 'html_url', 'web_url') instead of manually constructing them via string concatenation to ensure robustness.
| echo "## Projects" | ||
| echo "" | ||
| printf '%s' "$own_repos" | ||
| printf '%s\n' "$own_repos" |
There was a problem hiding this comment.
The jq command used to populate $own_repos already produces output with a trailing newline. Using printf '%s\n' adds an extra newline, which will result in an unwanted blank line in the generated README file after the list of projects. The original printf '%s' was correct in this context.
| printf '%s\n' "$own_repos" | |
| printf '%s' "$own_repos" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.agents/scripts/profile-readme-helper.sh (2)
665-677: Consider case-insensitive scheme matching.The regex
^https?://is case-sensitive. While rare, URLs with uppercase schemes (e.g.,HTTPS://example.com) would be rejected and return empty.♻️ Optional: Add case-insensitivity
_sanitize_url() { local url="$1" + # Convert to lowercase for scheme check + local url_lower="${url,,}" # Only allow http:// and https:// schemes - if [[ "$url" =~ ^https?:// ]]; then + if [[ "$url_lower" =~ ^https?:// ]]; then echo "$url" elseNote:
${url,,}requires bash 4.0+. If bash 3.2 compatibility is needed, use$(echo "$url" | tr '[:upper:]' '[:lower:]')for the check only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/profile-readme-helper.sh around lines 665 - 677, The scheme check in _sanitize_url uses a case-sensitive regex (^https?://) which will reject uppercase schemes like HTTPS://; update the validation to perform a case-insensitive match (or normalize the scheme to lowercase before testing) so _sanitize_url accepts any capitalization of http/https, e.g. use a case-insensitive regex or convert "$url" to lowercase for the check while still returning the original URL on success.
707-707: Consider adding--paginatefor users with >100 repos.The API call uses
per_page=100but no pagination. Users with more than 100 repositories would have incomplete project/contribution lists. Sibling scripts in this repo (e.g.,review-bot-gate-helper.sh) use--paginatefor API calls returning lists.♻️ Add pagination support
- repos_json=$(gh api "users/${gh_user}/repos?per_page=100&sort=updated") || repos_json="[]" + repos_json=$(gh api "users/${gh_user}/repos?per_page=100&sort=updated" --paginate) || repos_json="[]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/profile-readme-helper.sh at line 707, The gh API call that populates repos_json currently requests per_page=100 without pagination, so users with >100 repos will be missing results; update the call used to set repos_json (the gh api invocation that currently builds "users/${gh_user}/repos?per_page=100&sort=updated") to use gh's --paginate option (or otherwise loop through pages) so all repositories are retrieved instead of only the first 100; ensure any error fallback (repos_json="[]") and subsequent code that consumes repos_json continue to work with the paginated output.
🤖 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/profile-readme-helper.sh:
- Around line 739-743: The parallel fetch for fork_details currently swallows
all API errors via "2>/dev/null || true" and doesn't sanitize .description;
update the gh api call (repos/${gh_user}/{}) to capture and surface errors
instead of redirecting stderr to /dev/null (e.g., log failures to stderr or
collect stderr to a temp file while allowing the loop to continue) and remove
the "|| true" swallow so failures are visible; also apply the same markdown/HTML
sanitization used in the own_repos section to the .description field produced by
the jq expression (the ".description" in the jq output) to prevent markdown
injection.
- Around line 726-732: The repo descriptions in the own_repos generation are not
sanitized, allowing markdown injection; update the jq filter used in the
own_repos assignment (the jq call that reads repos_json with --arg user
"$gh_user") to sanitize .description before embedding it (e.g. replace or escape
characters that break Markdown links like "]", "(", ")" and backticks using jq's
gsub or a small sanitize filter), and keep the fallback to "No description"
(i.e., map("- **[\(.name)](\(.html_url))** -- \(.description // \"No
description\")") should become something like mapping that applies the sanitize
filter to .description so the output is safe). Ensure you modify the own_repos
variable generation and keep gh_user and repos_json usage unchanged.
---
Nitpick comments:
In @.agents/scripts/profile-readme-helper.sh:
- Around line 665-677: The scheme check in _sanitize_url uses a case-sensitive
regex (^https?://) which will reject uppercase schemes like HTTPS://; update the
validation to perform a case-insensitive match (or normalize the scheme to
lowercase before testing) so _sanitize_url accepts any capitalization of
http/https, e.g. use a case-insensitive regex or convert "$url" to lowercase for
the check while still returning the original URL on success.
- Line 707: The gh API call that populates repos_json currently requests
per_page=100 without pagination, so users with >100 repos will be missing
results; update the call used to set repos_json (the gh api invocation that
currently builds "users/${gh_user}/repos?per_page=100&sort=updated") to use gh's
--paginate option (or otherwise loop through pages) so all repositories are
retrieved instead of only the first 100; ensure any error fallback
(repos_json="[]") and subsequent code that consumes repos_json continue to work
with the paginated output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5226320c-0f8b-4fc5-a1cb-a4660b1cad3f
📒 Files selected for processing (1)
.agents/scripts/profile-readme-helper.sh
- Remove 2>/dev/null from xargs fork fetch (|| true suffices) - Tighten _sanitize_url to reject markdown-breaking chars in URLs using glob patterns (bash regex [^...] with escaped parens is unreliable across bash versions) - Strip tabs/newlines from jq user profile output to prevent tab-delimiter injection in bio/description fields - Sanitize repo names and descriptions in both own repos (jq gsub) and fork repos (_sanitize_md) before markdown embedding - Keep printf '%s\n' for own_repos (bash $() strips trailing newlines, so the explicit \n is needed for section spacing) Ref: PR #3963 review comments from gemini-code-assist
🔍 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: Mon Mar 9 05:53:54 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
#3963 review feedback Fixes remaining findings from coderabbit and gemini reviewers: - Add backtick stripping to jq gsub sanitization for own_repos names and descriptions (markdown injection via backticks) - Add backtick stripping to fork_details jq sanitization for consistency - Fix jq operator precedence: wrap (.description // "No description") in parens so gsub applies to the actual description, not just the fallback - Revert printf '%s\n' to printf '%s' for own_repos output (jq already includes trailing newline; extra newline caused blank line in README) - Make _sanitize_url scheme check case-insensitive (accepts HTTPS://) - Add --paginate to gh api repos fetch for users with >100 repos - Validate fork parent URLs with _sanitize_url before markdown embedding Closes #3984
…iew feedback (#4034) * fix: address remaining quality-debt in profile-readme-helper.sh from PR #3963 review feedback Fixes remaining findings from coderabbit and gemini reviewers: - Add backtick stripping to jq gsub sanitization for own_repos names and descriptions (markdown injection via backticks) - Add backtick stripping to fork_details jq sanitization for consistency - Fix jq operator precedence: wrap (.description // "No description") in parens so gsub applies to the actual description, not just the fallback - Revert printf '%s\n' to printf '%s' for own_repos output (jq already includes trailing newline; extra newline caused blank line in README) - Make _sanitize_url scheme check case-insensitive (accepts HTTPS://) - Add --paginate to gh api repos fetch for users with >100 repos - Validate fork parent URLs with _sanitize_url before markdown embedding Closes #3984 * fix: suppress SC2016 for intentional backtick in jq gsub pattern
1 participant
Summary
Addresses all actionable findings from Gemini Code Assist review on PR #3962.
Changes
Performance
jqcalls → single pass with tab-delimited output; removed2>/dev/nullstderr suppression so jq errors are visiblejqinvocations in while loop; singlejqpass generates all markdown linesgh apicalls withxargs -P 6for up to 6x speedup on users with many forksSecurity
_sanitize_md()helper — strips markdown-breaking characters ([]()and backticks) from user-controlled fields (display_name, bio)_sanitize_url()helper — rejectsjavascript:,data:, and non-http(s) schemes from blog URLsGemini findings addressed
xargs -P 6batch fetch_sanitize_url()rejects non-http(s)_sanitize_md()strips breaking charsTesting
johnwaldo(no blog/twitter) — correct output, parent URLs resolvedmarcusquinn(has blog + twitter) — correct sanitization and renderingSummary by CodeRabbit
Bug Fixes
Refactor