contrib-metrics: stop expanding dispatch inputs into the shell script - #2339
Open
nishantbkl3345-ship-it wants to merge 2 commits into
Open
Conversation
The "Set the start and end dates" step of the monthly contributor report
expanded ${{inputs.start_date}} and ${{inputs.end_date}} directly into
its run block. Actions substitutes those expressions into the script
before bash sees it, so the values were parsed as shell code rather than
read as data, and a command substitution in either input was executed by
the runner.
The step also appended the values verbatim to $GITHUB_ENV. A newline in
an input therefore defined arbitrary extra variables for every later
step of the job, including the steps that receive the org-wide read
token in GH_ACTION_METRICS_ORG_READ.
Pass both inputs through the environment so they stay data, and accept
only plain YYYY-MM-DD values. Validation is needed on top of the
environment passthrough because the values are still written to
$GITHUB_ENV and interpolated into the search queries and report text of
the following steps. An incomplete date range is now rejected instead of
being silently replaced by the computed fallback dates.
Add a regression test that expands and runs the step the way the runner
does, plus a workflow to run it, since this report is otherwise only
exercised once a month.
Signed-off-by: Nishant <nishantbkl3345-ship-it@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stop expanding
workflow_dispatchinputs into the metrics workflow's shell scriptThe
Set the start and end datesstep of.github/workflows/issue-pr-contrib-metrics.yamlexpanded${{inputs.start_date}}and${{inputs.end_date}}directly into itsrun:block. GitHub Actions substitutes those expressions into the script text before bash is invoked, so the input values became part of the script itself rather than data read by it. A command substitution in either input was therefore executed by the runner.The same step then appended the values verbatim to
$GITHUB_ENV. Because$GITHUB_ENVis line-oriented, a newline inside an input defined arbitrary additional environment variables for every subsequent step of the job — including the steps that receive the org-wide read tokensecrets.GH_ACTION_METRICS_ORG_READand the third-party actions that run under it.START_DATE/END_DATEare also interpolated into ten furtherrun:blocks, so an unvalidated value reaches those shells as code too.This is
template-injection(CWE-94 / CWE-78). It is reachable on the real, currentmain: the workflow is live and its scheduled runs file reports such as #2218. The dispatch inputs are supplied by whoever triggers the run, so this converts "can dispatch a workflow" into "can run arbitrary commands on the runner and read the job's secrets", and it turns a simple typo (a stray quote, an&) into a confusing mid-run failure.Fix. Pass both inputs through the step
env:block so they stay data, then accept only plainYYYY-MM-DDvalues. Both halves are needed: theenv:passthrough alone still lets a newline smuggle variables into$GITHUB_ENV(demonstrated by the mutation test below), and validation cannot be done safely without the passthrough. An incomplete date range is now rejected rather than being silently replaced by the computed fallback dates.The scheduled path is unchanged: with no inputs, both variables expand to empty and the
date -dfallback runs exactly as before.How to use
Review
.github/workflows/issue-pr-contrib-metrics.yaml— the change is confined to the first step (+20/−3). The other two files are the regression test and the workflow that runs it.To reproduce the original bug, check out the parent commit and run the test; to confirm the fix, run it on this branch:
The test reproduces what the runner does — it expands the step's
${{ ... }}expressions, then executes the resultingrun:block with bash against a temporary$GITHUB_ENV— so it exercises the real workflow file rather than a copy. It needs GNUdatefor the two cases that cover the scheduled fallback and skips them gracefully elsewhere.Testing done
1. The bug, before the fix (
main, on a host with GNUdate, matchingubuntu-latest):$(touch …)in an input created the marker file, andPWNED=yeslanded in$GITHUB_ENVwhere every later step would inherit it.2. After the fix:
Also run on a host without GNU
dateto confirm the skips behave:Ran 6 tests ... OK (skipped=2).3. Mutation test — confirming validation is load-bearing, not decorative. Removing only the format check while keeping the
env:passthrough:4. Static analysis (
zizmor1.16.3,--no-online-audits, whole.github/workflows/):template-injectiontemplate-injectionunpinned-usesThe four High findings were exactly the four
inputs.*expansions this PR removes. The new workflow reports no findings of its own.5. Other checks: both workflow files parse as YAML;
shellcheck -s bashover everyrun:block reports nothing for the changed step.Pre-existing, unrelated findings (not touched)
unpinned-uses× 18 — the metrics actions are referenced by tag (@v1,@v2). Present before this PR, unchanged by it, and a separate hardening decision. The one action this PR adds is pinned to a SHA.template-injectionLow × 20 —${{ env.START_DATE }}/${{ env.END_DATE }}in the laterrun:blocks.zizmoronly reports these under the pedanticauditorpersona. This PR makes them provably safe (the values can now only match[0-9]{4}-[0-9]{2}-[0-9]{2}), and rewriting ten unrelated blocks would have obscured the fix.excessive-permissions— workflow-levelissues: write. Pre-existing and out of scope.shellcheck SC2129(style) in the untouchedAssemble full reportstep. Verified pre-existing by runningshellcheckagainst the file atmain.Notes
changelog/directory and the change does not affect image builds.