Skip to content

Commit 3a8d4dc

Browse files
sandhosesandhose
authored
Show a better error page in most human-facing pages (#4816)
2 parents c7d0828 + 0b583b6 commit 3a8d4dc

File tree

9 files changed

+100
-89
lines changed

9 files changed

+100
-89
lines changed

‎crates/cli/src/server.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ pub fn build_router(
269269
}
270270
mas_config::HttpResource::OAuth => router.merge(mas_handlers::api_router::<AppState>()),
271271
mas_config::HttpResource::Compat => {
272-
router.merge(mas_handlers::compat_router::<AppState>())
272+
router.merge(mas_handlers::compat_router::<AppState>(templates.clone()))
273273
}
274274
mas_config::HttpResource::AdminApi => {
275275
let (_, api_router) = mas_handlers::admin_api_router::<AppState>();

‎crates/handlers/src/compat/login_sso_redirect.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use axum::{
99
response::IntoResponse,
1010
};
1111
use hyper::StatusCode;
12-
use mas_axum_utils::record_error;
12+
use mas_axum_utils::{GenericError, InternalError};
1313
use mas_router::{CompatLoginSsoAction, CompatLoginSsoComplete, UrlBuilder};
1414
use mas_storage::{BoxClock, BoxRepository, BoxRng, compat::CompatSsoLoginRepository};
1515
use rand::distributions::{Alphanumeric, DistString};
@@ -43,12 +43,12 @@ impl_from_error_for_route!(mas_storage::RepositoryError);
4343

4444
impl IntoResponse for RouteError {
4545
fn into_response(self) -> axum::response::Response {
46-
let sentry_event_id = record_error!(self, Self::Internal(_));
47-
let status_code = match &self {
48-
Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR,
49-
Self::MissingRedirectUrl | Self::InvalidRedirectUrl => StatusCode::BAD_REQUEST,
50-
};
51-
(status_code, sentry_event_id, format!("{self}")).into_response()
46+
match self {
47+
Self::Internal(e) => InternalError::new(e).into_response(),
48+
Self::MissingRedirectUrl | Self::InvalidRedirectUrl => {
49+
GenericError::new(StatusCode::BAD_REQUEST, self).into_response()
50+
}
51+
}
5252
}
5353
}
5454

‎crates/handlers/src/lib.rs‎

Lines changed: 45 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ where
257257
}
258258

259259
#[allow(clippy::trait_duplication_in_bounds)]
260-
pub fn compat_router<S>() -> Router<S>
260+
pub fn compat_router<S>(templates: Templates) -> Router<S>
261261
where
262262
S: Clone + Send + Sync + 'static,
263263
UrlBuilder: FromRef<S>,
@@ -272,7 +272,28 @@ where
272272
BoxClock: FromRequestParts<S>,
273273
BoxRng: FromRequestParts<S>,
274274
{
275-
Router::new()
275+
// A sub-router for human-facing routes with error handling
276+
let human_router = Router::new()
277+
.route(
278+
mas_router::CompatLoginSsoRedirect::route(),
279+
get(self::compat::login_sso_redirect::get),
280+
)
281+
.route(
282+
mas_router::CompatLoginSsoRedirectIdp::route(),
283+
get(self::compat::login_sso_redirect::get),
284+
)
285+
.route(
286+
mas_router::CompatLoginSsoRedirectSlash::route(),
287+
get(self::compat::login_sso_redirect::get),
288+
)
289+
.layer(AndThenLayer::new(
290+
async move |response: axum::response::Response| {
291+
Ok::<_, Infallible>(recover_error(&templates, response))
292+
},
293+
));
294+
295+
// A sub-router for API-facing routes with CORS
296+
let api_router = Router::new()
276297
.route(
277298
mas_router::CompatLogin::route(),
278299
get(self::compat::login::get).post(self::compat::login::post),
@@ -289,18 +310,6 @@ where
289310
mas_router::CompatRefresh::route(),
290311
post(self::compat::refresh::post),
291312
)
292-
.route(
293-
mas_router::CompatLoginSsoRedirect::route(),
294-
get(self::compat::login_sso_redirect::get),
295-
)
296-
.route(
297-
mas_router::CompatLoginSsoRedirectIdp::route(),
298-
get(self::compat::login_sso_redirect::get),
299-
)
300-
.route(
301-
mas_router::CompatLoginSsoRedirectSlash::route(),
302-
get(self::compat::login_sso_redirect::get),
303-
)
304313
.layer(
305314
CorsLayer::new()
306315
.allow_origin(Any)
@@ -314,7 +323,9 @@ where
314323
HeaderName::from_static("x-requested-with"),
315324
])
316325
.max_age(Duration::from_secs(60 * 60)),
317-
)
326+
);
327+
328+
Router::new().merge(human_router).merge(api_router)
318329
}
319330

