[VM] Fix #33979: az vm user update: Fix password parsing so ')' in --password does not break Windows VM password reset - #33980
Conversation
…' in --password does not break Windows VM password reset * Initial plan * [VM] az vm user update: fix ) in password breaking cmd.exe via az.bat GOTO refactor Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
microsoft-github-policy-service
Bot
added
Compute
Live test results —
|
x-engineering-agent
Bot
changed the title
az vm user update: Fix password parsing so ')' in --password does not break Windows VM password resetaz vm user update: Fix password parsing so ')' in --password does not break Windows VM password reset
|
VM |
![x-engineering-agent[bot]](https://avatars.githubusercontent.com/in/4205534?s=60&v=4){kind=small}
There was a problem hiding this comment.
Upstream CI
All 64 checks passed; no failures to report.
Test validation
- Live test: Passed (
azdev testagainst the changed VM test file, live run). - Regression coverage: Not applicable — the production change is in
src/azure-cli/az.bat, which is outside theazdev/module regression-coverage scope.
Review-skill findings
test-strength — src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_custom_vm_commands.py (new test test_reset_windows_admin_special_chars_in_password, lines 170-198)
The actual bug fix in this PR is entirely in src/azure-cli/az.bat: the previous IF EXIST (...) ELSE (...) block expanded %* inside a parenthesized block, so any argument containing ) (e.g. --password "Test)123") truncated/corrupted the batch script's command line before Python ever ran. The fix replaces the parenthesized IF/ELSE with a GOTO-based branch, which is the correct fix for this class of cmd.exe parsing bug.
However, the new test only exercises _reset_windows_admin in Python and asserts that protected_settings['Password'] is passed verbatim. That code path never went through az.bat and was not affected by the bug — this test would pass identically whether or not the az.bat fix is present or is reverted. It therefore provides no regression protection for the actual defect being fixed, and would not fail if the az.bat change were rolled back or broken again in the future.
Remediation: Since az.bat argument parsing can't be exercised through azdev/pytest, add a narrow platform-specific regression check instead (or explicitly note in the PR description why none is added), for example a Windows CI step that invokes az.bat --version (or a lightweight command) with a )-containing dummy argument via subprocess and asserts the process starts without a batch parsing error. Alternatively, at minimum call out in the PR description that the fix is verified manually on Windows, since the added Python unit test is not evidence the az.bat fix works or continues to work.
Verification: After adding a batch-level check, rerun it locally on Windows (cmd /c az.bat vm user update ... --password "Test)123") to confirm the script no longer breaks on ).
Risk assessment
0/100 · Low · Low confidence
The Low rating reflects the detected change scope with no elevated security, reliability, customer, operational, dependency, sovereign-cloud, generated-output, or cross-component signal.
- Change scope: 2 changed files, 41 changed lines (
+36/-5), including 0 production files. - Affected components: No production component was identified.
- Risk drivers: No elevated risk signal was detected.
- Regression evidence: No production-code regression-test signal applies.
- Confidence: Low because no production changed-line evidence was available.
- Required review: No additional owning-squad review signal was detected.
x-engineering-agent
Bot
added
azure-client-tools-agent
and removed
Azure Client Tools Agent Requested
labels
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes a Windows cmd.exe parsing failure where az vm user update --password "Test)123" could break inside az.bat before Python starts, by removing the parenthesized IF (...) ELSE (...) block that embedded %*.
Changes:
- Update
src/azure-cli/az.batto use aGOTO-based flow so)in arguments can’t terminate a parenthesized block duringcmd.exeparsing. - Add a VM unit test that mocks
VMExtensionCreateand verifies passwords containing),(,&,|, and^are passed through toprotected_settingsunchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_custom_vm_commands.py | Adds a regression unit test asserting special-character passwords reach the Windows access extension call path verbatim. |
| src/azure-cli/az.bat | Replaces the parenthesized IF/ELSE block with GOTO labels to avoid cmd.exe block-parsing issues with ) in %*. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@azure-client-tools-agent This is a correct fix but it needs a new live test to evalute the work. Also, it needs align the changes from this az.bat file with the other executable methods of az inclusive of https://github.com/Azure/azure-cli/blob/dev/build_scripts/windows/scripts/az_msi.cmd and https://github.com/Azure/azure-cli/blob/dev/build_scripts/windows/scripts/az_zip.cmd. Check historically, why was this issue not reported before if our file was incorrect all this time? |
Aditya Pujara (a0x1ab)
added
Azure Client Tools Agent Requested
and removed
azure-client-tools-agent
labels
Live test results —
|
There was a problem hiding this comment.
Automated review summary
CI: ✅ All 50/50 checks passed (breaking-change test, full test suite).
Live test: ✅ Reused prior dispatch — azdev test run for the changed test file completed successfully.
Regression coverage: ✅ Not applicable / no gap — the change touches az.bat (batch launcher) and adds a corresponding unit test (test_reset_windows_admin_special_chars_in_password) in the same PR.
Scope: The diff is limited to src/azure-cli/az.bat (replacing the IF EXIST (...) ELSE (...) parenthesis block with a GOTO-based pattern so ) and other cmd.exe metacharacters in --password don't break argument parsing) and a new unit test validating that special-character passwords reach protected_settings verbatim. Both changes match the stated title/description and issue #33919 (az vm user update password parsing fix).
Review-skill findings: none confirmed.
No further action needed from automation; ready for human merge decision.
x-engineering-agent
Bot
added
Azure Client Tools Agent Requested
and removed
Azure Client Tools Agent Requested
labels
|
Started a Copilot task using |
…and add special-password live test Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Live test results —
|
There was a problem hiding this comment.
Upstream CI
Azure.azure-cli Full Test
- Result: failure
- Relevance: Uncertain
- Evidence: Build #20260831.1 had test failures
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
Azure.azure-cli Full Testand confirm it passes on the new head.
Nested failing jobs in the same build:
- Automation Full Test Python312 Profile Latest instance2 — failure
- Automation Full Test Python314 Profile Latest instance2 — failure
Test validation
- Live test: Failed with conclusion
failure. Workflow run - Regression coverage: Not applicable to the changed files.
No review-skill findings were confirmed for this change.
Risk assessment
0/100 · Low · Low confidence
The Low rating reflects the detected change scope with no elevated security, reliability, customer, operational, dependency, sovereign-cloud, generated-output, or cross-component signal.
- Change scope: 5 changed files, 98 changed lines (
+79/-19), including 0 production files. - Affected components: No production component was identified.
- Risk drivers: No elevated risk signal was detected.
- Regression evidence: No production-code regression-test signal applies.
- Confidence: Low because no production changed-line evidence was available.
- Required review: No additional owning-squad review signal was detected.
|
Started a Copilot task using |
…rding exists) Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Live test results —
|
Live test results —
|
There was a problem hiding this comment.
Live test failure
The dispatched azdev test --live run (only the changed test files in this PR
were selected: test_custom_vm_commands.py, test_vm_commands.py) reported
1 failed, 8 passed:
azure-cli/src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py::VMImageListByAliasesScenarioTest::test_vm_image_list_by_alias FAILED
self.assertTrue(len(result) >= 1)
self.assertEqual(result[-1]['publisher'], 'Canonical')
> self.assertTrue('lts' in result[-1]['sku'])
E AssertionError: False is not true
This failing test (test_vm_image_list_by_alias) is a pre-existing scenario
test in test_vm_commands.py — it is unrelated to the new
test_vm_user_update_win_special_password regression test or the
_reset_windows_admin fix added by this PR. It exercises az vm image list-by-aliases and asserts the latest Canonical Ubuntu image SKU contains
'lts'; live Azure image publisher data may have changed the SKU naming
convention since the assertion was written, causing an environment/data drift
failure rather than a regression introduced here.
Next action: confirm whether test_vm_image_list_by_alias still reflects
the current Canonical Ubuntu SKU naming (query az vm image list --publisher Canonical --sku "*" --all --output table or the equivalent image list-by-
aliases output) and update the assertion if the naming convention changed.
If the assertion is confirmed stale/unrelated, update or relax it so it no
longer blocks this PR; otherwise investigate why the alias lookup no longer
returns an lts SKU. Re-run only the changed-file live test after the fix:
azdev test test_custom_vm_commands test_vm_commands --live.
Test validation
- Live test: Failed with conclusion
failure. Workflow run - Regression coverage: Not applicable to the changed files.
Risk assessment
0/100 · Low · Low confidence
The Low rating reflects the detected change scope with no elevated security, reliability, customer, operational, dependency, sovereign-cloud, generated-output, or cross-component signal.
- Change scope: 5 changed files, 99 changed lines (
+80/-19), including 0 production files. - Affected components: No production component was identified.
- Risk drivers: No elevated risk signal was detected.
- Regression evidence: No production-code regression-test signal applies.
- Confidence: Low because no production changed-line evidence was available.
- Required review: No additional owning-squad review signal was detected.
|
Started a Copilot task using |
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Live test results —
|
Live test results —
|
There was a problem hiding this comment.
Upstream CI
All required Azure DevOps checks passed (50/50 completed, 0 failed).
Test validation
- Live test:
azdev test test_custom_vm_commands test_vm_commands --live --seriesFAILED — run https://github.com/Azure/issue-sentinel/actions/runs/33355571170.- Only 11 of the selected tests ran before the suite aborted: pytest is configured with
maxfail=1, and the very first failure —VmReimageTest::test_vm_reimage— stopped the run before it ever reached the new regression test this PR added,VmUserUpdateWinScenarioTest::test_vm_user_update_win_special_password(defined immediately after the existingtest_vm_user_update_winintest_vm_commands.py). test_vm_reimage's failure is an environment/subscription issue unrelated to this PR's change:network public-ip create --ip-tags FirstPartyUsage=/NonProdfails withSubscriptionNotRegisteredForFeature: ... Microsoft.Network/AllowBringYourOwnPublicIpAddress. This test isn't touched by the diff, and the same test file already skips a different test with@unittest.skip('SubscriptionNotRegisteredForFeature'), confirming this subscription-feature gap is a known, pre-existing environment limitation rather than something introduced here.- Net effect: the actual regression test for this fix (
test_vm_user_update_win_special_password) has not yet been verified to pass, because the unrelated failure short-circuited the run.
- Only 11 of the selected tests ran before the suite aborted: pytest is configured with
- Regression coverage: Not applicable — no
command_modulesproduction files were changed (only Windows launcher scriptsaz.bat,az_msi.cmd,az_zip.cmd, and the two VM test files).
Please make the new regression test (test_vm_user_update_win_special_password) actually execute and pass. For example, mark or reorder so VmReimageTest::test_vm_reimage (an unrelated, already-known-flaky live test) does not abort the run under maxfail=1 before the new test runs — e.g. by skipping/deprioritizing the reimage test locally, or re-dispatching test selection so only the new test class runs. Confirm the new test passes live before requesting review again.
Risk assessment
0/100 · Low · Low confidence
The Low rating reflects the detected change scope with no elevated security, reliability, customer, operational, dependency, sovereign-cloud, generated-output, or cross-component signal.
- Change scope: 5 changed files, 111 changed lines (
+87/-24), including 0 production files. - Affected components: No production component was identified.
- Risk drivers: No elevated risk signal was detected.
- Regression evidence: No production-code regression-test signal applies.
- Confidence: Low because no production changed-line evidence was available.
- Required review: No additional owning-squad review signal was detected.
|
Started a Copilot task using |
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Live test results —
|
There was a problem hiding this comment.
Handoff summary
Automated Copilot fixing gave up after 3 attempts (agent_review cycle); leaving this PR for human review.
Upstream CI
All 50 required checks passed at head b737b2e6038e09c75982475b026c1fad8cebc041 (0 failed, 0 pending).
Test validation
- Live test (
azdev test --live --series, changed test files only) FAILED:test_vm_image_list_thru_services_deprecation_statusraisedazure.core.exceptions.HttpResponseError: (UnsupportedEdgeZone) Edge zone 'losangeles' not found. The available edge zones in location 'westus' are ''.This test exercisesvm image list --edge-zone losangeles, which is unrelated to this PR's actual change (Windows launcher-script)-in-password parsing fix foraz vm user update). The failure looks like a test-environment/subscription capability limitation (thelosangelesedge zone is not provisioned in the live-test subscription/region), not a regression introduced by this PR's diff. - Regression coverage: no gap — this PR adds a unit test (
test_reset_windows_admin_special_chars_in_password) and a@live_only()scenario test (test_vm_user_update_win_special_password) directly covering the fix, and already skips the known-flakytest_vm_reimagecase from an earlier iteration.
Next action
- A maintainer should re-run the live-test workflow (
live-test.yml) once, sincetest_vm_image_list_thru_services_deprecation_statusis unrelated to the code under test — if it passes with a fresh subscription/edge-zone allocation, this confirms the earlier result was environmental flake. - If it fails again, verify
losangelesedge-zone availability for the live-test subscription inwestus; this is a test-infrastructure issue, not something this PR'saz.bat/az_msi.cmd/az_zip.cmdchanges should touch. - No further source change is requested for the reported failure; this is a non-blocking, unrelated test-infrastructure issue.
Risk assessment
0/100 · Low · Low confidence
The Low rating reflects the detected change scope with no elevated security, reliability, customer, operational, dependency, sovereign-cloud, generated-output, or cross-component signal.
- Change scope: 5 changed files, 112 changed lines (
+88/-24), including 0 production files. - Affected components: No production component was identified.
- Risk drivers: No elevated risk signal was detected.
- Regression evidence: No production-code regression-test signal applies.
- Confidence: Low because no production changed-line evidence was available.
- Required review: No additional owning-squad review signal was detected.
Aditya Pujara (a0x1ab)
added
X Engineering Agent Reviewed
|
🔔 Routing this PR to @Azure/act-observability-squad. |
🤖 PR Validation — ️✔️ All clear
Description
Fixes #33979.
On Windows,
az vm user update --password 'Test)123'fails with123 was unexpected at this time.before Python starts. The)in the password closes theIF EXIST (...) ... ELSE (...)parenthesis block inaz.bat, breaking cmd.exe parsing.Related command
az vm user updateDescription
src/azure-cli/az.bat: Replace theIF EXIST (...) ELSE (...)block—which embeds%*inside parentheses—with a GOTO-based pattern so argument values containing)(or other cmd.exe metacharacters) never terminate a grouping block:test_custom_vm_commands.py: Addtest_reset_windows_admin_special_chars_in_password— mocksVMExtensionCreateand asserts passwords containing),(,&,|, and^reachprotected_settingsverbatim, confirming the Python-level extension call path does not corrupt special characters.Testing Guide
On Windows with cmd.exe or PowerShell, the following should now succeed rather than erroring before any Azure API call:
az vm user update -g myRg -n myWinVm --username AzureUser --password "Test)123"Unit test (offline, no Azure subscription needed):
History Notes
[VM]
az vm user update: Fix)and other shell metacharacters in--passwordbreaking Windows VM password reset on cmd.exe