fix(windows): elevate the installer when the install directory is read-only

PrivilegesRequired=lowest makes Inno Setup "always run in non administrative install mode" — the launching token is irrelevant. So a copy that ended up in C:\Program Files, which the destination page still lets an elevated wizard run pick, is registered under HKCU while living somewhere an ordinary process cannot write. UsePreviousAppDir then aims every later run straight back at that directory. WinSparkle launches the downloaded installer with plain ShellExecuteEx and no verb, so nothing along the in-app update path ever asks for elevation: the silent installer starts, cannot replace a single file, and the only way out was to quit Plezy, fetch the installer by hand and pick "Run as administrator". Inno's own PrivilegesRequiredOverridesAllowed plus UsePreviousPrivileges does not help here, because it reads the recorded install mode — which is exactly the non-administrative one that cannot write. Decide on write access instead. InitializeSetup probes the registered install directory and, when it is not writable, relaunches setup through ShellExec 'runas' pinned to that directory with /ALLUSERS, so the update lands in place instead of forking a second per-user copy. The relaunch carries a guard parameter and drops any conflicting mode override, and a refused UAC prompt now explains itself and points at the releases page rather than failing mutely. A machine-wide install that takes over a per-user directory also clears the stale uninstall entry and Start Menu group that would otherwise list Plezy twice in Apps & Features. Fresh installs are unchanged: still per-user, still no prompt. Only commandline is added to PrivilegesRequiredOverridesAllowed, since allowing dialog would make a silent install with no previous copy stop for the install-mode question — which is how winget installs. The script carried two near-identical copies of the whole .iss, one per architecture shape, so both would have needed this code. Collapse them into one template parameterised by architecture, add -EmitScriptOnly to generate the .iss without 7-Zip or Inno Setup, and guard the contract with check_windows_installer.py so the elevation path, the single-source AppId and the winget marker cannot rot. close #1705
2026-07-28 23:57:59 +02:00
parent 726dfc6507
commit 1165998dae
4 changed files with 504 additions and 146 deletions
@@ -0,0 +1,143 @@
+#!/usr/bin/env python3
+"""Guard the elevation contract in the generated Windows Inno Setup script.
+
+The installer is generated at build time by windows/build-installer.ps1, so
+there is no .iss in the tree to review. These checks pin the parts a silent
+in-app update depends on: a per-user default install that can still reach a
+machine-wide copy by relaunching itself elevated (issue #1705).
+"""
+
+from pathlib import Path
+import re
+import sys
+
+
+ROOT = Path(__file__).resolve().parents[1]
+DEFAULT_SCRIPT = ROOT / "windows/build-installer.ps1"
+if len(sys.argv) > 2:
+    raise SystemExit(f"Usage: {Path(sys.argv[0]).name} [build-installer-path]")
+SCRIPT = Path(sys.argv[1]).resolve() if len(sys.argv) == 2 else DEFAULT_SCRIPT
+APP_GUID = "4213385e-f7be-4f2b-95f9-54082a28bb8f"
+text = SCRIPT.read_text(encoding="utf-8")
+errors: list[str] = []
+
+
+def require(condition: bool, message: str) -> None:
+    if not condition:
+        errors.append(message)
+
+
+def template() -> str:
+    """The Inno Setup script emitted by New-InnoSetupScript."""
+    match = re.search(r'(?ms)^    return @"\r?\n(.*?)\r?\n"@\r?\n', text)
+    require(match is not None, "New-InnoSetupScript must return a single here-string template")
+    return match.group(1) if match else ""
+
+
+require(
+    "function New-InnoSetupScript" in text,
+    "the .iss must be built by New-InnoSetupScript so every architecture shares one template",
+)
+iss = template()
+
+# The script used to carry two near-identical copies of the whole .iss, one per
+# architecture shape. Anything that appears twice again has drifted apart.
+for once in (
+    r"^\[Setup\]$",
+    r"^\[Code\]$",
+    r"^PrivilegesRequired=",
+    r"^function InitializeSetup",
+):
+    require(
+        len(re.findall(once, text, re.MULTILINE)) == 1,
+        f"{once} must match exactly one line; a second copy of the template will drift",
+    )
+require(
+    text.count(APP_GUID) == 1,
+    "the AppId GUID must have a single source; AppId and the uninstall subkey both derive from it",
+)
+
+require("AppId={{$AppGuid}" in iss, "AppId must be built from the shared $AppGuid")
+require(
+    r"Uninstall\{$AppGuid}_is1" in iss,
+    "the uninstall subkey must be the shared AppId with Inno's _is1 suffix",
+)
+require(
+    "OutputBaseFilename=plezy-windows-installer" in iss,
+    "the release asset name is referenced by the appcast, winget and the website",
+)
+require(
+    "ArchitecturesAllowed=$ArchAllowed" in iss
+    and "ArchitecturesInstallIn64BitMode=$ArchAllowed" in iss,
+    "architectures must come from the template parameter, not be hard-coded",
+)
+require(
+    "Check: IsX64" in text and "Check: IsArm64" in text,
+    "the dual-architecture [Files] entries must keep their architecture checks",
+)
+
+# A fresh install stays per-user and prompts for nothing; only an existing
+# machine-wide copy pulls in elevation, and only via /ALLUSERS, which Inno
+# ignores unless the commandline override is allowed.
+require(
+    re.search(r"(?m)^PrivilegesRequired=lowest\s*$", iss) is not None,
+    "a fresh install must stay per-user; PrivilegesRequired=lowest",
+)
+overrides = re.search(r"(?m)^PrivilegesRequiredOverridesAllowed=(.+)$", iss)
+require(
+    overrides is not None and "commandline" in overrides.group(1),
+    "PrivilegesRequiredOverridesAllowed must allow commandline or /ALLUSERS is inert",
+)
+require(
+    overrides is None or "dialog" not in overrides.group(1),
+    "allowing dialog makes a silent install with no previous copy prompt; winget installs that way",
+)
+
+# The elevation path itself.
+require(
+    "IsAdminInstallMode" in iss,
+    "the elevation path must be skipped once Setup already runs in administrative install mode",
+)
+require(
+    "{param:ELEVATED|0}" in iss,
+    "the relaunched instance needs a guard parameter so it cannot elevate again",
+)
+require(
+    "SaveStringToFile(Probe" in iss,
+    "elevation must be driven by probing the install directory for write access",
+)
+require(
+    "ShellExec('runas'" in iss and "{srcexe}" in iss,
+    "a non-writable install directory must relaunch this installer elevated",
+)
+for parameter in ("/ALLUSERS", "/ELEVATED=1", "/DIR="):
+    require(
+        parameter in iss,
+        f"the elevated relaunch must pass {parameter}",
+    )
+require(
+    "'/CURRENTUSER'" in iss,
+    "the forwarded command line must drop /CURRENTUSER, which would undo /ALLUSERS",
+)
+require(
+    "CustomMessage('ElevationRequired')" in iss
+    and re.search(r"(?m)^ElevationRequired=\S", iss) is not None,
+    "a refused elevation must explain itself instead of failing silently",
+)
+
+# Behavior other tooling already depends on.
+require(
+    "{param:WINGET|0}" in iss and "{app}\\.winget" in iss,
+    "the winget marker file gates UpdateService.useNativeUpdater",
+)
+require(
+    "{param:NORUN|0}" in iss and "Check: not IsNoRun" in iss,
+    "the winget manifest passes /NORUN=1 and expects the launch entry to honor it",
+)
+
+if errors:
+    for error in errors:
+        print(f"ERROR: {error}", file=sys.stderr)
+    sys.exit(1)
+
+print("windows installer elevation checks passed")
@@ -23,7 +23,8 @@ for checker in \
  scripts/check_workflow_security.py \
  scripts/check_workflow_action_pins.py \
  scripts/check_container_image_pins.py \
