Conversation
hemarina
requested review from
JeffreyCA,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Windows MSI self-update flow to stop bypassing signature verification and adjusts the PowerShell invocation strategy per update channel.
Changes:
- Removes
-SkipVerifyusage when invokinginstall-azd.ps1so MSI Authenticode verification can run. - Changes
buildInstallScriptArgsbehavior: stable pipes the script toInvoke-Expression, daily downloads to a temp file and runs with-Version 'daily'and-InstallFolder. - Updates/extends unit tests for the new argument structures and removes the now-dead
versionFlagtest.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cli/azd/pkg/update/msi_windows.go |
Reworks PowerShell command construction to avoid -SkipVerify and to handle daily channel parameters. |
cli/azd/pkg/update/msi_windows_test.go |
Updates tests to validate the new stable/daily argument structure and adds negative-assertions for stable. |
spboyer
reviewed
Mar 26, 2026
Member
spboyer
left a comment
spboyer
left a comment
There was a problem hiding this comment.
Code Review
1. azd update now requires PowerShell 7
cli/azd/pkg/update/manager.go:513 — High
The MSI update path changed from powershell to pwsh. pwsh.exe (PowerShell 7) is not installed by default on Windows 10/11 — only powershell.exe (5.1) ships pre-installed. This will cause azd update to fail on machines without PowerShell 7. The rest of the codebase handles this with a pwsh -> powershell fallback (see pkg/tools/powershell/powershell.go:40-61). Additionally, verifyAuthenticode at line 713 in the same file still uses powershell, creating an inconsistency.
Fix: Keep using powershell (the install script and Get-AuthenticodeSignature work fine under 5.1), or add an availability check with fallback.
2. Daily-channel install path not escaped for PowerShell
cli/azd/pkg/update/msi_windows.go:169 — Medium
The daily update command formats -InstallFolder '%s' directly. If %LOCALAPPDATA% contains an apostrophe (e.g., username O'Connor), the generated PowerShell command becomes syntactically invalid.
Fix: Escape embedded ' characters for PowerShell single-quoted literals, or pass the install folder as a separate argument.
jongio
requested changes
Mar 27, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
PR Review - #7298
[azd update] apply code sign verification for windows msi install by @hemarina
Findings
| Category | Medium | Low |
|---|---|---|
| Refactoring | 1 | 0 |
| Tests | 1 | 0 |
| Logic | 1 | 0 |
| Quality | 0 | 1 |
| Total | 3 | 1 |
Key Findings
-
[MEDIUM] Refactoring: Both channels should use the same download-and-execute pattern. The old code had one path for all channels (download to temp, run, clean up). This PR splits into two (pipe for stable, download for daily). If the stable channel is also changed to download-to-file (which Copilot's inline feedback already suggested for signature validation), the paths converge back to the old architecture. Simpler approach: keep the single download-and-execute pattern, just remove
-SkipVerifyand conditionally add-Version/-InstallFolderfor daily. -
[MEDIUM] Tests: The
powershell->pwshchange inupdateViaMSIhas no test coverage. All PR tests coverbuildInstallScriptArgshelpers only. -
[MEDIUM] Logic: Stable channel no longer passes
-Version 'stable'explicitly. The remote install script has conflicting documentation vs implementation for its default (latestin help text,stableinparam()), making this a real drift risk.
Test Coverage Estimate
buildInstallScriptArgs(): Good coverage - both channels tested with positive and negative assertions, structure validatedupdateViaMSI()pwsh change: No coverage - the behavioral change to which executable gets invoked isn't testedversionFlag()removal: Properly handled - tests removed since function was deleted
What's Done Well
- Removing
-SkipVerifyis the right call - MSI Authenticode verification shouldn't be bypassed - The daily channel download approach is solid - temp file, execute, clean up
- Tests use
wantNotContainsto verify stable doesn't leak daily-only parameters - good defensive testing - Doc comment explains WHY stable and daily differ (pipe doesn't support named params)
3 inline comments below. Review converged after 2 analysis passes + 1 deepening pass + cross-validation.
3 existing comments/threads (from Copilot and @spboyer) were already addressing issues we also found - those aren't repeated here.
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
vhvb1989
approved these changes
Mar 28, 2026
jongio
reviewed
Mar 28, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
PR Review - #7298
[azd update] apply code sign verification for windows msi install by @hemarina
Findings
| Category | Medium |
|---|---|
| Tests | 2 |
Key Findings
- [MEDIUM] Tests: The PR's core goal (removing
-SkipVerify) isn't guarded by a negative test assertion - [MEDIUM] Tests:
TestUpdateViaMSI_InvokesPowerShellWithCorrectArgsnever actually runs in CI
Test Coverage Estimate
escapeForPSSingleQuote(): Solid - 4 cases including apostrophe edge casesbuildInstallScriptArgs(): Good - both channels, positive + negative assertions, structure validatedupdateViaMSI()integration: Effectively zero (test always skips)
What's Done Well
- Removing
-SkipVerifyis the right call - Authenticode verification shouldn't be bypassed - Both channels now use the same download-to-temp pattern - consistent and cleaner
escapeForPSSingleQuoteproperly handles O'Connor-style pathswantNotContainsassertions are good defensive testing for stable vs daily separation- All PowerShell constructs used (
Join-Path,Invoke-RestMethod -OutFile,& $scriptcall operator, single-quote escaping) work identically on Windows PowerShell 5.1 and PowerShell 7+
2 inline comments below. Review converged after 2 analysis passes + 1 deepening pass + cross-validation.
3 existing reviews were already addressing issues we also found - those aren't repeated here.
| "Remove-Item", | ||
| }, | ||
| wantNotContains: []string{ | ||
| "-InstallFolder", |
Member
There was a problem hiding this comment.
[MEDIUM] Tests: No regression guard for -SkipVerify removal
The whole point of this PR is removing -SkipVerify to enable Authenticode verification. But neither channel's test asserts that -SkipVerify is absent. If someone accidentally re-adds it in a future change, no test catches the regression.
Add "-SkipVerify" to wantNotContains for stable here, and add a wantNotContains: []string{"-SkipVerify"} for the daily channel too.
Suggested change
| "-InstallFolder", | |
| wantNotContains: []string{ | |
| "-InstallFolder", | |
| "-SkipVerify", | |
| }, |
| // TestUpdateViaMSI_InvokesPowerShellWithCorrectArgs verifies that updateViaMSI calls | ||
| // "powershell" with the arguments produced by buildInstallScriptArgs. | ||
| func TestUpdateViaMSI_InvokesPowerShellWithCorrectArgs(t *testing.T) { | ||
| // Point LOCALAPPDATA at the test binary so isStandardMSIInstall passes. |
Member
There was a problem hiding this comment.
[MEDIUM] Tests: This test always skips in CI/dev
Both t.Skip conditions fire in every realistic test environment: go test compiles to temp directories that never end with Programs\Azure Dev CLI, and backupCurrentExe would fail trying to rename the test binary. The integration coverage this was meant to provide is effectively zero.
This follows the existing TestIsStandardMSIInstall_StandardPath pattern in this file, so it's a known limitation. Consider adding a doc comment noting this test is for manual validation on actual MSI installs, or extracting isStandardMSIInstall into a mockable dependency.
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.
#7265
This pull request refactors how the Windows MSI installation script arguments are constructed and tested, improving clarity and correctness for different release channels (stable vs. daily). The main change is to simplify and clarify the logic for building PowerShell command arguments, ensuring that stable and daily channels are handled distinctly and tested accordingly.
Refactoring and logic improvements for install script arguments:
versionFlagfunction and refactoredbuildInstallScriptArgsto directly construct the appropriate PowerShell command for each channel, ensuring that stable uses a piped script and daily downloads and executes the script with additional parameters.Test updates for new logic and coverage:
TestVersionFlagand updatedTestBuildInstallScriptArgsto verify the presence or absence of key substrings in the generated arguments, matching the new logic for stable and daily channels. [1] [2] [3]TestBuildInstallScriptArgsandTestBuildInstallScriptArgs_Structureto check for correct argument construction, including validation that stable does not use temporary files or extra parameters, while daily does.