fix: provenance-file takes precedence over OIDC auto-generated provenance - #9882
Open
yunseo-kim wants to merge 3 commits into
Open
fix: provenance-file takes precedence over OIDC auto-generated provenance#9882yunseo-kim wants to merge 3 commits into
yunseo-kim wants to merge 3 commits into
Conversation
…ance When publishing with an externally generated provenance bundle under OIDC trusted publishing, the auto-enable logic in the OIDC flow unconditionally set `provenance: true`, which caused libnpmpublish to silently discard the supplied `--provenance-file` bundle and publish its own generated provenance instead. Skip auto-enabling provenance when a provenance file is configured, so the explicitly supplied bundle is verified and published as documented. Fixes: npm#9879 Signed-off-by: Yunseo Kim <git@yunseo.kim>
… set The two options are documented as mutually exclusive, but buildMetadata previously preferred generation when both were set, silently discarding the supplied bundle. Throw an EPROVENANCECONFLICT usage error instead of silently discarding a caller-supplied cryptographically meaningful artifact. Signed-off-by: Yunseo Kim <git@yunseo.kim>
Signed-off-by: Yunseo Kim <git@yunseo.kim>
yunseo-kim
added a commit
to windlasstech/slsa-builder
that referenced
this pull request
Aug 16, 2026
…on (#99) ## Summary Adds ADR 0082, deciding that the **publish-stage** npm CLI is provisioned as an explicitly pinned, integrity-verified distribution selected from a reviewed allowlist, replacing the current floating node-24-bundled npm **on the publish job only**. Build-stage npm selection is unchanged and remains governed by ADR 0016/0017/0027. - What changed? - New ADR 0082: pin the publish-stage npm CLI version with integrity-verified provisioning (version + distribution SHA-256 in the publish job), a reviewed allowlist recorded in the specification, and a bump procedure requiring an npm-internals diff-review checklist (`oidc.js` flow, `buildMetadata()` branch structure, `@npmcli/config` exclusive enforcement, `publishConfig` flatten timing, sigstore-js major) plus per-version compatibility fixtures. - Publish-side contract change (implementation lands with the #97 fix): the publish job's npm validates against the allowlist pin, replacing the current exact-equality check against the build-time `runtime.npm_version` record, since build npm (toolchain, floating) and publish npm (pinned) diverge by design. Build continues recording its actual npm in `runtime.npm_version`. - Relations updates (frontmatter only; accepted ADR bodies untouched): `partially-supersedes ADR-0027` scoped to the publish-stage npm provisioning clause (reverse edge added), `see-also` ADR 0016/0017/0029/0081 (reverse edges added). - Index rows for 0082 in `docs/decisions/README.md` and `README.ko.md`. - Why is this needed? - The #97 remediation mechanism depends on npm internals that already vary across the currently allowed range (11.5.1 / 11.17.0 / 12.0.2: `oidc.js` structure, exclusive env carve-out, sigstore-js v3–v5). The fourth dogfood showed fail-closed read-back protects acceptance but not registry state (`@windlass/vers-js@0.1.2` is permanently burned). A floating toolchain under a source-dependent publish mechanism leaves the verification surface open, and a node-24 image update is a correlated-failure vector for all callers. The decision completes the builder-owned npm principle (ADR 0016/0017) and ADR 0027's recorded pinned-setup intent on the publish path, and makes the specification's per-version fixture obligation satisfiable. Upstream npm/cli#9882 adoption becomes a deliberate pinned bump. - Scope note (from review): an earlier draft pinned npm across all jobs; per reviewer decision the pin is publish-only, and a manifest-selected build npm option was considered and excluded from this ADR (build-stage selection may be revisited by a separate future decision). - How to test: documentation-only; `adr-relations-check` passes (83 ADRs, 184 edges, NO PROBLEMS); prettier and markdownlint clean. ## Related Issues - Related #97 (motivating defect), #30 (dogfood evidence) ## Change Type - [x] Documentation ## Changelog - Category: None - User-facing note: Architecture decision record; no runtime behavior change in this PR (spec amendments and provisioning changes follow in the #97 fix implementation). Changelog update: - [x] Not needed because this change is not user-facing ## Checklist ### General - [x] PR title follows [Conventional Commits](https://www.conventionalcommits.org/) format: `type(scope): Summary` - [x] This PR does not expose backend/internal implementation details in a public repo. - [x] No secrets, tokens, keys, or private endpoints are included. - [x] Changes stay within this repository's intended scope. ### Protocol / Compatibility Impact - [x] No protocol/spec impact in this PR; ADR 0082's consequences (spec allowlist, publish-side pin validation, provisioning) land in the follow-up implementation PR. ## Testing - [x] Lint and format pass - [x] Manual verification performed Describe test evidence: `adr-relations-check` reports 83 ADRs / 184 edges / NO PROBLEMS; prettier and markdownlint clean on `docs/decisions/`; verified via git diff that existing ADRs changed only in frontmatter `relations` (bodies immutable per convention). ## Documentation - [x] Spec/docs updated (this PR is the ADR; index READMEs updated in both languages) - [x] Changelog decision completed above ## Rollout / Risk - Risk level: Low (documentation only) - Rollback plan: revert this branch. ## Reviewer Checklist - [x] Scope is clear and minimal - [x] Security and boundary checks passed - [x] Tests and docs are sufficient - [x] Compatibility impact is correctly handled --------- Signed-off-by: Yunseo Kim <git@yunseo.kim>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why Needed
When publishing with an externally generated provenance bundle under OIDC trusted publishing
(
npm publish <tarball> --provenance-file=<bundle>), npm silently discarded the supplied bundle and publishedits own auto-generated provenance instead. Three layers interacted:
lib/utils/oidc.jsauto-enabled provenance (opts.provenance = true) whenever theprovenanceconfig was atits default, without checking whether a
provenance-filewas supplied.libnpmpublish'sbuildMetadata(), the innerprovenance === truebranch then rangenerateProvenance(),so the
verifyProvenance(subject, provenanceFile)branch, the only code path that reads the supplied file,never executed.
--provenance-file(config-layer mutual exclusivity error, env carve-out, or publishConfig flatten timing).
What Changes
lib/utils/oidc.js: skip auto-enabling provenance when a provenance file is configured (opts.provenanceFile).optsalready carriesprovenanceFilefrom every config source (CLI/env/npmrc/publishConfig) by the time theOIDC flow runs, so this covers all entry paths. The supplied bundle is now verified via
verifyProvenance()andpublished, as documented.
libnpmpublish:buildMetadata()now throwsEPROVENANCECONFLICTwhen bothprovenance: trueandprovenanceFileare provided.provenance/provenance-fileand the libnpmpublish README now state theprecedence rule.
This PR introduces
EPROVENANCECONFLICTin libnpmpublish, thrown when bothprovenance: trueandprovenanceFileare provided programmatically.Rationale: the README already documents the two as mutually exclusive, and silently preferring either direction
discards a cryptographically meaningful artifact. Note the CLI's config layer reports the same conflict as a
TypeErrorwithout an error code (pre-existing). Happy to align onEUSAGEor another convention perreview.
Testing
provenance-fileconfig asserts the published packument'ssigstore attachment deep-equals the supplied bundle (and that sigstore generation is never invoked).
EPROVENANCECONFLICT, no registry PUT,generation not invoked.
References
Fixes #9879
Out of scope (noted for follow-up)
publishConfig.provenance: falsedoes not block the OIDC auto-enable (publishConfig flattens intooptsonly,so
config.isDefault('provenance')stays true). A separate behavioral question aboutisDefaultsemantics.config.set('provenance', true, 'user')bypassing load-time exclusivity is unreachable in this flow after this fix;hardening
config.setitself is an@npmcli/configsemver-major conversation.