docs(ci): pass untrusted PR fields via env to prevent script injection#16
docs(ci): pass untrusted PR fields via env to prevent script injection#16chethanuk wants to merge 1 commit into
Conversation
The CI integration docs interpolated github.event.pull_request.title,
github.base_ref and github.head_ref directly inside shell run: blocks.
GitHub substitutes ${{ }} textually before the shell parses the line, so a
PR title or branch name containing shell metacharacters executes on the
runner of anyone who copies the snippet.
Hoist all three into env: mappings and reference them as shell variables,
matching action.yml:228-244 and the repo's own review rule at
internal/config/rules/rule_docs/github_workflows.md:6.
Applied identically to en, ja and zh; the three code blocks were
byte-identical before and remain so.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughGitHub Actions CI documentation examples in English, Japanese, and Chinese now pass pull request titles and branch references through ChangesCI documentation updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 updates the CI integration documentation in English, Japanese, and Chinese to recommend passing PR-controlled values (such as the PR title and branch references) through environment variables (env:) rather than interpolating them directly into the run: shell script. This is a critical security improvement to prevent potential shell injection vulnerabilities on GitHub Actions runners. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 25 minutes. |
1 participant
Description
The CI integration docs teach a workflow snippet that interpolates three
attacker-controlled PR fields directly inside a shell
run:block:${{ }}is textual substitution performed before the shell parses the line, so thequotes in the rendered command belong to the attacker, not to the template. Anyone who
copies this snippet — which is exactly what the page tells them to do — gets arbitrary
command execution on their runner, triggered by opening a PR.
This moves all three fields into an
env:mapping and references them as ordinary shellvariables. Three code blocks per locale, three locales, no prose beyond one added
sentence explaining the rule.
This is the repo's own rule
internal/config/rules/rule_docs/github_workflows.md:6— the review rule OpenCodeReviewships and applies to other people's workflows:
And
action.ymlalready complies, uniformly — all ten inputs are hoisted at:228-238and the
run:block at:240-244touches only shell variables:action.yml:169-175does the same for the branch refs —EVENT_BASE_REF: ${{ github.event.pull_request.base.ref }}inenv:, thenBASE_REF="${INPUT_BASE_REF:-$EVENT_BASE_REF}"in the
run:block.And the GitLab half of this same page already gets it right (
en/integrations/ci.md:352-355):So the shipped action, the shipped rule, and this page's own GitLab recipe all agree; the
GitHub Actions section was the lone outlier — within a single file. This PR makes the page
internally consistent rather than introducing a new opinion.
Verification — I ran the attack
A docs claim about a security bug is worth very little without the log line, so I
reproduced it on a hosted runner in a fork. One workflow, two jobs, differing only in how
the title reaches the shell; one PR titled:
Run
29827744839,job logs verbatim:
vulnerablerun: echo "background=${{ github.event.pull_request.title }}"background=demoPWNED_INJECTION_SUCCEEDEDsafeenv: PR_TITLE:+run: echo "background=$PR_TITLE"background=demo"; echo PWNED_INJECTION_SUCCEEDED; #The
vulnerablejob's rendered command line, as GitHub logged it:The payload became a second statement and executed. In the
safejob the identical titlearrives as data and prints as literal text. The payload is a bare
echo; nothing touchedthe network, filesystem, or any secret, and the branch and test PR are deleted.
base_ref/head_refare injectable too, not just the titleThe first two fields are obvious; branch names are the ones people assume are safe. git
disagrees:
All five are legal branch names, and a fork PR lets an attacker choose the branch name.
GitHub's own hardening guidance lists
github.head_refas attacker-controlled.There is also a non-malicious case:
a$HOMEbis a legal branch name, and under the oldform the shell expands
$HOMEb— so the snippet silently builds the wrong ref. Theenv:form treats it as data and gets it right. This is a correctness fix as well as asecurity one.
Gates run
Before the change there were 21
${{ github.* }}expressions insiderun:blocks —7 per locale (1 ×
title, 3 ×base_ref, 3 ×head_ref). After it there are 0, andall 21 surviving occurrences are
env:assignments. The three YAML blocks werebyte-identical across the three locales before this change and still are, so no locale
was missed.
Scope
Only the three code blocks and one added sentence per file. Specifically not touched:
if: |block (en:192,ja:148,zh:168)github.*in an expression context, never handed to a shell. Correct as written.examples/github_actions/README.md:222background: ${{ ... }}is awith:input to the composite action, whichaction.yml:237hoists toenv:itself. Safe..github/workflows/*${{ github.* }}uses areconcurrency:group expressions, not shell.I deliberately did not add a
# zizmor: ignore[...]or actionlint suppression comment —those markers exist to silence warnings, and annotating the correct pattern would read
as cargo cult.
One pre-existing issue this PR does not fix
--to "origin/$HEAD_REF"is unreliable for fork PRs regardless of quoting:origin/<fork-branch>does not exist on the base remote. That is exactly whyaction.yml:191-199— the step literally named "Fetch PR head (fork-safe)" — fetchespull/${PR_NUM}/headand uses the SHA instead. It predatesthis change and is a different fix, so I left it alone rather than widening a security
patch — but I would rather name it than have you find it. Happy to file it separately, or
fold it in here if you would prefer one pass over the section.
Limitations
snippets, so the
testcheck on this PR does not exercise the change. The evidenceabove is the verification.
self-hostedpool. The mechanism is GitHub's expression substitution, which happensbefore any runner-specific behaviour, so I do not believe the runner type matters — but
I did not test it on
self-hostedand am not claiming I did.fenced code blocks and one paragraph, with fences balanced and frontmatter untouched.
against the surrounding text in each file. A native reviewer's correction is welcome and
I will take it verbatim.
Type of Change
How Has This Been Tested?
vulnerablejob, printed as literal text in thesafejob (run29827744839,both log lines quoted above)
git check-ref-format --branchon five metacharacter branch names — all five legalactionlintv1.7.12 exit 0 on the three fixed snippets reassembled into a realworkflow, and exit 0 repo-wide on all four existing workflows
${{ github.* }}remaining inside anyrun:block in all threelocales
diffacross en/ja/zh code blocks — identical before and aftergrep -rn '\${{ github\.'across the whole repo to confirm no other file teachesthe same pattern (classification table above)
Checklist
test harness renders or lints these fenced snippets. Verified by the live
reproduction and the awk/diff gates above instead.
AI assistance: Claude Code helped research and verify this change. I reviewed the full
diff and take responsibility for it.
Summary by CodeRabbit