From a9bb1c256281b9c48d63df1ffd221e5a4048496a Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 14 Sep 2026 23:44:19 -0400 Subject: [PATCH 1/2] Treat a path parameter as a segment, not as URL syntax MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Path parameters are substituted into a template — `…/{username}/{style_id}/ static/{overlay}/…` — and the value went in unencoded, so one carrying URL syntax changed the shape of the request rather than naming a segment within it. A `?` appended query parameters the caller never asked for, `..` and its backslash spelling moved the path, and `#` truncated it. Each went out with the caller's token and the command's method. The host was never reachable: `//other`, `https://other`, `%2f%2f…` and `x@other` all still resolve to the API's own host, so nothing could be directed at another server. What was reachable was a different path on the API — worst shaped as `styles delete`, a `DELETE` whose target was influenced by its argument. It matters most where the value does not come from the person typing: an agent building a command from a prompt, or a script interpolating a variable from upstream. `/`, `?`, `#` and `\` are now percent-encoded. `\` is in that list because a special scheme treats it as a separator too, which `reqwest::Url` confirms — encoding `/` alone would have left it open. **Only those four, deliberately.** These values carry punctuation on purpose: `static get-image`'s template is `…/static/{overlay}/{lon},{lat},{zoom},{bearing},{pitch}/{width}x{height}{highRes}{format}`, where the overlay is `pin-s+f74e4e(-122.46,37.77)`, `{highRes}` is `@2x` and three placeholders share one comma-separated segment. Encoding everything outside RFC 3986's unreserved set would rewrite all of that and risk breaking requests that work today. Encoding only what alters the URL's shape cannot change a request that does not already contain those characters — pinned by a test that walks real values and asserts they pass through byte for byte, and without allocating. A value of `.` or `..` is **refused** rather than encoded, because encoding does not stop it: a special scheme reads `%2e%2e` as a double-dot segment too, so it climbs however it is spelled. Encoding the separators handles the multi-level case by collapsing it to one segment; this handles the remainder. Eight tests, including that an empty optional parameter still drops its whole segment — several operations rely on that and it had to survive. 595 tests, fmt and clippy clean. --- docs/commands.md | 1 + src/executor.rs | 165 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 163 insertions(+), 3 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 4c4180f..23829b4 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3497,6 +3497,7 @@ faults and are not: | `request_timed_out` | The request ran out of its time budget. Its own code because it is the one transport failure worth retrying or raising `--timeout` for. | | `missing_path_parameters` | A `{username}`/`{owner}`/`{account}` placeholder went unresolved. | +| `invalid_path_parameter` | A path parameter was `.` or `..`, which would move the request to a different endpoint. Other URL syntax in a path parameter (`/`, `?`, `#`, `\`) is percent-encoded rather than refused, so it names a segment instead of changing the URL's shape. | | `invalid_data` | `--data` was not valid JSON, or a `@`/`@-` body was empty. | | `invalid_file` | A file could not be read: one named by `--file`, or one named by `--data @`. Also a `@` that is not valid UTF-8, which a JSON body has to be. | | `binary_response` | The response was bytes and stdout is a terminal. Redirect it to a file. | diff --git a/src/executor.rs b/src/executor.rs index fbee348..f86deaa 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::time::Duration; use anyhow::{anyhow, Result}; @@ -124,7 +125,8 @@ fn dispatch( // Substitute other path params from positional args for param in &op.path_params { if let Some(val) = matches.get_one::(¶m.arg_name) { - path = substitute_path_param(&path, ¶m.name, val, param.required); + let safe = path_segment(¶m.name, val)?; + path = substitute_path_param(&path, ¶m.name, &safe, param.required); } } @@ -1087,6 +1089,68 @@ fn empty_success_line(method: &str, subject: Option<&str>) -> String { /// Parameters that are only part of a segment (`{width}x{height}{format}`) /// are substituted as they are: an empty value there is the format's default, /// which is what the spec means by optional. +/// The characters that would change the URL's *structure* rather than name a +/// segment within it. +/// +/// `\` is here because WHATWG treats it as a path separator for special +/// schemes, so `..\..\x` traverses exactly as `../../x` does — verified +/// against `reqwest::Url`, not assumed. +const PATH_STRUCTURAL: [char; 4] = ['/', '?', '#', '\\']; + +/// One path parameter's value, safe to splice into the URL's path. +/// +/// The path is built by substituting into a template +/// (`…/{username}/{style_id}/static/{overlay}/…`), so a value carrying URL +/// syntax used to change which request went out. With the caller's token and +/// the command's method attached, `styles delete '../../x'` aimed a `DELETE` +/// at a path nobody asked for, and `'x?fresh=true'` appended a query +/// parameter — the same shape as mapbox/mcp-server's `directions_tool` fix. +/// +/// **Only the four structural characters are encoded, deliberately.** Path +/// parameters here carry punctuation on purpose: a static-images overlay is +/// `pin-s+f74e4e(-122.46,37.77)`, `{highRes}` is `@2x`, `{format}` is `.png`, +/// and `{lon},{lat},{zoom}` are three placeholders sharing one comma- +/// separated segment. Percent-encoding everything outside RFC 3986's +/// unreserved set would rewrite all of that and risk breaking requests that +/// work today. Encoding only what alters the URL's shape cannot change any +/// request that does not already contain those four characters. +/// +/// Dot segments are refused rather than encoded, because encoding does not +/// stop them: WHATWG reads `%2e%2e` as a double-dot segment too, so a value +/// of exactly `..` still climbs a level however it is spelled. Encoding the +/// separators is what defeats the multi-level `../../x` case — it collapses +/// to a single segment — and this catches the single-level remainder. +fn path_segment<'a>(name: &str, value: &'a str) -> Result> { + // `%2e` is a dot as far as the URL parser is concerned, in either case. + let as_dots = value.replace("%2e", ".").replace("%2E", "."); + if as_dots == "." || as_dots == ".." { + return Err(CliError::new( + "invalid_path_parameter", + format!( + "`{value}` cannot be used as {name}: a path segment of `.` or `..` would move \ + the request to a different endpoint." + ), + ) + .into()); + } + + if !value.contains(PATH_STRUCTURAL) { + return Ok(Cow::Borrowed(value)); + } + + let mut encoded = String::with_capacity(value.len()); + for c in value.chars() { + match c { + '/' => encoded.push_str("%2F"), + '?' => encoded.push_str("%3F"), + '#' => encoded.push_str("%23"), + '\\' => encoded.push_str("%5C"), + other => encoded.push(other), + } + } + Ok(Cow::Owned(encoded)) +} + fn substitute_path_param(path: &str, name: &str, value: &str, required: bool) -> String { let placeholder = format!("{{{name}}}"); let segment = format!("/{placeholder}"); @@ -1272,10 +1336,12 @@ fn write_binary(body: &[u8], content_type: &str) -> Result<()> { mod tests { use super::{ describe_body, empty_success_line, file_name_of, is_binary_content_type, part_media_type, - payload_of, query_pairs, redacted_url, request_id, resolve_body_source, resolve_data, - shell_value, substitute_path_param, with_page_context, BodySource, NextPage, + path_segment, payload_of, query_pairs, redacted_url, request_id, resolve_body_source, + resolve_data, shell_value, substitute_path_param, with_page_context, BodySource, NextPage, ResponseHeaders, ACCESS_TOKEN, REQUEST_ID_HEADERS, }; + use std::borrow::Cow; + use crate::http::Payload; use crate::output::CliError; use crate::spec::{Parameter, RequestBody}; @@ -2121,4 +2187,97 @@ mod tests { ); assert_eq!(err.downcast_ref::().unwrap().fix, None); } + + /// The shape mapbox/mcp-server fixed in `directions_tool`: a value spliced + /// into the path used to append query parameters the caller never asked + /// for. Verified against `reqwest::Url` at the time — `x?fresh=true` gave + /// `query = fresh=true&access_token=…`. + #[test] + fn a_path_parameter_cannot_inject_a_query_string() { + let safe = path_segment("style_id", "x?fresh=true").expect("encoded, not refused"); + assert_eq!(safe, "x%3Ffresh=true"); + } + + /// With the caller's token and the command's method attached, this aimed a + /// `DELETE` at whatever path the value resolved to. The host was never + /// reachable — `//evil`, `https://evil` and `x@evil` all stay on + /// `api.mapbox.com` — but the path was. + #[test] + fn a_path_parameter_cannot_retarget_the_path() { + let safe = path_segment("style_id", "../../tokens/v2/victim").expect("encoded"); + assert_eq!(safe, "..%2F..%2Ftokens%2Fv2%2Fvictim"); + assert!(!safe.contains('/'), "one segment, not four: {safe}"); + } + + /// `\` is a path separator too, for a special scheme — WHATWG says so and + /// `reqwest::Url` agrees: `..\..\tokens` resolved just as `../../tokens` + /// did. Encoding `/` alone would have left this open. + #[test] + fn a_backslash_cannot_retarget_the_path_either() { + let safe = path_segment("style_id", r"..\..\tokens").expect("encoded"); + assert_eq!(safe, "..%5C..%5Ctokens"); + } + + /// A fragment is not sent to the server, so this silently truncated the + /// path rather than redirecting it — a request to somewhere the caller + /// could not see in what they typed. + #[test] + fn a_fragment_cannot_truncate_the_path() { + assert_eq!( + path_segment("style_id", "x#frag").expect("encoded"), + "x%23frag" + ); + } + + /// **Refused, not encoded, and that distinction is the point.** Encoding + /// does not stop a dot segment: WHATWG reads `%2e%2e` as one too, so a + /// value of exactly `..` climbs a level however it is spelled. Encoding + /// the separators handles the multi-level case by collapsing it into one + /// segment; this handles what is left. + #[test] + fn a_dot_segment_is_refused_however_it_is_spelled() { + for value in ["..", ".", "%2e%2e", "%2E%2E", "%2e", ".%2e", "%2e."] { + let err = path_segment("style_id", value).expect_err(value); + assert_eq!(refusal(err).code, "invalid_path_parameter", "{value}"); + } + } + + /// **The reason this encodes four characters and not everything outside + /// RFC 3986's unreserved set.** These values carry punctuation on purpose, + /// and percent-encoding it would rewrite requests that work today — + /// `static get-image`'s template alone is + /// `…/static/{overlay}/{lon},{lat},{zoom},{bearing},{pitch}/{width}x{height}{highRes}{format}`. + #[test] + fn punctuation_a_path_parameter_legitimately_carries_is_untouched() { + for value in [ + "pin-s+f74e4e(-122.46,37.77)", + "@2x", + ".png", + "-122.4194,37.7749,12,0,0", + "mapbox.mapbox-streets-v8", + "Arial Unicode MS Regular", + "0-255", + "ckstyle00000000000000001a", + ] { + let safe = path_segment("p", value).expect("not refused"); + assert_eq!(safe, value, "should pass through byte for byte"); + assert!(matches!(safe, Cow::Borrowed(_)), "and without allocating"); + } + } + + /// An empty optional parameter still drops its whole segment, which + /// several operations rely on — encoding must not have taken that away. + #[test] + fn an_empty_optional_parameter_still_drops_its_segment() { + let safe = path_segment("draft", "").expect("empty is not a dot segment"); + assert_eq!(safe, ""); + assert_eq!( + substitute_path_param("/styles/v1/{u}/{id}/draft", "draft", &safe, false), + "/styles/v1/{u}/{id}/draft" + ); + assert_eq!( + substitute_path_param("/styles/v1/{u}/{id}/{draft}", "draft", &safe, false), + "/styles/v1/{u}/{id}" + ); + } } From 3ba3480c2ab05b46ea64dbe90562d2a41ae483dc Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 15 Sep 2026 00:01:22 -0400 Subject: [PATCH 2/2] Pin that an encoded separator stays one segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised in review: `%2e%2e%2fvictim` has no raw structural character, so it passes through unchanged. True — and it names one segment rather than traversing, because the URL parser never decodes `%2f` into a separator. Encoded dots with *raw* slashes are the case that does resolve, and the encoding this adds is what stops it; the two were only distinguishable by reading the parser, so now a test says which is which. Left alone rather than blocked on purpose. Percent-encoded values in path parameters are a documented Mapbox feature: a custom marker overlay is `url-https%3A%2F%2Fexample.com%2Fmarker.png(…)`, `geojson(…)` takes URI-encoded GeoJSON, and the spec says a bbox's brackets may be sent "literally or percent-encoded as %5B". Refusing them would break all three. --- src/executor.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/executor.rs b/src/executor.rs index f86deaa..96c5f61 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -2242,6 +2242,39 @@ mod tests { } } + /// Encoded dots *and* encoded slashes together, which is the shape that + /// looks like traversal and is not. + /// + /// `%2f` is never decoded into a separator by the URL parser, so this + /// stays one segment on the wire — unlike encoded dots with *raw* slashes + /// (`%2e%2e/%2e%2e/x`), which the parser does resolve and which the + /// encoding above is what stops. It is left alone on purpose: percent- + /// encoded values are a documented Mapbox feature, not an attack + /// signature. A custom marker overlay is + /// `url-https%3A%2F%2Fexample.com%2Fmarker.png(…)`, `geojson(…)` takes + /// URI-encoded GeoJSON, and the spec says a bbox's brackets "may be sent + /// literally or percent-encoded as `%5B`". Refusing these would break all + /// three. + #[test] + fn a_percent_encoded_separator_stays_one_segment() { + let value = "%2e%2e%2fvictim"; + let safe = path_segment("style_id", value).expect("not refused"); + assert_eq!(safe, value, "passed through, because it names one segment"); + + let url = reqwest::Url::parse(&format!("https://api.mapbox.com/styles/v1/u/{safe}")) + .expect("parses"); + let segments: Vec<&str> = url + .path_segments() + .map(Iterator::collect) + .unwrap_or_default(); + assert_eq!( + segments, + ["styles", "v1", "u", "%2e%2e%2fvictim"], + "one segment under /u/, not a traversal: {}", + url.path() + ); + } + /// **The reason this encodes four characters and not everything outside /// RFC 3986's unreserved set.** These values carry punctuation on purpose, /// and percent-encoding it would rewrite requests that work today —