From 007baaf4818181a858f3ef122a7359b3a3513fc9 Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Fri, 26 Jan 2024 14:21:14 +0800 Subject: [PATCH 1/6] rm feature backend impl --- src/args.rs | 11 ++++++++ src/config.rs | 38 ++++++++++++++++++++------- src/errors.rs | 5 ++++ src/file_op.rs | 71 ++++++++++++++++++++++++++++++++++++++++++++++++-- src/main.rs | 4 +++ 5 files changed, 117 insertions(+), 12 deletions(-) diff --git a/src/args.rs b/src/args.rs index dd09f65e4..8da4d2172 100644 --- a/src/args.rs +++ b/src/args.rs @@ -198,6 +198,17 @@ pub struct CliArgs { #[arg(short = 'o', long = "overwrite-files", env = "OVERWRITE_FILES")] pub overwrite_files: bool, + /// Enable file and directory deletion (and optionally specify for which directory) + #[arg( + short = 'R', + long = "rm-files", + value_hint = ValueHint::DirPath, + num_args(0..=1), + value_delimiter(','), + env = "MINISERVE_ALLOWED_RM_DIR" + )] + pub allowed_rm_dir: Option>, + /// Enable uncompressed tar archive generation #[arg(short = 'r', long = "enable-tar", env = "MINISERVE_ENABLE_TAR")] pub enable_tar: bool, diff --git a/src/config.rs b/src/config.rs index f468365ad..adc677ed0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,7 +2,7 @@ use std::{ fs::File, io::{BufRead, BufReader}, net::{IpAddr, Ipv4Addr, Ipv6Addr}, - path::PathBuf, + path::{Path, PathBuf}, }; use actix_web::http::header::HeaderMap; @@ -110,6 +110,12 @@ pub struct MiniserveConfig { /// Enable upload to override existing files pub overwrite_files: bool, + /// Enable file and directory deletion + pub rm_enabled: bool, + + /// List of allowed deletion directories + pub allowed_rm_dir: Vec, + /// If false, creation of uncompressed tar archives is disabled pub tar_enabled: bool, @@ -257,15 +263,14 @@ impl MiniserveConfig { let allowed_upload_dir = args .allowed_upload_dir .as_ref() - .map(|v| { - v.iter() - .map(|p| { - sanitize_path(p, args.hidden) - .map(|p| p.display().to_string().replace('\\', "/")) - .ok_or(anyhow!("Illegal path {p:?}")) - }) - .collect() - }) + .map(|paths| validate_allowed_paths(paths, args.hidden)) + .transpose()? + .unwrap_or_default(); + + let allowed_rm_dir = args + .allowed_rm_dir + .as_ref() + .map(|paths| validate_allowed_paths(paths, args.hidden)) .transpose()? .unwrap_or_default(); @@ -294,6 +299,8 @@ impl MiniserveConfig { file_upload: args.allowed_upload_dir.is_some(), allowed_upload_dir, uploadable_media_type, + rm_enabled: args.allowed_rm_dir.is_some(), + allowed_rm_dir, tar_enabled: args.enable_tar, tar_gz_enabled: args.enable_tar_gz, zip_enabled: args.enable_zip, @@ -311,3 +318,14 @@ impl MiniserveConfig { }) } } + +fn validate_allowed_paths(paths: &[impl AsRef], allow_hidden: bool) -> Result> { + paths + .iter() + .map(|p| { + sanitize_path(p, allow_hidden) + .map(|p| p.display().to_string().replace('\\', "/")) + .ok_or(anyhow!("Illegal path {:?}", p.as_ref())) + }) + .collect() +} diff --git a/src/errors.rs b/src/errors.rs index 21f8f12be..82c033f6b 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -44,6 +44,10 @@ pub enum RuntimeError { #[error("Upload not allowed to this directory")] UploadForbiddenError, + /// Remove not allowed + #[error("Remove not allowed to this directory")] + RmForbiddenError, + /// Any error related to an invalid path (failed to retrieve entry name, unexpected entry type, etc) #[error("Invalid path\ncaused by: {0}")] InvalidPathError(String), @@ -86,6 +90,7 @@ impl ResponseError for RuntimeError { E::MultipartError(_) => S::BAD_REQUEST, E::DuplicateFileError => S::CONFLICT, E::UploadForbiddenError => S::FORBIDDEN, + E::RmForbiddenError => S::FORBIDDEN, E::InvalidPathError(_) => S::BAD_REQUEST, E::InsufficientPermissionsError(_) => S::FORBIDDEN, E::ParseError(_, _) => S::BAD_REQUEST, diff --git a/src/file_op.rs b/src/file_op.rs index 18bdcbe21..4d9ff151c 100644 --- a/src/file_op.rs +++ b/src/file_op.rs @@ -7,8 +7,10 @@ use actix_web::{http::header, web, HttpRequest, HttpResponse}; use futures::TryFutureExt; use futures::TryStreamExt; use serde::Deserialize; -use tokio::fs::File; -use tokio::io::AsyncWriteExt; +use tokio::{ + fs::{self, File}, + io::AsyncWriteExt, +}; use crate::{ config::MiniserveConfig, errors::RuntimeError, file_utils::contains_symlink, @@ -243,3 +245,68 @@ pub async fn upload_file( .append_header((header::LOCATION, return_path)) .finish()) } + +/// Handle incoming request to remove a file or directory. +/// +/// Target file path is expected as path parameter in URI and is interpreted as relative from +/// server root directory. Any path which will go outside of this directory is considered +/// invalid. +pub async fn rm_file( + req: HttpRequest, + query: web::Query, +) -> Result { + let conf = req.app_data::().unwrap(); + let rm_path = sanitize_path(&query.path, conf.show_hidden).ok_or_else(|| { + RuntimeError::InvalidPathError("Invalid value for 'path' parameter".to_string()) + })?; + let app_root_dir = conf.path.canonicalize().map_err(|e| { + RuntimeError::IoError("Failed to resolve path served by miniserve".to_string(), e) + })?; + + // Disallow paths outside of allowed directories + let rm_allowed = conf.allowed_rm_dir.is_empty() + || conf.allowed_rm_dir.iter().any(|s| rm_path.starts_with(s)); + + if !rm_allowed { + return Err(RuntimeError::RmForbiddenError); + } + + // Disallow the target path to go outside of the served directory + let canonicalized_rm_path = match app_root_dir.join(&rm_path).canonicalize() { + Ok(path) if !conf.no_symlinks => Ok(path), + Ok(path) if path.starts_with(&app_root_dir) => Ok(path), + _ => Err(RuntimeError::InvalidHttpRequestError( + "Invalid value for 'path' parameter".to_string(), + )), + }?; + + // Handle non-existent path + if !canonicalized_rm_path.exists() { + return Err(RuntimeError::RouteNotFoundError(format!( + "{rm_path:?} does not exist" + ))); + } + + // Remove + let rm_res = if canonicalized_rm_path.is_dir() { + fs::remove_dir_all(&canonicalized_rm_path).await + } else { + fs::remove_file(&canonicalized_rm_path).await + }; + if let Err(err) = rm_res { + Err(RuntimeError::IoError( + format!("Failed to remove {rm_path:?}"), + err, + ))?; + } + + let return_path = req + .headers() + .get(header::REFERER) + .and_then(|h| h.to_str().ok()) + .unwrap_or("/"); + + Ok(HttpResponse::SeeOther() + .append_header((header::LOCATION, return_path)) + .finish()) +} diff --git a/src/main.rs b/src/main.rs index 1434a0ce4..f7d2dfe45 100644 --- a/src/main.rs +++ b/src/main.rs @@ -373,6 +373,10 @@ fn configure_app(app: &mut web::ServiceConfig, conf: &MiniserveConfig) { // Allow file upload app.service(web::resource("/upload").route(web::post().to(file_op::upload_file))); } + if conf.rm_enabled { + // Allow file and directory deletion + app.service(web::resource("/rm").route(web::post().to(file_op::rm_file))); + } // Handle directories app.service(dir_service()); } From 9d38752765165c548ecf254f630b36e625bb2377 Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Fri, 26 Jan 2024 16:56:02 +0800 Subject: [PATCH 2/6] rm feature frontend impl --- src/renderer.rs | 51 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/src/renderer.rs b/src/renderer.rs index 9c60dcc9f..0d7dc9240 100644 --- a/src/renderer.rs +++ b/src/renderer.rs @@ -36,6 +36,7 @@ pub fn page( } let upload_route = format!("{}/upload", &conf.route_prefix); + let rm_route = format!("{}/rm", &conf.route_prefix); let (sort_method, sort_order) = (query_params.sort, query_params.order); let upload_action = build_upload_action(&upload_route, encoded_dir, sort_method, sort_order); @@ -48,6 +49,17 @@ pub fn page( .allowed_upload_dir .iter() .any(|x| encoded_dir.starts_with(&format!("/{x}"))); + let rm_allowed = conf.allowed_rm_dir.is_empty() + || conf + .allowed_rm_dir + .iter() + .any(|x| encoded_dir.starts_with(&format!("/{x}"))); + + // OR with other conditions in the future if more actions are added + let show_actions = conf.rm_enabled && rm_allowed; + let actions_conf = show_actions.then(|| ActionsConf { + rm_route: &rm_route, + }); html! { (DOCTYPE) @@ -132,14 +144,17 @@ pub fn page( } table { thead { - th.name { (build_link("name", "Name", sort_method, sort_order)) } - th.size { (build_link("size", "Size", sort_method, sort_order)) } - th.date { (build_link("date", "Last modification", sort_method, sort_order)) } + th.name { (sortable_title("name", "Name", sort_method, sort_order)) } + th.size { (sortable_title("size", "Size", sort_method, sort_order)) } + th.date { (sortable_title("date", "Last modification", sort_method, sort_order)) } + @if show_actions { + th.actions { span { "Actions" } } + } } tbody { @if !is_root { tr { - td colspan="3" { + td colspan=(3 + show_actions as usize) { p { span.root-chevron { (chevron_left()) } a.root href=(parametrized_link("../", sort_method, sort_order, false)) { @@ -150,7 +165,7 @@ pub fn page( } } @for entry in entries { - (entry_row(entry, sort_method, sort_order, false)) + (entry_row(entry, sort_method, sort_order, false, actions_conf)) } } } @@ -204,7 +219,7 @@ pub fn raw(entries: Vec, is_root: bool) -> Markup { } } @for entry in entries { - (entry_row(entry, None, None, true)) + (entry_row(entry, None, None, true, None)) } } } @@ -450,7 +465,7 @@ fn parametrized_link( } /// Partial: table header link -fn build_link( +fn sortable_title( name: &str, title: &str, sort_method: Option, @@ -482,12 +497,29 @@ fn build_link( } } +/// Partial: rm form +fn rm_form(rm_route: &str, encoded_path: &str) -> Markup { + let rm_action = format!("{rm_route}?path={encoded_path}"); + html! { + form class="rm_form" action=(rm_action) method="POST" { + button type="submit" title="Delete" { "✗" } + } + } +} + +#[derive(Copy, Clone, Debug)] +struct ActionsConf<'a> { + /// Route prefix for file removal POST requests. + rm_route: &'a str, +} + /// Partial: row for an entry fn entry_row( entry: Entry, sort_method: Option, sort_order: Option, raw: bool, + actions_conf: Option, ) -> Markup { html! { tr { @@ -545,6 +577,11 @@ fn entry_row( } } } + @if let Some(conf) = actions_conf { + td.actions-cell { + (rm_form(conf.rm_route, &entry.link)) + } + } } } } From 55804bc530d0bcba807971a0f11fcddf99edf6ec Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Fri, 26 Jan 2024 17:25:15 +0800 Subject: [PATCH 3/6] Frontend styling --- data/style.scss | 20 ++++++++++++++++++++ data/themes/archlinux.scss | 2 ++ data/themes/monokai.scss | 2 ++ data/themes/squirrel.scss | 2 ++ data/themes/zenburn.scss | 2 ++ 5 files changed, 28 insertions(+) diff --git a/data/style.scss b/data/style.scss index 32d29890f..04beee3b9 100644 --- a/data/style.scss +++ b/data/style.scss @@ -252,6 +252,10 @@ table thead th.date { width: 21em; } +table thead th.actions { + width: 4em; +} + table tbody tr:nth-child(odd) { background: var(--odd_row_background); } @@ -277,6 +281,22 @@ td.date-cell { justify-content: space-between; } +td.actions-cell button { + padding: 0.1rem 0.3rem; + border-radius: 0.2rem; + border: none; +} + +td.actions-cell .rm_form { + display: flex; + place-content: center; +} + +td.actions-cell .rm_form button { + background: var(--rm_button_background); + color: var(--rm_button_text_color); +} + .history { color: var(--date_text_color); } diff --git a/data/themes/archlinux.scss b/data/themes/archlinux.scss index f95b8bb45..6ff5daeb0 100644 --- a/data/themes/archlinux.scss +++ b/data/themes/archlinux.scss @@ -38,6 +38,8 @@ $generate_default: true !default; --upload_form_background: #4b5162; --upload_button_background: #ea95ff; --upload_button_text_color: #ffffff; + --rm_button_background: #ea95ff; + --rm_button_text_color: #ffffff; --drag_background: #3333338f; --drag_border_color: #fefefe; --drag_text_color: #fefefe; diff --git a/data/themes/monokai.scss b/data/themes/monokai.scss index 4a477947b..f1eb885cd 100644 --- a/data/themes/monokai.scss +++ b/data/themes/monokai.scss @@ -38,6 +38,8 @@ $generate_default: true !default; --upload_form_background: #49483e; --upload_button_background: #ae81ff; --upload_button_text_color: #f8f8f0; + --rm_button_background: #ae81ff; + --rm_button_text_color: #f8f8f0; --drag_background: #3333338f; --drag_border_color: #f8f8f2; --drag_text_color: #f8f8f2; diff --git a/data/themes/squirrel.scss b/data/themes/squirrel.scss index 9eb572e16..f88a07a49 100644 --- a/data/themes/squirrel.scss +++ b/data/themes/squirrel.scss @@ -38,6 +38,8 @@ $generate_default: true !default; --upload_form_background: #f2f2f2; --upload_button_background: #d02474; --upload_button_text_color: #ffffff; + --rm_button_background: #d02474; + --rm_button_text_color: #ffffff; --drag_background: #3333338f; --drag_border_color: #ffffff; --drag_text_color: #ffffff; diff --git a/data/themes/zenburn.scss b/data/themes/zenburn.scss index 9eb4d112b..2aa97649a 100644 --- a/data/themes/zenburn.scss +++ b/data/themes/zenburn.scss @@ -38,6 +38,8 @@ $generate_default: true !default; --upload_form_background: #777777; --upload_button_background: #cc9393; --upload_button_text_color: #efefef; + --rm_button_background: #cc9393; + --rm_button_text_color: #efefef; --drag_background: #3333338f; --drag_border_color: #efefef; --drag_text_color: #efefef; From f566cb4468ce21f90aba30ad0b452a088f67e502 Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Sun, 7 Jul 2024 11:23:37 +0800 Subject: [PATCH 4/6] Add tests for rm feature --- tests/fixtures/mod.rs | 17 +++- tests/rm_files.rs | 193 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 4 deletions(-) create mode 100644 tests/rm_files.rs diff --git a/tests/fixtures/mod.rs b/tests/fixtures/mod.rs index 96b800247..60470d276 100644 --- a/tests/fixtures/mod.rs +++ b/tests/fixtures/mod.rs @@ -42,6 +42,11 @@ pub static DIRECTORIES: &[&str] = &["dira/", "dirb/", "dirc/"]; #[allow(dead_code)] pub static HIDDEN_DIRECTORIES: &[&str] = &[".hidden_dir1/", ".hidden_dir2/"]; +/// Files nested at different levels under the same root directory +#[allow(dead_code)] +pub static NESTED_FILES_UNDER_SINGLE_ROOT: &[&str] = + &["someDir/alpha", "someDir/some_sub_dir/bravo"]; + /// Name of a deeply nested file #[allow(dead_code)] pub static DEEPLY_NESTED_FILE: &str = "very/deeply/nested/test.rs"; @@ -72,10 +77,14 @@ pub fn tmpdir() -> TempDir { } } - tmpdir - .child(DEEPLY_NESTED_FILE) - .write_str("File in a deeply nested directory.") - .expect("Couldn't write to file"); + let mut nested_files = NESTED_FILES_UNDER_SINGLE_ROOT.to_vec(); + nested_files.push(DEEPLY_NESTED_FILE); + for file in nested_files { + tmpdir + .child(file) + .write_str("File in a deeply nested directory.") + .expect("Couldn't write to file"); + } tmpdir } diff --git a/tests/rm_files.rs b/tests/rm_files.rs new file mode 100644 index 000000000..b3aca1c75 --- /dev/null +++ b/tests/rm_files.rs @@ -0,0 +1,193 @@ +mod fixtures; + +use anyhow::bail; +use assert_fs::fixture::TempDir; +use fixtures::{server, server_no_stderr, tmpdir, TestServer}; +use reqwest::blocking::Client; +use reqwest::StatusCode; +use rstest::rstest; +use std::path::Path; +use url::Url; + +use crate::fixtures::{ + DEEPLY_NESTED_FILE, DIRECTORIES, FILES, HIDDEN_DIRECTORIES, HIDDEN_FILES, + NESTED_FILES_UNDER_SINGLE_ROOT, +}; + +fn assert_rm_ok(base_url: Url, paths: &[impl AsRef]) -> anyhow::Result<()> { + let client = Client::new(); + + for path in paths.iter().map(AsRef::as_ref) { + // check path exists + let _get_res = client + .get(base_url.join(path)?) + .send()? + .error_for_status()?; + + // delete + let req_path = format!("rm?path=/{path}"); + let _del_res = client + .post(base_url.join(&req_path)?) + .send()? + .error_for_status()?; + + // check path is gone + let get_res = client.get(base_url.join(path)?).send()?; + if get_res.status() != StatusCode::NOT_FOUND { + bail!("Unexpected status code: {}", get_res.status()); + } + } + + Ok(()) +} + +/// The `check_paths_exist` parameter allows skipping this check before and after +/// the deletion attempt in case these paths should be inaccessible via GET. +fn assert_rm_err( + base_url: Url, + paths: &[impl AsRef], + check_paths_exist: bool, +) -> anyhow::Result<()> { + let client = Client::new(); + + for path in paths.iter().map(AsRef::as_ref) { + // check path exists + if check_paths_exist { + let _get_res = client + .get(base_url.join(path)?) + .send()? + .error_for_status()?; + } + + // delete + let req_path = format!("rm?path=/{path}"); + let del_res = client.post(base_url.join(&req_path)?).send()?; + if !del_res.status().is_client_error() { + bail!("Unexpected status code: {}", del_res.status()); + } + + // check path still exists + if check_paths_exist { + let _get_res = client + .get(base_url.join(path)?) + .send()? + .error_for_status()?; + } + } + + Ok(()) +} + +#[rstest] +fn rm_disabled_by_default(server: TestServer) -> anyhow::Result<()> { + assert_rm_err( + server.url(), + &[ + FILES, + HIDDEN_FILES, + DIRECTORIES, + HIDDEN_DIRECTORIES, + &[DEEPLY_NESTED_FILE], + ] + .concat(), + true, + ) +} + +#[rstest] +fn rm_works(#[with(&["-R"])] server: TestServer) -> anyhow::Result<()> { + assert_rm_ok( + server.url(), + &[FILES, DIRECTORIES, &[DEEPLY_NESTED_FILE]].concat(), + ) +} + +#[rstest] +fn cannot_rm_hidden_when_disallowed( + #[with(&["-R"])] server_no_stderr: TestServer, +) -> anyhow::Result<()> { + assert_rm_err( + server_no_stderr.url(), + &[HIDDEN_FILES, HIDDEN_DIRECTORIES].concat(), + false, + ) +} + +#[rstest] +fn can_rm_hidden_when_allowed( + #[with(&["-R", "-H"])] server_no_stderr: TestServer, +) -> anyhow::Result<()> { + assert_rm_ok( + server_no_stderr.url(), + &[HIDDEN_FILES, HIDDEN_DIRECTORIES].concat(), + ) +} + +/// This test runs the server with --allowed-rm-dir argument and checks that +/// deletions in a different directory are actually prevented. +#[rstest] +#[case(server_no_stderr(&["-R", "someOtherDir"]))] +#[case(server_no_stderr(&["-R", "someDir/some_other_sub_dir"]))] +fn rm_is_restricted(#[case] server: TestServer) -> anyhow::Result<()> { + assert_rm_err(server.url(), NESTED_FILES_UNDER_SINGLE_ROOT, true) +} + +/// This test runs the server with --allowed-rm-dir argument and checks that +/// deletions of the specified directories themselves are allowed. +/// +/// Both ways of specifying multiple directories are tested. +#[rstest] +#[case(server(&["-R", "dira,dirb,dirc"]))] +#[case(server(&["-R", "dira", "-R", "dirb", "-R", "dirc"]))] +fn can_rm_allowed_dir(#[case] server: TestServer) -> anyhow::Result<()> { + assert_rm_ok(server.url(), DIRECTORIES) +} + +/// This tests that we can delete from directories specified by --allow-rm-dir. +#[rstest] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/alpha")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir//alpha")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/././alpha")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/some_sub_dir")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/some_sub_dir/")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir//some_sub_dir")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/./some_sub_dir")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/some_sub_dir/bravo")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir//some_sub_dir//bravo")] +#[case(server_no_stderr(&["-R", "someDir"]), "someDir/./some_sub_dir/../some_sub_dir/bravo")] +#[case(server_no_stderr(&["-R", "someDir/some_sub_dir"]), "someDir/some_sub_dir/bravo")] +#[case(server_no_stderr(&["-R", Path::new("someDir/some_sub_dir").to_str().unwrap()]), + "someDir/some_sub_dir/bravo")] +fn can_rm_from_allowed_dir(#[case] server: TestServer, #[case] file: &str) -> anyhow::Result<()> { + assert_rm_ok(server.url(), &[file]) +} + +/// Test deleting from symlinked directories that point to outside the server root. +#[rstest] +#[case(server(&["-R"]), true)] +#[case(server_no_stderr(&["-R", "--no-symlinks"]), false)] +fn rm_from_symlinked_dir( + #[case] server: TestServer, + #[case] should_succeed: bool, + #[from(tmpdir)] target: TempDir, +) -> anyhow::Result<()> { + #[cfg(unix)] + use std::os::unix::fs::symlink as symlink_dir; + #[cfg(windows)] + use std::os::windows::fs::symlink_dir; + + // create symlink + const LINK_NAME: &str = "linked"; + symlink_dir(target.path(), server.path().join(LINK_NAME))?; + + let files_through_link = &[FILES, DIRECTORIES] + .concat() + .iter() + .map(|name| format!("{LINK_NAME}/{name}")) + .collect::>(); + if should_succeed { + assert_rm_ok(server.url(), files_through_link) + } else { + assert_rm_err(server.url(), files_through_link, false) + } +} From 7c97704d9f1bbeeae6f0ee4fe2b38f5152d7f248 Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Sat, 14 Sep 2024 16:38:50 +0800 Subject: [PATCH 5/6] Separate `path_utils.rs` from `listing.rs` This allows the encoding set definitions to be `include!`ed into integration tests --- src/listing.rs | 23 +---------------------- src/main.rs | 1 + src/path_utils.rs | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 22 deletions(-) create mode 100644 src/path_utils.rs diff --git a/src/listing.rs b/src/listing.rs index a9d2e3a34..928aa4ba1 100644 --- a/src/listing.rs +++ b/src/listing.rs @@ -17,30 +17,9 @@ use strum::{Display, EnumString}; use crate::archive::ArchiveMethod; use crate::auth::CurrentUser; use crate::errors::{self, RuntimeError}; +use crate::path_utils::percent_encode_sets::COMPONENT; use crate::renderer; -use self::percent_encode_sets::COMPONENT; - -/// "percent-encode sets" as defined by WHATWG specs: -/// https://url.spec.whatwg.org/#percent-encoded-bytes -mod percent_encode_sets { - use percent_encoding::{AsciiSet, CONTROLS}; - pub const QUERY: &AsciiSet = &CONTROLS.add(b' ').add(b'"').add(b'#').add(b'<').add(b'>'); - pub const PATH: &AsciiSet = &QUERY.add(b'?').add(b'`').add(b'{').add(b'}'); - pub const USERINFO: &AsciiSet = &PATH - .add(b'/') - .add(b':') - .add(b';') - .add(b'=') - .add(b'@') - .add(b'[') - .add(b'\\') - .add(b']') - .add(b'^') - .add(b'|'); - pub const COMPONENT: &AsciiSet = &USERINFO.add(b'$').add(b'%').add(b'&').add(b'+').add(b','); -} - /// Query parameters used by listing APIs #[derive(Deserialize, Default)] pub struct ListingQueryParameters { diff --git a/src/main.rs b/src/main.rs index f7d2dfe45..60fde3f49 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,6 +25,7 @@ mod errors; mod file_op; mod file_utils; mod listing; +mod path_utils; mod pipe; mod renderer; diff --git a/src/path_utils.rs b/src/path_utils.rs new file mode 100644 index 000000000..8d73fba34 --- /dev/null +++ b/src/path_utils.rs @@ -0,0 +1,19 @@ +/// "percent-encode sets" as defined by WHATWG specs: +/// https://url.spec.whatwg.org/#percent-encoded-bytes +pub mod percent_encode_sets { + use percent_encoding::{AsciiSet, CONTROLS}; + pub const QUERY: &AsciiSet = &CONTROLS.add(b' ').add(b'"').add(b'#').add(b'<').add(b'>'); + pub const PATH: &AsciiSet = &QUERY.add(b'?').add(b'`').add(b'{').add(b'}'); + pub const USERINFO: &AsciiSet = &PATH + .add(b'/') + .add(b':') + .add(b';') + .add(b'=') + .add(b'@') + .add(b'[') + .add(b'\\') + .add(b']') + .add(b'^') + .add(b'|'); + pub const COMPONENT: &AsciiSet = &USERINFO.add(b'$').add(b'%').add(b'&').add(b'+').add(b','); +} From 2cb106b8bd84338955cc8027b0a6c73fe66a27a1 Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Sun, 15 Sep 2024 23:28:26 +0800 Subject: [PATCH 6/6] Fix failing tests --- tests/fixtures/mod.rs | 27 ++++++--- tests/navigation.rs | 40 ++++++------ tests/rm_files.rs | 137 +++++++++++++++++++++++++++--------------- 3 files changed, 127 insertions(+), 77 deletions(-) diff --git a/tests/fixtures/mod.rs b/tests/fixtures/mod.rs index 60470d276..63b520873 100644 --- a/tests/fixtures/mod.rs +++ b/tests/fixtures/mod.rs @@ -4,7 +4,9 @@ use assert_fs::prelude::*; use port_check::free_local_port; use reqwest::Url; use rstest::fixture; +use std::path::Path; use std::process::{Child, Command, Stdio}; +use std::sync::LazyLock; use std::thread::sleep; use std::time::{Duration, Instant}; @@ -43,13 +45,24 @@ pub static DIRECTORIES: &[&str] = &["dira/", "dirb/", "dirc/"]; pub static HIDDEN_DIRECTORIES: &[&str] = &[".hidden_dir1/", ".hidden_dir2/"]; /// Files nested at different levels under the same root directory +/// +/// This is not a `&[&str]` because in percent-encoding, path segments and full paths +/// are encoded differently. #[allow(dead_code)] -pub static NESTED_FILES_UNDER_SINGLE_ROOT: &[&str] = - &["someDir/alpha", "someDir/some_sub_dir/bravo"]; - -/// Name of a deeply nested file +pub static NESTED_FILES_UNDER_SINGLE_ROOT: LazyLock> = LazyLock::new(|| { + vec![ + Path::new("someDir/alpha"), + Path::new("someDir/some_sub_dir/bravo"), + ] +}); + +/// Path to a deeply nested file +/// +/// This is not a `&str` because in percent-encoding, path segments and full paths +/// are encoded differently. #[allow(dead_code)] -pub static DEEPLY_NESTED_FILE: &str = "very/deeply/nested/test.rs"; +pub static DEEPLY_NESTED_FILE: LazyLock<&Path> = + LazyLock::new(|| Path::new("very/deeply/nested/test.rs")); /// Test fixture which creates a temporary directory with a few files and directories inside. /// The directories also contain files. @@ -77,8 +90,8 @@ pub fn tmpdir() -> TempDir { } } - let mut nested_files = NESTED_FILES_UNDER_SINGLE_ROOT.to_vec(); - nested_files.push(DEEPLY_NESTED_FILE); + let mut nested_files = NESTED_FILES_UNDER_SINGLE_ROOT.clone(); + nested_files.push(&DEEPLY_NESTED_FILE); for file in nested_files { tmpdir .child(file) diff --git a/tests/navigation.rs b/tests/navigation.rs index c8fd49419..ccbd35255 100644 --- a/tests/navigation.rs +++ b/tests/navigation.rs @@ -5,6 +5,7 @@ use fixtures::{server, Error, TestServer, DEEPLY_NESTED_FILE, DIRECTORIES}; use pretty_assertions::{assert_eq, assert_ne}; use rstest::rstest; use select::document::Document; +use std::path::Component; use std::process::{Command, Stdio}; use utils::get_link_from_text; use utils::get_link_hrefs_with_prefix; @@ -69,16 +70,19 @@ fn can_navigate_into_dirs_and_back(server: TestServer) -> Result<(), Error> { #[rstest] /// We can navigate deep into the file tree and back using shown links. fn can_navigate_deep_into_dirs_and_back(server: TestServer) -> Result<(), Error> { - // Create a vector of directory names. We don't need to fetch the file and so we'll - // remove that part. - let dir_names = { - let mut comps = DEEPLY_NESTED_FILE - .split('/') - .map(|d| format!("{d}/")) - .collect::>(); - comps.pop(); - comps - }; + // Create a vector of parent directory names. + let dir_names = DEEPLY_NESTED_FILE + .parent() + .unwrap() + .components() + .map(|comp| { + let Component::Normal(dir) = comp else { + unreachable!() + }; + dir.to_str().unwrap() + }) + .collect::>(); + dbg!(&dir_names); let base_url = server.url(); // First we'll go forwards through the directory tree and then we'll go backwards. @@ -88,7 +92,8 @@ fn can_navigate_deep_into_dirs_and_back(server: TestServer) -> Result<(), Error> let resp = reqwest::blocking::get(next_url.as_str())?; let body = resp.error_for_status()?; let parsed = Document::from_read(body)?; - let dir_elem = get_link_from_text(&parsed, dir_name).expect("Dir not found."); + let dir_elem = + get_link_from_text(&parsed, &format!("{dir_name}/")).expect("Dir not found."); next_url = next_url.join(&dir_elem)?; } assert_ne!(base_url, next_url); @@ -115,19 +120,10 @@ fn can_navigate_using_breadcrumbs( #[case] server: TestServer, #[case] title_name: String, ) -> Result<(), Error> { - // Create a vector of directory names. We don't need to fetch the file and so we'll - // remove that part. - let dir: String = { - let mut comps = DEEPLY_NESTED_FILE - .split('/') - .map(|d| format!("{d}/")) - .collect::>(); - comps.pop(); - comps.join("") - }; + let dir = DEEPLY_NESTED_FILE.parent().unwrap().to_str().unwrap(); let base_url = server.url(); - let nested_url = base_url.join(&dir)?; + let nested_url = base_url.join(dir)?; let resp = reqwest::blocking::get(nested_url.as_str())?; let body = resp.error_for_status()?; diff --git a/tests/rm_files.rs b/tests/rm_files.rs index b3aca1c75..884165585 100644 --- a/tests/rm_files.rs +++ b/tests/rm_files.rs @@ -3,10 +3,14 @@ mod fixtures; use anyhow::bail; use assert_fs::fixture::TempDir; use fixtures::{server, server_no_stderr, tmpdir, TestServer}; +use percent_encoding::utf8_percent_encode; use reqwest::blocking::Client; use reqwest::StatusCode; use rstest::rstest; -use std::path::Path; +use std::{ + iter, + path::{Component, Path}, +}; use url::Url; use crate::fixtures::{ @@ -14,25 +18,54 @@ use crate::fixtures::{ NESTED_FILES_UNDER_SINGLE_ROOT, }; -fn assert_rm_ok(base_url: Url, paths: &[impl AsRef]) -> anyhow::Result<()> { +include!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/", + "src/path_utils.rs" +)); + +/// Construct a path for a GET request, +/// with each path component being separately encoded. +fn make_get_path(unencoded_path: impl AsRef) -> String { + unencoded_path + .as_ref() + .components() + .map(|comp| match comp { + Component::Prefix(_) | Component::RootDir => unreachable!("Not currently used"), + Component::CurDir => ".", + Component::ParentDir => "..", + Component::Normal(comp) => comp.to_str().unwrap(), + }) + .map(|comp| utf8_percent_encode(comp, percent_encode_sets::COMPONENT).to_string()) + .collect::>() + .join("/") +} + +/// Construct a path for a deletion POST request without any further encoding. +/// +/// This should be kept consistent with implementation. +fn make_del_path(unencoded_path: impl AsRef) -> String { + format!("rm?path=/{}", make_get_path(unencoded_path)) +} + +fn assert_rm_ok(base_url: Url, unencoded_paths: &[impl AsRef]) -> anyhow::Result<()> { let client = Client::new(); - for path in paths.iter().map(AsRef::as_ref) { + for file_path in unencoded_paths.iter().map(AsRef::as_ref) { + // encode + let get_url = base_url.join(&make_get_path(file_path))?; + let del_url = base_url.join(&make_del_path(file_path))?; + println!("===== {file_path:?} ====="); + println!("{get_url}, {del_url}"); + // check path exists - let _get_res = client - .get(base_url.join(path)?) - .send()? - .error_for_status()?; + let _get_res = client.get(get_url.clone()).send()?.error_for_status()?; // delete - let req_path = format!("rm?path=/{path}"); - let _del_res = client - .post(base_url.join(&req_path)?) - .send()? - .error_for_status()?; + let _del_res = client.post(del_url).send()?.error_for_status()?; // check path is gone - let get_res = client.get(base_url.join(path)?).send()?; + let get_res = client.get(get_url).send()?; if get_res.status() != StatusCode::NOT_FOUND { bail!("Unexpected status code: {}", get_res.status()); } @@ -45,33 +78,32 @@ fn assert_rm_ok(base_url: Url, paths: &[impl AsRef]) -> anyhow::Result<()> /// the deletion attempt in case these paths should be inaccessible via GET. fn assert_rm_err( base_url: Url, - paths: &[impl AsRef], + unencoded_paths: &[impl AsRef], check_paths_exist: bool, ) -> anyhow::Result<()> { let client = Client::new(); - for path in paths.iter().map(AsRef::as_ref) { + for file_path in unencoded_paths.iter().map(AsRef::as_ref) { + // encode + let get_url = base_url.join(&make_get_path(file_path))?; + let del_url = base_url.join(&make_del_path(file_path))?; + println!("===== {file_path:?} ====="); + println!("{get_url}, {del_url}"); + // check path exists if check_paths_exist { - let _get_res = client - .get(base_url.join(path)?) - .send()? - .error_for_status()?; + let _get_res = client.get(get_url.clone()).send()?.error_for_status()?; } // delete - let req_path = format!("rm?path=/{path}"); - let del_res = client.post(base_url.join(&req_path)?).send()?; + let del_res = client.post(del_url).send()?; if !del_res.status().is_client_error() { bail!("Unexpected status code: {}", del_res.status()); } // check path still exists if check_paths_exist { - let _get_res = client - .get(base_url.join(path)?) - .send()? - .error_for_status()?; + let _get_res = client.get(get_url).send()?.error_for_status()?; } } @@ -80,26 +112,35 @@ fn assert_rm_err( #[rstest] fn rm_disabled_by_default(server: TestServer) -> anyhow::Result<()> { - assert_rm_err( - server.url(), - &[ - FILES, - HIDDEN_FILES, - DIRECTORIES, - HIDDEN_DIRECTORIES, - &[DEEPLY_NESTED_FILE], - ] - .concat(), - true, - ) + let paths = [FILES, DIRECTORIES] + .concat() + .into_iter() + .map(Path::new) + .chain(iter::once(DEEPLY_NESTED_FILE.as_ref())) + .collect::>(); + assert_rm_err(server.url(), &paths, true) +} + +#[rstest] +fn rm_disabled_by_default_with_hidden(#[with(&["-H"])] server: TestServer) -> anyhow::Result<()> { + let paths = [FILES, HIDDEN_FILES, DIRECTORIES, HIDDEN_DIRECTORIES] + .concat() + .into_iter() + .map(Path::new) + .chain(iter::once(DEEPLY_NESTED_FILE.as_ref())) + .collect::>(); + assert_rm_err(server.url(), &paths, true) } #[rstest] fn rm_works(#[with(&["-R"])] server: TestServer) -> anyhow::Result<()> { - assert_rm_ok( - server.url(), - &[FILES, DIRECTORIES, &[DEEPLY_NESTED_FILE]].concat(), - ) + let paths = [FILES, DIRECTORIES] + .concat() + .into_iter() + .map(Path::new) + .chain(iter::once(DEEPLY_NESTED_FILE.as_ref())) + .collect::>(); + assert_rm_ok(server.url(), &paths) } #[rstest] @@ -129,7 +170,7 @@ fn can_rm_hidden_when_allowed( #[case(server_no_stderr(&["-R", "someOtherDir"]))] #[case(server_no_stderr(&["-R", "someDir/some_other_sub_dir"]))] fn rm_is_restricted(#[case] server: TestServer) -> anyhow::Result<()> { - assert_rm_err(server.url(), NESTED_FILES_UNDER_SINGLE_ROOT, true) + assert_rm_err(server.url(), &NESTED_FILES_UNDER_SINGLE_ROOT, true) } /// This test runs the server with --allowed-rm-dir argument and checks that @@ -177,17 +218,17 @@ fn rm_from_symlinked_dir( use std::os::windows::fs::symlink_dir; // create symlink - const LINK_NAME: &str = "linked"; - symlink_dir(target.path(), server.path().join(LINK_NAME))?; + let link: &Path = Path::new("linked"); + symlink_dir(target.path(), server.path().join(link))?; - let files_through_link = &[FILES, DIRECTORIES] + let files_through_link = [FILES, DIRECTORIES] .concat() .iter() - .map(|name| format!("{LINK_NAME}/{name}")) + .map(|name| link.join(name)) .collect::>(); if should_succeed { - assert_rm_ok(server.url(), files_through_link) + assert_rm_ok(server.url(), &files_through_link) } else { - assert_rm_err(server.url(), files_through_link, false) + assert_rm_err(server.url(), &files_through_link, false) } }