diff --git a/first_time_install.sh b/first_time_install.sh index 1eaf2d68..251ab73d 100644 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1483,34 +1483,38 @@ if [ -f "$PROJECT_ROOT_DIR/config/config.json" ]; then fi # Set proper permissions for secrets file (restrictive: owner rw, group r) -# If service runs as root, set ownership to root so it can read as owner -# Otherwise, use ACTUAL_USER and rely on group membership +# Owned by whoever WRITES the file, which is the web interface. +# +# This used to read the User= of ledmatrix.service — the display service — +# and, finding root, hand the file to root:ledmatrix 640. But the display +# service only ever reads secrets, and root can read any file regardless of +# mode. The account that *writes* them is the web interface: it saves config +# edits and performs backup restores, and it deliberately does not run as root +# (a web server should not). So a root-owned, group-read-only file left the web +# UI unable to write its own secrets, and restoring a backup failed with +# "Permission denied: config_secrets.json" while every other file in the same +# backup restored fine. +# +# Owning by the writer keeps the tighter 640 rather than loosening to +# group-writable, and root still reads it as superuser. if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then - # Check if service runs as root (from service file or template) - SERVICE_USER="root" - if [ -f "/etc/systemd/system/ledmatrix.service" ]; then - SERVICE_USER=$(grep "^User=" /etc/systemd/system/ledmatrix.service | cut -d'=' -f2 || echo "root") - elif [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then - SERVICE_USER=$(grep "^User=" "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" | cut -d'=' -f2 || echo "root") + # The web service is the writer; fall back to the display service, then to + # the installing user, so an unusual layout still lands somewhere sensible. + SECRETS_OWNER="" + for unit in "/etc/systemd/system/ledmatrix-web.service" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service"; do + if [ -f "$unit" ]; then + SECRETS_OWNER=$(grep -m1 "^User=" "$unit" | cut -d'=' -f2) + [ -n "$SECRETS_OWNER" ] && break + fi + done + if [ -z "$SECRETS_OWNER" ]; then + SECRETS_OWNER="$ACTUAL_USER" fi - - if [ "$SERVICE_USER" = "root" ]; then - # Service runs as root - set ownership to root so it can read as owner - chown "root:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true - echo "✓ Secrets file permissions set (root:$LEDMATRIX_GROUP for root service)" - else - # Service runs as regular user - use ACTUAL_USER and rely on group membership - chown "$ACTUAL_USER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true - echo "✓ Secrets file permissions set ($ACTUAL_USER:$LEDMATRIX_GROUP)" - fi - # Group-writable, not just group-readable. The web interface performs - # backup restores, and it does not necessarily run as the file's owner: on - # a fresh install the secrets file ended up owned by root while the web - # service ran as the login user, so restoring a backup failed with - # "Permission denied: config_secrets.json" while every other file in the - # same backup restored fine. Read-only for the group makes secrets the one - # thing a restore cannot put back. - chmod 660 "$PROJECT_ROOT_DIR/config/config_secrets.json" + # A root-owned file is only correct when the writer really is root. + chown "$SECRETS_OWNER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true + echo "✓ Secrets file owned by the web service user ($SECRETS_OWNER:$LEDMATRIX_GROUP, mode 640)" + chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json" fi # Set proper permissions for YTM auth file (readable by all users including root service) diff --git a/src/backup_manager.py b/src/backup_manager.py index af3247b6..b3cdb84b 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -16,6 +16,7 @@ import json import logging import os import shutil +import stat import socket import tempfile import zipfile @@ -491,8 +492,61 @@ def _extract_zip_safe(zip_path: Path, dest_dir: Path) -> None: def _copy_file(src: Path, dst: Path) -> None: + """Replace ``dst`` with ``src``, atomically, without needing to own ``dst``. + + ``shutil.copy2`` opens the destination for writing, so it needs write + permission on the *existing file*. Several config files are installed + root-owned and group-readable while the web interface — which is what runs + a restore — deliberately runs as a non-root user. Restoring those failed + with EACCES even though the account could create files in the same + directory perfectly well. + + Writing a temporary file alongside and renaming over the target needs only + directory permission, which the web user has. It is also atomic: a crash + mid-restore can no longer leave a half-written config behind. + + The destination's existing mode is preserved when there is one, so + restoring secrets does not silently widen them to the umask default. + """ dst.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(src, dst) + + existing_mode: Optional[int] = None + existing_owner: Optional[Tuple[int, int]] = None + if dst.exists(): + try: + info = dst.stat() + existing_mode = stat.S_IMODE(info.st_mode) + existing_owner = (info.st_uid, info.st_gid) + except OSError: + existing_mode = None + existing_owner = None + + fd, tmp_name = tempfile.mkstemp(dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp") + os.close(fd) + tmp_path = Path(tmp_name) + try: + shutil.copyfile(src, tmp_path) + if existing_mode is not None: + os.chmod(tmp_path, existing_mode) + else: + shutil.copymode(src, tmp_path) + if existing_owner is not None: + # Replacing a file creates a new inode owned by whoever is running, + # which would silently move a root-owned config to the web user. + # Carry the previous owner across when the OS permits it — only + # root can hand a file to another user, so this is best-effort and + # a plain restore as the web user simply keeps its own ownership. + try: + os.chown(tmp_path, existing_owner[0], existing_owner[1]) + except (OSError, PermissionError): + pass + os.replace(tmp_path, dst) + except BaseException: + try: + tmp_path.unlink() + except OSError: + pass + raise def restore_backup( diff --git a/test/test_backup_manager.py b/test/test_backup_manager.py index 97c4d000..74134de8 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -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