From f5dc25ae0d5b8d6fb15a534fa49557385d6894d0 Mon Sep 17 00:00:00 2001 From: Manos Pitsidianakis Date: Sun, 20 Mar 2022 16:31:55 +0200 Subject: [PATCH] conf.rs: check that all conf flags are recognized in validation This commit adds logic in configuration file validation that checks that each account "extra" field is empty after getting it back from the backend validation. This is to ensure the user doesn't set options that are invalidly stated in the documentation or by accident. Closes #135 Configuration error (xxx): the following flags are set but are not recognized: ["index_style"] https://git.meli.delivery/meli/meli/issues/135 --- Cargo.lock | 27 +++++++++ Cargo.toml | 5 +- docs/meli.conf.5 | 6 +- docs/samples/sample-config.toml | 12 ++-- melib/src/backends.rs | 4 +- melib/src/backends/imap.rs | 8 +-- melib/src/backends/jmap.rs | 29 +++++++++- melib/src/backends/maildir/backend.rs | 2 +- melib/src/backends/mbox.rs | 29 +++++++++- melib/src/backends/nntp.rs | 8 +-- melib/src/backends/notmuch.rs | 6 +- src/bin.rs | 2 +- src/conf.rs | 80 +++++++++++++++++++++++---- 13 files changed, 178 insertions(+), 40 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f9537aac..bd94774c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14,6 +14,15 @@ version = "0.4.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "739f4a8db6605981345c5654f3a85b056ce52f37a39d34da03f25bf2151ea16e" +[[package]] +name = "aho-corasick" +version = "0.7.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7404febffaa47dac81aa44dba71523c9d069b1bdc50a77db41195149e17f68e5" +dependencies = [ + "memchr", +] + [[package]] name = "arrayref" version = "0.3.6" @@ -1117,6 +1126,7 @@ dependencies = [ "pcre2", "proc-macro2", "quote 1.0.8", + "regex", "serde", "serde_derive", "serde_json", @@ -1673,6 +1683,23 @@ dependencies = [ "rust-argon2", ] +[[package]] +name = "regex" +version = "1.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2a26af418b574bd56588335b3a3659a65725d4e636eb1016c2f9e3b38c7cc759" +dependencies = [ + "aho-corasick", + "memchr", + "regex-syntax", +] + +[[package]] +name = "regex-syntax" +version = "0.6.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f497285884f3fcff424ffc933e56d7cbca511def0c9831a7f9b5f6153e3cc89b" + [[package]] name = "remove_dir_all" version = "0.5.3" diff --git a/Cargo.toml b/Cargo.toml index bc530354..b9563d2a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ futures = "0.3.5" async-task = "3.0.0" num_cpus = "1.12.0" flate2 = { version = "1.0.16", optional = true } - + [target.'cfg(target_os="linux")'.dependencies] notify-rust = { version = "^4", optional = true } @@ -64,6 +64,9 @@ quote = "^1.0" proc-macro2 = "1.0.18" flate2 = { version = "1.0.16", optional = true } +[dev-dependencies] +regex = "1" + [profile.release] lto = "fat" codegen-units = 1 diff --git a/docs/meli.conf.5 b/docs/meli.conf.5 index 4d8fc741..99a265af 100644 --- a/docs/meli.conf.5 +++ b/docs/meli.conf.5 @@ -77,7 +77,7 @@ example configuration [accounts.account-name] root_mailbox = "/path/to/root/folder" format = "Maildir" -index_style = "Compact" +listing.index_style = "Compact" identity="email@example.com" subscribed_mailboxes = ["folder", "folder/Sent"] # or [ "*", ] for all mailboxes display_name = "Name" @@ -93,7 +93,7 @@ display_name = "Name" [accounts.mbox] root_mailbox = "/var/mail/username" format = "mbox" -index_style = "Compact" +listing.index_style = "Compact" identity="username@hostname.local" composing.send_mail = { hostname = "localhost", port = 25, auth = { type = "none" }, security = { type = "none" } } @@ -138,8 +138,6 @@ The glob wildcard can be used to match every mailbox name and path. .It Ic identity Ar String Your e-mail address that is inserted in the From: headers of outgoing mail. -.It Ic index_style Ar String -Sets the way mailboxes are displayed. .El .TS allbox tab(:); diff --git a/docs/samples/sample-config.toml b/docs/samples/sample-config.toml index 58ff1134..2b2e5e92 100644 --- a/docs/samples/sample-config.toml +++ b/docs/samples/sample-config.toml @@ -11,7 +11,7 @@ #[accounts.account-name] #root_mailbox = "/path/to/root/mailbox" #format = "Maildir" -#index_style = "Conversations" # or [plain, threaded, compact] +#listing.index_style = "Conversations" # or [plain, threaded, compact] #identity="email@example.com" #display_name = "Name" #subscribed_mailboxes = ["INBOX", "INBOX/Sent", "INBOX/Drafts", "INBOX/Junk"] @@ -26,7 +26,7 @@ #[accounts.mbox] #root_mailbox = "/var/mail/username" #format = "mbox" -#index_style = "Compact" +#listing.index_style = "Compact" #identity="username@hostname.local" # ## Setting up an IMAP account @@ -36,10 +36,10 @@ #server_hostname="mail.example.com" #server_password="pha2hiLohs2eeeish2phaii1We3ood4chakaiv0hien2ahie3m" #server_username="username@example.com" -#server_port="993" # imaps +##server_port="993" # imaps #server_port="143" # STARTTLS #use_starttls=true #optional -#index_style = "Conversations" +#listing.index_style = "Conversations" #identity = "username@example.com" #display_name = "Name Name" ### match every mailbox: @@ -51,7 +51,7 @@ #[accounts.notmuch] #root_mailbox = "/path/to/folder" # where .notmuch/ directory is located #format = "notmuch" -#index_style = "conversations" +#listing.index_style = "conversations" #identity="username@example.com" #display_name = "Name Name" # # notmuch mailboxes are virtual, they are defined by their alias and the notmuch query that corresponds to their content. @@ -68,7 +68,7 @@ #server_password="password" #server_username="username@gmail.com" #server_port="993" -#index_style = "Conversations" +#listing.index_style = "Conversations" #identity = "username@gmail.com" #display_name = "Name Name" ### match every mailbox: diff --git a/melib/src/backends.rs b/melib/src/backends.rs index 57ef9feb..f50f1816 100644 --- a/melib/src/backends.rs +++ b/melib/src/backends.rs @@ -97,7 +97,7 @@ pub struct Backends { pub struct Backend { pub create_fn: Box BackendCreator>, - pub validate_conf_fn: Box Result<()>>, + pub validate_conf_fn: Box Result<()>>, } impl Default for Backends { @@ -200,7 +200,7 @@ impl Backends { self.map.insert(key, backend); } - pub fn validate_config(&self, key: &str, s: &AccountSettings) -> Result<()> { + pub fn validate_config(&self, key: &str, s: &mut AccountSettings) -> Result<()> { (self .map .get(key) diff --git a/melib/src/backends/imap.rs b/melib/src/backends/imap.rs index b7c947e1..b9b5bca3 100644 --- a/melib/src/backends/imap.rs +++ b/melib/src/backends/imap.rs @@ -1500,12 +1500,12 @@ impl ImapType { Ok(mailboxes) } - pub fn validate_config(s: &AccountSettings) -> Result<()> { + pub fn validate_config(s: &mut AccountSettings) -> Result<()> { let mut keys: HashSet<&'static str> = Default::default(); macro_rules! get_conf_val { ($s:ident[$var:literal]) => {{ keys.insert($var); - $s.extra.get($var).ok_or_else(|| { + $s.extra.remove($var).ok_or_else(|| { MeliError::new(format!( "Configuration error ({}): IMAP connection requires the field `{}` set", $s.name.as_str(), @@ -1516,9 +1516,9 @@ impl ImapType { ($s:ident[$var:literal], $default:expr) => {{ keys.insert($var); $s.extra - .get($var) + .remove($var) .map(|v| { - <_>::from_str(v).map_err(|e| { + <_>::from_str(&v).map_err(|e| { MeliError::new(format!( "Configuration error ({}): Invalid value for field `{}`: {}\n{}", $s.name.as_str(), diff --git a/melib/src/backends/jmap.rs b/melib/src/backends/jmap.rs index b8848adc..8421d130 100644 --- a/melib/src/backends/jmap.rs +++ b/melib/src/backends/jmap.rs @@ -874,7 +874,34 @@ impl JmapType { })) } - pub fn validate_config(s: &AccountSettings) -> Result<()> { + pub fn validate_config(s: &mut AccountSettings) -> Result<()> { + macro_rules! get_conf_val { + ($s:ident[$var:literal]) => { + $s.extra.remove($var).ok_or_else(|| { + MeliError::new(format!( + "Configuration error ({}): JMAP connection requires the field `{}` set", + $s.name.as_str(), + $var + )) + }) + }; + ($s:ident[$var:literal], $default:expr) => { + $s.extra + .remove($var) + .map(|v| { + <_>::from_str(&v).map_err(|e| { + MeliError::new(format!( + "Configuration error ({}): Invalid value for field `{}`: {}\n{}", + $s.name.as_str(), + $var, + v, + e + )) + }) + }) + .unwrap_or_else(|| Ok($default)) + }; + } get_conf_val!(s["server_hostname"])?; get_conf_val!(s["server_username"])?; get_conf_val!(s["server_password"])?; diff --git a/melib/src/backends/maildir/backend.rs b/melib/src/backends/maildir/backend.rs index e515c2c1..84e4515b 100644 --- a/melib/src/backends/maildir/backend.rs +++ b/melib/src/backends/maildir/backend.rs @@ -1316,7 +1316,7 @@ impl MaildirType { Ok(()) } - pub fn validate_config(s: &AccountSettings) -> Result<()> { + pub fn validate_config(s: &mut AccountSettings) -> Result<()> { let root_path = PathBuf::from(s.root_mailbox()).expand(); if !root_path.exists() { return Err(MeliError::new(format!( diff --git a/melib/src/backends/mbox.rs b/melib/src/backends/mbox.rs index fbc3cbb4..5dad56fa 100644 --- a/melib/src/backends/mbox.rs +++ b/melib/src/backends/mbox.rs @@ -1339,7 +1339,34 @@ impl MboxType { Ok(Box::new(ret)) } - pub fn validate_config(s: &AccountSettings) -> Result<()> { + pub fn validate_config(s: &mut AccountSettings) -> Result<()> { + macro_rules! get_conf_val { + ($s:ident[$var:literal]) => { + $s.extra.remove($var).ok_or_else(|| { + MeliError::new(format!( + "Configuration error ({}): mbox backend requires the field `{}` set", + $s.name.as_str(), + $var + )) + }) + }; + ($s:ident[$var:literal], $default:expr) => { + $s.extra + .remove($var) + .map(|v| { + <_>::from_str(&v).map_err(|e| { + MeliError::new(format!( + "Configuration error ({}): Invalid value for field `{}`: {}\n{}", + $s.name.as_str(), + $var, + v, + e + )) + }) + }) + .unwrap_or_else(|| Ok($default)) + }; + } let path = Path::new(s.root_mailbox.as_str()).expand(); if !path.exists() { return Err(MeliError::new(format!( diff --git a/melib/src/backends/nntp.rs b/melib/src/backends/nntp.rs index bf7fd0e0..342ddca9 100644 --- a/melib/src/backends/nntp.rs +++ b/melib/src/backends/nntp.rs @@ -654,12 +654,12 @@ impl NntpType { Ok(()) } - pub fn validate_config(s: &AccountSettings) -> Result<()> { + pub fn validate_config(s: &mut AccountSettings) -> Result<()> { let mut keys: HashSet<&'static str> = Default::default(); macro_rules! get_conf_val { ($s:ident[$var:literal]) => {{ keys.insert($var); - $s.extra.get($var).ok_or_else(|| { + $s.extra.remove($var).ok_or_else(|| { MeliError::new(format!( "Configuration error ({}): NNTP connection requires the field `{}` set", $s.name.as_str(), @@ -670,9 +670,9 @@ impl NntpType { ($s:ident[$var:literal], $default:expr) => {{ keys.insert($var); $s.extra - .get($var) + .remove($var) .map(|v| { - <_>::from_str(v).map_err(|e| { + <_>::from_str(&v).map_err(|e| { MeliError::new(format!( "Configuration error ({}) NNTP: Invalid value for field `{}`: {}\n{}", $s.name.as_str(), diff --git a/melib/src/backends/notmuch.rs b/melib/src/backends/notmuch.rs index fa7c3406..7bdb3fa0 100644 --- a/melib/src/backends/notmuch.rs +++ b/melib/src/backends/notmuch.rs @@ -372,7 +372,7 @@ impl NotmuchDb { })) } - pub fn validate_config(s: &AccountSettings) -> Result<()> { + pub fn validate_config(s: &mut AccountSettings) -> Result<()> { let path = Path::new(s.root_mailbox.as_str()).expand(); if !path.exists() { return Err(MeliError::new(format!( @@ -381,8 +381,8 @@ impl NotmuchDb { s.name() ))); } - for (k, f) in s.mailboxes.iter() { - if f.extra.get("query").is_none() { + for (k, f) in s.mailboxes.iter_mut() { + if f.extra.remove("query").is_none() { return Err(MeliError::new(format!( "notmuch mailbox configuration entry \"{}\" should have a \"query\" value set.", k diff --git a/src/bin.rs b/src/bin.rs index 750cf292..37689757 100644 --- a/src/bin.rs +++ b/src/bin.rs @@ -223,7 +223,7 @@ fn run_app(opt: Opt) -> Result<()> { } else { crate::conf::get_config_file()? }; - conf::FileSettings::validate(config_path, true)?; // TODO: test for tty/interaction + conf::FileSettings::validate(config_path, true, false)?; // TODO: test for tty/interaction return Ok(()); } Some(SubCommand::CreateConfig { path }) => { diff --git a/src/conf.rs b/src/conf.rs index 09bf7bb7..b92cf79b 100644 --- a/src/conf.rs +++ b/src/conf.rs @@ -350,10 +350,10 @@ impl FileSettings { return Err(MeliError::new("No configuration file found.")); } - FileSettings::validate(config_path, true) + FileSettings::validate(config_path, true, false) } - pub fn validate(path: PathBuf, interactive: bool) -> Result { + pub fn validate(path: PathBuf, interactive: bool, clear_extras: bool) -> Result { let s = pp::pp(&path)?; let map: toml::map::Map = toml::from_str(&s).map_err(|e| { MeliError::new(format!( @@ -391,7 +391,7 @@ This is required so that you don't accidentally start meli and find out later th err )) })?; - return FileSettings::validate(path, interactive); + return FileSettings::validate(path, interactive, clear_extras); } } return Err(MeliError::new(format!( @@ -439,7 +439,7 @@ This is required so that you don't accidentally start meli and find out later th } s.terminal.themes.validate()?; - for (name, acc) in &s.accounts { + for (name, acc) in s.accounts.iter_mut() { let FileAccount { root_mailbox, format, @@ -456,7 +456,7 @@ This is required so that you don't accidentally start meli and find out later th } = acc.clone(); let lowercase_format = format.to_lowercase(); - let s = AccountSettings { + let mut s = AccountSettings { name: name.to_string(), root_mailbox, format: format.clone(), @@ -471,7 +471,16 @@ This is required so that you don't accidentally start meli and find out later th .collect(), extra: extra.into_iter().collect(), }; - backends.validate_config(&lowercase_format, &s)?; + backends.validate_config(&lowercase_format, &mut s)?; + if !s.extra.is_empty() { + return Err(MeliError::new(format!( + "Unrecognised configuration values: {:?}", + s.extra + ))); + } + if clear_extras { + acc.extra.clear(); + } } Ok(s) @@ -1136,7 +1145,7 @@ fn test_config_parse() { [accounts.account-name] root_mailbox = "/path/to/root/mailbox" format = "Maildir" -index_style = "Conversations" # or [plain, threaded, compact] +listing.index_style = "Conversations" # or [plain, threaded, compact] identity="email@example.com" display_name = "Name" subscribed_mailboxes = ["INBOX", "INBOX/Sent", "INBOX/Drafts", "INBOX/Junk"] @@ -1151,11 +1160,25 @@ subscribed_mailboxes = ["INBOX", "INBOX/Sent", "INBOX/Drafts", "INBOX/Junk"] [accounts.mbox] root_mailbox = "/var/mail/username" format = "mbox" +listing.index_style = "Compact" +identity="username@hostname.local" +"#; + + const EXTRA_CONFIG: &str = r#" +[accounts.mbox] +root_mailbox = "/" +format = "mbox" index_style = "Compact" identity="username@hostname.local" + +[composing] +send_mail = '/bin/false' "#; + + const EXAMPLE_CONFIG: &str = include_str!("../docs/samples/sample-config.toml"); + impl ConfigFile { - fn new() -> std::result::Result { + fn new(content: &str) -> std::result::Result { let mut f = fs::File::open("/dev/urandom")?; let mut buf = [0u8; 16]; f.read_exact(&mut buf)?; @@ -1169,7 +1192,7 @@ identity="username@hostname.local" .create_new(true) .append(true) .open(&path)?; - file.write_all(TEST_CONFIG.as_bytes())?; + file.write_all(content.as_bytes())?; Ok(ConfigFile { path, file }) } } @@ -1180,13 +1203,46 @@ identity="username@hostname.local" } } - let mut new_file = ConfigFile::new().unwrap(); - let err = FileSettings::validate(new_file.path.clone(), false).unwrap_err(); + let mut new_file = ConfigFile::new(TEST_CONFIG).unwrap(); + let err = FileSettings::validate(new_file.path.clone(), false, true).unwrap_err(); assert!(err.details.as_ref().starts_with("You must set a global `composing` option. If you override `composing` in each account, you can use a dummy global like follows")); new_file .file .write_all("[composing]\nsend_mail = '/bin/false'\n".as_bytes()) .unwrap(); - let err = FileSettings::validate(new_file.path.clone(), false).unwrap_err(); + let err = FileSettings::validate(new_file.path.clone(), false, true).unwrap_err(); assert_eq!(err.details.as_ref(), "Configuration error (account-name): root_path `/path/to/root/mailbox` is not a valid directory."); + + /* Test unrecognised configuration entries error */ + + let new_file = ConfigFile::new(EXTRA_CONFIG).unwrap(); + let err = FileSettings::validate(new_file.path.clone(), false, true).unwrap_err(); + assert_eq!( + err.details.as_ref(), + "Unrecognised configuration values: {\"index_style\": \"Compact\"}" + ); + + /* Test sample config */ + + let example_config = EXAMPLE_CONFIG.replace("\n#", "\n"); + let re = regex::Regex::new(r#"root_mailbox\s*=\s*"[^"]*""#).unwrap(); + let example_config = re.replace_all( + &example_config, + &format!( + r#"root_mailbox = "{}""#, + std::env::temp_dir().to_str().unwrap() + ), + ); + + let new_file = ConfigFile::new(&example_config).unwrap(); + let config = FileSettings::validate(new_file.path.clone(), false, true) + .expect("Could not parse example config!"); + for (accname, acc) in config.accounts.iter() { + if !acc.extra.is_empty() { + panic!( + "In example config, account `{}` has unrecognised configuration entries: {:?}", + accname, acc.extra + ); + } + } }