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());