fix: stop requiring npm tokens#39
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR updates action/version references to v3, adds OIDC trusted publishing (making Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Action as npm-publish-pr Action
participant OIDC as OIDC Provider
participant NPM as npm Registry
GH->>Action: start (no `npm-token`)
Action->>OIDC: request ID token
OIDC-->>Action: return ID token
Action->>NPM: run `npm publish --provenance` (with ID token)
NPM-->>Action: publish result
Action->>GH: set outputs (version, package-name) and (optionally) comment
sequenceDiagram
participant GH as GitHub Actions
participant Action as npm-publish-pr Action
participant PM as Package Manager (npm/yarn/pnpm)
participant NPM as npm Registry
GH->>Action: start (with `npm-token`)
Action->>Action: detect package manager (lockfile)
Action->>PM: authenticate with token & publish
PM->>NPM: publish request
NPM-->>PM: publish result
PM-->>Action: return version
Action->>GH: set outputs (version, package-name) and (optionally) comment
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
🚀 Integration Test Resultsnpm-pr-version action test:
The action is working correctly! 🎉 |
🧪 Test Results✅ All tests passed successfully!
|
codfish
force-pushed
the
npm-trusted-publishing
branch
from
4b32fbf to
edc4419
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
34f040f to
f286d9a
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
f286d9a to
a237428
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
a237428 to
df06ea9
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
df06ea9 to
439dada
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
439dada to
f168d93
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
f168d93 to
fa8fe3f
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
070ff46 to
7ef0016
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
7ef0016 to
e91f672
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
e91f672 to
7041140
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
6d815a3 to
1c8f77b
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
1c8f77b to
5d30266
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
codfish
force-pushed
the
npm-trusted-publishing
branch
from
5d30266 to
e36c776
Compare
|
Claude finished @codfish's task in 4m 20s —— View job 🎯 Comprehensive Code Review CompleteI've completed a thorough review of this PR which adds OIDC trusted publishing support for npm packages. Overall, this is a well-implemented feature with excellent documentation. Below is my detailed analysis across all requested areas. 📊 SummaryOverall Assessment: ✅ Approve with Minor Improvements Recommended This PR successfully implements OIDC trusted publishing while maintaining backward compatibility with token-based authentication. The documentation is comprehensive and the feature is well-thought-out. However, there are a few issues that should be addressed before merging. Key Strengths
Issues Found
🔒 1. Security Analysis✅ StrengthsOIDC Implementation
Documentation
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
codfish
force-pushed
the
npm-trusted-publishing
branch
from
e36c776 to
5c21876
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
comment/README.md (1)
20-32:⚠️ Potential issue | 🟡 MinorAdd required outputs markers to this action README.
Each action README must include outputs markers even if there are no outputs to list.
As per coding guidelines, each action README must contain inputs/outputs markers: ``/`` and ``/``.➕ Add outputs section
@@ <!-- end inputs --> +## Outputs + +<!-- start outputs --> + +*No outputs* + +<!-- end outputs --> + ## Examplesbin/generate-docs.js (1)
59-88:⚠️ Potential issue | 🟠 MajorReplace filename-based file reads with file descriptors to prevent TOCTOU vulnerabilities.
The
extractUsageExamplemethod (line 62-67) has a classic TOCTOU pattern:fs.existsSyncis followed byfs.readFileSync, allowing the file to be deleted or modified between the check and read. TheparseActionFilemethod (line 39) similarly needs to use file descriptors for consistency. Both should usefs.openSync/fs.readSyncwith proper error handling and cleanup infinallyblocks, following the pattern already established elsewhere in this file (lines 217-240 and 295-320).🔒 Suggested fix for `extractUsageExample`
- try { - const content = fs.readFileSync(readmeFile, 'utf8'); + let fd; + try { + fd = fs.openSync(readmeFile, 'r'); + const stats = fs.fstatSync(fd); + const buffer = Buffer.alloc(stats.size); + fs.readSync(fd, buffer, 0, stats.size, 0); + const content = buffer.toString('utf8'); @@ - } catch (error) { + } catch (error) { console.error(`Error reading README for ${dirName}:`, error.message); return this.generateBasicExample(dirName); + } finally { + if (fd !== undefined) { + try { + fs.closeSync(fd); + } catch (closeError) { + console.error(`Error closing README for ${dirName}:`, closeError.message); + } + } }Apply the same pattern to
parseActionFile(line 35-54).npm-publish-pr/action.yml (1)
39-39:⚠️ Potential issue | 🟡 MinorComment action still references
@v2instead of@v3.The PR updates documentation to reference
v3for all actions, but this action.yml still usescodfish/actions/comment@v2in three places. This should likely be updated for consistency.✏️ Suggested fix
- - uses: codfish/actions/comment@v2 + - uses: codfish/actions/comment@v3Apply this change at lines 39, 215, and 227.
Also applies to: 215-215, 227-227
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 173-175: The workflow example in README.md uses inconsistent
GitHub Action refs: change uses: actions/checkout@v3 to match other examples
(actions/checkout@v6), and replace uses:
codfish/actions/setup-node-and-install@main with a stable tag (e.g., `@v3`) to
match the repository's convention; update both action refs in the snippet so all
examples use the same pinned versions (actions/checkout and
codfish/actions/setup-node-and-install).
In `@setup-node-and-install/README.md`:
- Around line 141-145: The README example uses an undefined input
"cache-key-suffix" which is not declared in setup-node-and-install/action.yml;
either remove the "cache-key-suffix" line from the example in README.md or add a
matching input entry named "cache-key-suffix" to action.yml (with a description
and default) so the example and the action inputs agree; search for the README
snippet showing "uses: codfish/actions/setup-node-and-install@v3" and the
action.yml inputs to update the inconsistency.
🧹 Nitpick comments (1)
README.md (1)
162-166: Security consideration withpull_request_targetand OIDC.Using
pull_request_targetwithid-token: writecan be risky if the workflow checks out and runs code from the PR branch (e.g.,actions/checkoutwithout explicit ref). This could allow malicious PRs from forks to steal the OIDC token. The npm-publish-pr/README.md correctly shows usingpull_requestwith a fork check condition as a safer pattern.Consider adding a note about the security implications or switching the example to use
pull_requestwith a fork check.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 6
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
codfish
force-pushed
the
npm-trusted-publishing
branch
from
5c21876 to
a43d806
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
npm-publish-pr/action.yml (1)
39-39:⚠️ Potential issue | 🟡 MinorInconsistent
commentaction version.The action uses
codfish/actions/comment@v2while the rest of the PR has migrated to v3. Consider updating for consistency.✏️ Suggested fix
- - uses: codfish/actions/comment@v2 + - uses: codfish/actions/comment@v3Apply this change at lines 39, 208, and 220.
Also applies to: 208-208, 220-220
🤖 Fix all issues with AI agents
In `@npm-publish-pr/action.yml`:
- Around line 176-183: In the case "$package_manager" branch for "yarn", remove
the redundant --new-version $version flag from the yarn publish invocation so it
reads a plain yarn publish --access public --tag pr --no-git-tag-version
--skip-check-working-tree (preserving capture of publish_output and
publish_exit_code and the existing call to handle_publish_error), ensuring
consistency with the npm/pnpm publish behavior that relies on the package.json
version already set earlier.
In `@npm-publish-pr/README.md`:
- Around line 223-232: The inputs table was edited inside the auto-generated
HTML block delimited by <!-- start inputs --> and <!-- end inputs --> in
README.md; revert any manual changes inside that block, update the upstream
generator or source-of-truth that produces the inputs table (not the README
directly), then re-generate the README so the correct table is produced; ensure
future edits to inputs/outputs/doctoc blocks are made in the generator files
rather than editing content between any HTML comment markers.
- Around line 238-244: The README's OIDC trusted publishing command string is
out of sync: update the documentation text that currently shows "npm publish
--access public --tag pr" to include the provenence flag so it reads "npm
publish --access public --tag pr --provenance"; locate the sentence block
describing OIDC trusted publishing (the bullet list under "When `npm-token` is
not provided") and replace the Command line accordingly so docs match the
implementation.
🧹 Nitpick comments (1)
npm-publish-pr/action.yml (1)
50-50: Consider stricter bash error handling.Using
set +edisables exit-on-error for the entire script. A more robust approach is to use strict mode (set -euo pipefail) at the start and handle specific commands that may fail using|| trueor explicit conditionals.However, given the current structure with explicit exit code checks after each critical command, the current approach is functional. This is a minor suggestion for future consideration.
BREAKING CHANGE: `npm` is upgraded to v11 in setup-node-and-install (pinned to ^11.5.1). This may affect workflows with npm v10-specific dependencies (rare). - Upgrade npm to 11.5.1+ in setup-node-and-install for OIDC compatibility - Migrate to actions/setup-node@v6 - Make npm-token optional (OIDC used when not provided) - Update all documentation with OIDC usage examples and authentication modes test: token test: token
codfish
force-pushed
the
npm-trusted-publishing
branch
from
a43d806 to
88701a1
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
npm-pr-versionno longer requiresnpm-token. Ifnpm-tokenis omitted it now publishes via OIDC trusted publishing usingnpm publish --provenance, validatesid-token: writeavailability, and tightens bash error handling; token-based mode still detectsnpm/yarn/pnpm.setup-node-and-installnow upgrades npm for OIDC. It moves toactions/setup-node@v6, adds a configurableupgrade-npmstep (default on) pinned tonpm@^11.5.1, and exposes additional outputs.https://docs.npmjs.com/trusted-publishers
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.