Skip to content

fix(ci): stop persisting a write-scoped git credential through npm ci - #51

Open
wyre-agent-fleet[bot] wants to merge 1 commit into
mainfrom
fix/cwe-250-persist-credentials
Open

wyre-agent-fleet[bot] wants to merge 1 commit into
mainfrom
fix/cwe-250-persist-credentials

Conversation

@wyre-agent-fleet

@wyre-agent-fleet wyre-agent-fleet Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Stops persisting a write-scoped git credential through npm ci (CWE-250).

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 dependency install, build and test, readable by any compromised
dependency lifecycle script during that window. persist-credentials: false
is semantic-release's own documented GitHub Actions recipe: it authenticates
its own pushes from GITHUB_TOKEN directly and never needed the persisted
credential.

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.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

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.
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 7abe9e27-a13d-416f-9f50-bbcd44dcf0fd

📥 Commits

Reviewing files that changed from the base of the PR and between e782f30 and b70c6a0.

📒 Files selected for processing (2)
  • .github/workflows/release.yml
  • CHANGELOG.md

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

@wyre-agent-fleet

Copy link
Copy Markdown
Author

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:

  • release.yml: adds persist-credentials: false to the release job's actions/checkout step, with the same explanatory comment across all 18 PRs (CWE-250: the release job's contents: write permission makes checkout's default persisted credential write-scoped and live in .git/config through npm ci; persist-credentials: false is semantic-release's own documented recipe since it authenticates pushes via GITHUB_TOKEN directly). The core fix content is byte-identical across the batch -- raw diff size varies only where a repo's pre-existing checkout step used a different YAML style (inline vs multi-line map).
  • CHANGELOG.md: entry added under this repo's existing Unreleased section, correct per this repo's own convention.
  • No unrelated files touched -- scope is clean.

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 asachs01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Change: Adds persist-credentials: false to the actions/checkout step in the release workflow, plus an inline comment explaining the rationale and a CHANGELOG entry.

Correctness: Verified — the release job declares contents: write, which overrides the repo default read-only workflow permission, so the default checkout credential is write-scoped and would otherwise sit in .git/config through npm ci/build/test. Setting persist-credentials: false is the documented semantic-release mitigation (it auths its own pushes via GITHUB_TOKEN), so the fix directly addresses the described CWE-250 exposure with no loss of functionality.

Security: This is the security fix — closes a window where a compromised dependency lifecycle script during npm ci could read a write-scoped git credential off disk. No new secrets, no injection vectors introduced. Correct placement (under the checkout step, before npm ci runs).

Code quality: Minimal, single-purpose diff. Inline comment is clear and justifies the change well.

Tests: No test coverage possible/needed for a workflow-permission change like this; acceptable for CI config.

Docs/changelog: CHANGELOG entry accurately describes the fix and matches the PR description.

No issues found. Approving.

@asachs01

Copy link
Copy Markdown
Contributor

Code Review Summary — Claude Code

Verdict: Approve

Critical

None

Warnings

None

Suggestions

None

Looks Good

  • Minimal, single-purpose diff scoped to .github/workflows/release.yml: adds persist-credentials: false to the actions/checkout step in the release job.
  • Root cause correctly identified: the job's contents: write permission overrides the repo's read-only default, so the checkout-persisted credential was write-scoped and lived on disk in .git/config through npm ci, build, and test — a real CWE-250 exposure window if any dependency lifecycle script were compromised.
  • Fix is the documented semantic-release recipe: semantic-release authenticates its own pushes via GITHUB_TOKEN and never needed the persisted git credential, so disabling persistence has no functional downside.
  • Inline comment explains the "why" clearly for future maintainers.
  • CHANGELOG.md updated consistently with the change.
  • No test changes needed — this is a CI-config-only fix with no application code impact.

@asachs01 asachs01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@asachs01

Copy link
Copy Markdown
Contributor

Code Review Summary (Reviewed by Hermes Agent)

Looks Good

  • Correctly identifies the gap: the release job's contents: write permission overrides the repo default, so actions/checkout's persisted credential is write-scoped and remains on disk through npm ci/build/test — exploitable by a compromised dependency lifecycle script (CWE-250).
  • persist-credentials: false is the right, minimal fix and matches semantic-release's own documented recipe (it authenticates pushes via GITHUB_TOKEN directly, not the persisted git credential).
  • Change is scoped to the checkout step only; no functional/build behavior affected.
  • CHANGELOG entry documents the change clearly for downstream consumers.

Suggestions

  • Consider adding this as a lint/policy check (e.g. a repo-wide CI guard) across the org's other release workflows so the pattern doesn't need to be reintroduced PR-by-PR per repo.

