From 095df1b2162412566cb8fb67c55365b7c4059414 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Sat, 4 Jan 2025 10:33:01 +1000 Subject: [PATCH] cookies don't clear unless you set domain (#3332) * make everything cookie consistent * Stricter on expiry * Relearn a painful lesson about needing domains in removal cookies * fix: DRY cookie creation code and reduce the sins --- server/core/src/https/views/cookies.rs | 50 +++++++------- server/core/src/https/views/login.rs | 42 +++++------- server/core/src/https/views/oauth2.rs | 90 ++++++++++++++------------ server/core/src/https/views/reset.rs | 8 +-- 4 files changed, 93 insertions(+), 97 deletions(-) diff --git a/server/core/src/https/views/cookies.rs b/server/core/src/https/views/cookies.rs index 966d0ee65..2f0d0d899 100644 --- a/server/core/src/https/views/cookies.rs +++ b/server/core/src/https/views/cookies.rs @@ -6,11 +6,29 @@ use compact_jwt::{Jws, JwsSigner}; use serde::de::DeserializeOwned; use serde::Serialize; -#[instrument(name = "views::cookies::destroy", level = "debug", skip(jar))] -pub fn destroy(jar: CookieJar, ck_id: &str) -> CookieJar { +fn new_cookie<'a>(state: &'_ ServerState, ck_id: &'a str, value: String) -> Cookie<'a> { + let mut token_cookie = Cookie::new(ck_id, value); + token_cookie.set_secure(state.secure_cookies); + token_cookie.set_same_site(SameSite::Lax); + // Prevent Document.cookie accessing this. Still works with fetch. + token_cookie.set_http_only(true); + // We set a domain here because it allows subdomains + // of the idm to share the cookie. If domain was incorrect + // then webauthn won't work anyway! + token_cookie.set_domain(state.domain.clone()); + token_cookie.set_path("/"); + token_cookie +} + +#[instrument(name = "views::cookies::destroy", level = "debug", skip(jar, state))] +pub fn destroy(jar: CookieJar, ck_id: &str, state: &ServerState) -> CookieJar { if let Some(ck) = jar.get(ck_id) { let mut removal_cookie = ck.clone(); removal_cookie.make_removal(); + + // Need to be set to domain else the cookie isn't removed! + removal_cookie.set_domain(state.domain.clone()); + // Need to be set to / to remove on all parent paths. // If you don't set a path, NOTHING IS REMOVED!!! removal_cookie.set_path("/"); @@ -21,30 +39,14 @@ pub fn destroy(jar: CookieJar, ck_id: &str) -> CookieJar { } } -pub fn make_unsigned<'a>( - state: &'_ ServerState, - ck_id: &'a str, - value: String, - path: &'a str, -) -> Cookie<'a> { - let mut token_cookie = Cookie::new(ck_id, value); - token_cookie.set_secure(state.secure_cookies); - token_cookie.set_same_site(SameSite::Lax); - // Prevent Document.cookie accessing this. Still works with fetch. - token_cookie.set_http_only(true); - // We set a domain here because it allows subdomains - // of the idm to share the cookie. If domain was incorrect - // then webauthn won't work anyway! - token_cookie.set_domain(state.domain.clone()); - token_cookie.set_path(path); - token_cookie +pub fn make_unsigned<'a>(state: &'_ ServerState, ck_id: &'a str, value: String) -> Cookie<'a> { + new_cookie(state, ck_id, value) } pub fn make_signed<'a, T: Serialize>( state: &'_ ServerState, ck_id: &'a str, value: &'_ T, - path: &'a str, ) -> Option> { let kref = &state.jws_signer; @@ -63,13 +65,7 @@ pub fn make_signed<'a, T: Serialize>( }) .ok()?; - let mut token_cookie = Cookie::new(ck_id, token); - token_cookie.set_secure(state.secure_cookies); - token_cookie.set_same_site(SameSite::Lax); - token_cookie.set_http_only(true); - token_cookie.set_path(path); - token_cookie.set_domain(state.domain.clone()); - Some(token_cookie) + Some(new_cookie(state, ck_id, token)) } pub fn get_signed( diff --git a/server/core/src/https/views/login.rs b/server/core/src/https/views/login.rs index ec6b7e38b..e9066e2a6 100644 --- a/server/core/src/https/views/login.rs +++ b/server/core/src/https/views/login.rs @@ -177,10 +177,10 @@ pub async fn view_logout_get( }; // Always clear cookies even on an error. - jar = cookies::destroy(jar, COOKIE_BEARER_TOKEN); - jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); - jar = cookies::destroy(jar, COOKIE_AUTH_SESSION_ID); - jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); + jar = cookies::destroy(jar, COOKIE_BEARER_TOKEN, &state); + jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state); + jar = cookies::destroy(jar, COOKIE_AUTH_SESSION_ID, &state); + jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state); (jar, response).into_response() } @@ -195,7 +195,7 @@ pub async fn view_reauth_get( ) -> Response { // No matter what, we always clear the stored oauth2 cookie to prevent // ui loops - let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); + let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state); let session_valid_result = state .qe_r_ref @@ -322,7 +322,7 @@ pub async fn view_index_get( // No matter what, we always clear the stored oauth2 cookie to prevent // ui loops - let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); + let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state); match session_valid_result { Ok(()) => { @@ -915,14 +915,10 @@ async fn view_login_step( // Update jar let token_str = token.to_string(); - // Important - this can be make unsigned as token_str has it's own + // Important - this can be make unsigned as token_str has its own // signatures. - let mut bearer_cookie = cookies::make_unsigned( - &state, - COOKIE_BEARER_TOKEN, - token_str.clone(), - "/", - ); + let mut bearer_cookie = + cookies::make_unsigned(&state, COOKIE_BEARER_TOKEN, token_str.clone()); // Important - can be permanent as the token has its own expiration time internally bearer_cookie.make_permanent(); @@ -933,7 +929,6 @@ async fn view_login_step( &state, COOKIE_USERNAME, session_context.username.clone(), - Urls::Login.as_ref(), ); username_cookie.make_permanent(); jar.add(username_cookie) @@ -980,16 +975,11 @@ fn add_session_cookie( jar: CookieJar, session_context: &SessionContext, ) -> Result { - cookies::make_signed( - state, - COOKIE_AUTH_SESSION_ID, - session_context, - Urls::Login.as_ref(), - ) - .map(|mut cookie| { - // Not needed when redirecting into this site - cookie.set_same_site(SameSite::Strict); - jar.add(cookie) - }) - .ok_or(OperationError::InvalidSessionState) + cookies::make_signed(state, COOKIE_AUTH_SESSION_ID, session_context) + .map(|mut cookie| { + // Not needed when redirecting into this site + cookie.set_same_site(SameSite::Strict); + jar.add(cookie) + }) + .ok_or(OperationError::InvalidSessionState) } diff --git a/server/core/src/https/views/oauth2.rs b/server/core/src/https/views/oauth2.rs index ce0a7de7c..373d47208 100644 --- a/server/core/src/https/views/oauth2.rs +++ b/server/core/src/https/views/oauth2.rs @@ -26,7 +26,6 @@ use axum_extra::extract::cookie::{CookieJar, SameSite}; use axum_htmx::HX_REDIRECT; use serde::Deserialize; -use super::constants::Urls; use super::login::{LoginDisplayCtx, Oauth2Ctx}; use super::{cookies, UnrecoverableErrorView}; @@ -96,7 +95,7 @@ async fn oauth2_auth_req( ) -> Response { // No matter what, we always clear the stored oauth2 cookie to prevent // ui loops - let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); + let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state); // If the auth_req was cross-signed, old, or just bad, error. But we have *cleared* it // from the cookie which means we won't see it again. @@ -149,14 +148,17 @@ async fn oauth2_auth_req( consent_token, }) => { // We can just render the form now, the consent token has everything we need. - ConsentRequestView { - client_name, - // scopes, - pii_scopes, - consent_token, - redirect: None, - } - .into_response() + ( + jar, + ConsentRequestView { + client_name, + // scopes, + pii_scopes, + consent_token, + redirect: None, + }, + ) + .into_response() } Ok(AuthoriseResponse::AuthenticationRequired { @@ -165,20 +167,19 @@ async fn oauth2_auth_req( }) => { // Sign the auth req and hide it in our cookie - we'll come back for // you later. - let maybe_jar = - cookies::make_signed(&state, COOKIE_OAUTH2_REQ, &auth_req, Urls::Ui.as_ref()) - .map(|mut cookie| { - cookie.set_same_site(SameSite::Strict); - // Expire at the end of the session. - cookie.set_expires(None); - // Could experiment with this to a shorter value, but session should be enough. - cookie.set_max_age(None); - jar.add(cookie) - }) - .ok_or(OperationError::InvalidSessionState); + let maybe_jar = cookies::make_signed(&state, COOKIE_OAUTH2_REQ, &auth_req) + .map(|mut cookie| { + cookie.set_same_site(SameSite::Strict); + // Expire at the end of the session. + cookie.set_expires(None); + // Could experiment with this to a shorter value, but session should be enough. + cookie.set_max_age(time::Duration::minutes(15)); + jar.clone().add(cookie) + }) + .ok_or(OperationError::InvalidSessionState); match maybe_jar { - Ok(jar) => { + Ok(new_jar) => { let display_ctx = LoginDisplayCtx { domain_info, oauth2: Some(Oauth2Ctx { client_name }), @@ -186,21 +187,27 @@ async fn oauth2_auth_req( error: None, }; - super::login::view_oauth2_get(jar, display_ctx, login_hint) + super::login::view_oauth2_get(new_jar, display_ctx, login_hint) } - Err(err_code) => UnrecoverableErrorView { - err_code, - operation_id: kopid.eventid, - } - .into_response(), + Err(err_code) => ( + jar, + UnrecoverableErrorView { + err_code, + operation_id: kopid.eventid, + }, + ) + .into_response(), } } Err(Oauth2Error::AccessDenied) => { // If scopes are not available for this account. - AccessDeniedView { - operation_id: kopid.eventid, - } - .into_response() + ( + jar, + AccessDeniedView { + operation_id: kopid.eventid, + }, + ) + .into_response() } /* RFC - If the request fails due to a missing, invalid, or mismatching @@ -219,11 +226,14 @@ async fn oauth2_auth_req( &err_code.to_string() ); - UnrecoverableErrorView { - err_code: OperationError::InvalidState, - operation_id: kopid.eventid, - } - .into_response() + ( + jar, + UnrecoverableErrorView { + err_code: OperationError::InvalidState, + operation_id: kopid.eventid, + }, + ) + .into_response() } } } @@ -237,13 +247,13 @@ pub struct ConsentForm { } pub async fn view_consent_post( - State(state): State, + State(server_state): State, Extension(kopid): Extension, VerifiedClientInformation(client_auth_info): VerifiedClientInformation, jar: CookieJar, Form(consent_form): Form, ) -> Result { - let res = state + let res = server_state .qe_w_ref .handle_oauth2_authorise_permit(client_auth_info, consent_form.consent_token, kopid.eventid) .await; @@ -254,7 +264,7 @@ pub async fn view_consent_post( state, code, }) => { - let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); + let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &server_state); if let Some(redirect) = consent_form.redirect { Ok(( diff --git a/server/core/src/https/views/reset.rs b/server/core/src/https/views/reset.rs index d64b6cdf4..7e6dd55d6 100644 --- a/server/core/src/https/views/reset.rs +++ b/server/core/src/https/views/reset.rs @@ -220,7 +220,7 @@ pub(crate) async fn commit( .await?; // No longer need the cookie jar. - let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); + let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state); Ok((jar, HxLocation::from(Uri::from_static("/ui")), "").into_response()) } @@ -241,7 +241,7 @@ pub(crate) async fn cancel_cred_update( .await?; // No longer need the cookie jar. - let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); + let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state); Ok(( jar, @@ -688,7 +688,7 @@ fn add_cu_cookie( cu_session_token: CUSessionToken, ) -> CookieJar { let mut token_cookie = - cookies::make_unsigned(state, COOKIE_CU_SESSION_TOKEN, cu_session_token.token, "/"); + cookies::make_unsigned(state, COOKIE_CU_SESSION_TOKEN, cu_session_token.token); token_cookie.set_same_site(SameSite::Strict); jar.add(token_cookie) } @@ -788,7 +788,7 @@ pub(crate) async fn view_reset_get( | OperationError::InvalidState, ) => { // If our previous credential update session expired we want to see the reset form again. - jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); + jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state); if let Some(token) = params.token { let token_uri_string = format!("{}?token={}", Urls::CredReset, token);