From cf12f864ef497ed588be8c4baa22269c73f5ec85 Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 15:08:32 +0000 Subject: [PATCH 1/8] refactor: prefer in-place edit to shallow-copying --- packages/forwarded/src/index.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/forwarded/src/index.ts b/packages/forwarded/src/index.ts index 4715f2e1..0522c105 100644 --- a/packages/forwarded/src/index.ts +++ b/packages/forwarded/src/index.ts @@ -5,11 +5,12 @@ import type { IncomingMessage } from "node:http" */ export function forwarded(req: Pick): Array { // simple header parsing - const proxyAddrs = parse((req.headers["x-forwarded-for"] as string) || "") + const proxyAddresses: (string | undefined)[] = parse((req.headers["x-forwarded-for"] as string) || "") const socketAddr = req.socket.remoteAddress // return all addresses - return [socketAddr].concat(proxyAddrs) + proxyAddresses.unshift(socketAddr) + return proxyAddresses } /** From 714304d9adb6d1652f21aaaf08fbee7f5acda522 Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 16:31:37 +0000 Subject: [PATCH 2/8] feat!: parse `x-forwarded-for` header and `socket.address` using `ipaddr.js` --- packages/forwarded/package.json | 4 ++- packages/forwarded/src/index.ts | 21 ++++++++++--- packages/proxy-address/src/index.ts | 48 ++++++++++++++--------------- packages/request/src/addresses.ts | 8 ++--- packages/request/src/prototype.ts | 10 +++--- pnpm-lock.yaml | 6 +++- 6 files changed, 56 insertions(+), 41 deletions(-) diff --git a/packages/forwarded/package.json b/packages/forwarded/package.json index ffce4a2f..72cd10c2 100644 --- a/packages/forwarded/package.json +++ b/packages/forwarded/package.json @@ -21,5 +21,7 @@ "build": "tsup", "prepack": "pnpm build" }, - "dependencies": {} + "dependencies": { + "ipaddr.js": "^2.2.0" + } } diff --git a/packages/forwarded/src/index.ts b/packages/forwarded/src/index.ts index 0522c105..956ec5a2 100644 --- a/packages/forwarded/src/index.ts +++ b/packages/forwarded/src/index.ts @@ -1,12 +1,13 @@ import type { IncomingMessage } from "node:http" +import ipaddr, { type IPv6, type IPv4 } from "ipaddr.js" /** * Get all addresses in the request, using the `X-Forwarded-For` header. */ -export function forwarded(req: Pick): Array { +export function forwarded(req: Pick): Array { // simple header parsing - const proxyAddresses: (string | undefined)[] = parse((req.headers["x-forwarded-for"] as string) || "") - const socketAddr = req.socket.remoteAddress + const proxyAddresses: (IPv4 | IPv6 | undefined)[] = parse((req.headers["x-forwarded-for"] as string) || "") + const socketAddr = req.socket.remoteAddress != null ? ipaddr.parse(req.socket.remoteAddress) : undefined // return all addresses proxyAddresses.unshift(socketAddr) @@ -14,9 +15,9 @@ export function forwarded(req: Pick): Arr } /** - * Parse the X-Forwarded-For header. + * Parse the X-Forwarded-For header, returning a {@link string} for each entry. */ -export function parse(header: string): string[] { +export function parseRaw(header: string): string[] { let end = header.length const list: string[] = [] let start = header.length @@ -46,3 +47,13 @@ export function parse(header: string): string[] { return list } + +/** + * Parse the X-Forwarded-For header, returning an IP address (see `ipaddr.js` {@link IPv4}, {@link IPv6}) for each entry. + */ +export function parse(header: string): (IPv4 | IPv6)[] { + const raw = parseRaw(header) + return raw.map(ipaddr.parse) +} + +export type { IPv4, IPv6 } diff --git a/packages/proxy-address/src/index.ts b/packages/proxy-address/src/index.ts index 9206e831..2947e4d9 100644 --- a/packages/proxy-address/src/index.ts +++ b/packages/proxy-address/src/index.ts @@ -5,7 +5,7 @@ import ipaddr, { type IPv6, type IPv4 } from "ipaddr.js" type Req = Pick export type TrustParameter = string | number | string[] -export type TrustFunction = (addr: string | undefined, i: number) => boolean +export type TrustFunction = (addr: IPv4 | IPv6 | undefined, i: number) => boolean export type Trust = TrustFunction | TrustParameter type Subnet = { @@ -14,8 +14,8 @@ type Subnet = { } const DIGIT_REGEXP = /^[0-9]+$/ -const isip = ipaddr.isValid -const parseip = ipaddr.parse +const isIp = ipaddr.isValid +const parseIp = ipaddr.parse /** * Pre-defined IP ranges. */ @@ -55,20 +55,21 @@ const trustNone: TrustFunction = () => false * @param req * @param trust */ -function allAddresses(req: Req, trust?: Trust): Array { +function allAddresses(req: Req, trust?: Trust): Array { // get addresses - const addrs = forwarded(req) + const addresses = forwarded(req) - if (trust == null) return addrs + if (trust == null) return addresses if (typeof trust !== "function") trust = compile(trust) - for (let i = 0; i < addrs.length - 1; i++) { - if (trust(addrs[i], i)) continue - addrs.length = i + 1 + for (let i = 0; i < addresses.length - 1; i++) { + if (trust(addresses[i], i)) continue + addresses.length = i + 1 // https://stackoverflow.com/a/26568611 + break } - return addrs + return addresses } /** * Compile argument into trust function. @@ -128,9 +129,9 @@ export function parseIPNotation(note: string): Subnet { const pos = note.lastIndexOf("/") const str = pos !== -1 ? note.substring(0, pos) : note - if (!isip(str)) throw new TypeError(`invalid IP address: ${str}`) + if (!isIp(str)) throw new TypeError(`invalid IP address: ${str}`) - let ip = parseip(str) + let ip = parseIp(str) const max = ip.kind() === "ipv6" ? 128 : 32 if (pos === -1) { @@ -142,7 +143,7 @@ export function parseIPNotation(note: string): Subnet { let range: number | null = null if (DIGIT_REGEXP.test(rangeString)) range = Number.parseInt(rangeString, 10) - else if (ip.kind() === "ipv4" && isip(rangeString)) range = parseNetmask(rangeString) + else if (ip.kind() === "ipv4" && isIp(rangeString)) range = parseNetmask(rangeString) if (range == null || range <= 0 || range > max) throw new TypeError(`invalid range on address: ${note}`) return { ip, range } @@ -154,7 +155,7 @@ export function parseIPNotation(note: string): Subnet { * @private */ function parseNetmask(netmask: string) { - const ip = parseip(netmask) + const ip = parseIp(netmask) return ip.kind() === "ipv4" ? ip.prefixLengthFromSubnetMask() : null } /** @@ -164,21 +165,19 @@ function parseNetmask(netmask: string) { * @param trust * @public */ -function proxyAddress(req: Req, trust: Trust): string | undefined { +function proxyAddress(req: Req, trust: Trust): IPv4 | IPv6 | undefined { if (trust == null) throw new TypeError("trust argument cannot be null-ish") - const addrs = allAddresses(req, trust) + const addresses = allAddresses(req, trust) - return addrs[addrs.length - 1] + return addresses[addresses.length - 1] } /** * Compile trust function for multiple subnets. */ function trustMulti(subnets: Subnet[]): TrustFunction { - return function trust(addr: string | undefined) { - if (addr == null) return false - if (!isip(addr)) return false - const ip = parseip(addr) + return function trust(ip: IPv4 | IPv6 | undefined) { + if (ip == null) return false let ipconv: IPv4 | IPv6 | null = null const kind = ip.kind() for (let i = 0; i < subnets.length; i++) { @@ -205,10 +204,8 @@ function trustMulti(subnets: Subnet[]): TrustFunction { function trustSingle(subnet: Subnet): TrustFunction { const subnetKind = subnet.ip.kind() const subnetIsIPv4 = subnetKind === "ipv4" - return function trust(addr: string | undefined) { - if (addr == null) return false - if (!isip(addr)) return false - let ip = parseip(addr) + return function trust(ip: IPv4 | IPv6 | undefined) { + if (ip == null) return false const kind = ip.kind() if (kind !== subnetKind) { if (subnetIsIPv4 && !(ip as IPv6).isIPv4MappedAddress()) return false @@ -220,3 +217,4 @@ function trustSingle(subnet: Subnet): TrustFunction { } export { allAddresses, compile, proxyAddress } +export type { IPv4, IPv6 } diff --git a/packages/request/src/addresses.ts b/packages/request/src/addresses.ts index de2fccd3..6ed8d563 100644 --- a/packages/request/src/addresses.ts +++ b/packages/request/src/addresses.ts @@ -1,8 +1,8 @@ -import { type Trust, allAddresses, proxyAddress } from "@otterhttp/proxy-address" +import { type IPv4, type IPv6, type Trust, allAddresses, proxyAddress } from "@otterhttp/proxy-address" import type { HasHeaders, HasSocket } from "./types" -export const getIP = (req: HasHeaders & HasSocket, trust: Trust): string | undefined => - proxyAddress(req, trust)?.replace(/^.*:/, "") // stripping the redundant prefix added by OS to IPv4 address +export const getIP = (req: HasHeaders & HasSocket, trust: Trust): IPv4 | IPv6 | undefined => proxyAddress(req, trust) -export const getIPs = (req: HasHeaders & HasSocket, trust: Trust): Array => allAddresses(req, trust) +export const getIPs = (req: HasHeaders & HasSocket, trust: Trust): Array => + allAddresses(req, trust) diff --git a/packages/request/src/prototype.ts b/packages/request/src/prototype.ts index b784dfb1..870bd5b7 100644 --- a/packages/request/src/prototype.ts +++ b/packages/request/src/prototype.ts @@ -2,7 +2,7 @@ import { IncomingMessage } from "node:http" import type { ParsedUrlQuery } from "node:querystring" import { Accepts } from "@otterhttp/accepts" import { ContentType } from "@otterhttp/content-type" -import type { Trust } from "@otterhttp/proxy-address" +import type { IPv4, IPv6, Trust } from "@otterhttp/proxy-address" import type { Middleware } from "@otterhttp/router" import { type URLParams, getQueryParams } from "@otterhttp/url" import type { Result as RangeParseResult, Options as RangeParsingOptions, Ranges } from "header-range-parser" @@ -36,8 +36,8 @@ export class Request extends IncomingMessage { private declare _hostname: string private declare _port: number | undefined private declare _subdomains: string[] - private declare _ip: string | undefined - private declare _ips: (string | undefined)[] + private declare _ip: IPv4 | IPv6 | undefined + private declare _ips: (IPv4 | IPv6 | undefined)[] // own members private _cookies?: Record @@ -109,10 +109,10 @@ export class Request extends IncomingMessage { get subdomains(): readonly string[] { return this._subdomains } - get ip(): string | undefined { + get ip(): IPv4 | IPv6 | undefined { return this._ip } - get ips(): readonly (string | undefined)[] { + get ips(): readonly (IPv4 | IPv6 | undefined)[] { return this._ips } get xhr(): boolean { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d9745419..69f6be9c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -125,7 +125,11 @@ importers: packages/etag: {} - packages/forwarded: {} + packages/forwarded: + dependencies: + ipaddr.js: + specifier: ^2.2.0 + version: 2.2.0 packages/ip-filter: dependencies: From 9f7fc5c9cc78176e78817f4ef5194ebdf0e408cc Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 20:48:55 +0000 Subject: [PATCH 3/8] refactor: do things r.e. proxied stuff in a *better* way --- package.json | 1 + packages/forwarded/src/index.ts | 43 ++++-- packages/proxy-address/src/index.ts | 24 ++- packages/request/src/host.ts | 4 +- packages/request/src/protocol.ts | 4 +- packages/request/src/prototype.ts | 4 +- packages/request/src/types.ts | 3 + .../request/src/util/trust-remote-address.ts | 7 +- pnpm-lock.yaml | 3 + test_helpers/ip-tagged-template.ts | 9 ++ tests/core/request.test.ts | 14 +- tests/modules/forwarded.test.ts | 9 +- tests/modules/proxy-addr.test.ts | 145 ++++++++++-------- 13 files changed, 168 insertions(+), 102 deletions(-) create mode 100644 test_helpers/ip-tagged-template.ts diff --git a/package.json b/package.json index 1ca4d1ee..8ad57c3f 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "eta": "2.2.0", "header-range-parser": "^1.1.3", "husky": "9.0.11", + "ipaddr.js": "^2.2.0", "jsonwebtoken": "9.0.2", "regexparam": "3.0.0", "supertest-fetch": "1.5.0", diff --git a/packages/forwarded/src/index.ts b/packages/forwarded/src/index.ts index 956ec5a2..344781fd 100644 --- a/packages/forwarded/src/index.ts +++ b/packages/forwarded/src/index.ts @@ -1,17 +1,35 @@ +import assert from "node:assert/strict" import type { IncomingMessage } from "node:http" import ipaddr, { type IPv6, type IPv4 } from "ipaddr.js" +/** + * Type-guard to determine whether a parsed IP address is an IPv4 address. + */ +const isIPv4 = (value: IPv4 | IPv6): value is IPv4 => value.kind() === "ipv4" +/** + * Type-guard to determine whether a parsed IP address is an IPv6 address. + */ +const isIPv6 = (value: IPv4 | IPv6): value is IPv6 => value.kind() === "ipv6" + +function parseIp(value: string): IPv4 | IPv6 { + const ip = ipaddr.parse(value) + if (isIPv6(ip) && ip.isIPv4MappedAddress()) return ip.toIPv4Address() + return ip +} + /** * Get all addresses in the request, using the `X-Forwarded-For` header. */ -export function forwarded(req: Pick): Array { - // simple header parsing - const proxyAddresses: (IPv4 | IPv6 | undefined)[] = parse((req.headers["x-forwarded-for"] as string) || "") - const socketAddr = req.socket.remoteAddress != null ? ipaddr.parse(req.socket.remoteAddress) : undefined - - // return all addresses - proxyAddresses.unshift(socketAddr) - return proxyAddresses +export function* forwarded(req: Pick): Generator { + const socketAddress = req.socket.remoteAddress != null ? parseIp(req.socket.remoteAddress) : undefined + yield socketAddress + + const xForwardedHeader = req.headers["x-forwarded-for"] + if (!xForwardedHeader) return + // https://github.com/nodejs/node/blob/58a7b0011a1858f4fde2fe553240153b39c13cd0/lib/_http_incoming.js#L381 + assert(!Array.isArray(xForwardedHeader)) + + yield* parse(xForwardedHeader) } /** @@ -49,11 +67,14 @@ export function parseRaw(header: string): string[] { } /** - * Parse the X-Forwarded-For header, returning an IP address (see `ipaddr.js` {@link IPv4}, {@link IPv6}) for each entry. + * Parse the X-Forwarded-For header, returning an IP address (see `ipaddr.js` {@link IPv4}, {@link IPv6}) + * for each entry. */ -export function parse(header: string): (IPv4 | IPv6)[] { +export function* parse(header: string): Generator { const raw = parseRaw(header) - return raw.map(ipaddr.parse) + for (const entry of raw) { + yield parseIp(entry) + } } export type { IPv4, IPv6 } diff --git a/packages/proxy-address/src/index.ts b/packages/proxy-address/src/index.ts index 2947e4d9..8eb78c7a 100644 --- a/packages/proxy-address/src/index.ts +++ b/packages/proxy-address/src/index.ts @@ -16,6 +16,7 @@ type Subnet = { const DIGIT_REGEXP = /^[0-9]+$/ const isIp = ipaddr.isValid const parseIp = ipaddr.parse + /** * Pre-defined IP ranges. */ @@ -57,20 +58,21 @@ const trustNone: TrustFunction = () => false */ function allAddresses(req: Req, trust?: Trust): Array { // get addresses + const addressGenerator = forwarded(req) - const addresses = forwarded(req) - - if (trust == null) return addresses - + if (trust == null) return Array.from(addressGenerator) if (typeof trust !== "function") trust = compile(trust) - for (let i = 0; i < addresses.length - 1; i++) { - if (trust(addresses[i], i)) continue - addresses.length = i + 1 // https://stackoverflow.com/a/26568611 - break + let i = 0 + const addresses: Array = [] + for (const address of addressGenerator) { + addresses.push(address) + if (!trust(address, i)) break + i += 1 } return addresses } + /** * Compile argument into trust function. * @@ -94,6 +96,7 @@ function compile(val: string | number | string[]): TrustFunction { } return compileTrust(compileRangeSubnets(trust)) } + /** * Compile 'hops' number into trust function. * @@ -109,6 +112,7 @@ function compileHopsTrust(hops: number): TrustFunction { function compileRangeSubnets(arr: string[]) { return arr.map((ip) => parseIPNotation(ip)) } + /** * Compile range subnet array into trust function. * @@ -119,6 +123,7 @@ function compileTrust(rangeSubnets: Subnet[]): TrustFunction { const len = rangeSubnets.length return len === 0 ? trustNone : len === 1 ? trustSingle(rangeSubnets[0]) : trustMulti(rangeSubnets) } + /** * Parse IP notation string into range subnet. * @@ -148,6 +153,7 @@ export function parseIPNotation(note: string): Subnet { if (range == null || range <= 0 || range > max) throw new TypeError(`invalid range on address: ${note}`) return { ip, range } } + /** * Parse netmask string into CIDR range. * @@ -158,6 +164,7 @@ function parseNetmask(netmask: string) { const ip = parseIp(netmask) return ip.kind() === "ipv4" ? ip.prefixLengthFromSubnetMask() : null } + /** * Determine address of proxied request. * @@ -196,6 +203,7 @@ function trustMulti(subnets: Subnet[]): TrustFunction { return false } } + /** * Compile trust function for single subnet. * diff --git a/packages/request/src/host.ts b/packages/request/src/host.ts index ae4e8fea..146eb199 100644 --- a/packages/request/src/host.ts +++ b/packages/request/src/host.ts @@ -2,7 +2,7 @@ import type { Trust } from "@otterhttp/proxy-address" import { isIP } from "node:net" import { getRequestHeader } from "./get-header" -import type { HasHeaders, HasSocket } from "./types" +import type { HasHeaders, HasIpAddresses, HasSocket } from "./types" import { trustRemoteAddress } from "./util/trust-remote-address" export type Host = { @@ -38,7 +38,7 @@ const getDefaultHeaderHostString = (req: HasHeaders): string | undefined => { return normalizeHostString(host) } -const getHostString = (req: HasHeaders & HasSocket, trust: Trust): string | undefined => { +const getHostString = (req: HasHeaders & HasIpAddresses, trust: Trust): string | undefined => { if (trustRemoteAddress(req, trust)) { const forwardedHost = getForwardedHeaderHostString(req) if (forwardedHost) return forwardedHost diff --git a/packages/request/src/protocol.ts b/packages/request/src/protocol.ts index e965a447..6d54c0be 100644 --- a/packages/request/src/protocol.ts +++ b/packages/request/src/protocol.ts @@ -1,7 +1,7 @@ import { TLSSocket } from "node:tls" import type { Trust } from "@otterhttp/proxy-address" -import type { HasHeaders, HasSocket } from "./types" +import type { HasHeaders, HasIpAddresses, HasSocket } from "./types" import { trustRemoteAddress } from "./util/trust-remote-address" export type Protocol = "http" | "https" | string @@ -10,7 +10,7 @@ const hasSecureConnection = (req: HasHeaders & HasSocket): boolean => { return req.socket instanceof TLSSocket && req.socket.encrypted } -export const getProtocol = (req: HasHeaders & HasSocket, trust: Trust): Protocol => { +export const getProtocol = (req: HasHeaders & HasSocket & HasIpAddresses, trust: Trust): Protocol => { const proto = `http${hasSecureConnection(req) ? "s" : ""}` if (!trustRemoteAddress(req, trust)) return proto diff --git a/packages/request/src/prototype.ts b/packages/request/src/prototype.ts index 870bd5b7..6f3c683e 100644 --- a/packages/request/src/prototype.ts +++ b/packages/request/src/prototype.ts @@ -47,13 +47,13 @@ export class Request extends IncomingMessage { populate({ trust, subdomainOffset }: { trust: Trust; subdomainOffset: number | undefined }) { this._acceptsMeta = new Accepts(this) this._query = getQueryParams(this.url) + this._ip = getIP(this, trust) + this._ips = getIPs(this, trust) this._protocol = getProtocol(this, trust) const host = getHost(this, trust) this._hostname = host.hostname this._port = host.port this._subdomains = getSubdomains(host, subdomainOffset) - this._ip = getIP(this, trust) - this._ips = getIPs(this, trust) } getHeader>(header: HeaderName): Headers[HeaderName] { diff --git a/packages/request/src/types.ts b/packages/request/src/types.ts index 6a472aab..2f06d04d 100644 --- a/packages/request/src/types.ts +++ b/packages/request/src/types.ts @@ -1,6 +1,9 @@ import type { IncomingHttpHeaders } from "node:http" import type { Socket } from "node:net" +import type { IPv4, IPv6 } from "@otterhttp/proxy-address" + +export type HasIpAddresses = { ips: Array } export type HasHeaders = { headers: IncomingHttpHeaders } export type HasSocket = { socket: Socket } diff --git a/packages/request/src/util/trust-remote-address.ts b/packages/request/src/util/trust-remote-address.ts index 54f51c2a..c1db35bd 100644 --- a/packages/request/src/util/trust-remote-address.ts +++ b/packages/request/src/util/trust-remote-address.ts @@ -1,9 +1,8 @@ import { type Trust, compile } from "@otterhttp/proxy-address" -import type { HasSocket } from "../types" +import type { HasIpAddresses } from "../types" -export const trustRemoteAddress = ({ socket }: HasSocket, trust: Trust): boolean => { - const val = socket.remoteAddress +export const trustRemoteAddress = ({ ips }: HasIpAddresses, trust: Trust): boolean => { if (typeof trust !== "function") trust = compile(trust) - return trust(val, 0) + return trust(ips[0], 0) } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 69f6be9c..cc054539 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -35,6 +35,9 @@ importers: husky: specifier: 9.0.11 version: 9.0.11 + ipaddr.js: + specifier: ^2.2.0 + version: 2.2.0 jsonwebtoken: specifier: 9.0.2 version: 9.0.2 diff --git a/test_helpers/ip-tagged-template.ts b/test_helpers/ip-tagged-template.ts new file mode 100644 index 00000000..daf55f86 --- /dev/null +++ b/test_helpers/ip-tagged-template.ts @@ -0,0 +1,9 @@ +import assert from "node:assert/strict" +import ipaddr from "ipaddr.js" + +export function ip(strings: TemplateStringsArray, ...values: unknown[]) { + assert(strings.length === 1) + assert(values.length === 0) + + return ipaddr.parse(strings[0]) +} diff --git a/tests/core/request.test.ts b/tests/core/request.test.ts index 2670af54..cac890e5 100644 --- a/tests/core/request.test.ts +++ b/tests/core/request.test.ts @@ -3,10 +3,10 @@ import { makeFetch } from "supertest-fetch" import { assert, afterEach, describe, expect, it, vi } from "vitest" import { App, type Request, type Response } from "@/packages/app/src" +import * as reqGetHeader from "@/packages/request/src/get-header" import { InitAppAndTest } from "@/test_helpers/initAppAndTest" -import * as reqGetHeader from "../../packages/request/src/get-header" -vi.mock(import("../../packages/request/src/get-header"), async (importOriginal) => { +vi.mock(import("@/packages/request/src/get-header"), async (importOriginal) => { const module = await importOriginal() return { @@ -172,8 +172,8 @@ describe("Request properties", () => { describe("Network extensions", () => { const ipHandler = (req, res) => { res.json({ - ip: req.ip, - ips: req.ips, + ip: req.ip.toString(), + ips: req.ips.map((ip) => ip.toString()), }) } @@ -183,7 +183,7 @@ describe("Request properties", () => { const agent = new Agent({ family: 4 }) // ensure IPv4 only await fetch("/", { agent }).expect(200, { ip: "127.0.0.1", - ips: ["::ffff:127.0.0.1"], + ips: ["127.0.0.1"], }) }) if (process.env.GITHUB_ACTION) { @@ -204,7 +204,7 @@ describe("Request properties", () => { const agent = new Agent({ family: 4 }) // ensure IPv4 only await fetch("/", { agent, headers: { "x-forwarded-for": "10.0.0.1, 10.0.0.2, 127.0.0.2" } }).expect(200, { ip: "127.0.0.1", - ips: ["::ffff:127.0.0.1"], + ips: ["127.0.0.1"], }) }) it('IPv4 req.ip & req.ips support trusted proxies with "trust proxy"', async () => { @@ -214,7 +214,7 @@ describe("Request properties", () => { const agent = new Agent({ family: 4 }) // ensure IPv4 only await fetch("/", { agent, headers: { "x-forwarded-for": "10.0.0.1, 10.0.0.2, 127.0.0.2" } }).expect(200, { ip: "127.0.0.2", - ips: ["::ffff:127.0.0.1", "127.0.0.2"], + ips: ["127.0.0.1", "127.0.0.2"], }) }) it("req.protocol is http by default", async () => { diff --git a/tests/modules/forwarded.test.ts b/tests/modules/forwarded.test.ts index 62addcdc..122d8ddf 100644 --- a/tests/modules/forwarded.test.ts +++ b/tests/modules/forwarded.test.ts @@ -3,32 +3,33 @@ import { describe, expect, it } from "vitest" import { forwarded } from "@/packages/forwarded/src" import { createReq } from "@/test_helpers/createReq" +import { ip } from "@/test_helpers/ip-tagged-template" describe("forwarded(req)", () => { it("should work with `X-Forwarded-For` header", () => { const req = createReq("127.0.0.1") as IncomingMessage - expect(forwarded(req)).toEqual(["127.0.0.1"]) + expect(Array.from(forwarded(req))).toStrictEqual([ip`127.0.0.1`]) }) it("should include entries from `X-Forwarded-For`", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.2, 10.0.0.1", }) as IncomingMessage - expect(forwarded(req)).toEqual(["127.0.0.1", "10.0.0.1", "10.0.0.2"]) + expect(Array.from(forwarded(req))).toStrictEqual([ip`127.0.0.1`, ip`10.0.0.1`, ip`10.0.0.2`]) }) it("should skip blank entries", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.2,, 10.0.0.1", }) as IncomingMessage - expect(forwarded(req)).toEqual(["127.0.0.1", "10.0.0.1", "10.0.0.2"]) + expect(Array.from(forwarded(req))).toStrictEqual([ip`127.0.0.1`, ip`10.0.0.1`, ip`10.0.0.2`]) }) it("should trim leading OWS", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": " 10.0.0.2 , , 10.0.0.1 ", }) as IncomingMessage - expect(forwarded(req)).toEqual(["127.0.0.1", "10.0.0.1", "10.0.0.2"]) + expect(Array.from(forwarded(req))).toStrictEqual([ip`127.0.0.1`, ip`10.0.0.1`, ip`10.0.0.2`]) }) }) diff --git a/tests/modules/proxy-addr.test.ts b/tests/modules/proxy-addr.test.ts index 2a89189d..7fca70a3 100644 --- a/tests/modules/proxy-addr.test.ts +++ b/tests/modules/proxy-addr.test.ts @@ -1,5 +1,6 @@ // Original test cases are taken from https://github.com/jshttp/proxy-addr/blob/master/test/test.js +import ipaddr from "ipaddr.js" import { describe, expect, it } from "vitest" import { allAddresses, compile, proxyAddress } from "@/packages/proxy-address/src" @@ -31,48 +32,48 @@ describe("proxyaddr(req, trust)", () => { it("should accept a function", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, trustAll)).toBe("127.0.0.1") + expect(proxyAddress(req, trustAll)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept an array", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, [])).toBe("127.0.0.1") + expect(proxyAddress(req, [])).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept a number", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, 1)).toBe("127.0.0.1") + expect(proxyAddress(req, 1)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept IPv4", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, "127.0.0.1")).toBe("127.0.0.1") + expect(proxyAddress(req, "127.0.0.1")).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept IPv6", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, "::1")).toBe("127.0.0.1") + expect(proxyAddress(req, "::1")).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept IPv4-style IPv6", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, "::ffff:127.0.0.1")).toBe("127.0.0.1") + expect(proxyAddress(req, "::ffff:127.0.0.1")).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept pre-defined names", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, "loopback")).toBe("127.0.0.1") + expect(proxyAddress(req, "loopback")).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should accept pre-defined names in an array", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, ["loopback", "10.0.0.1"])).toBe("127.0.0.1") + expect(proxyAddress(req, ["loopback", "10.0.0.1"])).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should not alter input array", () => { const arr = ["loopback", "10.0.0.1"] const req = createReq("127.0.0.1") - expect(proxyAddress(req, arr)).toBe("127.0.0.1") + expect(proxyAddress(req, arr)).toStrictEqual(ipaddr.parse("127.0.0.1")) expect(arr).toEqual(["loopback", "10.0.0.1"]) }) it.each(["blegh", "10.0.300.1", "::ffff:30.168.1.9000", "-1"])("should reject non-IP '%s'", (value: unknown) => { @@ -116,9 +117,10 @@ describe("proxyaddr(req, trust)", () => { return true }) - expect(log).toStrictEqual([ - ["127.0.0.1", 0], - ["10.0.0.1", 1], + expect(log).toMatchObject([ + [ipaddr.parse("127.0.0.1"), 0], + [ipaddr.parse("10.0.0.1"), 1], + [ipaddr.parse("192.168.0.1"), 2], ]) }) }) @@ -128,21 +130,21 @@ describe("proxyaddr(req, trust)", () => { it("should return socket address with no headers", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, trustAll)).toBe("127.0.0.1") + expect(proxyAddress(req, trustAll)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should return header value", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1", }) - expect(proxyAddress(req, trustAll)).toBe("10.0.0.1") + expect(proxyAddress(req, trustAll)).toStrictEqual(ipaddr.parse("10.0.0.1")) }) it("should return furthest header value", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1, 10.0.0.2", }) - expect(proxyAddress(req, trustAll)).toBe("10.0.0.1") + expect(proxyAddress(req, trustAll)).toStrictEqual(ipaddr.parse("10.0.0.1")) }) }) @@ -150,14 +152,14 @@ describe("proxyaddr(req, trust)", () => { it("should return socket address with no headers", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, trustNone)).toBe("127.0.0.1") + expect(proxyAddress(req, trustNone)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should return socket address with headers", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1", }) - expect(proxyAddress(req, trustNone)).toBe("127.0.0.1") + expect(proxyAddress(req, trustNone)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) }) @@ -165,35 +167,35 @@ describe("proxyaddr(req, trust)", () => { it("should return socket address with no headers", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, trust10x)).toBe("127.0.0.1") + expect(proxyAddress(req, trust10x)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should return socket address when not trusted", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1, 10.0.0.2", }) - expect(proxyAddress(req, trust10x)).toBe("127.0.0.1") + expect(proxyAddress(req, trust10x)).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should return header when socket trusted", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1", }) - expect(proxyAddress(req, trust10x)).toBe("192.168.0.1") + expect(proxyAddress(req, trust10x)).toStrictEqual(ipaddr.parse("192.168.0.1")) }) it("should return first untrusted after trusted", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, trust10x)).toBe("192.168.0.1") + expect(proxyAddress(req, trust10x)).toStrictEqual(ipaddr.parse("192.168.0.1")) }) it("should not skip untrusted", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "10.0.0.3, 192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, trust10x)).toBe("192.168.0.1") + expect(proxyAddress(req, trust10x)).toStrictEqual(ipaddr.parse("192.168.0.1")) }) }) @@ -203,35 +205,44 @@ describe("proxyaddr(req, trust)", () => { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toBe("192.168.0.1") + expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toStrictEqual(ipaddr.parse("192.168.0.1")) }) - it("should not trust non-IP addresses", () => { + it("should throw for non-IP address set by trusted proxy", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.2, localhost", }) - expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toBe("localhost") + expect(() => { + proxyAddress(req, ["10.0.0.1", "10.0.0.2"]) + }).toThrow() + }) + it("should not throw for non-IP address set by untrusted proxy", () => { + const req = createReq("10.0.0.1", { + "x-forwarded-for": "192.168.0.1, 10.0.0.2, localhost", + }) + + expect(proxyAddress(req, [])).toStrictEqual(ipaddr.parse("10.0.0.1")) }) it("should return socket address if none match", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, ["127.0.0.1", "192.168.0.100"])).toBe("10.0.0.1") + expect(proxyAddress(req, ["127.0.0.1", "192.168.0.100"])).toStrictEqual(ipaddr.parse("10.0.0.1")) }) describe("when array is empty", () => { it("should return socket address", () => { const req = createReq("127.0.0.1") - expect(proxyAddress(req, [])).toBe("127.0.0.1") + expect(proxyAddress(req, [])).toStrictEqual(ipaddr.parse("127.0.0.1")) }) it("should return socket address with headers", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1, 10.0.0.2", }) - expect(proxyAddress(req, [])).toBe("127.0.0.1") + expect(proxyAddress(req, [])).toStrictEqual(ipaddr.parse("127.0.0.1")) }) }) }) @@ -242,21 +253,21 @@ describe("proxyaddr(req, trust)", () => { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toBe("192.168.0.1") + expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toStrictEqual(ipaddr.parse("192.168.0.1")) }) it("should accept CIDR notation", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.200", }) - expect(proxyAddress(req, "10.0.0.2/26")).toBe("10.0.0.200") + expect(proxyAddress(req, "10.0.0.2/26")).toStrictEqual(ipaddr.parse("10.0.0.200")) }) it("should accept netmask notation", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.200", }) - expect(proxyAddress(req, "10.0.0.2/255.255.255.192")).toBe("10.0.0.200") + expect(proxyAddress(req, "10.0.0.2/255.255.255.192")).toStrictEqual(ipaddr.parse("10.0.0.200")) }) }) describe("when given IPv6 address", () => { @@ -265,14 +276,14 @@ describe("proxyaddr(req, trust)", () => { "x-forwarded-for": "2002:c000:203::1, fe80::2", }) - expect(proxyAddress(req, ["fe80::1", "fe80::2"])).toBe("2002:c000:203::1") + expect(proxyAddress(req, ["fe80::1", "fe80::2"])).toStrictEqual(ipaddr.parse("2002:c000:203::1")) }) it("should accept CIDR notation", () => { const req = createReq("fe80::1", { "x-forwarded-for": "2002:c000:203::1, fe80::ff00", }) - expect(proxyAddress(req, "fe80::/125")).toBe("fe80::ff00") + expect(proxyAddress(req, "fe80::/125")).toStrictEqual(ipaddr.parse("fe80::ff00")) }) }) describe("with mixed IP versions", () => { @@ -281,14 +292,14 @@ describe("proxyaddr(req, trust)", () => { "x-forwarded-for": "2002:c000:203::1", }) - expect(proxyAddress(req, ["127.0.0.1", "::1"])).toBe("2002:c000:203::1") + expect(proxyAddress(req, ["127.0.0.1", "::1"])).toStrictEqual(ipaddr.parse("2002:c000:203::1")) }) it("should not match IPv4 to IPv6", () => { const req = createReq("::1", { "x-forwarded-for": "2002:c000:203::1", }) - expect(proxyAddress(req, "127.0.0.1")).toBe("::1") + expect(proxyAddress(req, "127.0.0.1")).toStrictEqual(ipaddr.parse("::1")) }) }) @@ -298,42 +309,42 @@ describe("proxyaddr(req, trust)", () => { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toBe("192.168.0.1") + expect(proxyAddress(req, ["10.0.0.1", "10.0.0.2"])).toStrictEqual(ipaddr.parse("192.168.0.1")) }) it("should match IPv4 netmask trust to IPv6 request", () => { const req = createReq("::ffff:a00:1", { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, ["10.0.0.1/16"])).toBe("192.168.0.1") + expect(proxyAddress(req, ["10.0.0.1/16"])).toStrictEqual(ipaddr.parse("192.168.0.1")) }) it("should match IPv6 trust to IPv4 request", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.2", }) - expect(proxyAddress(req, ["::ffff:a00:1", "::ffff:a00:2"])).toBe("192.168.0.1") + expect(proxyAddress(req, ["::ffff:a00:1", "::ffff:a00:2"])).toStrictEqual(ipaddr.parse("192.168.0.1")) }) it("should match CIDR notation for IPv4-mapped address", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.200", }) - expect(proxyAddress(req, "::ffff:a00:2/122")).toBe("10.0.0.200") + expect(proxyAddress(req, "::ffff:a00:2/122")).toStrictEqual(ipaddr.parse("10.0.0.200")) }) it("should match CIDR notation for IPv4-mapped address mixed with IPv6 CIDR", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.200", }) - expect(proxyAddress(req, ["::ffff:a00:2/122", "fe80::/125"])).toBe("10.0.0.200") + expect(proxyAddress(req, ["::ffff:a00:2/122", "fe80::/125"])).toStrictEqual(ipaddr.parse("10.0.0.200")) }) it("should match CIDR notation for IPv4-mapped address mixed with IPv4 addresses", () => { const req = createReq("10.0.0.1", { "x-forwarded-for": "192.168.0.1, 10.0.0.200", }) - expect(proxyAddress(req, ["::ffff:a00:2/122", "127.0.0.1"])).toBe("10.0.0.200") + expect(proxyAddress(req, ["::ffff:a00:2/122", "127.0.0.1"])).toStrictEqual(ipaddr.parse("10.0.0.200")) }) }) @@ -343,47 +354,53 @@ describe("proxyaddr(req, trust)", () => { "x-forwarded-for": "2002:c000:203::1, fe80::2", }) - expect(proxyAddress(req, "linklocal")).toBe("2002:c000:203::1") + expect(proxyAddress(req, "linklocal")).toStrictEqual(ipaddr.parse("2002:c000:203::1")) }) it("should accept multiple pre-defined names", () => { const req = createReq("::1", { "x-forwarded-for": "2002:c000:203::1, fe80::2", }) - expect(proxyAddress(req, ["loopback", "linklocal"])).toBe("2002:c000:203::1") + expect(proxyAddress(req, ["loopback", "linklocal"])).toStrictEqual(ipaddr.parse("2002:c000:203::1")) }) }) describe("when header contains non-ip addresses", () => { - it("should stop at first non-trusted non-ip", () => { + it("should throw at non-ip set by a trusted proxy", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "myrouter, 127.0.0.1, proxy", }) - expect(proxyAddress(req, "127.0.0.1")).toBe("proxy") + expect(() => { + proxyAddress(req, "127.0.0.1") + }).toThrow() }) - it("should stop at first non-trusted malformed ip", () => { + it("should throw at malformed ip set by a trusted proxy", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "myrouter, 127.0.0.1, ::8:8:8:8:8:8:8:8:8", }) - expect(proxyAddress(req, "127.0.0.1")).toBe("::8:8:8:8:8:8:8:8:8") + expect(() => { + proxyAddress(req, "127.0.0.1") + }).toThrow() }) - it("should provide all values to function", () => { + it("should test all values up to first non-ip set by a trusted proxy, then throw", () => { const log: Array<[string | undefined, number]> = [] const req = createReq("127.0.0.1", { - "x-forwarded-for": "myrouter, 127.0.0.1, proxy", + "x-forwarded-for": "myrouter, ::1, ::fe80", }) - proxyAddress(req, (addr, i) => { - log.push([addr, i]) - return true - }) + expect(() => { + proxyAddress(req, (addr, i) => { + log.push([addr, i]) + return true + }) + }).toThrow() expect(log).toStrictEqual([ - ["127.0.0.1", 0], - ["proxy", 1], - ["127.0.0.1", 2], + [ipaddr.parse("127.0.0.1"), 0], + [ipaddr.parse("::fe80"), 1], + [ipaddr.parse("::1"), 2], ]) }) }) @@ -412,7 +429,7 @@ describe("proxyaddr(req, trust)", () => { }) it(`should use the address that is at most ${trust} hops away`, () => { - expect(proxyAddress(req, trust)).toBe(address) + expect(proxyAddress(req, trust)).toStrictEqual(ipaddr.parse(address)) }) }) }) @@ -440,7 +457,7 @@ describe("proxyaddr.all(req, trust?)", () => { describe("with no headers", () => { it("should return socket address", () => { const req = createReq("127.0.0.1") - expect(allAddresses(req)).toStrictEqual(["127.0.0.1"]) + expect(allAddresses(req)).toMatchObject([ipaddr.parse("127.0.0.1")]) }) }) @@ -450,14 +467,18 @@ describe("proxyaddr.all(req, trust?)", () => { "x-forwarded-for": "10.0.0.1", }) - expect(allAddresses(req)).toStrictEqual(["127.0.0.1", "10.0.0.1"]) + expect(allAddresses(req)).toStrictEqual([ipaddr.parse("127.0.0.1"), ipaddr.parse("10.0.0.1")]) }) it("should include x-forwarded-for in the correct order", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1, 10.0.0.2", }) - expect(allAddresses(req)).toStrictEqual(["127.0.0.1", "10.0.0.2", "10.0.0.1"]) + expect(allAddresses(req)).toMatchObject([ + ipaddr.parse("127.0.0.1"), + ipaddr.parse("10.0.0.2"), + ipaddr.parse("10.0.0.1"), + ]) }) }) @@ -467,14 +488,14 @@ describe("proxyaddr.all(req, trust?)", () => { "x-forwarded-for": "10.0.0.1, 10.0.0.2", }) - expect(allAddresses(req, "127.0.0.1")).toStrictEqual(["127.0.0.1", "10.0.0.2"]) + expect(allAddresses(req, "127.0.0.1")).toMatchObject([ipaddr.parse("127.0.0.1"), ipaddr.parse("10.0.0.2")]) }) it("should return only socket address when nothing is trusted", () => { const req = createReq("127.0.0.1", { "x-forwarded-for": "10.0.0.1, 10.0.0.2", }) - expect(allAddresses(req, [])).toStrictEqual(["127.0.0.1"]) + expect(allAddresses(req, [])).toMatchObject([ipaddr.parse("127.0.0.1")]) }) }) }) From 1185a74507c02b1e37951556311e086ec1f89e8c Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 20:49:33 +0000 Subject: [PATCH 4/8] fix: type-checking complaint --- packages/request/src/host.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/request/src/host.ts b/packages/request/src/host.ts index 146eb199..ee1b974b 100644 --- a/packages/request/src/host.ts +++ b/packages/request/src/host.ts @@ -56,7 +56,7 @@ const getHostString = (req: HasHeaders & HasIpAddresses, trust: Trust): string | return authorityHost ?? defaultHost ?? undefined } -export const getHost = (req: HasHeaders & HasSocket, trust: Trust): Host => { +export const getHost = (req: HasHeaders & HasIpAddresses, trust: Trust): Host => { const host = getHostString(req, trust) if (!host) throw new Error("Request does not include valid host information") From 5a128d9b41062211a3d2d5898ef01d629ffdc5a9 Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 20:50:46 +0000 Subject: [PATCH 5/8] fix: another type-checking complaint --- packages/request/src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/request/src/types.ts b/packages/request/src/types.ts index 2f06d04d..4e1ee258 100644 --- a/packages/request/src/types.ts +++ b/packages/request/src/types.ts @@ -3,7 +3,7 @@ import type { Socket } from "node:net" import type { IPv4, IPv6 } from "@otterhttp/proxy-address" -export type HasIpAddresses = { ips: Array } +export type HasIpAddresses = { ips: readonly (IPv4 | IPv6 | undefined)[] } export type HasHeaders = { headers: IncomingHttpHeaders } export type HasSocket = { socket: Socket } From 90b8989259d5ded73d20e7dee2872da984f0b565 Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 23:05:41 +0000 Subject: [PATCH 6/8] fix: `x-forwarded-proto` handling --- packages/request/src/addresses.ts | 4 +-- packages/request/src/protocol.ts | 2 +- packages/request/src/prototype.ts | 4 +-- test_helpers/initAppAndTest.ts | 6 ++-- tests/core/request.test.ts | 47 +++++++++++++++++++++++++++++++ 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/packages/request/src/addresses.ts b/packages/request/src/addresses.ts index 6ed8d563..617dae31 100644 --- a/packages/request/src/addresses.ts +++ b/packages/request/src/addresses.ts @@ -1,8 +1,6 @@ -import { type IPv4, type IPv6, type Trust, allAddresses, proxyAddress } from "@otterhttp/proxy-address" +import { type IPv4, type IPv6, type Trust, allAddresses } from "@otterhttp/proxy-address" import type { HasHeaders, HasSocket } from "./types" -export const getIP = (req: HasHeaders & HasSocket, trust: Trust): IPv4 | IPv6 | undefined => proxyAddress(req, trust) - export const getIPs = (req: HasHeaders & HasSocket, trust: Trust): Array => allAddresses(req, trust) diff --git a/packages/request/src/protocol.ts b/packages/request/src/protocol.ts index 6d54c0be..778e1e22 100644 --- a/packages/request/src/protocol.ts +++ b/packages/request/src/protocol.ts @@ -15,7 +15,7 @@ export const getProtocol = (req: HasHeaders & HasSocket & HasIpAddresses, trust: if (!trustRemoteAddress(req, trust)) return proto - const header = (req.headers["X-Forwarded-Proto"] as string) || proto + const header = (req.headers["x-forwarded-proto"] as string) || proto const index = header.indexOf(",") diff --git a/packages/request/src/prototype.ts b/packages/request/src/prototype.ts index 6f3c683e..052306a0 100644 --- a/packages/request/src/prototype.ts +++ b/packages/request/src/prototype.ts @@ -7,7 +7,7 @@ import type { Middleware } from "@otterhttp/router" import { type URLParams, getQueryParams } from "@otterhttp/url" import type { Result as RangeParseResult, Options as RangeParsingOptions, Ranges } from "header-range-parser" -import { getIP, getIPs } from "./addresses" +import { getIPs } from "./addresses" import { type Cookie, parseCookieHeader } from "./cookies" import { getRequestHeader } from "./get-header" import { getHost, getSubdomains } from "./host" @@ -47,8 +47,8 @@ export class Request extends IncomingMessage { populate({ trust, subdomainOffset }: { trust: Trust; subdomainOffset: number | undefined }) { this._acceptsMeta = new Accepts(this) this._query = getQueryParams(this.url) - this._ip = getIP(this, trust) this._ips = getIPs(this, trust) + this._ip = this._ips[this._ips.length - 1] this._protocol = getProtocol(this, trust) const host = getHost(this, trust) this._hostname = host.hostname diff --git a/test_helpers/initAppAndTest.ts b/test_helpers/initAppAndTest.ts index a49d58c3..9bef3650 100644 --- a/test_helpers/initAppAndTest.ts +++ b/test_helpers/initAppAndTest.ts @@ -1,14 +1,14 @@ import type { Server } from "node:http" +import { type FetchFunction, makeFetch } from "supertest-fetch" + import { App, type Request, type Response } from "@/packages/app/src" import type { Handler } from "@/packages/app/src" -import { type FetchFunction, makeFetch } from "supertest-fetch" export const InitAppAndTest = ( handler: Handler, - route?: string, + route?: string | undefined, method = "get", settings: ConstructorParameters[0] = {}, - // @ts-ignore https://github.com/DefinitelyTyped/DefinitelyTyped/pull/70289 ): { fetch: FetchFunction; app: App; server: Server> } => { const app = new App(settings) diff --git a/tests/core/request.test.ts b/tests/core/request.test.ts index cac890e5..355f4683 100644 --- a/tests/core/request.test.ts +++ b/tests/core/request.test.ts @@ -224,6 +224,53 @@ describe("Request properties", () => { await fetch("/").expect(200, "protocol: http") }) + it("req.protocol respects X-Forwarded-Proto when proxy is trusted", async () => { + const { fetch, app } = InitAppAndTest((req, res) => { + res.send(`protocol: ${req.protocol}`) + }) + app.set("trust proxy", ["loopback"]) + + const agentIpv4 = new Agent({ family: 4 }) // ensure IPv4 only + await fetch("/", { + agent: agentIpv4, + headers: new Headers({ + "x-forwarded-proto": "https", + "x-forwarded-for": "127.0.0.1", + }), + }).expect(200, "protocol: https") + + const agentIpv6 = new Agent({ family: 6 }) // ensure IPv4 only + await fetch("/", { + agent: agentIpv6, + headers: new Headers({ + "x-forwarded-proto": "https", + "x-forwarded-for": "::1", + }), + }).expect(200, "protocol: https") + }) + it("req.protocol does not respect X-Forwarded-Proto when proxy is untrusted", async () => { + const { fetch, app } = InitAppAndTest((req, res) => { + res.send(`protocol: ${req.protocol}`) + }) + + const agentIpv4 = new Agent({ family: 4 }) // ensure IPv4 only + await fetch("/", { + agent: agentIpv4, + headers: new Headers({ + "x-forwarded-proto": "https", + "x-forwarded-for": "127.0.0.1", + }), + }).expect(200, "protocol: http") + + const agentIpv6 = new Agent({ family: 6 }) // ensure IPv4 only + await fetch("/", { + agent: agentIpv6, + headers: new Headers({ + "x-forwarded-proto": "https", + "x-forwarded-for": "::1", + }), + }).expect(200, "protocol: http") + }) it("req.secure is false by default", async () => { const { fetch } = InitAppAndTest((req, res) => { res.send(`secure: ${req.secure}`) From 364e29ecbc36faefdba34ead54c8723e6535ca3b Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 23:07:32 +0000 Subject: [PATCH 7/8] chore: create changeset --- .changeset/weak-toes-refuse.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/weak-toes-refuse.md diff --git a/.changeset/weak-toes-refuse.md b/.changeset/weak-toes-refuse.md new file mode 100644 index 00000000..1c507ca9 --- /dev/null +++ b/.changeset/weak-toes-refuse.md @@ -0,0 +1,7 @@ +--- +"@otterhttp/proxy-address": patch +"@otterhttp/forwarded": patch +"@otterhttp/request": patch +--- + +Fix: `req.ip`, `req.ips`, `req.proto` inaccuracies when (trusted) proxies are involved From a3b62f5de4e344afc0f22bd9cc6d81a2013e564d Mon Sep 17 00:00:00 2001 From: Lordfirespeed <28568841+Lordfirespeed@users.noreply.github.com> Date: Tue, 5 Nov 2024 23:39:06 +0000 Subject: [PATCH 8/8] fix: conditional testcase is incorrect --- tests/core/request.test.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/core/request.test.ts b/tests/core/request.test.ts index 355f4683..ea8e0fbc 100644 --- a/tests/core/request.test.ts +++ b/tests/core/request.test.ts @@ -186,18 +186,15 @@ describe("Request properties", () => { ips: ["127.0.0.1"], }) }) - if (process.env.GITHUB_ACTION) { - // skip ipv6 tests only for github ci/cd - it("IPv6 req.ip & req.ips is being parsed properly", async () => { - const { fetch } = InitAppAndTest(ipHandler) + it("IPv6 req.ip & req.ips is being parsed properly", async () => { + const { fetch } = InitAppAndTest(ipHandler) - const agent = new Agent({ family: 6 }) // ensure IPv6 only - await fetch("/", { agent }).expect(200, { - ip: "1", - ips: ["::1"], - }) + const agent = new Agent({ family: 6 }) // ensure IPv6 only + await fetch("/", { agent }).expect(200, { + ip: "::1", + ips: ["::1"], }) - } + }) it("IPv4 req.ip & req.ips do not trust proxies by default", async () => { const { fetch } = InitAppAndTest(ipHandler)