From c3c921f2f7a1256131033c29ae8a4733f7217e7d Mon Sep 17 00:00:00 2001 From: Parideboy Date: Tue, 18 Aug 2026 05:21:03 +0200 Subject: [PATCH] test(install/windows): verify the PATH guard against the real HKCU registry (#3068) ## Description Follow-up requested in review of #2972, on top of the merged fix for #2970 (#2985). Test-only; no production code is touched and the `HEADROOM_INSTALL_PATH_SCOPE` mechanism is unchanged. `test_powershell_installer_does_not_leak_into_user_path` currently guards the fix by comparing the entry count of `[Environment]::GetEnvironmentVariable('Path','User')` across an installer run. That infers success from the environment variable rather than verifying it, and it leaves three gaps: - The .NET getter expands `%USERPROFILE%`-style references, so it cannot observe a change of the registry value kind (`REG_EXPAND_SZ` vs `REG_SZ`) at all. - A count comparison passes when an entry is replaced or reordered rather than appended. - There is no restore path. If the guard regresses, the test reports the leak and then leaves the polluted value behind in the contributor's registry, which is precisely the damage #2970 described: the test that detects the pollution also causes it. This PR reads `HKCU\Environment` directly instead, so the assertion verifies the guard rather than assuming it. ## Type of Change - [ ] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [x] Code refactoring (no functional changes) ## Changes Made - `tests/test_install/test_native_installers.py`: new `_read_user_path_entry` helper returning the raw `HKCU\Environment` `Path` value together with its registry kind (or `None` when the value is absent), and `_restore_user_path_entry` writing that exact value and kind back. Both import `winreg` inside the function body, so the module still imports on non-Windows hosts. - `tests/test_install/test_native_installers.py`: `test_powershell_installer_does_not_leak_into_user_path` now records the raw value before the run and asserts both that the throwaway install dir is absent from the value afterwards (naming the #2970 symptom in the failure message) and that value and kind are byte-identical. The PowerShell subprocess that counted PATH entries is gone, so the test also spawns one process fewer. - `tests/test_install/test_native_installers.py`: the test now runs under `try/finally`. The `finally` cleans up the fake docker state, which this test was missing relative to its sibling `test_powershell_native_installer_supports_persistent_docker_lifecycle`, and restores the recorded registry value only when it actually changed, so a passing run performs zero registry writes and a regressed run cannot leave the contributor's PATH polluted. The scope allow-list tests added by #2985 (`_ENSURE_PATH_SCOPE_HARNESS`, `test_path_scope_accepts_process_case_insensitively`, `test_path_scope_rejects_machine_and_invalid_values`) are untouched. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ uv run pytest tests/test_install/test_native_installers.py -q platform win32 -- Python 3.13.11, pytest-9.0.3, pluggy-1.6.0 collected 5 items tests\test_install\test_native_installers.py s.... [100%] ======================== 4 passed, 1 skipped in 23.59s ======================== $ uv run ruff check . All checks passed! $ uv run ruff format --check tests/test_install/test_native_installers.py 1 file already formatted $ uv run mypy headroom --ignore-missing-imports Success: no issues found in 521 source files ``` The strengthened assertion was proven to detect a regression by temporarily neutralising the scope override in `scripts/install.ps1` (`if ($false -and $env:HEADROOM_INSTALL_PATH_SCOPE)`), so `Ensure-PathEntry` writes the `User` scope unconditionally again: ```text $ uv run pytest tests/test_install/test_native_installers.py -q -k does_not_leak_into_user_path tests\test_install\test_native_installers.py:638: in test_powershell_installer_does_not_leak_into_user_path assert str(home) not in (after[0] if after else ""), ( E AssertionError: installer leaked the throwaway install dir into the real User PATH: E C:\Users\\AppData\Local\Temp\pytest-of-\pytest-154\test_powershell_installer_does0\home ======================= 1 failed, 4 deselected in 2.72s ======================= ``` ## Real Behavior Proof - Environment: Windows 11 Pro 10.0.26200, PowerShell 7, Python 3.13.11, pytest 9.0.3, headroom at `main` (`a6ab359a`), provider Anthropic - Exact command / steps: recorded the raw `HKCU\Environment` `Path` value with `python -c "import winreg; ...QueryValueEx(k,'Path')"`, capturing its registry kind, entry count and a SHA-256 of the value; ran the full installer test file on the patched tree; re-read the registry; then neutralised the scope override in `scripts/install.ps1` as shown above, re-ran the single leak test, and re-read the registry a third time to confirm the failure path restored it. - Observed result: baseline `kind 1 entries 21 sha256 683ee646a95b8a28`. After the passing run the value was identical (`kind 1 entries 21 sha256 683ee646a95b8a28`), so a passing run writes nothing. With the override neutralised the test failed as quoted above and the registry read afterwards was again byte-identical to the recorded backup (compared as an exact `{value, kind}` match, `True`), confirming the `finally` restore. After reverting `scripts/install.ps1`, the full file is back to 4 passed, 1 skipped with the registry still unchanged. - Not tested: non-Windows hosts (the changed test is Windows-only and already skipped elsewhere; `scripts/install.sh` is untouched), elevated/admin installs, and the `Machine` scope, which `Ensure-PathEntry` rejects outright. One open question this change is positioned to catch but does not resolve: on this host the `HKCU\Environment` `Path` value is `REG_SZ` (kind `1`), not `REG_EXPAND_SZ`. A real install persists through `[Environment]::SetEnvironmentVariable(..., 'User')`, which is the API class known to rewrite that value, so it is possible that a production install silently downgrades an expandable PATH and freezes `%USERPROFILE%`-style entries. I have not verified whether headroom's installer caused it on this machine or whether the value was always `REG_SZ`, and this PR deliberately does not chase it. Happy to open a separate issue if that is worth investigating. ## Runtime Rollout Safety - Rollout-managed feature(s): none (test-only change) - Minimum rollout channel: n/a - Stable/default behavior changed: no; no production code path is modified - Kill switch / disable path: n/a - Unsafe override required: no - Qualification impact: none - Rollback path: revert this commit; the test returns to the entry-count comparison ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review Co-authored-by: Claude Opus 5 --- tests/test_install/test_native_installers.py | 86 +++++++++++++++----- 1 file changed, 64 insertions(+), 22 deletions(-) diff --git a/tests/test_install/test_native_installers.py b/tests/test_install/test_native_installers.py index 37bcaac3e..fdf61eb43 100644 --- a/tests/test_install/test_native_installers.py +++ b/tests/test_install/test_native_installers.py @@ -560,6 +560,39 @@ def _powershell_executable() -> str | None: return shutil.which("pwsh") or shutil.which("powershell") or shutil.which("powershell.exe") +def _read_user_path_entry() -> tuple[str, int] | None: + """Read the raw ``HKCU\\Environment`` PATH value and its registry kind, if it exists. + + Reading the registry directly rather than through + ``[Environment]::GetEnvironmentVariable('Path','User')`` keeps two things visible that the + .NET getter hides: the unexpanded value (the getter expands ``%USERPROFILE%``-style + references) and the value kind, so a ``REG_EXPAND_SZ`` -> ``REG_SZ`` downgrade cannot pass + unnoticed. + """ + import winreg + + with winreg.OpenKey(winreg.HKEY_CURRENT_USER, "Environment") as key: + try: + value, kind = winreg.QueryValueEx(key, "Path") + except FileNotFoundError: + return None + return str(value), int(kind) + + +def _restore_user_path_entry(previous: tuple[str, int] | None) -> None: + import winreg + + with winreg.OpenKey(winreg.HKEY_CURRENT_USER, "Environment", 0, winreg.KEY_SET_VALUE) as key: + if previous is None: + try: + winreg.DeleteValue(key, "Path") + except FileNotFoundError: + pass + return + value, kind = previous + winreg.SetValueEx(key, "Path", 0, kind, value) + + @pytest.mark.skipif( os.name != "nt" or _powershell_executable() is None, reason="Windows PowerShell coverage runs on Windows hosts only", @@ -572,37 +605,46 @@ def test_powershell_installer_does_not_leak_into_user_path(tmp_path: Path) -> No 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. + + The assertion reads ``HKCU\\Environment`` itself instead of counting the entries the .NET + getter reports, so it verifies the guard rather than trusting the environment variable to + have taken effect: it catches a count-preserving mutation and a change of the value kind, + neither of which an entry count can see. """ 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}" + before = _read_user_path_entry() + try: + _run( + [ + powershell, + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-File", + str(REPO_ROOT / "scripts" / "install.ps1"), + ], + env=env, + cwd=REPO_ROOT, + ) + + after = _read_user_path_entry() + assert str(home) not in (after[0] if after else ""), ( + f"installer leaked the throwaway install dir into the real User PATH: {home}" + ) + assert after == before, "installer mutated the real User PATH" + finally: + _cleanup_fake_docker(env) + # A passing run never writes to the registry; this only fires if the guard regresses, so + # that a failing test cannot leave the developer's PATH polluted. + if _read_user_path_entry() != before: + _restore_user_path_entry(before) # AST-extract Ensure-PathEntry from install.ps1 and invoke it in isolation under