From 00ae7f6898e935ed7d2c013b67c26c74e9ff39d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 10:15:16 +0000 Subject: [PATCH] ci(ci): Refuse to resolve a ref under a trigger that carries secrets resolve_ref.sh takes a pull request number, fetches refs/pull//head, and the job builds that commit and plays it. Here that is safe: both match workflows run from the actions tab, which needs write access, they declare contents: read at the top and neither is given a secret. Wired instead 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. The script now runs under workflow_dispatch, push, schedule and release, and exits non-zero under anything else. Those are the triggers where the ref was chosen by somebody who can already push here. With GITHUB_EVENT_NAME unset it runs, since that is a machine and not a runner, and a workflow called with workflow_call sees the event of the workflow that called it, so the release workflow's call is checked as well. The tests name the trigger rather than inheriting it, because they run inside a workflow themselves and would otherwise check whatever started them. That is the whole of what a script can check, and the documentation says so rather than a comment saying it. A job's permissions block is exposed to nothing running inside the job, and the repository's secrets cannot be enumerated from a step, so the check says the ref came from somebody with write access and says nothing about how privileged the job is. The contents: read at the top of strength.yml and calibrate.yml is what it does not replace. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XhA13EsiXpMQ5vea2P3Fc9 --- docs/DEVELOPMENT.md | 36 ++++++++++++++++++++ scripts/resolve_ref.sh | 26 ++++++++++++++ scripts/tests/test_resolve_ref.py | 56 ++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 1 deletion(-) 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