From a1353c368dcf0895c388544ca9e7e0058a9e056c Mon Sep 17 00:00:00 2001 From: Joey Ballentine Date: Fri, 31 Jul 2026 13:23:45 -0500 Subject: [PATCH] fix(api): restrict /api/fetch to the hosts we actually fetch from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/fetch` passed its request body straight to `fetch`, so the dev server would request whatever it was pointed at. It is dev-only — the deployed site is a static export with no API routes, and the one caller guards on `location.hostname === 'localhost'` — so this is hardening a local tool rather than closing a hole on the live site. Three layers, in `src/lib/server/safe-fetch.ts`: - A host allowlist. `image-util.ts` already knows the only three hosts we fetch HTML from; `src/lib/fetchable-hosts.ts` makes that list something both sides import instead of each keeping its own copy. The suffix match anchors on a dot, so `imgbox.com.evil.com` does not pass. - A resolved-address check, covering the ranges that are easy to miss by hand: IPv4-mapped IPv6, CGNAT, multicast and reserved space. - Manual redirect handling, revalidating every hop. Without it an allowed host can redirect us to link-local metadata and the allowlist means nothing. This does not stop DNS rebinding, and says so in a comment rather than implying otherwise: `fetch` resolves independently of our lookup. The allowlist is what makes that acceptable — exploiting it needs authoritative DNS for one of three specific domains. Rejected URLs now answer 400 instead of 500, so "unsupported link" and "upstream is down" are distinguishable. Co-Authored-By: Claude Opus 5 (1M context) --- src/lib/fetchable-hosts.ts | 25 ++++ src/lib/image-util.ts | 9 ++ src/lib/server/safe-fetch.ts | 151 +++++++++++++++++++ src/pages/api/fetch.ts | 9 +- tests/lib/safe-fetch.test.ts | 273 +++++++++++++++++++++++++++++++++++ 5 files changed, 465 insertions(+), 2 deletions(-) create mode 100644 src/lib/fetchable-hosts.ts create mode 100644 src/lib/server/safe-fetch.ts create mode 100644 tests/lib/safe-fetch.test.ts diff --git a/src/lib/fetchable-hosts.ts b/src/lib/fetchable-hosts.ts new file mode 100644 index 00000000..58f6d6d2 --- /dev/null +++ b/src/lib/fetchable-hosts.ts @@ -0,0 +1,25 @@ +/** + * The hosts we will fetch HTML from to extract image metadata. + * + * This is deliberately *not* the same list as the URL prefixes in + * `extractImage`. Those pick which parser to run, and include hosts we never + * fetch at all — an `i.imgur.com` or `cdn.discordapp.com` link is already a + * direct image URL, so it is returned without a request. This list answers a + * narrower question: which hosts may `/api/fetch` be pointed at. + * + * Keeping it in its own module means the dev-only API route and the browser + * agree on the answer instead of drifting apart. + */ +export const FETCHABLE_HOSTS = ['imgsli.com', 'slow.pics', 'imgbox.com'] as const; + +/** + * Whether `hostname` is an allowlisted host or a subdomain of one. + * + * The suffix match anchors on a dot on purpose. Without it, `imgbox.com.evil.com` + * and `evilimgbox.com` would both pass — the first is a domain the attacker + * controls, and the second is one they can simply register. + */ +export function isFetchableHost(hostname: string): boolean { + const host = hostname.toLowerCase(); + return FETCHABLE_HOSTS.some((allowed) => host === allowed || host.endsWith(`.${allowed}`)); +} diff --git a/src/lib/image-util.ts b/src/lib/image-util.ts index 4f340102..6cbd2c4b 100644 --- a/src/lib/image-util.ts +++ b/src/lib/image-util.ts @@ -1,8 +1,17 @@ +import { isFetchableHost } from './fetchable-hosts'; import { Image, PairedImage, StandaloneImage } from './schema'; export type GetDocument = (url: string) => Promise; export async function fetchHtml(url: string): Promise { + // `/api/fetch` enforces this too, and has to — it is reachable without going + // through this function. Checking here as well means an unsupported host + // fails the same way in dev and on the deployed site, rather than getting a + // 400 from our own API in one and a cors-anywhere error in the other. + if (!isFetchableHost(new URL(url).hostname)) { + throw new Error(`Cannot fetch from this host: ${url}`); + } + if (location.hostname === 'localhost') { // we should have access to our API routes const res = await fetch('/api/fetch', { diff --git a/src/lib/server/safe-fetch.ts b/src/lib/server/safe-fetch.ts new file mode 100644 index 00000000..4bc9cb50 --- /dev/null +++ b/src/lib/server/safe-fetch.ts @@ -0,0 +1,151 @@ +import dns from 'node:dns'; +import net from 'node:net'; +import { isFetchableHost } from '../fetchable-hosts'; + +/** + * Fetching a URL that came from a request body means the server can be aimed at + * anything it can reach, including services that are only listening because + * they assumed nobody outside the machine could talk to them. The guard here is + * three layers, because each one covers a hole the others leave open: + * + * 1. A host allowlist. The strongest of the three, and the reason the other + * two are cheap: the set of hosts we ever need to fetch is three entries + * long, so anything else — including every address literal — is refused + * before a packet is sent. + * 2. A resolved-address check, so an allowlisted name that happens to point at + * a private address is still refused. + * 3. Manual redirect handling, so a response from an allowed host cannot + * redirect us somewhere we would never have agreed to fetch directly. + * + * Layer 2 does not eliminate DNS rebinding: `fetch` resolves the name again + * itself, and nothing here pins the address we validated to the socket it opens. + * Closing that properly needs a custom dispatcher, which is not worth it for a + * route that only exists under `next dev` — and layer 1 already means an + * attacker would need authoritative DNS for imgbox, slow.pics or imgsli, at + * which point the rebinding is the least of the problems. + */ + +const MAX_REDIRECTS = 5; + +/** Validation failures, kept distinct so the route can answer 400 rather than 500. */ +export class UrlNotAllowedError extends Error { + constructor(message: string) { + super(message); + this.name = 'UrlNotAllowedError'; + } +} + +function isPrivateIPv4(ip: string): boolean { + const parts = ip.split('.').map(Number); + const [a, b] = parts; + + if (a === 0) return true; // 0.0.0.0/8 "this network" + if (a === 10) return true; // RFC1918 + if (a === 127) return true; // loopback + if (a === 169 && b === 254) return true; // link-local, incl. cloud metadata at 169.254.169.254 + if (a === 172 && b >= 16 && b <= 31) return true; // RFC1918 + if (a === 192 && b === 0) return true; // IETF protocol assignments + if (a === 192 && b === 168) return true; // RFC1918 + if (a === 198 && b >= 18 && b <= 19) return true; // benchmarking + if (a === 100 && b >= 64 && b <= 127) return true; // carrier-grade NAT + if (a >= 224) return true; // multicast, reserved, and the broadcast address + + return false; +} + +/** + * Whether `ip` belongs to a range that should never be reachable from a + * user-supplied URL. + * + * Anything that is not a valid address returns false — callers reject unknown + * hosts through the allowlist, and reporting a hostname as "private" here would + * only produce a confusing error. + */ +export function isPrivateAddress(ip: string): boolean { + if (net.isIPv4(ip)) { + return isPrivateIPv4(ip); + } + + if (net.isIPv6(ip)) { + const lower = ip.toLowerCase(); + + // An IPv4-mapped address routes wherever its embedded IPv4 address + // does, so ::ffff:127.0.0.1 is loopback. Nothing about the IPv6 text + // says so, which is what makes a prefix check on the string miss it. + const mapped = /^::ffff:(\d+\.\d+\.\d+\.\d+)$/.exec(lower); + if (mapped && net.isIPv4(mapped[1])) { + return isPrivateIPv4(mapped[1]); + } + + if (lower === '::' || lower === '::1') return true; + if (/^f[cd]/.test(lower)) return true; // fc00::/7 unique local + if (/^fe[89ab]/.test(lower)) return true; // fe80::/10 link local + if (/^ff/.test(lower)) return true; // ff00::/8 multicast + + return false; + } + + return false; +} + +/** + * Parses `raw` and rejects it unless it is an https URL on an allowlisted host + * that resolves to a public address. + */ +export async function assertSafeUrl(raw: string | URL): Promise { + let url: URL; + try { + url = new URL(raw); + } catch { + throw new UrlNotAllowedError(`Invalid URL: ${String(raw)}`); + } + + if (url.protocol !== 'https:') { + throw new UrlNotAllowedError(`Protocol not allowed: ${url.protocol}`); + } + + // `URL.hostname` keeps the brackets around an IPv6 literal, and `net.isIPv6` + // does not accept them — so `[::1]` reads as "not an IP address" unless they + // come off first. + const hostname = url.hostname.replace(/^\[|\]$/g, ''); + + if (!isFetchableHost(hostname)) { + throw new UrlNotAllowedError(`Host not allowed: ${hostname}`); + } + + const addresses = await dns.promises.lookup(hostname, { all: true }); + for (const { address } of addresses) { + if (isPrivateAddress(address)) { + throw new UrlNotAllowedError(`Host resolves to a private address: ${hostname} -> ${address}`); + } + } + + return url; +} + +/** + * Fetches `raw` as text, validating the initial URL and every redirect it + * follows. + */ +export async function safeFetchText(raw: string): Promise { + let url = await assertSafeUrl(raw); + + for (let hop = 0; hop <= MAX_REDIRECTS; hop += 1) { + const res = await fetch(url, { redirect: 'manual' }); + + if (res.status < 300 || res.status >= 400) { + return await res.text(); + } + + const location = res.headers.get('location'); + if (!location) { + throw new Error(`Redirect with no Location header from ${url.href}`); + } + + // Relative redirects are common and legal, so resolve against the URL we + // just requested before revalidating. + url = await assertSafeUrl(new URL(location, url)); + } + + throw new Error(`Too many redirects starting from ${String(raw)}`); +} diff --git a/src/pages/api/fetch.ts b/src/pages/api/fetch.ts index 00a59b3e..a9fb782d 100644 --- a/src/pages/api/fetch.ts +++ b/src/pages/api/fetch.ts @@ -1,4 +1,5 @@ import { NextApiRequest, NextApiResponse } from 'next'; +import { UrlNotAllowedError, safeFetchText } from '../../lib/server/safe-fetch'; interface Body { url: string; @@ -7,9 +8,13 @@ interface Body { export default async function fetchReq(req: NextApiRequest, res: NextApiResponse) { try { const body = JSON.parse(req.body as string) as Body; - const text = await fetch(body.url).then((res) => res.text()); + const text = await safeFetchText(body.url); res.status(200).json({ ok: true, data: text }); } catch (err) { - res.status(500).json({ ok: false, error: String(err) }); + // A rejected URL is the caller's mistake and an upstream failure is not, + // so they get different statuses — otherwise "you pasted a link we do + // not support" and "imgbox is down" are the same 500. + const status = err instanceof UrlNotAllowedError ? 400 : 500; + res.status(status).json({ ok: false, error: String(err) }); } } diff --git a/tests/lib/safe-fetch.test.ts b/tests/lib/safe-fetch.test.ts new file mode 100644 index 00000000..214d1ce7 --- /dev/null +++ b/tests/lib/safe-fetch.test.ts @@ -0,0 +1,273 @@ +import { describe, expect, it, vi } from 'vitest'; +import { isFetchableHost } from '../../src/lib/fetchable-hosts'; +import { UrlNotAllowedError, assertSafeUrl, isPrivateAddress, safeFetchText } from '../../src/lib/server/safe-fetch'; + +/** + * `assertSafeUrl` resolves the hostname, so every test that reaches that stage + * has to say what DNS returns. Real lookups would make the suite depend on the + * network and on imgbox's current A records. + */ +vi.mock('node:dns', () => ({ + default: { + promises: { + lookup: (hostname: string) => { + const addresses = dnsResponses.get(hostname); + if (!addresses) { + return Promise.reject(new Error(`ENOTFOUND ${hostname}`)); + } + return Promise.resolve( + addresses.map((address) => ({ address, family: address.includes(':') ? 6 : 4 })) + ); + }, + }, + }, +})); + +const dnsResponses = new Map([ + ['imgsli.com', ['93.184.216.34']], + ['slow.pics', ['93.184.216.34']], + ['imgbox.com', ['93.184.216.34']], + ['images2.imgbox.com', ['93.184.216.34']], + // The rebinding case: an allowlisted host whose DNS points inside. + ['evil.imgbox.com', ['127.0.0.1']], +]); + +describe('isPrivateAddress', () => { + it.each([ + ['0.0.0.0', 'unspecified'], + ['0.1.2.3', '0.0.0.0/8'], + ['10.0.0.1', 'RFC1918 10/8'], + ['100.64.0.1', 'CGNAT'], + ['100.127.255.255', 'CGNAT upper bound'], + ['127.0.0.1', 'loopback'], + ['169.254.169.254', 'cloud metadata'], + ['172.16.0.1', 'RFC1918 lower bound'], + ['172.31.255.255', 'RFC1918 upper bound'], + ['192.0.0.1', 'IETF protocol assignments'], + ['192.168.1.1', 'RFC1918 192.168/16'], + ['198.18.0.1', 'benchmarking'], + ['224.0.0.1', 'multicast'], + ['255.255.255.255', 'broadcast'], + ])('rejects %s (%s)', (ip) => { + expect(isPrivateAddress(ip)).toBe(true); + }); + + it.each([ + // The boundaries either side of RFC1918's 172.16/12 are the ones a + // hand-written check gets wrong. + ['172.15.255.255'], + ['172.32.0.0'], + ['100.63.255.255'], + ['100.128.0.0'], + ['93.184.216.34'], + ['8.8.8.8'], + ])('accepts public %s', (ip) => { + expect(isPrivateAddress(ip)).toBe(false); + }); + + it.each([ + ['::1', 'loopback'], + ['::', 'unspecified'], + ['fc00::1', 'unique local'], + ['fd12:3456::1', 'unique local'], + ['fe80::1', 'link local'], + ['ff02::1', 'multicast'], + ])('rejects IPv6 %s (%s)', (ip) => { + expect(isPrivateAddress(ip)).toBe(true); + }); + + it('accepts public IPv6', () => { + expect(isPrivateAddress('2606:2800:220:1:248:1893:25c8:1946')).toBe(false); + }); + + /** + * A string check on the IPv6 text misses these entirely: they neither start + * with `fc`/`fd`/`fe80` nor equal `::1`, but they route to loopback. + */ + it.each([['::ffff:127.0.0.1'], ['::ffff:169.254.169.254'], ['::FFFF:10.0.0.1']])('rejects IPv4-mapped %s', (ip) => { + expect(isPrivateAddress(ip)).toBe(true); + }); + + it('accepts an IPv4-mapped public address', () => { + expect(isPrivateAddress('::ffff:8.8.8.8')).toBe(false); + }); + + it('is not fooled by a non-address string', () => { + expect(isPrivateAddress('not-an-ip')).toBe(false); + }); +}); + +describe('isFetchableHost', () => { + it.each([['imgsli.com'], ['slow.pics'], ['imgbox.com'], ['images2.imgbox.com'], ['IMGBOX.COM']])( + 'accepts %s', + (host) => { + expect(isFetchableHost(host)).toBe(true); + } + ); + + /** + * The suffix match has to anchor on a dot. Both of these contain the + * allowlisted host as a substring and neither belongs to it. + */ + it.each([['imgbox.com.evil.com'], ['evilimgbox.com'], ['imgur.com'], ['localhost'], ['127.0.0.1']])( + 'rejects %s', + (host) => { + expect(isFetchableHost(host)).toBe(false); + } + ); +}); + +describe('assertSafeUrl', () => { + it('accepts an allowlisted https url', async () => { + const url = await assertSafeUrl('https://imgbox.com/abc123'); + expect(url.href).toBe('https://imgbox.com/abc123'); + }); + + it('accepts a subdomain of an allowlisted host', async () => { + await expect(assertSafeUrl('https://images2.imgbox.com/cc/e1/x_o.png')).resolves.toBeInstanceOf(URL); + }); + + it('rejects a malformed url', async () => { + await expect(assertSafeUrl('not a url')).rejects.toThrow(UrlNotAllowedError); + }); + + it('rejects http', async () => { + await expect(assertSafeUrl('http://imgbox.com/abc123')).rejects.toThrow(/protocol/i); + }); + + it('rejects a non-allowlisted host', async () => { + await expect(assertSafeUrl('https://example.com/')).rejects.toThrow(/not allowed/i); + }); + + it('rejects a host that merely ends with an allowlisted name', async () => { + await expect(assertSafeUrl('https://imgbox.com.evil.com/')).rejects.toThrow(/not allowed/i); + }); + + /** Rejected by the allowlist before DNS is ever consulted. */ + it.each([['https://127.0.0.1/'], ['https://169.254.169.254/latest/meta-data/'], ['https://[::1]/']])( + 'rejects the address literal %s', + async (url) => { + await expect(assertSafeUrl(url)).rejects.toThrow(UrlNotAllowedError); + } + ); + + /** Layer 2: allowlisted name, but it resolves inside. */ + it('rejects an allowlisted host that resolves to a private address', async () => { + await expect(assertSafeUrl('https://evil.imgbox.com/')).rejects.toThrow(/resolves/i); + }); +}); + +type Step = { status: number; location?: string } | { status: number; body: string }; + +/** Builds a `fetch` stub that walks a scripted list of responses. */ +function stubFetch(steps: Step[]) { + const calls: string[] = []; + let i = 0; + // `_init` is unused, but declaring it is what lets a test assert that the + // caller passed `redirect: 'manual'`. + const fn = vi.fn((input: URL | string, _init?: RequestInit) => { + calls.push(String(input)); + // `.at` rather than `[]` so overrunning the script is a typed + // possibility and the guard below is not dead code. + const step = steps.at(i++); + if (!step) throw new Error('fetch called more times than the test scripted'); + const headers = new Headers(); + if ('location' in step && step.location) headers.set('location', step.location); + return Promise.resolve({ + status: step.status, + headers, + text: () => Promise.resolve('body' in step ? step.body : ''), + } as Response); + }); + return { fn, calls }; +} + +describe('safeFetchText', () => { + it('returns the body when there is no redirect', async () => { + const { fn, calls } = stubFetch([{ status: 200, body: 'ok' }]); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://imgbox.com/abc123')).resolves.toBe('ok'); + expect(calls).toEqual(['https://imgbox.com/abc123']); + + vi.unstubAllGlobals(); + }); + + it('follows a redirect within the allowlist', async () => { + const { fn, calls } = stubFetch([ + { status: 302, location: 'https://images2.imgbox.com/final.png' }, + { status: 200, body: 'final' }, + ]); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://imgbox.com/abc123')).resolves.toBe('final'); + expect(calls).toEqual(['https://imgbox.com/abc123', 'https://images2.imgbox.com/final.png']); + + vi.unstubAllGlobals(); + }); + + it('resolves a relative Location against the current url', async () => { + const { fn, calls } = stubFetch([ + { status: 301, location: '/canonical/' }, + { status: 200, body: 'ok' }, + ]); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://slow.pics/c/abc')).resolves.toBe('ok'); + expect(calls[1]).toBe('https://slow.pics/canonical/'); + + vi.unstubAllGlobals(); + }); + + /** + * The bypass that a validate-once implementation misses: the first request + * goes to a host we allow, and its response points at cloud metadata. + */ + it('rejects a redirect to an internal address', async () => { + const { fn } = stubFetch([{ status: 302, location: 'http://169.254.169.254/latest/meta-data/' }]); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://imgbox.com/abc123')).rejects.toThrow(UrlNotAllowedError); + + vi.unstubAllGlobals(); + }); + + it('rejects a redirect off the allowlist', async () => { + const { fn } = stubFetch([{ status: 302, location: 'https://example.com/' }]); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://imgbox.com/abc123')).rejects.toThrow(/not allowed/i); + + vi.unstubAllGlobals(); + }); + + it('rejects a 3xx with no Location header', async () => { + const { fn } = stubFetch([{ status: 302 }]); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://imgbox.com/abc123')).rejects.toThrow(/location/i); + + vi.unstubAllGlobals(); + }); + + it('gives up after too many redirects', async () => { + const { fn } = stubFetch( + Array.from({ length: 10 }, () => ({ status: 302, location: 'https://imgbox.com/loop' })) + ); + vi.stubGlobal('fetch', fn); + + await expect(safeFetchText('https://imgbox.com/abc123')).rejects.toThrow(/too many redirects/i); + + vi.unstubAllGlobals(); + }); + + it('does not let fetch follow redirects on its own', async () => { + const { fn } = stubFetch([{ status: 200, body: 'ok' }]); + vi.stubGlobal('fetch', fn); + + await safeFetchText('https://imgbox.com/abc123'); + expect(fn.mock.calls[0][1]).toMatchObject({ redirect: 'manual' }); + + vi.unstubAllGlobals(); + }); +});