mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-07 19:58:08 +00:00
fix(backup): restore must not depend on owning the file it replaces
Follow-up from testing the previous commit on real hardware, where the secrets fix turned out to be both too narrow and slightly wrong. Too narrow: config.json, wifi_config.json and ytm_auth.json are installed root-owned and group-readable exactly like the secrets file, so all four were unrestorable by the web service, not just one. `shutil.copy2` opens the destination for writing, which needs permission on the *existing file*; the web user could create files in that directory all day and still not replace them. Slightly wrong: the previous commit loosened the secrets file to group-writable. That was treating the symptom. The real error was deciding ownership from `ledmatrix.service` -- the display service, which runs as root and only ever *reads* secrets -- when the account that *writes* them is the web interface, which deliberately does not run as root. Ownership now follows the web service's user and the mode stays 640. `_copy_file` writes a temporary file alongside the target and renames over it. That needs only directory permission, so a restore no longer cares who owns the destination, and it is atomic: a crash mid-restore can no longer leave a half-written config. The destination's mode is carried across so restoring secrets does not widen them to the umask, and its owner is carried across too when the OS allows it -- only root can hand a file to another user, so a restore run by the web service keeps its own ownership rather than pretending to preserve root's. Verified on a device with all four config files set root-owned 640 and unwritable by the web user: before, every one failed with EACCES; after, the restore reports success with no errors and all four sections restored, mode still 640, root still able to read them and the web service still able to write them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
This commit is contained in:
co-authored by
Claude Opus 5
parent
d44e31ac9d
commit
7d83ca742a
@@ -3,6 +3,7 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import stat
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
|
||||
@@ -293,3 +294,39 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N
|
||||
# validate_backup catches it before extraction.
|
||||
assert not result.success
|
||||
assert any("unsafe" in e.lower() for e in result.errors)
|
||||
|
||||
|
||||
def test_restore_over_a_file_the_user_cannot_write(
|
||||
project: Path, empty_project: Path, tmp_path: Path
|
||||
) -> None:
|
||||
"""Restore must not need write permission on the destination *file*.
|
||||
|
||||
Reproduces what a fresh install leaves behind: config files owned by root
|
||||
and only group-readable, while the web interface that performs the restore
|
||||
runs as a non-root user. shutil.copy2 opens the destination for writing and
|
||||
failed with EACCES; writing alongside and renaming needs only directory
|
||||
permission, which that account has.
|
||||
|
||||
Simulated here by making the destination read-only — the owner cannot
|
||||
open it for writing either, but can still replace it within its directory.
|
||||
"""
|
||||
zip_path = create_backup(project, output_dir=tmp_path / "exports")
|
||||
|
||||
# Pre-existing, read-only destinations.
|
||||
(empty_project / "config").mkdir(parents=True, exist_ok=True)
|
||||
for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"):
|
||||
target = empty_project / "config" / name
|
||||
target.write_text("{}", encoding="utf-8")
|
||||
target.chmod(0o444)
|
||||
|
||||
result = restore_backup(zip_path, empty_project, RestoreOptions())
|
||||
|
||||
assert result.success, result.errors
|
||||
for section in ("config", "secrets", "wifi", "ytm_auth"):
|
||||
assert section in result.restored, f"{section} not restored: {result.errors}"
|
||||
|
||||
restored = json.loads((empty_project / "config" / "config.json").read_text())
|
||||
assert restored["my-plugin"]["favorites"] == ["A", "B"]
|
||||
|
||||
# The destination's mode is preserved rather than widened to the umask.
|
||||
assert stat.S_IMODE((empty_project / "config" / "config_secrets.json").stat().st_mode) == 0o444
|
||||
|
||||
Reference in New Issue
Block a user