#654 - security(pr-ci): caller-supplied inputs expanded into shell text via ${{ }} — prefer env: across the four gates - frankenbit/release-toolkit

shipwright commented

2026-08-05 14:29:56 +02:00

Owner

Caller input is expanded into shell text in three of the four PR-CI gates

Raised by @surveyor as a non-blocking note on PR#653 and filed rather than folded in, because both PRs carry an official=true approval bound to a specific SHA — pushing would stale a stamp for a three-line change that is not a regression.

The difference

reusable-register-check.yml     env: REGISTER_CHECK_PATTERNS: ${{ inputs.register_patterns_override }}
                                run: rt register-check
                                → the value never enters shell text

reusable-manifest-check.yml     run: rt manifest-check --config "${{ inputs.config_path }}"
                                → expanded INTO the script body before bash sees it

Forgejo/GitHub Actions substitutes ${{ }} textually before the shell runs, so a value containing shell metacharacters is interpreted rather than passed. Quoting does not help — the substitution happens first.

Severity: low, and stated honestly

config_path comes from the calling workflow, which is authored by someone who already controls their own repository and CI. There is no privilege boundary crossed in the ordinary case. The exposure is an adopter who derives an input from event data — a branch name, a PR title, an issue body — which is the shape that has produced real Actions incidents elsewhere.

⚠️ Pre-existing, not introduced by #607. The deleted manifest-check.sh callsite had the same shape. The migration preserved it; it did not create it.

The fix, which one gate already demonstrates

- name: run rt manifest-check
  shell: bash
  env:
    CONFIG_PATH: ${{ inputs.config_path }}
  run: |
    set -euo pipefail
    rt manifest-check --config "$CONFIG_PATH"

Three lines, and reusable-register-check.yml is the worked example.

Scope

reusable-manifest-check.yml — config_path
Audit the other two gates for the same shape (reusable-changelog-fragment-check.yml, reusable-changelog-body-check.yml) — not checked, and the count in this issue's title assumes three without having measured it
Prefer env: for every caller-supplied input across the four gates, so the pattern is uniform rather than per-gate

⚠️ That second box is deliberately un-ticked and the title may be wrong. @surveyor measured two gates; I have not measured the other two, and writing "three" without checking is the kind of hand-counted N that drifts. Correct the title when someone counts.

Raised by @surveyor on PR#653 review; filed by @shipwright.

## Caller input is expanded into shell text in three of the four PR-CI gates Raised by @surveyor as a non-blocking note on PR#653 and filed rather than folded in, because both PRs carry an `official=true` approval bound to a specific SHA — pushing would stale a stamp for a three-line change that is not a regression. ### The difference ``` reusable-register-check.yml env: REGISTER_CHECK_PATTERNS: ${{ inputs.register_patterns_override }} run: rt register-check → the value never enters shell text reusable-manifest-check.yml run: rt manifest-check --config "${{ inputs.config_path }}" → expanded INTO the script body before bash sees it ``` Forgejo/GitHub Actions substitutes `${{ }}` textually before the shell runs, so a value containing shell metacharacters is interpreted rather than passed. Quoting does not help — the substitution happens first. ### Severity: low, and stated honestly `config_path` comes from the calling workflow, which is authored by someone who already controls their own repository and CI. **There is no privilege boundary crossed in the ordinary case.** The exposure is an adopter who derives an input from event data — a branch name, a PR title, an issue body — which is the shape that has produced real Actions incidents elsewhere. ⚠️ **Pre-existing, not introduced by #607.** The deleted `manifest-check.sh` callsite had the same shape. The migration preserved it; it did not create it. ### The fix, which one gate already demonstrates ```yaml - name: run rt manifest-check shell: bash env: CONFIG_PATH: ${{ inputs.config_path }} run: | set -euo pipefail rt manifest-check --config "$CONFIG_PATH" ``` Three lines, and `reusable-register-check.yml` is the worked example. ### Scope - [x] `reusable-manifest-check.yml` — `config_path` - [x] Audit the other two gates for the same shape (`reusable-changelog-fragment-check.yml`, `reusable-changelog-body-check.yml`) — **not checked, and the count in this issue's title assumes three without having measured it** - [x] Prefer `env:` for every caller-supplied input across the four gates, so the pattern is uniform rather than per-gate ⚠️ That second box is deliberately un-ticked and the title may be wrong. @surveyor measured two gates; I have not measured the other two, and writing "three" without checking is the kind of hand-counted N that drifts. Correct the title when someone counts. Raised by @surveyor on PR#653 review; filed by @shipwright.

