Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/eighty-moons-repeat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@opennextjs/cloudflare": patch
---

fix: validate each redirect target in the image loader

`remotePatterns` is only applied to the `url` query parameter, so once an allowed host answered with a `Location` header it decided where the loader went next. Every hop is now checked for its scheme and for a literal non-routable address, and a rejected target returns 400 rather than being followed. A `Location` written as a relative reference is now resolved against the URL of the hop that sent it instead of being passed to `fetch` as-is.
65 changes: 65 additions & 0 deletions packages/cloudflare/src/cli/templates/images.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { describe, expect, it } from "vitest";
import type { LocalPattern } from "./images.js";
import {
detectImageContentType,
isNonRoutableHost,
matchLocalPattern,
matchRemotePattern as mRP,
parseCdnCgiImageRequest,
Expand Down Expand Up @@ -578,3 +579,67 @@ describe("detectImageContentType", () => {
expect(detectImageContentType(buffer)).toBeNull();
});
});

describe("isNonRoutableHost", () => {
it.each([
"localhost",
"app.localhost",
"127.0.0.1",
"127.1.2.3",
"0.0.0.0",
"10.0.0.1",
"172.16.0.1",
"172.31.255.255",
"192.168.1.1",
"169.254.169.254",
"::1",
"::",
"fc00::1",
"fd12:3456::1",
"fe80::1",
"febf::1",
// IPv4 mapped, both as written and as URL.hostname serializes it
"::ffff:127.0.0.1",
"[::ffff:7f00:1]",
"[::ffff:a9fe:a9fe]",
"[::ffff:a00:1]",
"[0:0:0:0:0:ffff:c0a8:1]",
// NAT64 well known prefix
"[64:ff9b::7f00:1]",
"100.64.0.1",
"100.127.255.255",
"198.18.0.1",
"224.0.0.1",
"255.255.255.255",
])("rejects %s", (hostname) => {
expect(isNonRoutableHost(hostname)).toBe(true);
});

it.each([
"example.com",
"cdn.example.com",
"1.1.1.1",
"8.8.8.8",
"172.32.0.1",
"172.15.0.1",
"192.169.1.1",
"169.253.0.1",
"11.0.0.1",
"2606:4700::1111",
"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]",
"[2606:4700::1111]",
"fec0::1",
])("allows %s", (hostname) => {
expect(isNonRoutableHost(hostname)).toBe(false);
});

it("is case insensitive", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this test be folded into the 2 previous ones?

expect(isNonRoutableHost("LOCALHOST")).toBe(true);
expect(isNonRoutableHost("FE80::1")).toBe(true);
});
});
149 changes: 142 additions & 7 deletions packages/cloudflare/src/cli/templates/images.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ export async function handleImageRequest(
status: 504,
});
}
if (fetchImageResult.error === "invalid_redirect") {
return new Response('"url" parameter is valid but upstream response is invalid', {
status: 400,
});
}
Comment on lines +65 to +69

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arpitjain099 could you please address this issue by running pnpm changeset

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (fetchImageResult.error === "too_many_redirects") {
return new Response('"url" parameter is valid but upstream response is invalid', {
status: 508,
Expand Down Expand Up @@ -351,13 +356,21 @@ async function fetchWithRedirects(
};
return result;
}
let redirectTarget: string;
if (locationHeader.startsWith("/")) {
redirectTarget = new URL(locationHeader, url).href;
} else {
redirectTarget = locationHeader;
// Location may be any relative reference, so it is always resolved against
// the URL of the hop that sent it.
let parsedTarget: URL;
try {
parsedTarget = new URL(locationHeader, url);
} catch {
return { ok: false, error: "invalid_redirect" } satisfies FetchWithRedirectsErrorResult;
}
const result = await fetchWithRedirects(redirectTarget, timeoutMS, maxRedirectCount - 1);
// 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.
if (!["http:", "https:"].includes(parsedTarget.protocol) || isNonRoutableHost(parsedTarget.hostname)) {
return { ok: false, error: "invalid_redirect" } satisfies FetchWithRedirectsErrorResult;
}
const result = await fetchWithRedirects(parsedTarget.href, timeoutMS, maxRedirectCount - 1);
return result;
}
}
Expand All @@ -380,7 +393,7 @@ type FetchWithRedirectsErrorResult = {
error: FetchImageError;
};

type FetchImageError = "timed_out" | "too_many_redirects";
type FetchImageError = "timed_out" | "too_many_redirects" | "invalid_redirect";

const redirectResponseStatuses = [301, 302, 303, 307, 308];

Expand Down Expand Up @@ -703,6 +716,128 @@ type ParseRelativeURLResult = {
search: string;
};

/**
* Checks whether a hostname is a literal address that should never be reached by
* following a redirect.
*
* Only literal addresses are considered. The Workers runtime has no DNS resolution
* API, so a hostname cannot be resolved to find out where it points, and this is a
* coarse filter rather than a substitute for the `remotePatterns` allow list that is
* applied to the original URL.
*/
export function isNonRoutableHost(hostname: string): boolean {
const host = hostname.toLowerCase();

if (host === "localhost" || host.endsWith(".localhost")) {
return true;
}

// IPv6 arrives from URL.hostname wrapped in brackets.
if (host.startsWith("[") || host.includes(":")) {
const v6 = host.startsWith("[") ? host.slice(1, -1) : host;
return isNonRoutableIPv6(v6);
}

return isNonRoutableIPv4(host);
}

function isNonRoutableIPv4(host: string): boolean {
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
(a === 100 && b >= 64 && b <= 127) || // 100.64.0.0/10 carrier grade NAT
(a === 198 && (b === 18 || b === 19)) || // 198.18.0.0/15 benchmarking
a >= 224 // multicast and reserved, includes 255.255.255.255
);
}

