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(), + "/" + ); }