From 9a8833ef60cd66a241bcee7217948e06400c8d35 Mon Sep 17 00:00:00 2001 From: Christian Visintin Date: Sat, 21 Mar 2026 13:06:25 +0100 Subject: [PATCH] fix: stabilize core error handling Remove production panic and unwrap paths from core modules. Propagate bookmark encryption failures, harden file watcher and temp mapped file handling, and clean up dead code in shared utilities. --- src/activity_manager.rs | 43 ++- src/cli/remote.rs | 12 +- src/explorer/builder.rs | 2 +- src/explorer/formatter.rs | 41 ++- src/host.rs | 2 +- src/host/remote_bridged/temp_mapped_file.rs | 89 ++++-- src/support/import_ssh_hosts.rs | 9 +- src/system/bookmarks_client.rs | 314 ++++++++++++-------- src/system/watcher.rs | 4 +- src/ui/activities/auth/bookmarks.rs | 10 +- src/ui/store.rs | 116 +------- src/utils.rs | 1 - src/utils/crypto.rs | 14 +- src/utils/fmt.rs | 6 +- src/utils/parser.rs | 27 +- 15 files changed, 365 insertions(+), 325 deletions(-) diff --git a/src/activity_manager.rs b/src/activity_manager.rs index 5083a13..5846654 100644 --- a/src/activity_manager.rs +++ b/src/activity_manager.rs @@ -94,7 +94,9 @@ impl ActivityManager { // local dir is remote_args.local_dir if set, otherwise current dir let local_dir = remote_args .local_dir - .unwrap_or_else(|| env::current_dir().unwrap()); + .map(Ok) + .unwrap_or_else(env::current_dir) + .map_err(|err| format!("Could not resolve current directory: {err}"))?; debug!("host bridge is None, setting local dir to {:?}", local_dir,); self.set_host_params( @@ -143,27 +145,26 @@ impl ActivityManager { match host { HostParams::HostBridge(HostBridgeParams::Localhost(path)) => { - self.context - .as_mut() - .unwrap() + self.context_mut()? .set_host_bridge_params(HostBridgeParams::Localhost(path)); } HostParams::HostBridge(HostBridgeParams::Remote(_, _)) => { - let (protocol, params) = remote_params.unwrap(); - self.context - .as_mut() - .unwrap() + let (protocol, params) = remote_params.ok_or_else(|| { + String::from("Missing remote parameters for host bridge configuration") + })?; + self.context_mut()? .set_host_bridge_params(HostBridgeParams::Remote(protocol, params)); } HostParams::Remote(_) => { - let (protocol, params) = remote_params.unwrap(); + let (protocol, params) = remote_params + .ok_or_else(|| String::from("Missing remote parameters for remote host"))?; let params = FileTransferParams { local_path: remote_local_path, remote_path: remote_remote_path, protocol, params, }; - self.context.as_mut().unwrap().set_remote_params(params); + self.context_mut()?.set_remote_params(params); } } Ok(()) @@ -185,8 +186,10 @@ impl ActivityManager { ) && params.generic_params().is_some() { // * if protocol is SCP or SFTP check whether a SSH key is registered for this remote, in case not ask password - let storage = SshKeyStorage::from(self.context.as_ref().unwrap().config()); - let generic_params = params.generic_params().unwrap(); + let storage = SshKeyStorage::from(self.context_ref()?.config()); + let generic_params = params.generic_params().ok_or_else(|| { + String::from("Missing generic parameters for SSH password resolution") + })?; let username = generic_params .username .clone() @@ -219,7 +222,7 @@ impl ActivityManager { /// Prompt user for password to set into params. fn prompt_password(&mut self, params: &mut ProtocolParams) -> Result<(), String> { - let ctx = self.context.as_mut().unwrap(); + let ctx = self.context_mut()?; let prompt = format!("Password for {}: ", params.host_name()); match tty::read_secret_from_tty(ctx.terminal(), prompt) { @@ -244,7 +247,7 @@ impl ActivityManager { bookmark_name: &str, password: Option<&str>, ) -> Result<(), String> { - if let Some(bookmarks_client) = self.context.as_mut().unwrap().bookmarks_client_mut() { + if let Some(bookmarks_client) = self.context_mut()?.bookmarks_client_mut() { let params = match bookmarks_client.get_bookmark(bookmark_name) { None => { return Err(format!( @@ -269,6 +272,18 @@ impl ActivityManager { } } + fn context_mut(&mut self) -> Result<&mut Context, String> { + self.context + .as_mut() + .ok_or_else(|| String::from("Activity manager context is not initialized")) + } + + fn context_ref(&self) -> Result<&Context, String> { + self.context + .as_ref() + .ok_or_else(|| String::from("Activity manager context is not initialized")) + } + /// /// Loop for activity manager. You need to provide the activity to start with /// Returns the exitcode diff --git a/src/cli/remote.rs b/src/cli/remote.rs index b146513..c455912 100644 --- a/src/cli/remote.rs +++ b/src/cli/remote.rs @@ -73,10 +73,16 @@ impl TryFrom<&Args> for RemoteArgs { // set args based on hosts len if hosts.len() == 1 { - remote_args.remote = hosts.pop().unwrap(); + remote_args.remote = hosts + .pop() + .ok_or_else(|| String::from("Missing remote host configuration"))?; } else if hosts.len() == 2 { - remote_args.host_bridge = hosts.pop().unwrap(); - remote_args.remote = hosts.pop().unwrap(); + remote_args.host_bridge = hosts + .pop() + .ok_or_else(|| String::from("Missing host-bridge configuration"))?; + remote_args.remote = hosts + .pop() + .ok_or_else(|| String::from("Missing remote host configuration"))?; } Ok(remote_args) diff --git a/src/explorer/builder.rs b/src/explorer/builder.rs index 59a3e89..c41276b 100644 --- a/src/explorer/builder.rs +++ b/src/explorer/builder.rs @@ -24,7 +24,7 @@ impl FileExplorerBuilder { /// Take FileExplorer out of builder pub fn build(&mut self) -> FileExplorer { - self.explorer.take().unwrap() + self.explorer.take().unwrap_or_default() } /// Enable HIDDEN_FILES option diff --git a/src/explorer/formatter.rs b/src/explorer/formatter.rs index b900095..4c6518f 100644 --- a/src/explorer/formatter.rs +++ b/src/explorer/formatter.rs @@ -108,7 +108,7 @@ impl CallChainBlock { None => { self.next_block = Some(Box::new(CallChainBlock::new( func, prefix, fmt_len, fmt_extra, - ))) + ))); } Some(block) => block.push(func, prefix, fmt_len, fmt_extra), } @@ -375,23 +375,20 @@ impl Formatter { // Add to cur str, prefix and the key value //format!("{cur_str}{prefix}{size:10}", size = size.display().si()) } else if fsentry.metadata().symlink.is_some() { - let size = ByteSize( - fsentry - .metadata() - .symlink - .as_ref() - .unwrap() - .to_string_lossy() - .len() as u64, - ); - let mut fmt = size.display().si().to_string(); - // pad with up to len 10 - let pad = 10usize.saturating_sub(fmt.len()); - for _ in 0..pad { - fmt.push(' '); - } + match fsentry.metadata().symlink.as_ref() { + Some(symlink) => { + let size = ByteSize(symlink.to_string_lossy().len() as u64); + let mut fmt = size.display().si().to_string(); + // pad with up to len 10 + let pad = 10usize.saturating_sub(fmt.len()); + for _ in 0..pad { + fmt.push(' '); + } - format!("{cur_str}{prefix}{fmt}") + format!("{cur_str}{prefix}{fmt}") + } + None => format!("{cur_str}{prefix} "), + } } else { // Add to cur str, prefix and the key value format!("{cur_str}{prefix} ") @@ -476,12 +473,14 @@ impl Formatter { let mut last_index: usize = 0; // Match fmt str against regex for regex_match in FMT_KEY_REGEX.captures_iter(fmt_str) { - // Get match index (unwrap is safe, since always exists) - let index: usize = fmt_str.find(®ex_match[0]).unwrap(); + let Some(full_match) = regex_match.get(0) else { + continue; + }; + let index: usize = full_match.start(); // Get prefix let prefix: String = String::from(&fmt_str[last_index..index]); // Increment last index (sum prefix lenght and the length of the key) - last_index += prefix.len() + regex_match[0].len(); + last_index += prefix.len() + full_match.as_str().len(); // Match attributes match FMT_ATTR_REGEX.captures(®ex_match[1]) { Some(regex_match) => { @@ -516,7 +515,7 @@ impl Formatter { match callchain.as_mut() { None => { callchain = - Some(CallChainBlock::new(callback, prefix, fmt_len, fmt_extra)) + Some(CallChainBlock::new(callback, prefix, fmt_len, fmt_extra)); } Some(chain_block) => chain_block.push(callback, prefix, fmt_len, fmt_extra), } diff --git a/src/host.rs b/src/host.rs index f1a1560..a74e5ff 100644 --- a/src/host.rs +++ b/src/host.rs @@ -19,7 +19,6 @@ pub type HostResult = Result; /// HostErrorType provides an overview of the specific host error #[derive(Error, Debug)] -#[allow(dead_code)] pub enum HostErrorType { #[error("No such file or directory")] NoSuchFileOrDirectory, @@ -37,6 +36,7 @@ pub enum HostErrorType { ExecutionFailed, #[error("Could not delete file")] DeleteFailed, + #[cfg(win)] #[error("Not implemented")] NotImplemented, #[error("remote fs error: {0}")] diff --git a/src/host/remote_bridged/temp_mapped_file.rs b/src/host/remote_bridged/temp_mapped_file.rs index 11cf6f6..4783cef 100644 --- a/src/host/remote_bridged/temp_mapped_file.rs +++ b/src/host/remote_bridged/temp_mapped_file.rs @@ -16,23 +16,20 @@ pub struct TempMappedFile { impl Write for TempMappedFile { fn write(&mut self, buf: &[u8]) -> std::io::Result { - let rc = self.write_hnd()?; - let mut ref_mut = rc.lock().unwrap(); - ref_mut.as_mut().unwrap().write(buf) + let mut handle = self.write_hnd()?; + handle.write(buf) } fn flush(&mut self) -> std::io::Result<()> { - let rc = self.write_hnd()?; - let mut ref_mut = rc.lock().unwrap(); - ref_mut.as_mut().unwrap().flush() + let mut handle = self.write_hnd()?; + handle.flush() } } impl Read for TempMappedFile { fn read(&mut self, buf: &mut [u8]) -> std::io::Result { - let rc = self.read_hnd()?; - let mut ref_mut = rc.lock().unwrap(); - ref_mut.as_mut().unwrap().read(buf) + let mut handle = self.read_hnd()?; + handle.read(buf) } } @@ -57,7 +54,13 @@ impl TempMappedFile { /// Must be called pub fn sync(&mut self) -> HostResult<()> { { - let mut lock = self.handle.lock().unwrap(); + let mut lock = self.lock_handle().map_err(|e| { + HostError::new( + HostErrorType::FileNotAccessible, + Some(e), + self.tempfile.path(), + ) + })?; if let Some(hnd) = lock.take() { hnd.sync_all().map_err(|e| { @@ -73,28 +76,62 @@ impl TempMappedFile { Ok(()) } - fn write_hnd(&mut self) -> io::Result>>> { - { - let mut lock = self.handle.lock().unwrap(); - if lock.is_none() { - let hnd = File::create(self.tempfile.path())?; - lock.replace(hnd); - } + fn write_hnd(&mut self) -> io::Result> { + let mut lock = self.lock_handle()?; + if lock.is_none() { + let hnd = File::create(self.tempfile.path())?; + lock.replace(hnd); } - Ok(self.handle.clone()) + Ok(FileHandle::new(lock)) } - fn read_hnd(&mut self) -> io::Result>>> { - { - let mut lock = self.handle.lock().unwrap(); - if lock.is_none() { - let hnd = File::open(self.tempfile.path())?; - lock.replace(hnd); - } + fn read_hnd(&mut self) -> io::Result> { + let mut lock = self.lock_handle()?; + if lock.is_none() { + let hnd = File::open(self.tempfile.path())?; + lock.replace(hnd); } - Ok(self.handle.clone()) + Ok(FileHandle::new(lock)) + } + + fn lock_handle(&self) -> io::Result>> { + self.handle + .lock() + .map_err(|_| io::Error::other("temporary file handle lock poisoned")) + } +} + +struct FileHandle<'a> { + guard: std::sync::MutexGuard<'a, Option>, +} + +impl<'a> FileHandle<'a> { + fn new(guard: std::sync::MutexGuard<'a, Option>) -> Self { + Self { guard } + } + + fn file_mut(&mut self) -> io::Result<&mut File> { + self.guard + .as_mut() + .ok_or_else(|| io::Error::other("temporary file handle is not initialized")) + } +} + +impl Write for FileHandle<'_> { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.file_mut()?.write(buf) + } + + fn flush(&mut self) -> io::Result<()> { + self.file_mut()?.flush() + } +} + +impl Read for FileHandle<'_> { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + self.file_mut()?.read(buf) } } diff --git a/src/support/import_ssh_hosts.rs b/src/support/import_ssh_hosts.rs index 9e8ae9b..b395d32 100644 --- a/src/support/import_ssh_hosts.rs +++ b/src/support/import_ssh_hosts.rs @@ -52,7 +52,9 @@ pub fn import_ssh_hosts(ssh_config: Option, keyring: bool) -> Result<() .flat_map(host_to_params) .for_each(|(name, params, identity_file_params)| { debug!("Adding bookmark for host: {name} with params: {params:?}"); - bookmarks_client.add_bookmark(name, params, false); + if let Err(err) = bookmarks_client.add_bookmark(name, params, false) { + error!("Could not add imported bookmark: {err}"); + } // add ssh key if any if let Some(identity_file_params) = identity_file_params { @@ -255,8 +257,7 @@ mod tests { struct SshTestConfig { config: NamedTempFile, - #[allow(dead_code)] - identity_file: NamedTempFile, + _identity_file: NamedTempFile, } fn ssh_test_config() -> SshTestConfig { @@ -320,7 +321,7 @@ Host test3 SshTestConfig { config: file, - identity_file, + _identity_file: identity_file, } } } diff --git a/src/system/bookmarks_client.rs b/src/system/bookmarks_client.rs index 57d4712..601eda5 100644 --- a/src/system/bookmarks_client.rs +++ b/src/system/bookmarks_client.rs @@ -187,15 +187,15 @@ impl BookmarksClient { name: S, params: FileTransferParams, save_password: bool, - ) { + ) -> Result<(), SerializerError> { let name: String = name.as_ref().to_string(); if name.is_empty() { error!("Bookmark name is empty; ignoring add_bookmark request"); - return; + return Ok(()); } // Make bookmark info!("Added bookmark {}", name); - let mut host: Bookmark = self.make_bookmark(params); + let mut host: Bookmark = self.make_bookmark(params)?; // If not save_password, set secrets to `None` if !save_password { host.password = None; @@ -205,6 +205,7 @@ impl BookmarksClient { } } self.hosts.bookmarks.insert(name, host); + Ok(()) } /// Delete entry from bookmarks @@ -226,9 +227,9 @@ impl BookmarksClient { } /// Add a new recent to bookmarks - pub fn add_recent(&mut self, params: FileTransferParams) { + pub fn add_recent(&mut self, params: FileTransferParams) -> Result<(), SerializerError> { // Make bookmark - let mut host: Bookmark = self.make_bookmark(params); + let mut host: Bookmark = self.make_bookmark(params)?; // Null password for recents host.password = None; if let Some(s3) = host.s3.as_mut() { @@ -240,7 +241,7 @@ impl BookmarksClient { if *value == host { debug!("Discarding recent since duplicated ({})", key); // Don't save duplicates - return; + return Ok(()); } } // If hosts size is bigger than self.recents_size; pop last @@ -265,6 +266,7 @@ impl BookmarksClient { let name: String = fmt_time(SystemTime::now(), "ISO%Y%m%dT%H%M%S"); info!("Saved recent host {}", name); self.hosts.recents.insert(name, host); + Ok(()) } /// Delete entry from recents @@ -329,27 +331,29 @@ impl BookmarksClient { } /// Make bookmark from credentials - fn make_bookmark(&self, params: FileTransferParams) -> Bookmark { + fn make_bookmark(&self, params: FileTransferParams) -> Result { let mut bookmark: Bookmark = Bookmark::from(params); // Encrypt password if let Some(pwd) = bookmark.password { - bookmark.password = Some(self.encrypt_str(pwd.as_str())); + bookmark.password = Some(self.encrypt_str(pwd.as_str())?); } // Encrypt aws s3 params if let Some(s3) = bookmark.s3.as_mut() { if let Some(access_key) = s3.access_key.as_mut() { - *access_key = self.encrypt_str(access_key.as_str()); + *access_key = self.encrypt_str(access_key.as_str())?; } if let Some(secret_access_key) = s3.secret_access_key.as_mut() { - *secret_access_key = self.encrypt_str(secret_access_key.as_str()); + *secret_access_key = self.encrypt_str(secret_access_key.as_str())?; } } - bookmark + Ok(bookmark) } /// Encrypt provided string using AES-128. Encrypted buffer is then converted to BASE64 - fn encrypt_str(&self, txt: &str) -> String { - crypto::aes128_b64_crypt(self.key.as_str(), txt) + fn encrypt_str(&self, txt: &str) -> Result { + crypto::aes128_b64_crypt(self.key.as_str(), txt).map_err(|err| { + SerializerError::new_ex(SerializerErrorKind::Serialization, err.to_string()) + }) } /// Decrypt provided string using AES-128 @@ -403,24 +407,32 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add some bookmarks - client.add_bookmark( - "raspberry", - make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - ), - true, + assert!( + client + .add_bookmark( + "raspberry", + make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + ), + true, + ) + .is_ok() + ); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() ); - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - )); let recent_key: String = String::from(client.iter_recents().next().unwrap()); assert!(client.write_bookmarks().is_ok()); let key: String = client.key.clone(); @@ -451,7 +463,11 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add s3 bookmark - client.add_bookmark("my-bucket", make_s3_ftparams(), true); + assert!( + client + .add_bookmark("my-bucket", make_s3_ftparams(), true) + .is_ok() + ); // Verify bookmark let bookmark = client.get_bookmark("my-bucket").unwrap(); assert_eq!(bookmark.protocol, FileTransferProtocol::AwsS3); @@ -471,7 +487,11 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add s3 bookmark - client.add_bookmark("my-bucket", make_s3_ftparams(), false); + assert!( + client + .add_bookmark("my-bucket", make_s3_ftparams(), false) + .is_ok() + ); // Verify bookmark let bookmark = client.get_bookmark("my-bucket").unwrap(); assert_eq!(bookmark.protocol, FileTransferProtocol::AwsS3); @@ -492,7 +512,7 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add s3 bookmark - client.add_recent(make_s3_ftparams()); + assert!(client.add_recent(make_s3_ftparams()).is_ok()); // Verify bookmark let bookmark = client.iter_recents().next().unwrap(); let bookmark = client.get_recent(bookmark).unwrap(); @@ -515,27 +535,35 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add bookmark - client.add_bookmark( - "raspberry", - make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - ), - true, + assert!( + client + .add_bookmark( + "raspberry", + make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + ), + true, + ) + .is_ok() ); - client.add_bookmark( - "raspberry2", - make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword2"), - ), - true, + assert!( + client + .add_bookmark( + "raspberry2", + make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword2"), + ), + true, + ) + .is_ok() ); // Iter assert_eq!(client.iter_bookmarks().count(), 2); @@ -564,16 +592,20 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add bookmark with empty name should be silently ignored - client.add_bookmark( - "", - make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - ), - true, + assert!( + client + .add_bookmark( + "", + make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + ), + true, + ) + .is_ok() ); // No bookmark should have been added assert_eq!(client.iter_bookmarks().count(), 0); @@ -587,16 +619,20 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add bookmark - client.add_bookmark( - "raspberry", - make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - ), - false, + assert!( + client + .add_bookmark( + "raspberry", + make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + ), + false, + ) + .is_ok() ); let bookmark = ftparams_to_tup(client.get_bookmark(&String::from("raspberry")).unwrap()); assert_eq!(bookmark.0, String::from("192.168.1.31")); @@ -615,13 +651,17 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add bookmark - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - )); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() + ); // Iter assert_eq!(client.iter_recents().count(), 1); let key: String = String::from(client.iter_recents().next().unwrap()); @@ -651,20 +691,28 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add bookmark - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - )); - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - )); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() + ); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() + ); // There should be only one recent assert_eq!(client.iter_recents().count(), 1); } @@ -679,31 +727,43 @@ mod tests { BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 2, true).unwrap(); // Add recent, wait 1 second for each one (cause the name depends on time) // 1 - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.1", - 22, - "pi", - Some("mypassword"), - )); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.1", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() + ); sleep(Duration::from_secs(1)); // 2 - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.2", - 22, - "pi", - Some("mypassword"), - )); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.2", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() + ); sleep(Duration::from_secs(1)); // 3 - client.add_recent(make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.3", - 22, - "pi", - Some("mypassword"), - )); + assert!( + client + .add_recent(make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.3", + 22, + "pi", + Some("mypassword"), + )) + .is_ok() + ); // Limit is 2 assert_eq!(client.iter_recents().count(), 2); // Check that 192.168.1.1 has been removed @@ -745,16 +805,20 @@ mod tests { let mut client: BookmarksClient = BookmarksClient::new(cfg_path.as_path(), key_path.as_path(), 16, true).unwrap(); // Add bookmark with empty name should be silently ignored - client.add_bookmark( - "", - make_generic_ftparams( - FileTransferProtocol::Sftp, - "192.168.1.31", - 22, - "pi", - Some("mypassword"), - ), - true, + assert!( + client + .add_bookmark( + "", + make_generic_ftparams( + FileTransferProtocol::Sftp, + "192.168.1.31", + 22, + "pi", + Some("mypassword"), + ), + true, + ) + .is_ok() ); // No bookmark should have been added assert_eq!(client.iter_bookmarks().count(), 0); diff --git a/src/system/watcher.rs b/src/system/watcher.rs index 0da7fa1..c465e10 100644 --- a/src/system/watcher.rs +++ b/src/system/watcher.rs @@ -27,6 +27,8 @@ pub enum FsWatcherError { PathNotWatched, #[error("unable to watch path, since it's already watched")] PathAlreadyWatched, + #[error("watcher event channel disconnected")] + Disconnected, #[error("unknown event: {0}")] UnknownEvent(&'static str), #[error("worker error: {0}")] @@ -114,7 +116,7 @@ impl FsWatcher { let res = match self.receiver.recv_timeout(Duration::from_millis(1)) { Ok(res) => res, Err(RecvTimeoutError::Timeout) => return Ok(None), - Err(RecvTimeoutError::Disconnected) => panic!("File watcher died"), + Err(RecvTimeoutError::Disconnected) => return Err(FsWatcherError::Disconnected), }; // convert event to FsChange diff --git a/src/ui/activities/auth/bookmarks.rs b/src/ui/activities/auth/bookmarks.rs index 06f9e7f..110cbac 100644 --- a/src/ui/activities/auth/bookmarks.rs +++ b/src/ui/activities/auth/bookmarks.rs @@ -71,7 +71,10 @@ impl AuthActivity { }; if let Some(bookmarks_cli) = self.bookmarks_client_mut() { - bookmarks_cli.add_bookmark(name.clone(), params, save_password); + if let Err(err) = bookmarks_cli.add_bookmark(name.clone(), params, save_password) { + self.mount_error(format!("Could not save bookmark: {err}")); + return; + } // Save bookmarks self.write_bookmarks(); // Remove `name` from bookmarks if exists @@ -121,7 +124,10 @@ impl AuthActivity { } }; if let Some(bookmarks_cli) = self.bookmarks_client_mut() { - bookmarks_cli.add_recent(params); + if let Err(err) = bookmarks_cli.add_recent(params) { + self.mount_error(format!("Could not save recent host: {err}")); + return; + } // Save bookmarks self.write_bookmarks(); } diff --git a/src/ui/store.rs b/src/ui/store.rs index 8fd24ca..abc255a 100644 --- a/src/ui/store.rs +++ b/src/ui/store.rs @@ -9,14 +9,10 @@ use std::collections::HashMap; // -- store state /// Store state describes a value in the store -#[allow(dead_code)] enum StoreState { - Str(String), // String - Signed(isize), // Signed number - Unsigned(usize), // Unsigned number - Float(f64), // Floating point number - Boolean(bool), // Boolean value - Flag, // Empty value; used to work as a Flag (set unset) + Str(String), // String + Boolean(bool), // Boolean value + Flag, // Empty value; used to work as a Flag (set unset) } // -- store @@ -28,7 +24,6 @@ pub(crate) struct Store { store: HashMap, } -#[allow(dead_code)] impl Store { /// Initialize a new Store pub fn init() -> Self { @@ -47,30 +42,6 @@ impl Store { } } - /// Get signed from store - pub fn get_signed(&self, key: &str) -> Option { - match self.store.get(key) { - Some(StoreState::Signed(i)) => Some(*i), - _ => None, - } - } - - /// Get unsigned from store - pub fn get_unsigned(&self, key: &str) -> Option { - match self.store.get(key) { - Some(StoreState::Unsigned(u)) => Some(*u), - _ => None, - } - } - - /// get float from store - pub fn get_float(&self, key: &str) -> Option { - match self.store.get(key) { - Some(StoreState::Float(f)) => Some(*f), - _ => None, - } - } - /// get boolean from store pub fn get_boolean(&self, key: &str) -> Option { match self.store.get(key) { @@ -91,22 +62,6 @@ impl Store { self.store.insert(key.to_string(), StoreState::Str(val)); } - /// Set signed number - pub fn set_signed(&mut self, key: &str, val: isize) { - self.store.insert(key.to_string(), StoreState::Signed(val)); - } - - /// Set unsigned number - pub fn set_unsigned(&mut self, key: &str, val: usize) { - self.store - .insert(key.to_string(), StoreState::Unsigned(val)); - } - - /// Set floating point number - pub fn set_float(&mut self, key: &str, val: f64) { - self.store.insert(key.to_string(), StoreState::Float(val)); - } - /// Set boolean pub fn set_boolean(&mut self, key: &str, val: bool) { self.store.insert(key.to_string(), StoreState::Boolean(val)); @@ -118,46 +73,6 @@ impl Store { } // -- Consumers - - /// Take string from store - pub fn take_string(&mut self, key: &str) -> Option { - match self.store.remove(key) { - Some(StoreState::Str(s)) => Some(s), - _ => None, - } - } - - /// Take signed from store - pub fn take_signed(&mut self, key: &str) -> Option { - match self.store.remove(key) { - Some(StoreState::Signed(i)) => Some(i), - _ => None, - } - } - - /// Take unsigned from store - pub fn take_unsigned(&mut self, key: &str) -> Option { - match self.store.remove(key) { - Some(StoreState::Unsigned(u)) => Some(u), - _ => None, - } - } - - /// Take float from store - pub fn take_float(&mut self, key: &str) -> Option { - match self.store.remove(key) { - Some(StoreState::Float(f)) => Some(f), - _ => None, - } - } - - /// Take boolean from store - pub fn take_boolean(&mut self, key: &str) -> Option { - match self.store.remove(key) { - Some(StoreState::Boolean(b)) => Some(b), - _ => None, - } - } } #[cfg(test)] @@ -174,39 +89,14 @@ mod tests { // Test string store.set_string("test", String::from("hello")); assert_eq!(*store.get_string("test").as_ref().unwrap(), "hello"); - assert_eq!(store.take_string("test").unwrap(), "hello".to_string()); - assert_eq!(store.take_string("test"), None); - // Test isize - store.set_signed("number", 3005); - assert_eq!(store.get_signed("number").unwrap(), 3005); - assert_eq!(store.take_signed("number").unwrap(), 3005); - assert_eq!(store.take_signed("number"), None); - store.set_signed("number", -123); - assert_eq!(store.get_signed("number").unwrap(), -123); - // Test usize - store.set_unsigned("unumber", 1024); - assert_eq!(store.get_unsigned("unumber").unwrap(), 1024); - assert_eq!(store.take_unsigned("unumber").unwrap(), 1024); - assert_eq!(store.take_unsigned("unumber"), None); - // Test float - store.set_float("float", 3.33); - assert_eq!(store.get_float("float").unwrap(), 3.33); - assert_eq!(store.take_float("float").unwrap(), 3.33); - assert_eq!(store.take_float("float"), None); // Test boolean store.set_boolean("bool", true); assert_eq!(store.get_boolean("bool").unwrap(), true); - assert_eq!(store.take_boolean("bool").unwrap(), true); - assert_eq!(store.take_boolean("bool"), None); // Test flag store.set("myflag"); assert_eq!(store.isset("myflag"), true); // Test unexisting assert!(store.get_boolean("unexisting-key").is_none()); - assert!(store.get_float("unexisting-key").is_none()); - assert!(store.get_signed("unexisting-key").is_none()); - assert!(store.get_signed("unexisting-key").is_none()); assert!(store.get_string("unexisting-key").is_none()); - assert!(store.get_unsigned("unexisting-key").is_none()); } } diff --git a/src/utils.rs b/src/utils.rs index 7fb9c9e..ee5955d 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -15,5 +15,4 @@ pub mod tty; pub mod ui; #[cfg(test)] -#[allow(dead_code)] pub mod test_helpers; diff --git a/src/utils/crypto.rs b/src/utils/crypto.rs index 62f2091..3662777 100644 --- a/src/utils/crypto.rs +++ b/src/utils/crypto.rs @@ -17,19 +17,19 @@ const GCM_NONCE_LEN: usize = 12; /// Encrypt a string with AES-128-GCM. The output is `nonce || ciphertext`, /// encoded as standard Base64. -pub fn aes128_b64_crypt(key: &str, input: &str) -> String { +pub fn aes128_b64_crypt(key: &str, input: &str) -> Result { let derived = derive_gcm_key(key); let cipher = Aes128Gcm::new(&derived.into()); let nonce_bytes = Aes128Gcm::generate_nonce(&mut aes_gcm::aead::OsRng); let ciphertext = cipher .encrypt(&nonce_bytes, input.as_bytes()) - .expect("AES-GCM encryption must not fail for valid inputs"); + .map_err(|_| CryptoError::AesGcm)?; let mut combined = Vec::with_capacity(GCM_NONCE_LEN + ciphertext.len()); combined.extend_from_slice(&nonce_bytes); combined.extend_from_slice(&ciphertext); - B64.encode(combined) + Ok(B64.encode(combined)) } /// Decrypt a Base64-encoded string. Tries AES-128-GCM first; on failure falls @@ -104,7 +104,7 @@ mod tests { fn test_encrypt_decrypt_roundtrip() { let key = "MYSUPERSECRETKEY"; let input = "Hello world!"; - let secret = aes128_b64_crypt(key, input); + let secret = aes128_b64_crypt(key, input).unwrap(); let decrypted = aes128_b64_decrypt(key, &secret).unwrap(); assert_eq!(decrypted, input); } @@ -123,8 +123,8 @@ mod tests { fn test_different_encryptions_produce_different_ciphertexts() { let key = "MYSUPERSECRETKEY"; let input = "Hello world!"; - let secret1 = aes128_b64_crypt(key, input); - let secret2 = aes128_b64_crypt(key, input); + let secret1 = aes128_b64_crypt(key, input).unwrap(); + let secret2 = aes128_b64_crypt(key, input).unwrap(); // Due to random nonces, ciphertexts must differ assert_ne!(secret1, secret2); // But both decrypt to the same plaintext @@ -134,7 +134,7 @@ mod tests { #[test] fn test_wrong_key_fails() { - let secret = aes128_b64_crypt("correct-key", "sensitive data"); + let secret = aes128_b64_crypt("correct-key", "sensitive data").unwrap(); assert!(aes128_b64_decrypt("wrong-key", &secret).is_err()); } diff --git a/src/utils/fmt.rs b/src/utils/fmt.rs index e8ccc61..3e7f77a 100644 --- a/src/utils/fmt.rs +++ b/src/utils/fmt.rs @@ -61,8 +61,10 @@ pub fn fmt_path_elide_ex(p: &Path, width: usize, extra_len: usize) -> String { let mut ancestors = p.ancestors(); let mut elided_path: PathBuf = PathBuf::new(); // If ancestors_len's size is bigger than 2, push count - 2 - if ancestors_len > 2 { - elided_path.push(ancestors.nth(ancestors_len - 2).unwrap()); + if ancestors_len > 2 + && let Some(ancestor) = ancestors.nth(ancestors_len - 2) + { + elided_path.push(ancestor); } // If ancestors_len is bigger than 3, push '…' and parent too if ancestors_len > 3 { diff --git a/src/utils/parser.rs b/src/utils/parser.rs index 40bfe92..bb712ed 100644 --- a/src/utils/parser.rs +++ b/src/utils/parser.rs @@ -111,6 +111,17 @@ static SEMVER_REGEX: Lazy = lazy_regex!(r"v?((0|[1-9]\d*)\.(0|[1-9]\d*)\. */ static BYTESIZE_REGEX: Lazy = lazy_regex!(r"(:?([0-9])+)( )*(:?[KMGTP])?B$"); +fn capture_group_to_string( + groups: ®ex::Captures<'_>, + index: usize, + field_name: &str, +) -> Result { + groups + .get(index) + .map(|group| group.as_str().to_string()) + .ok_or_else(|| format!("Missing {field_name}")) +} + // -- remote opts /// Parse remote option string. Returns in case of success a RemoteOptions struct @@ -265,9 +276,9 @@ fn parse_generic_remote_opt( fn parse_webdav_remote_opt(s: &str, prefix: &str) -> Result { match REMOTE_WEBDAV_OPT_REGEX.captures(s) { Some(groups) => { - let username = groups.get(1).map(|x| x.as_str().to_string()).unwrap(); - let password = groups.get(2).map(|x| x.as_str().to_string()).unwrap(); - let uri = groups.get(3).map(|x| x.as_str().to_string()).unwrap(); + let username = capture_group_to_string(&groups, 1, "username")?; + let password = capture_group_to_string(&groups, 2, "password")?; + let uri = capture_group_to_string(&groups, 3, "server URI")?; let remote_path: Option = groups.get(4).map(|group| PathBuf::from(group.as_str())); @@ -474,7 +485,7 @@ pub fn parse_bytesize>(bytes: S) -> Option { .map(|x| x.as_str().parse::().unwrap_or(0))?; let unit = groups.get(4).map(|x| x.as_str().to_string()); let unit = format!("{}B", unit.unwrap_or_default()); - let unit = ByteUnit::from_str(unit.as_str()).unwrap(); + let unit = ByteUnit::from_str(unit.as_str()).ok()?; Some(match unit { ByteUnit::Byte => ByteSize::b(amount), ByteUnit::Gigabyte => ByteSize::gib(amount), @@ -681,6 +692,13 @@ mod tests { assert_eq!(params.password.as_str(), "password"); } + #[test] + fn should_reject_malformed_webdav_options() { + let result = parse_remote_opt("https://omar@myserver:4445/myshare"); + + assert!(result.is_err()); + } + #[test] fn parse_aws_s3_opt() { // Simple @@ -873,6 +891,7 @@ mod tests { assert_eq!(parse_bytesize("2 GB").unwrap().as_u64(), 2147483648); assert_eq!(parse_bytesize("1 TB").unwrap().as_u64(), 1099511627776); assert!(parse_bytesize("1 XB").is_none()); + assert!(parse_bytesize("10 YB").is_none()); assert!(parse_bytesize("1 GB aaaaa").is_none()); assert!(parse_bytesize("1 GBaaaaa").is_none()); assert!(parse_bytesize("1MBaaaaa").is_none());