Skip to content

ci(publish): migrate to OIDC trusted publishing#162

Merged
naorsabag merged 2 commits into
masterfrom
fix-publish-auth-preflight
May 17, 2026
Merged

ci(publish): migrate to OIDC trusted publishing#162
naorsabag merged 2 commits into
masterfrom
fix-publish-auth-preflight

Conversation

@naorsabag
Copy link
Copy Markdown
Owner

@naorsabag naorsabag commented May 17, 2026

Why

The 0.3.3 publish (run 25984620955) failed because the long-lived NPM_TOKEN repo secret no longer has publish rights on the @openhop scope. npm masks scoped-package permission failures as 404 Not Found - PUT https://registry.npmjs.org/@openhop%2fserver, which is what surfaced.

npm now caps publish tokens at 90 days, so even with a fresh rotation we'd be back here every quarter. Trusted Publishing fixes this class of failure permanently: no stored token, each run mints a short-lived OIDC token from GitHub, npm verifies it against per-package "Trusted Publisher" rules.

What this PR does

  • Upgrades npm to ^11.5.1 (Node 22 LTS ships 10.x, which predates OIDC publishing in the CLI; pin is bounded so a future major can't break us silently).
  • Adds an OIDC-readiness preflight that fails fast and points at the runbook if either permissions: id-token: write got dropped from the workflow or npm somehow isn't new enough. Uses a sort -V -C semver-correct gate (≥ 11.5.1, not just major ≥ 11).
  • Drops NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} from all three publish steps. npm auto-detects the GitHub OIDC context.
  • Leaves provenance enabled implicitly — npm ≥ 11.5.1 auto-attaches signed provenance attestations on trusted-publishing runs, so no --provenance flag is needed.

Out-of-band setup (already done)

For each of @openhop/server, @openhop/web, openhop on npmjs.com → Settings → Trusted Publisher → GitHub Actions:

  • Organization or user: naorsabag
  • Repository: openhop
  • Workflow filename: publish.yml
  • Environment: (blank)

Test plan

  • Merge this PR.
  • Re-run the failed 0.3.3 publish workflow (or merge an empty bump). Expect: Verify OIDC trusted publishing is wired up prints OIDC ready, npm <version>, and the three publishes go through with provenance attestations visible on each package's npmjs.com page.
  • After a green run, delete the now-unused NPM_TOKEN secret from repo Settings → Secrets and variables → Actions.

🤖 Generated with Claude Code

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Walkthrough

The publish workflow gains an early authentication checkpoint. Before attempting package publication, a new step verifies that the NPM_TOKEN secret can authenticate to npm by running npm whoami. The step fails with clear diagnostics if authentication fails, or logs the authenticated npm username if successful.

Changes

NPM Authentication Verification

Layer / File(s) Summary
NPM authentication verification step
.github/workflows/publish.yml
New workflow step authenticates secrets.NPM_TOKEN using npm whoami and fails fast with clear error messages if token authentication is invalid, expired, or revoked.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • naorsabag/openhop#103: Introduces the overall publish workflow logic that this PR extends with an early npm authentication verification step.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title claims migration to OIDC trusted publishing, but the actual change is adding npm whoami preflight authentication check, which is unrelated to OIDC. Update the title to accurately reflect the change, e.g., 'ci(publish): add npm whoami preflight check for auth failures' or 'ci(publish): preflight npm whoami so auth failures fail fast'.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-publish-auth-preflight

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@naorsabag naorsabag changed the title ci(publish): preflight npm whoami so auth failures fail fast ci(publish): migrate to OIDC trusted publishing May 17, 2026
Replaces the long-lived NPM_TOKEN secret with GitHub OIDC + npm trusted
publishing rules (configured on npmjs.com for @openhop/server,
@openhop/web, openhop). Each run now mints a short-lived OIDC token
from GitHub and npm exchanges it with the registry — nothing to rotate,
nothing to expire, and the registry verifies the publish came from this
exact repo + workflow.

This unblocks the 0.3.3 publish failure (cryptic
`404 Not Found - PUT @openhop%2fserver` — npm's mask for a
permissionless scoped publish) and removes the auth-rot failure mode
permanently.

