fix(ci): stop persisting a write-scoped git credential through npm ci - #44
wyre-agent-fleet[bot] wants to merge 1 commit into
Conversation
The release job declares `contents: write`, which overrides this repo's read-only default workflow permission, so actions/checkout's default persisted credential was write-scoped and stayed live in .git/config through npm ci / build / test -- readable by any compromised dependency lifecycle script. persist-credentials: false is semantic-release's own documented recipe; it authenticates its pushes from GITHUB_TOKEN directly. Part of the CWE-250 pattern-set fix across WYRE-AI/node-*. Sibling PRs: node-spanning#46, node-domotz#48, node-kaseya-quote-manager#16, node-alternative-payments#20 (already merged/merging), plus this repo and 17 others in the same follow-up set.
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Comment |
|
Peer review (comment-only, per the shared-identity self-approval gate). Verdict: approve, no blockers. Verified independently against the actual diff content (not just warden's filing summary), as part of the 18-PR CWE-250 follow-up set-review:
Full batch (all independently verified): node-axcient#2, node-blumira#41, node-clio#2, node-connectwise-cpq#2, node-datto-bcdr#55, node-datto-saas-protection#55, node-huntress#43, node-iqms#39, node-kaseya-bms#48, node-kaseya-vsa#50, node-mailprotector#2, node-mimecast#44, node-ncentral#4, node-proofpoint-essentials#1, node-rootly#27, node-scalepad#2, node-threatlocker#31, node-unitrends#51 |
asachs01
left a comment
There was a problem hiding this comment.
Review: stop persisting write-scoped git credential through npm ci
Verified fix is correct and matches the described issue:
- Release job grants
contents: write, which upgrades the defaultactions/checkoutcredential to write-scoped and (without this fix) that credential stays live in.git/configthroughnpm ci/build/test — exposed to any compromised dependency lifecycle script (CWE-250). persist-credentials: falseis the correct, minimal mitigation and matches semantic-release's own documented recipe, since semantic-release authenticates pushes viaGITHUB_TOKENdirectly and never reads the persisted credential.- Diff is scoped to the
actions/checkoutstep only — no unrelated changes, no regressions to the release/publish flow.
Security: no new secrets, no injection surface; this closes a credential-exposure window rather than opening one.
Docs: No CHANGELOG.md entry, and that's appropriate here — this repo has no Keep-a-Changelog ## [Unreleased] scaffolding to hang a manual entry off of (purely semantic-release auto-generated), consistent with the stated precedent (node-domotz#48).
Tests: none needed — CI workflow config change, verified by inspection.
Approving.
Code Review Summary — Claude CodeVerdict: Approve CriticalNone WarningsNone Suggestions
Looks Good
|
asachs01
left a comment
There was a problem hiding this comment.
Reviewed by Claude Code. Adds persist-credentials: false to the release job's checkout step, eliminating the write-scoped git credential (from contents: write) that otherwise stayed live in .git/config through npm ci/build/test; semantic-release already authenticates via GITHUB_TOKEN directly, so no needed auth is removed. No CHANGELOG scaffolding exists in this repo to append an entry to, which is reasonable.
Code Review Summary (Reviewed by Hermes Agent)Security fix verified: Critical: None. Warnings: None. No secrets are logged or exposed; no auth bypass introduced. Suggestions:
Looks good. Approving from a review-comment perspective (no formal PR review submitted per bot policy). |
|
Reviewed against the verified fleet-wide fix pattern: this diff adds Reviewed by Hermes Agent |
Review — headRefOid
|
asachs01
left a comment
There was a problem hiding this comment.
Reviewed via automated sweep.
Correctness: The fix is sound. contents: write on the release job escalates actions/checkout's default persisted credential to write-scope, and it otherwise stays live in .git/config for the duration of npm ci/build/test — readable by any compromised dependency install/lifecycle script (classic CWE-250 exposure). Adding persist-credentials: false closes that window.
Verified no regression: checked the full release.yml — the release step authenticates via npx semantic-release with GITHUB_TOKEN/NPM_TOKEN env vars (this repo also uses a GitHub Packages registry auth token written to .npmrc for the build job, unrelated to git credentials), not the persisted git credential. No other git push/git tag step relies on it. Clean drop-in with no functional side effects.
Security: genuine, low-risk hardening fix — the standard, documented semantic-release recipe for this exact scenario.
Scope/quality: single-purpose, minimal diff, good inline comment explaining the "why". No CHANGELOG entry, per the PR description this repo has no Unreleased scaffolding to hang a manual entry off — reasonable, not a blocker. No tests needed for a CI workflow config change.
Verdict: approve.
Reviewed SHA: 98b76a0
Claude Code ReviewVerdict: Approve SummaryAdds Correctness
Security
Code Quality
Tests / Docs
Looks Good
|
asachs01
left a comment
There was a problem hiding this comment.
Hermes Agent Review
Verdict: Approve
Correct, minimal fix for CWE-250 (write-scoped git credential left live in .git/config through npm ci). persist-credentials: false is the documented semantic-release recipe since it authenticates its own pushes via GITHUB_TOKEN — no functional regression, changelog entry included, scoped to the affected job only.
Looks Good
- Fix is correctly scoped (only the release job, which is the one declaring
contents: write) - Rationale comment inline explains the CWE and why
persist-credentials: falseis safe here - Changelog entry present
No blocking issues.
Reviewed by Hermes Agent
Stops persisting a write-scoped git credential through
npm ci(CWE-250).The release job declares
contents: write, which overrides this repo'sread-only default workflow permission — so
actions/checkout's defaultpersisted credential was write-scoped and stayed live in
.git/configthrough dependency install, build and test, readable by any compromised
dependency lifecycle script during that window.
persist-credentials: falseis semantic-release's own documented GitHub Actions recipe: it authenticates
its own pushes from
GITHUB_TOKENdirectly and never needed the persistedcredential.
No CHANGELOG.md entry: this repo has no Keep-a-Changelog
## [Unreleased]scaffolding (purely semantic-release auto-generated, same shape asnode-domotz— seenode-domotz#48's review comment for the detailed precedent). Nowhere existing to add a manual entry without inventing new structure.Part of a pattern-set fix across WYRE-AI/node-*. Originally found and
fixed on 4 repos (node-spanning#46, node-domotz#48,
node-kaseya-quote-manager#16, node-alternative-payments#20), then a
propagation scope-check found the same pattern on 18 more repos. Full set
(18, this repo included) so reviewers can see membership:
node-axcient, node-blumira, node-clio, node-connectwise-cpq, node-datto-bcdr, node-datto-saas-protection, node-huntress, node-iqms, node-kaseya-bms, node-kaseya-vsa, node-mailprotector, node-mimecast, node-ncentral, node-proofpoint-essentials, node-rootly, node-scalepad, node-threatlocker, node-unitrends
Generated by warden's Gate-3 CWE-250 review, per boss's set-completeness ruling.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.