fix(update): validate package names in updateAllPackages#1357
fix(update): validate package names in updateAllPackages#1357asdaxzcwqsa wants to merge 1 commit into
Conversation
|
📝 WalkthroughWalkthroughA shared npm package-name regex constant ( ChangesPackage-name validation centralization and updateAllPackages security fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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 `@packages/core/src/utils/update/index.ts`:
- Around line 346-350: The new `success: false` return from `updateAllPackages`
is being swallowed by the API layer, so clients still see success. Update
`packages/server-core/src/handlers/update.handlers.ts` in the `update` handler
to inspect the result returned by `await updateAllPackages()` and forward its
`success` and `message` instead of always responding with `success: true`. Keep
the behavior aligned with the `updateAllPackages` early return path for “No
valid packages available for updating” so the handler surfaces failures
correctly.
🪄 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: 19e657ea-09cc-4468-9e12-55976ed4dc11
📒 Files selected for processing (1)
packages/core/src/utils/update/index.ts
| if (packagesToUpdate.length === 0) { | ||
| return { | ||
| success: false, | ||
| message: "No valid packages available for updating", | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
success: false early-return is currently masked by the API handler.
Line 346 introduces a new { success: false } path, but packages/server-core/src/handlers/update.handlers.ts always returns success: true after await updateAllPackages() without checking the returned result. This means clients will still get a success response when this branch is hit. Either propagate the returned success/message in the handler or make this path fail in a way the handler surfaces.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@packages/core/src/utils/update/index.ts` around lines 346 - 350, The new
`success: false` return from `updateAllPackages` is being swallowed by the API
layer, so clients still see success. Update
`packages/server-core/src/handlers/update.handlers.ts` in the `update` handler
to inspect the result returned by `await updateAllPackages()` and forward its
`success` and `message` instead of always responding with `success: true`. Keep
the behavior aligned with the `updateAllPackages` early return path for “No
valid packages available for updating” so the handler surfaces failures
correctly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/utils/update/index.ts">
<violation number="1" location="packages/core/src/utils/update/index.ts:35">
P1: Package name regex allows leading dashes, permitting argument injection</violation>
<violation number="2" location="packages/core/src/utils/update/index.ts:348">
P2: This new `success: false` early-return path may not be surfaced to clients if the upstream handler (`update.handlers.ts`) always returns `success: true` after calling `updateAllPackages()` without inspecting the result. Either the handler should propagate the returned `success`/`message`, or this function should fail in a way the handler already surfaces (e.g., throwing an error).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| */ | ||
| type PackageManager = "npm" | "pnpm" | "yarn" | "bun"; | ||
|
|
||
| const NPM_PACKAGE_NAME_REGEX = |
There was a problem hiding this comment.
P1: Package name regex allows leading dashes, permitting argument injection
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/utils/update/index.ts, line 35:
<comment>Package name regex allows leading dashes, permitting argument injection</comment>
<file context>
@@ -32,6 +32,12 @@ export type PackageUpdateInfo = {
*/
type PackageManager = "npm" | "pnpm" | "yarn" | "bun";
+const NPM_PACKAGE_NAME_REGEX =
+ /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/;
+
</file context>
| const NPM_PACKAGE_NAME_REGEX = | |
| const NPM_PACKAGE_NAME_REGEX = | |
| /^(@[a-z0-9~][a-z0-9-._~]*\/)?[a-z0-9~][a-z0-9-._~]*$/; |
|
|
||
| if (packagesToUpdate.length === 0) { | ||
| return { | ||
| success: false, |
There was a problem hiding this comment.
P2: This new success: false early-return path may not be surfaced to clients if the upstream handler (update.handlers.ts) always returns success: true after calling updateAllPackages() without inspecting the result. Either the handler should propagate the returned success/message, or this function should fail in a way the handler already surfaces (e.g., throwing an error).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/utils/update/index.ts, line 348:
<comment>This new `success: false` early-return path may not be surfaced to clients if the upstream handler (`update.handlers.ts`) always returns `success: true` after calling `updateAllPackages()` without inspecting the result. Either the handler should propagate the returned `success`/`message`, or this function should fail in a way the handler already surfaces (e.g., throwing an error).</comment>
<file context>
@@ -334,8 +340,16 @@ export const updateAllPackages = async (
+ if (packagesToUpdate.length === 0) {
+ return {
+ success: false,
+ message: "No valid packages available for updating",
+ };
</file context>
1 participant
Summary
updateAllPackages()before building shell commandsProblem
updateAllPackages()was interpolating package names fromcheckForUpdates()directly intoexecSync()commands, whileupdateSinglePackage()already validated names first. That left a command-injection gap between the two code paths.Notes
This keeps the fix intentionally small and follows the validation pattern already used in
updateSinglePackage().Fixes #1205
Summary by cubic
Validate package names in
updateAllPackages()to close a command-injection gap and avoid empty package-manager runs. Reuses a shared regex helper and returns early when no valid packages remain.isValidPackageNamehelper and use it inupdateAllPackages().updateSinglePackage()to use the shared helper for consistency.Written for commit d5e39d7. Summary will update on new commits.
Summary by CodeRabbit