[William (william051200)]

🤖 PR Validation — ️✔️ All clear

Breaking Changes
️✔️ None

This PR hardens nested Azure CLI execution in  vm-repair  without rejecting valid percent-encoded URIs.

On Windows, _call_az_command() now invokes the current Python interpreter with -m azure.cli and shell=False, instead of launching through cmd.exe and az.cmd. This prevents shell command execution and environment-variable expansion while preserving literal % and ! characters.

Existing command builders remain in place. Double-quote/control-character restrictions, secret masking, error reporting, and asynchronous behavior are preserved. Linux/macOS retain their existing launch path.

---

This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

• Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
• Have you run python scripts/ci/test_index.py -q locally? (pip install azdev required)
• My extension version conforms to the Extension version schema

For new extensions:

• My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

[Copilot]

Copilot review overview

🟡 Changes recommended

Command expansion remains unguarded, and validation errors can disclose sensitive parameters.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Cmd expansion bypasses validation and enables command injection · New
• Rejected token is disclosed in errors, logs, and telemetry · New
• Windows restore command lacks portable validation coverage · New

What changed in this PR

This PR hardens Windows nested Azure CLI commands in vm-repair by adding shared token validation.

Changes:

• Rejects quotes and control characters before Windows command execution.
• Reuses validation for tags and adds injection regression tests.
• Updates the version and release notes to 2.4.6.

File	Description
src/​vm-repair/​setup.py	Bumps version to 2.4.6.
src/​vm-repair/​HISTORY.rst	Documents Windows command hardening.
src/​vm-repair/​azext_vm_repair/​tests/​latest/​test_osdisk_uri_injection.py	Adds unmanaged-disk injection regression tests.
src/​vm-repair/​azext_vm_repair/​repair_utils.py	Adds shared validation before Windows command execution.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Validation can reject a repaired disk URI after detachment, leaving restore unable to retry normally.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

• Validate attach command before detaching repaired disks · New
• Validate tokenized --set arguments instead of raw command text · New
• Document percent and exclamation restrictions and compatibility impact · New

Resolved since last review (3)

• Rejected token is disclosed in errors, logs, and telemetry
• Cmd expansion bypasses validation and enables command injection
• Windows restore command lacks portable validation coverage

[William (william051200)]

• Unmanaged restore validates the attach command at custom.py:686, before detach at line 690.
• Preflight and the execution sink both use _validate_command_token(). The sink guard remains intact.
• % is now allowed and passed literally through shell-free execution, so percent-encoded URIs no longer trigger rejection.

[Copilot]

Copilot review overview

🟡 Changes recommended

Secret redaction lets unsafe execution arguments bypass the Windows validation guard.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Validate original execution tokens before running cmd.exe · New
• Validate attach command before detaching repaired disks
• Add restore tests for unsafe attach values and cleanup prevention · New

Resolved since last review (2)

• Validate tokenized --set arguments instead of raw command text
• Document percent and exclamation restrictions and compatibility impact

[Copilot]

Copilot review overview

🟡 Changes recommended

The non-leaking regression assertion must detect disclosure of the actual rejected token.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Validate attach command before detaching repaired disks
• Assertion fails to detect rejected token disclosure · New
• Add restore tests for unsafe attach values and cleanup prevention

Resolved since last review (1)

• Validate original execution tokens before running cmd.exe

[Copilot]

🟡 Changes recommended

Unescaped URI quotes still bypass validation and inject additional Azure CLI arguments.

3 open findings

• Unescaped quotes in disk URIs enable Azure CLI argument injection · New
• Validate attach command before detaching repaired disks
• Add restore tests for unsafe attach values and cleanup prevention

1 resolved since last review

• Assertion fails to detect rejected token disclosure

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Copilot]

🔵 Needs a closer look

Credential rejection can occur after disk copying, leaving a billable disk outside the cleanup scope.

2 open findings

• Validate attach command before detaching repaired disks
• Add restore tests for unsafe attach values and cleanup prevention

1 resolved since last review

• Unescaped quotes in disk URIs enable Azure CLI argument injection

Previously missed (1)

In code that hasn't changed since last review

Validate VM credentials before creating disks to prevent orphaned resources

src/​vm-repair/​azext_vm_repair/​repair_utils.py:196

On a Windows host, a password such as CorrectHorse7! passes validate_vm_password, but this new guard rejects it when _create_repair_vm() validates the VM template. By then, managed-disk create() has already copied the OS disk into the source resource group (custom.py:402–428). Failure cleanup deletes only the repair resource group (custom.py:591–594), leaving the copied disk behind and accruing charges. Keep this sink-level guard, but validate the credentials or preflight the VM-create command before creating resources. Add a regression test asserting that rejected credentials cause no resource mutations.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Yong Zhang (yonzhan)]

vm-repair

[x-engineering-agent]

X Engineering Agent Report

Review — Comments

William (@william051200)

