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..31bd40bb1 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,2 +1,5 @@ allow-unwrap-in-tests = true allow-panic-in-tests = true +disallowed-methods = [ + { 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" }, +] diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 53bc0d2e1..e8cb0d447 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -6,17 +6,23 @@ All notable changes to this project will be documented in this file. ### Added +- Add `UrlExt::join_safe` function to safely append path segments to a base URL ([#1319]). - Add `crd::containerdebug.rs` module with the `ContainerdebugConfig` CRD struct as well as helper functions on `ValidatedContainerdebugConfig` ([#1308]) -[#1308]: https://github.com/stackabletech/operator-rs/pull/1308 - ### 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]). + +[#1308]: https://github.com/stackabletech/operator-rs/pull/1308 [#1315]: https://github.com/stackabletech/operator-rs/pull/1315 +[#1319]: https://github.com/stackabletech/operator-rs/pull/1319 ## [0.121.0] - 2026-10-14 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. /// diff --git a/crates/stackable-operator/src/utils/url.rs b/crates/stackable-operator/src/utils/url.rs index 81ca9356e..4fdcd65f0 100644 --- a/crates/stackable-operator/src/utils/url.rs +++ b/crates/stackable-operator/src/utils/url.rs @@ -1,55 +1,142 @@ +use snafu::{Snafu, ensure}; + +#[derive(Debug, PartialEq, Eq, Snafu)] +pub enum UrlError { + #[snafu(display( + "failed to use {segments} as url path segment{plural}", + segments = segments.join(", "), + plural = if segments.len() == 1 { "" } else { "s" } + ))] + InvalidSegments { segments: Vec }, + + #[snafu(display("failed to append path segments to url as it cannot be a base"))] + 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 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. - /// - /// ### Example + /// Returns a clone of this URL with each of `segments` appended as exactly one URL path segment + /// to the existing path segments. /// - /// ``` - /// use stackable_operator::utils::UrlExt; + /// Compared to [`url::Url::join`], it protects users against (accidental) misuse (especially + /// when dealing with untrusted input) in two ways: /// - /// let url = url::Url::parse("http://example.com").unwrap(); - /// let url = url.join_many(vec!["realms/", "master/", "myuser"]).unwrap(); - /// - /// assert_eq!(url.as_str(), "http://example.com/realms/master/myuser"); - /// ``` - fn join_many<'a>( - self, - inputs: impl IntoIterator, - ) -> Result; + /// 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; } 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(&self, segments: &[T]) -> Result + where + T: AsRef, + { + let invalid_segments = segments + .iter() + .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 + } + ); + + 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 + // - 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); Ok(url) } } #[cfg(test)] -mod test { +mod tests { + use rstest::rstest; + use super::*; + #[rstest] + #[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 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); + } + + #[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 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}")); + } + + #[rstest] + #[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 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"); + + assert_eq!( + error, + UrlError::InvalidSegments { + segments: segments.iter().map(|s| (*s).to_owned()).collect() + } + ); + } + #[test] - fn url_join_many() { - let url = url::Url::parse("http://example.com").unwrap(); - let url = url.join_many(vec!["realms/", "master/", "myuser"]).unwrap(); + fn join_safe_rejects_invalid_base() { + 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)"); - assert_eq!(url.as_str(), "http://example.com/realms/master/myuser"); + assert_eq!(error, UrlError::CannotBeABase); } }