Fixed parsing of x-forwarded-* headers (#12130)

* Fixed parsing of x-forwarded-* headers * changeset * remotePort * port fix * port fix * port fix * Update .changeset/slimy-buses-agree.md Co-authored-by: Emanuele Stoppa <[email protected]> * Update packages/astro/src/core/app/node.ts Co-authored-by: Emanuele Stoppa <[email protected]> * Reverted formating change --------- Co-authored-by: Emanuele Stoppa <[email protected]>
withastro · Oct 9, 2024 · e96bcae · e96bcae
1 parent 22c70a2
commit e96bcae
Show file tree

Hide file tree

Showing 4 changed files with 255 additions and 29 deletions.
diff --git a/.changeset/slimy-buses-agree.md b/.changeset/slimy-buses-agree.md
@@ -0,0 +1,7 @@
+---
+'astro': patch
+---
+
+Fixes a bug in the parsing of  `x-forwarded-\*` `Request` headers, where multiple values assigned to those headers were not correctly parsed.
+
+Now, headers like `x-forwarded-proto: https,http` are correctly parsed.
diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts
@@ -61,16 +61,33 @@ export class NodeApp extends App {
 	 * ```
 	 */
 	static createRequest(req: NodeRequest, { skipBody = false } = {}): Request {
-		const protocol =
-			req.headers['x-forwarded-proto'] ??
-			('encrypted' in req.socket && req.socket.encrypted ? 'https' : 'http');
-		const hostname =
-			req.headers['x-forwarded-host'] ?? req.headers.host ?? req.headers[':authority'];
-		const port = req.headers['x-forwarded-port'];
+		const isEncrypted = 'encrypted' in req.socket && req.socket.encrypted;
+
+		// Parses multiple header and returns first value if available.
+		const getFirstForwardedValue = (multiValueHeader?: string | string[]) => {
+			return multiValueHeader
+				?.toString()
+				?.split(',')
+				.map((e) => e.trim())?.[0];
+		};
+
+		// Get the used protocol between the end client and first proxy.
+		// NOTE: Some proxies append values with spaces and some do not.
+		// We need to handle it here and parse the header correctly.
+		// @example "https, http,http" => "http"
+		const forwardedProtocol = getFirstForwardedValue(req.headers['x-forwarded-proto']);
+		const protocol = forwardedProtocol ?? (isEncrypted ? 'https' : 'http');
+
+		// @example "example.com,www2.example.com" => "example.com"
+		const forwardedHostname = getFirstForwardedValue(req.headers['x-forwarded-host']);
+		const hostname = forwardedHostname ?? req.headers.host ?? req.headers[':authority'];
 
-		const portInHostname =
-			typeof hostname === 'string' && typeof port === 'string' && hostname.endsWith(port);
-		const hostnamePort = portInHostname ? hostname : hostname + (port ? `:${port}` : '');
+		// @example "443,8080,80" => "443"
+		const forwardedPort = getFirstForwardedValue(req.headers['x-forwarded-port']);
+		const port = forwardedPort ?? req.socket?.remotePort?.toString() ?? (isEncrypted ? '443' : '80');
+
+		const portInHostname = typeof hostname === 'string' && /:\d+$/.test(hostname);
+		const hostnamePort = portInHostname ? hostname : `${hostname}:${port}`
 
 		const url = `${protocol}://${hostnamePort}${req.url}`;
 		const options: RequestInit = {
@@ -81,14 +98,17 @@ export class NodeApp extends App {
 		if (bodyAllowed) {
 			Object.assign(options, makeRequestBody(req));
 		}
+
 		const request = new Request(url, options);
 
-		const clientIp = req.headers['x-forwarded-for'];
-		if (clientIp) {
+		// Get the IP of end client behind the proxy.
+		// @example "1.1.1.1,8.8.8.8" => "1.1.1.1"
+		const forwardedClientIp = getFirstForwardedValue(req.headers['x-forwarded-for']);
+		const clientIp = forwardedClientIp || req.socket?.remoteAddress;
+		if (clientIp) { 
 			Reflect.set(request, clientAddressSymbol, clientIp);
-		} else if (req.socket?.remoteAddress) {
-			Reflect.set(request, clientAddressSymbol, req.socket.remoteAddress);
 		}
+
 		return request;
 	}
 

diff --git a/packages/astro/test/client-address-node.test.js b/packages/astro/test/client-address-node.test.js
@@ -5,23 +5,47 @@ import { loadFixture } from './test-utils.js';
 import { createRequestAndResponse } from './units/test-utils.js';
 
 describe('NodeClientAddress', () => {
-	it('clientAddress is 1.1.1.1', async () => {
-		const fixture = await loadFixture({
-			root: './fixtures/client-address-node/',
+	describe('single value', () => {
+		it('clientAddress is 1.1.1.1', async () => {
+			const fixture = await loadFixture({
+				root: './fixtures/client-address-node/',
+			});
+			await fixture.build();
+			const handle = await fixture.loadNodeAdapterHandler();
+			const { req, res, text } = createRequestAndResponse({
+				method: 'GET',
+				url: '/',
+				headers: {
+					'x-forwarded-for': '1.1.1.1',
+				},
+			});
+			handle(req, res);
+			const html = await text();
+			const $ = cheerio.load(html);
+			assert.equal(res.statusCode, 200);
+			assert.equal($('#address').text(), '1.1.1.1');
 		});
-		await fixture.build();
-		const handle = await fixture.loadNodeAdapterHandler();
-		const { req, res, text } = createRequestAndResponse({
-			method: 'GET',
-			url: '/',
-			headers: {
-				'x-forwarded-for': '1.1.1.1',
-			},
+	});
+
+	describe('multiple values', () => {
+		it('clientAddress is 1.1.1.1', async () => {
+			const fixture = await loadFixture({
+				root: './fixtures/client-address-node/',
+			});
+			await fixture.build();
+			const handle = await fixture.loadNodeAdapterHandler();
+			const { req, res, text } = createRequestAndResponse({
+				method: 'GET',
+				url: '/',
+				headers: {
+					'x-forwarded-for': '1.1.1.1,8.8.8.8, 8.8.8.2',
+				},
+			});
+			handle(req, res);
+			const html = await text();
+			const $ = cheerio.load(html);
+			assert.equal(res.statusCode, 200);
+			assert.equal($('#address').text(), '1.1.1.1');
 		});
-		handle(req, res);
-		const html = await text();
-		const $ = cheerio.load(html);
-		assert.equal(res.statusCode, 200);
-		assert.equal($('#address').text(), '1.1.1.1');
 	});
 });
diff --git a/packages/astro/test/units/app/node.test.js b/packages/astro/test/units/app/node.test.js
@@ -0,0 +1,175 @@
+import * as assert from 'node:assert/strict';
+import { describe, it } from 'node:test';
+import { NodeApp } from '../../../dist/core/app/node.js';
+
+const mockNodeRequest = {
+	url: '/',
+	method: 'GET',
+	headers: {
+		host: 'example.com',
+	},
+	socket: {
+		encrypted: true,
+		remoteAddress: '2.2.2.2',
+	},
+};
+
+describe('NodeApp', () => {
+	describe('createRequest', () => {
+		describe('x-forwarded-for', () => {
+			it('parses client IP from single-value x-forwarded-for header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						'x-forwarded-for': '1.1.1.1',
+					},
+				});
+				assert.equal(result[Symbol.for('astro.clientAddress')], '1.1.1.1');
+			});
+
+			it('parses client IP from multi-value x-forwarded-for header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						'x-forwarded-for': '1.1.1.1,8.8.8.8',
+					},
+				});
+				assert.equal(result[Symbol.for('astro.clientAddress')], '1.1.1.1');
+			});
+
+			it('parses client IP from multi-value x-forwarded-for header with spaces', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						'x-forwarded-for': ' 1.1.1.1, 8.8.8.8, 8.8.8.2',
+					},
+				});
+				assert.equal(result[Symbol.for('astro.clientAddress')], '1.1.1.1');
+			});
+
+			it('fallbacks to remoteAddress when no x-forwarded-for header is present', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {},
+				});
+				assert.equal(result[Symbol.for('astro.clientAddress')], '2.2.2.2');
+			});
+		});
+
+		describe('x-forwarded-host', () => {
+			it('parses host from single-value x-forwarded-host header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						'x-forwarded-host': 'www2.example.com',
+					},
+				});
+				assert.equal(result.url, 'https://www2.example.com/');
+			});
+
+			it('parses host from multi-value x-forwarded-host header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						'x-forwarded-host': 'www2.example.com,www3.example.com',
+					},
+				});
+				assert.equal(result.url, 'https://www2.example.com/');
+			});
+
+			it('fallbacks to host header when no x-forwarded-host header is present', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com',
+					},
+				});
+				assert.equal(result.url, 'https://example.com/');
+			});
+		});
+
+		describe('x-forwarded-proto', () => {
+			it('parses protocol from single-value x-forwarded-proto header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com',
+						'x-forwarded-proto': 'http',
+						'x-forwarded-port': '80',
+					},
+				});
+				assert.equal(result.url, 'http://example.com/');
+			});
+
+			it('parses protocol from multi-value x-forwarded-proto header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com',
+						'x-forwarded-proto': 'http,https',
+						'x-forwarded-port': '80,443',
+					},
+				});
+				assert.equal(result.url, 'http://example.com/');
+			});
+
+			it('fallbacks to encrypted property when no x-forwarded-proto header is present', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com',
+					},
+				});
+				assert.equal(result.url, 'https://example.com/');
+			});
+		});
+
+		describe('x-forwarded-port', () => {
+			it('parses port from single-value x-forwarded-port header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com',
+						'x-forwarded-port': '8443',
+					},
+				});
+				assert.equal(result.url, 'https://example.com:8443/');
+			});
+
+			it('parses port from multi-value x-forwarded-port header', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com',
+						'x-forwarded-port': '8443,3000',
+					},
+				});
+				assert.equal(result.url, 'https://example.com:8443/');
+			});
+
+			it('prefers port from host', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com:3000',
+						'x-forwarded-port': '443',
+					},
+				});
+				assert.equal(result.url, 'https://example.com:3000/');
+			});
+
+
+			it('prefers port from x-forwarded-host', () => {
+				const result = NodeApp.createRequest({
+					...mockNodeRequest,
+					headers: {
+						host: 'example.com:443',
+						'x-forwarded-host': 'example.com:3000',
+						'x-forwarded-port': '443',
+					},
+				});
+				assert.equal(result.url, 'https://example.com:3000/');
+			});
+		});
+	});
+});