fix: address CodeQL alert and review findings

- config_manager: the "secrets list longer than config list" warning now
  interpolates only config-side data (no key name or secrets-derived
  values), resolving the CodeQL clear-text-logging alert.
- base_plugin: validate_config rejects bool display_duration, matching
  get_display_duration (bool is an int subclass and would otherwise pass
  as a positive number).
- config_helper: merge_configs deep-copies override values in the
  non-recursive branch so mutating the merged result cannot reach back
  into override_config.
- saved_repositories: saves are atomic (temp file + fsync + os.replace),
  so a failed write can no longer truncate saved_repositories.json.
- tests: regression cases for each fix, plus a pin that whole-item
  array secrets (key[] + key[].field both marked) strip to empty {}
  skeletons — no secret values can reach config.json.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

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
     

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

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
 

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

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

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):

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

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):