From fa7dbedd5d878305adbae1c20cd46207f8d45261 Mon Sep 17 00:00:00 2001 From: Stefan Melmuk Date: Sun, 27 Nov 2022 00:00:01 +0100 Subject: [PATCH] redirect to admin login page when forward fails currently, if the admin guard fails the user will get a 404 page. and when the session times out after 20 minutes post methods will give the reason "undefined" as a response while generating the support string will fail without any user feedback. this commit changes the error handling on admin pages * by removing the reliance on Rockets forwarding and making the login page an explicit route that can be redirected to from all admin pages * by removing the obsolete and mostly unused Referer struct we can redirect the user back to the requested admin page directley * by providing an error message for json requests the `get_diagnostics_config` and all post methods can return a more comprehensible message and the user can be alerted * the `admin_url()` function can be simplified because rfc2616 has been obsoleted by rfc7231 in 2014 (and also by the recently released rfc9110) which allows relative urls in the Location header. c.f. https://www.rfc-editor.org/rfc/rfc7231#section-7.1.2 and https://www.rfc-editor.org/rfc/rfc9110#section-10.2.2 --- src/api/admin.rs | 100 ++++++++++----------- src/api/mod.rs | 1 + src/main.rs | 1 + src/static/templates/admin/diagnostics.hbs | 8 +- 4 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 30ffdec2..74a1eeac 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -6,10 +6,10 @@ use std::env; use rocket::serde::json::Json; use rocket::{ form::Form, - http::{Cookie, CookieJar, SameSite, Status}, - request::{self, FromRequest, Outcome, Request}, + http::{Cookie, CookieJar, MediaType, SameSite, Status}, + request::{FromRequest, Outcome, Request}, response::{content::RawHtml as Html, Redirect}, - Route, + Catcher, Route, }; use crate::{ @@ -57,6 +57,23 @@ pub fn routes() -> Vec { ] } +pub fn catchers() -> Vec { + if !CONFIG.disable_admin_token() && !CONFIG.is_admin_token_set() { + catchers![] + } else { + catchers![unauthorized] + } +} + +#[catch(401)] +fn unauthorized(request: &Request<'_>) -> Result { + if request.format() == Some(&MediaType::JSON) { + err_code!("Authorization failed.", Status::Unauthorized.code); + } + let redirect = request.segments::(0..).unwrap_or_default().display().to_string(); + Ok(Redirect::to(admin_redirect_url(&redirect))) +} + static DB_TYPE: Lazy<&str> = Lazy::new(|| { DbConnType::from_url(&CONFIG.database_url()) .map(|t| match t { @@ -85,15 +102,8 @@ fn admin_path() -> String { format!("{}{}", CONFIG.domain_path(), ADMIN_PATH) } -struct Referer(Option); - -#[rocket::async_trait] -impl<'r> FromRequest<'r> for Referer { - type Error = (); - - async fn from_request(request: &'r Request<'_>) -> request::Outcome { - Outcome::Success(Referer(request.headers().get_one("Referer").map(str::to_string))) - } +fn admin_redirect_url(redirect: &str) -> String { + format!("{}/?redirect=/{}", admin_path(), redirect) } #[derive(Debug)] @@ -118,39 +128,20 @@ impl<'r> FromRequest<'r> for IpHeader { } } -/// Used for `Location` response headers, which must specify an absolute URI -/// (see https://tools.ietf.org/html/rfc2616#section-14.30). -fn admin_url(referer: Referer) -> String { - // If we get a referer use that to make it work when, DOMAIN is not set - if let Some(mut referer) = referer.0 { - if let Some(start_index) = referer.find(ADMIN_PATH) { - referer.truncate(start_index + ADMIN_PATH.len()); - return referer; - } - } - - if CONFIG.domain_set() { - // Don't use CONFIG.domain() directly, since the user may want to keep a - // trailing slash there, particularly when running under a subpath. - format!("{}{}{}", CONFIG.domain_origin(), CONFIG.domain_path(), ADMIN_PATH) - } else { - // Last case, when no referer or domain set, technically invalid but better than nothing - ADMIN_PATH.to_string() - } +fn admin_url() -> String { + format!("{}{}", CONFIG.domain_origin(), admin_path()) } #[derive(Responder)] enum AdminResponse { - #[response(status = 200)] - Ok(ApiResult>), #[response(status = 401)] Unauthorized(ApiResult>), #[response(status = 429)] TooManyRequests(ApiResult>), } -#[get("/", rank = 2)] -fn admin_login() -> ApiResult> { +#[get("/?<_redirect..>")] +fn admin_login(_redirect: &str) -> ApiResult> { render_admin_login(None) } @@ -174,18 +165,23 @@ struct LoginForm { token: String, } -#[post("/", data = "")] -fn post_admin_login(data: Form, cookies: &CookieJar<'_>, ip: ClientIp) -> AdminResponse { +#[post("/?", data = "")] +fn post_admin_login( + data: Form, + redirect: &str, + cookies: &CookieJar<'_>, + ip: ClientIp, +) -> Result { let data = data.into_inner(); if crate::ratelimit::check_limit_admin(&ip.ip).is_err() { - return AdminResponse::TooManyRequests(render_admin_login(Some("Too many requests, try again later."))); + return Err(AdminResponse::TooManyRequests(render_admin_login(Some("Too many requests, try again later.")))); } // If the token is invalid, redirect to login page if !_validate_token(&data.token) { error!("Invalid admin token. IP: {}", ip.ip); - AdminResponse::Unauthorized(render_admin_login(Some("Invalid admin token, please try again."))) + Err(AdminResponse::Unauthorized(render_admin_login(Some("Invalid admin token, please try again.")))) } else { // If the token received is valid, generate JWT and save it as a cookie let claims = generate_admin_claims(); @@ -199,7 +195,7 @@ fn post_admin_login(data: Form, cookies: &CookieJar<'_>, ip: ClientIp .finish(); cookies.add(cookie); - AdminResponse::Ok(render_admin_page()) + Ok(Redirect::to(format!("{}{}", admin_path(), redirect))) } } @@ -251,16 +247,12 @@ impl AdminTemplateData { } } -fn render_admin_page() -> ApiResult> { +#[get("/")] +fn admin_page(_token: AdminToken) -> ApiResult> { let text = AdminTemplateData::new().render()?; Ok(Html(text)) } -#[get("/", rank = 1)] -fn admin_page(_token: AdminToken) -> ApiResult> { - render_admin_page() -} - #[derive(Deserialize, Debug)] #[allow(non_snake_case)] struct InviteData { @@ -312,9 +304,9 @@ async fn test_smtp(data: Json, _token: AdminToken) -> EmptyResult { } #[get("/logout")] -fn logout(cookies: &CookieJar<'_>, referer: Referer) -> Redirect { +fn logout(cookies: &CookieJar<'_>) -> Redirect { cookies.remove(Cookie::build(COOKIE_NAME, "").path(admin_path()).finish()); - Redirect::temporary(admin_url(referer)) + Redirect::to(admin_path()) } #[get("/users")] @@ -603,7 +595,7 @@ async fn diagnostics(_token: AdminToken, ip_header: IpHeader, mut conn: DbConn) "uses_proxy": uses_proxy, "db_type": *DB_TYPE, "db_version": get_sql_server_version(&mut conn).await, - "admin_url": format!("{}/diagnostics", admin_url(Referer(None))), + "admin_url": format!("{}/diagnostics", admin_url()), "overrides": &CONFIG.get_overrides().join(", "), "server_time_local": Local::now().format("%Y-%m-%d %H:%M:%S %Z").to_string(), "server_time": Utc::now().format("%Y-%m-%d %H:%M:%S UTC").to_string(), // Run the date/time check as the last item to minimize the difference @@ -645,15 +637,15 @@ pub struct AdminToken {} impl<'r> FromRequest<'r> for AdminToken { type Error = &'static str; - async fn from_request(request: &'r Request<'_>) -> request::Outcome { + async fn from_request(request: &'r Request<'_>) -> Outcome { if CONFIG.disable_admin_token() { - Outcome::Success(AdminToken {}) + Outcome::Success(Self {}) } else { let cookies = request.cookies(); let access_token = match cookies.get(COOKIE_NAME) { Some(cookie) => cookie.value(), - None => return Outcome::Forward(()), // If there is no cookie, redirect to login + None => return Outcome::Failure((Status::Unauthorized, "Unauthorized")), }; let ip = match ClientIp::from_request(request).await { @@ -665,10 +657,10 @@ impl<'r> FromRequest<'r> for AdminToken { // Remove admin cookie cookies.remove(Cookie::build(COOKIE_NAME, "").path(admin_path()).finish()); error!("Invalid or expired admin JWT. IP: {}.", ip); - return Outcome::Forward(()); + return Outcome::Failure((Status::Unauthorized, "Session expired")); } - Outcome::Success(AdminToken {}) + Outcome::Success(Self {}) } } } diff --git a/src/api/mod.rs b/src/api/mod.rs index 49283dd2..9b151300 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -9,6 +9,7 @@ use rocket::serde::json::Json; use serde_json::Value; pub use crate::api::{ + admin::catchers as admin_catchers, admin::routes as admin_routes, core::catchers as core_catchers, core::purge_sends, diff --git a/src/main.rs b/src/main.rs index 83b3b64d..a14eea97 100644 --- a/src/main.rs +++ b/src/main.rs @@ -435,6 +435,7 @@ async fn launch_rocket(pool: db::DbPool, extra_debug: bool) -> Result<(), Error> .mount([basepath, "/notifications"].concat(), api::notifications_routes()) .register([basepath, "/"].concat(), api::web_catchers()) .register([basepath, "/api"].concat(), api::core_catchers()) + .register([basepath, "/admin"].concat(), api::admin_catchers()) .manage(pool) .manage(api::start_notification_server()) .attach(util::AppHeaders()) diff --git a/src/static/templates/admin/diagnostics.hbs b/src/static/templates/admin/diagnostics.hbs index 20d6f371..cb63eb4c 100644 --- a/src/static/templates/admin/diagnostics.hbs +++ b/src/static/templates/admin/diagnostics.hbs @@ -352,7 +352,13 @@ supportString += "* Reverse proxy and version: \n"; supportString += "* Other relevant information: \n"; - let jsonResponse = await fetch('{{urlpath}}/admin/diagnostics/config'); + let jsonResponse = await fetch('{{urlpath}}/admin/diagnostics/config', { + 'headers': { 'Accept': 'application/json' } + }); + if (!jsonResponse.ok) { + alert("Generation failed: " + jsonResponse.statusText); + throw new Error(jsonResponse); + } const configJson = await jsonResponse.json(); supportString += "\n### Config (Generated via diagnostics page)\n
Show Running Config\n" supportString += "\n**Environment settings which are overridden:** {{page_data.overrides}}\n"