fix(ci): harden release-pr workflow against expression injection#1393
Conversation
The `Resolve version` step directly interpolated the free-form
`inputs.version` string into a shell script via `${{ inputs.version }}`.
Because GitHub Actions expands `${{ }}` before bash sees the script, a
crafted version input such as `0.8.0"; malicious_command; echo "` could
break out of the string context and run arbitrary commands with the
job's `contents: write` + `pull-requests: write` permissions plus the
generated App token.
Pass all workflow-context values (`inputs.*`, `steps.*.outputs.*`)
through an `env:` block and reference them as `"$VAR"` in the run
scripts. `inputs.bump` is `type: choice` (validated at dispatch) so it
was not exploitable, but env-hardening keeps the pattern consistent and
guards against future type changes.
Reported by OX Security (issue 019d8c18-6fe2-7bb9-8b99-73f3be56b6ca).
This comment has been minimized.
This comment has been minimized.
|
Automated review bot (@chaodu-agent) has already LGTM-approved above. Flagging for maintainer eyes when convenient — happy to iterate on wording if useful. |
|
Force-refreshed CI on new SHA — all checks green. Ready for maintainer review. |
|
Note LGTM ✅ — Textbook fix for a real expression-injection vulnerability; env-block pattern is the canonical GitHub-recommended mitigation. What This PR DoesThe How It WorksThree
No behavioral change — same logic, just a safer delivery mechanism for variable values. Findings
Finding Details🟢 F1: Canonical mitigation applied correctlyAll 🟢 F2: Proper shell quotingEvery env var reference uses double quotes ( 🟢 F3: Consistent hardeningEven 🟢 F4: Threat model documentationThe PR body clearly documents the attack vector, blast radius, and exploit shape — excellent for future reference. Baseline Check
What's Good (🟢)
5️⃣ Three Reasons We Might Not Need This PR
All three reasons are outweighed by the low-risk, high-value nature of this fix. The change is minimal, behavioral-neutral, and eliminates a real (if mitigated) attack vector. |
…lows Apply the same env-based pattern from release-pr.yml across every workflow that interpolated inputs.*, github.event.*, steps.*.outputs.*, or needs.*.outputs.* directly into run: scripts: - gateway-release-pr: sister vulnerability of release-pr (free-form inputs.version + contents:write + App token) - build-binaries, build-gateway, build-images, build-oabctl, build-operator, bundle-pre-seed-utils, pre-beta-build, snapshot-build, pr-preview: dispatch inputs and derived step/needs outputs - pre-beta-build/snapshot-build: matrix.agent/matrix.variant are derived from free-form CSV inputs, so they are hardened too - ci, release: github.event.* context moved to env for consistency Static matrix constants defined inline in the workflow are left as-is (not attacker-controlled). Verified with actionlint (only pre-existing finding remains) and a scanner asserting zero tainted interpolations in run blocks.
Found during a follow-up audit after #1393. Pins the five third-party (non-actions/*, non-docker/*) GitHub Actions used across this repo's workflows to the commit SHA their mutable tag currently resolves to: - dtolnay/rust-toolchain@stable -> @4be7066a (branch, not a tag) - azure/setup-helm@v4 -> @1a275c3b (v4.3.1) - aws-actions/configure-aws-credentials@v4 -> @7474bc46 (v4.3.1) - Swatinem/rust-cache@v2 -> @e18b4977 (v2, 2026-03-12) - helm/chart-releaser-action@v1.6.0 -> @a917fd15 (already a full semver tag, but tags are still mutable refs an attacker with push access to the upstream repo could retarget) Rationale: a tag (branch or lightweight/annotated) can be moved by anyone with push access to the upstream action repo, silently swapping in malicious code the next time a workflow run pulls it. A commit SHA is immutable. This is the mitigation GitHub's own security hardening guide recommends for any third-party action, and is already how this repo pins actions/checkout, docker/*, etc. via major version tags maintained by GitHub/Docker directly (lower-risk, first-party) -- third-party individual/org-maintained actions get the stricter treatment. Each pin keeps the version as a trailing comment for human readability and to make future version bumps a one-line diff. No behavior change -- every SHA is the exact commit the previous tag pointed to at time of audit, verified via the GitHub API tags list endpoint (returns commit SHA directly, avoiding annotated-tag object SHA confusion). Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>
…pin actions (#37) Follow-up to a workflow-security audit across oablab/chi, oablab/ecsctl, and openabdev/ghpool (companion of openabdev/openab#1393/#1395/#1396, which fixed the same pattern in openab's release-pr.yml). release-pr.yml and release.yml expanded ${{ inputs.version }}, ${{ inputs.tag }}, and ${{ steps.*.outputs.* }} directly inside run: shell blocks across five jobs (resolve-version, create-release, build, docker, docker-manifest). Actions expands ${{ }} before bash ever sees the script, so a crafted version/tag string can break out of the shell string context. Threat model: release-pr.yml is workflow_dispatch-only (requires repo write access + carries an App token with contents:write + pull-requests:write via steps.app-token.outputs.token). release.yml triggers on 'push: tags' too, which is also gated on write access to push a tag. Same insider/leaked-credential threat model as openab's release-pr.yml. Fix: every inputs.*/steps.*.outputs.* reference in a run: block now goes through env: and is referenced as "$VAR" in shell, matching the canonical pattern already applied across openab's workflows. No behavioral change. Also SHA-pins dtolnay/rust-toolchain@stable and Swatinem/rust-cache@v2 across ci.yml, e2e.yml, and release.yml (same rationale as openabdev/openab#1396: a mutable tag/branch ref can be moved by anyone with push access to the upstream action repo; a commit SHA cannot). Version kept as a trailing comment for readability. Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>
Summary
Harden
.github/workflows/release-pr.ymlagainst GitHub Actions expression injection in theResolve versionstep. The free-forminputs.versionstring was expanded into therun:bash script via${{ inputs.version }}before bash ever saw the script, so a crafted input such as0.8.0"; malicious_command; echo "would break the shell string context and execute arbitrary commands with the job'scontents: write+pull-requests: writepermissions and the generated App token.Pass all workflow-context values (
inputs.*andsteps.*.outputs.*) through anenv:block and reference them as"$VAR"inside the run scripts — the canonical GitHub-recommended pattern.Discussion
Threat model
workflow_dispatch→ requires write access to the repo (mitigating), but still exposes insider-threat / compromised-credential paths.contents: write,pull-requests: write, plus an App token generated viaactions/create-github-app-token@v3.${{ inputs.version }}expansion inside arun:block.inputs.bump(type: choice) is validated at dispatch time so it is not directly exploitable, but env-hardening it too keeps the pattern consistent and guards against future type changes.steps.version.outputs.versionand.stableare derived from the tainted input unchanged, soUpdate version filesandCreate release PRare also hardened.Files
.github/workflows/release-pr.yml${{ inputs.* }}/${{ steps.*.outputs.* }}reference in arun:block into anenv:block; use"$VAR"in shellTest plan
python3 -c "import yaml; yaml.safe_load(...)"on the edited workflow — parses cleanResolve version(INPUT_VERSION, INPUT_BUMP),Update version files(VERSION, STABLE), andCreate release PR(VERSION alongside existing GH_TOKEN); all shell interpolations swapped to"$VAR"Resolve versionstep still resolves both auto-bump and explicit-version cases (behavior is unchanged — same variables, just passed through env)Reference
019d8c18-6fe2-7bb9-8b99-73f3be56b6ca)