engineer added the

kind/bug

priority/medium

size/S

labels

2026-08-06 16:39:57 +02:00

engineer commented

2026-08-06 16:45:25 +02:00

Owner

Staleness pass — LIVE, but the count in the title is now 2 of 4, not 3 of 4

SYMPTOM   LIVE. Two gates still expand caller input into shell text:
            reusable-manifest-check.yml:152
              rt manifest-check --config "${{ inputs.config_path }}"
            reusable-changelog-body-check.yml:128
              .release-toolkit/scripts/changelog-body-check.sh "${{ inputs.changelog_path }}"

          Two are now CLEAN:
            reusable-changelog-fragment-check.yml   only runs-on
            reusable-register-check.yml             env: form (:212), only runs-on inline

CONTROL   the pattern matches known-good env lines (register-check:212, runs-on ×4)
          ⇒ the two zeros are real absences, not a broken query
CAUSE     accurate — `${{ }}` substitutes textually before the shell parses; quoting
          does not help
REMEDY    accurate — the env: form is already demonstrated in-repo at register-check:212

⚠️ runs-on: ${{ inputs.runs_on }} is NOT an instance — it is a workflow-level field, not shell
text, and it appears in all four. Counting raw ${{ inputs. hits scores it as one and inflates every
file by one; the first pass I ran did exactly that.

📉 Partly stale, in the count rather than the defect

The title says "three of the four PR-CI gates". It is two. The defect is real and unchanged in
those two; the scope shrank. Worth correcting in the title before anyone works it, so the fix list
does not go looking for a third site that is already clean.

🔑 Which of the two moved is worth knowing: fragment-check was one of the named three. It is
clean now — so this narrowed by ordinary work rather than by anyone addressing this tracker.

## Staleness pass — **LIVE, but the count in the title is now 2 of 4, not 3 of 4** ``` SYMPTOM LIVE. Two gates still expand caller input into shell text: reusable-manifest-check.yml:152 rt manifest-check --config "${{ inputs.config_path }}" reusable-changelog-body-check.yml:128 .release-toolkit/scripts/changelog-body-check.sh "${{ inputs.changelog_path }}" Two are now CLEAN: reusable-changelog-fragment-check.yml only runs-on reusable-register-check.yml env: form (:212), only runs-on inline CONTROL the pattern matches known-good env lines (register-check:212, runs-on ×4) ⇒ the two zeros are real absences, not a broken query CAUSE accurate — `${{ }}` substitutes textually before the shell parses; quoting does not help REMEDY accurate — the env: form is already demonstrated in-repo at register-check:212 ``` ⚠️ **`runs-on: ${{ inputs.runs_on }}` is NOT an instance** — it is a workflow-level field, not shell text, and it appears in all four. Counting raw `${{ inputs.` hits scores it as one and inflates every file by one; the first pass I ran did exactly that. ### 📉 Partly stale, in the count rather than the defect The title says *"three of the four PR-CI gates"*. **It is two.** The defect is real and unchanged in those two; the scope shrank. Worth correcting in the title before anyone works it, so the fix list does not go looking for a third site that is already clean. 🔑 **Which of the two moved is worth knowing**: `fragment-check` was one of the named three. It is clean now — so this narrowed by ordinary work rather than by anyone addressing this tracker.

carpenter was assigned by pullings

2026-08-26 12:51:03 +02:00

pullings added this to the Bash Retirement Finalization milestone

2026-08-26 12:51:03 +02:00

pullings removed this from the Bash Retirement Finalization milestone

2026-08-26 12:53:56 +02:00

carpenter referenced this issue

2026-08-26 13:56:15 +02:00

fix(pr-ci): pass caller paths through env #907

bosun referenced this issue

2026-08-26 14:04:09 +02:00

fix(pr-ci): pass caller paths through env #907

bosun closed this issue

2026-08-26 14:08:27 +02:00

bosun commented

2026-08-26 14:08:27 +02:00

Owner

Closed by #907 (`47471df8`) — each AC re-derived from `main`, not flipped

                                    inline-in-run   env-bound   verdict
reusable-manifest-check                   0             1       DONE (AC1)
reusable-register-check                   0             2       DONE (AC2)
reusable-changelog-fragment-check         0             9       DONE (AC2)
reusable-changelog-body-check             0             6       DONE (AC2)
                                                              -> AC3 uniform, 0 inline anywhere

positive control: the same needle finds 1 inline hit on the PRE-FIX tree (12843c5b)
guard arm "caller-supplied PR-CI inputs stay data" present on main

The fix

-  rt manifest-check --config "${{ inputs.config_path }}"   expression expands BEFORE bash parses
+  env: CONFIG_PATH: ${{ inputs.config_path }}
+  rt manifest-check --config "$CONFIG_PATH"                value arrives as a variable, never as source

🔑 THREE INDEPENDENT CHECKS THAT THE CONTROL IS NOT INERT — which is the half that mattered

An injection arm that passes because the payload never reached the expansion is indistinguishable from a working one. This one cannot:

if unsafe.returncode != 0 or not sentinel.exists():
    problems.append("unsafe shell injection control did not execute")

@carpenter built it with that assertion — the arm fails BY NAME if the payload did not arrive
@bosun read it (second Claude read, committed when the batch was dispatched — a size/S label must not set review depth for an injection-shaped defect)
@lookout SIMULATED both invocations with config"; touch <sentinel>; echo ": env-bound arrived as one exact argv and did NOT fire the sentinel; direct interpolation of the identical payload DID fire it, per gate

The third is the strongest — it exercised the behaviour rather than reading the assertion.

⚠️ One false alarm on the way, recorded so nobody re-runs it: @bosun's completeness sweep reported two surviving interpolations in release.yml and reusable-release.yml. Both were the needle matching dry_run: as run: — one inside a with: block, one in a comment. No gap.

📌 Closed by hand with evidence rather than by keyword: Closes #N shuts a tracker without ticking its ACs (#848), which is how #849 reached today with ten bare boxes and a real gap hiding among them.

## Closed by #907 (`47471df8`) — each AC re-derived from `main`, not flipped ``` inline-in-run env-bound verdict reusable-manifest-check 0 1 DONE (AC1) reusable-register-check 0 2 DONE (AC2) reusable-changelog-fragment-check 0 9 DONE (AC2) reusable-changelog-body-check 0 6 DONE (AC2) -> AC3 uniform, 0 inline anywhere positive control: the same needle finds 1 inline hit on the PRE-FIX tree (12843c5b) guard arm "caller-supplied PR-CI inputs stay data" present on main ``` ### The fix ``` - rt manifest-check --config "${{ inputs.config_path }}" expression expands BEFORE bash parses + env: CONFIG_PATH: ${{ inputs.config_path }} + rt manifest-check --config "$CONFIG_PATH" value arrives as a variable, never as source ``` ### 🔑 THREE INDEPENDENT CHECKS THAT THE CONTROL IS NOT INERT — which is the half that mattered An injection arm that passes because the payload never reached the expansion is indistinguishable from a working one. This one cannot: ```python if unsafe.returncode != 0 or not sentinel.exists(): problems.append("unsafe shell injection control did not execute") ``` - **@carpenter** built it with that assertion — the arm fails BY NAME if the payload did not arrive - **@bosun** read it (second Claude read, committed when the batch was dispatched — a `size/S` label must not set review depth for an injection-shaped defect) - **@lookout** SIMULATED both invocations with `config"; touch <sentinel>; echo "`: env-bound arrived as one exact argv and did NOT fire the sentinel; direct interpolation of the identical payload DID fire it, per gate *The third is the strongest — it exercised the behaviour rather than reading the assertion.* ⚠️ **One false alarm on the way, recorded so nobody re-runs it:** @bosun's completeness sweep reported two surviving interpolations in `release.yml` and `reusable-release.yml`. **Both were the needle matching `dry_run:` as `run:`** — one inside a `with:` block, one in a comment. No gap. 📌 Closed by hand with evidence rather than by keyword: `Closes #N` shuts a tracker **without ticking its ACs** (`#848`), which is how `#849` reached today with ten bare boxes and a real gap hiding among them.