From 7c798d62a52ca891dd5f36eb21084587c8e5f0f5 Mon Sep 17 00:00:00 2001 From: mullinmax Date: Sat, 18 Jul 2026 20:17:40 +0000 Subject: [PATCH 1/2] Treat an empty password as valid, distinct from unset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some Vector boards have no password configured, which the firmware authenticates by signing with an empty-string HMAC key. The client conflated an empty password with "no password set": the Machine.password property coerced "" to the $VECTOR_PASSWORD fallback, and both the auth preflight and the HTTP transport rejected a falsy password outright, so an empty password could never be used. Distinguish None (unset — still falls back to the environment and raises AuthenticationRequiredError when nothing is configured) from "" (a valid empty password that is signed as such): - Machine.password returns an explicitly-set empty string instead of the env fallback; only None falls back. - Machine._preflight_auth and HttpTransport._send guard on `is None` rather than falsiness. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014Y49XvgGpmTHrYLDeTvmbA --- tests/test_http.py | 17 +++++++++++++++++ tests/test_machine.py | 17 +++++++++++++++++ warpedpinball/machine.py | 14 +++++++++++--- warpedpinball/transports/http.py | 4 +++- 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/tests/test_http.py b/tests/test_http.py index 7ed63b2..0c93ab6 100644 --- a/tests/test_http.py +++ b/tests/test_http.py @@ -77,6 +77,23 @@ def test_no_password_raises_before_any_request(): assert session.get_calls == [] # not even a challenge fetch +def test_empty_password_is_valid_and_signs(): + # A board with no password configured signs with an empty HMAC key; an + # empty string is a real password, not "no password". + session = FakeSession(challenge=CHALLENGE) + t = make_transport(session, password="") + body = {"id": 0} + t.request("/api/player/update", body=body, authenticated=True) + + assert session.get_calls == ["http://192.168.1.42" + auth.CHALLENGE_PATH] + req = session.requests[0] + body_str = json.dumps(body, separators=(",", ":")) + assert req["headers"][auth.CHALLENGE_HEADER] == CHALLENGE + assert req["headers"][auth.HMAC_HEADER] == auth.sign( + "", CHALLENGE, "/api/player/update", body_str + ) + + def test_expired_challenge_retried_once_with_fresh_challenge(): session = FakeSession() session.next_challenges = ["11" * 32, "22" * 32] diff --git a/tests/test_machine.py b/tests/test_machine.py index 4b50b9f..47db633 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -107,6 +107,23 @@ def test_no_preflight_error_when_transport_needs_no_password(): assert transport.calls == [("/api/settings/reboot", None, True)] +def test_empty_password_passes_preflight(): + # An empty string is a valid (empty) password, so authenticated routes work + # rather than raising AuthenticationRequiredError. + machine, transport = make_machine(password="") + machine.reboot() + assert transport.calls == [("/api/settings/reboot", None, True)] + assert transport.password == "" + + +def test_empty_password_not_overridden_by_env(monkeypatch): + # An explicit empty password wins over $VECTOR_PASSWORD; only an unset + # (None) password falls back to the environment. + monkeypatch.setenv("VECTOR_PASSWORD", "from-env") + machine, _ = make_machine(password="") + assert machine.password == "" + + def test_env_var_password_fallback(monkeypatch): monkeypatch.setenv("VECTOR_PASSWORD", "from-env") machine, transport = make_machine(password=None) diff --git a/warpedpinball/machine.py b/warpedpinball/machine.py index 60d0c67..8c113a0 100644 --- a/warpedpinball/machine.py +++ b/warpedpinball/machine.py @@ -83,8 +83,16 @@ def __repr__(self) -> str: @property def password(self) -> Optional[str]: - """Password for HMAC auth; falls back to $VECTOR_PASSWORD.""" - return self._password or os.environ.get(PASSWORD_ENV_VAR) + """Password for HMAC auth; falls back to $VECTOR_PASSWORD. + + An empty string is a valid password (some boards have no password + configured, which the firmware signs with an empty HMAC key) and is + kept distinct from ``None``, which means no password has been set and + triggers the ``$VECTOR_PASSWORD`` fallback. + """ + if self._password is not None: + return self._password + return os.environ.get(PASSWORD_ENV_VAR) @password.setter def password(self, value: Optional[str]) -> None: @@ -136,7 +144,7 @@ def _preflight_auth(self, path: str, authenticated: bool) -> None: if ( authenticated and self.transport.requires_password - and not self.password + and self.password is None ): raise AuthenticationRequiredError( f"Route {path!r} requires authentication but no password is set; " diff --git a/warpedpinball/transports/http.py b/warpedpinball/transports/http.py index beef20d..3806bd3 100644 --- a/warpedpinball/transports/http.py +++ b/warpedpinball/transports/http.py @@ -129,7 +129,9 @@ def _send( if body_str is not None: headers["Content-Type"] = "application/json" if authenticated: - if not self.password: + # An empty string is a valid (empty) password and is signed as + # such; only ``None`` means no password has been set. + if self.password is None: raise AuthenticationRequiredError( f"Route {path!r} requires authentication but no password is set" ) From bd23a717015a9c629d764b67ee317b1f6413af9c Mon Sep 17 00:00:00 2001 From: mullinmax Date: Sat, 18 Jul 2026 20:31:44 +0000 Subject: [PATCH 2/2] Require a version bump on every PR; bump to 0.2.2 The empty-password fix (#17) merged without a version bump, so main could ship a behavior change under the already-released 0.2.1. Add a CI gate that fails a pull request unless it raises __version__ above the base branch, and bump to 0.2.2 to cover the merged fix. - scripts/check_version_bump.py compares the head __version__ against the base branch's (read from git) and requires head > base; a missing base file (new package) passes. PEP 440 ordering via packaging when available, with a numeric-plus-suffix fallback. - ci.yml gains a version-bump job (pull_request only) that runs it against the PR base sha. - Unit tests cover the comparison logic. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014Y49XvgGpmTHrYLDeTvmbA --- .github/workflows/ci.yml | 17 +++++ scripts/check_version_bump.py | 126 +++++++++++++++++++++++++++++++ tests/test_check_version_bump.py | 39 ++++++++++ warpedpinball/__init__.py | 2 +- 4 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 scripts/check_version_bump.py create mode 100644 tests/test_check_version_bump.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0f0cebf..c78353c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,6 +16,23 @@ jobs: - name: Verify pyproject.toml and package version agree run: python scripts/check_version.py + version-bump: + # Every pull request must raise __version__ above the base branch, so no + # change merges without a version bump and main is always publishable. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: actions/setup-python@v6 + with: + python-version: "3.12" + - name: Ensure the version was bumped + run: | + git fetch --no-tags --depth=1 origin "${{ github.event.pull_request.base.sha }}" + python scripts/check_version_bump.py --base-ref "${{ github.event.pull_request.base.sha }}" + lint: runs-on: ubuntu-latest steps: diff --git a/scripts/check_version_bump.py b/scripts/check_version_bump.py new file mode 100644 index 0000000..bfd8546 --- /dev/null +++ b/scripts/check_version_bump.py @@ -0,0 +1,126 @@ +#!/usr/bin/env python3 +"""Ensure a pull request bumps the package version. + +The single source of truth for the version is ``warpedpinball/__init__.py`` +(``__version__``). This gate compares the version on the current checkout +("head") against the version on the base branch and fails unless head is +strictly greater, so no change ever merges without a version bump — which is +what keeps every release publishable straight from ``main``. + +The base version is read from git (``--base-ref``, default ``origin/main``); +if the base has no ``__init__.py`` yet (a brand-new package) the check passes. +Exit status is 0 when head > base (or the base is unavailable), 1 otherwise, so +it works as a CI gate. +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +INIT_REL = "warpedpinball/__init__.py" +INIT_PY = REPO_ROOT / INIT_REL + +_INIT_VERSION_RE = re.compile( + r"""^__version__\s*=\s*["']([^"']+)["']""", re.MULTILINE +) + + +def parse_version_text(text: str) -> str | None: + """Extract the ``__version__`` string from an ``__init__.py`` body.""" + m = _INIT_VERSION_RE.search(text) + return m.group(1) if m else None + + +def version_key(version: str) -> tuple: + """A comparable key for a simple ``X.Y.Z[...]`` version. + + Numeric release segments compare numerically; any trailing pre-release + (e.g. ``rc1`` in ``0.3.0rc1``) sorts *before* the same release without it, + matching PEP 440 ordering for the cases this project uses. Prefers + ``packaging`` when available for full correctness. + """ + try: + from packaging.version import Version + + return (0, Version(version)) + except Exception: + pass + # Lightweight fallback: split the leading numeric release from any suffix. + m = re.match(r"^(\d+(?:\.\d+)*)(.*)$", version.strip()) + if not m: + raise ValueError(f"Unrecognized version string: {version!r}") + release = tuple(int(p) for p in m.group(1).split(".")) + suffix = m.group(2) + # No suffix sorts after a pre-release suffix on the same release. + return (1, release, 1 if suffix == "" else 0, suffix) + + +def is_bump(base: str, head: str) -> bool: + """True when ``head`` is a strictly higher version than ``base``.""" + return version_key(head) > version_key(base) + + +def head_version() -> str: + text = INIT_PY.read_text(encoding="utf-8") + version = parse_version_text(text) + if version is None: + raise SystemExit(f"Could not find __version__ in {INIT_PY}") + return version + + +def base_version(base_ref: str) -> str | None: + """Read ``__version__`` from ``base_ref`` via git, or None if unavailable.""" + try: + text = subprocess.check_output( + ["git", "show", f"{base_ref}:{INIT_REL}"], + cwd=REPO_ROOT, + stderr=subprocess.DEVNULL, + text=True, + ) + except subprocess.CalledProcessError: + return None # base branch has no package file yet (new package) + return parse_version_text(text) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base-ref", + default="origin/main", + help="Git ref of the base branch to compare against (default origin/main).", + ) + args = parser.parse_args(argv) + + head = head_version() + base = base_version(args.base_ref) + + if base is None: + print( + f"Version bump check OK: no base version found at {args.base_ref}; " + f"head is {head}." + ) + return 0 + + if not is_bump(base, head): + print( + "Version bump check FAILED: this branch must raise the package " + "version.\n" + f" base ({args.base_ref}) = {base}\n" + f" head = {head}\n" + "Bump __version__ in warpedpinball/__init__.py above the base " + "version before merging.", + file=sys.stderr, + ) + return 1 + + print(f"Version bump check OK: {base} -> {head}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_check_version_bump.py b/tests/test_check_version_bump.py new file mode 100644 index 0000000..64fc514 --- /dev/null +++ b/tests/test_check_version_bump.py @@ -0,0 +1,39 @@ +"""Tests for scripts/check_version_bump.py version comparison logic.""" + +import importlib.util +from pathlib import Path + +import pytest + +_SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "check_version_bump.py" + + +def _load(): + spec = importlib.util.spec_from_file_location("check_version_bump", _SCRIPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +cvb = _load() + + +@pytest.mark.parametrize( + "base,head,expected", + [ + ("0.2.1", "0.2.2", True), + ("0.2.1", "0.3.0", True), + ("0.2.1", "1.0.0", True), + ("0.2.1", "0.2.1", False), # unchanged: no bump + ("0.2.2", "0.2.1", False), # downgrade + ("0.2.0rc1", "0.2.0", True), # release supersedes its pre-release + ("0.2.0", "0.2.0rc1", False), # pre-release does not supersede release + ], +) +def test_is_bump(base, head, expected): + assert cvb.is_bump(base, head) is expected + + +def test_parse_version_text(): + assert cvb.parse_version_text('__version__ = "1.2.3"\n') == "1.2.3" + assert cvb.parse_version_text("x = 1\n") is None diff --git a/warpedpinball/__init__.py b/warpedpinball/__init__.py index 176d13a..f2efd58 100644 --- a/warpedpinball/__init__.py +++ b/warpedpinball/__init__.py @@ -32,7 +32,7 @@ from .machine import GameEvent, Machine from .transports.http import HttpTransport -__version__ = "0.2.1" +__version__ = "0.2.2" __all__ = [ "connect",