diff --git a/crates/cli/src/server.rs b/crates/cli/src/server.rs index 8fb85a703..c61c1090f 100644 --- a/crates/cli/src/server.rs +++ b/crates/cli/src/server.rs @@ -269,7 +269,7 @@ pub fn build_router( } mas_config::HttpResource::OAuth => router.merge(mas_handlers::api_router::()), mas_config::HttpResource::Compat => { - router.merge(mas_handlers::compat_router::()) + router.merge(mas_handlers::compat_router::(templates.clone())) } mas_config::HttpResource::AdminApi => { let (_, api_router) = mas_handlers::admin_api_router::(); diff --git a/crates/handlers/src/compat/login_sso_redirect.rs b/crates/handlers/src/compat/login_sso_redirect.rs index 3662da849..e2d63728d 100644 --- a/crates/handlers/src/compat/login_sso_redirect.rs +++ b/crates/handlers/src/compat/login_sso_redirect.rs @@ -9,7 +9,7 @@ use axum::{ response::IntoResponse, }; use hyper::StatusCode; -use mas_axum_utils::record_error; +use mas_axum_utils::{GenericError, InternalError}; use mas_router::{CompatLoginSsoAction, CompatLoginSsoComplete, UrlBuilder}; use mas_storage::{BoxClock, BoxRepository, BoxRng, compat::CompatSsoLoginRepository}; use rand::distributions::{Alphanumeric, DistString}; @@ -43,12 +43,12 @@ impl_from_error_for_route!(mas_storage::RepositoryError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { - let sentry_event_id = record_error!(self, Self::Internal(_)); - let status_code = match &self { - Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::MissingRedirectUrl | Self::InvalidRedirectUrl => StatusCode::BAD_REQUEST, - }; - (status_code, sentry_event_id, format!("{self}")).into_response() + match self { + Self::Internal(e) => InternalError::new(e).into_response(), + Self::MissingRedirectUrl | Self::InvalidRedirectUrl => { + GenericError::new(StatusCode::BAD_REQUEST, self).into_response() + } + } } } diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index 5cba740a5..7a55bcbe7 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -257,7 +257,7 @@ where } #[allow(clippy::trait_duplication_in_bounds)] -pub fn compat_router() -> Router +pub fn compat_router(templates: Templates) -> Router where S: Clone + Send + Sync + 'static, UrlBuilder: FromRef, @@ -272,7 +272,28 @@ where BoxClock: FromRequestParts, BoxRng: FromRequestParts, { - Router::new() + // A sub-router for human-facing routes with error handling + let human_router = Router::new() + .route( + mas_router::CompatLoginSsoRedirect::route(), + get(self::compat::login_sso_redirect::get), + ) + .route( + mas_router::CompatLoginSsoRedirectIdp::route(), + get(self::compat::login_sso_redirect::get), + ) + .route( + mas_router::CompatLoginSsoRedirectSlash::route(), + get(self::compat::login_sso_redirect::get), + ) + .layer(AndThenLayer::new( + async move |response: axum::response::Response| { + Ok::<_, Infallible>(recover_error(&templates, response)) + }, + )); + + // A sub-router for API-facing routes with CORS + let api_router = Router::new() .route( mas_router::CompatLogin::route(), get(self::compat::login::get).post(self::compat::login::post), @@ -289,18 +310,6 @@ where mas_router::CompatRefresh::route(), post(self::compat::refresh::post), ) - .route( - mas_router::CompatLoginSsoRedirect::route(), - get(self::compat::login_sso_redirect::get), - ) - .route( - mas_router::CompatLoginSsoRedirectIdp::route(), - get(self::compat::login_sso_redirect::get), - ) - .route( - mas_router::CompatLoginSsoRedirectSlash::route(), - get(self::compat::login_sso_redirect::get), - ) .layer( CorsLayer::new() .allow_origin(Any) @@ -314,7 +323,9 @@ where HeaderName::from_static("x-requested-with"), ]) .max_age(Duration::from_secs(60 * 60)), - ) + ); + + Router::new().merge(human_router).merge(api_router) } #[allow(clippy::too_many_lines)] @@ -454,22 +465,29 @@ where ) .layer(AndThenLayer::new( async move |response: axum::response::Response| { - // Error responses should have an ErrorContext attached to them - let ext = response.extensions().get::(); - if let Some(ctx) = ext { - if let Ok(res) = templates.render_error(ctx) { - let (mut parts, _original_body) = response.into_parts(); - parts.headers.remove(CONTENT_TYPE); - parts.headers.remove(CONTENT_LENGTH); - return Ok((parts, Html(res)).into_response()); - } - } - - Ok::<_, Infallible>(response) + Ok::<_, Infallible>(recover_error(&templates, response)) }, )) } +fn recover_error( + templates: &Templates, + response: axum::response::Response, +) -> axum::response::Response { + // Error responses should have an ErrorContext attached to them + let ext = response.extensions().get::(); + if let Some(ctx) = ext { + if let Ok(res) = templates.render_error(ctx) { + let (mut parts, _original_body) = response.into_parts(); + parts.headers.remove(CONTENT_TYPE); + parts.headers.remove(CONTENT_LENGTH); + return (parts, Html(res)).into_response(); + } + } + + response +} + /// The fallback handler for all routes that don't match anything else. /// /// # Errors diff --git a/crates/handlers/src/oauth2/authorization/consent.rs b/crates/handlers/src/oauth2/authorization/consent.rs index 7588a8e99..4a10b96a3 100644 --- a/crates/handlers/src/oauth2/authorization/consent.rs +++ b/crates/handlers/src/oauth2/authorization/consent.rs @@ -11,9 +11,9 @@ use axum::{ use axum_extra::TypedHeader; use hyper::StatusCode; use mas_axum_utils::{ + GenericError, InternalError, cookies::CookieJar, csrf::{CsrfExt, ProtectedForm}, - record_error, }; use mas_data_model::AuthorizationGrantStage; use mas_keystore::Keystore; @@ -64,13 +64,15 @@ impl_from_error_for_route!(super::callback::CallbackDestinationError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { - let sentry_event_id = record_error!(self, Self::Internal(_) | Self::NoSuchClient(_)); - ( - StatusCode::INTERNAL_SERVER_ERROR, - sentry_event_id, - self.to_string(), - ) - .into_response() + match self { + Self::Internal(e) => InternalError::new(e).into_response(), + e @ Self::NoSuchClient(_) => InternalError::new(Box::new(e)).into_response(), + e @ Self::GrantNotFound => GenericError::new(StatusCode::NOT_FOUND, e).into_response(), + e @ Self::GrantNotPending(_) => { + GenericError::new(StatusCode::CONFLICT, e).into_response() + } + e @ Self::Csrf(_) => GenericError::new(StatusCode::BAD_REQUEST, e).into_response(), + } } } diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index a3aa24bdc..339dc0408 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -9,7 +9,7 @@ use axum::{ response::{IntoResponse, Response}, }; use hyper::StatusCode; -use mas_axum_utils::{SessionInfoExt, cookies::CookieJar, record_error}; +use mas_axum_utils::{GenericError, InternalError, SessionInfoExt, cookies::CookieJar}; use mas_data_model::{AuthorizationCode, Pkce}; use mas_router::{PostAuthAction, UrlBuilder}; use mas_storage::{ @@ -53,29 +53,15 @@ pub enum RouteError { impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { - let sentry_event_id = record_error!(self, Self::Internal(_)); - // TODO: better error pages - let response = match self { - RouteError::Internal(e) => { - (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response() + match self { + Self::Internal(e) => InternalError::new(e).into_response(), + e @ (Self::ClientNotFound + | Self::InvalidResponseMode + | Self::IntoCallbackDestination(_) + | Self::UnknownRedirectUri(_)) => { + GenericError::new(StatusCode::BAD_REQUEST, e).into_response() } - RouteError::ClientNotFound => { - (StatusCode::BAD_REQUEST, "could not find client").into_response() - } - RouteError::InvalidResponseMode => { - (StatusCode::BAD_REQUEST, "invalid response mode").into_response() - } - RouteError::IntoCallbackDestination(e) => { - (StatusCode::BAD_REQUEST, e.to_string()).into_response() - } - RouteError::UnknownRedirectUri(e) => ( - StatusCode::BAD_REQUEST, - format!("Invalid redirect URI ({e})"), - ) - .into_response(), - }; - - (sentry_event_id, response).into_response() + } } } diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index 89159a6da..7927c1431 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -319,7 +319,7 @@ impl TestState { let app = crate::healthcheck_router() .merge(crate::discovery_router()) .merge(crate::api_router()) - .merge(crate::compat_router()) + .merge(crate::compat_router(self.templates.clone())) .merge(crate::human_router(self.templates.clone())) // We enable undocumented_oauth2_access for the tests, as it is easier to query the API // with it diff --git a/crates/handlers/src/upstream_oauth2/authorize.rs b/crates/handlers/src/upstream_oauth2/authorize.rs index 4168fc49e..016dd36d3 100644 --- a/crates/handlers/src/upstream_oauth2/authorize.rs +++ b/crates/handlers/src/upstream_oauth2/authorize.rs @@ -9,7 +9,7 @@ use axum::{ response::{IntoResponse, Redirect}, }; use hyper::StatusCode; -use mas_axum_utils::{cookies::CookieJar, record_error}; +use mas_axum_utils::{GenericError, InternalError, cookies::CookieJar}; use mas_data_model::UpstreamOAuthProvider; use mas_oidc_client::requests::authorization_code::AuthorizationRequestData; use mas_router::{PostAuthAction, UrlBuilder}; @@ -41,13 +41,12 @@ impl_from_error_for_route!(mas_storage::RepositoryError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { - let sentry_event_id = record_error!(self, Self::Internal(_)); - let response = match self { - Self::ProviderNotFound => (StatusCode::NOT_FOUND, "Provider not found").into_response(), - Self::Internal(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(), - }; - - (sentry_event_id, response).into_response() + match self { + e @ Self::ProviderNotFound => { + GenericError::new(StatusCode::NOT_FOUND, e).into_response() + } + Self::Internal(e) => InternalError::new(e).into_response(), + } } } diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index e368c9b72..f6ffe1e7a 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -13,7 +13,7 @@ use axum::{ response::{Html, IntoResponse, Response}, }; use hyper::StatusCode; -use mas_axum_utils::{cookies::CookieJar, record_error}; +use mas_axum_utils::{GenericError, InternalError, cookies::CookieJar}; use mas_data_model::{UpstreamOAuthProvider, UpstreamOAuthProviderResponseMode}; use mas_jose::claims::TokenHash; use mas_keystore::{Encrypter, Keystore}; @@ -153,15 +153,13 @@ impl_from_error_for_route!(super::cookie::UpstreamSessionNotFound); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { - let sentry_event_id = record_error!(self, Self::Internal(_)); - let response = match self { - Self::ProviderNotFound => (StatusCode::NOT_FOUND, "Provider not found").into_response(), - Self::SessionNotFound => (StatusCode::NOT_FOUND, "Session not found").into_response(), - Self::Internal(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(), - e => (StatusCode::BAD_REQUEST, e.to_string()).into_response(), - }; - - (sentry_event_id, response).into_response() + match self { + Self::Internal(e) => InternalError::new(e).into_response(), + e @ (Self::ProviderNotFound | Self::SessionNotFound) => { + GenericError::new(StatusCode::NOT_FOUND, e).into_response() + } + e => GenericError::new(StatusCode::BAD_REQUEST, e).into_response(), + } } } diff --git a/crates/oauth2-types/src/oidc.rs b/crates/oauth2-types/src/oidc.rs index 25518ab75..a9befbea1 100644 --- a/crates/oauth2-types/src/oidc.rs +++ b/crates/oauth2-types/src/oidc.rs @@ -647,7 +647,10 @@ impl ProviderMetadata { let metadata = self.insecure_verify_metadata()?; if metadata.issuer() != issuer { - return Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch); + return Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch { + expected: issuer.to_owned(), + actual: metadata.issuer().to_owned(), + }); } validate_url( @@ -1064,8 +1067,13 @@ pub enum ProviderMetadataVerificationError { UrlWithFragment(&'static str, Url), /// The issuer URL doesn't match the one that was discovered. - #[error("issuer URLs don't match")] - IssuerUrlsDontMatch, + #[error("issuer URLs don't match: expected {expected:?}, got {actual:?}")] + IssuerUrlsDontMatch { + /// The expected issuer URL. + expected: String, + /// The issuer URL that was discovered. + actual: String, + }, /// `openid` is missing from the supported scopes. #[error("missing openid scope")] @@ -1314,7 +1322,7 @@ mod tests { metadata.issuer = Some("https://example.com/".to_owned()); assert_matches!( metadata.clone().validate(&issuer), - Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch) + Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch { .. }) ); // Err - Not https