No security regressions introduced; fix closes the stated gap.

@asachs01

Copy link
Copy Markdown
Contributor

Reviewed against the verified fleet-wide fix pattern: this diff adds persist-credentials: false to the actions/checkout step in .github/workflows/release.yml (with matching CHANGELOG entry), preventing the write-scoped git credential from persisting through npm ci in the release job (CWE-250). semantic-release authenticates its own pushes via GITHUB_TOKEN directly, so this credential was unnecessary and its removal introduces no functional regression. Matches the pattern confirmed in #51. Approved.

Reviewed by Hermes Agent

This was referenced Sep 17, 2026
@asachs01

Copy link
Copy Markdown
Contributor

Review — headRefOid b70c6a0e02efa3529b5a94044028cbc284617ad1

Scope: CI-only fix. Adds persist-credentials: false to the actions/checkout step in the release workflow, plus a CHANGELOG entry. Part of an 18-repo pattern-set fix for CWE-250 (write-scoped git credential persisted through npm ci).

Critical

  • None.

Warnings

  • None — this is exactly the correct, minimal fix for the described issue. persist-credentials: false is the documented semantic-release recipe and doesn't affect fetch-depth: 0's ability to do a full clone (the flag only controls whether the token is written to .git/config afterward, not the checkout itself).

Suggestions

  • Consider adding a repo-wide default (permissions: contents: read at the top level, already implied as in place) plus this same persist-credentials: false pattern to any other workflows in this repo that also check out code, if not already covered, to close the same class of issue everywhere in one pass rather than per-workflow.
  • No automated test can meaningfully cover "a credential wasn't persisted to disk during CI" — acceptable to rely on inspection/config review here; no test gap to flag beyond that.

Looks Good

  • Root cause correctly diagnosed: contents: write job permission overriding checkout's persisted credential scope.
  • Fix matches the well-established pattern already applied to 4 other repos and rolled out consistently to this set of 18.
  • CHANGELOG entry is clear and accurately describes the security rationale.
  • No secrets, source code, or business logic touched.

Verdict: Comment only

@asachs01 asachs01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Verdict: Approve

Correctness & Security

  • Adds persist-credentials: false to the release job's checkout step. Correctly identified: this job declares contents: write (overriding the repo's read-only default workflow permission), so the checkout's default persisted credential is write-scoped and otherwise stays live in .git/config through npm ci, lint, test, and build — readable by any compromised dependency's install/lifecycle scripts. This is CWE-250 and the fix is the standard, semantic-release-documented mitigation (it authenticates its own push via GITHUB_TOKEN directly and never needed the persisted credential).
  • Minimal, surgical diff — one line added in the right place, no behavior change to the rest of the pipeline.

Code Quality

  • Inline comment clearly explains the why, which is good given this is a subtle permissions-inheritance issue that isn't obvious from the workflow file alone.

Tests / Docs

  • No code path changed, so no test impact. CHANGELOG.md entry added consistent with the pattern-set fix across sibling repos.

Consistency

  • Matches the identical fix already applied/proposed across the other 17 repos in this pattern-set (node-domotz, node-spanning, node-scalepad, node-threatlocker, etc.) — no repo-specific quirks that would require a different approach here.

No issues found.


Reviewed SHA: b70c6a0

@asachs01

Copy link
Copy Markdown
Contributor

Claude Code Review

Verdict: Approve

Adds persist-credentials: false to the actions/checkout step in the release workflow, preventing a write-scoped git credential (from the job's contents: write permission) from persisting in .git/config through npm ci/build/test, where a compromised dependency lifecycle script could read and exfiltrate/misuse it (CWE-250). This is semantic-release's own documented recipe, since it authenticates pushes via GITHUB_TOKEN directly and doesn't need the persisted credential.

Critical

None.

Warnings

None in this diff. Given the PR states this is one of 18 sibling repos getting the identical fix, it's worth spot-checking that node-unitrends's release job actually behaves the same way as the others (e.g., no repo-specific customizations to the checkout/release steps that would make persist-credentials: false break something like a subsequent step needing git push access) — but nothing in the diff suggests that's the case here.

Suggestions

  • None beyond what's already good practice — the inline comment explaining the CWE-250 rationale directly in the workflow file is a nice touch and should make future maintainers less likely to "fix" this by reverting it.

Looks Good

  • Correct, minimal, well-justified security hardening.
  • Clear CHANGELOG entry explaining the vulnerability and fix rationale.
  • Consistent with the pattern-set fix already applied/being applied across the other 21 repos mentioned.

@asachs01 asachs01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: false is safe here
  • Changelog entry present

No blocking issues.


Reviewed by Hermes Agent

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