/**
* Expands an IPv6 literal to its eight 16 bit groups, or returns undefined when it
* does not parse. A trailing dotted quad (`::ffff:127.0.0.1`) becomes the last two
* groups, which is how an IPv4 mapped address is written.
*/
function expandIPv6(address: string): number[] | undefined {
let text = address;
const trailingIPv4 = /:((?:\d{1,3}\.){3}\d{1,3})$/.exec(text);
if (trailingIPv4) {
const quad = trailingIPv4[1]!.split(".").map(Number);
if (quad.some((octet) => octet > 255)) {
return undefined;
}
const [a, b, c, d] = quad as [number, number, number, number];
text = `${text.slice(0, trailingIPv4.index)}:${((a << 8) | b).toString(16)}:${((c << 8) | d).toString(16)}`;
}

const halves = text.split("::");
if (halves.length > 2) {
return undefined;
}
const parseGroups = (part: string) =>
part === ""
? []
: part.split(":").map((group) => (/^[0-9a-f]{1,4}$/.test(group) ? parseInt(group, 16) : NaN));

const head = parseGroups(halves[0]!);
const tail = halves.length === 2 ? parseGroups(halves[1]!) : [];
if ([...head, ...tail].some(Number.isNaN)) {
return undefined;
}

if (halves.length === 1) {
return head.length === 8 ? head : undefined;
}
const missing = 8 - head.length - tail.length;
if (missing < 1) {
return undefined;
}
return [...head, ...Array<number>(missing).fill(0), ...tail];
}

function isNonRoutableIPv6(address: string): boolean {
const groups = expandIPv6(address);
if (groups === undefined) {
return false;
}

// An address carrying an IPv4 one is only as safe as the address it carries:
// ::ffff:127.0.0.1 is loopback, and the well known NAT64 prefix reaches IPv4 too.
const embedsIPv4 =
(groups.slice(0, 5).every((group) => group === 0) && groups[5] === 0xffff) ||
(groups[0] === 0x0064 && groups[1] === 0xff9b && groups.slice(2, 6).every((group) => group === 0));
if (embedsIPv4) {
const high = groups[6]!;
const low = groups[7]!;
const dotted = `${high >> 8}.${high & 0xff}.${low >> 8}.${low & 0xff}`;
return isNonRoutableIPv4(dotted);
}

if (groups.every((group) => group === 0)) {
return true; // ::
}
if (groups.slice(0, 7).every((group) => group === 0) && groups[7] === 1) {
return true; // ::1
}

const first = groups[0]!;
return (
(first & 0xfe00) === 0xfc00 || // fc00::/7 unique local
(first & 0xffc0) === 0xfe80 // fe80::/10 link local
);
}
Comment on lines +728 to +839

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟨 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export function matchLocalPattern(pattern: LocalPattern, url: { pathname: string; search: string }): boolean {
if (pattern.search !== undefined && pattern.search !== url.search) {
return false;
Expand Down
Loading