Skip to content

fix(update): validate package names in updateAllPackages #1357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asdaxzcwqsa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(update): validate package names in updateAllPackages #1357

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions packages/core/src/utils/update/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ export type PackageUpdateInfo = {
*/
type PackageManager = "npm" | "pnpm" | "yarn" | "bun";

const NPM_PACKAGE_NAME_REGEX =

@cubic-dev-ai cubic-dev-ai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
const NPM_PACKAGE_NAME_REGEX =
const NPM_PACKAGE_NAME_REGEX =
/^(@[a-z0-9~][a-z0-9-._~]*\/)?[a-z0-9~][a-z0-9-._~]*$/;

/^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/;

const isValidPackageName = (packageName: string): boolean =>
NPM_PACKAGE_NAME_REGEX.test(packageName);

/**
* Detects the package manager being used in the project
*/
Expand Down Expand Up @@ -334,8 +340,16 @@ export const updateAllPackages = async (
// 3. Prepare the package list for updating
const packagesToUpdate = updateCheckResult.updates
.filter((pkg) => pkg.type !== "latest")
.filter((pkg) => isValidPackageName(pkg.name))
.map((pkg) => `${pkg.name}@latest`);

if (packagesToUpdate.length === 0) {
return {
success: false,

@cubic-dev-ai cubic-dev-ai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

message: "No valid packages available for updating",
};
Comment on lines +346 to +350

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

}

const logger = new LoggerProxy({ component: "update-checker" });
logger.info(`Updating ${packagesToUpdate.length} packages in ${rootDir}`);

Expand Down Expand Up @@ -410,10 +424,8 @@ export const updateSinglePackage = async (
}

// Command injection protection - only allow valid NPM package names
const isValidPackageName = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/.test(
packageName,
);
if (!isValidPackageName) {
const isPackageNameValid = isValidPackageName(packageName);
if (!isPackageNameValid) {
return {
success: false,
message: `Invalid package name: ${packageName}`,
Expand Down