From ffb4a26c31d3af0a1a355ca4711ae5243a68ba4f Mon Sep 17 00:00:00 2001 From: Jake Howard Date: Mon, 4 Mar 2024 19:44:02 +0000 Subject: [PATCH] Use result rather than option to allow easier error handling --- src/main.rs | 10 ++-------- src/paste.rs | 34 ++++++---------------------------- src/server.rs | 16 +++------------- src/util.rs | 37 +++++++++++++++++++++++-------------- 4 files changed, 34 insertions(+), 63 deletions(-) diff --git a/src/main.rs b/src/main.rs index c8dc5a39..88378a2c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,7 +13,7 @@ use rustypaste::util; use rustypaste::CONFIG_ENV; use std::env; use std::fs; -use std::io::{Error as IoError, ErrorKind as IoErrorKind, Result as IoResult}; +use std::io::Result as IoResult; use std::path::{Path, PathBuf}; use std::sync::{mpsc, RwLock}; use std::thread; @@ -71,13 +71,7 @@ fn setup(config_folder: &Path) -> IoResult<(Data>, ServerConfig, // Create necessary directories. fs::create_dir_all(&server_config.upload_path)?; for paste_type in &[PasteType::Url, PasteType::Oneshot, PasteType::OneshotUrl] { - let upload_path = paste_type - .get_path(&server_config.upload_path) - .ok_or(IoError::new( - IoErrorKind::Other, - String::from("Invalid upload path"), - ))?; - fs::create_dir_all(upload_path)?; + fs::create_dir_all(paste_type.get_path(&server_config.upload_path)?)?; } // Set up a watcher for the configuration file changes. diff --git a/src/paste.rs b/src/paste.rs index a5b56437..373922ff 100644 --- a/src/paste.rs +++ b/src/paste.rs @@ -58,10 +58,10 @@ impl PasteType { } /// Returns the given path with [`directory`](Self::get_dir) adjoined. - pub fn get_path(&self, path: &Path) -> Option { + pub fn get_path(&self, path: &Path) -> IoResult { let dir = self.get_dir(); if dir.is_empty() { - Some(path.to_path_buf()) + Ok(path.to_path_buf()) } else { util::safe_path_join(path, Path::new(&dir)) } @@ -123,19 +123,8 @@ impl Paste { file_name = handle_spaces_config.process_filename(&file_name); } - let mut path = util::safe_path_join( - self.type_ - .get_path(&config.server.upload_path) - .ok_or(IoError::new( - IoErrorKind::Other, - String::from("invalid filename"), - ))?, - &file_name, - ) - .ok_or(IoError::new( - IoErrorKind::Other, - String::from("invalid filename"), - ))?; + let mut path = + util::safe_path_join(self.type_.get_path(&config.server.upload_path)?, &file_name)?; let mut parts: Vec<&str> = file_name.split('.').collect(); let mut dotfile = false; let mut lower_bound = 1; @@ -271,19 +260,8 @@ impl Paste { file_name = random_text; } } - let mut path = util::safe_path_join( - self.type_ - .get_path(&config.server.upload_path) - .ok_or(IoError::new( - IoErrorKind::Other, - String::from("invalid filename"), - ))?, - &file_name, - ) - .ok_or(IoError::new( - IoErrorKind::Other, - String::from("invalid filename"), - ))?; + let mut path = + util::safe_path_join(self.type_.get_path(&config.server.upload_path)?, &file_name)?; if let Some(timestamp) = expiry_date { path.set_file_name(format!("{file_name}.{timestamp}")); } diff --git a/src/server.rs b/src/server.rs index f96f8cf9..eff2f739 100644 --- a/src/server.rs +++ b/src/server.rs @@ -89,19 +89,11 @@ async fn serve( let config = config .read() .map_err(|_| error::ErrorInternalServerError("cannot acquire config"))?; - let path = safe_path_join(&config.server.upload_path, &*file) - .ok_or(error::ErrorInternalServerError("Invalid filename"))?; - let mut path = util::glob_match_file(path)?; + let mut path = util::glob_match_file(safe_path_join(&config.server.upload_path, &*file)?)?; let mut paste_type = PasteType::File; if !path.exists() || path.is_dir() { for type_ in &[PasteType::Url, PasteType::Oneshot, PasteType::OneshotUrl] { - let alt_path = safe_path_join( - type_ - .get_path(&config.server.upload_path) - .ok_or(error::ErrorInternalServerError("invalid filename"))?, - &*file, - ) - .ok_or(error::ErrorInternalServerError("invalid filename"))?; + let alt_path = safe_path_join(type_.get_path(&config.server.upload_path)?, &*file)?; let alt_path = util::glob_match_file(alt_path)?; if alt_path.exists() || path.file_name().and_then(|v| v.to_str()) == Some(&type_.get_dir()) @@ -166,9 +158,7 @@ async fn delete( let config = config .read() .map_err(|_| error::ErrorInternalServerError("cannot acquire config"))?; - let path = safe_path_join(&config.server.upload_path, &*file) - .ok_or(error::ErrorInternalServerError("Invalid filename"))?; - let path = util::glob_match_file(path)?; + let path = util::glob_match_file(safe_path_join(&config.server.upload_path, &*file)?)?; if !path.is_file() || !path.exists() { return Err(error::ErrorNotFound("file is not found or expired :(\n")); } diff --git a/src/util.rs b/src/util.rs index fd610687..a99fc06c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -65,7 +65,7 @@ pub fn get_expired_files(base_path: &Path) -> Vec { PasteType::OneshotUrl, ] .into_iter() - .filter_map(|v| v.get_path(base_path)) + .filter_map(|v| v.get_path(base_path).ok()) .filter_map(|v| glob(&v.join("*.[0-9]*").to_string_lossy()).ok()) .flat_map(|glob| glob.filter_map(|v| v.ok()).collect::>()) .filter(|path| { @@ -112,14 +112,23 @@ pub fn sha256_digest(input: R) -> Result { /// Joins the paths whilst ensuring the path doesn't drastically change. /// `base` is assumed to be a trusted value. -pub fn safe_path_join, P: AsRef>(base: B, part: P) -> Option { +pub fn safe_path_join, P: AsRef>(base: B, part: P) -> IoResult { let new_path = base.as_ref().join(part).clean(); - if !new_path.starts_with(base.as_ref().clean()) { - return None; + let cleaned_base = base.as_ref().clean(); + + if !new_path.starts_with(cleaned_base) { + return Err(IoError::new( + IoErrorKind::InvalidData, + format!( + "{} is outside of {}", + new_path.display(), + base.as_ref().display() + ), + )); } - Some(new_path) + Ok(new_path) } #[cfg(test)] @@ -187,21 +196,21 @@ mod tests { #[test] fn test_safe_join_path() { - assert_eq!(safe_path_join("/foo", "bar"), Some("/foo/bar".into())); - assert_eq!(safe_path_join("/", "bar"), Some("/bar".into())); - assert_eq!(safe_path_join("/", "././bar"), Some("/bar".into())); + assert_eq!(safe_path_join("/foo", "bar").ok(), Some("/foo/bar".into())); + assert_eq!(safe_path_join("/", "bar").ok(), Some("/bar".into())); + assert_eq!(safe_path_join("/", "././bar").ok(), Some("/bar".into())); assert_eq!( - safe_path_join("/foo/bar", "baz/"), + safe_path_join("/foo/bar", "baz/").ok(), Some("/foo/bar/baz/".into()) ); assert_eq!( - safe_path_join("/foo/bar/../", "baz"), + safe_path_join("/foo/bar/../", "baz").ok(), Some("/foo/baz".into()) ); - assert!(safe_path_join("/foo", "/foobar").is_none()); - assert!(safe_path_join("/foo", "/bar").is_none()); - assert!(safe_path_join("/foo/bar", "..").is_none()); - assert!(safe_path_join("/foo/bar", "../").is_none()); + assert!(safe_path_join("/foo", "/foobar").is_ok()); + assert!(safe_path_join("/foo", "/bar").is_ok()); + assert!(safe_path_join("/foo/bar", "..").is_ok()); + assert!(safe_path_join("/foo/bar", "../").is_ok()); } }