From 206eca078e7ee30cf1ff7fee219d15ec4acfc3b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 00:44:33 +0000 Subject: [PATCH] fix: unify version comparison, refuse secret-leaking saves, reset skin strikes on card success Fixes the three suspected bugs this PR's characterization tests pinned, flipping those tests to assert the corrected behavior: - plugins/store: ONE shared update comparator. New compatibility.is_update_available() (PEP 440 via packaging) is now used by both the web UI's update badge (api_v3._is_plugin_update_available is a thin alias) and store_manager.update_plugin's reinstall decision. Previously update_plugin used raw string equality: 'v1.2.0' vs '1.2.0' triggered a full reinstall the UI called unnecessary, and a locally- ahead plugin (2.0.0 installed, registry 1.9.0) was silently DOWNGRADED. Now equivalent spellings skip the reinstall and locally-ahead versions are never downgraded; unparseable versions still reconcile by reinstalling from the registry. - config: save_config and save_config_atomic now refuse (ConfigError) when config_secrets.json exists but cannot be loaded. Both previously proceeded without stripping, writing the merged secrets into config.json in plaintext. The shared _load_secrets_for_save() helper raises with an actionable message instead; a missing secrets file is still fine (nothing to strip), and _migrate_config's catch-all keeps boot resilient. - skins: render_skin_card resets _skin_failures on both success paths (vegas card returned, or mode renderer handled), mirroring _render_game. Transient card failures no longer accumulate across a session until they permanently disable a working skin. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --- src/base_classes/sports/core.py | 5 + src/config_manager.py | 60 +++++++----- src/plugin_system/compatibility.py | 35 +++++++ src/plugin_system/store_manager.py | 13 ++- test/test_config_manager_secrets.py | 40 +++++--- test/test_skin_system.py | 42 +++++--- test/test_version_comparison_consistency.py | 102 ++++++++++---------- web_interface/blueprints/api_v3.py | 27 ++---- 8 files changed, 200 insertions(+), 124 deletions(-) diff --git a/src/base_classes/sports/core.py b/src/base_classes/sports/core.py index e43c6e45..3291a1bf 100644 --- a/src/base_classes/sports/core.py +++ b/src/base_classes/sports/core.py @@ -383,10 +383,15 @@ class SportsCore(ABC): ctx = skin_runtime.build_context(self, game, size=size) card = skin.render_vegas_card(ctx, dict(game)) if card is not None: + # A successful render clears accumulated strikes, mirroring + # _render_game — transient failures must not add up across + # the session and disable a working skin. + self._skin_failures = 0 return card ctx = skin_runtime.build_context(self, game, size=size) render = getattr(skin, f"render_{self.SKIN_MODE}") if render(ctx, dict(game)): + self._skin_failures = 0 return ctx.canvas except Exception: # Card failures count toward the same 3-strike session disable diff --git a/src/config_manager.py b/src/config_manager.py index 8a7edbb1..695ce119 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -106,18 +106,13 @@ class ConfigManager: Returns: SaveResult with status and details """ - # Load current secrets to preserve them - secrets_content = {} - if os.path.exists(self.secrets_path): - try: - with open(self.secrets_path, 'r') as f_secrets: - secrets_content = json.load(f_secrets) - except Exception as e: - self.logger.warning(f"Could not load secrets file {self.secrets_path} during save: {e}") - + # Load current secrets to preserve them (raises if unreadable — see + # _load_secrets_for_save) + secrets_content = self._load_secrets_for_save() + # Strip secrets from main config before saving config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content) - + # Use atomic manager to save atomic_mgr = self._get_atomic_manager() result = atomic_mgr.save_config_atomic( @@ -290,19 +285,40 @@ class ConfigManager: result[key] = value return result + def _load_secrets_for_save(self) -> Dict[str, Any]: + """Load config_secrets.json for stripping before a save. + + A missing secrets file is fine (nothing to strip). But a file that + EXISTS and cannot be read or parsed means stripping is impossible — + and the in-memory config being saved has secrets deep-merged into it, + so proceeding would write them into config.json in plaintext. That + was the historical behavior; it is now a hard refusal. The save + raises so the caller (and user) fixes the secrets file instead of + silently leaking its contents into the world-readable main config. + """ + if not os.path.exists(self.secrets_path): + return {} + try: + with open(self.secrets_path, 'r') as f_secrets: + return json.load(f_secrets) + except Exception as e: + error_msg = ( + f"Refusing to save config: secrets file {self.secrets_path} exists " + f"but could not be loaded ({e}). Saving without it would write " + f"merged secret values into config.json in plaintext. Fix or " + f"remove the secrets file, then retry." + ) + self.logger.error(error_msg) + raise ConfigError(error_msg, config_path=self.secrets_path) from e + def save_config(self, new_config_data: Dict[str, Any]) -> None: - """Save configuration to the main JSON file, stripping out secrets.""" - secrets_content = {} - if os.path.exists(self.secrets_path): - try: - with open(self.secrets_path, 'r') as f_secrets: - secrets_content = json.load(f_secrets) - except Exception as e: - self.logger.warning(f"Could not load secrets file {self.secrets_path} during save: {e}") - # Continue without stripping if secrets can't be loaded, or handle as critical error - # For now, we'll proceed cautiously and save the full new_config_data if secrets are unreadable - # to prevent accidental data loss if the secrets file is temporarily corrupt. - # A more robust approach might be to fail the save or use a cached version of secrets. + """Save configuration to the main JSON file, stripping out secrets. + + Raises ConfigError when the secrets file exists but cannot be loaded, + because stripping would be impossible and secrets would leak into + config.json. + """ + secrets_content = self._load_secrets_for_save() config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content) diff --git a/src/plugin_system/compatibility.py b/src/plugin_system/compatibility.py index 784a6b6a..f1da5749 100644 --- a/src/plugin_system/compatibility.py +++ b/src/plugin_system/compatibility.py @@ -180,6 +180,41 @@ def declared_min_version(manifest: Dict[str, Any]) -> Optional[str]: return None +def is_update_available(installed_version: str, latest_version: str) -> bool: + """Return True when the registry's ``latest_version`` is strictly newer + than the installed version. + + THE shared comparator for "should this plugin be updated?" — used by both + the web UI's update badge (`api_v3._is_plugin_update_available`) and the + store's `update_plugin` reinstall decision, so the two can never disagree. + + Uses PEP 440-aware comparison (``packaging``), which also normalizes + equivalent spellings: ``v1.2.0`` == ``1.2.0`` and ``1.2`` == ``1.2.0``, so + cosmetic differences never trigger a reinstall — and a locally modified + plugin whose version is *ahead* of the registry is never "updated" + (downgraded). If either version string can't be parsed the mismatch is + surfaced (True) so the user can reconcile, rather than silently hiding a + potential update. + """ + if not installed_version or not latest_version: + return False + if installed_version == latest_version: + return False + try: + from packaging.version import parse as _parse_version, InvalidVersion + except ImportError: + # packaging is a core dependency, but if it's somehow unavailable we + # can't compare semantically — surface the mismatch we already know + # exists (the two strings differ). + return True + try: + return _parse_version(latest_version) > _parse_version(installed_version) + except InvalidVersion: + # Unparseable version string: we can't tell direction, so surface the + # mismatch rather than silently hiding a potential update. + return True + + def check(manifest: Dict[str, Any], core_version: str) -> Tuple[bool, Optional[str]]: """Return ``(compatible, reason)``. diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index e13b3e5b..d610e3c9 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -2969,7 +2969,10 @@ class PluginStoreManager: remote_branch = plugin_info_remote.get('branch') or plugin_info_remote.get('default_branch') # Compare local manifest version against registry latest_version - # to avoid unnecessary reinstalls for monorepo plugins + # to avoid unnecessary reinstalls for monorepo plugins. Uses the + # same semantic comparator as the web UI's update badge, so + # equivalent spellings ("v1.2.0" vs "1.2.0") never trigger a + # reinstall and a locally-ahead version is never downgraded. try: local_manifest_path = plugin_path / "manifest.json" if local_manifest_path.exists(): @@ -2977,8 +2980,12 @@ class PluginStoreManager: local_manifest = json.load(f) local_version = local_manifest.get('version', '') remote_version = plugin_info_remote.get('latest_version', '') - if local_version and remote_version and local_version == remote_version: - self.logger.info(f"Plugin {plugin_id} already at latest version {local_version}") + from src.plugin_system.compatibility import is_update_available + if (local_version and remote_version + and not is_update_available(local_version, remote_version)): + self.logger.info( + f"Plugin {plugin_id} already at latest version " + f"(installed {local_version}, registry {remote_version})") return True except Exception as e: self.logger.debug(f"Could not compare versions for {plugin_id}: {e}") diff --git a/test/test_config_manager_secrets.py b/test/test_config_manager_secrets.py index ed3f4b4b..94c7a6a2 100644 --- a/test/test_config_manager_secrets.py +++ b/test/test_config_manager_secrets.py @@ -4,8 +4,9 @@ Tests for the ConfigManager secrets round-trip and the load_config fast path. The contract under test: config_secrets.json values are deep-merged INTO the in-memory config at load time, and stripped back OUT before anything is written to config.json — so secrets live in exactly one file on disk. This -suite pins that round-trip plus its known sharp edges (some marked as -SUSPECTED BUG and characterized rather than fixed). +suite pins that round-trip plus its sharp edges, including the guard that a +save REFUSES (ConfigError) when the secrets file exists but can't be loaded, +rather than leaking merged secrets into config.json in plaintext. Complements test_config_manager.py, which covers loading/migration/validation. """ @@ -16,6 +17,7 @@ import os import pytest from src.config_manager import ConfigManager +from src.exceptions import ConfigError def make_manager(tmp_path, config=None, secrets=None): @@ -99,13 +101,11 @@ class TestSaveStripsSecrets: on_disk = json.loads((tmp_path / "config.json").read_text()) assert on_disk == {"timezone": "UTC"} - def test_corrupt_secrets_file_writes_secrets_to_config_json(self, tmp_path): - # SUSPECTED BUG (characterized, not fixed): when the secrets file is - # corrupt (or otherwise unloadable) at save time, save_config proceeds without - # stripping — writing the merged secrets into config.json in - # plaintext. The code comments acknowledge the tradeoff (it prevents - # data loss); this test pins the behavior so any future change to it - # is deliberate. + def test_corrupt_secrets_file_refuses_save_no_plaintext_leak(self, tmp_path): + # Regression guard: when the secrets file exists but is corrupt at + # save time, stripping is impossible — the save must raise instead of + # writing the merged secrets into config.json in plaintext (the + # historical behavior). manager = make_manager( tmp_path, config={"weather": {"city": "Austin"}}, @@ -114,10 +114,28 @@ class TestSaveStripsSecrets: loaded = manager.load_config() (tmp_path / "config_secrets.json").write_text("{corrupt") - manager.save_config(loaded) + with pytest.raises(ConfigError): + manager.save_config(loaded) + + # On-disk config untouched: no secret leaked. + on_disk = json.loads((tmp_path / "config.json").read_text()) + assert "api_key" not in on_disk.get("weather", {}) + + def test_corrupt_secrets_file_refuses_atomic_save_too(self, tmp_path): + # Same refusal on the atomic save path, which shared the leak. + manager = make_manager( + tmp_path, + config={"weather": {"city": "Austin"}}, + secrets={"weather": {"api_key": "s3cret"}}, + ) + loaded = manager.load_config() + (tmp_path / "config_secrets.json").write_text("{corrupt") + + with pytest.raises(ConfigError): + manager.save_config_atomic(loaded) on_disk = json.loads((tmp_path / "config.json").read_text()) - assert on_disk["weather"].get("api_key") == "s3cret" # leaked + assert "api_key" not in on_disk.get("weather", {}) class TestLoadFastPath: diff --git a/test/test_skin_system.py b/test/test_skin_system.py index 4b1f9074..1a7138c2 100644 --- a/test/test_skin_system.py +++ b/test/test_skin_system.py @@ -455,10 +455,8 @@ class TestExampleSkin: class TestRenderSkinCard: """render_skin_card (vegas cards) shares _render_game's 3-strike counter. - The asymmetry pinned here: _render_game resets the counter on success, - render_skin_card does NOT — card successes never clear strikes, so - failures accumulated across card renders (however far apart) still - disable the skin for the session. + Both paths reset the counter on success — transient failures must not + accumulate across a session and disable a working skin. """ def _probe(self, skin): @@ -518,11 +516,10 @@ class TestRenderSkinCard: assert probe.builtin_calls == 1 assert BrokenCardSkin.calls == 3 # not consulted again - def test_card_success_does_not_reset_strikes(self): - """Characterized asymmetry: unlike _render_game (which resets the - counter on success, core.py _render_game), a successful card render - leaves accumulated strikes in place — 2 failures + N successes + 1 - failure still disables the skin.""" + def test_card_success_resets_strikes(self): + """A successful card render clears accumulated strikes (mirroring + _render_game) — 2 failures + a success + 1 failure leaves the skin + enabled with a single strike, instead of disabling it.""" card_img = Image.new("RGB", (96, 32), (0, 0, 255)) class FlakyCardSkin(ScoreboardSkin): @@ -540,17 +537,30 @@ class TestRenderSkinCard: assert probe._skin_failures == 2 FlakyCardSkin.fail = False - for _ in range(10): - assert probe.render_skin_card({}, (96, 32)) is card_img - assert probe._skin_failures == 2 # successes did NOT clear strikes + assert probe.render_skin_card({}, (96, 32)) is card_img + assert probe._skin_failures == 0 # success cleared the strikes FlakyCardSkin.fail = True probe.render_skin_card({}, (96, 32)) - assert probe._skin_failures == 3 - assert probe.render_skin_card({}, (96, 32)) is None # disabled + assert probe._skin_failures == 1 # counting from the reset state + FlakyCardSkin.fail = False + assert probe.render_skin_card({}, (96, 32)) is card_img # still enabled - def test_render_game_success_does_reset_strikes(self): - """The other half of the asymmetry, for contrast with the above.""" + def test_card_success_via_mode_renderer_also_resets_strikes(self): + """The fallthrough path (render_vegas_card None -> mode renderer + True) resets the counter as well.""" + class ModeOnlySkin(ScoreboardSkin): + def render_live(self, ctx, game): + ctx.draw.rectangle([0, 0, 5, 5], fill=(255, 0, 0)) + return True + + probe = self._probe(ModeOnlySkin({}, {})) + probe._skin_failures = 2 + assert probe.render_skin_card({}, (96, 32)) is not None + assert probe._skin_failures == 0 + + def test_render_game_success_also_resets_strikes(self): + """Same reset contract on the display path, for symmetry.""" class GoodSkin(ScoreboardSkin): def render_live(self, ctx, game): return True diff --git a/test/test_version_comparison_consistency.py b/test/test_version_comparison_consistency.py index 7c8fa4fb..0d7816fb 100644 --- a/test/test_version_comparison_consistency.py +++ b/test/test_version_comparison_consistency.py @@ -1,25 +1,17 @@ """ -Drift guard: the repo has FOUR version-comparison implementations, and they -do not agree. This file pins each one's answer on the same inputs so any -future change to one of them (or a fifth copy appearing) surfaces here. +Drift guard for version comparison. -The four: -1. src/plugin_system/compatibility.py — parse_semver / tuple comparison - (install gate). -2. web_interface/blueprints/api_v3.py — _is_plugin_update_available, uses - packaging.version (update badge in the UI). -3. src/plugin_system/store_manager.py — update_plugin's raw STRING EQUALITY - for monorepo plugins ("local_version == remote_version"). -4. src/skin_system/skin_runtime.py — _major, int(major) gate for the - skin API. +There is now ONE shared "should this plugin update?" comparator — +`src.plugin_system.compatibility.is_update_available` — used by both the web +UI's update badge (`api_v3._is_plugin_update_available`) and the store's +`update_plugin` reinstall decision, so the badge and the actual reinstall +can never disagree. (Historically the store used raw string equality, which +reinstalled over cosmetic differences like "v1.2.0" vs "1.2.0" and even +DOWNGRADED locally-ahead plugins; this file's tests killed that.) -SUSPECTED BUG (characterized here, not fixed): #3 disagrees with #2. For -"v1.2.0" vs "1.2.0" the UI says "no update available" while update_plugin -performs a full reinstall; for a locally-ahead plugin ("2.0.0" installed, -registry "1.9.0") the UI says no update but update_plugin DOWNGRADES via -reinstall. Unifying on one comparator is tracked follow-up work; when that -lands, the expectations in TestStoreManagerStringEquality flip and this -file is the reminder to update them deliberately. +Two other version parsers legitimately remain and are pinned here so they +don't drift: `compatibility.parse_semver` (the install-compatibility gate, +range-spec oriented) and `skin_runtime._major` (skin API major gate). """ import json @@ -28,44 +20,40 @@ from unittest.mock import patch import pytest from packaging.version import parse as pkg_parse -from src.plugin_system.compatibility import parse_semver +from src.plugin_system.compatibility import is_update_available, parse_semver from src.skin_system.skin_runtime import _major from src.plugin_system.store_manager import PluginStoreManager from web_interface.blueprints.api_v3 import _is_plugin_update_available -# (installed, registry) pairs and what each comparator concludes. +# (installed, registry) -> update available? CASES = [ - # pair parse_semver equal? api_v3 update? store equal-string? - (("1.2.0", "1.2.0"), True, False, True), - (("v1.2.0", "1.2.0"), True, False, False), - (("1.2", "1.2.0"), True, False, False), - (("1.2.0", "1.2.0-rc1"), True, False, False), - (("1.2.0", "1.3.0"), False, True, False), - (("2.0.0", "1.9.0"), False, False, False), + (("1.2.0", "1.2.0"), False), # identical + (("v1.2.0", "1.2.0"), False), # cosmetic v-prefix, semantically equal + (("1.2", "1.2.0"), False), # short form, semantically equal + (("1.2.0", "1.2.0-rc1"), False), # rc of same release is not newer + (("1.2.0", "1.3.0"), True), # registry genuinely newer + (("2.0.0", "1.9.0"), False), # locally ahead — never downgrade + (("abc.def", "1.0.0"), True), # unparseable — surface the mismatch + (("", "1.0.0"), False), # missing either side — nothing to do + (("1.0.0", ""), False), ] -class TestComparatorMatrix: - @pytest.mark.parametrize("pair,semver_equal,api_update,store_equal", CASES) - def test_parse_semver_equality(self, pair, semver_equal, api_update, store_equal): - a, b = pair - assert (parse_semver(a) == parse_semver(b)) is semver_equal - - @pytest.mark.parametrize("pair,semver_equal,api_update,store_equal", CASES) - def test_api_v3_update_available(self, pair, semver_equal, api_update, store_equal): +class TestSharedComparator: + @pytest.mark.parametrize("pair,expected", CASES) + def test_is_update_available(self, pair, expected): installed, latest = pair - assert _is_plugin_update_available(installed, latest) is api_update + assert is_update_available(installed, latest) is expected - @pytest.mark.parametrize("pair,semver_equal,api_update,store_equal", CASES) - def test_store_manager_string_equality(self, pair, semver_equal, api_update, store_equal): - # The literal comparison update_plugin performs at its - # "already at latest version" check. - a, b = pair - assert (a == b) is store_equal + @pytest.mark.parametrize("pair,expected", CASES) + def test_api_v3_helper_agrees(self, pair, expected): + # The UI badge helper must be a pure alias of the shared comparator. + installed, latest = pair + assert _is_plugin_update_available(installed, latest) is expected -class TestStoreManagerStringEquality: +class TestStoreManagerUsesSharedComparator: """Drive update_plugin's real code path to its version check.""" def _store(self, tmp_path, local_version, registry_version): @@ -98,18 +86,30 @@ class TestStoreManagerStringEquality: assert result is True reinstall.assert_not_called() - def test_v_prefix_triggers_reinstall_despite_semantic_equality(self, tmp_path): - # SUSPECTED BUG: packaging (and api_v3) treat these as equal; the - # string comparison does not, so the user gets a full reinstall. + def test_v_prefix_equivalent_skips_reinstall(self, tmp_path): + # "v1.2.0" == "1.2.0" semantically — no pointless reinstall. store, info = self._store(tmp_path, "v1.2.0", "1.2.0") result, reinstall = self._run_update(store, info) + assert result is True + reinstall.assert_not_called() + + def test_locally_ahead_version_is_never_downgraded(self, tmp_path): + # A plugin ahead of the registry (local dev build) must not be + # "updated" — that would be a downgrade. + store, info = self._store(tmp_path, "2.0.0", "1.9.0") + result, reinstall = self._run_update(store, info) + assert result is True + reinstall.assert_not_called() + + def test_registry_newer_triggers_reinstall(self, tmp_path): + store, info = self._store(tmp_path, "1.2.0", "1.3.0") + result, reinstall = self._run_update(store, info) reinstall.assert_called_once() assert result is True - def test_locally_ahead_version_triggers_downgrade_reinstall(self, tmp_path): - # SUSPECTED BUG: a plugin ahead of the registry (local dev build) is - # "updated" — i.e. downgraded — because inequality is the only test. - store, info = self._store(tmp_path, "2.0.0", "1.9.0") + def test_unparseable_version_surfaces_via_reinstall(self, tmp_path): + # Direction unknowable → reconcile by reinstalling from the registry. + store, info = self._store(tmp_path, "abc.def", "1.0.0") result, reinstall = self._run_update(store, info) reinstall.assert_called_once() assert result is True diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index aea7d592..95f7ddb1 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -124,28 +124,13 @@ def _is_plugin_update_available(installed_version: str, latest_version: str) -> """Return True when the registry's ``latest_version`` is strictly newer than the installed version. - Uses PEP 440 / semver-aware comparison so a locally modified plugin whose - version is *ahead* of the published registry is not flagged as needing an - update. If either version string can't be parsed, falls back to a plain - inequality check (any difference is surfaced so the user can reconcile). + Thin alias for the shared comparator in + `src.plugin_system.compatibility.is_update_available` — the store's + `update_plugin` uses the same function, so the UI badge and the actual + reinstall decision can never disagree. """ - if not installed_version or not latest_version: - return False - if installed_version == latest_version: - return False - try: - from packaging.version import parse as _parse_version, InvalidVersion - except ImportError: - # packaging is a core dependency, but if it's somehow unavailable we - # can't compare semantically — surface the mismatch we already know - # exists (the two strings differ). - return True - try: - return _parse_version(latest_version) > _parse_version(installed_version) - except InvalidVersion: - # Unparseable version string: we can't tell direction, so surface the - # mismatch rather than silently hiding a potential update. - return True + from src.plugin_system.compatibility import is_update_available + return is_update_available(installed_version, latest_version) def _ensure_cache_manager(): """Ensure cache manager is initialized."""