From f557e998d90caedf31af4730b41017b5858c4092 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:40:54 -0400 Subject: [PATCH 1/9] Find, verify and apply a newer release from the command line The installer existed with nothing to drive it. This adds the client the release plan's updater boundary described, meeting the conditions it set: signature, publisher and embedded version all verified before elevation, and the /D= target computed from the running executable rather than read from the uninstall registry key, which anything running as the user can write. GitHub Releases is the feed, so there is no manifest server to run and no second place a version is recorded. A tag that is not a version this project publishes is refused rather than string-compared, and the asset has to be named for the version the tag claims, so a file attached beside the real installer cannot be served in its place. An updater executes code that arrived over the network, with elevation, so what it refuses matters more than how it installs. The host allowlist is applied after redirects, because that is where the host can change. The size ceiling holds against Content-Length and again while the bytes arrive, since a hostile server owes us no honest header. The digest is taken while streaming and compared again before launch, closing the gap in which anything able to write to the download directory could swap the file. Authenticode must report Valid, with this project's thumbprint and publisher name, and the installer's own embedded FileVersion must match the release: without that last check, re-uploading an older but still validly signed installer under a newer asset name would roll every install back onto a build whose faults are already fixed. Two things the reference implementation does are deliberately not carried over. It terminates the running application before installing, which would contradict the promise that maintenance never force-kills a copy mid-transfer; this verifies and hands over instead, and says so before the elevation prompt so the installer's refusal does not read as a broken update. And it rejects any tag that is not X.Y.Z, which would make the updater blind to Offloader's own first packaged release. The grammar here is the one versioning.py already enforces, with a test asserting both orderings agree, because a disagreement would let an update install a build Windows then considers older. The in-app check, notification and progress surface stay deferred. --- CHANGELOG.md | 19 ++ README.md | 1 + docs/release-plan.md | 27 ++- docs/updates.md | 108 +++++++++ src/offloader/cli.py | 72 +++++- src/offloader/update.py | 383 ++++++++++++++++++++++++++++++++ tests/test_update.py | 477 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 1076 insertions(+), 11 deletions(-) create mode 100644 docs/updates.md create mode 100644 src/offloader/update.py create mode 100644 tests/test_update.py diff --git a/CHANGELOG.md b/CHANGELOG.md index dfa183d..ff8e5d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,25 @@ project uses [semantic versioning][semver]. ### Added +- **`offloader update` finds, verifies and applies a newer release.** GitHub + Releases is the feed, so there is no manifest server and no second place a + version is written down. Before anything runs: HTTPS with a host allowlist + re-checked after redirects, a size ceiling held against `Content-Length` and + again mid-stream, a SHA-256 taken while streaming and compared before + launch, and an Authenticode check that requires a valid status, this + project's certificate thumbprint, its publisher name, and an embedded + `FileVersion` matching the release. That last check is what stops a rollback: + re-serving an older, still validly signed installer under a newer asset name + would otherwise move every install back onto a build whose faults are fixed. + The install target is computed from the running executable rather than read + from the uninstall registry key, which anything running as the user can + write. Prereleases are ordered rather than rejected, matching the grammar + `build/windows/versioning.py` already enforces, and a test asserts the two + orderings agree. The updater never closes a running copy: the installer's + refusal while a transfer is in flight is the guarantee, so the command says + so before the elevation prompt appears. See + [`docs/updates.md`](docs/updates.md); the in-app check is still deferred. + - Windows desktop/CLI bundles and an NSIS installer, with pinned dependencies, embedded version metadata, signing by default, explicit unsigned CI builds, source/file inventories, checksums, and headless artifact checks. Installation diff --git a/README.md b/README.md index 3197da7..ac1646d 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,7 @@ offloader verify D:\video\080426\A001 | `report` | regenerate paperwork for an existing tree, copying nothing | | `info` | show tool and environment status | | `gui` | launch the desktop app (also `offloader-gui`) | +| `update` | check for a newer release, and install it (Windows) | ### `offload` and `report` diff --git a/docs/release-plan.md b/docs/release-plan.md index ad02213..346c5a2 100644 --- a/docs/release-plan.md +++ b/docs/release-plan.md @@ -57,7 +57,7 @@ tests or builds were run for this documentation task. | Dependencies | Minimum versions and optional extras | Recorded build environment and pinned release dependency sets | | Media tools | ffmpeg/ffprobe discovered externally; copying works without them | Explicit installer dependency policy and useful missing-tool messaging | | Integrity | Detailed guarantees and remaining limits in `data-safety.md` | Release-specific regression evidence and operational validation | -| Updates | No updater found | Manual updates for the beta; documented safe upgrade and recovery | +| Updates | `offloader update` checks GitHub Releases, verifies the signed installer and hands it over; no in-app check or notification | Wire the check into the desktop interface; qualify an end-to-end update against a published release | Sources: [`pyproject.toml`](../pyproject.toml), [`CI`](../.github/workflows/ci.yml), [`README`](../README.md), @@ -107,8 +107,10 @@ credentials, update endpoints, or installation paths. - **Timeline support:** include and test OpenTimelineIO and the currently declared adapter in the desktop bundle if timeline import is advertised for that bundle. Otherwise mark that capability source-only for the beta. -- **Updates:** manual installation initially. Refuse replacement while the app - or CLI has an active job; never force-kill a copy to install an update. +- **Updates:** `offloader update` finds and verifies a release and runs the + signed installer; the in-app check remains deferred. Refuse replacement + while the app or CLI has an active job; never force-kill a copy to install + an update. - **Scope freeze:** defer new media features, cloud services, notifications, auto-update, and a marketing website. Fix integrity and packaging blockers discovered during qualification. @@ -194,13 +196,18 @@ user data. Remove only inventoried application files, never recursively erase an arbitrary install directory that might contain user material. **Updater boundary:** installer parity includes the silent-install contract, -user-context relaunch, and safe settings preservation. An in-app download/ -update client is still a separate deferred feature. Future callers must verify -the installer signature, publisher, and version before elevation. Preserve -NSIS's `/D=` contract: last argument, unquoted even when the path has spaces, -and computed from a trusted installation target rather than an unvalidated -registry command. A generic unattended deployment must not launch an app in -a missing or unrelated user's session. +user-context relaunch, and safe settings preservation. A generic unattended +deployment must not launch an app in a missing or unrelated user's session. + +The command-line half of the update client is now implemented in +`src/offloader/update.py` and documented in [updates.md](updates.md). It meets +the conditions this section set: the signature, publisher and embedded version +are all verified before elevation, and the `/D=` target is computed from the +running executable rather than from the uninstall registry key. It does not +close a running instance, so an update requires the operator to finish first +and the installer's refusal remains the guarantee. What is still deferred is +the in-app part: an automatic check, a notification, and a progress surface in +the desktop interface. **Acceptance gates:** exercise first install, custom path, same-version reinstall, upgrade, failed upgrade recovery, silent install, and uninstall on diff --git a/docs/updates.md b/docs/updates.md new file mode 100644 index 0000000..ac0aeff --- /dev/null +++ b/docs/updates.md @@ -0,0 +1,108 @@ +# Updating an installed copy + +`offloader update` asks GitHub whether a newer release exists, and +`offloader update --install` downloads it, proves it came from this project, +and hands it to the installer. + +An updater runs code that arrived over the network, with elevation, on someone +else's machine. So the design question is not how it installs but what it +refuses, and every check below fails closed: an unreadable version, a +mismatched digest, a signature from another certificate all mean "no update", +never "install it anyway". + +## The feed + +GitHub Releases, read through the REST API. There is no manifest server to run +or keep honest, and no second place a version number is written down. + +``` +https://api.github.com/repos/owenpkent/offloader/releases/latest +``` + +That URL is compiled into every shipped build, so moving it orphans every +install that already exists. Renaming or transferring the repository is a +breaking change for installed copies, not an administrative detail. + +Three fields are read. `tag_name` must be a version this project publishes +(`0.2.0`, `0.1.0b1`, with or without a leading `v`); anything else, including +`latest` or `1.0.3-evil`, is refused rather than compared as a string. +`assets[]` must contain an entry named exactly +`Offloader-Setup-{version}.exe` for the version the tag claims, so a file +attached beside the real installer cannot be served in its place. `body` +becomes the release notes. + +## Version ordering + +The grammar is the one `build/windows/versioning.py` already enforces, which +means prereleases are ordered rather than rejected: `0.1.0a2` < `0.1.0b1` < +`0.1.0rc1` < `0.1.0`. A late alpha never outranks a first beta, matching the +disjoint numeric ranges that module reserves when it writes the installer's +Windows version fields. `tests/test_update.py` asserts both implementations +agree on a list of versions, because if they diverge an update can install a +build that Windows then considers older than the one it replaced. + +If either version is unreadable the answer is "not newer". String comparison +is what makes `1.0.10` look older than `1.0.9`. + +## What is checked before anything runs + +| Check | What it stops | +| --- | --- | +| HTTPS, and a host allowlist applied **after** redirects | GitHub redirects asset downloads to another domain, so the host that actually serves the bytes is the one that matters | +| A size ceiling, enforced against `Content-Length` and again while streaming | A hostile or confused feed filling the disk; a truthful `Content-Length` is not something a server owes us | +| SHA-256 taken while streaming, compared again before launch | The gap between writing the file and executing it, in which anything able to write to the download directory could swap it | +| Authenticode status is `Valid` | An unsigned or tampered installer | +| The signing certificate's thumbprint matches | A valid signature from somebody else. Anyone can obtain one; the question is whether this is the certificate the release was built with | +| The publisher name matches | The same, read the way the UAC prompt will read it | +| The installer's **embedded** `FileVersion` matches the release version | A rollback. Someone able to re-upload an asset could otherwise re-serve an older, still validly signed installer under a newer name, moving every install back onto a build whose faults are already fixed | + +The certificate values are the ones in `build/windows/sign.py`, repeated in +`src/offloader/update.py` rather than imported, because the build scripts are +not part of the shipped package. + +## Applying it + +The verified installer is run through `ShellExecuteW` with the `runas` verb, +silently: `/S /D=`. A plain process spawn does not honour the +installer's manifest request for elevation and fails instead of prompting. + +Two details are easy to break silently. `/D=` must be the **last** argument +and must **not** be quoted even when the path contains spaces: quoting it +installs into a directory whose name contains a quote, and anything placed +after it is swallowed into the path. And the target is computed from the +running executable, never read from the uninstall registry key, which is +writable by anything running as the user and would otherwise let a planted +value redirect an elevated silent install. + +## What it deliberately does not do + +**It does not close anything.** Offloader's installer refuses maintenance +while a transfer is in flight and exits non-zero without changing the +installation; it never force-kills a running copy. That refusal is a promise +in [data-safety.md](data-safety.md), so the updater's job is to verify and +hand over, not to clear the way. `offloader update --install` says so before +the elevation prompt appears, because an operator who does not know it reads +the installer's refusal as a broken update. + +The consequence is that updating requires closing the app and any CLI +transfer first. That is the intended trade: a packaged updater that could +interrupt a card offload would be a worse tool than one that asks. + +**There is no rollback.** A failed install leaves the previous version in +place where the installer's own transactional recovery manages it (see +`installation.py`); the updater does not attempt a second recovery mechanism +on top. Nothing is deleted by the updater itself. + +**There is no automatic check yet.** `offloader update` is explicit. An in-app +check, a notification and a timer belong with the desktop interface and are +not implemented here; see [release-plan.md](release-plan.md). + +## Reference + +Modelled on Alpha-OSK's `src/updater.py`, which solves the same problem for a +different app. Two of its decisions are deliberately not carried over: it +terminates the running application before installing, and it rejects any tag +that is not `X.Y.Z`. The first conflicts with the guarantee above; the second +would make this updater blind to Offloader's own first packaged release, which +is planned as a beta. Nothing else of its configuration, endpoints or product +identifiers is reused. diff --git a/src/offloader/cli.py b/src/offloader/cli.py index 440a875..cc72589 100644 --- a/src/offloader/cli.py +++ b/src/offloader/cli.py @@ -311,6 +311,15 @@ def build_parser() -> argparse.ArgumentParser: sub.add_parser("info", help="show tool and environment status") sub.add_parser("gui", help="launch the desktop interface") + + update = sub.add_parser( + "update", help="check for a newer release, and install it (Windows)") + update.add_argument("--install", action="store_true", + help="download, verify and run the installer rather " + "than only reporting what is available") + update.add_argument("--dir", type=Path, default=None, metavar="PATH", + help="where to download the installer (default: a " + "private temporary directory)") return parser @@ -598,11 +607,72 @@ def cmd_gui(_args: argparse.Namespace) -> int: return gui_main([sys.argv[0]]) +def cmd_update(args: argparse.Namespace) -> int: + """Report or apply a newer release. + + Exit status: 0 when up to date or an install was started, 1 when an update + exists but was not applied, 2 on a usage or verification failure. A caller + scripting this can therefore tell "nothing to do" from "something to do". + """ + import tempfile + + from . import update as update_mod + + release = update_mod.check() + if release is None: + print(f"Offloader {__version__} is the newest release available.") + return 0 + + print(f"Offloader {release.version} is available " + f"(installed: {__version__}).") + if not args.install: + print(" offloader update --install download, verify and install") + return 1 + + if sys.platform != "win32": + print("error: only the Windows release can be installed this way", + file=sys.stderr) + return 2 + + directory = args.dir + if directory is None: + directory = Path(tempfile.mkdtemp(prefix="offloader-update-")) + directory.mkdir(parents=True, exist_ok=True) + + def report(done: int, total: int) -> None: + if total: + sys.stderr.write(f"\r downloading {done * 100 // total:3d}%") + else: + sys.stderr.write(f"\r downloading {format_size(done)}") + sys.stderr.flush() + + try: + installer, digest = update_mod.download(release, directory, + progress=report) + sys.stderr.write("\r" + " " * 32 + "\r") + update_mod.verify(installer, release, + expected_digest=digest, actual_digest=digest) + print(f" verified {installer.name}") + print(f" sha256 {digest}") + # Said before the prompt appears, because the installer refuses rather + # than closing anything: an operator who does not know that reads the + # refusal as a broken update. + print("\nThe installer will not replace a running Offloader. Close " + "the app and any\nCLI transfer first; an active job makes it " + "exit without changing anything.") + update_mod.apply(installer) + except update_mod.UpdateError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 + return 0 + + def main(argv: list[str] | None = None) -> int: args = build_parser().parse_args(argv) handlers = {"offload": cmd_offload, "report": cmd_report, "verify": cmd_verify, "info": cmd_info, "gui": cmd_gui, - "resolve": cmd_resolve, "control": cmd_control} + "resolve": cmd_resolve, "control": cmd_control, + "update": cmd_update} try: return handlers[args.command](args) except KeyboardInterrupt: diff --git a/src/offloader/update.py b/src/offloader/update.py new file mode 100644 index 0000000..74f1659 --- /dev/null +++ b/src/offloader/update.py @@ -0,0 +1,383 @@ +"""Finding, verifying and applying a newer Windows release. + +The shape follows Alpha-OSK's `src/updater.py`: GitHub Releases is the feed, so +there is no manifest server to run or keep honest; the downloaded installer is +checked against the same code-signing certificate that produced it; and nothing +is executed until every check has passed. + +Two of its decisions are deliberately **not** copied, because Offloader's +guarantees differ: + +* **Nothing is force-closed.** Alpha-OSK's installer terminates the running app + and a helper process restarts it afterwards. Offloader's installer refuses + maintenance while a copy is in flight and never force-kills — that is a + data-safety promise in `docs/data-safety.md`, not an implementation detail. + So this module verifies and hands over; it never tries to clear the way. +* **Prereleases are not rejected.** Alpha-OSK's updater only accepts `X.Y.Z` + tags. Offloader's first packaged release is planned as `0.1.0b1`, so the same + rule would make the updater blind to the very build it is shipped in. The + grammar here is the one `build/windows/versioning.py` already enforces, and + the ordering agrees with the Windows version fields that installer writes. + +Everything fails closed. A feed that cannot be parsed, a version that cannot be +read, a digest that does not match, a signature from the wrong certificate: all +of them mean "no update", never "install it anyway". +""" + +from __future__ import annotations + +import hashlib +import json +import os +import re +import subprocess +import sys +import urllib.error +import urllib.parse +import urllib.request +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from ._version import __version__ + +#: Where releases are published. Pinned deliberately: this URL is compiled into +#: every shipped build, so moving it orphans every install that already exists. +#: Treat a rename or transfer of the repository as a breaking change. +FEED_URL = "https://api.github.com/repos/owenpkent/offloader/releases/latest" + +#: The published installer's name. Part of the release contract — the asset has +#: to be identifiable without trusting anything else in the release. +ASSET_TEMPLATE = "Offloader-Setup-{version}.exe" + +#: Hosts a download may come from, checked before *and* after redirects. GitHub +#: serves release assets off a separate domain and changes which one, so the +#: post-redirect check is the one that matters. +ALLOWED_HOSTS = frozenset({ + "api.github.com", + "github.com", + "objects.githubusercontent.com", + "release-assets.githubusercontent.com", +}) + +#: A ceiling, not an estimate. Bounds what a compromised or confused feed can +#: make this write to disk. +MAX_DOWNLOAD_BYTES = 500 * 1024 * 1024 + +#: The certificate the installer is signed with, and the name it must present. +#: The same values as `build/windows/sign.py`, repeated rather than imported +#: because that module is a build script and is not shipped. +EXPECTED_CERT_THUMBPRINT = "FC22B5221318F3F3F6B3EB2D969D7F99091557BF" +EXPECTED_SIGNER_NAME = "OK Studio Inc." + +#: How long a signature or version probe may take before it is a failure. +PROBE_TIMEOUT = 60 + +_VERSION_RE = re.compile(r"^(\d+)\.(\d+)\.(\d+)(?:(a|b|rc)(\d+))?$") +_TAG_RE = re.compile(r"^v?(.+)$") + +#: Alpha before beta before release candidate before the final release. Mirrors +#: the disjoint ranges `versioning.windows_version` reserves, so a late alpha +#: never sorts above the first beta here either. +_STAGE_ORDER = {"a": 0, "b": 1, "rc": 2, None: 3} + + +class UpdateError(RuntimeError): + """An update was found but could not be trusted or applied.""" + + +@dataclass(frozen=True) +class Release: + """A candidate release, as far as the feed describes it.""" + + version: str + asset_name: str + download_url: str + notes: str = "" + + +def parse_version(value: str) -> tuple[int, int, int, int, int] | None: + """`0.2.0b3` as an orderable tuple, or None if it is not a version. + + None rather than an exception: an unreadable version is a reason to decline + an update, and every caller here treats it that way. + """ + match = _VERSION_RE.match(value.strip()) + if match is None: + return None + major, minor, patch = (int(part) for part in match.group(1, 2, 3)) + stage = match.group(4) + return (major, minor, patch, _STAGE_ORDER[stage], int(match.group(5) or 0)) + + +def is_newer(candidate: str, installed: str) -> bool: + """Whether `candidate` should replace `installed`. + + Fails closed. If either side is unreadable the answer is no, because the + alternative is deciding an upgrade path from a string nobody can order. + """ + left, right = parse_version(candidate), parse_version(installed) + if left is None or right is None: + return False + return left > right + + +def _assert_allowed(url: str) -> None: + parsed = urllib.parse.urlparse(url) + if parsed.scheme != "https": + raise UpdateError(f"refusing a non-HTTPS download: {parsed.scheme}://") + if parsed.hostname not in ALLOWED_HOSTS: + raise UpdateError(f"refusing a download from {parsed.hostname!r}") + + +def _fetch_json(url: str, *, timeout: int = 30) -> Any: + _assert_allowed(url) + request = urllib.request.Request( + url, + headers={"Accept": "application/vnd.github+json", + "User-Agent": f"Offloader/{__version__}"}, + ) + with urllib.request.urlopen(request, timeout=timeout) as response: # noqa: S310 + _assert_allowed(response.geturl()) + return json.loads(response.read().decode("utf-8")) + + +def release_from_feed(payload: Any, *, installed: str = __version__) -> Release | None: + """The newer release the feed describes, or None. + + Reads only what it needs, and requires the asset to be named for the + version the tag claims: an extra or renamed file in a release cannot then + be mistaken for the installer. + """ + if not isinstance(payload, dict): + return None + tag = payload.get("tag_name") + if not isinstance(tag, str): + return None + tag_match = _TAG_RE.match(tag.strip()) + if tag_match is None: + return None + version = tag_match.group(1) + if parse_version(version) is None or not is_newer(version, installed): + return None + + expected = ASSET_TEMPLATE.format(version=version) + assets = payload.get("assets") + if not isinstance(assets, list): + return None + for asset in assets: + if not isinstance(asset, dict) or asset.get("name") != expected: + continue + url = asset.get("browser_download_url") + if not isinstance(url, str): + return None + notes = payload.get("body") + return Release(version=version, asset_name=expected, download_url=url, + notes=notes if isinstance(notes, str) else "") + return None + + +def check(installed: str = __version__, *, url: str = FEED_URL, + fetch: Callable[[str], Any] = _fetch_json) -> Release | None: + """Ask the feed for a newer release. Never raises. + + Called on a timer and from a menu item, where the cost of an exception is + an interrupted app and the cost of returning None is one missed check. + """ + try: + return release_from_feed(fetch(url), installed=installed) + except Exception: + return None + + +def download(release: Release, directory: Path, *, + opener: Callable[..., Any] = urllib.request.urlopen, + progress: Callable[[int, int], None] | None = None) -> tuple[Path, str]: + """Stream the installer into `directory`. Returns its path and SHA-256. + + Hashed while streaming rather than re-read afterwards, so the digest + describes the bytes that were actually written. + """ + _assert_allowed(release.download_url) + target = Path(directory) / release.asset_name + digest = hashlib.sha256() + written = 0 + + request = urllib.request.Request( + release.download_url, + headers={"User-Agent": f"Offloader/{__version__}"}, + ) + try: + with opener(request, timeout=60) as response: + # The redirect is where the host can change, so this is the check + # that counts; the pre-flight above only rejects an obvious feed. + _assert_allowed(response.geturl()) + declared = response.headers.get("Content-Length") + total = int(declared) if declared and declared.isdigit() else 0 + if total > MAX_DOWNLOAD_BYTES: + raise UpdateError( + f"refusing a {total} byte download; the ceiling is " + f"{MAX_DOWNLOAD_BYTES}") + with open(target, "wb") as handle: + while True: + block = response.read(1 << 20) + if not block: + break + written += len(block) + if written > MAX_DOWNLOAD_BYTES: + raise UpdateError( + "download exceeded the size ceiling mid-stream") + digest.update(block) + handle.write(block) + if progress is not None: + progress(written, total) + except urllib.error.URLError as exc: + raise UpdateError(f"could not download the installer: {exc}") from exc + + if written == 0: + raise UpdateError("the installer download was empty") + return target, digest.hexdigest() + + +def _powershell(script: str, *, + run: Callable[..., subprocess.CompletedProcess[str]] | None = None + ) -> dict[str, Any]: + runner = run or subprocess.run + result = runner( + ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], + capture_output=True, text=True, timeout=PROBE_TIMEOUT, check=False, + ) + if result.returncode != 0: + raise UpdateError("could not read the installer's signature") + try: + data = json.loads(result.stdout or "{}") + except json.JSONDecodeError as exc: + raise UpdateError("unreadable signature output") from exc + return data if isinstance(data, dict) else {} + + +def inspect_installer(path: Path, *, run: Callable[..., Any] | None = None + ) -> dict[str, Any]: + """The downloaded file's Authenticode status and embedded version.""" + literal = str(Path(path)) + script = ( + "$ErrorActionPreference='Stop';" + f"$p='{literal}';" + "$s=Get-AuthenticodeSignature -LiteralPath $p;" + "$v=(Get-Item -LiteralPath $p).VersionInfo;" + "[pscustomobject]@{" + "Status=[string]$s.Status;" + "Thumbprint=[string]$s.SignerCertificate.Thumbprint;" + "Subject=[string]$s.SignerCertificate.Subject;" + "TimestampThumbprint=[string]$s.TimeStamperCertificate.Thumbprint;" + "FileVersion=[string]$v.FileVersion;" + "ProductVersion=[string]$v.ProductVersion" + "} | ConvertTo-Json -Compress" + ) + return _powershell(script, run=run) + + +def _subject_name(subject: str) -> str: + for part in subject.split(","): + key, _, value = part.strip().partition("=") + if key.upper() == "CN": + return value.strip().strip('"') + return "" + + +def verify(path: Path, release: Release, *, + run: Callable[..., Any] | None = None, + expected_digest: str | None = None, + actual_digest: str | None = None) -> dict[str, Any]: + """Refuse the download unless everything about it agrees. + + Four separate questions, because each one fails on its own: + + * the bytes are the ones that were hashed while streaming — closing the + window between writing the file and reading it back; + * Windows trusts the signature; + * it is *our* certificate and our name, not merely a valid one; + * the version compiled into the executable is the version the release + claimed. Without this last check, anyone able to re-upload an asset could + re-serve an older, still validly signed installer under a newer name and + roll every install back onto a build whose bugs are already fixed. + """ + if expected_digest is not None and actual_digest is not None: + if expected_digest.lower() != actual_digest.lower(): + raise UpdateError("the installer changed after it was downloaded") + + record = inspect_installer(path, run=run) + status = str(record.get("Status", "")) + if status != "Valid": + raise UpdateError(f"the installer's signature is {status or 'missing'}") + + thumbprint = str(record.get("Thumbprint", "")).replace(" ", "").upper() + if thumbprint != EXPECTED_CERT_THUMBPRINT: + raise UpdateError("the installer was signed by a different certificate") + + signer = _subject_name(str(record.get("Subject", ""))) + if signer != EXPECTED_SIGNER_NAME: + raise UpdateError(f"the installer's publisher is {signer!r}") + + embedded = str(record.get("FileVersion", "")).strip() + if parse_version(release.version) != parse_version(embedded.split("+")[0]): + raise UpdateError( + f"the installer reports version {embedded!r} but the release " + f"claims {release.version!r}") + return record + + +def install_target() -> Path: + """Where an update should be installed. + + Computed from the running executable, never read from the registry. The + registry value is writable by anything running as the user, so trusting it + would let a planted key redirect an elevated silent install into a + directory of someone else's choosing. + """ + if getattr(sys, "frozen", False): + return Path(sys.executable).resolve().parent + base = os.environ.get("ProgramFiles") or r"C:\Program Files" + return Path(base) / "Offloader" + + +def install_command(installer: Path, target: Path) -> tuple[str, str]: + """The installer path and the argument string to elevate it with. + + `/D=` has two rules NSIS enforces silently: it must be the last argument, + and it must not be quoted even when the path contains spaces. Quoting it + installs into a directory with a quote in its name; putting anything after + it is read as part of the path. + """ + return str(installer), f"/S /D={target}" + + +def apply(installer: Path, target: Path | None = None, *, + shell_execute: Callable[..., int] | None = None) -> None: + """Run the verified installer elevated, silently. + + `ShellExecuteW` with `runas` rather than `subprocess`: the installer's + manifest asks for elevation, and only the shell honours that with a UAC + prompt — a plain spawn fails outright. + + This does not close the running app, and must not. Offloader's installer + refuses maintenance while a transfer is in flight and exits non-zero + instead; that refusal is the guarantee, so the caller's job is to ask the + operator to finish, not to clear the way for it. + """ + destination = install_target() if target is None else Path(target) + path, arguments = install_command(Path(installer), destination) + + if shell_execute is None: # pragma: no cover - Windows + import ctypes + + shell_execute = ctypes.windll.shell32.ShellExecuteW + + result = shell_execute(None, "runas", path, arguments, None, 1) + # ShellExecuteW returns a fake HINSTANCE; anything of 32 or less is an + # error code, and 5 is the one that matters — the UAC prompt was declined. + if int(result) <= 32: + if int(result) == 5: + raise UpdateError("the update needs administrator approval") + raise UpdateError(f"could not start the installer (code {int(result)})") diff --git a/tests/test_update.py b/tests/test_update.py new file mode 100644 index 0000000..e2828a3 --- /dev/null +++ b/tests/test_update.py @@ -0,0 +1,477 @@ +"""Finding, verifying and applying a newer release. + +Every test here is about refusing something. An updater is a mechanism for +running code that arrived over the network on the operator's machine, with +elevation, so the interesting behaviour is not the happy path but each way the +happy path is declined: an unreadable version, an asset that is not the one the +release claims, a host the redirect moved to, a signature from somebody else, +and an installer whose embedded version does not match the release it came +from. +""" + +from __future__ import annotations + +import importlib.util +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +from offloader import update +from offloader.update import Release, UpdateError + +REPO = Path(__file__).resolve().parent.parent +GOOD_DIGEST = "a" * 64 + + +def _versioning(): + """`build/windows/versioning.py`, which is a build script, not a module.""" + path = REPO / "build" / "windows" / "versioning.py" + spec = importlib.util.spec_from_file_location("_versioning", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _feed(tag: str, *, assets: list[dict] | None = None, body: str = "") -> dict: + version = tag.lstrip("v") + if assets is None: + assets = [{ + "name": f"Offloader-Setup-{version}.exe", + "browser_download_url": + f"https://github.com/owenpkent/offloader/releases/download/" + f"{tag}/Offloader-Setup-{version}.exe", + }] + return {"tag_name": tag, "assets": assets, "body": body} + + +class _Response: + """The parts of an HTTP response the download path touches.""" + + def __init__(self, body: bytes, *, url: str, length: str | None = None): + self._body = body + self._at = 0 + self._url = url + self.headers = {} if length is None else {"Content-Length": length} + + def read(self, size: int = -1) -> bytes: + if size is None or size < 0: + size = len(self._body) - self._at + block = self._body[self._at:self._at + size] + self._at += len(block) + return block + + def geturl(self) -> str: + return self._url + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + +def _opener(body: bytes, *, url: str, length: str | None = None): + def open_it(_request, timeout=None): + return _Response(body, url=url, length=length) + return open_it + + +def _signature(**overrides) -> dict: + record = { + "Status": "Valid", + "Thumbprint": update.EXPECTED_CERT_THUMBPRINT, + "Subject": f"CN={update.EXPECTED_SIGNER_NAME}, O=OK Studio Inc., C=US", + "TimestampThumbprint": "B" * 40, + "FileVersion": "0.2.0", + "ProductVersion": "0.2.0", + } + record.update(overrides) + return record + + +def _runner(record: dict, *, returncode: int = 0): + def run(_command, **_kwargs): + return subprocess.CompletedProcess( + _command, returncode, stdout=json.dumps(record), stderr="") + return run + + +# ------------------------------------------------------------------- versions + + +@pytest.mark.parametrize("value", [ + "0.1.0", "1.2.3", "10.0.0", "0.1.0a1", "0.1.0b2", "0.1.0rc9", +]) +def test_release_versions_parse(value: str): + assert update.parse_version(value) is not None + + +@pytest.mark.parametrize("value", [ + "", "1.2", "1.2.3.4", "v1.2.3", "1.2.3-beta", "1.2.3dev1", "latest", + "1.2.3+local", "one.two.three", +]) +def test_anything_else_does_not_parse(value: str): + """Fails closed. A version nobody can order is not a version to upgrade + to, and string comparison on these is how `1.0.10` ends up older than + `1.0.9`.""" + assert update.parse_version(value) is None + + +def test_prereleases_are_ordered_before_the_release(): + """The deliberate difference from the reference implementation, which + rejects any tag that is not X.Y.Z. Offloader's first packaged release is + planned as a beta, so that rule would make this updater blind to it.""" + assert update.is_newer("0.1.0", "0.1.0rc1") + assert update.is_newer("0.1.0rc1", "0.1.0b1") + assert update.is_newer("0.1.0b1", "0.1.0a1") + assert not update.is_newer("0.1.0a9", "0.1.0b1") + + +def test_a_late_alpha_does_not_outrank_a_first_beta(): + """The invariant `versioning.py` reserves disjoint numeric ranges for. It + has to hold in both places or the updater and the installer disagree about + which build is newer.""" + assert not update.is_newer("0.1.0a999", "0.1.0b1") + + +def test_the_ordering_agrees_with_the_windows_version_fields(): + """Two implementations of the same ordering: this module compares tuples, + and the installer writes four 16-bit fields. If they ever disagree, an + update can install a build that Windows then considers older than the one + it replaced.""" + windows_version = _versioning().windows_version + ordered = ["0.1.0a1", "0.1.0a2", "0.1.0b1", "0.1.0rc1", "0.1.0", + "0.1.1a1", "0.1.1", "0.2.0", "1.0.0"] + for lower, higher in zip(ordered, ordered[1:], strict=False): + assert update.is_newer(higher, lower), f"{higher} !> {lower}" + assert windows_version(higher) > windows_version(lower), \ + f"windows fields disagree: {higher} !> {lower}" + + +def test_an_unreadable_version_on_either_side_declines(): + assert not update.is_newer("nonsense", "0.1.0") + assert not update.is_newer("0.2.0", "nonsense") + + +# ---------------------------------------------------------------------- feed + + +def test_a_newer_release_is_reported(): + release = update.release_from_feed(_feed("v0.2.0"), installed="0.1.0") + assert release is not None + assert release.version == "0.2.0" + assert release.asset_name == "Offloader-Setup-0.2.0.exe" + + +def test_a_tag_without_the_v_prefix_is_accepted(): + release = update.release_from_feed(_feed("0.2.0"), installed="0.1.0") + assert release is not None and release.version == "0.2.0" + + +def test_a_prerelease_tag_is_accepted(): + release = update.release_from_feed(_feed("v0.1.0b1"), installed="0.1.0a1") + assert release is not None and release.version == "0.1.0b1" + + +@pytest.mark.parametrize("tag", ["v0.1.0", "v0.0.9"]) +def test_the_same_or_an_older_release_is_not_an_update(tag: str): + assert update.release_from_feed(_feed(tag), installed="0.1.0") is None + + +def test_a_tag_that_is_not_a_version_is_refused(): + """`latest` or `v1.0.3-evil` must not be string-compared into an upgrade.""" + assert update.release_from_feed(_feed("v1.0.3-evil"), + installed="0.1.0") is None + + +def test_an_asset_named_for_a_different_version_is_not_the_installer(): + """The asset has to be identifiable without trusting the rest of the + release, or a file attached beside the real one can be served instead.""" + payload = _feed("v0.2.0", assets=[{ + "name": "Offloader-Setup-0.1.0.exe", + "browser_download_url": "https://github.com/x/y/z.exe", + }]) + assert update.release_from_feed(payload, installed="0.1.0") is None + + +def test_an_extra_asset_does_not_confuse_the_match(): + payload = _feed("v0.2.0", assets=[ + {"name": "checksums.txt", + "browser_download_url": "https://github.com/a/b/checksums.txt"}, + {"name": "Offloader-Setup-0.2.0.exe", + "browser_download_url": "https://github.com/a/b/setup.exe"}, + ]) + release = update.release_from_feed(payload, installed="0.1.0") + assert release is not None and release.download_url.endswith("setup.exe") + + +@pytest.mark.parametrize("payload", [ + None, [], "release", {}, {"tag_name": 2}, {"tag_name": "v0.2.0"}, + {"tag_name": "v0.2.0", "assets": "none"}, +]) +def test_a_malformed_feed_is_not_an_update(payload): + assert update.release_from_feed(payload, installed="0.1.0") is None + + +def test_check_never_raises(): + """It runs on a timer inside a desktop app. An exception here would take + the app down over a failed DNS lookup.""" + def explode(_url): + raise OSError("no network") + + assert update.check("0.1.0", fetch=explode) is None + + +# ------------------------------------------------------------------ download + + +@pytest.mark.parametrize("url", [ + "http://github.com/a/b.exe", + "https://evil.example.com/a/b.exe", + "https://github.com.evil.example/a/b.exe", + "file:///C:/windows/system32/calc.exe", +]) +def test_a_download_from_the_wrong_place_is_refused(url: str, tmp_path: Path): + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url=url) + with pytest.raises(UpdateError): + update.download(release, tmp_path) + + +def test_a_redirect_to_another_host_is_refused(tmp_path: Path): + """The pre-flight check only sees the feed's URL. GitHub redirects asset + downloads to a different domain, so the host that actually serves the bytes + is the one that has to be checked.""" + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + opener = _opener(b"MZ", url="https://evil.example.com/setup.exe") + + with pytest.raises(UpdateError, match="evil.example.com"): + update.download(release, tmp_path, opener=opener) + + +def test_a_declared_size_over_the_ceiling_is_refused(tmp_path: Path): + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + opener = _opener(b"MZ", url="https://github.com/a/b/setup.exe", + length=str(update.MAX_DOWNLOAD_BYTES + 1)) + + with pytest.raises(UpdateError, match="ceiling"): + update.download(release, tmp_path, opener=opener) + + +def test_a_body_over_the_ceiling_is_refused_mid_stream(tmp_path: Path, + monkeypatch): + """A truthful Content-Length is not required of a hostile server, so the + cap has to hold while the bytes are arriving too.""" + monkeypatch.setattr(update, "MAX_DOWNLOAD_BYTES", 1024) + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + opener = _opener(b"x" * 4096, url="https://github.com/a/b/setup.exe") + + with pytest.raises(UpdateError, match="ceiling"): + update.download(release, tmp_path, opener=opener) + + +def test_the_digest_describes_the_bytes_that_were_written(tmp_path: Path): + import hashlib + + body = b"installer bytes" * 1000 + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + opener = _opener(body, url="https://objects.githubusercontent.com/x") + + path, digest = update.download(release, tmp_path, opener=opener) + + assert path.read_bytes() == body + assert digest == hashlib.sha256(body).hexdigest() + + +def test_an_empty_download_is_refused(tmp_path: Path): + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + opener = _opener(b"", url="https://github.com/a/b/setup.exe") + + with pytest.raises(UpdateError, match="empty"): + update.download(release, tmp_path, opener=opener) + + +def test_progress_is_reported_while_downloading(tmp_path: Path): + body = b"y" * (3 << 20) + release = Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + opener = _opener(body, url="https://github.com/a/b/setup.exe", + length=str(len(body))) + seen: list[tuple[int, int]] = [] + + update.download(release, tmp_path, opener=opener, + progress=lambda done, total: seen.append((done, total))) + + assert seen and seen[-1] == (len(body), len(body)) + + +# -------------------------------------------------------------- verification + + +def _written(tmp_path: Path) -> Path: + path = tmp_path / "Offloader-Setup-0.2.0.exe" + path.write_bytes(b"MZ") + return path + + +def test_a_correctly_signed_installer_verifies(tmp_path: Path): + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + record = update.verify(path, release, run=_runner(_signature()), + expected_digest=GOOD_DIGEST, + actual_digest=GOOD_DIGEST) + assert record["Status"] == "Valid" + + +def test_a_digest_that_changed_after_download_is_refused(tmp_path: Path): + """The window between writing the file and executing it. Anything local + that can write to the download directory could swap it in that gap.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + with pytest.raises(UpdateError, match="changed after"): + update.verify(path, release, run=_runner(_signature()), + expected_digest=GOOD_DIGEST, actual_digest="b" * 64) + + +@pytest.mark.parametrize("status", ["NotSigned", "HashMismatch", + "UnknownError", ""]) +def test_an_untrusted_signature_is_refused(tmp_path: Path, status: str): + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + with pytest.raises(UpdateError, match="signature"): + update.verify(path, release, run=_runner(_signature(Status=status))) + + +def test_another_valid_certificate_is_still_refused(tmp_path: Path): + """A valid signature is not the test. Anyone can buy one; the question is + whether this is the certificate the release was built with.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + with pytest.raises(UpdateError, match="different certificate"): + update.verify(path, release, run=_runner(_signature(Thumbprint="C" * 40))) + + +def test_a_thumbprint_is_compared_without_spacing_or_case(tmp_path: Path): + """PowerShell has returned it both ways; a formatting difference must not + read as a different certificate.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + spaced = " ".join(update.EXPECTED_CERT_THUMBPRINT[i:i + 4] + for i in range(0, 40, 4)).lower() + + update.verify(path, release, run=_runner(_signature(Thumbprint=spaced))) + + +def test_a_different_publisher_name_is_refused(tmp_path: Path): + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + subject = "CN=Someone Else Ltd., O=Someone Else Ltd., C=US" + + with pytest.raises(UpdateError, match="publisher"): + update.verify(path, release, run=_runner(_signature(Subject=subject))) + + +def test_an_installer_reporting_another_version_is_refused(tmp_path: Path): + """The rollback defence. Someone able to re-upload an asset could + otherwise re-serve an older, still validly signed installer under a newer + name and move every install back onto a build whose faults are fixed.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + with pytest.raises(UpdateError, match="0.1.0"): + update.verify(path, release, + run=_runner(_signature(FileVersion="0.1.0"))) + + +def test_a_trailing_windows_build_field_still_matches(tmp_path: Path): + """The installer's embedded FileVersion carries a fourth field the release + version does not have.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + update.verify(path, release, run=_runner(_signature(FileVersion="0.2.0"))) + + +def test_an_unreadable_signature_probe_is_a_failure(tmp_path: Path): + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + + with pytest.raises(UpdateError): + update.verify(path, release, run=_runner({}, returncode=1)) + + +# ------------------------------------------------------------------ applying + + +def test_the_target_is_computed_not_read_from_the_registry(monkeypatch): + """The uninstall key is writable by anything running as the user, so a + planted value would redirect an elevated silent install.""" + monkeypatch.setattr(sys, "frozen", True, raising=False) + monkeypatch.setattr(sys, "executable", + r"C:\Program Files\Offloader\offloader.exe") + + assert update.install_target() == Path(r"C:\Program Files\Offloader") + + +def test_the_install_directory_argument_is_last_and_unquoted(): + """Two rules NSIS enforces silently. Quoting `/D=` installs into a + directory whose name contains a quote; anything after it is swallowed into + the path.""" + _path, arguments = update.install_command( + Path(r"C:\tmp\Offloader-Setup-0.2.0.exe"), + Path(r"C:\Program Files\Offloader")) + + assert arguments == r"/S /D=C:\Program Files\Offloader" + assert arguments.endswith(r"Offloader") + assert '"' not in arguments + + +def test_applying_elevates_with_the_runas_verb(tmp_path: Path): + """A plain spawn does not honour the installer's manifest, so it fails + instead of prompting.""" + calls: list[tuple] = [] + + def shell_execute(handle, verb, path, arguments, directory, show): + calls.append((handle, verb, path, arguments, directory, show)) + return 42 + + update.apply(tmp_path / "setup.exe", Path(r"C:\Program Files\Offloader"), + shell_execute=shell_execute) + + assert calls[0][1] == "runas" + assert calls[0][3] == r"/S /D=C:\Program Files\Offloader" + + +def test_a_declined_elevation_prompt_is_reported_as_such(tmp_path: Path): + """Cancelling the UAC prompt is the most likely outcome of all, and it is + not a broken download.""" + with pytest.raises(UpdateError, match="administrator"): + update.apply(tmp_path / "setup.exe", Path(r"C:\x"), + shell_execute=lambda *_: 5) + + +def test_a_failure_to_start_the_installer_is_reported(tmp_path: Path): + with pytest.raises(UpdateError, match="code 2"): + update.apply(tmp_path / "setup.exe", Path(r"C:\x"), + shell_execute=lambda *_: 2) From edf35de3d34614bc49a55a101b403e8aca363508 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:12:11 -0400 Subject: [PATCH 2/9] Test the install target on the platform running the test The assertion hardcoded a Windows path, and a backslash is not a separator off Windows: install_target resolves the running executable, so on Linux and macOS the literal resolved against the working directory and the test failed everywhere except where it was written. The property it checks is platform-independent, so it now uses a real directory and the fallback is asserted separately. --- tests/test_update.py | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/tests/test_update.py b/tests/test_update.py index e2828a3..f85cb1f 100644 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -424,14 +424,33 @@ def test_an_unreadable_signature_probe_is_a_failure(tmp_path: Path): # ------------------------------------------------------------------ applying -def test_the_target_is_computed_not_read_from_the_registry(monkeypatch): +def test_the_target_is_computed_not_read_from_the_registry(monkeypatch, + tmp_path: Path): """The uninstall key is writable by anything running as the user, so a - planted value would redirect an elevated silent install.""" + planted value would redirect an elevated silent install. + + Uses a real directory rather than a literal Windows path: `install_target` + resolves the running executable, and a backslash is not a separator off + Windows, so a hardcoded `C:\\...` resolves against the working directory + and the assertion would only hold on one platform. + """ + installed = tmp_path / "Offloader" + installed.mkdir() + executable = installed / "Offloader.exe" + executable.write_bytes(b"MZ") monkeypatch.setattr(sys, "frozen", True, raising=False) - monkeypatch.setattr(sys, "executable", - r"C:\Program Files\Offloader\offloader.exe") + monkeypatch.setattr(sys, "executable", str(executable)) - assert update.install_target() == Path(r"C:\Program Files\Offloader") + assert update.install_target() == installed.resolve() + + +def test_an_unfrozen_checkout_falls_back_to_program_files(monkeypatch): + """Not a path a release takes, but it must not resolve to the source tree + and offer to install over it.""" + monkeypatch.setattr(sys, "frozen", False, raising=False) + monkeypatch.setenv("ProgramFiles", r"C:\Program Files") + + assert update.install_target() == Path(r"C:\Program Files") / "Offloader" def test_the_install_directory_argument_is_last_and_unquoted(): From 57d119e4ae2a67d75916247c99583068e625ca23 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:44:06 -0400 Subject: [PATCH 3/9] Read the releases collection, which is the only place the betas are /releases/latest is documented as returning the newest published full release and excluding prereleases. Every release this project has planned so far is a prerelease: the candidate workflow publishes with --prerelease, and the first packaged build is 0.1.0b1. So an installed 0.1.0b1 could not discover 0.1.0b2 or 0.1.0rc1 through that endpoint, and a repository holding only betas answered with nothing at all. The module already went out of its way not to reject prereleases the way Alpha-OSK's updater does, and then asked an endpoint that had already dropped them. The feed is the collection now. It is ordered by creation date rather than by version, so every entry is read and the greatest eligible one wins: a patched 0.1.0b2 published after 0.1.0rc1 is listed above it without being above it. Drafts are skipped, because a draft is visible to anyone who can write to the repository and its assets are not published, so offering one would hand an installer to this machine before the release exists for anybody else. Which releases an install is offered is now stated rather than implied. Newer, and on the channel it is already on: a build that is itself a prerelease is testing the prereleases and takes the next one, and a stable install is offered only stable releases. 0.1.0b1 finding 0.1.0b2 must not also mean 1.0.0 finding 1.0.1b1. A prerelease install still takes the stable release when it arrives, because 0.1.0b2 < 0.1.0 in the existing ordering, which is how a beta tester ends up on the shipping build without doing anything. Changing FEED_URL is free today and would not be later: it is compiled into every shipped build, and nothing has shipped. Eight feed-level tests against a collection holding betas, a release candidate, a stable release and a draft, rather than version comparison against one hand-picked payload. One of them covers an unusable entry not hiding the rest of the feed. docs/updates.md updated to match. --- docs/updates.md | 37 ++++++++++++++++--- src/offloader/update.py | 56 +++++++++++++++++++++++++++-- tests/test_update.py | 80 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 8 deletions(-) diff --git a/docs/updates.md b/docs/updates.md index ac0aeff..3be7d3b 100644 --- a/docs/updates.md +++ b/docs/updates.md @@ -16,21 +16,48 @@ GitHub Releases, read through the REST API. There is no manifest server to run or keep honest, and no second place a version number is written down. ``` -https://api.github.com/repos/owenpkent/offloader/releases/latest +https://api.github.com/repos/owenpkent/offloader/releases?per_page=30 ``` That URL is compiled into every shipped build, so moving it orphans every install that already exists. Renaming or transferring the repository is a breaking change for installed copies, not an administrative detail. -Three fields are read. `tag_name` must be a version this project publishes -(`0.2.0`, `0.1.0b1`, with or without a leading `v`); anything else, including -`latest` or `1.0.3-evil`, is refused rather than compared as a string. -`assets[]` must contain an entry named exactly +The collection, not `/releases/latest`. GitHub documents that endpoint as +returning the newest published *full* release and excluding prereleases, so an +installed `0.1.0b1` could never discover `0.1.0b2` through it, and a repository +holding only the betas the candidate workflow publishes with `--prerelease` +would answer with nothing at all. Ordering prereleases is the point of the +grammar below, so the feed has to be one that carries them. + +The collection is ordered by creation date, which is not the same as version +order: a patched `0.1.0b2` published after `0.1.0rc1` is listed above it. So +every entry is read and the greatest eligible version wins, rather than the +first one that parses. + +Four fields are read per release. `draft` excludes it: a draft is visible to +anyone who can write to the repository and its assets are not published, so +offering one would hand an installer to the maintainer's own machine before the +release exists for anybody else. `tag_name` must be a version this project +publishes (`0.2.0`, `0.1.0b1`, with or without a leading `v`); anything else, +including `latest` or `1.0.3-evil`, is refused rather than compared as a +string. `assets[]` must contain an entry named exactly `Offloader-Setup-{version}.exe` for the version the tag claims, so a file attached beside the real installer cannot be served in its place. `body` becomes the release notes. +## Which releases an install is offered + +Newer, and on the channel it is already on. An installation that is itself a +prerelease is testing the prereleases, so it takes the next one, beta or +release candidate or final. An installation on a stable release is offered only +stable releases: it did not volunteer for the next beta, and `0.1.0b1` finding +`0.1.0b2` must not also mean `1.0.0` finding `1.0.1b1`. + +A prerelease install still takes the stable release when it arrives, because +`0.1.0b2` < `0.1.0` in the ordering below. That is how a beta tester ends up on +the shipping build without doing anything. + ## Version ordering The grammar is the one `build/windows/versioning.py` already enforces, which diff --git a/src/offloader/update.py b/src/offloader/update.py index 74f1659..9af7131 100644 --- a/src/offloader/update.py +++ b/src/offloader/update.py @@ -45,7 +45,16 @@ #: Where releases are published. Pinned deliberately: this URL is compiled into #: every shipped build, so moving it orphans every install that already exists. #: Treat a rename or transfer of the repository as a breaking change. -FEED_URL = "https://api.github.com/repos/owenpkent/offloader/releases/latest" +#: +#: The collection rather than `/releases/latest`. GitHub documents that endpoint +#: as returning the newest published *full* release and excluding prereleases, +#: so an installed `0.1.0b1` could never see `0.1.0b2` through it, and a +#: repository holding only betas — which is what the candidate workflow's +#: `--prerelease` produces — would answer with nothing at all. Ordering +#: prereleases is the whole point of the grammar below, so the feed has to be +#: one that carries them. +FEED_URL = ("https://api.github.com/repos/owenpkent/offloader/releases" + "?per_page=30") #: The published installer's name. Part of the release contract — the asset has #: to be identifiable without trusting anything else in the release. @@ -144,7 +153,43 @@ def _fetch_json(url: str, *, timeout: int = 30) -> Any: def release_from_feed(payload: Any, *, installed: str = __version__) -> Release | None: - """The newer release the feed describes, or None. + """The best newer release the feed describes, or None. + + Takes the releases collection, or a single release object for a caller that + already has one. The greatest eligible version wins rather than whichever + the feed happens to list first: GitHub orders by creation date, and a + patched `0.1.0b2` published after `0.1.0rc1` would otherwise be offered as + the upgrade from it. + """ + if isinstance(payload, list): + candidates = [_release_entry(item, installed=installed) for item in payload] + ranked = [(parse_version(found.version), found) + for found in candidates if found is not None] + if not ranked: + return None + return max(ranked, key=lambda pair: pair[0])[1] + return _release_entry(payload, installed=installed) + + +def _eligible(version: str, installed: str) -> bool: + """Whether an installation on `installed` should be offered `version`. + + Newer, and not a step off the channel this install is already on. A build + that is itself a prerelease is testing the prereleases, so it takes the + next one; a stable install is not volunteered for a beta it did not ask + for. Neither side being readable means no, as everywhere else here. + """ + if not is_newer(version, installed): + return False + running, offered = parse_version(installed), parse_version(version) + if running is None or offered is None: + return False + stable = _STAGE_ORDER[None] + return running[3] != stable or offered[3] == stable + + +def _release_entry(payload: Any, *, installed: str) -> Release | None: + """One release from the feed, if it is one this install should be offered. Reads only what it needs, and requires the asset to be named for the version the tag claims: an extra or renamed file in a release cannot then @@ -152,6 +197,11 @@ def release_from_feed(payload: Any, *, installed: str = __version__) -> Release """ if not isinstance(payload, dict): return None + if payload.get("draft"): + # A draft is visible to anyone who can write to the repository and its + # assets are not published. Offering one would hand an installer to the + # maintainer's own machine before the release exists for anybody else. + return None tag = payload.get("tag_name") if not isinstance(tag, str): return None @@ -159,7 +209,7 @@ def release_from_feed(payload: Any, *, installed: str = __version__) -> Release if tag_match is None: return None version = tag_match.group(1) - if parse_version(version) is None or not is_newer(version, installed): + if parse_version(version) is None or not _eligible(version, installed): return None expected = ASSET_TEMPLATE.format(version=version) diff --git a/tests/test_update.py b/tests/test_update.py index f85cb1f..6fdabf3 100644 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -216,6 +216,86 @@ def test_a_malformed_feed_is_not_an_update(payload): assert update.release_from_feed(payload, installed="0.1.0") is None +# ------------------------------------------------------- the whole collection + + +def _collection() -> list[dict]: + """What the releases endpoint actually returns: newest created first, betas + and release candidates among them, and a draft nobody else can see yet.""" + return [ + dict(_feed("v0.2.0b1"), draft=True, prerelease=True), + dict(_feed("v0.1.0b2"), prerelease=True), + dict(_feed("v0.1.0rc1"), prerelease=True), + dict(_feed("v0.1.0b1"), prerelease=True), + dict(_feed("v0.0.9"), prerelease=False), + ] + + +def test_a_beta_finds_the_next_prerelease_in_the_collection(): + """REGRESSION. `/releases/latest` excludes prereleases, so a shipped + `0.1.0b1` could not see `0.1.0b2` or `0.1.0rc1` through it, and a + repository holding only betas answered with nothing at all.""" + release = update.release_from_feed(_collection(), installed="0.1.0b1") + assert release is not None and release.version == "0.1.0rc1" + + +def test_the_greatest_version_wins_not_the_one_listed_first(): + """The feed is ordered by creation date, so a patched `0.1.0b2` published + after `0.1.0rc1` sits above it in the list without being above it.""" + release = update.release_from_feed(_collection(), installed="0.1.0a1") + assert release is not None and release.version == "0.1.0rc1" + + +def test_a_draft_release_is_not_offered(): + """Drafts are visible to anyone who can write to the repository, and their + assets are not published.""" + feed = [dict(_feed("v0.3.0"), draft=True)] + assert update.release_from_feed(feed, installed="0.1.0") is None + + +def test_a_stable_install_is_not_offered_a_prerelease(): + """A build that is itself a prerelease is testing them. One that is not did + not volunteer for the next beta.""" + feed = [dict(_feed("v0.2.0b1"), prerelease=True), + dict(_feed("v0.1.0"), prerelease=False)] + assert update.release_from_feed(feed, installed="0.1.0") is None + + feed.append(dict(_feed("v0.2.0"), prerelease=False)) + release = update.release_from_feed(feed, installed="0.1.0") + assert release is not None and release.version == "0.2.0" + + +def test_a_prerelease_install_still_takes_the_stable_release(): + feed = [dict(_feed("v0.1.0b2"), prerelease=True), + dict(_feed("v0.1.0"), prerelease=False)] + release = update.release_from_feed(feed, installed="0.1.0b1") + assert release is not None and release.version == "0.1.0" + + +def test_a_collection_with_nothing_newer_is_not_an_update(): + assert update.release_from_feed(_collection(), installed="0.2.0") is None + + +def test_an_unusable_entry_does_not_hide_the_rest_of_the_collection(): + """One release with a renamed asset or an unreadable tag is skipped, not a + reason to decline every other release in the feed.""" + feed = [ + {"tag_name": "latest", "assets": []}, + dict(_feed("v0.2.0"), assets=[{"name": "Offloader-Setup-0.1.0.exe", + "browser_download_url": "https://x/y"}]), + dict(_feed("v0.1.5")), + ] + release = update.release_from_feed(feed, installed="0.1.0") + assert release is not None and release.version == "0.1.5" + + +def test_the_feed_url_is_the_collection_not_the_latest_endpoint(): + """`/releases/latest` is documented as excluding prereleases, which is the + only kind of release this project has so far.""" + assert not update.FEED_URL.endswith("/releases/latest") + assert "/releases" in update.FEED_URL + + def test_check_never_raises(): """It runs on a timer inside a desktop app. An exception here would take the app down over a failed DNS lookup.""" From 10da5354de61cc68ebfb659f2cfa75ffd5ca72c8 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:37:36 -0400 Subject: [PATCH 4/9] Record the release feed and the channel rule in the changelog --- CHANGELOG.md | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff8e5d5..5944fed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,9 +22,26 @@ project uses [semantic versioning][semver]. would otherwise move every install back onto a build whose faults are fixed. The install target is computed from the running executable rather than read from the uninstall registry key, which anything running as the user can - write. Prereleases are ordered rather than rejected, matching the grammar + write. + + Prereleases are ordered rather than rejected, matching the grammar `build/windows/versioning.py` already enforces, and a test asserts the two - orderings agree. The updater never closes a running copy: the installer's + orderings agree. The feed is the releases collection rather than + `/releases/latest`, which GitHub documents as excluding prereleases: on that + endpoint an installed `0.1.0b1` could never see `0.1.0b2`, and a repository + holding only the betas the candidate workflow publishes would answer with + nothing at all. The collection is ordered by creation date rather than by + version, so every entry is read and the greatest eligible one wins; drafts + are skipped, since their assets are not published. + + **An install is offered what is newer on the channel it is already on.** A + build that is itself a prerelease is testing the prereleases and takes the + next one, and takes the stable release when it arrives, because `0.1.0b2` is + older than `0.1.0`. An install on a stable release is offered only stable + releases: `0.1.0b1` finding `0.1.0b2` must not also mean `1.0.0` finding + `1.0.1b1`. + + The updater never closes a running copy: the installer's refusal while a transfer is in flight is the guarantee, so the command says so before the elevation prompt appears. See [`docs/updates.md`](docs/updates.md); the in-app check is still deferred. From fe5bb2ccf808d2a9770b7c7a750b35c298e27d2a Mon Sep 17 00:00:00 2001 From: Owen Kent Date: Thu, 8 Oct 2026 20:25:46 -0400 Subject: [PATCH 5/9] Update test counts --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d47c29e..f5bd824 100644 --- a/README.md +++ b/README.md @@ -595,7 +595,7 @@ ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -820 tests at 84% line coverage. They cover formatting against the reference's +891 tests at 84% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, From 367cec9b7e49555a7e4ad9e5521c9238992a2f30 Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:29:00 -0600 Subject: [PATCH 6/9] Update test counts --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 41bdae8..f7e2f37 100644 --- a/README.md +++ b/README.md @@ -600,7 +600,7 @@ ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -895 tests at 84% line coverage. They cover formatting against the reference's +899 tests at 84% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, From 312eb10ae9bd517929028cda38a0967b5d35a2ba Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:12:07 -0600 Subject: [PATCH 7/9] Harden the command-line updater: real pre-launch hash, trusted PowerShell path, honest failures The compare-before-launch check hashed nothing: verify was handed the same digest on both sides. The file is now re-hashed on disk before and after the signature probe and again immediately before launch, and a mismatch refuses. The signature verdict came from a bare "powershell" name, which Windows resolves through the current directory first. PowerShell is now run by its absolute System32 path with -NoProfile -NonInteractive from the system root, and the installer path reaches the probe through an environment variable rather than script text, so quoting cannot break or inject it. A feed that could not be read printed "newest release available" and exited 0; it now fails with exit 5. The installer's own refusal was never seen; the updater now waits where it can and reports the exit code, and an installed build that cannot wait exits 6 instead of claiming success. Downloads go to an exclusively created partial, are fsynced, promoted only after verification, and refuse linked directories. Non-installed builds are refused with a message. The certificate pin accepts an announced successor thumbprint so rotation does not strand installs. --- CHANGELOG.md | 55 +++- docs/updates.md | 138 +++++++- src/offloader/cli.py | 90 +++++- src/offloader/update.py | 510 ++++++++++++++++++++++++----- tests/test_update.py | 686 +++++++++++++++++++++++++++++++++++++--- 5 files changed, 1333 insertions(+), 146 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c69c4c..b185a4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,46 @@ project uses [semantic versioning][semver]. ### Fixed +- **`offloader update` reported a feed it could not read as "up to date".** + Being offline, an HTTP error or rate limit, a redirect off the allowlist and + malformed JSON all printed "the newest release available" and exited 0, so + a broken proxy could suppress updates indefinitely. Each is now printed as a + failure to check, with status 5 (see `docs/updates.md` for every status). +- **The updater's digest check compared the streamed digest with itself.** + The installer is now re-hashed on disk before and after its signature is + read and immediately before launch; a mismatch refuses to run it and names + the file. The remaining window between the last hash and the launch is + documented. +- **The signature verdict came from whichever `powershell` the search order + found first,** which includes the current directory, often a card root. + PowerShell is now run by absolute path under `%SystemRoot%` with a fixed + working directory, and the elevated installer runs in its own directory. +- **An apostrophe in the installer's path broke signature checking.** The + path was pasted into a quoted PowerShell literal; a profile named O'Brien + failed every update and a crafted directory name became script. It is now + passed through an environment variable and never becomes script text. +- **A refused silent install was reported as started.** The installer is + launched with `ShellExecuteExW` so its exit code can be read, and a non-zero + code is reported as a refusal or failure. The installed CLI runs from the + installation it replaces and cannot wait without blocking that installer, + so there it says it cannot see the result and exits with status 6 instead + of claiming success. +- **A failed download left a plausible installer behind.** Bytes now go to an + exclusively created `.offloader-partial` file, flushed to disk, and get the + installer's name only after the size and signature checks pass; any failure + deletes the partial and the temporary directory. A short body against its + `Content-Length`, a linked or junctioned download directory, and a link at + the installer's name are refused, and a dropped connection is an update + error rather than a traceback. +- **`offloader update --install` guessed a target for builds it cannot + update.** A pip or source install fell back to `Program Files`, risking a + second copy; a portable bundle was handed to an installer that always + refuses it. Both are now refused before downloading, naming the build that + was detected and how to update it instead. +- **One certificate thumbprint left no path for renewal.** The updater + accepts a tuple of pinned thumbprints, current first, so the next + certificate can be shipped ahead of a rotation; the publisher name is still + required. `docs/updates.md` describes the rotation steps. - **A silent installer `/D=` target was overridden by the registered location.** NSIS consumes `/D=` before `.onInit`, so the old `$CMDLINE` check never saw it and `setup.exe /S /D=B` reinstalled into the registered location A. An explicit @@ -100,15 +140,18 @@ project uses [semantic versioning][semver]. Releases is the feed, so there is no manifest server and no second place a version is written down. Before anything runs: HTTPS with a host allowlist re-checked after redirects, a size ceiling held against `Content-Length` and - again mid-stream, a SHA-256 taken while streaming and compared before - launch, and an Authenticode check that requires a valid status, this - project's certificate thumbprint, its publisher name, and an embedded + again mid-stream, a SHA-256 taken while streaming and checked against the + file on disk before and after the signature is read and immediately before + launch, and an Authenticode check that requires a valid status, one of this + project's pinned certificate thumbprints, its publisher name, and an embedded `FileVersion` matching the release. That last check is what stops a rollback: re-serving an older, still validly signed installer under a newer asset name would otherwise move every install back onto a build whose faults are fixed. - The install target is computed from the running executable rather than read - from the uninstall registry key, which anything running as the user can - write. + The install target is the running installed build's own directory rather + than the uninstall key's `InstallLocation`, which records where the last + install went rather than which copy is running (the key is under HKLM; when + the running copy is in the default location the installer still prefers the + registered one, a documented limit). Prereleases are ordered rather than rejected, matching the grammar `build/windows/versioning.py` already enforces, and a test asserts the two diff --git a/docs/updates.md b/docs/updates.md index 3be7d3b..06690f3 100644 --- a/docs/updates.md +++ b/docs/updates.md @@ -46,6 +46,26 @@ string. `assets[]` must contain an entry named exactly attached beside the real installer cannot be served in its place. `body` becomes the release notes. +A feed that cannot be read is a failure, not an answer. Being offline, an HTTP +error (including GitHub's 403 or 429 when the unauthenticated rate limit is +reached), a redirect to a host outside the allowlist, or a body that is not +the expected JSON makes `offloader update` print the reason to stderr and exit +with status 5. It never prints "the newest release available" in that case: +reporting a failure as "up to date" would let a broken proxy or a network +attacker suppress updates indefinitely without anyone noticing. (The library's +`check()` still returns None on any failure for a future background caller; +the command uses `find_release()`, which raises.) + +## Exit status + +| Status | Meaning | +| --- | --- | +| 0 | Up to date, or the installer ran and reported success | +| 1 | A newer release exists and was not installed (no `--install`) | +| 2 | A usage error, a build that cannot update itself, or a download, verification, launch or installer failure (including a declined UAC prompt and an installer that refused) | +| 5 | The release feed could not be fetched or read, so nothing is known | +| 6 | The installer was started but its result could not be observed (see [Applying it](#applying-it)) | + ## Which releases an install is offered Newer, and on the channel it is already on. An installation that is itself a @@ -77,39 +97,131 @@ is what makes `1.0.10` look older than `1.0.9`. | --- | --- | | HTTPS, and a host allowlist applied **after** redirects | GitHub redirects asset downloads to another domain, so the host that actually serves the bytes is the one that matters | | A size ceiling, enforced against `Content-Length` and again while streaming | A hostile or confused feed filling the disk; a truthful `Content-Length` is not something a server owes us | -| SHA-256 taken while streaming, compared again before launch | The gap between writing the file and executing it, in which anything able to write to the download directory could swap it | +| Staged download: written to `Offloader-Setup-{version}.exe.offloader-partial`, created exclusively, flushed to disk, and given its real name only after the size and signature checks pass | Incomplete or unverified bytes left under a plausible installer name after a failed or interrupted download. Any failure deletes the partial file, and a temporary directory the command created is removed | +| A download directory, or an existing file at either download name, that is a symlink or junction is refused | A shared `--dir` redirecting where the installer is written | +| A body shorter than its declared `Content-Length` is refused | A truncated download reaching the signature check | +| SHA-256 taken while streaming, then the file on disk re-hashed before the signature is read, after it is read, and immediately before launch | A file replaced after it was hashed: a mismatch at any point refuses, names the file, and nothing is launched | | Authenticode status is `Valid` | An unsigned or tampered installer | -| The signing certificate's thumbprint matches | A valid signature from somebody else. Anyone can obtain one; the question is whether this is the certificate the release was built with | +| The signing certificate's thumbprint is one of the pinned thumbprints | A valid signature from somebody else. Anyone can obtain one; the question is whether this is the certificate the release was built with | | The publisher name matches | The same, read the way the UAC prompt will read it | | The installer's **embedded** `FileVersion` matches the release version | A rollback. Someone able to re-upload an asset could otherwise re-serve an older, still validly signed installer under a newer name, moving every install back onto a build whose faults are already fixed | +The signature and version are read by Windows PowerShell, launched by absolute +path (`%SystemRoot%\System32\WindowsPowerShell\v1.0\powershell.exe`) with +`-NoProfile -NonInteractive -ExecutionPolicy Bypass` and the Windows directory +as its working directory. A bare `powershell` would be found by the Windows +search order, which tries the current directory before System32, and the CLI +is routinely run from a card's root. The installer's path reaches the script +through an environment variable, never as script text, so an apostrophe or `$` +in a profile or `--dir` name is just part of the path. The Security and +Utility modules are imported from `$PSHOME` explicitly and their cmdlets +called by qualified name. + +**Known limits.** The re-hashes and the launch are separate opens of the path: +a process able to write the download directory in the moment between the last +hash and `ShellExecuteExW` opening the file could still swap it. The default +directory is a fresh per-user temporary directory, which narrows that to the +operator's own account; `--dir` refuses links but does not check who else can +write the directory, so point it only at a private location. A swapped, +unsigned binary would also show an "Unknown publisher" UAC prompt. Holding one +handle with write sharing denied, or verifying with `WinVerifyTrust` on that +handle, would close the gap and is not implemented. + +### Certificate rotation + The certificate values are the ones in `build/windows/sign.py`, repeated in `src/offloader/update.py` rather than imported, because the build scripts are -not part of the shipped package. +not part of the shipped package. A test asserts the current thumbprint and the +publisher name still match that script. + +`EXPECTED_CERT_THUMBPRINTS` is a tuple, current certificate first, so a +renewal does not strand installed copies. A build that knows only one leaf +certificate refuses every release signed by its successor, and recovering +from that takes a manual reinstall. To rotate: + +1. Before the current certificate is replaced, ship a release (signed with the + current certificate) whose tuple lists the next certificate's thumbprint + after the current one. +2. Give installs time to take that release. +3. Sign releases with the new certificate and move its thumbprint to the front + (and update `DEFAULT_CERT_THUMBPRINT` in `sign.py`). Drop the old one once + it is no longer used. + +Every accepted certificate must still present `EXPECTED_SIGNER_NAME`; a +renewal that changes the publisher name needs that constant changed in the +same step-1 release. The revocation behaviour of `Get-AuthenticodeSignature` +is whatever Windows applies by default; it is not configured explicitly. + +## Which builds can update themselves + +Only an installed packaged build: a frozen executable whose directory carries +the installer's `.offloader-install.json` manifest. `offloader update +--install` checks this before downloading anything, and refuses with a message +naming what it found: + +- **A pip install or a source checkout.** The installer does not replace + those, and guessing `Program Files` would install a second copy beside the + registered one. Update it the way it was installed (`pip install --upgrade`, + `git pull`) or run the installer from the releases page. +- **A portable bundle** (frozen, no manifest). The installer's maintenance + step refuses a directory it does not own, every time, so the command + refuses up front instead. Run the installer, or replace the folder by hand. + +`offloader update` without `--install` still reports what is available on any +build. ## Applying it -The verified installer is run through `ShellExecuteW` with the `runas` verb, -silently: `/S /D=`. A plain process spawn does not honour the -installer's manifest request for elevation and fails instead of prompting. +The verified installer is run through `ShellExecuteExW` with the `runas` verb, +silently: `/S /D=`, with the installer's own directory as its working +directory rather than the operator's. A plain process spawn does not honour +the installer's manifest request for elevation and fails instead of +prompting. A declined UAC prompt is reported as such (status 2). Two details are easy to break silently. `/D=` must be the **last** argument and must **not** be quoted even when the path contains spaces: quoting it installs into a directory whose name contains a quote, and anything placed -after it is swallowed into the path. And the target is computed from the -running executable, never read from the uninstall registry key, which is -writable by anything running as the user and would otherwise let a planted -value redirect an elevated silent install. +after it is swallowed into the path. And the target is the running +executable's directory, not the uninstall key's `InstallLocation`. That key is +under HKLM and only an administrator can write it; the reason is that it +records where the last install went, which is not necessarily the copy that is +running. **Known limit:** NSIS consumes `/D=` before `.onInit`, and the only +sign of an explicit target is that it differs from the compiled default +(`$PROGRAMFILES64\Offloader`). When the running copy is in that default +location, the installer cannot tell `/D=` from no `/D=` and uses the +registered `InstallLocation` instead, so in the common case the registry value +still decides the target. + +**Waiting for the result.** `ShellExecuteExW` is called with +`SEE_MASK_NOCLOSEPROCESS`, so the process can be waited for and its exit code +read. Silent mode suppresses the installer's own message box, so that exit +code is the only report of a refusal: a non-zero code is printed as "the +installer refused or failed" and the command exits 2. + +In practice the installed CLI cannot wait. It runs from the installation it is +replacing: for its whole lifetime it holds that installation's shared lock, +which maintenance needs exclusively, and its own executable, which maintenance +must move and delete. Waiting would make every update refuse. So when the +running executable is inside the target, the command starts the installer, +exits at once to release the installation, prints that it cannot see the +result, and returns status 6. Confirm the update afterwards with `offloader +--version`. The download directory is left in place in that case, because the +installer is still running from it. Exiting promptly is what lets maintenance +take the lock; this ordering is practical (the UAC prompt alone takes longer) +rather than enforced. ## What it deliberately does not do **It does not close anything.** Offloader's installer refuses maintenance -while a transfer is in flight and exits non-zero without changing the +while any Offloader program (the desktop app or a CLI process, busy or idle) +holds the installation's shared lock, and exits non-zero without changing the installation; it never force-kills a running copy. That refusal is a promise in [data-safety.md](data-safety.md), so the updater's job is to verify and hand over, not to clear the way. `offloader update --install` says so before the elevation prompt appears, because an operator who does not know it reads -the installer's refusal as a broken update. +the installer's refusal as a broken update. Under `/S` the refusal is visible +only through the exit code, which the command reports when it can wait and +says it cannot see when it cannot. The consequence is that updating requires closing the app and any CLI transfer first. That is the intended trade: a packaged updater that could @@ -118,7 +230,7 @@ interrupt a card offload would be a worse tool than one that asks. **There is no rollback.** A failed install leaves the previous version in place where the installer's own transactional recovery manages it (see `installation.py`); the updater does not attempt a second recovery mechanism -on top. Nothing is deleted by the updater itself. +on top. The updater deletes nothing but its own download files. **There is no automatic check yet.** `offloader update` is explicit. An in-app check, a notification and a timer belong with the desktop interface and are diff --git a/src/offloader/cli.py b/src/offloader/cli.py index 4e2e048..6cc1a79 100644 --- a/src/offloader/cli.py +++ b/src/offloader/cli.py @@ -623,15 +623,27 @@ def cmd_gui(_args: argparse.Namespace) -> int: def cmd_update(args: argparse.Namespace) -> int: """Report or apply a newer release. - Exit status: 0 when up to date or an install was started, 1 when an update - exists but was not applied, 2 on a usage or verification failure. A caller - scripting this can therefore tell "nothing to do" from "something to do". + Exit status (documented in docs/updates.md): + + * 0: up to date, or the installer ran and reported success; + * 1: an update exists but was not applied (no `--install`); + * 2: usage, a build that cannot update itself, or a download, + verification, launch or installer failure; + * 5: the release feed could not be fetched or read, so nothing is known; + * 6: the installer was started but its result could not be observed. + + A caller scripting this can therefore tell "nothing to do" from "something + to do" from "could not find out". """ import tempfile from . import update as update_mod - release = update_mod.check() + try: + release = update_mod.find_release() + except update_mod.UpdateError as exc: + print(f"error: could not check for updates: {exc}", file=sys.stderr) + return 5 if release is None: print(f"Offloader {__version__} is the newest release available.") return 0 @@ -647,10 +659,11 @@ def cmd_update(args: argparse.Namespace) -> int: file=sys.stderr) return 2 - directory = args.dir - if directory is None: - directory = Path(tempfile.mkdtemp(prefix="offloader-update-")) - directory.mkdir(parents=True, exist_ok=True) + try: + target = update_mod.install_target() + except update_mod.UpdateError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 def report(done: int, total: int) -> None: if total: @@ -659,12 +672,28 @@ def report(done: int, total: int) -> None: sys.stderr.write(f"\r downloading {format_size(done)}") sys.stderr.flush() + created: Path | None = None + staged: Path | None = None + installer: Path | None = None + observed: bool | None = None + wait = False try: - installer, digest = update_mod.download(release, directory, - progress=report) + directory = args.dir + if directory is None: + directory = created = Path(tempfile.mkdtemp(prefix="offloader-update-")) + elif update_mod.is_link(directory): + raise update_mod.UpdateError( + f"refusing a download directory that is a link or junction: " + f"{directory}") + else: + directory.mkdir(parents=True, exist_ok=True) + + staged, digest = update_mod.download(release, directory, + progress=report) sys.stderr.write("\r" + " " * 32 + "\r") - update_mod.verify(installer, release, - expected_digest=digest, actual_digest=digest) + update_mod.verify(staged, release, expected_digest=digest) + installer = update_mod.promote(staged) + staged = None print(f" verified {installer.name}") print(f" sha256 {digest}") # Said before the prompt appears, because the installer refuses rather @@ -673,11 +702,44 @@ def report(done: int, total: int) -> None: print("\nThe installer will not replace a running Offloader. Close " "the app and any\nCLI transfer first; an active job makes it " "exit without changing anything.") - update_mod.apply(installer) + # A process running from the installation it replaces cannot wait: + # it holds that installation's lock and files open, which is exactly + # what makes the installer refuse. + wait = not update_mod.runs_from(target) + observed = update_mod.apply(installer, target, expected_digest=digest, + wait=wait) except update_mod.UpdateError as exc: print(f"error: {exc}", file=sys.stderr) return 2 - return 0 + finally: + if staged is not None: + update_mod.discard(staged) + # A temporary directory this command created is removed unless the + # installer may still be running from it (started, not waited for). + if created is not None and observed is not False: + if installer is not None: + update_mod.discard(installer) + try: + created.rmdir() + except OSError: + pass + + if observed: + print(f"Offloader {release.version} was installed.") + return 0 + if wait: + print("The installer has started, but Windows returned no process to " + "wait on, so its result is unknown. Silent mode shows no " + "message if it refuses; run `offloader --version` once it has " + f"finished to confirm {release.version} is installed.") + else: + print("The installer has started. This command runs from the " + "installation it is replacing, so it exits now rather than hold " + "that installation open, and it cannot see the installer's " + "result. Silent mode shows no message if it refuses; run " + f"`offloader --version` once it has finished to confirm " + f"{release.version} is installed.") + return 6 def main(argv: list[str] | None = None) -> int: diff --git a/src/offloader/update.py b/src/offloader/update.py index 9af7131..09092f0 100644 --- a/src/offloader/update.py +++ b/src/offloader/update.py @@ -10,8 +10,9 @@ * **Nothing is force-closed.** Alpha-OSK's installer terminates the running app and a helper process restarts it afterwards. Offloader's installer refuses - maintenance while a copy is in flight and never force-kills — that is a - data-safety promise in `docs/data-safety.md`, not an implementation detail. + maintenance while any Offloader program holds the installation open, and + never force-kills. That is a data-safety promise in `docs/data-safety.md`, + not an implementation detail. So this module verifies and hands over; it never tries to clear the way. * **Prereleases are not rejected.** Alpha-OSK's updater only accepts `X.Y.Z` tags. Offloader's first packaged release is planned as `0.1.0b1`, so the same @@ -19,17 +20,20 @@ grammar here is the one `build/windows/versioning.py` already enforces, and the ordering agrees with the Windows version fields that installer writes. -Everything fails closed. A feed that cannot be parsed, a version that cannot be -read, a digest that does not match, a signature from the wrong certificate: all -of them mean "no update", never "install it anyway". +Everything fails closed. A version that cannot be read, a digest that does not +match, a signature from the wrong certificate: all of them mean "do not +install", never "install it anyway". A feed that cannot be fetched or parsed is +a failure to check (`find_release` raises), never "up to date". """ from __future__ import annotations import hashlib +import http.client import json import os import re +import stat import subprocess import sys import urllib.error @@ -38,7 +42,7 @@ from collections.abc import Callable from dataclasses import dataclass from pathlib import Path -from typing import Any +from typing import Any, NamedTuple from ._version import __version__ @@ -74,15 +78,39 @@ #: make this write to disk. MAX_DOWNLOAD_BYTES = 500 * 1024 * 1024 -#: The certificate the installer is signed with, and the name it must present. -#: The same values as `build/windows/sign.py`, repeated rather than imported -#: because that module is a build script and is not shipped. -EXPECTED_CERT_THUMBPRINT = "FC22B5221318F3F3F6B3EB2D969D7F99091557BF" +#: Where an operator is sent when this build cannot update itself. +RELEASES_PAGE = "https://github.com/owenpkent/offloader/releases" + +#: The certificates an installer may be signed with, current one first, and +#: the name every one of them must present. The values come from +#: `build/windows/sign.py`, repeated rather than imported because that module +#: is a build script and is not shipped. +#: +#: A tuple so a renewed certificate can be rotated in without stranding the +#: field: ship a release that lists the next thumbprint *after* the current +#: one, wait for installs to take it, then sign with the new certificate and +#: move it to the front. A build that only knows one leaf certificate refuses +#: every release signed by its successor and can only be recovered by a +#: manual reinstall. See docs/updates.md. +EXPECTED_CERT_THUMBPRINTS: tuple[str, ...] = ( + "FC22B5221318F3F3F6B3EB2D969D7F99091557BF", +) +#: The certificate currently used for releases. +EXPECTED_CERT_THUMBPRINT = EXPECTED_CERT_THUMBPRINTS[0] EXPECTED_SIGNER_NAME = "OK Studio Inc." #: How long a signature or version probe may take before it is a failure. PROBE_TIMEOUT = 60 +#: Suffix of the download while it is incomplete or unverified. Nothing lands +#: under the installer's own name until the bytes are complete and signed. +PARTIAL_SUFFIX = ".offloader-partial" + +#: Environment variable the signature probe reads the installer's path from. +#: The path never appears in the script text, so no character in it can end a +#: string literal or be read as PowerShell. +_PROBE_PATH_VARIABLE = "OFFLOADER_UPDATE_INSTALLER" + _VERSION_RE = re.compile(r"^(\d+)\.(\d+)\.(\d+)(?:(a|b|rc)(\d+))?$") _TAG_RE = re.compile(r"^v?(.+)$") @@ -228,36 +256,135 @@ def _release_entry(payload: Any, *, installed: str) -> Release | None: return None +def find_release(installed: str = __version__, *, url: str = FEED_URL, + fetch: Callable[[str], Any] = _fetch_json) -> Release | None: + """Ask the feed for a newer release, raising if the feed cannot be read. + + None means the feed was read and holds nothing newer for this install. + Every failure to get that answer (offline, an HTTP error or rate limit, a + redirect off the allowlist, a body that is not the expected JSON) raises + `UpdateError` instead, because reporting it as "up to date" would let a + broken proxy or a network attacker suppress updates without anyone + noticing. + """ + try: + payload = fetch(url) + except UpdateError as exc: + raise UpdateError(f"the release feed was refused: {exc}") from exc + except urllib.error.HTTPError as exc: + hint = (" (GitHub's rate limit for unauthenticated requests may have " + "been reached; try again later)" + if exc.code in (403, 429) else "") + raise UpdateError( + f"the release feed answered HTTP {exc.code}{hint}") from exc + except urllib.error.URLError as exc: + raise UpdateError( + f"could not reach the release feed: {exc.reason}") from exc + except (ValueError, UnicodeDecodeError) as exc: + raise UpdateError("the release feed was not readable JSON") from exc + except (OSError, http.client.HTTPException) as exc: + raise UpdateError(f"could not read the release feed: {exc}") from exc + if not isinstance(payload, (list, dict)): + raise UpdateError("the release feed was not a list of releases") + return release_from_feed(payload, installed=installed) + + def check(installed: str = __version__, *, url: str = FEED_URL, fetch: Callable[[str], Any] = _fetch_json) -> Release | None: """Ask the feed for a newer release. Never raises. - Called on a timer and from a menu item, where the cost of an exception is - an interrupted app and the cost of returning None is one missed check. + For a background caller (a timer or a menu item), where the cost of an + exception is an interrupted app and the cost of returning None is one + missed check. None here does not mean "up to date": anything that has to + tell the operator so, like the `update` command, uses `find_release`. """ try: - return release_from_feed(fetch(url), installed=installed) + return find_release(installed, url=url, fetch=fetch) except Exception: return None +def is_link(path: Path) -> bool: + """Whether `path` is a symlink, a junction or any other reparse point. + + False when nothing exists there. A junction is not a symlink to + `os.path.islink`, so the reparse attribute is checked as well. + """ + try: + info = os.lstat(path) + except FileNotFoundError: + return False + attributes = getattr(info, "st_file_attributes", 0) + reparse = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0x400) + return stat.S_ISLNK(info.st_mode) or bool(attributes & reparse) + + +def discard(path: Path) -> None: + """Delete a staging file this module created, never following a link.""" + try: + if not is_link(path) and path.is_file(): + path.unlink() + except OSError: + pass + + +def file_sha256(path: Path) -> str: + """SHA-256 of the file as it is on disk now.""" + digest = hashlib.sha256() + with open(path, "rb") as handle: + for block in iter(lambda: handle.read(1 << 20), b""): + digest.update(block) + return digest.hexdigest() + + +def _assert_unchanged(path: Path, expected_digest: str, when: str) -> None: + try: + actual = file_sha256(path) + except OSError as exc: + raise UpdateError(f"could not re-read {path}: {exc}") from exc + if actual.lower() != expected_digest.lower(): + raise UpdateError(f"the installer changed after it was downloaded " + f"({when}): {path}") + + def download(release: Release, directory: Path, *, opener: Callable[..., Any] = urllib.request.urlopen, progress: Callable[[int, int], None] | None = None) -> tuple[Path, str]: - """Stream the installer into `directory`. Returns its path and SHA-256. + """Stream the installer into `directory`. Returns the staged path and SHA-256. Hashed while streaming rather than re-read afterwards, so the digest describes the bytes that were actually written. + + The bytes go to `.offloader-partial`, created exclusively so an + existing file or a planted link at that name is never written through, and + flushed to disk before returning. They stay under that name until + `promote` is called after verification, so an interrupted or refused + download never leaves something under the installer's own name that a + person could double-click. Any failure deletes the partial file. """ _assert_allowed(release.download_url) - target = Path(directory) / release.asset_name + directory = Path(directory) + if is_link(directory): + raise UpdateError(f"refusing a download directory that is a link or " + f"junction: {directory}") + if not directory.is_dir(): + raise UpdateError(f"the download directory does not exist: {directory}") + final = directory / release.asset_name + staged = directory / (release.asset_name + PARTIAL_SUFFIX) + for path in (final, staged): + if is_link(path): + raise UpdateError(f"refusing to download over a link: {path}") + # A partial file of ours left by an interrupted earlier run. + discard(staged) + digest = hashlib.sha256() written = 0 - + total = 0 request = urllib.request.Request( release.download_url, headers={"User-Agent": f"Offloader/{__version__}"}, ) + created = False try: with opener(request, timeout=60) as response: # The redirect is where the host can change, so this is the check @@ -269,7 +396,10 @@ def download(release: Release, directory: Path, *, raise UpdateError( f"refusing a {total} byte download; the ceiling is " f"{MAX_DOWNLOAD_BYTES}") - with open(target, "wb") as handle: + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_BINARY", 0) + descriptor = os.open(staged, flags, 0o600) + created = True + with os.fdopen(descriptor, "wb") as handle: while True: block = response.read(1 << 20) if not block: @@ -282,22 +412,88 @@ def download(release: Release, directory: Path, *, handle.write(block) if progress is not None: progress(written, total) - except urllib.error.URLError as exc: - raise UpdateError(f"could not download the installer: {exc}") from exc + handle.flush() + os.fsync(handle.fileno()) + if written == 0: + raise UpdateError("the installer download was empty") + if total and written != total: + raise UpdateError(f"the download stopped at {written} of {total} " + f"bytes") + except BaseException as exc: + if created: + discard(staged) + if isinstance(exc, UpdateError): + raise + if isinstance(exc, urllib.error.URLError): + raise UpdateError( + f"could not download the installer: {exc}") from exc + if isinstance(exc, (OSError, http.client.HTTPException)): + raise UpdateError( + f"could not download the installer: {exc}") from exc + raise + return staged, digest.hexdigest() + + +def promote(staged: Path) -> Path: + """Give a verified download its installer name. Returns the final path. + + Called only after `verify` has accepted the staged bytes. Refuses to + replace a link at the final name; a plain file of that name (an earlier + download into the same `--dir`) is replaced atomically. + """ + staged = Path(staged) + if not staged.name.endswith(PARTIAL_SUFFIX): + raise UpdateError(f"not a staged download: {staged}") + final = staged.with_name(staged.name[:-len(PARTIAL_SUFFIX)]) + if is_link(staged) or is_link(final): + raise UpdateError(f"refusing to promote over a link: {final}") + try: + os.replace(staged, final) + except OSError as exc: + raise UpdateError(f"could not rename the verified download: {exc}") from exc + return final + - if written == 0: - raise UpdateError("the installer download was empty") - return target, digest.hexdigest() +def system_root() -> Path: + """The Windows directory, from `%SystemRoot%`, which must be absolute.""" + value = os.environ.get("SystemRoot") or "" + root = Path(value) + if not value or not root.is_absolute(): + raise UpdateError("cannot locate Windows: %SystemRoot% is not an " + "absolute path") + return root + + +def powershell_executable() -> Path: + """Windows PowerShell by absolute path, never by a search of PATH. + + A bare `powershell` is resolved by the Windows search order, which looks + in the application's directory and the current directory before System32. + The CLI is routinely run from a card's root, so a `powershell.exe` planted + there would otherwise both run and decide the signature verdict. + """ + return (system_root() / "System32" / "WindowsPowerShell" / "v1.0" + / "powershell.exe") def _powershell(script: str, *, + variables: dict[str, str] | None = None, run: Callable[..., subprocess.CompletedProcess[str]] | None = None ) -> dict[str, Any]: runner = run or subprocess.run - result = runner( - ["powershell", "-NoProfile", "-NonInteractive", "-Command", script], - capture_output=True, text=True, timeout=PROBE_TIMEOUT, check=False, - ) + environment = dict(os.environ) + environment.update(variables or {}) + try: + result = runner( + [str(powershell_executable()), "-NoProfile", "-NonInteractive", + "-ExecutionPolicy", "Bypass", "-Command", script], + capture_output=True, text=True, timeout=PROBE_TIMEOUT, check=False, + # Not the operator's current directory, which may be untrusted + # removable media. + cwd=str(system_root()), env=environment, + ) + except (OSError, subprocess.SubprocessError) as exc: + raise UpdateError(f"could not read the installer's signature: {exc}") from exc if result.returncode != 0: raise UpdateError("could not read the installer's signature") try: @@ -307,25 +503,39 @@ def _powershell(script: str, *, return data if isinstance(data, dict) else {} +#: Constant script text. The path arrives through `_PROBE_PATH_VARIABLE`, and +#: the modules the verdict depends on are loaded from `$PSHOME` explicitly and +#: called by qualified name, so a same-named module earlier on the user's +#: `PSModulePath` is not consulted. +_PROBE_SCRIPT = ( + "$ErrorActionPreference='Stop';" + "foreach($m in 'Microsoft.PowerShell.Security','Microsoft.PowerShell.Utility')" + "{Import-Module -Name (Join-Path $PSHOME ('Modules\\'+$m+'\\'+$m+'.psd1'))};" + f"$p=$env:{_PROBE_PATH_VARIABLE};" + "$s=Microsoft.PowerShell.Security\\Get-AuthenticodeSignature -LiteralPath $p;" + "$v=[System.Diagnostics.FileVersionInfo]::GetVersionInfo($p);" + "[pscustomobject]@{" + "Status=[string]$s.Status;" + "Thumbprint=[string]$s.SignerCertificate.Thumbprint;" + "Subject=[string]$s.SignerCertificate.Subject;" + "TimestampThumbprint=[string]$s.TimeStamperCertificate.Thumbprint;" + "FileVersion=[string]$v.FileVersion;" + "ProductVersion=[string]$v.ProductVersion" + "} | Microsoft.PowerShell.Utility\\ConvertTo-Json -Compress" +) + + def inspect_installer(path: Path, *, run: Callable[..., Any] | None = None ) -> dict[str, Any]: - """The downloaded file's Authenticode status and embedded version.""" - literal = str(Path(path)) - script = ( - "$ErrorActionPreference='Stop';" - f"$p='{literal}';" - "$s=Get-AuthenticodeSignature -LiteralPath $p;" - "$v=(Get-Item -LiteralPath $p).VersionInfo;" - "[pscustomobject]@{" - "Status=[string]$s.Status;" - "Thumbprint=[string]$s.SignerCertificate.Thumbprint;" - "Subject=[string]$s.SignerCertificate.Subject;" - "TimestampThumbprint=[string]$s.TimeStamperCertificate.Thumbprint;" - "FileVersion=[string]$v.FileVersion;" - "ProductVersion=[string]$v.ProductVersion" - "} | ConvertTo-Json -Compress" - ) - return _powershell(script, run=run) + """The downloaded file's Authenticode status and embedded version. + + The path is handed over in an environment variable rather than written + into the script, so an apostrophe, `$` or backtick in it is just a + character of the path. + """ + absolute = str(Path(path).absolute()) + return _powershell(_PROBE_SCRIPT, + variables={_PROBE_PATH_VARIABLE: absolute}, run=run) def _subject_name(subject: str) -> str: @@ -336,16 +546,18 @@ def _subject_name(subject: str) -> str: return "" + + def verify(path: Path, release: Release, *, run: Callable[..., Any] | None = None, - expected_digest: str | None = None, - actual_digest: str | None = None) -> dict[str, Any]: + expected_digest: str | None = None) -> dict[str, Any]: """Refuse the download unless everything about it agrees. Four separate questions, because each one fails on its own: - * the bytes are the ones that were hashed while streaming — closing the - window between writing the file and reading it back; + * the bytes on disk are the ones that were hashed while streaming. The + file is re-hashed before the signature is read and again afterwards, so + what Authenticode inspected is what was downloaded; * Windows trusts the signature; * it is *our* certificate and our name, not merely a valid one; * the version compiled into the executable is the version the release @@ -353,9 +565,8 @@ def verify(path: Path, release: Release, *, re-serve an older, still validly signed installer under a newer name and roll every install back onto a build whose bugs are already fixed. """ - if expected_digest is not None and actual_digest is not None: - if expected_digest.lower() != actual_digest.lower(): - raise UpdateError("the installer changed after it was downloaded") + if expected_digest is not None: + _assert_unchanged(path, expected_digest, "before its signature was read") record = inspect_installer(path, run=run) status = str(record.get("Status", "")) @@ -363,7 +574,9 @@ def verify(path: Path, release: Release, *, raise UpdateError(f"the installer's signature is {status or 'missing'}") thumbprint = str(record.get("Thumbprint", "")).replace(" ", "").upper() - if thumbprint != EXPECTED_CERT_THUMBPRINT: + accepted = {value.replace(" ", "").upper() + for value in EXPECTED_CERT_THUMBPRINTS} + if not thumbprint or thumbprint not in accepted: raise UpdateError("the installer was signed by a different certificate") signer = _subject_name(str(record.get("Subject", ""))) @@ -375,21 +588,71 @@ def verify(path: Path, release: Release, *, raise UpdateError( f"the installer reports version {embedded!r} but the release " f"claims {release.version!r}") + + if expected_digest is not None: + _assert_unchanged(path, expected_digest, "while its signature was read") return record def install_target() -> Path: - """Where an update should be installed. + """The installation this process belongs to, which an update replaces. + + Only an installed packaged build can update itself: a frozen executable + whose directory carries the installer's `.offloader-install.json` + manifest. Anything else is refused with a message naming what was found, + rather than guessing a directory: + + * a pip install or a source checkout is not what the installer replaces, + and guessing `Program Files` would install a second copy beside the + registered one (or into `Program Files (x86)` under a 32-bit Python); + * a portable bundle has no manifest, so the installer's maintenance step + would refuse that directory every time. + + The target is the running executable's directory rather than the + `InstallLocation` registry value: that key is under HKLM and only an + administrator can write it, but it records where the *last* install went, + which is not necessarily the copy that is running. Known limit: when this + directory equals the installer's compiled default, the installer cannot + tell `/D=` from no `/D=` and uses the registered location instead (see + `installer.nsi`'s `.onInit`). + """ + if not getattr(sys, "frozen", False): + raise UpdateError( + "this Offloader runs from a Python installation (pip or a source " + "checkout), not the installed Windows app, so it cannot install " + "the release itself. Update it the way it was installed, for " + "example `python -m pip install --upgrade` or `git pull`, or " + f"download the installer from {RELEASES_PAGE}") + from .installation import MANIFEST_NAME, InstallationError, _read_manifest + + directory = Path(sys.executable).resolve().parent + try: + manifest = _read_manifest(directory) + except (InstallationError, OSError): + manifest = None + if manifest is None: + raise UpdateError( + f"this Offloader is a portable bundle in {directory} (there is no " + f"readable {MANIFEST_NAME} beside it), so the installer cannot " + f"replace it. Download Offloader-Setup from {RELEASES_PAGE} and " + "run it, or replace the portable folder by hand") + return directory + - Computed from the running executable, never read from the registry. The - registry value is writable by anything running as the user, so trusting it - would let a planted key redirect an elevated silent install into a - directory of someone else's choosing. +def runs_from(target: Path) -> bool: + """Whether this process's executable lives inside `target`. + + Such a process cannot wait for the installer: it holds the installation's + shared lock and its own files open for as long as it runs, so the + installer's maintenance step would refuse, or fail to remove the old + executable after committing. """ - if getattr(sys, "frozen", False): - return Path(sys.executable).resolve().parent - base = os.environ.get("ProgramFiles") or r"C:\Program Files" - return Path(base) / "Offloader" + try: + executable = Path(sys.executable).resolve() + executable.relative_to(Path(target).resolve()) + except (OSError, ValueError): + return False + return True def install_command(installer: Path, target: Path) -> tuple[str, str]: @@ -403,31 +666,124 @@ def install_command(installer: Path, target: Path) -> tuple[str, str]: return str(installer), f"/S /D={target}" +class ShellResult(NamedTuple): + """What `ShellExecuteExW` reported. + + `error` is the Win32 error when the launch failed, otherwise 0. + `exit_code` is the installer's exit code, or None when it was not waited + for or no process handle came back. + """ + + error: int + exit_code: int | None + + +#: `ShellExecuteExW` failing because the UAC prompt was declined. +_ERROR_CANCELLED = 1223 + + +def _shell_execute_ex(verb: str, path: str, arguments: str, directory: str, + wait: bool) -> ShellResult: # pragma: no cover - Windows + """`ShellExecuteExW` with `SEE_MASK_NOCLOSEPROCESS`, optionally waiting.""" + import ctypes + from ctypes import wintypes + + class ShellExecuteInfo(ctypes.Structure): + _fields_ = [ + ("cbSize", wintypes.DWORD), ("fMask", wintypes.ULONG), + ("hwnd", wintypes.HWND), ("lpVerb", wintypes.LPCWSTR), + ("lpFile", wintypes.LPCWSTR), ("lpParameters", wintypes.LPCWSTR), + ("lpDirectory", wintypes.LPCWSTR), ("nShow", ctypes.c_int), + ("hInstApp", wintypes.HINSTANCE), ("lpIDList", ctypes.c_void_p), + ("lpClass", wintypes.LPCWSTR), ("hkeyClass", wintypes.HKEY), + ("dwHotKey", wintypes.DWORD), ("hIconOrMonitor", wintypes.HANDLE), + ("hProcess", wintypes.HANDLE), + ] + + see_mask_nocloseprocess, see_mask_noasync = 0x40, 0x100 + wait_object_0, wait_timeout = 0, 0x102 + shell32 = ctypes.WinDLL("shell32", use_last_error=True) + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + shell32.ShellExecuteExW.argtypes = [ctypes.POINTER(ShellExecuteInfo)] + shell32.ShellExecuteExW.restype = wintypes.BOOL + kernel32.WaitForSingleObject.argtypes = [wintypes.HANDLE, wintypes.DWORD] + kernel32.WaitForSingleObject.restype = wintypes.DWORD + kernel32.GetExitCodeProcess.argtypes = [wintypes.HANDLE, + ctypes.POINTER(wintypes.DWORD)] + kernel32.GetExitCodeProcess.restype = wintypes.BOOL + kernel32.CloseHandle.argtypes = [wintypes.HANDLE] + kernel32.CloseHandle.restype = wintypes.BOOL + + info = ShellExecuteInfo() + info.cbSize = ctypes.sizeof(info) + info.fMask = see_mask_nocloseprocess | see_mask_noasync + info.lpVerb, info.lpFile = verb, path + info.lpParameters, info.lpDirectory = arguments, directory + info.nShow = 1 + if not shell32.ShellExecuteExW(ctypes.byref(info)): + return ShellResult(ctypes.get_last_error() or -1, None) + if not info.hProcess: + return ShellResult(0, None) + try: + if not wait: + return ShellResult(0, None) + while True: + # Short slices rather than INFINITE so Ctrl+C is still delivered. + state = kernel32.WaitForSingleObject(info.hProcess, 500) + if state == wait_object_0: + break + if state != wait_timeout: + return ShellResult(0, None) + code = wintypes.DWORD() + if not kernel32.GetExitCodeProcess(info.hProcess, ctypes.byref(code)): + return ShellResult(0, None) + return ShellResult(0, int(code.value)) + finally: + kernel32.CloseHandle(info.hProcess) + + def apply(installer: Path, target: Path | None = None, *, - shell_execute: Callable[..., int] | None = None) -> None: + expected_digest: str, + wait: bool = True, + shell_execute: Callable[..., ShellResult] | None = None) -> bool: """Run the verified installer elevated, silently. - `ShellExecuteW` with `runas` rather than `subprocess`: the installer's + Returns True when the installer was waited for and exited 0, False when it + was started but its outcome could not be observed (`wait=False`, or no + process handle came back). Raises `UpdateError` when it could not start, + when the file changed since verification, or when it exited non-zero. + + The file is re-hashed immediately before launch and compared with the + digest that verification accepted; a mismatch refuses to run it. + + `ShellExecuteExW` with `runas` rather than `subprocess`: the installer's manifest asks for elevation, and only the shell honours that with a UAC - prompt — a plain spawn fails outright. + prompt; a plain spawn fails outright. It runs in the installer's own + directory, not the operator's current one. This does not close the running app, and must not. Offloader's installer - refuses maintenance while a transfer is in flight and exits non-zero - instead; that refusal is the guarantee, so the caller's job is to ask the - operator to finish, not to clear the way for it. + refuses maintenance while any Offloader program holds the installation + open and exits non-zero instead. Silent mode hides its own message, so + that exit code is the only report of the refusal, and it is surfaced here. """ + installer = Path(installer).absolute() destination = install_target() if target is None else Path(target) - path, arguments = install_command(Path(installer), destination) - - if shell_execute is None: # pragma: no cover - Windows - import ctypes + path, arguments = install_command(installer, destination) - shell_execute = ctypes.windll.shell32.ShellExecuteW + _assert_unchanged(installer, expected_digest, "before it was launched") - result = shell_execute(None, "runas", path, arguments, None, 1) - # ShellExecuteW returns a fake HINSTANCE; anything of 32 or less is an - # error code, and 5 is the one that matters — the UAC prompt was declined. - if int(result) <= 32: - if int(result) == 5: + launch = shell_execute or _shell_execute_ex + result = launch("runas", path, arguments, str(installer.parent), wait) + if result.error: + if result.error == _ERROR_CANCELLED: raise UpdateError("the update needs administrator approval") - raise UpdateError(f"could not start the installer (code {int(result)})") + raise UpdateError(f"could not start the installer (code {result.error})") + if result.exit_code is None: + return False + if result.exit_code != 0: + raise UpdateError( + f"the installer refused or failed (exit code {result.exit_code}). " + "Silent mode hides its own message; the usual cause is an " + "Offloader window or CLI transfer still running. Nothing was " + "force-closed; close Offloader and run the update again") + return True diff --git a/tests/test_update.py b/tests/test_update.py index 6fdabf3..493c23d 100644 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -11,19 +11,29 @@ from __future__ import annotations +import hashlib import importlib.util +import io import json +import os import subprocess import sys +import urllib.error from pathlib import Path import pytest -from offloader import update -from offloader.update import Release, UpdateError +from offloader import cli, update +from offloader.update import Release, ShellResult, UpdateError REPO = Path(__file__).resolve().parent.parent -GOOD_DIGEST = "a" * 64 + + +@pytest.fixture(autouse=True) +def _system_root(monkeypatch): + """An absolute `%SystemRoot%` on every platform. Nothing is ever run from + it: every probe in this file goes through an injected or patched seam.""" + monkeypatch.setenv("SystemRoot", str(REPO)) def _versioning(): @@ -35,6 +45,19 @@ def _versioning(): return module +def _signing(): + """`build/windows/sign.py`, loaded the same way.""" + path = REPO / "build" / "windows" / "sign.py" + spec = importlib.util.spec_from_file_location("_signing_for_update", path) + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + try: + spec.loader.exec_module(module) + finally: + sys.modules.pop(spec.name, None) + return module + + def _feed(tag: str, *, assets: list[dict] | None = None, body: str = "") -> dict: version = tag.lstrip("v") if assets is None: @@ -408,21 +431,38 @@ def test_a_correctly_signed_installer_verifies(tmp_path: Path): download_url="https://github.com/a/b") record = update.verify(path, release, run=_runner(_signature()), - expected_digest=GOOD_DIGEST, - actual_digest=GOOD_DIGEST) + expected_digest=hashlib.sha256(b"MZ").hexdigest()) assert record["Status"] == "Valid" def test_a_digest_that_changed_after_download_is_refused(tmp_path: Path): - """The window between writing the file and executing it. Anything local - that can write to the download directory could swap it in that gap.""" + """The file on disk is re-hashed, not the streamed digest compared with + itself. Anything local that can write to the download directory could + swap the bytes after they were hashed.""" path = _written(tmp_path) release = Release(version="0.2.0", asset_name=path.name, download_url="https://github.com/a/b") with pytest.raises(UpdateError, match="changed after"): update.verify(path, release, run=_runner(_signature()), - expected_digest=GOOD_DIGEST, actual_digest="b" * 64) + expected_digest="b" * 64) + + +def test_a_file_swapped_while_its_signature_was_read_is_refused(tmp_path: Path): + """REGRESSION. Bytes changed between the first hash and the signature + probe are what Authenticode inspected; the second hash catches that.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + honest = _runner(_signature()) + + def swap_then_answer(command, **kwargs): + path.write_bytes(b"MZ swapped") + return honest(command, **kwargs) + + with pytest.raises(UpdateError, match="while its signature was read"): + update.verify(path, release, run=swap_then_answer, + expected_digest=hashlib.sha256(b"MZ").hexdigest()) @pytest.mark.parametrize("status", ["NotSigned", "HashMismatch", @@ -482,14 +522,23 @@ def test_an_installer_reporting_another_version_is_refused(tmp_path: Path): run=_runner(_signature(FileVersion="0.1.0"))) -def test_a_trailing_windows_build_field_still_matches(tmp_path: Path): - """The installer's embedded FileVersion carries a fourth field the release - version does not have.""" +def test_the_embedded_version_string_is_compared_as_the_release_version( + tmp_path: Path): + """`installer.nsi` writes the release version itself (`0.2.0`, `0.1.0b1`) + into the FileVersion string; the four-field Windows number lives in + `VIProductVersion` and is not what is read. So the string matches the + release exactly, a `+local` suffix is ignored, and a four-field value is + refused rather than guessed at.""" path = _written(tmp_path) release = Release(version="0.2.0", asset_name=path.name, download_url="https://github.com/a/b") update.verify(path, release, run=_runner(_signature(FileVersion="0.2.0"))) + update.verify(path, release, + run=_runner(_signature(FileVersion="0.2.0+build7"))) + with pytest.raises(UpdateError, match="0.2.0.0"): + update.verify(path, release, + run=_runner(_signature(FileVersion="0.2.0.0"))) def test_an_unreadable_signature_probe_is_a_failure(tmp_path: Path): @@ -501,36 +550,280 @@ def test_an_unreadable_signature_probe_is_a_failure(tmp_path: Path): update.verify(path, release, run=_runner({}, returncode=1)) + + +def test_a_release_signed_by_an_announced_next_certificate_verifies( + tmp_path: Path, monkeypatch): + """Rotation. A build that lists the renewed certificate ahead of time + keeps taking releases once they are signed with it; the publisher name is + still required of every accepted certificate.""" + path = _written(tmp_path) + release = Release(version="0.2.0", asset_name=path.name, + download_url="https://github.com/a/b") + following = "D" * 40 + monkeypatch.setattr(update, "EXPECTED_CERT_THUMBPRINTS", + (update.EXPECTED_CERT_THUMBPRINT, following)) + + update.verify(path, release, run=_runner(_signature(Thumbprint=following))) + with pytest.raises(UpdateError, match="publisher"): + update.verify(path, release, run=_runner(_signature( + Thumbprint=following, Subject="CN=Someone Else Ltd."))) + with pytest.raises(UpdateError, match="different certificate"): + update.verify(path, release, run=_runner(_signature(Thumbprint=""))) + + +def test_the_pinned_certificate_is_the_one_releases_are_signed_with(): + """The values are repeated from the build script rather than imported, so + nothing else would notice them drifting apart.""" + signing = _signing() + assert update.EXPECTED_CERT_THUMBPRINTS[0] == signing.DEFAULT_CERT_THUMBPRINT + assert update.EXPECTED_CERT_THUMBPRINT == signing.DEFAULT_CERT_THUMBPRINT + assert update.EXPECTED_SIGNER_NAME == signing.EXPECTED_CERTIFICATE_NAME + + +# ------------------------------------------------------------ signature probe + + +def _capturing_runner(seen: list[tuple[list[str], dict]], record: dict | None = None): + def run(command, **kwargs): + seen.append((list(command), kwargs)) + return subprocess.CompletedProcess( + command, 0, stdout=json.dumps(record or _signature()), stderr="") + return run + + +def test_powershell_is_run_by_absolute_path_from_a_trusted_directory( + tmp_path: Path, monkeypatch): + """REGRESSION. A bare `powershell` is found by the Windows search order, + which tries the current directory before System32; the CLI is routinely + run from a card's root, where a planted `powershell.exe` would both run + and decide the signature verdict.""" + windows = tmp_path / "Windows" + card = tmp_path / "card" + card.mkdir() + monkeypatch.setenv("SystemRoot", str(windows)) + monkeypatch.chdir(card) + seen: list[tuple[list[str], dict]] = [] + + update.inspect_installer(_written(tmp_path), run=_capturing_runner(seen)) + + command, kwargs = seen[0] + expected = windows / "System32" / "WindowsPowerShell" / "v1.0" / "powershell.exe" + assert command[0] == str(expected) + assert Path(command[0]).is_absolute() + assert command[1:5] == ["-NoProfile", "-NonInteractive", + "-ExecutionPolicy", "Bypass"] + assert kwargs["cwd"] == str(windows) + assert Path(kwargs["cwd"]).resolve() != card.resolve() + + +@pytest.mark.parametrize("value", [None, "", "Windows", r"\Windows"]) +def test_an_unusable_system_root_is_refused_before_anything_runs( + tmp_path: Path, monkeypatch, value): + if value is None: + monkeypatch.delenv("SystemRoot", raising=False) + else: + monkeypatch.setenv("SystemRoot", value) + seen: list[tuple[list[str], dict]] = [] + + with pytest.raises(UpdateError, match="SystemRoot"): + update.inspect_installer(_written(tmp_path), run=_capturing_runner(seen)) + assert seen == [] + + +def test_the_installer_path_never_becomes_script_text(tmp_path: Path): + """REGRESSION. The path used to be pasted into a single-quoted literal, so + an apostrophe ended it (a profile named O'Brien broke every update) and a + crafted directory name became PowerShell in the probe whose output is the + trust verdict.""" + hostile = tmp_path / "O'Brien $profile `n" + hostile.mkdir() + path = hostile / "Offloader-Setup-0.2.0.exe" + path.write_bytes(b"MZ") + seen: list[tuple[list[str], dict]] = [] + + update.inspect_installer(path, run=_capturing_runner(seen)) + + command, kwargs = seen[0] + assert all("O'Brien" not in part for part in command) + assert kwargs["env"]["OFFLOADER_UPDATE_INSTALLER"] == str(path.absolute()) + assert "$env:OFFLOADER_UPDATE_INSTALLER" in command[-1] + + +def test_a_probe_that_cannot_start_is_a_failure(tmp_path: Path): + def missing(_command, **_kwargs): + raise FileNotFoundError("powershell.exe") + + with pytest.raises(UpdateError, match="signature"): + update.inspect_installer(_written(tmp_path), run=missing) + + +# --------------------------------------------------------- download hygiene + + +def _release() -> Release: + return Release(version="0.2.0", asset_name="Offloader-Setup-0.2.0.exe", + download_url="https://github.com/a/b/setup.exe") + + +def _link_dir(link: Path, target: Path) -> None: + """A directory symlink where the platform allows one, else a junction.""" + try: + os.symlink(target, link, target_is_directory=True) + return + except (OSError, NotImplementedError): + pass + if sys.platform == "win32": + import _winapi + + _winapi.CreateJunction(str(target), str(link)) + return + pytest.skip("cannot create a directory link here") + + +def test_the_download_is_staged_until_it_is_promoted(tmp_path: Path): + """Nothing carries the installer's own name until verification passed.""" + body = b"installer bytes" + staged, _digest = update.download( + _release(), tmp_path, + opener=_opener(body, url="https://github.com/a/b/setup.exe")) + + assert staged.name == "Offloader-Setup-0.2.0.exe" + update.PARTIAL_SUFFIX + assert not (tmp_path / "Offloader-Setup-0.2.0.exe").exists() + + final = update.promote(staged) + assert final == tmp_path / "Offloader-Setup-0.2.0.exe" + assert final.read_bytes() == body + assert not staged.exists() + + +def test_a_partial_download_leaves_no_file_under_the_installer_name( + tmp_path: Path, monkeypatch): + """REGRESSION. The bytes went straight to `Offloader-Setup-X.exe`, so a + refused or interrupted download left a plausible installer behind.""" + monkeypatch.setattr(update, "MAX_DOWNLOAD_BYTES", 1024) + opener = _opener(b"x" * 4096, url="https://github.com/a/b/setup.exe") + + with pytest.raises(UpdateError, match="ceiling"): + update.download(_release(), tmp_path, opener=opener) + assert list(tmp_path.iterdir()) == [] + + +def test_a_body_shorter_than_its_content_length_is_refused(tmp_path: Path): + opener = _opener(b"MZ" * 5, url="https://github.com/a/b/setup.exe", + length="100") + + with pytest.raises(UpdateError, match="stopped at 10 of 100"): + update.download(_release(), tmp_path, opener=opener) + assert list(tmp_path.iterdir()) == [] + + +def test_a_connection_lost_mid_stream_is_an_update_error(tmp_path: Path): + """A socket error mid-read used to escape as a bare OSError or traceback, + leaving the partial file under the final name.""" + class Dropping(_Response): + def read(self, size: int = -1) -> bytes: + if self._at: + raise ConnectionResetError("reset by peer") + return super().read(4) + + def opener(_request, timeout=None): + return Dropping(b"MZ" * 100, url="https://github.com/a/b/setup.exe") + + with pytest.raises(UpdateError, match="reset by peer"): + update.download(_release(), tmp_path, opener=opener) + assert list(tmp_path.iterdir()) == [] + + +def test_a_stale_partial_from_an_earlier_run_is_replaced(tmp_path: Path): + stale = tmp_path / ("Offloader-Setup-0.2.0.exe" + update.PARTIAL_SUFFIX) + stale.write_bytes(b"old interrupted bytes") + + staged, _digest = update.download( + _release(), tmp_path, + opener=_opener(b"new", url="https://github.com/a/b/setup.exe")) + assert staged.read_bytes() == b"new" + + +def test_a_linked_download_directory_is_refused(tmp_path: Path): + """A shared `--dir` that is really a link would let whoever owns its + target choose where the installer is written.""" + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + _link_dir(link, real) + + with pytest.raises(UpdateError, match="link or junction"): + update.download(_release(), link, opener=_opener( + b"MZ", url="https://github.com/a/b/setup.exe")) + assert list(real.iterdir()) == [] + + +def test_a_link_at_the_installer_name_is_refused(tmp_path: Path): + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + downloads = tmp_path / "downloads" + downloads.mkdir() + _link_dir(downloads / "Offloader-Setup-0.2.0.exe", elsewhere) + + with pytest.raises(UpdateError, match="link"): + update.download(_release(), downloads, opener=_opener( + b"MZ", url="https://github.com/a/b/setup.exe")) + + # ------------------------------------------------------------------ applying -def test_the_target_is_computed_not_read_from_the_registry(monkeypatch, - tmp_path: Path): - """The uninstall key is writable by anything running as the user, so a - planted value would redirect an elevated silent install. +def _install_dir(tmp_path: Path, *, manifest: bool = True) -> tuple[Path, Path]: + """An installed copy as the installer leaves it, or a portable bundle.""" + installed = tmp_path / "Offloader" + installed.mkdir() + executable = installed / "offloader-cli.exe" + executable.write_bytes(b"MZ") + if manifest: + (installed / ".offloader-install.json").write_text(json.dumps({ + "format": 1, + "files": {"offloader-cli.exe": {"sha256": "0" * 64, "size": 2}}, + }), encoding="utf-8") + return installed, executable + + +def test_the_target_is_the_running_installation(monkeypatch, tmp_path: Path): + """Computed from the running executable rather than the registry, and + only for a frozen build the installer put there. Uses a real directory rather than a literal Windows path: `install_target` resolves the running executable, and a backslash is not a separator off Windows, so a hardcoded `C:\\...` resolves against the working directory and the assertion would only hold on one platform. """ - installed = tmp_path / "Offloader" - installed.mkdir() - executable = installed / "Offloader.exe" - executable.write_bytes(b"MZ") + installed, executable = _install_dir(tmp_path) monkeypatch.setattr(sys, "frozen", True, raising=False) monkeypatch.setattr(sys, "executable", str(executable)) assert update.install_target() == installed.resolve() -def test_an_unfrozen_checkout_falls_back_to_program_files(monkeypatch): - """Not a path a release takes, but it must not resolve to the source tree - and offer to install over it.""" +def test_a_pip_or_source_install_is_refused(monkeypatch): + """REGRESSION. It used to fall back to `%ProgramFiles%\\Offloader`, which + installs a second copy beside the registered one (or under Program Files + (x86) for a 32-bit Python) and compares the pip package's version with a + build it is not.""" monkeypatch.setattr(sys, "frozen", False, raising=False) - monkeypatch.setenv("ProgramFiles", r"C:\Program Files") - assert update.install_target() == Path(r"C:\Program Files") / "Offloader" + with pytest.raises(UpdateError, match="Python installation"): + update.install_target() + + +def test_a_portable_bundle_is_refused(monkeypatch, tmp_path: Path): + """REGRESSION. Without the installer's manifest the maintenance step + refuses the directory every time, silently under `/S`.""" + installed, executable = _install_dir(tmp_path, manifest=False) + monkeypatch.setattr(sys, "frozen", True, raising=False) + monkeypatch.setattr(sys, "executable", str(executable)) + + with pytest.raises(UpdateError, match="portable bundle"): + update.install_target() def test_the_install_directory_argument_is_last_and_unquoted(): @@ -546,31 +839,352 @@ def test_the_install_directory_argument_is_last_and_unquoted(): assert '"' not in arguments +class _Shell: + """Stands in for `ShellExecuteExW`; nothing is ever launched.""" + + def __init__(self, result: ShellResult | None = None): + self.result = result or ShellResult(0, 0) + self.calls: list[tuple[str, str, str, str, bool]] = [] + + def __call__(self, verb, path, arguments, directory, wait): + self.calls.append((verb, path, arguments, directory, wait)) + return self.result + + +def _installer(tmp_path: Path) -> tuple[Path, str]: + path = tmp_path / "Offloader-Setup-0.2.0.exe" + path.write_bytes(b"MZ installer") + return path, hashlib.sha256(b"MZ installer").hexdigest() + + def test_applying_elevates_with_the_runas_verb(tmp_path: Path): """A plain spawn does not honour the installer's manifest, so it fails instead of prompting.""" - calls: list[tuple] = [] + installer, digest = _installer(tmp_path) + shell = _Shell() + + assert update.apply(installer, Path(r"C:\Program Files\Offloader"), + expected_digest=digest, shell_execute=shell) is True + + verb, path, arguments, _directory, wait = shell.calls[0] + assert verb == "runas" + assert path == str(installer.absolute()) + assert arguments == r"/S /D=C:\Program Files\Offloader" + assert wait is True + - def shell_execute(handle, verb, path, arguments, directory, show): - calls.append((handle, verb, path, arguments, directory, show)) - return 42 +def test_the_installer_runs_in_its_own_directory(tmp_path: Path, monkeypatch): + """REGRESSION. `lpDirectory` was None, so the elevated installer inherited + the operator's current directory, which may be a card.""" + card = tmp_path / "card" + card.mkdir() + monkeypatch.chdir(card) + installer, digest = _installer(tmp_path) + shell = _Shell() - update.apply(tmp_path / "setup.exe", Path(r"C:\Program Files\Offloader"), - shell_execute=shell_execute) + update.apply(installer, Path(r"C:\x"), expected_digest=digest, + shell_execute=shell) - assert calls[0][1] == "runas" - assert calls[0][3] == r"/S /D=C:\Program Files\Offloader" + directory = shell.calls[0][3] + assert directory == str(installer.parent.absolute()) + assert Path(directory).resolve() != card.resolve() + + +def test_an_installer_that_refuses_is_reported_as_a_failure(tmp_path: Path): + """REGRESSION. The launch returned as soon as the process existed, so a + silent installer refusing because Offloader was still running (exit 1, + its message box suppressed by `/S`) was reported as success.""" + installer, digest = _installer(tmp_path) + + with pytest.raises(UpdateError, match=r"refused or failed \(exit code 1\)"): + update.apply(installer, Path(r"C:\x"), expected_digest=digest, + shell_execute=_Shell(ShellResult(0, 1))) + + +def test_an_installer_not_waited_for_is_not_reported_as_success(tmp_path: Path): + installer, digest = _installer(tmp_path) + shell = _Shell(ShellResult(0, None)) + + assert update.apply(installer, Path(r"C:\x"), expected_digest=digest, + wait=False, shell_execute=shell) is False + assert shell.calls[0][4] is False + + +def test_a_file_changed_after_verification_is_not_launched(tmp_path: Path): + """REGRESSION. The digest was compared with itself; the file was never + re-read before it was elevated.""" + installer, digest = _installer(tmp_path) + installer.write_bytes(b"MZ something else") + shell = _Shell() + + with pytest.raises(UpdateError, match="Offloader-Setup-0.2.0.exe"): + update.apply(installer, Path(r"C:\x"), expected_digest=digest, + shell_execute=shell) + assert shell.calls == [] def test_a_declined_elevation_prompt_is_reported_as_such(tmp_path: Path): """Cancelling the UAC prompt is the most likely outcome of all, and it is not a broken download.""" + installer, digest = _installer(tmp_path) with pytest.raises(UpdateError, match="administrator"): - update.apply(tmp_path / "setup.exe", Path(r"C:\x"), - shell_execute=lambda *_: 5) + update.apply(installer, Path(r"C:\x"), expected_digest=digest, + shell_execute=_Shell(ShellResult(1223, None))) def test_a_failure_to_start_the_installer_is_reported(tmp_path: Path): + installer, digest = _installer(tmp_path) with pytest.raises(UpdateError, match="code 2"): - update.apply(tmp_path / "setup.exe", Path(r"C:\x"), - shell_execute=lambda *_: 2) + update.apply(installer, Path(r"C:\x"), expected_digest=digest, + shell_execute=_Shell(ShellResult(2, None))) + + +# ------------------------------------------------------ the update command + + +NEXT = "99.0.0" +NEXT_ASSET = f"Offloader-Setup-{NEXT}.exe" +NEXT_BYTES = b"MZ the next installer" + + +class _Flow: + """`offloader update --install` with every outside effect replaced. + + The feed goes through the real `_fetch_json` with `urlopen` patched, the + download through the real `download` with an injected opener, and the + signature probe through the real `_powershell` with `subprocess.run` + patched. `ShellExecuteExW` is a recorder. Nothing reaches the network, + PowerShell, the registry or UAC. + """ + + def __init__(self, monkeypatch, tmp_path: Path): + self.tmp_path = tmp_path + self.temp = tmp_path / "temp" + self.temp.mkdir() + self.target, self.executable = _install_dir(tmp_path) + self.shell = _Shell() + self.probes: list[list[str]] = [] + self.downloads = 0 + + import tempfile + + monkeypatch.setattr(tempfile, "tempdir", str(self.temp)) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setattr(update, "install_target", lambda: self.target) + monkeypatch.setattr(update, "_shell_execute_ex", self.shell) + + feed = json.dumps([_feed(f"v{NEXT}")]).encode() + monkeypatch.setattr( + update.urllib.request, "urlopen", + lambda _request, timeout=None: _Response( + feed, url="https://api.github.com/repos/owenpkent/offloader/releases")) + + real_download = update.download + + def download(release, directory, **kwargs): + self.downloads += 1 + return real_download(release, directory, opener=_opener( + NEXT_BYTES, url="https://objects.githubusercontent.com/x", + length=str(len(NEXT_BYTES))), **kwargs) + + monkeypatch.setattr(update, "download", download) + + def run(command, **kwargs): + self.probes.append(list(command)) + return subprocess.CompletedProcess( + command, 0, stdout=json.dumps(_signature(FileVersion=NEXT)), + stderr="") + + monkeypatch.setattr(subprocess, "run", run) + + def main(self, *extra: str) -> int: + return cli.main(["update", "--install", *extra]) + + +@pytest.fixture +def flow(monkeypatch, tmp_path: Path) -> _Flow: + return _Flow(monkeypatch, tmp_path) + + +def test_cmd_update_installs_a_verified_release_end_to_end(flow: _Flow, capsys): + status = flow.main() + out = capsys.readouterr().out + + assert status == 0 + assert f"verified {NEXT_ASSET}" in out + assert hashlib.sha256(NEXT_BYTES).hexdigest() in out + assert f"Offloader {NEXT} was installed." in out + assert Path(flow.probes[0][0]).is_absolute() + verb, path, arguments, directory, wait = flow.shell.calls[0] + assert verb == "runas" + assert Path(path).name == NEXT_ASSET + assert directory == str(Path(path).parent) + assert arguments == f"/S /D={flow.target}" + assert wait is True + # The private temporary directory is removed once the installer is done. + assert list(flow.temp.iterdir()) == [] + + +def test_cmd_update_reports_an_installer_refusal(flow: _Flow, capsys): + """REGRESSION. A refused silent install used to exit 0.""" + flow.shell.result = ShellResult(0, 1) + + status = flow.main() + captured = capsys.readouterr() + + assert status == 2 + assert "refused or failed" in captured.err + assert "was installed" not in captured.out + assert list(flow.temp.iterdir()) == [] + + +def test_cmd_update_from_inside_the_installation_does_not_claim_success( + flow: _Flow, monkeypatch, capsys): + """The installed CLI holds its own installation open, so it cannot wait + for the installer and must say so rather than report an install.""" + monkeypatch.setattr(sys, "executable", str(flow.executable)) + flow.shell.result = ShellResult(0, None) + + status = flow.main() + out = capsys.readouterr().out + + assert status == 6 + assert flow.shell.calls[0][4] is False + assert "cannot see the installer's result" in out + assert "was installed" not in out + + +def test_cmd_update_refuses_a_file_swapped_after_verification( + flow: _Flow, monkeypatch, capsys): + """REGRESSION. `verify` was given the streamed digest twice, so a file + replaced after hashing was elevated anyway.""" + real_promote = update.promote + + def promote_then_swap(staged): + final = real_promote(staged) + final.write_bytes(b"MZ not what was verified") + return final + + monkeypatch.setattr(update, "promote", promote_then_swap) + + status = flow.main("--dir", str(flow.tmp_path / "downloads")) + err = capsys.readouterr().err + + assert status == 2 + assert NEXT_ASSET in err and "before it was launched" in err + assert flow.shell.calls == [] + + +def test_cmd_update_refuses_a_linked_download_directory(flow: _Flow, capsys): + real = flow.tmp_path / "real" + real.mkdir() + link = flow.tmp_path / "link" + _link_dir(link, real) + + status = flow.main("--dir", str(link)) + + assert status == 2 + assert "link or junction" in capsys.readouterr().err + assert flow.downloads == 0 and flow.shell.calls == [] + + +def test_cmd_update_leaves_no_partial_after_a_refused_signature( + flow: _Flow, monkeypatch, capsys): + def unsigned(command, **_kwargs): + return subprocess.CompletedProcess( + command, 0, stdout=json.dumps(_signature(Status="NotSigned")), + stderr="") + + monkeypatch.setattr(subprocess, "run", unsigned) + downloads = flow.tmp_path / "downloads" + + status = flow.main("--dir", str(downloads)) + + assert status == 2 + assert "NotSigned" in capsys.readouterr().err + assert list(downloads.iterdir()) == [] + assert flow.shell.calls == [] + + +@pytest.mark.parametrize("frozen, manifest, message", [ + (False, True, "Python installation"), + (True, False, "portable bundle"), +]) +def test_cmd_update_refuses_a_build_that_cannot_update_itself( + monkeypatch, tmp_path: Path, capsys, frozen, manifest, message): + """REGRESSION. Detection runs before anything is downloaded, and says + which build it found.""" + shell = _Shell() + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setattr(update, "_shell_execute_ex", shell) + feed = json.dumps([_feed(f"v{NEXT}")]).encode() + monkeypatch.setattr( + update.urllib.request, "urlopen", + lambda _request, timeout=None: _Response(feed, url=update.FEED_URL)) + called: list[str] = [] + monkeypatch.setattr(update, "download", + lambda *a, **k: called.append("download")) + _installed, executable = _install_dir(tmp_path, manifest=manifest) + monkeypatch.setattr(sys, "frozen", frozen, raising=False) + monkeypatch.setattr(sys, "executable", str(executable)) + + status = cli.main(["update", "--install"]) + + assert status == 2 + assert message in capsys.readouterr().err + assert called == [] and shell.calls == [] + + +def _urlopen_raising(exc: BaseException): + def urlopen(_request, timeout=None): + raise exc + return urlopen + + +@pytest.mark.parametrize("urlopen, reason", [ + (_urlopen_raising(urllib.error.URLError("no route to host")), + "could not reach the release feed"), + (_urlopen_raising(urllib.error.HTTPError( + update.FEED_URL, 403, "rate limit exceeded", None, io.BytesIO(b""))), + "HTTP 403"), + (lambda _request, timeout=None: _Response( + b"captive portal", url=update.FEED_URL), + "not readable JSON"), + (lambda _request, timeout=None: _Response( + b"[]", url="https://evil.example/releases"), + "evil.example"), +], ids=["offline", "rate-limited", "bad-json", "redirected"]) +def test_a_feed_failure_is_not_reported_as_up_to_date(monkeypatch, capsys, + urlopen, reason): + """REGRESSION. Every one of these used to print "the newest release + available" and exit 0, the documented "up to date" status, so a broken + proxy or a network attacker could suppress updates indefinitely.""" + monkeypatch.setattr(update.urllib.request, "urlopen", urlopen) + + status = cli.main(["update"]) + captured = capsys.readouterr() + + assert status == 5 + assert status not in (0, 1) + assert "could not check for updates" in captured.err + assert reason in captured.err + assert "newest" not in captured.out + captured.err + assert "up to date" not in captured.out + captured.err + + +def test_an_empty_feed_is_still_up_to_date(monkeypatch, capsys): + monkeypatch.setattr(update.urllib.request, "urlopen", + lambda _request, timeout=None: _Response( + b"[]", url=update.FEED_URL)) + + assert cli.main(["update"]) == 0 + assert "newest release available" in capsys.readouterr().out + + +def test_find_release_raises_where_check_returns_none(): + def explode(_url): + raise urllib.error.URLError("offline") + + with pytest.raises(UpdateError, match="offline"): + update.find_release("0.1.0", fetch=explode) + assert update.check("0.1.0", fetch=explode) is None From 7a50fa8f1f49d3669515e2ab52964a9a432a5e5e Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:55:05 -0600 Subject: [PATCH 8/9] Update test counts --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8f99f81..7d48d0b 100644 --- a/README.md +++ b/README.md @@ -625,7 +625,7 @@ ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -899 tests at 84% line coverage. They cover formatting against the reference's +1006 tests at 84% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing, From 5ff82482c8bd7b187e7d80eaac03b3b13b9337ad Mon Sep 17 00:00:00 2001 From: owenpkent <20529132+owenpkent@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:28:50 -0600 Subject: [PATCH 9/9] Update test counts --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 792c5b9..8829a88 100644 --- a/README.md +++ b/README.md @@ -630,7 +630,7 @@ ruff check src tests pytest --cov=offloader --cov-report=term-missing ``` -1006 tests at 84% line coverage. They cover formatting against the reference's +1025 tests at 84% line coverage. They cover formatting against the reference's exact strings, checksum vectors and streaming equivalence, copy/verify behaviour including simulated destination corruption, pause/resume/cancel concurrency, retry discrimination, BRAW container parsing, ffprobe parsing,