From 47e67c663068be298438181357e759f48f65529b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Wed, 17 Sep 2025 16:58:50 +0200 Subject: [PATCH 1/6] Fixes pentest issue DG25-24 from 2025-09-02 --- Cargo.lock | 36 +++++------ .../defguard_core/src/db/models/auth_code.rs | 63 ++++++++++++------- .../defguard_core/src/handlers/openid_flow.rs | 32 +++++----- .../tests/integration/api/common/client.rs | 2 - .../tests/integration/api/mod.rs | 2 + .../tests/integration/api/openid.rs | 58 ++++++++++------- .../tests/integration/api/user.rs | 7 ++- 7 files changed, 117 insertions(+), 83 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f4c4ff4e17..4081902eb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2402,9 +2402,9 @@ dependencies = [ [[package]] name = "js-sys" -version = "0.3.79" +version = "0.3.80" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6247da8b8658ad4e73a186e747fcc5fc2a29f979d6fe6269127fdb5fd08298d0" +checksum = "852f13bec5eba4ba9afbeb93fd7c13fe56147f055939ae21c43a29a0ecb2702e" dependencies = [ "once_cell", "wasm-bindgen", @@ -5160,9 +5160,9 @@ dependencies = [ [[package]] name = "tokio-rustls" -version = "0.26.2" +version = "0.26.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e727b36a1a0e8b74c376ac2211e40c2c8af09fb4013c60d910495810f008e9b" +checksum = "05f63835928ca123f1bef57abbcd23bb2ba0ac9ae1235f1e65bda0d06e7786bd" dependencies = [ "rustls", "tokio", @@ -5809,9 +5809,9 @@ checksum = "b8dad83b4f25e74f184f64c43b150b91efe7647395b42289f38e50566d82855b" [[package]] name = "wasm-bindgen" -version = "0.2.102" +version = "0.2.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4ad224d2776649cfb4f4471124f8176e54c1cca67a88108e30a0cd98b90e7ad3" +checksum = "ab10a69fbd0a177f5f649ad4d8d3305499c42bab9aef2f7ff592d0ec8f833819" dependencies = [ "cfg-if", "once_cell", @@ -5822,9 +5822,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-backend" -version = "0.2.102" +version = "0.2.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3a1364104bdcd3c03f22b16a3b1c9620891469f5e9f09bc38b2db121e593e732" +checksum = "0bb702423545a6007bbc368fde243ba47ca275e549c8a28617f56f6ba53b1d1c" dependencies = [ "bumpalo", "log", @@ -5836,9 +5836,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-futures" -version = "0.4.52" +version = "0.4.53" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9c0a08ecf5d99d5604a6666a70b3cde6ab7cc6142f5e641a8ef48fc744ce8854" +checksum = "a0b221ff421256839509adbb55998214a70d829d3a28c69b4a6672e9d2a42f67" dependencies = [ "cfg-if", "js-sys", @@ -5849,9 +5849,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro" -version = "0.2.102" +version = "0.2.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0d7ab4ca3e367bb1ed84ddbd83cc6e41e115f8337ed047239578210214e36c76" +checksum = "fc65f4f411d91494355917b605e1480033152658d71f722a90647f56a70c88a0" dependencies = [ "quote", "wasm-bindgen-macro-support", @@ -5859,9 +5859,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro-support" -version = "0.2.102" +version = "0.2.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4a518014843a19e2dbbd0ed5dfb6b99b23fb886b14e6192a00803a3e14c552b0" +checksum = "ffc003a991398a8ee604a401e194b6b3a39677b3173d6e74495eb51b82e99a32" dependencies = [ "proc-macro2", "quote", @@ -5872,9 +5872,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-shared" -version = "0.2.102" +version = "0.2.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "255eb0aa4cc2eea3662a00c2bbd66e93911b7361d5e0fcd62385acfd7e15dcee" +checksum = "293c37f4efa430ca14db3721dfbe48d8c33308096bd44d80ebaa775ab71ba1cf" dependencies = [ "unicode-ident", ] @@ -5894,9 +5894,9 @@ dependencies = [ [[package]] name = "web-sys" -version = "0.3.79" +version = "0.3.80" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "50462a022f46851b81d5441d1a6f5bac0b21a1d72d64bd4906fbdd4bf7230ec7" +checksum = "fbe734895e869dc429d78c4b433f8d17d95f8d05317440b4fad5ab2d33e596dc" dependencies = [ "js-sys", "wasm-bindgen", diff --git a/crates/defguard_core/src/db/models/auth_code.rs b/crates/defguard_core/src/db/models/auth_code.rs index 6b2fd73b53..5cf60f13dd 100644 --- a/crates/defguard_core/src/db/models/auth_code.rs +++ b/crates/defguard_core/src/db/models/auth_code.rs @@ -1,29 +1,30 @@ use chrono::Utc; use model_derive::Model; -use sqlx::{Error as SqlxError, PgPool, query_as}; +use sqlx::{PgExecutor, query_as}; use crate::{ db::{Id, NoId}, random::gen_alphanumeric, }; -#[derive(Model, Clone)] +#[derive(Model)] #[table(authorization_code)] -pub struct AuthCode { +pub(crate) struct AuthCode { + #[allow(dead_code)] id: I, - pub user_id: Id, - pub client_id: String, - pub code: String, - pub redirect_uri: String, - pub scope: String, - pub auth_time: i64, - pub nonce: Option, - pub code_challenge: Option, + pub(crate) user_id: Id, + pub(crate) client_id: String, + pub(crate) code: String, + pub(crate) redirect_uri: String, + pub(crate) scope: String, + pub(crate) auth_time: i64, + pub(crate) nonce: Option, + pub(crate) code_challenge: Option, } impl AuthCode { #[must_use] - pub fn new( + pub(crate) fn new( user_id: Id, client_id: String, redirect_uri: String, @@ -46,21 +47,41 @@ impl AuthCode { } } +impl From> for AuthCode { + fn from(value: AuthCode) -> Self { + Self { + id: NoId, + user_id: value.user_id, + client_id: value.client_id, + code: value.code, + redirect_uri: value.redirect_uri, + scope: value.scope, + auth_time: value.auth_time, + nonce: value.nonce, + code_challenge: value.code_challenge, + } + } +} + impl AuthCode { /// Find by code. - pub async fn find_code(pool: &PgPool, code: &str) -> Result, SqlxError> { + /// If found, delete `AuthCode` from the database right away, so it can't be reused. + pub(crate) async fn find_code<'e, E>( + executor: E, + code: &str, + ) -> Result>, sqlx::Error> + where + E: PgExecutor<'e>, + { query_as!( Self, - "SELECT id, user_id, client_id, code, redirect_uri, scope, auth_time, nonce, \ - code_challenge FROM authorization_code WHERE code = $1", + "DELETE FROM authorization_code WHERE code = $1 \ + RETURNING id, user_id, client_id, code, redirect_uri, scope, auth_time, nonce, \ + code_challenge", code ) - .fetch_optional(pool) + .fetch_optional(executor) .await - } - - // Remove a used authorization_code - pub async fn consume(self, pool: &PgPool) -> Result<(), SqlxError> { - self.delete(pool).await + .map(|inner_option| inner_option.map(Into::into)) } } diff --git a/crates/defguard_core/src/handlers/openid_flow.rs b/crates/defguard_core/src/handlers/openid_flow.rs index dce8b6d569..4141f502f4 100644 --- a/crates/defguard_core/src/handlers/openid_flow.rs +++ b/crates/defguard_core/src/handlers/openid_flow.rs @@ -43,7 +43,7 @@ use crate::{ appstate::AppState, auth::{AccessUserInfo, SessionInfo, UserClaims}, db::{ - Id, OAuth2AuthorizedApp, OAuth2Token, Session, SessionState, User, + Id, NoId, OAuth2AuthorizedApp, OAuth2Token, Session, SessionState, User, models::{auth_code::AuthCode, oauth2client::OAuth2Client}, }, error::WebError, @@ -538,7 +538,7 @@ pub struct GroupClaims { impl AdditionalClaims for GroupClaims {} -pub async fn get_group_claims(pool: &PgPool, user: &User) -> Result { +async fn get_group_claims(pool: &PgPool, user: &User) -> Result { let groups = user.member_of_names(pool).await?; Ok(GroupClaims { groups: Some(groups), @@ -663,7 +663,7 @@ impl TokenRequest { fn authorization_code_flow( &self, - auth_code: &AuthCode, + auth_code: &AuthCode, token: &OAuth2Token, claims: StandardClaims, base_url: &Url, @@ -794,20 +794,16 @@ pub async fn token( // for logging let form_client_id = match &form.client_id { - Some(id) => id.clone(), - None => String::from("N/A"), + Some(id) => id, + None => "N/A", }; if let Some(code) = &form.code { - if let Some(stored_auth_code) = AuthCode::find_code(&appstate.pool, code).await? { - // copy data before removing used token - let auth_code = stored_auth_code.clone(); - // remove authorization_code from DB so it cannot be reused - debug!( - "Removing used authorization_code {code}, client_id `{}`", - form_client_id - ); - stored_auth_code.consume(&appstate.pool).await?; + // Look for `AuthCode`. If found, it will be deleted from the database to avoid + // concurrent requests that might return multiple tokens for the same code. + // This addresses DG25-24 and conforms to RFC 6749. + if let Some(auth_code) = AuthCode::find_code(&appstate.pool, code).await? { + debug!("Consumed authorization_code {code}, client_id `{form_client_id}`"); if let Some(client) = oauth2client.or(form.oauth2client(&appstate.pool).await) { if let Some(user) = User::find_by_id(&appstate.pool, auth_code.user_id).await? @@ -824,7 +820,7 @@ pub async fn token( "Issuing new token for user {} client {}", user.username, client.name ); - // Remove existing token in case same client asks for new token + // Remove existing token in case the same client asks for new token. if let Some(token) = OAuth2Token::find_by_authorized_app_id( &appstate.pool, authorized_app.id, @@ -867,8 +863,8 @@ pub async fn token( } Err(err) => { error!( - "Error issuing new token for user {} client {}: {}", - user.username, client.name, err + "Error issuing new token for user {} client {}: {err}", + user.username, client.name ); let response = StandardErrorResponse::::new( @@ -895,7 +891,7 @@ pub async fn token( error!("OAuth auth code not found"); } } else { - error!("No code provided in request for client id `{form_client_id}`",); + error!("No code provided in request for client id `{form_client_id}`"); } } "refresh_token" => { diff --git a/crates/defguard_core/tests/integration/api/common/client.rs b/crates/defguard_core/tests/integration/api/common/client.rs index 61a45135dc..9292b31a3e 100644 --- a/crates/defguard_core/tests/integration/api/common/client.rs +++ b/crates/defguard_core/tests/integration/api/common/client.rs @@ -176,8 +176,6 @@ impl RequestBuilder { /// This is conventient for tests where panics are what you want. For access to /// non-panicking versions or the complete `Response` API use `into_inner()` or /// `as_ref()`. -#[derive(Debug)] - pub struct TestResponse { response: reqwest::Response, } diff --git a/crates/defguard_core/tests/integration/api/mod.rs b/crates/defguard_core/tests/integration/api/mod.rs index 4891501678..218ff73cf8 100644 --- a/crates/defguard_core/tests/integration/api/mod.rs +++ b/crates/defguard_core/tests/integration/api/mod.rs @@ -19,3 +19,5 @@ mod wireguard_network_devices; mod wireguard_network_import; mod wireguard_network_stats; mod worker; + +const TEST_SERVER_URL: &str = "http://localhost:3000/"; diff --git a/crates/defguard_core/tests/integration/api/openid.rs b/crates/defguard_core/tests/integration/api/openid.rs index 7b701bba14..862461f5ec 100644 --- a/crates/defguard_core/tests/integration/api/openid.rs +++ b/crates/defguard_core/tests/integration/api/openid.rs @@ -26,8 +26,11 @@ use rsa::RsaPrivateKey; use serde::Deserialize; use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; -use super::common::{ - client::TestClient, make_client, make_client_with_state, make_test_client, setup_pool, +use super::{ + TEST_SERVER_URL, + common::{ + client::TestClient, make_client, make_client_with_state, make_test_client, setup_pool, + }, }; #[derive(Deserialize)] @@ -48,7 +51,7 @@ async fn test_openid_client(_: PgPoolOptions, options: PgConnectOptions) { let mut openid_client = NewOpenIDClient { name: "Test".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; @@ -107,7 +110,7 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { assert_eq!(response.status(), StatusCode::OK); let openid_client = NewOpenIDClient { name: "Test".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; @@ -125,6 +128,7 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { let response = client.get("/api/v1/oauth").send().await; assert_eq!(response.status(), StatusCode::OK); + // Try invalid request for `response_type = code id_token token`. let response = client .post(format!( "/api/v1/oauth/authorize?\ @@ -147,6 +151,7 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { .unwrap(); assert!(location.contains("error=invalid_request")); + // Try invalid request for `response_type = id_token`. let response = client .post(format!( "/api/v1/oauth/authorize?\ @@ -193,11 +198,11 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { .to_str() .unwrap(); let (location, query) = location.split_once('?').unwrap(); - assert_eq!(location, "http://localhost:3000/"); + assert_eq!(location, TEST_SERVER_URL); let auth_response: AuthenticationResponse = serde_qs::from_str(query).unwrap(); assert_eq!(auth_response.state, "ABCDEF"); - // exchange wrong code for token should fail + // Exchanging a wrong code for a token should fail. let response = client .post("/api/v1/oauth/token") .header(CONTENT_TYPE, "application/x-www-form-urlencoded") @@ -213,23 +218,33 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); - // exchange correct code for token + // Exchange correct code for a token. + let token_body = format!( + "grant_type=authorization_code&\ + code={}&\ + redirect_uri=http%3A%2F%2Flocalhost%3A3000%2F&\ + client_id={}&\ + client_secret={}", + auth_response.code, openid_client.client_id, openid_client.client_secret + ); let response = client .post("/api/v1/oauth/token") .header(CONTENT_TYPE, "application/x-www-form-urlencoded") - .body(format!( - "grant_type=authorization_code&\ - code={}&\ - redirect_uri=http%3A%2F%2Flocalhost%3A3000%2F&\ - client_id={}&\ - client_secret={}", - auth_response.code, openid_client.client_id, openid_client.client_secret - )) + .body(token_body.clone()) .send() .await; assert_eq!(response.status(), StatusCode::OK); - // make sure access token cannot be used to manage defguard server itself + // Try to get another authentication code for the same code. + let another_response = client + .post("/api/v1/oauth/token") + .header(CONTENT_TYPE, "application/x-www-form-urlencoded") + .body(token_body) + .send() + .await; + assert_eq!(another_response.status(), StatusCode::BAD_REQUEST); + + // Make sure access token cannot be used to manage Defguard server itself. client.post("/api/v1/auth/logout").send().await; let token_response: CoreTokenResponse = response.json().await; let bearer = format!("Bearer {}", token_response.access_token().secret()); @@ -247,7 +262,6 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { assert_eq!(response.status(), StatusCode::UNAUTHORIZED); // log back in - let auth = Auth::new("admin", "pass123"); let response = client.post("/api/v1/auth").json(&auth).send().await; assert_eq!(response.status(), StatusCode::OK); @@ -630,7 +644,7 @@ async fn dg25_23_test_openid_client_scope_change_clears_authorizations( // Create OAuth2 client with initial scopes let oauth2client = NewOpenIDClient { name: "Test Client".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into(), "email".into()], enabled: true, }; @@ -677,7 +691,7 @@ async fn dg25_23_test_openid_client_scope_change_clears_authorizations( // Update the client with different scopes let updated_client = NewOpenIDClient { name: "Test Client".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into(), "profile".into()], // Changed from email to profile enabled: true, }; @@ -737,7 +751,7 @@ async fn dg25_23_test_openid_client_scope_change_clears_authorizations( // Update the client without changing scopes (only name) let same_scope_update = NewOpenIDClient { name: "Test Client Updated Name".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into(), "profile".into()], // Same scopes enabled: true, }; @@ -961,7 +975,7 @@ async fn test_openid_flow_new_login_mail(_: PgPoolOptions, options: PgConnectOpt assert_eq!(response.status(), StatusCode::OK); let openid_client = NewOpenIDClient { name: "Test".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; @@ -1002,7 +1016,7 @@ async fn test_openid_flow_new_login_mail(_: PgPoolOptions, options: PgConnectOpt .to_str() .unwrap(); let (location, query) = location.split_once('?').unwrap(); - assert_eq!(location, "http://localhost:3000/"); + assert_eq!(location, TEST_SERVER_URL); let auth_response: AuthenticationResponse = serde_qs::from_str(query).unwrap(); assert_eq!(auth_response.state, "ABCDEF"); diff --git a/crates/defguard_core/tests/integration/api/user.rs b/crates/defguard_core/tests/integration/api/user.rs index a2c6307e59..197eea3afc 100644 --- a/crates/defguard_core/tests/integration/api/user.rs +++ b/crates/defguard_core/tests/integration/api/user.rs @@ -9,7 +9,10 @@ use reqwest::{StatusCode, header::USER_AGENT}; use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; use tokio_stream::{self as stream, StreamExt}; -use super::common::{fetch_user_details, make_client, make_network, make_test_client, setup_pool}; +use super::{ + TEST_SERVER_URL, + common::{fetch_user_details, make_client, make_network, make_test_client, setup_pool}, +}; #[sqlx::test] async fn test_authenticate(_: PgPoolOptions, options: PgConnectOptions) { @@ -399,7 +402,7 @@ async fn test_user_unregister_authorized_app(_: PgPoolOptions, options: PgConnec assert_eq!(response.status(), StatusCode::OK); let openid_client = NewOpenIDClient { name: "Test".into(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; From 43c11bf947e58994f51487577dcbc1c546172f2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 18 Sep 2025 08:37:53 +0200 Subject: [PATCH 2/6] sqlx json --- ...993df10ea337195635ad0e24e4855aa42cdceab2712f8c8dbef4.json} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename .sqlx/{query-321ce9355db39c3d93d8b915ba7888357f5403c2e9e97fa17e502223bbcc918a.json => query-27f3c5ea4443993df10ea337195635ad0e24e4855aa42cdceab2712f8c8dbef4.json} (82%) diff --git a/.sqlx/query-321ce9355db39c3d93d8b915ba7888357f5403c2e9e97fa17e502223bbcc918a.json b/.sqlx/query-27f3c5ea4443993df10ea337195635ad0e24e4855aa42cdceab2712f8c8dbef4.json similarity index 82% rename from .sqlx/query-321ce9355db39c3d93d8b915ba7888357f5403c2e9e97fa17e502223bbcc918a.json rename to .sqlx/query-27f3c5ea4443993df10ea337195635ad0e24e4855aa42cdceab2712f8c8dbef4.json index c4bb584637..00ccd67e94 100644 --- a/.sqlx/query-321ce9355db39c3d93d8b915ba7888357f5403c2e9e97fa17e502223bbcc918a.json +++ b/.sqlx/query-27f3c5ea4443993df10ea337195635ad0e24e4855aa42cdceab2712f8c8dbef4.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT id, user_id, client_id, code, redirect_uri, scope, auth_time, nonce, code_challenge FROM authorization_code WHERE code = $1", + "query": "DELETE FROM authorization_code WHERE code = $1 RETURNING id, user_id, client_id, code, redirect_uri, scope, auth_time, nonce, code_challenge", "describe": { "columns": [ { @@ -66,5 +66,5 @@ true ] }, - "hash": "321ce9355db39c3d93d8b915ba7888357f5403c2e9e97fa17e502223bbcc918a" + "hash": "27f3c5ea4443993df10ea337195635ad0e24e4855aa42cdceab2712f8c8dbef4" } From 8d4b8dcb4ec1c79b7758878d70832108772cc642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 18 Sep 2025 08:59:57 +0200 Subject: [PATCH 3/6] Use TEST_SERVER_URL --- crates/defguard_core/tests/integration/api/openid.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/defguard_core/tests/integration/api/openid.rs b/crates/defguard_core/tests/integration/api/openid.rs index 786c165296..12069d7ceb 100644 --- a/crates/defguard_core/tests/integration/api/openid.rs +++ b/crates/defguard_core/tests/integration/api/openid.rs @@ -427,7 +427,7 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { .unwrap() .to_str() .unwrap(); - assert!(location.starts_with("http://localhost:3000")); + assert!(location.starts_with(TEST_SERVER_URL)); assert!(location.contains("error=access_denied")); } @@ -859,7 +859,7 @@ async fn dg25_17_test_openid_open_redirects(_: PgPoolOptions, options: PgConnect // Create OAuth2 client let oauth2client = NewOpenIDClient { name: "Test Client".into(), - redirect_uri: vec!["http://localhost:3000/".into(), "http://safe.net/".into()], + redirect_uri: vec![TEST_SERVER_URL.into(), "http://safe.net/".into()], scope: vec!["openid".into(), "email".into()], enabled: true, }; @@ -1233,7 +1233,7 @@ async fn dg25_21_test_openid_html_injection(_: PgPoolOptions, options: PgConnect for name in invalid_names { let openid_client = NewOpenIDClient { name: name.to_string(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; @@ -1248,7 +1248,7 @@ async fn dg25_21_test_openid_html_injection(_: PgPoolOptions, options: PgConnect // create valid openid client let openid_client = NewOpenIDClient { name: "Test".to_string(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; @@ -1265,7 +1265,7 @@ async fn dg25_21_test_openid_html_injection(_: PgPoolOptions, options: PgConnect for name in invalid_names { let openid_client = NewOpenIDClient { name: name.to_string(), - redirect_uri: vec!["http://localhost:3000/".into()], + redirect_uri: vec![TEST_SERVER_URL.into()], scope: vec!["openid".into()], enabled: true, }; From d35dbe00f987ebb2e1401ecbbfed5e2d2ccaa841 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 18 Sep 2025 10:27:19 +0200 Subject: [PATCH 4/6] Fixes --- .../src/db/models/oauth2client.rs | 45 ++++++++++++++----- .../defguard_core/src/handlers/openid_flow.rs | 2 +- .../tests/integration/api/openid.rs | 31 +++++-------- 3 files changed, 47 insertions(+), 31 deletions(-) diff --git a/crates/defguard_core/src/db/models/oauth2client.rs b/crates/defguard_core/src/db/models/oauth2client.rs index 0adea97e73..750d4d87a8 100644 --- a/crates/defguard_core/src/db/models/oauth2client.rs +++ b/crates/defguard_core/src/db/models/oauth2client.rs @@ -103,23 +103,21 @@ impl OAuth2Client { .await } - /// Checks if `url` matches client config (ignoring trailing slashes) + /// Checks if `url` matches client config (ignoring trailing slashes). pub(crate) fn contains_redirect_url(&self, url: &str) -> bool { - let parsed_redirect_uris: Vec = self - .redirect_uri - .iter() - .map(|uri| uri.trim_end_matches('/').into()) - .collect(); - - // extract origin from url + // Extract origin from `url`. let Ok(url) = Url::parse(url) else { return false; }; let url = url.origin().ascii_serialization(); + let url_trimmed = url.trim_end_matches('/'); - !url.split(' ') - .map(|uri| uri.trim_end_matches('/')) - .all(|uri| !parsed_redirect_uris.iter().any(|u| u == uri)) + for redirect in &self.redirect_uri { + if url_trimmed == redirect.trim_end_matches('/') { + return true; + } + } + false } } @@ -140,3 +138,28 @@ impl From> for OAuth2ClientSafe { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_contains_redirect_url() { + let oauth2client = OAuth2Client { + id: 1, + client_id: String::new(), + client_secret: String::new(), + redirect_uri: vec![ + String::from("http://localhost/"), + String::from("http://safe.net/"), + ], + scope: Vec::new(), + name: String::new(), + enabled: true, + }; + assert!(oauth2client.contains_redirect_url("http://safe.net")); + assert!(oauth2client.contains_redirect_url("http://localhost")); + assert!(oauth2client.contains_redirect_url("http://safe.net/api")); + assert!(!oauth2client.contains_redirect_url("http://nonexistent:8000")); + } +} diff --git a/crates/defguard_core/src/handlers/openid_flow.rs b/crates/defguard_core/src/handlers/openid_flow.rs index 41ada91741..cd2690286c 100644 --- a/crates/defguard_core/src/handlers/openid_flow.rs +++ b/crates/defguard_core/src/handlers/openid_flow.rs @@ -396,7 +396,7 @@ pub async fn authorization( ); // FIXME: do not panic return Ok(redirect_to( - format!("/consent?{}", serde_urlencoded::to_string(data).unwrap(),), + format!("/consent?{}", serde_urlencoded::to_string(data).unwrap()), private_cookies, )); } diff --git a/crates/defguard_core/tests/integration/api/openid.rs b/crates/defguard_core/tests/integration/api/openid.rs index 12069d7ceb..927c9549ca 100644 --- a/crates/defguard_core/tests/integration/api/openid.rs +++ b/crates/defguard_core/tests/integration/api/openid.rs @@ -20,7 +20,7 @@ use openidconnect::{ }; use reqwest::{ StatusCode, Url, - header::{AUTHORIZATION, CONTENT_TYPE, HeaderName, USER_AGENT}, + header::{AUTHORIZATION, CONTENT_TYPE, HeaderName, LOCATION, USER_AGENT}, }; use rsa::RsaPrivateKey; use serde::Deserialize; @@ -111,7 +111,7 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { assert_eq!(response.status(), StatusCode::OK); let openid_client = NewOpenIDClient { name: "Test".into(), - redirect_uri: vec![TEST_SERVER_URL.into()], + redirect_uri: vec![TEST_SERVER_URL.into(), "http://safe.net".into()], scope: vec!["openid".into()], enabled: true, }; @@ -375,10 +375,10 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { .unwrap() .to_str() .unwrap(); - // assert!(location.starts_with("http://safe.net"), "{location}"); + assert!(location.starts_with("http://safe.net"), "{location}"); assert!(location.contains("error=invalid_scope")); - // assert!(location.contains("value1=one")); - // assert!(location.contains("value2=two")); + assert!(location.contains("value1=one")); + assert!(location.contains("value2=two")); // test wrong redirect uri let response = client @@ -873,17 +873,10 @@ async fn dg25_17_test_openid_open_redirects(_: PgPoolOptions, options: PgConnect let oauth2client: OAuth2Client = response.json().await; fn redirect_url(response: &TestResponse) -> String { - Url::parse( - response - .headers() - .get(reqwest::header::LOCATION) - .unwrap() - .to_str() - .unwrap(), - ) - .unwrap() - .origin() - .ascii_serialization() + Url::parse(response.headers().get(LOCATION).unwrap().to_str().unwrap()) + .unwrap() + .origin() + .ascii_serialization() } let fallback_url = state @@ -908,7 +901,7 @@ async fn dg25_17_test_openid_open_redirects(_: PgPoolOptions, options: PgConnect .send() .await; assert_eq!(response.status(), StatusCode::FOUND); - assert_eq!(redirect_url(&response), fallback_url,); + assert_eq!(redirect_url(&response), fallback_url); let response = client .get( @@ -1010,7 +1003,7 @@ async fn dg25_17_test_openid_open_redirects(_: PgPoolOptions, options: PgConnect .send() .await; assert_eq!(response.status(), StatusCode::FOUND); - assert_eq!(redirect_url(&response), "http://safe.net",); + assert_eq!(redirect_url(&response), "http://safe.net"); let response = client .get(format!( @@ -1027,7 +1020,7 @@ async fn dg25_17_test_openid_open_redirects(_: PgPoolOptions, options: PgConnect .send() .await; assert_eq!(response.status(), StatusCode::FOUND); - assert_eq!(redirect_url(&response), "http://safe.net",); + assert_eq!(redirect_url(&response), "http://safe.net"); } #[sqlx::test] From 441532776c1ab31294e0b281d647c760aaffcd4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 18 Sep 2025 10:39:45 +0200 Subject: [PATCH 5/6] Formatting --- .../defguard_core/src/handlers/openid_flow.rs | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/crates/defguard_core/src/handlers/openid_flow.rs b/crates/defguard_core/src/handlers/openid_flow.rs index cd2690286c..823066ede9 100644 --- a/crates/defguard_core/src/handlers/openid_flow.rs +++ b/crates/defguard_core/src/handlers/openid_flow.rs @@ -324,7 +324,7 @@ async fn generate_auth_code_redirect( if let Some(state) = data.state { query_pairs.append_pair("state", &state); } - }; + } Ok(url.to_string()) } @@ -412,7 +412,8 @@ pub async fn authorization( // If session expired return login if session.expired() { info!( - "Session {} for user id {} has expired, redirecting to login", + "Session {} for user id {} has expired, redirecting to \ + login", session.id, session.user_id ); let _result = session.delete(&appstate.pool).await; @@ -427,8 +428,9 @@ pub async fn authorization( user.verify_mfa_state(&appstate.pool).await?; - // Session exists even if user hasn't completed MFA verification yet, - // thus we need to check if MFA is enabled and the verification is done. + // Session exists even if user hasn't completed MFA verification + // yet, thus we need to check if MFA is enabled and the + // verification is done. if user.mfa_enabled && session.state != SessionState::MultiFactorVerified { @@ -439,8 +441,9 @@ pub async fn authorization( return Ok(login_redirect(&data, private_cookies)); } - // If session is present check if app is in user authorized apps. - // If yes return auth code and state else redirect to consent form. + // If session is present check if app is in user authorized + // apps. If yes, return auth code and state else redirect to + // consent form. if let Some(app) = OAuth2AuthorizedApp::find_by_user_and_oauth2client_id( &appstate.pool, @@ -450,7 +453,8 @@ pub async fn authorization( .await? { info!( - "OAuth client id {} authorized by user id {}, returning auth code", + "OAuth client id {} authorized by user id {}, \ + returning auth code", app.oauth2client_id, session.user_id ); let private_cookies = private_cookies @@ -465,7 +469,8 @@ pub async fn authorization( } else { // If authorized app not found redirect to consent form info!( - "OAuth client id {} not yet authorized by user id {}, redirecting to consent form", + "OAuth client id {} not yet authorized by user id {}, \ + redirecting to consent form", oauth2client.id, session.user_id ); Ok(redirect_to( @@ -478,7 +483,7 @@ pub async fn authorization( } } } else { - // If session is not present in db redirect to login + // If session is not present in database, redirect to login. info!( "Session {} not found, redirecting to login page", session_cookie.value() @@ -878,7 +883,8 @@ pub async fn token( } } error!( - "Can't issue token - authorized app not found for user {}, client {}", + "Can't issue token - authorized app not found for user {}, client \ + {}", user.username, client.name ); } else { From d4b9c356d7d4f06cf383a61956578f10ed9f54a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Thu, 18 Sep 2025 11:06:31 +0200 Subject: [PATCH 6/6] Don't compare only redirect URL's origin --- crates/defguard_core/src/db/models/oauth2client.rs | 9 ++------- crates/defguard_core/tests/integration/api/openid.rs | 6 ++---- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/crates/defguard_core/src/db/models/oauth2client.rs b/crates/defguard_core/src/db/models/oauth2client.rs index 750d4d87a8..4a886c23d7 100644 --- a/crates/defguard_core/src/db/models/oauth2client.rs +++ b/crates/defguard_core/src/db/models/oauth2client.rs @@ -1,5 +1,4 @@ use model_derive::Model; -use reqwest::Url; use sqlx::{Error as SqlxError, PgExecutor, PgPool, query_as}; use super::NewOpenIDClient; @@ -105,11 +104,6 @@ impl OAuth2Client { /// Checks if `url` matches client config (ignoring trailing slashes). pub(crate) fn contains_redirect_url(&self, url: &str) -> bool { - // Extract origin from `url`. - let Ok(url) = Url::parse(url) else { - return false; - }; - let url = url.origin().ascii_serialization(); let url_trimmed = url.trim_end_matches('/'); for redirect in &self.redirect_uri { @@ -117,6 +111,7 @@ impl OAuth2Client { return true; } } + false } } @@ -159,7 +154,7 @@ mod tests { }; assert!(oauth2client.contains_redirect_url("http://safe.net")); assert!(oauth2client.contains_redirect_url("http://localhost")); - assert!(oauth2client.contains_redirect_url("http://safe.net/api")); + assert!(!oauth2client.contains_redirect_url("http://safe.net/api")); assert!(!oauth2client.contains_redirect_url("http://nonexistent:8000")); } } diff --git a/crates/defguard_core/tests/integration/api/openid.rs b/crates/defguard_core/tests/integration/api/openid.rs index 927c9549ca..1a3882d4c2 100644 --- a/crates/defguard_core/tests/integration/api/openid.rs +++ b/crates/defguard_core/tests/integration/api/openid.rs @@ -359,7 +359,7 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { "/api/v1/oauth/authorize?\ response_type=code&\ client_id={}&\ - redirect_uri=http://safe.net%3Fvalue1=one%26value2=two&\ + redirect_uri=http://safe.net&\ scope=profile&\ state=ABCDEF&\ allow=true&\ @@ -375,10 +375,8 @@ async fn test_openid_flow(_: PgPoolOptions, options: PgConnectOptions) { .unwrap() .to_str() .unwrap(); - assert!(location.starts_with("http://safe.net"), "{location}"); + assert!(location.starts_with("http://safe.net")); assert!(location.contains("error=invalid_scope")); - assert!(location.contains("value1=one")); - assert!(location.contains("value2=two")); // test wrong redirect uri let response = client