fix(core): add package name validation to updateAllPackages#1208
fix(core): add package name validation to updateAllPackages#1208kagura-agent wants to merge 2 commits intoVoltAgent:mainfrom
Conversation
…t#1205) updateAllPackages concatenated package names from package.json directly into shell commands passed to execSync() without validation, while the sibling updateSinglePackage already had this check. - Extract validation regex into shared VALID_PKG_NAME_RE constant - Filter invalid package names in updateAllPackages before building command - Log warning for skipped packages with invalid names - Reuse constant in updateSinglePackage (dedup)
🦋 Changeset detectedLatest commit: 15c4cd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds npm package name validation to the updater: extracts a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/utils/update/index.ts`:
- Around line 352-356: The filtered packagesToUpdate array can be empty even
when hasUpdates is true, causing execSync to run an install/add with no package
args; update the logic after computing packagesToUpdate (and before
building/executing the install command) to guard against an empty list: if
packagesToUpdate.length === 0, log a clear message (via logger.info or
logger.debug) and skip the install step (either return early from the function
or set hasUpdates=false) so execSync is never invoked with a bare command;
reference the packagesToUpdate variable and the code path that later invokes
execSync for the install/add.
🪄 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
Run ID: a14fb864-e36d-4482-b3ae-60f108d5a2ac
📒 Files selected for processing (2)
.changeset/fix-update-all-packages-injection.mdpackages/core/src/utils/update/index.ts
…mand Addresses CodeRabbit review: after filtering invalid names, packagesToUpdate can be empty while hasUpdates is true, which would run a bare install/add command with no packages.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/utils/update/index.ts (1)
352-355: Consider a single-pass split for valid vs invalid packages.This currently re-checks similar predicates in separate chains. A single pass would reduce drift risk and keep this block easier to maintain.
♻️ Suggested refactor
- const skippedPackages = updateCheckResult.updates.filter( - (pkg) => pkg.type !== "latest" && !VALID_PKG_NAME_RE.test(pkg.name), - ); + const nonLatestUpdates = updateCheckResult.updates.filter( + (pkg) => pkg.type !== "latest", + ); + + const { skippedPackages, packagesToUpdate } = nonLatestUpdates.reduce( + (acc, pkg) => { + if (VALID_PKG_NAME_RE.test(pkg.name)) { + acc.packagesToUpdate.push(`${pkg.name}@latest`); + } else { + acc.skippedPackages.push(pkg); + } + return acc; + }, + { skippedPackages: [] as PackageUpdateInfo[], packagesToUpdate: [] as string[] }, + ); - const packagesToUpdate = updateCheckResult.updates - .filter((pkg) => pkg.type !== "latest") - .filter((pkg) => VALID_PKG_NAME_RE.test(pkg.name)) - .map((pkg) => `${pkg.name}@latest`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/update/index.ts` around lines 352 - 355, The current creation of packagesToUpdate runs multiple array passes over updateCheckResult.updates; replace it with a single-pass loop (e.g., use reduce or a for..of) that checks each pkg once against pkg.type !== "latest" and VALID_PKG_NAME_RE.test(pkg.name) and pushes valid entries as `${pkg.name}@latest` into packagesToUpdate (and optionally collects invalid package names into a separate list) to avoid duplicated predicate evaluation; update the code around packagesToUpdate and reference updateCheckResult.updates and VALID_PKG_NAME_RE accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/utils/update/index.ts`:
- Around line 352-355: The current creation of packagesToUpdate runs multiple
array passes over updateCheckResult.updates; replace it with a single-pass loop
(e.g., use reduce or a for..of) that checks each pkg once against pkg.type !==
"latest" and VALID_PKG_NAME_RE.test(pkg.name) and pushes valid entries as
`${pkg.name}@latest` into packagesToUpdate (and optionally collects invalid
package names into a separate list) to avoid duplicated predicate evaluation;
update the code around packagesToUpdate and reference updateCheckResult.updates
and VALID_PKG_NAME_RE accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a0e0ee3-ebbf-42ee-8154-818b486903a5
📒 Files selected for processing (1)
packages/core/src/utils/update/index.ts
|
Thanks @coderabbitai — addressed in 15c4cd8. Added an early return when |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
updateAllPackages()concatenates package names frompackage.jsondirectly into shell commands passed toexecSync()without validation. The sibling functionupdateSinglePackage()already validates names against a regex, butupdateAllPackages()does not.Changes
VALID_PKG_NAME_RE— the npm package name validation regex previously duplicated between functionsupdateAllPackages: packages with invalid names are now filtered out before command constructionupdateSinglePackageto use the shared constantTesting
Verified the regex correctly:
lodash,@voltagent/core,my-pkgpkg$(curl evil.com),pkg; rm -rf /,pkg && echo pwnedCloses #1205
Summary by cubic
Add npm package name validation to
updateAllPackagesto block command injection and avoid bare installs when only invalid names are found. Aligns behavior withupdateSinglePackage, logs warnings on skipped names, and fixes #1205.VALID_PKG_NAME_REused inupdateAllPackagesandupdateSinglePackage.packagesToUpdateis empty.Written for commit 15c4cd8. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes