From fe6843d113fcee69960ea11ef738192f290ecb6f Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Mon, 7 Sep 2026 02:48:30 +0000 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=E2=9C=A8=EF=BC=9Ahold?= =?UTF-8?q?=20the=20workflows=20to=20their=20own=20pinning=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ported from OpenINF/.github, where this landed as 7164b11. The workflows here say "Actions are pinned by commit, never by tag", and nothing has been checking it. Everything is pinned today; the rule has been a comment and a habit, and habits are what renovate edits over. A tag is a name its owner can repoint, 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. Container images are held to the same rule, since `image:` is the same kind of promise, and a digest is the only way to name one 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. Checked against all seven workflows here, deploy.yml and vendored-sync.yml included: every one already passes. Verified it is not passing vacuously by unpinning actions/checkout to `@v4`, which it reported with exit 1. 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 | 1 + pnpm-lock.yaml | 3 + project-terms.txt | 1 + 5 files changed, 209 insertions(+) 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 000000000..899d380f4 --- /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/portal/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 54e0a939c..19c903a7f 100644 --- a/package-scripts.yml +++ b/package-scripts.yml @@ -29,6 +29,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 68efa370a..63145e26f 100644 --- a/package.json +++ b/package.json @@ -95,6 +95,7 @@ "typescript": "7.0.2", "unified": "11.0.5", "vnu-jar": "26.8.30", + "yaml": "2.9.0", "zod": "4.5.4", "zod-validation-error": "5.0.0" } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2e8bd39a5..35212355f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -221,6 +221,9 @@ importers: vnu-jar: specifier: 26.8.30 version: 26.8.30 + yaml: + specifier: 2.9.0 + version: 2.9.0 zod: specifier: 4.5.4 version: 4.5.4 diff --git a/project-terms.txt b/project-terms.txt index 7d6b9b6f7..c7cf22082 100644 --- a/project-terms.txt +++ b/project-terms.txt @@ -63,6 +63,7 @@ openpgp recompiles remarkrc renovaterc +repoint repoints retarget retargets From c9d0e8a328668b15413b42c3f26ca2356df35cf3 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Mon, 7 Sep 2026 03:50:11 +0000 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9As?= =?UTF-8?q?top=20reading=20a=20step's=20input=20as=20an=20action?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch: the visitor matched every `uses` key in the document, and `with:` and `env:` hold names the action being called chose. A step passing an input that happens to be called `uses` was read as naming an action, so an ordinary workflow failed a check it had no way to satisfy. Those two mappings are skipped now. The review also asked for the opposite of this -- to narrow the walk to `jobs..uses` and `jobs..steps[*].uses`, the two paths GitHub executes from. That would open a hole rather than close one. A step written once under a YAML anchor and aliased into a job is a scalar only where the anchor is defined, which is nowhere near either path, and the alias itself is not a scalar for the visitor to read. Checked: an unpinned action defined under a top-level `x-defs:` anchor and aliased into a step is caught today, at the line the anchor is written on, and is missed entirely if the walk is narrowed. So the walk stays broad and only the two input mappings are excluded. Checked against fixtures: an input called `uses` under `with:` and under `env:` is ignored; a tag-pinned step, a commit with no version comment, a job-level reusable workflow on a branch, and an anchored unpinned action are all still reported; a digest-pinned `docker://` still passes. The seven workflows here still pass. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/tasks/verify/verify-workflows.mts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/build/tasks/verify/verify-workflows.mts b/build/tasks/verify/verify-workflows.mts index 899d380f4..c50788ef2 100644 --- a/build/tasks/verify/verify-workflows.mts +++ b/build/tasks/verify/verify-workflows.mts @@ -71,7 +71,7 @@ function usesIn(text: string) { // 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) => { + visit(parseDocument(text), (_, node, path) => { if (!isPair(node)) return; const { key, value } = node as { key: unknown; value: unknown }; @@ -86,6 +86,27 @@ function usesIn(text: string) { return; } + // `with:` and `env:` hold values whose names the action being called + // chose, so a step passing an input that happens to be called `uses` is + // passing a string rather than naming an action. Read as one, an + // ordinary workflow fails a check it has no way to satisfy. + // + // Only those two are skipped, and the walk stays broad everywhere else on + // purpose. A step written once under a YAML anchor and aliased into a job + // is a scalar only where the anchor is defined, which is nowhere near + // `jobs..steps`; narrowing this to the two paths GitHub executes from + // would stop reading the one place such a reference is written down. + if ( + path.some( + (ancestor) => + isPair(ancestor) && + isScalar(ancestor.key) && + (ancestor.key.value === 'with' || ancestor.key.value === 'env') + ) + ) { + return; + } + const start = value.range[0]; const valueEnd = value.range[1]; const newline = text.indexOf('\n', valueEnd);