fix(ci): harden release scripts and workflow (P0 security)#6017
fix(ci): harden release scripts and workflow (P0 security)#6017killagu wants to merge 3 commits into
Conversation
Low-risk, code-only hardening of the release pipeline (stacked on the npm
publish fix). No change to the publish set or versions.
scripts/version.js:
- build the version commit and tag via `execFileSync('git', [...])` argv
instead of an interpolated shell string — the commit message embeds package
names, which was a shell-injection sink
- validate every package name against the npm name grammar before bumping
scripts/publish.js:
- reject any malformed package name before `npm publish` (a typo would publish
a brand-new bogus package)
- refuse to publish a prerelease version to the `latest` dist-tag (last line of
defence against a prerelease becoming the default install)
- isPublished(): distinguish a real 404 (quiet, expected) from network/registry
errors (warn loudly) instead of silently swallowing every failure
- use verbose npm logging only on --dry-run (avoid surfacing auth headers in
real publish logs)
scripts/utils.js:
- add shared isValidNpmPackageName / assertValidNpmPackageName helpers
.github/workflows/release.yml:
- constrain the `branch` input to a `next`/`master` choice, and add a guard
step that re-validates the allowlist at runtime (choice options are not
enforced for API-triggered dispatches) and requires the workflow to be
dispatched from the very branch it releases (dispatch ref == release branch)
Verified: oxfmt + oxlint clean; version.js and publish.js --dry-run still
process all 79 packages with 0 failures; the prerelease->latest guard trips as
expected; release.yml parses.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe release workflow now restricts branch selection and validates dispatch origin, while release scripts validate npm package names, harden version/tag execution, refine publish checks, and update the release decision documentation and wiki index/log. ChangesRelease Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Deploying egg-v3 with
|
| Latest commit: |
ba786ba
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9b797cc6.egg-v3.pages.dev |
| Branch Preview URL: | https://claude-release-p0-hardening.egg-v3.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #6017 +/- ##
=======================================
Coverage 81.94% 81.94%
=======================================
Files 677 677
Lines 20652 20652
Branches 4100 4100
=======================================
Hits 16923 16923
Misses 3215 3215
Partials 514 514 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several security and safety improvements to the release scripts. It adds validation for npm package names to prevent typos or malicious names, blocks publishing prerelease versions to the "latest" tag, handles non-404 errors gracefully when checking if a package is published, and switches from execSync to execFileSync for git commands to avoid shell injection. Additionally, it restricts verbose npm logging to dry runs to prevent leaking auth headers. The review feedback suggests explicitly setting NPM_CONFIG_LOGLEVEL to notice during a real publish to ensure any environment-inherited verbose settings do not leak credentials, and safely handling the caught error in isPublished by checking if err is an instance of Error before accessing err.message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Verbose logging only on dry-run; on a real publish it can surface auth | ||
| // headers in CI logs. | ||
| env: { ...process.env, ...(isDryRun ? { NPM_CONFIG_LOGLEVEL: 'verbose' } : {}) }, |
There was a problem hiding this comment.
To prevent potential credential leakage in CI logs, it is safer to explicitly override NPM_CONFIG_LOGLEVEL to a non-verbose level (such as notice) when not running a dry run. If the environment already has NPM_CONFIG_LOGLEVEL set to verbose, the current implementation would still inherit and use it, potentially exposing sensitive auth headers.
| // Verbose logging only on dry-run; on a real publish it can surface auth | |
| // headers in CI logs. | |
| env: { ...process.env, ...(isDryRun ? { NPM_CONFIG_LOGLEVEL: 'verbose' } : {}) }, | |
| // Verbose logging only on dry-run; on a real publish it can surface auth | |
| // headers in CI logs. Force 'notice' on real publish to prevent leaks. | |
| env: { ...process.env, NPM_CONFIG_LOGLEVEL: isDryRun ? 'verbose' : 'notice' }, |
| // warn loudly. We still return false, but npm's version immutability prevents | ||
| // an accidental overwrite if a publish is then attempted. | ||
| if (!/E404|404 Not Found/i.test(stderr)) { | ||
| console.warn(` ⚠️ could not verify ${name}@${version} on npm: ${stderr.split('\n')[0] || err.message}`); |
There was a problem hiding this comment.
When logging the error, accessing err.message directly can be unsafe if err is not an instance of Error (or is null/undefined). To prevent potential runtime errors and ensure a meaningful error message is always displayed, check if err is an Error instance before accessing message, or use String(err) as a fallback.
const errMsg = err instanceof Error ? err.message : String(err);
console.warn(` ⚠️ could not verify ${name}@${version} on npm: ${stderr.split('\n')[0] || errMsg}`);References
- When creating a new Error from a caught exception, check if the caught value is an
Errorinstance before accessing itsmessageproperty. UseString(err)as a fallback for non-Error values to ensure a meaningful error message.
Deploying egg with
|
| Latest commit: |
ba786ba
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0d53f1a0.egg-cci.pages.dev |
| Branch Preview URL: | https://claude-release-p0-hardening.egg-cci.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/publish.js`:
- Around line 130-132: The publish env setup still inherits a caller-provided
NPM_CONFIG_LOGLEVEL during real publishes, so update the env construction in
publish.js to explicitly override or remove that key when isDryRun is false. Use
the existing isDryRun conditional around the env object to force a non-verbose
loglevel for real publishes, preventing any inherited verbose setting from
leaking into the publish command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71e2c5ce-67bc-407c-8b49-f576fc2385b6
📒 Files selected for processing (4)
.github/workflows/release.ymlscripts/publish.jsscripts/utils.jsscripts/version.js
- publish.js: force NPM_CONFIG_LOGLEVEL=notice on real publishes instead of only conditionally adding verbose for dry-run, so an inherited NPM_CONFIG_LOGLEVEL=verbose can't leak auth headers into CI logs (CodeRabbit/Gemini) - publish.js: guard err.message access for non-Error throws in isPublished() - utils.js: add JSDoc to the new npm-name validation helpers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the review feedback in 5bc567b:
Verified: |
Decision page capturing the target hardened release pipeline (GitHub-Release- triggered, Environment-gated, OIDC pinned, build/publish split, GitHub App token) and the phased migration. P0 of this design landed in the accompanying script / workflow hardening. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wiki/decisions/secure-release-pipeline.md (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language specifier to the diagram code block.
The ASCII architecture diagram uses a fenced code block without a language specifier, which triggers markdownlint
MD040. Addingtextpreserves the diagram formatting while satisfying the linter.-``` +```text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/decisions/secure-release-pipeline.md` at line 90, The architecture diagram in the secure release pipeline decision document uses a fenced code block without a language tag, causing markdownlint MD040. Update the diagram’s fenced block to use a text specifier so the formatting is preserved while satisfying the linter; locate the diagram block in the decision document and adjust the fence accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wiki/decisions/secure-release-pipeline.md`:
- Line 90: The architecture diagram in the secure release pipeline decision
document uses a fenced code block without a language tag, causing markdownlint
MD040. Update the diagram’s fenced block to use a text specifier so the
formatting is preserved while satisfying the linter; locate the diagram block in
the decision document and adjust the fence accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adbcefb1-38e0-4dec-9e42-c868f7169f92
📒 Files selected for processing (3)
wiki/decisions/secure-release-pipeline.mdwiki/index.mdwiki/log.md
✅ Files skipped from review due to trivial changes (1)
- wiki/index.md
Follow-up to #6016 (now merged). This is P0 of a release-security hardening design — deliberately scoped to low-risk, code-only changes. No change to the publish set or versions.
Why
A multi-agent security review of the release pipeline surfaced concrete weaknesses. These are the ones fixable without restructuring the workflow:
scripts/version.jsexecSync(\git commit -m "${msg}"`)` interpolates package names into a shell string → injection sinkexecFileSync('git', [...])argv; validate package namesscripts/publish.jsnpm publishscripts/publish.jslatestdist-taglatest(last line of defence)scripts/publish.jsisPublished()silently swallowed all errors (network/5xx ⇒ "not published")scripts/publish.jsNPM_CONFIG_LOGLEVEL: verboseon real publish may surface auth headers in logs--dry-run.github/workflows/release.ymlbranchwas a free-form string → any ref releasable; reviewing the workflow ≠ what shipsbranch→next/masterchoice+ a runtime guard (choice options aren't enforced for API-triggered dispatches) requiring dispatch-ref == release-branchDeferred to P1 (intentionally not here)
npm publish --ignore-scripts— unsafe as-is:@eggjs/egg-bundlerhasprepublishOnly: npm run build. Belongs with the build/publish job split wheredist/is pre-built + digest-verified.GIT_TOKENPAT with a GitHub App, npm trusted-publisher pinning, and moving to a GitHub-Release-triggered model.Verification
oxfmt --check+oxlint --type-awareclean; pre-commit hook passes.version.js --dry-runandpublish.js --dry-runstill process all 79 packages, 0 failures; name validation does not false-trigger.latestguard correctly refuses and exits non-zero.release.ymlparses; guard step runs first.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
latestdist-tag.Documentation