From ca5ec1767ff95b0317f1ad0abf166df191fa6215 Mon Sep 17 00:00:00 2001 From: Chris Sherwood Date: Sat, 6 Jun 2026 18:49:07 -0700 Subject: [PATCH] fix(security): harden assertNotPrivateUrl with ipaddr.js + host normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the regex blocklist in assertNotPrivateUrl with ipaddr.js range classification and normalizes the host before checking it. Consolidates two community proposals (#930 ipaddr.js parsing, #912 trailing-dot normalization) into one validator so the SSRF-critical path lives in-house with full tests. - Classify literal IPs by range (loopback / linkLocal / unspecified) via ipaddr.js instead of a hand-maintained regex list, which also catches alternate IPv4 encodings and avoids over-blocking mapped public IPs (the old `::ffff:` regex blocked every mapped address, including public ones). IPv4- mapped IPv6 is reduced to its embedded IPv4 before classification. - Strip a trailing root dot from the host so `localhost.` / `127.0.0.1.` can't bypass the checks (they resolve to the same target as the dotless form, #911). - Strip IPv6 brackets and lowercase for the localhost comparison. - RFC1918, bare LAN hostnames (e.g. `nomad3`), and external FQDNs remain allowed — LAN appliances need them, and DNS rebinding is a fetch-time concern outside this guard's scope. Adds a consolidated unit spec covering loopback/link-local/unspecified literals, alternate encodings, IPv4-mapped v6, mixed-case + trailing-dot localhost, and the allowed LAN/FQDN/mapped-public cases. Resolves #922. Supersedes #930 and #912 (thanks @Gujiassh and @luyua9). Co-Authored-By: Claude Opus 4.8 (1M context) --- admin/app/validators/common.ts | 48 +++++++++++++++++-------- admin/tests/unit/private_url.spec.ts | 53 ++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 15 deletions(-) create mode 100644 admin/tests/unit/private_url.spec.ts diff --git a/admin/app/validators/common.ts b/admin/app/validators/common.ts index f9f4e63..077dcd3 100644 --- a/admin/app/validators/common.ts +++ b/admin/app/validators/common.ts @@ -14,25 +14,43 @@ import ipaddr from 'ipaddr.js' */ export function assertNotPrivateUrl(urlString: string): void { const parsed = new URL(urlString) - const hostname = parsed.hostname.toLowerCase() - // `URL.hostname` strips the surrounding brackets from IPv6 literals - // (e.g. `http://[::1]/` → hostname `::1`), so IPv6 patterns must match - // the unbracketed form. - const blockedPatterns = [ - /^localhost$/, - /^127\.\d+\.\d+\.\d+$/, - /^0\.0\.0\.0$/, - /^169\.254\.\d+\.\d+$/, // Link-local / cloud metadata - /^::1$/, // IPv6 loopback - /^fe80:/i, // IPv6 link-local - /^::ffff:/i, // IPv4-mapped IPv6 (e.g. ::ffff:7f00:1 = 127.0.0.1) - /^::$/, // IPv6 all-zeros (equivalent to 0.0.0.0) - ] + // Normalize the host before classifying it: + // - lowercase for the `localhost` comparison + // - strip the surrounding brackets `URL.hostname` leaves on IPv6 literals + // (`http://[::1]/` → `::1`) + // - strip any trailing root dot(s): `localhost.` and `127.0.0.1.` resolve to + // the same target as the dotless form, so they must not slip past the + // checks below (#911). + const hostname = parsed.hostname + .toLowerCase() + .replace(/^\[|\]$/g, '') + .replace(/\.+$/, '') - if (blockedPatterns.some((re) => re.test(hostname))) { + if (hostname === 'localhost') { throw new Error(`Download URL must not point to a loopback or link-local address: ${hostname}`) } + + // Anything that isn't a literal IP (DNS names, bare LAN hostnames like + // `nomad3`, external FQDNs) is allowed — LAN appliances need them, and DNS + // rebinding is a fetch-time concern outside this guard's scope. Classifying + // literal addresses with ipaddr.js (rather than a regex list) catches + // alternate encodings and normalizes address ranges correctly (#922). + if (!ipaddr.isValid(hostname)) return + + let addr = ipaddr.parse(hostname) + if (addr.kind() === 'ipv6' && (addr as ipaddr.IPv6).isIPv4MappedAddress()) { + // e.g. ::ffff:127.0.0.1 — classify by the embedded IPv4 so a mapped + // loopback/link-local is blocked while a mapped public IP is allowed. + addr = (addr as ipaddr.IPv6).toIPv4Address() + } + + const range = addr.range() + if (range === 'loopback' || range === 'linkLocal' || range === 'unspecified') { + throw new Error( + `Download URL must not point to a loopback or link-local address: ${addr.toNormalizedString()}` + ) + } } /** diff --git a/admin/tests/unit/private_url.spec.ts b/admin/tests/unit/private_url.spec.ts new file mode 100644 index 0000000..43181ed --- /dev/null +++ b/admin/tests/unit/private_url.spec.ts @@ -0,0 +1,53 @@ +import * as assert from 'node:assert/strict' +import { test } from 'node:test' + +import { assertNotPrivateUrl } from '../../app/validators/common.js' + +const expectBlocked = (url: string) => { + assert.throws(() => assertNotPrivateUrl(url), /loopback or link-local/) +} + +const expectAllowed = (url: string) => { + assert.doesNotThrow(() => assertNotPrivateUrl(url)) +} + +test('blocks loopback and unspecified IPv4 literals', () => { + expectBlocked('http://127.0.0.1/file.zim') + expectBlocked('http://127.1.2.3/file.zim') + expectBlocked('http://0.0.0.0/file.zim') + // Alternate encodings normalized to 127.0.0.1 / 0.0.0.0 by the WHATWG URL parser + expectBlocked('http://0177.0.0.1/file.zim') + expectBlocked('http://2130706433/file.zim') + expectBlocked('http://0/file.zim') +}) + +test('blocks link-local IPv4 literals including cloud metadata', () => { + expectBlocked('http://169.254.169.254/latest/meta-data/') + expectBlocked('http://169.254.1.1/file.zim') +}) + +test('blocks IPv6 loopback, link-local, unspecified, and IPv4-mapped loopback', () => { + expectBlocked('http://[::1]/file.zim') + expectBlocked('http://[fe80::1]/file.zim') + expectBlocked('http://[::]/file.zim') + expectBlocked('http://[::ffff:127.0.0.1]/file.zim') + expectBlocked('http://[::ffff:7f00:1]/file.zim') +}) + +test('blocks localhost, including mixed-case and trailing-root-dot forms (#911)', () => { + expectBlocked('http://localhost/file.zim') + expectBlocked('http://LOCALHOST/file.zim') + expectBlocked('http://localhost./file.zim') + expectBlocked('http://LocalHost./file.zim') +}) + +test('allows RFC1918 LAN literals, bare LAN hostnames, and public FQDNs', () => { + expectAllowed('http://10.0.0.2/file.zim') + expectAllowed('http://172.16.0.2/file.zim') + expectAllowed('http://192.168.1.10/file.zim') + expectAllowed('http://nomad3/file.zim') + expectAllowed('http://my-nas.local/file.zim') + expectAllowed('https://downloads.example.com/file.zim') + // A mapped *public* IP must not be blocked + expectAllowed('http://[::ffff:8.8.8.8]/file.zim') +})