Treat a path parameter as a segment, not as URL syntax

[mattpodwysocki]

Path parameters are substituted into a URL template — …/{username}/{style_id}/static/{overlay}/… — and the value went in unencoded. A value carrying URL syntax therefore changed the shape of the request instead of naming a segment within it.

What changes

/, ?, # and \ in a path parameter are now percent-encoded, so they name a segment. A value of . or .. is refused as invalid_path_parameter.

\ is in that list because a special scheme treats it as a path separator too — reqwest::Url confirms it — so encoding / alone would have left an equivalent route open.

./.. are refused rather than encoded, because encoding doesn't stop them: a special scheme reads %2e%2e as a double-dot segment as well, so such a value climbs however it's spelled. Encoding the separators handles the multi-level case by collapsing it into a single segment; the refusal handles what's left.

Only those four characters, deliberately

These values carry punctuation on purpose. static get-image's template alone is:

…/static/{overlay}/{lon},{lat},{zoom},{bearing},{pitch}/{width}x{height}{highRes}{format}

— where the overlay is pin-s+f74e4e(-122.46,37.77), {highRes} is @2x, {format} is .png, and three placeholders share one comma-separated segment.

Percent-encoding everything outside RFC 3986's unreserved set would rewrite all of that, and risk breaking requests that work today. Encoding only what alters the URL's shape cannot change any request that doesn't already contain those four characters — pinned by a test that walks real values (overlay, @2x, .png, mapbox.mapbox-streets-v8, Arial Unicode MS Regular, 0-255) and asserts each passes through byte for byte, and without allocating.

Scope

The host was never reachable — //other, https://other, %2f%2f… and x@other all still resolve to the API's own host, so nothing could be directed at another server. What was reachable was a different path on the API, with the caller's token and the command's method.

It matters most where the value doesn't come from the person typing: an agent building a command from a prompt, or a script interpolating a variable from upstream. For someone typing their own argument it was a footgun rather than a risk.

Tests

Eight, including that an empty optional parameter still drops its whole segment — several operations depend on that and it had to survive the change.

595 tests, cargo fmt --check and cargo clippy --locked --all-targets clean. docs/commands.md gains the new error code.

[zmofei]

Found 1 issue:

1. Percent-encoded structural characters bypass the new path-traversal protection

path_segment() only checks for raw / ? # \ via value.contains(PATH_STRUCTURAL) before deciding whether to percent-encode, and only normalizes %2e/%2E (dots) before the dot-segment check. A value like %2e%2e%2fvictim contains no raw structural character, so it's returned unchanged (Cow::Borrowed(value)) and spliced straight into the path. Once decoded downstream (as HTTP servers do when routing), it becomes ../victim — the exact traversal this PR sets out to prevent. None of the new tests exercise percent-encoded dots and slashes together, only raw ../../x or pure %2e%2e tokens in isolation.

mapbox-cli/src/executor.rs

Lines 1122 to 1139 in a9bb1c2

	/// to a single segment — and this catches the single-level remainder.
	fn path_segment<'a>(name: &str, value: &'a str) -> Result<Cow<'a, str>> {
	// `%2e` is a dot as far as the URL parser is concerned, in either case.
	let as_dots = value.replace("%2e", ".").replace("%2E", ".");
	if as_dots == "." || as_dots == ".." {
	return Err(CliError::new(
	"invalid_path_parameter",
	format!(
	"`{value}` cannot be used as {name}: a path segment of `.` or `..` would move \
	the request to a different endpoint."
	),
	)
	.into());
	}
	if !value.contains(PATH_STRUCTURAL) {
	return Ok(Cow::Borrowed(value));
	}

[mattpodwysocki]

Thanks — the observation is accurate, the conclusion isn't, and I checked both rather than assuming either way.

Accurate: %2e%2e%2fvictim contains no raw structural character, so path_segment returns it borrowed and unchanged. That part is exactly right.

Not accurate: that it becomes ../victim. The URL parser does not decode %2f into a separator — it stays one segment:

path=/styles/v1/u/%2e%2e%2fvictim
   segments=["styles", "v1", "u", "%2e%2e%2fvictim"]

Compare the case this PR does fix, where the dots are encoded but the slashes are raw — there the parser does traverse, which is why %2e is normalised before the dot-segment check:

IN : …/u/%2e%2e/%2e%2e/tokens/v2/victim
OUT: …/styles/tokens/v2/victim

So the value goes on the wire as a single segment. It would only traverse if a server decoded %2f before routing — the classic encoded-slash bypass, which is a server-side property and identical for curl, a browser or any SDK. Not something this client can fix by encoding, and not something it introduces.

And blocking it would break documented behaviour. Percent-encoded values in path parameters are a Mapbox feature, not an attack signature. From the static-images spec in this repo:

url-{percent_encoded_url}({lon},{lat}) — custom image, PNG or JPG only
geojson({uri_encoded_geojson})
bbox: "The square brackets may be sent literally or percent-encoded as %5B"

A custom marker overlay is url-https%3A%2F%2Fexample.com%2Fmarker.png(...) — it must carry %2F. Refusing or re-encoding percent-escapes would break custom markers, GeoJSON overlays and the documented %5B bbox form. That trade-off is why this PR encodes only the four characters that change the URL's shape and refuses only dot segments: it cannot alter a request that doesn't already contain them, which a test pins by walking real values and asserting they pass through byte for byte.

On test coverage: the suggestion to exercise encoded dots and slashes together is a fair one, so I've added it — asserting the documented behaviour (single segment, no traversal) rather than a block, so the guarantee is pinned rather than assumed.

[zmofei]

Percent-encoding argument checks out — %2F stays a single path segment at the URL-parsing level, so this doesn't reopen the traversal the PR fixes. LGTM.