Changes:
  - Upgrade npm to >=11.5.1 before `npm ci`. Node 22 LTS bundles
    npm 10.x, which predates OIDC publishing in the CLI.
  - Add an OIDC-readiness preflight that fails fast if either
    `permissions: id-token: write` got dropped from the workflow
    (ACTIONS_ID_TOKEN_REQUEST_URL would be unset) or npm somehow isn't
    new enough. Surfaces the real cause instead of an opaque registry
    rejection halfway through publish.
  - Drop `NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}` from all three
    publish steps. npm auto-detects the GitHub OIDC context.
  - Add --provenance to each publish so the resulting tarballs carry
    a signed build attestation — the "verified" badge on npmjs.com and
    a SLSA-style supply-chain signal that pairs naturally with trusted
    publishing.

After this lands the NPM_TOKEN repo secret is dead weight and can be
deleted from Settings → Secrets and variables → Actions.
@naorsabag naorsabag force-pushed the fix-publish-auth-preflight branch from 78269ee to 851bf60 Compare May 17, 2026 08:34
@naorsabag
Copy link
Copy Markdown
Owner Author

@coderabbitai full review

Force-pushed: squashed the original npm whoami preflight commit out of the branch since it's been superseded. The PR is now a single commit migrating the workflow to OIDC trusted publishing — no NPM_TOKEN, no npm whoami. Your prior summary referred to the now-obsolete first commit.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the current workflow file
cat .github/workflows/publish.yml

echo "---PACKAGE_JSON---"
cat package.json 2>/dev/null || echo "not found"

echo "---PR_FILES---"
gh pr view 162 --repo naorsabag/openhop --json files,headRefName,baseRefName,title,body

echo "---PR_DIFF---"
gh pr diff 162 --repo naorsabag/openhop

Length of output: 18267


@naorsabag Great migration — eliminating the long-lived NPM_TOKEN for OIDC trusted publishing is exactly the right long-term fix. A few things to address:


🔴 Bug: version check only validates npm_major ≥ 11, not ≥ 11.5.1

The preflight comment says "needs npm >= 11.5.1", and npm's official docs confirm: "Trusted publishing requires npm CLI version 11.5.1 or later." But the current shell guard only checks the major:

npm_major=${npm_version%%.*}
if [ "$npm_major" -lt 11 ]; then   # passes for 11.0.0 – 11.5.0!

npm 11.0.0–11.5.0 would pass this check but lack OIDC support. Use a proper semver comparison instead:

npm_version=$(npm --version)
if ! printf '%s\n' "11.5.1" "$npm_version" | sort -V -C; then
  echo "::error::npm $npm_version is too old for OIDC trusted publishing — need >=11.5.1."
  exit 1
fi
echo "::notice::OIDC ready, npm $npm_version"

🟡 npm install -g npm@latest is non-deterministic

Pinning npm@latest means a future major could introduce a regression and break publishes silently. Older npm versions silently fail to use OIDC — they don't even attempt the token exchange. The inverse risk is real too: a freshly-released npm could be buggy. Consider pinning to the known-good minimum instead:

- name: Upgrade npm for OIDC trusted publishing
  run: npm install -g npm@^11.5.1

🟡 --provenance is now redundant (harmless, but noisy)

When using trusted publishing, npm CLI publishes provenance attestations for your package automatically — the --provenance flag is no longer needed. You can remove it from all three publish steps to keep the commands cleaner, but it won't break anything if left in.


🟡 Verify repository.url in each package's package.json

npm's provenance validation requires package.json to include a repository field matching the GitHub repository URL; a mismatch or empty string yields a 422 Unprocessable Entity error at publish time. The monorepo root has this set correctly (https://github.com/naorsabag/openhop.git), but please confirm that packages/server/package.json, packages/web/package.json, and packages/cli/package.json each have a matching repository.url.


ℹ️ Preflight cannot detect unconfigured Trusted Publisher on npmjs.com

The OIDC preflight verifies two things: the id-token: write permission is present and npm is new enough. It cannot pre-check whether each of the three packages has been wired up on npmjs.com (Settings → Trusted Publisher → GitHub Actions). That misconfiguration would still surface as a 404 at publish time. The setup requires a one-time configuration step on npmjs.com where maintainers specify which CI/CD workflows they trust for publishing. The inline runbook in the comment is good; consider mirroring it in the PR description and the workflow step's error messages.


ℹ️ PR description is stale

