From c4b3dc833b02d0826490b58d892ec35f7cb5d7f3 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Thu, 26 Mar 2026 11:35:14 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20Audit-Findings=20=E2=80=94=20Bugs,=20Sec?= =?UTF-8?q?urity-Hardening=20und=20Performance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BUG-02: cancel_session bei mehrstufiger Auth (z.B. TOTP) - BUG-03: CalledProcessError bei reboot/shutdown abfangen - M-1+EH-01: configparser interpolation=None + Error-Handling - H-1: Exec-Cmd Validierung (absoluter Pfad erforderlich) - PERF: Theme-Guard, Default-Avatar-Cache, Avatar-Cache per User - Mutable Default Arguments in sessions.py (list → tuple) - Ungenutzten Gio-Import entfernt --- src/moongreet/greeter.py | 61 +++++++++++++++++++++++++++++---------- src/moongreet/sessions.py | 8 ++--- src/moongreet/users.py | 7 +++-- tests/test_integration.py | 32 ++++++++++++++++++++ tests/test_power.py | 15 ++++++++++ tests/test_users.py | 37 ++++++++++++++++++++++++ 6 files changed, 139 insertions(+), 21 deletions(-) diff --git a/src/moongreet/greeter.py b/src/moongreet/greeter.py index d3461c1..595e1ec 100644 --- a/src/moongreet/greeter.py +++ b/src/moongreet/greeter.py @@ -5,13 +5,14 @@ import os import shlex import socket import stat +import subprocess from importlib.resources import files from pathlib import Path import gi gi.require_version("Gtk", "4.0") gi.require_version("Gdk", "4.0") -from gi.repository import Gtk, Gdk, GLib, Gio, GdkPixbuf +from gi.repository import Gtk, Gdk, GLib, GdkPixbuf from moongreet.config import load_config from moongreet.ipc import create_session, post_auth_response, start_session, cancel_session @@ -38,6 +39,8 @@ class GreeterWindow(Gtk.ApplicationWindow): self._sessions = get_sessions() self._selected_user: User | None = None self._greetd_sock: socket.socket | None = None + self._default_avatar_pixbuf: GdkPixbuf.Pixbuf | None = None + self._avatar_cache: dict[str, GdkPixbuf.Pixbuf] = {} self._build_ui() self._select_initial_user() @@ -217,16 +220,19 @@ class GreeterWindow(Gtk.ApplicationWindow): self._password_entry.set_text("") self._error_label.set_visible(False) - # Update avatar - avatar_path = get_avatar_path( - user.username, home_dir=user.home - ) - if avatar_path and avatar_path.exists(): - self._set_avatar_from_file(avatar_path) - elif DEFAULT_AVATAR_PATH.exists(): - self._set_default_avatar() + # Update avatar (use cache if available) + if user.username in self._avatar_cache: + self._avatar_image.set_from_pixbuf(self._avatar_cache[user.username]) else: - self._avatar_image.set_from_icon_name("avatar-default-symbolic") + avatar_path = get_avatar_path( + user.username, home_dir=user.home + ) + if avatar_path and avatar_path.exists(): + self._set_avatar_from_file(avatar_path, user.username) + elif DEFAULT_AVATAR_PATH.exists(): + self._set_default_avatar() + else: + self._avatar_image.set_from_icon_name("avatar-default-symbolic") # Apply user's GTK theme if available self._apply_user_theme(user) @@ -243,9 +249,10 @@ class GreeterWindow(Gtk.ApplicationWindow): if settings is None: return - if theme_name: + current = settings.get_property("gtk-theme-name") + if theme_name and current != theme_name: settings.set_property("gtk-theme-name", theme_name) - else: + elif not theme_name and current: settings.reset_property("gtk-theme-name") def _get_foreground_color(self) -> str: @@ -258,6 +265,9 @@ class GreeterWindow(Gtk.ApplicationWindow): def _set_default_avatar(self) -> None: """Load the default avatar SVG, tinted with the GTK foreground color.""" + if self._default_avatar_pixbuf: + self._avatar_image.set_from_pixbuf(self._default_avatar_pixbuf) + return try: svg_text = DEFAULT_AVATAR_PATH.read_text() fg_color = self._get_foreground_color() @@ -269,16 +279,19 @@ class GreeterWindow(Gtk.ApplicationWindow): loader.close() pixbuf = loader.get_pixbuf() if pixbuf: + self._default_avatar_pixbuf = pixbuf self._avatar_image.set_from_pixbuf(pixbuf) except (GLib.Error, OSError): self._avatar_image.set_from_icon_name("avatar-default-symbolic") - def _set_avatar_from_file(self, path: Path) -> None: + def _set_avatar_from_file(self, path: Path, username: str | None = None) -> None: """Load an image file and set it as the avatar, scaled to AVATAR_SIZE.""" try: pixbuf = GdkPixbuf.Pixbuf.new_from_file_at_scale( str(path), AVATAR_SIZE, AVATAR_SIZE, True ) + if username: + self._avatar_cache[username] = pixbuf self._avatar_image.set_from_pixbuf(pixbuf) except GLib.Error: self._avatar_image.set_from_icon_name("avatar-default-symbolic") @@ -379,9 +392,21 @@ class GreeterWindow(Gtk.ApplicationWindow): self._close_greetd_sock() return + if response.get("type") == "auth_message": + # Multi-stage auth (e.g. TOTP) is not supported + cancel_session(sock) + self._close_greetd_sock() + self._show_error("Mehrstufige Authentifizierung wird nicht unterstützt") + return + # Step 3: Start session if response.get("type") == "success": cmd = shlex.split(session.exec_cmd) + if not cmd or not Path(cmd[0]).is_absolute(): + self._show_error("Ungültiger Session-Befehl") + cancel_session(sock) + self._close_greetd_sock() + return response = start_session(sock, cmd) if response.get("type") == "success": @@ -438,11 +463,17 @@ class GreeterWindow(Gtk.ApplicationWindow): def _on_reboot_clicked(self, button: Gtk.Button) -> None: """Handle reboot button click.""" - reboot() + try: + reboot() + except subprocess.CalledProcessError: + self._show_error("Neustart fehlgeschlagen") def _on_shutdown_clicked(self, button: Gtk.Button) -> None: """Handle shutdown button click.""" - shutdown() + try: + shutdown() + except subprocess.CalledProcessError: + self._show_error("Herunterfahren fehlgeschlagen") @staticmethod def _load_last_user() -> str | None: diff --git a/src/moongreet/sessions.py b/src/moongreet/sessions.py index a8a162e..a5af249 100644 --- a/src/moongreet/sessions.py +++ b/src/moongreet/sessions.py @@ -5,8 +5,8 @@ import configparser from dataclasses import dataclass from pathlib import Path -DEFAULT_WAYLAND_DIRS = [Path("/usr/share/wayland-sessions")] -DEFAULT_XSESSION_DIRS = [Path("/usr/share/xsessions")] +DEFAULT_WAYLAND_DIRS = (Path("/usr/share/wayland-sessions"),) +DEFAULT_XSESSION_DIRS = (Path("/usr/share/xsessions"),) @dataclass @@ -37,8 +37,8 @@ def _parse_desktop_file(path: Path, session_type: str) -> Session | None: def get_sessions( - wayland_dirs: list[Path] = DEFAULT_WAYLAND_DIRS, - xsession_dirs: list[Path] = DEFAULT_XSESSION_DIRS, + wayland_dirs: tuple[Path, ...] = DEFAULT_WAYLAND_DIRS, + xsession_dirs: tuple[Path, ...] = DEFAULT_XSESSION_DIRS, ) -> list[Session]: """Discover available sessions from .desktop files.""" sessions: list[Session] = [] diff --git a/src/moongreet/users.py b/src/moongreet/users.py index 83c0e35..358c0c1 100644 --- a/src/moongreet/users.py +++ b/src/moongreet/users.py @@ -95,8 +95,11 @@ def get_user_gtk_theme(config_dir: Path | None = None) -> str | None: if not settings_file.exists(): return None - config = configparser.ConfigParser() - config.read(settings_file) + config = configparser.ConfigParser(interpolation=None) + try: + config.read(settings_file) + except configparser.Error: + return None if config.has_option("Settings", "gtk-theme-name"): return config.get("Settings", "gtk-theme-name") diff --git a/tests/test_integration.py b/tests/test_integration.py index 71064c2..d988d6a 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -122,6 +122,38 @@ class TestLoginFlow: finally: mock.stop() + def test_multi_stage_auth_sends_cancel(self, tmp_path: Path) -> None: + """When greetd sends a second auth_message after password, cancel the session.""" + sock_path = tmp_path / "greetd.sock" + mock = MockGreetd(sock_path) + mock.expect({"type": "auth_message", "auth_message_type": "secret", "auth_message": "Password:"}) + mock.expect({"type": "auth_message", "auth_message_type": "secret", "auth_message": "TOTP:"}) + mock.expect({"type": "success"}) # Response to cancel_session + mock.start() + + try: + sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + sock.connect(str(sock_path)) + + # Step 1: Create session + response = create_session(sock, "dominik") + assert response["type"] == "auth_message" + + # Step 2: Send password — greetd responds with another auth_message + response = post_auth_response(sock, "geheim") + assert response["type"] == "auth_message" + + # Step 3: Cancel because multi-stage auth is not supported + response = cancel_session(sock) + assert response["type"] == "success" + + sock.close() + finally: + mock.stop() + + # Verify cancel was sent + assert mock.received[2] == {"type": "cancel_session"} + def test_cancel_session(self, tmp_path: Path) -> None: """Simulate cancelling a session after create.""" sock_path = tmp_path / "greetd.sock" diff --git a/tests/test_power.py b/tests/test_power.py index 21eccf3..2adae3d 100644 --- a/tests/test_power.py +++ b/tests/test_power.py @@ -1,6 +1,7 @@ # ABOUTME: Tests for power actions — reboot and shutdown via loginctl. # ABOUTME: Uses mocking to avoid actually calling system commands. +import subprocess from unittest.mock import patch, call import pytest @@ -19,6 +20,13 @@ class TestReboot: ["loginctl", "reboot"], check=True ) + @patch("moongreet.power.subprocess.run") + def test_raises_on_failure(self, mock_run) -> None: + mock_run.side_effect = subprocess.CalledProcessError(1, "loginctl") + + with pytest.raises(subprocess.CalledProcessError): + reboot() + class TestShutdown: """Tests for the shutdown power action.""" @@ -30,3 +38,10 @@ class TestShutdown: mock_run.assert_called_once_with( ["loginctl", "poweroff"], check=True ) + + @patch("moongreet.power.subprocess.run") + def test_raises_on_failure(self, mock_run) -> None: + mock_run.side_effect = subprocess.CalledProcessError(1, "loginctl") + + with pytest.raises(subprocess.CalledProcessError): + shutdown() diff --git a/tests/test_users.py b/tests/test_users.py index 0968795..0180ca8 100644 --- a/tests/test_users.py +++ b/tests/test_users.py @@ -174,3 +174,40 @@ class TestGetUserGtkTheme: result = get_user_gtk_theme(config_dir=gtk_dir) assert result is None + + def test_returns_none_for_corrupt_settings_ini(self, tmp_path: Path) -> None: + """settings.ini without section header should not crash.""" + gtk_dir = tmp_path / ".config" / "gtk-4.0" + gtk_dir.mkdir(parents=True) + settings = gtk_dir / "settings.ini" + settings.write_text("gtk-theme-name=Adwaita-dark\n") + + result = get_user_gtk_theme(config_dir=gtk_dir) + + assert result is None + + def test_handles_interpolation_characters(self, tmp_path: Path) -> None: + """Theme names with % characters should not trigger interpolation errors.""" + gtk_dir = tmp_path / ".config" / "gtk-4.0" + gtk_dir.mkdir(parents=True) + settings = gtk_dir / "settings.ini" + settings.write_text("[Settings]\ngtk-theme-name=My%Theme\n") + + result = get_user_gtk_theme(config_dir=gtk_dir) + + assert result == "My%Theme" + + def test_ignores_symlinked_accountsservice_icon(self, tmp_path: Path) -> None: + """AccountsService icon as symlink should be ignored to prevent traversal.""" + icons_dir = tmp_path / "icons" + icons_dir.mkdir() + target = tmp_path / "secret.txt" + target.write_text("sensitive data") + icon = icons_dir / "attacker" + icon.symlink_to(target) + + result = get_avatar_path( + "attacker", accountsservice_dir=icons_dir + ) + + assert result is None