Avoid using SameSite=None by re-submitting incoming form data

sandhose · sandhose · commit 04e6960339b8 · 2024-11-21T14:35:07.000+01:00
diff --git 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
@@ -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<T: Serialize + Send + Sync>(
         self,
         templates: &Templates,
+        locale: &DataLocale,
         params: T,
     ) -> Result<Response, CallbackDestinationError> {
         #[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
@@ -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
@@ -170,6 +170,7 @@ pub(crate) async fn get(
     let res: Result<Response, RouteError> = ({
         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
@@ -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,17 +37,17 @@ 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,
 
     #[serde(flatten)]
     code_or_error: CodeOrError,
 }
 
-#[derive(Deserialize)]
+#[derive(Serialize, Deserialize)]
 #[serde(untagged)]
 enum CodeOrError {
     Code {
@@ -115,6 +117,7 @@ pub(crate) enum RouteError {
     Internal(Box<dyn std::error::Error>),
 }
 
+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,23 +155,38 @@ pub(crate) async fn handler(
     State(encrypter): State<Encrypter>,
     State(keystore): State<Keystore>,
     State(client): State<reqwest::Client>,
+    State(templates): State<Templates>,
+    PreferredLanguage(locale): PreferredLanguage,
     cookie_jar: CookieJar,
     Path(provider_id): Path<Ulid>,
     query_params: Option<Query<Params>>,
     form_params: Option<Form<Params>>,
-) -> Result<impl IntoResponse, RouteError> {
+) -> Result<Response, RouteError> {
     let provider = repo
         .upstream_oauth_provider()
         .lookup(provider_id)
         .await?
         .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, &params.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
@@ -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<C>(self, cookie_jar: CookieJar, clock: &C) -> CookieJar
     where
diff --git 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<T> {
-    redirect_uri: Url,
+    redirect_uri: Option<Url>,
     params: T,
 }
 
@@ -1473,21 +1473,42 @@ impl<T: TemplateContext> TemplateContext for FormPostContext<T> {
         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()
     }
 }
 
 impl<T> FormPostContext<T> {
-    /// 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<Self> {
+        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
@@ -368,7 +368,7 @@ register_templates! {
     pub fn render_reauth(WithLanguage<WithCsrf<WithSession<ReauthContext>>>) { "pages/reauth.html" }
 
     /// Render the form used by the form_post response mode
-    pub fn render_form_post<T: Serialize>(FormPostContext<T>) { "form_post.html" }
+    pub fn render_form_post<T: Serialize>(WithLanguage<FormPostContext<T>>) { "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
diff --git a/translations/en.json b/translations/en.json

Original file line number	Diff line number	Diff line change
`@@ -141,7 +141,7 @@ pub(crate) async fn get(`
`141`	`141`	`.await`
`142`	`142`	`{`
`143`	`143`	`Ok(params) => {`
`144`		`- let res = callback_destination.go(&templates, params).await?;`
	`144`	`+ let res = callback_destination.go(&templates, &locale, params).await?;`
`145`	`145`	`Ok((cookie_jar, res).into_response())`
`146`	`146`	`}`
`147`	`147`	`Err(GrantCompletionError::RequiresReauth) => Ok((`
Original file line number	Diff line number	Diff line change
`@@ -61,6 +61,11 @@ impl UpstreamSessions {`
`61`	`61`	`}`
`62`	`62`	`}`
`63`	`63`
	`64`	`+ /// Returns true if the cookie is empty`
	`65`	`+ pub fn is_empty(&self) -> bool {`
	`66`	`+ self.0.is_empty()`
	`67`	`+ }`
	`68`	`+`
`64`	`69`	`/// Save the upstreams sessions to the cookie jar`
`65`	`70`	`pub fn save<C>(self, cookie_jar: CookieJar, clock: &C) -> CookieJar`
`66`	`71`	`where`