fix: Audit-Findings — Bugs, Security-Hardening und Performance

- 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
This commit is contained in:
nevaforget 2026-03-26 11:35:14 +01:00
parent af01e0a44d
commit c4b3dc833b
6 changed files with 139 additions and 21 deletions

View File

@ -5,13 +5,14 @@ import os
import shlex import shlex
import socket import socket
import stat import stat
import subprocess
from importlib.resources import files from importlib.resources import files
from pathlib import Path from pathlib import Path
import gi import gi
gi.require_version("Gtk", "4.0") gi.require_version("Gtk", "4.0")
gi.require_version("Gdk", "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.config import load_config
from moongreet.ipc import create_session, post_auth_response, start_session, cancel_session 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._sessions = get_sessions()
self._selected_user: User | None = None self._selected_user: User | None = None
self._greetd_sock: socket.socket | 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._build_ui()
self._select_initial_user() self._select_initial_user()
@ -217,16 +220,19 @@ class GreeterWindow(Gtk.ApplicationWindow):
self._password_entry.set_text("") self._password_entry.set_text("")
self._error_label.set_visible(False) self._error_label.set_visible(False)
# Update avatar # Update avatar (use cache if available)
avatar_path = get_avatar_path( if user.username in self._avatar_cache:
user.username, home_dir=user.home self._avatar_image.set_from_pixbuf(self._avatar_cache[user.username])
)
if avatar_path and avatar_path.exists():
self._set_avatar_from_file(avatar_path)
elif DEFAULT_AVATAR_PATH.exists():
self._set_default_avatar()
else: 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 # Apply user's GTK theme if available
self._apply_user_theme(user) self._apply_user_theme(user)
@ -243,9 +249,10 @@ class GreeterWindow(Gtk.ApplicationWindow):
if settings is None: if settings is None:
return 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) settings.set_property("gtk-theme-name", theme_name)
else: elif not theme_name and current:
settings.reset_property("gtk-theme-name") settings.reset_property("gtk-theme-name")
def _get_foreground_color(self) -> str: def _get_foreground_color(self) -> str:
@ -258,6 +265,9 @@ class GreeterWindow(Gtk.ApplicationWindow):
def _set_default_avatar(self) -> None: def _set_default_avatar(self) -> None:
"""Load the default avatar SVG, tinted with the GTK foreground color.""" """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: try:
svg_text = DEFAULT_AVATAR_PATH.read_text() svg_text = DEFAULT_AVATAR_PATH.read_text()
fg_color = self._get_foreground_color() fg_color = self._get_foreground_color()
@ -269,16 +279,19 @@ class GreeterWindow(Gtk.ApplicationWindow):
loader.close() loader.close()
pixbuf = loader.get_pixbuf() pixbuf = loader.get_pixbuf()
if pixbuf: if pixbuf:
self._default_avatar_pixbuf = pixbuf
self._avatar_image.set_from_pixbuf(pixbuf) self._avatar_image.set_from_pixbuf(pixbuf)
except (GLib.Error, OSError): except (GLib.Error, OSError):
self._avatar_image.set_from_icon_name("avatar-default-symbolic") 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.""" """Load an image file and set it as the avatar, scaled to AVATAR_SIZE."""
try: try:
pixbuf = GdkPixbuf.Pixbuf.new_from_file_at_scale( pixbuf = GdkPixbuf.Pixbuf.new_from_file_at_scale(
str(path), AVATAR_SIZE, AVATAR_SIZE, True str(path), AVATAR_SIZE, AVATAR_SIZE, True
) )
if username:
self._avatar_cache[username] = pixbuf
self._avatar_image.set_from_pixbuf(pixbuf) self._avatar_image.set_from_pixbuf(pixbuf)
except GLib.Error: except GLib.Error:
self._avatar_image.set_from_icon_name("avatar-default-symbolic") self._avatar_image.set_from_icon_name("avatar-default-symbolic")
@ -379,9 +392,21 @@ class GreeterWindow(Gtk.ApplicationWindow):
self._close_greetd_sock() self._close_greetd_sock()
return 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 # Step 3: Start session
if response.get("type") == "success": if response.get("type") == "success":
cmd = shlex.split(session.exec_cmd) 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) response = start_session(sock, cmd)
if response.get("type") == "success": if response.get("type") == "success":
@ -438,11 +463,17 @@ class GreeterWindow(Gtk.ApplicationWindow):
def _on_reboot_clicked(self, button: Gtk.Button) -> None: def _on_reboot_clicked(self, button: Gtk.Button) -> None:
"""Handle reboot button click.""" """Handle reboot button click."""
reboot() try:
reboot()
except subprocess.CalledProcessError:
self._show_error("Neustart fehlgeschlagen")
def _on_shutdown_clicked(self, button: Gtk.Button) -> None: def _on_shutdown_clicked(self, button: Gtk.Button) -> None:
"""Handle shutdown button click.""" """Handle shutdown button click."""
shutdown() try:
shutdown()
except subprocess.CalledProcessError:
self._show_error("Herunterfahren fehlgeschlagen")
@staticmethod @staticmethod
def _load_last_user() -> str | None: def _load_last_user() -> str | None:

