Skip to content

Commit 7604885

Browse files
committed
Stop a non-elevated run from continuing past the admin guard
Ensure-Admin.ps1 is dot-sourced, and `exit` inside a dot-sourced script does not terminate the caller. Every exit in that guard was therefore inert: WinSwift printed "must be run as Administrator" and then carried on into the apply pipeline. Reproduced non-elevated with stdin redirected, which is what any scripted or CI invocation looks like. Read-Host returns an empty string immediately, the prompt reads as declined, `exit 1` does nothing, and the run proceeds: WinSwift must be run as Administrator. ... [WhatIf] Create registry backup [WhatIf] Apply 15 registry changes from 'Disable_Telemetry.reg' [WhatIf] Disable Scheduled Task: ...Microsoft Compatibility Appraiser exit code 0 Under -DryRun nothing is written. Without it, an unprivileged process would attempt real registry imports and scheduled task changes, fail partway through on access denied, and leave a half-applied system. The same defect breaks the relaunch path: after Start-Process -Verb RunAs succeeds, `exit 0` does not stop the parent, so the elevated child and the original non-elevated process run WinSwift concurrently. Confirmed the mechanism with a minimal repro. A dot-sourced script invoked with arguments cannot terminate its caller through `exit`, with or without [CmdletBinding()]; `throw` and caller-side handling both do. Fix: the guard reports its outcome through $script:ElevationOutcome, which propagates to the caller because dot-sourcing shares scope, and WinSwift.ps1 exits on anything other than 'Elevated' before any runtime module loads. 'Relaunched' exits 0 because the elevated child owns the run; 'Denied' and 'Failed' exit 1. Also stop prompting when no console can answer. A redirected read returned instantly and was indistinguishable from a declined prompt, so that case now reports why it cannot continue. Verified after the fix: both the read-only -Verify path and the -DryRun apply path exit 1 without reaching the pipeline. Adds source assertions for the outcome contract and its position ahead of module loading, plus a behavioral test that runs the entry script unelevated and asserts the pipeline is never reached. That test skips when already elevated, which is the case on CI runners.
1 parent ba6088b commit 7604885

4 files changed

Lines changed: 206 additions & 129 deletions

File tree

Scripts/Helpers/Ensure-Admin.ps1

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,30 @@ param(
55
[array]$OriginalUnboundArguments
66
)
77

8+
# This script is dot-sourced, and `exit` inside a dot-sourced script does not
9+
# terminate the caller. Relying on it let a non-elevated run continue into the
10+
# apply pipeline. The outcome is reported through $script:ElevationOutcome
11+
# instead, and WinSwift.ps1 exits on anything other than 'Elevated'.
12+
$script:ElevationOutcome = 'Elevated'
13+
814
$isAdmin = ([Security.Principal.WindowsPrincipal] `
915
[Security.Principal.WindowsIdentity]::GetCurrent()
1016
).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)
1117

1218
if (-not $isAdmin) {
1319
Write-Host "WinSwift must be run as Administrator." -ForegroundColor Red
1420

21+
# Prompting is pointless when nothing can answer, and a redirected read
22+
# returns immediately, which previously read as a declined prompt.
23+
$inputIsRedirected = $false
24+
try { $inputIsRedirected = [Console]::IsInputRedirected } catch { }
25+
26+
if ($inputIsRedirected) {
27+
Write-Host "No interactive console is available to confirm elevation. Re-run WinSwift from an elevated session." -ForegroundColor Red
28+
$script:ElevationOutcome = 'Denied'
29+
return
30+
}
31+
1532
$choice = Read-Host "Restart as Administrator? (y/n)"
1633

1734
if ($choice -match '^[Yy]$') {
@@ -55,11 +72,16 @@ if (-not $isAdmin) {
5572
}
5673
catch {
5774
Write-Error "Failed to start WinSwift as Administrator: $_"
58-
exit 1
75+
$script:ElevationOutcome = 'Failed'
76+
return
5977
}
6078

61-
exit 0
79+
# The elevated child owns the run from here; this process must stop so the
80+
# two do not execute concurrently.
81+
$script:ElevationOutcome = 'Relaunched'
82+
return
6283
}
6384

64-
exit 1
85+
$script:ElevationOutcome = 'Denied'
86+
return
6587
}

Tests/Unit/Test-SafetyGuards.ps1

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,48 @@ Describe 'WinSwift startup safety guards' {
3131
$script:adminScript | Should -Match '-ErrorAction Stop'
3232
}
3333

34+
It 'reports the elevation outcome instead of relying on exit' {
35+
# `exit` inside a dot-sourced script does not terminate the caller, so a
36+
# non-elevated run used to continue into the apply pipeline.
37+
$script:adminScript | Should -Match "\`$script:ElevationOutcome = 'Elevated'"
38+
$script:adminScript | Should -Match "\`$script:ElevationOutcome = 'Denied'"
39+
$script:adminScript | Should -Match "\`$script:ElevationOutcome = 'Relaunched'"
40+
$script:adminScript | Should -Match "\`$script:ElevationOutcome = 'Failed'"
41+
$script:adminScript | Should -Not -Match '(?m)^\s*exit \d'
42+
}
43+
44+
It 'stops the run when elevation was not obtained' {
45+
$guardPosition = $script:entryScript.IndexOf('$script:ElevationOutcome -ne')
46+
$environmentPosition = $script:entryScript.IndexOf('Initialize-Environment.ps1')
47+
48+
$guardPosition | Should -BeGreaterThan -1
49+
$guardPosition | Should -BeLessThan $environmentPosition -Because 'the run must stop before any runtime module loads'
50+
}
51+
52+
It 'refuses a non-interactive run that cannot elevate' -Skip:(
53+
([Security.Principal.WindowsPrincipal] [Security.Principal.WindowsIdentity]::GetCurrent()
54+
).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)
55+
) {
56+
# Only meaningful unelevated. CI runners are administrators, so this is
57+
# skipped there and the source assertions above carry the contract.
58+
$repoRoot = Resolve-Path (Join-Path $PSScriptRoot '..\..')
59+
$psi = New-Object System.Diagnostics.ProcessStartInfo
60+
$psi.FileName = 'powershell.exe'
61+
$psi.Arguments = '-NoProfile -ExecutionPolicy Bypass -File "{0}" -DryRun -Silent -CLI -DisableTelemetry' -f (Join-Path $repoRoot 'WinSwift.ps1')
62+
$psi.RedirectStandardOutput = $true
63+
$psi.RedirectStandardError = $true
64+
$psi.RedirectStandardInput = $true
65+
$psi.UseShellExecute = $false
66+
67+
$process = [System.Diagnostics.Process]::Start($psi)
68+
$process.StandardInput.Close()
69+
$stdout = $process.StandardOutput.ReadToEnd()
70+
$null = $process.WaitForExit(90000)
71+
72+
$process.ExitCode | Should -Not -Be 0
73+
$stdout | Should -Not -Match '\[WhatIf\]' -Because 'the apply pipeline must never be reached without elevation'
74+
}
75+
3476
It 'quotes bound arrays and unbound arguments during elevation' {
3577
$script:adminScript | Should -Match 'paramValue -is \[array\]'
3678
$script:adminScript | Should -Match 'OriginalUnboundArguments'

WinSwift-Standalone.ps1

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)