fix: scope backup_diff to server sections and mask secrets
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 <noreply@anthropic.com>
This commit is contained in:
@@ -1646,6 +1646,8 @@ class MainWindow(QMainWindow):
|
|||||||
return
|
return
|
||||||
if self.dirty and not self._confirm_discard():
|
if self.dirty and not self._confirm_discard():
|
||||||
return
|
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)
|
dlg = RestoreDialog(self, self.current_profile, self.full_config)
|
||||||
if not dlg.exec():
|
if not dlg.exec():
|
||||||
return
|
return
|
||||||
|
|||||||
+35
-10
@@ -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:]}"
|
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(
|
def backup_diff(
|
||||||
config_path: Path | str,
|
config_path: Path | str,
|
||||||
backup_path: Path | str,
|
backup_path: Path | str,
|
||||||
current_cfg: dict | None = None,
|
current_cfg: dict | None = None,
|
||||||
) -> str:
|
) -> str:
|
||||||
"""
|
"""
|
||||||
Unified diff showing what the current config would look like after restoring
|
Unified diff showing what the server sections would look like after restoring
|
||||||
the backup (servers only — non-server keys are preserved).
|
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.
|
fromfile = current state, tofile = what will be written after restore.
|
||||||
Pass current_cfg to use an already-loaded (possibly repaired) in-memory config
|
Pass current_cfg to diff against an already-loaded in-memory config instead
|
||||||
as the baseline instead of re-reading from disk.
|
of re-reading from disk.
|
||||||
"""
|
"""
|
||||||
p = Path(config_path)
|
p = Path(config_path)
|
||||||
bp = Path(backup_path)
|
bp = Path(backup_path)
|
||||||
@@ -299,12 +318,18 @@ def backup_diff(
|
|||||||
after_cfg = dict(current_cfg)
|
after_cfg = dict(current_cfg)
|
||||||
apply_servers(after_cfg, backup_servers)
|
apply_servers(after_cfg, backup_servers)
|
||||||
|
|
||||||
before_lines = (json.dumps(current_cfg, indent=2, ensure_ascii=False) + "\n").splitlines(
|
def _server_sections(cfg: dict) -> dict:
|
||||||
keepends=True
|
out: dict = {"mcpServers": _redact_servers_block(cfg.get("mcpServers"))}
|
||||||
)
|
if DISABLED_KEY in cfg:
|
||||||
after_lines = (json.dumps(after_cfg, indent=2, ensure_ascii=False) + "\n").splitlines(
|
out[DISABLED_KEY] = _redact_servers_block(cfg.get(DISABLED_KEY))
|
||||||
keepends=True
|
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(
|
result = "".join(
|
||||||
difflib.unified_diff(
|
difflib.unified_diff(
|
||||||
|
|||||||
@@ -433,3 +433,70 @@ def test_restore_with_current_cfg_passed(config_with_backup):
|
|||||||
restored = json.loads(config_with_backup.read_text())
|
restored = json.loads(config_with_backup.read_text())
|
||||||
assert restored.get("extraKey") == "preserved"
|
assert restored.get("extraKey") == "preserved"
|
||||||
assert "server-v1" in restored.get("mcpServers", {})
|
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", {})
|
||||||
|
|||||||
Reference in New Issue
Block a user