diff --git a/package.json b/package.json index 2761718..2110df4 100644 --- a/package.json +++ b/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", diff --git a/src/controllers/packages.ts b/src/controllers/packages.ts index 4075d43..04c9da2 100644 --- a/src/controllers/packages.ts +++ b/src/controllers/packages.ts @@ -416,9 +416,15 @@ export class PackagesController implements Resource { private async upgradePackage(req: Request, res: Response, next: NextFunction): Promise { 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) { diff --git a/src/services/packages.ts b/src/services/packages.ts index 44f9a18..c315084 100644 --- a/src/services/packages.ts +++ b/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,7 +962,7 @@ export class PackageService { */ async upgradePackage( serverName: string, - version?: string + version?: string | null ): Promise<{ success: boolean package?: PackageInfo @@ -921,13 +970,24 @@ export class PackageService { 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 }) diff --git a/test/packages-injection.spec.ts b/test/packages-injection.spec.ts index dc5ccf7..e49b986 100644 --- a/test/packages-injection.spec.ts +++ b/test/packages-injection.spec.ts @@ -108,6 +108,9 @@ describe('PackageService command-injection hardening', () => { readFileMock.mockResolvedValue('{}') rmMock.mockResolvedValue(undefined) execPromisifiedMock.mockResolvedValue({ stdout: '', stderr: '' }) + // mockResolvedValue REPLACES any per-test mockImplementation set by a + // previous test, so custom implementations (e.g. the pip index/show + // mocks below) cannot leak across tests. execFilePromisifiedMock.mockResolvedValue({ stdout: '', stderr: '' }) }) @@ -158,6 +161,102 @@ describe('PackageService command-injection hardening', () => { expect(result.success).toBe(true) }) + + // The API documents `version` as "optional, defaults to latest". Clients + // (including the GUI) send "" or null to mean "no specific version", and + // before the injection hardening any falsy version installed latest. + // These inputs must NOT be rejected by the version allowlist. + const emptyVersions: Array<[string, unknown]> = [ + ['empty string', ''], + ['whitespace string', ' '], + ['null', null], + ['undefined', undefined] + ] + test.each(emptyVersions)('treats %s version as "install latest"', async (_label, empty) => { + const { service } = createService() + + const result = await service.installPackage({ + name: 'left-pad', + version: empty as string | undefined, + serverName: 'left-pad-server' + }) + + expect(result.success).toBe(true) + + // The install spec must be the bare package name (no trailing @). + const installCall = execFilePromisifiedMock.mock.calls.find(([, args]) => + Array.isArray(args) && (args as string[]).includes('install') + ) + expect(installCall).toBeDefined() + const cmdArgs = installCall![1] as string[] + expect(cmdArgs[cmdArgs.length - 1]).toBe('left-pad') + }) + + // Non-string version types must be rejected, not coerced to strings — + // `true` must never install the npm tag "true". + const nonStringVersions: Array<[string, unknown]> = [ + ['boolean', true], + ['number', 123], + ['object', { version: '1.2.3' }], + ['array', ['1.2.3']] + ] + test.each(nonStringVersions)('rejects non-string version type (%s)', async (_label, bad) => { + const { service } = createService() + + const result = await service.installPackage({ + name: 'left-pad', + version: bad as unknown as string, + serverName: 'left-pad-server' + }) + + expect(result.success).toBe(false) + expect(result.error).toMatch(/version/i) + const installCalls = execFilePromisifiedMock.mock.calls.filter(([, args]) => + Array.isArray(args) && (args as string[]).includes('install') + ) + expect(installCalls.length).toBe(0) + }) + + test('upgradePackage rejects non-string version type', async () => { + const { service, dbMock } = createService() + dbMock.findOne.mockResolvedValue({ + name: 'left-pad', + version: '1.0.0', + installPath: 'packages/left-pad', + status: 'installed', + installed: new Date('2025-01-01T00:00:00.000Z'), + mcpServerId: 'left-pad-server', + enabled: true, + runtime: 'node' + }) + + const result = await service.upgradePackage('left-pad-server', true as unknown as string) + + expect(result.success).toBe(false) + expect(result.error).toMatch(/version/i) + const installCalls = execFilePromisifiedMock.mock.calls.filter(([, args]) => + Array.isArray(args) && (args as string[]).includes('install') + ) + expect(installCalls.length).toBe(0) + }) + + test('trims surrounding whitespace from an otherwise valid version', async () => { + const { service } = createService() + + const result = await service.installPackage({ + name: 'left-pad', + version: ' 1.2.3 ', + serverName: 'left-pad-server' + }) + + expect(result.success).toBe(true) + const installCall = execFilePromisifiedMock.mock.calls.find(([, args]) => + Array.isArray(args) && (args as string[]).includes('install') + ) + expect(installCall).toBeDefined() + const cmdArgs = installCall![1] as string[] + expect(cmdArgs[cmdArgs.length - 1]).toBe('left-pad@1.2.3') + }) }) test('installPackage uses execFile (not shell exec) for npm install with package and version as separate spec arg', async () => { @@ -354,6 +453,36 @@ describe('PackageService command-injection hardening', () => { const cmdArgs = installCalls[0][1] as string[] expect(cmdArgs).toEqual(['install', 'left-pad@latest']) }) + + test('upgradePackage treats empty-string version as "upgrade to latest"', async () => { + const { service, dbMock, mcpMock } = createService() + dbMock.findOne.mockResolvedValue({ ...existingPackage }) + + const serverShape = { + name: 'left-pad-server', + transportType: 'stdio', + command: 'node', + args: ['./packages/left-pad/node_modules/left-pad/index.js'], + env: {}, + status: 'connected', + enabled: true + } + mcpMock.getServer.mockResolvedValue({ ...serverShape }) + mcpMock.disableServer.mockResolvedValue({ ...serverShape, status: 'disconnected', enabled: false }) + mcpMock.enableServer.mockResolvedValue({ ...serverShape }) + mcpMock.updateServer.mockResolvedValue({ ...serverShape }) + readFileMock.mockResolvedValue(JSON.stringify({ version: '2.0.0', main: 'index.js' })) + + const result = await service.upgradePackage('left-pad-server', '') + + expect(result.success).toBe(true) + + const installCall = execFilePromisifiedMock.mock.calls.find(([, args]) => + Array.isArray(args) && (args as string[]).includes('left-pad@latest') + ) + expect(installCall).toBeDefined() + expect(installCall![1]).toEqual(['install', 'left-pad@latest']) + }) }) test('checkForUpdates uses execFile (not shell exec) for npm view', async () => { @@ -457,6 +586,101 @@ describe('PackageService command-injection hardening', () => { expect(installCalls.length).toBe(0) }) + test('checkForUpdates still checks python packages with uppercase names via pip', async () => { + // PyPI names may legally contain uppercase letters; they must not be + // filtered out by the npm name allowlist. + const { service, dbMock } = createService() + dbMock.find.mockResolvedValue([ + { + name: 'MarkupSafe', + version: '2.0.0', + installPath: 'packages/python/markupsafe-server', + venvPath: 'packages/python/markupsafe-server', + status: 'installed', + installed: new Date('2025-01-01T00:00:00.000Z'), + mcpServerId: 'markupsafe-server', + enabled: true, + runtime: 'python' + } + ]) + + execFilePromisifiedMock.mockImplementation(async (...args: unknown[]) => { + const cmdArgs = (args[1] as string[]) ?? [] + if (cmdArgs[0] === 'index' && cmdArgs[1] === 'versions') { + // Mirrors real `pip index versions ` output, which + // pipIndexLatestVersion parses by locating the line starting with + // "Available versions:" and taking the first comma-separated entry. + return { stdout: 'Available versions: 3.0.0, 2.0.0\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + + const result = await service.checkForUpdates() + + expect(result.updates).toEqual([ + { + serverName: 'markupsafe-server', + currentVersion: '2.0.0', + latestVersion: '3.0.0', + updateAvailable: true + } + ]) + }) + + test('upgradePackage allows python packages with uppercase names', async () => { + const { service, dbMock, mcpMock } = createService() + dbMock.findOne.mockResolvedValue({ + name: 'MarkupSafe', + version: '2.0.0', + installPath: 'packages/python/markupsafe-server', + venvPath: 'packages/python/markupsafe-server', + status: 'installed', + installed: new Date('2025-01-01T00:00:00.000Z'), + mcpServerId: 'markupsafe-server', + enabled: false, + runtime: 'python', + pythonModule: 'markupsafe' + }) + mcpMock.getServer.mockResolvedValue({ + name: 'markupsafe-server', + transportType: 'stdio', + command: 'python', + args: ['-u', '-m', 'markupsafe'], + env: {}, + status: 'disconnected', + enabled: false + }) + + execFilePromisifiedMock.mockImplementation(async (...args: unknown[]) => { + const cmdArgs = (args[1] as string[]) ?? [] + if (cmdArgs[0] === 'show') { + return { stdout: 'Name: MarkupSafe\nVersion: 3.0.0\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + + const result = await service.upgradePackage('markupsafe-server') + + expect(result.success).toBe(true) + expect(result.package?.version).toBe('3.0.0') + + // Confirm the pip code path actually ran: the upgrade must go through + // `pip install --upgrade MarkupSafe` and the new version must have been + // read from `pip show MarkupSafe` (not from a node package.json). + const pipInstallCall = execFilePromisifiedMock.mock.calls.find(([, args]) => + Array.isArray(args) && + (args as string[])[0] === 'install' && + (args as string[]).includes('--upgrade') && + (args as string[]).includes('MarkupSafe') + ) + expect(pipInstallCall).toBeDefined() + const pipShowCall = execFilePromisifiedMock.mock.calls.find(([, args]) => + Array.isArray(args) && (args as string[])[0] === 'show' + ) + expect(pipShowCall).toBeDefined() + expect(pipShowCall![1]).toEqual(['show', 'MarkupSafe']) + }) + test('checkForUpdates skips packages with invalid persisted names', async () => { const { service, dbMock } = createService() dbMock.find.mockResolvedValue([ diff --git a/test/packages-python.spec.ts b/test/packages-python.spec.ts index 0998f14..06d38c3 100644 --- a/test/packages-python.spec.ts +++ b/test/packages-python.spec.ts @@ -116,6 +116,26 @@ describe('PackageService python runtime support', () => { expect(result.error).toBe('pythonModule is required for python runtime.') }) + // Boundary rules only: consecutive separators (e.g. `my..pkg`) are + // intentionally NOT rejected here — they are shell-inert and deferred to + // pip for rejection (see PYTHON_NAME_RE in src/services/packages.ts). + test.each(['trailing-dash-', 'trailing-dot.', 'trailing_underscore_', '-leading-dash'])( + 'installPackage rejects python package name with non-alphanumeric boundary %p', + async (badName) => { + const { service } = createService() + + const result = await service.installPackage({ + name: badName, + serverName: 'python-server', + runtime: 'python', + pythonModule: 'my_mcp_server' + }) + + expect(result.success).toBe(false) + expect(result.error).toBe(`Invalid Python package name: ${badName}`) + } + ) + test('installPackage configures stdio server for python runtime with venv python -m module', async () => { const { service, dbMock, mcpMock } = createService()