From 8c480b7e39e911ddbfc4d564932d09dbdd7b6af6 Mon Sep 17 00:00:00 2001 From: JasonLG1979 Date: Sun, 5 Dec 2021 13:00:33 -0600 Subject: [PATCH] Fix Command line arguments incorrectly echoed in TRACE Fix up for #886 Closes: #898 And... * Don't silently ignore non-Unicode while parsing env vars. * Iterating over `std::env::args` will panic! on invalid unicode. Let's not do that. `getopts` will catch missing args and exit if those args are required after our error message about the arg not being valid unicode. * Gaurd against empty strings. There are a few places while parsing options strings that we don't immediately evaluate their validity let's at least makes sure that they are not empty if present. * `args` is only used in `get_setup` it doesn't need to be in main. * Nicer help header. * Get rid of `use std::io::{stderr, Write};` and just use `rpassword::prompt_password_stderr`. * Get rid of `get_credentials` it was clunky, ugly and only used once. There is no need for it to be a separate function. * Handle an empty password prompt and password prompt parsing errors. * + Other random misc clean ups. --- src/main.rs | 415 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 240 insertions(+), 175 deletions(-) diff --git a/src/main.rs b/src/main.rs index 2dec56ae..789654ff 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,7 +26,6 @@ mod player_event_handler; use player_event_handler::{emit_sink_event, run_program_on_events}; use std::env; -use std::io::{stderr, Write}; use std::ops::RangeInclusive; use std::path::Path; use std::pin::Pin; @@ -40,27 +39,16 @@ fn device_id(name: &str) -> String { } fn usage(program: &str, opts: &getopts::Options) -> String { - let brief = format!("Usage: {} [options]", program); + let repo_home = env!("CARGO_PKG_REPOSITORY"); + let desc = env!("CARGO_PKG_DESCRIPTION"); + let version = get_version_string(); + let brief = format!( + "{}\n\n{}\n\n{}\n\nUsage: {} []", + version, desc, repo_home, program + ); opts.usage(&brief) } -fn arg_to_var(arg: &str) -> String { - // To avoid name collisions environment variables must be prepended - // with `LIBRESPOT_` so option/flag `foo-bar` becomes `LIBRESPOT_FOO_BAR`. - format!("LIBRESPOT_{}", arg.to_uppercase().replace("-", "_")) -} - -fn env_var_present(arg: &str) -> bool { - env::var(arg_to_var(arg)).is_ok() -} - -fn env_var_opt_str(option: &str) -> Option { - match env::var(arg_to_var(option)) { - Ok(value) => Some(value), - Err(_) => None, - } -} - fn setup_logging(quiet: bool, verbose: bool) { let mut builder = env_logger::Builder::new(); match env::var("RUST_LOG") { @@ -102,29 +90,6 @@ fn list_backends() { } } -pub fn get_credentials Option>( - username: Option, - password: Option, - cached_credentials: Option, - prompt: F, -) -> Option { - if let Some(username) = username { - if let Some(password) = password { - return Some(Credentials::with_password(username, password)); - } - - match cached_credentials { - Some(credentials) if username == credentials.username => Some(credentials), - _ => { - let password = prompt(&username)?; - Some(Credentials::with_password(username, password)) - } - } - } else { - cached_credentials - } -} - #[derive(Debug, Error)] pub enum ParseFileSizeError { #[error("empty argument")] @@ -218,9 +183,10 @@ struct Setup { emit_sink_events: bool, } -fn get_setup(args: &[String]) -> Setup { - const VALID_NORMALISATION_KNEE_RANGE: RangeInclusive = 0.0..=2.0; +fn get_setup() -> Setup { + const VALID_INITIAL_VOLUME_RANGE: RangeInclusive = 0..=100; const VALID_VOLUME_RANGE: RangeInclusive = 0.0..=100.0; + const VALID_NORMALISATION_KNEE_RANGE: RangeInclusive = 0.0..=2.0; const VALID_NORMALISATION_PREGAIN_RANGE: RangeInclusive = -10.0..=10.0; const VALID_NORMALISATION_THRESHOLD_RANGE: RangeInclusive = -10.0..=0.0; const VALID_NORMALISATION_ATTACK_RANGE: RangeInclusive = 1..=500; @@ -596,25 +562,72 @@ fn get_setup(args: &[String]) -> Setup { "PORT", ); + let args: Vec<_> = std::env::args_os() + .filter_map(|s| match s.into_string() { + Ok(valid) => Some(valid), + Err(s) => { + eprintln!( + "Command line argument was not valid Unicode and will not be evaluated: {:?}", + s + ); + None + } + }) + .collect(); + let matches = match opts.parse(&args[1..]) { Ok(m) => m, - Err(f) => { - eprintln!( - "Error parsing command line options: {}\n{}", - f, - usage(&args[0], &opts) - ); + Err(e) => { + eprintln!("Error parsing command line options: {}", e); + println!("\n{}", usage(&args[0], &opts)); exit(1); } }; - let opt_present = |opt| matches.opt_present(opt) || env_var_present(opt); + let stripped_env_key = |k: &str| { + k.trim_start_matches("LIBRESPOT_") + .replace("_", "-") + .to_lowercase() + }; + + let env_vars: Vec<_> = env::vars_os().filter_map(|(k, v)| { + let mut env_var = None; + if let Ok(key) = k.into_string() { + if key.starts_with("LIBRESPOT_") { + let stripped_key = stripped_env_key(&key); + // Only match against long option/flag names. + // Something like LIBRESPOT_V for example is + // not valid because there are both -v and -V flags + // but env vars are assumed to be all uppercase. + let len = stripped_key.chars().count(); + if len > 1 && matches.opt_defined(&stripped_key) { + match v.into_string() { + Ok(value) => { + env_var = Some((key, value)); + }, + Err(s) => { + eprintln!("Environment variable was not valid Unicode and will not be evaluated: {}={:?}", key, s); + } + } + } + } + } + + env_var + }) + .collect(); + + let opt_present = + |opt| matches.opt_present(opt) || env_vars.iter().any(|(k, _)| stripped_env_key(k) == opt); let opt_str = |opt| { if matches.opt_present(opt) { matches.opt_str(opt) } else { - env_var_opt_str(opt) + env_vars + .iter() + .find(|(k, _)| stripped_env_key(k) == opt) + .map(|(_, v)| v.to_string()) } }; @@ -632,55 +645,43 @@ fn get_setup(args: &[String]) -> Setup { info!("{}", get_version_string()); - let librespot_env_vars: Vec = env::vars_os() - .filter_map(|(k, v)| { - let mut env_var = None; - if let Some(key) = k.to_str() { - if key.starts_with("LIBRESPOT_") { - if matches!(key, "LIBRESPOT_PASSWORD" | "LIBRESPOT_USERNAME") { - // Don't log creds. - env_var = Some(format!("\t\t{}=XXXXXXXX", key)); - } else if let Some(value) = v.to_str() { - env_var = Some(format!("\t\t{}={}", key, value)); - } - } - } - - env_var - }) - .collect(); - - if !librespot_env_vars.is_empty() { + if !env_vars.is_empty() { trace!("Environment variable(s):"); - for kv in librespot_env_vars { - trace!("{}", kv); + for (k, v) in &env_vars { + if matches!(k.as_str(), "LIBRESPOT_PASSWORD" | "LIBRESPOT_USERNAME") { + trace!("\t\t{}=\"XXXXXXXX\"", k); + } else if v.is_empty() { + trace!("\t\t{}=", k); + } else { + trace!("\t\t{}=\"{}\"", k, v); + } } } - let cmd_args = &args[1..]; + let args_len = args.len(); - let cmd_args_len = cmd_args.len(); - - if cmd_args_len > 0 { + if args_len > 1 { trace!("Command line argument(s):"); - for (index, key) in cmd_args.iter().enumerate() { - if key.starts_with('-') || key.starts_with("--") { - if matches!(key.as_str(), "--password" | "-p" | "--username" | "-u") { - // Don't log creds. - trace!("\t\t{} XXXXXXXX", key); - } else { - let mut value = "".to_string(); - let next = index + 1; - if next < cmd_args_len { - let next_key = cmd_args[next].clone(); - if !next_key.starts_with('-') && !next_key.starts_with("--") { - value = next_key; - } - } + for (index, key) in args.iter().enumerate() { + let opt = key.trim_start_matches('-'); - trace!("\t\t{} {}", key, value); + if index > 0 + && &args[index - 1] != key + && matches.opt_defined(opt) + && matches.opt_present(opt) + { + if matches!(opt, PASSWORD | PASSWORD_SHORT | USERNAME | USERNAME_SHORT) { + // Don't log creds. + trace!("\t\t{} \"XXXXXXXX\"", key); + } else { + let value = matches.opt_str(opt).unwrap_or_else(|| "".to_string()); + if value.is_empty() { + trace!("\t\t{}", key); + } else { + trace!("\t\t{} \"{}\"", key, value); + } } } } @@ -707,7 +708,7 @@ fn get_setup(args: &[String]) -> Setup { let backend = audio_backend::find(backend_name).unwrap_or_else(|| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", BACKEND, BACKEND_SHORT, opt_str(BACKEND).unwrap_or_default() @@ -720,7 +721,10 @@ fn get_setup(args: &[String]) -> Setup { .as_deref() .map(|format| { AudioFormat::from_str(format).unwrap_or_else(|_| { - error!("Invalid `--{}` / `-{}`: {}", FORMAT, FORMAT_SHORT, format); + error!( + "Invalid `--{}` / `-{}`: \"{}\"", + FORMAT, FORMAT_SHORT, format + ); println!( "Valid `--{}` / `-{}` values: F64, F32, S32, S24, S24_3, S16", FORMAT, FORMAT_SHORT @@ -743,9 +747,17 @@ fn get_setup(args: &[String]) -> Setup { feature = "rodio-backend", feature = "portaudio-backend" ))] - if device == Some("?".into()) { - backend(device, format); - exit(0); + if let Some(ref value) = device { + if value == "?" { + backend(device, format); + exit(0); + } else if value.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + DEVICE, DEVICE_SHORT + ); + exit(1); + } } #[cfg(not(any( @@ -774,7 +786,7 @@ fn get_setup(args: &[String]) -> Setup { let mixer = mixer::find(mixer_type.as_deref()).unwrap_or_else(|| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", MIXER_TYPE, MIXER_TYPE_SHORT, opt_str(MIXER_TYPE).unwrap_or_default() @@ -807,7 +819,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|index| { index.parse::().unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", ALSA_MIXER_INDEX, ALSA_MIXER_INDEX_SHORT, index ); println!("Default: {}", mixer_default_config.index); @@ -822,6 +834,15 @@ fn get_setup(args: &[String]) -> Setup { #[cfg(feature = "alsa-backend")] let control = opt_str(ALSA_MIXER_CONTROL).unwrap_or(mixer_default_config.control); + #[cfg(feature = "alsa-backend")] + if control.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + ALSA_MIXER_CONTROL, ALSA_MIXER_CONTROL_SHORT + ); + exit(1); + } + #[cfg(not(feature = "alsa-backend"))] let control = mixer_default_config.control; @@ -829,7 +850,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|range| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", VOLUME_RANGE, VOLUME_RANGE_SHORT, range ); println!( @@ -871,7 +892,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|volume_ctrl| { VolumeCtrl::from_str_with_range(volume_ctrl, volume_range).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", VOLUME_CTRL, VOLUME_CTRL_SHORT, volume_ctrl ); println!( @@ -918,7 +939,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|e| { e.unwrap_or_else(|e| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", CACHE_SIZE_LIMIT, CACHE_SIZE_LIMIT_SHORT, e ); exit(1); @@ -945,20 +966,57 @@ fn get_setup(args: &[String]) -> Setup { }; let credentials = { - let cached_credentials = cache.as_ref().and_then(Cache::credentials); + let cached_creds = cache.as_ref().and_then(Cache::credentials); - let password = |username: &String| -> Option { - write!(stderr(), "Password for {}: ", username).ok()?; - stderr().flush().ok()?; - rpassword::read_password().ok() - }; + if let Some(username) = opt_str(USERNAME) { + if username.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + USERNAME, USERNAME_SHORT + ); + exit(1); + } + if let Some(password) = opt_str(PASSWORD) { + if password.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + PASSWORD, PASSWORD_SHORT + ); + exit(1); + } + Some(Credentials::with_password(username, password)) + } else { + match cached_creds { + Some(creds) if username == creds.username => Some(creds), + _ => { + let prompt = &format!("Password for {}: ", username); - get_credentials( - opt_str(USERNAME), - opt_str(PASSWORD), - cached_credentials, - password, - ) + match rpassword::prompt_password_stderr(prompt) { + Ok(password) => { + if !password.is_empty() { + Some(Credentials::with_password(username, password)) + } else { + trace!("Password was empty."); + if cached_creds.is_some() { + trace!("Using cached credentials."); + } + cached_creds + } + } + Err(e) => { + warn!("Cannot parse password: {}", e); + if cached_creds.is_some() { + trace!("Using cached credentials."); + } + cached_creds + } + } + } + } + } + } else { + cached_creds + } }; let enable_discovery = !opt_present(DISABLE_DISCOVERY); @@ -980,12 +1038,14 @@ fn get_setup(args: &[String]) -> Setup { .map(|port| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", ZEROCONF_PORT, ZEROCONF_PORT_SHORT, port ); println!( - "Valid `--{}` / `-{}` values: 1 - 65535", - ZEROCONF_PORT, ZEROCONF_PORT_SHORT + "Valid `--{}` / `-{}` values: 1 - {}", + ZEROCONF_PORT, + ZEROCONF_PORT_SHORT, + u16::MAX ); }; @@ -1011,16 +1071,27 @@ fn get_setup(args: &[String]) -> Setup { let name = opt_str(NAME).unwrap_or_else(|| connect_default_config.name.clone()); + if name.is_empty() { + error!( + "`--{}` / `-{}` can not be an empty string", + NAME, NAME_SHORT + ); + exit(1); + } + let initial_volume = opt_str(INITIAL_VOLUME) .map(|initial_volume| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", INITIAL_VOLUME, INITIAL_VOLUME_SHORT, initial_volume ); println!( - "Valid `--{}` / `-{}` values: 0 - 100", - INITIAL_VOLUME, INITIAL_VOLUME_SHORT + "Valid `--{}` / `-{}` values: {} - {}", + INITIAL_VOLUME, + INITIAL_VOLUME_SHORT, + VALID_INITIAL_VOLUME_RANGE.start(), + VALID_INITIAL_VOLUME_RANGE.end() ); #[cfg(feature = "alsa-backend")] println!( @@ -1039,7 +1110,7 @@ fn get_setup(args: &[String]) -> Setup { exit(1); }); - if volume > 100 { + if !(VALID_INITIAL_VOLUME_RANGE).contains(&volume) { on_error(); exit(1); } @@ -1056,7 +1127,7 @@ fn get_setup(args: &[String]) -> Setup { .as_deref() .map(|device_type| { DeviceType::from_str(device_type).unwrap_or_else(|_| { - error!("Invalid `--{}` / `-{}`: {}", DEVICE_TYPE, DEVICE_TYPE_SHORT, device_type); + error!("Invalid `--{}` / `-{}`: \"{}\"", DEVICE_TYPE, DEVICE_TYPE_SHORT, device_type); println!("Valid `--{}` / `-{}` values: computer, tablet, smartphone, speaker, tv, avr, stb, audiodongle, \ gameconsole, castaudio, castvideo, automobile, smartwatch, chromebook, carthing, homething", DEVICE_TYPE, DEVICE_TYPE_SHORT @@ -1079,55 +1150,50 @@ fn get_setup(args: &[String]) -> Setup { } }; - let session_config = { - let device_id = device_id(&connect_config.name); - - SessionConfig { - user_agent: version::VERSION_STRING.to_string(), - device_id, - proxy: opt_str(PROXY).or_else(|| std::env::var("http_proxy").ok()).map( - |s| { - match Url::parse(&s) { - Ok(url) => { - if url.host().is_none() || url.port_or_known_default().is_none() { - error!("Invalid proxy url, only URLs on the format \"http://host:port\" are allowed"); - exit(1); - } - - if url.scheme() != "http" { - error!("Only unsecure http:// proxies are supported"); - exit(1); - } - - url - }, - Err(e) => { - error!("Invalid proxy URL: {}, only URLs in the format \"http://host:port\" are allowed", e); + let session_config = SessionConfig { + user_agent: version::VERSION_STRING.to_string(), + device_id: device_id(&connect_config.name), + proxy: opt_str(PROXY).or_else(|| std::env::var("http_proxy").ok()).map( + |s| { + match Url::parse(&s) { + Ok(url) => { + if url.host().is_none() || url.port_or_known_default().is_none() { + error!("Invalid proxy url, only URLs on the format \"http://host:port\" are allowed"); exit(1); } - } - }, - ), - ap_port: opt_str(AP_PORT) - .map(|port| { - let on_error = || { - error!("Invalid `--{}` / `-{}`: {}", AP_PORT, AP_PORT_SHORT, port); - println!("Valid `--{}` / `-{}` values: 1 - 65535", AP_PORT, AP_PORT_SHORT); - }; - let port = port.parse::().unwrap_or_else(|_| { - on_error(); - exit(1); - }); + if url.scheme() != "http" { + error!("Only unsecure http:// proxies are supported"); + exit(1); + } - if port == 0 { - on_error(); + url + }, + Err(e) => { + error!("Invalid proxy URL: \"{}\", only URLs in the format \"http://host:port\" are allowed", e); exit(1); } + } + }, + ), + ap_port: opt_str(AP_PORT).map(|port| { + let on_error = || { + error!("Invalid `--{}` / `-{}`: \"{}\"", AP_PORT, AP_PORT_SHORT, port); + println!("Valid `--{}` / `-{}` values: 1 - {}", AP_PORT, AP_PORT_SHORT, u16::MAX); + }; - port - }), - } + let port = port.parse::().unwrap_or_else(|_| { + on_error(); + exit(1); + }); + + if port == 0 { + on_error(); + exit(1); + } + + port + }), }; let player_config = { @@ -1138,7 +1204,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|bitrate| { Bitrate::from_str(bitrate).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", BITRATE, BITRATE_SHORT, bitrate ); println!( @@ -1200,7 +1266,7 @@ fn get_setup(args: &[String]) -> Setup { let method = NormalisationMethod::from_str(method).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT, method ); println!( @@ -1227,7 +1293,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|gain_type| { NormalisationType::from_str(gain_type).unwrap_or_else(|_| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_GAIN_TYPE, NORMALISATION_GAIN_TYPE_SHORT, gain_type ); println!( @@ -1244,7 +1310,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|pregain| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_PREGAIN, NORMALISATION_PREGAIN_SHORT, pregain ); println!( @@ -1275,7 +1341,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|threshold| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_THRESHOLD, NORMALISATION_THRESHOLD_SHORT, threshold ); println!( @@ -1309,7 +1375,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|attack| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_ATTACK, NORMALISATION_ATTACK_SHORT, attack ); println!( @@ -1343,7 +1409,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|release| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_RELEASE, NORMALISATION_RELEASE_SHORT, release ); println!( @@ -1377,7 +1443,7 @@ fn get_setup(args: &[String]) -> Setup { .map(|knee| { let on_error = || { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", NORMALISATION_KNEE, NORMALISATION_KNEE_SHORT, knee ); println!( @@ -1418,7 +1484,7 @@ fn get_setup(args: &[String]) -> Setup { Some(dither::find_ditherer(ditherer_name).unwrap_or_else(|| { error!( - "Invalid `--{}` / `-{}`: {}", + "Invalid `--{}` / `-{}`: \"{}\"", DITHER, DITHER_SHORT, opt_str(DITHER).unwrap_or_default() @@ -1488,8 +1554,7 @@ async fn main() { env::set_var(RUST_BACKTRACE, "full") } - let args: Vec = std::env::args().collect(); - let setup = get_setup(&args); + let setup = get_setup(); let mut last_credentials = None; let mut spirc: Option = None;