From 6ab4789a70454249e685555ece1a7322b76759e7 Mon Sep 17 00:00:00 2001 From: AJ Date: Thu, 2 Jul 2026 01:40:24 -0400 Subject: [PATCH] fix: scope backup_diff to server sections and mask secrets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff preview in RestoreDialog was serializing the full config dict, exposing secret env values and token args in cleartext on a pasteable surface. - Add _redact_server_data / _redact_servers_block helpers that apply redact_args to args and mask env values for is_secret_key() keys - Rewrite backup_diff to compare only {mcpServers, _disabledMcpServers} sections (sanitized), not the whole file — also avoids double-serializing multi-MB ~/.claude.json for a servers-only diff - Add clarifying comment in _restore_from_backup about why full_config is the right base after confirm-discard - Add test: backup_diff with secret args/env → MASK in output, raw values absent - Add test: restore_backup restores _disabledMcpServers correctly Co-Authored-By: Claude Sonnet 4.6 --- bcc.py | 2 ++ bcc_core.py | 45 ++++++++++++++++++++++++------- tests/test_core.py | 67 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 10 deletions(-) diff --git a/bcc.py b/bcc.py index 3155f2b..0aa7239 100644 --- a/bcc.py +++ b/bcc.py @@ -1646,6 +1646,8 @@ class MainWindow(QMainWindow): return if self.dirty and not self._confirm_discard(): return + # full_config is the right base: discarded edits were server changes; + # non-server keys from the in-memory state are what we want to preserve. dlg = RestoreDialog(self, self.current_profile, self.full_config) if not dlg.exec(): return diff --git a/bcc_core.py b/bcc_core.py index f32621f..3f2702b 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -273,18 +273,37 @@ def backup_label(backup_path: Path | str) -> str: return f"{ts[:4]}-{ts[4:6]}-{ts[6:8]} {ts[9:11]}:{ts[11:13]}:{ts[13:]}" +def _redact_server_data(data: dict) -> dict: + """Return a copy of a server definition with secrets masked for display.""" + out = dict(data) + if "args" in out: + out["args"] = redact_args(list(out["args"] or [])) + if "env" in out: + out["env"] = {k: (MASK if is_secret_key(k) else v) for k, v in (out["env"] or {}).items()} + return out + + +def _redact_servers_block(block: dict | None) -> dict: + """Return a sanitized copy of a mcpServers / _disabledMcpServers block.""" + if not block: + return {} + return {name: _redact_server_data(data) for name, data in block.items()} + + def backup_diff( config_path: Path | str, backup_path: Path | str, current_cfg: dict | None = None, ) -> str: """ - Unified diff showing what the current config would look like after restoring - the backup (servers only — non-server keys are preserved). + Unified diff showing what the server sections would look like after restoring + the backup. Only mcpServers and _disabledMcpServers are compared; non-server + keys are excluded entirely (they are preserved by restore, not changed). + Secret values in args and env are masked so the diff is safe to share. fromfile = current state, tofile = what will be written after restore. - Pass current_cfg to use an already-loaded (possibly repaired) in-memory config - as the baseline instead of re-reading from disk. + Pass current_cfg to diff against an already-loaded in-memory config instead + of re-reading from disk. """ p = Path(config_path) bp = Path(backup_path) @@ -299,12 +318,18 @@ def backup_diff( after_cfg = dict(current_cfg) apply_servers(after_cfg, backup_servers) - before_lines = (json.dumps(current_cfg, indent=2, ensure_ascii=False) + "\n").splitlines( - keepends=True - ) - after_lines = (json.dumps(after_cfg, indent=2, ensure_ascii=False) + "\n").splitlines( - keepends=True - ) + def _server_sections(cfg: dict) -> dict: + out: dict = {"mcpServers": _redact_servers_block(cfg.get("mcpServers"))} + if DISABLED_KEY in cfg: + out[DISABLED_KEY] = _redact_servers_block(cfg.get(DISABLED_KEY)) + return out + + before_lines = ( + json.dumps(_server_sections(current_cfg), indent=2, ensure_ascii=False) + "\n" + ).splitlines(keepends=True) + after_lines = ( + json.dumps(_server_sections(after_cfg), indent=2, ensure_ascii=False) + "\n" + ).splitlines(keepends=True) result = "".join( difflib.unified_diff( diff --git a/tests/test_core.py b/tests/test_core.py index 5a8c442..b3b7e77 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -433,3 +433,70 @@ def test_restore_with_current_cfg_passed(config_with_backup): restored = json.loads(config_with_backup.read_text()) assert restored.get("extraKey") == "preserved" assert "server-v1" in restored.get("mcpServers", {}) + + +def test_backup_diff_masks_secrets(tmp_path): + cfgpath = tmp_path / "cfg.json" + current = { + "mcpServers": { + "s": { + "command": "node", + "args": ["--token", "super-secret"], + "env": {"API_KEY": "my-api-key-value"}, + } + } + } + cfgpath.write_text(json.dumps(current, indent=2)) + bdir = cfgpath.parent / c.BACKUP_DIRNAME + bdir.mkdir() + import time as _t + + bp = bdir / f"cfg.{_t.strftime('%Y%m%d-%H%M%S')}.json" + bp.write_text( + json.dumps( + { + "mcpServers": { + "s2": { + "command": "python", + "args": ["--api-key", "another-secret"], + "env": {"SECRET_KEY": "backup-secret-value"}, + } + } + }, + indent=2, + ) + ) + diff = c.backup_diff(cfgpath, bp) + # Raw secret values must not appear in the diff + assert "super-secret" not in diff + assert "my-api-key-value" not in diff + assert "another-secret" not in diff + assert "backup-secret-value" not in diff + # The mask placeholder must appear (secrets were present in both sides) + assert c.MASK in diff + + +def test_restore_backup_restores_disabled_servers(tmp_path): + cfgpath = tmp_path / "cfg.json" + original = { + "mcpServers": {"active": {"command": "node"}}, + c.DISABLED_KEY: {"parked": {"command": "python"}}, + } + cfgpath.write_text(json.dumps(original, indent=2)) + + # Overwrite: add new server, drop the disabled one + cfg = c.load_config(cfgpath) + servers = c.extract_servers(cfg) + servers = [s for s in servers if s.name != "parked"] + servers.append(c.ServerEntry("newcomer", {"command": "uvx"}, True)) + c.apply_servers(cfg, servers) + c.write_config(cfgpath, cfg) + + # Restore from the v1 backup + backups = c.list_backups(cfgpath) + c.restore_backup(cfgpath, backups[0]) + restored = json.loads(cfgpath.read_text()) + + assert "parked" in restored.get(c.DISABLED_KEY, {}) + assert "active" in restored.get("mcpServers", {}) + assert "newcomer" not in restored.get("mcpServers", {})