diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 91df45a6..d7d193d4 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -531,6 +531,42 @@ most of what it claimed. It is not enough to settle a difference of five, so the release match is a check on the batch rather than a measurement of any change in it. +### The triggers a match can run under + +`scripts/resolve_ref.sh` turns a workflow input into a commit. A number is a +pull request, so it fetches `refs/pull//head`, and the job then builds that +commit and plays it. That is code somebody without write access wrote, running +on a runner. + +It is safe here. Both match workflows run from the actions tab, which needs +write access to the repository, they declare `contents: read` at the top, and +neither is given a secret. It is not safe in general. Wired to +`pull_request_target`, to `workflow_run` or to a comment event, the same script +would run a fork's branch in a job holding this repository's secrets and a +token that can write to it. + +So the script refuses. It runs under `workflow_dispatch`, `push`, `schedule` +and `release`, which are the triggers where the ref was chosen by somebody who +can already push here, and exits non-zero under anything else. With no +`GITHUB_EVENT_NAME` at all it runs, since that is a machine and not a runner. A +workflow called with `workflow_call` sees the event of the workflow that called +it, so the release workflow's call is checked too. + +**What that does not cover.** The trigger is the only part of this a script can +read. A job's `permissions:` block is exposed to nothing running inside the +job, and the repository's secrets are not enumerable either: a secret reaches a +step only when a step above passes it, and its absence from the environment +says nothing about what else the job could be handed. So the check says the ref +came from somebody with write access. It does not say the job is unprivileged, +and a `workflow_dispatch` workflow that granted `contents: write` and passed a +secret would pass it. + +The rest is the workflow's to declare and not the script's to detect. The +`permissions: contents: read` at the top of `strength.yml` and `calibrate.yml`, +and the fact that neither names a secret, are what make the pull request number +input safe to offer. Anyone reusing this has to write those two lines +themselves. A script cannot check them and does not pretend to. + ### Which openings are played Strength and Calibrate both take a `book` input, and the books it can name are diff --git a/scripts/resolve_ref.sh b/scripts/resolve_ref.sh index ba74dbe6..bd92f8d2 100755 --- a/scripts/resolve_ref.sh +++ b/scripts/resolve_ref.sh @@ -13,6 +13,32 @@ # each having their own idea of what a ref is. set -euo pipefail +# Refuse outside the triggers where the ref was chosen by somebody who can +# already push here. A number is a pull request, whose head anybody can write, +# and the caller builds and runs whatever this prints. Under a trigger such as +# pull_request_target or workflow_run the job also holds this repository's +# secrets and a token that can write to it, and the two together hand a fork +# the repository. +# +# GITHUB_EVENT_NAME is what this checks, because it is the only part of that +# worth checking from here. The runner sets it on every job, an env block +# cannot override a GITHUB_ name, and a workflow called with workflow_call sees +# the event of the workflow that called it, so a call through a reusable +# workflow is checked as well. It is not a check on permissions or on secrets: +# a step cannot read either, and docs/DEVELOPMENT.md says what that leaves +# open. Unset means this is not a runner at all, where there is nothing to +# take. +case "${GITHUB_EVENT_NAME-}" in + "" | workflow_dispatch | push | schedule | release) ;; + *) + echo "resolve_ref.sh: refusing to resolve a ref under" \ + "${GITHUB_EVENT_NAME}. It builds a ref that somebody without" \ + "write access can choose, and that trigger runs with this" \ + "repository's secrets." >&2 + exit 1 + ;; +esac + ref=${1:?usage: resolve_ref.sh } if [ "$ref" -eq "$ref" ] 2>/dev/null; then diff --git a/scripts/tests/test_resolve_ref.py b/scripts/tests/test_resolve_ref.py index e10dcc23..1dce6d1a 100644 --- a/scripts/tests/test_resolve_ref.py +++ b/scripts/tests/test_resolve_ref.py @@ -7,8 +7,13 @@ workflow input can take is resolved offline: a commit already present, a branch or tag that has to be fetched, and a bare number meaning a pull request's head. + +The trigger the script is run under is an argument here rather than whatever +this process inherited, since these tests themselves run inside a workflow and +would otherwise be checking the trigger that happened to start them. """ +import os import subprocess import sys from pathlib import Path @@ -58,13 +63,20 @@ def clone(tmp_path): return clone -def resolve(clone, ref): +def resolve(clone, ref, event=None): + """Run the script with the named trigger, or with none, which is what a + machine that is not a runner looks like.""" + environment = dict(os.environ) + environment.pop("GITHUB_EVENT_NAME", None) + if event is not None: + environment["GITHUB_EVENT_NAME"] = event return subprocess.run( [str(SCRIPT), ref], cwd=clone, check=False, capture_output=True, text=True, + env=environment, ) @@ -105,3 +117,45 @@ def test_a_ref_that_exists_nowhere_fails(clone): def test_a_number_with_no_pull_request_fails(clone): result = resolve(clone, "999") assert result.returncode != 0 + + +# What the script refuses, and what that refusal is and is not. It builds a ref +# somebody without write access can choose, so it runs only under the triggers +# where the ref was chosen by somebody who can already push. The trigger is the +# one part of that a step can read: the permissions a job was given and the +# secrets it holds are not visible from a script at all. + + +def test_a_dispatch_is_how_the_match_workflows_run(clone): + assert resolve(clone, "v1.0.0", "workflow_dispatch").returncode == 0 + + +def test_a_push_is_how_the_release_workflow_runs(clone): + assert resolve(clone, "v1.0.0", "push").returncode == 0 + + +def test_no_trigger_at_all_is_a_machine_that_is_not_a_runner(clone): + assert resolve(clone, "v1.0.0").returncode == 0 + + +def test_pull_request_target_is_refused(clone): + result = resolve(clone, "7", "pull_request_target") + assert result.returncode != 0 + assert "pull_request_target" in result.stderr + assert result.stdout == "" + + +def test_workflow_run_is_refused(clone): + result = resolve(clone, "7", "workflow_run") + assert result.returncode != 0 + assert "workflow_run" in result.stderr + + +def test_a_comment_event_is_refused(clone): + result = resolve(clone, "7", "issue_comment") + assert result.returncode != 0 + assert "issue_comment" in result.stderr + + +def test_the_refusal_covers_a_named_ref_and_not_only_a_number(clone): + assert resolve(clone, "v1.0.0", "pull_request_target").returncode != 0