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
This commit is contained in:
ChuckBuilds
2026-08-21 10:29:16 -04:00
co-authored by Claude Opus 5
parent b7a7e26bfe
commit ab8ff0e7e9
2 changed files with 44 additions and 3 deletions
+15 -3
View File
@@ -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(
+29
View File
@@ -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}"