diff --git a/CHANGELOG.md b/CHANGELOG.md index ec93124..b769e81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.2.0-alpha.3] + +### Security + +- **Process-invocation hardening.** All Azure CLI (`az`), Azure Developer CLI + (`azd`), and local dev-server child processes are now launched through a single + shared, shell-free process launcher that resolves platform executable shims + (including Windows `.cmd`/`.bat`) without spawning a command interpreter. Command + arguments are never routed through a shell, so shell metacharacters in + user-influenced values (for example subscription IDs and resource-group names) + can no longer alter the executed command line. In addition, Azure subscription + IDs and resource-group names are now strictly validated against an allowlist at + the tool boundary and re-asserted at the process-invocation boundary as + defense-in-depth. Valid inputs and existing behavior are unchanged. + ## [0.2.0-alpha.1] ### Added diff --git a/package-lock.json b/package-lock.json index 1e4094a..cfc43a0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,17 +1,18 @@ { "name": "@microsoft/spe-mcp", - "version": "0.2.0-alpha.1", + "version": "0.2.0-alpha.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@microsoft/spe-mcp", - "version": "0.2.0-alpha.1", + "version": "0.2.0-alpha.3", "license": "MIT", "dependencies": { "@azure/msal-node": "^5.4.1", "@modelcontextprotocol/sdk": "^1.27.1", "commander": "^15.0.0", + "cross-spawn": "^7.0.6", "open": "^11.0.0", "zod": "^4.4.3", "zod-to-json-schema": "^3.25.2" @@ -23,6 +24,7 @@ "@eslint/js": "^10.0.1", "@microsoft/microsoft-graph-types": "2.43.1", "@microsoft/microsoft-graph-types-beta": "0.44.0-preview", + "@types/cross-spawn": "^6.0.6", "@types/node": "^26.1.1", "@typescript-eslint/eslint-plugin": "^8.57.2", "@typescript-eslint/parser": "^8.57.2", @@ -747,6 +749,16 @@ "assertion-error": "^2.0.1" } }, + "node_modules/@types/cross-spawn": { + "version": "6.0.6", + "resolved": "https://ms-feed-2.pkgs.visualstudio.com/1es-public/_packaging/npm-public/npm/registry/@types/cross-spawn/-/cross-spawn-6.0.6.tgz", + "integrity": "sha1-AWPQt5pvhUCeDey43MoXFH+B/SI=", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/deep-eql": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/@types/deep-eql/-/deep-eql-4.0.2.tgz", @@ -1457,8 +1469,8 @@ }, "node_modules/cross-spawn": { "version": "7.0.6", - "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", - "integrity": "sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==", + "resolved": "https://ms-feed-25.pkgs.visualstudio.com/1es-public/_packaging/npm-public/npm/registry/cross-spawn/-/cross-spawn-7.0.6.tgz", + "integrity": "sha1-ilj+ePANzXDDcEUXWd+/rwPo7p8=", "license": "MIT", "dependencies": { "path-key": "^3.1.0", diff --git a/package.json b/package.json index 7db0ffd..c611908 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@microsoft/spe-mcp", - "version": "0.2.0-alpha.1", + "version": "0.2.0-alpha.3", "description": "SharePoint Embedded MCP Server — manage container types, containers, and content via any MCP client", "keywords": [ "mcp", @@ -56,6 +56,7 @@ "@azure/msal-node": "^5.4.1", "@modelcontextprotocol/sdk": "^1.27.1", "commander": "^15.0.0", + "cross-spawn": "^7.0.6", "open": "^11.0.0", "zod": "^4.4.3", "zod-to-json-schema": "^3.25.2" @@ -64,6 +65,7 @@ "@eslint/js": "^10.0.1", "@microsoft/microsoft-graph-types": "2.43.1", "@microsoft/microsoft-graph-types-beta": "0.44.0-preview", + "@types/cross-spawn": "^6.0.6", "@types/node": "^26.1.1", "@typescript-eslint/eslint-plugin": "^8.57.2", "@typescript-eslint/parser": "^8.57.2", diff --git a/server.json b/server.json index d7cee05..da487dd 100644 --- a/server.json +++ b/server.json @@ -2,7 +2,7 @@ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-12-11/server.schema.json", "name": "com.microsoft/sharepointembedded-mcp-server", "description": "MCP server for SharePoint Embedded management", - "version": "0.1.0-alpha.1", + "version": "0.2.0-alpha.3", "repository": { "url": "https://github.com/microsoft/SharePoint-Embedded-MCP-Server", "source": "github" @@ -13,7 +13,7 @@ "registryType": "npm", "registryBaseUrl": "https://registry.npmjs.org", "identifier": "@microsoft/spe-mcp", - "version": "0.1.0-alpha.1", + "version": "0.2.0-alpha.3", "runtimeHint": "npx", "transport": { "type": "stdio" diff --git a/src/azure-cli.proc.test.ts b/src/azure-cli.proc.test.ts new file mode 100644 index 0000000..adb2fb8 --- /dev/null +++ b/src/azure-cli.proc.test.ts @@ -0,0 +1,165 @@ +// Process-invocation contract for the Azure CLI helpers. +// +// These tests pin two boundary guarantees at the `az` process seam, independent +// of the higher-level tool handlers: +// 1. Reject-before-spawn: a malformed, externally influenced identifier is +// refused BEFORE any child process is launched (the proc-exec seam is never +// reached). +// 2. Discrete argv: a valid — but shell-sensitive — identifier that legitimately +// reaches `az` is passed as ONE discrete argv element, and the options bag +// never requests a shell. +// +// The launcher (`proc-exec`) is mocked so the argv/opts that each helper builds +// are observable without shelling out. + +import { describe, it, expect, beforeEach, vi } from "vitest"; + +vi.mock("./proc-exec.js", () => ({ + runCommand: vi.fn(), +})); + +import { runCommand } from "./proc-exec.js"; +import { + listResourceGroups, + resourceGroupExists, + showSyntexProvider, + registerSyntexProvider, +} from "./azure-cli.js"; + +const run = vi.mocked(runCommand); + +// A canonical valid subscription id (strict GUID) and a valid resource-group +// name that nonetheless contains shell-significant punctuation `.()` — a good +// "legal but shell-sensitive" argument. +const VALID_SUBSCRIPTION_ID = "3fa85f64-5717-4562-b3fc-2c963f66afa6"; +const VALID_RESOURCE_GROUP = "rg-spe-demo_01.(prod)"; + +// Subscription ids are validated as strict GUIDs, so ANY non-GUID string is +// rejected — including plain words and shell metacharacter payloads. +const MALFORMED_SUBSCRIPTION_IDS = [ + "not-a-guid", + "--query", + "a b", + "3fa85f64-5717-4562-b3fc-2c963f66afa6 &", + "$()", + "``", + "sub |", + "sub ;", +]; + +// Resource-group names accept letters/digits/`_.()-`, so a plain word like +// "not-a-guid" is VALID and must NOT appear here — only names that fail the +// allowlist (whitespace, shell metacharacters, path traversal, flag-lookalikes). +const MALFORMED_RESOURCE_GROUP_NAMES = [ + "rg &", + "rg |", + "rg $()", + "``", + "--query", + "a b", + "rg/../", + "rg ;", +]; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("azure-cli process seam — reject before spawn", () => { + it.each(MALFORMED_SUBSCRIPTION_IDS)( + "listResourceGroups rejects a malformed subscription id (%j) before spawning", + async (badId) => { + await expect(listResourceGroups(badId)).rejects.toThrow(); + expect(run).not.toHaveBeenCalled(); + }, + ); + + it.each(MALFORMED_SUBSCRIPTION_IDS)( + "showSyntexProvider rejects a malformed subscription id (%j) before spawning", + async (badId) => { + await expect(showSyntexProvider(badId)).rejects.toThrow(); + expect(run).not.toHaveBeenCalled(); + }, + ); + + it.each(MALFORMED_SUBSCRIPTION_IDS)( + "registerSyntexProvider rejects a malformed subscription id (%j) before spawning", + async (badId) => { + await expect(registerSyntexProvider(badId)).rejects.toThrow(); + expect(run).not.toHaveBeenCalled(); + }, + ); + + it.each(MALFORMED_RESOURCE_GROUP_NAMES)( + "resourceGroupExists refuses a malformed group name (%j) without spawning", + async (badName) => { + // Non-throwing probe: an invalid input resolves to `undefined` (indeterminate) + // and must never reach the process seam. + await expect(resourceGroupExists(badName, VALID_SUBSCRIPTION_ID)).resolves.toBeUndefined(); + expect(run).not.toHaveBeenCalled(); + }, + ); + + it.each(MALFORMED_SUBSCRIPTION_IDS)( + "resourceGroupExists refuses a malformed subscription id (%j) without spawning", + async (badId) => { + await expect(resourceGroupExists(VALID_RESOURCE_GROUP, badId)).resolves.toBeUndefined(); + expect(run).not.toHaveBeenCalled(); + }, + ); +}); + +describe("azure-cli process seam — discrete argv, no shell", () => { + it("passes a valid subscription id to az as one discrete argv element", async () => { + run.mockResolvedValue({ stdout: "[]", stderr: "" }); + + await listResourceGroups(VALID_SUBSCRIPTION_ID); + + expect(run).toHaveBeenCalledTimes(1); + const [cmd, args, opts] = run.mock.calls[0] as unknown as [ + string, + string[], + Record, + ]; + expect(cmd).toBe("az"); + expect(args).toEqual([ + "group", + "list", + "--subscription", + VALID_SUBSCRIPTION_ID, + "--output", + "json", + ]); + // The id is exactly one element — never concatenated into a shell string. + expect(args[3]).toBe(VALID_SUBSCRIPTION_ID); + expect(opts).not.toHaveProperty("shell"); + }); + + it("passes a punctuation-bearing resource-group name to az as one discrete argv element", async () => { + run.mockResolvedValue({ stdout: "{}", stderr: "" }); + + const exists = await resourceGroupExists(VALID_RESOURCE_GROUP, VALID_SUBSCRIPTION_ID); + + expect(exists).toBe(true); + expect(run).toHaveBeenCalledTimes(1); + const [cmd, args, opts] = run.mock.calls[0] as unknown as [ + string, + string[], + Record, + ]; + expect(cmd).toBe("az"); + expect(args).toEqual([ + "group", + "show", + "--name", + VALID_RESOURCE_GROUP, + "--subscription", + VALID_SUBSCRIPTION_ID, + "--output", + "json", + ]); + // The `.()`-bearing name stays a single argv element. + expect(args[3]).toBe(VALID_RESOURCE_GROUP); + expect(opts).not.toHaveProperty("shell"); + }); +}); diff --git a/src/azure-cli.ts b/src/azure-cli.ts index dfc3b42..7d842b2 100644 --- a/src/azure-cli.ts +++ b/src/azure-cli.ts @@ -11,12 +11,16 @@ * first-party app or pre-authorization required. */ -import { execFile } from "node:child_process"; import { randomUUID } from "node:crypto"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { getSignedInIdentity } from "./bootstrap.js"; +import { runCommand } from "./proc-exec.js"; +import { + assertAzureSubscriptionId, + assertAzureResourceGroupName, +} from "./validation.js"; import { isConditionalAccessOrClaimsError, asConditionalAccessError, @@ -25,10 +29,6 @@ import { const AZ_TIMEOUT_MS = 30_000; -function azNeedsShell(): boolean { - return process.platform === "win32"; -} - /** * Best-effort tenant id from the current `az` sign-in, used to interpolate the * exact re-auth command into Conditional Access guidance. Never throws — a @@ -56,25 +56,11 @@ function isNotInstalled(message: string): boolean { const NOT_INSTALLED_MSG = "Azure CLI ('az') is not installed. Install it from https://aka.ms/install-azure-cli, then run `az login --allow-no-subscriptions`."; -function execFileAsync( - cmd: string, - args: string[], - opts: { timeout: number; shell?: boolean }, -): Promise<{ stdout: string; stderr: string }> { - return new Promise((resolve, reject) => { - execFile(cmd, args, opts, (err, stdout, stderr) => { - if (err) reject(err); - else resolve({ stdout, stderr }); - }); - }); -} - /** Run an `az` command with `--output json` appended and parse the result. */ export async function azJson(args: string[]): Promise { try { - const { stdout } = await execFileAsync("az", [...args, "--output", "json"], { + const { stdout } = await runCommand("az", [...args, "--output", "json"], { timeout: AZ_TIMEOUT_MS, - shell: azNeedsShell(), }); return JSON.parse(stdout) as T; } catch (error) { @@ -123,9 +109,8 @@ export async function listSubscriptions(): Promise { */ export async function isSignedIn(): Promise { try { - await execFileAsync("az", ["account", "show", "--output", "json"], { + await runCommand("az", ["account", "show", "--output", "json"], { timeout: AZ_TIMEOUT_MS, - shell: azNeedsShell(), }); return true; } catch (error) { @@ -141,6 +126,7 @@ export async function isSignedIn(): Promise { /** List resource groups in a subscription. */ export async function listResourceGroups(subscriptionId: string): Promise { + assertAzureSubscriptionId(subscriptionId); return azJson(["group", "list", "--subscription", subscriptionId]); } @@ -163,10 +149,12 @@ export async function resourceGroupExists( subscriptionId: string, ): Promise { try { - await execFileAsync( + assertAzureResourceGroupName(name); + assertAzureSubscriptionId(subscriptionId); + await runCommand( "az", ["group", "show", "--name", name, "--subscription", subscriptionId, "--output", "json"], - { timeout: AZ_TIMEOUT_MS, shell: azNeedsShell() }, + { timeout: AZ_TIMEOUT_MS }, ); return true; } catch (error) { @@ -206,6 +194,7 @@ function defaultSleep(ms: number): Promise { export async function showSyntexProvider( subscriptionId: string, ): Promise { + assertAzureSubscriptionId(subscriptionId); return azJson([ "provider", "show", "--namespace", SYNTEX_NAMESPACE, "--subscription", subscriptionId, ]).catch(() => null); @@ -214,10 +203,11 @@ export async function showSyntexProvider( /** Trigger registration of the Syntex RP (async on the Azure side). */ export async function registerSyntexProvider(subscriptionId: string): Promise { try { - await execFileAsync( + assertAzureSubscriptionId(subscriptionId); + await runCommand( "az", ["provider", "register", "--namespace", SYNTEX_NAMESPACE, "--subscription", subscriptionId], - { timeout: AZ_TIMEOUT_MS, shell: azNeedsShell() }, + { timeout: AZ_TIMEOUT_MS }, ); } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -406,9 +396,8 @@ interface SyntexAccountRequestBody { /** Run `az rest ... --output json` and parse the response body. */ async function azRestJson(args: string[]): Promise { try { - const { stdout } = await execFileAsync("az", ["rest", ...args, "--output", "json"], { + const { stdout } = await runCommand("az", ["rest", ...args, "--output", "json"], { timeout: AZ_TIMEOUT_MS, - shell: azNeedsShell(), }); const out = stdout.trim(); return (out ? JSON.parse(out) : {}) as T; @@ -436,7 +425,7 @@ async function putSyntexAccountViaAz( const bodyFile = join(dir, "account.json"); try { writeFileSync(bodyFile, JSON.stringify(body), "utf-8"); - const { stdout } = await execFileAsync( + const { stdout } = await runCommand( "az", [ "rest", @@ -446,7 +435,7 @@ async function putSyntexAccountViaAz( "--body", `@${bodyFile}`, "--output", "json", ], - { timeout: AZ_TIMEOUT_MS, shell: azNeedsShell() }, + { timeout: AZ_TIMEOUT_MS }, ); const out = stdout.trim(); return (out ? JSON.parse(out) : { id: "" }) as SyntexAccount; @@ -476,6 +465,8 @@ export async function getSyntexAccounts( subscriptionId: string, resourceGroup: string, ): Promise { + assertAzureSubscriptionId(subscriptionId); + assertAzureResourceGroupName(resourceGroup); const url = `${ARM_BASE}/subscriptions/${subscriptionId}/resourceGroups/${resourceGroup}` + `/providers/Microsoft.Syntex/accounts?api-version=${SYNTEX_ACCOUNT_API_VERSION}`; @@ -486,10 +477,10 @@ export async function getSyntexAccounts( /** Delete a Microsoft.Syntex account by its ARM resource id (partial-account cleanup). */ export async function deleteSyntexAccount(resourceId: string): Promise { try { - await execFileAsync( + await runCommand( "az", ["rest", "--method", "delete", "--url", `${ARM_BASE}${resourceId}?api-version=${SYNTEX_ACCOUNT_API_VERSION}`], - { timeout: AZ_TIMEOUT_MS, shell: azNeedsShell() }, + { timeout: AZ_TIMEOUT_MS }, ); } catch (error) { const message = error instanceof Error ? error.message : String(error); diff --git a/src/bootstrap.test.ts b/src/bootstrap.test.ts index 793387c..4987d9c 100644 --- a/src/bootstrap.test.ts +++ b/src/bootstrap.test.ts @@ -3,29 +3,22 @@ /** * Unit tests for the Azure CLI bootstrap module. - * `node:child_process.execFile` is mocked so these run offline. + * The shared shell-free launcher (`./proc-exec.js` `runCommand`) is mocked so + * these run offline. */ import { describe, it, expect, vi, beforeEach } from "vitest"; -vi.mock("node:child_process", () => ({ execFile: vi.fn() })); +vi.mock("./proc-exec.js", () => ({ runCommand: vi.fn() })); -import { execFile } from "node:child_process"; +import { runCommand } from "./proc-exec.js"; import { assertAzCli, getSignedInIdentity, getBootstrapToken } from "./bootstrap.js"; -type ExecCb = (err: Error | null, stdout: string, stderr: string) => void; - function mockExec(result: { stdout?: string; error?: Error }): void { - vi.mocked(execFile).mockImplementation((( - _cmd: string, - _args: string[], - _opts: unknown, - cb: ExecCb, - ) => { - if (result.error) cb(result.error, "", ""); - else cb(null, result.stdout ?? "", ""); - return {} as never; - }) as never); + vi.mocked(runCommand).mockImplementation(async () => { + if (result.error) throw result.error; + return { stdout: result.stdout ?? "", stderr: "" }; + }); } beforeEach(() => { @@ -110,21 +103,19 @@ describe("getBootstrapToken", () => { }); }); -describe("cross-platform az invocation", () => { - // `az` is a native binary on macOS/Linux but a `.cmd` shim on Windows that - // must be resolved through a shell. bootstrap.ts sets `shell: true` only on - // win32; this asserts the invocation adapts to the current platform so the - // command works on both Windows and Linux. - it("passes shell:true on Windows and falsy elsewhere", async () => { +describe("shell-free az invocation", () => { + // `az` is a native binary on macOS/Linux but a `.cmd` shim on Windows. It is + // launched through the shared shell-free launcher (`./proc-exec.js`), which + // never routes arguments through a shell on any platform — so shell + // metacharacters in values are passed through literally, not interpreted. + it("invokes az through the launcher without any shell option", async () => { mockExec({ stdout: '{"azure-cli":"2.60.0"}' }); await assertAzCli(); - const opts = vi.mocked(execFile).mock.calls[0]?.[2] as { shell?: boolean }; - if (process.platform === "win32") { - expect(opts.shell).toBe(true); - } else { - expect(opts.shell).toBeFalsy(); - } + const call = vi.mocked(runCommand).mock.calls[0] as unknown as [string, string[], Record?]; + const opts = call[2] ?? {}; + // The launcher takes no `shell` option — args are never shell-interpreted. + expect(opts).not.toHaveProperty("shell"); }); // Regardless of platform, args are passed as an array (never a concatenated @@ -133,7 +124,7 @@ describe("cross-platform az invocation", () => { mockExec({ stdout: '{"azure-cli":"2.60.0"}' }); await assertAzCli(); - const [cmd, args] = vi.mocked(execFile).mock.calls[0] as unknown as [string, string[]]; + const [cmd, args] = vi.mocked(runCommand).mock.calls[0] as unknown as [string, string[]]; expect(cmd).toBe("az"); expect(Array.isArray(args)).toBe(true); expect(args).toContain("version"); diff --git a/src/bootstrap.ts b/src/bootstrap.ts index ac0f296..511109c 100644 --- a/src/bootstrap.ts +++ b/src/bootstrap.ts @@ -13,10 +13,10 @@ * The SECOND token (SPE-scoped, acquired via MSAL device-code AS the * newly-created owning app) lives in auth.ts and is wired in Phase 1. * - * Cross-platform: shells out to `az`, which is available on Windows/macOS/Linux. + * Cross-platform: invokes `az`, which is available on Windows/macOS/Linux. */ -import { execFile } from "node:child_process"; +import { runCommand } from "./proc-exec.js"; import { isConditionalAccessOrClaimsError, asConditionalAccessError, @@ -34,24 +34,6 @@ function log(message: string, data?: unknown): void { } } -function execFileAsync( - cmd: string, - args: string[], - opts: { timeout: number; shell?: boolean }, -): Promise<{ stdout: string; stderr: string }> { - return new Promise((resolve, reject) => { - execFile(cmd, args, opts, (err, stdout, stderr) => { - if (err) reject(err); - else resolve({ stdout, stderr }); - }); - }); -} - -/** True on Windows, where `az` is a `.cmd` shim that needs a shell to resolve. */ -function azNeedsShell(): boolean { - return process.platform === "win32"; -} - function isNotInstalledError(message: string): boolean { return ( message.includes("ENOENT") || @@ -93,9 +75,8 @@ export interface BootstrapToken { */ export async function assertAzCli(): Promise { try { - await execFileAsync("az", ["version", "--output", "json"], { + await runCommand("az", ["version", "--output", "json"], { timeout: AZ_TIMEOUT_MS, - shell: azNeedsShell(), }); } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -112,12 +93,8 @@ export async function assertAzCli(): Promise { */ export async function getSignedInIdentity(): Promise { try { - // NOTE: do NOT use `--query` here. On Windows `az` is a `.cmd` shim that - // requires shell:true, and a `--query` value containing spaces/braces gets - // word-split by the shell. Fetch the full JSON and parse it in JS instead. - const { stdout } = await execFileAsync("az", ["account", "show", "--output", "json"], { + const { stdout } = await runCommand("az", ["account", "show", "--output", "json"], { timeout: AZ_TIMEOUT_MS, - shell: azNeedsShell(), }); const parsed = JSON.parse(stdout) as { tenantId?: string; @@ -158,10 +135,10 @@ async function resolveTenantIdBestEffort(): Promise { export async function getBootstrapToken(resource: string = GRAPH_RESOURCE): Promise { log(`Acquiring bootstrap token for ${resource}`); try { - const { stdout } = await execFileAsync( + const { stdout } = await runCommand( "az", ["account", "get-access-token", "--resource", resource, "--output", "json"], - { timeout: AZ_TIMEOUT_MS, shell: azNeedsShell() }, + { timeout: AZ_TIMEOUT_MS }, ); const parsed = JSON.parse(stdout) as { accessToken?: string; diff --git a/src/proc-exec.test.ts b/src/proc-exec.test.ts new file mode 100644 index 0000000..1f3a126 --- /dev/null +++ b/src/proc-exec.test.ts @@ -0,0 +1,150 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { EventEmitter } from "node:events"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runCommand, spawnProcess } from "./proc-exec.js"; + +const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })); +vi.mock("cross-spawn", () => ({ default: spawnMock })); + +/** Minimal ChildProcess stand-in: EventEmitter with stdout/stderr streams + kill. */ +class FakeChild extends EventEmitter { + stdout = new EventEmitter(); + stderr = new EventEmitter(); + kill = vi.fn(); +} + +afterEach(() => { + spawnMock.mockReset(); + vi.useRealTimers(); +}); + +describe("runCommand", () => { + it("buffers stdout/stderr and resolves on a zero exit", async () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const promise = runCommand("az", ["version"]); + child.stdout.emit("data", Buffer.from("hel")); + child.stdout.emit("data", Buffer.from("lo")); + child.stderr.emit("data", Buffer.from("warn")); + child.emit("close", 0); + + await expect(promise).resolves.toEqual({ stdout: "hello", stderr: "warn" }); + }); + + it("never spawns with a shell", async () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const promise = runCommand("az", ["account", "show"], { cwd: "/tmp" }); + child.emit("close", 0); + await promise; + + expect(spawnMock).toHaveBeenCalledTimes(1); + const [command, args, opts] = spawnMock.mock.calls[0]; + expect(command).toBe("az"); + expect(args).toEqual(["account", "show"]); + expect(opts.shell).toBe(false); + expect(opts.windowsHide).toBe(true); + expect(opts.cwd).toBe("/tmp"); + }); + + it("passes a punctuation-heavy argument through as one discrete argv element (never a shell string)", async () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + // A single argument packed with shell-significant punctuation must reach the + // child verbatim as ONE argv element — never split, expanded, or interpreted. + const packed = "rg & | ; $() (sub)"; + const promise = runCommand("az", ["group", "show", "--name", packed]); + child.emit("close", 0); + await promise; + + const [command, args, opts] = spawnMock.mock.calls[0]; + expect(command).toBe("az"); + // Exactly one element per logical argument; the packed value is untouched. + expect(args).toEqual(["group", "show", "--name", packed]); + expect(args[3]).toBe(packed); + expect(opts.shell).toBe(false); + }); + + it("rejects on a non-zero exit with stdout/stderr/code and stderr in the message", async () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const promise = runCommand("az", ["group", "list"]); + child.stdout.emit("data", Buffer.from("partial")); + child.stderr.emit("data", Buffer.from("AADSTS50076 boom")); + child.emit("close", 2); + + await expect(promise).rejects.toMatchObject({ + stdout: "partial", + stderr: "AADSTS50076 boom", + code: 2, + }); + await promise.catch((e: Error) => { + expect(e.message).toContain("AADSTS50076 boom"); + }); + }); + + it("preserves ENOENT (missing command) as message text and code", async () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const promise = runCommand("az", ["version"]); + child.emit("error", Object.assign(new Error("spawn az ENOENT"), { code: "ENOENT" })); + + await expect(promise).rejects.toMatchObject({ code: "ENOENT" }); + await promise.catch((e: Error) => { + expect(e.message).toContain("ENOENT"); + }); + }); + + it("kills the child and rejects with ETIMEDOUT when the timeout elapses", async () => { + vi.useFakeTimers(); + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const promise = runCommand("az", ["version"], { timeout: 1000 }); + vi.advanceTimersByTime(1000); + + await expect(promise).rejects.toMatchObject({ code: "ETIMEDOUT" }); + expect(child.kill).toHaveBeenCalledTimes(1); + }); +}); + +describe("spawnProcess", () => { + it("returns the child and forces shell off even if a caller requests one", () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const result = spawnProcess("npm", ["run", "dev"], { + cwd: "/proj", + detached: true, + shell: true, + }); + + expect(result).toBe(child); + const [command, args, opts] = spawnMock.mock.calls[0]; + expect(command).toBe("npm"); + expect(args).toEqual(["run", "dev"]); + expect(opts.detached).toBe(true); + expect(opts.shell).toBe(false); + }); + + it("passes a punctuation-heavy argument through as one discrete argv element", () => { + const child = new FakeChild(); + spawnMock.mockReturnValue(child); + + const packed = "run & | ; $()"; + spawnProcess("npm", ["run", packed], { detached: true }); + + const [command, args, opts] = spawnMock.mock.calls[0]; + expect(command).toBe("npm"); + expect(args).toEqual(["run", packed]); + expect(args[1]).toBe(packed); + expect(opts.shell).toBe(false); + }); +}); diff --git a/src/proc-exec.ts b/src/proc-exec.ts new file mode 100644 index 0000000..c433f9e --- /dev/null +++ b/src/proc-exec.ts @@ -0,0 +1,152 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +/** + * Shared, shell-free child-process launcher. + * + * Every `az` / `azd` / dev-server invocation in this codebase goes through this + * module so process spawning is centralised on ONE hardened seam: + * + * - We NEVER pass `shell: true`. On Windows, `az`/`azd`/`npm`/`func` are `.cmd` + * batch shims; letting a shell resolve them routes arguments through + * `cmd.exe`, so any user-influenced argument becomes a metacharacter / + * command-injection vector. `cross-spawn` resolves `.cmd`/`.bat` shims via + * `PATHEXT` itself and passes arguments to the child process literally — no + * shell, no metacharacter interpretation. + * - Centralising here also gives tests a single module to mock + * (`vi.mock("../proc-exec.js", …)`) instead of stubbing `node:child_process`. + * + * The error shape mirrors Node's `execFile` rejection (`.stdout` / `.stderr` / + * `.code`, with stderr appended to `.message`) so existing error classifiers + * keep working unchanged. + */ + +import type { ChildProcess, SpawnOptions } from "node:child_process"; +import spawn from "cross-spawn"; + +export interface RunCommandOptions { + cwd?: string; + env?: NodeJS.ProcessEnv; + /** Kill the child and reject once this many milliseconds have elapsed. */ + timeout?: number; +} + +export interface RunCommandResult { + stdout: string; + stderr: string; +} + +/** + * Error thrown by {@link runCommand} when the child exits non-zero, cannot be + * spawned, or times out. Shaped like Node's `execFile` rejection so callers that + * read `.stdout` / `.stderr` / `.code` or match `.message` keep working. + */ +export interface RunCommandError extends Error { + stdout: string; + stderr: string; + code?: number | string; +} + +/** + * Run a command to completion and buffer its output. Never uses a shell; on + * Windows, `cross-spawn` resolves `.cmd`/`.bat` shims without `cmd.exe`, so + * arguments are passed to the child literally. + */ +export function runCommand( + command: string, + args: readonly string[] = [], + options: RunCommandOptions = {}, +): Promise { + return new Promise((resolvePromise, reject) => { + // shell:false is the entire point — cross-spawn resolves Windows .cmd shims + // itself, so we must never delegate to a shell. + const child = spawn(command, [...args], { + cwd: options.cwd, + env: options.env, + shell: false, + windowsHide: true, + stdio: ["ignore", "pipe", "pipe"], + }); + + let stdout = ""; + let stderr = ""; + let settled = false; + let timer: NodeJS.Timeout | undefined; + + const finish = (fn: () => void): void => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + fn(); + }; + + child.stdout?.on("data", (chunk: Buffer) => { + stdout += chunk.toString(); + }); + child.stderr?.on("data", (chunk: Buffer) => { + stderr += chunk.toString(); + }); + + child.on("error", (err: NodeJS.ErrnoException) => { + // Spawn failures land here — notably ENOENT for a missing command. + // Preserve the original message text (callers match "ENOENT" / "not + // recognized") and surface `.code` so `e.code === "ENOENT"` checks work. + finish(() => { + const e = err as RunCommandError; + e.stdout = stdout; + e.stderr = stderr; + if (err.code !== undefined) e.code = err.code; + reject(e); + }); + }); + + child.on("close", (exitCode) => { + finish(() => { + if (exitCode === 0) { + resolvePromise({ stdout, stderr }); + return; + } + // Append stderr to the message (as Node's execFile does) so classifiers + // that inspect `error.message` (AADSTS / "az login" / "not recognized") + // still see the underlying CLI output. + const suffix = stderr.trim().length > 0 ? `\n${stderr}` : ""; + const e = new Error( + `Command failed: ${command} (exit code ${exitCode ?? "unknown"})${suffix}`, + ) as RunCommandError; + e.stdout = stdout; + e.stderr = stderr; + if (exitCode !== null) e.code = exitCode; + reject(e); + }); + }); + + if (options.timeout && options.timeout > 0) { + timer = setTimeout(() => { + finish(() => { + child.kill(); + const e = new Error( + `Command timed out after ${options.timeout}ms: ${command}`, + ) as RunCommandError; + e.stdout = stdout; + e.stderr = stderr; + e.code = "ETIMEDOUT"; + reject(e); + }); + }, options.timeout); + if (typeof timer.unref === "function") timer.unref(); + } + }); +} + +/** + * Spawn a streaming child process without a shell. Thin `cross-spawn` + * passthrough for callers that need the live `ChildProcess` (detached + * dev-servers, custom stdio, event handling). `shell` is always forced off. + */ +export function spawnProcess( + command: string, + args: readonly string[] = [], + options: SpawnOptions = {}, +): ChildProcess { + return spawn(command, [...args], { ...options, shell: false }); +} diff --git a/src/tools/deploy-azure.test.ts b/src/tools/deploy-azure.test.ts index 9dd4d45..9c1ec94 100644 --- a/src/tools/deploy-azure.test.ts +++ b/src/tools/deploy-azure.test.ts @@ -12,23 +12,22 @@ * - extracts the live endpoint and reports the managed-identity infra, * - surfaces a friendly message when `azd` is not installed. * - * node:child_process, node:fs and state are mocked so nothing actually runs. + * node:fs, the shared shell-free launcher (`../proc-exec.js` `runCommand`) and + * state are mocked so nothing actually runs. */ import { describe, it, expect, vi, beforeEach } from "vitest"; let azureYamlExists = true; -type ExecCb = (err: (Error & { code?: string }) | null, stdout: string, stderr: string) => void; -let execImpl: (cmd: string, args: string[], opts: { env?: NodeJS.ProcessEnv }, cb: ExecCb) => void; +type RunResult = { stdout: string; stderr: string }; +let runImpl: (cmd: string, args: string[], opts: { env?: NodeJS.ProcessEnv }) => Promise; vi.mock("node:fs", () => ({ existsSync: vi.fn((p: string) => (String(p).endsWith("azure.yaml") ? azureYamlExists : true)), })); -vi.mock("node:child_process", () => ({ - execFile: vi.fn((cmd: string, args: string[], opts: { env?: NodeJS.ProcessEnv }, cb: ExecCb) => - execImpl(cmd, args, opts, cb), - ), +vi.mock("../proc-exec.js", () => ({ + runCommand: vi.fn((cmd: string, args: string[], opts: { env?: NodeJS.ProcessEnv }) => runImpl(cmd, args, opts)), })); const stateStore: Record = {}; @@ -44,16 +43,27 @@ vi.mock("../bootstrap.js", () => ({ bootstrapTokenProvider: vi.fn(async () => "boot-token"), })); -import { execFile } from "node:child_process"; +import { runCommand } from "../proc-exec.js"; import { deployAzureTool } from "../tools/deploy-azure.js"; +/** Build a rejected-launcher error carrying the same `.stdout`/`.stderr`/`.code` shape runCommand attaches. */ +function runError(message: string, extra: { stdout?: string; stderr?: string; code?: string } = {}): Error { + return Object.assign(new Error(message), { + stdout: extra.stdout ?? "", + stderr: extra.stderr ?? "", + code: extra.code, + }); +} + beforeEach(() => { vi.clearAllMocks(); azureYamlExists = true; for (const k of Object.keys(stateStore)) delete stateStore[k]; // Default: a successful azd up that prints a live endpoint. - execImpl = (_cmd, _args, _opts, cb) => - cb(null, "Deploying service web\n - Endpoint: https://demo.happyrock-1.eastus.azurecontainerapps.io/\n", ""); + runImpl = async () => ({ + stdout: "Deploying service web\n - Endpoint: https://demo.happyrock-1.eastus.azurecontainerapps.io/\n", + stderr: "", + }); // Default SPA patch: origin newly added. addSpaRedirectUrisMock.mockResolvedValue({ added: ["x"], redirectUris: ["x"] }); }); @@ -64,7 +74,7 @@ describe("project_deploy", () => { const r = await deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); expect(r.isError).toBe(true); expect(r.content[0].text).toContain("no `azure.yaml`"); - expect(execFile).not.toHaveBeenCalled(); + expect(runCommand).not.toHaveBeenCalled(); }); it("errors when no region is supplied (subscription-scoped template needs one)", async () => { @@ -72,7 +82,7 @@ describe("project_deploy", () => { const r = await deployAzureTool.handler({ projectDir: "/proj" }); expect(r.isError).toBe(true); expect(r.content[0].text).toContain("no Azure region"); - expect(execFile).not.toHaveBeenCalled(); + expect(runCommand).not.toHaveBeenCalled(); }); it("runs azd up --no-prompt with env wired from state and returns the endpoint", async () => { @@ -82,8 +92,8 @@ describe("project_deploy", () => { const r = await deployAzureTool.handler({ projectDir: "/proj", environmentName: "spe-demo", location: "eastus" }); expect(r.isError).toBeFalsy(); - expect(execFile).toHaveBeenCalledTimes(1); - const [cmd, args, opts] = vi.mocked(execFile).mock.calls[0] as unknown as [ + expect(runCommand).toHaveBeenCalledTimes(1); + const [cmd, args, opts] = vi.mocked(runCommand).mock.calls[0] as unknown as [ string, string[], { env?: NodeJS.ProcessEnv }, @@ -99,21 +109,45 @@ describe("project_deploy", () => { expect(r.content[0].text).toContain("subscription-scoped"); }); + it("passes a punctuation-heavy environment name to azd as one discrete argv element", async () => { + // An azd environment name is not the validated az subscription/RG boundary, + // so a punctuation-bearing value flows through to azd. It must still reach + // the process as a SINGLE argv element (defended by the shell-free launcher), + // never split into multiple arguments or interpreted by a shell. + const packed = "spe & | ; $()"; + + stateStore.azureSubscriptionId = "sub-123"; + stateStore.containerTypeId = "ct-456"; + + const r = await deployAzureTool.handler({ projectDir: "/proj", environmentName: packed, location: "eastus" }); + + expect(r.isError).toBeFalsy(); + expect(runCommand).toHaveBeenCalledTimes(1); + const [cmd, args, opts] = vi.mocked(runCommand).mock.calls[0] as unknown as [ + string, + string[], + { env?: NodeJS.ProcessEnv }, + ]; + expect(cmd).toBe("azd"); + expect(args).toEqual(["up", "--no-prompt", "--environment", packed]); + expect(args[3]).toBe(packed); // one discrete element, unmodified + expect(opts.env?.AZURE_ENV_NAME).toBe(packed); + expect(opts).not.toHaveProperty("shell"); + }); + it("retries the deploy alone when azd up loses the Resource Graph indexing race", async () => { vi.useFakeTimers(); - execImpl = (_cmd, args, _opts, cb) => { + runImpl = async (_cmd, args) => { if (args[0] === "up") { // Provisioned, but the publish step could not find the freshly-created // resource by its azd-service-name tag yet (ARG indexing lag). - cb( - new Error("exit status 1"), - "(done) Static Web App\nERROR: publishing service web: getting target resource: resource not found: unable to find a resource tagged with 'azd-service-name: web'", - "", - ); - } else { - // azd deploy retry succeeds once ARG has caught up. - cb(null, "web: Done\n- Endpoint: https://retried-app.7.azurestaticapps.net/\n", ""); + throw runError("exit status 1", { + stdout: + "(done) Static Web App\nERROR: publishing service web: getting target resource: resource not found: unable to find a resource tagged with 'azd-service-name: web'", + }); } + // azd deploy retry succeeds once ARG has caught up. + return { stdout: "web: Done\n- Endpoint: https://retried-app.7.azurestaticapps.net/\n", stderr: "" }; }; const pending = deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); @@ -123,29 +157,29 @@ describe("project_deploy", () => { expect(r.isError).toBeFalsy(); expect(r.content[0].text).toContain("https://retried-app.7.azurestaticapps.net/"); - const calls = vi.mocked(execFile).mock.calls as unknown as [string, string[]][]; + const calls = vi.mocked(runCommand).mock.calls as unknown as [string, string[]][]; expect(calls.some((c) => c[1][0] === "up")).toBe(true); expect(calls.some((c) => c[1][0] === "deploy")).toBe(true); }); it("does not retry (and surfaces the error) when azd up fails for a non-indexing reason", async () => { - execImpl = (_cmd, args, _opts, cb) => { + runImpl = async (_cmd, args) => { if (args[0] === "up") { - cb(new Error("exit status 1"), "ERROR: deployment failed: InvalidTemplate — bad bicep", ""); - } else { - cb(null, "should not be called", ""); + throw runError("exit status 1", { stdout: "ERROR: deployment failed: InvalidTemplate — bad bicep" }); } + return { stdout: "should not be called", stderr: "" }; }; const r = await deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); expect(r.isError).toBe(true); expect(r.content[0].text).toContain("InvalidTemplate"); - const calls = vi.mocked(execFile).mock.calls as unknown as [string, string[]][]; + const calls = vi.mocked(runCommand).mock.calls as unknown as [string, string[]][]; expect(calls.some((c) => c[1][0] === "deploy")).toBe(false); }); it("reports a friendly message when azd is not installed", async () => { - execImpl = (_cmd, _args, _opts, cb) => - cb(Object.assign(new Error("spawn azd ENOENT"), { code: "ENOENT" }), "", ""); + runImpl = async () => { + throw runError("spawn azd ENOENT", { code: "ENOENT" }); + }; const r = await deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); expect(r.isError).toBe(true); expect(r.content[0].text).toContain("Azure Developer CLI (`azd`) is not installed"); @@ -154,8 +188,7 @@ describe("project_deploy", () => { it("auto-registers the deployed origin as a SPA redirect URI on the owning app", async () => { stateStore.appObjectId = "obj-owning"; stateStore.appId = "app-owning"; - execImpl = (_cmd, _args, _opts, cb) => - cb(null, "web\n - Endpoint: https://my-spa-123.7.azurestaticapps.net/\n", ""); + runImpl = async () => ({ stdout: "web\n - Endpoint: https://my-spa-123.7.azurestaticapps.net/\n", stderr: "" }); const r = await deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); @@ -172,8 +205,7 @@ describe("project_deploy", () => { it("emits manual SPA-redirect guidance when no owning app is recorded in state", async () => { // No appObjectId in state → cannot auto-patch. - execImpl = (_cmd, _args, _opts, cb) => - cb(null, "web\n - Endpoint: https://no-owner.7.azurestaticapps.net/\n", ""); + runImpl = async () => ({ stdout: "web\n - Endpoint: https://no-owner.7.azurestaticapps.net/\n", stderr: "" }); const r = await deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); @@ -186,8 +218,7 @@ describe("project_deploy", () => { stateStore.appObjectId = "obj-owning"; stateStore.appId = "app-owning"; addSpaRedirectUrisMock.mockResolvedValue(undefined); // best-effort failure - execImpl = (_cmd, _args, _opts, cb) => - cb(null, "web\n - Endpoint: https://patch-fail.7.azurestaticapps.net/\n", ""); + runImpl = async () => ({ stdout: "web\n - Endpoint: https://patch-fail.7.azurestaticapps.net/\n", stderr: "" }); const r = await deployAzureTool.handler({ projectDir: "/proj", location: "eastus" }); diff --git a/src/tools/deploy-azure.ts b/src/tools/deploy-azure.ts index 33940c0..616a92e 100644 --- a/src/tools/deploy-azure.ts +++ b/src/tools/deploy-azure.ts @@ -15,12 +15,12 @@ * the SPE container type id) from the recorded provisioning state. */ -import { execFile } from "node:child_process"; import { existsSync } from "node:fs"; import { join, resolve } from "node:path"; import { bootstrapTokenProvider } from "../bootstrap.js"; import { addSpaRedirectUris } from "../graph-client.js"; import { readState } from "../state.js"; +import { runCommand } from "../proc-exec.js"; import type { McpTool } from "../types.js"; interface DeployArgs { @@ -48,19 +48,6 @@ function sleep(ms: number): Promise { return new Promise((resolve_) => setTimeout(resolve_, ms)); } -function execFileAsync( - cmd: string, - args: string[], - opts: { timeout: number; cwd: string; shell?: boolean; env?: NodeJS.ProcessEnv }, -): Promise<{ stdout: string; stderr: string }> { - return new Promise((resolve_, reject) => { - execFile(cmd, args, opts, (err, stdout, stderr) => { - if (err) reject(Object.assign(err, { stdout, stderr })); - else resolve_({ stdout, stderr }); - }); - }); -} - /** Extract the deployed app endpoint azd prints. */ function extractEndpoint(output: string): string | null { // Prefer azd's explicit "Endpoint:" line — that is the deployed app URL. @@ -139,18 +126,18 @@ async function deployWithArgLagRetry( dir: string, env: NodeJS.ProcessEnv, ): Promise<{ stdout: string; stderr: string }> { - const opts = { timeout: AZD_TIMEOUT_MS, cwd: dir, shell: process.platform === "win32", env }; + const opts = { timeout: AZD_TIMEOUT_MS, cwd: dir, env }; const argLag = (err: { stdout?: string; stderr?: string }) => ARG_LAG_PATTERN.test([err.stdout, err.stderr].filter(Boolean).join("\n")); try { - return await execFileAsync("azd", ["up", "--no-prompt", "--environment", environmentName], opts); + return await runCommand("azd", ["up", "--no-prompt", "--environment", environmentName], opts); } catch (error) { if (!argLag(error as { stdout?: string; stderr?: string })) throw error; let lastError: unknown = error; for (let attempt = 1; attempt <= DEPLOY_RETRY_ATTEMPTS; attempt++) { await sleep(DEPLOY_RETRY_DELAY_MS); try { - return await execFileAsync("azd", ["deploy", "--no-prompt", "--environment", environmentName], opts); + return await runCommand("azd", ["deploy", "--no-prompt", "--environment", environmentName], opts); } catch (retryError) { lastError = retryError; // A different failure means retrying will not help — surface it now. @@ -221,10 +208,9 @@ export const deployAzureTool: McpTool = { let subscriptionId = state.azureSubscriptionId; if (!subscriptionId) { try { - const { stdout } = await execFileAsync("az", ["account", "show", "--query", "id", "--output", "tsv"], { + const { stdout } = await runCommand("az", ["account", "show", "--query", "id", "--output", "tsv"], { timeout: 30_000, cwd: dir, - shell: process.platform === "win32", env: childEnv, }); subscriptionId = stdout.trim() || undefined; diff --git a/src/tools/list-azure.test.ts b/src/tools/list-azure.test.ts index e846eae..655f3d1 100644 --- a/src/tools/list-azure.test.ts +++ b/src/tools/list-azure.test.ts @@ -20,7 +20,7 @@ vi.mock("../azure-cli.js", () => ({ })); import * as azureCli from "../azure-cli.js"; -import { listSubscriptionsTool } from "../tools/list-azure.js"; +import { listSubscriptionsTool, listResourceGroupsTool } from "../tools/list-azure.js"; beforeEach(() => { vi.clearAllMocks(); @@ -74,3 +74,41 @@ describe("azure_subscriptions_list", () => { expect(result.content[0].text).toContain("az not installed"); }); }); + +describe("azure_resource_groups_list — subscriptionId validation", () => { + it("rejects a missing subscriptionId before invoking the CLI", async () => { + const result = await listResourceGroupsTool.handler({}); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("subscriptionId is required"); + expect(azureCli.listResourceGroups).not.toHaveBeenCalled(); + }); + + it.each([ + "3fa85f64-5717-4562-b3fc-2c963f66afa6 &", + "sub-1 |", + "$()", + "``", + "--query", + "not-a-guid", + ])("rejects an invalid / injection subscriptionId (%s) before invoking the CLI", async (value) => { + const result = await listResourceGroupsTool.handler({ subscriptionId: value }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("valid Azure subscription ID"); + expect(azureCli.listResourceGroups).not.toHaveBeenCalled(); + }); + + it("accepts a valid subscription GUID and forwards it to the CLI", async () => { + const validId = "3fa85f64-5717-4562-b3fc-2c963f66afa6"; + vi.mocked(azureCli.listResourceGroups).mockResolvedValue([ + { name: "rg-spe-demo", location: "eastus", id: "/subscriptions/x/resourceGroups/rg-spe-demo" }, + ]); + + const result = await listResourceGroupsTool.handler({ subscriptionId: validId }); + + expect(result.isError).toBeFalsy(); + expect(azureCli.listResourceGroups).toHaveBeenCalledWith(validId); + expect(result.content[0].text).toContain("rg-spe-demo"); + }); +}); diff --git a/src/tools/list-azure.ts b/src/tools/list-azure.ts index e94761e..abbbe65 100644 --- a/src/tools/list-azure.ts +++ b/src/tools/list-azure.ts @@ -10,6 +10,7 @@ */ import { isSignedIn, listResourceGroups, listSubscriptions } from "../azure-cli.js"; +import { requireAzureSubscriptionId } from "../validation.js"; import type { McpTool } from "../types.js"; export const listSubscriptionsTool: McpTool = { @@ -73,10 +74,9 @@ export const listResourceGroupsTool: McpTool = { required: ["subscriptionId"], }, handler: async (args) => { - const subscriptionId = (args.subscriptionId as string | undefined)?.trim(); - if (!subscriptionId) { - return { content: [{ type: "text" as const, text: "Error: subscriptionId is required" }], isError: true }; - } + const parsed = requireAzureSubscriptionId(args.subscriptionId); + if (!parsed.ok) return parsed.error; + const subscriptionId = parsed.value; try { const groups = await listResourceGroups(subscriptionId); if (groups.length === 0) { diff --git a/src/tools/run-local.test.ts b/src/tools/run-local.test.ts index 65b79ad..944a8b6 100644 --- a/src/tools/run-local.test.ts +++ b/src/tools/run-local.test.ts @@ -10,7 +10,7 @@ * - A failed process launch (spawn error / ENOENT) is reflected as isError * instead of a false "running" success. * - * node:child_process and node:fs are mocked so nothing actually spawns. + * ../proc-exec.js and node:fs are mocked so nothing actually spawns. */ import { describe, it, expect, vi, beforeEach } from "vitest"; @@ -29,16 +29,16 @@ vi.mock("node:fs", () => ({ }), })); -vi.mock("node:child_process", () => ({ - spawn: vi.fn(() => { +vi.mock("../proc-exec.js", () => ({ + spawnProcess: vi.fn(() => { const child = new EventEmitter() as EventEmitter & { unref: () => void }; child.unref = () => {}; queueMicrotask(() => { if (spawnBehavior === "error") { child.emit("error", new Error(spawnError)); } else if (spawnBehavior === "exit-nonzero") { - // win32 shell:true false-success path: the OS spawns cmd.exe ('spawn' - // fires), then the shell exits non-zero because the toolchain is missing. + // Early non-zero exit path: 'spawn' fires, then the child exits + // non-zero because the toolchain is missing. child.emit("spawn"); child.emit("exit", 1, null); } else { @@ -60,7 +60,7 @@ vi.mock("../server-readiness.js", () => ({ waitForServerReady: vi.fn(async () => serverReady), })); -import { spawn } from "node:child_process"; +import { spawnProcess } from "../proc-exec.js"; import { waitForServerReady } from "../server-readiness.js"; import { runLocalTool } from "../tools/run-local.js"; @@ -110,14 +110,14 @@ describe("project_run_local — URL/port detection", () => { expect(result.isError).toBeFalsy(); expect(result.content[0].text).toContain("http://localhost:5000"); - expect(spawn).toHaveBeenCalledWith("dotnet", ["run"], expect.objectContaining({ detached: true })); + expect(spawnProcess).toHaveBeenCalledWith("dotnet", ["run"], expect.objectContaining({ detached: true })); }); it("errors when no runnable project is present", async () => { files = {}; const result = await runLocalTool.handler({ projectDir: "/proj" }); expect(result.isError).toBe(true); - expect(spawn).not.toHaveBeenCalled(); + expect(spawnProcess).not.toHaveBeenCalled(); }); }); @@ -143,7 +143,7 @@ describe("project_run_local — start-outcome reflection", () => { expect(result.content[0].text).toContain("failed to start"); }); - it("surfaces a non-zero early EXIT as isError (win32 shell:true false-success path, Node)", async () => { + it("surfaces a non-zero early EXIT as isError (early-exit false-success path, Node)", async () => { files = { "package.json": pkg({ dev: "vite" }) }; spawnBehavior = "exit-nonzero"; diff --git a/src/tools/run-local.ts b/src/tools/run-local.ts index fdd9bf7..c8a75eb 100644 --- a/src/tools/run-local.ts +++ b/src/tools/run-local.ts @@ -17,11 +17,11 @@ * accepting connections — never a URL that refuses. */ -import { spawn } from "node:child_process"; import { existsSync, readFileSync } from "node:fs"; import { join, resolve } from "node:path"; import { LOCAL_DEV_PORT } from "../constants.js"; import { waitForServerReady } from "../server-readiness.js"; +import { spawnProcess } from "../proc-exec.js"; import type { McpTool } from "../types.js"; interface RunLocalArgs { @@ -84,16 +84,16 @@ interface SpawnOutcome { /** * Spawn a detached process and resolve with its immediate launch outcome: * `ok:false` if the OS could not start it (e.g. ENOENT for a missing command) - * OR if it exits with a non-zero code within the grace window (the win32 - * `shell:true` case, where a missing toolchain spawns cmd.exe and only the shell - * exit code reveals the failure); `ok:true` once it has spawned and survived the - * grace window without an early non-zero exit. Resolves optimistically after the - * short grace period so we never block the MCP server. + * OR if it exits with a non-zero code within the grace window (a missing + * toolchain can surface only via an early non-zero exit code); `ok:true` once it + * has spawned and survived the grace window without an early non-zero exit. + * Resolves optimistically after the short grace period so we never block the MCP + * server. * * Note: we deliberately do NOT resolve success on `'spawn'` alone — `'spawn'` - * only means the OS created the process (or the shell), not that the underlying - * command exists. We wait out the grace window so an early non-zero exit can - * still flip the outcome to failure. + * only means the OS created the process, not that the underlying command + * exists. We wait out the grace window so an early non-zero exit can still flip + * the outcome to failure. */ function startDetached(command: string, args: string[], cwd: string): Promise { return new Promise((resolveOutcome) => { @@ -107,11 +107,10 @@ function startDetached(command: string, args: string[], cwd: string): Promise { @@ -182,10 +181,9 @@ function runToCompletion(command: string, args: string[], cwd: string): Promise< let child; try { - child = spawn(command, args, { + child = spawnProcess(command, args, { cwd, stdio: "ignore", - shell: process.platform === "win32", }); } catch (error) { finish({ ok: false, error: error instanceof Error ? error.message : String(error) }); diff --git a/src/validation.test.ts b/src/validation.test.ts index 9cbe72a..0ba9616 100644 --- a/src/validation.test.ts +++ b/src/validation.test.ts @@ -6,7 +6,16 @@ */ import { describe, it, expect } from "vitest"; -import { requireString, validationError } from "./validation.js"; +import { + requireString, + validationError, + isAzureSubscriptionId, + isAzureResourceGroupName, + requireAzureSubscriptionId, + requireAzureResourceGroupName, + assertAzureSubscriptionId, + assertAzureResourceGroupName, +} from "./validation.js"; import type { McpToolResult } from "./types.js"; describe("requireString", () => { @@ -69,3 +78,120 @@ describe("documented usage example (module JSDoc)", () => { } }); }); + +const VALID_SUBSCRIPTION_ID = "3fa85f64-5717-4562-b3fc-2c963f66afa6"; +const VALID_RESOURCE_GROUP = "rg-spe-demo_01.(prod)"; + +// Shell metacharacters / argument-injection payloads that must never be +// accepted as an Azure identifier, even though process spawning is shell-free. +const INJECTION_PAYLOADS = [ + "sub &", + "sub |", + "sub ;", + "sub``", + "sub$()", + "sub >", + "sub %%", + "sub !!", + "s\nub", + "--query", + "-g", +]; + +describe("isAzureSubscriptionId", () => { + it("accepts a canonical GUID", () => { + expect(isAzureSubscriptionId(VALID_SUBSCRIPTION_ID)).toBe(true); + }); + + it.each([...INJECTION_PAYLOADS, "not-a-guid", "3fa85f64", "", 123, null, undefined])( + "rejects invalid / injection input (%p)", + (value) => { + expect(isAzureSubscriptionId(value)).toBe(false); + }, + ); +}); + +describe("isAzureResourceGroupName", () => { + it.each(["rg1", "my_group", "a", VALID_RESOURCE_GROUP, "group-1"])( + "accepts a valid resource-group name (%p)", + (value) => { + expect(isAzureResourceGroupName(value)).toBe(true); + }, + ); + + it.each([ + ...INJECTION_PAYLOADS, + "-startsWithHyphen", + "endsWithDot.", + "has space", + "", + `${"a".repeat(91)}`, + ])("rejects invalid / injection input (%p)", (value) => { + expect(isAzureResourceGroupName(value)).toBe(false); + }); +}); + +describe("requireAzureSubscriptionId", () => { + it("accepts and returns a trimmed GUID", () => { + const r = requireAzureSubscriptionId(` ${VALID_SUBSCRIPTION_ID} `); + expect(r.ok).toBe(true); + if (r.ok) expect(r.value).toBe(VALID_SUBSCRIPTION_ID); + }); + + it.each(INJECTION_PAYLOADS)("rejects injection payload (%p) with a GUID message", (value) => { + const r = requireAzureSubscriptionId(value); + expect(r.ok).toBe(false); + if (!r.ok) { + expect(r.error.isError).toBe(true); + expect(r.error.content[0].text).toBe( + "Error: subscriptionId must be a valid Azure subscription ID (GUID)", + ); + } + }); + + it("uses the standard required message when the value is missing", () => { + const r = requireAzureSubscriptionId(undefined); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error.content[0].text).toBe("Error: subscriptionId is required"); + }); +}); + +describe("requireAzureResourceGroupName", () => { + it("accepts and returns a trimmed name", () => { + const r = requireAzureResourceGroupName(" rg-1 "); + expect(r.ok).toBe(true); + if (r.ok) expect(r.value).toBe("rg-1"); + }); + + it.each(INJECTION_PAYLOADS)("rejects injection payload (%p) with a name message", (value) => { + const r = requireAzureResourceGroupName(value); + expect(r.ok).toBe(false); + if (!r.ok) { + expect(r.error.content[0].text).toBe( + "Error: resourceGroup must be a valid Azure resource group name", + ); + } + }); +}); + +describe("assertAzureSubscriptionId", () => { + it("does not throw for a valid GUID", () => { + expect(() => assertAzureSubscriptionId(VALID_SUBSCRIPTION_ID)).not.toThrow(); + }); + + it.each(INJECTION_PAYLOADS)("throws a generic error for injection payload (%p)", (value) => { + expect(() => assertAzureSubscriptionId(value)).toThrow("Invalid Azure subscription ID"); + }); +}); + +describe("assertAzureResourceGroupName", () => { + it("does not throw for a valid name", () => { + expect(() => assertAzureResourceGroupName("rg-1")).not.toThrow(); + }); + + it.each(INJECTION_PAYLOADS)("throws a generic error for injection payload (%p)", (value) => { + expect(() => assertAzureResourceGroupName(value)).toThrow( + "Invalid Azure resource group name", + ); + }); +}); diff --git a/src/validation.ts b/src/validation.ts index cdaaddf..0554108 100644 --- a/src/validation.ts +++ b/src/validation.ts @@ -52,3 +52,96 @@ export function requireString( } return { ok: true, value: value.trim() }; } + +/** + * Strict allowlist patterns for identifiers that are passed as arguments to the + * Azure CLI. Even though process spawning is shell-free (see `./proc-exec.ts`), + * validating these values before they become CLI arguments is defence in depth: + * it blocks argument-injection (a leading `-`/`--` being read as a CLI flag) and + * keeps obviously malformed input from reaching Azure. + */ + +/** Canonical Azure subscription ID form: a lowercase/uppercase GUID. */ +const AZURE_SUBSCRIPTION_ID_RE = + /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/; + +/** + * Azure resource-group naming rules: 1–90 chars of letters, digits, `_`, `.`, + * `(`, `)`, `-`; may not start with `-` (which would be read as a CLI flag) and + * may not end with `.`. The first-char class deliberately omits `-`. + */ +const AZURE_RESOURCE_GROUP_RE = /^[A-Za-z0-9_.()][A-Za-z0-9_.()-]{0,89}$/; + +/** True when `value` is a syntactically valid Azure subscription ID (GUID). */ +export function isAzureSubscriptionId(value: unknown): value is string { + return typeof value === "string" && AZURE_SUBSCRIPTION_ID_RE.test(value); +} + +/** True when `value` is a syntactically valid Azure resource-group name. */ +export function isAzureResourceGroupName(value: unknown): value is string { + return ( + typeof value === "string" && + AZURE_RESOURCE_GROUP_RE.test(value) && + !value.endsWith(".") + ); +} + +/** + * Require that `value` is a valid Azure subscription ID (GUID). Returns the + * trimmed value on success, or the standard MCP error envelope on failure. + */ +export function requireAzureSubscriptionId( + value: unknown, + name = "subscriptionId", +): { ok: true; value: string } | { ok: false; error: McpToolResult } { + const parsed = requireString(value, name); + if (!parsed.ok) return parsed; + if (!isAzureSubscriptionId(parsed.value)) { + return { + ok: false, + error: validationError(`${name} must be a valid Azure subscription ID (GUID)`), + }; + } + return { ok: true, value: parsed.value }; +} + +/** + * Require that `value` is a valid Azure resource-group name. Returns the trimmed + * value on success, or the standard MCP error envelope on failure. + */ +export function requireAzureResourceGroupName( + value: unknown, + name = "resourceGroup", +): { ok: true; value: string } | { ok: false; error: McpToolResult } { + const parsed = requireString(value, name); + if (!parsed.ok) return parsed; + if (!isAzureResourceGroupName(parsed.value)) { + return { + ok: false, + error: validationError(`${name} must be a valid Azure resource group name`), + }; + } + return { ok: true, value: parsed.value }; +} + +/** + * Defence-in-depth assertion: throw if `value` is not a valid Azure subscription + * ID. Used inside the concrete `az`-invoking helpers so a malformed value can + * never reach the CLI even if a future caller bypasses the tool boundary. The + * message is intentionally generic (no echoed input). + */ +export function assertAzureSubscriptionId(value: unknown): asserts value is string { + if (!isAzureSubscriptionId(value)) { + throw new Error("Invalid Azure subscription ID"); + } +} + +/** + * Defence-in-depth assertion: throw if `value` is not a valid Azure + * resource-group name. Message is intentionally generic (no echoed input). + */ +export function assertAzureResourceGroupName(value: unknown): asserts value is string { + if (!isAzureResourceGroupName(value)) { + throw new Error("Invalid Azure resource group name"); + } +} diff --git a/src/version.test.ts b/src/version.test.ts index 706e3fa..4287152 100644 --- a/src/version.test.ts +++ b/src/version.test.ts @@ -23,6 +23,10 @@ const pkgRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); const pkgVersion = ( JSON.parse(readFileSync(join(pkgRoot, "package.json"), "utf8")) as { version: string } ).version; +const serverManifest = JSON.parse(readFileSync(join(pkgRoot, "server.json"), "utf8")) as { + version: string; + packages: Array<{ version: string }>; +}; describe("version: single source of truth", () => { it("sources PACKAGE_VERSION from package.json", () => { @@ -32,4 +36,10 @@ describe("version: single source of truth", () => { it("derives USER_AGENT from package.json in the spe-mcp-server/ format", () => { expect(USER_AGENT).toBe(`spe-mcp-server/${pkgVersion}`); }); + + it("keeps the MCP registry manifest aligned with package.json", () => { + expect(serverManifest.version).toBe(pkgVersion); + expect(serverManifest.packages).toHaveLength(1); + expect(serverManifest.packages[0]?.version).toBe(pkgVersion); + }); });