Skip to content

Commit

Permalink
SAML relay state shouldn't result in internal server error (#6537)
Browse files Browse the repository at this point in the history
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 `/`.
  • Loading branch information
papertigers authored Sep 7, 2024
1 parent b45ec6d commit 2c95d1b
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 9 deletions.
12 changes: 5 additions & 7 deletions nexus/src/external_api/console_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
32 changes: 30 additions & 2 deletions nexus/tests/integration_tests/saml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(),
"/"
);
}

0 comments on commit 2c95d1b

Please sign in to comment.