chore: minor security improvements - #1149
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
The registry setting does not prevent inherited scope-specific registries from redirecting scoped dependencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens dependency installation, build reproducibility, and workflow supply-chain security.
Changes:
- Restricts dependency scripts and pins the npm registry.
- Pins external GitHub Actions and adds Dependabot updates.
- Locks Anchor builds and updates release tooling.
File summaries
| File | Description |
|---|---|
.npmrc |
Pins the default registry. |
pnpm-workspace.yaml |
Blocks dependency build scripts. |
pnpm-lock.yaml |
Locks updated release tooling. |
package.json |
Updates release tools to v1.2.0. |
packages/dynamic-client/package.json |
Adds locked Anchor builds. |
.github/dependabot.yml |
Enables action updates. |
.github/workflows/main.yml |
Pins external actions. |
.github/workflows/cut.yml |
Pins the reusable workflow. |
.github/workflows/promote.yml |
Pins the reusable workflow. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -0,0 +1,2 @@ | |||
| # Pin resolution to the official registry so an inherited global or org config cannot redirect it. | |||
| registry=https://registry.npmjs.org/ | |||
There was a problem hiding this comment.
0c065fa
Slightly overstated, but right that this only covers the default registry - comment narrowed to say so. Not maintaining a scope list: 23 scopes, mostly transitive, and a new one would be silently unprotected. Integrity hashes in the lockfile plus --frozen-lockfile are the actual control.
| packages: | ||
| - "packages/*" | ||
|
|
||
| # No dependency may run install or build scripts. |
There was a problem hiding this comment.
| # No dependency may run install or build scripts. | |
| minimumReleaseAge: 10080 | |
| # No dependency may run install or build scripts. |
I'd suggest adding a delay for the dependencies to sync with the delay for dependabot.
But this requires updating to pnpm@10.16
There was a problem hiding this comment.
Yeah, i deliberately decided not to add this one in current changes.
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Supply-chain hardening, no runtime code changes: pins the default npm registry via a new .npmrc, makes the pnpm "no dependency build scripts" policy explicit with onlyBuiltDependencies: [], pins all third-party actions and the release-tools reusable workflows to full commit SHAs (with version comments), adds a github-actions Dependabot ecosystem so those pins keep moving, bumps @codama/release-tools from v1.0.0 to v1.2.0, and adds --locked to the Anchor test-program build.
All sensible. A few things worth a second look before merging:
Things to watch out for
-
Two different SHAs are labelled
v1.2.0forrelease-tools. The workflows pincc0b8c9589e43c869233e2533dacb35f25b188e4 # v1.2.0, whilepnpm-lock.yamlresolvedgithub:codama-idl/release-tools#v1.2.0to97d4c3bdd1bf5623cdc53a083bf2a00bf24843c5. pnpm resolves through the GitHub API to the peeled commit, so if the tag is annotated the workflow value might be the tag object rather than the commit (whichuses:won't accept), or the two were taken from different refs. I couldn't reach therelease-toolsrepo to check which one is right — a quickgit ls-remote https://github.com/codama-idl/release-tools v1.2.0would settle it. (Detail inline oncut.yml;promote.ymlL10 has the same pin.) -
Dependabot auto-merge now covers action bumps. The existing
dependabotjob inmain.ymlauto-approves and auto-merges any Dependabot PR whoseupdate-typeis minor or patch. With the newgithub-actionsecosystem grouping minor/patch bumps into one PR, SHA updates topnpm/action-setup,changesets/action,dependabot/fetch-metadataand therelease-toolsreusable workflows will land without a human looking at them. The 7-day cooldown mitigates the "compromised tag" scenario a lot, but it's worth making sure that's a deliberate trade-off — the whole point of SHA pinning is that a human decides when the SHA moves. If you'd rather keep eyes on those, the job could skip whensteps.metadata.outputs.package-ecosystem == 'github_actions'. (Inline ondependabot.yml.) -
First-party actions are left on tags.
actions/checkout@v7,actions/setup-node@v7andactions/create-github-app-token@v3(unchanged lines inmain.yml) are still floating. That's a common and defensible line to draw foractions/*, butcreate-github-app-tokenis the step that handles the release app's private key, so it's arguably the one most worth pinning. Dependabot would maintain it the same way as the others. Not blocking — just flagging the inconsistency so it's a choice rather than an omission. -
anchor:build --lockedisn't exercised in CI. Nothing inmain.ymlruns the Anchor build, so this change is only validated by whoever runsanchor:sync:buildlocally.anchor build -- <args>forwards tocargo build-sbf, which should pass--lockedthrough to cargo, but worth confirming it actually took effect (e.g. it fails if you tweak a version inCargo.tomlwithout updating the lock).test/programs/anchor/Cargo.lockis committed, so the flag has something to lock against. -
onlyBuiltDependencies: []— as the description says, this is pnpm 10's default behaviour, so the change is declarative. One thing to check: pnpm 10 prints an "Ignored build scripts: … runpnpm approve-builds" warning after install for packages that wanted to run scripts (esbuild etc.). If an empty allowlist doesn't suppress it,ignoredBuiltDependenciesis the idiomatic way to record the per-package decision and silence the nag.
Notes for subsequent reviewers
- I could not independently verify that the
pnpm/action-setup,changesets/actionanddependabot/fetch-metadataSHAs match their# vX.Y.Zcomments. If someone hasghhandy,gh api repos/<owner>/<repo>/git/ref/tags/<tag>for each is a 30-second check and the most valuable thing to do on this PR. .npmrc's scoped-registry caveat (raised by Copilot) is already acknowledged in the file comment; the lockfile integrity hashes remain the real control. Nothing further needed there.- The lockfile still reports
version: 1.0.0for therelease-toolstarball — that's justrelease-tools' ownpackage.jsonnot being bumped on tag, not a problem for this repo, but it's why the tag→SHA mapping in point 1 deserves a look rather than being assumed.
Summary
.npmrc- pin resolution tohttps://registry.npmjs.org/.pnpm-workspace.yaml- addonlyBuiltDependencies: []. (pnpm 10 already blocks dependency install and build scripts by default).dependabot.yml- add thegithub-actionsso the new SHA pins do not rot.@codama/release-tools- bumpedv1.0.0->v1.2.0to match the workflow pin.anchor:build- add--locked.