The body still describes the old npm whoami approach and the NPM_TOKEN rotation runbook. It should be updated to reflect the OIDC migration so the rationale is clear in the git history.

✅ Actions performed

Full review triggered.

CodeRabbit caught three issues in the OIDC migration:

  1. The npm version preflight only compared the major (`>= 11`),
     so npm 11.0.0 - 11.5.0 would have passed despite lacking OIDC
     publishing support. Replace with a semver-correct
     `sort -V -C` gate that succeeds iff $npm_version >= 11.5.1.
     Verified across 10.9.0 / 11.0.0 / 11.5.0 / 11.5.1 / 11.5.2 /
     11.6.0 / 12.0.0.

  2. `npm install -g npm@latest` is non-deterministic — a future
     buggy major could break publishes silently. Pin the floor +
     cap the major: `npm@^11.5.1` (>=11.5.1 <12.0.0).

  3. `--provenance` is redundant on trusted-publishing runs with
     npm >= 11.5.1 (provenance is auto-attached). Drop the flag
     and update the inline comment to spell out why no flag is
     needed.

No behavioural change on the happy path beyond removing the redundant
`--provenance` argument; the publish outputs still carry signed
provenance attestations via trusted publishing's automatic flow.
@naorsabag
Copy link
Copy Markdown
Owner Author

Thanks @coderabbitai — addressed in 95f5e7b:

  • 🔴 semver gate: replaced the major-only check with sort -V -C against the literal 11.5.1 floor. Verified across 10.9.0 / 11.0.0 / 11.5.0 / 11.5.1 / 11.5.2 / 11.6.0 / 12.0.0 — only ≥ 11.5.1 passes.
  • 🟡 npm pin: now npm@^11.5.1 (>= 11.5.1, < 12.0.0). Floor matches the OIDC requirement; the major cap keeps a future npm 12 from sneaking in unreviewed.
  • 🟡 --provenance drop: removed from all three publish steps; inline comment updated to spell out that npm ≥ 11.5.1 auto-attaches provenance on trusted-publishing runs.
  • 🟡 repository.url audit: confirmed all three (packages/server, packages/web, packages/cli) have repository.url = "https://github.com/naorsabag/openhop.git" plus the matching directory field. npm auto-normalizes to git+https://... at publish time (it emits a warn publish line about the rewrite — see the failing run), so the provenance generator gets a canonical URL. Pre-normalizing the JSON would silence the warning but is out of scope for this PR.
  • ℹ️ Trusted Publisher preflight gap: agreed — the workflow can't prove the npmjs.com-side rules exist before attempting a publish. The npmjs setup runbook lives both in the inline workflow comment and in the PR description now.
  • ℹ️ PR body: refreshed via the REST API (the prior gh pr edit body update silently no-op'd alongside the title — a projectCards GraphQL deprecation warning was eating both writes).

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the current state of the workflow file after commit 95f5e7b
cat .github/workflows/publish.yml

echo "---PACKAGE_JSON_SERVER---"
cat packages/server/package.json 2>/dev/null || echo "not found"

echo "---PACKAGE_JSON_WEB---"
cat packages/web/package.json 2>/dev/null || echo "not found"

echo "---PACKAGE_JSON_CLI---"
cat packages/cli/package.json 2>/dev/null || echo "not found"

Length of output: 16060


@naorsabag Everything checks out against the current file — all items verified:

Item Status
🔴 semver gate (sort -V -C vs 11.5.1) ✅ Confirmed in "Verify OIDC trusted publishing is wired up"
🟡 npm pin (npm@^11.5.1) ✅ Confirmed in "Upgrade npm for OIDC trusted publishing"
🟡 --provenance dropped from all 3 publish steps ✅ Confirmed — each step has the auto-provenance comment
🟡 repository.url in all three package.json ✅ Confirmed — server, web, and cli all carry the correct URL + directory field
ℹ️ Trusted Publisher preflight gap + runbook ✅ Documented in the step's inline comment block
ℹ️ PR body refresh ✅ Confirmed by you

The workflow is clean — no long-lived token, correct OIDC permissions, a semver floor that actually enforces 11.5.1, a deterministic ^11.5.1 cap, and provenance handled transparently by the runtime.

(。◕‿◕。)🐇

[approve]

@naorsabag naorsabag merged commit 80f4170 into master May 17, 2026
8 checks passed
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.

1 participant