Repository navigation
feat(js): Publish without a token where the registry trusts it - #18
Conversation
|
This repository is not connected to any of your workspaces. Please connect it at https://app.sourceant.ai to get reviews on it. |
1285d59 to
06650e3
Compare
Code Review Summary✨ ReviewThe new preconditions step is the right place to fail fast, but its host comparison is too literal: it only rejects the exact lowercase, port-free, credential-free form. Because npm lower-cases and normalises the host when it connects, inputs like 🚀 Key Improvements
📉 Regressions
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| npm publish $ACCESS | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ inputs.token }} | ||
| # Unset when the registry trusts the workflow: a token in the environment takes |
There was a problem hiding this comment.
This expression never evaluates to the empty string. GitHub Actions has no ternary operator: cond && a || b returns a only when a is truthy. '' is falsy, so inputs.trusted == 'true' && '' yields '', which makes the || fall through to inputs.token. The result is that when trusted is true — the one case where the token must be absent — the token is set, the publish is authenticated by token, and it is not attested. Invert the condition so the falsy branch is the final fallback and the truthy branch is the token. Note this still leaves an empty-but-present variable; if npm rejects that, replace it by splitting the publish into a trusted step with no env and a non-trusted step with NODE_AUTH_TOKEN.
| # Unset when the registry trusts the workflow: a token in the environment takes | |
| # Unset when the registry trusts the workflow: a token in the environment takes | |
| # precedence, and the publish stops being attested. GitHub expressions have no | |
| # ternary, and `cond && '' || token` evaluates to `token` because `''` is falsy, | |
| # so the condition is inverted here. | |
| NODE_AUTH_TOKEN: ${{ inputs.trusted != 'true' && inputs.token || '' }} |
| - name: Upgrade npm | ||
| if: inputs.trusted == 'true' | ||
| shell: bash | ||
| run: npm install -g npm@latest |
There was a problem hiding this comment.
The comment above this step names npm 11.5.1 as the floor the registry requires, but the step installs npm@latest. That is unpinned and non-reproducible — a future major can change flag/registry behaviour, and there is no guarantee the installed version clears the documented floor if the registry ever lags. Pin to the major that satisfies the floor and let patch releases float.
| run: npm install -g npm@latest | |
| run: npm install -g 'npm@^11.5.1' |
| registry-url: ${{ inputs.registry }} | ||
| scope: ${{ inputs.scope }} | ||
|
|
||
| # Node 22 ships npm 10, and a registry only trusts a workflow from 11.5.1 onward. |
There was a problem hiding this comment.
The trusted path upgrades npm with npm@latest and never checks that the configured registry actually supports OIDC trusted publishing. Two consequences: (1) @latest is non-reproducible, so the toolchain the registry gates on (>= 11.5.1) can silently jump to a future major; (2) with the action's default registry (https://npm.pkg.github.com), the upgrade step installs the npm CLI through an auth-required registry while NODE_AUTH_TOKEN is empty, and any subsequent npm publish fails with an opaque 401 instead of naming the real precondition. Fail fast on the registry and pin the npm floor.
| # Node 22 ships npm 10, and a registry only trusts a workflow from 11.5.1 onward. | |
| # Only npmjs.org trusts a workflow, and only from npm 11.5.1 onward; Node 22 ships npm 10. | |
| - name: Prepare trusted publishing | |
| if: inputs.trusted == 'true' | |
| shell: bash | |
| run: | | |
| if [ "${{ inputs.registry }}" != "https://registry.npmjs.org" ]; then | |
| echo "::error::trusted publishing requires registry https://registry.npmjs.org" | |
| exit 1 | |
| fi | |
| npm install -g npm@^11.5.1 |
| @@ -410,6 +411,26 @@ token for it: | |||
| access: public | |||
There was a problem hiding this comment.
The trusted-publishing example mixes a top-level permissions: block with a with: fragment indented eight spaces, so it is not valid YAML and does not show the caller where the with: belongs. id-token: write is a job/workflow permission, not something that can sit beside a step's with:. Present a complete, correctly indented job so the documented contract (permission + trusted/registry inputs) matches what a caller must actually write.
| access: public | |
| Set `trusted` where the registry trusts this workflow instead of a token. The | |
| action then runs Node 22.14 with a current npm, and publishes with no token in | |
| the environment, so the release carries a provenance attestation. The job needs | |
| `id-token: write`, and the registry needs the repository and the workflow | |
| filename recorded against the package: | |
| ```yaml | |
| permissions: | |
| contents: write | |
| id-token: write | |
| jobs: | |
| release: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: whilesmart/workflows/js/publish@main | |
| with: | |
| token: ${{ secrets.GITHUB_TOKEN }} | |
| registry: https://registry.npmjs.org | |
| access: public | |
| trusted: 'true' |
token is still read, for the GitHub release.
| npm publish $ACCESS | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ inputs.token }} | ||
| # Unset when the registry trusts the workflow: a token in the environment takes |
There was a problem hiding this comment.
The GitHub Actions ternary idiom cond && value || fallback only acts as a ternary when value is truthy. Here the true-branch is '', which is falsy, so inputs.trusted == 'true' && '' || inputs.token evaluates to inputs.token in every case. When trusted: 'true' the token is therefore still present in the environment, npm takes it in preference to OIDC, and the publish is not attested — the change silently fails at its one job. Invert the condition so the token sits in the truthy branch.
| # Unset when the registry trusts the workflow: a token in the environment takes | |
| # Empty when the registry trusts the workflow: a token in the environment takes | |
| # precedence, and the publish stops being attested. | |
| NODE_AUTH_TOKEN: ${{ inputs.trusted != 'true' && inputs.token || '' }} |
06650e3 to
61aa868
Compare
|
Both in. The token one was a real defect: the change failed at the one thing it exists to do.
The registry and version points are in too:
|
| registry-url: ${{ inputs.registry }} | ||
| scope: ${{ inputs.scope }} | ||
|
|
||
| - name: Check the registry can trust a workflow |
There was a problem hiding this comment.
This guard is both narrower and weaker than the documented contract. It only rejects the exact default string https://npm.pkg.github.com, so registry: https://npm.pkg.github.com/ (or any other registry without OIDC support, e.g. a private Verdaccio) passes and then fails later inside npm publish with an opaque auth error — the opposite of the fail-fast this step exists for. Matching by pattern covers the trailing-slash and subdomain cases. The step also does not check the other precondition trusted publishing needs: the calling job must grant id-token: write. GitHub exports ACTIONS_ID_TOKEN_REQUEST_URL only when that permission is present, so it is a cheap, deterministic check that turns a confusing OIDC error into an actionable one. Passing inputs.registry through env: also keeps the caller-supplied value out of the shell text.
| - name: Check the registry can trust a workflow | |
| - name: Check the job can mint an OIDC token | |
| if: inputs.trusted == 'true' | |
| shell: bash | |
| run: | | |
| if [ -z "$ACTIONS_ID_TOKEN_REQUEST_URL" ]; then | |
| echo '::error::trusted publishing needs the id-token: write permission on the calling job' | |
| exit 1 | |
| fi | |
| - name: Check the registry can trust a workflow | |
| if: inputs.trusted == 'true' | |
| shell: bash | |
| env: | |
| REGISTRY: ${{ inputs.registry }} | |
| run: | | |
| case "$REGISTRY" in | |
| *npm.pkg.github.com*) | |
| echo '::error::trusted publishing needs a registry that supports it; set the registry input' | |
| exit 1 | |
| ;; | |
| esac |
| - `js/publish` takes `trusted`, publishing with a registry that trusts the workflow rather | ||
| than a token. It pins Node to 22.14 and npm to 11.5.1 or later, the versions a registry | ||
| requires before it will accept an attested publish, and publishes from a step that | ||
| carries no token at all. It refuses to run against a registry that cannot trust a |
There was a problem hiding this comment.
The CHANGELOG promises more than the action enforces. js/publish/action.yml line 56 rejects exactly one host (https://npm.pkg.github.com); it has no way to tell whether any other registry trusts a workflow, so a private Verdaccio/Artifactory/Nexus registry, or even the default host written with a trailing slash or different casing, passes the guard and then dies as an opaque registry error. README.md (line 417) already states the narrower, accurate guarantee ("refuses to run against GitHub Packages"). Bring the CHANGELOG in line so the two documents describe the same contract as the code.
| carries no token at all. It refuses to run against a registry that cannot trust a | |
| + carries no token at all. It refuses to run against GitHub Packages, which cannot | |
| + trust a workflow. |
| registry-url: ${{ inputs.registry }} | ||
| scope: ${{ inputs.scope }} | ||
|
|
||
| - name: Check the registry can trust a workflow |
There was a problem hiding this comment.
This guard is a second implementation of a fact the action already encodes: inputs.registry is declared above with default: 'https://npm.pkg.github.com', and the guard copies that URL as a literal to decide which registry cannot trust a workflow. Two things follow from the copy. (1) Drift: if the input default ever changes, the guard silently stops rejecting the default, and trusted: 'true' proceeds against GitHub Packages — the exact case it exists to stop. (2) The [ ... = ... ] test matches the string exactly, so the equivalent https://npm.pkg.github.com/ (trailing slash) slips through and the publish fails later instead of at the guard. Normalise the value and keep the knowledge in one place, so the guard's promise matches the CHANGELOG's "refuses to run against a registry that cannot trust a workflow".
| - name: Check the registry can trust a workflow | |
| - name: Check the registry can trust a workflow | |
| if: inputs.trusted == 'true' | |
| shell: bash | |
| run: | | |
| REGISTRY="${{ inputs.registry }}" | |
| # The value the `registry` input defaults to is the one registry known here not to | |
| # trust a workflow; compare it normalised so the two cannot drift apart and a | |
| # trailing slash is still caught. | |
| if [ "${REGISTRY%/}" = 'https://npm.pkg.github.com' ]; then | |
| echo "::error::trusted publishing needs a registry that supports it; $REGISTRY cannot" | |
| exit 1 | |
| fi | |
A registry that trusts a workflow mints a short-lived credential for the job and attests the release to the commit that built it, so nothing long-lived is stored. It accepts this only from Node 22.14 and a current npm, and refuses to attest when a token is in the environment.
61aa868 to
1673a96
Compare
| exit 1 | ||
| fi | ||
|
|
||
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[a-z]+://##; s#/.*$##') |
There was a problem hiding this comment.
The host parse only lowercases nothing and strips only scheme:// and the path: sed -E 's#^[a-z]+://##; s#/.*$##' leaves npm.pkg.github.com for the common spelling, but HTTPS://npm.pkg.github.com (uppercase scheme), npm.pkg.github.com:443 (port) and https://user@npm.pkg.github.com (credentials) all survive the transformation and therefore pass the guard, then fail later inside npm publish with an opaque error — the same class of hole the previous pass asked to close. Normalise the value with parameter expansion and compare case-insensitively so every spelling of the host is caught. Note this still cannot detect a non-GitHub registry without OIDC support, which is why README.md documents the narrower guarantee.
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[a-z]+://##; s#/.*$##') | |
| # Normalise how a registry URL is spelled, so scheme case, credentials and a port | |
| # cannot hide the one host this guard is able to detect. | |
| HOST="${REGISTRY#*://}" | |
| HOST="${HOST##*@}" | |
| HOST="${HOST%%/*}" | |
| HOST="${HOST%%:*}" | |
| if [ "${HOST,,}" = 'npm.pkg.github.com' ]; then | |
| echo "::error::$HOST cannot trust a workflow; set the registry input to one that can" | |
| exit 1 | |
| fi |
| exit 1 | ||
| fi | ||
|
|
||
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[a-z]+://##; s#/.*$##') |
There was a problem hiding this comment.
The host is extracted with a lower-case-only scheme pattern and the trailing s#/.*$## keeps any port, so https://NPM.PKG.GITHUB.COM (scheme not stripped, host uppercased) and https://npm.pkg.github.com:443 (port retained) both compare unequal to npm.pkg.github.com and pass the guard. npm lower-cases the host when it connects, so those inputs hit GitHub Packages anyway and fail later inside npm publish with an opaque auth error — exactly what this fail-fast step is here to prevent. Normalise scheme/port/path and case before comparing.
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[a-z]+://##; s#/.*$##') | |
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[A-Za-z]+://##; s#[/:].*$##' | tr 'A-Z' 'a-z') | |
| if [ "$HOST" = 'npm.pkg.github.com' ]; then |
| exit 1 | ||
| fi | ||
|
|
||
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[a-z]+://##; s#/.*$##') |
There was a problem hiding this comment.
The host comparison is case-sensitive. Host names are case-insensitive, so registry: https://NPM.PKG.GITHUB.COM/ extracts the host unchanged and compares unequal to npm.pkg.github.com, letting the trusted publish proceed against the one registry the guard exists to reject — then fail later inside npm publish as an opaque auth error. Fold the value to lower case before matching so the guard's promise holds for any spelling of the same host. This mirrors the earlier request to normalise the value rather than test it verbatim; the trailing-slash half of that fix is already applied here, the case half is not.
| HOST=$(printf '%s' "$REGISTRY" | sed -E 's#^[a-z]+://##; s#/.*$##') | |
| HOST=$(printf '%s' "$REGISTRY" | tr '[:upper:]' '[:lower:]' | sed -E 's#^[a-z][a-z0-9+.-]*://##; s#/.*$##') | |
| if [ "$HOST" = 'npm.pkg.github.com' ]; then | |
| echo "::error::$HOST cannot trust a workflow; set the registry input to one that can" | |
| exit 1 | |
| fi |
js/publishtakestrusted, publishing with a registry that trusts the workflow rather than a token. The job needsid-token: write, and the registry needs the repository and the workflow filename recorded against the package.Two version floors come with it, both required before a registry will accept an attested publish: Node is pinned to 22.14 and npm is upgraded, since Node 22 still ships npm 10. The token is left out of the environment, because one present takes precedence and the publish stops being attested.
tokenis still read, for the GitHub release.