The current head addresses the earlier Copilot reviewer findings: Windows validation rejects quotes, expansion syntax and control characters in the original execution tokens; errors omit rejected values; unmanaged restore preserves the complete URI assignment as one argument; and restore/NIC-revert validation precedes detach/IP swap.

Upstream CI has completed with no failed checks. The author-added src/vm-repair/azext_vm_repair/tests/latest/test_command_injection.py exercises the Windows sink with mocked process creation, masked-credential rejection, both restore branches, embedded-quote argument injection, and NIC rejection/happy paths. HISTORY.rst documents the Windows %/! compatibility restrictions and matches version 2.4.6. No unresolved changed-line finding was confirmed. Real Windows launcher behavior and live Azure disk/NIC transitions were not independently verified by the Agent.

Test validation

• Agent live test: Not run; this is a review-only PR. Check author-provided test evidence and upstream CI separately.
• Regression coverage: Not applicable to the changed files.

Risk assessment — 77/100 · Critical · High confidence

The Critical rating is driven by security-sensitive behavior, public CLI behavior, dependency or supply chain and 1 additional signal(s).

• Change scope: 5 changed files, 551 changed lines (+490 / -61), including 3 production files.
• Affected components: vm-repair
• Risk drivers: security-sensitive behavior (+28); public CLI behavior (+18); dependency or supply chain (+18); failure-handling behavior (+12)
• Regression evidence: Changed regression tests are included, reducing risk.
• Confidence: High because changed-line patches were available for every production file.
• Required review: Owning-squad review is required for vm-repair before merge.

Posted by x-engineering-agent (Reviewer)

[x-engineering-agent]

William (@william051200) Ready for human review. See the X Engineering Agent Report.

[Copilot]

🟡 Changes recommended

Non-isolated interpreter startup allows an untrusted local Python package to replace Azure CLI.

3 open issues

• Validate attach command before detaching repaired disks
• Use isolated Python mode to prevent import hijacking · New
• Add restore tests for unsafe attach values and cleanup prevention

_{Review effort: Balanced}

[Edwin Bernal Microsoft (EdwinBernal1)]

[P1] Preserve interpreter isolation when bypassing the Windows launcher

Please add -I before -m azure.cli. On a standard Python installation, this child inherits import lookup from the working directory and PYTHONPATH, so a local azure/cli package can run instead of the installed CLI with the operator's permissions. I reproduced both routes through the production helper using a harmless print-only package; the same interpreter with -I loaded the genuine CLI successfully. The standard Windows launcher already uses -IBm.

Update the argv expectations and replace the PYTHONPATH-dependent fake-CLI receiver with coverage that verifies untrusted local packages cannot replace Azure CLI. Preserve the existing literal-argument and error-handling tests. This confirms the existing open isolation thread; it does not need a duplicate inline discussion.

Existing review comments: disposition

Topic	Current-head assessment
Shell expansion through percent signs and exclamation marks	Shell-free argument-list execution addresses the earlier cmd.exe expansion path; real-process literal-value tests pass on Windows.
Sensitive rejected tokens appearing in validation errors	The sink validates original execution tokens and omits their values from rejection errors; focused tests pass.
Managed/unmanaged restore validation after detachment	Preflight checks now precede detach; end-to-end mocked restore rejection tests pass. One older thread remains unresolved in GitHub, but the reported ordering problem is addressed in code.
Embedded quotes injecting additional unmanaged-restore arguments	The complete URI assignment is quoted as one token; rejection and single-token regression tests pass.
NIC revert validation after the IP swap	Revert-command validation now precedes the swap; rejection and happy-path tests pass.
Compatibility of percent-encoded URIs	Percent signs and exclamation marks are accepted literally; the percent-encoded restore test passes. The changelog describes the current shell-free behavior.
Missing interpreter isolation	Still open and independently reproduced; blocks approval.

The older engineering-agent report describes a prior head and is not evidence that this revision is ready to approve.

Validation

Validation used an isolated archive of the exact reviewed commit. The maintainer's dirty checkout was not changed. An existing development virtual environment supplied dependencies; no packages were installed.

Check	Result
Focused PR tests	PASS: 92 tests across the two changed regression-test modules, including real Windows subprocess tests.
All safe extension unit tests	PASS: 146 tests, 2 subtests; 3 warnings noted below. This total includes the focused tests, not an additional 146 distinct tests.
Independent launcher-isolation checks	FAIL: 2 regression assertions demonstrate working-directory and PYTHONPATH substitution; PASS: 1 isolated-interpreter control. These are reviewer-added checks, not failures in the author's existing suite.
Extension wheel build	PASS: version 2.4.6 wheel built locally. Setuptools emitted deprecation notices.
GitHub check runs	23 succeeded, 1 skipped. No failed or pending check runs were returned.
Legacy combined commit status	API returned pending with zero status contexts; this is distinct from the completed check-run results.
Local azdev style/linter	Not rerun; corresponding current-head CI checks succeeded.
License, credential scan, initialization files, extension index	Corresponding current-head CI checks succeeded. No independent local credential scan was claimed.
Breaking-change check	Current-head CI succeeded.
Live Azure integration tests	Not run. The live-only scenario module was explicitly excluded. No resource provisioning or disk/NIC changes were performed.

