From ec883e15efdacaa5fbb3c6961c3e052ad928b950 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 21 Nov 2024 14:30:08 +0100 Subject: [PATCH] Avoid using SameSite=None by re-submitting incoming form data --- crates/axum-utils/src/cookies.rs | 10 +---- .../src/oauth2/authorization/callback.rs | 4 +- .../src/oauth2/authorization/complete.rs | 2 +- .../handlers/src/oauth2/authorization/mod.rs | 23 +++++++++-- .../handlers/src/upstream_oauth2/callback.rs | 38 +++++++++++++----- crates/handlers/src/upstream_oauth2/cookie.rs | 5 +++ crates/templates/src/context.rs | 31 ++++++++++++--- crates/templates/src/lib.rs | 2 +- templates/form_post.html | 39 ++++++++++++------- translations/en.json | 6 ++- 10 files changed, 113 insertions(+), 47 deletions(-) diff --git a/crates/axum-utils/src/cookies.rs b/crates/axum-utils/src/cookies.rs index ecfcf319a..9371d5422 100644 --- a/crates/axum-utils/src/cookies.rs +++ b/crates/axum-utils/src/cookies.rs @@ -101,15 +101,7 @@ impl CookieOption { cookie.set_http_only(true); cookie.set_secure(self.secure()); cookie.set_path(self.path().to_owned()); - - // The `form_post` callback requires that, as it means a 3rd party origin will - // POST to MAS. This is presumably fine, as our forms are protected with a CSRF - // token - cookie.set_same_site(if self.secure() { - SameSite::None - } else { - SameSite::Lax - }); + cookie.set_same_site(SameSite::Lax); cookie } } diff --git a/crates/handlers/src/oauth2/authorization/callback.rs b/crates/handlers/src/oauth2/authorization/callback.rs index ba35e3c52..b76722d5a 100644 --- a/crates/handlers/src/oauth2/authorization/callback.rs +++ b/crates/handlers/src/oauth2/authorization/callback.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use axum::response::{Html, IntoResponse, Redirect, Response}; use mas_data_model::AuthorizationGrant; +use mas_i18n::DataLocale; use mas_templates::{FormPostContext, Templates}; use oauth2_types::requests::ResponseMode; use serde::Serialize; @@ -103,6 +104,7 @@ impl CallbackDestination { pub async fn go( self, templates: &Templates, + locale: &DataLocale, params: T, ) -> Result { #[derive(Serialize)] @@ -155,7 +157,7 @@ impl CallbackDestination { state, params, }; - let ctx = FormPostContext::new(redirect_uri, merged); + let ctx = FormPostContext::new_for_url(redirect_uri, merged).with_language(locale); let rendered = templates.render_form_post(&ctx)?; Ok(Html(rendered).into_response()) } diff --git a/crates/handlers/src/oauth2/authorization/complete.rs b/crates/handlers/src/oauth2/authorization/complete.rs index 3eedf71e6..a9efb2ae2 100644 --- a/crates/handlers/src/oauth2/authorization/complete.rs +++ b/crates/handlers/src/oauth2/authorization/complete.rs @@ -141,7 +141,7 @@ pub(crate) async fn get( .await { Ok(params) => { - let res = callback_destination.go(&templates, params).await?; + let res = callback_destination.go(&templates, &locale, params).await?; Ok((cookie_jar, res).into_response()) } Err(GrantCompletionError::RequiresReauth) => Ok(( diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index 236e8a795..f56f06133 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -170,6 +170,7 @@ pub(crate) async fn get( let res: Result = ({ let templates = templates.clone(); let callback_destination = callback_destination.clone(); + let locale = locale.clone(); async move { let maybe_session = session_info.load_session(&mut repo).await?; let prompt = params.auth.prompt.as_deref().unwrap_or_default(); @@ -180,6 +181,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::RequestNotSupported), ) .await?); @@ -189,6 +191,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::RequestUriNotSupported), ) .await?); @@ -200,6 +203,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::UnsupportedResponseType), ) .await?); @@ -211,6 +215,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::UnauthorizedClient), ) .await?); @@ -220,6 +225,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::RegistrationNotSupported), ) .await?); @@ -230,6 +236,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::LoginRequired), ) .await?); @@ -241,6 +248,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::UnauthorizedClient), ) .await?); @@ -266,6 +274,7 @@ pub(crate) async fn get( return Ok(callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::InvalidRequest), ) .await?); @@ -350,11 +359,12 @@ pub(crate) async fn get( ) .await { - Ok(params) => callback_destination.go(&templates, params).await?, + Ok(params) => callback_destination.go(&templates, &locale, params).await?, Err(GrantCompletionError::RequiresConsent) => { callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::ConsentRequired), ) .await? @@ -363,13 +373,14 @@ pub(crate) async fn get( callback_destination .go( &templates, + &locale, ClientError::from(ClientErrorCode::InteractionRequired), ) .await? } Err(GrantCompletionError::PolicyViolation(_grant, _res)) => { callback_destination - .go(&templates, ClientError::from(ClientErrorCode::AccessDenied)) + .go(&templates, &locale, ClientError::from(ClientErrorCode::AccessDenied)) .await? } Err(GrantCompletionError::Internal(e)) => { @@ -400,7 +411,7 @@ pub(crate) async fn get( ) .await { - Ok(params) => callback_destination.go(&templates, params).await?, + Ok(params) => callback_destination.go(&templates, &locale, params).await?, Err(GrantCompletionError::RequiresConsent) => { url_builder.redirect(&mas_router::Consent(grant_id)).into_response() } @@ -440,7 +451,11 @@ pub(crate) async fn get( Err(err) => { tracing::error!(%err); callback_destination - .go(&templates, ClientError::from(ClientErrorCode::ServerError)) + .go( + &templates, + &locale, + ClientError::from(ClientErrorCode::ServerError), + ) .await? } }; diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index a06e80b7c..927874a3f 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -6,9 +6,10 @@ use axum::{ extract::{Path, Query, State}, - response::IntoResponse, + response::{IntoResponse, Response}, Form, }; +use axum_extra::response::Html; use hyper::StatusCode; use mas_axum_utils::{cookies::CookieJar, sentry::SentryEventID}; use mas_data_model::{UpstreamOAuthProvider, UpstreamOAuthProviderResponseMode}; @@ -24,8 +25,9 @@ use mas_storage::{ }, BoxClock, BoxRepository, BoxRng, Clock, }; +use mas_templates::{FormPostContext, Templates}; use oauth2_types::errors::ClientErrorCode; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use thiserror::Error; use ulid::Ulid; @@ -35,9 +37,9 @@ use super::{ template::{environment, AttributeMappingContext}, UpstreamSessionsCookie, }; -use crate::{impl_from_error_for_route, upstream_oauth2::cache::MetadataCache}; +use crate::{impl_from_error_for_route, upstream_oauth2::cache::MetadataCache, PreferredLanguage}; -#[derive(Deserialize)] +#[derive(Serialize, Deserialize)] pub struct Params { state: String, @@ -45,7 +47,7 @@ pub struct Params { code_or_error: CodeOrError, } -#[derive(Deserialize)] +#[derive(Serialize, Deserialize)] #[serde(untagged)] enum CodeOrError { Code { @@ -115,6 +117,7 @@ pub(crate) enum RouteError { Internal(Box), } +impl_from_error_for_route!(mas_templates::TemplateError); impl_from_error_for_route!(mas_storage::RepositoryError); impl_from_error_for_route!(mas_oidc_client::error::DiscoveryError); impl_from_error_for_route!(mas_oidc_client::error::JwksError); @@ -152,11 +155,13 @@ pub(crate) async fn handler( State(encrypter): State, State(keystore): State, State(client): State, + State(templates): State, + PreferredLanguage(locale): PreferredLanguage, cookie_jar: CookieJar, Path(provider_id): Path, query_params: Option>, form_params: Option>, -) -> Result { +) -> Result { let provider = repo .upstream_oauth_provider() .lookup(provider_id) @@ -164,11 +169,24 @@ pub(crate) async fn handler( .filter(UpstreamOAuthProvider::enabled) .ok_or(RouteError::ProviderNotFound)?; + let sessions_cookie = UpstreamSessionsCookie::load(&cookie_jar); + // Read the parameters from the query or the form, depending on what // response_mode the provider uses let params = match (provider.response_mode, query_params, form_params) { - (UpstreamOAuthProviderResponseMode::Query, Some(query_params), None) => query_params.0, - (UpstreamOAuthProviderResponseMode::FormPost, None, Some(form_params)) => form_params.0, + (UpstreamOAuthProviderResponseMode::Query, Some(Query(query_params)), None) => query_params, + (UpstreamOAuthProviderResponseMode::FormPost, None, Some(Form(form_params))) => { + // We got there from a cross-site form POST, so we need to render a form with + // the same values, which posts back to the same URL + if sessions_cookie.is_empty() { + let context = + FormPostContext::new_for_current_url(form_params).with_language(&locale); + let html = templates.render_form_post(&context)?; + return Ok(Html(html).into_response()); + } + + form_params + } (UpstreamOAuthProviderResponseMode::Query, None, None) => { return Err(RouteError::MissingQueryParams) } @@ -179,7 +197,6 @@ pub(crate) async fn handler( (expected, _, _) => return Err(RouteError::InvalidParamsMode { expected }), }; - let sessions_cookie = UpstreamSessionsCookie::load(&cookie_jar); let (session_id, _post_auth_action) = sessions_cookie .find_session(provider_id, ¶ms.state) .map_err(|_| RouteError::MissingCookie)?; @@ -327,5 +344,6 @@ pub(crate) async fn handler( Ok(( cookie_jar, url_builder.redirect(&mas_router::UpstreamOAuth2Link::new(link.id)), - )) + ) + .into_response()) } diff --git a/crates/handlers/src/upstream_oauth2/cookie.rs b/crates/handlers/src/upstream_oauth2/cookie.rs index 6a9769865..cbcfb5148 100644 --- a/crates/handlers/src/upstream_oauth2/cookie.rs +++ b/crates/handlers/src/upstream_oauth2/cookie.rs @@ -61,6 +61,11 @@ impl UpstreamSessions { } } + /// Returns true if the cookie is empty + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + /// Save the upstreams sessions to the cookie jar pub fn save(self, cookie_jar: CookieJar, clock: &C) -> CookieJar where diff --git a/crates/templates/src/context.rs b/crates/templates/src/context.rs index 762930b64..3f384b303 100644 --- a/crates/templates/src/context.rs +++ b/crates/templates/src/context.rs @@ -1460,7 +1460,7 @@ impl TemplateContext for DeviceConsentContext { /// Context used by the `form_post.html` template #[derive(Serialize)] pub struct FormPostContext { - redirect_uri: Url, + redirect_uri: Option, params: T, } @@ -1473,7 +1473,7 @@ impl TemplateContext for FormPostContext { sample_params .into_iter() .map(|params| FormPostContext { - redirect_uri: "https://example.com/callback".parse().unwrap(), + redirect_uri: "https://example.com/callback".parse().ok(), params, }) .collect() @@ -1481,13 +1481,34 @@ impl TemplateContext for FormPostContext { } impl FormPostContext { - /// Constructs a context for the `form_post` response mode form - pub fn new(redirect_uri: Url, params: T) -> Self { + /// Constructs a context for the `form_post` response mode form for a given + /// URL + pub fn new_for_url(redirect_uri: Url, params: T) -> Self { Self { - redirect_uri, + redirect_uri: Some(redirect_uri), params, } } + + /// Constructs a context for the `form_post` response mode form for the + /// current URL + pub fn new_for_current_url(params: T) -> Self { + Self { + redirect_uri: None, + params, + } + } + + /// Add the language to the context + /// + /// This is usually implemented by the [`TemplateContext`] trait, but it is + /// annoying to make it work because of the generic parameter + pub fn with_language(self, lang: &DataLocale) -> WithLanguage { + WithLanguage { + lang: lang.to_string(), + inner: self, + } + } } /// Context used by the `error.html` template diff --git a/crates/templates/src/lib.rs b/crates/templates/src/lib.rs index 7d668a5ee..9314d4e33 100644 --- a/crates/templates/src/lib.rs +++ b/crates/templates/src/lib.rs @@ -368,7 +368,7 @@ register_templates! { pub fn render_reauth(WithLanguage>>) { "pages/reauth.html" } /// Render the form used by the form_post response mode - pub fn render_form_post(FormPostContext) { "form_post.html" } + pub fn render_form_post(WithLanguage>) { "form_post.html" } /// Render the HTML error page pub fn render_error(ErrorContext) { "pages/error.html" } diff --git a/templates/form_post.html b/templates/form_post.html index 87d4e1050..2fb62ead9 100644 --- a/templates/form_post.html +++ b/templates/form_post.html @@ -6,18 +6,27 @@ SPDX-License-Identifier: AGPL-3.0-only Please see LICENSE in the repository root for full details. -#} - - - - - Redirecting to client - - - -
- {% for key, value in params|items %} - - {% endfor %} -
- - +{% extends "base.html" %} + +{% block content %} +
+
+

{{ _("common.loading") }}

+
+
+ +
+ {% for key, value in params|items %} + + {% endfor %} + + +
+ + {# Submit the form in JavaScript on the next tick, so that if the browser + wants to display the placeholder instead of a blank page, it can #} + +{% endblock %} diff --git a/translations/en.json b/translations/en.json index 9b1c11a58..811aa9726 100644 --- a/translations/en.json +++ b/translations/en.json @@ -10,7 +10,7 @@ }, "continue": "Continue", "@continue": { - "context": "pages/account/emails/add.html:37:26-46, pages/account/emails/verify.html:52:26-46, pages/consent.html:55:28-48, pages/device_consent.html:121:13-33, pages/device_link.html:40:26-46, pages/login.html:58:30-50, pages/reauth.html:32:28-48, pages/recovery/start.html:38:26-46, pages/register.html:76:28-48, pages/sso.html:37:28-48" + "context": "form_post.html:25:28-48, pages/account/emails/add.html:37:26-46, pages/account/emails/verify.html:52:26-46, pages/consent.html:55:28-48, pages/device_consent.html:121:13-33, pages/device_link.html:40:26-46, pages/login.html:58:30-50, pages/reauth.html:32:28-48, pages/recovery/start.html:38:26-46, pages/register.html:76:28-48, pages/sso.html:37:28-48" }, "create_account": "Create Account", "@create_account": { @@ -77,6 +77,10 @@ "@email_address": { "context": "pages/account/emails/add.html:33:33-58, pages/recovery/start.html:34:33-58, pages/register.html:40:35-60, pages/upstream_oauth2/do_register.html:79:37-62" }, + "loading": "Loading…", + "@loading": { + "context": "form_post.html:14:27-46" + }, "mxid": "Matrix ID", "@mxid": { "context": "pages/upstream_oauth2/do_register.html:58:35-51"