From 7c105164734534c571b5a34ac3ee44174a22f660 Mon Sep 17 00:00:00 2001
From: nevaforget <dom@moonarch.de>
Date: Sat, 28 Mar 2026 22:47:21 +0100
Subject: [PATCH] =?UTF-8?q?fix:=20re-audit=20findings=20=E2=80=94=20avatar?=
 =?UTF-8?q?=20path=20safety,=20persistence=20logging,=20tests?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Reject non-UTF-8 avatar paths early instead of passing empty string to GDK
- Log persistence write failures with warn! instead of silently discarding
- Reduce API surface: create_background_picture pub→fn
- Add boundary test for MAX_USERNAME_LENGTH and socket connect failure test
---
 src/greeter.rs | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/src/greeter.rs b/src/greeter.rs
index 346257c..a3e8d6b 100644
--- a/src/greeter.rs
+++ b/src/greeter.rs
@@ -174,7 +174,7 @@ pub fn create_wallpaper_window(
 }
 
 /// Create a Picture widget for the wallpaper background, optionally with GPU blur.
-pub fn create_background_picture(texture: &gdk::Texture, blur_radius: Option<f32>) -> gtk::Picture {
+fn create_background_picture(texture: &gdk::Texture, blur_radius: Option<f32>) -> gtk::Picture {
     let background = gtk::Picture::for_paintable(texture);
     background.set_content_fit(gtk::ContentFit::Cover);
     background.set_hexpand(true);
@@ -654,7 +654,13 @@ fn set_avatar_from_file(
         Ok(_) => {}
     }
 
-    match Pixbuf::from_file_at_scale(path.to_str().unwrap_or(""), AVATAR_SIZE, AVATAR_SIZE, true) {
+    let Some(path_str) = path.to_str() else {
+        log::debug!("Non-UTF-8 avatar path, skipping: {}", path.display());
+        image.set_icon_name(Some("avatar-default-symbolic"));
+        return;
+    };
+
+    match Pixbuf::from_file_at_scale(path_str, AVATAR_SIZE, AVATAR_SIZE, true) {
         Ok(pixbuf) => {
             let texture = gdk::Texture::for_pixbuf(&pixbuf);
             if let Some(name) = username {
@@ -1156,18 +1162,24 @@ fn save_last_user(username: &str) {
 
 fn save_last_user_to(path: &Path, username: &str) {
     log::debug!("Saving last user: {username}");
-    if let Some(parent) = path.parent() {
-        let _ = std::fs::create_dir_all(parent);
+    if let Some(parent) = path.parent()
+        && let Err(e) = std::fs::create_dir_all(parent)
+    {
+        log::warn!("Failed to create cache dir {}: {e}", parent.display());
+        return;
     }
     use std::os::unix::fs::OpenOptionsExt;
     use std::io::Write;
-    let _ = std::fs::OpenOptions::new()
+    if let Err(e) = std::fs::OpenOptions::new()
         .create(true)
         .write(true)
         .truncate(true)
         .mode(0o600)
         .open(path)
-        .and_then(|mut f| f.write_all(username.as_bytes()));
+        .and_then(|mut f| f.write_all(username.as_bytes()))
+    {
+        log::warn!("Failed to save last user to {}: {e}", path.display());
+    }
 }
 
 fn load_last_session(username: &str) -> Option<String> {
@@ -1212,13 +1224,16 @@ fn save_last_session_to(path: &Path, session_name: &str) {
     log::debug!("Saving last session: {session_name}");
     use std::os::unix::fs::OpenOptionsExt;
     use std::io::Write;
-    let _ = std::fs::OpenOptions::new()
+    if let Err(e) = std::fs::OpenOptions::new()
         .create(true)
         .write(true)
         .truncate(true)
         .mode(0o600)
         .open(path)
-        .and_then(|mut f| f.write_all(session_name.as_bytes()));
+        .and_then(|mut f| f.write_all(session_name.as_bytes()))
+    {
+        log::warn!("Failed to save last session to {}: {e}", path.display());
+    }
 }
 
 #[cfg(test)]
@@ -1233,6 +1248,7 @@ mod tests {
         assert!(is_valid_username("test.user"));
         assert!(is_valid_username("_admin"));
         assert!(is_valid_username("user@domain"));
+        assert!(is_valid_username(&"a".repeat(MAX_USERNAME_LENGTH)));
     }
 
     #[test]
@@ -1601,6 +1617,18 @@ mod tests {
         assert!(matches!(result, LoginResult::Cancelled));
     }
 
+    #[test]
+    fn login_worker_connect_failure() {
+        let cancelled = Arc::new(std::sync::atomic::AtomicBool::new(false));
+        let result = login_worker(
+            "alice", "pass", "/usr/bin/niri",
+            "/nonexistent/sock", &default_greetd_sock(), &cancelled,
+            load_strings(Some("en")),
+        );
+
+        assert!(result.is_err());
+    }
+
     #[test]
     fn login_worker_invalid_exec_cmd() {
         let (sock_path, handle) = fake_greetd(|stream| {