diff --git a/CHANGELOG.md b/CHANGELOG.md index eb1db80..ecbb112 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,14 @@ the Mapbox APIs' own response bodies are not. gets the 900-second transfer budget rather than the 60-second one, since nothing bounds a file the way a command line bounds what can be typed. +### Changed + +- The advice under a transport failure now names `ALL_PROXY` alongside + `HTTPS_PROXY` and `NO_PROXY`, and says that a SOCKS proxy is not supported. + `ALL_PROXY=socks5://…` fails the request rather than being ignored, and + `unsupported scheme socks5` in the message is the part that distinguishes + it from the network being down. + ## 0.1.8 - 2026-09-14 Initial beta release. The next release is `0.2.0`. diff --git a/README.md b/README.md index c54e17c..7d272fc 100644 --- a/README.md +++ b/README.md @@ -245,12 +245,27 @@ not before. A read-only command rejects it. | Request | Budget | | --- | --- | -| A normal request (`GET`, or a `--data` body) | 60 seconds | -| A `--file` upload | 15 minutes | +| A normal request (`GET`, or a typed `--data` body) | 60 seconds | +| A `--file` upload, or a `--data @` / `@-` body | 15 minutes | `--timeout ` overrides either, `MAPBOX_TIMEOUT` sets it for a whole shell. +### Proxies + +The standard variables are honoured — `HTTPS_PROXY`, `HTTP_PROXY`, +`ALL_PROXY` and `NO_PROXY` — so a CLI behind a corporate proxy needs no +configuration of its own. + +Two things are worth knowing, because both are easy to spend an afternoon on: + +- **`HTTP_PROXY` alone does not carry Mapbox requests.** Every Mapbox base + URL is `https`, and that variable applies to `http` URLs only. Set + `HTTPS_PROXY` (or `ALL_PROXY`) instead. +- **A SOCKS proxy is not supported.** `ALL_PROXY=socks5://…` fails the request + rather than being ignored, and the failure says `unsupported scheme socks5` + — so if you see that, it is the proxy and not the network. + ### Output format `--output`/`-o` (or `MAPBOX_OUTPUT`) picks the shape: diff --git a/docs/commands.md b/docs/commands.md index 9ad9d50..e518a71 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -453,7 +453,7 @@ either. | `--profile ` | Which stored credentials to use. | | `--output`, `-o` | `auto` \| `text` \| `json`. | | `--id ` | On a command that returns a list, print just the row with that `id` or `name`. | -| `--timeout ` | How long one request may take, connection included. Defaults to 60 seconds, or 900 for a body read from `--file`. Also `MAPBOX_TIMEOUT`. | +| `--timeout ` | How long one request may take, connection included. Defaults to 60 seconds, or 900 for a body read from `--file` or from a `--data @`/`@-`. Also `MAPBOX_TIMEOUT`. | An operation with a request body takes `--data`/`-d` when that body is text the caller types — JSON for most, a bare `true`/`false` for `star-file` — @@ -3541,6 +3541,20 @@ names the flag rather than offering a login that could not outrank it. | --- | --- | | `http_` | The API answered non-2xx. Carries `status` and the response `body`. | | `request_failed` | Transport failure — proxy, DNS, TLS. Never carries the URL, because the access token rides in its query string. | + +The CLI honours `HTTPS_PROXY`, `HTTP_PROXY`, `ALL_PROXY` and `NO_PROXY`, and +needs no proxy configuration of its own. Two things that look like network +faults and are not: + +- **`HTTP_PROXY` alone does not carry Mapbox requests.** Every base URL is + `https`, and that variable covers `http` URLs only — so a request goes + direct and a proxy-only network refuses it. `HTTPS_PROXY` or `ALL_PROXY` is + the one to set. +- **SOCKS is not supported.** `ALL_PROXY=socks5://…` fails rather than being + ignored, with `unsupported scheme socks5` in the message. `tests/proxy.rs` + pins that wording, because a bare "the network failed" on a machine where + every other tool works is the expensive version of this answer. + | `request_timed_out` | The request ran out of its time budget. Its own code because it is the one transport failure worth retrying or raising `--timeout` for. | | `missing_path_parameters` | A `{username}`/`{owner}`/`{account}` placeholder went unresolved. | | `invalid_data` | `--data` was not valid JSON, or a `@`/`@-` body was empty. | diff --git a/src/remedy.rs b/src/remedy.rs index e93d174..231cab2 100644 --- a/src/remedy.rs +++ b/src/remedy.rs @@ -210,7 +210,9 @@ pub fn for_transport() -> Remedy { .with_fix( "Nothing was reached, so nothing was rejected — this is the network, not \ the request. Check connectivity and any proxy in the environment \ - (HTTPS_PROXY, NO_PROXY).", + (HTTPS_PROXY, ALL_PROXY, NO_PROXY). A SOCKS proxy is not supported: \ + the message above says `unsupported scheme socks5` when that is the \ + cause.", ) .with_doc(Some(STATUS_PAGE)) } diff --git a/tests/proxy.rs b/tests/proxy.rs new file mode 100644 index 0000000..03a180d --- /dev/null +++ b/tests/proxy.rs @@ -0,0 +1,141 @@ +//! What the CLI does with the standard proxy environment variables. +//! +//! Nothing in `src/` mentions a proxy: `http::build` never calls +//! `.no_proxy()`, so `reqwest`'s own system-proxy detection applies and +//! `HTTP_PROXY` / `HTTPS_PROXY` / `ALL_PROXY` / `NO_PROXY` are honoured. That +//! is an important behaviour for anyone on a corporate network and it rests +//! entirely on a library default nobody here chose — a single `.no_proxy()` +//! added to fix something else would remove it, and no existing test would +//! notice. +//! +//! So these tests assert the behaviour rather than the absence of a call. A +//! grep for `no_proxy` would pass just as well if the client were rebuilt +//! somewhere else, or if a future `reqwest` changed its default. +//! +//! **Nothing leaves the machine**, and that is what decided which tests are +//! here. Proving a *positive* — the proxy was used — needs only a loopback +//! listener, which refuses the tunnel as soon as it has learned which host was +//! asked for. Proving a *negative* — that `HTTP_PROXY` alone does not carry an +//! https request, or that `NO_PROXY` exempts a host — means the request goes +//! to the real API instead, so those tests would put the internet in the suite +//! to assert behaviour that belongs to `reqwest` rather than to this crate. +//! They are in `docs/commands.md` as prose instead. + +use std::io::{Read, Write}; +use std::net::{TcpListener, TcpStream}; +use std::process::Command; + +/// A read that returns what arrived rather than blocking for a full buffer. +fn read_some(stream: &mut TcpStream) -> Vec { + stream + .set_read_timeout(Some(std::time::Duration::from_secs(10))) + .expect("a read timeout"); + let mut buffer = [0u8; 1024]; + match stream.read(&mut buffer) { + Ok(n) => buffer[..n].to_vec(), + Err(_) => vec![], + } +} + +/// Runs `mapbox styles list` with one proxy variable set, and returns what the +/// fake proxy on the other end saw. +/// +/// The token is deliberately a fake: a request that reaches the proxy has +/// already proved the point, and one that somehow bypassed it would get a 401 +/// rather than touching a real account. +fn what_the_proxy_saw( + variable: &str, + scheme: &str, + handler: fn(&mut TcpStream) -> String, +) -> String { + let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); + let port = listener.local_addr().expect("the bound address").port(); + + let accepted = std::thread::spawn(move || { + listener + .set_nonblocking(false) + .expect("a blocking listener"); + match listener.incoming().next() { + Some(Ok(mut stream)) => handler(&mut stream), + _ => String::new(), + } + }); + + let output = Command::new(env!("CARGO_BIN_EXE_mapbox")) + .args(["styles", "list", "--username", "someone", "-o", "json"]) + .env_clear() + .env("PATH", std::env::var("PATH").unwrap_or_default()) + .env("MAPBOX_ACCESS_TOKEN", "pk.a-fake-token-for-a-proxy-test") + .env("MAPBOX_NO_UPDATE_CHECK", "1") + .env(variable, format!("{scheme}://127.0.0.1:{port}")) + .output() + .expect("the binary runs"); + + // The request cannot succeed — the proxy refuses it — so a success here + // would mean the proxy was bypassed entirely. + assert!( + !output.status.success(), + "the request should have failed at the fake proxy: {}", + String::from_utf8_lossy(&output.stderr) + ); + + accepted.join().expect("the fake proxy thread") +} + +/// An HTTP proxy is asked to tunnel an https request with `CONNECT`. +fn http_proxy(stream: &mut TcpStream) -> String { + let request = read_some(stream); + let _ = stream.write_all(b"HTTP/1.1 502 Bad Gateway\r\n\r\n"); + String::from_utf8_lossy(&request) + .lines() + .next() + .unwrap_or_default() + .to_string() +} + +/// **The behaviour this file exists for.** An `HTTPS_PROXY` in the environment +/// is used, and the proxy is asked for the host the CLI was going to reach. +#[test] +fn an_https_proxy_in_the_environment_is_used() { + let seen = what_the_proxy_saw("HTTPS_PROXY", "http", http_proxy); + + assert!( + seen.starts_with("CONNECT api.mapbox.com:443"), + "the proxy should have been asked to tunnel to the API, saw: {seen:?}" + ); +} + +/// SOCKS is **not** supported, and this pins the fact that the failure says so. +/// +/// `reqwest` is built without its `socks` feature, so a `socks5://` proxy is +/// rejected when the connection is made rather than ignored. The message +/// carries `unsupported scheme socks5` from `reqwest` itself, which is the +/// part a user needs — a bare "the network failed" on a machine where every +/// other tool works would be a long afternoon. +/// +/// If this ever starts failing because SOCKS began working, that is a feature +/// rather than a break: delete the test and document the support. +#[test] +fn a_socks_proxy_is_refused_with_a_reason() { + let output = Command::new(env!("CARGO_BIN_EXE_mapbox")) + .args(["styles", "list", "--username", "someone", "-o", "json"]) + .env_clear() + .env("PATH", std::env::var("PATH").unwrap_or_default()) + .env("MAPBOX_ACCESS_TOKEN", "pk.a-fake-token-for-a-proxy-test") + .env("MAPBOX_NO_UPDATE_CHECK", "1") + // Port 9 (discard) rather than a live one: the scheme is rejected + // before anything is dialled, so nothing needs to be listening. + .env("ALL_PROXY", "socks5://127.0.0.1:9") + .output() + .expect("the binary runs"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("unsupported scheme socks5"), + "the failure should name the unsupported scheme, got: {stderr}" + ); + assert!( + stderr.contains("ALL_PROXY"), + "the advice should name the variable that caused it, got: {stderr}" + ); +}