runtime: harden external process invocation - #94
Open
Gregory Joseph (gnjoseph) wants to merge 3 commits into
Open
runtime: harden external process invocation#94Gregory Joseph (gnjoseph) wants to merge 3 commits into
Gregory Joseph (gnjoseph) wants to merge 3 commits into
Conversation
Centralize external CLI launches behind a shell-free process abstraction, validate Azure identifiers at the tool and helper boundaries, and add focused regression coverage. AB#3116729 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gregory Joseph (gnjoseph)
requested review from
Diego Luces (dluces),
Marc Windle (marcwindle) and
pemtaira-msft
and
a lite review from Copilot
August 20, 2026 22:00
There was a problem hiding this comment.
Pull request overview
This PR promotes the external process-invocation hardening work into main by centralizing az/azd/dev-server launches behind a shell-free process abstraction and tightening validation for Azure identifiers before they become CLI arguments.
Changes:
- Introduces
src/proc-exec.ts(runCommand/spawnProcess) to ensure shell-free child process execution (including Windows.cmd/.batshim resolution viacross-spawn). - Adds strict Azure subscription ID + resource-group validation helpers (predicate/require/assert) and applies subscription validation in
azure_resource_groups_list. - Updates multiple tools/helpers and tests to use the new process seam; bumps package version to
0.2.0-alpha.2and records the change in the changelog.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/validation.ts | Adds allowlist-based Azure subscription/resource-group validation helpers (predicate/require/assert). |
| src/validation.test.ts | Adds regression coverage for the new Azure identifier validation helpers. |
| src/tools/run-local.ts | Switches dev-server spawning to the shared shell-free launcher. |
| src/tools/run-local.test.ts | Updates mocks/assertions to target proc-exec rather than node:child_process. |
| src/tools/list-azure.ts | Centralizes subscriptionId validation via requireAzureSubscriptionId before calling CLI helpers. |
| src/tools/list-azure.test.ts | Adds tests proving invalid/missing subscription IDs are rejected before invoking the CLI helper. |
| src/tools/deploy-azure.ts | Replaces direct execFile usage with runCommand for azd/az invocations. |
| src/tools/deploy-azure.test.ts | Updates mocks to the new launcher seam and adds argv-discreteness regression coverage. |
| src/proc-exec.ts | New shell-free process abstraction built on cross-spawn. |
| src/proc-exec.test.ts | Adds contract tests for shell-free behavior, argv discreteness, error shaping, and timeouts. |
| src/bootstrap.ts | Routes Azure CLI bootstrap calls through runCommand (shell-free). |
| src/bootstrap.test.ts | Updates mocking strategy to proc-exec and asserts shell-free invocation contract. |
| src/azure-cli.ts | Routes Azure CLI helper calls through runCommand and asserts identifier validity at the process seam. |
| src/azure-cli.proc.test.ts | Adds process-boundary contract tests (reject-before-spawn + discrete argv). |
| package.json | Bumps version to 0.2.0-alpha.2 and adds cross-spawn + typings. |
| package-lock.json | Updates lockfile for the version bump and new direct dependency. |
| CHANGELOG.md | Documents the process hardening and validation changes for 0.2.0-alpha.2. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+61
to
+75
| // 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; | ||
|
|
Reserve a new npm version for the main-based ESRP release because 0.2.0-alpha.2 has already been submitted. AB#3116729 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep server.json synchronized with package.json and enforce the invariant in CI. AB#3116729 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes the process-invocation hardening validated in #93 to
main:@microsoft/spe-mcp@0.2.0-alpha.3Validation
npm run cipassed with 893 tests and 7 skipsReviewer notes
The main-branch dependency versions were preserved while promoting
cross-spawnfrom transitive to direct. Public fixtures use inert punctuation-only inputs.