From: Jérôme Benoit Date: Sun, 14 Jun 2026 20:07:08 +0000 (+0200) Subject: fix(ui-server): post-#1891 access policy and WS upgrade hardening (#1898) X-Git-Tag: cli@v4.9.0~9 X-Git-Url: https://git.piment-noir.org/?a=commitdiff_plain;h=bf4bc1b519edc264e29d56100778f32cf3240fc7;p=e-mobility-charging-stations-simulator.git fix(ui-server): post-#1891 access policy and WS upgrade hardening (#1898) * fix(ui-server): reject ports in allowedHosts, fix host normalization edges - ConfigurationSchema: reject allowedHosts entries that carry a port (e.g., 'gateway.example.com:8080', '[::1]:8080'). Runtime host matching strips ports, so a port-bearing entry was silently equivalent to its host-only form. Operators are now forced to spell out host-only entries at config load time, matching how matching actually works. - UIServerNet: normalizeHost strips a trailing dot after the colon split rather than from the whole input, so a Host header like 'localhost.:80' now resolves to 'localhost' instead of leaving the trailing dot intact and failing the allowlist match. - UIServerAccessPolicy: nonEmpty treats whitespace-only forwarded values as absent, and hasForwardedHeaders consumes nonEmpty so the two helpers agree. A whitespace-only X-Forwarded-Proto no longer flips forwardedHeadersPresent to true and triggers an Ambiguous* denial. Signed-off-by: Jérôme Benoit * fix(ui-server): attach socket error listener before WS upgrade rejection writes The onSocketError listener was previously attached only after the prologue and bad-header guards had already written their rejection responses, and was removed before the auth-failure rejection write. A client TCP-reset between an upgrade event and the destroy callback could fire 'error' on the Duplex socket with no listener, which Node escalates to an unhandled exception and crashes the process. The listener is now attached at the top of the upgrade handler and only removed once webSocketServer.handleUpgrade takes ownership of the socket. All three rejection paths (bad upgrade headers, prologue failure, auth failure) keep the listener until socket.destroy(), neutralising the DoS vector. Signed-off-by: Jérôme Benoit * refactor(ui-server): tighten access-policy schema messages and forwarded parsing - Schema: split UIServerListenOptions validation so non-object values surface 'must be a non-array object' instead of the misleading 'accessPolicy must be configured under uiServer' message. - Forwarded parser: unescape RFC 7230 quoted-pair sequences inside double-quoted Forwarded parameter values. - getForwardedClientAddress: propagate forwarded.kind === 'error' before the trustedProxy gate, matching getForwardedProtocol and getForwardedHost (no behavioral change today; closes a latent asymmetry if the upstream untrusted-peer gate is reordered). - UIServerAccessCache: document identity-based cache invalidation for trustedProxies; reload flows must construct a new configuration. - getForwardedProtocol / getForwardedHost: distinguish length === 0 (new InvalidForwardedProtocol / InvalidForwardedHost reasons) from length > 1 (existing Ambiguous reasons), matching the existing InvalidForwardedClient / AmbiguousForwardedClient pattern. Signed-off-by: Jérôme Benoit --------- Signed-off-by: Jérôme Benoit --- diff --git a/src/charging-station/ui-server/UIServerAccessPolicy.ts b/src/charging-station/ui-server/UIServerAccessPolicy.ts index 97e0e2f0..5d14ad88 100644 --- a/src/charging-station/ui-server/UIServerAccessPolicy.ts +++ b/src/charging-station/ui-server/UIServerAccessPolicy.ts @@ -36,6 +36,8 @@ export enum UIServerAccessDenialReason { ForwardedFromUntrustedPeer = 'forwarded-from-untrusted-peer', HostNotAllowed = 'host-not-allowed', InvalidForwardedClient = 'invalid-forwarded-client', + InvalidForwardedHost = 'invalid-forwarded-host', + InvalidForwardedProtocol = 'invalid-forwarded-protocol', LoopbackProxyDisabled = 'loopback-proxy-disabled', OriginNotAllowed = 'origin-not-allowed', ProxyTlsRequired = 'proxy-tls-required', @@ -60,6 +62,10 @@ const DENIAL_MESSAGES: Readonly> = { [UIServerAccessDenialReason.HostNotAllowed]: 'Host header is not allowed', [UIServerAccessDenialReason.InvalidForwardedClient]: 'Invalid X-Forwarded-For header is not allowed', + [UIServerAccessDenialReason.InvalidForwardedHost]: + 'Invalid X-Forwarded-Host header is not allowed', + [UIServerAccessDenialReason.InvalidForwardedProtocol]: + 'Invalid X-Forwarded-Proto header is not allowed', [UIServerAccessDenialReason.LoopbackProxyDisabled]: 'Loopback proxy forwarding requires accessPolicy.allowLoopbackProxy=true', [UIServerAccessDenialReason.OriginNotAllowed]: 'Origin header is not allowed', @@ -73,6 +79,10 @@ const DENIAL_MESSAGES: Readonly> = { * requests and the normalized trusted-proxy index of the active * configuration. Both maps are weakly keyed so entries are released with * their owning object. + * + * Cache invalidation is identity-based: mutating + * `accessPolicy.trustedProxies` in place will not refresh the normalized + * set. Reload flows must construct a new {@link UIServerConfiguration}. */ export interface UIServerAccessCache { readonly decisions: WeakMap @@ -202,12 +212,12 @@ const getForwardedClientAddress = ( trustedProxy: boolean, forwarded: ParseOutcome ): ParseOutcome => { - if (!trustedProxy) { - return ABSENT - } if (forwarded.kind === 'error') { return forwarded } + if (!trustedProxy) { + return ABSENT + } const picked = pickForwardedValue( nonEmpty(getSingleHeaderValue(req, 'x-forwarded-for')), forwarded.kind === 'ok' ? forwarded.value.for : undefined, @@ -255,7 +265,10 @@ const getForwardedProtocol = ( } if (xForwardedProtocol != null) { const protocols = splitHeaderList(xForwardedProtocol) - if (protocols.length !== 1) { + if (protocols.length === 0) { + return { kind: 'error', reason: UIServerAccessDenialReason.InvalidForwardedProtocol } + } + if (protocols.length > 1) { return { kind: 'error', reason: UIServerAccessDenialReason.AmbiguousForwardedProtocol } } return { kind: 'ok', value: protocols[0].toLowerCase() } @@ -281,7 +294,10 @@ const getForwardedHost = ( } if (xForwardedHost != null) { const hosts = splitHeaderList(xForwardedHost) - if (hosts.length !== 1) { + if (hosts.length === 0) { + return { kind: 'error', reason: UIServerAccessDenialReason.InvalidForwardedHost } + } + if (hosts.length > 1) { return { kind: 'error', reason: UIServerAccessDenialReason.AmbiguousForwardedHost } } return { kind: 'ok', value: hosts[0] } @@ -302,7 +318,7 @@ const pickForwardedValue = ( } const nonEmpty = (value: string | undefined): string | undefined => - value == null || value === '' ? undefined : value + value == null || value.trim() === '' ? undefined : value // RFC 7239 §6: "unknown" and obfuscated node identifiers ("_" + token chars). // Optional ":port" suffix is stripped before comparison. @@ -327,12 +343,9 @@ const parseSingleForwardedHeader = (req: IncomingMessage): ParseOutcome FORWARDED_HEADER_NAMES.some(headerName => - getHeaderValues(req, headerName).some(value => value !== '') + getHeaderValues(req, headerName).some(value => nonEmpty(value) != null) ) const isHostAllowed = ( diff --git a/src/charging-station/ui-server/UIServerNet.ts b/src/charging-station/ui-server/UIServerNet.ts index 75103705..5cd329f1 100644 --- a/src/charging-station/ui-server/UIServerNet.ts +++ b/src/charging-station/ui-server/UIServerNet.ts @@ -54,7 +54,7 @@ export const normalizeIPAddress = ( * @returns The bare host, or `undefined` when the input is malformed. */ export const normalizeHost = (host: string): string | undefined => { - const trimmedHost = host.trim().toLowerCase().replace(/\.$/, '') + const trimmedHost = host.trim().toLowerCase() if (trimmedHost === '') { return undefined } @@ -69,7 +69,20 @@ export const normalizeHost = (host: string): string | undefined => { if (parts.length > 2 || (parts.length === 2 && !isValidPort(parts[1]))) { return undefined } - return HOSTNAME_PATTERN.test(parts[0]) ? parts[0] : undefined + const hostPart = parts[0].replace(/\.$/, '') + if (hostPart === '') { + return undefined + } + return HOSTNAME_PATTERN.test(hostPart) ? hostPart : undefined +} + +export const isHostLiteralWithoutPort = (value: string): boolean => { + const normalized = normalizeHost(value) + if (normalized == null) { + return false + } + const stripped = value.trim().toLowerCase().replace(/\.$/, '') + return stripped === normalized || stripped === `[${normalized}]` } const HOSTNAME_PATTERN = /^[a-z0-9._-]+$/ diff --git a/src/charging-station/ui-server/UIWebSocketServer.ts b/src/charging-station/ui-server/UIWebSocketServer.ts index a866d683..2130e4f9 100644 --- a/src/charging-station/ui-server/UIWebSocketServer.ts +++ b/src/charging-station/ui-server/UIWebSocketServer.ts @@ -183,6 +183,17 @@ export class UIWebSocketServer extends AbstractUIServer { }) }) this.httpServer.on('upgrade', (req: IncomingMessage, socket: Duplex, head: Buffer): void => { + const onSocketError = (error: Error): void => { + logger.error( + `${this.logPrefix( + moduleName, + 'start.httpServer.on.upgrade' + )} Socket error at connection upgrade event handling:`, + error + ) + } + socket.on('error', onSocketError) + const connectionHeader = req.headers.connection ?? '' const upgradeHeader = req.headers.upgrade ?? '' if (!/upgrade/i.test(connectionHeader) || !/^websocket$/i.test(upgradeHeader)) { @@ -209,18 +220,7 @@ export class UIWebSocketServer extends AbstractUIServer { return } - const onSocketError = (error: Error): void => { - logger.error( - `${this.logPrefix( - moduleName, - 'start.httpServer.on.upgrade' - )} Socket error at connection upgrade event handling:`, - error - ) - } - socket.on('error', onSocketError) if (!this.authenticate(req)) { - socket.removeListener('error', onSocketError) const unauthorized = this.getUnauthorizedDenial() socket.write( buildUpgradeRejectionResponse( diff --git a/src/utils/ConfigurationSchema.ts b/src/utils/ConfigurationSchema.ts index 0e181b52..a9131e3b 100644 --- a/src/utils/ConfigurationSchema.ts +++ b/src/utils/ConfigurationSchema.ts @@ -4,7 +4,7 @@ import type { ResourceLimits } from 'node:worker_threads' import { isIP } from 'node:net' import { z } from 'zod' -import { normalizeHost } from '../charging-station/ui-server/UIServerNet.js' +import { isHostLiteralWithoutPort } from '../charging-station/ui-server/UIServerNet.js' import { ApplicationProtocol, ApplicationProtocolVersion, @@ -105,8 +105,8 @@ export const UIServerAccessPolicySchema = z .object({ allowedHosts: z .array( - z.string().refine(value => normalizeHost(value) != null, { - message: 'must be a valid host (no path, query, or fragment)', + z.string().refine(isHostLiteralWithoutPort, { + message: 'must be a host literal without port (no path, query, or fragment)', }) ) .optional(), @@ -139,14 +139,14 @@ export const UIServerAccessPolicySchema = z }) .strict() -const UIServerListenOptionsSchema = z.custom(value => { - return ( - value != null && - typeof value === 'object' && - !Array.isArray(value) && - !Object.hasOwn(value, 'accessPolicy') +const UIServerListenOptionsSchema = z + .custom( + value => value != null && typeof value === 'object' && !Array.isArray(value), + { message: 'must be a non-array object' } ) -}, "'accessPolicy' must be configured under 'uiServer', not 'uiServer.options'") + .refine(value => !Object.hasOwn(value as object, 'accessPolicy'), { + message: "'accessPolicy' must be configured under 'uiServer', not 'uiServer.options'", + }) /** * UIServerConfiguration — UI server configuration section. diff --git a/tests/charging-station/ui-server/UIServerAccessPolicy.test.ts b/tests/charging-station/ui-server/UIServerAccessPolicy.test.ts index 98c0457d..dcf26fdc 100644 --- a/tests/charging-station/ui-server/UIServerAccessPolicy.test.ts +++ b/tests/charging-station/ui-server/UIServerAccessPolicy.test.ts @@ -334,6 +334,39 @@ await describe('UIServerAccessPolicy', async () => { expectDenied(decision, UIServerAccessDenialReason.AmbiguousForwardedHost) }) + await it('should reject empty X-Forwarded-Proto comma-only lists from a trusted proxy', () => { + const decision = evaluate( + createAccessPolicyRequest({ + headers: { + host: 'gateway.example.com', + 'x-forwarded-for': '203.0.113.10', + 'x-forwarded-proto': ',,,', + }, + remoteAddress: '192.0.2.10', + }), + createGatewayConfigWithTrustedProxy() + ) + + expectDenied(decision, UIServerAccessDenialReason.InvalidForwardedProtocol) + }) + + await it('should reject empty X-Forwarded-Host comma-only lists from a trusted proxy', () => { + const decision = evaluate( + createAccessPolicyRequest({ + headers: { + host: 'gateway.example.com', + 'x-forwarded-for': '203.0.113.10', + 'x-forwarded-host': ',,,', + 'x-forwarded-proto': 'https', + }, + remoteAddress: '192.0.2.10', + }), + createGatewayConfigWithTrustedProxy() + ) + + expectDenied(decision, UIServerAccessDenialReason.InvalidForwardedHost) + }) + await it('should reject Forwarded headers with multiple comma-separated entries', () => { const decision = evaluate( createAccessPolicyRequest({ @@ -818,6 +851,21 @@ await describe('UIServerAccessPolicy', async () => { expectDenied(decision, UIServerAccessDenialReason.TlsRequired) }) + await it('should treat whitespace-only forwarded headers as absent', () => { + const decision = evaluate( + createAccessPolicyRequest({ + headers: { + host: 'localhost:8080', + 'x-forwarded-proto': ' ', + }, + }), + createAccessPolicyConfiguration() + ) + + assert.strictEqual(decision.allowed, true) + assert.strictEqual(decision.clientAddress, '127.0.0.1') + }) + await it('should accept empty forwarded headers from a loopback peer', () => { const decision = evaluate( createAccessPolicyRequest({ @@ -1047,6 +1095,31 @@ await describe('UIServerAccessPolicy', async () => { assert.strictEqual(decision.clientAddress, '2001:db8:0:0:0:0:0:1') }) + await it('should unescape RFC 7230 quoted-pair sequences in Forwarded parameter values', () => { + const decision = evaluate( + createAccessPolicyRequest({ + headers: { + forwarded: + 'for="203.0.113.10";host="\\g\\a\\t\\e\\w\\a\\y\\.\\e\\x\\a\\m\\p\\l\\e\\.\\c\\o\\m";proto=https', + host: 'gateway.example.com', + }, + remoteAddress: '192.0.2.10', + }), + createAccessPolicyConfiguration({ + accessPolicy: { + allowedHosts: ['gateway.example.com'], + allowedOrigins: [], + allowLoopbackProxy: false, + requireTlsForNonLoopback: true, + trustedProxies: ['192.0.2.10'], + }, + }) + ) + + assert.strictEqual(decision.allowed, true) + assert.strictEqual(decision.clientAddress, '203.0.113.10') + }) + await it('should still require TLS when a trusted proxy claims a loopback client', () => { const decision = evaluate( createAccessPolicyRequest({ diff --git a/tests/charging-station/ui-server/UIServerNet.test.ts b/tests/charging-station/ui-server/UIServerNet.test.ts index 33d6a647..10b6e42e 100644 --- a/tests/charging-station/ui-server/UIServerNet.test.ts +++ b/tests/charging-station/ui-server/UIServerNet.test.ts @@ -132,6 +132,7 @@ await describe('UIServerNet', async () => { await it('should drop a single trailing dot', () => { assert.strictEqual(normalizeHost('gateway.example.com.'), 'gateway.example.com') + assert.strictEqual(normalizeHost('localhost.:80'), 'localhost') }) await it('should lowercase the result', () => { diff --git a/tests/charging-station/ui-server/UIWebSocketServer.test.ts b/tests/charging-station/ui-server/UIWebSocketServer.test.ts index cccc895f..1700b1b4 100644 --- a/tests/charging-station/ui-server/UIWebSocketServer.test.ts +++ b/tests/charging-station/ui-server/UIWebSocketServer.test.ts @@ -11,7 +11,8 @@ import { afterEach, describe, it } from 'node:test' import type { UUIDv4 } from '../../../src/types/index.js' import { ProcedureName, ResponseStatus } from '../../../src/types/index.js' -import { standardCleanup } from '../../helpers/TestLifecycleHelpers.js' +import { logger } from '../../../src/utils/Logger.js' +import { createLoggerMocks, standardCleanup } from '../../helpers/TestLifecycleHelpers.js' import { TEST_UUID } from './UIServerTestConstants.js' import { createMockIncomingMessage, @@ -308,4 +309,30 @@ await describe('UIWebSocketServer', async () => { assert.match(response, /Connection: close/) assert.match(response, /Content-Length: 0/) }) + + await it('should attach an error listener before writing upgrade rejection', async t => { + const { errorMock } = createLoggerMocks(t, logger) + const config = createMockUIServerConfiguration({ + options: { host: 'localhost', port: 0 }, + }) + const server = new TestableUIWebSocketServer(config) + const socket = new MockUpgradeSocket() + + try { + server.start() + await server.waitUntilListening() + server.emitUpgrade( + createMockIncomingMessage({ + headers: { connection: 'keep-alive', host: 'localhost', upgrade: 'http/1.1' }, + socket: { encrypted: false, remoteAddress: '127.0.0.1' } as never, + }), + socket as unknown as Duplex + ) + + assert.doesNotThrow(() => socket.emit('error', new Error('connection reset'))) + assert.ok(errorMock.mock.calls.length >= 1) + } finally { + server.stop() + } + }) }) diff --git a/tests/utils/ConfigurationSchema.test.ts b/tests/utils/ConfigurationSchema.test.ts index c15e52bb..6c0a4096 100644 --- a/tests/utils/ConfigurationSchema.test.ts +++ b/tests/utils/ConfigurationSchema.test.ts @@ -248,6 +248,40 @@ await describe('ConfigurationSchema', async () => { assert.ok(result.error.issues.some(i => i.path.join('.').includes('uiServer.options'))) }) + await it('should reject uiServer options as null with object-shape message', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + options: null, + }, + }) + ) + assert.ok(!result.success) + assert.ok( + result.error.issues.some( + i => + i.path.join('.').includes('uiServer.options') && i.message.includes('non-array object') + ) + ) + }) + + await it('should reject uiServer options as array with object-shape message', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + options: [], + }, + }) + ) + assert.ok(!result.success) + assert.ok( + result.error.issues.some( + i => + i.path.join('.').includes('uiServer.options') && i.message.includes('non-array object') + ) + ) + }) + await it('should reject hostnames in trustedProxies', () => { const result = ConfigurationSchema.safeParse( buildMinimalConfiguration({ @@ -329,18 +363,38 @@ await describe('ConfigurationSchema', async () => { } }) + await it('should reject allowedHosts entries with a port', () => { + for (const portBearingHost of [ + 'gateway.example.com:8080', + '127.0.0.1:8080', + '127.0.0.1:08080', + '[::1]:8080', + ]) { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + accessPolicy: { + allowedHosts: [portBearingHost], + }, + enabled: true, + type: 'ws', + }, + }) + ) + assert.ok( + !result.success, + `Expected '${portBearingHost}' to be rejected as allowedHosts entry` + ) + assert.ok(result.error.issues.some(i => i.path.join('.').includes('allowedHosts'))) + } + }) + await it('should accept hostnames and IP literals in allowedHosts', () => { const result = ConfigurationSchema.safeParse( buildMinimalConfiguration({ uiServer: { accessPolicy: { - allowedHosts: [ - 'gateway.example.com', - '127.0.0.1', - '[::1]', - '[::1]:8080', - 'localhost', - ], + allowedHosts: ['gateway.example.com', '127.0.0.1', '[::1]', 'localhost'], }, enabled: true, type: 'ws',