fix: restore "empty version means latest" behavior broken by PR #44 hardening

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@missionsquad/mcp-api",
-  "version": "1.11.10",
+  "version": "1.11.11",
   "description": "MCP Servers exposed via HTTP API",
   "main": "dist/index.js",
   "repository": "missionsquad/mcp-api",

src/controllers/packages.ts

@@ -416,9 +416,15 @@ export class PackagesController implements Resource {
   private async upgradePackage(req: Request, res: Response, next: NextFunction): Promise<void> {
     try {
       const serverName = req.params.name
-      const version = req.body.version as string | undefined
-
-      log({ level: 'info', msg: `Upgrading package ${serverName}${version ? ' to version ' + version : ''}` })
+      // The cast does not validate the body at runtime — PackageService
+      // deliberately re-checks the version's type and format (see
+      // isVersionInputTypeValid), so that guard is load-bearing, not
+      // redundant with this annotation. `null` is included because JSON
+      // bodies can carry {"version": null}.
+      const version = req.body.version as string | null | undefined
+
+      const versionForLog = typeof version === 'string' ? version.trim() : ''
+      log({ level: 'info', msg: `Upgrading package ${serverName}${versionForLog ? ' to version ' + versionForLog : ''}` })
 
       const result = await this.packageService.upgradePackage(serverName, version)
       if (result.success) {

src/services/packages.ts

@@ -115,6 +115,41 @@ export class PackageService {
   // passed to `.cmd`/`.bat` files.
   private static readonly NPM_VERSION_SPEC_RE = /^(?!-)[A-Za-z0-9.\-+~^<>=|*x_]+$/
 
+  // Python (PyPI) package names: letters (either case), digits, `_`, `.`, `-`,
+  // starting AND ending with an alphanumeric (single-character names allowed),
+  // matching PEP 508's boundary rules. Used both at python install time and to
+  // re-validate persisted names before they reach a pip subprocess.
+  // Consecutive separators (e.g. `my..pkg`) are INTENTIONALLY accepted and
+  // deferred to pip for rejection: every character in such names is still
+  // shell-inert and passed via execFile argv, so they pose no
+  // subprocess-argument risk — they are merely non-canonical per PEP 508 and
+  // fail at pip-resolution time with pip's own error.
+  private static readonly PYTHON_NAME_RE = /^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$/
+
+  // The public API documents `version` as "optional, defaults to latest".
+  // Callers commonly send `""` or `null` to mean "no specific version", and
+  // the pre-validation code treated any falsy version that way. Normalize
+  // those inputs to `undefined` (and trim whitespace) BEFORE validating, so
+  // the strict allowlist only ever rejects versions the caller actually set.
+  // Non-string types are NOT coerced — callers must reject them (see
+  // isVersionInputTypeValid) so `true`/`123` cannot sneak past the allowlist
+  // as stringified tags.
+  private static normalizeVersionInput(version: string | null | undefined): string | undefined {
+    if (version === undefined || version === null) {
+      return undefined
+    }
+    const trimmed = version.trim()
+    return trimmed.length === 0 ? undefined : trimmed
+  }
+
+  private static isVersionInputTypeValid(version: unknown): version is string | null | undefined {
+    return version === undefined || version === null || typeof version === 'string'
+  }
+
+  private static isValidPythonPackageName(name: string): boolean {
+    return typeof name === 'string' && PackageService.PYTHON_NAME_RE.test(name)
+  }
+
   private static isValidNpmName(name: string): boolean {
     if (typeof name !== 'string' || name.length === 0 || name.length > 214) {
       return false
@@ -404,7 +439,6 @@ export class PackageService {
   }> {
     const {
       name,
-      version,
       serverName,
       transportType,
       command,
@@ -416,6 +450,16 @@ export class PackageService {
       enabled = true,
       failOnWarning = false
     } = request
+    // Reject non-string version types outright (booleans, numbers, objects
+    // in the JSON body) rather than coercing them to strings, then treat
+    // empty/null versions as "latest" (see normalizeVersionInput).
+    if (!PackageService.isVersionInputTypeValid(request.version)) {
+      return {
+        success: false,
+        error: `Invalid package version: ${String(request.version)}. Version must be a valid npm semver, range, or tag.`
+      }
+    }
+    const version = PackageService.normalizeVersionInput(request.version)
     const resolvedTransportType: MCPTransportType = transportType ?? 'stdio'
     const runtime: PackageRuntime = request.runtime ?? 'node'
     const serverMetadata = this.buildServerMetadataInput(request)
@@ -427,7 +471,7 @@ export class PackageService {
       if (!/^[a-zA-Z0-9_.]+$/.test(request.pythonModule)) {
         return { success: false, error: `Invalid pythonModule: ${request.pythonModule}` }
       }
-      if (!/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(name)) {
+      if (!PackageService.isValidPythonPackageName(name)) {
         return { success: false, error: `Invalid Python package name: ${name}` }
       }
       if (resolvedTransportType !== 'stdio') {
@@ -828,14 +872,19 @@ export class PackageService {
           // Skip packages without mcpServerId
           if (!pkg.mcpServerId) continue
 
-          // Skip packages whose stored name would fail the strict allowlist.
-          // `pkg.name` originates from the install-time request and is
-          // re-validated here as defense-in-depth before it reaches the
-          // npm subprocess.
-          if (!PackageService.isValidNpmName(pkg.name)) {
+          // Re-validate the persisted name with the allowlist for the
+          // package's runtime before it reaches a subprocess. Python names
+          // may legally contain uppercase letters, so they must NOT be
+          // checked against the npm allowlist (that would skip legitimate
+          // python packages installed before this check existed).
+          const nameIsValidForRuntime =
+            pkg.runtime === 'python'
+              ? PackageService.isValidPythonPackageName(pkg.name)
+              : PackageService.isValidNpmName(pkg.name)
+          if (!nameIsValidForRuntime) {
             log({
               level: 'warn',
-              msg: `Skipping update check for package with invalid name: ${pkg.name}`
+              msg: `Skipping update check for package with invalid ${pkg.runtime ?? 'node'} name: ${pkg.name}`
             })
             updates.push({
               serverName: pkg.mcpServerId,
@@ -913,21 +962,32 @@ export class PackageService {
    */
   async upgradePackage(
     serverName: string,
-    version?: string
+    version?: string | null
   ): Promise<{
     success: boolean
     package?: PackageInfo
     server?: MCPServer
     error?: string
   }> {
     try {
+      // Reject non-string version types outright (the controller passes
+      // req.body.version through without validating its type), then treat
+      // empty/null versions as "latest" (see normalizeVersionInput).
+      if (!PackageService.isVersionInputTypeValid(version)) {
+        return {
+          success: false,
+          error: `Invalid package version: ${String(version)}. Version must be a valid npm semver, range, or tag.`
+        }
+      }
+      const normalizedVersion = PackageService.normalizeVersionInput(version)
+
       // Validate version against the same strict allowlist used for install,
       // since this value is passed verbatim to npm. Without this check, a
       // caller-controlled version would otherwise reach the npm command line.
-      if (version !== undefined && !PackageService.isValidNpmVersionSpec(version)) {
+      if (normalizedVersion !== undefined && !PackageService.isValidNpmVersionSpec(normalizedVersion)) {
         return {
           success: false,
-          error: `Invalid package version: ${version}. Version must be a valid npm semver, range, or tag.`
+          error: `Invalid package version: ${normalizedVersion}. Version must be a valid npm semver, range, or tag.`
         }
       }
 
@@ -937,11 +997,22 @@ export class PackageService {
         return { success: false, error: `Package ${serverName} not found` }
       }
 
-      // Re-validate the persisted package name. Although names are validated
-      // at install time, defending the npm invocation here makes the upgrade
-      // path safe even if a record predates the install-time check or was
-      // written through some other path.
-      if (!PackageService.isValidNpmName(packageInfo.name)) {
+      // Re-validate the persisted package name with the allowlist for the
+      // package's runtime. Although names are validated at install time,
+      // defending the subprocess invocation here makes the upgrade path safe
+      // even if a record predates the install-time check or was written
+      // through some other path. Python names may legally contain uppercase
+      // letters, so they are checked against the python allowlist, not npm's.
+      const packageRuntime: PackageRuntime = packageInfo.runtime ?? 'node'
+      const nameIsValidForRuntime =
+        packageRuntime === 'python'
+          ? PackageService.isValidPythonPackageName(packageInfo.name)
+          : PackageService.isValidNpmName(packageInfo.name)
+      if (!nameIsValidForRuntime) {
+        log({
+          level: 'warn',
+          msg: `Refusing to upgrade ${serverName}: persisted package name ${packageInfo.name} fails the ${packageRuntime} name allowlist`
+        })
         return {
           success: false,
           error: `Invalid package name on existing record: ${packageInfo.name}.`
@@ -967,13 +1038,12 @@ export class PackageService {
       }
 
       try {
-        const runtime: PackageRuntime = packageInfo.runtime ?? 'node'
-        if (runtime === 'python') {
+        if (packageRuntime === 'python') {
           const venvAbsolutePath = packageInfo.venvPath
             ? path.resolve(process.cwd(), packageInfo.venvPath)
             : path.resolve(process.cwd(), packageInfo.installPath)
 
-          const spec = version ? `${packageInfo.name}==${version}` : packageInfo.name
+          const spec = normalizedVersion ? `${packageInfo.name}==${normalizedVersion}` : packageInfo.name
           await this.pipInstall(
             venvAbsolutePath,
             spec,
@@ -1009,7 +1079,7 @@ export class PackageService {
 
         // Perform the upgrade. Pass the package@version spec as a single argument
         // to execFile-based runNpm so shell metacharacters can never be interpreted.
-        const upgradeSpec = `${packageInfo.name}@${version ?? 'latest'}`
+        const upgradeSpec = `${packageInfo.name}@${normalizedVersion ?? 'latest'}`
         log({ level: 'info', msg: `Upgrading package: npm install ${upgradeSpec}` })
         const upgradeResult = await this.runNpm(['install', upgradeSpec], { cwd: packageDir })