320331
#[allow(clippy::too_many_lines)]
@@ -454,22 +465,29 @@ where
454465
)
455466
.layer(AndThenLayer::new(
456467
async move |response: axum::response::Response| {
457-
// Error responses should have an ErrorContext attached to them
458-
let ext = response.extensions().get::<ErrorContext>();
459-
if let Some(ctx) = ext {
460-
if let Ok(res) = templates.render_error(ctx) {
461-
let (mut parts, _original_body) = response.into_parts();
462-
parts.headers.remove(CONTENT_TYPE);
463-
parts.headers.remove(CONTENT_LENGTH);
464-
return Ok((parts, Html(res)).into_response());
465-
}
466-
}
467-
468-
Ok::<_, Infallible>(response)
468+
Ok::<_, Infallible>(recover_error(&templates, response))
469469
},
470470
))
471471
}
472472

473+
fn recover_error(
474+
templates: &Templates,
475+
response: axum::response::Response,
476+
) -> axum::response::Response {
477+
// Error responses should have an ErrorContext attached to them
478+
let ext = response.extensions().get::<ErrorContext>();
479+
if let Some(ctx) = ext {
480+
if let Ok(res) = templates.render_error(ctx) {
481+
let (mut parts, _original_body) = response.into_parts();
482+
parts.headers.remove(CONTENT_TYPE);
483+
parts.headers.remove(CONTENT_LENGTH);
484+
return (parts, Html(res)).into_response();
485+
}
486+
}
487+
488+
response
489+
}
490+
473491
/// The fallback handler for all routes that don't match anything else.
474492
///
475493
/// # Errors

