Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions crates/axum-utils/src/cookies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. With this change it doesn't seem to allow auth when element-web (and mas login) are iframed from a different origin.

For example, while developing locally, I have a Vue app at http://localhost:3000. I have element-web running locally via docker at https://foo.bar.

When I access the chat component in my Vue app (where element-web is iframed) I can see the login screen etc jst fine. However, when I POST to the login endpoint, I get a 500 response as the cookie is not honoured with this change in place.

curl 'https://auth.foo.bar/login?kind=continue_authorization_grant&id=01JDSK1J6KSAZER4X4PJG6KKKM' \ -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7' \ -H 'Accept-Language: en-GB,en-US;q=0.9,en;q=0.8' \ -H 'Cache-Control: no-cache' \ -H 'Connection: keep-alive' \ -H 'Content-Type: application/x-www-form-urlencoded' \ -H 'Origin: https://auth.foo.bar' \ -H 'Pragma: no-cache' \ -H 'Referer: https://auth.foo.bar/login?kind=continue_authorization_grant&id=01JDSK1J6KSAZER4X4PJG6KKKM' \ -H 'Sec-Fetch-Dest: iframe' \ -H 'Sec-Fetch-Mode: navigate' \ -H 'Sec-Fetch-Site: same-origin' \ -H 'Sec-Fetch-User: ?1' \ -H 'Upgrade-Insecure-Requests: 1' \ -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36' \ -H 'sec-ch-ua: "Google Chrome";v="131", "Chromium";v="131", "Not_A Brand";v="24"' \ -H 'sec-ch-ua-mobile: ?0' \ -H 'sec-ch-ua-platform: "macOS"' \ --data-raw 'csrf=vrgo4jxAFbaTvA-OKXjX2pMNCFt_rCtC2SCf3G04Yow&username=simon&password=xxxxxxxx'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you happen to know of any workaround?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a temporary change in this PR, which ended up getting reverted. Is the flow working as intended on a known-good client? You should try with:

I'd suggest opening a discussion or joining #matrix-auth:matrix.org for further help on troubleshooting

cookie
}
}
Expand Down
4 changes: 3 additions & 1 deletion crates/handlers/src/oauth2/authorization/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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())
}
Expand Down
2 changes: 1 addition & 1 deletion crates/handlers/src/oauth2/authorization/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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((
Expand Down
23 changes: 19 additions & 4 deletions crates/handlers/src/oauth2/authorization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -180,6 +181,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::RequestNotSupported),
)
.await?);
Expand All @@ -189,6 +191,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::RequestUriNotSupported),
)
.await?);
Expand All @@ -200,6 +203,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::UnsupportedResponseType),
)
.await?);
Expand All @@ -211,6 +215,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::UnauthorizedClient),
)
.await?);
Expand All @@ -220,6 +225,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::RegistrationNotSupported),
)
.await?);
Expand All @@ -230,6 +236,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::LoginRequired),
)
.await?);
Expand All @@ -241,6 +248,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::UnauthorizedClient),
)
.await?);
Expand All @@ -266,6 +274,7 @@ pub(crate) async fn get(
return Ok(callback_destination
.go(
&templates,
&locale,
ClientError::from(ClientErrorCode::InvalidRequest),
)
.await?);
Expand Down Expand Up @@ -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?
Expand All @@ -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)) => {
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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?
}
};
Expand Down
38 changes: 28 additions & 10 deletions crates/handlers/src/upstream_oauth2/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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;

Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)?;
Expand Down Expand Up @@ -327,5 +344,6 @@ pub(crate) async fn handler(
Ok((
cookie_jar,
url_builder.redirect(&mas_router::UpstreamOAuth2Link::new(link.id)),
))
)
.into_response())
}
5 changes: 5 additions & 0 deletions crates/handlers/src/upstream_oauth2/cookie.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 26 additions & 5 deletions crates/templates/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/templates/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
Loading
Loading