From ddd9f76729d5662201b84bd0a51281cd3ac64ad3 Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Mon, 17 Aug 2026 03:35:04 +0530 Subject: [PATCH] fix(install): stop the PowerShell installer leaking temp dirs into the real user PATH (#2985) ## Description `scripts/install.ps1` persists the install directory to the user's PATH through `Ensure-PathEntry`, which calls `[Environment]::SetEnvironmentVariable('Path', ..., 'User')`. That value lives in the `HKCU\Environment` registry key, so it is **not** scoped by a `HOME` / `USERPROFILE` override. `tests/test_install/test_native_installers.py::test_powershell_native_installer_supports_persistent_docker_lifecycle` runs that real installer against a `tmp_path` fake home. Every run therefore prepended the test's throwaway shim directory to the developer's actual, persistent user PATH -- and it stayed there after the test finished. The entries accumulate one per run, ahead of the real install dir; and since the installer also drops `headroom.ps1`/`headroom.cmd` into that dir, `headroom` in a fresh shell could then resolve to a leftover wrapper from a deleted temp directory (#2970). ## Fix Make the persistence scope configurable via `HEADROOM_INSTALL_PATH_SCOPE`, defaulting to `'User'` so production behavior is unchanged: ```powershell $scope = if ($env:HEADROOM_INSTALL_PATH_SCOPE) { $env:HEADROOM_INSTALL_PATH_SCOPE } else { 'User' } $currentPath = [Environment]::GetEnvironmentVariable('Path', $scope) ... [Environment]::SetEnvironmentVariable('Path', ($newPath -join ';'), $scope) ``` The installer tests (`_build_env`) set `HEADROOM_INSTALL_PATH_SCOPE=Process`, so the PATH update stays in the spawned PowerShell process (discarded when it exits) instead of writing to the registry. Fixes #2970 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature - [ ] Breaking change - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `scripts/install.ps1` (`Ensure-PathEntry`): read/write the PATH via `$env:HEADROOM_INSTALL_PATH_SCOPE` (default `'User'`). - `tests/test_install/test_native_installers.py`: `_build_env` sets `HEADROOM_INSTALL_PATH_SCOPE=Process` for every installer invocation; add a Windows-only `test_powershell_installer_does_not_leak_into_user_path` asserting the real User PATH entry count is unchanged across an installer run. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check`) - [x] New test added ### Test Output ```text tests/test_install/test_native_installers.py -k does_not_leak_into_user_path 1 passed # uvx ruff@0.15.22 check tests/test_install/test_native_installers.py -> All checks passed! ``` ## Real Behavior Proof - Environment: Windows 11, Windows PowerShell 5.1, Python 3.12.11, project venv, pytest 9.1.1, ruff 0.15.22 via uvx. - Exact command / steps: recorded the real user PATH entry count (`([Environment]::GetEnvironmentVariable('Path','User') -split ';').Count` = 27), ran the PowerShell installer test with the fix, then re-read the count: still 27 -- no leak. The new `test_powershell_installer_does_not_leak_into_user_path` formalizes this (before == after). - Observed result: running the installer test suite no longer mutates the developer's persistent user PATH; production installs still persist to `'User'` as before. - Not tested: the sibling `test_powershell_native_installer_supports_persistent_docker_lifecycle` fails on my Windows host on an unrelated `trusted_cidrs` dashboard-gateway assertion (it fails identically on `main` without this change, and the whole PowerShell suite is skipped on the Linux CI runners). This PR does not touch that path. ## Runtime Rollout Safety - Rollout-managed feature(s): none. This is the native PowerShell installer script, not a rollout-channel-gated runtime feature. - Minimum rollout channel: N/A (no rollout-managed behavior). - Stable/default behavior changed: no. Production installs still persist PATH to the `User` scope exactly as before; the new `HEADROOM_INSTALL_PATH_SCOPE` override defaults to `User` and is used only by the test suite to avoid mutating the developer's persistent PATH. - Kill switch / disable path: leave `HEADROOM_INSTALL_PATH_SCOPE` unset (the default) for the normal `User` behavior. - Unsafe override required: no. - Qualification impact: none. Installer-only; no proxy runtime path is touched. - Rollback path: revert this PR; the installer returns to writing the `User` PATH unconditionally. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective - [x] New and existing unit tests pass locally with my changes - [x] I did **not** edit `CHANGELOG.md`: it is generated by release-please from my Conventional Commit PR title ## Additional Notes The scope override defaults to `'User'`, so nothing changes for real installs. It doubles as an escape hatch for any environment (CI images, ephemeral containers) that must not touch the persistent user PATH. --------- Co-authored-by: JD Davis --- scripts/install.ps1 | 28 ++++- tests/test_install/test_native_installers.py | 124 +++++++++++++++++++ 2 files changed, 150 insertions(+), 2 deletions(-) diff --git a/scripts/install.ps1 b/scripts/install.ps1 index b7444fb2a..9926690de 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -22,14 +22,38 @@ function Require-Command { function Ensure-PathEntry { param([string]$PathEntry) - $currentPath = [Environment]::GetEnvironmentVariable('Path', 'User') + # Persist to the User PATH by default. The 'User' scope lives in + # HKCU\Environment and is NOT redirected by a HOME/USERPROFILE override, so a + # caller that must not mutate the real persistent PATH (the installer test + # suite, which runs this against a throwaway fake home) sets + # HEADROOM_INSTALL_PATH_SCOPE=Process to keep the update ephemeral instead of + # leaking the temp shim dir into the developer's actual user PATH (#2970). + # + # Only those two persistence modes are supported. The value is handed to + # .NET's EnvironmentVariableTarget, whose 'Machine' member would rewrite the + # SYSTEM-wide PATH if this variable were inherited by an elevated installer, + # and a typo would otherwise fail late with an opaque enum-conversion error. + # Normalize case-insensitively and allow-list 'User'/'Process', failing early + # and clearly for 'Machine' or anything else. + $scope = 'User' + if ($env:HEADROOM_INSTALL_PATH_SCOPE) { + switch ($env:HEADROOM_INSTALL_PATH_SCOPE.Trim().ToLowerInvariant()) { + 'user' { $scope = 'User' } + 'process' { $scope = 'Process' } + default { + throw "HEADROOM_INSTALL_PATH_SCOPE must be 'User' or 'Process' (got '$($env:HEADROOM_INSTALL_PATH_SCOPE)'); 'Machine' and other targets are not supported." + } + } + } + + $currentPath = [Environment]::GetEnvironmentVariable('Path', $scope) $parts = @() if ($currentPath) { $parts = $currentPath -split ';' | Where-Object { $_ } } if ($parts -notcontains $PathEntry) { $newPath = @($PathEntry) + $parts - [Environment]::SetEnvironmentVariable('Path', ($newPath -join ';'), 'User') + [Environment]::SetEnvironmentVariable('Path', ($newPath -join ';'), $scope) } } diff --git a/tests/test_install/test_native_installers.py b/tests/test_install/test_native_installers.py index 94fee0ba9..37bcaac3e 100644 --- a/tests/test_install/test_native_installers.py +++ b/tests/test_install/test_native_installers.py @@ -224,6 +224,11 @@ def _build_env(home: Path, tmp_path: Path) -> dict[str, str]: env["PATH"] = str(shim_dir) + os.pathsep + env.get("PATH", "") env["FAKE_DOCKER_STATE"] = str(tmp_path / "fake-docker-state.json") env["FAKE_DOCKER_LOG"] = str(tmp_path / "fake-docker.log") + # #2970: the PowerShell installer's Ensure-PathEntry persists to the 'User' + # PATH scope (HKCU\Environment), which a HOME/USERPROFILE override does not + # redirect. Keep the PATH update ephemeral (Process scope) so running these + # tests never leaks the throwaway shim dir into the developer's real PATH. + env["HEADROOM_INSTALL_PATH_SCOPE"] = "Process" return env @@ -555,6 +560,125 @@ def _powershell_executable() -> str | None: return shutil.which("pwsh") or shutil.which("powershell") or shutil.which("powershell.exe") +@pytest.mark.skipif( + os.name != "nt" or _powershell_executable() is None, + reason="Windows PowerShell coverage runs on Windows hosts only", +) +def test_powershell_installer_does_not_leak_into_user_path(tmp_path: Path) -> None: + """The installer must not mutate the real HKCU User PATH (#2970). + + ``Ensure-PathEntry`` persists to the 'User' scope, which a HOME/USERPROFILE + override does not redirect, so running the installer against a throwaway home + used to leak the temp shim dir into the developer's real PATH. ``_build_env`` + now sets ``HEADROOM_INSTALL_PATH_SCOPE=Process`` to keep the update + ephemeral; the real User PATH must be unchanged across the run. + """ + powershell = _powershell_executable() + assert powershell is not None + + count_cmd = [ + powershell, + "-NoProfile", + "-Command", + "([Environment]::GetEnvironmentVariable('Path','User') -split ';').Count", + ] + before = _run(count_cmd, env=os.environ.copy()).stdout.strip() + + home = tmp_path / "home" + (home / ".local").mkdir(parents=True) + env = _build_env(home, tmp_path) + env["HEADROOM_DOCKER_IMAGE"] = "headroom:test-image" + _run( + [ + powershell, + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-File", + str(REPO_ROOT / "scripts" / "install.ps1"), + ], + env=env, + cwd=REPO_ROOT, + ) + + after = _run(count_cmd, env=os.environ.copy()).stdout.strip() + assert after == before, f"installer leaked into the real User PATH: {before} -> {after}" + + +# AST-extract Ensure-PathEntry from install.ps1 and invoke it in isolation under +# a given HEADROOM_INSTALL_PATH_SCOPE, so the scope allow-list is exercised +# without running the whole installer. Parsing via the PowerShell AST (not a +# regex) keeps this pinned to the real function body. Only 'Process' (ephemeral) +# and the throwing paths are driven — never 'User', which would mutate the real +# HKCU PATH. +_ENSURE_PATH_SCOPE_HARNESS = r""" +param([string]$InstallScript, [string]$ScopeValue) +$ErrorActionPreference = 'Stop' +$ast = [System.Management.Automation.Language.Parser]::ParseFile( + $InstallScript, [ref]$null, [ref]$null) +$fn = $ast.FindAll({ + param($n) + $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and + $n.Name -eq 'Ensure-PathEntry' +}, $true) | Select-Object -First 1 +if (-not $fn) { Write-Output 'NOFUNC'; exit 3 } +Invoke-Expression $fn.Extent.Text +$env:HEADROOM_INSTALL_PATH_SCOPE = $ScopeValue +try { + Ensure-PathEntry -PathEntry 'C:\headroom-scope-test-marker' + Write-Output 'OK' +} catch { + Write-Output ('ERR:' + $_.Exception.Message) +} +""" + + +def _invoke_scope_harness(scope_value: str, tmp_path: Path) -> str: + powershell = _powershell_executable() + assert powershell is not None + harness = tmp_path / "scope_harness.ps1" + harness.write_text(_ENSURE_PATH_SCOPE_HARNESS, encoding="utf-8") + result = _run( + [ + powershell, + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-File", + str(harness), + "-InstallScript", + str(REPO_ROOT / "scripts" / "install.ps1"), + "-ScopeValue", + scope_value, + ], + env=os.environ.copy(), + check=False, + ) + return result.stdout.strip() + + +@pytest.mark.skipif( + os.name != "nt" or _powershell_executable() is None, + reason="Windows PowerShell coverage runs on Windows hosts only", +) +def test_path_scope_accepts_process_case_insensitively(tmp_path: Path) -> None: + """'Process' (any case) is a supported ephemeral target: Ensure-PathEntry runs.""" + assert _invoke_scope_harness("process", tmp_path).endswith("OK") + assert _invoke_scope_harness("Process", tmp_path).endswith("OK") + + +@pytest.mark.skipif( + os.name != "nt" or _powershell_executable() is None, + reason="Windows PowerShell coverage runs on Windows hosts only", +) +def test_path_scope_rejects_machine_and_invalid_values(tmp_path: Path) -> None: + """'Machine' (system-wide) and typos must fail early, before any PATH write.""" + for bad in ("Machine", "machine", "system", "bogus"): + out = _invoke_scope_harness(bad, tmp_path) + assert out.startswith("ERR:"), f"scope {bad!r} was not rejected: {out!r}" + assert "User" in out and "Process" in out, out + + @pytest.mark.skipif( os.name != "nt" or _powershell_executable() is None, reason="Windows PowerShell coverage runs on Windows hosts only",