Test commands

From the isolated extension directory, with that directory on PYTHONPATH:

python -m pytest azext_vm_repair\tests\latest\test_command_injection.py azext_vm_repair\tests\latest\test_nested_az_invocation.py -q --tb=short
python -m pytest azext_vm_repair\tests\latest -q --tb=short --ignore=azext_vm_repair\tests\latest\test_repair_commands.py
python setup.py bdist_wheel

Reviewer-added checks:

python -m pytest test_pr10443_launcher_isolation.py -q --tb=short

The test runner in VS Code did not discover the isolated files, so pytest was invoked directly using the existing repository virtual environment.

Warnings and limitations

• Three existing returned-warning tests emitted an unraisable-exception warning because command_helper.__del__ accessed a missing is_verbose attribute. The relevant files are not changed by this PR; this is not counted as a new PR finding.
• Full resource lifecycle behavior was not exercised. Restore and NIC ordering evidence comes from mocked unit tests.
• Actual source-VM repair scripts and their catalog are unchanged. No cross-repository contract change was found.
• Version 2.4.6 and its changelog entry are present. The extension index is unchanged.
• No other high-confidence changed-line defect was confirmed. This is a scoped PR review, not a guarantee that the extension is free of vulnerabilities.

[Copilot]

🟡 Changes recommended

Original secret values can bypass validation, altering passwords or introducing extra CLI arguments.

2 open issues

• Validate raw secure parameters before shell tokenization · New
• Validate attach command before detaching repaired disks

2 resolved since last review

• Use isolated Python mode to prevent import hijacking
• Add restore tests for unsafe attach values and cleanup prevention

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The security-sensitive launcher and recovery-flow changes need human validation on supported Windows installations.

1 open issue

• Validate attach command before detaching repaired disks

1 resolved since last review

• Validate raw secure parameters before shell tokenization

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The security-sensitive Windows launcher has an unresolved user-site installation regression and needs native Windows validation.

2 open issues

• Windows user installs fail when -I hides the running CLI · New
• Validate attach command before detaching repaired disks

_{Review effort: Balanced}

[Edwin Bernal Microsoft (EdwinBernal1)]

[P2] Preserve the running CLI when its installation is in the user site

The -I change fixes the import-isolation issue, but it also disables user site-packages. I confirmed a compatibility regression with the actual helper under a controlled user-site-only visibility setup: the real CLI's normal version command succeeds, and the previously reviewed helper succeeds, while this revision's nested command fails with No module named azure.cli.

Please keep isolation, but bootstrap the trusted running CLI installation and its dependencies for the child. Add a user-site-only installation regression alongside the clean/working-directory/PYTHONPATH isolation cases. Do not simply remove -I. This confirms the existing open compatibility thread rather than introducing a separate duplicate issue.

Validation

All local checks used an isolated snapshot of the exact latest head and an existing development environment. The maintainer's dirty checkout was not modified. No packages were installed.

Check	Result
Changed regression modules	100 tests passed on Windows Python 3.13.
Reviewer isolation tests from prior review	3 passed. Combined focused invocation: 103 passed.
Full safe extension unit suite	154 tests passed, 2 subtests passed, including the 100 changed-module tests; 3 pre-existing warnings.
Reviewer user-site compatibility tests, previous head	2 passed.
Reviewer user-site compatibility tests, latest head	1 passed, 1 failed. The failure is the confirmed actual-helper import regression, not an author-suite failure.
Local wheel build	PASS, version 2.4.6. Setuptools deprecation notices only.
Current-head GitHub checks	23 succeeded, 1 skipped; no failed or pending check runs returned.
Legacy combined status endpoint	pending with zero status contexts; distinct from completed check-run results.
Style, linter, license, credential scan, breaking-change checks	Current-head CI succeeded; not independently rerun locally.
Live Azure scenarios	Not run. The live-only scenario module was explicitly excluded.

Test coverage and limitations

• The complete safe suite was run with the live-only scenario module excluded.
• Current-head tests cover literal shell metacharacters, percent-encoded URIs, expansion-like values, secret masking, rejection before tokenization, restore preflight, NIC revert ordering, asynchronous invocation, and CLI error reporting.
• Restore and NIC state-transition evidence is mocked; no live VM, disk, or NIC changes were performed.
• The same three unraisable destructor warnings seen in the previous review remain in unchanged warning-return tests. They are not new PR findings.
• The current head retains version 2.4.6 and updates its changelog for isolation and pre-tokenization validation.
• No script-library catalog or run-id changes are involved. No new cross-repository dependency was found.
• No other high-confidence delta defect was confirmed.