-  scripts/check_update_packages_workflow.py; do
+  scripts/check_update_packages_workflow.py \
+  scripts/check_windows_installer.py; do
  python3 "$checker"
 done

@@ -0,0 +1,129 @@
+#!/usr/bin/env python3
+"""Behavior tests for the Windows installer elevation guard."""
+
+from pathlib import Path
+import subprocess
+import sys
+import tempfile
+import unittest
+
+
+ROOT = Path(__file__).resolve().parents[1]
+CHECKER = ROOT / "scripts/check_windows_installer.py"
+SCRIPT = ROOT / "windows/build-installer.ps1"
+
+
+class WindowsInstallerGuardTest(unittest.TestCase):
+    def _run(self, script: str) -> subprocess.CompletedProcess[str]:
+        with tempfile.TemporaryDirectory(prefix="plezy-windows-installer-test-") as directory:
+            fixture = Path(directory) / "build-installer.ps1"
+            fixture.write_text(script, encoding="utf-8")
+            return subprocess.run(
+                [sys.executable, str(CHECKER), str(fixture)],
+                cwd=ROOT,
+                check=False,
+                capture_output=True,
+                text=True,
+            )
+
+    def _script(self) -> str:
+        return SCRIPT.read_text(encoding="utf-8")
+
+    def _mutate(self, old: str, new: str) -> str:
+        script = self._script().replace(old, new, 1)
+        self.assertNotEqual(script, self._script(), f"fixture mutation no longer matches: {old!r}")
+        return script
+
+    def test_current_script_passes(self) -> None:
+        result = self._run(self._script())
+
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("elevation checks passed", result.stdout)
+
+    def test_missing_commandline_override_is_rejected(self) -> None:
+        # Without the override /ALLUSERS is silently ignored and the relaunched
+        # instance installs per-user again, which is issue #1705.
+        script = self._mutate(
+            "PrivilegesRequiredOverridesAllowed=commandline",
+            "PrivilegesRequiredOverridesAllowed=",
+        )
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("/ALLUSERS is inert", result.stderr)
+
+    def test_dialog_override_is_rejected(self) -> None:
+        script = self._mutate(
+            "PrivilegesRequiredOverridesAllowed=commandline",
+            "PrivilegesRequiredOverridesAllowed=commandline dialog",
+        )
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("winget installs that way", result.stderr)
+
+    def test_admin_default_is_rejected(self) -> None:
+        script = self._mutate("PrivilegesRequired=lowest", "PrivilegesRequired=admin")
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("must stay per-user", result.stderr)
+
+    def test_dropping_the_elevated_relaunch_is_rejected(self) -> None:
+        script = self._mutate(
+            "  if ShellExec('runas', ExpandConstant('{srcexe}'), Params, '', SW_SHOW, ewNoWait, ErrorCode) then",
+            "  if False then",
+        )
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("relaunch this installer elevated", result.stderr)
+
+    def test_dropping_the_recursion_guard_is_rejected(self) -> None:
+        script = self._mutate("/ELEVATED=1 /DIR=", "/DIR=")
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("/ELEVATED=1", result.stderr)
+
+    def test_silent_elevation_failure_is_rejected(self) -> None:
+        script = self._mutate(
+            "  SuppressibleMsgBox(FmtMessage(CustomMessage('ElevationRequired'), [PreviousDir]),\n"
+            "    mbCriticalError, MB_OK, IDOK);\n",
+            "",
+        )
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("explain itself", result.stderr)
+
+    def test_second_template_copy_is_rejected(self) -> None:
+        # The regression this guard exists for: the script used to hold one
+        # whole .iss per architecture shape, and they drifted.
+        script = self._script()
+        marker = "[Setup]\n"
+        self.assertIn(marker, script)
+        script = script.replace(marker, marker + "[Setup]\n", 1)
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("a second copy of the template will drift", result.stderr)
+
+    def test_losing_the_winget_marker_is_rejected(self) -> None:
+        script = self._mutate("{param:WINGET|0}", "{param:NOTWINGET|0}")
+
+        result = self._run(script)
+
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("winget marker file", result.stderr)
+
+
+if __name__ == "__main__":
+    unittest.main()