Compare commits

..

No commits in common. "main" and "v0.8.3" have entirely different histories.
main ... v0.8.3

9 changed files with 51 additions and 132 deletions

2
Cargo.lock generated
View File

@ -616,7 +616,7 @@ dependencies = [
[[package]] [[package]]
name = "moonset" name = "moonset"
version = "0.8.5" version = "0.8.3"
dependencies = [ dependencies = [
"dirs", "dirs",
"gdk-pixbuf", "gdk-pixbuf",

View File

@ -1,6 +1,6 @@
[package] [package]
name = "moonset" name = "moonset"
version = "0.8.5" version = "0.8.3"
edition = "2024" edition = "2024"
description = "Wayland session power menu with GTK4 and Layer Shell" description = "Wayland session power menu with GTK4 and Layer Shell"
license = "MIT" license = "MIT"

View File

@ -2,20 +2,6 @@
Architectural and design decisions for Moonset, in reverse chronological order. Architectural and design decisions for Moonset, in reverse chronological order.
## 2026-04-24 Audit LOW fixes: dead uid field, home_dir warn, clippy sweep, debug value (v0.8.5)
- **Who**: ClaudeCode, Dom
- **Why**: Five LOW findings cleared in one pass. (1) `User::uid` was populated from `getuid()` but never read — a compiler `dead_code` warning for a field on the public API. (2) Falling back to a synthetic user when `get_current_user()` returned None used `uid: u32::MAX`, an undocumented sentinel that became moot once uid was removed. (3) `dirs::home_dir().unwrap_or_default()` silently yielded `PathBuf::new()` on failure; avatars would then look for `.face` in the current working directory. (4) `cargo clippy` flagged three suggestions (two collapsible `if`, one redundant closure) that had crept in. (5) `MOONSET_DEBUG` promoted log verbosity on mere presence, leaking path information into the journal.
- **Tradeoffs**: Dropping `uid` from `User` is a minor API break for any internal caller expecting the field — none existed. The synthetic fallback now surfaces `log::warn!` when home resolution fails, which should be rare outside of pathological sandbox environments.
- **How**: (1) Remove `pub uid: u32` from `User` and the `uid: uid.as_raw()` assignment in `get_current_user`. (2) Panel fallback drops the `uid` field entirely. (3) `dirs::home_dir().unwrap_or_else(|| { log::warn!(...); PathBuf::new() })`. (4) `cargo clippy --fix` for the two collapsible ifs, manual collapse of `if-let` + `&&` chain, redundant closure replaced with the function itself. (5) `MOONSET_DEBUG` now requires the literal value `"1"` to escalate to Debug.
## 2026-04-24 Audit MEDIUM fixes: timeout guard, POSIX locale, button desensitize, wallpaper allowlist (v0.8.4)
- **Who**: ClaudeCode, Dom
- **Why**: Five MEDIUM findings: (1) `run_command`'s timeout thread leaked a 30 s gio::spawn_blocking slot if `child.wait()` errored, because `done.store(true)` ran after the `?`. (2) Timeout detection compared `status.signal() == Some(9)` — a hardcoded signal number that also misclassifies OOM-killer SIGKILL as our timeout. (3) `execute_action` never desensitized the button_box, so a double-click or accidental keyboard repeat fired the action twice. (4) `detect_locale` read only `LANG`, ignoring POSIX priority order (`LC_ALL` > `LC_MESSAGES` > `LANG`) — a common dual-language setup picked the wrong UI language. (5) The wallpaper path was passed to gdk-pixbuf without extension or size restriction, widening the image-parser attack surface and allowing unbounded decode latency.
- **Tradeoffs**: The extension allowlist (`jpg`, `jpeg`, `png`, `webp`) rejects exotic formats users might have used before. The 10 MB size cap rejects uncompressed/high-quality 4K wallpapers; acceptable for a power menu. Memory ordering on the `done` flag is now `Release`/`Acquire` instead of `Relaxed` — no runtime cost but correct across threads.
- **How**: (1) RAII `DoneGuard` struct sets `done.store(true, Release)` in its `Drop`, so the flag fires on every function exit path. A second `timed_out` AtomicBool distinguishes our SIGKILL from an external one. (2) Replace `Some(9)` with the `timed_out` flag check. (3) `execute_action` now takes `button_box: &gtk::Box`, calls `set_sensitive(false)` on entry and re-enables it on error paths; success paths that quit skip the re-enable. All call sites updated. (4) `detect_locale` reads `LC_ALL`, `LC_MESSAGES`, `LANG` in order, picking the first non-empty value before falling back to `/etc/locale.conf`. (5) `accept_wallpaper` helper applies extension allowlist + symlink rejection + `MAX_WALLPAPER_FILE_SIZE = 10 MB`, and is called for both config-path and Moonarch fallback.
## 2026-04-24 Audit fix: avoid latent stdout pipe deadlock in run_command (v0.8.3) ## 2026-04-24 Audit fix: avoid latent stdout pipe deadlock in run_command (v0.8.3)
- **Who**: ClaudeCode, Dom - **Who**: ClaudeCode, Dom

View File

@ -62,56 +62,21 @@ pub fn resolve_background_path(config: &Config) -> Option<PathBuf> {
resolve_background_path_with(config, Path::new(MOONARCH_WALLPAPER)) resolve_background_path_with(config, Path::new(MOONARCH_WALLPAPER))
} }
/// Wallpapers are passed to gdk-pixbuf's image loader; restrict to common image
/// extensions to reduce the parser-attack surface for user-controlled paths.
const ALLOWED_BG_EXT: &[&str] = &["jpg", "jpeg", "png", "webp"];
/// Bound wallpaper decode latency (10 MB covers typical 4K JPEGs at Q95).
const MAX_WALLPAPER_FILE_SIZE: u64 = 10 * 1024 * 1024;
fn is_allowed_wallpaper(path: &Path) -> bool {
match path.extension().and_then(|e| e.to_str()) {
Some(ext) => ALLOWED_BG_EXT.iter().any(|a| a.eq_ignore_ascii_case(ext)),
None => false,
}
}
fn accept_wallpaper(path: &Path) -> bool {
if !is_allowed_wallpaper(path) {
log::warn!("Wallpaper rejected (extension not in allowlist): {}", path.display());
return false;
}
match path.symlink_metadata() {
Ok(meta) if meta.file_type().is_symlink() => {
log::warn!("Wallpaper rejected (symlink): {}", path.display());
false
}
Ok(meta) if !meta.is_file() => false,
Ok(meta) if meta.len() > MAX_WALLPAPER_FILE_SIZE => {
log::warn!(
"Wallpaper rejected ({} bytes > {} limit): {}",
meta.len(), MAX_WALLPAPER_FILE_SIZE, path.display()
);
false
}
Ok(_) => true,
Err(_) => false,
}
}
/// Resolve with configurable moonarch wallpaper path (for testing). /// Resolve with configurable moonarch wallpaper path (for testing).
pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) -> Option<PathBuf> { pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) -> Option<PathBuf> {
// User-configured path — reject symlinks, non-image extensions, and oversized files // User-configured path — reject symlinks to prevent path traversal
if let Some(ref bg) = config.background_path { if let Some(ref bg) = config.background_path {
let path = PathBuf::from(bg); let path = PathBuf::from(bg);
if accept_wallpaper(&path) { if let Ok(meta) = path.symlink_metadata() {
log::debug!("Wallpaper source: config ({})", path.display()); if meta.is_file() && !meta.file_type().is_symlink() {
return Some(path); log::debug!("Wallpaper source: config ({})", path.display());
return Some(path);
}
} }
} }
// Moonarch ecosystem default — apply the same checks for consistency // Moonarch ecosystem default
if accept_wallpaper(moonarch_wallpaper) { if moonarch_wallpaper.is_file() {
log::debug!("Wallpaper source: moonarch default ({})", moonarch_wallpaper.display()); log::debug!("Wallpaper source: moonarch default ({})", moonarch_wallpaper.display());
return Some(moonarch_wallpaper.to_path_buf()); return Some(moonarch_wallpaper.to_path_buf());
} }

View File

@ -110,22 +110,15 @@ fn read_lang_from_conf(path: &Path) -> Option<String> {
None None
} }
/// Determine the system language from POSIX locale env vars or /etc/locale.conf. /// Determine the system language from LANG env var or /etc/locale.conf.
/// Checks LC_ALL, LC_MESSAGES, LANG in POSIX priority order (LC_ALL overrides
/// everything; LC_MESSAGES overrides LANG for text categories).
pub fn detect_locale() -> String { pub fn detect_locale() -> String {
let env_val = env::var("LC_ALL") detect_locale_with(env::var("LANG").ok().as_deref(), Path::new(DEFAULT_LOCALE_CONF))
.ok()
.filter(|s| !s.is_empty())
.or_else(|| env::var("LC_MESSAGES").ok().filter(|s| !s.is_empty()))
.or_else(|| env::var("LANG").ok().filter(|s| !s.is_empty()));
detect_locale_with(env_val.as_deref(), Path::new(DEFAULT_LOCALE_CONF))
} }
/// Determine locale with configurable inputs (for testing). /// Determine locale with configurable inputs (for testing).
pub fn detect_locale_with(env_lang: Option<&str>, locale_conf_path: &Path) -> String { pub fn detect_locale_with(env_lang: Option<&str>, locale_conf_path: &Path) -> String {
let (raw, source) = if let Some(val) = env_lang.filter(|s| !s.is_empty()) { let (raw, source) = if let Some(val) = env_lang.filter(|s| !s.is_empty()) {
(Some(val.to_string()), "env") (Some(val.to_string()), "LANG env")
} else if let Some(val) = read_lang_from_conf(locale_conf_path) { } else if let Some(val) = read_lang_from_conf(locale_conf_path) {
(Some(val), "locale.conf") (Some(val), "locale.conf")
} else { } else {

View File

@ -88,12 +88,10 @@ fn setup_logging() {
eprintln!("Failed to create journal logger: {e}"); eprintln!("Failed to create journal logger: {e}");
} }
} }
// Require MOONSET_DEBUG=1 to raise verbosity so mere presence (empty let level = if std::env::var("MOONSET_DEBUG").is_ok() {
// value in a session script) cannot escalate journal noise with path log::LevelFilter::Debug
// information an attacker could use. } else {
let level = match std::env::var("MOONSET_DEBUG").ok().as_deref() { log::LevelFilter::Info
Some("1") => log::LevelFilter::Debug,
_ => log::LevelFilter::Info,
}; };
log::set_max_level(level); log::set_max_level(level);
} }

