Don't generate and send a nonce for non-OIDC-compliant auth requests

Author: sandhose

crates/data-model/src/upstream_oauth2/session.rs

@@ -250,7 +250,7 @@ pub struct UpstreamOAuthAuthorizationSession {
     pub provider_id: Ulid,
     pub state_str: String,
     pub code_challenge_verifier: Option<String>,
-    pub nonce: String,
+    pub nonce: Option<String>,
     pub created_at: DateTime<Utc>,
 }
 

crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs

@@ -108,14 +108,7 @@ mod tests {
         // Pretend it was linked by an authorization session
         let session = repo
             .upstream_oauth_session()
-            .add(
-                &mut rng,
-                &state.clock,
-                &provider,
-                String::new(),
-                None,
-                String::new(),
-            )
+            .add(&mut rng, &state.clock, &provider, String::new(), None, None)
             .await
             .unwrap();
 

crates/handlers/src/upstream_oauth2/callback.rs

@@ -356,10 +356,12 @@ pub(crate) async fn handler(
             )
             .map_err(mas_oidc_client::error::IdTokenError::from)?;
 
-        // Nonce must match.
-        mas_jose::claims::NONCE
-            .extract_required_with_options(&mut claims, session.nonce.as_str())
-            .map_err(mas_oidc_client::error::IdTokenError::from)?;
+        // Nonce must match if present.
+        if let Some(nonce) = session.nonce.as_deref() {
+            mas_jose::claims::NONCE
+                .extract_required_with_options(&mut claims, nonce)
+                .map_err(mas_oidc_client::error::IdTokenError::from)?;
+        }
 
         context = context.with_id_token_claims(claims);
     }

crates/handlers/src/upstream_oauth2/link.rs

@@ -998,7 +998,7 @@ mod tests {
                 &provider,
                 "state".to_owned(),
                 None,
-                "nonce".to_owned(),
+                None,
             )
             .await
             .unwrap();

crates/oidc-client/src/requests/authorization_code.rs

@@ -191,7 +191,7 @@ pub struct AuthorizationValidationData {
     pub state: String,
 
     /// A string to mitigate replay attacks.
-    pub nonce: String,
+    pub nonce: Option<String>,
 
     /// The URI where the end-user will be redirected after authorization.
     pub redirect_uri: Url,
@@ -216,7 +216,7 @@ fn build_authorization_request(
 ) -> Result<(FullAuthorizationRequest, AuthorizationValidationData), AuthorizationError> {
     let AuthorizationRequestData {
         client_id,
-        mut scope,
+        scope,
         redirect_uri,
         code_challenge_methods_supported,
         display,
@@ -229,9 +229,13 @@ fn build_authorization_request(
         response_mode,
     } = authorization_data;
 
+    let is_openid = scope.contains(&OPENID);
+
     // Generate a random CSRF "state" token and a nonce.
     let state = Alphanumeric.sample_string(rng, 16);
-    let nonce = Alphanumeric.sample_string(rng, 16);
+
+    // Generate a random nonce if we're in 'OpenID Connect' mode
+    let nonce = is_openid.then(|| Alphanumeric.sample_string(rng, 16));
 
     // Use PKCE, whenever possible.
     let (pkce, code_challenge_verifier) = if code_challenge_methods_supported
@@ -263,7 +267,7 @@ fn build_authorization_request(
             scope,
             state: Some(state.clone()),
             response_mode,
-            nonce: Some(nonce.clone()),
+            nonce: nonce.clone(),
             display,
             prompt,
             max_age,
@@ -440,10 +444,12 @@ pub async fn access_token_with_authorization_code(
             .extract_optional_with_options(&mut claims, TokenHash::new(signing_alg, &code))
             .map_err(IdTokenError::from)?;
 
-        // Nonce must match.
-        claims::NONCE
-            .extract_required_with_options(&mut claims, validation_data.nonce.as_str())
-            .map_err(IdTokenError::from)?;
+        // Nonce must match if we have one.
+        if let Some(nonce) = validation_data.nonce.as_deref() {
+            claims::NONCE
+                .extract_required_with_options(&mut claims, nonce)
+                .map_err(IdTokenError::from)?;
+        }
 
         Some(id_token.into_owned())
     } else {

crates/oidc-client/tests/it/requests/authorization_code.rs

@@ -186,7 +186,7 @@ async fn pass_access_token_with_authorization_code() {
     let redirect_uri = Url::parse(REDIRECT_URI).unwrap();
     let validation_data = AuthorizationValidationData {
         state: "some_state".to_owned(),
-        nonce: NONCE.to_owned(),
+        nonce: Some(NONCE.to_owned()),
         redirect_uri,
         code_challenge_verifier: Some(CODE_VERIFIER.to_owned()),
     };
@@ -244,7 +244,7 @@ async fn fail_access_token_with_authorization_code_wrong_nonce() {
     let redirect_uri = Url::parse(REDIRECT_URI).unwrap();
     let validation_data = AuthorizationValidationData {
         state: "some_state".to_owned(),
-        nonce: "wrong_nonce".to_owned(),
+        nonce: Some("wrong_nonce".to_owned()),
         redirect_uri,
         code_challenge_verifier: Some(CODE_VERIFIER.to_owned()),
     };
@@ -306,7 +306,7 @@ async fn fail_access_token_with_authorization_code_no_id_token() {
     let nonce = "some_nonce".to_owned();
     let validation_data = AuthorizationValidationData {
         state: "some_state".to_owned(),
-        nonce: nonce.clone(),
+        nonce: Some(nonce.clone()),
         redirect_uri,
         code_challenge_verifier: Some(CODE_VERIFIER.to_owned()),
     };

crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json

@@ -0,0 +1,8 @@
+-- Copyright 2025 New Vector Ltd.
+--
+-- SPDX-License-Identifier: AGPL-3.0-only
+-- Please see LICENSE in the repository root for full details.
+
+-- Make the nonce column optional on the upstream oauth sessions
+ALTER TABLE "upstream_oauth_authorization_sessions"
+    ALTER COLUMN "nonce" DROP NOT NULL;

crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql

@@ -108,7 +108,7 @@ mod tests {
                 &provider,
                 "some-state".to_owned(),
                 None,
-                "some-nonce".to_owned(),
+                Some("some-nonce".to_owned()),
             )
             .await
             .unwrap();

crates/storage-pg/src/upstream_oauth2/mod.rs

@@ -38,7 +38,7 @@ struct SessionLookup {
     upstream_oauth_link_id: Option<Uuid>,
     state: String,
     code_challenge_verifier: Option<String>,
-    nonce: String,
+    nonce: Option<String>,
     id_token: Option<String>,
     userinfo: Option<serde_json::Value>,
     created_at: DateTime<Utc>,
@@ -191,7 +191,7 @@ impl UpstreamOAuthSessionRepository for PgUpstreamOAuthSessionRepository<'_> {
         upstream_oauth_provider: &UpstreamOAuthProvider,
         state_str: String,
         code_challenge_verifier: Option<String>,
-        nonce: String,
+        nonce: Option<String>,
     ) -> Result<UpstreamOAuthAuthorizationSession, Self::Error> {
         let created_at = clock.now();
         let id = Ulid::from_datetime_with_source(created_at.into(), rng);