From 4abcd0e4f95d9c53bc151df80a76f6c8348cff32 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:25:58 -0400 Subject: [PATCH] Fix PermissionError reading config_secrets.json in web interface (#416) ledmatrix.service (main display) runs as root while ledmatrix-web.service runs as the non-root install user (install_web_service.sh). Both config_manager.py and config_manager_atomic.py only chmod'd config_secrets.json to 0o640 without ever fixing its group, so a file written by the root service ended up group-owned by root and unreadable by the web user, crashing the settings page with a raw PermissionError. Add ensure_shared_group_ownership() to chgrp secrets/config files (best effort, root-only) to the project directory's owning group whenever they are created or saved, and self-heal existing files on load. Also make get_raw_file_content() tolerate an unreadable secrets file the same way load_config() already does, degrading to empty secrets instead of a 500. Co-authored-by: Claude --- src/common/permission_utils.py | 54 ++++++++++++++++++++++++++++++++++ src/config_manager.py | 26 +++++++++++++++- src/config_manager_atomic.py | 8 +++++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/common/permission_utils.py b/src/common/permission_utils.py index 3679c253..40ed98a9 100644 --- a/src/common/permission_utils.py +++ b/src/common/permission_utils.py @@ -146,6 +146,60 @@ def ensure_file_permissions(path: Path, mode: int = 0o644) -> None: raise +_shared_group_gid_cache: Optional[int] = None + + +def get_shared_group_gid() -> Optional[int]: + """ + Return the gid that should own config/secrets files shared between the + root-run ``ledmatrix.service`` (main display) and the non-root user that + ``ledmatrix-web.service`` runs as (see install_web_service.sh, which sets + ``User=$SUDO_USER``). + + Resolved once from the project root directory's current group (normally + the login user's group from the initial ``git clone``), since that user + is stable across reinstalls unlike any single file's ownership. + + Returns: + The gid, or None if it cannot be determined. + """ + global _shared_group_gid_cache + if _shared_group_gid_cache is not None: + return _shared_group_gid_cache + try: + project_root = Path(__file__).resolve().parent.parent.parent + _shared_group_gid_cache = project_root.stat().st_gid + return _shared_group_gid_cache + except OSError: + return None + + +def ensure_shared_group_ownership(path: Path) -> None: + """ + Best-effort chgrp of ``path`` to the shared group (see + :func:`get_shared_group_gid`) when running as root. + + Only root can change a file's group to one the calling process isn't a + member of, which is exactly the case that causes the web interface + (running as a non-root user) to get ``PermissionError`` reading files + the root-run display service just wrote with a 0o640/2775 mode: the mode + is group-readable, but without this the group is root's, not the web + user's. Silently does nothing if not running as root or on any error — + this is a hardening step, not a required one. + """ + if os.geteuid() != 0: + return + gid = get_shared_group_gid() + if gid is None: + return + try: + if path.exists() and path.stat().st_gid != gid: + os.chown(path, -1, gid) + logger.debug(f"Set shared group ownership (gid {gid}) on {path}") + except OSError as e: + logger.debug(f"Could not set shared group ownership on {path}: {e}") + + def get_config_file_mode(file_path: Path) -> int: """ Return appropriate permission mode for config files. diff --git a/src/config_manager.py b/src/config_manager.py index e4c5cc4b..3eb888f7 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -38,6 +38,7 @@ from src.config_manager_atomic import ( from src.common.permission_utils import ( ensure_directory_permissions, ensure_file_permissions, + ensure_shared_group_ownership, get_config_file_mode, get_config_dir_mode ) @@ -234,6 +235,11 @@ class ConfigManager: # Load and merge secrets if they exist (be permissive on errors) if os.path.exists(self.secrets_path): + # Self-heal stale group ownership (e.g. the root-run display + # service wrote this file before the web user was granted + # group access) before every load attempt; no-op unless + # running as root and the group is already wrong. + ensure_shared_group_ownership(Path(self.secrets_path)) try: with open(self.secrets_path, 'r') as f: secrets = json.load(f) @@ -363,6 +369,7 @@ class ConfigManager: # Set proper file permissions after creation config_path_obj = Path(self.config_path) ensure_file_permissions(config_path_obj, get_config_file_mode(config_path_obj)) + ensure_shared_group_ownership(config_path_obj) self.logger.info(f"Created config.json from template at {os.path.abspath(self.config_path)}") @@ -475,6 +482,11 @@ class ConfigManager: self.logger.error(error_msg) raise ConfigError(error_msg, config_path=path_to_load) + if file_type == "secrets": + # Best-effort self-heal: no-op unless running as root and the + # group is stale (see load_config for why this can happen). + ensure_shared_group_ownership(Path(path_to_load)) + try: with open(path_to_load, 'r') as f: return json.load(f) @@ -482,7 +494,18 @@ class ConfigManager: error_msg = f"Error parsing {file_type} configuration file: {path_to_load}" self.logger.error(error_msg, exc_info=True) raise ConfigError(error_msg, config_path=path_to_load) from e - except (IOError, OSError, PermissionError) as e: + except PermissionError as e: + if file_type == "secrets": + # Match load_config()'s tolerance: a secrets file the web + # process can't read (e.g. written 0640 by the root-run + # display service before the group was fixed up) shouldn't + # 500 the settings page — degrade to "no secrets" instead. + self.logger.warning(f"Secrets file not readable ({path_to_load}): {e}. Returning empty secrets.") + return {} + error_msg = f"Error loading {file_type} configuration file {path_to_load}: {str(e)}" + self.logger.error(error_msg, exc_info=True) + raise ConfigError(error_msg, config_path=path_to_load) from e + except (IOError, OSError) as e: error_msg = f"Error loading {file_type} configuration file {path_to_load}: {str(e)}" self.logger.error(error_msg, exc_info=True) raise ConfigError(error_msg, config_path=path_to_load) from e @@ -539,6 +562,7 @@ class ConfigManager: # Ensure final file has correct permissions try: ensure_file_permissions(path_obj, file_mode) + ensure_shared_group_ownership(path_obj) except OSError as perm_error: # If we can't set permissions but file was written, log warning but don't fail self.logger.warning( diff --git a/src/config_manager_atomic.py b/src/config_manager_atomic.py index 3e56d4d3..7b2610a1 100644 --- a/src/config_manager_atomic.py +++ b/src/config_manager_atomic.py @@ -17,6 +17,7 @@ from enum import Enum from src.exceptions import ConfigError from src.logging_config import get_logger +from src.common.permission_utils import ensure_shared_group_ownership class SaveResultStatus(Enum): @@ -410,6 +411,13 @@ class AtomicConfigManager: # This is important because temp files may have different permissions # and we need root service to be able to read config.json os.chmod(destination, target_mode) + + # Also fix group ownership when this save is running as root + # (the display service): 0o640 alone only helps the non-root web + # user read a root-written secrets file if its group already + # matches the web user's group, which isn't guaranteed. See + # permission_utils.ensure_shared_group_ownership for why. + ensure_shared_group_ownership(destination) except Exception as e: raise ConfigError(f"Error during atomic move: {e}") from e