fix: validate each redirect target in the image loader - #1338
fix: validate each redirect target in the image loader#1338arpitjain099 wants to merge 2 commits into
Conversation
fetchWithRedirects followed a Location header without checking where it pointed. The scheme check and the remotePatterns allow list are applied to the url query parameter only, so from hop one onwards an allowlisted host could send the loader anywhere, including to a literal loopback, link local or RFC1918 address. Each hop is now checked for scheme and for a literal non-routable address before the recursion, and the loader returns 400 rather than following it. Only literal addresses are checked. The Workers runtime has no DNS resolution API, so a hostname cannot be resolved to find out where it points, which is why this is a coarse filter and not a per-hop equivalent of the allow list. Re-running hasRemoteMatch on every hop was considered and rejected: it would break allowlisted hosts that legitimately redirect to a signed URL on another domain. isNonRoutableHost is exported and unit tested against loopback, link local including the metadata address, RFC1918, IPv6 unique local and link local, and a set of routable addresses that must keep working. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
🦋 Changeset detectedLatest commit: 2b098e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (fetchImageResult.error === "invalid_redirect") { | ||
| return new Response('"url" parameter is valid but upstream response is invalid', { | ||
| status: 400, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Missing changeset for a user-visible behaviour change
The image loader now rejects redirects it previously followed (invalid_redirect handling at packages/cloudflare/src/cli/templates/images.ts:65-69) but the change ships without a changeset entry, so the release notes will not mention it.
Impact: Users upgrading get a behaviour change in image loading with nothing in the changelog explaining it.
Repository rule requiring a changeset for behavioural changes
AGENTS.md states: "Any behavioural change to packages/cloudflare needs one. Skip for internal refactors, test-only changes, example/doc tweaks." This PR changes runtime behaviour of handleImageRequest/fetchWithRedirects in packages/cloudflare/src/cli/templates/images.ts, yet .changeset/ only contains README.md and config.json. A patch changeset of the form fix: <imperative title> should be added.
Prompt for agents
AGENTS.md/CONTRIBUTING.md require a changeset for any behavioural change to packages/cloudflare. This PR changes how the image loader handles redirects (new 400 response for rejected redirect targets) but no file was added under .changeset/. Add a patch-level changeset for "@opennextjs/cloudflare" following the documented format (`fix: <imperative title>` plus a body explaining the why).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
@arpitjain099 could you please address this issue by running pnpm changeset
There was a problem hiding this comment.
Added: .changeset/eighty-moons-repeat.md, patch on @opennextjs/cloudflare, following the <type>: <imperative title> plus body format in AGENTS.md. It covers both the per-hop validation and the relative Location resolution, since both are user-visible.
| } else { | ||
| redirectTarget = locationHeader; | ||
| } | ||
| // The allow list is applied to the original URL only, so each hop is | ||
| // re-validated here. Scheme and literal address are all that can be checked: | ||
| // the Workers runtime has no DNS resolution API. | ||
| let parsedTarget: URL; | ||
| try { | ||
| parsedTarget = new URL(redirectTarget); | ||
| } catch { | ||
| return { ok: false, error: "invalid_redirect" } satisfies FetchWithRedirectsErrorResult; | ||
| } | ||
| if (!["http:", "https:"].includes(parsedTarget.protocol) || isNonRoutableHost(parsedTarget.hostname)) { | ||
| return { ok: false, error: "invalid_redirect" } satisfies FetchWithRedirectsErrorResult; | ||
| } |
There was a problem hiding this comment.
🟡 Image redirects that use a relative path are now rejected with an error page
A redirect whose target is written as a path relative to the current image URL is parsed without a base (new URL(redirectTarget) at packages/cloudflare/src/cli/templates/images.ts:370), so it fails to parse and the request is refused instead of being followed.
Impact: Images hosted behind servers that redirect using relative locations fail to load and return an error.
Mechanism: only absolute and root-relative Location values are resolved
packages/cloudflare/src/cli/templates/images.ts:359-364 resolves the Location header against the current URL only when it starts with /; otherwise it is used verbatim. HTTP allows any relative reference (e.g. Location: image-2.png or Location: ?v=2). Such a value is not an absolute URL, so new URL(redirectTarget) throws and the new catch returns invalid_redirect, producing a 400. Resolving the header against the current URL in all cases (new URL(locationHeader, url)) both fixes the resolution and keeps the scheme/address validation intact.
(Refers to lines 354-376)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right, and your suggested fix is the correct one. Taken.
Location may be any relative reference, and the existing code only resolved it when it started with /. Anything else was passed through verbatim, so Location: image-2.png or Location: ?v=2 reached new URL(redirectTarget) with no base and threw.
Worth being precise about what changed and when, since this is not purely something my patch broke: before this PR the unresolved value went straight to fetch(), which rejects a relative URL too. So those redirects were already failing, just as a thrown TypeError rather than a 400. My patch turned it into a clean rejection, which is what made it visible. Either way it is wrong, and resolving properly is better than either.
The whole block now resolves once against the URL of the hop that sent it:
let parsedTarget: URL;
try {
parsedTarget = new URL(locationHeader, url);
} catch { ... }and the recursion takes parsedTarget.href. That subsumes the startsWith("/") branch, so it is gone, and the scheme and address checks still run on the resolved target. Relative references now behave as HTTP specifies:
new URL("image-2.png", "https://cdn.example.com/a/b.png") -> https://cdn.example.com/a/image-2.png
new URL("?v=2", "https://cdn.example.com/a/b.png") -> https://cdn.example.com/a/b.png?v=2
No unit test for this one: fetchWithRedirects is not exported and there is no fetch-mocking harness in images.spec.ts today. Happy to add one if you want the coverage; it would mean exporting the function or introducing a mocked fetch into that spec, so I did not want to make that call unilaterally.
| export function isNonRoutableHost(hostname: string): boolean { | ||
| const host = hostname.toLowerCase(); | ||
|
|
||
| if (host === "localhost" || host.endsWith(".localhost")) { | ||
| return true; | ||
| } | ||
|
|
||
| // IPv6 arrives from URL.hostname without its surrounding brackets. | ||
| if (host.includes(":")) { | ||
| const v6 = host.startsWith("[") ? host.slice(1, -1) : host; | ||
| if (v6 === "::" || v6 === "::1") { | ||
| return true; | ||
| } | ||
| // fc00::/7 unique local, fe80::/10 link local. | ||
| return /^f[cd][0-9a-f]{2}:/.test(v6) || /^fe[89ab][0-9a-f]:/.test(v6); | ||
| } | ||
|
|
||
| const octets = host.split("."); | ||
| if (octets.length !== 4) { | ||
| return false; | ||
| } | ||
| const parsed = octets.map((octet) => (/^\d{1,3}$/.test(octet) ? Number(octet) : NaN)); | ||
| if (parsed.some((octet) => Number.isNaN(octet) || octet > 255)) { | ||
| return false; | ||
| } | ||
| const [a, b] = parsed as [number, number, number, number]; | ||
| return ( | ||
| a === 0 || // 0.0.0.0/8 | ||
| a === 127 || // loopback | ||
| a === 10 || // RFC1918 | ||
| (a === 172 && b >= 16 && b <= 31) || // RFC1918 | ||
| (a === 192 && b === 168) || // RFC1918 | ||
| (a === 169 && b === 254) // link local, includes the metadata address | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟨 Redirect address filter misses IPv4-mapped/embedded IPv6 loopback and private addresses
The new per-hop guard rejects literal loopback/private hosts, but only in plain IPv4 dotted form or in the ::/::1/fc00::/7/fe80::/10 IPv6 ranges. A redirect to an IPv4-mapped IPv6 literal such as http://[::ffff:127.0.0.1]/ (which the URL parser serializes as [::ffff:7f00:1]) or http://[::ffff:169.254.169.254]/ passes isNonRoutableHost and is followed, defeating the intent of the check. Other internal ranges (e.g. 100.64.0.0/10 CGNAT, 192.0.2.0/24, 198.18.0.0/15, multicast) are also not covered.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and fixed. This one was a real hole in the guard I added.
new URL("http://[::ffff:127.0.0.1]/").hostname gives [::ffff:7f00:1], which matched none of the literal forms I was testing for, so the redirect was followed:
http://[::ffff:127.0.0.1]/ -> [::ffff:7f00:1]
http://[::ffff:169.254.169.254]/ -> [::ffff:a9fe:a9fe]
http://[0:0:0:0:0:ffff:127.0.0.1]/ -> [::ffff:7f00:1]
Rather than add more string patterns, the address is now expanded to its eight 16 bit groups and matched numerically. An IPv4 mapped address (::ffff:0:0/96) is decoded back to its IPv4 and run through the same IPv4 check, so it inherits every range rather than needing its own list. I did the same for the NAT64 well known prefix 64:ff9b::/96, since that also reaches IPv4. The dotted-quad and the hex serialization both go through the same path.
On the other ranges: added carrier grade NAT (100.64.0.0/10), benchmarking (198.18.0.0/15), and >= 224 which covers multicast and the reserved space including 255.255.255.255. I left the TEST-NET blocks out. They are documentation ranges rather than anything that reaches an internal service, and every address added here is one a legitimate image origin can no longer use, so I would rather keep the list to things with a real reason.
11 new cases in images.spec.ts cover the mapped forms, NAT64 and the new ranges, and I pinned the boundaries on the routable side too (100.63.255.255, 100.128.0.1, 198.17.0.1, 198.20.0.1, 223.255.255.255, [::ffff:8.8.8.8]). All 11 fail against the version you reviewed.
commit: |
| expect(isNonRoutableHost(hostname)).toBe(false); | ||
| }); | ||
|
|
||
| it("is case insensitive", () => { |
There was a problem hiding this comment.
could this test be folded into the 2 previous ones?
Three review points. isNonRoutableHost missed an IPv4 address carried inside an IPv6 one: http://[::ffff:127.0.0.1]/ serializes to [::ffff:7f00:1] and passed. The address is now expanded to its eight groups, an IPv4 mapped or NAT64 prefixed address is decoded back to the IPv4 and checked as one, and the IPv4 ranges gain carrier grade NAT, the benchmarking range, multicast and reserved. A Location header may be any relative reference, not just an absolute URL or a root relative path. It is now resolved against the URL of the hop that sent it, so a redirect to image-2.png follows instead of failing to parse. Adds the changeset that AGENTS.md asks for on a behavioural change. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
2 participants
Closes #1337.
What this changes
fetchWithRedirectsfollowed aLocationheader without checking where it pointed. The scheme check andhasRemoteMatchboth run against theurlquery parameter only, so from hop one onwards an allowlisted host decided where the loader went next.Each hop is now checked for scheme and for a literal non-routable address before the recursion, and the loader returns 400 instead of following.
Why only literal addresses
The Workers runtime has no DNS resolution API, so a hostname cannot be resolved to find out where it points.
isNonRoutableHostis therefore a coarse filter over literal addresses, not a per-hop equivalent of the allow list. It covers loopback,0.0.0.0/8, RFC1918, link-local including169.254.169.254,localhostand*.localhost, and IPv6::,::1, unique-localfc00::/7and link-localfe80::/10.Next's optimizer solves the same problem by re-entering
fetchExternalImageon every hop so its private-address guard re-runs, but that guard uses Node'sdnsmodule and does not port here.Why not re-run hasRemoteMatch per hop
That was the first thing I tried and I do not think you want it. It breaks allowlisted hosts that legitimately redirect to a signed URL on a sibling domain outside
remotePatterns, which seems common enough to matter. Happy to switch to it, or to put it behind an opt-in, if you would rather have the stricter behaviour.Tests
isNonRoutableHostis exported and unit tested inimages.spec.ts: 15 addresses that must be rejected, 10 routable ones that must keep working including172.32.0.1,172.15.0.1,192.169.1.1and169.253.0.1so the range boundaries are pinned, plus case-insensitivity.npx vitest run src/cli/templates/images.spec.tsis green at 67 tests.tsc --noEmitandprettier --checkare clean.Note on scope
Raised privately first. Cloudflare's security team reviewed it and concluded it is a hardening item rather than a security-boundary violation, and suggested a public issue and PR. Framed accordingly.