Skip to content

fix: provenance-file takes precedence over OIDC auto-generated provenance - #9882

Open
yunseo-kim wants to merge 3 commits into
npm:latestfrom
yunseo-kim:fix/provenance-file-precedence
Open

fix: provenance-file takes precedence over OIDC auto-generated provenance#9882
yunseo-kim wants to merge 3 commits into
npm:latestfrom
yunseo-kim:fix/provenance-file-precedence

Conversation

@yunseo-kim

@yunseo-kim yunseo-kim commented Aug 16, 2026

Copy link
Copy Markdown

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 published
its own auto-generated provenance instead. Three layers interacted:

  1. lib/utils/oidc.js auto-enabled provenance (opts.provenance = true) whenever the provenance config was at
    its default, without checking whether a provenance-file was supplied.
  2. In libnpmpublish's buildMetadata(), the inner provenance === true branch then ran generateProvenance(),
    so the verifyProvenance(subject, provenanceFile) branch, the only code path that reads the supplied file,
    never executed.
  3. Every documented way to disable automatic provenance was unusable in combination with --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).
    opts already carries provenanceFile from every config source (CLI/env/npmrc/publishConfig) by the time the
    OIDC flow runs, so this covers all entry paths. The supplied bundle is now verified via verifyProvenance() and
    published, as documented.
  • libnpmpublish: buildMetadata() now throws EPROVENANCECONFLICT when both provenance: true and
    provenanceFile are provided.
  • Docs: config descriptions for provenance / provenance-file and the libnpmpublish README now state the
    precedence rule.

⚠️ New error code (feedback requested)

This PR introduces EPROVENANCECONFLICT in libnpmpublish, thrown when both provenance: true and
provenanceFile are 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
TypeError without an error code (pre-existing). Happy to align on EUSAGE or another convention per
review.

Testing

  • New CLI regression test: OIDC trusted publishing + provenance-file config asserts the published packument's
    sigstore attachment deep-equals the supplied bundle (and that sigstore generation is never invoked).
  • New libnpmpublish test: both options set → rejects with EPROVENANCECONFLICT, no registry PUT,
    generation not invoked.
  • Full root suite green with 100% coverage; libnpmpublish workspace suite green; lint clean.

References

Fixes #9879

Out of scope (noted for follow-up)

  • publishConfig.provenance: false does not block the OIDC auto-enable (publishConfig flattens into opts only,
    so config.isDefault('provenance') stays true). A separate behavioral question about isDefault semantics.
  • config.set('provenance', true, 'user') bypassing load-time exclusivity is unreachable in this flow after this fix;
    hardening config.set itself is an @npmcli/config semver-major 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>
@yunseo-kim
yunseo-kim requested review from a team as code owners August 16, 2026 10:32
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Trusted publishing auto-provenance silently discards the --provenance-file bundle

1 participant