diff --git a/.claude/rules/backend.md b/.claude/rules/backend.md index b4933b6..4cb34d6 100644 --- a/.claude/rules/backend.md +++ b/.claude/rules/backend.md @@ -28,7 +28,10 @@ paths: ## Authorization - Every server fn that reads or changes non-public data starts with `perm_check!(is_logged_in)` or `perm_check!(is_admin)`; use `user_check!(login)` when a non-admin may only touch their own record. The macros return early with 403 and `ApiResponse::Error`, so they only work in functions returning `Result, _>`. -- The auth middleware protects only `/admin*` page requests. `/api/*` is not covered by it — a server fn without `perm_check!` is public. Only the public booking form endpoints should be. +- The auth middleware protects only `/admin*` page requests. `/api/*` is not covered by it — a server fn without `perm_check!` is public, whether or not any page calls it. Only the endpoints of the public booking form and of the customer's booking page should be. +- A server fn that does not return `ApiResponse` can't use the macros; it starts with `require_login().await?` (`src/backend/user.rs`). +- The test `every_server_fn_checks_permissions_or_is_listed_as_public` in `src/backend/mod.rs` scans the sources and fails for a server fn that neither checks the user nor is on its `PUBLIC_SERVER_FNS` list. Put a function on the list only when it returns nothing about other people — in particular never a booking `uuid`, which is the secret of the customer's booking page. +- Server-side code must not call a permission-checked server fn to get data for a request of a customer (e.g. mail templates when a booking is made); use the helper underneath (`message_for_type`). - Plain Actix handlers must check the session themselves. - Files are uploaded through a multipart server function (`upload_image` in `src/backend/appearance.rs`): `#[server(input = MultipartFormData)]`, called in the browser through `Action::new_local` with a `web_sys::FormData`. The server stores a file under a name it makes itself, checks size and content, and never trusts the name sent by the browser. The multipart codec panics on a request without a boundary; `is_broken_upload` in `src/main.rs` refuses those first — extend it when adding another multipart function. - HTML that comes from a user and is rendered with `inner_html` (the text under the banner) must go through `sanitize_text` in `src/backend/rich_text.rs` before it is stored. Extend its allow-list together with the editor toolbar, never the other way round. diff --git a/src/backend/mail.rs b/src/backend/mail.rs index 4b84af3..e9b5254 100644 --- a/src/backend/mail.rs +++ b/src/backend/mail.rs @@ -253,8 +253,13 @@ cfg_if! { if #[cfg(feature = "ssr")] { } }} +/// A mail template for the mail settings. Code that sends e-mails reads templates with +/// `message_for_type`, it runs for requests without login too. #[server] pub async fn get_message(msg_type: MessageType) -> Result { + use crate::backend::user::require_login; + + require_login().await?; let pool = get_pool().await?; Ok(message_for_type(&msg_type, &pool).await?) diff --git a/src/backend/mod.rs b/src/backend/mod.rs index 9194e0a..d15fe7e 100644 --- a/src/backend/mod.rs +++ b/src/backend/mod.rs @@ -135,4 +135,85 @@ cfg_if!{ Ok(data.mailer().clone()) } } -} \ No newline at end of file +} + +#[cfg(test)] +mod tests { + use std::fs; + use regex::Regex; + + /// Server functions that anybody may call. Everything under `/api` is reachable without + /// login unless the function checks the user itself, so each of these is here on purpose: + /// the public booking form, the customer's page of a booking, and logging in. + /// Before adding a name, make sure the function returns nothing about other people. + const PUBLIC_SERVER_FNS: [&str; 18] = [ + // Look of the public page + "get_appearance", + // The customer's own data from their cookie + "get_remembered", + // Data of the booking form + "get_hours", "get_hours_for_day", "get_closing_time", "get_closing_times", + "get_properties", "get_active_properties", "get_public_form_data", "get_pow", + "create_reservation", + // The customer's page of a booking; the uuid from the e-mail is the secret + "get_booking", "cancel_booking", + // Session of the caller + "login", "logout", "auth_check", "admin_check", "get_user", + ]; + + /// Names of the server functions in a source file with the information whether they check the user + fn server_fns(source: &str) -> Vec<(String, bool)> { + let header = Regex::new(r"#\[server[^\]]*\]\s*pub async fn (\w+)").unwrap(); + let headers = header.captures_iter(source) + .map(|c| (c.get(0).unwrap().start(), c[1].to_string())) + .collect::>(); + + headers.iter().enumerate().map(|(index, (start, name))| { + // The body ends where the next server function starts; close enough for a check + let end = headers.get(index + 1).map(|next| next.0).unwrap_or(source.len()); + let body = &source[*start..end]; + let checked = body.contains("perm_check!(") || body.contains("user_check!(") || body.contains("require_login().await?"); + + (name.clone(), checked) + }).collect() + } + + #[test] + fn server_fns_are_recognized() { + let source = "#[server]\npub async fn open() -> X {\n 1\n}\n\n\ + #[server(name = A, prefix = \"/api\", endpoint = \"a\")]\npub async fn closed() -> X {\n perm_check!(is_admin);\n}\n\n\ + #[server]\npub async fn other() -> X {\n require_login().await?;\n}\n"; + + assert_eq!(server_fns(source), vec![ + ("open".to_string(), false), ("closed".to_string(), true), ("other".to_string(), true)]); + } + + /// Fails when a server function neither checks the user nor is listed as public + #[test] + fn every_server_fn_checks_permissions_or_is_listed_as_public() { + let dir = concat!(env!("CARGO_MANIFEST_DIR"), "/src/backend"); + let mut unchecked = vec![]; + let mut found = 0; + + for entry in fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.extension().is_some_and(|e| e == "rs") { + for (name, checked) in server_fns(&fs::read_to_string(&path).unwrap()) { + found += 1; + if !checked { + unchecked.push(name); + } + } + } + } + unchecked.sort(); + + let mut public = PUBLIC_SERVER_FNS.to_vec(); + public.sort(); + + assert!(found > 40, "only {} server functions found, the scan is broken", found); + assert_eq!(unchecked, public, + "server functions without a permission check differ from the list of public ones; \ + add perm_check!/user_check!/require_login to the function, or put it on the list if it is meant to be public"); + } +} diff --git a/src/backend/reservation.rs b/src/backend/reservation.rs index 9b57456..0cbcf74 100644 --- a/src/backend/reservation.rs +++ b/src/backend/reservation.rs @@ -23,7 +23,7 @@ cfg_if! { if #[cfg(feature = "ssr")] { use crate::backend::data::{PublicBookingItem}; use chrono::NaiveDateTime; use crate::backend::mail::MailMessage; - use crate::backend::mail::get_message; + use crate::backend::mail::message_for_type; use crate::error::AppError; use sqlx::PgPool; use crate::backend::user::admin_email; @@ -229,9 +229,15 @@ cfg_if! { if #[cfg(feature = "ssr")] { Ok(()) } + /// Mail template of the given type. Not through the `get_message` server function: that one + /// is for logged-in users, and e-mails are sent for requests of customers too. + async fn template(msg_type: MessageType) -> Result { + Ok(message_for_type(&msg_type, &get_pool().await?).await?) + } + async fn notify_new_all(admin_mail: String, reservation: &ResSumWithItems) -> Result<(), AppError> { let mailing = get_mailing().await?; - let msg = get_message(MessageType::NewReservation).await?; + let msg = template(MessageType::NewReservation).await?; let link = link_for(&reservation.summary.uuid).await; @@ -244,7 +250,7 @@ cfg_if! { if #[cfg(feature = "ssr")] { async fn notify_new(uuid: Uuid) -> Result<(), AppError> { let mailing = get_mailing().await?; - let msg = get_message(MessageType::NewReservationCust).await?; + let msg = template(MessageType::NewReservationCust).await?; let reservation = reservation_by_uuid(uuid).await?; let admin_mail = admin_email().await; @@ -277,12 +283,12 @@ cfg_if! { if #[cfg(feature = "ssr")] { } async fn notify_approve(uuid: Uuid) -> Result<(), AppError> { - send_notify(uuid, get_message(MessageType::ReservationApp).await?, true).await + send_notify(uuid, template(MessageType::ReservationApp).await?, true).await } /// There is nothing left to cancel, so no link is appended async fn notify_cancel(uuid: Uuid) -> Result<(), AppError> { - send_notify(uuid, get_message(MessageType::ReservationCanceled).await?, false).await + send_notify(uuid, template(MessageType::ReservationCanceled).await?, false).await } /// The customer cancelled: a confirmation for them and a message for the staff @@ -290,7 +296,7 @@ cfg_if! { if #[cfg(feature = "ssr")] { notify_cancel(uuid).await?; let mailing = get_mailing().await?; - let msg = get_message(MessageType::CustomerCanceled).await?; + let msg = template(MessageType::CustomerCanceled).await?; let reservation = reservation_by_uuid(uuid).await?; let Some(admin_mail) = admin_email().await else { return Err(AppError::MailSendError("No admin mail".to_string())) @@ -497,15 +503,6 @@ fn num_days(month: u32, year: i32) -> i64 { .num_days() } -#[server] -pub async fn get_reservations_for_month(month: u32, year: i32) -> Result>, ServerFnError> { - let data = reservations_in_range(&NaiveDate::from_ymd_opt(year, month, 1).unwrap(), - &NaiveDate::from_ymd_opt(year, month, num_days(month, year).to_u32().unwrap_or_default()).unwrap(), - None, Page::default()).await?; - - Ok(ApiResponse::Data(data.0)) -} - #[server] pub async fn approve(uuid: String) -> Result, ServerFnError> { use crate::perm_check; @@ -667,6 +664,9 @@ pub async fn year_chart(dark: bool) -> Result, ServerFnError #[server] pub async fn years() -> Result, ServerFnError> { + use crate::backend::user::require_login; + + require_login().await?; let pool = get_pool().await?; let data: Result, Error> = query_as("SELECT DISTINCT date_part('year', date) as year FROM reservation_sum ORDER BY year DESC") @@ -678,6 +678,9 @@ pub async fn years() -> Result, ServerFnError> { #[server] pub async fn months(year: i32) -> Result, ServerFnError> { + use crate::backend::user::require_login; + + require_login().await?; let pool = get_pool().await?; let data: Result, Error> = query_as("SELECT DISTINCT date_part('month', date) as month \ diff --git a/src/backend/user.rs b/src/backend/user.rs index 3ddff18..79d321d 100644 --- a/src/backend/user.rs +++ b/src/backend/user.rs @@ -72,6 +72,21 @@ cfg_if! { if #[cfg(feature = "ssr")] { logged_in_user().await.is_some() } + /// Permission check for server functions that don't return `ApiResponse`, where the + /// `perm_check!` macro can't be used: answers 403 with an error unless a user is logged in. + pub async fn require_login() -> Result<(), ServerFnError> { + if is_logged_in().await { + return Ok(()) + } + + if let Some(response) = use_context::() { + response.set_status(actix_web::http::StatusCode::FORBIDDEN); + } + warn!("Permission denied for a request without login"); + + Err(ServerFnError::new("Forbidden")) + } + pub async fn is_admin() -> bool { if let Some(user) = logged_in_user().await { user.admin