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/4] 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/4] 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/4] 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/4] 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.