From b383b656b72b5e300f23476d1c2417df6dea8c3c Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Sat, 5 Oct 2024 18:55:50 +0000 Subject: [PATCH 1/9] Fixed parsing of x-forwarded-* headers --- packages/astro/src/core/app/node.ts | 47 ++++-- .../astro/test/client-address-node.test.js | 56 +++++-- packages/astro/test/units/app/node.test.js | 151 ++++++++++++++++++ 3 files changed, 223 insertions(+), 31 deletions(-) create mode 100644 packages/astro/test/units/app/node.test.js diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index 9fa1b645ff22..796c8e38d09c 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -60,12 +60,30 @@ 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']; + + // @example "443,8080,80" => "443" + const forwardedPort = getFirstForwardedValue(req.headers['x-forwarded-port']); + const port = forwardedPort ?? (isEncrypted ? '443' : '80'); const portInHostname = typeof hostname === 'string' && typeof port === 'string' && hostname.endsWith(port); @@ -77,17 +95,16 @@ export class NodeApp extends App { headers: makeRequestHeaders(req), }; const bodyAllowed = options.method !== 'HEAD' && options.method !== 'GET' && skipBody === false; - if (bodyAllowed) { - Object.assign(options, makeRequestBody(req)); - } + if (bodyAllowed) Object.assign(options, makeRequestBody(req)); + const request = new Request(url, options); - const clientIp = req.headers['x-forwarded-for']; - if (clientIp) { - Reflect.set(request, clientAddressSymbol, clientIp); - } else if (req.socket?.remoteAddress) { - Reflect.set(request, clientAddressSymbol, req.socket.remoteAddress); - } + // 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); + return request; } diff --git a/packages/astro/test/client-address-node.test.js b/packages/astro/test/client-address-node.test.js index 8114ea42d210..2cc582ac1e3a 100644 --- 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 new file mode 100644 index 000000000000..3e2430cd8974 --- /dev/null +++ b/packages/astro/test/units/app/node.test.js @@ -0,0 +1,151 @@ +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/'); + }); + }); + }); +}); From e051eea45139270a660b921736b078eee58de9f2 Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Sat, 5 Oct 2024 19:12:45 +0000 Subject: [PATCH 2/9] changeset --- .changeset/slimy-buses-agree.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/slimy-buses-agree.md diff --git a/.changeset/slimy-buses-agree.md b/.changeset/slimy-buses-agree.md new file mode 100644 index 000000000000..800c1b0a7e34 --- /dev/null +++ b/.changeset/slimy-buses-agree.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixed parsing of x-forwarded-\* headers From 880e84f5f1464983c6b2b330ffec006df54dbf89 Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Sat, 5 Oct 2024 19:29:42 +0000 Subject: [PATCH 3/9] remotePort --- packages/astro/src/core/app/node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index 796c8e38d09c..c8955d012f39 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -83,7 +83,7 @@ export class NodeApp extends App { // @example "443,8080,80" => "443" const forwardedPort = getFirstForwardedValue(req.headers['x-forwarded-port']); - const port = forwardedPort ?? (isEncrypted ? '443' : '80'); + const port = forwardedPort ?? req.socket?.remotePort ?? (isEncrypted ? '443' : '80'); const portInHostname = typeof hostname === 'string' && typeof port === 'string' && hostname.endsWith(port); From ac0a7124861213e89b27449f9824f04882f7dbae Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Sat, 5 Oct 2024 19:51:43 +0000 Subject: [PATCH 4/9] port fix --- packages/astro/src/core/app/node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index c8955d012f39..64f6b67a6964 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -86,7 +86,7 @@ export class NodeApp extends App { const port = forwardedPort ?? req.socket?.remotePort ?? (isEncrypted ? '443' : '80'); const portInHostname = - typeof hostname === 'string' && typeof port === 'string' && hostname.endsWith(port); + typeof hostname === 'string' && typeof port === 'string' && /:\d+$/.test(hostname); const hostnamePort = portInHostname ? hostname : hostname + (port ? `:${port}` : ''); const url = `${protocol}://${hostnamePort}${req.url}`; From 5b7022533e83512b670f3fb4a75794c8e4bc21e8 Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Sat, 5 Oct 2024 20:13:24 +0000 Subject: [PATCH 5/9] port fix --- packages/astro/src/core/app/node.ts | 7 +++---- packages/astro/test/units/app/node.test.js | 24 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index 64f6b67a6964..66d2e6f9e48c 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -83,11 +83,10 @@ export class NodeApp extends App { // @example "443,8080,80" => "443" const forwardedPort = getFirstForwardedValue(req.headers['x-forwarded-port']); - const port = forwardedPort ?? req.socket?.remotePort ?? (isEncrypted ? '443' : '80'); + const port = forwardedPort ?? req.socket?.remotePort?.toString() ?? (isEncrypted ? '443' : '80'); - const portInHostname = - typeof hostname === 'string' && typeof port === 'string' && /:\d+$/.test(hostname); - const hostnamePort = portInHostname ? hostname : hostname + (port ? `:${port}` : ''); + const portInHostname = typeof hostname === 'string' && /:\d+$/.test(hostname); + const hostnamePort = portInHostname ? hostname : hostname?.toString() + (port ? `:${port}` : ''); const url = `${protocol}://${hostnamePort}${req.url}`; const options: RequestInit = { diff --git a/packages/astro/test/units/app/node.test.js b/packages/astro/test/units/app/node.test.js index 3e2430cd8974..06065406edb5 100644 --- a/packages/astro/test/units/app/node.test.js +++ b/packages/astro/test/units/app/node.test.js @@ -146,6 +146,30 @@ describe('NodeApp', () => { }); 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/'); + }); }); }); }); From 23ed31c4385811ce7446ef06ea4851140044a0aa Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Sat, 5 Oct 2024 20:18:15 +0000 Subject: [PATCH 6/9] port fix --- packages/astro/src/core/app/node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index 66d2e6f9e48c..ff816360544a 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -86,7 +86,7 @@ export class NodeApp extends App { const port = forwardedPort ?? req.socket?.remotePort?.toString() ?? (isEncrypted ? '443' : '80'); const portInHostname = typeof hostname === 'string' && /:\d+$/.test(hostname); - const hostnamePort = portInHostname ? hostname : hostname?.toString() + (port ? `:${port}` : ''); + const hostnamePort = portInHostname ? hostname : `${hostname}:${port}` const url = `${protocol}://${hostnamePort}${req.url}`; const options: RequestInit = { From fe8c473d75ce6de394366d2cbf6e068dd0fcd1af Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Tue, 8 Oct 2024 20:28:35 +0200 Subject: [PATCH 7/9] Update .changeset/slimy-buses-agree.md Co-authored-by: Emanuele Stoppa --- .changeset/slimy-buses-agree.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.changeset/slimy-buses-agree.md b/.changeset/slimy-buses-agree.md index 800c1b0a7e34..9dad2acaf676 100644 --- a/.changeset/slimy-buses-agree.md +++ b/.changeset/slimy-buses-agree.md @@ -2,4 +2,6 @@ 'astro': patch --- -Fixed parsing of x-forwarded-\* headers +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. From 1a889cb6d9b6e5801d8c9b67aafa205aa181231d Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Tue, 8 Oct 2024 20:29:16 +0200 Subject: [PATCH 8/9] Update packages/astro/src/core/app/node.ts Co-authored-by: Emanuele Stoppa --- packages/astro/src/core/app/node.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index 72e4d989fc81..861c5ab1e1b6 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -103,7 +103,9 @@ export class NodeApp extends App { // @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); + if (clientIp) { + Reflect.set(request, clientAddressSymbol, clientIp); + } return request; } From af15d0ca057701eb903ea1e489a52961c48c7429 Mon Sep 17 00:00:00 2001 From: Jan Havlena Date: Tue, 8 Oct 2024 18:32:58 +0000 Subject: [PATCH 9/9] Reverted formating change --- packages/astro/src/core/app/node.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/astro/src/core/app/node.ts b/packages/astro/src/core/app/node.ts index 861c5ab1e1b6..8dd312760f25 100644 --- a/packages/astro/src/core/app/node.ts +++ b/packages/astro/src/core/app/node.ts @@ -95,7 +95,9 @@ export class NodeApp extends App { headers: makeRequestHeaders(req), }; const bodyAllowed = options.method !== 'HEAD' && options.method !== 'GET' && skipBody === false; - if (bodyAllowed) Object.assign(options, makeRequestBody(req)); + if (bodyAllowed) { + Object.assign(options, makeRequestBody(req)); + } const request = new Request(url, options);