From e75b7bf07e6b10c2b018dd257242db49128e1a75 Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 29 Oct 2026 15:14:43 +0100 Subject: [PATCH 01/15] feat(operator)!: Add UrlExt::join_safe function This removes the unsafe and no longer needed UrlExt::join_many function. Co-authored-by: Lars Francke --- crates/stackable-operator/src/utils/url.rs | 97 ++++++++++++++-------- 1 file changed, 61 insertions(+), 36 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 81ca9356e..f6be01382 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -1,55 +1,80 @@ +use snafu::Snafu; + +#[derive(Debug, Snafu)] +pub enum UrlError { + InvalidSegment { segment: String }, + + CannotBeABase, +} + pub trait UrlExt: Sized { - /// Joins many path segments to the [`Url`][url::Url]. Note: Passing - /// segments without a trailing slash while not being the final segment - /// will **allocate** on the heap to append the missing trailing slash. - /// See for - /// more information about why the trailing slash is important. + /// Joins one or more segments to the end of the existing URL path segments. /// - /// ### Example + /// Compared to [`url::Url::join`], it protects users against (accidental) misuse (especially + /// when dealing with untrusted input) in two ways: /// - /// ``` - /// use stackable_operator::utils::UrlExt; + /// 1. The function validates that each segment is not an invalid segment. This for example + /// prevents path traversals. + /// 2. The upstream function can replace the entire base URL in these two situations: + /// - A [scheme relative special URL][1] as input replaces everything in the base URL after + /// the scheme. + /// - An absolute URL (with a scheme) as input replaces the whole base URL (even the + /// scheme). /// - /// let url = url::Url::parse("http://example.com").unwrap(); - /// let url = url.join_many(vec!["realms/", "master/", "myuser"]).unwrap(); + /// _From (Accessed 2026-10-19, + /// version 2.5.8)_ /// - /// assert_eq!(url.as_str(), "http://example.com/realms/master/myuser"); - /// ``` - fn join_many<'a>( - self, - inputs: impl IntoIterator, - ) -> Result; + /// This for example allows users to construct malicious URLs. This function constructs the + /// final URL in a way in which the segments are only appended to any existing path segments. + /// + /// [1]: https://url.spec.whatwg.org/#scheme-relative-special-url-string + fn join_safe(&mut self, segments: &[T]) -> Result<(), UrlError> + where + T: AsRef; } impl UrlExt for url::Url { - fn join_many<'a>( - self, - inputs: impl IntoIterator, - ) -> Result { - let mut iter = inputs.into_iter().peekable(); - let mut url = self; - - while let Some(input) = iter.next() { - url = if !input.ends_with('/') && iter.peek().is_some() { - url.join(&format!("{input}/"))? - } else { - url.join(input)? - }; + fn join_safe(&mut self, segments: &[T]) -> Result<(), UrlError> + where + T: AsRef, + { + let invalid_segment = segments.iter().find(|segment| { + let segment = segment.as_ref(); + matches!(segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) + }); + + if let Some(segment) = invalid_segment { + return InvalidSegmentSnafu { + segment: segment.as_ref(), + } + .fail(); } - Ok(url) + self.path_segments_mut() + .map_err(|()| CannotBeABaseSnafu.build())? + .pop_if_empty() + .extend(segments); + + Ok(()) } } #[cfg(test)] -mod test { +mod tests { + use rstest::rstest; + use super::*; - #[test] - fn url_join_many() { - let url = url::Url::parse("http://example.com").unwrap(); - let url = url.join_many(vec!["realms/", "master/", "myuser"]).unwrap(); + #[rstest] + #[case::path_slash("http://example.com/a/", "http://example.com/a/foo/bar/baz")] + #[case::path("http://example.com/a", "http://example.com/a/foo/bar/baz")] + #[case::root_slash("http://example.com/", "http://example.com/foo/bar/baz")] + #[case::root("http://example.com", "http://example.com/foo/bar/baz")] + fn join_safe_keeps_base_path(#[case] base: &str, #[case] expected: &str) { + let mut url = url::Url::parse(base).expect("base URL must parse"); + url.join_safe(&["foo", "bar", "baz"]) + .expect("must join, all segments are valid"); - assert_eq!(url.as_str(), "http://example.com/realms/master/myuser"); + assert_eq!(url.as_str(), expected); } } From 82f97693b59058a3e620ec09e1cd6f4c74816f4a Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 11:01:54 +0100 Subject: [PATCH 02/15] chore(operator): Add error messages for UrlError --- crates/stackable-operator/src/utils/url.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index f6be01382..8050cbc6e 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -2,8 +2,10 @@ use snafu::Snafu; #[derive(Debug, Snafu)] pub enum UrlError { + #[snafu(display("failed to use {segment:?} as url path segment"))] InvalidSegment { segment: String }, + #[snafu(display("failed to append path segments to url as it cannot be a base"))] CannotBeABase, } From 72bb480e0e2fbe4c929e0254d2022b93482fab60 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 11:02:38 +0100 Subject: [PATCH 03/15] test(operator): Add UrlExt::join_safe unit tests --- crates/stackable-operator/src/utils/url.rs | 59 ++++++++++++++++++++-- 1 file changed, 55 insertions(+), 4 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 8050cbc6e..92d6899d9 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -68,10 +68,10 @@ mod tests { use super::*; #[rstest] - #[case::path_slash("http://example.com/a/", "http://example.com/a/foo/bar/baz")] - #[case::path("http://example.com/a", "http://example.com/a/foo/bar/baz")] - #[case::root_slash("http://example.com/", "http://example.com/foo/bar/baz")] - #[case::root("http://example.com", "http://example.com/foo/bar/baz")] + #[case::path_slash("https://example.org/a/", "https://example.org/a/foo/bar/baz")] + #[case::path("https://example.org/a", "https://example.org/a/foo/bar/baz")] + #[case::root_slash("https://example.org/", "https://example.org/foo/bar/baz")] + #[case::root("https://example.org", "https://example.org/foo/bar/baz")] fn join_safe_keeps_base_path(#[case] base: &str, #[case] expected: &str) { let mut url = url::Url::parse(base).expect("base URL must parse"); url.join_safe(&["foo", "bar", "baz"]) @@ -79,4 +79,55 @@ mod tests { assert_eq!(url.as_str(), expected); } + + #[rstest] + #[case::absolute_url("https://evil.example.com/", "https:%2F%2Fevil.example.com%2F")] + #[case::scheme_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#@example.org", + "alice_example.com%23EXT%23@example.org" + )] + fn join_safe_appends_malicious_segment(#[case] segment: &str, #[case] expected: &str) { + let mut url = url::Url::parse("http://example.org").expect("base URL must parse"); + url.join_safe(&[segment]) + .expect("must join, all segments are valid"); + + assert_eq!(url.as_str(), format!("http://example.org/{expected}")); + } + + #[rstest] + #[case::empty("")] + #[case::current(".")] + #[case::parent("..")] + #[case::tab("ad\tmin")] + #[case::line_feed("ad\nmin")] + #[case::carriage_return("ad\rmin")] + fn join_safe_rejects_invalid_segment(#[case] segment: &str) { + let mut url = url::Url::parse("http://example.org").expect("base URL must parse"); + let error = url + .join_safe(&[segment]) + .expect_err("must be error because invalid segments are not appended"); + + assert_eq!( + error, + UrlError::InvalidSegment { + segment: segment.to_owned() + } + ); + } + + #[test] + fn join_safe_rejects_invalid_base() { + let mut url = url::Url::parse("mailto:user@example.org").expect("base URL must parse"); + let error = url + .join_safe(&["foo", "bar", "baz"]) + .expect_err("must be error because the base URL cannot be a base (opaque path state)"); + + assert_eq!(error, UrlError::CannotBeABase); + } } From 7a8fa508cd961f9258b8c3e8669bcafcf39db039 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 11:04:50 +0100 Subject: [PATCH 04/15] chore(operator): Add dev comment about cannot-be-a-base --- crates/stackable-operator/src/utils/url.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 92d6899d9..e398caa51 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -1,6 +1,6 @@ use snafu::Snafu; -#[derive(Debug, Snafu)] +#[derive(Debug, PartialEq, Eq, Snafu)] pub enum UrlError { #[snafu(display("failed to use {segment:?} as url path segment"))] InvalidSegment { segment: String }, @@ -53,6 +53,14 @@ impl UrlExt for url::Url { } self.path_segments_mut() + // A URL cannot be a base when: + // + // - its scheme is not one of the "special" schemes (http, https, ws, wss, ftp, file), and + // - it has no authority (no // after the scheme) + // + // Examples: mailto:user@example.com, javascript:alert(1) + // + // See https://url.spec.whatwg.org/#cannot-be-a-base-url-path-state .map_err(|()| CannotBeABaseSnafu.build())? .pop_if_empty() .extend(segments); From 64fcd1b4d7c459dcb4f9d7698258e896154d688a Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 11:26:05 +0100 Subject: [PATCH 05/15] docs(operator): Add doc comment for UrlExt trait --- crates/stackable-operator/src/utils/url.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index e398caa51..bcb1fdabd 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -9,6 +9,8 @@ pub enum UrlError { CannotBeABase, } +/// An extension trait for [`url::Url`], most notably providing a safe alternative for +/// [`url::Url::join`]: [`UrlExt::join_safe`]. pub trait UrlExt: Sized { /// Joins one or more segments to the end of the existing URL path segments. /// From 1dc743adaf3d09d22ab5a15faa570a103df987b9 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 12:00:17 +0100 Subject: [PATCH 06/15] refactor(operator): Collect all invalid path segements in UrlExt::join_Safe --- crates/stackable-operator/src/utils/url.rs | 55 ++++++++++++++-------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index bcb1fdabd..4f80970cd 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -2,8 +2,12 @@ use snafu::Snafu; #[derive(Debug, PartialEq, Eq, Snafu)] pub enum UrlError { - #[snafu(display("failed to use {segment:?} as url path segment"))] - InvalidSegment { segment: String }, + #[snafu(display( + "failed to use {segments} as url path segment{plural}", + segments = segments.join(", "), + plural = if segments.is_empty() { "" } else { "s" } + ))] + InvalidSegments { segments: Vec }, #[snafu(display("failed to append path segments to url as it cannot be a base"))] CannotBeABase, @@ -42,14 +46,22 @@ impl UrlExt for url::Url { where T: AsRef, { - let invalid_segment = segments.iter().find(|segment| { - let segment = segment.as_ref(); - matches!(segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) - }); - - if let Some(segment) = invalid_segment { - return InvalidSegmentSnafu { - segment: segment.as_ref(), + let mut invalid_segments = segments + .iter() + .filter(|segment| { + let segment = segment.as_ref(); + matches!(segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) + }) + // Make the iterator peekable so that we don't need to collect it here (which allocates). + // This enables us to only allocate in the error-case only - when there is at least one + // invalid path segment. + .peekable(); + + if invalid_segments.peek().is_some() { + return InvalidSegmentsSnafu { + segments: invalid_segments + .map(|s| s.as_ref().to_owned()) + .collect::>(), } .fail(); } @@ -111,22 +123,25 @@ mod tests { } #[rstest] - #[case::empty("")] - #[case::current(".")] - #[case::parent("..")] - #[case::tab("ad\tmin")] - #[case::line_feed("ad\nmin")] - #[case::carriage_return("ad\rmin")] - fn join_safe_rejects_invalid_segment(#[case] segment: &str) { + #[case::multiple(&["", ".", ".."])] + #[case::empty(&[""])] + #[case::current(&["."])] + #[case::parent(&[".."])] + #[case::tab(&["ad\tmin"])] + #[case::line_feed(&["ad\nmin"])] + #[case::carriage_return(&["ad\rmin"])] + fn join_safe_rejects_invalid_segment(#[case] segments: &[&str]) { let mut url = url::Url::parse("http://example.org").expect("base URL must parse"); let error = url - .join_safe(&[segment]) + .join_safe(segments) .expect_err("must be error because invalid segments are not appended"); assert_eq!( error, - UrlError::InvalidSegment { - segment: segment.to_owned() + UrlError::InvalidSegments { + // NOTE (@Techassi): We use to_string instead of to_owned (which we normally prefer) + // because we have a double reference (&&str) here. + segments: segments.iter().map(ToString::to_string).collect() } ); } From d0a257da937a6e8eea6e13986ea8d026865b3593 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 12:06:08 +0100 Subject: [PATCH 07/15] chore(operator): Add changelog entry --- crates/stackable-operator/CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index bd9b6115d..14c7845b0 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,13 +4,23 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- Add `UrlExt::join_safe` function to safely append path segments to a base URL ([#1319]). + ### Changed - BREAKING: Bump the RustCrypto dependencies to their new generation, which affects the re-exported `certs` and `webhook` modules. See the changelogs of `stackable-certs` and `stackable-webhook` for details ([#1315]). +### Removed + +- BREAKING: Remove the unsafe and no longer needed `UrlExt::join_many` function. Use + `UrlExt::join_safe` instead ([#1319]). + [#1315]: https://github.com/stackabletech/operator-rs/pull/1315 +[#1319]: https://github.com/stackabletech/operator-rs/pull/1319 ## [0.121.0] - 2026-10-14 From e947ac7b10db7209eb99432c6a66fba250d01be0 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 12:34:29 +0100 Subject: [PATCH 08/15] chore: Disallow Url::join function and deny disallowed_methods lint --- Cargo.toml | 1 + clippy.toml | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 81d959112..75937d414 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,6 +137,7 @@ derive_partial_eq_without_eq = "deny" unnecessary_struct_initialization = "deny" # Additional restriction lints we enforce +disallowed_methods = "deny" unwrap_in_result = "deny" unwrap_used = "deny" panic = "deny" diff --git a/clippy.toml b/clippy.toml index f69b4a67f..1bb3b8dbb 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,2 +1,8 @@ allow-unwrap-in-tests = true allow-panic-in-tests = true +disallowed-methods = [ + { path = "url::Url::join", reason = """ + Url::join is unsafe in contexts in which users have control over the to-be-joined input, because + the function will replace the original base URL in certain situations. Bad actors can craft + malicious inputs to point at an attacker controlled URL""", replacement = "join_safe" }, +] From 47249d4c4e765ea92cfc089b0914d79ef47ec519 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 13:38:43 +0100 Subject: [PATCH 09/15] refactor(operator): Let UrlExt::join_safe tale &self --- crates/stackable-operator/src/utils/url.rs | 26 +++++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 4f80970cd..2e5e2400e 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -16,7 +16,8 @@ pub enum UrlError { /// An extension trait for [`url::Url`], most notably providing a safe alternative for /// [`url::Url::join`]: [`UrlExt::join_safe`]. pub trait UrlExt: Sized { - /// Joins one or more segments to the end of the existing URL path segments. + /// Joins one or more segments to the end of the existing URL path segments and returns a new + /// [`url::Url`]. /// /// Compared to [`url::Url::join`], it protects users against (accidental) misuse (especially /// when dealing with untrusted input) in two ways: @@ -36,13 +37,13 @@ pub trait UrlExt: Sized { /// final URL in a way in which the segments are only appended to any existing path segments. /// /// [1]: https://url.spec.whatwg.org/#scheme-relative-special-url-string - fn join_safe(&mut self, segments: &[T]) -> Result<(), UrlError> + fn join_safe(&self, segments: &[T]) -> Result where T: AsRef; } impl UrlExt for url::Url { - fn join_safe(&mut self, segments: &[T]) -> Result<(), UrlError> + fn join_safe(&self, segments: &[T]) -> Result where T: AsRef, { @@ -66,7 +67,8 @@ impl UrlExt for url::Url { .fail(); } - self.path_segments_mut() + let mut url = self.clone(); + url.path_segments_mut() // A URL cannot be a base when: // // - its scheme is not one of the "special" schemes (http, https, ws, wss, ftp, file), and @@ -79,7 +81,7 @@ impl UrlExt for url::Url { .pop_if_empty() .extend(segments); - Ok(()) + Ok(url) } } @@ -95,8 +97,9 @@ mod tests { #[case::root_slash("https://example.org/", "https://example.org/foo/bar/baz")] #[case::root("https://example.org", "https://example.org/foo/bar/baz")] fn join_safe_keeps_base_path(#[case] base: &str, #[case] expected: &str) { - let mut url = url::Url::parse(base).expect("base URL must parse"); - url.join_safe(&["foo", "bar", "baz"]) + let url = url::Url::parse(base).expect("base URL must parse"); + let url = url + .join_safe(&["foo", "bar", "baz"]) .expect("must join, all segments are valid"); assert_eq!(url.as_str(), expected); @@ -115,8 +118,9 @@ mod tests { "alice_example.com%23EXT%23@example.org" )] fn join_safe_appends_malicious_segment(#[case] segment: &str, #[case] expected: &str) { - let mut url = url::Url::parse("http://example.org").expect("base URL must parse"); - url.join_safe(&[segment]) + let url = url::Url::parse("http://example.org").expect("base URL must parse"); + let url = url + .join_safe(&[segment]) .expect("must join, all segments are valid"); assert_eq!(url.as_str(), format!("http://example.org/{expected}")); @@ -131,7 +135,7 @@ mod tests { #[case::line_feed(&["ad\nmin"])] #[case::carriage_return(&["ad\rmin"])] fn join_safe_rejects_invalid_segment(#[case] segments: &[&str]) { - let mut url = url::Url::parse("http://example.org").expect("base URL must parse"); + let url = url::Url::parse("http://example.org").expect("base URL must parse"); let error = url .join_safe(segments) .expect_err("must be error because invalid segments are not appended"); @@ -148,7 +152,7 @@ mod tests { #[test] fn join_safe_rejects_invalid_base() { - let mut url = url::Url::parse("mailto:user@example.org").expect("base URL must parse"); + let url = url::Url::parse("mailto:user@example.org").expect("base URL must parse"); let error = url .join_safe(&["foo", "bar", "baz"]) .expect_err("must be error because the base URL cannot be a base (opaque path state)"); From 51bfea8c9aade5e35e8fb5f86d48b4eddd454199 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 16:05:57 +0100 Subject: [PATCH 10/15] chore(operator): Expose UrlError --- crates/stackable-operator/src/utils/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/mod.rs b/crates/stackable-operator/src/utils/mod.rs index 2c2b606a9..c14ced9f4 100644 --- a/crates/stackable-operator/src/utils/mod.rs +++ b/crates/stackable-operator/src/utils/mod.rs @@ -19,7 +19,10 @@ pub use self::bash::COMMON_BASH_TRAP_FUNCTIONS; since = "0.61.1" )] pub use self::logging::print_startup_string; -pub use self::{option::OptionExt, url::UrlExt}; +pub use self::{ + option::OptionExt, + url::{UrlError, UrlExt}, +}; /// Returns the fully qualified controller name, which should be used when a single controller needs to be referred to uniquely. /// From 28f8aafee741273621021b571e5221ee2bd01fec Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 16:20:29 +0100 Subject: [PATCH 11/15] fix(operator): Correctly format invalid segments error message --- crates/stackable-operator/src/utils/url.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 2e5e2400e..50020de7c 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -5,7 +5,7 @@ pub enum UrlError { #[snafu(display( "failed to use {segments} as url path segment{plural}", segments = segments.join(", "), - plural = if segments.is_empty() { "" } else { "s" } + plural = if segments.len() == 1 { "" } else { "s" } ))] InvalidSegments { segments: Vec }, From 705c07aaef7fefa0058a4e93fe2a9952c26d81d0 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 16:21:40 +0100 Subject: [PATCH 12/15] refactor(operator): Optimize invalid segment validtion --- crates/stackable-operator/src/utils/url.rs | 28 +++++++++------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 50020de7c..f71986cb1 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -1,4 +1,4 @@ -use snafu::Snafu; +use snafu::{Snafu, ensure}; #[derive(Debug, PartialEq, Eq, Snafu)] pub enum UrlError { @@ -47,25 +47,19 @@ impl UrlExt for url::Url { where T: AsRef, { - let mut invalid_segments = segments + let invalid_segments = segments .iter() - .filter(|segment| { - let segment = segment.as_ref(); - matches!(segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) - }) - // Make the iterator peekable so that we don't need to collect it here (which allocates). - // This enables us to only allocate in the error-case only - when there is at least one - // invalid path segment. - .peekable(); - - if invalid_segments.peek().is_some() { - return InvalidSegmentsSnafu { + .map(AsRef::as_ref) + .filter(|s| matches!(*s, "" | "." | "..") || s.contains(['\t', '\n', '\r'])) + .map(ToOwned::to_owned) + .collect::>(); + + ensure!( + invalid_segments.is_empty(), + InvalidSegmentsSnafu { segments: invalid_segments - .map(|s| s.as_ref().to_owned()) - .collect::>(), } - .fail(); - } + ); let mut url = self.clone(); url.path_segments_mut() From 2d6ccb0829462c62f2726a046f6b6d3e95962dde Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 16:26:57 +0100 Subject: [PATCH 13/15] test(operator): Improve terator chain in unit test --- crates/stackable-operator/src/utils/url.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index f71986cb1..19e2cc001 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -137,9 +137,7 @@ mod tests { assert_eq!( error, UrlError::InvalidSegments { - // NOTE (@Techassi): We use to_string instead of to_owned (which we normally prefer) - // because we have a double reference (&&str) here. - segments: segments.iter().map(ToString::to_string).collect() + segments: segments.iter().map(|s| (*s).to_owned()).collect() } ); } From 2bf82f7bc0bd46336398490c25d071bbf334c5b1 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Oct 2026 16:27:53 +0100 Subject: [PATCH 14/15] chore(operator): Improve disallowed method reason --- clippy.toml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/clippy.toml b/clippy.toml index 1bb3b8dbb..31bd40bb1 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,8 +1,5 @@ allow-unwrap-in-tests = true allow-panic-in-tests = true disallowed-methods = [ - { path = "url::Url::join", reason = """ - Url::join is unsafe in contexts in which users have control over the to-be-joined input, because - the function will replace the original base URL in certain situations. Bad actors can craft - malicious inputs to point at an attacker controlled URL""", replacement = "join_safe" }, + { path = "url::Url::join", reason = "the function resolves its input as a URL, allowing bad actors to craft malicious inputs which replace the URL entirely or traverse the path; use UrlExt::join_safe instead" }, ] From 1f6d4e71474e76b60cf9b8502af7e392fc477361 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 2 Nov 2026 11:44:24 +0100 Subject: [PATCH 15/15] docs: Shorten and improve docs for UrlExt::join_safe --- crates/stackable-operator/src/utils/url.rs | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 19e2cc001..4fdcd65f0 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -16,27 +16,15 @@ pub enum UrlError { /// An extension trait for [`url::Url`], most notably providing a safe alternative for /// [`url::Url::join`]: [`UrlExt::join_safe`]. pub trait UrlExt: Sized { - /// Joins one or more segments to the end of the existing URL path segments and returns a new - /// [`url::Url`]. + /// Returns a clone of this URL with each of `segments` appended as exactly one URL path segment + /// to the existing path segments. /// /// Compared to [`url::Url::join`], it protects users against (accidental) misuse (especially /// when dealing with untrusted input) in two ways: /// - /// 1. The function validates that each segment is not an invalid segment. This for example - /// prevents path traversals. - /// 2. The upstream function can replace the entire base URL in these two situations: - /// - A [scheme relative special URL][1] as input replaces everything in the base URL after - /// the scheme. - /// - An absolute URL (with a scheme) as input replaces the whole base URL (even the - /// scheme). - /// - /// _From (Accessed 2026-10-19, - /// version 2.5.8)_ - /// - /// This for example allows users to construct malicious URLs. This function constructs the - /// final URL in a way in which the segments are only appended to any existing path segments. - /// - /// [1]: https://url.spec.whatwg.org/#scheme-relative-special-url-string + /// 1. It rejects invalid segments: Empty segments, `.`, `..`, and segments containing any of + /// `\t`, `\n`, `\r`. + /// 2. It only appends segments, the base URL is never replaced. fn join_safe(&self, segments: &[T]) -> Result where T: AsRef;