fix(ci): stop persisting a write-scoped git credential through npm ci - #27
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
Change: Adds persist-credentials: false to the actions/checkout step in the release workflow, plus an inline comment explaining the rationale. No CHANGELOG.md change in this PR.
Correctness: Verified — the release job declares contents: write, overriding the repo default read-only workflow permission, so the default checkout credential was write-scoped and persisted through npm ci/build/test. persist-credentials: false is the correct, documented semantic-release mitigation and is placed at the right point (under checkout, before npm ci).
Security: This is the fix itself — closes the CWE-250 exposure window where a compromised dependency lifecycle script could read the write-scoped credential off disk. No new secrets or injection vectors.
Code quality: Minimal, single-purpose diff with a clear explanatory comment.
Tests: Not applicable for a CI permission change.
Docs/changelog: No CHANGELOG.md entry, but the PR description explains this repo has no Keep-a-Changelog ## [Unreleased] scaffolding to hang an entry on (purely semantic-release auto-generated changelog) — reasonable, verified there is no existing manual-entry section to use, consistent with precedent cited (node-domotz#48).
No issues found. Approving.
Code Review Summary — Claude CodeVerdict: Approve CriticalNone WarningsNone SuggestionsNone Looks Good
|
asachs01
left a comment
There was a problem hiding this comment.
Reviewed by Claude Code. Adds persist-credentials: false to actions/checkout in the release workflow so the write-scoped git credential (from this job's contents: write permission) no longer lingers in .git/config through npm ci; semantic-release still authenticates pushes via GITHUB_TOKEN, so nothing breaks. Safe and correctly scoped.
Code Review Summary (Reviewed by Hermes Agent)Looks Good
Suggestions
No security regressions introduced; fix closes the stated gap. |
|
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/NODE_AUTH_TOKEN env vars, not the persisted git credential, and there's no other git push/git tag step relying on it. So this is a clean drop-in with no functional side effects.
Security: this is a 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 tests needed for a CI workflow config change.
Verdict: approve.
Reviewed SHA: 6e59207
Claude Code ReviewVerdict: Approve CorrectnessAdds SecurityCorrect fix for CWE-250. No new risk introduced. Code QualityInline comment is clear and consistent with the other repos in this pattern-set. TestsN/A — CI workflow config only. DocsNo CHANGELOG.md entry, but the PR description explains why: this repo has no Keep-a-Changelog Looks GoodCorrect, minimal, well-justified security fix. Safe to merge. |
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.