From 9ebfc85c55b6d49ca8ab144a3848a66fce6c6e8d Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Thu, 22 Oct 2026 18:32:31 +0200 Subject: [PATCH 1/8] fix: Encode user ids and names as single URL path segments in UIF The Keycloak and Entra backends joined request-supplied user ids and names onto the base URL as text, so `/`, `?`, `#` and `..` in them changed which URL was requested. Both now append them as one percent-encoded path segment through `append_path_segments`, and the Keycloak backend sends the username as a query parameter. An empty, `.` or `..` segment and one containing a line break are rejected with `400`, as the url crate would drop or strip them and look up a different user. A Keycloak `rootPath` without a trailing slash is now kept, which `Url::join` dropped. --- CHANGELOG.md | 5 + Cargo.lock | 4 + Cargo.toml | 1 + rust/info-fetcher-commons/Cargo.toml | 3 + rust/info-fetcher-commons/src/utils/mod.rs | 1 + .../src/utils/url_path.rs | 133 ++++++++++ rust/user-info-fetcher/Cargo.toml | 1 + rust/user-info-fetcher/src/backend/entra.rs | 135 ++++++++-- .../user-info-fetcher/src/backend/keycloak.rs | 231 +++++++++++++++--- rust/user-info-fetcher/src/backend/mod.rs | 61 ++++- 10 files changed, 519 insertions(+), 56 deletions(-) create mode 100644 rust/info-fetcher-commons/src/utils/url_path.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f6e9786..26581016 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,10 @@ All notable changes to this project will be documented in this file. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#871]). - The reconciler now applies resources and derives the cluster status in discrete apply and update_status steps for the `opa_controller` ([#872]). +- The user-info-fetcher Keycloak and Entra backends now encode user ids and names as a single URL + path segment, and the Keycloak backend sends the username as a query parameter. An empty, `.` or + `..` id or name, or one containing a line break, is rejected with `400` ([#909]). +- The user-info-fetcher Keycloak backend now keeps a `rootPath` without a trailing slash ([#909]). [#873]: https://github.com/stackabletech/opa-operator/pull/873 [#852]: https://github.com/stackabletech/opa-operator/pull/852 @@ -113,6 +117,7 @@ All notable changes to this project will be documented in this file. [#905]: https://github.com/stackabletech/opa-operator/pull/905 [#906]: https://github.com/stackabletech/opa-operator/pull/906 [#908]: https://github.com/stackabletech/opa-operator/pull/908 +[#909]: https://github.com/stackabletech/opa-operator/pull/909 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.lock b/Cargo.lock index 79c42d6c..f33216fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1708,7 +1708,9 @@ dependencies = [ "hyper", "moka", "native-tls", + "percent-encoding", "reqwest", + "rstest", "rustls-pki-types", "serde", "serde_json", @@ -1717,6 +1719,7 @@ dependencies = [ "stackable-operator", "tokio", "tracing", + "url", ] [[package]] @@ -3782,6 +3785,7 @@ dependencies = [ "krb5", "ldap3", "native-tls", + "percent-encoding", "pin-project", "reqwest", "rustls-pki-types", diff --git a/Cargo.toml b/Cargo.toml index 63fbe0aa..511dedc1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ ldap3 = { version = "0.12", features = ["gssapi", "tls"] } md5 = "0.8" moka = { version = "0.12", default-features = false, features = ["future"] } native-tls = "0.2.12" +percent-encoding = "2.3" pin-project = "1.1" # `default-features = false` drops reqwest 0.13's `default-tls = ["rustls"]`; we select # `native-tls` (OpenSSL) to stay on the same TLS stack as ldap3 (see utils/tls.rs). The diff --git a/rust/info-fetcher-commons/Cargo.toml b/rust/info-fetcher-commons/Cargo.toml index bb8e82e7..3909cbba 100644 --- a/rust/info-fetcher-commons/Cargo.toml +++ b/rust/info-fetcher-commons/Cargo.toml @@ -23,6 +23,9 @@ serde_json.workspace = true snafu.workspace = true tracing.workspace = true tokio.workspace = true +url.workspace = true [dev-dependencies] futures.workspace = true +percent-encoding.workspace = true +rstest.workspace = true diff --git a/rust/info-fetcher-commons/src/utils/mod.rs b/rust/info-fetcher-commons/src/utils/mod.rs index 46cf2b0a..8045b5d7 100644 --- a/rust/info-fetcher-commons/src/utils/mod.rs +++ b/rust/info-fetcher-commons/src/utils/mod.rs @@ -3,3 +3,4 @@ pub mod http; pub mod secret; pub mod tls; pub mod token; +pub mod url_path; diff --git a/rust/info-fetcher-commons/src/utils/url_path.rs b/rust/info-fetcher-commons/src/utils/url_path.rs new file mode 100644 index 00000000..def99793 --- /dev/null +++ b/rust/info-fetcher-commons/src/utils/url_path.rs @@ -0,0 +1,133 @@ +use snafu::Snafu; +use url::Url; + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("unable to use {segment:?} as a URL path segment"))] + InvalidPathSegment { segment: String }, + + #[snafu(display("unable to append path segments to {url:?}, as it cannot be a base"))] + CannotBeABase { url: Url }, +} + +/// Returns `base` with each of `segments` appended as exactly one path segment. +/// +/// Use this rather than [`Url::join`] or [`Url::set_path`] to create URLs from user/untrusted input. +/// Both those methods follow the specs but are easy to use insecurely because e.g. if you provide +/// an absolute URL they replace the host from `base` and a few other things (`../` treatment). +/// +/// Fails on an empty, `.` or `..` segment and on a segment containing a tab or line break. +pub fn append_path_segments(base: &Url, segments: &[&str]) -> Result { + let invalid_segment = segments.iter().find(|segment| { + matches!(**segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) + }); + if let Some(segment) = invalid_segment { + return InvalidPathSegmentSnafu { segment: *segment }.fail(); + } + + let mut url = base.clone(); + url.path_segments_mut() + .map_err(|()| CannotBeABaseSnafu { url: base.clone() }.build())? + .pop_if_empty() + .extend(segments); + + Ok(url) +} + +#[cfg(test)] +mod tests { + use percent_encoding::percent_decode_str; + use rstest::rstest; + + use super::*; + + #[rstest] + #[case::root("http://keycloak:8080", "http://keycloak:8080/realms/my-realm")] + #[case::root_with_slash("http://keycloak:8080/", "http://keycloak:8080/realms/my-realm")] + #[case::root_path( + "http://keycloak:8080/auth", + "http://keycloak:8080/auth/realms/my-realm" + )] + #[case::root_path_with_slash( + "http://keycloak:8080/auth/", + "http://keycloak:8080/auth/realms/my-realm" + )] + fn append_keeps_the_base_path(#[case] base: &str, #[case] expected: &str) { + let base = Url::parse(base).expect("valid base URL"); + let url = append_path_segments(&base, &["realms", "my-realm"]).expect("valid segments"); + + assert_eq!(url.as_str(), expected); + } + + #[rstest] + #[case::absolute_url("https://evil.example.com/", "https:%2F%2Fevil.example.com%2F")] + #[case::protocol_relative("//evil.example.com", "%2F%2Fevil.example.com")] + #[case::traversal("../../realms/master", "..%2F..%2Frealms%2Fmaster")] + #[case::percent_encoded_dots("%2e%2e", "%252e%252e")] + #[case::backslash("..\\admin", "..%5Cadmin")] + #[case::query("alice?exact=false", "alice%3Fexact=false")] + #[case::fragment("alice#", "alice%23")] + #[case::guest_principal_name( + "alice_example.com#EXT#@contoso.com", + "alice_example.com%23EXT%23@contoso.com" + )] + fn append_encodes_a_hostile_segment(#[case] segment: &str, #[case] expected: &str) { + let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); + let url = append_path_segments(&base, &[segment]).expect("valid segment"); + + assert_eq!( + url.as_str(), + format!("http://keycloak:8080/users/{expected}") + ); + } + + /// The URL is sent as a string and parsed again by the server, so every character has to stay + /// inside its segment after a round trip, not only in the [`Url`] built here. + #[test] + fn append_keeps_every_character_inside_its_segment() { + let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); + let characters = (' '..='~').chain(['\0', '\x1b', '\x7f', 'ä', '€', '🦀']); + + for character in characters { + let segment = format!("a{character}b"); + let url = append_path_segments(&base, &[&segment]).expect("valid segment"); + let sent = Url::parse(url.as_str()).expect("the built URL parses"); + + assert_eq!(sent, url, "{segment:?} changes in a round trip"); + assert_eq!(sent.host_str(), Some("keycloak"), "{segment:?}"); + assert_eq!(sent.query(), None, "{segment:?}"); + assert_eq!(sent.fragment(), None, "{segment:?}"); + let sent_segments: Vec<&str> = sent.path_segments().into_iter().flatten().collect(); + let [_, sent_segment] = sent_segments.as_slice() else { + panic!("{segment:?} splits the path into {sent_segments:?}"); + }; + assert_eq!( + percent_decode_str(sent_segment).decode_utf8_lossy(), + segment, + "{segment:?} changes on its way to the server" + ); + } + } + + #[rstest] + #[case::empty("")] + #[case::current(".")] + #[case::parent("..")] + #[case::tab("ad\tmin")] + #[case::line_feed("ad\nmin")] + #[case::carriage_return("ad\rmin")] + fn append_rejects_a_segment_the_url_would_change(#[case] segment: &str) { + let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); + let error = append_path_segments(&base, &["admin", segment]).expect_err("rejected"); + + assert!(matches!(error, Error::InvalidPathSegment { .. }), "{error}"); + } + + #[test] + fn append_rejects_a_base_without_a_path() { + let base = Url::parse("mailto:admin@example.com").expect("valid base URL"); + let error = append_path_segments(&base, &["users"]).expect_err("rejected"); + + assert!(matches!(error, Error::CannotBeABase { .. }), "{error}"); + } +} diff --git a/rust/user-info-fetcher/Cargo.toml b/rust/user-info-fetcher/Cargo.toml index eb87ff70..2a3bee60 100644 --- a/rust/user-info-fetcher/Cargo.toml +++ b/rust/user-info-fetcher/Cargo.toml @@ -35,6 +35,7 @@ url.workspace = true uuid.workspace = true [dev-dependencies] +percent-encoding.workspace = true wiremock.workspace = true [build-dependencies] diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index 0a630161..7bdb0307 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -7,6 +7,7 @@ use info_fetcher_commons::utils::{ http::{is_unauthorized, send_json_request}, secret::Secret, token::{CachedToken, MintedToken, OAuthResponse}, + url_path::append_path_segments, }; use serde::Deserialize; use snafu::{ResultExt, Snafu}; @@ -54,6 +55,12 @@ pub enum Error { endpoint: String, }, + #[snafu(display("failed to construct the Graph URL for user {user:?}"))] + ConstructUserUrl { + source: utils::url_path::Error, + user: String, + }, + #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -77,6 +84,11 @@ impl http_error::Error for Error { Self::UserNotFoundByName { .. } => StatusCode::NOT_FOUND, Self::RequestUserGroups { .. } => StatusCode::BAD_GATEWAY, Self::BuildEntraEndpoint { .. } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { + source: utils::url_path::Error::InvalidPathSegment { .. }, + .. + } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -243,7 +255,7 @@ impl ResolvedEntraBackend { let user_id = req.id.clone(); send_json_request::( self.http_client - .get(entra_backend.user_info(&user_id)) + .get(entra_backend.user_info(&user_id)?) .bearer_auth(access_token.expose()), ) .await @@ -260,7 +272,7 @@ impl ResolvedEntraBackend { let username = req.username.clone(); send_json_request::( self.http_client - .get(entra_backend.user_info(&username)) + .get(entra_backend.user_info(&username)?) .bearer_auth(access_token.expose()), ) .await @@ -274,7 +286,7 @@ impl ResolvedEntraBackend { }; let mut groups = Vec::new(); - let mut next_url = Some(entra_backend.group_info(&user_info.id)); + let mut next_url = Some(entra_backend.group_info(&user_info.id)?); let mut pages_remaining = MAX_GROUP_PAGES; while let Some(url) = next_url { @@ -356,22 +368,24 @@ impl EntraBackend { } // Works both with id/oid and userPrincipalName - pub fn user_info(&self, user: &str) -> Url { - let mut user_info_url = self.user_info_endpoint_url.clone(); - user_info_url.set_path(&format!("/v1.0/users/{user}")); - user_info_url + pub fn user_info(&self, user: &str) -> Result { + append_path_segments(&self.user_info_endpoint_url, &["v1.0", "users", user]) + .context(ConstructUserUrlSnafu { user }) } /// Requests the first page of the user's directory memberships. /// /// `memberOf` returns every directory object the user belongs to: groups as well as directory /// roles and administrative units. - pub fn group_info(&self, user: &str) -> Url { - let mut user_info_url = self.user_info_endpoint_url.clone(); - user_info_url.set_path(&format!("/v1.0/users/{user}/memberOf")); + pub fn group_info(&self, user: &str) -> Result { + let mut group_info_url = append_path_segments( + &self.user_info_endpoint_url, + &["v1.0", "users", user, "memberOf"], + ) + .context(ConstructUserUrlSnafu { user })?; // 999 is the largest page size Graph accepts, and keeps the number of round-trips down. - user_info_url.set_query(Some("$select=displayName&$top=999")); - user_info_url + group_info_url.set_query(Some("$select=displayName&$top=999")); + Ok(group_info_url) } /// Rebases an `@odata.nextLink` onto the configured user info endpoint. @@ -402,7 +416,10 @@ mod tests { }; use super::*; - use crate::UserInfoRequestById; + use crate::{ + UserInfoRequestById, + backend::test_mocks::{HOSTILE_USERS, assert_looked_up_below, received_lookups}, + }; const TENANT_ID: &str = "1234-5678-1234-5678"; const USER_ID: &str = "8765-4321-8765-4321"; @@ -479,10 +496,13 @@ mod tests { .await; } - async fn look_up_by_id(backend: &ResolvedEntraBackend) -> Result { + async fn look_up_by_id( + backend: &ResolvedEntraBackend, + id: &str, + ) -> Result { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { - id: USER_ID.to_owned(), + id: id.to_owned(), })) .await } @@ -507,7 +527,7 @@ mod tests { let mock_server = MockServer::start().await; mock_token_and_failing_user(&mock_server, USER_ID, 404).await; - let error = look_up_by_id(&backend_for(&mock_server)) + let error = look_up_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a user Entra does not have cannot be looked up"); @@ -527,7 +547,7 @@ mod tests { let mock_server = MockServer::start().await; mock_token_and_failing_user(&mock_server, USER_ID, status).await; - let error = look_up_by_id(&backend_for(&mock_server)) + let error = look_up_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a failing lookup surfaces as an error"); @@ -560,6 +580,79 @@ mod tests { ); } + /// Mounts the token endpoint plus an answer of `404` to every lookup. + async fn mock_token_and_no_users(mock_server: &MockServer) { + mock_token(mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(404)) + .mount(mock_server) + .await; + } + + /// A request-supplied id stays one segment under `/v1.0/users/`, so `../` cannot reach another + /// Graph endpoint with the access token. + #[tokio::test] + async fn entra_keeps_a_hostile_id_inside_the_users_endpoint() { + for &user_id in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token_and_no_users(&mock_server).await; + + let error = look_up_by_id(&backend_for(&mock_server), user_id) + .await + .expect_err("Entra does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundById { .. }), + "for {user_id:?} got {error}" + ); + assert_looked_up_below(&mock_server, "/v1.0/users/", user_id).await; + } + } + + /// Graph takes the principal name in the same path segment as the id, so it needs the same + /// protection. + #[tokio::test] + async fn entra_keeps_a_hostile_username_inside_the_users_endpoint() { + for &username in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token_and_no_users(&mock_server).await; + + let error = look_up_by_name(&backend_for(&mock_server), username) + .await + .expect_err("Entra does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundByName { .. }), + "for {username:?} got {error}" + ); + assert_looked_up_below(&mock_server, "/v1.0/users/", username).await; + } + } + + /// An empty, `.` or `..` user would be dropped from the path, turning the lookup of one user + /// into a listing of all of them, and a line break would be stripped, looking up another user. + #[tokio::test] + async fn entra_rejects_a_user_that_cannot_be_a_path_segment() { + for username in ["", ".", "..", "ad\nmin"] { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + + let error = look_up_by_name(&backend_for(&mock_server), username) + .await + .expect_err("the user is rejected before Entra is asked"); + + assert!( + matches!(error, Error::ConstructUserUrl { .. }), + "for {username:?} got {error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_REQUEST + ); + assert!(received_lookups(&mock_server).await.is_empty()); + } + } + async fn get_user_info_by_id(backend: &ResolvedEntraBackend) -> UserInfo { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { @@ -723,11 +816,11 @@ mod tests { .unwrap() ); assert_eq!( - entra.user_info(user), + entra.user_info(user).expect("valid user"), Url::parse(&format!("https://graph.microsoft.com/v1.0/users/{user}")).unwrap() ); assert_eq!( - entra.group_info(user), + entra.group_info(user).expect("valid user"), Url::parse(&format!( "https://graph.microsoft.com/v1.0/users/{user}/memberOf?$select=displayName&$top=999" )) @@ -763,11 +856,11 @@ mod tests { .unwrap() ); assert_eq!( - entra.user_info(user), + entra.user_info(user).expect("valid user"), Url::parse(&format!("http://graph.mock.com:8080/v1.0/users/{user}")).unwrap() ); assert_eq!( - entra.group_info(user), + entra.group_info(user).expect("valid user"), Url::parse(&format!( "http://graph.mock.com:8080/v1.0/users/{user}/memberOf?$select=displayName&$top=999" )) diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index 0c4f440e..15ade271 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -7,6 +7,7 @@ use info_fetcher_commons::utils::{ http::{is_unauthorized, send_json_request}, secret::Secret, token::{CachedToken, MintedToken, OAuthResponse}, + url_path::append_path_segments, }; use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; @@ -52,7 +53,13 @@ pub enum Error { ParseOidcEndpointUrl { source: oidc::v1alpha1::Error }, #[snafu(display("failed to construct OIDC endpoint path"))] - ConstructOidcEndpointPath { source: url::ParseError }, + ConstructOidcEndpointPath { source: utils::url_path::Error }, + + #[snafu(display("failed to construct the Keycloak URL for user id {user_id:?}"))] + ConstructUserUrl { + source: utils::url_path::Error, + user_id: String, + }, #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -79,6 +86,11 @@ impl http_error::Error for Error { Self::RequestUserGroups { .. } => StatusCode::BAD_GATEWAY, Self::ParseOidcEndpointUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructOidcEndpointPath { .. } => StatusCode::INTERNAL_SERVER_ERROR, + Self::ConstructUserUrl { + source: utils::url_path::Error::InvalidPathSegment { .. }, + .. + } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -216,16 +228,15 @@ impl ResolvedKeycloakBackend { client_secret: Secret, ) -> Result { let admin_realm = &self.config.admin_realm; + let token_url = append_path_segments( + keycloak_url, + &["realms", admin_realm, "protocol", "openid-connect", "token"], + ) + .context(ConstructOidcEndpointPathSnafu)?; let response = send_json_request::( self.http_client - .post( - keycloak_url - .join(&format!( - "realms/{admin_realm}/protocol/openid-connect/token" - )) - .context(ConstructOidcEndpointPathSnafu)?, - ) + .post(token_url) .basic_auth(&self.client_id, Some(client_secret.expose())) .form(&[("grant_type", "client_credentials")]), ) @@ -244,21 +255,19 @@ impl ResolvedKeycloakBackend { access_token: Secret, ) -> Result { let user_realm = &self.config.user_realm; - - let users_base_url = keycloak_url - .join(&format!("admin/realms/{user_realm}/users/")) - .context(ConstructOidcEndpointPathSnafu)?; + let users_url = + append_path_segments(keycloak_url, &["admin", "realms", user_realm, "users"]) + .context(ConstructOidcEndpointPathSnafu)?; let user_info = match req { UserInfoRequest::UserInfoRequestById(req) => { let user_id = req.id.clone(); + let user_url = append_path_segments(&users_url, &[&user_id]) + .context(ConstructUserUrlSnafu { user_id: &user_id })?; + send_json_request::( self.http_client - .get( - users_base_url - .join(&req.id) - .context(ConstructOidcEndpointPathSnafu)?, - ) + .get(user_url) .bearer_auth(access_token.expose()), ) .await @@ -273,13 +282,10 @@ impl ResolvedKeycloakBackend { } UserInfoRequest::UserInfoRequestByName(req) => { let username = &req.username; - let users_url = users_base_url - .join(&format!("?username={username}&exact=true")) - .context(ConstructOidcEndpointPathSnafu)?; - let users = send_json_request::>( self.http_client - .get(users_url) + .get(users_url.clone()) + .query(&[("username", username.as_str()), ("exact", "true")]) .bearer_auth(access_token.expose()), ) .await @@ -296,13 +302,15 @@ impl ResolvedKeycloakBackend { } }; + let groups_url = append_path_segments(&users_url, &[&user_info.id, "groups"]).context( + ConstructUserUrlSnafu { + user_id: &user_info.id, + }, + )?; + let groups = send_json_request::>( self.http_client - .get( - users_base_url - .join(&format!("{}/groups", user_info.id)) - .context(ConstructOidcEndpointPathSnafu)?, - ) + .get(groups_url) .bearer_auth(access_token.expose()), ) .await @@ -334,7 +342,13 @@ mod tests { }; use super::*; - use crate::{UserInfoRequestById, backend::keycloak::ResolvedKeycloakBackend}; + use crate::{ + UserInfoRequestById, UserInfoRequestByName, + backend::{ + keycloak::ResolvedKeycloakBackend, + test_mocks::{HOSTILE_USERS, assert_looked_up_below, received_lookups}, + }, + }; const ADMIN_REALM: &str = "master"; const USER_REALM: &str = "my-realm"; @@ -395,14 +409,28 @@ mod tests { async fn get_user_info_by_id( backend: &ResolvedKeycloakBackend, + id: &str, ) -> Result { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { - id: USER_ID.to_owned(), + id: id.to_owned(), })) .await } + async fn get_user_info_by_name( + backend: &ResolvedKeycloakBackend, + username: &str, + ) -> Result { + backend + .get_user_info(&UserInfoRequest::UserInfoRequestByName( + UserInfoRequestByName { + username: username.to_owned(), + }, + )) + .await + } + /// The access token was minted for every single lookup, doubling the round trips. Keycloak reports /// how long it is valid for, so it only has to be minted once. #[tokio::test] @@ -412,8 +440,12 @@ mod tests { mock_user_and_groups(&mock_server, 200).await; let backend = backend_for(&mock_server); - get_user_info_by_id(&backend).await.expect("lookup works"); - get_user_info_by_id(&backend).await.expect("lookup works"); + get_user_info_by_id(&backend, USER_ID) + .await + .expect("lookup works"); + get_user_info_by_id(&backend, USER_ID) + .await + .expect("lookup works"); // The token endpoint's `.expect(1)` is verified when `mock_server` is dropped. } @@ -426,7 +458,7 @@ mod tests { mock_token(&mock_server, Some(3600), 1).await; mock_user_and_groups(&mock_server, 404).await; - let error = get_user_info_by_id(&backend_for(&mock_server)) + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a user Keycloak does not have cannot be looked up"); @@ -447,7 +479,7 @@ mod tests { mock_token(&mock_server, Some(3600), 1).await; mock_user_and_groups(&mock_server, status).await; - let error = get_user_info_by_id(&backend_for(&mock_server)) + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a failing lookup surfaces as an error"); @@ -462,6 +494,137 @@ mod tests { } } + /// A request-supplied id stays one segment under `users/`. + /// An absolute URL must never replace the host. + /// And `../` is also not allowed to point the admin token at another endpoint. + #[tokio::test] + async fn keycloak_keeps_a_hostile_id_inside_the_users_endpoint() { + for &user_id in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(404)) + .mount(&mock_server) + .await; + + let error = get_user_info_by_id(&backend_for(&mock_server), user_id) + .await + .expect_err("Keycloak does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundById { .. }), + "for {user_id:?} got {error}" + ); + let users_path = format!("/admin/realms/{USER_REALM}/users/"); + assert_looked_up_below(&mock_server, &users_path, user_id).await; + } + } + + /// A request-supplied username stays the value of the `username` query parameter, so it cannot + /// switch off the exact match or add parameters. + #[tokio::test] + async fn keycloak_keeps_a_hostile_username_inside_its_query_parameter() { + for &username in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&mock_server) + .await; + + let error = get_user_info_by_name(&backend_for(&mock_server), username) + .await + .expect_err("Keycloak does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundByName { .. }), + "for {username:?} got {error}" + ); + let lookups = received_lookups(&mock_server).await; + let [lookup] = lookups.as_slice() else { + panic!("{username:?} was not looked up exactly once: {lookups:?}"); + }; + let query: Vec<(String, String)> = lookup.url.query_pairs().into_owned().collect(); + assert_eq!( + lookup.url.path(), + format!("/admin/realms/{USER_REALM}/users"), + "{username:?}" + ); + assert_eq!( + query, + [ + ("username".to_owned(), username.to_owned()), + ("exact".to_owned(), "true".to_owned()), + ], + "{username:?}" + ); + } + } + + /// An empty, `.` or `..` id would be dropped from the path, turning the lookup of one user into a + /// listing of all of them, and a line break would be stripped, looking up another user. + #[tokio::test] + async fn keycloak_rejects_an_id_that_cannot_be_a_path_segment() { + for user_id in ["", ".", "..", "ad\nmin"] { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + + let error = get_user_info_by_id(&backend_for(&mock_server), user_id) + .await + .expect_err("the id is rejected before Keycloak is asked"); + + assert!( + matches!(error, Error::ConstructUserUrl { .. }), + "for {user_id:?} got {error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_REQUEST + ); + assert!(received_lookups(&mock_server).await.is_empty()); + } + } + + /// A root path without a trailing slash is kept, so a Keycloak under `/auth` is asked at + /// `/auth/realms/...`. + #[tokio::test] + async fn keycloak_keeps_the_root_path() { + let mock_server = MockServer::start().await; + crate::backend::test_mocks::mock_token( + &mock_server, + format!("/auth/realms/{ADMIN_REALM}/protocol/openid-connect/token"), + Some(3600), + Some(1), + ) + .await; + Mock::given(method("GET")) + .and(path(format!( + "/auth/admin/realms/{USER_REALM}/users/{USER_ID}" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": USER_ID, + "username": "alice", + }))) + .expect(1) + .mount(&mock_server) + .await; + Mock::given(method("GET")) + .and(path(format!( + "/auth/admin/realms/{USER_REALM}/users/{USER_ID}/groups" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .expect(1) + .mount(&mock_server) + .await; + + let mut backend = backend_for(&mock_server); + backend.config.root_path = "/auth".to_owned(); + + get_user_info_by_id(&backend, USER_ID) + .await + .expect("Keycloak is reached under its root path"); + } + /// A token can stop being accepted before it expires, e.g. by being revoked. The rejection is the /// only way to find that out, so it has to trigger exactly one re-authentication. Not none /// (the lookup would keep failing until the token expired), and not a retry loop. @@ -471,7 +634,7 @@ mod tests { mock_token(&mock_server, Some(3600), 2).await; mock_user_and_groups(&mock_server, 401).await; - let error = get_user_info_by_id(&backend_for(&mock_server)) + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a permanently rejected token must surface as an error"); diff --git a/rust/user-info-fetcher/src/backend/mod.rs b/rust/user-info-fetcher/src/backend/mod.rs index f4198765..5771e471 100644 --- a/rust/user-info-fetcher/src/backend/mod.rs +++ b/rust/user-info-fetcher/src/backend/mod.rs @@ -6,11 +6,70 @@ pub mod xfsc_aas; #[cfg(test)] mod test_mocks { + use percent_encoding::percent_decode_str; use wiremock::{ - Mock, MockServer, ResponseTemplate, + Mock, MockServer, Request, ResponseTemplate, matchers::{method, path}, }; + /// User ids and names that address something other than the user when put into a URL as text: + /// another host, another path or extra query parameters. + pub(super) const HOSTILE_USERS: &[&str] = &[ + "https://evil.example.com/", + "//evil.example.com", + "../../master", + "..%2F..%2Fmaster", + "%2e%2e/%2e%2e/master", + "..\\..\\master", + "alice/groups", + "alice?briefRepresentation=true", + "alice&exact=false", + "alice#fragment", + "alice;x=1", + "alice@example.com", + ]; + + /// The `GET` requests `mock_server` received, leaving out the token requests. + pub(super) async fn received_lookups(mock_server: &MockServer) -> Vec { + let requests = mock_server + .received_requests() + .await + .expect("request recording is enabled by default"); + + requests + .into_iter() + .filter(|request| request.method.as_str() == "GET") + .collect() + } + + /// Asserts that `mock_server` received a single lookup, addressed to exactly one path segment + /// below `parent_path` that decodes to `user`. + pub(super) async fn assert_looked_up_below( + mock_server: &MockServer, + parent_path: &str, + user: &str, + ) { + let lookups = received_lookups(mock_server).await; + let [lookup] = lookups.as_slice() else { + panic!("{user:?} was not looked up exactly once: {lookups:?}"); + }; + + let path = lookup.url.path(); + let segment = path + .strip_prefix(parent_path) + .unwrap_or_else(|| panic!("{user:?} was looked up at {path}, outside {parent_path}")); + + assert!( + !segment.contains('/'), + "{user:?} was looked up at {path}, more than one segment below {parent_path}" + ); + assert_eq!( + percent_decode_str(segment).decode_utf8_lossy(), + user, + "{user:?} was looked up at {path}" + ); + } + /// Mounts an OAuth2 `client_credentials` token endpoint at `token_path`, answering with a token /// that is valid for `expires_in` seconds. Without an `expires_in` the token is deliberately not /// cached, see [`info_fetcher_commons::utils::token::CachedToken`]. From a81cb0b7cdc3af158b187caac844f6599fbd84ff Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Fri, 23 Oct 2026 09:08:11 +0200 Subject: [PATCH 2/8] chore: Regenerate Cargo.nix --- Cargo.nix | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Cargo.nix b/Cargo.nix index d3f68a88..034fa926 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -5536,12 +5536,24 @@ rec { name = "tracing"; packageId = "tracing"; } + { + name = "url"; + packageId = "url"; + } ]; devDependencies = [ { name = "futures"; packageId = "futures"; } + { + name = "percent-encoding"; + packageId = "percent-encoding"; + } + { + name = "rstest"; + packageId = "rstest"; + } ]; }; @@ -12603,6 +12615,10 @@ rec { } ]; devDependencies = [ + { + name = "percent-encoding"; + packageId = "percent-encoding"; + } { name = "wiremock"; packageId = "wiremock"; From e353540b47f1d8bce64ee7e35a2f436d4b7da0b7 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Fri, 23 Oct 2026 09:55:13 +0200 Subject: [PATCH 3/8] chore: Bump rustls-webpki to 0.103.16 --- Cargo.lock | 6 +++--- Cargo.nix | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f33216fd..d5c54628 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3248,9 +3248,9 @@ checksum = "eec689c0bc40ff2458a5977b6619cb718087084a18e02a131c599b62d05e1a5f" [[package]] name = "rustls-webpki" -version = "0.103.15" +version = "0.103.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" +checksum = "4443b1b6d39d7c663139942cc4558262562d2c84123bb6282ef177009ca8b281" dependencies = [ "aws-lc-rs", "ring", @@ -4111,7 +4111,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.3", + "getrandom 0.3.4", "once_cell", "rustix", "windows-sys 0.61.2", diff --git a/Cargo.nix b/Cargo.nix index 034fa926..0c00baac 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -10794,9 +10794,9 @@ rec { }; "rustls-webpki" = rec { crateName = "rustls-webpki"; - version = "0.103.15"; + version = "0.103.16"; edition = "2021"; - sha256 = "1hhanq3lz384v4nccacnjfwsyy99n3yc6m6iw8kljz8yicfwzhzk"; + sha256 = "10djm2f00xzi5qlbcfqjhhn2smk2h9aw8b4l74qncz4xsfvb2hs4"; libName = "webpki"; dependencies = [ { @@ -13687,7 +13687,7 @@ rec { } { name = "getrandom"; - packageId = "getrandom 0.4.3"; + packageId = "getrandom 0.3.4"; optional = true; usesDefaultFeatures = false; target = { target, features }: ((target."unix" or false) || (target."windows" or false) || ("wasi" == target."os" or null)); From 4fead7ac0fd5d80d71ad58d874f8e30e395b385b Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 6 Nov 2026 11:18:40 +0100 Subject: [PATCH 4/8] chore: Use UrlExt::join_safe from stackable-operator --- Cargo.lock | 3 - Cargo.nix | 12 -- Cargo.toml | 1 + rust/info-fetcher-commons/Cargo.toml | 3 - rust/info-fetcher-commons/src/utils/mod.rs | 1 - .../src/utils/url_path.rs | 133 ------------------ rust/user-info-fetcher/src/backend/entra.rs | 22 +-- .../user-info-fetcher/src/backend/keycloak.rs | 39 ++--- 8 files changed, 33 insertions(+), 181 deletions(-) delete mode 100644 rust/info-fetcher-commons/src/utils/url_path.rs diff --git a/Cargo.lock b/Cargo.lock index a4510d2c..3e1065d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1700,9 +1700,7 @@ dependencies = [ "hyper", "moka", "native-tls", - "percent-encoding", "reqwest", - "rstest", "rustls-pki-types", "serde", "serde_json", @@ -1711,7 +1709,6 @@ dependencies = [ "stackable-operator", "tokio", "tracing", - "url", ] [[package]] diff --git a/Cargo.nix b/Cargo.nix index db153e51..6eb97dbb 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -5681,24 +5681,12 @@ rec { name = "tracing"; packageId = "tracing"; } - { - name = "url"; - packageId = "url"; - } ]; devDependencies = [ { name = "futures"; packageId = "futures"; } - { - name = "percent-encoding"; - packageId = "percent-encoding"; - } - { - name = "rstest"; - packageId = "rstest"; - } ]; }; diff --git a/Cargo.toml b/Cargo.toml index 42870f1d..a0063b1e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ ldap3 = { version = "0.12", features = ["gssapi", "tls"] } md5 = "0.8" moka = { version = "0.12", default-features = false, features = ["future"] } native-tls = "0.2" +percent-encoding = "2.3" pin-project = "1.1" # `default-features = false` drops reqwest 0.13's `default-tls = ["rustls"]`; we select # `native-tls` (OpenSSL) to stay on the same TLS stack as ldap3 (see utils/tls.rs). The diff --git a/rust/info-fetcher-commons/Cargo.toml b/rust/info-fetcher-commons/Cargo.toml index 3909cbba..bb8e82e7 100644 --- a/rust/info-fetcher-commons/Cargo.toml +++ b/rust/info-fetcher-commons/Cargo.toml @@ -23,9 +23,6 @@ serde_json.workspace = true snafu.workspace = true tracing.workspace = true tokio.workspace = true -url.workspace = true [dev-dependencies] futures.workspace = true -percent-encoding.workspace = true -rstest.workspace = true diff --git a/rust/info-fetcher-commons/src/utils/mod.rs b/rust/info-fetcher-commons/src/utils/mod.rs index 8045b5d7..46cf2b0a 100644 --- a/rust/info-fetcher-commons/src/utils/mod.rs +++ b/rust/info-fetcher-commons/src/utils/mod.rs @@ -3,4 +3,3 @@ pub mod http; pub mod secret; pub mod tls; pub mod token; -pub mod url_path; diff --git a/rust/info-fetcher-commons/src/utils/url_path.rs b/rust/info-fetcher-commons/src/utils/url_path.rs deleted file mode 100644 index def99793..00000000 --- a/rust/info-fetcher-commons/src/utils/url_path.rs +++ /dev/null @@ -1,133 +0,0 @@ -use snafu::Snafu; -use url::Url; - -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("unable to use {segment:?} as a URL path segment"))] - InvalidPathSegment { segment: String }, - - #[snafu(display("unable to append path segments to {url:?}, as it cannot be a base"))] - CannotBeABase { url: Url }, -} - -/// Returns `base` with each of `segments` appended as exactly one path segment. -/// -/// Use this rather than [`Url::join`] or [`Url::set_path`] to create URLs from user/untrusted input. -/// Both those methods follow the specs but are easy to use insecurely because e.g. if you provide -/// an absolute URL they replace the host from `base` and a few other things (`../` treatment). -/// -/// Fails on an empty, `.` or `..` segment and on a segment containing a tab or line break. -pub fn append_path_segments(base: &Url, segments: &[&str]) -> Result { - let invalid_segment = segments.iter().find(|segment| { - matches!(**segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) - }); - if let Some(segment) = invalid_segment { - return InvalidPathSegmentSnafu { segment: *segment }.fail(); - } - - let mut url = base.clone(); - url.path_segments_mut() - .map_err(|()| CannotBeABaseSnafu { url: base.clone() }.build())? - .pop_if_empty() - .extend(segments); - - Ok(url) -} - -#[cfg(test)] -mod tests { - use percent_encoding::percent_decode_str; - use rstest::rstest; - - use super::*; - - #[rstest] - #[case::root("http://keycloak:8080", "http://keycloak:8080/realms/my-realm")] - #[case::root_with_slash("http://keycloak:8080/", "http://keycloak:8080/realms/my-realm")] - #[case::root_path( - "http://keycloak:8080/auth", - "http://keycloak:8080/auth/realms/my-realm" - )] - #[case::root_path_with_slash( - "http://keycloak:8080/auth/", - "http://keycloak:8080/auth/realms/my-realm" - )] - fn append_keeps_the_base_path(#[case] base: &str, #[case] expected: &str) { - let base = Url::parse(base).expect("valid base URL"); - let url = append_path_segments(&base, &["realms", "my-realm"]).expect("valid segments"); - - assert_eq!(url.as_str(), expected); - } - - #[rstest] - #[case::absolute_url("https://evil.example.com/", "https:%2F%2Fevil.example.com%2F")] - #[case::protocol_relative("//evil.example.com", "%2F%2Fevil.example.com")] - #[case::traversal("../../realms/master", "..%2F..%2Frealms%2Fmaster")] - #[case::percent_encoded_dots("%2e%2e", "%252e%252e")] - #[case::backslash("..\\admin", "..%5Cadmin")] - #[case::query("alice?exact=false", "alice%3Fexact=false")] - #[case::fragment("alice#", "alice%23")] - #[case::guest_principal_name( - "alice_example.com#EXT#@contoso.com", - "alice_example.com%23EXT%23@contoso.com" - )] - fn append_encodes_a_hostile_segment(#[case] segment: &str, #[case] expected: &str) { - let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); - let url = append_path_segments(&base, &[segment]).expect("valid segment"); - - assert_eq!( - url.as_str(), - format!("http://keycloak:8080/users/{expected}") - ); - } - - /// The URL is sent as a string and parsed again by the server, so every character has to stay - /// inside its segment after a round trip, not only in the [`Url`] built here. - #[test] - fn append_keeps_every_character_inside_its_segment() { - let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); - let characters = (' '..='~').chain(['\0', '\x1b', '\x7f', 'ä', '€', '🦀']); - - for character in characters { - let segment = format!("a{character}b"); - let url = append_path_segments(&base, &[&segment]).expect("valid segment"); - let sent = Url::parse(url.as_str()).expect("the built URL parses"); - - assert_eq!(sent, url, "{segment:?} changes in a round trip"); - assert_eq!(sent.host_str(), Some("keycloak"), "{segment:?}"); - assert_eq!(sent.query(), None, "{segment:?}"); - assert_eq!(sent.fragment(), None, "{segment:?}"); - let sent_segments: Vec<&str> = sent.path_segments().into_iter().flatten().collect(); - let [_, sent_segment] = sent_segments.as_slice() else { - panic!("{segment:?} splits the path into {sent_segments:?}"); - }; - assert_eq!( - percent_decode_str(sent_segment).decode_utf8_lossy(), - segment, - "{segment:?} changes on its way to the server" - ); - } - } - - #[rstest] - #[case::empty("")] - #[case::current(".")] - #[case::parent("..")] - #[case::tab("ad\tmin")] - #[case::line_feed("ad\nmin")] - #[case::carriage_return("ad\rmin")] - fn append_rejects_a_segment_the_url_would_change(#[case] segment: &str) { - let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); - let error = append_path_segments(&base, &["admin", segment]).expect_err("rejected"); - - assert!(matches!(error, Error::InvalidPathSegment { .. }), "{error}"); - } - - #[test] - fn append_rejects_a_base_without_a_path() { - let base = Url::parse("mailto:admin@example.com").expect("valid base URL"); - let error = append_path_segments(&base, &["users"]).expect_err("rejected"); - - assert!(matches!(error, Error::CannotBeABase { .. }), "{error}"); - } -} diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index 7bdb0307..d0f82c44 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -7,12 +7,14 @@ use info_fetcher_commons::utils::{ http::{is_unauthorized, send_json_request}, secret::Secret, token::{CachedToken, MintedToken, OAuthResponse}, - url_path::append_path_segments, }; use serde::Deserialize; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::user_info_fetcher::v1alpha2; -use stackable_operator::commons::{networking::HostName, tls_verification::TlsClientDetails}; +use stackable_operator::{ + commons::{networking::HostName, tls_verification::TlsClientDetails}, + utils::UrlExt, +}; use url::Url; use crate::{UserInfo, UserInfoRequest, http_error}; @@ -57,7 +59,7 @@ pub enum Error { #[snafu(display("failed to construct the Graph URL for user {user:?}"))] ConstructUserUrl { - source: utils::url_path::Error, + source: stackable_operator::utils::UrlError, user: String, }, @@ -85,7 +87,7 @@ impl http_error::Error for Error { Self::RequestUserGroups { .. } => StatusCode::BAD_GATEWAY, Self::BuildEntraEndpoint { .. } => StatusCode::BAD_REQUEST, Self::ConstructUserUrl { - source: utils::url_path::Error::InvalidPathSegment { .. }, + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, .. } => StatusCode::BAD_REQUEST, Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, @@ -369,7 +371,8 @@ impl EntraBackend { // Works both with id/oid and userPrincipalName pub fn user_info(&self, user: &str) -> Result { - append_path_segments(&self.user_info_endpoint_url, &["v1.0", "users", user]) + self.user_info_endpoint_url + .join_safe(&["v1.0", "users", user]) .context(ConstructUserUrlSnafu { user }) } @@ -378,11 +381,10 @@ impl EntraBackend { /// `memberOf` returns every directory object the user belongs to: groups as well as directory /// roles and administrative units. pub fn group_info(&self, user: &str) -> Result { - let mut group_info_url = append_path_segments( - &self.user_info_endpoint_url, - &["v1.0", "users", user, "memberOf"], - ) - .context(ConstructUserUrlSnafu { user })?; + let mut group_info_url = self + .user_info_endpoint_url + .join_safe(&["v1.0", "users", user, "memberOf"]) + .context(ConstructUserUrlSnafu { user })?; // 999 is the largest page size Graph accepts, and keeps the number of round-trips down. group_info_url.set_query(Some("$select=displayName&$top=999")); Ok(group_info_url) diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index 15ade271..d2e1f613 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -7,12 +7,11 @@ use info_fetcher_commons::utils::{ http::{is_unauthorized, send_json_request}, secret::Secret, token::{CachedToken, MintedToken, OAuthResponse}, - url_path::append_path_segments, }; use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_opa_operator::crd::user_info_fetcher::v1alpha2; -use stackable_operator::crd::authentication::oidc; +use stackable_operator::{crd::authentication::oidc, utils::UrlExt}; use url::Url; use crate::{UserInfo, UserInfoRequest, http_error}; @@ -53,11 +52,13 @@ pub enum Error { ParseOidcEndpointUrl { source: oidc::v1alpha1::Error }, #[snafu(display("failed to construct OIDC endpoint path"))] - ConstructOidcEndpointPath { source: utils::url_path::Error }, + ConstructOidcEndpointPath { + source: stackable_operator::utils::UrlError, + }, #[snafu(display("failed to construct the Keycloak URL for user id {user_id:?}"))] ConstructUserUrl { - source: utils::url_path::Error, + source: stackable_operator::utils::UrlError, user_id: String, }, @@ -87,7 +88,7 @@ impl http_error::Error for Error { Self::ParseOidcEndpointUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructOidcEndpointPath { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructUserUrl { - source: utils::url_path::Error::InvalidPathSegment { .. }, + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, .. } => StatusCode::BAD_REQUEST, Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, @@ -228,11 +229,9 @@ impl ResolvedKeycloakBackend { client_secret: Secret, ) -> Result { let admin_realm = &self.config.admin_realm; - let token_url = append_path_segments( - keycloak_url, - &["realms", admin_realm, "protocol", "openid-connect", "token"], - ) - .context(ConstructOidcEndpointPathSnafu)?; + let token_url = keycloak_url + .join_safe(&["realms", admin_realm, "protocol", "openid-connect", "token"]) + .context(ConstructOidcEndpointPathSnafu)?; let response = send_json_request::( self.http_client @@ -255,14 +254,15 @@ impl ResolvedKeycloakBackend { access_token: Secret, ) -> Result { let user_realm = &self.config.user_realm; - let users_url = - append_path_segments(keycloak_url, &["admin", "realms", user_realm, "users"]) - .context(ConstructOidcEndpointPathSnafu)?; + let users_url = keycloak_url + .join_safe(&["admin", "realms", user_realm, "users"]) + .context(ConstructOidcEndpointPathSnafu)?; let user_info = match req { UserInfoRequest::UserInfoRequestById(req) => { let user_id = req.id.clone(); - let user_url = append_path_segments(&users_url, &[&user_id]) + let user_url = users_url + .join_safe(&[&user_id]) .context(ConstructUserUrlSnafu { user_id: &user_id })?; send_json_request::( @@ -302,11 +302,12 @@ impl ResolvedKeycloakBackend { } }; - let groups_url = append_path_segments(&users_url, &[&user_info.id, "groups"]).context( - ConstructUserUrlSnafu { - user_id: &user_info.id, - }, - )?; + let groups_url = + users_url + .join_safe(&[&user_info.id, "groups"]) + .context(ConstructUserUrlSnafu { + user_id: &user_info.id, + })?; let groups = send_json_request::>( self.http_client From 7bc2cb579d17d84fbcac85da21aab67e50beb752 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 6 Nov 2026 12:13:56 +0100 Subject: [PATCH 5/8] chore: Upgrade serde-saphyr --- Cargo.lock | 6 +++--- Cargo.nix | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e1065d0..20808d3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3339,12 +3339,12 @@ dependencies = [ [[package]] name = "serde-saphyr" -version = "2.1.0" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f4dc2714fc5632417a6e979602d7044bad8f2880c63fc7217087180acec31b6" +checksum = "d43fb82d550b07e6f929a58d7d0935182602f48ddead875f5ee313c41c8f07fe" dependencies = [ "annotate-snippets", - "base64 0.23.1", + "base64 0.22.1", "encoding_rs_io", "granit-parser", "nohash-hasher", diff --git a/Cargo.nix b/Cargo.nix index 6eb97dbb..aa36bc7b 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -11470,10 +11470,10 @@ rec { }; "serde-saphyr" = rec { crateName = "serde-saphyr"; - version = "2.1.0"; + version = "2.1.1"; edition = "2024"; crateBin = []; - sha256 = "1diixjn80w882xrgqqqci3rdifj4f0nn0yg9lqbj8qy59xqw4k9z"; + sha256 = "1zh7iwfc84z3brgqgbfyips049hq6l4pv3d557wyc1qbalnvhgyl"; libName = "serde_saphyr"; license = "MIT OR Apache-2.0"; dependencies = [ @@ -11484,7 +11484,7 @@ rec { } { name = "base64"; - packageId = "base64 0.23.1"; + packageId = "base64 0.22.1"; optional = true; } { From 2bee5cba0b74b87388248f6876bb0bde22888ff0 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 6 Nov 2026 14:47:10 +0100 Subject: [PATCH 6/8] chore: Add ConstructUserGroupsUrl error case --- CHANGELOG.md | 13 +++-- rust/user-info-fetcher/src/backend/entra.rs | 51 ++++++++++++++++-- .../user-info-fetcher/src/backend/keycloak.rs | 54 ++++++++++++++++--- 3 files changed, 105 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26581016..b4e557dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,10 +92,15 @@ All notable changes to this project will be documented in this file. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#871]). - The reconciler now applies resources and derives the cluster status in discrete apply and update_status steps for the `opa_controller` ([#872]). -- The user-info-fetcher Keycloak and Entra backends now encode user ids and names as a single URL - path segment, and the Keycloak backend sends the username as a query parameter. An empty, `.` or - `..` id or name, or one containing a line break, is rejected with `400` ([#909]). -- The user-info-fetcher Keycloak backend now keeps a `rootPath` without a trailing slash ([#909]). +- The user-info-fetcher Entra backend now encodes the user id and the username as a single URL path + segment. An empty, `.` or `..` id or username, or one containing a line break, is rejected with + `400` ([#909]). +- The user-info-fetcher Keycloak backend now encodes the user id as a single URL path segment, and + sends the username as a query parameter instead of appending it to the URL as text, so that a + username can no longer add or override search parameters. An empty, `.` or `..` id, or one + containing a line break, is rejected with `400` ([#909]). +- The user-info-fetcher Keycloak backend no longer drops a configured `rootPath` from the URLs it + requests, so a Keycloak served below a sub-path, like `/auth`, can now be reached ([#909]). [#873]: https://github.com/stackabletech/opa-operator/pull/873 [#852]: https://github.com/stackabletech/opa-operator/pull/852 diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index d0f82c44..40cc262e 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -63,6 +63,14 @@ pub enum Error { user: String, }, + #[snafu(display( + "failed to construct the Graph groups URL for user id {user_id:?} returned by Entra" + ))] + ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError, + user_id: String, + }, + #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -91,6 +99,13 @@ impl http_error::Error for Error { .. } => StatusCode::BAD_REQUEST, Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, + // The groups URL is built from the user id in Graph's own response, so an id that + // cannot be a path segment is a fault of the backend and not of the caller. + Self::ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, + .. + } => StatusCode::BAD_GATEWAY, + Self::ConstructUserGroupsUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -380,11 +395,11 @@ impl EntraBackend { /// /// `memberOf` returns every directory object the user belongs to: groups as well as directory /// roles and administrative units. - pub fn group_info(&self, user: &str) -> Result { + pub fn group_info(&self, user_id: &str) -> Result { let mut group_info_url = self .user_info_endpoint_url - .join_safe(&["v1.0", "users", user, "memberOf"]) - .context(ConstructUserUrlSnafu { user })?; + .join_safe(&["v1.0", "users", user_id, "memberOf"]) + .context(ConstructUserGroupsUrlSnafu { user_id })?; // 999 is the largest page size Graph accepts, and keeps the number of round-trips down. group_info_url.set_query(Some("$select=displayName&$top=999")); Ok(group_info_url) @@ -655,6 +670,36 @@ mod tests { } } + /// The group memberships are requested for the id in Graph's own answer, so an id that cannot be + /// a path segment is a fault of the backend and must not be reported as a bad request of the + /// caller. + #[tokio::test] + async fn entra_reports_an_unusable_id_in_its_own_answer_as_a_backend_fault() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .and(path(format!("/v1.0/users/{USER_ID}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "", + "userPrincipalName": "alice@example.com", + }))) + .mount(&mock_server) + .await; + + let error = look_up_by_id(&backend_for(&mock_server), USER_ID) + .await + .expect_err("the group memberships of a user without an id cannot be requested"); + + assert!( + matches!(error, Error::ConstructUserGroupsUrl { .. }), + "{error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_GATEWAY + ); + } + async fn get_user_info_by_id(backend: &ResolvedEntraBackend) -> UserInfo { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index d2e1f613..08dbd036 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -62,6 +62,14 @@ pub enum Error { user_id: String, }, + #[snafu(display( + "failed to construct the Keycloak groups URL for user id {user_id:?} returned by Keycloak" + ))] + ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError, + user_id: String, + }, + #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -92,6 +100,13 @@ impl http_error::Error for Error { .. } => StatusCode::BAD_REQUEST, Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, + // The groups URL is built from the user id in Keycloak's own response, so an id that + // cannot be a path segment is a fault of the backend and not of the caller. + Self::ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, + .. + } => StatusCode::BAD_GATEWAY, + Self::ConstructUserGroupsUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -302,12 +317,10 @@ impl ResolvedKeycloakBackend { } }; - let groups_url = - users_url - .join_safe(&[&user_info.id, "groups"]) - .context(ConstructUserUrlSnafu { - user_id: &user_info.id, - })?; + let user_id = &user_info.id; + let groups_url = users_url + .join_safe(&[user_id, "groups"]) + .context(ConstructUserGroupsUrlSnafu { user_id })?; let groups = send_json_request::>( self.http_client @@ -586,6 +599,35 @@ mod tests { } } + /// The groups are requested for the id in Keycloak's own answer, so an id that cannot be a path + /// segment is a fault of the backend and must not be reported as a bad request of the caller. + #[tokio::test] + async fn keycloak_reports_an_unusable_id_in_its_own_answer_as_a_backend_fault() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .and(path(format!("/admin/realms/{USER_REALM}/users/{USER_ID}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "", + "username": "alice", + }))) + .mount(&mock_server) + .await; + + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) + .await + .expect_err("the groups of a user without an id cannot be requested"); + + assert!( + matches!(error, Error::ConstructUserGroupsUrl { .. }), + "{error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_GATEWAY + ); + } + /// A root path without a trailing slash is kept, so a Keycloak under `/auth` is asked at /// `/auth/realms/...`. #[tokio::test] From 1bae2bc5f0b0848625283cc8684e3b8226d3c77e Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 6 Nov 2026 14:52:21 +0100 Subject: [PATCH 7/8] test: Remove unnecessary test case --- rust/user-info-fetcher/src/backend/entra.rs | 27 +++---------------- .../user-info-fetcher/src/backend/keycloak.rs | 6 +++-- rust/user-info-fetcher/src/backend/mod.rs | 17 +++++++++++- 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index 40cc262e..12506312 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -435,7 +435,7 @@ mod tests { use super::*; use crate::{ UserInfoRequestById, - backend::test_mocks::{HOSTILE_USERS, assert_looked_up_below, received_lookups}, + backend::test_mocks::{HOSTILE_PATH_USERS, assert_looked_up_below, received_lookups}, }; const TENANT_ID: &str = "1234-5678-1234-5678"; @@ -607,10 +607,11 @@ mod tests { } /// A request-supplied id stays one segment under `/v1.0/users/`, so `../` cannot reach another - /// Graph endpoint with the access token. + /// Graph endpoint with the access token. A username needs no test of its own: Graph addresses + /// the user by id or by principal name in that very same segment. #[tokio::test] async fn entra_keeps_a_hostile_id_inside_the_users_endpoint() { - for &user_id in HOSTILE_USERS { + for &user_id in HOSTILE_PATH_USERS { let mock_server = MockServer::start().await; mock_token_and_no_users(&mock_server).await; @@ -626,26 +627,6 @@ mod tests { } } - /// Graph takes the principal name in the same path segment as the id, so it needs the same - /// protection. - #[tokio::test] - async fn entra_keeps_a_hostile_username_inside_the_users_endpoint() { - for &username in HOSTILE_USERS { - let mock_server = MockServer::start().await; - mock_token_and_no_users(&mock_server).await; - - let error = look_up_by_name(&backend_for(&mock_server), username) - .await - .expect_err("Entra does not have the user"); - - assert!( - matches!(error, Error::UserNotFoundByName { .. }), - "for {username:?} got {error}" - ); - assert_looked_up_below(&mock_server, "/v1.0/users/", username).await; - } - } - /// An empty, `.` or `..` user would be dropped from the path, turning the lookup of one user /// into a listing of all of them, and a line break would be stripped, looking up another user. #[tokio::test] diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index 08dbd036..0daf12fe 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -360,7 +360,9 @@ mod tests { UserInfoRequestById, UserInfoRequestByName, backend::{ keycloak::ResolvedKeycloakBackend, - test_mocks::{HOSTILE_USERS, assert_looked_up_below, received_lookups}, + test_mocks::{ + HOSTILE_PATH_USERS, HOSTILE_USERS, assert_looked_up_below, received_lookups, + }, }, }; @@ -513,7 +515,7 @@ mod tests { /// And `../` is also not allowed to point the admin token at another endpoint. #[tokio::test] async fn keycloak_keeps_a_hostile_id_inside_the_users_endpoint() { - for &user_id in HOSTILE_USERS { + for &user_id in HOSTILE_PATH_USERS { let mock_server = MockServer::start().await; mock_token(&mock_server, Some(3600), 1).await; Mock::given(method("GET")) diff --git a/rust/user-info-fetcher/src/backend/mod.rs b/rust/user-info-fetcher/src/backend/mod.rs index 5771e471..3958a5f9 100644 --- a/rust/user-info-fetcher/src/backend/mod.rs +++ b/rust/user-info-fetcher/src/backend/mod.rs @@ -12,8 +12,23 @@ mod test_mocks { matchers::{method, path}, }; + /// User ids and names that reach for another path or host when put into a URL as text. One per + /// mechanism is enough here: that every other shape is encoded as well is a property of + /// [`stackable_operator::utils::UrlExt::join_safe`] and is covered by its own tests. What these + /// pin down is only that the request-supplied value is put through it at all. + pub(super) const HOSTILE_PATH_USERS: &[&str] = &[ + // Would climb out of the users endpoint. + "../../master", + // Would replace the host the access token is sent to. + "https://evil.example.com/", + // Would survive a single round of escaping and be decoded by the server. + "%2e%2e/%2e%2e/master", + ]; + /// User ids and names that address something other than the user when put into a URL as text: - /// another host, another path or extra query parameters. + /// another host, another path or extra query parameters. Used where the value ends up as the + /// value of a query parameter rather than as a path segment, and is therefore escaped by + /// `reqwest` instead of by [`stackable_operator::utils::UrlExt::join_safe`]. pub(super) const HOSTILE_USERS: &[&str] = &[ "https://evil.example.com/", "//evil.example.com", From 9cccafbe80e617a8d78b68efa1c03ac7ba5ebd1d Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 6 Nov 2026 15:03:28 +0100 Subject: [PATCH 8/8] chore: Remove the dependency to percent-encoding --- Cargo.lock | 1 - Cargo.nix | 4 --- Cargo.toml | 1 - rust/user-info-fetcher/Cargo.toml | 1 - rust/user-info-fetcher/src/backend/mod.rs | 33 +++++++++++++---------- 5 files changed, 19 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 20808d3b..da9ea86e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3738,7 +3738,6 @@ dependencies = [ "krb5", "ldap3", "native-tls", - "percent-encoding", "pin-project", "reqwest", "rustls-pki-types", diff --git a/Cargo.nix b/Cargo.nix index aa36bc7b..0f24cd2b 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -12883,10 +12883,6 @@ rec { } ]; devDependencies = [ - { - name = "percent-encoding"; - packageId = "percent-encoding"; - } { name = "wiremock"; packageId = "wiremock"; diff --git a/Cargo.toml b/Cargo.toml index a0063b1e..42870f1d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,6 @@ ldap3 = { version = "0.12", features = ["gssapi", "tls"] } md5 = "0.8" moka = { version = "0.12", default-features = false, features = ["future"] } native-tls = "0.2" -percent-encoding = "2.3" pin-project = "1.1" # `default-features = false` drops reqwest 0.13's `default-tls = ["rustls"]`; we select # `native-tls` (OpenSSL) to stay on the same TLS stack as ldap3 (see utils/tls.rs). The diff --git a/rust/user-info-fetcher/Cargo.toml b/rust/user-info-fetcher/Cargo.toml index 2a3bee60..eb87ff70 100644 --- a/rust/user-info-fetcher/Cargo.toml +++ b/rust/user-info-fetcher/Cargo.toml @@ -35,7 +35,6 @@ url.workspace = true uuid.workspace = true [dev-dependencies] -percent-encoding.workspace = true wiremock.workspace = true [build-dependencies] diff --git a/rust/user-info-fetcher/src/backend/mod.rs b/rust/user-info-fetcher/src/backend/mod.rs index 3958a5f9..bb67fc76 100644 --- a/rust/user-info-fetcher/src/backend/mod.rs +++ b/rust/user-info-fetcher/src/backend/mod.rs @@ -6,7 +6,7 @@ pub mod xfsc_aas; #[cfg(test)] mod test_mocks { - use percent_encoding::percent_decode_str; + use url::Url; use wiremock::{ Mock, MockServer, Request, ResponseTemplate, matchers::{method, path}, @@ -57,8 +57,12 @@ mod test_mocks { .collect() } - /// Asserts that `mock_server` received a single lookup, addressed to exactly one path segment - /// below `parent_path` that decodes to `user`. + /// Asserts that `mock_server` received a single lookup, addressed to `user` as exactly one path + /// segment below `parent_path`. + /// + /// The expected path is spelled out with [`url`]'s own encoder rather than by decoding what was + /// received. That still catches a call site which interpolates the user into the path as text, + /// and it saves this test helper a percent-decoder of its own. pub(super) async fn assert_looked_up_below( mock_server: &MockServer, parent_path: &str, @@ -69,19 +73,20 @@ mod test_mocks { panic!("{user:?} was not looked up exactly once: {lookups:?}"); }; - let path = lookup.url.path(); - let segment = path - .strip_prefix(parent_path) - .unwrap_or_else(|| panic!("{user:?} was looked up at {path}, outside {parent_path}")); + let mut expected_url = + Url::parse("https://expected.example.com").expect("a constant URL parses"); + expected_url.set_path(parent_path); + expected_url + .path_segments_mut() + .expect("an https URL can be a base") + .pop_if_empty() + .push(user); - assert!( - !segment.contains('/'), - "{user:?} was looked up at {path}, more than one segment below {parent_path}" - ); assert_eq!( - percent_decode_str(segment).decode_utf8_lossy(), - user, - "{user:?} was looked up at {path}" + lookup.url.path(), + expected_url.path(), + "{user:?} was looked up at {}", + lookup.url.path() ); }