‎crates/handlers/src/oauth2/authorization/consent.rs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@ use axum::{
1111
use axum_extra::TypedHeader;
1212
use hyper::StatusCode;
1313
use mas_axum_utils::{
14+
GenericError, InternalError,
1415
cookies::CookieJar,
1516
csrf::{CsrfExt, ProtectedForm},
16-
record_error,
1717
};
1818
use mas_data_model::AuthorizationGrantStage;
1919
use mas_keystore::Keystore;
@@ -64,13 +64,15 @@ impl_from_error_for_route!(super::callback::CallbackDestinationError);
6464

6565
impl IntoResponse for RouteError {
6666
fn into_response(self) -> axum::response::Response {
67-
let sentry_event_id = record_error!(self, Self::Internal(_) | Self::NoSuchClient(_));
68-
(
69-
StatusCode::INTERNAL_SERVER_ERROR,
70-
sentry_event_id,
71-
self.to_string(),
72-
)
73-
.into_response()
67+
match self {
68+
Self::Internal(e) => InternalError::new(e).into_response(),
69+
e @ Self::NoSuchClient(_) => InternalError::new(Box::new(e)).into_response(),
70+
e @ Self::GrantNotFound => GenericError::new(StatusCode::NOT_FOUND, e).into_response(),
71+
e @ Self::GrantNotPending(_) => {
72+
GenericError::new(StatusCode::CONFLICT, e).into_response()
73+
}
74+
e @ Self::Csrf(_) => GenericError::new(StatusCode::BAD_REQUEST, e).into_response(),
75+
}
7476
}
7577
}
7678

‎crates/handlers/src/oauth2/authorization/mod.rs‎

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use axum::{
99
response::{IntoResponse, Response},
1010
};
1111
use hyper::StatusCode;
12-
use mas_axum_utils::{SessionInfoExt, cookies::CookieJar, record_error};
12+
use mas_axum_utils::{GenericError, InternalError, SessionInfoExt, cookies::CookieJar};
1313
use mas_data_model::{AuthorizationCode, Pkce};
1414
use mas_router::{PostAuthAction, UrlBuilder};
1515
use mas_storage::{
@@ -53,29 +53,15 @@ pub enum RouteError {
5353

5454
impl IntoResponse for RouteError {
5555
fn into_response(self) -> axum::response::Response {
56-
let sentry_event_id = record_error!(self, Self::Internal(_));
57-
// TODO: better error pages
58-
let response = match self {
59-
RouteError::Internal(e) => {
60-
(StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response()
56+
match self {
57+
Self::Internal(e) => InternalError::new(e).into_response(),
58+
e @ (Self::ClientNotFound
59+
| Self::InvalidResponseMode
60+
| Self::IntoCallbackDestination(_)
61+
| Self::UnknownRedirectUri(_)) => {
62+
GenericError::new(StatusCode::BAD_REQUEST, e).into_response()
6163
}
62-
RouteError::ClientNotFound => {
63-
(StatusCode::BAD_REQUEST, "could not find client").into_response()
64-
}
65-
RouteError::InvalidResponseMode => {
66-
(StatusCode::BAD_REQUEST, "invalid response mode").into_response()
67-
}
68-
RouteError::IntoCallbackDestination(e) => {
69-
(StatusCode::BAD_REQUEST, e.to_string()).into_response()
70-
}
71-
RouteError::UnknownRedirectUri(e) => (
72-
StatusCode::BAD_REQUEST,
73-
format!("Invalid redirect URI ({e})"),
74-
)
75-
.into_response(),
76-
};
77-
78-
(sentry_event_id, response).into_response()
64+
}
7965
}
8066
}
8167

‎crates/handlers/src/test_utils.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ impl TestState {
319319
let app = crate::healthcheck_router()
320320
.merge(crate::discovery_router())
321321
.merge(crate::api_router())
322-
.merge(crate::compat_router())
322+
.merge(crate::compat_router(self.templates.clone()))
323323
.merge(crate::human_router(self.templates.clone()))
324324
// We enable undocumented_oauth2_access for the tests, as it is easier to query the API
325325
// with it

‎crates/handlers/src/upstream_oauth2/authorize.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use axum::{
99
response::{IntoResponse, Redirect},
1010
};
1111
use hyper::StatusCode;
12-
use mas_axum_utils::{cookies::CookieJar, record_error};
12+
use mas_axum_utils::{GenericError, InternalError, cookies::CookieJar};
1313
use mas_data_model::UpstreamOAuthProvider;
1414
use mas_oidc_client::requests::authorization_code::AuthorizationRequestData;
1515
use mas_router::{PostAuthAction, UrlBuilder};
@@ -41,13 +41,12 @@ impl_from_error_for_route!(mas_storage::RepositoryError);
4141

4242
impl IntoResponse for RouteError {
4343
fn into_response(self) -> axum::response::Response {
44-
let sentry_event_id = record_error!(self, Self::Internal(_));
45-
let response = match self {
46-
Self::ProviderNotFound => (StatusCode::NOT_FOUND, "Provider not found").into_response(),
47-
Self::Internal(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(),
48-
};
49-
50-
(sentry_event_id, response).into_response()
44+
match self {
45+
e @ Self::ProviderNotFound => {
46+
GenericError::new(StatusCode::NOT_FOUND, e).into_response()
47+
}
48+
Self::Internal(e) => InternalError::new(e).into_response(),
49+
}
5150
}
5251
}
5352

‎crates/handlers/src/upstream_oauth2/callback.rs‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use axum::{
1313
response::{Html, IntoResponse, Response},
1414
};
1515
use hyper::StatusCode;
16-
use mas_axum_utils::{cookies::CookieJar, record_error};
16+
use mas_axum_utils::{GenericError, InternalError, cookies::CookieJar};
1717
use mas_data_model::{UpstreamOAuthProvider, UpstreamOAuthProviderResponseMode};
1818
use mas_jose::claims::TokenHash;
1919
use mas_keystore::{Encrypter, Keystore};
@@ -153,15 +153,13 @@ impl_from_error_for_route!(super::cookie::UpstreamSessionNotFound);
153153

154154
impl IntoResponse for RouteError {
155155
fn into_response(self) -> axum::response::Response {
156-
let sentry_event_id = record_error!(self, Self::Internal(_));
157-
let response = match self {
158-
Self::ProviderNotFound => (StatusCode::NOT_FOUND, "Provider not found").into_response(),
159-
Self::SessionNotFound => (StatusCode::NOT_FOUND, "Session not found").into_response(),
160-
Self::Internal(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(),
161-
e => (StatusCode::BAD_REQUEST, e.to_string()).into_response(),
162-
};
163-
164-
(sentry_event_id, response).into_response()
156+
match self {
157+
Self::Internal(e) => InternalError::new(e).into_response(),
158+
e @ (Self::ProviderNotFound | Self::SessionNotFound) => {
159+
GenericError::new(StatusCode::NOT_FOUND, e).into_response()
160+
}
161+
e => GenericError::new(StatusCode::BAD_REQUEST, e).into_response(),
162+
}
165163
}
166164
}
167165

‎crates/oauth2-types/src/oidc.rs‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,10 @@ impl ProviderMetadata {
647647
let metadata = self.insecure_verify_metadata()?;
648648

649649
if metadata.issuer() != issuer {
650-
return Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch);
650+
return Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch {
651+
expected: issuer.to_owned(),
652+
actual: metadata.issuer().to_owned(),
653+
});
651654
}
652655

653656
validate_url(
@@ -1064,8 +1067,13 @@ pub enum ProviderMetadataVerificationError {
10641067
UrlWithFragment(&'static str, Url),
10651068

10661069
/// The issuer URL doesn't match the one that was discovered.
1067-
#[error("issuer URLs don't match")]
1068-
IssuerUrlsDontMatch,
1070+
#[error("issuer URLs don't match: expected {expected:?}, got {actual:?}")]
1071+
IssuerUrlsDontMatch {
1072+
/// The expected issuer URL.
1073+
expected: String,
1074+
/// The issuer URL that was discovered.
1075+
actual: String,
1076+
},
10691077

10701078
/// `openid` is missing from the supported scopes.
10711079
#[error("missing openid scope")]
@@ -1314,7 +1322,7 @@ mod tests {
13141322
metadata.issuer = Some("https://example.com/".to_owned());
13151323
assert_matches!(
13161324
metadata.clone().validate(&issuer),
1317-
Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch)
1325+
Err(ProviderMetadataVerificationError::IssuerUrlsDontMatch { .. })
13181326
);
13191327

13201328
// Err - Not https

0 commit comments

Comments
 (0)