diff --git a/src/common/config_helper.py b/src/common/config_helper.py index 319af082..b3e5aeac 100644 --- a/src/common/config_helper.py +++ b/src/common/config_helper.py @@ -172,8 +172,9 @@ class ConfigHelper: # Recursively merge nested dictionaries merged[key] = self.merge_configs(merged[key], value) else: - # Override with new value - merged[key] = value + # Override with new value — deep-copied so mutating the + # merged result can't reach back into override_config. + merged[key] = copy.deepcopy(value) return merged diff --git a/src/config_manager.py b/src/config_manager.py index 9872be6c..453e73b1 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -402,10 +402,13 @@ class ConfigManager: tlist = target[key] for i, s_item in enumerate(value): if i >= len(tlist): + # Interpolate only config-side data here — nothing + # iterated out of the secrets dict (not even the key + # name) may reach the log. self.logger.warning( - "Secrets list for %r is longer than the config list " - "(%d > %d); ignoring the extra entries", - key, len(value), len(tlist)) + "A secrets list is longer than the config list it " + "parallels (config has %d item(s)); ignoring the " + "extra entries", len(tlist)) break if not s_item: continue # {} placeholder: item i has no secrets diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index e821bbc4..ceee88bf 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -797,10 +797,12 @@ class BasePlugin(ABC): self.logger.error("'enabled' must be a boolean") return False - # Check display_duration if present + # Check display_duration if present. bool is excluded explicitly: + # it's an int subclass, and get_display_duration rejects it too. if "display_duration" in self.config: duration = self.config["display_duration"] - if not isinstance(duration, (int, float)) or duration <= 0: + if (not isinstance(duration, (int, float)) + or isinstance(duration, bool) or duration <= 0): self.logger.error("'display_duration' must be a positive number") return False diff --git a/src/plugin_system/saved_repositories.py b/src/plugin_system/saved_repositories.py index 9635437a..c8da3c5b 100644 --- a/src/plugin_system/saved_repositories.py +++ b/src/plugin_system/saved_repositories.py @@ -6,6 +6,7 @@ Manages saved GitHub repository URLs for easy plugin discovery and installation. import json import logging +import os from pathlib import Path from typing import List, Dict, Optional @@ -43,18 +44,31 @@ class SavedRepositoriesManager: return [] def _save_repositories(self) -> bool: - """Save repositories to file.""" + """Save repositories to file atomically. + + Writes to a temp file in the same directory and os.replace()s it + over the target, so a failed write can never truncate or + half-overwrite an existing saved_repositories.json. + """ + tmp_path = self.config_path.with_suffix(self.config_path.suffix + '.tmp') try: # Ensure directory exists self.config_path.parent.mkdir(parents=True, exist_ok=True) - - with open(self.config_path, 'w') as f: + + with open(tmp_path, 'w') as f: json.dump(self.repositories, f, indent=2) - + f.flush() + os.fsync(f.fileno()) + os.replace(tmp_path, self.config_path) + self.logger.info(f"Saved {len(self.repositories)} repositories to {self.config_path}") return True except Exception as e: self.logger.error(f"Error saving repositories: {e}") + try: + tmp_path.unlink(missing_ok=True) + except OSError: + pass return False @staticmethod diff --git a/test/test_base_plugin_duration.py b/test/test_base_plugin_duration.py index ffe03153..af7fe976 100644 --- a/test/test_base_plugin_duration.py +++ b/test/test_base_plugin_duration.py @@ -122,3 +122,25 @@ class TestConfigFallback: def test_config_bool_uses_default(self): assert make_plugin({"display_duration": True}).get_display_duration() == 15.0 assert make_plugin({"display_duration": False}).get_display_duration() == 15.0 + + +class TestValidateConfigDuration: + # validate_config must agree with get_display_duration about what a + # valid duration is — a config it accepts must not then be rejected + # (or silently defaulted) when the duration is actually read. + + def test_positive_number_valid(self): + assert make_plugin({"display_duration": 20}).validate_config() is True + + def test_zero_and_negative_invalid(self): + assert make_plugin({"display_duration": 0}).validate_config() is False + assert make_plugin({"display_duration": -5}).validate_config() is False + + def test_bool_invalid(self): + # bool is an int subclass; True would otherwise pass as "positive + # number" here while get_display_duration rejects it. + assert make_plugin({"display_duration": True}).validate_config() is False + assert make_plugin({"display_duration": False}).validate_config() is False + + def test_missing_duration_valid(self): + assert make_plugin({}).validate_config() is True diff --git a/test/test_config_helper.py b/test/test_config_helper.py index c8e9ae85..4931fccc 100644 --- a/test/test_config_helper.py +++ b/test/test_config_helper.py @@ -139,6 +139,16 @@ class TestMergeConfigs: assert base == {'a': {'b': 1}} assert override == {'a': {'c': 2}} + def test_no_aliasing_of_override_values(self, helper): + # The non-recursive branch must deep-copy the override value too: + # mutating a merged-in list or dict must not reach back into + # override_config. + override = {'teams': ['A', 'B'], 'nested': {'x': [1]}} + merged = helper.merge_configs({}, override) + merged['teams'].append('C') + merged['nested']['x'].append(2) + assert override == {'teams': ['A', 'B'], 'nested': {'x': [1]}} + class TestValidateConfig: def test_no_schema_dict_is_valid(self, helper): diff --git a/test/test_config_manager_secrets.py b/test/test_config_manager_secrets.py index 83452bf0..f7070c9c 100644 --- a/test/test_config_manager_secrets.py +++ b/test/test_config_manager_secrets.py @@ -299,3 +299,37 @@ class TestArraySecretStripAndMerge: {"name": "a", "token": "s3cret-a"}, {"name": "b", "token": "s3cret-b"}, ] + + def test_whole_item_secret_list_never_leaks_values(self, tmp_path): + # When the ENTIRE array item is secret (schema marks both key[] + # and key[].field), separate_secrets stores the full item dicts in + # the secrets file. That shape also matches the parallel-list + # discriminator — which is safe: strip drops every leaf key that + # appears in the secret item, so only empty {} skeletons (item + # count, no values) can reach config.json, and merge-on-load + # restores the full items from those skeletons. + from src.web_interface.secret_helpers import ( + find_secret_fields, separate_secrets) + schema_props = {"accounts": { + "type": "array", + "items": {"type": "object", "x-secret": True, "properties": { + "id": {"type": "string"}, + "token": {"type": "string", "x-secret": True}, + }}, + }} + paths = find_secret_fields(schema_props) + assert paths == {"accounts[]", "accounts[].token"} + full = {"accounts": [{"id": "i1", "token": "s3cret-a"}, + {"id": "i2", "token": "s3cret-b"}]} + _, secrets = separate_secrets(full, paths) + assert secrets == full # whole items are secret + + manager = make_manager(tmp_path) + stripped = manager._strip_secrets_recursive(full, secrets) + assert stripped == {"accounts": [{}, {}]} + + raw = json.dumps(stripped) + assert "s3cret" not in raw and "i1" not in raw + + manager._deep_merge(stripped, secrets) + assert stripped == full # round trip restores the items diff --git a/test/test_saved_repositories.py b/test/test_saved_repositories.py index 66a069cf..39bed1e4 100644 --- a/test/test_saved_repositories.py +++ b/test/test_saved_repositories.py @@ -207,6 +207,23 @@ class TestSaveFailureRollback: # Disk still has the entry too — memory and disk stay in sync. assert len(json.loads(path.read_text())) == 1 + def test_failed_write_leaves_existing_file_intact(self, tmp_path, monkeypatch): + # The save is atomic (temp file + os.replace): a write that dies + # mid-serialization must neither truncate the existing file nor + # leave a stray .tmp behind. + path = tmp_path / "repos.json" + manager = make_manager(path) + manager.add("https://github.com/user/repo") # real save + before = path.read_text() + + def boom(*args, **kwargs): + raise OSError("disk full") + monkeypatch.setattr(json, "dump", boom) + assert manager.add("https://github.com/user/other") is False + + assert path.read_text() == before + assert list(tmp_path.glob("*.tmp")) == [] + class TestGetAllCopy: def test_get_all_is_shallow_copy(self, tmp_path):