From 7b5abf14ca86e54efa5943d5cbc0c4600ce89c4e Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 5 Sep 2026 03:15:11 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=E2=9C=A8=EF=BC=9Ahold=20t?= =?UTF-8?q?he=20workflows=20to=20their=20own=20pinning=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these files say "Actions are pinned by commit, never by tag", and nothing has been checking it. Everything is pinned today; the rule has simply been a comment and a habit, and habits are what renovate edits over. A tag is a name its owner can move, so an action referenced by one is code that can change under a workflow holding a token. For commit-queue.yml that is a token which can merge to the default branch, and it runs as `pull_request_target`, where the point of every other precaution in the file is that nothing from the branch under review reaches the runner. An action taken from a moved tag would walk straight past all of it. Container images are held to the same rule, since `image:` is the same kind of promise: semgrep.yml runs one, and a digest is the only way to name it immutably. The trailing `# v4.1.2` is required too. A commit alone says nothing to a reader about what is pinned, and it is what renovate reads to know what to move the pin to; without it a pin silently stops being updated. Actions are read as text, because that trailing comment is half of what is being checked and parsing throws comments away. Images are read from parsed YAML, because they are not one shape: a job may write `container: name`, or `container: {image: name}`, or name several under `services:`. A first draft knew only `image:` and read only `*.yml`, which left a workflow saved as `.yaml` running a tagged action and two untagged images past it without a word. Its own test file proved it -- exit 0, five workflows checked, four violations in a sixth it never opened. Matching no workflows is a failure rather than a pass. Without that the task answered an empty directory with "everything they run is pinned" -- a security check reporting success over nothing, which is the shape of answer it exists to make impossible. Actions come from the parsed document too, in the end. Quoting the value made the check reject a correct pin for being punctuation; quoting the *key* -- `- "uses": attacker/action@v1`, which GitHub runs exactly as it runs the bare spelling -- made it skip the step and report everything pinned. Two shapes of the same mistake, and looking for `uses:` in the text could only ever chase them one at a time. Asking the document ends the class, and picked up reusable workflows called through `jobs.*.uses` as well, which no draft had noticed at all. `uses: docker://image` is a container, not an action, and is named the way containers are. Read as an action it looked like a pin to `sha256:...` and the check refused a reference already immutable -- the same wrong answer, arrived at from the other direction. Where each reference is written comes from the document as well. The trailing comment is the one thing parsing discards, and finding the line by searching the text for the value found whichever line mentioned it first -- a commented-out copy above the real one would lend its version comment to a reference that had none. The parser knows where it read each value, so it is asked that too. That is why `yaml` is a dependency now. Matching structure with regular expressions is how a check like this comes to be worth less than the confidence people put in it. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/tasks/verify/verify-workflows.mts | 203 ++++++++++++++++++++++++ package-scripts.yml | 1 + package.json | 3 +- pnpm-lock.yaml | 3 + project-terms.txt | 1 + 5 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 build/tasks/verify/verify-workflows.mts diff --git a/build/tasks/verify/verify-workflows.mts b/build/tasks/verify/verify-workflows.mts new file mode 100644 index 00000000..4e7b69a0 --- /dev/null +++ b/build/tasks/verify/verify-workflows.mts @@ -0,0 +1,203 @@ +/** + * @file Verify the workflows pin what they run to something immutable. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/tasks/verify/verify-workflows + * + * Two of these files say "Actions are pinned by commit, never by tag" and + * nothing has been checking it. A tag is a name its owner can move, so an + * action referenced by one is code that can change under a workflow holding a + * token -- which for `commit-queue.yml` is a token that can merge. + * + * Actions are read as text, because the version a commit belongs to lives in + * a trailing comment, which parsing throws away, and that comment is both how + * a reader knows what is pinned and how renovate knows what to move it to. + * + * Images are read from parsed YAML, because they are not one shape. A job may + * write `container: name`, or `container: {image: name}`, or name several + * under `services:` -- and a line-oriented reader that knew only `image:` + * would pass the first of those without looking, which is how a check comes + * to be worth less than the confidence people place in it. + */ + +import { readFile } from 'node:fs/promises'; +import { glob } from '@openinf/.github/build/utils'; +import { isPair, isScalar, parseDocument, visit } from 'yaml'; + +/** What a job may say about the containers it runs in, and what it runs. */ + +/** What a commit looks like, and nothing else does. */ +const COMMIT = /^[0-9a-f]{40}$/; + +/** + * A digest, which is how an image is named immutably. The name before it may + * carry a registry with a port -- `registry.example:5000/team/tool` -- so the + * only colon that matters is the one introducing the digest. + */ +const DIGEST = /@sha256:[0-9a-f]{64}$/; + +/** The trailing comment naming what the pin is, as `# v4.1.2` or `# latest`. */ +const VERSION_COMMENT = /#\s*\S+/; + +type Job = { + container?: string | { image?: string }; + services?: Record; + steps?: { uses?: string }[]; + uses?: string; +}; + +/** One `uses:` value, and where the file says it. */ +type Use = { action: string; line: number; trailing: string }; + +/** + * Collects everything a workflow runs from somebody else's repository, and + * where each is written. + * + * Taken from the parsed document rather than from the lines, because YAML has + * more ways to write a key than a reader expects: `- "uses": owner/action@v1` + * is the same step as the bare spelling and GitHub runs it just the same, + * while a pattern looking for `uses:` walks straight past it. + * + * The position comes from the document as well. Searching the text for the + * value instead would find whichever line mentions it first -- a commented-out + * copy above the real one, say -- and read that line's version comment as + * though it belonged to the reference below. + * @param {string} text The workflow as written. + * @returns {Use[]} The action references, with their line and what trails them. + */ +function usesIn(text: string) { + const found: Use[] = []; + + // The plain-function visitor rather than the keyed one: `{ Pair() {} }` is + // the library's own spelling, and the naming rule reads it as a method that + // should be camelCase. + visit(parseDocument(text), (_, node) => { + if (!isPair(node)) return; + + const { key, value } = node as { key: unknown; value: unknown }; + + if ( + !isScalar(key) || + key.value !== 'uses' || + !isScalar(value) || + typeof value.value !== 'string' || + value.range == null + ) { + return; + } + + const start = value.range[0]; + const valueEnd = value.range[1]; + const newline = text.indexOf('\n', valueEnd); + + found.push({ + action: value.value, + line: text.slice(0, start).split('\n').length, + trailing: text.slice(valueEnd, newline === -1 ? undefined : newline), + }); + }); + + return found; +} + +/** + * Collects every image a workflow runs, whichever way it names them. + * @param {unknown} document The parsed workflow. + * @returns {string[]} The image references found in it. + */ +function imagesIn(document: unknown) { + const jobs = (document as { jobs?: Record } | null)?.jobs ?? {}; + const images: string[] = []; + + for (const job of Object.values(jobs)) { + const container = job?.container; + + if (typeof container === 'string') images.push(container); + else if (typeof container?.image === 'string') images.push(container.image); + + for (const service of Object.values(job?.services ?? {})) { + if (typeof service?.image === 'string') images.push(service.image); + } + } + + return images; +} + +// GitHub reads both extensions, so a check that reads one is a check with a +// way around it. +const files = await glob([ + '.github/workflows/*.yml', + '.github/workflows/*.yaml', +]); +const problems: string[] = []; + +// A pattern that matches nothing would otherwise report that everything this +// project runs is pinned, having read no workflows at all -- the shape of +// answer a check must never give, and the one `verify.unit` has guarded +// against from the start. +if (files.length === 0) { + console.error( + 'No workflows matched `.github/workflows/*.{yml,yaml}`. Nothing was checked, so nothing is known to be pinned.' + ); + process.exit(1); +} + +for (const file of files) { + const text = await readFile(file, 'utf8'); + + for (const { action, line, trailing } of usesIn(text)) { + // A path inside this repository is not a third party and has no commit of + // its own to name. + if (action.startsWith('./')) continue; + + const where = `${file}:${line}`; + + // `uses: docker://image` runs a container rather than an action, so it is + // named the way containers are -- by digest, not by commit. Read as an + // action it looks like a pin to `sha256:…`, and the check would refuse a + // reference that is already immutable. + if (action.startsWith('docker://')) { + const image = action.slice('docker://'.length); + + if (!DIGEST.test(image)) { + problems.push( + `${where}: the container image "${image}" is not pinned to a digest. A tag is a name its owner can repoint at other code.` + ); + } + + continue; + } + + const at = action.lastIndexOf('@'); + const name = at === -1 ? action : action.slice(0, at); + const ref = at === -1 ? '' : action.slice(at + 1); + + if (!COMMIT.test(ref)) { + problems.push( + `${where}: ${name} is pinned to "${ref}", which is a tag or a branch. A tag can be moved by whoever owns it; a commit cannot.` + ); + } else if (!VERSION_COMMENT.test(trailing)) { + problems.push( + `${where}: ${name} is pinned to a commit with no trailing comment saying which version that is. Nobody can read the pin, and renovate has nothing to move.` + ); + } + } + + for (const image of imagesIn(parseDocument(text).toJS())) { + if (DIGEST.test(image)) continue; + + problems.push( + `${file}: the container image "${image}" is not pinned to a digest. A tag is a name its owner can repoint at other code.` + ); + } +} + +if (problems.length > 0) { + console.error('Workflows run code that could change underneath them:\n'); + for (const problem of problems) console.error(` ${problem}`); + process.exitCode = 1; +} else { + console.log( + `Checked ${files.length} workflow${files.length === 1 ? '' : 's'}; everything they run is pinned to a commit or a digest.` + ); +} diff --git a/package-scripts.yml b/package-scripts.yml index 8e08cb27..fd036d07 100644 --- a/package-scripts.yml +++ b/package-scripts.yml @@ -19,6 +19,7 @@ scripts: ts: node build/tasks/verify/verify-ts.mts unit: node build/tasks/verify/verify-unit.mts validForEC: node build/tasks/verify/verify-valid-for-ec.mts + workflows: node build/tasks/verify/verify-workflows.mts yaml: node build/tasks/verify/verify-yaml.mts format: all: 'rc=0; for i in build/tasks/format/*.mts; do node "$i" || rc=1; done; exit $rc' diff --git a/package.json b/package.json index eb568127..998edf50 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "retext-syntax-mentions": "4.0.0", "retext-syntax-urls": "4.0.0", "typescript": "7.0.2", - "unified": "11.0.5" + "unified": "11.0.5", + "yaml": "2.9.0" } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 51587748..1a49048c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -158,6 +158,9 @@ importers: unified: specifier: 11.0.5 version: 11.0.5 + yaml: + specifier: 2.9.0 + version: 2.9.0 packages: diff --git a/project-terms.txt b/project-terms.txt index d7c3d397..0698774e 100644 --- a/project-terms.txt +++ b/project-terms.txt @@ -72,6 +72,7 @@ recompiles remarkrc Renovatebot renovaterc +repoint repoints retarget retargets