diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ac7ab93..ab5a1ea4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ``` SQLPage now keeps the variable value, producing `https://api.example.com/john.doe` as expected. +- A request that resolves to a directory now returns a 404 page. Directory names that contain a dot are routed to the static file handler, which used to fail with a server error instead. - A `content_security_policy` that does not contain `'nonce-{NONCE}'` is now sent as written, instead of being silently dropped and leaving the response with no `Content-Security-Policy` header at all. Setting the option to the empty string still disables the header, as documented. ## v0.46 diff --git a/src/filesystem.rs b/src/filesystem.rs index 4b29483f..71c8fba7 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -90,8 +90,7 @@ impl FileSystem { .await } (Err(e), _) => { - let status = io_error_status(&e) - .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); + let status = io_error_status(&local_path, &e).await; Err(e).with_status(status).with_context(|| { format!("Unable to read local file metadata for {}", path.display()) }) @@ -146,12 +145,8 @@ impl FileSystem { // no local file, try the database db_fs.read_file(app_state, path.as_ref()).await } - (Err(e), None) if is_path_missing_error(&e) => Err(e) - .with_status(actix_web::http::StatusCode::NOT_FOUND) - .with_context(|| format!("Unable to read local file {}", path.display())), (Err(e), _) => { - let status = io_error_status(&e) - .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); + let status = io_error_status(&local_path, &e).await; Err(e) .with_status(status) .with_context(|| format!("Unable to read local file {}", path.display())) @@ -188,12 +183,11 @@ impl FileSystem { ) -> anyhow::Result { let path = access.path(); let safe_path = self.safe_local_path(app_state, access); - let local_exists = match tokio::fs::try_exists(safe_path).await { + let local_exists = match tokio::fs::try_exists(&safe_path).await { Ok(exists) => exists, Err(e) if is_path_missing_error(&e) => false, Err(e) => { - let status = io_error_status(&e) - .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); + let status = io_error_status(&safe_path, &e).await; return Err(e).with_status(status).with_context(|| { format!("Unable to check if {} exists locally", path.display()) }); @@ -252,16 +246,27 @@ fn is_path_missing_error(error: &std::io::Error) -> bool { matches!(error.kind(), ErrorKind::NotFound | ErrorKind::NotADirectory) } -fn io_error_status(error: &std::io::Error) -> Option { +async fn io_error_status(local_path: &Path, error: &std::io::Error) -> actix_web::http::StatusCode { match error.kind() { - ErrorKind::NotFound | ErrorKind::NotADirectory => { - Some(actix_web::http::StatusCode::NOT_FOUND) + ErrorKind::NotFound | ErrorKind::NotADirectory => actix_web::http::StatusCode::NOT_FOUND, + // Reading a directory reports IsADirectory on unix but PermissionDenied on + // windows, so the path itself has to be inspected to tell the two apart. + ErrorKind::IsADirectory | ErrorKind::PermissionDenied + if is_local_directory(local_path).await => + { + actix_web::http::StatusCode::NOT_FOUND } - ErrorKind::PermissionDenied => Some(actix_web::http::StatusCode::FORBIDDEN), - _ => None, + ErrorKind::PermissionDenied => actix_web::http::StatusCode::FORBIDDEN, + _ => actix_web::http::StatusCode::INTERNAL_SERVER_ERROR, } } +async fn is_local_directory(path: &Path) -> bool { + tokio::fs::metadata(path) + .await + .is_ok_and(|metadata| metadata.is_dir()) +} + async fn file_modified_since_local(path: &Path, since: DateTime) -> tokio::io::Result { tokio::fs::metadata(path) .await @@ -506,3 +511,25 @@ async fn test_sql_file_read_utf8() -> anyhow::Result<()> { Ok(()) } + +#[actix_web::test] +async fn test_local_file_modification_time() -> anyhow::Result<()> { + let config = crate::app_config::tests::test_config(); + let state = AppState::init(&config).await?; + let fs = FileSystem::init(".", &state.db).await; + let committed_file = || FileAccess::unprivileged("tests/it_works.txt".as_ref()); + + assert!( + fs.modified_since(&state, committed_file()?, DateTime::UNIX_EPOCH) + .await? + ); + assert!( + !fs.modified_since( + &state, + committed_file()?, + Utc::now() + chrono::Duration::hours(1) + ) + .await? + ); + Ok(()) +} diff --git a/tests/errors/is_a_directory.d/contents.txt b/tests/errors/is_a_directory.d/contents.txt new file mode 100644 index 00000000..d3d9f263 --- /dev/null +++ b/tests/errors/is_a_directory.d/contents.txt @@ -0,0 +1 @@ +not directly servable diff --git a/tests/errors/mod.rs b/tests/errors/mod.rs index fc66d22b..a6bfe55a 100644 --- a/tests/errors/mod.rs +++ b/tests/errors/mod.rs @@ -173,3 +173,13 @@ async fn test_default_404_when_request_path_descends_into_file() { assert!(body.contains("The page you were looking for does not exist")); assert!(!body.contains("error")); } + +#[actix_web::test] +async fn test_requesting_a_directory_is_not_found() { + let resp_result = req_path("/tests/errors/is_a_directory.d").await; + let status = match resp_result { + Ok(resp) => resp.status(), + Err(e) => e.as_response_error().status_code(), + }; + assert_eq!(status, StatusCode::NOT_FOUND); +}