From 2662c59fcffe1b62e019b08d1e22c1cd5c741066 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Tue, 2 Aug 2022 15:02:09 +0200 Subject: Added option restrict-upload-dir --- src/args.rs | 4 ++++ src/config.rs | 4 ++++ src/file_upload.rs | 13 +++++++++++++ src/renderer.rs | 9 ++++++++- 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/args.rs b/src/args.rs index a8718bd..4edf455 100644 --- a/src/args.rs +++ b/src/args.rs @@ -111,6 +111,10 @@ pub struct CliArgs { #[clap(short = 'u', long = "upload-files")] pub file_upload: bool, + /// Restrict upload directories + #[clap(long = "restrict-upload-dir")] + pub restrict_upload_dir: Vec, + /// Enable creating directories #[clap(short = 'U', long = "mkdir", requires = "file-upload")] pub mkdir_enabled: bool, diff --git a/src/config.rs b/src/config.rs index 5bcbd62..380cf5a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -87,6 +87,9 @@ pub struct MiniserveConfig { /// Enable file upload pub file_upload: bool, + /// Restrict file upload dirs + pub restrict_upload_dir: Vec, + /// HTML accept attribute value pub uploadable_media_type: Option, @@ -248,6 +251,7 @@ impl MiniserveConfig { show_qrcode: args.qrcode, mkdir_enabled: args.mkdir_enabled, file_upload: args.file_upload, + restrict_upload_dir: args.restrict_upload_dir, uploadable_media_type, tar_enabled: args.enable_tar, tar_gz_enabled: args.enable_tar_gz, diff --git a/src/file_upload.rs b/src/file_upload.rs index 6643c68..747d0de 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -171,6 +171,19 @@ pub async fn upload_file( ContextualError::IoError("Failed to resolve path served by miniserve".to_string(), e) })?; + + // Disallow paths outside of restricted directories + // TODO: Probably not the most rust-ic style... + if !conf.restrict_upload_dir.is_empty() { + let upl_path = upload_path.clone().into_os_string().into_string().unwrap(); + + if !(conf.restrict_upload_dir.contains(&upl_path)){ + // not good + return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); + } + } + + // Disallow the target path to go outside of the served directory // The target directory shouldn't be canonicalized when it gets passed to // handle_multipart so that it can check for symlinks if needed diff --git a/src/renderer.rs b/src/renderer.rs index c8958fe..8bee00f 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -38,8 +38,15 @@ pub fn page( let upload_action = build_upload_action(&upload_route, encoded_dir, sort_method, sort_order); let mkdir_action = build_mkdir_action(&upload_route, encoded_dir); + let upload_allowed = conf.restrict_upload_dir.contains(&encoded_dir[1..].to_string()); let title_path = breadcrumbs_to_path_string(breadcrumbs); + let title_path = breadcrumbs + .iter() + .map(|el| el.name.clone()) + .collect::>() + .join("/"); + html! { (DOCTYPE) html { @@ -120,7 +127,7 @@ pub fn page( } } div.toolbar_box_group { - @if conf.file_upload { + @if conf.file_upload && upload_allowed { div.toolbar_box { form id="file_submit" action=(upload_action) method="POST" enctype="multipart/form-data" { p { "Select a file to upload or drag it anywhere into the window" } -- cgit v1.2.3 From 193bd544ff92734eba7ce26c218d6a7d014e02e8 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Tue, 2 Aug 2022 17:28:49 +0200 Subject: fixed rendering of upload if non-restricted --- src/renderer.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/renderer.rs b/src/renderer.rs index 8bee00f..cae09df 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -38,8 +38,9 @@ pub fn page( let upload_action = build_upload_action(&upload_route, encoded_dir, sort_method, sort_order); let mkdir_action = build_mkdir_action(&upload_route, encoded_dir); - let upload_allowed = conf.restrict_upload_dir.contains(&encoded_dir[1..].to_string()); let title_path = breadcrumbs_to_path_string(breadcrumbs); + let upload_allowed = conf.restrict_upload_dir.is_empty() || + conf.restrict_upload_dir.contains(&encoded_dir[1..].to_string()); let title_path = breadcrumbs .iter() -- cgit v1.2.3 From 93bfc372474199367519f0b10820cd0d5b332e66 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Tue, 2 Aug 2022 17:40:05 +0200 Subject: Test that uploads fail if outside restricted dir --- tests/upload_files.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 71fcbc4..a7a0a10 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -80,6 +80,44 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } +#[rstest] +fn uploading_files_is_restricted( + #[with(&["-u", "--restrict-upload-dir", "someDir"])] server: TestServer +) -> Result<(), Error> { + let test_file_name = "uploaded test file.txt"; + + // Before uploading, check whether the uploaded file does not yet exist. + let body = reqwest::blocking::get(server.url())?.error_for_status()?; + let parsed = Document::from_read(body)?; + assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); + + // Ensure the file upload form is not present + assert!(parsed.find(Attr("id", "file_submit")).next().is_none()); + + // Then try to upload anyway + let form = multipart::Form::new(); + let part = multipart::Part::text("this should not be uploaded") + .file_name(test_file_name) + .mime_str("text/plain")?; + let form = form.part("file_to_upload", part); + + let client = Client::new(); + // Ensure uploading fails and returns an error + assert!(client + .post(server.url().join("/upload?path=/")?) + .multipart(form) + .send()? + .error_for_status() + .is_err()); + + // After uploading, check whether the uploaded file is now getting listed. + let body = reqwest::blocking::get(server.url())?; + let parsed = Document::from_read(body)?; + assert!(!parsed.find(Text).any(|x| x.text() == test_file_name)); + + Ok(()) +} + /// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in /// file name (Content-Disposition) /// -- cgit v1.2.3 From fdf38e9bbbe41c00df09f31a1cc3176c21d90f02 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Tue, 2 Aug 2022 20:28:08 +0200 Subject: Removed redundant test code, fixed comments --- tests/upload_files.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index a7a0a10..a046426 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -72,7 +72,7 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { .error_for_status() .is_err()); - // After uploading, check whether the uploaded file is now getting listed. + // After uploading, check whether the uploaded file is NOT getting listed. let body = reqwest::blocking::get(server.url())?; let parsed = Document::from_read(body)?; assert!(!parsed.find(Text).any(|x| x.text() == test_file_name)); @@ -80,21 +80,15 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } +// This test runs the server with --restrict-upload-dir argument and +// checks that file upload to a different directory is actually prevented. #[rstest] fn uploading_files_is_restricted( #[with(&["-u", "--restrict-upload-dir", "someDir"])] server: TestServer ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; - // Before uploading, check whether the uploaded file does not yet exist. - let body = reqwest::blocking::get(server.url())?.error_for_status()?; - let parsed = Document::from_read(body)?; - assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); - - // Ensure the file upload form is not present - assert!(parsed.find(Attr("id", "file_submit")).next().is_none()); - - // Then try to upload anyway + // Then try to upload file to root directory (which is not the --restrict-upload-dir) let form = multipart::Form::new(); let part = multipart::Part::text("this should not be uploaded") .file_name(test_file_name) @@ -110,7 +104,7 @@ fn uploading_files_is_restricted( .error_for_status() .is_err()); - // After uploading, check whether the uploaded file is now getting listed. + // After uploading, check whether the uploaded file is NOT getting listed. let body = reqwest::blocking::get(server.url())?; let parsed = Document::from_read(body)?; assert!(!parsed.find(Text).any(|x| x.text() == test_file_name)); -- cgit v1.2.3 From 7cd15dfb54829979ff9ebb4c6ad2a463f1534491 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Tue, 2 Aug 2022 20:35:45 +0200 Subject: Added positive test for --restrict-upload-dir --- tests/upload_files.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index a046426..afe2972 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -112,6 +112,51 @@ fn uploading_files_is_restricted( Ok(()) } +// This tests that we can upload files to the directory specified by --restrict-upload-dir +#[rstest] +fn uploading_files_to_restricted_dir_works( + #[with(&["-u", "--restrict-upload-dir", "someDir"])] server: TestServer +) -> Result<(), Error> { + let test_file_name = "uploaded test file.txt"; + + // Create test directory + use std::fs::create_dir_all; + create_dir_all(server.path().join("someDir")).unwrap(); + + // Before uploading, check whether the uploaded file does not yet exist. + let body = reqwest::blocking::get(server.url().join("someDir")?)?.error_for_status()?; + let parsed = Document::from_read(body)?; + assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); + + // Perform the actual upload. + let upload_action = parsed + .find(Attr("id", "file_submit")) + .next() + .expect("Couldn't find element with id=file_submit") + .attr("action") + .expect("Upload form doesn't have action attribute"); + let form = multipart::Form::new(); + let part = multipart::Part::text("this should be uploaded") + .file_name(test_file_name) + .mime_str("text/plain")?; + let form = form.part("file_to_upload", part); + + let client = Client::new(); + client + .post(server.url().join(upload_action)?) + .multipart(form) + .send()? + .error_for_status()?; + + // After uploading, check whether the uploaded file is now getting listed. + let body = reqwest::blocking::get(server.url().join("someDir")?)?; + let parsed = Document::from_read(body)?; + assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); + + Ok(()) +} + + /// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in /// file name (Content-Disposition) /// -- cgit v1.2.3 From 66f34ca7035c404ea7ac08c2601d428985a54dd1 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 3 Aug 2022 07:50:07 +0200 Subject: Added dependency to -u for --restrict-upload-dir --- src/args.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/args.rs b/src/args.rs index 4edf455..c7c988b 100644 --- a/src/args.rs +++ b/src/args.rs @@ -112,7 +112,8 @@ pub struct CliArgs { pub file_upload: bool, /// Restrict upload directories - #[clap(long = "restrict-upload-dir")] + #[clap(long = "restrict-upload-dir", requires = "file-upload")] + pub restrict_upload_dir: Vec, /// Enable creating directories -- cgit v1.2.3 From 455abe23d0fd2114f7836694502892990180577d Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 3 Aug 2022 13:02:21 +0200 Subject: Switched to use of PathBuf, fixed for subdirs --- src/args.rs | 5 ++--- src/config.rs | 2 +- src/file_upload.rs | 11 ++++++++--- src/renderer.rs | 23 +++++++++++++++-------- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/args.rs b/src/args.rs index c7c988b..6c6d6e0 100644 --- a/src/args.rs +++ b/src/args.rs @@ -112,9 +112,8 @@ pub struct CliArgs { pub file_upload: bool, /// Restrict upload directories - #[clap(long = "restrict-upload-dir", requires = "file-upload")] - - pub restrict_upload_dir: Vec, + #[clap(long = "restrict-upload-dir", requires = "file-upload", value_hint = ValueHint::FilePath)] + pub restrict_upload_dir: Vec, /// Enable creating directories #[clap(short = 'U', long = "mkdir", requires = "file-upload")] diff --git a/src/config.rs b/src/config.rs index 380cf5a..3b5c1d7 100644 --- a/src/config.rs +++ b/src/config.rs @@ -88,7 +88,7 @@ pub struct MiniserveConfig { pub file_upload: bool, /// Restrict file upload dirs - pub restrict_upload_dir: Vec, + pub restrict_upload_dir: Vec, /// HTML accept attribute value pub uploadable_media_type: Option, diff --git a/src/file_upload.rs b/src/file_upload.rs index 747d0de..56112f3 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -175,10 +175,15 @@ pub async fn upload_file( // Disallow paths outside of restricted directories // TODO: Probably not the most rust-ic style... if !conf.restrict_upload_dir.is_empty() { - let upl_path = upload_path.clone().into_os_string().into_string().unwrap(); + let mut upload_allowed = false; + for restricted_dir in conf.restrict_upload_dir.iter() { + if upload_path.starts_with(restricted_dir) { + upload_allowed = true; + break; + } + } - if !(conf.restrict_upload_dir.contains(&upl_path)){ - // not good + if !upload_allowed { return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); } } diff --git a/src/renderer.rs b/src/renderer.rs index cae09df..2b3d1fa 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -39,15 +39,22 @@ pub fn page( let mkdir_action = build_mkdir_action(&upload_route, encoded_dir); let title_path = breadcrumbs_to_path_string(breadcrumbs); - let upload_allowed = conf.restrict_upload_dir.is_empty() || - conf.restrict_upload_dir.contains(&encoded_dir[1..].to_string()); - - let title_path = breadcrumbs - .iter() - .map(|el| el.name.clone()) - .collect::>() - .join("/"); + // TODO: Probably not very idiomatic + let mut upload_allowed = false; + + if conf.restrict_upload_dir.is_empty() { + upload_allowed = true; + } else { + for restricted_dir in conf.restrict_upload_dir.iter() { + let full_restricted_path = &format!("/{}", restricted_dir.display()); + if encoded_dir.starts_with(full_restricted_path) { + upload_allowed = true; + break; + } + } + } + html! { (DOCTYPE) html { -- cgit v1.2.3 From e5804835e491b77625541b025669efa5524d8102 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 3 Aug 2022 13:32:57 +0200 Subject: cleaned up code using any() --- src/file_upload.rs | 16 +++++----------- src/renderer.rs | 16 ++-------------- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/file_upload.rs b/src/file_upload.rs index 56112f3..e5f6173 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -174,21 +174,15 @@ pub async fn upload_file( // Disallow paths outside of restricted directories // TODO: Probably not the most rust-ic style... - if !conf.restrict_upload_dir.is_empty() { - let mut upload_allowed = false; - for restricted_dir in conf.restrict_upload_dir.iter() { - if upload_path.starts_with(restricted_dir) { - upload_allowed = true; - break; - } - } + let upload_allowed = conf.restrict_upload_dir.is_empty() || + conf.restrict_upload_dir.iter().any(|s| upload_path.starts_with(s)); - if !upload_allowed { - return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); - } + if !(upload_allowed) { + return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); } + // Disallow the target path to go outside of the served directory // The target directory shouldn't be canonicalized when it gets passed to // handle_multipart so that it can check for symlinks if needed diff --git a/src/renderer.rs b/src/renderer.rs index 2b3d1fa..0ee26af 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -40,20 +40,8 @@ pub fn page( let title_path = breadcrumbs_to_path_string(breadcrumbs); - // TODO: Probably not very idiomatic - let mut upload_allowed = false; - - if conf.restrict_upload_dir.is_empty() { - upload_allowed = true; - } else { - for restricted_dir in conf.restrict_upload_dir.iter() { - let full_restricted_path = &format!("/{}", restricted_dir.display()); - if encoded_dir.starts_with(full_restricted_path) { - upload_allowed = true; - break; - } - } - } + let upload_allowed = conf.restrict_upload_dir.is_empty() || conf.restrict_upload_dir.iter().any( + |x| encoded_dir.starts_with(&format!("/{}", x.display())) ); html! { (DOCTYPE) -- cgit v1.2.3 From b51cbc9e6dcc2b563f8403c520609d1a4b074d6d Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 3 Aug 2022 14:42:40 +0200 Subject: added test cases for sub directory --- tests/upload_files.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index afe2972..7bd85df 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -83,8 +83,10 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { // This test runs the server with --restrict-upload-dir argument and // checks that file upload to a different directory is actually prevented. #[rstest] +#[case(server(&["-u", "--restrict-upload-dir", "someDir"]))] +#[case(server(&["-u", "--restrict-upload-dir", "someDir/some_sub_dir"]))] fn uploading_files_is_restricted( - #[with(&["-u", "--restrict-upload-dir", "someDir"])] server: TestServer + #[case] server: TestServer ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; @@ -114,17 +116,20 @@ fn uploading_files_is_restricted( // This tests that we can upload files to the directory specified by --restrict-upload-dir #[rstest] +#[case(server(&["-u", "--restrict-upload-dir", "someDir"]), "someDir")] +#[case(server(&["-u", "--restrict-upload-dir", "someDir/some_sub_dir"]), "someDir/some_sub_dir")] fn uploading_files_to_restricted_dir_works( - #[with(&["-u", "--restrict-upload-dir", "someDir"])] server: TestServer + #[case] server: TestServer, + #[case] upload_dir: &str, ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; // Create test directory use std::fs::create_dir_all; - create_dir_all(server.path().join("someDir")).unwrap(); + create_dir_all(server.path().join(upload_dir)).unwrap(); // Before uploading, check whether the uploaded file does not yet exist. - let body = reqwest::blocking::get(server.url().join("someDir")?)?.error_for_status()?; + let body = reqwest::blocking::get(server.url().join(upload_dir)?)?.error_for_status()?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); @@ -149,7 +154,7 @@ fn uploading_files_to_restricted_dir_works( .error_for_status()?; // After uploading, check whether the uploaded file is now getting listed. - let body = reqwest::blocking::get(server.url().join("someDir")?)?; + let body = reqwest::blocking::get(server.url().join(upload_dir)?)?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); -- cgit v1.2.3 From 36b041a07183c69eb40c9ae74b0a9ab4c6ccbd53 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 3 Aug 2022 14:51:20 +0200 Subject: Avoid error message during testing of restricted --- tests/upload_files.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 7bd85df..35569cb 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -83,8 +83,8 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { // This test runs the server with --restrict-upload-dir argument and // checks that file upload to a different directory is actually prevented. #[rstest] -#[case(server(&["-u", "--restrict-upload-dir", "someDir"]))] -#[case(server(&["-u", "--restrict-upload-dir", "someDir/some_sub_dir"]))] +#[case(server_no_stderr(&["-u", "--restrict-upload-dir", "someDir"]))] +#[case(server_no_stderr(&["-u", "--restrict-upload-dir", "someDir/some_sub_dir"]))] fn uploading_files_is_restricted( #[case] server: TestServer ) -> Result<(), Error> { -- cgit v1.2.3 From 550ae0151c1dadc6c1f00df300d88528c29fbf49 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Thu, 4 Aug 2022 11:20:37 +0200 Subject: Renamed option for more clarity --- src/args.rs | 8 +++++--- src/config.rs | 4 ++-- src/file_upload.rs | 5 ++--- src/renderer.rs | 2 +- tests/upload_files.rs | 8 ++++---- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/args.rs b/src/args.rs index 6c6d6e0..56c834c 100644 --- a/src/args.rs +++ b/src/args.rs @@ -111,9 +111,11 @@ pub struct CliArgs { #[clap(short = 'u', long = "upload-files")] pub file_upload: bool, - /// Restrict upload directories - #[clap(long = "restrict-upload-dir", requires = "file-upload", value_hint = ValueHint::FilePath)] - pub restrict_upload_dir: Vec, + /// Allowed upload directories (together with -u) + /// + /// If this is set, uploads are only allowed into the provided directories. + #[clap(long, requires = "file-upload", value_hint = ValueHint::FilePath)] + pub allowed_upload_dir: Vec, /// Enable creating directories #[clap(short = 'U', long = "mkdir", requires = "file-upload")] diff --git a/src/config.rs b/src/config.rs index 3b5c1d7..bf67595 100644 --- a/src/config.rs +++ b/src/config.rs @@ -88,7 +88,7 @@ pub struct MiniserveConfig { pub file_upload: bool, /// Restrict file upload dirs - pub restrict_upload_dir: Vec, + pub allowed_upload_dir: Vec, /// HTML accept attribute value pub uploadable_media_type: Option, @@ -251,7 +251,7 @@ impl MiniserveConfig { show_qrcode: args.qrcode, mkdir_enabled: args.mkdir_enabled, file_upload: args.file_upload, - restrict_upload_dir: args.restrict_upload_dir, + allowed_upload_dir: args.allowed_upload_dir, uploadable_media_type, tar_enabled: args.enable_tar, tar_gz_enabled: args.enable_tar_gz, diff --git a/src/file_upload.rs b/src/file_upload.rs index e5f6173..231999f 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -173,9 +173,8 @@ pub async fn upload_file( // Disallow paths outside of restricted directories - // TODO: Probably not the most rust-ic style... - let upload_allowed = conf.restrict_upload_dir.is_empty() || - conf.restrict_upload_dir.iter().any(|s| upload_path.starts_with(s)); + let upload_allowed = conf.allowed_upload_dir.is_empty() || + conf.allowed_upload_dir.iter().any(|s| upload_path.starts_with(s)); if !(upload_allowed) { return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); diff --git a/src/renderer.rs b/src/renderer.rs index 0ee26af..107f0a4 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -40,7 +40,7 @@ pub fn page( let title_path = breadcrumbs_to_path_string(breadcrumbs); - let upload_allowed = conf.restrict_upload_dir.is_empty() || conf.restrict_upload_dir.iter().any( + let upload_allowed = conf.allowed_upload_dir.is_empty() || conf.allowed_upload_dir.iter().any( |x| encoded_dir.starts_with(&format!("/{}", x.display())) ); html! { diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 35569cb..508d7c6 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -83,8 +83,8 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { // This test runs the server with --restrict-upload-dir argument and // checks that file upload to a different directory is actually prevented. #[rstest] -#[case(server_no_stderr(&["-u", "--restrict-upload-dir", "someDir"]))] -#[case(server_no_stderr(&["-u", "--restrict-upload-dir", "someDir/some_sub_dir"]))] +#[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir"]))] +#[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]))] fn uploading_files_is_restricted( #[case] server: TestServer ) -> Result<(), Error> { @@ -116,8 +116,8 @@ fn uploading_files_is_restricted( // This tests that we can upload files to the directory specified by --restrict-upload-dir #[rstest] -#[case(server(&["-u", "--restrict-upload-dir", "someDir"]), "someDir")] -#[case(server(&["-u", "--restrict-upload-dir", "someDir/some_sub_dir"]), "someDir/some_sub_dir")] +#[case(server(&["-u", "--allowed-upload-dir", "someDir"]), "someDir")] +#[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]), "someDir/some_sub_dir")] fn uploading_files_to_restricted_dir_works( #[case] server: TestServer, #[case] upload_dir: &str, -- cgit v1.2.3 From 54738df913e7b41967b6e56b871ba0a6bf32a190 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Thu, 4 Aug 2022 11:11:47 +0200 Subject: Update src/file_upload.rs Co-authored-by: Sven-Hendrik Haase --- src/file_upload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/file_upload.rs b/src/file_upload.rs index 231999f..68371e8 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -176,7 +176,7 @@ pub async fn upload_file( let upload_allowed = conf.allowed_upload_dir.is_empty() || conf.allowed_upload_dir.iter().any(|s| upload_path.starts_with(s)); - if !(upload_allowed) { + if !upload_allowed { return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); } -- cgit v1.2.3 From 40b7d26c53692ccdaf139e612dd8b6d346c32b8e Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Thu, 4 Aug 2022 11:26:45 +0200 Subject: renaming of option for clarity in test --- tests/upload_files.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 508d7c6..a819abe 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -80,8 +80,8 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } -// This test runs the server with --restrict-upload-dir argument and -// checks that file upload to a different directory is actually prevented. +/// This test runs the server with --allowed-upload-dir argument and +/// checks that file upload to a different directory is actually prevented. #[rstest] #[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir"]))] #[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]))] @@ -90,7 +90,7 @@ fn uploading_files_is_restricted( ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; - // Then try to upload file to root directory (which is not the --restrict-upload-dir) + // Then try to upload file to root directory (which is not the --allowed-upload-dir) let form = multipart::Form::new(); let part = multipart::Part::text("this should not be uploaded") .file_name(test_file_name) @@ -114,11 +114,11 @@ fn uploading_files_is_restricted( Ok(()) } -// This tests that we can upload files to the directory specified by --restrict-upload-dir +/// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] #[case(server(&["-u", "--allowed-upload-dir", "someDir"]), "someDir")] #[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]), "someDir/some_sub_dir")] -fn uploading_files_to_restricted_dir_works( +fn uploading_files_to_allowed_dir_works( #[case] server: TestServer, #[case] upload_dir: &str, ) -> Result<(), Error> { -- cgit v1.2.3 From ed925eaa2985b4cc9051371119591ed329b1a57b Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Sun, 14 Aug 2022 20:58:40 +0200 Subject: clarity of comment Co-authored-by: Sven-Hendrik Haase --- src/file_upload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/file_upload.rs b/src/file_upload.rs index 68371e8..c6e7ac6 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -172,7 +172,7 @@ pub async fn upload_file( })?; - // Disallow paths outside of restricted directories + // Disallow paths outside of allowed directories let upload_allowed = conf.allowed_upload_dir.is_empty() || conf.allowed_upload_dir.iter().any(|s| upload_path.starts_with(s)); -- cgit v1.2.3 From 751bf58dd7c08e8b4212680503016362fbcd1dfc Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Sun, 14 Aug 2022 21:11:22 +0200 Subject: Clarity of comment Co-authored-by: Sven-Hendrik Haase --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index bf67595..2b10440 100644 --- a/src/config.rs +++ b/src/config.rs @@ -87,7 +87,7 @@ pub struct MiniserveConfig { /// Enable file upload pub file_upload: bool, - /// Restrict file upload dirs + /// List of allowed upload directories pub allowed_upload_dir: Vec, /// HTML accept attribute value -- cgit v1.2.3 From c1c70790de73225077f6f4b762088e2cdbf946c8 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 15 Aug 2022 20:59:37 +0200 Subject: Moved use to global --- tests/upload_files.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index a819abe..aac9978 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -1,5 +1,6 @@ mod fixtures; +use std::fs::create_dir_all; use assert_fs::fixture::TempDir; use fixtures::{server, server_no_stderr, tmpdir, Error, TestServer}; use reqwest::blocking::{multipart, Client}; @@ -125,7 +126,6 @@ fn uploading_files_to_allowed_dir_works( let test_file_name = "uploaded test file.txt"; // Create test directory - use std::fs::create_dir_all; create_dir_all(server.path().join(upload_dir)).unwrap(); // Before uploading, check whether the uploaded file does not yet exist. @@ -180,7 +180,6 @@ fn prevent_path_traversal_attacks( #[case] expected: &str, ) -> Result<(), Error> { // Create test directories - use std::fs::create_dir_all; create_dir_all(server.path().join("foo")).unwrap(); if !cfg!(windows) { for dir in &["C:/foo/C:", r"C:\foo", r"\foo"] { -- cgit v1.2.3 From 0049bb2037d8adaf0d7393069447b06eed0e0c95 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 15 Aug 2022 21:26:45 +0200 Subject: test case with two allowed dirs --- tests/upload_files.rs | 74 +++++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 35 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index aac9978..cf00d47 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -117,47 +117,51 @@ fn uploading_files_is_restricted( /// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] -#[case(server(&["-u", "--allowed-upload-dir", "someDir"]), "someDir")] -#[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]), "someDir/some_sub_dir")] +#[case(server(&["-u", "--allowed-upload-dir", "someDir"]), vec!["someDir"])] +#[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir"])] +#[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir", "--allowed-upload-dir", "someDir/some_other_dir"]), + vec!["someDir/some_sub_dir", "someDir/some_other_dir"])] fn uploading_files_to_allowed_dir_works( #[case] server: TestServer, - #[case] upload_dir: &str, + #[case] upload_dirs: Vec<&str>, ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; - // Create test directory - create_dir_all(server.path().join(upload_dir)).unwrap(); - - // Before uploading, check whether the uploaded file does not yet exist. - let body = reqwest::blocking::get(server.url().join(upload_dir)?)?.error_for_status()?; - let parsed = Document::from_read(body)?; - assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); - - // Perform the actual upload. - let upload_action = parsed - .find(Attr("id", "file_submit")) - .next() - .expect("Couldn't find element with id=file_submit") - .attr("action") - .expect("Upload form doesn't have action attribute"); - let form = multipart::Form::new(); - let part = multipart::Part::text("this should be uploaded") - .file_name(test_file_name) - .mime_str("text/plain")?; - let form = form.part("file_to_upload", part); - - let client = Client::new(); - client - .post(server.url().join(upload_action)?) - .multipart(form) - .send()? - .error_for_status()?; - - // After uploading, check whether the uploaded file is now getting listed. - let body = reqwest::blocking::get(server.url().join(upload_dir)?)?; - let parsed = Document::from_read(body)?; - assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); + for upload_dir in upload_dirs{ + // Create test directory + create_dir_all(server.path().join(upload_dir)).unwrap(); + + // Before uploading, check whether the uploaded file does not yet exist. + let body = reqwest::blocking::get(server.url().join(upload_dir)?)?.error_for_status()?; + let parsed = Document::from_read(body)?; + assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); + + // Perform the actual upload. + let upload_action = parsed + .find(Attr("id", "file_submit")) + .next() + .expect("Couldn't find element with id=file_submit") + .attr("action") + .expect("Upload form doesn't have action attribute"); + let form = multipart::Form::new(); + let part = multipart::Part::text("this should be uploaded") + .file_name(test_file_name) + .mime_str("text/plain")?; + let form = form.part("file_to_upload", part); + + let client = Client::new(); + client + .post(server.url().join(upload_action)?) + .multipart(form) + .send()? + .error_for_status()?; + + // After uploading, check whether the uploaded file is now getting listed. + let body = reqwest::blocking::get(server.url().join(upload_dir)?)?; + let parsed = Document::from_read(body)?; + assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); + } Ok(()) } -- cgit v1.2.3 From 384c2b808f1ad0db28ce2ab115399e60c23a2972 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 15 Aug 2022 21:39:06 +0200 Subject: check status code when restricted; fix formatting --- tests/upload_files.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index cf00d47..de96aff 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -1,12 +1,12 @@ mod fixtures; -use std::fs::create_dir_all; use assert_fs::fixture::TempDir; use fixtures::{server, server_no_stderr, tmpdir, Error, TestServer}; use reqwest::blocking::{multipart, Client}; use rstest::rstest; use select::document::Document; use select::predicate::{Attr, Text}; +use std::fs::create_dir_all; #[rstest] fn uploading_files_works(#[with(&["-u"])] server: TestServer) -> Result<(), Error> { @@ -81,14 +81,12 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } -/// This test runs the server with --allowed-upload-dir argument and +/// This test runs the server with --allowed-upload-dir argument and /// checks that file upload to a different directory is actually prevented. #[rstest] #[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir"]))] #[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]))] -fn uploading_files_is_restricted( - #[case] server: TestServer -) -> Result<(), Error> { +fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; // Then try to upload file to root directory (which is not the --allowed-upload-dir) @@ -100,12 +98,14 @@ fn uploading_files_is_restricted( let client = Client::new(); // Ensure uploading fails and returns an error - assert!(client - .post(server.url().join("/upload?path=/")?) - .multipart(form) - .send()? - .error_for_status() - .is_err()); + assert_eq!( + 500, + client + .post(server.url().join("/upload?path=/")?) + .multipart(form) + .send()? + .status() + ); // After uploading, check whether the uploaded file is NOT getting listed. let body = reqwest::blocking::get(server.url())?; @@ -127,7 +127,7 @@ fn uploading_files_to_allowed_dir_works( ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; - for upload_dir in upload_dirs{ + for upload_dir in upload_dirs { // Create test directory create_dir_all(server.path().join(upload_dir)).unwrap(); @@ -160,12 +160,10 @@ fn uploading_files_to_allowed_dir_works( let body = reqwest::blocking::get(server.url().join(upload_dir)?)?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); - } Ok(()) } - /// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in /// file name (Content-Disposition) /// -- cgit v1.2.3 From e2ae526727e0154a1bc618971011788ee24e8748 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 15 Aug 2022 22:15:57 +0200 Subject: Use argument -u instead of --allowed-upload-dir --- src/args.rs | 16 +++++----------- src/config.rs | 4 ++-- tests/upload_files.rs | 10 +++++----- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/args.rs b/src/args.rs index 56c834c..8cd8ffa 100644 --- a/src/args.rs +++ b/src/args.rs @@ -108,28 +108,22 @@ pub struct CliArgs { pub qrcode: bool, /// Enable file uploading - #[clap(short = 'u', long = "upload-files")] - pub file_upload: bool, - - /// Allowed upload directories (together with -u) - /// - /// If this is set, uploads are only allowed into the provided directories. - #[clap(long, requires = "file-upload", value_hint = ValueHint::FilePath)] - pub allowed_upload_dir: Vec, + #[clap(short = 'u', long = "upload-files", value_hint = ValueHint::FilePath, min_values = 0)] + pub allowed_upload_dir: Option>, /// Enable creating directories - #[clap(short = 'U', long = "mkdir", requires = "file-upload")] + #[clap(short = 'U', long = "mkdir", requires = "allowed-upload-dir")] pub mkdir_enabled: bool, /// Specify uploadable media types - #[clap(arg_enum, short = 'm', long = "media-type", requires = "file-upload")] + #[clap(arg_enum, short = 'm', long = "media-type", requires = "allowed-upload-dir")] pub media_type: Option>, /// Directly specify the uploadable media type expression #[clap( short = 'M', long = "raw-media-type", - requires = "file-upload", + requires = "allowed-upload-dir", conflicts_with = "media-type" )] pub media_type_raw: Option, diff --git a/src/config.rs b/src/config.rs index 2b10440..4f794d1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -250,8 +250,8 @@ impl MiniserveConfig { overwrite_files: args.overwrite_files, show_qrcode: args.qrcode, mkdir_enabled: args.mkdir_enabled, - file_upload: args.file_upload, - allowed_upload_dir: args.allowed_upload_dir, + file_upload: !args.allowed_upload_dir.is_none(), + allowed_upload_dir: args.allowed_upload_dir.unwrap_or(vec![]), uploadable_media_type, tar_enabled: args.enable_tar, tar_gz_enabled: args.enable_tar_gz, diff --git a/tests/upload_files.rs b/tests/upload_files.rs index de96aff..ca9f007 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -84,8 +84,8 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { /// This test runs the server with --allowed-upload-dir argument and /// checks that file upload to a different directory is actually prevented. #[rstest] -#[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir"]))] -#[case(server_no_stderr(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]))] +#[case(server_no_stderr(&["-u", "someDir"]))] +#[case(server_no_stderr(&["-u", "someDir/some_sub_dir"]))] fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; @@ -117,9 +117,9 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error /// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] -#[case(server(&["-u", "--allowed-upload-dir", "someDir"]), vec!["someDir"])] -#[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir"])] -#[case(server(&["-u", "--allowed-upload-dir", "someDir/some_sub_dir", "--allowed-upload-dir", "someDir/some_other_dir"]), +#[case(server(&["-u", "someDir"]), vec!["someDir"])] +#[case(server(&["-u", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir"])] +#[case(server(&["-u", "someDir/some_sub_dir", "-u", "someDir/some_other_dir"]), vec!["someDir/some_sub_dir", "someDir/some_other_dir"])] fn uploading_files_to_allowed_dir_works( #[case] server: TestServer, -- cgit v1.2.3 From 5404e4fcb513bd8bf355e730aa37546b16164cad Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 17 Aug 2022 10:28:11 +0200 Subject: sanitize allowed upload paths for cases like ./dir --- src/config.rs | 3 ++- src/file_upload.rs | 2 +- tests/upload_files.rs | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 4f794d1..1331e7d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -16,6 +16,7 @@ use rustls_pemfile as pemfile; use crate::{ args::{CliArgs, MediaType}, auth::RequiredAuth, + file_upload::sanitize_path }; /// Possible characters for random routes @@ -251,7 +252,7 @@ impl MiniserveConfig { show_qrcode: args.qrcode, mkdir_enabled: args.mkdir_enabled, file_upload: !args.allowed_upload_dir.is_none(), - allowed_upload_dir: args.allowed_upload_dir.unwrap_or(vec![]), + allowed_upload_dir: args.allowed_upload_dir.unwrap_or(vec![]).iter().map(|x| sanitize_path(x, false).unwrap()).collect(), uploadable_media_type, tar_enabled: args.enable_tar, tar_gz_enabled: args.enable_tar_gz, diff --git a/src/file_upload.rs b/src/file_upload.rs index c6e7ac6..4d4f225 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -218,7 +218,7 @@ pub async fn upload_file( /// 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 { +pub fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { let mut buf = PathBuf::new(); for comp in path.components() { diff --git a/tests/upload_files.rs b/tests/upload_files.rs index ca9f007..ecb7ddf 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -118,6 +118,7 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error /// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] #[case(server(&["-u", "someDir"]), vec!["someDir"])] +#[case(server(&["-u", "./someDir"]), vec!["./someDir"])] #[case(server(&["-u", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir"])] #[case(server(&["-u", "someDir/some_sub_dir", "-u", "someDir/some_other_dir"]), vec!["someDir/some_sub_dir", "someDir/some_other_dir"])] -- cgit v1.2.3 From d905b68ca93c42769c3ebddf472a2916dc75b012 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 17 Aug 2022 10:28:46 +0200 Subject: cargo fmt --- src/args.rs | 7 ++++++- src/config.rs | 9 +++++++-- src/file_upload.rs | 14 ++++++++------ src/renderer.rs | 9 ++++++--- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/args.rs b/src/args.rs index 8cd8ffa..6ed1e0d 100644 --- a/src/args.rs +++ b/src/args.rs @@ -116,7 +116,12 @@ pub struct CliArgs { pub mkdir_enabled: bool, /// Specify uploadable media types - #[clap(arg_enum, short = 'm', long = "media-type", requires = "allowed-upload-dir")] + #[clap( + arg_enum, + short = 'm', + long = "media-type", + requires = "allowed-upload-dir" + )] pub media_type: Option>, /// Directly specify the uploadable media type expression diff --git a/src/config.rs b/src/config.rs index 1331e7d..073a80a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -16,7 +16,7 @@ use rustls_pemfile as pemfile; use crate::{ args::{CliArgs, MediaType}, auth::RequiredAuth, - file_upload::sanitize_path + file_upload::sanitize_path, }; /// Possible characters for random routes @@ -252,7 +252,12 @@ impl MiniserveConfig { show_qrcode: args.qrcode, mkdir_enabled: args.mkdir_enabled, file_upload: !args.allowed_upload_dir.is_none(), - allowed_upload_dir: args.allowed_upload_dir.unwrap_or(vec![]).iter().map(|x| sanitize_path(x, false).unwrap()).collect(), + allowed_upload_dir: args + .allowed_upload_dir + .unwrap_or(vec![]) + .iter() + .map(|x| sanitize_path(x, false).unwrap()) + .collect(), uploadable_media_type, tar_enabled: args.enable_tar, tar_gz_enabled: args.enable_tar_gz, diff --git a/src/file_upload.rs b/src/file_upload.rs index 4d4f225..0232c7e 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -171,17 +171,19 @@ pub async fn upload_file( ContextualError::IoError("Failed to resolve path served by miniserve".to_string(), e) })?; - // Disallow paths outside of allowed directories - let upload_allowed = conf.allowed_upload_dir.is_empty() || - conf.allowed_upload_dir.iter().any(|s| upload_path.starts_with(s)); + let upload_allowed = conf.allowed_upload_dir.is_empty() + || conf + .allowed_upload_dir + .iter() + .any(|s| upload_path.starts_with(s)); if !upload_allowed { - return Err(ContextualError::InvalidPathError("Not allowed to upload to this path".to_string())); + return Err(ContextualError::InvalidPathError( + "Not allowed to upload to this path".to_string(), + )); } - - // Disallow the target path to go outside of the served directory // The target directory shouldn't be canonicalized when it gets passed to // handle_multipart so that it can check for symlinks if needed diff --git a/src/renderer.rs b/src/renderer.rs index 107f0a4..b98a595 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -40,9 +40,12 @@ pub fn page( let title_path = breadcrumbs_to_path_string(breadcrumbs); - let upload_allowed = conf.allowed_upload_dir.is_empty() || conf.allowed_upload_dir.iter().any( - |x| encoded_dir.starts_with(&format!("/{}", x.display())) ); - + let upload_allowed = conf.allowed_upload_dir.is_empty() + || conf + .allowed_upload_dir + .iter() + .any(|x| encoded_dir.starts_with(&format!("/{}", x.display()))); + html! { (DOCTYPE) html { -- cgit v1.2.3 From 092b6195423134763db638652dfb3e31b46d7277 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 17 Aug 2022 10:30:00 +0200 Subject: Test dir with starting - --- tests/upload_files.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index ecb7ddf..63374a2 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -118,7 +118,7 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error /// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] #[case(server(&["-u", "someDir"]), vec!["someDir"])] -#[case(server(&["-u", "./someDir"]), vec!["./someDir"])] +#[case(server(&["-u", "./-someDir"]), vec!["./-someDir"])] #[case(server(&["-u", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir"])] #[case(server(&["-u", "someDir/some_sub_dir", "-u", "someDir/some_other_dir"]), vec!["someDir/some_sub_dir", "someDir/some_other_dir"])] -- cgit v1.2.3 From 0d722da1e66b7217f3486351f39faa90e5911fd3 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Wed, 17 Aug 2022 10:31:10 +0200 Subject: Improved output of -h --- src/args.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/args.rs b/src/args.rs index 6ed1e0d..8757bc8 100644 --- a/src/args.rs +++ b/src/args.rs @@ -107,7 +107,7 @@ pub struct CliArgs { #[clap(short = 'q', long = "qrcode")] pub qrcode: bool, - /// Enable file uploading + /// Enable file uploading (and optionally specify for which directory) #[clap(short = 'u', long = "upload-files", value_hint = ValueHint::FilePath, min_values = 0)] pub allowed_upload_dir: Option>, -- cgit v1.2.3 From 234422cc6908557e6a3139444759151a9dae82eb Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 22 Aug 2022 13:40:50 +0200 Subject: fix lint errors --- src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 073a80a..69ef0c4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -251,10 +251,10 @@ impl MiniserveConfig { overwrite_files: args.overwrite_files, show_qrcode: args.qrcode, mkdir_enabled: args.mkdir_enabled, - file_upload: !args.allowed_upload_dir.is_none(), + file_upload: args.allowed_upload_dir.is_some(), allowed_upload_dir: args .allowed_upload_dir - .unwrap_or(vec![]) + .unwrap_or_default() .iter() .map(|x| sanitize_path(x, false).unwrap()) .collect(), -- cgit v1.2.3 From 7e4d1cff19337bee3d2271eead46c39720682ae0 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Tue, 30 Aug 2022 17:05:47 +0200 Subject: trying to handle paths in a way that works for windows --- tests/upload_files.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 63374a2..d96f296 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -117,23 +117,27 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error /// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] -#[case(server(&["-u", "someDir"]), vec!["someDir"])] -#[case(server(&["-u", "./-someDir"]), vec!["./-someDir"])] -#[case(server(&["-u", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir"])] +#[case(server(&["-u", "someDir"]), vec!["someDir".to_string()])] +#[case(server(&["-u", "./-someDir"]), vec!["./-someDir".to_string()])] +#[case(server(&["-u", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir".to_string()])] #[case(server(&["-u", "someDir/some_sub_dir", "-u", "someDir/some_other_dir"]), - vec!["someDir/some_sub_dir", "someDir/some_other_dir"])] + vec!["someDir/some_sub_dir".to_string(), "someDir/some_other_dir".to_string()])] fn uploading_files_to_allowed_dir_works( #[case] server: TestServer, - #[case] upload_dirs: Vec<&str>, + #[case] mut upload_dirs: Vec, ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; + + if cfg!(target_os = "windows"){ + upload_dirs = upload_dirs.iter().map(|x| x.replace("/", "\\")).collect(); + } for upload_dir in upload_dirs { // Create test directory - create_dir_all(server.path().join(upload_dir)).unwrap(); + create_dir_all(server.path().join(upload_dir.as_str())).unwrap(); // Before uploading, check whether the uploaded file does not yet exist. - let body = reqwest::blocking::get(server.url().join(upload_dir)?)?.error_for_status()?; + let body = reqwest::blocking::get(server.url().join(upload_dir.as_str())?)?.error_for_status()?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); @@ -158,7 +162,7 @@ fn uploading_files_to_allowed_dir_works( .error_for_status()?; // After uploading, check whether the uploaded file is now getting listed. - let body = reqwest::blocking::get(server.url().join(upload_dir)?)?; + let body = reqwest::blocking::get(server.url().join(upload_dir.as_str())?)?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); } -- cgit v1.2.3 From d34b411c4052b9127ee8eb853a071be8149289c8 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Fri, 16 Sep 2022 14:55:48 +0200 Subject: Fixed formatting --- tests/upload_files.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index d96f296..13e39ef 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -127,8 +127,8 @@ fn uploading_files_to_allowed_dir_works( #[case] mut upload_dirs: Vec, ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; - - if cfg!(target_os = "windows"){ + + if cfg!(target_os = "windows") { upload_dirs = upload_dirs.iter().map(|x| x.replace("/", "\\")).collect(); } @@ -137,7 +137,8 @@ fn uploading_files_to_allowed_dir_works( create_dir_all(server.path().join(upload_dir.as_str())).unwrap(); // Before uploading, check whether the uploaded file does not yet exist. - let body = reqwest::blocking::get(server.url().join(upload_dir.as_str())?)?.error_for_status()?; + let body = + reqwest::blocking::get(server.url().join(upload_dir.as_str())?)?.error_for_status()?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); -- cgit v1.2.3 From 734f55da94c867ce37fa37af408dd4355f29d139 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Sun, 18 Sep 2022 21:15:35 +0200 Subject: Fixing (hopefully) issue with path on Windows --- tests/upload_files.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 13e39ef..3900514 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -119,19 +119,17 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error #[rstest] #[case(server(&["-u", "someDir"]), vec!["someDir".to_string()])] #[case(server(&["-u", "./-someDir"]), vec!["./-someDir".to_string()])] -#[case(server(&["-u", "someDir/some_sub_dir"]), vec!["someDir/some_sub_dir".to_string()])] -#[case(server(&["-u", "someDir/some_sub_dir", "-u", "someDir/some_other_dir"]), +#[case(server(&["-u", if cfg!(windows) {r"someDir\some_sub_dir"} else {"someDir/some_sub_dir"}]), + vec!["someDir/some_sub_dir".to_string()])] +#[case(server(&["-u", if cfg!(windows) {r"someDir\some_sub_dir"} else {"someDir/some_sub_dir"}, + "-u", if cfg!(windows) {r"someDir\some_other_dir"} else {"someDir/some_other_dir"}]), vec!["someDir/some_sub_dir".to_string(), "someDir/some_other_dir".to_string()])] fn uploading_files_to_allowed_dir_works( #[case] server: TestServer, - #[case] mut upload_dirs: Vec, + #[case] upload_dirs: Vec, ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; - if cfg!(target_os = "windows") { - upload_dirs = upload_dirs.iter().map(|x| x.replace("/", "\\")).collect(); - } - for upload_dir in upload_dirs { // Create test directory create_dir_all(server.path().join(upload_dir.as_str())).unwrap(); -- cgit v1.2.3 From 851c8d9ad17cd905e49ed460be4f93a74fc3d59e Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 19 Sep 2022 10:22:21 +0200 Subject: Switched to using Path in test to handle windows platform. --- tests/upload_files.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 3900514..98ddc2a 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -7,6 +7,7 @@ use rstest::rstest; use select::document::Document; use select::predicate::{Attr, Text}; use std::fs::create_dir_all; +use std::path::Path; #[rstest] fn uploading_files_works(#[with(&["-u"])] server: TestServer) -> Result<(), Error> { @@ -117,26 +118,25 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error /// This tests that we can upload files to the directory specified by --allow-upload-dir #[rstest] -#[case(server(&["-u", "someDir"]), vec!["someDir".to_string()])] -#[case(server(&["-u", "./-someDir"]), vec!["./-someDir".to_string()])] -#[case(server(&["-u", if cfg!(windows) {r"someDir\some_sub_dir"} else {"someDir/some_sub_dir"}]), - vec!["someDir/some_sub_dir".to_string()])] -#[case(server(&["-u", if cfg!(windows) {r"someDir\some_sub_dir"} else {"someDir/some_sub_dir"}, - "-u", if cfg!(windows) {r"someDir\some_other_dir"} else {"someDir/some_other_dir"}]), - vec!["someDir/some_sub_dir".to_string(), "someDir/some_other_dir".to_string()])] +#[case(server(&["-u", "someDir"]), vec!["someDir"])] +#[case(server(&["-u", "./-someDir"]), vec!["./-someDir"])] +#[case(server(&["-u", Path::new("someDir/some_sub_dir").to_str().unwrap()]), + vec!["someDir/some_sub_dir"])] +#[case(server(&["-u", Path::new("someDir/some_sub_dir").to_str().unwrap(), + "-u", Path::new("someDir/some_other_dir").to_str().unwrap()]), + vec!["someDir/some_sub_dir", "someDir/some_other_dir"])] fn uploading_files_to_allowed_dir_works( #[case] server: TestServer, - #[case] upload_dirs: Vec, + #[case] upload_dirs: Vec<&str>, ) -> Result<(), Error> { let test_file_name = "uploaded test file.txt"; for upload_dir in upload_dirs { // Create test directory - create_dir_all(server.path().join(upload_dir.as_str())).unwrap(); + create_dir_all(server.path().join(Path::new(upload_dir))).unwrap(); // Before uploading, check whether the uploaded file does not yet exist. - let body = - reqwest::blocking::get(server.url().join(upload_dir.as_str())?)?.error_for_status()?; + let body = reqwest::blocking::get(server.url().join(upload_dir)?)?.error_for_status()?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).all(|x| x.text() != test_file_name)); @@ -161,7 +161,7 @@ fn uploading_files_to_allowed_dir_works( .error_for_status()?; // After uploading, check whether the uploaded file is now getting listed. - let body = reqwest::blocking::get(server.url().join(upload_dir.as_str())?)?; + let body = reqwest::blocking::get(server.url().join(upload_dir)?)?; let parsed = Document::from_read(body)?; assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); } -- cgit v1.2.3 From 6577af2b8d802ad213968e4b7c9f2823c1ab52dc Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 19 Sep 2022 13:06:17 +0200 Subject: Changed handling of allowed path to fix Windows --- src/config.rs | 10 ++++++++-- src/renderer.rs | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/config.rs b/src/config.rs index 69ef0c4..cef7cda 100644 --- a/src/config.rs +++ b/src/config.rs @@ -89,7 +89,7 @@ pub struct MiniserveConfig { pub file_upload: bool, /// List of allowed upload directories - pub allowed_upload_dir: Vec, + pub allowed_upload_dir: Vec, /// HTML accept attribute value pub uploadable_media_type: Option, @@ -256,7 +256,13 @@ impl MiniserveConfig { .allowed_upload_dir .unwrap_or_default() .iter() - .map(|x| sanitize_path(x, false).unwrap()) + .map(|x| { + sanitize_path(x, false) + .unwrap() + .to_str() + .unwrap() + .replace(r"\", "/") + }) .collect(), uploadable_media_type, tar_enabled: args.enable_tar, diff --git a/src/renderer.rs b/src/renderer.rs index b98a595..38d2617 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -44,7 +44,7 @@ pub fn page( || conf .allowed_upload_dir .iter() - .any(|x| encoded_dir.starts_with(&format!("/{}", x.display()))); + .any(|x| encoded_dir.starts_with(&format!("/{}", x))); html! { (DOCTYPE) -- cgit v1.2.3 From 5d11d6abd1e14822826fdf04fbb175355e0aee4d Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 19 Sep 2022 13:26:24 +0200 Subject: Fixed clippy issue (single-char string to char) --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index cef7cda..7ca0693 100644 --- a/src/config.rs +++ b/src/config.rs @@ -261,7 +261,7 @@ impl MiniserveConfig { .unwrap() .to_str() .unwrap() - .replace(r"\", "/") + .replace('\\', "/") }) .collect(), uploadable_media_type, -- cgit v1.2.3 From 20a055dd82b009e94b1aa681cc4329f17e552f44 Mon Sep 17 00:00:00 2001 From: Jonas Diemer Date: Mon, 19 Sep 2022 16:43:50 +0200 Subject: Return 403 instead of 500 for upload errs --- src/errors.rs | 6 ++++++ src/file_upload.rs | 4 +--- tests/upload_files.rs | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index b2ed459..06569d3 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -22,6 +22,10 @@ pub enum ContextualError { #[error("File already exists, and the overwrite_files option has not been set")] DuplicateFileError, + /// Upload not allowed + #[error("Upload not allowed to this directory")] + UploadForbiddenError, + /// Any error related to an invalid path (failed to retrieve entry name, unexpected entry type, etc) #[error("Invalid path\ncaused by: {0}")] InvalidPathError(String), @@ -88,6 +92,8 @@ impl ResponseError for ContextualError { Self::InsufficientPermissionsError(_) => StatusCode::FORBIDDEN, Self::InvalidHttpCredentials => StatusCode::UNAUTHORIZED, Self::InvalidHttpRequestError(_) => StatusCode::BAD_REQUEST, + Self::DuplicateFileError => StatusCode::FORBIDDEN, + Self::UploadForbiddenError => StatusCode::FORBIDDEN, _ => StatusCode::INTERNAL_SERVER_ERROR, } } diff --git a/src/file_upload.rs b/src/file_upload.rs index 0232c7e..cf214b8 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -179,9 +179,7 @@ pub async fn upload_file( .any(|s| upload_path.starts_with(s)); if !upload_allowed { - return Err(ContextualError::InvalidPathError( - "Not allowed to upload to this path".to_string(), - )); + return Err(ContextualError::UploadForbiddenError); } // Disallow the target path to go outside of the served directory diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 98ddc2a..196f3cd 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -100,7 +100,7 @@ fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error let client = Client::new(); // Ensure uploading fails and returns an error assert_eq!( - 500, + 403, client .post(server.url().join("/upload?path=/")?) .multipart(form) -- cgit v1.2.3