fix(backup): address CodeRabbit and CodeQL findings on PR #439

first_time_install.sh

@@ -1508,10 +1508,26 @@ if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then
     if [ -z "$SECRETS_OWNER" ]; then
         SECRETS_OWNER="$ACTUAL_USER"
     fi
+    SECRETS_FILE="$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
+    if ! chown "$SECRETS_OWNER:$LEDMATRIX_GROUP" "$SECRETS_FILE"; then
+        echo "✗ ERROR: Failed to set ownership on $SECRETS_FILE to $SECRETS_OWNER:$LEDMATRIX_GROUP" >&2
+        echo "  Try: sudo chown $SECRETS_OWNER:$LEDMATRIX_GROUP $SECRETS_FILE" >&2
+        exit 1
+    fi
+    if ! chmod 640 "$SECRETS_FILE"; then
+        echo "✗ ERROR: Failed to set permissions on $SECRETS_FILE to 640" >&2
+        echo "  Try: sudo chmod 640 $SECRETS_FILE" >&2
+        exit 1
+    fi
+    ACTUAL_OWNERSHIP=$(stat -c '%U:%G' "$SECRETS_FILE" 2>/dev/null || echo "unknown")
+    ACTUAL_MODE=$(stat -c '%a' "$SECRETS_FILE" 2>/dev/null || echo "unknown")
+    if [ "$ACTUAL_OWNERSHIP" != "$SECRETS_OWNER:$LEDMATRIX_GROUP" ] || [ "$ACTUAL_MODE" != "640" ]; then
+        echo "✗ ERROR: $SECRETS_FILE ended up as $ACTUAL_OWNERSHIP mode $ACTUAL_MODE, expected $SECRETS_OWNER:$LEDMATRIX_GROUP mode 640" >&2
+        echo "  The web interface may be unable to read or write config_secrets.json." >&2
+        exit 1
+    fi
     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)

src/backup_manager.py

@@ -577,7 +577,8 @@
         try:
             _extract_zip_safe(Path(zip_path), tmp_dir)
         except (ValueError, zipfile.BadZipFile, OSError) as e:
-            result.errors.append(f"Failed to extract backup: {e}")
+            logger.error("[Backup] Failed to extract backup: %s", e, exc_info=True)
+            result.errors.append("Failed to extract backup")
             return result
 
         # Main config.
@@ -586,7 +587,8 @@
                 _copy_file(tmp_dir / _CONFIG_REL, project_root / _CONFIG_REL)
                 result.restored.append("config")
             except OSError as e:
-                result.errors.append(f"Failed to restore config.json: {e}")
+                logger.error("[Backup] Failed to restore config.json: %s", e, exc_info=True)
+                result.errors.append("Failed to restore config.json")
         elif (tmp_dir / _CONFIG_REL).exists():
             result.skipped.append("config")
 
@@ -596,7 +598,10 @@
                 _copy_file(tmp_dir / _SECRETS_REL, project_root / _SECRETS_REL)
                 result.restored.append("secrets")
             except OSError as e:
-                result.errors.append(f"Failed to restore config_secrets.json: {e}")
+                logger.error(
+                    "[Backup] Failed to restore config_secrets.json: %s", e, exc_info=True
+                )
+                result.errors.append("Failed to restore config_secrets.json")
         elif (tmp_dir / _SECRETS_REL).exists():
             result.skipped.append("secrets")
 
@@ -606,7 +611,10 @@
                 _copy_file(tmp_dir / _WIFI_REL, project_root / _WIFI_REL)
                 result.restored.append("wifi")
             except OSError as e:
-                result.errors.append(f"Failed to restore wifi_config.json: {e}")
+                logger.error(
+                    "[Backup] Failed to restore wifi_config.json: %s", e, exc_info=True
+                )
+                result.errors.append("Failed to restore wifi_config.json")
         elif (tmp_dir / _WIFI_REL).exists():
             result.skipped.append("wifi")
 
@@ -618,7 +626,8 @@
                 _copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL)
                 result.restored.append("ytm_auth")
             except OSError as e:
-                result.errors.append(f"Failed to restore ytm_auth.json: {e}")
+                logger.error("[Backup] Failed to restore ytm_auth.json: %s", e, exc_info=True)
+                result.errors.append("Failed to restore ytm_auth.json")
         elif (tmp_dir / _YTM_REL).exists():
             result.skipped.append("ytm_auth")
 
@@ -636,7 +645,10 @@
                     _copy_file(font, project_root / _FONTS_REL / font.name)
                     restored_count += 1
                 except OSError as e:
