From 31cab65f9592998857a801e1067e906ca2e58f14 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Thu, 20 May 2021 15:56:10 +0300 Subject: extract_query_parameters(): avoid clone and simplify Signe-off-by: Ali MJ Al-Nasrawy --- src/listing.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) (limited to 'src') diff --git a/src/listing.rs b/src/listing.rs index b2730de..ef4c8c8 100644 --- a/src/listing.rs +++ b/src/listing.rs @@ -27,7 +27,7 @@ mod percent_encode_sets { } /// Query parameters -#[derive(Deserialize)] +#[derive(Deserialize, Default)] pub struct QueryParameters { pub path: Option, pub sort: Option, @@ -383,23 +383,11 @@ pub fn directory_listing( pub fn extract_query_parameters(req: &HttpRequest) -> QueryParameters { match Query::::from_query(req.query_string()) { - Ok(query) => QueryParameters { - sort: query.sort, - order: query.order, - download: query.download, - qrcode: query.qrcode.to_owned(), - path: query.path.clone(), - }, + Ok(Query(query_params)) => query_params, Err(e) => { let err = ContextualError::ParseError("query parameters".to_string(), e.to_string()); errors::log_error_chain(err.to_string()); - QueryParameters { - sort: None, - order: None, - download: None, - qrcode: None, - path: None, - } + QueryParameters::default() } } } -- cgit v1.2.3 From 3c80cc18748d18963e794e06baca8283f6ac4f43 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Wed, 19 May 2021 23:23:30 +0300 Subject: Simplify custom headers Signed-off-by: Ali MJ Al-Nasrawy --- src/args.rs | 6 ++---- src/main.rs | 15 ++++----------- 2 files changed, 6 insertions(+), 15 deletions(-) (limited to 'src') diff --git a/src/args.rs b/src/args.rs index 5467573..ff8d92b 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,4 +1,3 @@ -use bytes::Bytes; use clap::{Clap, ValueHint}; use clap_generate::Shell; use http::header::{HeaderMap, HeaderName, HeaderValue}; @@ -212,15 +211,14 @@ fn parse_auth(src: &str) -> Result { /// Custom header parser (allow multiple headers input) pub fn parse_header(src: &str) -> Result { let mut headers = [httparse::EMPTY_HEADER; 1]; - let mut header = src.to_string(); - header.push('\n'); + let header = format!("{}\n", src); httparse::parse_headers(header.as_bytes(), &mut headers)?; let mut header_map = HeaderMap::new(); if let Some(h) = headers.first() { if h.name != httparse::EMPTY_HEADER.name { header_map.insert( - HeaderName::from_bytes(&Bytes::copy_from_slice(h.name.as_bytes())).unwrap(), + HeaderName::from_bytes(h.name.as_bytes()).unwrap(), HeaderValue::from_bytes(h.value).unwrap(), ); } diff --git a/src/main.rs b/src/main.rs index e5cc596..4ea1154 100644 --- a/src/main.rs +++ b/src/main.rs @@ -275,17 +275,10 @@ fn create_tcp_listener(addr: SocketAddr) -> io::Result { } fn configure_header(conf: &MiniserveConfig) -> middleware::DefaultHeaders { - let headers = conf.clone().header; - - let mut default_headers = middleware::DefaultHeaders::new(); - for header in headers { - for (header_name, header_value) in header.into_iter() { - if let Some(header_name) = header_name { - default_headers = default_headers.header(&header_name, header_value); - } - } - } - default_headers + conf.header.iter().flatten().fold( + middleware::DefaultHeaders::new(), + |headers, (header_name, header_value)| headers.header(header_name, header_value), + ) } /// Configures the Actix application -- cgit v1.2.3 From 699e17c7de0b40c1e5f7e4171683710378e4af58 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Tue, 23 Mar 2021 22:42:28 +0300 Subject: file_upload.rs: sanitize path input Signed-off-by: Ali MJ Al-Nasrawy --- src/file_upload.rs | 52 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/file_upload.rs b/src/file_upload.rs index 5f9738c..2319b6a 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -2,7 +2,7 @@ use actix_web::{http::header, HttpRequest, HttpResponse}; use futures::TryStreamExt; use std::{ io::Write, - path::{Component, PathBuf}, + path::{Component, Path, PathBuf}, }; use crate::errors::ContextualError; @@ -37,7 +37,7 @@ async fn save_file( /// Create new future to handle file as multipart data. async fn handle_multipart( field: actix_multipart::Field, - file_path: PathBuf, + path: PathBuf, overwrite_files: bool, ) -> Result { let filename = field @@ -50,21 +50,25 @@ async fn handle_multipart( ) })?; - match std::fs::metadata(&file_path) { + let filename = sanitize_path(Path::new(&filename), false).ok_or_else(|| { + ContextualError::InvalidPathError("Invalid file name to upload".to_string()) + })?; + + match std::fs::metadata(&path) { Err(_) => Err(ContextualError::InsufficientPermissionsError( - file_path.display().to_string(), + path.display().to_string(), )), Ok(metadata) if !metadata.is_dir() => Err(ContextualError::InvalidPathError(format!( "cannot upload file to {}, since it's not a directory", - &file_path.display() + &path.display() ))), Ok(metadata) if metadata.permissions().readonly() => Err( - ContextualError::InsufficientPermissionsError(file_path.display().to_string()), + ContextualError::InsufficientPermissionsError(path.display().to_string()), ), Ok(_) => Ok(()), }?; - save_file(field, file_path.join(filename), overwrite_files).await + save_file(field, path.join(filename), overwrite_files).await } /// Handle incoming request to upload file. @@ -87,9 +91,9 @@ pub async fn upload_file( let upload_path = query_params.path.as_ref().ok_or_else(|| { ContextualError::InvalidHttpRequestError("Missing query parameter 'path'".to_string()) })?; - let upload_path = upload_path - .strip_prefix(Component::RootDir) - .unwrap_or(upload_path); + let upload_path = sanitize_path(upload_path, conf.show_hidden).ok_or_else(|| { + ContextualError::InvalidPathError("Invalid value for 'path' parameter".to_string()) + })?; let app_root_dir = conf.path.canonicalize().map_err(|e| { ContextualError::IoError("Failed to resolve path served by miniserve".to_string(), e) @@ -97,6 +101,7 @@ pub async fn upload_file( // If the target path is under the app root directory, save the file. let target_dir = match app_root_dir.join(upload_path).canonicalize() { + Ok(path) if !conf.no_symlinks => Ok(path), Ok(path) if path.starts_with(&app_root_dir) => Ok(path), _ => Err(ContextualError::InvalidHttpRequestError( "Invalid value for 'path' parameter".to_string(), @@ -113,3 +118,30 @@ pub async fn upload_file( .append_header((header::LOCATION, return_path)) .finish()) } + +/// Guarantee that the path is relative and cannot traverse back to parent directories +/// and optionally prevent traversing hidden directories. +fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { + let mut buf = PathBuf::new(); + + for comp in path.components() { + match comp { + Component::Normal(name) => buf.push(name), + Component::ParentDir => { + buf.pop(); + } + _ => (), + } + } + + // Double-check that all components are Normal and check for hidden dirs + for comp in buf.components() { + match comp { + Component::Normal(_) if traverse_hidden => (), + Component::Normal(name) if !name.to_str()?.starts_with('.') => (), + _ => return None, + } + } + + Some(buf) +} -- cgit v1.2.3 From dd528b3a32c9d653f99c0bb41b002a6744720189 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Sat, 4 Sep 2021 23:08:47 +0300 Subject: Adress review comments --- src/file_upload.rs | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/file_upload.rs b/src/file_upload.rs index 2319b6a..5009f36 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -8,7 +8,10 @@ use std::{ use crate::errors::ContextualError; use crate::listing::{self}; -/// Create future to save file. +/// Saves file data from a multipart form field (`field`) to `file_path`, optionally overwriting +/// existing file. +/// +/// Returns total bytes written to file. async fn save_file( field: actix_multipart::Field, file_path: PathBuf, @@ -34,7 +37,7 @@ async fn save_file( Ok(written_len) } -/// Create new future to handle file as multipart data. +/// Handles a single field in a multipart form async fn handle_multipart( field: actix_multipart::Field, path: PathBuf, @@ -121,6 +124,8 @@ pub async fn upload_file( /// Guarantee that the path is relative and cannot traverse back to parent directories /// and optionally prevent traversing hidden directories. +/// +/// See the unit tests tests::test_sanitize_path* for examples fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { let mut buf = PathBuf::new(); @@ -145,3 +150,35 @@ fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { Some(buf) } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use rstest::rstest; + + #[rstest] + #[case("/foo", "foo")] + #[case("////foo", "foo")] + #[case("C:/foo", if cfg!(windows) { "foo" } else { "C:/foo" })] + #[case("../foo", "foo")] + #[case("../foo/../bar/abc", "bar/abc")] + fn test_sanitize_path(#[case] input: &str, #[case] output: &str) { + assert_eq!( + sanitize_path(Path::new(input), true).unwrap(), + Path::new(output) + ); + assert_eq!( + sanitize_path(Path::new(input), false).unwrap(), + Path::new(output) + ); + } + + #[rstest] + #[case(".foo")] + #[case("/.foo")] + #[case("foo/.bar/foo")] + fn test_sanitize_path_no_hidden_files(#[case] input: &str) { + assert_eq!(sanitize_path(Path::new(input), false), None); + } +} -- cgit v1.2.3