Repository navigation
ci(release): trust the local registry only after it proves its identity - #449
Merged
Merged
Conversation
The publish checks accepted any successful answer on the registry port, so another local account holding localhost:4873 could serve the packages the checks install and run. Child exit was only noticed after a failed answer. Bind Verdaccio to 127.0.0.1 after checking the port is free, require it to answer with a per-run secret seeded into its private storage, and stop any registry command once it exits. waitFor no longer accepts an answer from a port whose process has exited. After publishing, the registry must serve the exact tarball, and every install is compared file by file with it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 56 minutes. View limit details
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. |
🚀 Deploying Preview to Cloudflare 🚀Preview Deployments by commit
|
|
View your CI Pipeline Execution ↗ for commit f7080a0
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
The publish checks (
verify-publish.mjs,e2e-install.mjs) treated any successful answer on the registry port as "Verdaccio is ready". Another local account holdinglocalhost:4873could then serve the packages these checks install and run. Child exit was only noticed after a failed answer. (CWE-346,scripts/lib/local-registry.mjs:175-177.)Changes in
scripts/lib/local-registry.mjs:127.0.0.1instead oflocalhost, and the run checks the port is free before Verdaccio starts.waitForno longer accepts an answer once its process has exited, and every registry-backedrun/startfails if Verdaccio has exited.publish, the registry must list the tarball's sha512 and serve its exact bytes. Every install (both scenarios inverify-publish.mjs, eachcheckInstalledine2e-install.mjs, and the package audit) is compared file by file with the tarball viaverifyInstalled.publishis now async, so its callersawaitit.How it was verified
pnpm commit:check(commit messages follow the guidelines): passedpnpm format:check: N/A. Prettier--writewas run on the 3 changed files; the full check was not run.pnpm typecheck(includes thengctemplate checks): N/A, only.mjsscripts changed;node --checkpasses on all threepnpm test,pnpm test:devtoolsandpnpm test:panel: N/A, no package or panel code changedpnpm skills:check(when.claude/changed): N/Aapps/docsupdated andpnpm docs:buildpasses (when behavior, options, UI labels or agent tools changed), or theno-docslabel added with the reason below: N/A, internal scripts onlypnpm extension:buildandextension/uicommitted (whenapp/changed): N/AAlso run:
node scripts/e2e-install.mjs --audit-only: passed. It packs, starts Verdaccio, publishes, installs, and the installed package matches the tarball.127.0.0.1:4873: aborts with "127.0.0.1:4873 is taken" and exit code 1.readmeback.The full scenario matrix (all package managers) was not run locally. CI will cover it.
Screenshots
N/A
Notes for reviewers
SO_REUSEPORT), it could still answer some requests after the secret check. The tarball checks cover the package under test, but not other packages, like@angular/cli, that the registry proxies from npmjs.npxpath: thenpx/dlxrun in the CLI scenario is covered by the served-tarball check, not byverifyInstalled, because that package isn't installed into the app.verifyInstalledcompares every file byte for byte. If a package manager rewrites a file such aspackage.jsonon install, the check will fail and show which file.