From 2c95d1b88dee4f665651ad790f3b4382156044a5 Mon Sep 17 00:00:00 2001
From: Michael Zeller <mike@mikezeller.net>
Date: Fri, 6 Sep 2024 23:15:00 -0400
Subject: [PATCH] SAML relay state shouldn't result in internal server error
 (#6537)

The relay state can be set by an IDP to some string value. Nexus itself
is expecting that string to be in the form of JSON that
serializes/deserializes to our `RelayState` type. With this fix we now
attempt to parse this value so that we know where to redirect the user
upon successful authentication, however if we can't we just send the
user to the root aka `/`.
---
 nexus/src/external_api/console_api.rs | 12 +++++-----
 nexus/tests/integration_tests/saml.rs | 32 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs
index 4ea8290bf9..67b07d3488 100644
--- a/nexus/src/external_api/console_api.rs
+++ b/nexus/src/external_api/console_api.rs
@@ -317,14 +317,12 @@ pub(crate) async fn login_saml(
                 }
             };
 
+        // We used to return an internal server error if we failed to parse the
+        // relay state from the IDP. Since an IDP can technically put anything
+        // it wants here it's better to ignore the parse error and continue on
+        // with the login flow.
         let relay_state =
-            if let Some(value) = relay_state_string {
-                Some(RelayState::from_encoded(value).map_err(|e| {
-                    HttpError::for_internal_error(format!("{}", e))
-                })?)
-            } else {
-                None
-            };
+            relay_state_string.and_then(|v| RelayState::from_encoded(v).ok());
 
         let user = nexus
             .silo_user_from_authenticated_subject(
diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs
index e075f3e4da..4057c5dc3a 100644
--- a/nexus/tests/integration_tests/saml.rs
+++ b/nexus/tests/integration_tests/saml.rs
@@ -1310,7 +1310,7 @@ async fn test_post_saml_response_with_relay_state(
             + chrono::Duration::seconds(60),
     );
 
-    let result = NexusRequest::new(
+    let result_with_relay_state = NexusRequest::new(
         RequestBuilder::new(
             client,
             Method::POST,
@@ -1341,9 +1341,37 @@ async fn test_post_saml_response_with_relay_state(
     .await
     .expect("expected success");
 
-    assert!(result.headers["Location"]
+    assert!(result_with_relay_state.headers["Location"]
         .to_str()
         .unwrap()
         .to_string()
         .ends_with("/some/actual/nexus/url"));
+
+    let result_with_invalid_relay_state = NexusRequest::new(
+        RequestBuilder::new(
+            client,
+            Method::POST,
+            &format!(
+                "/login/{}/saml/some-totally-real-saml-provider",
+                SILO_NAME
+            ),
+        )
+        .raw_body(Some(
+            serde_urlencoded::to_string(SamlLoginPost {
+                saml_response: base64::engine::general_purpose::STANDARD
+                    .encode(SAML_RESPONSE),
+                relay_state: Some("some-idp-set-value".to_string()),
+            })
+            .unwrap(),
+        ))
+        .expect_status(Some(StatusCode::SEE_OTHER)),
+    )
+    .execute()
+    .await
+    .expect("expected success");
+
+    assert_eq!(
+        result_with_invalid_relay_state.headers["Location"].to_str().unwrap(),
+        "/"
+    );
 }