diff --git a/CHANGELOG.md b/CHANGELOG.md index a4d7beb..d6d1a59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,22 @@ that may never merge. They are not releases and are not listed here. unsigned binary carrying a quarantine flag, which a browser download sets and `curl` does not. +- The URL printed by `--debug`, and by a text-mode `--dry-run`, is now a URL. + Query values were concatenated raw, so any value containing a space rendered + with literal spaces and `curl` refused it outright — which is the one thing + that line is printed for. Found with `mapbox search forward --q "Dog + friendly coffee shops near me"`, an ordinary call now that the Search Box + API takes free text. The request itself was never affected: reqwest encodes + what it sends, and a JSON dry run keeps `url` and `query` as separate + fields, so only this rendering was wrong. Values are percent-encoded with + `%20` rather than form-urlencoding's `+`, and `,` `:` `/` `@` are kept + literal, so a coordinate or a style URI still reads as one. + + It also stops a value from misrepresenting the request. A value holding `&` + used to split into another `name=value` pair, so the line claimed a + parameter the request never carried, and anyone pasting it sent something + different from what was being debugged. + ## 0.2.2 - 2026-09-15 ### Changed diff --git a/src/executor.rs b/src/executor.rs index 309a6d2..b21a0a5 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -562,7 +562,19 @@ fn with_page_context(err: anyhow::Error, next_page: Option<&NextPage>) -> anyhow fn redacted_url(url: &str, query: &[(String, String)]) -> String { let rendered: Vec = query .iter() - .map(|(name, value)| format!("{name}={}", shown_value(name, value))) + .map(|(name, value)| { + let shown = shown_value(name, value); + // The token's stand-in is spliced in literally. It is not a value + // anyone sends, so encoding it would only turn an obvious + // placeholder into `%3Credacted%3E`, and the URL cannot be used + // until the reader puts their own token there regardless. + let shown = if name == ACCESS_TOKEN { + shown.to_string() + } else { + encode_query_value(shown) + }; + format!("{}={shown}", encode_query_value(name)) + }) .collect(); if rendered.is_empty() { @@ -572,6 +584,41 @@ fn redacted_url(url: &str, query: &[(String, String)]) -> String { } } +/// One query value, encoded so the rendered URL is a URL. +/// +/// This existed as plain string concatenation, and the result was a line that +/// could not be used for the one thing it is printed for. A free-text query — +/// `--q "Dog friendly coffee shops near me"`, which the Search Box API now +/// takes — rendered with literal spaces, and `curl` rejects that outright: +/// the request the CLI itself made was fine, because reqwest encodes what it +/// sends, but the URL beside it on stderr was not the URL that went out and +/// could not be pasted anywhere. +/// +/// `%20` rather than form-urlencoding's `+`. Both decode to a space at any +/// server that reads the query as a form, but only `%20` means a space +/// everywhere else, and `+` in a URL a person is reading is a character they +/// have to stop and think about. +/// +/// The kept set is the unreserved characters of RFC 3986 plus the four +/// sub-delims and gen-delims that are legal in a query and appear in real +/// values here: `,` in a coordinate pair or a bbox, `:` and `/` in a +/// route-geometry or a style URI, `@` in a static-images overlay. Everything +/// else is escaped, which matters most for `&`, `=` and `+` — left alone they +/// change the shape of the query rather than a value in it. +fn encode_query_value(value: &str) -> String { + let mut out = String::with_capacity(value.len()); + for byte in value.bytes() { + match byte { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' => { + out.push(byte as char); + } + b',' | b':' | b'/' | b'@' => out.push(byte as char), + _ => out.push_str(&format!("%{byte:02X}")), + } + } + out +} + /// One query parameter's value, or the stand-in when it is the token. /// /// The single place that decides what may be printed. Both renderings of the @@ -1660,6 +1707,84 @@ mod tests { ); } + /// The printed URL has to *be* a URL. + /// + /// Found by running the thing: the Search Box API takes free text now, so + /// `--q "Dog friendly coffee shops near me"` is an ordinary call, and the + /// URL beside it on stderr came out with literal spaces. `curl` answers + /// that with nothing at all — exit 3, no request made — which makes a + /// debugging aid useless for debugging. + /// + /// The request itself was always fine; reqwest encodes what it sends. It + /// was only this rendering, which is also the dry run's. + #[test] + fn a_free_text_query_renders_a_url_that_can_be_used() { + let query = [ + ("access_token".to_string(), "sk.a-real-token".to_string()), + ( + "q".to_string(), + "Dog friendly coffee shops near me".to_string(), + ), + ("proximity".to_string(), "-77.0336,38.8996".to_string()), + ]; + let rendered = redacted_url("https://api.mapbox.com/search/searchbox/v1/forward", &query); + + assert!( + rendered.contains("q=Dog%20friendly%20coffee%20shops%20near%20me"), + "{rendered}" + ); + // A comma is legal in a query and carries meaning to a reader, so it + // survives: `proximity=-77.0336%2C38.8996` would be correct and worse. + assert!( + rendered.contains("proximity=-77.0336,38.8996"), + "{rendered}" + ); + assert!(!rendered.contains("sk.a-real-token"), "{rendered}"); + + // The claim, checked rather than eyeballed: it parses, and every value + // comes back out the way it went in. + let parsed = reqwest::Url::parse(&rendered).expect("the rendered URL has to parse"); + let back: Vec<(String, String)> = parsed + .query_pairs() + .map(|(k, v)| (k.into_owned(), v.into_owned())) + .collect(); + assert_eq!( + back, + vec![ + ("access_token".to_string(), "".to_string()), + ( + "q".to_string(), + "Dog friendly coffee shops near me".to_string() + ), + ("proximity".to_string(), "-77.0336,38.8996".to_string()), + ] + ); + } + + /// A value cannot invent a parameter that was never sent. + /// + /// This is the half that is worse than ugly. Concatenated raw, a value + /// holding `&` splits into another `name=value` pair, so the line claims + /// the request carried something it did not — and anyone who pastes it + /// sends a different request than the one being debugged. `=` and `+` + /// are here for the same reason: one changes where a value starts, and + /// the other is read as a space by anything parsing a form. + #[test] + fn a_value_cannot_forge_another_query_parameter() { + let query = [( + "q".to_string(), + "coffee&limit=99&access_token=sk.theirs".to_string(), + )]; + let rendered = redacted_url("https://api.mapbox.com/search/searchbox/v1/forward", &query); + + let parsed = reqwest::Url::parse(&rendered).expect("parses"); + let names: Vec = parsed.query_pairs().map(|(k, _)| k.into_owned()).collect(); + assert_eq!(names, vec!["q"], "one parameter went in: {rendered}"); + + let (_, value) = parsed.query_pairs().next().expect("the one pair"); + assert_eq!(value, "coffee&limit=99&access_token=sk.theirs"); + } + /// An unauthenticated call has no query at all, and a URL ending in `?` /// is not the request that would be sent. #[test]