fix: harden IPv6 address parsing in URL safety check - #233
Draft
Valiunia wants to merge 9 commits into
Draft
Conversation
isSafeExternalUrl() blocks IP-literal hosts in loopback/private/link-local ranges before a custom-marker overlay URL is forwarded for server-side fetching. The IPv6 branch matched a handful of known string shapes for IPv4-in-IPv6 addresses (dotted-quad, ::ffff: hex form), which missed other standard encodings — bare IPv4-compatible, 6to4, and NAT64 — letting blocked IPv4 ranges through when expressed in one of those forms. Replace the pattern-matching with ipaddr.js, which parses the full address and classifies its range directly, and extract the embedded IPv4 address for every known encoding so it gets checked against the same blocklist as a plain IPv4 literal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Valiunia
marked this pull request as draft
July 29, 2026 12:23
The previous pass enumerated ipaddr.js range names (ipv4Mapped, rfc6052, 6to4) to find where to check for an embedded IPv4 address, which missed the IPv4-translated/SIIT form (::ffff:0:a.b.c.d, RFC 6145) since ipaddr.js classifies it as its own range name rather than ipv4Mapped. Detect the embedding from the address's raw byte structure instead: 6to4 and NAT64 keep their explicit range checks (their prefixes aren't zero), but IPv4-compatible, IPv4-mapped, and IPv4-translated are now recognized together as a single case — leading 64 bits zero, followed by one of the three IETF-defined 32-bit separators before the embedded address. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- NAT64 well-known alternate prefix (64:ff9b:1::/48) - case-insensitivity of the embedding detection - a public IPv6 address whose low bits happen to look like a blocked IPv4 address, but isn't a recognized embedding shape, stays allowed - document current behavior for Teredo (2001::/32), whose embedded IPv4 is XOR-obfuscated rather than directly readable from the address bytes, and is not handled by this check Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…embeddings Enumerating individual IPv4-in-IPv6 embedding schemes (mapped, compatible, 6to4, NAT64, SIIT) kept missing cases: a /48 NAT64 encoding that places the embedded address at hextet boundaries instead of the RFC-specified split around a reserved byte slipped past the byte-offset assumptions used to extract it. Switch to deny-by-default: any IPv6 address outside the plain global unicast range is now rejected outright, rather than decoding the embedded IPv4 for each transition/translation mechanism and checking it against the IPv4 blocklist. This also covers Teredo, whose embedded address is XOR-obfuscated and was previously unblockable by design, and mechanisms not embedding a blocked address at all — legitimate marker hosts have no reason to be addressed through any of them. One residual case needs an explicit check: ipaddr.js does not classify the deprecated IPv4-compatible form (::a.b.c.d) as special when written in hex, which is exactly the form Node's URL parser normalises a dotted-quad literal to. Also fixes a related hostname-normalization gap: "localhost." (with a trailing root-label dot, DNS-equivalent to "localhost") was not matched by the local-hostname blocklist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The IPv4 branch still used a hand-written regex plus a manually maintained list of "dangerous" octet ranges, unlike the IPv6 branch's deny-by-default approach. Route it through the same ipaddr.js range classification instead: allow only 'unicast', reject everything else. This closes gaps the hand-written list never covered — broadcast (255.255.255.255) and the RFC 5737 documentation/TEST-NET ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) were never blocked before. It also removes ~15 lines of manually maintained range logic in favor of one range check shared with the IPv6 path. Add regression tests for the newly-covered ranges, for IPv4 literals written in shorthand/hex/octal/decimal-integer notation (Node's URL parser already canonicalizes these before this function sees them, confirmed end-to-end), and for malformed out-of-range octets (rejected by the URL constructor itself, never reaching the fallback hostname path). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The custom-marker url field was validated with isSafeExternalUrl (which parses it via the WHATWG URL class) but then forwarded to the Static Images API as the caller's original raw string, just percent-encoded for transport. The receiving service decodes that back to the identical bytes and parses them again, independently, with its own URL parser. If a different parser disagrees with WHATWG on how to interpret some raw string (e.g. a backslash before an "@", which WHATWG treats as a path separator for special schemes but a more lenient parser could read as a userinfo separator), the host that was actually validated is not guaranteed to be the host that gets connected to downstream. Re-serialize the URL to its canonical WHATWG form after validation and forward that instead of the caller's raw string, so any downstream parser is working from an unambiguous, already-normalized string rather than re-parsing arbitrary input independently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
IP-literal handling was deny-by-default and near-exhaustive, but the
hostname side only blocked four exact names plus a .localhost suffix
— missing the more likely real-world vector, since cloud metadata
services are commonly reached by hostname rather than IP literal.
Add:
- a suffix blocklist for reserved local/internal-use DNS zones:
.internal (GCP/AWS service discovery — covers
metadata.google.internal, the actual GCP metadata hostname),
.local (mDNS), and .arpa (reverse-DNS and RFC 8375 home-network
zones, covering .in-addr.arpa, .ip6.arpa, and .home.arpa as
suffixes of it)
- rejection of dotless single-label hostnames (e.g. "metadata",
"intranet"), which only resolve via the fetcher's own network
search-domain suffixing and are never a legitimate public host
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…onent encodeURIComponent does not escape ( ) ! ' * (they're valid in a URI component per RFC 3986's "unreserved" set, a holdover from the older escape() behaviour). Every overlay value that gets embedded inside a parenthesis-delimited segment of the encoded overlay string — the custom-marker URL, the polyline, and the GeoJSON payload — could therefore contain a bare ")" that terminates that segment early from the Static Images API's own overlay-syntax parser's point of view, even though the value already passed URL/JSON validation on our side. Same class of bug as the URL-parser-differential issue this PR already addresses, one layer up. Add encodeOverlayComponent(), which escapes those five characters on top of encodeURIComponent, and use it for all three overlay value types. Also document two related, deliberately out-of-scope items in the urlSafety module comment: port restriction (belongs in the upstream fetcher, not this pre-fetch check) and the inherent incompleteness of enumerating local-network hostname suffixes (.corp, .lan, .home, etc. are network-specific and not enumerable in general) — both are the same class of problem as the already-documented DNS-rebinding limitation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The module comment had accumulated into a record of this PR's internal back-and-forth (re-arguing deny-by-default vs. the previous approach) rather than documentation for someone new to the file. Cut it down to the durable claims: what's rejected and why the three out-of-scope items (DNS rebinding, port restriction, non-enumerable local suffixes) are out of scope, without re-litigating already-superseded approaches. Also add a short comment noting that the dotless-hostname check depends on the trailing-dot strip running first — the two lines are coupled but were previously unlinked, which could trip up a future edit that reorders them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
isSafeExternalUrl()validates custom-marker overlay URLs before they're forwarded for server-side fetching, blocking hosts in loopback/private/link-local/internal ranges. The original implementation matched a handful of known string shapes for IPv4-in-IPv6 addresses (dotted-quad,::ffff:hex form) and a short hostname blocklist, which missed other standard encodings and hostname vectors — bare IPv4-compatible (::a.b.c.d), 6to4, NAT64, SIIT, and internal DNS hostnames likemetadata.google.internal.Change
ipaddr.js, deny-by-default: both address families are allowed only when classified as plain global "unicast." Every other named range (loopback, private, link-local, multicast, broadcast, CGNAT, reserved, and every IPv4-in-IPv6 transition/translation mechanism — mapped, compatible, 6to4, NAT64, SIIT, Teredo, AMT, ORCHID) is rejected outright, without needing to identify or check an embedded IPv4 address — coverage no longer depends on enumerating every encoding by hand..internal,.local,.arpa) and reject dotless single-label hostnames, closing hostname-based SSRF vectors (e.g.metadata.google.internal, the actual GCP metadata endpoint hostname) that the IP-literal-focused check never covered.localhost.) bypassed the local-hostname blocklist.@, which WHATWG treats as a path separator for special schemes but a more lenient parser could read as a userinfo separator).(and)in overlay values (custom-marker URL, GeoJSON payload) before embedding them in the encoded overlay string —encodeURIComponentleaves these unescaped, and they delimit overlay segments in the Static Images API's own syntax, so an unescaped)in a value could terminate that segment early.Test plan
npx vitest run— 771 tests passnpx tsc --noEmit— no errorsnpx eslint— no issues/48prefix, SIIT), hostname suffixes, dotless hostnames, canonicalization, and overlay-delimiter escapingNote on output changes
Custom-marker URLs and GeoJSON overlay payloads containing
()!'*will now have those characters percent-encoded in the request sent to the Static Images API (previously passed through unescaped). The API decodes percent-encoding, so this is semantically equivalent, but the literal request text differs from what a previous version of this tool would have produced. Encoded polylines are unaffected — the standard polyline alphabet (ASCII 63-126) never includes these characters.🤖 Generated with Claude Code