-                    result.errors.append(f"Failed to restore font {font.name}: {e}")
+                    logger.error(
+                        "[Backup] Failed to restore font %s: %s", font.name, e, exc_info=True
+                    )
+                    result.errors.append(f"Failed to restore font {font.name}")
             if restored_count:
                 result.restored.append(f"fonts ({restored_count})")
         elif tmp_fonts.exists():
@@ -657,7 +669,8 @@
                         _copy_file(src, project_root / rel)
                         count += 1
                     except OSError as e:
-                        result.errors.append(f"Failed to restore {rel}: {e}")
+                        logger.error("[Backup] Failed to restore %s: %s", rel, e, exc_info=True)
+                        result.errors.append(f"Failed to restore {rel}")
             if count:
                 result.restored.append(f"plugin_uploads ({count})")
         elif tmp_uploads.exists():
@@ -675,7 +688,8 @@
                         if isinstance(p, dict) and p.get("plugin_id")
                     ]
             except (OSError, json.JSONDecodeError) as e:
-                result.errors.append(f"Could not read plugins.json: {e}")
+                logger.error("[Backup] Could not read plugins.json: %s", e, exc_info=True)
+                result.errors.append("Could not read plugins.json")
 
     result.success = not result.errors
     return result

test/test_backup_manager.py

@@ -283,6 +283,10 @@ def test_restore_honors_options(project: Path, empty_project: Path, tmp_path: Pa
     assert result.plugins_to_install == []
     assert "secrets" in result.skipped
     assert "wifi" in result.skipped
+    # ytm_auth rides on restore_wifi rather than its own flag -- disabling
+    # wifi restore must not leave a stale session token behind.
+    assert "ytm_auth" in result.skipped
+    assert not (empty_project / "config" / "ytm_auth.json").exists()
 
 
 def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> None:

test/test_registry_id_resolution.py

@@ -75,6 +75,13 @@ def _ids(entry: Optional[Dict[str, Any]]) -> Optional[str]:
 
 
 class TestRegistryLookupByManifestId:
+    def test_get_plugin_info_resolves_manifest_id(self, store: PluginStoreManager) -> None:
+        """get_plugin_info() delegates to the same lookup as get_registry_info()."""
+        assert (
+            _ids(store.get_plugin_info("ledmatrix-weather", fetch_latest_from_github=False))
+            == "weather"
+        )
+
     def test_exact_registry_id_still_resolves(self, store: PluginStoreManager) -> None:
         assert _ids(store.get_registry_info("weather")) == "weather"
 

web_interface/blueprints/api_v3.py

@@ -7862,14 +7862,18 @@ def _resolve_backup_export_dir() -> Path:
     export at all.
     """
     preferred = PROJECT_ROOT.parent / "ledmatrix-backups"
+    fallback = PROJECT_ROOT / "config" / "backups" / "exports"
     try:
         preferred.mkdir(parents=True, exist_ok=True)
-        probe = preferred / ".writetest"
-        probe.write_text("", encoding="utf-8")
-        probe.unlink()
+        with tempfile.NamedTemporaryFile(dir=preferred, prefix=".writetest-"):
+            pass
         return preferred
-    except OSError:
-        return PROJECT_ROOT / "config" / "backups" / "exports"
+    except OSError as e:
+        logger.warning(
+            f"[Backup] Export dir {preferred} is not writable ({e}); "
+            f"falling back to {fallback}, which a reinstall will delete"
+        )
+        return fallback
 
 
 _BACKUP_EXPORT_DIR = _resolve_backup_export_dir()
@@ -8022,7 +8026,15 @@ def backup_restore():
                     else:
                         result.plugins_failed.append({'plugin_id': pid, 'error': 'Store manager unavailable'})
                 except Exception as pe:
-                    result.plugins_failed.append({'plugin_id': pid, 'error': str(pe)})
+                    logger.error(
+                        "[Backup] Failed to reinstall plugin %r: %s", pid, pe, exc_info=True
+                    )
+                    result.plugins_failed.append({'plugin_id': pid, 'error': 'Installation failed; see server logs'})
+
+        # A restore that dropped files can still report success if the only
+        # failures were plugin reinstalls, since those don't touch result.errors.
+        if result.plugins_failed:
+            result.success = False
 
         data = result.to_dict()
         if not result.success:
@@ -8032,7 +8044,9 @@ def backup_restore():
             # config restores and secrets do not. "Restore had errors" alone
             # left the user unable to tell a wholly failed restore from one
             # that quietly dropped their API keys.
-            failed_plugins = [p.get('plugin_id') for p in (result.plugins_failed or [])]
+            failed_plugins = [
+                str(p.get('plugin_id')) for p in (result.plugins_failed or []) if p.get('plugin_id')
+            ]
             parts = []
             if result.restored:
                 parts.append(f"restored: {', '.join(result.restored)}")