View File

@ -7,7 +7,7 @@ use glib::clone;
use gtk4::prelude::*; use gtk4::prelude::*;
use gtk4::{self as gtk, gio}; use gtk4::{self as gtk, gio};
use std::cell::RefCell; use std::cell::RefCell;
use std::path::{Path, PathBuf}; use std::path::Path;
use std::rc::Rc; use std::rc::Rc;
use std::time::Duration; use std::time::Duration;
@ -208,16 +208,11 @@ pub fn create_panel_window(texture: Option<&gdk::Texture>, blur_radius: Option<f
window.add_css_class("panel"); window.add_css_class("panel");
let strings = load_strings(None); let strings = load_strings(None);
let user = users::get_current_user().unwrap_or_else(|| { let user = users::get_current_user().unwrap_or_else(|| users::User {
let home = dirs::home_dir().unwrap_or_else(|| { username: "user".to_string(),
log::warn!("Could not resolve HOME — using an empty path"); display_name: "User".to_string(),
PathBuf::new() home: dirs::home_dir().unwrap_or_default(),
}); uid: u32::MAX,
users::User {
username: "user".to_string(),
display_name: "User".to_string(),
home,
}
}); });
log::debug!("User: {} ({})", user.display_name, user.username); log::debug!("User: {} ({})", user.display_name, user.username);
@ -450,7 +445,7 @@ fn on_action_clicked(
error_label.set_visible(false); error_label.set_visible(false);
if !action_def.needs_confirm { if !action_def.needs_confirm {
execute_action(action_def, strings, app, confirm_area, confirm_box, error_label, button_box); execute_action(action_def, strings, app, confirm_area, confirm_box, error_label);
return; return;
} }
@ -493,8 +488,6 @@ fn show_confirm(
confirm_box, confirm_box,
#[weak] #[weak]
error_label, error_label,
#[weak]
button_box,
move |_| { move |_| {
execute_action( execute_action(
&action_def_clone, &action_def_clone,
@ -503,7 +496,6 @@ fn show_confirm(
&confirm_area, &confirm_area,
&confirm_box, &confirm_box,
&error_label, &error_label,
&button_box,
); );
} }
)); ));
@ -551,7 +543,6 @@ fn execute_action(
confirm_area: &gtk::Box, confirm_area: &gtk::Box,
confirm_box: &Rc<RefCell<Option<gtk::Box>>>, confirm_box: &Rc<RefCell<Option<gtk::Box>>>,
error_label: &gtk::Label, error_label: &gtk::Label,
button_box: &gtk::Box,
) { ) {
dismiss_confirm(confirm_area, confirm_box); dismiss_confirm(confirm_area, confirm_box);
log::debug!("Executing power action: {}", action_def.name); log::debug!("Executing power action: {}", action_def.name);
@ -561,10 +552,6 @@ fn execute_action(
let quit_after = action_def.quit_after; let quit_after = action_def.quit_after;
let error_message = (action_def.error_attr)(strings).to_string(); let error_message = (action_def.error_attr)(strings).to_string();
// Desensitize buttons so a double-click or accidental keyboard repeat
// cannot fire the same action twice while it is in flight.
button_box.set_sensitive(false);
// Use glib::spawn_future_local + gio::spawn_blocking to avoid Send issues // Use glib::spawn_future_local + gio::spawn_blocking to avoid Send issues
// with GTK objects. The blocking closure runs on a thread pool, the result // with GTK objects. The blocking closure runs on a thread pool, the result
// is handled back on the main thread. // is handled back on the main thread.
@ -573,30 +560,24 @@ fn execute_action(
app, app,
#[weak] #[weak]
error_label, error_label,
#[weak]
button_box,
async move { async move {
let result = gio::spawn_blocking(action_fn).await; let result = gio::spawn_blocking(move || action_fn()).await;
match result { match result {
Ok(Ok(())) => { Ok(Ok(())) => {
if quit_after { if quit_after {
fade_out_and_quit(&app); fade_out_and_quit(&app);
} else {
button_box.set_sensitive(true);
} }
} }
Ok(Err(e)) => { Ok(Err(e)) => {
log::error!("Power action '{}' failed: {}", action_name, e); log::error!("Power action '{}' failed: {}", action_name, e);
error_label.set_text(&error_message); error_label.set_text(&error_message);
error_label.set_visible(true); error_label.set_visible(true);
button_box.set_sensitive(true);
} }
Err(_) => { Err(_) => {
log::error!("Power action '{}' panicked", action_name); log::error!("Power action '{}' panicked", action_name);
error_label.set_text(&error_message); error_label.set_text(&error_message);
error_label.set_visible(true); error_label.set_visible(true);
button_box.set_sensitive(true);
} }
} }
} }

View File

@ -52,50 +52,44 @@ fn run_command(action: &'static str, program: &str, args: &[&str]) -> Result<(),
let child_pid = nix::unistd::Pid::from_raw(child.id() as i32); let child_pid = nix::unistd::Pid::from_raw(child.id() as i32);
let done = Arc::new(AtomicBool::new(false)); let done = Arc::new(AtomicBool::new(false));
let timed_out = Arc::new(AtomicBool::new(false));
let done_clone = done.clone(); let done_clone = done.clone();
let timed_out_clone = timed_out.clone();
let _timeout_thread = std::thread::spawn(move || { let timeout_thread = std::thread::spawn(move || {
// Sleep in short intervals so we can exit early when the child finishes
let interval = Duration::from_millis(100); let interval = Duration::from_millis(100);
let mut elapsed = Duration::ZERO; let mut elapsed = Duration::ZERO;
while elapsed < POWER_TIMEOUT { while elapsed < POWER_TIMEOUT {
std::thread::sleep(interval); std::thread::sleep(interval);
if done_clone.load(Ordering::Acquire) { if done_clone.load(Ordering::Relaxed) {
return; return;
} }
elapsed += interval; elapsed += interval;
} }
// Record that we fired the kill so we don't misclassify an external
// SIGKILL (OOM killer, kill -9) as our timeout.
timed_out_clone.store(true, Ordering::Release);
// ESRCH if the process already exited — harmless // ESRCH if the process already exited — harmless
let _ = nix::sys::signal::kill(child_pid, nix::sys::signal::Signal::SIGKILL); let _ = nix::sys::signal::kill(child_pid, nix::sys::signal::Signal::SIGKILL);
}); });
// Drop guard ensures the timeout thread sees done=true even if child.wait()
// errors out — otherwise the thread sleeps its full 30 s holding a slot in
// the gio::spawn_blocking pool.
struct DoneGuard(Arc<AtomicBool>);
impl Drop for DoneGuard {
fn drop(&mut self) {
self.0.store(true, Ordering::Release);
}
}
let _done_guard = DoneGuard(done);
let status = child.wait().map_err(|e| PowerError::CommandFailed { let status = child.wait().map_err(|e| PowerError::CommandFailed {
action, action,
message: e.to_string(), message: e.to_string(),
})?; })?;
done.store(true, Ordering::Relaxed);
let _ = timeout_thread.join();
if status.success() { if status.success() {
log::debug!("Power action {action} completed"); log::debug!("Power action {action} completed");
Ok(()) Ok(())
} else { } else {
if timed_out.load(Ordering::Acquire) { // Check if killed by our timeout (SIGKILL = signal 9)
return Err(PowerError::Timeout { action }); #[cfg(unix)]
{
use std::os::unix::process::ExitStatusExt;
if status.signal() == Some(9) {
return Err(PowerError::Timeout { action });
}
} }
let mut stderr_buf = String::new(); let mut stderr_buf = String::new();
if let Some(mut stderr) = child.stderr.take() { if let Some(mut stderr) = child.stderr.take() {
let _ = stderr.read_to_string(&mut stderr_buf); let _ = stderr.read_to_string(&mut stderr_buf);

View File

@ -12,6 +12,7 @@ pub struct User {
pub username: String, pub username: String,
pub display_name: String, pub display_name: String,
pub home: PathBuf, pub home: PathBuf,
pub uid: u32,
} }
/// Get the currently logged-in user's info from the system. /// Get the currently logged-in user's info from the system.
@ -36,6 +37,7 @@ pub fn get_current_user() -> Option<User> {
username: nix_user.name, username: nix_user.name,
display_name, display_name,
home: nix_user.dir, home: nix_user.dir,
uid: uid.as_raw(),
}) })
} }
@ -63,16 +65,16 @@ pub fn get_avatar_path_with(
} }
// AccountsService icon fallback // AccountsService icon fallback
if let Some(name) = username if let Some(name) = username {
&& accountsservice_dir.exists() if accountsservice_dir.exists() {
{ let icon = accountsservice_dir.join(name);
let icon = accountsservice_dir.join(name); if let Ok(meta) = icon.symlink_metadata() {
if let Ok(meta) = icon.symlink_metadata() { if meta.file_type().is_symlink() {
if meta.file_type().is_symlink() { log::warn!("Rejecting symlink avatar: {}", icon.display());
log::warn!("Rejecting symlink avatar: {}", icon.display()); } else if meta.is_file() {
} else if meta.is_file() { log::debug!("Avatar: using AccountsService icon ({})", icon.display());
log::debug!("Avatar: using AccountsService icon ({})", icon.display()); return Some(icon);
return Some(icon); }
} }
} }
} }