diff --git a/README.md b/README.md index bf7808b..051779e 100644 --- a/README.md +++ b/README.md @@ -606,6 +606,7 @@ file, in that order of precedence. Flags work before or after the subcommand. | `--listener-port` | `SOULSEEK_LISTENER_PORT` | `listener_port` | `2234` | | `--no-listener` / `--listener` | `SOULSEEK_NO_LISTENER` | `disable_listener` | listener on | | — | — | `accept_children` | off | +| — | — | `use_username_dirs` | off | | `--max-concurrent-downloads` | `SOULSEEK_MAX_CONCURRENT_DOWNLOADS` | `max_concurrent_downloads` | `20` | | `--search-timeout` | `SOULSEEK_SEARCH_TIMEOUT` | `search_timeout` | `10` | | `--daemon ADDR` | `SOULSEEK_DAEMON` | `daemon` | local socket if one is up | @@ -627,10 +628,10 @@ to them. It is off by default — each child costs a socket and a copy of the network's whole search stream — and it needs the listener, since a child has to be able to dial in. -`config get` and `config set` cover the twelve settings the file holds +`config get` and `config set` cover the thirteen settings the file holds (`username`, `server`, `listener_port`, `disable_listener`, `accept_children`, -`download_dir`, `shared_dirs`, `max_concurrent_downloads`, `search_timeout`, -`password_cmd`, `daemon`, `daemon_token`), and list them back at you when you +`download_dir`, `use_username_dirs`, `shared_dirs`, `max_concurrent_downloads`, +`search_timeout`, `password_cmd`, `daemon`, `daemon_token`), and list them back at you when you name something else. An empty string clears a key, and `shared_dirs` takes a comma-separated list. Waits expressed in seconds (`--search-timeout`, `--timeout`, diff --git a/soulseek-rs/src/commands/mod.rs b/soulseek-rs/src/commands/mod.rs index 65192e9..dbafa5c 100644 --- a/soulseek-rs/src/commands/mod.rs +++ b/soulseek-rs/src/commands/mod.rs @@ -34,6 +34,8 @@ pub struct Ctx { pub out: Out, pub settings: ClientSettings, pub download_dir: String, + /// Save each peer's files under `download_dir/`. + pub use_username_dirs: bool, pub max_concurrent_downloads: usize, pub search_timeout: Duration, /// Where to find a daemon, when this run should use one rather than open @@ -47,6 +49,16 @@ pub struct Ctx { } impl Ctx { + /// Where a download from `username` lands. + #[must_use] + pub fn dir_for(&self, username: &str) -> String { + crate::directories::download_dir_for( + &self.download_dir, + username, + self.use_username_dirs, + ) + } + /// Re-read a "found nothing", "timed out" or "transfer failed" verdict /// against the session that produced it. /// diff --git a/soulseek-rs/src/commands/transfer.rs b/soulseek-rs/src/commands/transfer.rs index b97b054..80bef8a 100644 --- a/soulseek-rs/src/commands/transfer.rs +++ b/soulseek-rs/src/commands/transfer.rs @@ -458,7 +458,7 @@ fn download_all( client.as_ref(), &ctx.out, &request, - &ctx.download_dir, + &ctx.dir_for(&request.user), timeout, ); if sender.send((request, outcome)).is_err() { diff --git a/soulseek-rs/src/daemon/mod.rs b/soulseek-rs/src/daemon/mod.rs index 43453d9..adeb316 100644 --- a/soulseek-rs/src/daemon/mod.rs +++ b/soulseek-rs/src/daemon/mod.rs @@ -202,6 +202,7 @@ pub fn run( browses: Arc::clone(&browses), downloads: Arc::clone(&downloads), download_dir: Mutex::new(ctx.download_dir.clone()), + use_username_dirs: ctx.use_username_dirs, server: ctx.settings.server_address.to_string(), token: token.clone(), started: Instant::now(), diff --git a/soulseek-rs/src/daemon/server/mod.rs b/soulseek-rs/src/daemon/server/mod.rs index 201a6d0..f11ac23 100644 --- a/soulseek-rs/src/daemon/server/mod.rs +++ b/soulseek-rs/src/daemon/server/mod.rs @@ -38,6 +38,7 @@ pub struct Daemon { /// Where transfers land, on *this* machine. A caller may point it at a /// different folder with `download.set_dir`, but never per download. pub download_dir: Mutex, + pub use_username_dirs: bool, pub server: String, pub token: String, pub started: Instant, @@ -428,7 +429,11 @@ impl Daemon { // per download would scatter transfers in ways `daemon.status` cannot // report; changing the folder is a deliberate session-wide act, // `download.set_dir`. - let directory = self.download_dir(); + let directory = crate::directories::download_dir_for( + &self.download_dir(), + ¶ms.username, + self.use_username_dirs, + ); let (download, updates) = self .session .download_with_metadata( @@ -800,6 +805,7 @@ mod tests { browses: Arc::new(PendingBrowses::default()), downloads: Arc::new(Mutex::new(Vec::new())), download_dir: Mutex::new("/tmp".to_string()), + use_username_dirs: false, server: "server.invalid:2416".to_string(), token: token.to_string(), started: Instant::now(), diff --git a/soulseek-rs/src/directories.rs b/soulseek-rs/src/directories.rs index 28375bc..3130846 100644 --- a/soulseek-rs/src/directories.rs +++ b/soulseek-rs/src/directories.rs @@ -50,9 +50,54 @@ pub fn resolve_shared_directories(dirs: &[String]) -> Vec { resolved } +/// The folder a download from `username` lands in: `download_dir` itself, or +/// a per-user subfolder of it when `per_user` is set, as SoulseekQt does. +#[must_use] +pub fn download_dir_for( + download_dir: &str, + username: &str, + per_user: bool, +) -> String { + if !per_user { + return download_dir.to_string(); + } + std::path::Path::new(download_dir) + .join(sanitize_path_component(username)) + .display() + .to_string() +} + +/// A peer-chosen name made safe as one path component: separators and +/// characters Windows refuses become `_`, so `../..` cannot climb out. +fn sanitize_path_component(name: &str) -> String { + let mut out: String = name + .chars() + .map(|c| match c { + '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_', + c if c.is_control() => '_', + c => c, + }) + .collect(); + if out.is_empty() || out.ends_with(['.', ' ']) { + out.push('_'); + } + if is_reserved(&out) { + out.insert(0, '_'); + } + out +} + +/// Windows device names (`CON`, `COM1`, …) that cannot be a folder name. +fn is_reserved(name: &str) -> bool { + let stem = name.split('.').next().unwrap_or(name).to_ascii_uppercase(); + matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL") + || (matches!(stem.get(..3), Some("COM" | "LPT")) + && matches!(stem.as_bytes().get(3..), Some([b'1'..=b'9']))) +} + #[cfg(test)] mod tests { - use super::resolve_shared_directory; + use super::{download_dir_for, resolve_shared_directory}; use std::sync::atomic::{AtomicU32, Ordering}; fn unique_temp_dir() -> std::path::PathBuf { @@ -97,4 +142,23 @@ mod tests { assert!(result.is_err()); let _ = std::fs::remove_dir_all(dir); } + + fn dl(name: &str) -> String { + std::path::Path::new("/dl").join(name).display().to_string() + } + + #[test] + fn per_user_folder_is_a_child_of_the_download_dir() { + assert_eq!(download_dir_for("/dl", "alice", false), "/dl"); + assert_eq!(download_dir_for("/dl", "alice", true), dl("alice")); + } + + #[test] + fn a_hostile_username_cannot_leave_the_download_dir() { + assert_eq!(download_dir_for("/dl", "../..", true), dl(".._.._")); + assert_eq!(download_dir_for("/dl", "/tmp/x", true), dl("_tmp_x")); + assert_eq!(download_dir_for("/dl", "..", true), dl(".._")); + assert_eq!(download_dir_for("/dl", "", true), dl("_")); + assert_eq!(download_dir_for("/dl", "com1", true), dl("_com1")); + } } diff --git a/soulseek-rs/src/persist/config.rs b/soulseek-rs/src/persist/config.rs index 6a4578e..376f48b 100644 --- a/soulseek-rs/src/persist/config.rs +++ b/soulseek-rs/src/persist/config.rs @@ -176,7 +176,7 @@ impl FileConfig { "use_username_dirs wants true or false, got '{other}'" )); } - }) + }); } other => { return Err(format!( diff --git a/soulseek-rs/src/run.rs b/soulseek-rs/src/run.rs index 89f15b4..f3419cc 100644 --- a/soulseek-rs/src/run.rs +++ b/soulseek-rs/src/run.rs @@ -276,6 +276,7 @@ fn context( version: ClientVersion::REFERENCE_CLIENT, }, download_dir, + use_username_dirs: resolved.use_username_dirs, max_concurrent_downloads: resolved.max_concurrent_downloads, search_timeout: Duration::from_secs(resolved.search_timeout), daemon, diff --git a/soulseek-rs/src/ui/main_tui/downloads.rs b/soulseek-rs/src/ui/main_tui/downloads.rs index 37cf177..b71dfc8 100644 --- a/soulseek-rs/src/ui/main_tui/downloads.rs +++ b/soulseek-rs/src/ui/main_tui/downloads.rs @@ -1,10 +1,9 @@ use super::MainTui; +use crate::directories::download_dir_for; use crate::models::{DownloadEntry, FileDisplayData}; use crate::ui::panes::{InfoSubject, selected_transfer}; -use crate::ui::sanitize_path_component; use soulseek_rs::types::UploadStatus; use soulseek_rs::{DownloadStatus, types::Download}; -use std::path::Path; use std::{sync::mpsc, thread}; impl MainTui { @@ -209,21 +208,15 @@ impl MainTui { peer_upload_speed: Some(file.speed), peer_free_slots: Some(file.slots), }; - let download_dir = if use_username_dirs { - let sanitized_username = - sanitize_path_component(file.username.as_str()); - Path::new(download_dir.as_str()) - .join(sanitized_username.as_str()) - .display() - .to_string() - } else { - download_dir.clone() - }; match client.download_with_metadata( file.filename.clone(), file.username.clone(), file.size, - download_dir, + download_dir_for( + &download_dir, + &file.username, + use_username_dirs, + ), metadata, ) { Ok((download, rx)) => { diff --git a/soulseek-rs/src/ui/main_tui/mod.rs b/soulseek-rs/src/ui/main_tui/mod.rs index 2b0a396..58a1664 100644 --- a/soulseek-rs/src/ui/main_tui/mod.rs +++ b/soulseek-rs/src/ui/main_tui/mod.rs @@ -12,13 +12,11 @@ use crate::persist::{ snapshot::{Snapshot, restore_messages, restore_searches}, state::{PersistedMessage, StateStore}, }; -use crate::ui::sanitize_path_component; use color_eyre::Result; use ratatui::{ DefaultTerminal, crossterm::event::{self, Event, KeyEventKind, poll}, }; -use std::path::Path; use std::{sync::Arc, time::Duration}; pub struct MainTui { @@ -134,23 +132,12 @@ impl MainTui { } else { let client = self.client.clone(); let sender = sender.clone(); - let use_username_dirs = self.use_username_dirs; std::thread::spawn(move || { - let sanitized_username = - sanitize_path_component(entry.username.as_str()); - let download_dir = if use_username_dirs { - Path::new(entry.download_directory.as_str()) - .join(sanitized_username.as_str()) - .display() - .to_string() - } else { - entry.download_directory - }; match client.download( entry.filename.clone(), entry.username, entry.size, - download_dir, + entry.download_directory, ) { Ok((download, rx)) => { let _ = sender.send((download, rx)); @@ -268,6 +255,7 @@ impl MainTui { } } +#[expect(clippy::too_many_arguments, reason = "one call site per mode")] pub fn launch_main_tui( terminal: DefaultTerminal, client: Arc, diff --git a/soulseek-rs/src/ui/main_tui/settings.rs b/soulseek-rs/src/ui/main_tui/settings.rs index 8416dc2..7668fda 100644 --- a/soulseek-rs/src/ui/main_tui/settings.rs +++ b/soulseek-rs/src/ui/main_tui/settings.rs @@ -109,6 +109,7 @@ impl MainTui { if dir_applied { self.download_dir.clone_from(&download_dir); } + self.use_username_dirs = use_username_dirs; // Persist to config.toml so the change survives a restart. What was // refused live is not written either: the file must not promise a diff --git a/soulseek-rs/src/ui/main_tui/tests/settings.rs b/soulseek-rs/src/ui/main_tui/tests/settings.rs index 0a419d3..8f72269 100644 --- a/soulseek-rs/src/ui/main_tui/tests/settings.rs +++ b/soulseek-rs/src/ui/main_tui/tests/settings.rs @@ -108,6 +108,30 @@ fn applying_settings_locally_stays_on_this_machine() { assert_eq!(tui.download_dir, downloads_dir); } +#[test] +fn toggling_username_folders_applies_live_and_persists() { + let session = Arc::new(TalkativeSession::default()); + let scratch = tempfile::tempdir().expect("a temp dir"); + let config_path = scratch.path().join("config.toml"); + let mut tui = MainTui::new( + session, + scratch.path().to_str().expect("utf-8").to_string(), + false, + Duration::from_secs(1), + None, + Some(config_path.clone()), + ); + + tui.open_settings(); + tui.state.settings.as_mut().expect("open").use_username_dirs = true; + tui.apply_settings(); + + assert!(tui.use_username_dirs, "the next download uses it"); + let saved = crate::persist::config::FileConfig::load(&config_path) + .expect("config saved"); + assert_eq!(saved.use_username_dirs, Some(true)); +} + /// The account block answers "who is this session, and what is it /// offering" without a trip to another screen. #[test] diff --git a/soulseek-rs/src/ui/utils.rs b/soulseek-rs/src/ui/utils.rs index 83c97c1..fd09f44 100644 --- a/soulseek-rs/src/ui/utils.rs +++ b/soulseek-rs/src/ui/utils.rs @@ -72,41 +72,6 @@ pub const fn get_spinner_char(state: usize) -> &'static str { SPINNER_CHARS[state % SPINNER_CHARS.len()] } -#[must_use] -pub fn sanitize_path_component(name: &str) -> String { - let mut out = name - .chars() - .map(|c| match c { - '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_', - c if c.is_control() => '_', - c => c, - }) - .collect::(); - if out.is_empty() || out.ends_with(['.', ' ']) { - out.push('_'); - } - if is_reserved(out.as_str()) { - out.insert(0, '_'); - } - out -} - -#[cfg(target_os = "windows")] -fn is_reserved(name: &str) -> bool { - let stem = name.split('.').next().unwrap_or(name).to_ascii_uppercase(); - if matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL") { - true - } else { - matches!(stem.get(..3), Some("COM" | "LPT")) - && matches!(stem.as_bytes().get(3..), Some([b'1'..=b'9'])) - } -} - -#[cfg(not(target_os = "windows"))] -fn is_reserved(name: &str) -> bool { - false -} - #[cfg(test)] mod tests { use super::*; diff --git a/website/src/pages/install.tsx b/website/src/pages/install.tsx index 9e03fb0..eac4943 100644 --- a/website/src/pages/install.tsx +++ b/website/src/pages/install.tsx @@ -129,6 +129,11 @@ const SETTINGS: readonly Setting[] = [ ), }, + { + key: 'use_username_dirs', + fallback: 'false', + body: <>Save each peer's files in a subfolder named after them., + }, { key: 'max_concurrent_downloads', fallback: '20',