diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a71156..11c1878 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,18 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +- New `mapbox report ""` sends feedback about the CLI itself to + the Mapbox CLI team, with the CLI's version and OS appended. It is filed + under Mapbox's account, not yours. It is sent with the CLI's own token, + so it needs no login and no `user-feedback:write` scope; your token + stands in while the CLI's can't be used (including when it is + rate-limited), or when you pass `--token` or `--use-login`, and the + request then carries your token. + `--category` is optional and defaults to `general`. Two new error codes + say why the CLI's token couldn't be used when you have no token either: + `cli_token_unavailable` and `cli_token_forbidden` (see + [Errors](docs/commands.md#errors)). + - The CLI's own token, used for telemetry when you have no token of your own, is now fetched from `https://cli.mapbox.com/config/token.json` and kept in `~/.mapbox/cli-token.json` instead of being built into the binary, diff --git a/README.md b/README.md index f757e93..18edc2c 100644 --- a/README.md +++ b/README.md @@ -252,7 +252,7 @@ mapbox tilesets ``` `mapbox directions`, `mapbox isochrone`, `mapbox map-match`, -`mapbox matrix`, and `mapbox places` are the exceptions: each API has a +`mapbox matrix`, `mapbox places` and `mapbox report` are the exceptions: each API has a single exposed operation, so there's a bare command with no subcommand at all, the same shape `mapbox usage` already has, see [docs/commands.md](./docs/commands.md) for their own parameters. @@ -323,6 +323,21 @@ unexpectedly. It makes no request unless you pass `--verify`. See [Config](./docs/commands.md#config) for details, and [Command history](#command-history) below. +### Feedback about the CLI + +```sh +mapbox report "I'd like support for optimization commands" +mapbox report "tilesets query hangs behind my proxy" --category bug +``` + +The message goes to the Mapbox CLI team, with the CLI's version and OS +appended, and is filed under Mapbox's account, not yours, so +`mapbox feedback list` won't show it. It is sent with the CLI's own token, +so it needs no login; yours stands in while the CLI's can't be used, or +when you pass `--token` or `--use-login`, and the request then carries your +token. See +[Report](./docs/commands.md#report). + ### Shell completion Homebrew installs completions for you. Otherwise, `mapbox completion` @@ -476,9 +491,10 @@ and `--dry-run` show it alongside everything else on the request. ### The CLI's own requests -Requests the CLI makes for itself rather than for your commands use your own -token whenever you have one: `--token` or `MAPBOX_ACCESS_TOKEN`, then your -login. Only when you have none does it use the CLI's token: +The CLI has a token of its own, for requests it makes for itself. Telemetry +uses your own token whenever you have one: `--token` or +`MAPBOX_ACCESS_TOKEN`, then your login. Only when you have none does it use +the CLI's token: `MAPBOX_CLI_TOKEN`, or a public token fetched from `https://cli.mapbox.com/config/token.json` and kept in `~/.mapbox/cli-token.json`. It is replaced only when the Tokens API says it @@ -488,7 +504,9 @@ third in a row pauses telemetry's sending with it for an hour, as three fetches in a row that find no usable token pause fetching. The token is restricted to `https://cli.mapbox.com/`, so every request with it carries that as its `Referer`. The CLI's token is only used for what -`src/cli_token.rs` lists for it, today just telemetry. +`src/cli_token.rs` lists for it: telemetry, and `mapbox report`, which is +the other way around: it prefers the CLI's token to yours and falls back to +yours while the CLI's can't be used. ### Proxies diff --git a/custom-openapi/report/openapi/report.yaml b/custom-openapi/report/openapi/report.yaml new file mode 100644 index 0000000..33310f3 --- /dev/null +++ b/custom-openapi/report/openapi/report.yaml @@ -0,0 +1,77 @@ +openapi: "3.0.0" +# `parse_spec` turns `info.description` below into this service's clap +# `long_about`, so it also reaches `mapbox report --help`, `--schema` and +# `generate-skills` output. Keep it to API prose only — the provenance below +# is for whoever edits this file, not for a CLI user: +# +# An undocumented Feedback API endpoint the Feedback API team added for this +# CLI (AGI-1168): the same body as `POST /user-feedback/v1/feedback`, but +# filed under Mapbox's own `locationai-mapbox` account rather than the +# caller's, and needing no `user-feedback:write`. Not on docs.mapbox.com, so +# this file is the only description of it. See `custom-openapi/README.md` for +# how a file like this is wired in. +# +# One command, `mapbox report ""`, via `FLATTENED_SERVICES`. The +# message is `POSITIONAL_BODY_FIELDS`'s one entry and `lat`/`lon` are +# `FIXED_BODY_FIELDS` (both in `src/spec.rs`): feedback about the CLI has no +# place, and the API still requires the pair. `id`, `feature` and +# `screenshot` are left out on purpose, so this command takes no `--data`. +info: + title: "Mapbox CLI feedback" + description: >- + Send feedback about the Mapbox CLI itself — a bug, a missing command, or + anything else — to the team that builds it. It goes to Mapbox, not to + your account, so `mapbox feedback list` will not show it. + version: "0.0.0" +servers: + - url: https://api.mapbox.com + description: Feedback API +paths: + /user-feedback/v1/cli-feedback: + post: + operationId: report + summary: Send feedback about the Mapbox CLI. + description: >- + Sends one message to the Mapbox CLI team, with the CLI's version and + OS appended. It is sent with a token the CLI provides, so it needs no + login; while that token can't be used, or with `--token` or + `--use-login`, it is sent with yours. Either way it is filed under + Mapbox's account, not yours. Any valid token works; no extra scope is + needed. There is no way to read it back or delete it afterward. + parameters: + - name: "access_token" + in: query + required: true + description: "Mapbox API Access Token" + schema: + type: string + minLength: 1 + requestBody: + required: true + content: + application/json: + schema: + type: object + required: ["feedback", "lat", "lon", "category"] + properties: + feedback: + type: string + description: "What you want to tell the Mapbox CLI team." + lat: + type: number + description: "Latitude. Always 0: CLI feedback has no place." + lon: + type: number + description: "Longitude. Always 0: CLI feedback has no place." + category: + type: string + description: >- + What kind of feedback this is, such as `bug` or + `feature_request`. Free text. + responses: + "200": + description: The created feedback item. + "400": + description: Bad Request — a missing or invalid field. + "401": + description: Unauthorized diff --git a/docs/commands.md b/docs/commands.md index bc3c8a0..be5790b 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1,8 +1,8 @@ # Implemented commands -Every command the CLI ships, including five auth commands, 40 API operations (35 across 9 -command groups, plus `directions`, `isochrone`, `map-match`, `matrix` and -`places`), the tilesets-cli proxy, `completion` and `generate-skills`. Each is +Every command the CLI ships, including five auth commands, 41 API operations (35 across 9 +command groups, plus `directions`, `isochrone`, `map-match`, `matrix`, +`places` and `report`), the tilesets-cli proxy, `completion` and `generate-skills`. Each is shown in both of its renderings. Which one you get is decided by `--output`, whose default (`auto`) reads stdout: a terminal gets the left column, a pipe or redirect gets the right one. See @@ -81,6 +81,8 @@ nests, and is typed `mapbox styles draft get`. **[Places](#places)** — [places](#mapbox-places) +**[Report](#report)** — [report](#mapbox-report) + **[Search](#search)** — [search.forward](#mapbox-search-forward) · [search.reverse](#mapbox-search-reverse) · [search.category](#mapbox-search-category) · @@ -462,10 +464,11 @@ No stored profiles. Run `mapbox auth login` to create one. ## API command groups -35 operations across 9 command groups, plus five single-operation commands -with no group: `directions`, `isochrone`, `map-match`, `matrix` and `places`. +35 operations across 9 command groups, plus six single-operation commands +with no group: `directions`, `isochrone`, `map-match`, `matrix`, `places` +and `report`. Seven groups are generated from the OpenAPI specs vendored in `openapi/`; -`search`, `feedback` and the five single commands are the exceptions — +`search`, `feedback` and the six single commands are the exceptions — hand-authored specs versioned in this repo's own `custom-openapi/`, see `custom-openapi/README.md`. @@ -490,7 +493,7 @@ moved to `tilesets` or `static`. | [`styles`](#styles) | **9** | | [`tilesets`](#tilesets) | **3** | -The [Contents](#contents) list above names every one of the 40. +The [Contents](#contents) list above names every one of the 41. **Everything else the Mapbox specs describe is not here at all.** Not hidden, not shipped as a command that refuses: absent from the spec content @@ -598,8 +601,8 @@ Three things worth knowing about the read forms: A command that changes something takes `--dry-run`, which prints the request it would send, on stdout, and sends nothing. Which commands those are is not -a list anyone keeps: it is every `POST`, `PUT`, `PATCH` and `DELETE` — 14 of -the 40 operations — plus `auth login`, `auth logout`, `auth refresh` and +a list anyone keeps: it is every `POST`, `PUT`, `PATCH` and `DELETE` — 15 of +the 41 operations — plus `auth login`, `auth logout`, `auth refresh` and `generate-skills`. A read-only `GET` does not take it, so `mapbox styles list --dry-run` is a usage error rather than a no-op. It rehearses rather than describes: `--data` is parsed and every `--file` is read, so a @@ -2002,6 +2005,90 @@ $ echo $? --- +## Report + +Feedback about the CLI itself — a bug, a missing command, anything else — +sent to the Mapbox CLI team. It goes through an endpoint the Feedback API +added for this CLI and doesn't document on docs.mapbox.com, so this +section is its only description; see `custom-openapi/README.md` for how +that spec is wired in. + +### `mapbox report` + +Sends one message, with the CLI's version and OS appended to it +(`— mapbox-cli on `). It is filed under a Mapbox account, not +yours, so `mapbox feedback list` won't show it, and no `user-feedback:write` +scope is needed. There is no way to read it back or delete it, so +`--dry-run` shows the body, and which token it would go with, without +sending it. + +It is sent with the CLI's own token, not yours, so it works without a +login. A typed `--token` or `--use-login` sends with yours instead, and the +CLI's is not tried. If this API refuses +the CLI's token, a fresh one is fetched there and then and the message sent +again with it, so a token that was republished or fixed works at once. +A `MAPBOX_CLI_TOKEN` override is sent as it is and never replaced. +When the CLI's token still can't be used — it can't be fetched, it is +revoked with no new one yet, this API refuses it, or it is rate-limited +(`429`) — your own token is used instead, so the request then carries +your token, though the message is still filed under Mapbox's account. With +none of your own, the command fails with `cli_token_unavailable` or +`cli_token_forbidden`, or with the `429` itself. Each run tries the CLI's +token again; nothing here is put off for later. Telemetry settings don't +apply: the token is shared and public, and fetching it sends nothing about +you. + +#### Parameters + +`` (positional, required) — what you want to tell the team. +Quote it. An empty one is refused. One that starts with `-` goes after +`--`, with every flag before it: `mapbox report --category bug -- "-x is +ignored"`. + +| Flag | Effect | +| --- | --- | +| `--category ` | What kind of feedback this is, such as `bug` or `feature_request`. Free text. Defaults to `general`. | + +The API requires `lat` and `lon` as well; CLI feedback has no place, so +both are always sent as `0`. The command takes no `--data`. + +#### Examples + +```sh +mapbox report "I'd like support for optimization commands" +mapbox report "tilesets query hangs behind my proxy" --category bug +mapbox report "…" --dry-run +``` + +#### Outputs + +One report, captured live on 2026-10-19 and shown both ways. `-o text` +shows only that it was sent, the message and its id; `-o json` is the +answer as the API sent it: + + + + +
Terminal — -o textAgent — -o json
+ +```text +Sent to the Mapbox CLI team. +feedback Test from mapbox-cli development (report). Please ignore. + + — mapbox-cli 0.3.0 on macos +id 01a15503-8d64-75ed-aa9b-790ae4ba69a0 +``` + + + +```json +{"category":"general","created_at":"2026-10-19T16:34:18.626Z","feedback":"Test from mapbox-cli development (report). Please ignore.\n\n— mapbox-cli 0.3.0 on macos","has_screenshot":false,"id":"01a15503-8d64-75ed-aa9b-790ae4ba69a0","location":{"lat":0,"lon":0},"received_at":"2026-10-19T16:34:18.626Z","status":"received","updated_at":"2026-10-19T16:34:18.626Z"} +``` + +
+ +--- + ## Search The public, non-interactive surface of the Search Box API: text search, @@ -5082,6 +5169,8 @@ faults and are not: | `binary_response` | The response was bytes and stdout is a terminal. Redirect it to a file. | | `missing_subcommand` | A command group was named with no operation. | | `cancelled` | A delete was declined at the confirmation prompt. Nothing was sent. | +| `cli_token_unavailable` | A command that prefers the CLI's own token (`report`) could not use it — it could not be fetched, or it is revoked and no new one is published yet — and you have no token of your own to fall back to. | +| `cli_token_forbidden` | This API refused the CLI's own token, even after fetching it afresh, and you have no token of your own to fall back to. | | `usage` | Anything else clap rejected. Exit code 2, not 1. | | `error` | Unclassified. | diff --git a/src/cli_token.rs b/src/cli_token.rs index 7c2c37e..bce3892 100644 --- a/src/cli_token.rs +++ b/src/cli_token.rs @@ -25,10 +25,17 @@ //! billed to the account that owns it. So it is decided by what the request //! is for, not where it goes: a request the CLI makes for itself passes //! [`send`] an order that ends in [`Fallback::CliToken`], and every such order -//! is defined in this file (today only [`TELEMETRY`]). No command uses it. -//! Where the request is sent does not matter, so a URL overridden from the +//! is defined in this file (today only [`TELEMETRY`]); a command must be in +//! [`CLI_TOKEN_COMMANDS`], which also says how it uses it ([`Use`]). Where +//! the request is sent does not matter, so a URL overridden from the //! environment keeps the same fallback. //! +//! Either way the token is sent through [`send_with_cli_token`], which holds +//! the rules: check a refusal at once, replace a dead token, rest from an API +//! that refuses a valid one. A command that cannot use the CLI's token falls +//! back to the user's, and with none says why: unavailable right now +//! ([`Unavailable`]) or refused by this API ([`Refusal`]). +//! //! # The token is public //! //! It is served from a public URL, so it must be a dedicated `pk.` token with @@ -44,7 +51,7 @@ use crate::{auth, http}; mod fetch; -use fetch::Refusal; +pub(crate) use fetch::{Refusal, Unavailable}; /// The `Referer` every request with the CLI's token carries. The published /// token is restricted to this URL, so that a web page elsewhere cannot use @@ -79,6 +86,105 @@ pub(crate) const TELEMETRY: &[Fallback] = #[cfg(test)] const ORDERS: &[&[Fallback]] = &[TELEMETRY]; +/// How a command listed in [`CLI_TOKEN_COMMANDS`] uses the CLI's token. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Use { + /// The CLI's token first, and the user's own only while the CLI's + /// cannot be used: unavailable, resting, or refused by this API. + Preferred, +} + +/// Commands that may run on the CLI's token, by the name `--schema` reports, +/// each with how and with the reason the CLI's account should carry their +/// use. Command names are a compatibility promise, so an entry does not +/// quietly move to another API when a spec changes its URL. +/// +/// A command here must not read its body from stdin: a refused request is +/// sent again, with a fresh token or the user's, and stdin can only be read +/// once. `cli_token_commands_read_nothing_from_stdin` holds that. +const CLI_TOKEN_COMMANDS: &[(&str, Use, &str)] = &[( + "report", + Use::Preferred, + "Feedback about the CLI is filed under Mapbox's own account, so it is the \ + CLI's request rather than the user's, and it works for someone not \ + logged in.", +)]; + +/// Whether `command` may run on the CLI's token at all. +pub(crate) fn may_use(command: &str) -> bool { + use_for(command).is_some() +} + +fn use_for(command: &str) -> Option { + CLI_TOKEN_COMMANDS + .iter() + .find(|(name, _, _)| *name == command) + .map(|(_, how, _)| *how) +} + +/// The CLI's token as a command got it, for [`send_for_command`], which +/// handles a refusal of it by where it came from. +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct CommandToken { + pub(crate) token: String, + source: TokenSource, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TokenSource { + /// `MAPBOX_CLI_TOKEN`. + Override, + /// The one on disk. + Saved, + /// Fetched by this run. + Fetched, +} + +/// The CLI's token for `command`, or `None` when the command is not listed +/// for it: the only way to ask, so an unlisted command never gets one. +/// `fetch` is false where nothing may be sent, as in a dry run, and only a +/// token already on disk is returned then. +pub(crate) fn for_command(command: &str, fetch: bool) -> Option> { + match use_for(command)? { + Use::Preferred => Some(preferred_token(fetch)), + } +} + +fn preferred_token(fetch: bool) -> Result { + let given = |token: String, source| CommandToken { token, source }; + if let Some(token) = usable(std::env::var("MAPBOX_CLI_TOKEN").ok().as_deref()) { + return Ok(given(token.to_owned(), TokenSource::Override)); + } + if let Some(token) = fetch::saved_token() { + return Ok(given(token, TokenSource::Saved)); + } + if !fetch { + return Err(Unavailable::NotFetched); + } + // A command is not held by the backoff, which paces background fetches: + // someone is waiting, and the endpoint may have a new token by now. + fetch::token_now().map(|token| given(token, TokenSource::Fetched)) +} + +/// Sends a command's request with the CLI's token by [`send_with_cli_token`]'s +/// rules, as [`Disk::Command`] for a token from disk or the endpoint. +/// `MAPBOX_CLI_TOKEN` is sent once and never checked, counted or replaced, +/// as [`send`] treats it: a developer chose that token, and a refusal of it +/// is theirs to see. +pub(crate) fn send_for_command( + command: &str, + given: CommandToken, + send: impl FnMut(&str) -> R, + status: impl Fn(&R) -> Option, +) -> Sent { + let mut ledger = match given.source { + TokenSource::Override => Disk::Unchecked, + TokenSource::Saved => Disk::Command { fetched: false }, + TokenSource::Fetched => Disk::Command { fetched: true }, + }; + send_with_cli_token(command, given.token, &mut ledger, send, status) +} + /// The token the user gave this run, ranked as a command ranks it: with /// `--use-login` only a typed `--token` counts, otherwise the environment /// does too, `MapboxAccessToken` included, since that is the user's token @@ -115,27 +221,35 @@ pub(crate) fn send( ) -> Option> { let override_token = std::env::var("MAPBOX_CLI_TOKEN").ok(); let fetched = RefCell::new(None); - let dead = RefCell::new(None); - let mut attempt = |token: &str| { - let is_cli_token = fetched.borrow().as_deref() == Some(token) - || usable(override_token.as_deref()) == Some(token); - let request = request().query(&[("access_token", token)]); - let request = if is_cli_token { - request.header(reqwest::header::REFERER, REFERER) - } else { - request - }; - let response = http::send(request); - // Only the fetched token's answers are kept: the user's tokens, and - // `MAPBOX_CLI_TOKEN`, are never checked or counted. - if let (Ok(r), true) = (&response, fetched.borrow().as_deref() == Some(token)) { - if r.status().is_success() { - fetch::accepted(caller, token); - } else if is_refusal(r.status()) && fetch::refused(caller, token) == Refusal::Dead { - *dead.borrow_mut() = Some(token.to_owned()); - } + let send = |token: &str| http::send(request().query(&[("access_token", token)])); + // Any CLI token, a replacement for a dead one included. + let send_cli = |token: &str| { + http::send( + request() + .query(&[("access_token", token)]) + .header(reqwest::header::REFERER, REFERER), + ) + }; + let attempt = |token: &str| { + // Only the fetched token goes through the CLI token's rules: the + // user's tokens are never checked or counted, nor is + // `MAPBOX_CLI_TOKEN`, which still carries the CLI token's Referer. + if fetched.borrow().as_deref() != Some(token) { + return if usable(override_token.as_deref()) == Some(token) { + send_cli(token) + } else { + send(token) + }; + } + match send_with_cli_token( + caller, + token.to_owned(), + &mut Disk::Background, + send_cli, + |answer| answer.as_ref().ok().map(|r| r.status().as_u16()), + ) { + Sent::Answered(answer) | Sent::Refused(answer, _) => answer, } - response }; let fetch = || { // A caller resting from the CLI's token sends nothing with it. @@ -143,7 +257,7 @@ pub(crate) fn send( *fetched.borrow_mut() = Some(token.clone()); Some(token) }; - let answer = resolve( + resolve( order, Sources { user: user_token, @@ -151,21 +265,110 @@ pub(crate) fn send( }, || login_token(profile), fetch, - &mut attempt, + attempt, |response| response.status() == StatusCode::UNAUTHORIZED, - ); - // A dead token is replaced at once, and the request sent once more. - match dead.take() { - Some(dead) if matches!(&answer, Some(Ok(r)) if is_refusal(r.status())) => { - match fetch::token() { - Some(fresh) if fresh != dead => { - *fetched.borrow_mut() = Some(fresh.clone()); - Some(attempt(&fresh)) + ) +} + +/// What a request sent with the CLI's token came to. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum Sent { + /// An answer that was not a refusal, or no answer at all: the caller's. + Answered(R), + /// A `401` or `403` that replacing a dead token did not fix, and why. + Refused(R, Refusal), +} + +/// The state on disk, as [`send_with_cli_token`] uses it, so its rules can +/// be tested without it. +pub(crate) trait Ledger { + fn refused(&mut self, caller: &str, token: &str) -> Refusal; + fn accepted(&mut self, caller: &str, token: &str); + /// Whether a request refused for `refusal` is worth sending again with + /// a [`fresh`](Ledger::fresh) token. + fn retries(&self, refusal: Refusal) -> bool; + fn fresh(&mut self) -> Option; +} + +/// `/cli-token.json`, through [`fetch`], by kind of caller. +pub(crate) enum Disk { + /// Nobody is waiting, as for telemetry: paced, and a new token is tried + /// only for a dead one, when the backoff allows. + Background, + /// Someone ran it and is waiting: never paced, and any refusal tries a + /// token fetched there and then, so a token republished or fixed in the + /// Console works on the next try. Not when this run `fetched` the token + /// already: asking again at once would only get the same one back. + Command { fetched: bool }, + /// A token not from the endpoint (`MAPBOX_CLI_TOKEN`): the file is not + /// touched, and a refusal is handed back as it is. + Unchecked, +} + +impl Ledger for Disk { + fn refused(&mut self, caller: &str, token: &str) -> Refusal { + match self { + Disk::Background => fetch::refused(caller, token, true), + Disk::Command { .. } => fetch::refused(caller, token, false), + Disk::Unchecked => Refusal::NotFetched, + } + } + fn accepted(&mut self, caller: &str, token: &str) { + if !matches!(self, Disk::Unchecked) { + fetch::accepted(caller, token); + } + } + fn retries(&self, refusal: Refusal) -> bool { + match self { + Disk::Background => refusal == Refusal::Dead, + Disk::Command { fetched } => !fetched, + Disk::Unchecked => false, + } + } + fn fresh(&mut self) -> Option { + match self { + Disk::Background => fetch::token(), + Disk::Command { .. } => fetch::token_now().ok(), + Disk::Unchecked => None, + } + } +} + +/// Sends with the CLI's `token` on `caller`'s behalf: the one place the +/// rules for using it live, for telemetry and commands alike. +/// +/// A `2xx` ends `caller`'s cooldown. A `401` or `403` is checked with the +/// Tokens API at once ([`fetch::refused`]); when the ledger says it is worth +/// it ([`Ledger::retries`]) and a different token is to be had, the request +/// is sent once more with it, since a refused request was not acted on. +/// Otherwise the refusal is handed back. `status` is an answer's HTTP status, +/// or `None` when there was no answer. +pub(crate) fn send_with_cli_token( + caller: &str, + mut token: String, + ledger: &mut impl Ledger, + mut send: impl FnMut(&str) -> R, + status: impl Fn(&R) -> Option, +) -> Sent { + let mut replaced = false; + loop { + let answer = send(&token); + match status(&answer) { + Some(401 | 403) => { + let refusal = ledger.refused(caller, &token); + if ledger.retries(refusal) && !replaced { + if let Some(fresh) = ledger.fresh().filter(|fresh| *fresh != token) { + token = fresh; + replaced = true; + continue; + } } - _ => answer, + return Sent::Refused(answer, refusal); } + Some(200..=299) => ledger.accepted(caller, &token), + _ => {} } - _ => answer, + return Sent::Answered(answer); } } @@ -269,11 +472,6 @@ fn resolve( last.map(Ok) } -/// A `401` or `403`: the answers [`fetch::refused`] can explain. -pub(crate) fn is_refusal(status: StatusCode) -> bool { - status == StatusCode::UNAUTHORIZED || status == StatusCode::FORBIDDEN -} - fn usable(token: Option<&str>) -> Option<&str> { token.map(str::trim).filter(|t| !t.is_empty()) } @@ -517,6 +715,214 @@ mod tests { ); } + #[test] + fn every_cli_token_command_exists_and_says_why() { + let specs = crate::spec::effective_services().expect("the bundled specs parse"); + let commands: Vec = specs + .iter() + .flat_map(|svc| &svc.operations) + .map(|op| op.command()) + .collect(); + for (name, _, reason) in CLI_TOKEN_COMMANDS { + assert!( + commands.iter().any(|c| c == name), + "`{name}` is in CLI_TOKEN_COMMANDS but is not a command" + ); + assert!(!reason.trim().is_empty(), "`{name}` needs a reason"); + } + } + + #[test] + fn cli_token_commands_read_nothing_from_stdin() { + let specs = crate::spec::effective_services().expect("the bundled specs parse"); + for op in specs.iter().flat_map(|svc| &svc.operations) { + if use_for(&op.command()).is_none() { + continue; + } + let cmd = crate::build_operation_command(op); + for id in ["data", "file"] { + assert!( + cmd.get_arguments().all(|arg| arg.get_id() != id), + "`{}` takes `--{id}`, which can read stdin, and a refused request \ + is sent again", + op.command() + ); + } + } + } + + #[test] + fn an_unlisted_command_never_gets_the_clis_token() { + assert_eq!(for_command("styles list", true), None); + } + + /// A ledger that answers every refusal with `verdict` and has `fresh` + /// to hand out once, and records what it was told. + #[derive(Default)] + struct FakeLedger { + /// Retries after any refusal, as [`Disk::Command`] does, rather than + /// only a dead token, as [`Disk::Background`] does. + command: bool, + verdict: Option, + fresh: Option, + refused: Vec, + accepted: Vec, + } + + impl Ledger for FakeLedger { + fn refused(&mut self, _: &str, token: &str) -> Refusal { + self.refused.push(token.to_owned()); + self.verdict.expect("a verdict to give") + } + fn accepted(&mut self, _: &str, token: &str) { + self.accepted.push(token.to_owned()); + } + fn retries(&self, refusal: Refusal) -> bool { + self.command || refusal == Refusal::Dead + } + fn fresh(&mut self) -> Option { + self.fresh.take() + } + } + + /// Sends through [`send_with_cli_token`] to a fake API that answers + /// `statuses` in turn, and returns the outcome and the tokens sent. + fn send_cli( + ledger: &mut FakeLedger, + statuses: &[Option], + ) -> (Sent>, Vec) { + let mut sent = Vec::new(); + let mut answers = statuses.iter().copied(); + let outcome = send_with_cli_token( + "report", + "pk.old".to_owned(), + ledger, + |token| { + sent.push(token.to_owned()); + answers.next().expect("an answer for each send") + }, + |status| *status, + ); + (outcome, sent) + } + + #[test] + fn a_dead_token_is_replaced_and_sent_once_more() { + let mut ledger = FakeLedger { + verdict: Some(Refusal::Dead), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(401), Some(200)]); + assert_eq!(outcome, Sent::Answered(Some(200))); + assert_eq!(sent, ["pk.old", "pk.new"]); + assert_eq!(ledger.accepted, ["pk.new"]); + } + + #[test] + fn a_replacement_that_is_refused_too_is_not_replaced_again() { + let mut ledger = FakeLedger { + verdict: Some(Refusal::Dead), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(401), Some(401)]); + assert_eq!(outcome, Sent::Refused(Some(401), Refusal::Dead)); + assert_eq!(sent, ["pk.old", "pk.new"]); + } + + #[test] + fn a_dead_token_with_no_new_one_is_handed_back() { + let mut ledger = FakeLedger { + verdict: Some(Refusal::Dead), + fresh: Some("pk.old".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403)]); + assert_eq!(outcome, Sent::Refused(Some(403), Refusal::Dead)); + assert_eq!(sent, ["pk.old"], "the same token is not sent again"); + } + + #[test] + fn a_token_this_api_refuses_is_handed_back_without_a_fetch() { + for verdict in [Refusal::Forbidden, Refusal::Unknown] { + let mut ledger = FakeLedger { + verdict: Some(verdict), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403)]); + assert_eq!(outcome, Sent::Refused(Some(403), verdict)); + assert_eq!(sent, ["pk.old"]); + assert!(ledger.fresh.is_some(), "{verdict:?} fetched a new token"); + } + } + + #[test] + fn a_command_tries_a_fresh_token_after_any_refusal() { + for verdict in [Refusal::Forbidden, Refusal::Unknown, Refusal::NotFetched] { + let mut ledger = FakeLedger { + command: true, + verdict: Some(verdict), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403), Some(200)]); + assert_eq!(outcome, Sent::Answered(Some(200)), "{verdict:?}"); + assert_eq!(sent, ["pk.old", "pk.new"], "{verdict:?}"); + } + } + + #[test] + fn a_command_gets_the_same_token_back_and_does_not_resend_it() { + let mut ledger = FakeLedger { + command: true, + verdict: Some(Refusal::Forbidden), + fresh: Some("pk.old".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403)]); + assert_eq!(outcome, Sent::Refused(Some(403), Refusal::Forbidden)); + assert_eq!(sent, ["pk.old"]); + } + + #[test] + fn a_command_token_just_fetched_or_overridden_is_not_replaced() { + for (ledger, retries) in [ + (Disk::Command { fetched: false }, true), + (Disk::Command { fetched: true }, false), + (Disk::Unchecked, false), + ] { + for refusal in [ + Refusal::Dead, + Refusal::Forbidden, + Refusal::Unknown, + Refusal::NotFetched, + ] { + assert_eq!(ledger.retries(refusal), retries, "{refusal:?}"); + } + } + } + + #[test] + fn an_override_is_never_checked_or_replaced() { + let mut ledger = Disk::Unchecked; + assert_eq!(ledger.refused("report", "pk.override"), Refusal::NotFetched); + assert_eq!(ledger.fresh(), None); + } + + #[test] + fn only_a_2xx_counts_as_accepted() { + for status in [Some(200), Some(204), Some(404), Some(500), None] { + let mut ledger = FakeLedger::default(); + let (outcome, _) = send_cli(&mut ledger, &[status]); + assert_eq!(outcome, Sent::Answered(status)); + let accepted = status.is_some_and(|s| (200..300).contains(&s)); + assert_eq!(ledger.accepted.len(), usize::from(accepted), "{status:?}"); + assert!(ledger.refused.is_empty(), "{status:?} was checked"); + } + } + #[test] fn the_override_is_trimmed_before_it_is_sent() { let (answer, sent, _) = run( diff --git a/src/cli_token/fetch.rs b/src/cli_token/fetch.rs index 6ae04d3..167e28e 100644 --- a/src/cli_token/fetch.rs +++ b/src/cli_token/fetch.rs @@ -65,6 +65,35 @@ const TRIES_BEFORE_WAITING: u32 = 3; /// Long enough for any `pk.` token; anything longer is not one. const MAX_TOKEN_LEN: usize = 512; +/// Why there is no CLI token to use right now, as a command that wanted one +/// tells its user. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Unavailable { + /// The endpoint could not be reached. Asked again on the next run. + Unreachable, + /// The endpoint answered without a usable token. `revoked` when there + /// is a dead token it has not replaced yet, rather than never a token at + /// all. + NoToken { revoked: bool }, + /// None on disk, and the caller asked not to fetch, as a dry run does. + NotFetched, +} + +impl std::fmt::Display for Unavailable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unreachable => write!(f, "the token endpoint could not be reached"), + Self::NoToken { revoked: true } => { + write!(f, "it is revoked and no new one is published yet") + } + Self::NoToken { revoked: false } => { + write!(f, "the token endpoint did not return a usable token") + } + Self::NotFetched => write!(f, "it has not been fetched yet"), + } + } +} + /// What a fetch got back, before [`acquire`] judges it. #[derive(Debug)] pub(crate) enum Fetched { @@ -177,6 +206,35 @@ pub(crate) fn token() -> Option { token } +/// [`token`], but asking the endpoint now, whatever the backoff says and +/// even with a token on disk: for a command someone is waiting on, so that a +/// token republished or reconfigured is picked up at once. +/// +/// Only a token it gets is saved. A miss is not counted: the backoff paces +/// background fetches, and a command retried by hand must not lengthen it. +pub(crate) fn token_now() -> Result { + let mut state = read_state().unwrap_or_default(); + let before = state.clone(); + let fetched = get(&endpoint()); + let why = match fetched { + Fetched::Failed => Unavailable::Unreachable, + _ => Unavailable::NoToken { + revoked: state.rejected, + }, + }; + let token = judge(&mut state, now(), fetched, check).ok_or(why)?; + if state != before { + save_fetch(&before, state); + } + Ok(token) +} + +/// The token on disk while it is not dead, without asking the endpoint, for +/// a run that must not send anything, such as a dry run. +pub(crate) fn saved_token() -> Option { + read_state()?.usable_token().map(str::to_owned) +} + /// Writes what a fetch decided, unless another run decided first. The fetch /// can take seconds: if the token, its verdict or the count changed on disk /// meanwhile, that run's answer stands. Cooldowns another run added are @@ -232,7 +290,11 @@ pub(crate) enum Refusal { /// `token` was refused. A dead token is marked, so the next [`token`] /// fetches a new one; otherwise `caller`'s refusals are counted, and the /// third in a row rests it for an hour. -pub(crate) fn refused(caller: &str, token: &str) -> Refusal { +/// +/// Only a caller that `rests` is paced: one in the background, such as +/// telemetry. A command someone is waiting on is never held back; it tries +/// a fresh token instead ([`token_now`]). +pub(crate) fn refused(caller: &str, token: &str, rests: bool) -> Refusal { let now = now(); if read_state() .filter(|state| state.token.as_deref() == Some(token)) @@ -247,6 +309,8 @@ pub(crate) fn refused(caller: &str, token: &str) -> Refusal { // `verdict` never answers `NotFetched`; that was returned above. if refusal == Refusal::Dead { state.rejected = true; + } else if !rests { + return refusal; } else { let cooldown = state.cooldowns.entry(caller.to_owned()).or_default(); cooldown.failures += 1; @@ -293,17 +357,31 @@ fn acquire( if holds(state.retry_at, now) { return None; } - match get() { + judge(state, now, get(), check) +} + +/// What a fetch's answer does to the state. A token that is new, or the dead +/// one taken back, replaces what is kept, cooldowns and counts with it; the +/// kept token again, not dead, leaves it as it is. +fn judge( + state: &mut State, + now: u64, + fetched: Fetched, + check: impl FnOnce(&str) -> Refusal, +) -> Option { + match fetched { Fetched::Failed => None, Fetched::Token(token) if is_plausible(&token) && (!(state.rejected && state.token.as_deref() == Some(token.as_str())) || check(&token) == Refusal::Forbidden) => { - *state = State { - token: Some(token.clone()), - ..State::default() - }; + if state.rejected || state.token.as_deref() != Some(token.as_str()) { + *state = State { + token: Some(token.clone()), + ..State::default() + }; + } Some(token) } // The rejected token again and still not valid, something that is @@ -573,6 +651,31 @@ mod tests { assert_eq!(state, saved("pk.old", true), "the schedule is untouched"); } + #[test] + fn a_forced_fetch_of_the_kept_token_keeps_what_is_known() { + let mut state = State { + cooldowns: BTreeMap::from([( + "telemetry".to_owned(), + Cooldown { + failures: 1, + until: NOW, + }, + )]), + ..saved("pk.kept", false) + }; + let before = state.clone(); + assert_eq!( + judge(&mut state, NOW, token("pk.kept"), |_| Refusal::Dead), + Some("pk.kept".to_owned()) + ); + assert_eq!(state, before); + assert_eq!( + judge(&mut state, NOW, token("pk.new"), |_| Refusal::Dead), + Some("pk.new".to_owned()) + ); + assert_eq!(state, saved("pk.new", false), "a new token starts over"); + } + #[test] fn a_wait_beyond_an_hour_is_not_honored() { let mut state = State { @@ -646,6 +749,21 @@ mod tests { assert_eq!(merged(&before, failed, counted), None); } + #[test] + fn no_token_says_whether_one_was_ever_revoked() { + let reason = |revoked| Unavailable::NoToken { revoked }.to_string(); + assert!( + reason(true).starts_with("it is revoked"), + "{}", + reason(true) + ); + assert!( + reason(false).starts_with("the token endpoint did not return"), + "{}", + reason(false) + ); + } + #[test] fn only_a_dead_token_is_replaced() { for code in [ diff --git a/src/executor.rs b/src/executor.rs index 6664141..38687d0 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -5,6 +5,7 @@ use std::time::Duration; use anyhow::{anyhow, Result}; use clap::ArgMatches; +use crate::cli_token; use crate::confirm; use crate::http; use crate::link; @@ -107,6 +108,9 @@ pub struct RunFlags { /// it means "nobody said"; [`crate::http::budget`] then falls back to /// whatever default fits the request. pub timeout: Option, + /// Sent as the request's `Referer`, for a token restricted to a URL: + /// the CLI's own ([`crate::cli_token::REFERER`]). `None` for the user's. + pub referer: Option<&'static str>, } /// Runs one generated command — the request it would make, either sent or @@ -142,6 +146,7 @@ fn dispatch( assume_yes, dry_run, timeout, + referer, } = flags; let client = http::client_for(Some(op.service.as_str()))?; @@ -212,7 +217,12 @@ fn dispatch( let data = data_argument .as_ref() .map(|argument| argument.body.as_ref()); - let merged_body = merge_body_fields(&op.body_fields, matches, data)?; + let body_fields = op + .body_fields + .iter() + .chain(&op.body_positional) + .chain(&op.fixed_body_fields); + let merged_body = merge_body_fields(body_fields, matches, data, op.signed_body_field)?; let variadic_body = variadic_body_array_json(&op.variadic_body_array, matches); let data = variadic_body.as_deref().or(merged_body.as_deref()).or(data); let files: Vec<&str> = matches @@ -241,7 +251,8 @@ fn dispatch( "Note: no access token was resolved, so the real request would be unauthenticated.", ); } - return describe_request(op, mode, &url, &query, body_source.as_ref()); + let sent_with = token.is_some().then_some(referer); + return describe_request(op, mode, &url, &query, body_source.as_ref(), sent_with); } // Placed after the dry-run branch deliberately: a dry run sends @@ -257,6 +268,9 @@ fn dispatch( // request actually went out — never one that only looks like it did. if debug { eprintln!("[debug] {} {}", op.method, redacted_url(&url, &query)); + if let Some(referer) = referer { + eprintln!("[debug] Referer: {referer}"); + } } let method = op.method.as_str(); @@ -286,6 +300,9 @@ fn dispatch( ), )); + if let Some(referer) = referer { + req = req.header(reqwest::header::REFERER, referer); + } if let Some(source) = body_source { req = attach_body(req, source)?; } @@ -692,20 +709,40 @@ fn shown_value<'a>(name: &str, value: &'a str) -> &'a str { /// a note about it. So `-o json` gives a script an object to check before /// letting a real delete run, and a terminal gets one readable request /// line. +/// +/// `sent_with` is the token's `referer`, when there is a token: `Some(Some(_))` +/// for the CLI's own, `Some(None)` for the user's. fn describe_request( op: &Operation, mode: Mode, url: &str, query: &[(String, String)], body: Option<&BodySource<'_>>, + sent_with: Option>, ) -> Result<()> { let method = op.method.to_ascii_uppercase(); - let described = body.map(describe_body).transpose()?.flatten(); + // A positional body is the whole body, typed as an argument: there is + // no `--data` it came from, and nothing else shows the text a person + // is about to send. + let from_arguments = op.body_positional.is_some(); + let described = body + .map(|body| describe_body(body, from_arguments)) + .transpose()? + .flatten(); + // Only for a command that may go out on either token, where which one + // is not obvious from what was typed. + let token = sent_with.filter(|_| cli_token::may_use(&op.command())); let mut text = format!( "Dry run — nothing was sent.\n{method} {}", redacted_url(url, query) ); + if let Some(referer) = token { + text.push_str(&match referer { + Some(referer) => format!("\nToken: the CLI's own, sent with Referer {referer}"), + None => "\nToken: yours".to_owned(), + }); + } if let Some(described) = &described { text.push('\n'); text.push_str(&described.text); @@ -723,21 +760,21 @@ fn describe_request( }) .collect(); - output::emit( - mode, - &text, - serde_json::json!({ - "dry_run": true, - "command": op.command(), - "method": method, - // No query string here — `query` carries that separately. - // Rejoining them would produce a URL nobody can use, since the - // token in it is redacted. - "url": url, - "query": shown_query, - "body": described.map(|described| described.json), - }), - ) + let mut json = serde_json::json!({ + "dry_run": true, + "command": op.command(), + "method": method, + // No query string here — `query` carries that separately. + // Rejoining them would produce a URL nobody can use, since the + // token in it is redacted. + "url": url, + "query": shown_query, + "body": described.map(|described| described.json), + }); + if let Some(referer) = token { + json["token"] = serde_json::json!(if referer.is_some() { "cli" } else { "user" }); + } + output::emit(mode, &text, json) } /// A request body, as the dry run reports it. @@ -756,13 +793,30 @@ struct DescribedBody { /// call — exactly the surprise `--dry-run` is meant to rule out. The /// operations that take `--file` (sprites, upload chunks) are small, so /// the read costs nothing worth skipping. -fn describe_body(source: &BodySource<'_>) -> Result> { +/// +/// `from_arguments` when a JSON body was built from the command's arguments +/// alone, with no `--data`: it is then shown in full, in text as well. +fn describe_body(source: &BodySource<'_>, from_arguments: bool) -> Result> { Ok(match source { BodySource::Empty => None, BodySource::Json(data) => { let json: serde_json::Value = serde_json::from_str(data).map_err(|e| { CliError::new("invalid_data", format!("Invalid JSON for --data: {e}")) })?; + if from_arguments { + return Ok(Some(DescribedBody { + text: format!( + "Body ({JSON_CONTENT_TYPE}):\n{}", + serde_json::to_string_pretty(&json)? + ), + json: serde_json::json!({ + "source": "arguments", + "content_type": JSON_CONTENT_TYPE, + "bytes": data.len(), + "json": json, + }), + })); + } Some(DescribedBody { text: format!( "Body: {} bytes of {JSON_CONTENT_TYPE} from --data", @@ -1002,12 +1056,15 @@ fn variadic_body_array_json( /// `--data` with the body-field flags set on it, or `None` if nothing was /// set. A flag wins over `--data`; a default fills only what neither set. -fn merge_body_fields( - fields: &[Parameter], +/// The `signed` property, when one was set, carries [`signature`]. +fn merge_body_fields<'a>( + fields: impl IntoIterator, matches: &ArgMatches, data: Option<&str>, + signed: Option<&str>, ) -> Result> { - if fields.is_empty() { + let mut fields = fields.into_iter().peekable(); + if fields.peek().is_none() { return Ok(None); } @@ -1042,6 +1099,9 @@ fn merge_body_fields( (None, _) => continue, }; let value = match field.numeric { + None if signed == Some(field.name.as_str()) => { + serde_json::Value::String(format!("{raw}{}", signature())) + } None => serde_json::Value::String(raw.clone()), Some(_) => serde_json::from_str::(raw) .map(serde_json::Value::Number) @@ -1060,6 +1120,17 @@ fn merge_body_fields( Ok(changed.then(|| serde_json::Value::Object(body).to_string())) } +/// What a [`crate::spec::Operation::signed_body_field`] ends with: the CLI's +/// version and OS, which a report about the CLI needs and the Feedback API +/// has no field for. +fn signature() -> String { + format!( + "\n\n— mapbox-cli {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ) +} + /// Picks between `--data` and `--file` for an operation that takes a body. /// /// Kept pure: every rejection here is a mistake the caller can fix from @@ -2018,7 +2089,8 @@ mod tests { /// let it pass and fail later. #[test] fn a_dry_run_rejects_the_json_that_sending_would_reject() { - let err = describe_body(&BodySource::Json("{oops")).expect_err("invalid JSON is refused"); + let err = + describe_body(&BodySource::Json("{oops"), false).expect_err("invalid JSON is refused"); assert_eq!(refusal(err).code, "invalid_data"); } @@ -2026,10 +2098,13 @@ mod tests { /// a metadata check would wave an unreadable file through. #[test] fn a_dry_run_rejects_a_file_that_cannot_be_read() { - let err = describe_body(&BodySource::Raw { - path: "/nonexistent/zz-clitest.svg", - content_type: "image/svg+xml", - }) + let err = describe_body( + &BodySource::Raw { + path: "/nonexistent/zz-clitest.svg", + content_type: "image/svg+xml", + }, + false, + ) .expect_err("an unreadable file is refused"); assert_eq!(refusal(err).code, "invalid_file"); } @@ -2037,7 +2112,7 @@ mod tests { /// A "Body:" line on every `delete` would just be noise. #[test] fn a_body_that_was_never_given_is_not_described() { - assert!(describe_body(&BodySource::Empty) + assert!(describe_body(&BodySource::Empty, false) .expect("an absent body is not an error") .is_none()); } @@ -2055,10 +2130,13 @@ mod tests { std::fs::write(&icon, b"").expect("write icon"); let path = icon.to_str().expect("utf-8 temp path"); - let described = describe_body(&BodySource::Multipart { - paths: vec![path], - field: "images", - }) + let described = describe_body( + &BodySource::Multipart { + paths: vec![path], + field: "images", + }, + false, + ) .expect("a readable file describes") .expect("a multipart body is described"); @@ -2302,7 +2380,7 @@ mod tests { .arg(clap::Arg::new("lat").long("lat")) .arg(clap::Arg::new("category").long("category")) .get_matches_from(std::iter::once("create").chain(args.iter().copied())); - merge_body_fields(&fields, &matches, data) + merge_body_fields(&fields, &matches, data, None) } #[test] @@ -2310,6 +2388,25 @@ mod tests { assert_eq!(merge(&[], Some(r#"{"lat": 1.50}"#)).unwrap(), None); } + #[test] + fn a_signed_field_ends_with_the_version_and_os() { + let matches = clap::Command::new("report") + .arg(clap::Arg::new("category").long("category")) + .get_matches_from(["report", "--category", "bug"]); + let body = merge_body_fields(&[param("category")], &matches, None, Some("category")) + .unwrap() + .unwrap(); + let body: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!( + body["category"], + format!( + "bug\n\n— mapbox-cli {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ) + ); + } + #[test] fn a_default_fills_only_what_nothing_else_set() { let body = |args: &[&str], data| -> serde_json::Value { diff --git a/src/help_layout.rs b/src/help_layout.rs index 3561fba..99acdde 100644 --- a/src/help_layout.rs +++ b/src/help_layout.rs @@ -51,6 +51,7 @@ const GROUPS: &[(&str, &[&str])] = &[ "completion", "tilesets-cli", "uninstall", + "report", "help", ], ), @@ -95,6 +96,7 @@ const DESCRIPTIONS: &[(&str, &str)] = &[ "generate-skills", "Write Agent Skills describing this CLI's own commands", ), + ("report", "Send feedback about this CLI to Mapbox"), ("help", "Print help for mapbox or a command"), ]; diff --git a/src/main.rs b/src/main.rs index 7328ea7..3e37c13 100644 --- a/src/main.rs +++ b/src/main.rs @@ -7,6 +7,15 @@ use clap::builder::{ use clap::error::{ContextKind, ContextValue, ErrorKind}; use clap::{Arg, ArgAction, ArgMatches, Command}; +/// The repository's URL, as a macro so that `concat!` can build links on it +/// in a `const`, such as `remedy`'s table. Defined ahead of the modules, +/// which see it only from here on. +macro_rules! repo_url { + () => { + "https://github.com/mapbox/cli" + }; +} + mod account_usage; mod agent_detect; mod agent_skills; @@ -56,7 +65,7 @@ use spec::ServiceSpec; /// The two installers keep a copy of their own each. They are downloaded and /// run on their own, with nothing of this crate beside them, so this is the /// single source for the Rust half rather than for all three. -const REPO_URL: &str = "https://github.com/mapbox/cli"; +const REPO_URL: &str = repo_url!(); /// Opens the top-level help when an agent is driving the CLI. Agents explore /// by chaining `--help` calls, one command at a time; `--schema` answers the @@ -286,8 +295,9 @@ fn build_operation_command(op: &spec::Operation) -> Command { // A variadic positional builds the whole body on its own — see // below — so `--data` would be a second, redundant way to say the // same thing, with nothing else the body ever carries. - let takes_data = - (body.accepts_json() || text_body.is_some()) && op.variadic_body_array.is_none(); + let takes_data = (body.accepts_json() || text_body.is_some()) + && op.variadic_body_array.is_none() + && op.body_positional.is_none(); if takes_data { cmd = cmd.arg( Arg::new("data") @@ -349,6 +359,18 @@ fn build_operation_command(op: &spec::Operation) -> Command { cmd = cmd.arg(arg); } + // `merge_body_fields` reads it by `arg_name`, the same as a flag. A + // `fixed_body_fields` entry has no argument at all; its default is what + // gets sent. + if let Some(field) = &op.body_positional { + cmd = cmd.arg( + Arg::new(field.arg_name.clone()) + .required(true) + .value_parser(not_blank) + .help(help_text(field)), + ); + } + if let Some(variadic) = &op.variadic_body_array { let help = variadic .description @@ -954,6 +976,7 @@ fn cli() -> u8 { /// error that makes it matter most. fn report_parse_result(err: clap::Error, raw_argv: &[std::ffi::OsString]) -> u8 { let err = drop_subcommand_from_short_circuit_usage(err); + let err = whole_value_after_double_dash(err, raw_argv); // Clap uses 2 for a usage error and 0 for help/version; preserving that // is more useful to a caller than flattening everything to 1. @@ -1138,6 +1161,53 @@ fn drop_subcommand_from_short_circuit_usage(mut err: clap::Error) -> clap::Error err } +/// A positional body field is what the command is for, so an empty one +/// would send a request with nothing in it. +fn not_blank(value: &str) -> std::result::Result { + if value.trim().is_empty() { + return Err("it is empty".to_owned()); + } + Ok(value.to_owned()) +} + +/// Rewrites clap's tip for a value that starts with `-`, such as +/// `mapbox report "-x is broken"`. +/// +/// Clap names only the flag-like start of it (`-x`) and suggests +/// `'-- -x'`, which would send `-x` alone. The tip has to quote the +/// argument as typed, and say that flags go before the `--`: everything +/// after it is a value, so a `--dry-run` there would become part of the +/// message. Left alone for any other tip, such as a misspelled flag's. +fn whole_value_after_double_dash(mut err: clap::Error, argv: &[std::ffi::OsString]) -> clap::Error { + if err.kind() != ErrorKind::UnknownArgument { + return err; + } + let passes_as_value = matches!( + err.get(ContextKind::Suggested), + Some(ContextValue::StyledStrs(tips)) + if tips.iter().any(|tip| tip.to_string().starts_with("to pass ")) + ); + let Some(ContextValue::String(start)) = err.get(ContextKind::InvalidArg) else { + return err; + }; + if !passes_as_value { + return err; + } + let Some(typed) = argv + .iter() + .filter_map(|arg| arg.to_str()) + .find(|arg| arg.starts_with(start.as_str())) + else { + return err; + }; + let mut tip = StyledStr::new(); + tip.push_str(&format!( + "to pass '{typed}' as a value, put '--' before it, and every flag before the '--'" + )); + err.insert(ContextKind::Suggested, ContextValue::StyledStrs(vec![tip])); + err +} + /// Whether `text` is nothing but ANSI style sequences (`ESC [ … m`). fn is_style_codes(text: &str) -> bool { without_trailing_style_codes(text).is_empty() @@ -1456,6 +1526,51 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - deprecation::warn_endpoint(op, &op.command()); let dry_run = executor::wants_dry_run(op_matches); + let mut flags = executor::RunFlags { + debug, + assume_yes, + dry_run, + // Read here rather than beside the other globals at the + // top, so that `MAPBOX_TIMEOUT` is only looked at by a + // command it can apply to. A warning about an unreadable + // one in front of `generate-skills`, which sends nothing, + // would be a note about a request that is not happening. + timeout: http::requested(matches), + referer: None, + }; + let command = op.command(); + + // Ahead of the user's credentials, so a run the CLI's token + // serves never refreshes a login it will not send. A typed + // `--token` or `--use-login` says which token to send, so the + // CLI's is not tried at all. + let mut cli_failure = None; + let chose_own_token = use_login || auth::typed_token(matches).is_some(); + let cli = (!chose_own_token) + .then(|| cli_token::for_command(&command, !dry_run)) + .flatten(); + if let Some(cli) = cli { + let username = matches.get_one::("username").cloned(); + let attempt = try_cli_token( + op, + op_matches, + &command, + cli, + username.as_deref(), + flags, + mode, + ); + match attempt { + CliAttempt::Done(result) => return result, + CliAttempt::Unusable { error, why, sent } => { + // The user already confirmed the request that was + // refused; sending it again with their own token is + // the same request, not a second one to agree to. + flags.assume_yes |= sent; + cli_failure = Some((error, why)); + } + } + } // Resolve token: explicit flag/env takes priority, then stored // credentials (auto-refreshed, except under `--dry-run` — a stale @@ -1475,6 +1590,16 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - } .or_else(|| stored_creds.as_ref().map(|c| c.access_token.clone())); + // A command that could not use the CLI's token falls back to the + // user's, and with none says why the CLI's failed rather than + // sending a request bound to be refused. A dry run sends nothing + // and fetches nothing, so it plans with whatever there is. + if let Some((failure, why)) = cli_failure.filter(|_| !dry_run) { + if token.is_none() { + return Err((*failure).with_remedy(no_own_token_remedy()).into()); + } + output::progress(&format!("{why} Using your own token instead.")); + } if use_login && token.is_none() { return Err(no_stored_credentials(profile)); } @@ -1491,17 +1616,7 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - op_matches, token.as_deref(), username.as_deref(), - executor::RunFlags { - debug, - assume_yes, - dry_run, - // Read here rather than beside the other globals at the - // top, so that `MAPBOX_TIMEOUT` is only looked at by a - // command it can apply to. A warning about an unreadable - // one in front of `generate-skills`, which sends nothing, - // would be a note about a request that is not happening. - timeout: http::requested(matches), - }, + flags, mode, ) // The executor does not know where its token came from, and that @@ -1521,10 +1636,178 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - Ok(()) } +/// How [`try_cli_token`] ended. +enum CliAttempt { + /// The CLI's token was used; this is the command's own result. + Done(Result<()>), + /// It could not be used, and the user's token may stand in: `error` is + /// what the command fails with when there is none, and `why` says what + /// happened when there is. `sent` when a request went out and was + /// refused, so any confirmation was already given. + Unusable { + error: Box, + why: String, + sent: bool, + }, +} + +/// Runs a [`cli_token::Use::Preferred`] command on the CLI's token, given +/// what [`cli_token::for_command`] found for it, by the rules in +/// [`cli_token::send_with_cli_token`]. +fn try_cli_token( + op: &spec::Operation, + op_matches: &clap::ArgMatches, + command: &str, + cli: Result, + username: Option<&str>, + mut flags: executor::RunFlags, + mode: output::Mode, +) -> CliAttempt { + let token = match cli { + Ok(token) => token, + Err(why) => { + let error = cli_token_unavailable(command, &why); + return CliAttempt::Unusable { + why: error.message.clone(), + error: Box::new(error), + sent: false, + }; + } + }; + // Only for the CLI's token: the user's, which may stand in after, goes + // out without it. + flags.referer = Some(cli_token::REFERER); + let execute = + |token: &str, flags| executor::execute(op, op_matches, Some(token), username, flags, mode); + // Sends nothing, so there is nothing to be refused, or to count. + if flags.dry_run { + return CliAttempt::Done(execute(&token.token, flags)); + } + let sent = cli_token::send_for_command( + command, + token, + |token| { + let result = execute(token, flags); + // Confirmed once already, if the command asks. + flags.assume_yes = true; + result + }, + |result| match result { + Ok(()) => Some(200), + Err(error) => error.downcast_ref::().and_then(|e| e.status), + }, + ); + let (result, refusal) = match sent { + cli_token::Sent::Answered(result) => match rate_limited(result) { + // Every install shares the CLI's token, so a limit on it says + // nothing about the user's. A `429` was not acted on, so sending + // it again is not a second report; with no token of their own, + // the `429` is the answer. + Ok(error) => { + return CliAttempt::Unusable { + why: provided_token(command, "Mapbox is rate-limiting it right now"), + error, + sent: true, + } + } + Err(result) => return CliAttempt::Done(result), + }, + cli_token::Sent::Refused(result, refusal) => (result, refusal), + }; + let (code, reason) = match refusal { + cli_token::Refusal::Dead => ( + "cli_token_unavailable", + "it is revoked and no new one is published yet", + ), + cli_token::Refusal::Forbidden => ("cli_token_forbidden", "this API does not accept it"), + cli_token::Refusal::Unknown => ( + "cli_token_forbidden", + "this API refused it, and checking why failed", + ), + // Refused, and not checked why: `MAPBOX_CLI_TOKEN`, or the token on + // disk changed meanwhile or could not be written. + cli_token::Refusal::NotFetched => ("cli_token_forbidden", "this API refused it"), + }; + let request_id = result + .err() + .and_then(|e| e.downcast::().ok()) + .and_then(|e| e.request_id); + let message = provided_token(command, reason); + CliAttempt::Unusable { + why: message.clone(), + error: Box::new(CliError::new(code, message).with_request_id(request_id)), + sent: true, + } +} + +/// The error of a request the API answered `429`, or the result as it was. +fn rate_limited(result: Result<()>) -> std::result::Result, Result<()>> { + match result { + Err(error) if error.downcast_ref::().and_then(|e| e.status) == Some(429) => Ok( + Box::new(error.downcast::().expect("checked above")), + ), + result => Err(result), + } +} + +/// Why `command` could not use the token the CLI provides, in words for +/// someone who has never heard of it: what it is, then `reason`. +fn provided_token(command: &str, reason: &str) -> String { + format!( + "`mapbox {command}` sends with a token the CLI provides, so it needs no login, \ + but {reason}." + ) +} + +/// Why there was no CLI token to try, as the error a command fails with +/// when the user has no token of their own either. +fn cli_token_unavailable(command: &str, why: &cli_token::Unavailable) -> CliError { + let reason = match why { + cli_token::Unavailable::Unreachable => { + "cli.mapbox.com, where the CLI gets it, could not be reached" + } + cli_token::Unavailable::NoToken { revoked: true } => { + "it is revoked and no new one is published yet" + } + cli_token::Unavailable::NoToken { revoked: false } => { + "cli.mapbox.com did not return a usable one" + } + cli_token::Unavailable::NotFetched => "it has not been fetched yet", + }; + CliError::new("cli_token_unavailable", provided_token(command, reason)) +} + +/// What to do when a command could not use the CLI's token and the user has +/// none of their own: give it theirs. +fn no_own_token_remedy() -> Remedy { + Remedy::default() + .with_fix("Log in with `mapbox auth login`, or pass `--token`, and it is sent with yours.") + .with_action(Some("mapbox auth login".to_owned())) +} + #[cfg(test)] mod tests { use super::*; + /// Every install shares the CLI's token, so its rate limit is no reason + /// to fail someone whose own token would work; nothing else is. + #[test] + fn only_a_429_on_the_clis_token_falls_back() { + let failed = |status: u16| -> Result<()> { + Err(CliError { + status: Some(status), + ..CliError::new(format!("http_{status}"), "Refused") + } + .into()) + }; + let error = rate_limited(failed(429)).expect("a 429 falls back"); + assert_eq!(error.code, "http_429"); + for status in [400, 404, 500] { + assert!(rate_limited(failed(status)).is_err(), "{status}"); + } + assert!(matches!(rate_limited(Ok(())), Err(Ok(())))); + } + /// Half the world has a negative longitude. Clap reads `-74.0` as a /// cluster of short flags unless told otherwise, which made every /// operation taking a coordinate unusable west of Greenwich — as a diff --git a/src/output/render.rs b/src/output/render.rs index eda817b..3e69f2a 100644 --- a/src/output/render.rs +++ b/src/output/render.rs @@ -33,10 +33,46 @@ pub(super) fn list_rendering(value: &Value, service: Option<&str>) -> Option tilequery_feature_rows(value).map(|rows| render_feature_list(&rows)), + Some("report") => render_report(value), _ => None, } } +/// `report`'s answer, as what the sender can use: that it went, what was +/// sent, and the id to quote. The rest is the Feedback API's record of it — +/// timestamps, a status that is always `received`, the `0,0` this command +/// always sends, a screenshot it never attaches — none of which the sender +/// chose or can act on. `-o json` still has all of it. +/// +/// `None` for an answer without both, which then falls back to the whole +/// field list rather than hiding what came back. +fn render_report(value: &Value) -> Option { + const SENT: &str = "Sent to the Mapbox CLI team."; + let map = value.as_object()?; + // The message is several lines once the CLI's version and OS are on + // it; each goes under the first, not back at the label column. + let indent = format!("\n{}", " ".repeat("feedback".len() + 2)); + let feedback = map + .get("feedback")? + .as_str()? + .split('\n') + .collect::>() + .join(&indent) + .replace(&format!("{indent}{indent}"), &format!("\n{indent}")); + let fields = vec![ + ("feedback".to_owned(), feedback), + ("id".to_owned(), map.get("id")?.as_str()?.to_owned()), + ]; + Some(Rendered { + text: format!("{SENT}\n{}", field_lines(&fields, false)), + header: false, + fields: None, + colored: Some(format!("{SENT}\n{}", field_lines(&fields, true))), + shortened: false, + identifier: None, + }) +} + /// A human rendering, what it had to cut, and the first identifier in it. pub(super) struct Rendered { pub(super) text: String, @@ -2327,4 +2363,60 @@ mod tests { Some("Food and Drink".to_string()) ); } + + #[test] + fn a_report_shows_what_was_sent_and_its_id_alone() { + let answer = serde_json::json!({ + "id": "01a15433-408f-708c-929f-cad22dd7ef88", + "created_at": "2026-10-19T12:46:47.466Z", + "updated_at": "2026-10-19T12:46:47.466Z", + "received_at": "2026-10-19T12:46:47.466Z", + "feedback": "test from CLI", + "category": "general", + "status": "received", + "location": { "lat": 0, "lon": 0 }, + "has_screenshot": false + }); + let rendered = list_rendering(&answer, Some("report")).expect("a rendering"); + let lines: Vec<&str> = rendered.text.lines().collect(); + assert_eq!(lines[0], "Sent to the Mapbox CLI team."); + assert_eq!(lines.len(), 3, "{}", rendered.text); + assert!(lines[1].starts_with("feedback") && lines[1].ends_with("test from CLI")); + assert!( + lines[2].starts_with("id") + && lines[2].ends_with("01a15433-408f-708c-929f-cad22dd7ef88") + ); + for hidden in [ + "created_at", + "status", + "location", + "has_screenshot", + "category", + ] { + assert!(!rendered.text.contains(hidden), "{hidden} shown"); + } + } + + #[test] + fn a_reports_later_lines_line_up_under_its_first() { + let answer = serde_json::json!({ + "id": "an-id", + "feedback": "A test\n\n— mapbox-cli 0.3.0 on macos", + }); + let rendered = list_rendering(&answer, Some("report")).expect("a rendering"); + assert_eq!( + rendered.text, + "Sent to the Mapbox CLI team.\n\ + feedback A test\n\ + \n \ + — mapbox-cli 0.3.0 on macos\n\ + id an-id" + ); + } + + #[test] + fn a_report_answer_missing_its_id_is_shown_whole() { + let answer = serde_json::json!({ "feedback": "test", "status": "received" }); + assert!(list_rendering(&answer, Some("report")).is_none()); + } } diff --git a/src/remedy.rs b/src/remedy.rs index 578023d..506961b 100644 --- a/src/remedy.rs +++ b/src/remedy.rs @@ -95,6 +95,12 @@ const SERVICE_DOCS: &[(&str, &str)] = &[ ), ("matrix", "https://docs.mapbox.com/api/navigation/matrix/"), ("places", "https://docs.mapbox.com/api/search/places/"), + // Undocumented on docs.mapbox.com by design (see `report.yaml`), so + // this repository's own page is the only one. + ( + "report", + concat!(repo_url!(), "/blob/main/docs/commands.md#mapbox-report"), + ), ("search", "https://docs.mapbox.com/api/search/search-box/"), // Static Images and Static Tiles merged into one `static` command group // (#116); neither upstream page covers both, so this points at Static diff --git a/src/schema.rs b/src/schema.rs index 6e1edf8..2f8387d 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -478,7 +478,7 @@ fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry body_arguments( body, !op.body_fields.is_empty(), - op.variadic_body_array.is_some(), + op.variadic_body_array.is_some() || op.body_positional.is_some(), declared, ) }) @@ -491,6 +491,12 @@ fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry ..parameter(field, ArgKind::Option) })); + arguments.extend(op.body_positional.iter().map(|field| Argument { + required: true, + location: Some("body"), + ..parameter(field, ArgKind::Positional) + })); + arguments.extend(op.variadic_body_array.as_ref().map(|variadic| Argument { required: true, repeatable: true, diff --git a/src/spec.rs b/src/spec.rs index c3bc8ec..ec9e189 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -33,6 +33,15 @@ pub struct Operation { /// A single JSON body array field exposed as one or more positional /// arguments instead of `--data`. See [`VARIADIC_BODY_ARRAY`]. pub variadic_body_array: Option, + /// A JSON body property taken as the command's positional argument. + /// See [`POSITIONAL_BODY_FIELDS`]. + pub body_positional: Option, + /// JSON body properties sent with a fixed value and never typed. See + /// [`FIXED_BODY_FIELDS`]. + pub fixed_body_fields: Vec, + /// A string body property sent with the CLI's version and OS appended. + /// See [`SIGNED_BODY_FIELDS`]. + pub signed_body_field: Option<&'static str>, pub base_url: String, /// Set when the operation needs a scope that isn't registrable as OAuth /// (see `UNSUPPORTED_OPERATIONS` below). A `mapbox auth login` token can @@ -83,16 +92,43 @@ type BodyField = (&'static str, Option<&'static str>); /// The API requires `lat`/`lon`, but some feedback has no place. The /// Feedback API team's advice (2026-10-06) is to send 0,0 until the pair /// becomes optional; drop the defaults then. -const BODY_FIELD_FLAGS: &[(&str, &str, &[BodyField])] = &[( - "feedback", - "create", - &[ - ("feedback", None), - ("lat", Some("0")), - ("lon", Some("0")), - ("category", None), - ], -)]; +const BODY_FIELD_FLAGS: &[(&str, &str, &[BodyField])] = &[ + ( + "feedback", + "create", + &[ + ("feedback", None), + ("lat", Some("0")), + ("lon", Some("0")), + ("category", None), + ], + ), + ("report", "report", &[("category", Some("general"))]), +]; + +/// (service, operationId, body field name, CLI positional name) for a +/// required string body property that is what the command is for, typed as +/// its positional argument: `mapbox report ""`, not `--feedback`. +/// An operation with one takes no `--data`, the same as +/// [`VARIADIC_BODY_ARRAY`]'s. +const POSITIONAL_BODY_FIELDS: &[(&str, &str, &str, &str)] = + &[("report", "report", "feedback", "message")]; + +/// A body property and the value always sent for it. +type FixedBodyField = (&'static str, &'static str); + +/// JSON body properties sent with the same value on every request and never +/// offered as a flag, for a property the API requires but the command has no +/// use for. `report` sends feedback about the CLI, which has no place, and +/// the API still requires `lat`/`lon` — see [`BODY_FIELD_FLAGS`] for why 0. +const FIXED_BODY_FIELDS: &[(&str, &str, &[FixedBodyField])] = + &[("report", "report", &[("lat", "0"), ("lon", "0")])]; + +/// (service, operationId, body field name) for a string body property sent +/// with the CLI's version and OS appended. A report about the CLI is little +/// use without them, and the Feedback API has no field of its own for +/// either, so they go in the text, where `--dry-run` shows them too. +const SIGNED_BODY_FIELDS: &[(&str, &str, &str)] = &[("report", "report", "feedback")]; /// (service, operationId, body field name, CLI positional name, max count) /// for an operation whose JSON body takes a single array field, exposed as @@ -238,8 +274,14 @@ fn arg_name_override(service_name: &str, param_name: &str) -> Option<&'static st /// `generate-skills`, this file's own `command()` above — reads a /// [`FLATTENED_SERVICES`] service correctly for free, because they all go /// through `command()` rather than reconstructing the string themselves. -pub const FLATTENED_SERVICES: &[&str] = - &["directions", "isochrone", "map-match", "matrix", "places"]; +pub const FLATTENED_SERVICES: &[&str] = &[ + "directions", + "isochrone", + "map-match", + "matrix", + "places", + "report", +]; /// (service, path parameter name) pairs whose value is trusted to reach the /// URL unescaped, because every legitimate value already contains a @@ -827,6 +869,10 @@ pub const CUSTOM_SPEC_ENTRIES: &[SpecEntry] = &[ name: "places", yaml: include_str!("../custom-openapi/places/openapi/places.yaml"), }, + SpecEntry { + name: "report", + yaml: include_str!("../custom-openapi/report/openapi/report.yaml"), + }, ]; /// The list the CLI actually generates commands from: [`MAPBOX_SPEC_ENTRIES`], @@ -1170,6 +1216,30 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { None => None, }; + let body_positional = + match operation_id.and_then(|id| positional_body_field_names(service_name, id)) { + Some((field, arg_name)) => Some( + positional_body_field_parameter(op, &doc, field, arg_name).with_context( + || format!("{service_name} {}", operation_id.unwrap_or("")), + )?, + ), + None => None, + }; + + let fixed_body_fields = match operation_id + .and_then(|id| fixed_body_field_values(service_name, id)) + { + Some(values) => { + let fields: Vec = values + .iter() + .map(|(name, value)| (*name, Some(*value))) + .collect(); + body_field_parameters(op, &doc, &fields) + .with_context(|| format!("{service_name} {}", operation_id.unwrap_or("")))? + } + None => vec![], + }; + let disabled_scope = operation_id.and_then(|id| unsupported_scope_for(service_name, id)); @@ -1189,6 +1259,10 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { body, body_fields, variadic_body_array, + body_positional, + fixed_body_fields, + signed_body_field: operation_id + .and_then(|id| signed_body_field_name(service_name, id)), base_url: base_url.clone(), disabled_scope, detail: None, @@ -1279,6 +1353,60 @@ fn body_field_names(service: &str, operation_id: &str) -> Option<&'static [BodyF .map(|(_, _, names)| *names) } +fn positional_body_field_names( + service: &str, + operation_id: &str, +) -> Option<(&'static str, &'static str)> { + POSITIONAL_BODY_FIELDS + .iter() + .find(|(s, id, _, _)| *s == service && *id == operation_id) + .map(|(_, _, field, arg_name)| (*field, *arg_name)) +} + +fn signed_body_field_name(service: &str, operation_id: &str) -> Option<&'static str> { + SIGNED_BODY_FIELDS + .iter() + .find(|(s, id, _)| *s == service && *id == operation_id) + .map(|(_, _, field)| *field) +} + +fn fixed_body_field_values(service: &str, operation_id: &str) -> Option<&'static [FixedBodyField]> { + FIXED_BODY_FIELDS + .iter() + .find(|(s, id, _)| *s == service && *id == operation_id) + .map(|(_, _, values)| *values) +} + +/// A [`POSITIONAL_BODY_FIELDS`] entry as a required string [`Parameter`], +/// named for the positional rather than the property. Its description is +/// the property's own: there is no `--data` to carry it instead. +fn positional_body_field_parameter( + op: &Value, + full_spec: &Value, + field: &'static str, + arg_name: &str, +) -> Result { + let parameter = body_field_parameters(op, full_spec, &[(field, None)])?.remove(0); + anyhow::ensure!( + parameter.numeric.is_none() && parameter.enum_values.is_empty(), + "body property `{field}` is listed in POSITIONAL_BODY_FIELDS but is not a free string \ + in the spec" + ); + let schema = &op["requestBody"]["content"]["application/json"]["schema"]; + let schema = match schema["$ref"].as_str() { + Some(reference) => resolve_ref(full_spec, reference).unwrap_or(schema), + None => schema, + }; + Ok(Parameter { + arg_name: arg_name.to_string(), + required: true, + description: schema["properties"][field]["description"] + .as_str() + .map(first_paragraph), + ..parameter + }) +} + /// Nothing enforces `required`, since `--data` may carry the property; the /// description says it instead. fn body_field_parameters( @@ -2054,6 +2182,76 @@ requestBody: assert_eq!(variadic.max, 100); } + /// `report`'s message is a positional and `lat`/`lon` are never flags: + /// the table entries have to reach the parsed operation, typed by the + /// spec, or the request goes out with strings where numbers belong. + #[test] + fn report_takes_its_message_as_a_positional_and_fixes_its_place() { + let spec = parse_spec( + "report", + include_str!("../custom-openapi/report/openapi/report.yaml"), + ) + .expect("report.yaml parses"); + + assert!(FLATTENED_SERVICES.contains(&"report")); + let [op] = spec.operations.as_slice() else { + panic!("report.yaml describes exactly one operation"); + }; + assert_eq!(op.command(), "report"); + + let message = op.body_positional.as_ref().expect("a positional"); + assert_eq!( + (message.name.as_str(), message.arg_name.as_str()), + ("feedback", "message") + ); + assert!(message.required); + assert!(!message + .description + .as_deref() + .unwrap_or("") + .contains("--data")); + + let flags: Vec<&str> = op.body_fields.iter().map(|f| f.name.as_str()).collect(); + assert_eq!(flags, ["category"]); + assert_eq!(op.signed_body_field, Some("feedback")); + + let fixed: Vec<(&str, Option<&str>, Option)> = op + .fixed_body_fields + .iter() + .map(|f| (f.name.as_str(), f.default.as_deref(), f.numeric)) + .collect(); + assert_eq!( + fixed, + [ + ("lat", Some("0"), Some(Numeric::Float)), + ("lon", Some("0"), Some(Numeric::Float)), + ] + ); + } + + #[test] + fn a_positional_body_field_needs_a_free_string() { + let op: Value = serde_yaml::from_str( + r#" +requestBody: + content: + application/json: + schema: + type: object + required: [text] + properties: + text: { type: string, description: "The text." } + lat: { type: number } +"#, + ) + .unwrap(); + + let field = positional_body_field_parameter(&op, &op, "text", "message").unwrap(); + assert_eq!(field.description.as_deref(), Some("The text.")); + assert!(positional_body_field_parameter(&op, &op, "lat", "message").is_err()); + assert!(positional_body_field_parameter(&op, &op, "missing", "message").is_err()); + } + #[test] fn an_operation_with_no_request_body_has_none() { let svc = service(BODIES); diff --git a/tests/dry_run.rs b/tests/dry_run.rs index 052835f..92416b1 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -509,3 +509,394 @@ fn places_refuses_more_than_a_hundred_ids_before_sending() { stderr(&out) ); } + +/// `report`'s message is its positional, `lat`/`lon` are sent without being +/// typed, and it goes to the CLI endpoint, not the one `feedback create` +/// files under the caller's account. +#[test] +fn report_sends_the_message_to_the_cli_endpoint() { + let config = config_dir("report"); + let plan = |extra: &[&str]| { + let mut args = vec![ + "-o", + "json", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "A test", + ]; + args.extend_from_slice(extra); + let out = run(&config, &args); + assert!(out.status.success(), "{}", stderr(&out)); + json(&stdout(&out)) + }; + + let defaults = plan(&[]); + assert_eq!( + defaults["url"], + "https://api.mapbox.com/user-feedback/v1/cli-feedback" + ); + let feedback = format!( + "A test\n\n— mapbox-cli {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ); + assert_eq!( + defaults["body"]["json"], + serde_json::json!({ "feedback": feedback, "lat": 0, "lon": 0, "category": "general" }) + ); + assert_eq!(defaults["body"]["source"], "arguments"); + assert_eq!(defaults["token"], "user"); + + let categorized = plan(&["--category", "bug"]); + assert_eq!(categorized["body"]["json"]["category"], "bug"); +} + +/// A terminal's dry run shows the message as it would be sent, and which +/// token it would go with, rather than only the body's size. +#[test] +fn report_dry_run_shows_the_body_and_token_in_text() { + let config = config_dir("report-text"); + let out = run( + &config, + &[ + "-o", + "text", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "A test", + ], + ); + assert!(out.status.success(), "{}", stderr(&out)); + let text = stdout(&out); + assert!(text.contains("Token: yours"), "{text}"); + assert!( + text.contains(r#""feedback": "A test\n\n— mapbox-cli "#), + "{text}" + ); + assert!(!text.contains("--data"), "{text}"); +} + +/// Clap names only `-x` of `"-x is broken"` and would have it passed alone; +/// the tip quotes the whole message. +#[test] +fn report_says_how_to_send_a_message_starting_with_a_dash() { + let config = config_dir("report-dash"); + let out = run( + &config, + &[ + "-o", + "json", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "-x is broken", + ], + ); + assert_eq!(out.status.code(), Some(2), "{}", stderr(&out)); + let fix = json(&stderr(&out))["fix"] + .as_str() + .unwrap_or_default() + .to_owned(); + assert!( + fix.contains("'-x is broken'") && fix.contains("'--'"), + "{fix}" + ); + + let out = run( + &config, + &[ + "-o", + "json", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "--", + "-x is broken", + ], + ); + assert!(out.status.success(), "{}", stderr(&out)); + let feedback = json(&stdout(&out))["body"]["json"]["feedback"].clone(); + assert!( + feedback + .as_str() + .unwrap_or_default() + .starts_with("-x is broken\n"), + "{feedback}" + ); +} + +/// The positional is the whole body; a `--data` would be a second way to +/// say it, or a way to send a place this endpoint has no use for. An empty +/// message is refused before anything is sent. +#[test] +fn report_needs_a_message_and_takes_no_data() { + let config = config_dir("report-usage"); + for args in [ + &["--token", "sk.a-test-token", "report", "--dry-run"][..], + &["--token", "sk.a-test-token", "report", "--dry-run", ""][..], + &["--token", "sk.a-test-token", "report", "--dry-run", " \n "][..], + &[ + "--token", + "sk.a-test-token", + "report", + "A test", + "--data", + "{}", + "--dry-run", + ][..], + ] { + let out = run(&config, args); + assert_eq!(out.status.code(), Some(2), "{args:?}: {}", stderr(&out)); + assert!(stdout(&out).is_empty(), "{args:?}: {}", stdout(&out)); + } +} + +/// What `report` sends with, and what it says, when it cannot use the CLI's +/// token. Every case here is decided before a request: the token endpoint +/// is a dead port, and a cooldown is written to disk. +mod report_token { + use super::*; + use std::io::{Read, Write}; + use std::net::TcpListener; + + fn now() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("a clock after 1970") + .as_secs() + } + + fn write_state(config: &Path, state: &str) { + std::fs::write(config.join("cli-token.json"), state).expect("write the state"); + } + + fn error_code(out: &Output) -> String { + json(&stderr(out))["code"] + .as_str() + .unwrap_or_default() + .to_owned() + } + + /// A token endpoint on loopback that hands out `token` once. + fn token_endpoint(token: &'static str) -> String { + let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); + let addr = listener.local_addr().expect("the bound address"); + std::thread::spawn(move || { + let Ok((mut stream, _)) = listener.accept() else { + return; + }; + let mut head = Vec::new(); + let mut byte = [0u8; 1]; + while !head.ends_with(b"\r\n\r\n") { + match stream.read(&mut byte) { + Ok(1) => head.push(byte[0]), + _ => break, + } + } + let body = format!(r#"{{"token":"{token}"}}"#); + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n\ + Content-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + }); + format!("http://{addr}/config/token.json") + } + + /// A request that goes nowhere: the proxy is a dead port, so `--debug` + /// shows what was about to be sent without anything reaching Mapbox. + fn sent_nowhere(config: &Path, args: &[&str]) -> Command { + let mut cmd = command(config); + cmd.env("HTTPS_PROXY", "http://127.0.0.1:9") + .env_remove("https_proxy") + .env_remove("ALL_PROXY") + .env_remove("all_proxy") + .env_remove("NO_PROXY") + .env_remove("no_proxy") + .args(args); + cmd + } + + #[test] + fn with_no_token_at_all_it_says_the_clis_is_unavailable() { + let config = config_dir("report-no-tokens"); + let out = run(&config, &["-o", "json", "report", "A test"]); + assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); + assert!(stdout(&out).is_empty(), "{}", stdout(&out)); + assert_eq!(error_code(&out), "cli_token_unavailable"); + let error = json(&stderr(&out)); + assert!( + error["fix"] + .as_str() + .unwrap_or_default() + .contains("--token"), + "{error}" + ); + assert_eq!(error["next_actions"][0], "mapbox auth login"); + assert!( + !error["message"] + .as_str() + .unwrap_or_default() + .contains("CLI's token"), + "{error}" + ); + } + + /// The CLI's token is restricted to its URL, so a request with it says + /// it comes from there. + #[test] + fn the_clis_token_goes_with_its_referer() { + let config = config_dir("report-referer"); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .env("MAPBOX_CLI_TOKEN", "pk.cli") + .output() + .expect("run mapbox"); + let err = stderr(&out); + assert!( + err.contains("[debug] Referer: https://cli.mapbox.com/"), + "{err}" + ); + } + + /// Someone ran it, so a rest telemetry might take does not hold it back: + /// it sends with the CLI's token. + #[test] + fn a_cooldown_does_not_hold_report_back() { + let config = config_dir("report-not-resting"); + write_state( + &config, + &format!( + r#"{{"token":"pk.saved","overridden":true,"cooldowns":{{"report":{{"failures":3,"until":{}}}}}}}"#, + now() + 3600 + ), + ); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .output() + .expect("run mapbox"); + let err = stderr(&out); + assert!( + err.contains("[debug] Referer: https://cli.mapbox.com/"), + "{err}" + ); + assert!(!err.contains("recently"), "{err}"); + } + + /// Nor does the endpoint's backoff: with the kept token dead, it asks + /// for a new one there and then, and sends with it. + #[test] + fn report_fetches_through_the_backoff() { + let config = config_dir("report-through-backoff"); + write_state( + &config, + &format!( + r#"{{"token":"pk.dead","rejected":true,"overridden":true,"failures":3,"retry_at":{}}}"#, + now() + 3600 + ), + ); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .env("MAPBOX_INTERNAL_CLI_TOKEN_URL", token_endpoint("pk.fresh")) + .output() + .expect("run mapbox"); + let err = stderr(&out); + assert!( + err.contains("[debug] Referer: https://cli.mapbox.com/"), + "{err}" + ); + let state = std::fs::read_to_string(config.join("cli-token.json")).expect("the state"); + assert!(state.contains(r#""token":"pk.fresh""#), "{state}"); + } + + /// A command fetches past the backoff, so the reason it gives is what + /// that fetch found, not the backoff's wait, which it never waited for. + #[test] + fn an_unreachable_endpoint_is_reported_through_the_backoff() { + let config = config_dir("report-unreachable-backoff"); + write_state( + &config, + &format!( + r#"{{"overridden":true,"failures":3,"retry_at":{}}}"#, + now() + 3600 + ), + ); + let out = run(&config, &["-o", "json", "report", "A test"]); + assert_eq!(error_code(&out), "cli_token_unavailable"); + let err = stderr(&out); + assert!(err.contains("could not be reached"), "{err}"); + assert!(!err.contains("trying again"), "{err}"); + } + + /// A miss on a command's fetch does not count toward the backoff, which + /// paces telemetry's fetches in the background. + #[test] + fn a_commands_miss_leaves_the_backoff_alone() { + let config = config_dir("report-miss-uncounted"); + let state = r#"{"overridden":true,"failures":2}"#; + write_state(&config, state); + let out = command(&config) + .env( + "MAPBOX_INTERNAL_CLI_TOKEN_URL", + token_endpoint("not-a-token"), + ) + .args(["-o", "json", "report", "A test"]) + .output() + .expect("run mapbox"); + assert_eq!(error_code(&out), "cli_token_unavailable"); + let after = std::fs::read_to_string(config.join("cli-token.json")).expect("the state"); + assert_eq!(after, state); + } + + /// The user's own token is not restricted to anything of ours, and goes + /// without the CLI token's Referer when it stands in. + #[test] + fn the_users_token_goes_without_a_referer() { + let config = config_dir("report-no-referer"); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .env("MAPBOX_ACCESS_TOKEN", "sk.mine") + .output() + .expect("run mapbox"); + let err = stderr(&out); + assert!(err.contains("Using your own token instead"), "{err}"); + assert!(err.contains("[debug] POST"), "{err}"); + assert!(!err.contains("Referer"), "{err}"); + } + + /// A typed `--token` is the user's choice for this run, so the CLI's + /// token is not tried even when there is one. + #[test] + fn a_typed_token_outranks_the_clis() { + let config = config_dir("report-typed-token"); + let out = sent_nowhere( + &config, + &["--debug", "--token", "sk.mine", "report", "A test"], + ) + .env("MAPBOX_CLI_TOKEN", "pk.cli") + .output() + .expect("run mapbox"); + let err = stderr(&out); + assert!(err.contains("[debug] POST"), "{err}"); + assert!(!err.contains("Referer"), "{err}"); + assert!(!err.contains("CLI's token"), "{err}"); + } + + /// A dry run fetches nothing, so with no CLI token kept the plan goes out + /// with the user's. + #[test] + fn a_dry_run_with_no_kept_token_plans_with_the_users() { + let config = config_dir("report-dry-user"); + let out = command(&config) + .env("MAPBOX_ACCESS_TOKEN", "sk.mine") + .args(["report", "--dry-run", "A test"]) + .output() + .expect("run mapbox"); + assert!(out.status.success(), "{}", stderr(&out)); + let err = stderr(&out); + assert!(!err.contains("no access token was resolved"), "{err}"); + } +} diff --git a/tests/fixtures/api_command_surface.txt b/tests/fixtures/api_command_surface.txt index d2bc796..7c002d3 100644 --- a/tests/fixtures/api_command_surface.txt +++ b/tests/fixtures/api_command_surface.txt @@ -15,6 +15,7 @@ mapbox isochrone | aliases: (none) mapbox map-match | aliases: (none) mapbox matrix | aliases: (none) mapbox places | aliases: (none) +mapbox report | aliases: (none) mapbox search category | aliases: (none) mapbox search forward | aliases: (none) mapbox search list-category | aliases: (none)