Harden npm trusted publishing workflow#250
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workflow-level ChangesCI/CD Pipeline Hardening and Artifact-Based Release
Sequence Diagram(s)sequenceDiagram
participant BuildJob as build-release-artifact
participant Upload as actions/upload-artifact@<sha>
participant ReleaseJob as release
participant Semantic as cycjimmy/semantic-release-action@<sha>
BuildJob->>Upload: upload `dist` as `npm-package-dist`
ReleaseJob->>Upload: download `npm-package-dist` into `dist`
ReleaseJob->>Semantic: run pinned semantic-release action
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (3)
.github/workflows/validate.yml (3)
78-78: ⚡ Quick winConsider increasing artifact retention period.
The artifact retention is set to only 1 day, which is quite aggressive. If the release job is queued, delayed, or needs to be re-run due to transient failures, the artifact may have expired.
📦 Proposed fix to increase retention
- retention-days: 1 + retention-days: 7GitHub Actions automatically cleans up artifacts after job completion for workflow runs, so a longer retention period provides a safety buffer without significantly increasing storage costs.
🤖 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 @.github/workflows/validate.yml at line 78, The artifact retention is currently set to "retention-days: 1"; increase this value to a safer window (e.g., 7 or 14 days) so artifacts aren't pruned prematurely if the release job is delayed or needs rerun—update the retention-days key in the workflow (validate.yml) to the chosen integer and commit the change.
52-55: ⚖️ Poor tradeoffConditional logic is duplicated across jobs.
The same conditional expression appears in both
build-release-artifact(lines 52-55) andrelease(lines 90-93). This creates a maintenance burden and potential for drift.Consider defining the condition once using a workflow-level environment variable or ensuring the dependency chain (
releasedepends onbuild-release-artifact) implicitly enforces the condition. Sincereleasealready depends onbuild-release-artifact, the condition in thereleasejob may be redundant.Also applies to: 90-93
🤖 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 @.github/workflows/validate.yml around lines 52 - 55, The conditional expression is duplicated between the build-release-artifact and release jobs; remove duplication by centralizing the check: either define a single reusable condition as a workflow-level env/variable (e.g., set an env like RELEASE_CONDITION = "${{ github.repository == 'kentcdodds/kcd-scripts' && contains('refs/heads/main,refs/heads/beta,refs/heads/next,refs/heads/alpha', github.ref) && github.event_name == 'push' }}" and reference it in build-release-artifact and release) or delete the redundant if on the release job (keep the if on build-release-artifact and rely on release depending on build-release-artifact) so only build-release-artifact enforces the condition; update references in validate.yml for the build-release-artifact and release jobs accordingly.
105-109: 💤 Low valueConsider adding artifact integrity verification.
The workflow downloads a pre-built artifact without verifying its integrity. While GitHub Actions provides isolation between jobs, adding an explicit verification step would follow defense-in-depth principles.
🛡️ Optional: Add integrity check after download
- name: 📦 Download package artifact uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: name: npm-package-dist path: dist + + - name: 🔍 Verify artifact contents + run: | + if [ ! -d "dist" ] || [ -z "$(ls -A dist)" ]; then + echo "Error: dist directory is missing or empty" + exit 1 + fi + echo "Artifact contents verified"🤖 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 @.github/workflows/validate.yml around lines 105 - 109, Add an integrity verification step after the "Download package artifact" step that validates the downloaded artifact (name: npm-package-dist, path: dist) before using it: compute a cryptographic checksum (e.g., sha256) of the downloaded files in dist and compare it against a trusted checksum file or a checked-in/retrieved signature, or verify a GPG signature with a known public key; update the workflow to fail the job when the checksum/signature does not match, and reference the actions/download-artifact step and the artifact name npm-package-dist so the verification runs immediately after the download.
🤖 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 @.github/workflows/validate.yml:
- Around line 28-44: Step names in the workflow contain non-ASCII emoji/control
characters (e.g., the "�� Checkout repo", "� Setup node", "� Download
deps", "�� Run validate script", "�� Upload coverage report" step
titles) which break YAML parsing; update those step names to simple ASCII-only
labels (for example "Checkout repo", "Setup node", "Download deps", "Run
validate script", "Upload coverage report") so the YAML is valid for standard
parsers and tools like actionlint while leaving the step actions and inputs
(uses, with, run, matrix.node) unchanged.
- Around line 29-45: Replace the mutable action tags in the main job with pinned
commit SHAs to match the security practice used in build-release-artifact and
release: update the uses lines for actions/checkout@v4, actions/setup-node@v4,
bahmutov/npm-install@v1, and codecov/codecov-action@v3 to their corresponding
immutable commit SHAs (i.e., replace `@vX` with the specific full commit SHA for
each action); ensure you use the same SHA format used elsewhere in the workflow
and commit the updated workflow.
---
Nitpick comments:
In @.github/workflows/validate.yml:
- Line 78: The artifact retention is currently set to "retention-days: 1";
increase this value to a safer window (e.g., 7 or 14 days) so artifacts aren't
pruned prematurely if the release job is delayed or needs rerun—update the
retention-days key in the workflow (validate.yml) to the chosen integer and
commit the change.
- Around line 52-55: The conditional expression is duplicated between the
build-release-artifact and release jobs; remove duplication by centralizing the
check: either define a single reusable condition as a workflow-level
env/variable (e.g., set an env like RELEASE_CONDITION = "${{ github.repository
== 'kentcdodds/kcd-scripts' &&
contains('refs/heads/main,refs/heads/beta,refs/heads/next,refs/heads/alpha',
github.ref) && github.event_name == 'push' }}" and reference it in
build-release-artifact and release) or delete the redundant if on the release
job (keep the if on build-release-artifact and rely on release depending on
build-release-artifact) so only build-release-artifact enforces the condition;
update references in validate.yml for the build-release-artifact and release
jobs accordingly.
- Around line 105-109: Add an integrity verification step after the "Download
package artifact" step that validates the downloaded artifact (name:
npm-package-dist, path: dist) before using it: compute a cryptographic checksum
(e.g., sha256) of the downloaded files in dist and compare it against a trusted
checksum file or a checked-in/retrieved signature, or verify a GPG signature
with a known public key; update the workflow to fail the job when the
checksum/signature does not match, and reference the actions/download-artifact
step and the artifact name npm-package-dist so the verification runs immediately
after the download.
🪄 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 Plus
Run ID: 5a34cdd9-0d28-4ca2-9a6f-94905a492928
📒 Files selected for processing (1)
.github/workflows/validate.yml
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 @.github/workflows/validate.yml:
- Around line 53-55: The branch-check conditional in the validate workflow omits
the 'refs/heads/next-major' branch, causing pushes to that release branch to
skip artifact build/publish; update the contains(...) call in the conditional
that currently lists
'refs/heads/main,refs/heads/beta,refs/heads/next,refs/heads/alpha' to also
include 'refs/heads/next-major' so the expression (${ github.repository ==
'kentcdodds/kcd-scripts' && contains(..., github.ref) && github.event_name ==
'push' }) correctly returns true for next-major; ensure the same branch
identifier is used consistently with the on.push and semantic-release branch
configuration.
🪄 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 Plus
Run ID: 348b9316-13fc-4cab-93b2-22de263da5fa
📒 Files selected for processing (1)
.github/workflows/validate.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/validate.yml (1)
47-126: Excellent supply-chain hardening architecture.The two-stage build-then-publish pattern effectively implements the security improvements described in the PR:
Unprivileged build: The
build-release-artifactjob runs with minimal permissions (contents: read) and uses--ignore-scriptsto prevent compromised dependencies from executing malicious code.Privileged publish isolation: The
releasejob only downloads the pre-built artifact and publishes—no dependency installation or build execution in the privileged context.Action pinning: All actions use immutable commit SHAs rather than mutable tags.
NPM provenance: OIDC-based trusted publishing is correctly configured with
id-token: writepermission.This aligns with supply-chain security best practices following the TanStack compromise.
Additional hardening suggestion: Consider adding SLSA provenance generation (e.g.,
actions/attest-build-provenance) for the uploaded artifact to create a verifiable supply chain attestation.🤖 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 @.github/workflows/validate.yml around lines 47 - 126, Add SLSA provenance attestation for the built artifact by invoking an attestation action (e.g., actions/attest-build-provenance) in the build-release-artifact job after the "🏗 Run build script" step and before the "📦 Upload package artifact" step so the attestation file is generated and uploaded alongside dist; update the "Upload package artifact" step to include the attestation path (e.g., dist/provenance.json) and ensure the release job's "📦 Download package artifact" step also pulls that attestation so the "🚀 Release" step can publish it or include it in provenance metadata.
🤖 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 @.github/workflows/validate.yml:
- Line 64: Remove the invalid input parameter "package-manager-cache" from the
actions/setup-node@v4 step(s); replace or omit it so the step only uses valid
inputs (e.g., the supported "cache" input if you want caching) and ensure any
occurrences of package-manager-cache are deleted (these currently appear
alongside actions/setup-node@v4). Keep the setup-node action configured with
supported keys such as "node-version" and optionally "cache" ("npm", "yarn", or
"pnpm") instead of "package-manager-cache".
---
Nitpick comments:
In @.github/workflows/validate.yml:
- Around line 47-126: Add SLSA provenance attestation for the built artifact by
invoking an attestation action (e.g., actions/attest-build-provenance) in the
build-release-artifact job after the "🏗 Run build script" step and before the
"📦 Upload package artifact" step so the attestation file is generated and
uploaded alongside dist; update the "Upload package artifact" step to include
the attestation path (e.g., dist/provenance.json) and ensure the release job's
"📦 Download package artifact" step also pulls that attestation so the "🚀
Release" step can publish it or include it in provenance metadata.
🪄 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 Plus
Run ID: 705cd4d5-7dc5-425a-b70e-4455c4497b74
📒 Files selected for processing (1)
.github/workflows/validate.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/validate.yml (1)
19-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsblock at workflow level.The workflow currently lacks a top-level
permissionsblock, soGITHUB_TOKENscopes default to repository settings. For consistency (thereleaseandbuild-release-artifactjobs already declare explicit permissions) and to follow least-privilege principles, addpermissions: contents: readat the workflow level.🔒 Minimal fix
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + jobs: main:🤖 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 @.github/workflows/validate.yml around lines 19 - 46, Add a top-level permissions block to the workflow to enforce least-privilege for GITHUB_TOKEN: insert permissions: contents: read at the same indentation level as main (the workflow top-level), so the "main" job inherits only read access; keep existing explicit permissions in the release and build-release-artifact jobs unchanged.
🤖 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.
Outside diff comments:
In @.github/workflows/validate.yml:
- Around line 19-46: Add a top-level permissions block to the workflow to
enforce least-privilege for GITHUB_TOKEN: insert permissions: contents: read at
the same indentation level as main (the workflow top-level), so the "main" job
inherits only read access; keep existing explicit permissions in the release and
build-release-artifact jobs unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc1cea35-9e90-4271-9508-c638f7e5b95d
📒 Files selected for processing (1)
.github/workflows/validate.yml
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
3 participants
Summary
Context
Triggered by the TanStack npm supply-chain compromise postmortem: https://tanstack.com/blog/npm-supply-chain-compromise-postmortem
Testing
Follow-up
A committed lockfile would further improve release reproducibility. I left that out of this Kody-created PR because the generated lockfiles are large; it can be added in a follow-up repo-local commit.
Summary by CodeRabbit