From 0b413615faf2a0eaf3a819e15873c7615052df64 Mon Sep 17 00:00:00 2001 From: paulohenriquevn Date: Fri, 4 Sep 2026 22:32:45 -0300 Subject: [PATCH] =?UTF-8?q?fix(setup):=20remove=20registry-url=20=E2=80=94?= =?UTF-8?q?=20it=20served=20nobody=20and=20invited=20a=20harmful=20config?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The input added in #54 was justified by 'seven of the nine release.yml pass registry-url'. That number came from a grep that counted the word wherever it appeared, and where it appears is inside a comment in seven of those workflows explaining why the setting is deliberately absent: NO registry-url. It looks harmless and is not: setup-node writes //registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN} into an npmrc UNCONDITIONALLY and points NPM_CONFIG_USERCONFIG at it, then exports NODE_AUTH_TOKEN only if the caller supplied one -- which, under OIDC, nobody does. Matching the YAML key instead of the string: zero of nine use it, and all nine publish through actions/npm-oidc. The input served nobody and its docstring invited the exact configuration this ecosystem had already found and documented as harmful. The reasoning now lives beside the setup-node call so the absence reads as a decision, and the CI job asserts the opposite property: the action writes no _authToken npmrc on a caller's behalf. The finding that prompted #54 survives: 22 workflows set up Node and pnpm by hand and one file uses this action. The blocker was imagined; the duplication is real and the action was a drop-in all along. --- .github/workflows/ci.yml | 36 +++++++++++++----------------------- CHANGELOG.md | 38 +++++++++++++++++++++++--------------- actions/setup/action.yml | 27 +++++++++++---------------- 3 files changed, 47 insertions(+), 54 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e7555f2..d606a11 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -127,32 +127,22 @@ jobs: echo "wanted=${expected} actual=${actual}" case "${actual}" in "v${expected}"*) ;; *) echo "::error::node mismatch"; exit 1 ;; esac - # `registry-url` is what kept every release workflow off this action: measured 2026-09-05, - # seven of the nine `release.yml` in this ecosystem pass it, and the action did not accept - # it — so adopting the shared setup meant losing npm authentication. It is a passthrough to - # `actions/setup-node`, and a passthrough nobody exercises is a passthrough that can be - # silently dropped in a refactor. - - name: Run the action again, asking for a registry - uses: ./actions/setup - with: - node-version: ${{ matrix.node }} - working-directory: fixtures/pnpm-repo - # No install: the toolchain and the .npmrc are the whole subject here. - install: 'false' - registry-url: 'https://registry.npmjs.org' - - name: setup-node wrote an npmrc binding the token + # The action writes NO npmrc, and that is the property worth holding still. `setup-node` + # writes `_authToken=${NODE_AUTH_TOKEN}` whenever it is given a `registry-url`, and under + # OIDC nobody exports NODE_AUTH_TOKEN — so the publish would run against a config naming a + # credential that does not exist. Every release workflow in this ecosystem avoids that by + # not passing the input; this asserts the action does not reintroduce it on their behalf. + - name: No npmrc was written on the action's behalf shell: bash - # Asserting the FILE rather than `npm config get registry`, because the latter reads the - # effective config and would go green on a registry that came from somewhere else — the - # claim under test is that this action caused it. run: | set -euo pipefail - : "${NPM_CONFIG_USERCONFIG:?setup-node did not point NPM_CONFIG_USERCONFIG anywhere}" - echo "userconfig: ${NPM_CONFIG_USERCONFIG}" - test -f "${NPM_CONFIG_USERCONFIG}" - grep -q 'registry.npmjs.org' "${NPM_CONFIG_USERCONFIG}" - grep -q '_authToken' "${NPM_CONFIG_USERCONFIG}" - echo "npmrc names the registry and binds _authToken" + if [ -n "${NPM_CONFIG_USERCONFIG:-}" ] && [ -f "${NPM_CONFIG_USERCONFIG}" ] \ + && grep -q '_authToken' "${NPM_CONFIG_USERCONFIG}"; then + echo "::error::the setup action produced an npmrc binding _authToken; under OIDC that names a credential nobody exports" + cat "${NPM_CONFIG_USERCONFIG}" + exit 1 + fi + echo "no _authToken npmrc — an OIDC publish is not handed a phantom credential" npm-oidc-action: name: npm-oidc action diff --git a/CHANGELOG.md b/CHANGELOG.md index 57dafad..120e8ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,21 +10,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **`actions/setup` accepts `registry-url`, which is what kept every release workflow off it - (#54).** Measured 2026-09-05 across the ten consumers: **22 workflows set up Node and pnpm by - hand, and exactly one file used this action.** The largest group of duplicates is the nine - `release.yml`, and **seven of them pass `registry-url`** — so adopting the shared setup meant - losing npm authentication. One missing passthrough was holding the whole adoption. - - Everything else was already equivalent, including the part most likely to be lost in a rewrite: - the action's `cache-dependency-path` already lists `package.json` beside `pnpm-lock.yaml`, which - is the fix for a pnpm major bump restoring a store the new pnpm reads as inconsistent - (`ERR_PNPM_MISSING_PACKAGE_INDEX_FILE`). - - Exercised rather than asserted: the action's own CI job now runs it with a registry and checks - that `setup-node` wrote an `.npmrc` naming the registry and binding `_authToken`. The assertion - reads the FILE rather than `npm config get registry`, because the latter would go green on a - registry that came from somewhere else. +- **`actions/setup` does NOT accept `registry-url`, and now says why (#54, corrected).** The + entry this replaces claimed that seven of the nine `release.yml` pass `registry-url` and that + its absence was what kept them off this action. **Both halves were false.** The grep behind the + number counted the word wherever it appeared, and where it appears is inside a comment in seven + of those workflows explaining why the setting is deliberately absent: + + > *"NO `registry-url`. It looks harmless and is not: setup-node writes + > `//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}` into an npmrc UNCONDITIONALLY and points + > NPM_CONFIG_USERCONFIG at it, then exports NODE_AUTH_TOKEN only if the caller supplied one — + > which, under OIDC, nobody does."* + + Measured properly, by matching the YAML key rather than the string: **zero of nine use it**, and + all nine publish through `actions/npm-oidc`. The input served nobody, and its docstring invited + a configuration this ecosystem had already found and documented as harmful — worse than absent, + because the next person converting a release workflow would have believed it. + + Removed, with the reasoning moved next to the `setup-node` call so the absence reads as a + decision. The CI job now asserts the opposite property: that the action writes no `_authToken` + npmrc on a caller's behalf. + + What survives the correction is the finding that prompted it: **22 workflows across the ten + consumers set up Node and pnpm by hand and exactly one file uses this action.** The blocker was + imagined; the duplication is real, and the action was a drop-in replacement all along. - **`promotion-gate.yml` is now a reusable workflow, so the gate that protects `develop` has one home instead of ten (#51).** Measured 2026-09-05 across the ten consumers of this repository: diff --git a/actions/setup/action.yml b/actions/setup/action.yml index 1837b8c..e531388 100644 --- a/actions/setup/action.yml +++ b/actions/setup/action.yml @@ -35,20 +35,6 @@ inputs: has to say so here — otherwise the install runs against nothing and reports success. required: false default: '.' - registry-url: - description: >- - Passed straight to `actions/setup-node`, which writes an `.npmrc` binding - `_authToken` to `NODE_AUTH_TOKEN`. A publishing job needs it; nothing else does, so it is - empty by default and `setup-node` then writes no `.npmrc` at all. - - This input exists because its absence was what kept every release workflow off this action. - Measured 2026-09-05 across the ten consumers: 22 workflows set up Node and pnpm by hand and - exactly one file used this action. Of the nine `release.yml`, SEVEN pass `registry-url` — - so for the largest group of duplicates, adopting the shared setup meant losing npm - authentication. One missing passthrough was holding the whole adoption. - required: false - default: '' - outputs: package-manager-version: description: 'The resolved package manager version, so a caller can assert on it.' @@ -75,8 +61,17 @@ runs: with: node-version: ${{ inputs.node-version }} node-version-file: ${{ inputs.node-version == '' && format('{0}/.nvmrc', inputs.working-directory) || '' }} - # Empty writes no `.npmrc`, which is what every non-publishing job wants. - registry-url: ${{ inputs.registry-url }} + # DELIBERATELY NO `registry-url`, and this is load-bearing rather than an omission. + # `setup-node` writes `//registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}` into an npmrc + # UNCONDITIONALLY and points `NPM_CONFIG_USERCONFIG` at it, then exports NODE_AUTH_TOKEN + # only if the caller supplied one — which, under OIDC trusted publishing, nobody does. The + # job would publish with a config declaring a credential that does not exist. + # + # Measured 2026-09-05: all nine `release.yml` in this ecosystem publish through + # `actions/npm-oidc`, and ZERO pass `registry-url`. Seven carry a comment saying why not. + # A passthrough was briefly added here (#54) on the strength of a grep that counted those + # comments as uses; it served nobody and its documentation invited the harmful setting, so + # it was removed. If a non-OIDC consumer ever needs it, add it back WITH this warning. cache: pnpm # The cache key must change when the package manager does. setup-node keys the pnpm store on # the lockfile alone, so a pnpm 9 -> 10 bump leaves the key identical and the job restores a