View File

@ -5,8 +5,8 @@ import configparser
from dataclasses import dataclass from dataclasses import dataclass
from pathlib import Path from pathlib import Path
DEFAULT_WAYLAND_DIRS = [Path("/usr/share/wayland-sessions")] DEFAULT_WAYLAND_DIRS = (Path("/usr/share/wayland-sessions"),)
DEFAULT_XSESSION_DIRS = [Path("/usr/share/xsessions")] DEFAULT_XSESSION_DIRS = (Path("/usr/share/xsessions"),)
@dataclass @dataclass
@ -37,8 +37,8 @@ def _parse_desktop_file(path: Path, session_type: str) -> Session | None:
def get_sessions( def get_sessions(
wayland_dirs: list[Path] = DEFAULT_WAYLAND_DIRS, wayland_dirs: tuple[Path, ...] = DEFAULT_WAYLAND_DIRS,
xsession_dirs: list[Path] = DEFAULT_XSESSION_DIRS, xsession_dirs: tuple[Path, ...] = DEFAULT_XSESSION_DIRS,
) -> list[Session]: ) -> list[Session]:
"""Discover available sessions from .desktop files.""" """Discover available sessions from .desktop files."""
sessions: list[Session] = [] sessions: list[Session] = []

View File

@ -95,8 +95,11 @@ def get_user_gtk_theme(config_dir: Path | None = None) -> str | None:
if not settings_file.exists(): if not settings_file.exists():
return None return None
config = configparser.ConfigParser() config = configparser.ConfigParser(interpolation=None)
config.read(settings_file) try:
config.read(settings_file)
except configparser.Error:
return None
if config.has_option("Settings", "gtk-theme-name"): if config.has_option("Settings", "gtk-theme-name"):
return config.get("Settings", "gtk-theme-name") return config.get("Settings", "gtk-theme-name")

View File

@ -122,6 +122,38 @@ class TestLoginFlow:
finally: finally:
mock.stop() 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: def test_cancel_session(self, tmp_path: Path) -> None:
"""Simulate cancelling a session after create.""" """Simulate cancelling a session after create."""
sock_path = tmp_path / "greetd.sock" sock_path = tmp_path / "greetd.sock"

View File

@ -1,6 +1,7 @@
# ABOUTME: Tests for power actions — reboot and shutdown via loginctl. # ABOUTME: Tests for power actions — reboot and shutdown via loginctl.
# ABOUTME: Uses mocking to avoid actually calling system commands. # ABOUTME: Uses mocking to avoid actually calling system commands.
import subprocess
from unittest.mock import patch, call from unittest.mock import patch, call
import pytest import pytest
@ -19,6 +20,13 @@ class TestReboot:
["loginctl", "reboot"], check=True ["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: class TestShutdown:
"""Tests for the shutdown power action.""" """Tests for the shutdown power action."""
@ -30,3 +38,10 @@ class TestShutdown:
mock_run.assert_called_once_with( mock_run.assert_called_once_with(
["loginctl", "poweroff"], check=True ["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()

View File

@ -174,3 +174,40 @@ class TestGetUserGtkTheme:
result = get_user_gtk_theme(config_dir=gtk_dir) result = get_user_gtk_theme(config_dir=gtk_dir)
assert result is None 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