From 94b7285cbb4662678871ca6925adc68b3d8f71ed Mon Sep 17 00:00:00 2001 From: Firstyear Date: Thu, 13 Feb 2025 11:03:15 +1000 Subject: [PATCH] Support redirect uris with query parameters (#3422) RFC 6749 once again reminds us that given the room to do silly things, RFC authors absolutely will. In this case, it's query parameters in redirection uris which are absolutely horrifying and yet, here we are. We strictly match the query pairs during the redirection to ensure that if a query pair did allow open redirection, then we prevent it. --- server/lib/src/idm/oauth2.rs | 122 +++++++++++++++++++++++++++++------ 1 file changed, 103 insertions(+), 19 deletions(-) diff --git a/server/lib/src/idm/oauth2.rs b/server/lib/src/idm/oauth2.rs index 2fb259931..a6cb8e618 100644 --- a/server/lib/src/idm/oauth2.rs +++ b/server/lib/src/idm/oauth2.rs @@ -248,24 +248,32 @@ impl AuthorisePermitSuccess { pub fn build_redirect_uri(&self) -> Url { let mut redirect_uri = self.redirect_uri.clone(); - // Always clear query and fragment, regardless of the response mode - redirect_uri.set_query(None); + // Always clear the fragment per RFC redirect_uri.set_fragment(None); - // We can't set query pairs on fragments, only query. - let mut uri_builder = url::form_urlencoded::Serializer::new(String::new()); - - uri_builder.append_pair("code", &self.code); - - if let Some(state) = self.state.as_ref() { - uri_builder.append_pair("state", state); - }; - - let encoded = uri_builder.finish(); - match self.response_mode { - ResponseMode::Query => redirect_uri.set_query(Some(&encoded)), - ResponseMode::Fragment => redirect_uri.set_fragment(Some(&encoded)), + ResponseMode::Query => { + redirect_uri + .query_pairs_mut() + .append_pair("code", &self.code); + + if let Some(state) = self.state.as_ref() { + redirect_uri.query_pairs_mut().append_pair("state", state); + }; + } + ResponseMode::Fragment => { + redirect_uri.set_query(None); + + // Per [the RFC](https://www.rfc-editor.org/rfc/rfc6749#section-3.1.2), we can't set query pairs on fragment-containing redirects, only query ones. + let mut uri_builder = url::form_urlencoded::Serializer::new(String::new()); + uri_builder.append_pair("code", &self.code); + if let Some(state) = self.state.as_ref() { + uri_builder.append_pair("state", state); + }; + let encoded = uri_builder.finish(); + + redirect_uri.set_fragment(Some(&encoded)) + } } redirect_uri @@ -3020,7 +3028,7 @@ fn check_is_loopback(redirect_uri: &Url) -> bool { #[cfg(test)] mod tests { use base64::{engine::general_purpose, Engine as _}; - use std::collections::BTreeSet; + use std::collections::{BTreeMap, BTreeSet}; use std::convert::TryFrom; use std::str::FromStr; use std::time::Duration; @@ -3144,7 +3152,7 @@ mod tests { ), ( Attribute::OAuth2RsOrigin, - Value::new_url_s("https://portal.example.com").unwrap() + Value::new_url_s("https://portal.example.com/?custom=foo").unwrap() ), ( Attribute::OAuth2RsOrigin, @@ -3680,6 +3688,70 @@ mod tests { == Oauth2Error::InvalidOrigin ); + // * invalid uri (doesn't match query params) + let auth_req = AuthorisationRequest { + response_type: ResponseType::Code, + response_mode: None, + client_id: "test_resource_server".to_string(), + state: Some("123".to_string()), + pkce_request: pkce_request.clone(), + redirect_uri: Url::parse("https://portal.example.com/?custom=foo&too=many").unwrap(), + scope: btreeset![OAUTH2_SCOPE_OPENID.to_string()], + nonce: None, + oidc_ext: Default::default(), + max_age: None, + unknown_keys: Default::default(), + }; + + assert!( + idms_prox_read + .check_oauth2_authorisation(Some(&ident), &auth_req, ct) + .unwrap_err() + == Oauth2Error::InvalidOrigin + ); + + let auth_req = AuthorisationRequest { + response_type: ResponseType::Code, + response_mode: None, + client_id: "test_resource_server".to_string(), + state: Some("123".to_string()), + pkce_request: pkce_request.clone(), + redirect_uri: Url::parse("https://portal.example.com").unwrap(), + scope: btreeset![OAUTH2_SCOPE_OPENID.to_string()], + nonce: None, + oidc_ext: Default::default(), + max_age: None, + unknown_keys: Default::default(), + }; + + assert!( + idms_prox_read + .check_oauth2_authorisation(Some(&ident), &auth_req, ct) + .unwrap_err() + == Oauth2Error::InvalidOrigin + ); + + let auth_req = AuthorisationRequest { + response_type: ResponseType::Code, + response_mode: None, + client_id: "test_resource_server".to_string(), + state: Some("123".to_string()), + pkce_request: pkce_request.clone(), + redirect_uri: Url::parse("https://portal.example.com/?wrong=queryparam").unwrap(), + scope: btreeset![OAUTH2_SCOPE_OPENID.to_string()], + nonce: None, + oidc_ext: Default::default(), + max_age: None, + unknown_keys: Default::default(), + }; + + assert!( + idms_prox_read + .check_oauth2_authorisation(Some(&ident), &auth_req, ct) + .unwrap_err() + == Oauth2Error::InvalidOrigin + ); + // Not Authenticated let auth_req = AuthorisationRequest { response_type: ResponseType::Code, @@ -4041,6 +4113,8 @@ mod tests { // == Setup the authorisation request let (code_verifier, code_challenge) = create_code_verifier!("Whar Garble"); + let redirect_uri = Url::parse("https://portal.example.com/?custom=foo").unwrap(); + let auth_req = AuthorisationRequest { response_type: ResponseType::Code, response_mode: None, @@ -4050,7 +4124,7 @@ mod tests { code_challenge: code_challenge.clone(), code_challenge_method: CodeChallengeMethod::S256, }), - redirect_uri: Url::parse("https://portal.example.com").unwrap(), + redirect_uri: redirect_uri.clone(), scope: btreeset![OAUTH2_SCOPE_GROUPS.to_string()], nonce: Some("abcdef".to_string()), oidc_ext: Default::default(), @@ -4080,12 +4154,22 @@ mod tests { // Check we are reflecting the CSRF properly. assert_eq!(permit_success.state.as_deref(), None); + // Assert we followed the redirect uri including the query elements + // we have in the url. + let permit_redirect_uri = permit_success.build_redirect_uri(); + + assert_eq!(permit_redirect_uri.origin(), redirect_uri.origin()); + assert_eq!(permit_redirect_uri.path(), redirect_uri.path()); + let query = BTreeMap::from_iter(permit_redirect_uri.query_pairs().into_owned()); + // Assert the query pair wasn't changed + assert_eq!(query.get("custom").map(|s| s.as_str()), Some("foo")); + // == Submit the token exchange code. // ⚠️ This is where we submit a different origin! let token_req = AccessTokenRequest { grant_type: GrantTypeReq::AuthorizationCode { code: permit_success.code, - redirect_uri: Url::parse("https://portal.example.com").unwrap(), + redirect_uri, // From the first step. code_verifier: code_verifier.clone(), },