From ab8ff0e7e9684a87cb1e79c5e303ac5f3e48ae04 Mon Sep 17 00:00:00 2001
From: ChuckBuilds <charlesmynard@gmail.com>
Date: Fri, 21 Aug 2026 10:29:16 -0400
Subject: [PATCH] fix(backup): preserve the destination's mode and owner when
 merging

Self-review catch. The merge path wrote the combined config with write_text
and renamed it into place, bypassing _copy_file -- which preserves the
destination's mode and owner deliberately, because these files are installed
root-owned while the web interface that runs a restore is not root, and
because widening them to the umask default is exactly the wrong thing to do
to a file that sits next to secrets.

Measured: a config.json at 0600 came back 0664.

The merged result is now written to a scratch file and handed to _copy_file,
which does the atomic replace with the preservation it was written for. The
scratch file is removed in a finally block.

Two tests added: the mode survives a merge, and no scratch file is left
behind. Reverting to the bare rename fails the first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
---
 src/backup_manager.py                     | 18 +++++++++++---
 test/test_restore_keeps_panel_hardware.py | 29 +++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/src/backup_manager.py b/src/backup_manager.py
index ccb76cbb..a6dac631 100644
--- a/src/backup_manager.py
+++ b/src/backup_manager.py
@@ -604,9 +604,21 @@ def _restore_config_preserving_hardware(src: Path, dst: Path, keep_hardware: boo
             local_hw.get("cols"), local_hw.get("rows"),
             local_hw.get("chain_length"))
 
-    tmp_path = dst.with_suffix(dst.suffix + ".restore-tmp")
-    tmp_path.write_text(json.dumps(incoming, indent=2) + "\n", encoding="utf-8")
-    os.replace(tmp_path, dst)
+    # Write the merged result to a scratch file and hand it to _copy_file
+    # rather than renaming it into place here. _copy_file preserves the
+    # destination's mode and owner on purpose -- these config files are
+    # installed root-owned while the web interface that runs a restore is not
+    # root -- and a bare write would have replaced a root-owned config.json
+    # with one owned by the web user at whatever the umask allows.
+    scratch = dst.with_suffix(dst.suffix + ".merged-tmp")
+    try:
+        scratch.write_text(json.dumps(incoming, indent=2) + "\n", encoding="utf-8")
+        _copy_file(scratch, dst)
+    finally:
+        try:
+            scratch.unlink()
+        except OSError:
+            pass
 
 
 def restore_backup(
diff --git a/test/test_restore_keeps_panel_hardware.py b/test/test_restore_keeps_panel_hardware.py
index f100f8cb..4874a745 100644
--- a/test/test_restore_keeps_panel_hardware.py
+++ b/test/test_restore_keeps_panel_hardware.py
@@ -73,3 +73,32 @@ def test_unparseable_local_config_still_restores(tmp_path):
     _restore_config_preserving_hardware(src, dst, keep_hardware=True)
     assert json.loads(dst.read_text())["timezone"] == "America/New_York", (
         "a restore must never fail because of this merge")
+
+
+def test_the_destination_mode_is_preserved(tmp_path):
+    """config.json is installed with a deliberate mode; a merge must not widen it.
+
+    _copy_file preserves the destination's mode and owner on purpose -- these
+    files are root-owned while the web interface running the restore is not.
+    Writing the merged result directly would have replaced that with whatever
+    the umask allowed.
+    """
+    import os
+    import stat as statmod
+    src = tmp_path / "b.json"; src.write_text(json.dumps(BIG))
+    dst = tmp_path / "c.json"; dst.write_text(json.dumps(SMALL))
+    os.chmod(dst, 0o600)
+
+    _restore_config_preserving_hardware(src, dst, keep_hardware=True)
+
+    mode = statmod.S_IMODE(os.stat(dst).st_mode)
+    assert mode == 0o600, f"restore widened config.json from 0600 to {oct(mode)}"
+    assert json.loads(dst.read_text())["display"]["hardware"]["cols"] == 64
+
+
+def test_no_scratch_file_is_left_behind(tmp_path):
+    src = tmp_path / "b.json"; src.write_text(json.dumps(BIG))
+    dst = tmp_path / "c.json"; dst.write_text(json.dumps(SMALL))
+    _restore_config_preserving_hardware(src, dst, keep_hardware=True)
+    leftovers = [p.name for p in tmp_path.iterdir() if "tmp" in p.name]
+    assert not leftovers, f"scratch files left in place: {leftovers}"