From 18cba43f7f26f3ffec9c526df0bf6535058d5f5b Mon Sep 17 00:00:00 2001 From: jutty Date: Thu, 23 Apr 2026 16:09:22 -0300 Subject: [PATCH] Improve su_command parsing --- proptest-regressions/conf.txt | 9 ++ src/conf.rs | 186 +++++++++++++++++++++++++++++----- src/dev/test.rs | 40 ++++++-- 3 files changed, 199 insertions(+), 36 deletions(-) create mode 100644 proptest-regressions/conf.txt diff --git a/proptest-regressions/conf.txt b/proptest-regressions/conf.txt new file mode 100644 index 0000000..cf40de9 --- /dev/null +++ b/proptest-regressions/conf.txt @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 74b674162f67e0a42dccabd5bde0a39fa18f11dfaecb6971995502f93a07fa41 # shrinks to SuCommandWrapsIsReadFromConfigArgs = SuCommandWrapsIsReadFromConfigArgs { value: "" } +cc 00d0bfa64cfe51948411edf0dd89386ccc18f9644800172acf487b5373839cf8 # shrinks to ConfigurationParsesArgs = ConfigurationParsesArgs { raw: Raw { su_command: Some(""), su_command_wraps: None, merge_strategy: None } } +cc 7b467bb66998ed166bac12dfb9bc8980f02d852d65ae337e3fac7a5d93f03c9a # shrinks to ConfigurationParsesArgs = ConfigurationParsesArgs { raw: Raw { su_command: None, su_command_wraps: None, merge_strategy: Some("") } } diff --git a/src/conf.rs b/src/conf.rs index 65c3c3f..a01ad76 100644 --- a/src/conf.rs +++ b/src/conf.rs @@ -1,23 +1,29 @@ use std::{ collections::HashMap, fs::{self, DirEntry}, + os::unix::fs::PermissionsExt as _, path::PathBuf, }; -use crate::{ - dev::{log::elog}, - run::Command, -}; +use proptest_derive::Arbitrary; + +use crate::{dev::log::elog, run::Command}; + +#[derive(Debug, Arbitrary)] +struct Raw { + su_command: Option, + su_command_wraps: Option, + merge_strategy: Option, +} pub fn load() -> Result { elog("Loading configuration"); - let mut candidate = Configuration::default(); - let root = get_root(); elog(&format!("Reading 'tori.conf' from: {root:?}")); + let contents = fs::read_to_string(root.join("tori.conf"))?; - elog(&format!("Read configuration: {contents:?}")); + elog(&format!("Read configuration file: {contents:?}")); let map: HashMap = contents .lines() @@ -27,24 +33,49 @@ pub fn load() -> Result { elog(&format!("Assembled configuration map: {map:#?}")); - if let Some(su_command) = map.get("su_command") { - let wraps = map.get("su_command_wraps").is_some_and(|v| v == "true"); - candidate.su_command = parse_su_command(su_command, wraps)?; - } + let raw = Raw { + su_command: map.get("su_command").cloned(), + su_command_wraps: map.get("su_command_wraps").cloned(), + merge_strategy: map.get("merge_strategy").cloned(), + }; - if let Some(merge_strategy) = map.get("merge_strategy") { - candidate.merge_strategy = match merge_strategy.as_str() { - "prefer configuration" => MergeStrategy::PreferConfig, - "prefer system" => MergeStrategy::PreferSystem, - _ => MergeStrategy::default(), + elog(&format!("Read raw configuration: {raw:?}")); + Ok(parse(&raw)) +} + +fn parse(raw: &Raw) -> Configuration { + let default = Configuration::default(); + let mut candidate = default; + + if let Some(su_command_value) = &raw.su_command { + let wraps_value = match &raw.su_command_wraps { + Some(wraps) => wraps == "true", + None => false, + }; + + match parse_su_command(su_command_value, wraps_value) { + Ok(s) => candidate.su_command = s, + Err(error) => println!("Failed parsing su_comand configuration value: {error}"), } } - elog(&format!("Assembled configuration candidate: {candidate:?}")); - Ok(candidate) + if let Some(merge_strategy) = &raw.merge_strategy { + candidate.merge_strategy = match merge_strategy.as_str() { + "prefer configuration" => MergeStrategy::PreferConfig, + "prefer system" => MergeStrategy::PreferSystem, + any => { + println!("Unrecognized merge strategy: {any}"); + MergeStrategy::default() + } + } + } + + elog(&format!("Parsed configuration candidate: {candidate:?}")); + candidate } fn parse_su_command(config_value: &str, wraps: bool) -> Result { + // TODO this is a horrible way to split because it will unquote everything let split: Vec<&str> = config_value.split(' ').filter(|s| !s.is_empty()).collect(); let Some((base, args)) = split.split_first() else { @@ -54,13 +85,29 @@ fn parse_su_command(config_value: &str, wraps: bool) -> Result )); }; - let Ok(resolved_base) = resolve_command(base) else { - return Err(Error::new( - "su_command does not resolve to a command in PATH", - ErrorKind::CommandNotInPath, - )); + let resolved_base = if PathBuf::from(base).is_absolute() { + PathBuf::from(base) + } else { + resolve_command(base)? }; + if resolved_base.is_file() + && let Ok(metadata) = resolved_base.metadata() + { + let mode = metadata.permissions().mode(); + if mode & 0o111 == 0 { + return Err(Error::new( + "su_command path does not point to an executable file", + ErrorKind::WrongPermissions, + )); + } + } else { + return Err(Error::new( + "su_command path does not point to a file or its metadata is unreadable", + ErrorKind::MetadataUnreadable, + )); + } + let Some(resolved_base_str) = resolved_base.to_str() else { return Err(Error::new( "su_command path contains invalid characters (expected UTF-8)", @@ -227,11 +274,20 @@ impl From for Error { } } +#[cfg(test)] +impl From for proptest::test_runner::TestCaseError { + fn from(error: Error) -> proptest::test_runner::TestCaseError { + proptest::test_runner::TestCaseError::fail(format!("{}: {}", error.kind, error.message)) + } +} + #[derive(Debug)] pub enum ErrorKind { CommandNotInPath, VarError, MalformedConfigLine, + MetadataUnreadable, + WrongPermissions, UTF8, IO, } @@ -243,6 +299,8 @@ impl std::fmt::Display for ErrorKind { VarError => "Environment variable error", CommandNotInPath => "Command not in PATH", MalformedConfigLine => "Malformed configuration line", + MetadataUnreadable => "Metadata unreadable", + WrongPermissions => "Wrong permissions", UTF8 => "Invalid characters could not be decoded (expected UTF-8)", IO => "Input/Output error", }; @@ -250,13 +308,15 @@ impl std::fmt::Display for ErrorKind { } } +// TODO review this test #[cfg(test)] -#[expect(clippy::panic_in_result_fn, //clippy::unwrap_in_result -)] +#[expect(clippy::panic_in_result_fn)] mod serial_tests { - use std::{env, fs, os::unix::fs::PermissionsExt as _, io::{Write as _}}; + use proptest::property_test; + use std::{env, fs, io::Write as _, os::unix::fs::PermissionsExt as _}; + use super::*; - use crate::{dev::test::{Directories, Error}}; + use crate::dev::test::{Directories, Error}; #[test] fn failed_config_read() -> Result<(), Error> { @@ -299,8 +359,78 @@ mod serial_tests { let configuration = load()?; println!("configuration: {configuration:#?}"); - assert!(matches!(configuration.merge_strategy, MergeStrategy::PreferSystem)); + assert!(matches!( + configuration.merge_strategy, + MergeStrategy::PreferSystem + )); Ok(()) } + + #[property_test] + fn su_command_wrap_is_read_from_config(value: String) -> Result<(), Error> { + let dirs = Directories::setup("su_command_wraps_is_read_from_config")?; + + let mut conf = fs::File::create_new(&dirs.conf)?; + conf.write_all(format!("su_command_wraps = {value}").as_bytes())?; + conf.sync_all()?; + + let configuration = load()?; + let default = Configuration::default(); + + if value == "false" { + assert!(!configuration.su_command.wraps); + } else if configuration.su_command == default.su_command { + assert!(configuration.su_command.wraps); + } else { + assert!(value == "true"); + } + + Ok(()) + } + + #[property_test] + fn configuration_parses(raw: Raw) { + let parsed = parse(&raw); + let default = Configuration::default(); + + if let Some(su_command_value) = raw.su_command { + // these duplicated extractions are also in the tested + // code, this shpuld be in Command::from(&str) + let (base, args_opt) = match su_command_value.split_once(' ') { + Some((b, a)) => (b, Some(a)), + None => (su_command_value.as_str(), None), + }; + + let args = match args_opt { + Some(a) => vec![a], + None => vec![], + }; + + // this could also be a method of Command + if let Ok(resolved_su_command) = resolve_command(base) { + assert_eq!(parsed.su_command.command.base, resolved_su_command); + } else { + assert_eq!(parsed.su_command, default.su_command); + } + } else { + assert_eq!(parsed.su_command, default.su_command); + } + + if let Some(merge_strategy) = &raw.merge_strategy { + use MergeStrategy::*; + + // i guess this is fine (could be a match?) but it makes + // you think about how tests duplicate tautologies + if merge_strategy == "prefer system" { + assert!(matches!(parsed.merge_strategy, PreferSystem)); + } else if merge_strategy == "prefer configuration" { + assert!(matches!(parsed.merge_strategy, PreferConfig)); + } else { + assert!(matches!(parsed.merge_strategy, Interactive)); + } + } + + // TODO match raw.su_command_wraps {} + } } diff --git a/src/dev/test.rs b/src/dev/test.rs index 55bfd14..af6d452 100644 --- a/src/dev/test.rs +++ b/src/dev/test.rs @@ -1,6 +1,6 @@ use std::{env, fs, io, path::PathBuf}; -use crate::{dev::log::elog, conf}; +use crate::{conf, dev::log::elog}; #[derive(Debug)] pub struct Directories { @@ -31,14 +31,16 @@ impl Directories { return Err(Error::with_io( "Failed configuration root directory creation", error, - )) + )); } if let Err(error) = env::set_current_dir(&tube) { - return Err(Error::with_io("Failed current directory change", error)) + return Err(Error::with_io("Failed current directory change", error)); } - unsafe { env::set_var("XDG_CONFIG_DIR", &xdg_conf); } + unsafe { + env::set_var("XDG_CONFIG_DIR", &xdg_conf); + } Ok(Directories { original, @@ -76,7 +78,10 @@ impl Error { fn with_io(message: &str, inner: io::Error) -> Error { Error { message: String::from(message), - inner: Some(InnerErrors { io: Some(inner), conf: None }), + inner: Some(InnerErrors { + io: Some(inner), + conf: None, + }), } } } @@ -103,13 +108,18 @@ impl From for Error { } impl From<&str> for Error { - fn from(str: &str) -> Error { Error::from(String::from(str)) } + fn from(str: &str) -> Error { + Error::from(String::from(str)) + } } impl From for Error { fn from(inner: io::Error) -> Error { let mut error = Error::from(inner.to_string()); - error.inner = Some(InnerErrors { io: Some(inner), ..InnerErrors::default() }); + error.inner = Some(InnerErrors { + io: Some(inner), + ..InnerErrors::default() + }); error } } @@ -118,7 +128,21 @@ impl From for Error { fn from(conf_error: conf::Error) -> Error { Error { message: conf_error.message.clone(), - inner: Some(InnerErrors { conf: Some(conf_error), io: None }), + inner: Some(InnerErrors { + conf: Some(conf_error), + io: None, + }), } } } + +#[cfg(test)] +impl From for proptest::test_runner::TestCaseError { + fn from(error: Error) -> proptest::test_runner::TestCaseError { + proptest::test_runner::TestCaseError::fail(if let Some(inner) = error.inner { + format!("{}: {:#?}", error.message, inner) + } else { + error.message + }) + } +}