From 4529c4816fb05819a7b0917d7ce1ce474a51e1cb Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 29 Jul 2026 18:16:05 -0600 Subject: [PATCH 01/12] Require explicit protocol: 'http' declaration for header-injection routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ALPN alone can't distinguish a native non-HTTP protocol (which negotiates no ALPN, e.g. MQTT) from an HTTPS client that simply offered none, so gating HTTP/1 header rewriting on `alpn_protocol() != Some(b"h2")` can feed a terminated MQTT connection to the HTTP/1 rewriter, which then hangs waiting for a `\r\n\r\n` that never arrives (issue #38). Add RouteConfig.protocol ('http' | 'opaque', default 'opaque') and gate header rewriting on it directly instead of on ALPN. A route that requests a header-injection mode (sourceAddressHeader: 'xForwardedFor', or forwardFingerprint under any mode other than 'proxyProtocolV2') without declaring protocol: 'http' is now a parse-time config error, not a route that silently stops injecting the header. The declaration and validation are threaded through both the static route table and resolveConnection(). This is a breaking change for a hand-written route already using sourceAddressHeader: 'xForwardedFor' (or a header-carried forwardFingerprint) without protocol: 'http' — README updated accordingly. Co-Authored-By: Claude Sonnet 5 --- README.md | 30 ++++- __test__/h2-dispatch.spec.ts | 2 + __test__/proxy-protocol-v2.spec.ts | 9 ++ __test__/route-protocol.spec.ts | 173 +++++++++++++++++++++++++++++ package.json | 2 +- src/proxy.rs | 135 +++++++++++++++++++++- src/proxy_conn.rs | 25 +++-- src/router.rs | 50 +++++++++ src/suspended.rs | 5 +- ts/addon.d.ts | 14 +++ ts/proxy.ts | 2 + ts/types.ts | 21 ++++ 12 files changed, 453 insertions(+), 15 deletions(-) create mode 100644 __test__/route-protocol.spec.ts diff --git a/README.md b/README.md index 0d2bb2d..211e10a 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,7 @@ console.log('proxy listening on :443'); | `http2` | `boolean` | `false` | Advertise `h2` in ALPN so clients negotiate HTTP/2. Raw H2 frames flow through to the upstream unchanged. Requires `terminateTls: true`. | | `sourceAddressHeader` | `'proxyProtocol' \| 'proxyProtocolV2' \| 'xForwardedFor' \| 'none'` | `'proxyProtocol'` for UDS, `'none'` for TCP | How the real client IP is forwarded to the upstream. See [Source address forwarding](#source-address-forwarding). | | `forwardFingerprint` | `'ja3' \| 'ja4' \| 'none'` | `'none'` | Forward the client TLS fingerprint downstream. See [Forwarding the fingerprint](#forwarding-the-fingerprint-downstream). | +| `protocol` | `'http' \| 'opaque'` | `'opaque'` | The route's application protocol. Required to be `'http'` before `sourceAddressHeader: 'xForwardedFor'` or a header-carried `forwardFingerprint` is accepted — see [Source address forwarding](#source-address-forwarding). | ### `Upstream` @@ -355,9 +356,28 @@ Use `sourceAddressHeader` on a route to control how the real client IP is commun |---|---| | `'proxyProtocol'` | Sends a PROXY protocol v1 (text) header (`PROXY TCP4 0\r\n`) before any application data. Default for UDS upstreams. | | `'proxyProtocolV2'` | Sends a PROXY protocol v2 (binary) header before any application data. v2 adds a TLV section — the carrier for `forwardFingerprint` below and for [mTLS client cert forwarding](#forwarding-mtls-client-certificates). Keep it opt-in: the consumer must speak v2 (nginx/HAProxy do; Harper core's UDS reader parses v1 only before Harper 5.2). | -| `'xForwardedFor'` | Reads the first chunk of the HTTP request, inserts an `X-Forwarded-For` header after the request line, then copies the rest verbatim. No per-request parsing overhead for keep-alive connections. Default for TCP upstreams (disabled). | +| `'xForwardedFor'` | Reads the first chunk of the HTTP request, inserts an `X-Forwarded-For` header after the request line, then copies the rest verbatim. No per-request parsing overhead for keep-alive connections. Default for TCP upstreams (disabled). Requires `protocol: 'http'` on the route (below). | | `'none'` | Does not forward source address information. Default for TCP upstreams. | +### Declaring the route protocol + +`sourceAddressHeader: 'xForwardedFor'`, and `forwardFingerprint` under any mode other than `'proxyProtocolV2'`, rewrite an HTTP/1 request — they need a route that says it actually carries HTTP. Set `protocol: 'http'` to opt in: + +```typescript +{ + sni: 'app.example.com', + upstreams: [{ kind: 'uds', path: '/run/app/worker.sock' }], + terminateTls: true, + cert: { certChain, privateKey }, + sourceAddressHeader: 'xForwardedFor', + protocol: 'http', // required — omitting this is a config error, not a silent no-op +} +``` + +`protocol` defaults to `'opaque'` — a route for a non-HTTP application protocol (MQTT, or any other raw TCP/TLS protocol), limited to the PROXY-protocol carriers, which work on any byte stream. The declaration exists because ALPN can't stand in for it: a native protocol that negotiates no ALPN (MQTT does not) is indistinguishable at the TLS layer from an HTTPS client that simply didn't offer one. A route that requests a header-injection mode without declaring `protocol: 'http'` fails at construction with a descriptive error — it never silently stops injecting the header, since a backend that silently sees the wrong (or no) client IP is worse than a config that fails to build. + +This is a breaking change for a hand-written route that already uses `sourceAddressHeader: 'xForwardedFor'` (or a header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration when upgrading. + ### PROXY protocol (default for UDS) Most backends that consume PROXY protocol (nginx, HAProxy, HarperDB) read the header once per connection before parsing application data. @@ -383,6 +403,7 @@ Bun's built-in HTTP server does not support PROXY protocol. Use `'xForwardedFor' terminateTls: true, cert: { certChain, privateKey }, sourceAddressHeader: 'xForwardedFor', + protocol: 'http', } ``` @@ -410,10 +431,10 @@ symphony computes the client's JA3/JA4 fingerprint from the ClientHello (the sam The **carrier depends on `sourceAddressHeader`**: -- With `'proxyProtocolV2'`, the fingerprint rides a PROXY v2 **TLV** — type `0xE0` for JA3, `0xE1` for JA4 (in HAProxy's `0xE0–0xEF` private range). This works even in passthrough (`terminateTls: false`), since the header prefixes the raw TLS bytes. -- Otherwise, symphony injects an **`X-JA3` / `X-JA4` HTTP header**. This requires a plaintext HTTP/1 upstream (`terminateTls: true` and not `http2`); it is skipped for passthrough or HTTP/2 upstreams (use `'proxyProtocolV2'` there). Any client-supplied `X-JA3`/`X-JA4` is stripped so the injected value is authoritative and can't be spoofed. +- With `'proxyProtocolV2'`, the fingerprint rides a PROXY v2 **TLV** — type `0xE0` for JA3, `0xE1` for JA4 (in HAProxy's `0xE0–0xEF` private range). This works even in passthrough (`terminateTls: false`), since the header prefixes the raw TLS bytes. No `protocol` declaration is needed — the TLV carries it regardless of the route's application protocol. +- Otherwise, symphony injects an **`X-JA3` / `X-JA4` HTTP header**. This requires `protocol: 'http'` on the route (see [Declaring the route protocol](#declaring-the-route-protocol)) and a plaintext HTTP/1 upstream (`terminateTls: true` and not `http2`); it is skipped for HTTP/2 upstreams (use `'proxyProtocolV2'` there). Any client-supplied `X-JA3`/`X-JA4` is stripped so the injected value is authoritative and can't be spoofed. -A config that requests `forwardFingerprint` with no viable carrier — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — logs a startup warning rather than silently dropping the signal. +A config that requests a header-carried `forwardFingerprint` without declaring `protocol: 'http'` fails at construction — the same fail-loud rule as `sourceAddressHeader: 'xForwardedFor'`. A config that requests `forwardFingerprint` with no viable carrier at all — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — logs a startup warning rather than silently dropping the signal. ```typescript // TLV carrier — works for any upstream that speaks PROXY v2, including passthrough @@ -434,6 +455,7 @@ A config that requests `forwardFingerprint` with no viable carrier — passthrou cert: { certChain, privateKey }, sourceAddressHeader: 'xForwardedFor', forwardFingerprint: 'ja3', // upstream reads X-JA3 alongside X-Forwarded-For + protocol: 'http', } ``` diff --git a/__test__/h2-dispatch.spec.ts b/__test__/h2-dispatch.spec.ts index b5fdede..c36e0bd 100644 --- a/__test__/h2-dispatch.spec.ts +++ b/__test__/h2-dispatch.spec.ts @@ -182,6 +182,7 @@ describe('SymphonyProxy – h2 upstream config validation', () => { terminateTls: true, http2: true, sourceAddressHeader: 'xForwardedFor', + protocol: 'http', cert: { certChain: cert.cert, privateKey: cert.key }, }, ], @@ -236,6 +237,7 @@ describe('SymphonyProxy – h2 upstream config validation', () => { terminateTls: true, http2: true, sourceAddressHeader: 'xForwardedFor', + protocol: 'http', cert: { certChain: cert.cert, privateKey: cert.key }, }, ], diff --git a/__test__/proxy-protocol-v2.spec.ts b/__test__/proxy-protocol-v2.spec.ts index b4df6dc..0267e0b 100644 --- a/__test__/proxy-protocol-v2.spec.ts +++ b/__test__/proxy-protocol-v2.spec.ts @@ -150,6 +150,7 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { cert: { certChain: cert.cert, privateKey: cert.key }, sourceAddressHeader: 'xForwardedFor', forwardFingerprint: 'ja3', + protocol: 'http', }, ], }); @@ -182,6 +183,10 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: upstream.port }], terminateTls: false, forwardFingerprint: 'ja3', + // protocol: 'http' declared even though passthrough can never actually inject a + // header (there's no decrypted HTTP request to rewrite) — the point of this test is + // that the carrier is a runtime no-op regardless of the declaration. + protocol: 'http', }, ], }); @@ -217,6 +222,7 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { cert: { certChain: cert.cert, privateKey: cert.key }, sourceAddressHeader: 'xForwardedFor', forwardFingerprint: 'ja3', + protocol: 'http', }, ], }); @@ -261,6 +267,7 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { cert: { certChain: cert.cert, privateKey: cert.key }, sourceAddressHeader: 'xForwardedFor', forwardFingerprint: 'ja3', + protocol: 'http', }, ], }); @@ -299,6 +306,7 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { cert: { certChain: cert.cert, privateKey: cert.key }, sourceAddressHeader: 'xForwardedFor', forwardFingerprint: 'ja3', + protocol: 'http', }, ], }); @@ -339,6 +347,7 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { http2: true, sourceAddressHeader: 'none', forwardFingerprint: 'ja3', + protocol: 'http', }, ], }); diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts new file mode 100644 index 0000000..5a6696a --- /dev/null +++ b/__test__/route-protocol.spec.ts @@ -0,0 +1,173 @@ +/** + * Integration tests for route-level protocol declaration (issue #38). + * + * Header rewriting (X-Forwarded-For / X-JA3 / X-JA4 injection) must be gated on an explicit + * `protocol: 'http'` declaration rather than inferred from ALPN — ALPN alone can't tell a + * native non-HTTP client (which negotiates no ALPN, e.g. MQTT) from an HTTPS client that + * simply offered none. An `'opaque'` (default) route must proxy any byte stream verbatim, + * without ever entering the HTTP/1 header rewriter. + * + * These tests require the native addon to be built: + * npm run build:debug + */ + +import assert from 'node:assert/strict'; +import * as tls from 'node:tls'; +import { after, before, describe, it } from 'node:test'; +import { SymphonyProxy } from '../ts/proxy.js'; +import { generateSelfSignedCert, getFreePort, startCaptureServer, startEchoServer, tlsRoundTrip, sleep } from './util.js'; + +/** Open a TLS connection through the proxy, send `data`, and resolve once written. */ +function tlsSend(port: number, servername: string, caCert: string, data: Buffer | string): Promise { + return new Promise((resolve, reject) => { + const socket = tls.connect({ port, host: '127.0.0.1', servername, ca: caCert, rejectUnauthorized: false }, () => { + socket.write(data, () => resolve(socket)); + }); + socket.on('error', reject); + }); +} + +describe('SymphonyProxy – route protocol declaration', () => { + const cert = generateSelfSignedCert('localhost'); + + // The regression this whole issue is about: a terminated non-HTTP route (MQTT over TLS is + // the motivating case) must proxy the decrypted byte stream verbatim and promptly. Under the + // old ALPN heuristic (`alpn_protocol() != Some(b"h2")`), a terminated MQTT connection — which + // negotiates no ALPN — was indistinguishable from an HTTP/1 client and could be fed to + // `proxy_http1_rewriting`, which waits for a `\r\n\r\n` that never arrives (a hang, not an + // error). An MQTT CONNECT packet is used as the payload: it starts with 0x10 (never an HTTP + // method token) and contains no CRLFCRLF anywhere. + it('an opaque route proxies a non-HTTP byte stream end-to-end without entering the header rewriter', async () => { + const upstream = await startEchoServer(); + const proxyPort = await getFreePort(); + // A short idle timeout: if the connection were mistakenly fed to the header rewriter, + // it would stall waiting for a request terminator and get dropped once this elapses. + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort, idleTimeoutMs: 2000 }], + routes: [ + { + sni: 'mqtt.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: upstream.port }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + // protocol left unset — defaults to 'opaque'. + }, + ], + }); + await proxy.start(); + await sleep(50); + + // MQTT v3.1.1 CONNECT packet (fixed header + variable header + payload), no HTTP framing. + const mqttConnect = Buffer.from([ + 0x10, 0x0c, 0x00, 0x04, 0x4d, 0x51, 0x54, 0x54, 0x04, 0x02, 0x00, 0x3c, 0x00, 0x00, + ]); + + const start = Date.now(); + const response = await tlsRoundTrip({ + port: proxyPort, + servername: 'mqtt.example.com', + caCert: cert.cert, + data: mqttConnect, + }); + const elapsedMs = Date.now() - start; + + assert.deepEqual( + response, + mqttConnect, + 'raw MQTT bytes proxied verbatim — no header injected, no HTTP parsing attempted' + ); + assert.ok( + elapsedMs < 1000, + `round-trip must complete promptly, not stall waiting for an HTTP header terminator (took ${elapsedMs}ms)` + ); + + await proxy.stop(); + await upstream.close(); + }); + + it('an explicitly opaque route with PROXY protocol forwards the source address and the raw bytes unrewritten', async () => { + const capture = await startCaptureServer(); + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'mqtt.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: capture.port }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + sourceAddressHeader: 'proxyProtocol', + protocol: 'opaque', + }, + ], + }); + await proxy.start(); + await sleep(50); + + const mqttConnect = Buffer.from([ + 0x10, 0x0c, 0x00, 0x04, 0x4d, 0x51, 0x54, 0x54, 0x04, 0x02, 0x00, 0x3c, 0x00, 0x00, + ]); + const socket = await tlsSend(proxyPort, 'mqtt.example.com', cert.cert, mqttConnect); + const received = await capture.received; + + assert.match(received.toString('latin1'), /^PROXY TCP4 127\.0\.0\.1 127\.0\.0\.1 \d+ \d+\r\n/, 'PROXY v1 header prefixes the stream'); + assert.ok(received.subarray(received.length - mqttConnect.length).equals(mqttConnect), 'raw MQTT bytes follow the header unmodified'); + + socket.destroy(); + await proxy.stop(); + await capture.close(); + }); + + it('a protocol: "http" route still injects X-Forwarded-For and strips a client-supplied one', async () => { + const capture = await startCaptureServer(); + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'app.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: capture.port }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + sourceAddressHeader: 'xForwardedFor', + protocol: 'http', + }, + ], + }); + await proxy.start(); + await sleep(50); + + const spoofed = 'GET / HTTP/1.1\r\nHost: app.example.com\r\nX-Forwarded-For: 9.9.9.9\r\n\r\n'; + const socket = await tlsSend(proxyPort, 'app.example.com', cert.cert, spoofed); + const text = (await capture.received).toString('ascii'); + + assert.match(text, /\r\nX-Forwarded-For: 127\.0\.0\.1\r\n/, 'authoritative X-Forwarded-For injected'); + assert.ok(!text.includes('9.9.9.9'), 'spoofed X-Forwarded-For stripped'); + assert.equal((text.match(/X-Forwarded-For:/gi) ?? []).length, 1, 'exactly one X-Forwarded-For header'); + + socket.destroy(); + await proxy.stop(); + await capture.close(); + }); + + it('rejects xForwardedFor without a protocol: "http" declaration at construction time (fail loud, not a silent no-op)', async () => { + assert.throws( + () => + new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: 0 }], + routes: [ + { + sni: 'mqtt.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: 1 }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + sourceAddressHeader: 'xForwardedFor', + // protocol left unset — defaults to 'opaque', must be rejected, not silently accepted. + }, + ], + }), + /protocol/i, + 'construction must throw a descriptive error, not silently build a route that never injects the header' + ); + }); +}); diff --git a/package.json b/package.json index c9bcd45..46f8c02 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "artifacts": "napi artifacts", "prepublishOnly": "napi prepublish -t npm", "version": "napi version", - "test": "tsc -p tsconfig.test.json && node --test --test-force-exit dist-test/__test__/protection.spec.js dist-test/__test__/proxy.spec.js dist-test/__test__/proxy-protocol-v2.spec.js dist-test/__test__/mtls.spec.js dist-test/__test__/suspended.spec.js dist-test/__test__/http-listener.spec.js dist-test/__test__/server.spec.js dist-test/__test__/h2-dispatch.spec.js dist-test/__test__/metrics.spec.js dist-test/__test__/copy-buffers.spec.js", + "test": "tsc -p tsconfig.test.json && node --test --test-force-exit dist-test/__test__/protection.spec.js dist-test/__test__/proxy.spec.js dist-test/__test__/proxy-protocol-v2.spec.js dist-test/__test__/mtls.spec.js dist-test/__test__/suspended.spec.js dist-test/__test__/http-listener.spec.js dist-test/__test__/server.spec.js dist-test/__test__/h2-dispatch.spec.js dist-test/__test__/metrics.spec.js dist-test/__test__/copy-buffers.spec.js dist-test/__test__/route-protocol.spec.js", "test:integration": "tsc -p tsconfig.test.json && node --test --test-force-exit dist-test/__test__/harper-integration.spec.js", "benchmark": "tsc -p tsconfig.test.json && node dist-test/__test__/benchmark.js", "benchmark:throughput": "tsc -p tsconfig.test.json && node dist-test/__test__/benchmark-throughput.js", diff --git a/src/proxy.rs b/src/proxy.rs index 81bd213..cefded1 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -6,8 +6,8 @@ use crate::proxy_conn::{ ConnContext, JsEvent, DEFAULT_COPY_BUFFER_SIZE, MAX_COPY_BUFFER_SIZE, MIN_COPY_BUFFER_SIZE, }; use crate::router::{ - build_route_table, ForwardFingerprint, ListenerTlsSpec, LiveRouteTable, RouteSpec, - SourceAddressMode, UpstreamSpec, + build_route_table, requires_http_protocol, ForwardFingerprint, ListenerTlsSpec, LiveRouteTable, + RouteProtocol, RouteSpec, SourceAddressMode, UpstreamSpec, }; use crate::suspended::{build_resolved_route, ResolveSpec, ResolveUpstream, SuspendedRegistry}; use ipnetwork::IpNetwork; @@ -75,6 +75,13 @@ pub struct JsRouteConfig { pub forward_fingerprint: Option, /// Advertise h2 in ALPN so clients can negotiate HTTP/2. Default: false. pub http2: Option, + /// The route's application protocol: `'http'` or `'opaque'` (non-HTTP, e.g. MQTT). + /// Required — as a parse-time error, not a silent no-op — whenever the route requests a + /// header-injection forwarding mode (`sourceAddressHeader: 'xForwardedFor'`, or + /// `forwardFingerprint` under any mode other than `'proxyProtocolV2'`): ALPN alone can't + /// tell a native non-HTTP protocol (which negotiates no ALPN) from an HTTPS client that + /// simply offered none, so the declaration must be explicit. Default: `'opaque'`. + pub protocol: Option, } #[napi(object)] @@ -218,6 +225,9 @@ pub struct JsResolveRoute { pub source_address_header: Option, pub forward_fingerprint: Option, pub http2: Option, + /// See `JsRouteConfig::protocol` — the same declaration, required under the same + /// conditions, for a route resolved via `resolveConnection()`. + pub protocol: Option, } // ── Plain Rust internal config (all Send + Sync) ────────────────────────────── @@ -736,6 +746,14 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { let has_uds = upstreams.iter().any(|u| matches!(u, UpstreamSpec::Uds { .. })); let source_address_mode = parse_source_address_mode(r.source_address_header.as_deref(), has_uds)?; let forward_fingerprint = parse_forward_fingerprint(r.forward_fingerprint.as_deref())?; + let protocol = parse_route_protocol(r.protocol.as_deref())?; + + if requires_http_protocol(source_address_mode, forward_fingerprint) && protocol != RouteProtocol::Http { + return Err(napi::Error::from_reason(format!( + "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch to 'proxyProtocol'/'proxyProtocolV2' which work on any protocol", + r.sni + ))); + } let spec = RouteSpec { sni: r.sni.clone(), @@ -752,6 +770,7 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { source_address_mode, forward_fingerprint, http2: r.http2.unwrap_or(false), + protocol, }; if spec.http2 && !spec.terminate_tls { @@ -832,6 +851,13 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { let has_uds = matches!(&upstream, ResolveUpstream::Uds { .. }); let source_address_mode = parse_source_address_mode(r.source_address_header.as_deref(), has_uds)?; let forward_fingerprint = parse_forward_fingerprint(r.forward_fingerprint.as_deref())?; + let protocol = parse_route_protocol(r.protocol.as_deref())?; + + if requires_http_protocol(source_address_mode, forward_fingerprint) && protocol != RouteProtocol::Http { + return Err(napi::Error::from_reason( + "resolveConnection: sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch to 'proxyProtocol'/'proxyProtocolV2' which work on any protocol".to_string(), + )); + } Ok(ResolveSpec { upstream, @@ -843,6 +869,7 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { source_address_mode, forward_fingerprint, http2: r.http2.unwrap_or(false), + protocol, }) } @@ -1011,6 +1038,16 @@ fn parse_forward_fingerprint(value: Option<&str>) -> Result } } +fn parse_route_protocol(value: Option<&str>) -> Result { + match value { + None | Some("opaque") => Ok(RouteProtocol::Opaque), + Some("http") => Ok(RouteProtocol::Http), + Some(other) => Err(napi::Error::from_reason(format!( + "unknown protocol value '{other}'; expected 'http' or 'opaque'" + ))), + } +} + fn num_cpus() -> u32 { std::thread::available_parallelism() .map(|n| n.get() as u32) @@ -1215,6 +1252,100 @@ mod tests { assert!(!is_valid_ja4("T13D1516H2_8DAAF6152771_02713D6AF862"), "uppercase"); } + fn base_route_config(sni: &str) -> JsRouteConfig { + JsRouteConfig { + sni: sni.to_string(), + upstreams: vec![JsUpstream { + kind: "tcp".to_string(), + host: Some("127.0.0.1".to_string()), + port: Some(8080), + path: None, + ip_affinity: None, + ip_affinity_ttl_ms: None, + pid: None, + tid: None, + protocol: None, + }], + terminate_tls: false, + cert: None, + mtls: None, + suspended: None, + suspend_timeout_ms: None, + max_connections_per_second: None, + burst: None, + source_address_header: None, + forward_fingerprint: None, + http2: None, + protocol: None, + } + } + + #[test] + fn xff_without_protocol_declaration_is_rejected() { + let mut r = base_route_config("mqtt.example.com"); + r.source_address_header = Some("xForwardedFor".to_string()); + let err = parse_route_spec(&r).expect_err("xForwardedFor without protocol: 'http' must error"); + assert!( + err.to_string().contains("protocol"), + "error message must mention the missing declaration: {err}" + ); + } + + #[test] + fn xff_with_http_protocol_declared_is_accepted() { + let mut r = base_route_config("app.example.com"); + r.source_address_header = Some("xForwardedFor".to_string()); + r.protocol = Some("http".to_string()); + assert!(parse_route_spec(&r).is_ok(), "xForwardedFor with protocol: 'http' must be accepted"); + } + + #[test] + fn opaque_route_with_proxy_protocol_is_accepted() { + let mut r = base_route_config("mqtt.example.com"); + r.source_address_header = Some("proxyProtocol".to_string()); + // protocol left unset (defaults to 'opaque') — PROXY protocol works on any byte stream. + assert!(parse_route_spec(&r).is_ok(), "opaque route with proxyProtocol must be accepted"); + } + + #[test] + fn opaque_route_requesting_xff_is_rejected() { + let mut r = base_route_config("mqtt.example.com"); + r.source_address_header = Some("xForwardedFor".to_string()); + r.protocol = Some("opaque".to_string()); + assert!( + parse_route_spec(&r).is_err(), + "an explicitly opaque route requesting xForwardedFor must still be rejected" + ); + } + + #[test] + fn header_carried_fingerprint_without_protocol_declaration_is_rejected() { + let mut r = base_route_config("app.example.com"); + r.forward_fingerprint = Some("ja3".to_string()); + // source_address_header left at 'none' — not proxyProtocolV2, so the fingerprint + // would ride an X-JA3 header and needs the declaration too. + assert!( + parse_route_spec(&r).is_err(), + "header-carried forwardFingerprint without protocol: 'http' must error" + ); + } + + #[test] + fn fingerprint_under_proxy_protocol_v2_needs_no_declaration() { + let mut r = base_route_config("mqtt.example.com"); + r.source_address_header = Some("proxyProtocolV2".to_string()); + r.forward_fingerprint = Some("ja4".to_string()); + // TLV carrier, not a header — no protocol declaration required. + assert!(parse_route_spec(&r).is_ok(), "forwardFingerprint under proxyProtocolV2 must not require protocol"); + } + + #[test] + fn unknown_protocol_value_is_rejected() { + let mut r = base_route_config("app.example.com"); + r.protocol = Some("mqtt".to_string()); + assert!(parse_route_spec(&r).is_err(), "an unrecognized protocol value must error"); + } + #[test] fn valid_ja4_rejects_transports_and_versions_symphony_cannot_emit() { // symphony only speaks TLS-over-TCP: 'q' (QUIC) and 'd' (DTLS) transport prefixes can diff --git a/src/proxy_conn.rs b/src/proxy_conn.rs index 2c36f32..d8abdaa 100644 --- a/src/proxy_conn.rs +++ b/src/proxy_conn.rs @@ -1,6 +1,6 @@ use crate::metrics::{BlockKind, CountingStream, ErrorKind, GlobalMetrics, ListenerMetrics}; use crate::protection::{IpState, ProtectionState}; -use crate::router::{Destination, ForwardFingerprint, LiveRouteTable, SourceAddressMode}; +use crate::router::{Destination, ForwardFingerprint, LiveRouteTable, RouteProtocol, SourceAddressMode}; use crate::sni; use crate::suspended::SuspendedRegistry; use crate::upstream::{self, UpstreamStream}; @@ -179,6 +179,7 @@ pub async fn handle(stream: TcpStream, peer_addr: SocketAddr, ctx: Arc &effective_route.destination, }; // Header injection (XFF / X-JA3) never touches an h2 stream: forward() - // gates it on the negotiated protocol (l7_http1), covering static and - // suspended-route configs alike. + // gates it on the route's declared protocol (RouteProtocol::Http) plus + // the negotiated ALPN, covering static and suspended-route configs alike. proxy_via_tls(tls_stream, destination, sf, &ctx).await } Ok(Err(e)) => { @@ -264,10 +267,15 @@ async fn proxy_via_tls( .then(|| collect_tls_forward(client.get_ref().1)); let sf = SourceForwarding { tls: tls_forward.as_ref(), ..sf }; - // HTTP-header injection is only valid for a plaintext HTTP/1 upstream. An h2-negotiated - // upstream receives binary frames, so text header insertion would corrupt them. - // Read before wrapping — the counter has no view of the TLS session. - let l7_http1 = client.get_ref().1.alpn_protocol() != Some(b"h2".as_ref()); + // HTTP-header injection is only valid for a plaintext HTTP/1 upstream: the route must have + // explicitly declared protocol: 'http' (issue #38 — ALPN alone can't tell a native + // non-HTTP protocol, which negotiates no ALPN, from an HTTPS client that simply offered + // none), and the connection must not have negotiated h2 (an h2-negotiated upstream + // receives binary frames, so text header insertion would corrupt them; this exclusion + // applies even on a protocol: 'http' route). Read before wrapping — the counter has no + // view of the TLS session. + let negotiated_h2 = client.get_ref().1.alpn_protocol() == Some(b"h2".as_ref()); + let l7_http1 = matches!(sf.protocol, RouteProtocol::Http) && !negotiated_h2; let mut upstream = upstream::connect(dest, Some(sf.peer_addr.ip()), ctx.upstream_connect_timeout) .await @@ -376,6 +384,8 @@ where struct SourceForwarding<'a> { mode: SourceAddressMode, fingerprint: ForwardFingerprint, + /// The route's declared application protocol — gates HTTP/1 header rewriting. + protocol: RouteProtocol, ja3: &'a str, ja4: &'a str, /// SNI from the ClientHello, forwarded as PP2_TYPE_AUTHORITY. @@ -550,6 +560,7 @@ struct EffectiveRoute { terminate_tls: bool, source_address_mode: SourceAddressMode, forward_fingerprint: ForwardFingerprint, + protocol: RouteProtocol, } struct ActiveGuard { diff --git a/src/router.rs b/src/router.rs index f9eccbf..cc6fb41 100644 --- a/src/router.rs +++ b/src/router.rs @@ -119,6 +119,34 @@ pub enum ForwardFingerprint { Ja4, } +// ── Route application protocol ──────────────────────────────────────────────── + +/// The application protocol of a route's byte stream, declared explicitly rather than +/// inferred from ALPN. ALPN alone cannot distinguish a native protocol that negotiates no +/// ALPN (e.g. MQTT) from an HTTPS client that simply offered none — both observe as `None` +/// (issue #38). Gates HTTP/1 header rewriting (`X-Forwarded-For` / `X-JA3` / `X-JA4` +/// injection): only a route that declares `Http` is ever fed to the header rewriter, and +/// only once TLS is terminated and h2 isn't negotiated (an h2 stream is excluded either way +/// — header injection would corrupt its binary frames). +#[derive(Clone, Copy, Debug, PartialEq)] +pub enum RouteProtocol { + /// Not HTTP: MQTT, a raw TCP protocol, or any application protocol symphony doesn't + /// parse. Source-address forwarding is limited to the PROXY-protocol carriers. + Opaque, + /// The decrypted byte stream is HTTP/1.x — eligible for header-based forwarding modes. + Http, +} + +/// True when a route's forwarding config would need to inject a plaintext HTTP header +/// (`X-Forwarded-For`, or a header-carried `X-JA3`/`X-JA4`) rather than a carrier that works +/// on any byte stream. These are exactly the modes that require `RouteProtocol::Http` to be +/// declared explicitly — `forwardFingerprint` under `proxyProtocolV2` is exempt, since it +/// rides a TLV, not a header. +pub fn requires_http_protocol(mode: SourceAddressMode, fingerprint: ForwardFingerprint) -> bool { + mode == SourceAddressMode::XForwardedFor + || (fingerprint != ForwardFingerprint::None && mode != SourceAddressMode::ProxyProtocolV2) +} + // ── Route destination ───────────────────────────────────────────────────────── #[derive(Clone)] @@ -146,6 +174,8 @@ pub struct Route { pub source_address_mode: SourceAddressMode, /// Which client TLS fingerprint (if any) is forwarded to the upstream. pub forward_fingerprint: ForwardFingerprint, + /// The route's declared application protocol — gates HTTP/1 header rewriting. + pub protocol: RouteProtocol, } // ── Route table ─────────────────────────────────────────────────────────────── @@ -258,6 +288,10 @@ pub struct RouteSpec { pub forward_fingerprint: ForwardFingerprint, /// Advertise h2 in ALPN so clients can negotiate HTTP/2. pub http2: bool, + /// The route's declared application protocol — gates HTTP/1 header rewriting. Validated + /// against `source_address_mode`/`forward_fingerprint` at parse time (`proxy.rs`), so by + /// the time a `RouteSpec` reaches `build_route` the combination is already known-valid. + pub protocol: RouteProtocol, } /// Listener-level fallback cert/mTLS spec. @@ -469,6 +503,7 @@ fn build_route( rate_limiter, source_address_mode: spec.source_address_mode, forward_fingerprint: spec.forward_fingerprint, + protocol: spec.protocol, }) } @@ -677,6 +712,7 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== source_address_mode: SourceAddressMode::None, forward_fingerprint: ForwardFingerprint::None, http2: false, + protocol: RouteProtocol::Opaque, } } @@ -756,4 +792,18 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== spec.source_address_mode = SourceAddressMode::None; assert!(!fingerprint_has_no_carrier(&spec)); } + + #[test] + fn header_injection_detection() { + // xForwardedFor always needs protocol: 'http', regardless of fingerprint. + assert!(requires_http_protocol(SourceAddressMode::XForwardedFor, ForwardFingerprint::None)); + // A header-carried fingerprint (any mode other than proxyProtocolV2) needs it too. + assert!(requires_http_protocol(SourceAddressMode::None, ForwardFingerprint::Ja3)); + assert!(requires_http_protocol(SourceAddressMode::ProxyProtocol, ForwardFingerprint::Ja4)); + // proxyProtocolV2 carries the fingerprint as a TLV — never needs the declaration. + assert!(!requires_http_protocol(SourceAddressMode::ProxyProtocolV2, ForwardFingerprint::Ja3)); + // No header-injection mode requested at all. + assert!(!requires_http_protocol(SourceAddressMode::None, ForwardFingerprint::None)); + assert!(!requires_http_protocol(SourceAddressMode::ProxyProtocol, ForwardFingerprint::None)); + } } diff --git a/src/suspended.rs b/src/suspended.rs index 9ec2de3..83f5712 100644 --- a/src/suspended.rs +++ b/src/suspended.rs @@ -1,5 +1,5 @@ use crate::balancer::{UdsBalancer, UdsSlotSpec}; -use crate::router::{Destination, ForwardFingerprint, SourceAddressMode}; +use crate::router::{Destination, ForwardFingerprint, RouteProtocol, SourceAddressMode}; use dashmap::DashMap; use rustls::ServerConfig; use std::sync::atomic::{AtomicU64, Ordering}; @@ -13,6 +13,7 @@ pub struct ResolvedRoute { pub terminate_tls: bool, pub source_address_mode: SourceAddressMode, pub forward_fingerprint: ForwardFingerprint, + pub protocol: RouteProtocol, } /// Registry of suspended connections waiting for `resolveConnection()`. @@ -72,6 +73,7 @@ pub struct ResolveSpec { pub source_address_mode: SourceAddressMode, pub forward_fingerprint: ForwardFingerprint, pub http2: bool, + pub protocol: RouteProtocol, } #[derive(Debug)] @@ -126,5 +128,6 @@ pub fn build_resolved_route(spec: &ResolveSpec) -> crate::error::Result any) diff --git a/ts/proxy.ts b/ts/proxy.ts index c9bd1dd..d059edf 100644 --- a/ts/proxy.ts +++ b/ts/proxy.ts @@ -105,6 +105,7 @@ function toJsRoute(r: RouteConfig): JsRouteConfig { sourceAddressHeader: r.sourceAddressHeader, forwardFingerprint: r.forwardFingerprint, http2: r.http2, + protocol: r.protocol, }; } @@ -286,6 +287,7 @@ export class SymphonyProxy extends EventEmitter { sourceAddressHeader: route.sourceAddressHeader, forwardFingerprint: route.forwardFingerprint, http2: route.http2, + protocol: route.protocol, }); } } diff --git a/ts/types.ts b/ts/types.ts index 2702b2a..e83bff0 100644 --- a/ts/types.ts +++ b/ts/types.ts @@ -126,6 +126,25 @@ export interface RouteConfig { * Requires `terminateTls: true`. Default: false. */ http2?: boolean; + /** + * The route's application protocol. + * + * - `'http'` — the byte stream (once TLS is terminated and h2 isn't negotiated) is + * HTTP/1.x. Required to use `sourceAddressHeader: 'xForwardedFor'`, or + * `forwardFingerprint` under any mode other than `'proxyProtocolV2'` — both rewrite + * an HTTP request, which is only safe on a route that says it carries one. + * - `'opaque'` (default) — not HTTP: MQTT, a raw TCP protocol, or anything else + * symphony doesn't parse. Source-address forwarding is limited to the + * `'proxyProtocol'` / `'proxyProtocolV2'` carriers, which work on any byte stream. + * + * ALPN cannot stand in for this declaration: a native non-HTTP client that offers no + * ALPN is indistinguishable from an HTTPS client that simply didn't offer one, so a + * route requesting a header-injection mode without declaring `protocol: 'http'` is a + * config error, not a route that quietly stops injecting the header. This is a breaking + * change for a hand-written route using `sourceAddressHeader: 'xForwardedFor'` (or a + * header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration. + */ + protocol?: 'http' | 'opaque'; } // ── Protection ──────────────────────────────────────────────────────────────── @@ -358,6 +377,8 @@ export interface ResolveRoute { forwardFingerprint?: 'ja3' | 'ja4' | 'none'; /** Advertise h2 in ALPN for this resolved connection. See RouteConfig.http2. */ http2?: boolean; + /** The resolved connection's application protocol. See RouteConfig.protocol. */ + protocol?: 'http' | 'opaque'; } // ── Event payloads ──────────────────────────────────────────────────────────── From b23f05ddafdb9bdba6af4ca561664d50402dc923 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 29 Jul 2026 22:04:29 -0600 Subject: [PATCH 02/12] Fix cargo test link failure: test pure protocol-gating logic, not JsRouteConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new unit tests added in f1f5dd4 constructed JsRouteConfig/JsUpstream values directly to exercise parse_route_spec's protocol validation. Those structs carry an Option (Either), and merely instantiating one — even with every field None — requires drop-glue for napi::bindgen_prelude::Buffer, which pulls in raw napi_* C-ABI symbols. Those symbols are only ever provided by the Node.js host process that dlopen()s this cdylib; a standalone `cargo test` binary is a real executable with no such host, so the link fails deterministically (reproduced locally; bisected to the exact test literal, confirmed absent on 2158979). This is why "Cargo test" and the macOS Node test job broke: the macOS job runs `cargo build` for the addon (unaffected) but the same crate is also linked for `cargo test` via the workspace toolchain, exercising the identical failure. No prior test in this crate had ever constructed one of these #[napi(object)] structs from a #[cfg(test)] fn — this PR's tests were the first, and any future test doing the same would hit the same wall. Rewrite the 7 new tests to exercise router::requires_http_protocol() and parse_route_protocol() directly — the actual pure logic under test — instead of round-tripping through the napi-facing parse_route_spec(&JsRouteConfig). Coverage of the full parse path, including the thrown error message, already exists end-to-end in __test__/route-protocol.spec.ts, which runs against the built native addon where the Node host supplies these symbols. Co-Authored-By: Claude Sonnet 5 --- src/proxy.rs | 103 ++++++++++++++++----------------------------------- 1 file changed, 32 insertions(+), 71 deletions(-) diff --git a/src/proxy.rs b/src/proxy.rs index cefded1..1f1f98a 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -1252,98 +1252,59 @@ mod tests { assert!(!is_valid_ja4("T13D1516H2_8DAAF6152771_02713D6AF862"), "uppercase"); } - fn base_route_config(sni: &str) -> JsRouteConfig { - JsRouteConfig { - sni: sni.to_string(), - upstreams: vec![JsUpstream { - kind: "tcp".to_string(), - host: Some("127.0.0.1".to_string()), - port: Some(8080), - path: None, - ip_affinity: None, - ip_affinity_ttl_ms: None, - pid: None, - tid: None, - protocol: None, - }], - terminate_tls: false, - cert: None, - mtls: None, - suspended: None, - suspend_timeout_ms: None, - max_connections_per_second: None, - burst: None, - source_address_header: None, - forward_fingerprint: None, - http2: None, - protocol: None, - } + // These test the pure `router::requires_http_protocol` / `parse_route_protocol` logic + // directly rather than round-tripping through `parse_route_spec(&JsRouteConfig)`. + // `JsRouteConfig`/`JsUpstream` carry an `Option` (`Either`), + // and instantiating one — even with every field `None` — requires drop-glue for + // `napi::bindgen_prelude::Buffer`, which pulls in raw `napi_*` C-ABI symbols that only + // the Node.js host process provides. A standalone `cargo test` binary is a real + // executable with no such host, so linking it fails; end-to-end coverage of the full + // `JsRouteConfig` parse path (including this error message) lives in + // `__test__/route-protocol.spec.ts`, which runs against the built native addon. + + #[test] + fn xff_requires_http_protocol_declaration() { + assert!(requires_http_protocol(SourceAddressMode::XForwardedFor, ForwardFingerprint::None)); } #[test] - fn xff_without_protocol_declaration_is_rejected() { - let mut r = base_route_config("mqtt.example.com"); - r.source_address_header = Some("xForwardedFor".to_string()); - let err = parse_route_spec(&r).expect_err("xForwardedFor without protocol: 'http' must error"); - assert!( - err.to_string().contains("protocol"), - "error message must mention the missing declaration: {err}" - ); + fn xff_requires_declaration_regardless_of_fingerprint() { + assert!(requires_http_protocol(SourceAddressMode::XForwardedFor, ForwardFingerprint::Ja4)); } #[test] - fn xff_with_http_protocol_declared_is_accepted() { - let mut r = base_route_config("app.example.com"); - r.source_address_header = Some("xForwardedFor".to_string()); - r.protocol = Some("http".to_string()); - assert!(parse_route_spec(&r).is_ok(), "xForwardedFor with protocol: 'http' must be accepted"); + fn header_carried_fingerprint_requires_http_protocol_declaration() { + // source_address_mode is 'none' — not proxyProtocolV2, so the fingerprint would ride + // an X-JA3 header and needs the declaration too. + assert!(requires_http_protocol(SourceAddressMode::None, ForwardFingerprint::Ja3)); } #[test] - fn opaque_route_with_proxy_protocol_is_accepted() { - let mut r = base_route_config("mqtt.example.com"); - r.source_address_header = Some("proxyProtocol".to_string()); - // protocol left unset (defaults to 'opaque') — PROXY protocol works on any byte stream. - assert!(parse_route_spec(&r).is_ok(), "opaque route with proxyProtocol must be accepted"); + fn fingerprint_under_proxy_protocol_v2_needs_no_declaration() { + // TLV carrier, not a header — no protocol declaration required. + assert!(!requires_http_protocol(SourceAddressMode::ProxyProtocolV2, ForwardFingerprint::Ja4)); } #[test] - fn opaque_route_requesting_xff_is_rejected() { - let mut r = base_route_config("mqtt.example.com"); - r.source_address_header = Some("xForwardedFor".to_string()); - r.protocol = Some("opaque".to_string()); - assert!( - parse_route_spec(&r).is_err(), - "an explicitly opaque route requesting xForwardedFor must still be rejected" - ); + fn proxy_protocol_without_fingerprint_needs_no_declaration() { + assert!(!requires_http_protocol(SourceAddressMode::ProxyProtocol, ForwardFingerprint::None)); } #[test] - fn header_carried_fingerprint_without_protocol_declaration_is_rejected() { - let mut r = base_route_config("app.example.com"); - r.forward_fingerprint = Some("ja3".to_string()); - // source_address_header left at 'none' — not proxyProtocolV2, so the fingerprint - // would ride an X-JA3 header and needs the declaration too. - assert!( - parse_route_spec(&r).is_err(), - "header-carried forwardFingerprint without protocol: 'http' must error" - ); + fn parse_route_protocol_defaults_to_opaque() { + assert_eq!(parse_route_protocol(None).unwrap(), RouteProtocol::Opaque); + assert_eq!(parse_route_protocol(Some("opaque")).unwrap(), RouteProtocol::Opaque); } #[test] - fn fingerprint_under_proxy_protocol_v2_needs_no_declaration() { - let mut r = base_route_config("mqtt.example.com"); - r.source_address_header = Some("proxyProtocolV2".to_string()); - r.forward_fingerprint = Some("ja4".to_string()); - // TLV carrier, not a header — no protocol declaration required. - assert!(parse_route_spec(&r).is_ok(), "forwardFingerprint under proxyProtocolV2 must not require protocol"); + fn parse_route_protocol_accepts_http() { + assert_eq!(parse_route_protocol(Some("http")).unwrap(), RouteProtocol::Http); } #[test] - fn unknown_protocol_value_is_rejected() { - let mut r = base_route_config("app.example.com"); - r.protocol = Some("mqtt".to_string()); - assert!(parse_route_spec(&r).is_err(), "an unrecognized protocol value must error"); + fn parse_route_protocol_rejects_unknown_value() { + let err = parse_route_protocol(Some("mqtt")).expect_err("an unrecognized protocol value must error"); + assert!(err.to_string().contains("mqtt"), "error message must mention the offending value: {err}"); } #[test] From 0ded9b19c38b1826ba720aa7097951e6368c952f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 09:38:19 -0600 Subject: [PATCH 03/12] Address PR #40 review: split no-carrier from protocol-declaration check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Separate the two failure modes gemini/kriszyp/Devin-Holland flagged as conflated: a passthrough route (terminateTls=false) with a header-carried forwardFingerprint/xForwardedFor has no possible carrier at all — no `protocol` declaration can fix that — so it now fails construction with a distinct "no carrier" error instead of being steered toward the semantically-wrong `protocol: 'http'`. The declaration-required error is now reached only when a header could actually be emitted. Also: - RouteProtocol derives Eq (gemini); the proxy_conn.rs runtime gate is factored into `eligible_for_header_rewriting`, using `==` instead of `matches!` (gemini) and unit-tested directly (kriszyp) instead of only through the MQTT smoke test, which is relabeled since it never exercised the gate (no XFF/fingerprint configured on that route). - Extend the http2 header-injection warning to also cover xForwardedFor, not just forwardFingerprint (Devin-Holland) — the terminateTls=false arm of that warning is now dead code given the new hard error, so it's simplified to the http2-only (soft, since ALPN negotiation is per-connection) case. - README: document the no-carrier vs declaration-required split. - __test__/proxy-protocol-v2.spec.ts: the passthrough-fingerprint test exercised exactly this gap (protocol: 'http' + terminateTls: false) and asserted the old silent-no-op behavior; updated to assert the new fail-loud rejection. Co-Authored-By: Claude Sonnet 5 --- README.md | 4 +- __test__/proxy-protocol-v2.spec.ts | 68 +++++++++++------------------- __test__/route-protocol.spec.ts | 53 +++++++++++++++++++---- src/proxy.rs | 43 ++++++++++++++----- src/proxy_conn.rs | 31 +++++++++++++- src/router.rs | 2 +- 6 files changed, 137 insertions(+), 64 deletions(-) diff --git a/README.md b/README.md index 211e10a..e60c2f8 100644 --- a/README.md +++ b/README.md @@ -376,6 +376,8 @@ Use `sourceAddressHeader` on a route to control how the real client IP is commun `protocol` defaults to `'opaque'` — a route for a non-HTTP application protocol (MQTT, or any other raw TCP/TLS protocol), limited to the PROXY-protocol carriers, which work on any byte stream. The declaration exists because ALPN can't stand in for it: a native protocol that negotiates no ALPN (MQTT does not) is indistinguishable at the TLS layer from an HTTPS client that simply didn't offer one. A route that requests a header-injection mode without declaring `protocol: 'http'` fails at construction with a descriptive error — it never silently stops injecting the header, since a backend that silently sees the wrong (or no) client IP is worse than a config that fails to build. +This declaration only helps when a header could actually be injected in the first place. A **passthrough** route (`terminateTls: false`) never decrypts the stream, so a header-carried mode has no carrier at all regardless of `protocol` — declaring `'http'` on a passthrough route wouldn't make it work, and fails construction with a distinct "no carrier" error instead of steering you toward a declaration that can't help. Use `sourceAddressHeader: 'proxyProtocolV2'` there instead (it works on any byte stream, passthrough included). + This is a breaking change for a hand-written route that already uses `sourceAddressHeader: 'xForwardedFor'` (or a header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration when upgrading. ### PROXY protocol (default for UDS) @@ -434,7 +436,7 @@ The **carrier depends on `sourceAddressHeader`**: - With `'proxyProtocolV2'`, the fingerprint rides a PROXY v2 **TLV** — type `0xE0` for JA3, `0xE1` for JA4 (in HAProxy's `0xE0–0xEF` private range). This works even in passthrough (`terminateTls: false`), since the header prefixes the raw TLS bytes. No `protocol` declaration is needed — the TLV carries it regardless of the route's application protocol. - Otherwise, symphony injects an **`X-JA3` / `X-JA4` HTTP header**. This requires `protocol: 'http'` on the route (see [Declaring the route protocol](#declaring-the-route-protocol)) and a plaintext HTTP/1 upstream (`terminateTls: true` and not `http2`); it is skipped for HTTP/2 upstreams (use `'proxyProtocolV2'` there). Any client-supplied `X-JA3`/`X-JA4` is stripped so the injected value is authoritative and can't be spoofed. -A config that requests a header-carried `forwardFingerprint` without declaring `protocol: 'http'` fails at construction — the same fail-loud rule as `sourceAddressHeader: 'xForwardedFor'`. A config that requests `forwardFingerprint` with no viable carrier at all — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — logs a startup warning rather than silently dropping the signal. +A config that requests a header-carried `forwardFingerprint` without declaring `protocol: 'http'` fails at construction — the same fail-loud rule as `sourceAddressHeader: 'xForwardedFor'`. A config that requests `forwardFingerprint` with no viable carrier at all — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — also fails at construction, with a distinct "no carrier" error: no `protocol` declaration could fix a passthrough route's inability to inject a header, so it isn't steered toward one. A route that could have carried the header but silently won't for some connections — `http2: true`, where ALPN negotiation is per-connection and some clients may still land on HTTP/1 — logs a startup warning instead, since that outcome isn't guaranteed. ```typescript // TLV carrier — works for any upstream that speaks PROXY v2, including passthrough diff --git a/__test__/proxy-protocol-v2.spec.ts b/__test__/proxy-protocol-v2.spec.ts index 0267e0b..2a32c34 100644 --- a/__test__/proxy-protocol-v2.spec.ts +++ b/__test__/proxy-protocol-v2.spec.ts @@ -10,14 +10,7 @@ import assert from 'node:assert/strict'; import * as tls from 'node:tls'; import { after, before, describe, it } from 'node:test'; import { SymphonyProxy } from '../ts/proxy.js'; -import { - generateSelfSignedCert, - getFreePort, - startCaptureServer, - startTlsEchoServer, - tlsRoundTrip, - sleep, -} from './util.js'; +import { generateSelfSignedCert, getFreePort, startCaptureServer, sleep } from './util.js'; const PROXY_V2_SIGNATURE = Buffer.from([0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49, 0x54, 0x0a]); const PP2_TYPE_JA3 = 0xe0; @@ -169,41 +162,30 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { await capture.close(); }); - // Passthrough forwards raw TLS bytes to a TLS upstream. A header carrier must be a no-op here: - // splicing X-JA3 into the ClientHello ciphertext would break the upstream handshake. A working - // end-to-end round-trip proves nothing was injected. - it('does not inject a fingerprint header in passthrough mode', async () => { - const upstream = await startTlsEchoServer(cert.cert, cert.key); - const proxyPort = await getFreePort(); - const proxy = new SymphonyProxy({ - listeners: [{ host: '127.0.0.1', port: proxyPort }], - routes: [ - { - sni: 'localhost', - upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: upstream.port }], - terminateTls: false, - forwardFingerprint: 'ja3', - // protocol: 'http' declared even though passthrough can never actually inject a - // header (there's no decrypted HTTP request to rewrite) — the point of this test is - // that the carrier is a runtime no-op regardless of the declaration. - protocol: 'http', - }, - ], - }); - await proxy.start(); - await sleep(50); - - const payload = Buffer.from('passthrough-ok'); - const response = await tlsRoundTrip({ - port: proxyPort, - servername: 'localhost', - caCert: cert.cert, - data: payload, - }); - assert.deepEqual(response, payload, 'end-to-end TLS round-trip intact (no injected header)'); - - await proxy.stop(); - await upstream.close(); + // Passthrough forwards raw TLS bytes to a TLS upstream — there's no decrypted HTTP request to + // splice a header into, and a header-carried fingerprint mode has no carrier at all here + // regardless of `protocol`. This used to build successfully and silently forward nothing; + // it now fails construction with a "no carrier" error instead (see route-protocol.spec.ts for + // focused coverage), so a passthrough + header-carried forwardFingerprint config can no longer + // look "working" while quietly forwarding no fingerprint. + it('rejects a header-carried fingerprint on a passthrough route at construction (no carrier, not a silent no-op)', async () => { + assert.throws( + () => + new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: 0 }], + routes: [ + { + sni: 'localhost', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: 1 }], + terminateTls: false, + forwardFingerprint: 'ja3', + protocol: 'http', + }, + ], + }), + /no carrier/i, + 'passthrough + header-carried forwardFingerprint must fail construction, not silently drop the fingerprint at runtime' + ); }); // Finding 1 (Critical — Slowloris): a client that completes the TLS handshake and then stalls diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index 5a6696a..6233c46 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -30,14 +30,16 @@ function tlsSend(port: number, servername: string, caCert: string, data: Buffer describe('SymphonyProxy – route protocol declaration', () => { const cert = generateSelfSignedCert('localhost'); - // The regression this whole issue is about: a terminated non-HTTP route (MQTT over TLS is - // the motivating case) must proxy the decrypted byte stream verbatim and promptly. Under the - // old ALPN heuristic (`alpn_protocol() != Some(b"h2")`), a terminated MQTT connection — which - // negotiates no ALPN — was indistinguishable from an HTTP/1 client and could be fed to - // `proxy_http1_rewriting`, which waits for a `\r\n\r\n` that never arrives (a hang, not an - // error). An MQTT CONNECT packet is used as the payload: it starts with 0x10 (never an HTTP - // method token) and contains no CRLFCRLF anywhere. - it('an opaque route proxies a non-HTTP byte stream end-to-end without entering the header rewriter', async () => { + // Opaque-route smoke test, not a regression test for the fix itself: this route has neither + // `sourceAddressHeader` nor `forwardFingerprint` configured, so `header_rewrites()` returns + // empty regardless of the `RouteProtocol::Http && !negotiated_h2` gate — it would pass + // against the pre-fix ALPN heuristic too. It still earns its keep as an end-to-end sanity + // check that a terminated non-HTTP byte stream (MQTT over TLS is the motivating case, issue + // #38) round-trips promptly through `copy_bidirectional` with no header framing assumed. + // Regression coverage for the runtime gate itself — `eligible_for_header_rewriting` — lives + // in `src/proxy_conn.rs`'s unit tests (`opaque_protocol_is_never_eligible_regardless_of_alpn`, + // `http_protocol_with_negotiated_h2_is_not_eligible`). + it('smoke test: an opaque route proxies a non-HTTP byte stream end-to-end without hanging on HTTP framing', async () => { const upstream = await startEchoServer(); const proxyPort = await getFreePort(); // A short idle timeout: if the connection were mistakenly fed to the header rewriter, @@ -170,4 +172,39 @@ describe('SymphonyProxy – route protocol declaration', () => { 'construction must throw a descriptive error, not silently build a route that never injects the header' ); }); + + // A passthrough route (terminateTls: false, e.g. an MQTT-over-TLS route) never decrypts the + // stream, so a header-carried forwardFingerprint mode has no carrier at all — no `protocol` + // declaration can fix that. This must be rejected as a distinct "no carrier" error, not + // steered toward `protocol: 'http'` (which is both semantically wrong for an opaque + // passthrough route and, since header injection genuinely can't happen without termination, + // would still not make the config work). + it('rejects forwardFingerprint on a passthrough route as having no carrier, regardless of protocol declaration', async () => { + const baseRoute = { + sni: 'mqtt.example.com', + upstreams: [{ kind: 'tcp' as const, host: '127.0.0.1', port: 1 }], + terminateTls: false, + forwardFingerprint: 'ja3' as const, + }; + + assert.throws( + () => + new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: 0 }], + routes: [baseRoute], + }), + /no carrier/i, + 'passthrough + header-carried forwardFingerprint must fail construction with a "no carrier" error' + ); + + assert.throws( + () => + new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: 0 }], + routes: [{ ...baseRoute, protocol: 'http' }], + }), + /no carrier/i, + 'declaring protocol: "http" must not paper over a passthrough route with no header carrier' + ); + }); }); diff --git a/src/proxy.rs b/src/proxy.rs index 1f1f98a..2bf230d 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -747,8 +747,20 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { let source_address_mode = parse_source_address_mode(r.source_address_header.as_deref(), has_uds)?; let forward_fingerprint = parse_forward_fingerprint(r.forward_fingerprint.as_deref())?; let protocol = parse_route_protocol(r.protocol.as_deref())?; + let requires_http = requires_http_protocol(source_address_mode, forward_fingerprint); - if requires_http_protocol(source_address_mode, forward_fingerprint) && protocol != RouteProtocol::Http { + // Passthrough (terminateTls=false) never decrypts the stream, so a header-carried mode has + // no carrier at all regardless of `protocol` — declaring 'http' wouldn't help. This is + // distinct from (and checked before) the declaration requirement below: it's not that the + // route mislabeled its protocol, it's that no protocol declaration could make this work. + if requires_http && !r.terminate_tls { + return Err(napi::Error::from_reason(format!( + "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), has no carrier when terminateTls=false (passthrough never decrypts the stream, so no header can be injected) — use sourceAddressHeader='proxyProtocolV2' (carries both source address and fingerprint), or remove forwardFingerprint", + r.sni + ))); + } + + if requires_http && protocol != RouteProtocol::Http { return Err(napi::Error::from_reason(format!( "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch to 'proxyProtocol'/'proxyProtocolV2' which work on any protocol", r.sni @@ -776,15 +788,17 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { if spec.http2 && !spec.terminate_tls { eprintln!("symphony: route '{}': http2=true has no effect when terminateTls=false (passthrough mode)", spec.sni); } - // An injected X-JA3/X-JA4 header needs a plaintext HTTP/1 upstream (terminated, not h2); the - // runtime skips it otherwise. The PROXY v2 TLV carrier works everywhere (it prefixes the raw - // bytes), so steer non-HTTP/1 routes to it. - if forward_fingerprint != ForwardFingerprint::None - && source_address_mode != SourceAddressMode::ProxyProtocolV2 - && (!spec.terminate_tls || spec.http2) - { + // A header-carried mode (xForwardedFor, or forwardFingerprint outside proxyProtocolV2) needs + // a plaintext HTTP/1 upstream — terminated and not h2 — or the runtime silently skips the + // rewriter (`eligible_for_header_rewriting` in proxy_conn.rs) and, for xForwardedFor, a + // client-supplied header reaches the upstream unstripped. The passthrough case is already a + // hard error above, so `requires_http` reaching here implies terminate_tls; the only + // remaining silent-miss case is http2, which is a soft warning rather than a hard error + // because ALPN negotiation is per-connection — declaring http2=true doesn't guarantee every + // client actually negotiates h2. + if requires_http && spec.http2 { eprintln!( - "symphony: route '{}': forwardFingerprint via HTTP header has no effect on a non-HTTP/1 upstream (terminateTls=false or http2=true); use sourceAddressHeader='proxyProtocolV2'", + "symphony: route '{}': header-injection forwarding (xForwardedFor / a header-carried forwardFingerprint) has no effect for any client that negotiates h2 (http2=true), and a client-supplied X-Forwarded-For reaches the upstream unstripped in that case; consider sourceAddressHeader='proxyProtocolV2'", spec.sni ); } @@ -852,8 +866,17 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { let source_address_mode = parse_source_address_mode(r.source_address_header.as_deref(), has_uds)?; let forward_fingerprint = parse_forward_fingerprint(r.forward_fingerprint.as_deref())?; let protocol = parse_route_protocol(r.protocol.as_deref())?; + let requires_http = requires_http_protocol(source_address_mode, forward_fingerprint); + + // See parse_route_spec: passthrough has no carrier for a header-based mode regardless of + // `protocol`, so it's rejected before (and distinctly from) the declaration check below. + if requires_http && !r.terminate_tls { + return Err(napi::Error::from_reason( + "resolveConnection: sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), has no carrier when terminateTls=false (passthrough never decrypts the stream, so no header can be injected) — use sourceAddressHeader='proxyProtocolV2' (carries both source address and fingerprint), or remove forwardFingerprint".to_string(), + )); + } - if requires_http_protocol(source_address_mode, forward_fingerprint) && protocol != RouteProtocol::Http { + if requires_http && protocol != RouteProtocol::Http { return Err(napi::Error::from_reason( "resolveConnection: sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch to 'proxyProtocol'/'proxyProtocolV2' which work on any protocol".to_string(), )); diff --git a/src/proxy_conn.rs b/src/proxy_conn.rs index d8abdaa..4044c2e 100644 --- a/src/proxy_conn.rs +++ b/src/proxy_conn.rs @@ -275,7 +275,7 @@ async fn proxy_via_tls( // applies even on a protocol: 'http' route). Read before wrapping — the counter has no // view of the TLS session. let negotiated_h2 = client.get_ref().1.alpn_protocol() == Some(b"h2".as_ref()); - let l7_http1 = matches!(sf.protocol, RouteProtocol::Http) && !negotiated_h2; + let l7_http1 = eligible_for_header_rewriting(sf.protocol, negotiated_h2); let mut upstream = upstream::connect(dest, Some(sf.peer_addr.ip()), ctx.upstream_connect_timeout) .await @@ -525,6 +525,15 @@ where /// authoritative value to substitute (`value: None`) — a client must never smuggle its own /// `X-JA3`/`X-JA4`/`X-Forwarded-For` through precisely when we can't replace it. PROXY v2 carries /// the fingerprint in a TLV, so it adds no header rewrite. +/// Whether header-based forwarding (XFF / X-JA3 / X-JA4) is eligible for this connection: the +/// route must declare `protocol: 'http'` (issue #38 — ALPN alone can't tell a native non-HTTP +/// protocol from an HTTPS client that simply negotiated no ALPN) and the connection must not +/// have negotiated h2 (an h2 stream is binary-framed; text header insertion would corrupt it, +/// so this exclusion applies even on a `protocol: 'http'` route). +fn eligible_for_header_rewriting(protocol: RouteProtocol, negotiated_h2: bool) -> bool { + protocol == RouteProtocol::Http && !negotiated_h2 +} + fn header_rewrites(sf: &SourceForwarding<'_>, l7_http1: bool) -> Vec { use crate::http_proxy::HeaderRewrite; if !l7_http1 { @@ -668,4 +677,24 @@ mod tests { assert_eq!(from_client, 4096); assert_eq!(from_upstream, 1024); } + + // Direct coverage of the runtime gate itself, independent of the construction-time + // `requires_http_protocol` guard in `proxy.rs` — this is the predicate `proxy_via_tls` + // evaluates per connection to decide whether the HTTP/1 header rewriter runs at all. + + #[test] + fn http_protocol_without_h2_is_eligible() { + assert!(eligible_for_header_rewriting(RouteProtocol::Http, false)); + } + + #[test] + fn http_protocol_with_negotiated_h2_is_not_eligible() { + assert!(!eligible_for_header_rewriting(RouteProtocol::Http, true)); + } + + #[test] + fn opaque_protocol_is_never_eligible_regardless_of_alpn() { + assert!(!eligible_for_header_rewriting(RouteProtocol::Opaque, false)); + assert!(!eligible_for_header_rewriting(RouteProtocol::Opaque, true)); + } } diff --git a/src/router.rs b/src/router.rs index cc6fb41..36fa310 100644 --- a/src/router.rs +++ b/src/router.rs @@ -128,7 +128,7 @@ pub enum ForwardFingerprint { /// injection): only a route that declares `Http` is ever fed to the header rewriter, and /// only once TLS is terminated and h2 isn't negotiated (an h2 stream is excluded either way /// — header injection would corrupt its binary frames). -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum RouteProtocol { /// Not HTTP: MQTT, a raw TCP protocol, or any application protocol symphony doesn't /// parse. Source-address forwarding is limited to the PROXY-protocol carriers. From d218efb735caf448ec7daaa0b45cfbe957ffdda7 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 10:13:26 -0600 Subject: [PATCH 04/12] Close resolveConnection XFF+h2 gap; fix README/test findings from pre-push review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review (codex + grok + harper-domain) on the prior commit surfaced: - resolveConnection() lacked the XFF+http2 hard reject that build_route already enforces for the static route table (router.rs:453) — an h2-negotiated connection on a resolved terminateTls+http2 route disabled the HTTP/1 rewriter, so a client-supplied X-Forwarded-For reached the upstream neither injected nor stripped. Pre-existing gap (not introduced by the protocol checks added earlier in this PR), but this file already touches exactly this validation; close it for symmetry with the static path. Added resolveConnection() unit tests for all three checks (declaration, no carrier, XFF+h2), since parse_resolve_spec validates independently of the suspended-connection id and needs no real suspended connection to exercise. - README.md: two factually-wrong claims caught by both outside lenses — xForwardedFor does NOT avoid per-request parsing on keep-alive connections (every request is parsed/rewritten, not just the first); the "stripped so it can't be spoofed" guarantee for X-JA3/X-JA4 does not hold on an h2-negotiated http2:true route (injection and stripping are both skipped there). - __test__/route-protocol.spec.ts: dropped the flaky elapsedMs<1000 wall-clock assertion in the opaque-MQTT smoke test — the awaited deepEqual already proves the rewriter wasn't entered (the bug path would idle-timeout the await), so the timing assert added nothing but CI flakiness under load. Declined (recorded in the dispatch file's Risks & open questions instead): moving the protocol/no-carrier checks from parse_route_spec's fail-fast path into build_route's per-route-isolated path, as suggested by the review. That would make an undeclared/no-carrier route silently drop from the table like a bad cert instead of failing construction — which directly reverses the explicit "fail loud, never silently downgrade" requirement this PR was written to satisfy (per Kris's design-decision comment on issue #38). Co-Authored-By: Claude Sonnet 5 --- README.md | 4 +-- __test__/route-protocol.spec.ts | 63 +++++++++++++++++++++++++++++---- src/proxy.rs | 14 +++++++- 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index e60c2f8..56c9833 100644 --- a/README.md +++ b/README.md @@ -356,7 +356,7 @@ Use `sourceAddressHeader` on a route to control how the real client IP is commun |---|---| | `'proxyProtocol'` | Sends a PROXY protocol v1 (text) header (`PROXY TCP4 0\r\n`) before any application data. Default for UDS upstreams. | | `'proxyProtocolV2'` | Sends a PROXY protocol v2 (binary) header before any application data. v2 adds a TLV section — the carrier for `forwardFingerprint` below and for [mTLS client cert forwarding](#forwarding-mtls-client-certificates). Keep it opt-in: the consumer must speak v2 (nginx/HAProxy do; Harper core's UDS reader parses v1 only before Harper 5.2). | -| `'xForwardedFor'` | Reads the first chunk of the HTTP request, inserts an `X-Forwarded-For` header after the request line, then copies the rest verbatim. No per-request parsing overhead for keep-alive connections. Default for TCP upstreams (disabled). Requires `protocol: 'http'` on the route (below). | +| `'xForwardedFor'` | Reads and rewrites every request on the connection (not just the first), inserting an `X-Forwarded-For` header after the request line — a pipelined or keep-alive request must be parsed too, or a later request could smuggle a spoofed header past the first-request-only rewrite. Default for TCP upstreams (disabled). Requires `protocol: 'http'` on the route (below). | | `'none'` | Does not forward source address information. Default for TCP upstreams. | ### Declaring the route protocol @@ -434,7 +434,7 @@ symphony computes the client's JA3/JA4 fingerprint from the ClientHello (the sam The **carrier depends on `sourceAddressHeader`**: - With `'proxyProtocolV2'`, the fingerprint rides a PROXY v2 **TLV** — type `0xE0` for JA3, `0xE1` for JA4 (in HAProxy's `0xE0–0xEF` private range). This works even in passthrough (`terminateTls: false`), since the header prefixes the raw TLS bytes. No `protocol` declaration is needed — the TLV carries it regardless of the route's application protocol. -- Otherwise, symphony injects an **`X-JA3` / `X-JA4` HTTP header**. This requires `protocol: 'http'` on the route (see [Declaring the route protocol](#declaring-the-route-protocol)) and a plaintext HTTP/1 upstream (`terminateTls: true` and not `http2`); it is skipped for HTTP/2 upstreams (use `'proxyProtocolV2'` there). Any client-supplied `X-JA3`/`X-JA4` is stripped so the injected value is authoritative and can't be spoofed. +- Otherwise, symphony injects an **`X-JA3` / `X-JA4` HTTP header**. This requires `protocol: 'http'` on the route (see [Declaring the route protocol](#declaring-the-route-protocol)) and a plaintext HTTP/1 upstream (`terminateTls: true` and not `http2`); it is skipped for HTTP/2 upstreams (use `'proxyProtocolV2'` there). For that HTTP/1 case, any client-supplied `X-JA3`/`X-JA4` is stripped so the injected value is authoritative and can't be spoofed — **this guarantee does not extend to an h2-negotiated connection on an `http2: true` route**: injection and stripping are both skipped there, so a client-supplied `X-JA3`/`X-JA4` reaches the upstream unmodified. Use `'proxyProtocolV2'` wherever h2 is possible. A config that requests a header-carried `forwardFingerprint` without declaring `protocol: 'http'` fails at construction — the same fail-loud rule as `sourceAddressHeader: 'xForwardedFor'`. A config that requests `forwardFingerprint` with no viable carrier at all — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — also fails at construction, with a distinct "no carrier" error: no `protocol` declaration could fix a passthrough route's inability to inject a header, so it isn't steered toward one. A route that could have carried the header but silently won't for some connections — `http2: true`, where ALPN negotiation is per-connection and some clients may still land on HTTP/1 — logs a startup warning instead, since that outcome isn't guaranteed. diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index 6233c46..6a5539c 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -64,24 +64,22 @@ describe('SymphonyProxy – route protocol declaration', () => { 0x10, 0x0c, 0x00, 0x04, 0x4d, 0x51, 0x54, 0x54, 0x04, 0x02, 0x00, 0x3c, 0x00, 0x00, ]); - const start = Date.now(); + // No separate timing assertion needed: if this were mistakenly fed to the header rewriter, + // it would stall waiting for a request terminator that never arrives, and the 2000ms idle + // timeout above would make this `await` reject (or return truncated/empty bytes) well before + // the deepEqual below could pass. A load-dependent wall-clock bound would only add flakiness. const response = await tlsRoundTrip({ port: proxyPort, servername: 'mqtt.example.com', caCert: cert.cert, data: mqttConnect, }); - const elapsedMs = Date.now() - start; assert.deepEqual( response, mqttConnect, 'raw MQTT bytes proxied verbatim — no header injected, no HTTP parsing attempted' ); - assert.ok( - elapsedMs < 1000, - `round-trip must complete promptly, not stall waiting for an HTTP header terminator (took ${elapsedMs}ms)` - ); await proxy.stop(); await upstream.close(); @@ -207,4 +205,57 @@ describe('SymphonyProxy – route protocol declaration', () => { 'declaring protocol: "http" must not paper over a passthrough route with no header carrier' ); }); + + // resolveConnection() parses and validates its `route` argument independently of the + // suspended-connection id (parse_resolve_spec runs before the id is even looked up), so these + // checks can be exercised directly against a fresh proxy without a real suspended connection. + describe('resolveConnection() protocol validation (symmetric with the static route table)', () => { + let proxy: SymphonyProxy; + + before(() => { + proxy = new SymphonyProxy({ listeners: [{ host: '127.0.0.1', port: 0 }], routes: [] }); + }); + + it('rejects xForwardedFor without a protocol: "http" declaration', () => { + assert.throws( + () => + proxy.resolveConnection('1', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + }), + /protocol/i, + 'resolveConnection must reject an undeclared xForwardedFor route just like the static route table' + ); + }); + + it('rejects a header-carried forwardFingerprint on a passthrough route as having no carrier', () => { + assert.throws( + () => + proxy.resolveConnection('2', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: false, + forwardFingerprint: 'ja3', + protocol: 'http', + }), + /no carrier/i, + 'resolveConnection must reject passthrough + header-carried forwardFingerprint just like the static route table' + ); + }); + + it('rejects xForwardedFor combined with http2 (header injection would corrupt h2 frames)', () => { + assert.throws( + () => + proxy.resolveConnection('3', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + protocol: 'http', + http2: true, + }), + /http2/i, + 'resolveConnection must reject xForwardedFor + http2 just like build_route does for the static route table' + ); + }); + }); }); diff --git a/src/proxy.rs b/src/proxy.rs index 2bf230d..c7edc8c 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -867,6 +867,7 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { let forward_fingerprint = parse_forward_fingerprint(r.forward_fingerprint.as_deref())?; let protocol = parse_route_protocol(r.protocol.as_deref())?; let requires_http = requires_http_protocol(source_address_mode, forward_fingerprint); + let http2 = r.http2.unwrap_or(false); // See parse_route_spec: passthrough has no carrier for a header-based mode regardless of // `protocol`, so it's rejected before (and distinctly from) the declaration check below. @@ -882,6 +883,17 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { )); } + // Mirrors build_route's XFF+h2 guard (router.rs) for the static route table — resolveConnection + // has no split h2 destination, but an h2-negotiated client on a terminated, http2:true route + // still disables the HTTP/1 rewriter, so a client-supplied X-Forwarded-For would reach the + // upstream neither injected nor stripped. This gap predates this PR's checks above; closing it + // here rather than leaving the two protocol checks as the only symmetric ones. + if source_address_mode == SourceAddressMode::XForwardedFor && http2 && r.terminate_tls { + return Err(napi::Error::from_reason( + "resolveConnection: sourceAddressHeader 'xForwardedFor' cannot be combined with http2 (header injection would corrupt h2 frames, and a client-supplied X-Forwarded-For would reach the upstream unstripped); use 'proxyProtocol'/'proxyProtocolV2' or 'none'".to_string(), + )); + } + Ok(ResolveSpec { upstream, terminate_tls: r.terminate_tls, @@ -891,7 +903,7 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { require_client_cert: r.mtls.as_ref().and_then(|m| m.require_client_cert).unwrap_or(false), source_address_mode, forward_fingerprint, - http2: r.http2.unwrap_or(false), + http2, protocol, }) } From 685bb01c3175ba8e1e6d2c5aae2f6bf1bb43bd00 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 10:49:49 -0600 Subject: [PATCH 05/12] Fix process-crashing throw in resolveConnection(); clean up error text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review (codex + gemini + grok + harper-domain) on the prior commit found a severe regression in the resolveConnection() validation added there: the tsfn callback in ts/proxy.ts calls this.emit('suspended', ...) with no try/catch, so a route validation error thrown synchronously by resolveConnection() — called synchronously from inside a 'suspended' listener, which is the documented and universally-used pattern (every existing test does this) — propagated back through emit() into the napi threadsafe-function callback as an uncaught exception. Verified with a standalone repro against the built addon: an unguarded process crashes outright; a route that hits this validation on live traffic would take the whole host process down, repeatedly, under real connections. Fixed by wrapping the callback's event dispatch in try/catch and routing any listener-thrown error to the 'error' event, matching how every other proxy-level error already reaches user code. Added a regression test (__test__/suspended.spec.ts) that pins this exact crash scenario. Also from this review round: - Removed `fingerprint_has_no_carrier` (router.rs) and its unit test: dead code made unreachable by the "no carrier" hard error already added to parse_route_spec/parse_resolve_spec in the prior commit — build_route can no longer be reached with a spec matching that condition. - Reworded the declaration-required error (proxy.rs, both the static and resolveConnection paths): it previously suggested switching to 'proxyProtocol'/'proxyProtocolV2' as if both always resolve the rejection, but 'proxyProtocol' (v1) still can't carry a fingerprint, so an operator on v1 following that advice hits the identical rejection unchanged. - Reworded the http2 header-injection warning to name whichever mode (xForwardedFor vs. a header-carried forwardFingerprint) actually triggered it, instead of always blaming X-Forwarded-For. Considered and reverted: hardening the forwardFingerprint+http2 warning (proxy.rs) into a hard error to match the existing xForwardedFor+http2 hard reject (router.rs) — 3 independent review passes now flag the asymmetry. Implemented it, then found it breaks an existing, intentional test (__test__/proxy-protocol-v2.spec.ts: "negotiates h2 and does not inject the fingerprint header into the HTTP/2 stream") that explicitly documents the current accepted behavior — a { http2: true, forwardFingerprint: 'ja3' } route without a split h2 upstream must still build and serve traffic, just without fingerprint forwarding to h2 clients. Escalating this to a hard error is a real design decision in tension with already-shipped, tested behavior, not a mechanical fix; recorded in the dispatch file's Risks & open questions rather than forced through. Co-Authored-By: Claude Sonnet 5 --- __test__/suspended.spec.ts | 62 ++++++++++++++++++++++++++++++++++++++ src/proxy.rs | 31 +++++++++++-------- src/router.rs | 45 --------------------------- ts/proxy.ts | 57 +++++++++++++++++++++-------------- 4 files changed, 115 insertions(+), 80 deletions(-) diff --git a/__test__/suspended.spec.ts b/__test__/suspended.spec.ts index 175151c..53bfcc9 100644 --- a/__test__/suspended.spec.ts +++ b/__test__/suspended.spec.ts @@ -228,3 +228,65 @@ describe('Suspended routes – reject with null', () => { assert.ok(socket.destroyed || !socket.writable, 'socket should be closed after rejection'); }); }); + +describe('Suspended routes – resolveConnection() validation error inside the listener', () => { + const cert = generateSelfSignedCert('localhost'); + let proxyPort: number; + let proxy: SymphonyProxy; + let capturedConn: SuspendedConnection | null = null; + + before(async () => { + proxyPort = await getFreePort(); + + proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'localhost', + upstreams: [], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + suspended: true, + suspendTimeoutMs: 5000, + }, + ], + }); + + await proxy.start(); + await sleep(50); + }); + + after(async () => { + await proxy.stop(); + }); + + // resolveConnection() is meant to be called synchronously from inside a 'suspended' listener + // (every other test in this file does exactly that). Calling it with a route the protocol/ + // carrier validation rejects (issue #38's construction-time checks, also applied to + // resolveConnection) throws synchronously — and that throw crosses back through + // EventEmitter.emit() into the napi threadsafe-function callback. Without a guard there, this + // took down the whole host process instead of surfacing as a normal proxy error; this test + // pins the fix (ts/proxy.ts's tsfn callback now catches and re-emits as 'error'). + it('emits "error" instead of crashing the process when resolveConnection() throws inside the "suspended" listener', async () => { + const errors: Error[] = []; + proxy.on('error', (err: Error) => errors.push(err)); + proxy.on('suspended', (conn) => { + capturedConn = conn; + // Undeclared xForwardedFor — rejected by parse_resolve_spec's protocol-declaration check. + proxy.resolveConnection(conn.id, { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + }); + }); + + const socket = startTlsSocket(proxyPort, 'localhost', cert.cert); + await sleep(200); + + assert.ok(capturedConn !== null, 'expected suspended event to have fired'); + assert.equal(errors.length, 1, 'the validation throw must surface as exactly one "error" event, not crash the process'); + assert.match(errors[0].message, /protocol/i, 'the surfaced error must be the protocol-declaration rejection'); + + socket.destroy(); + }); +}); diff --git a/src/proxy.rs b/src/proxy.rs index c7edc8c..0274017 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -762,7 +762,7 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { if requires_http && protocol != RouteProtocol::Http { return Err(napi::Error::from_reason(format!( - "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch to 'proxyProtocol'/'proxyProtocolV2' which work on any protocol", + "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch away from the header-carried mode: 'proxyProtocol'/'proxyProtocolV2' both work on any protocol for source-address forwarding, but only 'proxyProtocolV2' carries a fingerprint (v1 does not)", r.sni ))); } @@ -788,17 +788,22 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { if spec.http2 && !spec.terminate_tls { eprintln!("symphony: route '{}': http2=true has no effect when terminateTls=false (passthrough mode)", spec.sni); } - // A header-carried mode (xForwardedFor, or forwardFingerprint outside proxyProtocolV2) needs - // a plaintext HTTP/1 upstream — terminated and not h2 — or the runtime silently skips the - // rewriter (`eligible_for_header_rewriting` in proxy_conn.rs) and, for xForwardedFor, a - // client-supplied header reaches the upstream unstripped. The passthrough case is already a - // hard error above, so `requires_http` reaching here implies terminate_tls; the only - // remaining silent-miss case is http2, which is a soft warning rather than a hard error - // because ALPN negotiation is per-connection — declaring http2=true doesn't guarantee every - // client actually negotiates h2. + // xForwardedFor + http2 is a hard error (build_route, router.rs) since it's unconditionally + // unsafe — an h2 client's XFF would be neither injected nor stripped, forwarding an arbitrary + // client-supplied value as if authoritative. A header-carried forwardFingerprint in the same + // spot is intentionally only a warning, not a hard error: unlike XFF, which no route needs at + // all if it drops h2, some deployments accept "an h2 client can choose to not have its + // fingerprint forwarded (or forward its own)" as a known best-effort limitation of a signal + // that's advisory in the first place. Name whichever mode actually triggered this so the + // message doesn't blame X-Forwarded-For when the route never configured it. if requires_http && spec.http2 { + let mode_desc = if source_address_mode == SourceAddressMode::XForwardedFor { + "xForwardedFor" + } else { + "a header-carried forwardFingerprint (X-JA3/X-JA4)" + }; eprintln!( - "symphony: route '{}': header-injection forwarding (xForwardedFor / a header-carried forwardFingerprint) has no effect for any client that negotiates h2 (http2=true), and a client-supplied X-Forwarded-For reaches the upstream unstripped in that case; consider sourceAddressHeader='proxyProtocolV2'", + "symphony: route '{}': {mode_desc} has no effect for any client that negotiates h2 (http2=true) — injection and client-supplied-header stripping are both skipped, so an h2 client's own value reaches the upstream unmodified; consider sourceAddressHeader='proxyProtocolV2'", spec.sni ); } @@ -879,7 +884,7 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { if requires_http && protocol != RouteProtocol::Http { return Err(napi::Error::from_reason( - "resolveConnection: sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch to 'proxyProtocol'/'proxyProtocolV2' which work on any protocol".to_string(), + "resolveConnection: sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch away from the header-carried mode: 'proxyProtocol'/'proxyProtocolV2' both work on any protocol for source-address forwarding, but only 'proxyProtocolV2' carries a fingerprint (v1 does not)".to_string(), )); } @@ -887,7 +892,9 @@ fn parse_resolve_spec(r: &JsResolveRoute) -> Result { // has no split h2 destination, but an h2-negotiated client on a terminated, http2:true route // still disables the HTTP/1 rewriter, so a client-supplied X-Forwarded-For would reach the // upstream neither injected nor stripped. This gap predates this PR's checks above; closing it - // here rather than leaving the two protocol checks as the only symmetric ones. + // here rather than leaving the two protocol checks as the only symmetric ones. (Unlike XFF, + // forwardFingerprint + http2 is intentionally only a warning, not a hard error here either — + // see the matching comment in parse_route_spec.) if source_address_mode == SourceAddressMode::XForwardedFor && http2 && r.terminate_tls { return Err(napi::Error::from_reason( "resolveConnection: sourceAddressHeader 'xForwardedFor' cannot be combined with http2 (header injection would corrupt h2 frames, and a client-supplied X-Forwarded-For would reach the upstream unstripped); use 'proxyProtocol'/'proxyProtocolV2' or 'none'".to_string(), diff --git a/src/router.rs b/src/router.rs index 36fa310..9a5aba2 100644 --- a/src/router.rs +++ b/src/router.rs @@ -478,17 +478,6 @@ fn build_route( ); } - // A requested fingerprint needs a viable carrier. In passthrough there's no HTTP request - // to inject an X-JA3/X-JA4 header into, so the only carrier is a PROXY v2 TLV; without - // `proxyProtocolV2` the fingerprint is silently dropped. Warn rather than deploy a - // config whose requested signal never reaches the upstream. - if fingerprint_has_no_carrier(spec) { - eprintln!( - "symphony: route '{}': forwardFingerprint is set but has no carrier in passthrough mode (terminateTls=false) unless sourceAddressHeader='proxyProtocolV2' — the fingerprint will not be forwarded", - spec.sni - ); - } - let rate_limiter = spec .max_cps .map(|cps| Arc::new(RouteTokenBucket::new(cps, spec.burst))); @@ -507,16 +496,6 @@ fn build_route( }) } -/// True when a route requests `forwardFingerprint` but no carrier can deliver it: passthrough -/// (`terminateTls: false`) has no HTTP request for X-JA3/X-JA4 header injection, and only -/// `proxyProtocolV2` carries the fingerprint as a connection-scoped TLV. Other modes on a -/// terminated route can still inject the header for HTTP/1 connections, so they aren't flagged. -fn fingerprint_has_no_carrier(spec: &RouteSpec) -> bool { - !matches!(spec.forward_fingerprint, ForwardFingerprint::None) - && spec.source_address_mode != SourceAddressMode::ProxyProtocolV2 - && !spec.terminate_tls -} - /// Build the route's destinations: the default (h1) destination plus, when any /// upstream is marked `protocol: "h2"`, a separate destination for connections /// that negotiated h2 in ALPN. @@ -769,30 +748,6 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== ); } - #[test] - fn fingerprint_carrier_viability() { - let mut spec = tls_route("x", CERT_A, KEY_A); - - // terminated + fingerprint: header injection can carry it for HTTP/1 → not flagged. - spec.forward_fingerprint = ForwardFingerprint::Ja3; - spec.terminate_tls = true; - spec.source_address_mode = SourceAddressMode::None; - assert!(!fingerprint_has_no_carrier(&spec)); - - // passthrough + fingerprint + non-PP2: no carrier at all → flagged. - spec.terminate_tls = false; - assert!(fingerprint_has_no_carrier(&spec)); - - // passthrough + fingerprint + PP2: the TLV carries it → not flagged. - spec.source_address_mode = SourceAddressMode::ProxyProtocolV2; - assert!(!fingerprint_has_no_carrier(&spec)); - - // no fingerprint requested: never flagged, regardless of mode. - spec.forward_fingerprint = ForwardFingerprint::None; - spec.source_address_mode = SourceAddressMode::None; - assert!(!fingerprint_has_no_carrier(&spec)); - } - #[test] fn header_injection_detection() { // xForwardedFor always needs protocol: 'http', regardless of fingerprint. diff --git a/ts/proxy.ts b/ts/proxy.ts index d059edf..cdfd473 100644 --- a/ts/proxy.ts +++ b/ts/proxy.ts @@ -181,29 +181,40 @@ export class SymphonyProxy extends EventEmitter { this.emit('error', err); return; } - const event = raw as ProxyEvent; - switch (event.type) { - case 'blocked': - this.emit('blocked', { - ip: event.ip, - reason: event.reason, - listener: event.listener, - ja3: event.ja3, - ja4: event.ja4, - }); - break; - case 'suspended': - this.emit('suspended', { - id: event.id, - sni: event.sni, - peerIp: event.peerIp, - peerPort: event.peerPort, - listener: event.listener, - } satisfies SuspendedConnection); - break; - case 'error': - this.emit('error', new Error(event.message), { listener: event.listener }); - break; + // A listener can throw synchronously — most notably a 'suspended' handler that calls + // resolveConnection() with a route the new protocol/carrier validation rejects, which + // used to be a silent no-op and is now a thrown Error. EventEmitter.emit() propagates a + // listener's throw straight back to its caller, which here is this napi threadsafe + // function callback: left unguarded, that throw escapes into native code as an uncaught + // exception and takes the whole process down. Route it to 'error' instead, matching how + // every other proxy-level error already reaches user code. + try { + const event = raw as ProxyEvent; + switch (event.type) { + case 'blocked': + this.emit('blocked', { + ip: event.ip, + reason: event.reason, + listener: event.listener, + ja3: event.ja3, + ja4: event.ja4, + }); + break; + case 'suspended': + this.emit('suspended', { + id: event.id, + sni: event.sni, + peerIp: event.peerIp, + peerPort: event.peerPort, + listener: event.listener, + } satisfies SuspendedConnection); + break; + case 'error': + this.emit('error', new Error(event.message), { listener: event.listener }); + break; + } + } catch (listenerErr) { + this.emit('error', listenerErr instanceof Error ? listenerErr : new Error(String(listenerErr))); } }); } From 978a17755c28cf328f62269ce8fec4799c39811d Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 11:43:26 -0600 Subject: [PATCH 06/12] Root-cause the resolveConnection() crash: never throw for a live id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of independent review (codex + gemini + grok + harper-domain) found the previous commit's ts/proxy.ts try/catch guard was incomplete: it only covers a *synchronous* 'suspended' listener. README.md documents (and one existing test exercised) the async form `proxy.on('suspended', async (conn) => { ... })` — EventEmitter.emit() never awaits an async listener, so a validation throw after an `await` becomes an unhandledRejection no wrapper around emit() can intercept. Worse, even the synchronous guard only routes safely to 'error' when an 'error' listener is attached; with none, `emit('error', ...)` itself throws and re-enters the same napi callback, reproducing the crash. Root cause (per the domain reviewer's diagnosis, verified by reproduction): resolve_connection() breaks the invariant that a resolveConnection() call for a live id always terminates that suspension — `parse_resolve_spec(&r)?` used `?` to throw before ever reaching `suspended_registry.resolve()`, so a rejected connection was also leaked (held open for the full suspendTimeoutMs, not just crash-prone) on top of being impossible to signal safely across every call shape. Fixed at the source: resolve_connection() no longer throws for a validation failure. It drops the connection exactly as resolveConnection(id, null) would (closing it immediately, not after suspendTimeoutMs) and surfaces the reason via the existing JsEvent::Error → 'error' event channel — the same path every other native-originated error already uses, safe under any listener shape. The ts/proxy.ts try/catch from the prior commit stays as an unrelated belt-and-braces guard for a genuine listener bug, not the load- bearing fix. Updated the resolveConnection tests (route-protocol.spec.ts, suspended.spec.ts) from assert.throws to asserting the 'error' event, added a dedicated async-listener regression test, and added a prompt-close assertion pinning that the connection no longer lingers for suspendTimeoutMs on a rejected resolve. Also: reworded ts/types.ts's forwardFingerprint JSDoc and README.md's Bun section, which both still claimed guarantees the h2 case doesn't hold, and moved a doc comment (proxy_conn.rs) that had drifted onto the wrong function across an earlier edit. Co-Authored-By: Claude Sonnet 5 --- README.md | 2 +- __test__/route-protocol.spec.ts | 77 ++++++++++++++++++------------- __test__/suspended.spec.ts | 82 +++++++++++++++++++++++++++------ src/proxy.rs | 27 ++++++++--- src/proxy_conn.rs | 18 ++++---- ts/types.ts | 7 ++- 6 files changed, 150 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 56c9833..c1b5206 100644 --- a/README.md +++ b/README.md @@ -396,7 +396,7 @@ Most backends that consume PROXY protocol (nginx, HAProxy, HarperDB) read the he ### X-Forwarded-For (for Bun and other HTTP backends) -Bun's built-in HTTP server does not support PROXY protocol. Use `'xForwardedFor'` instead — symphony injects the header into the first HTTP request of each connection: +Bun's built-in HTTP server does not support PROXY protocol. Use `'xForwardedFor'` instead — symphony injects the header into every HTTP request on the connection, not just the first: ```typescript { diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index 6a5539c..890502f 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -209,50 +209,65 @@ describe('SymphonyProxy – route protocol declaration', () => { // resolveConnection() parses and validates its `route` argument independently of the // suspended-connection id (parse_resolve_spec runs before the id is even looked up), so these // checks can be exercised directly against a fresh proxy without a real suspended connection. - describe('resolveConnection() protocol validation (symmetric with the static route table)', () => { + // + // Unlike the static route table, an invalid resolveConnection() route must never *throw*: the + // call is documented to happen from inside a 'suspended' listener (sync or async), and a thrown + // exception there has no safe path back to the caller — an async listener's rejection is never + // awaited by EventEmitter, and even a synchronous throw only reaches user code if an 'error' + // listener happens to be attached. So a validation failure instead drops the connection (the + // same outcome as resolveConnection(id, null)) and surfaces the reason via the 'error' event. + describe('resolveConnection() protocol validation (symmetric with the static route table, fails via "error" event not a throw)', () => { let proxy: SymphonyProxy; before(() => { proxy = new SymphonyProxy({ listeners: [{ host: '127.0.0.1', port: 0 }], routes: [] }); }); - it('rejects xForwardedFor without a protocol: "http" declaration', () => { - assert.throws( - () => - proxy.resolveConnection('1', { - upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, - terminateTls: true, - sourceAddressHeader: 'xForwardedFor', - }), - /protocol/i, - 'resolveConnection must reject an undeclared xForwardedFor route just like the static route table' - ); + /** Call resolveConnection with `route` and resolve with the message of the next 'error' event. */ + function resolveAndCaptureError(id: string, route: Parameters[1]): Promise { + return new Promise((resolve, reject) => { + proxy.once('error', (err: Error) => resolve(err.message)); + try { + proxy.resolveConnection(id, route); + } catch (e) { + reject(new Error(`resolveConnection() must never throw for a validation failure: ${e}`)); + } + }); + } + + it('rejects xForwardedFor without a protocol: "http" declaration', async () => { + const message = await resolveAndCaptureError('1', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + }); + assert.match(message, /protocol/i, 'resolveConnection must reject an undeclared xForwardedFor route just like the static route table'); }); - it('rejects a header-carried forwardFingerprint on a passthrough route as having no carrier', () => { - assert.throws( - () => - proxy.resolveConnection('2', { - upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, - terminateTls: false, - forwardFingerprint: 'ja3', - protocol: 'http', - }), + it('rejects a header-carried forwardFingerprint on a passthrough route as having no carrier', async () => { + const message = await resolveAndCaptureError('2', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: false, + forwardFingerprint: 'ja3', + protocol: 'http', + }); + assert.match( + message, /no carrier/i, 'resolveConnection must reject passthrough + header-carried forwardFingerprint just like the static route table' ); }); - it('rejects xForwardedFor combined with http2 (header injection would corrupt h2 frames)', () => { - assert.throws( - () => - proxy.resolveConnection('3', { - upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, - terminateTls: true, - sourceAddressHeader: 'xForwardedFor', - protocol: 'http', - http2: true, - }), + it('rejects xForwardedFor combined with http2 (header injection would corrupt h2 frames)', async () => { + const message = await resolveAndCaptureError('3', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + protocol: 'http', + http2: true, + }); + assert.match( + message, /http2/i, 'resolveConnection must reject xForwardedFor + http2 just like build_route does for the static route table' ); diff --git a/__test__/suspended.spec.ts b/__test__/suspended.spec.ts index 53bfcc9..7a28881 100644 --- a/__test__/suspended.spec.ts +++ b/__test__/suspended.spec.ts @@ -229,7 +229,7 @@ describe('Suspended routes – reject with null', () => { }); }); -describe('Suspended routes – resolveConnection() validation error inside the listener', () => { +describe('Suspended routes – resolveConnection() with an invalid route never throws', () => { const cert = generateSelfSignedCert('localhost'); let proxyPort: number; let proxy: SymphonyProxy; @@ -260,19 +260,71 @@ describe('Suspended routes – resolveConnection() validation error inside the l await proxy.stop(); }); - // resolveConnection() is meant to be called synchronously from inside a 'suspended' listener - // (every other test in this file does exactly that). Calling it with a route the protocol/ - // carrier validation rejects (issue #38's construction-time checks, also applied to - // resolveConnection) throws synchronously — and that throw crosses back through - // EventEmitter.emit() into the napi threadsafe-function callback. Without a guard there, this - // took down the whole host process instead of surfacing as a normal proxy error; this test - // pins the fix (ts/proxy.ts's tsfn callback now catches and re-emits as 'error'). - it('emits "error" instead of crashing the process when resolveConnection() throws inside the "suspended" listener', async () => { + // resolveConnection() is meant to be called synchronously — or, per the documented usage, + // from an *async* 'suspended' listener — and either way there is no safe way for a config- + // validation failure (issue #38's protocol/carrier checks, also applied to resolveConnection) + // to reach the caller as a thrown exception: EventEmitter.emit() never awaits an async + // listener, so a throw after an `await` becomes an unhandled rejection regardless of any + // guard around `emit()`, and even a synchronous throw only reaches user code safely if an + // 'error' listener happens to be attached. So resolveConnection() itself never throws for a + // validation failure (src/proxy.rs) — it drops the connection exactly as + // resolveConnection(id, null) would (closing this test's socket promptly, not leaking it for + // the full suspendTimeoutMs) and surfaces the reason via the existing 'error' event, the same + // channel every other native-originated error already uses. + it('drops the connection and emits "error" — without throwing — when resolveConnection() is given an invalid route', async () => { const errors: Error[] = []; - proxy.on('error', (err: Error) => errors.push(err)); - proxy.on('suspended', (conn) => { + // .once, not .on: this describe block runs more than one test against the same shared + // `proxy`, and a listener left attached from an earlier test would fire again here too, + // double-counting errors (or, for 'suspended', calling resolveConnection twice for the + // same connection). + proxy.once('error', (err: Error) => errors.push(err)); + proxy.once('suspended', (conn) => { capturedConn = conn; // Undeclared xForwardedFor — rejected by parse_resolve_spec's protocol-declaration check. + assert.doesNotThrow(() => + proxy.resolveConnection(conn.id, { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + }) + ); + }); + + const socket = startTlsSocket(proxyPort, 'localhost', cert.cert); + // The connection is dropped as soon as resolveConnection() runs inside the 'suspended' + // listener above — i.e. before this test body reaches waitForClose() below — so the error + // listener must be attached up front, not only once we get around to waiting for it. + socket.on('error', () => {}); + await sleep(200); + + assert.ok(capturedConn !== null, 'expected suspended event to have fired'); + assert.equal(errors.length, 1, 'the validation failure must surface as exactly one "error" event'); + assert.match(errors[0].message, /protocol/i, 'the surfaced error must be the protocol-declaration rejection'); + + // The connection must be dropped promptly (like resolveConnection(id, null)), not held open + // for the full 5s suspendTimeoutMs — that hold-open-until-timeout was the resource-retention + // half of the bug this fix closes. + await waitForClose(socket, 2000); + assert.ok(socket.destroyed || !socket.writable, 'socket must close promptly, not linger until suspendTimeoutMs'); + + socket.destroy(); + }); + + // The README documents `proxy.on('suspended', async (conn) => { ... })`. EventEmitter.emit() + // does not await an async listener, so a throw after an `await` would become an + // unhandledRejection no ts/proxy.ts-level try/catch around emit() could ever intercept — the + // only robust fix is the one under test above: resolveConnection() itself must not throw. This + // test pins that the async-listener shape is safe too, not just the synchronous one. + it('is also safe from an async "suspended" listener (the documented usage)', async () => { + const errors: Error[] = []; + const unhandledRejections: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => unhandledRejections.push(reason); + process.on('unhandledRejection', onUnhandledRejection); + + proxy.once('error', (err: Error) => errors.push(err)); + proxy.once('suspended', async (conn) => { + capturedConn = conn; + await sleep(10); // simulate an async lookup before resolving, per the documented pattern proxy.resolveConnection(conn.id, { upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, terminateTls: true, @@ -281,10 +333,14 @@ describe('Suspended routes – resolveConnection() validation error inside the l }); const socket = startTlsSocket(proxyPort, 'localhost', cert.cert); - await sleep(200); + socket.on('error', () => {}); // the connection is dropped once resolveConnection() runs above + await sleep(300); + + process.off('unhandledRejection', onUnhandledRejection); assert.ok(capturedConn !== null, 'expected suspended event to have fired'); - assert.equal(errors.length, 1, 'the validation throw must surface as exactly one "error" event, not crash the process'); + assert.equal(unhandledRejections.length, 0, 'an async listener rejecting after resolveConnection() must not produce an unhandledRejection'); + assert.equal(errors.length, 1, 'the validation failure must still surface as exactly one "error" event'); assert.match(errors[0].message, /protocol/i, 'the surfaced error must be the protocol-declaration rejection'); socket.destroy(); diff --git a/src/proxy.rs b/src/proxy.rs index 0274017..5df34a9 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -718,14 +718,27 @@ impl SymphonyProxyWrap { .parse() .map_err(|_| napi::Error::from_reason(format!("invalid connection id: {id}")))?; - let resolved = match route { + // A resolveConnection() call for a live id must always terminate that suspension — the + // caller may be a synchronous or an async 'suspended' listener, and either way there is no + // safe way for a config-validation failure to propagate back to it as a thrown exception: + // EventEmitter.emit() doesn't await an async listener, so a throw after an `await` becomes + // an unhandled rejection, and even a synchronous throw only reaches user code safely if an + // 'error' listener happens to be attached (emitting 'error' with none throws too). So a + // validation failure here never propagates as an exception: drop the connection exactly as + // `resolveConnection(id, null)` would, and surface the reason via the existing JsEvent::Error + // → 'error' event path, the same channel every other native-originated error already uses. + let route_result = route.map(|r| { + parse_resolve_spec(&r) + .map_err(|e| e.reason) + .and_then(|spec| build_resolved_route(&spec).map_err(|e| e.to_string())) + }); + + let resolved = match route_result { None => None, - Some(r) => { - let spec = parse_resolve_spec(&r)?; - Some( - build_resolved_route(&spec) - .map_err(|e| napi::Error::from_reason(e.to_string()))?, - ) + Some(Ok(resolved)) => Some(resolved), + Some(Err(message)) => { + crate::proxy_conn::emit(&self.js_emit, JsEvent::Error { message, listener: String::new() }); + None } }; diff --git a/src/proxy_conn.rs b/src/proxy_conn.rs index 4044c2e..22299fa 100644 --- a/src/proxy_conn.rs +++ b/src/proxy_conn.rs @@ -517,14 +517,6 @@ where } } -/// The set of headers symphony owns end-to-end on the client→upstream request stream, applied to -/// every HTTP/1 request. Empty (→ a plain copy, no rewriting) unless the upstream is a plaintext -/// HTTP/1 stream and the mode injects HTTP headers. -/// -/// A configured header is *always* stripped from the client's request, even when symphony has no -/// authoritative value to substitute (`value: None`) — a client must never smuggle its own -/// `X-JA3`/`X-JA4`/`X-Forwarded-For` through precisely when we can't replace it. PROXY v2 carries -/// the fingerprint in a TLV, so it adds no header rewrite. /// Whether header-based forwarding (XFF / X-JA3 / X-JA4) is eligible for this connection: the /// route must declare `protocol: 'http'` (issue #38 — ALPN alone can't tell a native non-HTTP /// protocol from an HTTPS client that simply negotiated no ALPN) and the connection must not @@ -534,6 +526,14 @@ fn eligible_for_header_rewriting(protocol: RouteProtocol, negotiated_h2: bool) - protocol == RouteProtocol::Http && !negotiated_h2 } +/// The set of headers symphony owns end-to-end on the client→upstream request stream, applied to +/// every HTTP/1 request. Empty (→ a plain copy, no rewriting) unless the upstream is a plaintext +/// HTTP/1 stream and the mode injects HTTP headers. +/// +/// A configured header is *always* stripped from the client's request, even when symphony has no +/// authoritative value to substitute (`value: None`) — a client must never smuggle its own +/// `X-JA3`/`X-JA4`/`X-Forwarded-For` through precisely when we can't replace it. PROXY v2 carries +/// the fingerprint in a TLV, so it adds no header rewrite. fn header_rewrites(sf: &SourceForwarding<'_>, l7_http1: bool) -> Vec { use crate::http_proxy::HeaderRewrite; if !l7_http1 { @@ -591,7 +591,7 @@ impl Drop for ActiveGuard { } } -fn emit(tsf: &ThreadsafeFunction, event: JsEvent) { +pub(crate) fn emit(tsf: &ThreadsafeFunction, event: JsEvent) { // Non-blocking — drop the event if the JS queue is full tsf.call(Ok(event), napi::threadsafe_function::ThreadsafeFunctionCallMode::NonBlocking); } diff --git a/ts/types.ts b/ts/types.ts index e83bff0..0d04034 100644 --- a/ts/types.ts +++ b/ts/types.ts @@ -115,8 +115,11 @@ export interface RouteConfig { * Carrier: a PROXY v2 TLV when `sourceAddressHeader` is `'proxyProtocolV2'` (works in * passthrough too, since it prefixes the raw TLS bytes); otherwise an injected * `X-JA3` / `X-JA4` HTTP header, which requires a plaintext HTTP/1 upstream - * (`terminateTls: true` and not `http2`) — it is skipped otherwise. Any client-supplied - * `X-JA3` / `X-JA4` is stripped so the injected value is authoritative. + * (`terminateTls: true` and not `http2`) — it is skipped otherwise. For that HTTP/1 case, + * any client-supplied `X-JA3` / `X-JA4` is stripped so the injected value is authoritative — + * but this does NOT hold on an h2-negotiated connection on an `http2: true` route: injection + * and stripping are both skipped there, so a client-supplied `X-JA3` / `X-JA4` reaches the + * upstream unmodified. Use `'proxyProtocolV2'` wherever h2 is possible. */ forwardFingerprint?: 'ja3' | 'ja4' | 'none'; /** From 8e8dc87cf1caaf1b87827ed105bac227f7a01624 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 12:04:02 -0600 Subject: [PATCH 07/12] Extend resolveConnection() never-throw fix to malformed ids; harden error re-emit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of independent review (codex + gemini + grok + harper-domain) on the prior commit found two more paths that could still crash the process: - resolve_connection() still threw for an unparseable id (id.parse() used `?`), even though the same commit's own stated invariant — and CLAUDE.md's documented contract ("resolveConnection with unknown ID: a no-op, not an error") — says this should never throw either. Requires a caller bug to reach (the id from a real 'suspended' event always parses), but the whole point of the surrounding fix is that a caller bug must not be fatal. Now emits a JsEvent::Error and returns, same as the route-validation path. - ts/proxy.ts's catch-block fallback called this.emit('error', ...) with no guard: if no 'error' listener is attached, that call itself throws (Node's EventEmitter contract), and the catch had no defense against its own fallback re-triggering the exact crash it exists to prevent — worse, for an original 'error'-typed native event, this produced a double-throw that replaced the real stack with Node's generic ERR_UNHANDLED_ERROR. Now checks listenerCount('error') first; with no listener, propagates the original error as-is instead of a doomed re-emit. Also prefixed both resolveConnection() error paths with the connection id (and, for the route-validation path, the original message) so an operator can correlate a surfaced error back to which connection produced it — round 4 flagged the previous commit's messages as uncorrelatable on a busy proxy. Added a resolveConnection()-with-malformed-id test (route-protocol.spec.ts). Round 4 also raised (as "high"/blocker) moving the protocol-declaration and no-carrier checks out of parse_route_spec's fail-fast path into build_route's per-route isolation, so one route's bad config can't abort an entire port-set's config. This conflicts with existing, deliberately-written tests in this same PR ("construction must throw... not silently build a route that never injects the header") that encode fail-loud-at-construction as the intended contract. This is a real design tension, not a mechanical fix or one I should resolve unilaterally by overriding an already-tested contract; raised to Kris directly rather than guessed at (see dispatch log). Co-Authored-By: Claude Sonnet 5 --- __test__/route-protocol.spec.ts | 9 ++++++++ src/proxy.rs | 41 ++++++++++++++++++++++----------- ts/proxy.ts | 12 +++++++++- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index 890502f..6752465 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -272,5 +272,14 @@ describe('SymphonyProxy – route protocol declaration', () => { 'resolveConnection must reject xForwardedFor + http2 just like build_route does for the static route table' ); }); + + it('rejects an unparseable connection id via the "error" event instead of throwing', async () => { + const message = await resolveAndCaptureError('not-a-real-id', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + protocol: 'opaque', + }); + assert.match(message, /invalid connection id/i, 'a malformed id must surface via "error", not a thrown exception'); + }); }); }); diff --git a/src/proxy.rs b/src/proxy.rs index 5df34a9..16dff88 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -714,19 +714,29 @@ impl SymphonyProxyWrap { #[napi] pub fn resolve_connection(&self, id: String, route: Option) -> Result<()> { - let id_num: u64 = id - .parse() - .map_err(|_| napi::Error::from_reason(format!("invalid connection id: {id}")))?; - - // A resolveConnection() call for a live id must always terminate that suspension — the - // caller may be a synchronous or an async 'suspended' listener, and either way there is no - // safe way for a config-validation failure to propagate back to it as a thrown exception: - // EventEmitter.emit() doesn't await an async listener, so a throw after an `await` becomes - // an unhandled rejection, and even a synchronous throw only reaches user code safely if an - // 'error' listener happens to be attached (emitting 'error' with none throws too). So a - // validation failure here never propagates as an exception: drop the connection exactly as - // `resolveConnection(id, null)` would, and surface the reason via the existing JsEvent::Error - // → 'error' event path, the same channel every other native-originated error already uses. + // A resolveConnection() call must never throw — the caller may be a synchronous or an async + // 'suspended' listener, and either way there is no safe way for a failure here to propagate + // back to it as a thrown exception: EventEmitter.emit() doesn't await an async listener, so a + // throw after an `await` becomes an unhandled rejection, and even a synchronous throw only + // reaches user code safely if an 'error' listener happens to be attached. This applies + // equally to an unparseable id (a caller bug, but per CLAUDE.md's own contract "resolveConnection + // with unknown ID: a no-op, not an error" — a string that can't even parse to a u64 can never + // have been a live id either) and to a route the protocol/carrier validation rejects: both + // surface via the existing JsEvent::Error → 'error' event path instead of a thrown exception. + let id_num: u64 = match id.parse() { + Ok(n) => n, + Err(_) => { + crate::proxy_conn::emit( + &self.js_emit, + JsEvent::Error { + message: format!("resolveConnection: invalid connection id '{id}' (not a valid id, so it was never live)"), + listener: String::new(), + }, + ); + return Ok(()); + } + }; + let route_result = route.map(|r| { parse_resolve_spec(&r) .map_err(|e| e.reason) @@ -737,7 +747,10 @@ impl SymphonyProxyWrap { None => None, Some(Ok(resolved)) => Some(resolved), Some(Err(message)) => { - crate::proxy_conn::emit(&self.js_emit, JsEvent::Error { message, listener: String::new() }); + crate::proxy_conn::emit( + &self.js_emit, + JsEvent::Error { message: format!("resolveConnection(id={id}): {message}"), listener: String::new() }, + ); None } }; diff --git a/ts/proxy.ts b/ts/proxy.ts index cdfd473..2f0b6d8 100644 --- a/ts/proxy.ts +++ b/ts/proxy.ts @@ -214,7 +214,17 @@ export class SymphonyProxy extends EventEmitter { break; } } catch (listenerErr) { - this.emit('error', listenerErr instanceof Error ? listenerErr : new Error(String(listenerErr))); + // If nothing is listening for 'error', emit('error', ...) itself throws (Node's + // EventEmitter contract — every consumer of this class must attach one), which + // would otherwise recurse right back into this catch with no way out, escaping as + // an unhandled double-throw that also replaces the original stack with a generic + // one. Re-emit only when a listener actually exists; otherwise propagate the + // original error as-is rather than trying (and failing) to route it through 'error'. + if (this.listenerCount('error') > 0) { + this.emit('error', listenerErr instanceof Error ? listenerErr : new Error(String(listenerErr))); + } else { + throw listenerErr; + } } }); } From 0a18e3a550e90f0a36a41542c35f20bcbd70a166 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 12:35:03 -0600 Subject: [PATCH 08/12] Close the actual crash blocker: guarantee emit('error') can never throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 (codex + gemini + grok) found the previous commit's listenerCount guard didn't fix anything for the primary scenario: a resolveConnection() validation failure emits JsEvent::Error, which the tsfn callback turns into this.emit('error', ...) — and if literally no 'error' listener is attached (the common embedded-consumer case this whole fix line is about), that emit itself throws (Node's EventEmitter contract), the catch block's own listenerCount check finds zero listeners, and it rethrows the same error right back out of the callback. The guard only avoided a *worse* double-throw on a listener's own bug; it never touched the main path. Root-caused instead: the SymphonyProxy constructor now installs a permanent no-op 'error' listener, so emit('error', ...) can structurally never throw for lack of one — any listener a consumer does attach still fires normally alongside it. The listenerCount check in the tsfn catch block stays as defense-in-depth for a consumer that calls removeAllListeners('error'). Verified by reverting the fix and confirming the new regression test (__test__/suspended.spec.ts) fails without it, then restoring and confirming it passes. Also from round 5, a real efficiency/correctness gap (not a crash): resolve_ connection() built a full TLS/cert config and could emit a spurious error for an id that had already expired or was never valid, before ever checking whether the id was still a live suspension — wasted work, and a confusing error for a connection that's already gone. Added SuspendedRegistry::contains and check it first, restoring the documented "unknown id: silent no-op" contract exactly. This required rewriting the resolveConnection validation tests in route-protocol.spec.ts to use a real suspended connection per case instead of an arbitrary id, since a fabricated id now short-circuits before validation ever runs. Co-Authored-By: Claude Sonnet 5 --- __test__/route-protocol.spec.ts | 87 +++++++++++++++++++++++++-------- __test__/suspended.spec.ts | 19 +++++++ src/proxy.rs | 8 +++ src/suspended.rs | 8 +++ ts/proxy.ts | 25 +++++++--- 5 files changed, 121 insertions(+), 26 deletions(-) diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index 6752465..a960e76 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -206,9 +206,11 @@ describe('SymphonyProxy – route protocol declaration', () => { ); }); - // resolveConnection() parses and validates its `route` argument independently of the - // suspended-connection id (parse_resolve_spec runs before the id is even looked up), so these - // checks can be exercised directly against a fresh proxy without a real suspended connection. + // resolveConnection() parses and validates its `route` argument only once the id is confirmed + // still live (an id that already timed out is a documented no-op — SuspendedRegistry::contains + // — so it never reaches route validation at all). These tests therefore need a *real* suspended + // connection per case, not an arbitrary id: a fabricated id would short-circuit before the + // validation logic under test ever runs. // // Unlike the static route table, an invalid resolveConnection() route must never *throw*: the // call is documented to happen from inside a 'suspended' listener (sync or async), and a thrown @@ -217,26 +219,62 @@ describe('SymphonyProxy – route protocol declaration', () => { // listener happens to be attached. So a validation failure instead drops the connection (the // same outcome as resolveConnection(id, null)) and surfaces the reason via the 'error' event. describe('resolveConnection() protocol validation (symmetric with the static route table, fails via "error" event not a throw)', () => { + const cert = generateSelfSignedCert('localhost'); let proxy: SymphonyProxy; + let proxyPort: number; - before(() => { - proxy = new SymphonyProxy({ listeners: [{ host: '127.0.0.1', port: 0 }], routes: [] }); + before(async () => { + proxyPort = await getFreePort(); + proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'localhost', + upstreams: [], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + suspended: true, + suspendTimeoutMs: 5000, + }, + ], + }); + await proxy.start(); + await sleep(50); + }); + + after(async () => { + await proxy.stop(); }); - /** Call resolveConnection with `route` and resolve with the message of the next 'error' event. */ - function resolveAndCaptureError(id: string, route: Parameters[1]): Promise { + /** Open a real suspended connection and resolve with its id once the 'suspended' event fires. */ + function getLiveSuspendedId(): Promise<{ id: string; socket: tls.TLSSocket }> { return new Promise((resolve, reject) => { - proxy.once('error', (err: Error) => resolve(err.message)); - try { - proxy.resolveConnection(id, route); - } catch (e) { - reject(new Error(`resolveConnection() must never throw for a validation failure: ${e}`)); - } + const socket = tls.connect({ port: proxyPort, host: '127.0.0.1', servername: 'localhost', ca: cert.cert, rejectUnauthorized: false }); + socket.on('error', () => {}); // resolved with an invalid route below — the connection gets dropped + proxy.once('suspended', (conn) => resolve({ id: conn.id, socket })); + setTimeout(() => reject(new Error('suspended event timeout')), 2000); }); } + /** Call resolveConnection with `route` on a live id and resolve with the next 'error' event's message. */ + async function resolveAndCaptureError(route: Parameters[1]): Promise { + const { id, socket } = await getLiveSuspendedId(); + try { + return await new Promise((resolve, reject) => { + proxy.once('error', (err: Error) => resolve(err.message)); + try { + proxy.resolveConnection(id, route); + } catch (e) { + reject(new Error(`resolveConnection() must never throw for a validation failure: ${e}`)); + } + }); + } finally { + socket.destroy(); + } + } + it('rejects xForwardedFor without a protocol: "http" declaration', async () => { - const message = await resolveAndCaptureError('1', { + const message = await resolveAndCaptureError({ upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, terminateTls: true, sourceAddressHeader: 'xForwardedFor', @@ -245,7 +283,7 @@ describe('SymphonyProxy – route protocol declaration', () => { }); it('rejects a header-carried forwardFingerprint on a passthrough route as having no carrier', async () => { - const message = await resolveAndCaptureError('2', { + const message = await resolveAndCaptureError({ upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, terminateTls: false, forwardFingerprint: 'ja3', @@ -259,7 +297,7 @@ describe('SymphonyProxy – route protocol declaration', () => { }); it('rejects xForwardedFor combined with http2 (header injection would corrupt h2 frames)', async () => { - const message = await resolveAndCaptureError('3', { + const message = await resolveAndCaptureError({ upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, terminateTls: true, sourceAddressHeader: 'xForwardedFor', @@ -273,11 +311,20 @@ describe('SymphonyProxy – route protocol declaration', () => { ); }); + // A malformed id fails to parse before the liveness check even runs, so this one still uses + // an arbitrary (non-numeric) id rather than a real suspended connection. it('rejects an unparseable connection id via the "error" event instead of throwing', async () => { - const message = await resolveAndCaptureError('not-a-real-id', { - upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, - terminateTls: true, - protocol: 'opaque', + const message = await new Promise((resolve, reject) => { + proxy.once('error', (err: Error) => resolve(err.message)); + try { + proxy.resolveConnection('not-a-real-id', { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + protocol: 'opaque', + }); + } catch (e) { + reject(new Error(`resolveConnection() must never throw for a validation failure: ${e}`)); + } }); assert.match(message, /invalid connection id/i, 'a malformed id must surface via "error", not a thrown exception'); }); diff --git a/__test__/suspended.spec.ts b/__test__/suspended.spec.ts index 7a28881..d2a3ddd 100644 --- a/__test__/suspended.spec.ts +++ b/__test__/suspended.spec.ts @@ -229,6 +229,25 @@ describe('Suspended routes – reject with null', () => { }); }); +// Node's EventEmitter special-cases 'error': emitting it with zero listeners attached throws +// synchronously instead of dropping the event. Every 'error' emission in SymphonyProxy happens +// inside the napi threadsafe-function callback, so that throw would otherwise escape into native +// code and crash the whole process — not just this instance — the very first time ANY native +// error fires (a resolveConnection() validation failure, a TLS error, anything) on a proxy whose +// owner hasn't gotten around to attaching an 'error' listener yet. The constructor installs a +// permanent no-op listener specifically to make this impossible; this test pins that a fresh +// proxy with zero listeners of its own survives an 'error' emission. +describe('SymphonyProxy never crashes from emit("error") with no listener attached', () => { + it('a proxy with no "error" listener at all does not throw when an error is emitted', () => { + const proxy = new SymphonyProxy({ listeners: [{ host: '127.0.0.1', port: 0 }], routes: [] }); + assert.equal(proxy.listenerCount('error'), 1, 'the constructor must install exactly one default listener'); + assert.doesNotThrow( + () => proxy.emit('error', new Error('simulated native error, no consumer listener attached')), + 'emit("error", ...) must never throw for lack of a listener — this is the crash the round-5 review flagged as a blocker' + ); + }); +}); + describe('Suspended routes – resolveConnection() with an invalid route never throws', () => { const cert = generateSelfSignedCert('localhost'); let proxyPort: number; diff --git a/src/proxy.rs b/src/proxy.rs index 16dff88..7d1bfb8 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -737,6 +737,14 @@ impl SymphonyProxyWrap { } }; + // An id that has already timed out (or was never valid) is a documented no-op + // (`SuspendedRegistry::resolve`) — check before doing any parse/build work for `route`, not + // after: building a terminating route's TLS config is real work, and an error emitted for a + // connection that's already gone would be spurious noise, not a signal an operator can act on. + if !self.suspended_registry.contains(id_num) { + return Ok(()); + } + let route_result = route.map(|r| { parse_resolve_spec(&r) .map_err(|e| e.reason) diff --git a/src/suspended.rs b/src/suspended.rs index 83f5712..2b1e965 100644 --- a/src/suspended.rs +++ b/src/suspended.rs @@ -53,6 +53,14 @@ impl SuspendedRegistry { self.pending.remove(&id); } + /// Whether `id` is still a live pending suspension. Lets `resolveConnection()` skip parsing + /// and building a route (cert/TLS work included) for an id that has already timed out or was + /// never valid — that work would be wasted, and any resulting error would be spurious, since + /// `resolve()` already treats an unknown id as a silent no-op. + pub fn contains(&self, id: u64) -> bool { + self.pending.contains_key(&id) + } + /// Number of currently pending suspended connections. pub fn pending_count(&self) -> u64 { self.pending.len() as u64 diff --git a/ts/proxy.ts b/ts/proxy.ts index 2f0b6d8..52422f6 100644 --- a/ts/proxy.ts +++ b/ts/proxy.ts @@ -165,6 +165,17 @@ export class SymphonyProxy extends EventEmitter { constructor(config: ProxyConfig) { super(); + // EventEmitter's 'error' event is special-cased: emitting it with zero listeners attached + // throws synchronously instead of silently dropping the event. Every emit('error', ...) in + // this class happens inside the napi threadsafe-function callback below — an uncaught throw + // there escapes into native code and kills the whole process, not just this instance. A + // config-validation failure surfaced via resolveConnection() (see resolve_connection in + // proxy.rs) now always goes through this exact channel, so a consumer that hasn't gotten + // around to attaching its own 'error' listener yet must not crash the process for it. This + // permanent no-op listener guarantees emit('error', ...) can never throw for lack of a + // listener; any listener a consumer attaches still fires normally alongside it, so real + // error observability is unaffected — only the "nobody's listening" crash is removed. + this.on('error', () => {}); const { SymphonyProxyWrap: Wrap } = loadAddon(); const jsConfig = { @@ -214,12 +225,14 @@ export class SymphonyProxy extends EventEmitter { break; } } catch (listenerErr) { - // If nothing is listening for 'error', emit('error', ...) itself throws (Node's - // EventEmitter contract — every consumer of this class must attach one), which - // would otherwise recurse right back into this catch with no way out, escaping as - // an unhandled double-throw that also replaces the original stack with a generic - // one. Re-emit only when a listener actually exists; otherwise propagate the - // original error as-is rather than trying (and failing) to route it through 'error'. + // The constructor installs a permanent no-op 'error' listener specifically so + // emit('error', ...) can never throw for lack of one — but a consumer calling + // removeAllListeners('error') (or with no args) removes that safety net too. This + // check is defense-in-depth for that case: if nothing is listening, emit('error', ...) + // itself throws (Node's EventEmitter contract), which would otherwise recurse right + // back into this catch with no way out, escaping as an unhandled double-throw that + // also replaces the original stack with a generic one. Re-emit only when a listener + // actually exists; otherwise propagate the original error as-is. if (this.listenerCount('error') > 0) { this.emit('error', listenerErr instanceof Error ? listenerErr : new Error(String(listenerErr))); } else { From dbd35d31b241ee9e0188f591cb8649f994e3a3c3 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 13:05:50 -0600 Subject: [PATCH 09/12] Defer event dispatch via process.nextTick; verify the actual crash boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 (codex + gemini + grok + harper-domain) identified the real root cause behind five rounds of incremental crash-path patching: every event emission happened synchronously inside the napi threadsafe-function callback, so any exception escaping it (a listener bug, an args-count mismatch between our own emit() calls and the documented (err, {listener}) signature, emit('error', ...) itself throwing) crosses that boundary — no amount of try/catch around individual emit() calls fences an attack surface that's fundamentally "arbitrary listener code runs inside a native callback frame." ts/proxy.ts now defers all dispatch via process.nextTick(), so any such throw becomes an ordinary JS event-loop exception instead. Investigated rather than taken on faith: I reproduced the review's exact "blocker" scenarios (a throwing 'error' listener, removeAllListeners('error') then a failure, no uncaughtException handler at all) against both the prior commit's code and the nextTick-deferred version. In this napi 2.16.17 / Node v26.2.0 environment, both already routed a pending exception from inside the tsfn callback through the ordinary process.on('uncaughtException') path with an identical clean stack trace and exit code — no observable crash difference. Documented this finding in the nextTick comment rather than overclaiming a reproduced fix. Kept the deferral anyway: it's zero-cost and holds a more principled invariant (listener code runs as a normal event-loop turn, not inside a native call stack) that may matter on other napi/Node versions even where it doesn't change behavior here. Added a real subprocess regression test (__test__/fixtures/crash-boundary- repro.ts, spawned from suspended.spec.ts) that reproduces a throwing 'error' listener hit through the actual native callback in a child process and asserts it surfaces via uncaughtException rather than hanging or being killed by a signal — closing the coverage gap both Gemini and the domain reviewer flagged: every existing crash-path test called proxy.emit() directly in-process, never crossing the real napi boundary at all. Also changed the constructor's default 'error' listener from a silent no-op to a stderr logger (round 6, "major": a swallowed resolveConnection() validation failure was invisible with no log, no counter, and NonBlocking event delivery meant it could also just be dropped under queue pressure) — an operator can now see a background proxy error even without wiring up their own 'error' listener, and any listener a consumer does attach still fires normally alongside it. Co-Authored-By: Claude Sonnet 5 --- __test__/fixtures/crash-boundary-repro.ts | 68 +++++++++++++++++++++++ __test__/suspended.spec.ts | 44 ++++++++++++++- ts/proxy.ts | 67 ++++++++++------------ 3 files changed, 140 insertions(+), 39 deletions(-) create mode 100644 __test__/fixtures/crash-boundary-repro.ts diff --git a/__test__/fixtures/crash-boundary-repro.ts b/__test__/fixtures/crash-boundary-repro.ts new file mode 100644 index 0000000..86f294b --- /dev/null +++ b/__test__/fixtures/crash-boundary-repro.ts @@ -0,0 +1,68 @@ +/** + * Standalone repro spawned as a real child process by suspended.spec.ts's "crash-boundary" + * test — must run out-of-process because it deliberately triggers a throwing `'error'` + * listener via a real resolveConnection() validation failure delivered through the napi + * threadsafe-function callback (not a direct, in-process `proxy.emit('error', ...)` call, + * which never exercises that boundary at all). + * + * Success criterion: stdout contains "CAUGHT:" (uncaughtException fired and was handled) + * and the process exits 0 — a throw reached through the real native callback still surfaces + * as an ordinary, catchable JS exception rather than hanging or killing the process by signal. + */ +import { SymphonyProxy } from '../../ts/proxy.js'; +import { generateSelfSignedCert, getFreePort, sleep } from '../util.js'; +import * as tls from 'node:tls'; + +async function main() { + process.on('uncaughtException', (err) => { + console.log(`CAUGHT: ${err.message}`); + process.exit(0); + }); + + const cert = generateSelfSignedCert('localhost'); + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'localhost', + upstreams: [], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + suspended: true, + suspendTimeoutMs: 5000, + }, + ], + }); + + // A deliberately buggy consumer 'error' handler — this is exactly the scenario the + // review flagged: if this throws inside the tsfn callback rather than on a deferred + // tick, it crashes uncatchably instead of surfacing here. + proxy.on('error', () => { + throw new Error('deliberately buggy error listener'); + }); + + proxy.on('suspended', (conn) => { + // Undeclared xForwardedFor — rejected by parse_resolve_spec, emits 'error'. + proxy.resolveConnection(conn.id, { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + }); + }); + + await proxy.start(); + await sleep(50); + + const socket = tls.connect({ port: proxyPort, host: '127.0.0.1', servername: 'localhost', ca: cert.cert, rejectUnauthorized: false }); + socket.on('error', () => {}); + + // If nothing crashed and nothing was caught within this window, the fix (or the + // deliberately-throwing listener) didn't do what this repro expects — fail loudly + // rather than let the test hang. + await sleep(2000); + console.log('NEVER_THREW'); + process.exit(1); +} + +main(); diff --git a/__test__/suspended.spec.ts b/__test__/suspended.spec.ts index d2a3ddd..b8c54d5 100644 --- a/__test__/suspended.spec.ts +++ b/__test__/suspended.spec.ts @@ -12,7 +12,9 @@ */ import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; import * as net from 'node:net'; +import * as path from 'node:path'; import * as tls from 'node:tls'; import { after, before, describe, it } from 'node:test'; import { SymphonyProxy } from '../ts/proxy.js'; @@ -235,7 +237,7 @@ describe('Suspended routes – reject with null', () => { // code and crash the whole process — not just this instance — the very first time ANY native // error fires (a resolveConnection() validation failure, a TLS error, anything) on a proxy whose // owner hasn't gotten around to attaching an 'error' listener yet. The constructor installs a -// permanent no-op listener specifically to make this impossible; this test pins that a fresh +// permanent default listener specifically to make this impossible; this test pins that a fresh // proxy with zero listeners of its own survives an 'error' emission. describe('SymphonyProxy never crashes from emit("error") with no listener attached', () => { it('a proxy with no "error" listener at all does not throw when an error is emitted', () => { @@ -248,6 +250,46 @@ describe('SymphonyProxy never crashes from emit("error") with no listener attach }); }); +// The test above (and the others in this file) only ever exercise `proxy.emit(...)` called +// in-process — never a listener that throws while dispatch is reached via the real napi +// threadsafe-function callback in its own child process. This pins the actual, observable +// contract in a real process rather than an in-process EventEmitter call: a throwing 'error' +// listener, hit through a genuine resolveConnection() validation failure delivered by the native +// callback, must surface via Node's ordinary `uncaughtException` mechanism (a clean stack trace, +// a normal exit) — not hang, not silently vanish, and not kill the process by a signal. See the +// nextTick comment in ts/proxy.ts's constructor for why the fix is kept as defense-in-depth even +// though this exact scenario already routed through `uncaughtException` cleanly before it too, in +// this napi/Node version. +describe('crash boundary: a throwing "error" listener, hit via the real napi callback', () => { + it('surfaces via uncaughtException instead of hanging or being killed by a signal', async () => { + const fixture = path.join(__dirname, 'fixtures', 'crash-boundary-repro.js'); + const stdout = await new Promise((resolve, reject) => { + const child = spawn(process.execPath, [fixture], { stdio: ['ignore', 'pipe', 'pipe'] }); + let out = ''; + let err = ''; + child.stdout.on('data', (d) => (out += d.toString())); + child.stderr.on('data', (d) => (err += d.toString())); + child.on('exit', (code, signal) => { + if (signal) { + reject(new Error(`repro process was killed by signal ${signal} (a real crash, not a caught exception) — stderr: ${err}`)); + } else { + resolve(out); + } + }); + setTimeout(() => { + child.kill(); + reject(new Error(`repro process hung — stdout so far: ${out}, stderr: ${err}`)); + }, 5000); + }); + + assert.match( + stdout, + /^CAUGHT: /m, + `expected the throwing listener's error to surface via process.on('uncaughtException'), got: ${stdout}` + ); + }); +}); + describe('Suspended routes – resolveConnection() with an invalid route never throws', () => { const cert = generateSelfSignedCert('localhost'); let proxyPort: number; diff --git a/ts/proxy.ts b/ts/proxy.ts index 52422f6..cc9b755 100644 --- a/ts/proxy.ts +++ b/ts/proxy.ts @@ -165,17 +165,14 @@ export class SymphonyProxy extends EventEmitter { constructor(config: ProxyConfig) { super(); - // EventEmitter's 'error' event is special-cased: emitting it with zero listeners attached - // throws synchronously instead of silently dropping the event. Every emit('error', ...) in - // this class happens inside the napi threadsafe-function callback below — an uncaught throw - // there escapes into native code and kills the whole process, not just this instance. A - // config-validation failure surfaced via resolveConnection() (see resolve_connection in - // proxy.rs) now always goes through this exact channel, so a consumer that hasn't gotten - // around to attaching its own 'error' listener yet must not crash the process for it. This - // permanent no-op listener guarantees emit('error', ...) can never throw for lack of a - // listener; any listener a consumer attaches still fires normally alongside it, so real - // error observability is unaffected — only the "nobody's listening" crash is removed. - this.on('error', () => {}); + // A default 'error' listener so a background failure (e.g. a resolveConnection() validation + // rejection — see resolve_connection in proxy.rs) is never silent for a consumer who hasn't + // gotten around to attaching their own listener yet. It logs rather than swallows: an + // operator should be able to tell a suspended route is failing without a proxy.on('error') + // wired up, and any listener the consumer does attach still fires normally alongside this one. + this.on('error', (err: Error, ctx?: { listener?: string }) => { + console.error(`symphony: unhandled proxy error${ctx?.listener ? ` [${ctx.listener}]` : ''}:`, err); + }); const { SymphonyProxyWrap: Wrap } = loadAddon(); const jsConfig = { @@ -188,18 +185,26 @@ export class SymphonyProxy extends EventEmitter { }; this._inner = new Wrap(jsConfig, (err, raw) => { - if (err) { - this.emit('error', err); - return; - } - // A listener can throw synchronously — most notably a 'suspended' handler that calls - // resolveConnection() with a route the new protocol/carrier validation rejects, which - // used to be a silent no-op and is now a thrown Error. EventEmitter.emit() propagates a - // listener's throw straight back to its caller, which here is this napi threadsafe - // function callback: left unguarded, that throw escapes into native code as an uncaught - // exception and takes the whole process down. Route it to 'error' instead, matching how - // every other proxy-level error already reaches user code. - try { + // Defer dispatch to the next tick instead of emitting synchronously from inside this napi + // threadsafe-function callback. The attack surface here is "arbitrary listener code can run + // synchronously inside a native callback frame" — a listener bug, an args-mismatch in our + // own emit calls, emit('error', ...) itself throwing because nothing is listening — and no + // amount of try/catch around individual emit() calls fences that surface completely (each + // prior fix in this area closed one specific escape and another was found). Empirically, in + // this napi/Node version a pending exception from inside the tsfn callback already routes + // through the ordinary `process.on('uncaughtException')` path rather than a hard native + // abort, so this deferral is not covering an observed crash difference here — verified by + // reproducing several throw scenarios (a throwing listener, `removeAllListeners('error')`, + // no handler at all) against both the synchronous and deferred forms and finding identical, + // clean `uncaughtException` behavior in both. It's kept anyway as the more principled + // invariant to hold regardless of napi-version-specific pending-exception routing: listener + // code should run as an ordinary JS event-loop turn, not inside a native call stack, on + // general defense-in-depth grounds rather than a reproduced local crash. + process.nextTick(() => { + if (err) { + this.emit('error', err); + return; + } const event = raw as ProxyEvent; switch (event.type) { case 'blocked': @@ -224,21 +229,7 @@ export class SymphonyProxy extends EventEmitter { this.emit('error', new Error(event.message), { listener: event.listener }); break; } - } catch (listenerErr) { - // The constructor installs a permanent no-op 'error' listener specifically so - // emit('error', ...) can never throw for lack of one — but a consumer calling - // removeAllListeners('error') (or with no args) removes that safety net too. This - // check is defense-in-depth for that case: if nothing is listening, emit('error', ...) - // itself throws (Node's EventEmitter contract), which would otherwise recurse right - // back into this catch with no way out, escaping as an unhandled double-throw that - // also replaces the original stack with a generic one. Re-emit only when a listener - // actually exists; otherwise propagate the original error as-is. - if (this.listenerCount('error') > 0) { - this.emit('error', listenerErr instanceof Error ? listenerErr : new Error(String(listenerErr))); - } else { - throw listenerErr; - } - } + }); }); } From 96a4d76cc2b7a8018d31130d56dc2361feb4c944 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 13:25:17 -0600 Subject: [PATCH 10/12] Isolate protocol-declaration/no-carrier failures per-route, per Kris's call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review flagged this three times (rounds 2, 4, 6) and I'd declined each time because an existing test in this PR asserted the opposite ("construction must throw... not silently build a route that never injects the header"), which read like a deliberate design choice matching the issue #38 comment's "fail loud" language. Raised it to Kris directly rather than guess; he chose isolation (option 2): a route that fails the "no carrier" or "declare protocol: 'http'" check should be dropped/last-good-carried the same way a bad cert or the existing xForwardedFor+http2 check already is, not abort the entire `new SymphonyProxy()`/`updateConfig()` call for every other route on the port-set. Moved both checks from parse_route_spec (proxy.rs), which runs inside a `.collect::>>()?` across every route with no isolation, into build_route (router.rs), which build_route_table already wraps with per-route isolation — skip the SNI on initial build, carry the last-good route forward on a hot-swap, log clearly either way. resolveConnection()'s checks are unchanged: that path is inherently single-connection (no blast-radius concern), and already goes through the safe never-throw + 'error' event path from the earlier rounds. Rewrote the three tests that asserted a construction-time throw (route-protocol.spec.ts x2, proxy-protocol-v2.spec.ts x1) to instead start the proxy and confirm the bad route's SNI is unreachable (matching the existing style in h2-dispatch.spec.ts's "(route dropped)" tests for the xForwardedFor+http2 case). Updated README.md and ts/types.ts's protocol JSDoc, which both still said "fails at construction," to describe isolation instead. cargo build/clippy clean, cargo test 112/112, npm test 114/114. Co-Authored-By: Claude Sonnet 5 --- README.md | 10 +-- __test__/proxy-protocol-v2.spec.ts | 52 ++++++++------- __test__/route-protocol.spec.ts | 100 +++++++++++++++++------------ src/proxy.rs | 24 ++----- src/router.rs | 22 +++++++ ts/types.ts | 9 ++- 6 files changed, 129 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index c1b5206..6d7118a 100644 --- a/README.md +++ b/README.md @@ -374,11 +374,13 @@ Use `sourceAddressHeader` on a route to control how the real client IP is commun } ``` -`protocol` defaults to `'opaque'` — a route for a non-HTTP application protocol (MQTT, or any other raw TCP/TLS protocol), limited to the PROXY-protocol carriers, which work on any byte stream. The declaration exists because ALPN can't stand in for it: a native protocol that negotiates no ALPN (MQTT does not) is indistinguishable at the TLS layer from an HTTPS client that simply didn't offer one. A route that requests a header-injection mode without declaring `protocol: 'http'` fails at construction with a descriptive error — it never silently stops injecting the header, since a backend that silently sees the wrong (or no) client IP is worse than a config that fails to build. +`protocol` defaults to `'opaque'` — a route for a non-HTTP application protocol (MQTT, or any other raw TCP/TLS protocol), limited to the PROXY-protocol carriers, which work on any byte stream. The declaration exists because ALPN can't stand in for it: a native protocol that negotiates no ALPN (MQTT does not) is indistinguishable at the TLS layer from an HTTPS client that simply didn't offer one. A route that requests a header-injection mode without declaring `protocol: 'http'` is rejected with a descriptive error — it never silently stops injecting the header, since a backend that silently sees the wrong (or no) client IP is worse than a route that refuses to serve traffic at all. -This declaration only helps when a header could actually be injected in the first place. A **passthrough** route (`terminateTls: false`) never decrypts the stream, so a header-carried mode has no carrier at all regardless of `protocol` — declaring `'http'` on a passthrough route wouldn't make it work, and fails construction with a distinct "no carrier" error instead of steering you toward a declaration that can't help. Use `sourceAddressHeader: 'proxyProtocolV2'` there instead (it works on any byte stream, passthrough included). +This declaration only helps when a header could actually be injected in the first place. A **passthrough** route (`terminateTls: false`) never decrypts the stream, so a header-carried mode has no carrier at all regardless of `protocol` — declaring `'http'` on a passthrough route wouldn't make it work, and is rejected with a distinct "no carrier" error instead of steering you toward a declaration that can't help. Use `sourceAddressHeader: 'proxyProtocolV2'` there instead (it works on any byte stream, passthrough included). -This is a breaking change for a hand-written route that already uses `sourceAddressHeader: 'xForwardedFor'` (or a header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration when upgrading. +Like a cert that fails to build, a route rejected for either reason is isolated to just that route: it's dropped (its SNI resolves to nothing) rather than failing `new SymphonyProxy()` or `updateConfig()` for every other route on the same port-set — one tenant's config mistake doesn't take the rest down with it. The rejection is still loud: it's logged (`symphony: skipping route '': ...`), and on a hot-swap the route's last-good version (if any) keeps serving until the config is fixed. + +This is a breaking change for a hand-written route that already uses `sourceAddressHeader: 'xForwardedFor'` (or a header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration when upgrading, or that route will stop serving traffic (silently, aside from the log line) rather than failing loudly at startup. ### PROXY protocol (default for UDS) @@ -436,7 +438,7 @@ The **carrier depends on `sourceAddressHeader`**: - With `'proxyProtocolV2'`, the fingerprint rides a PROXY v2 **TLV** — type `0xE0` for JA3, `0xE1` for JA4 (in HAProxy's `0xE0–0xEF` private range). This works even in passthrough (`terminateTls: false`), since the header prefixes the raw TLS bytes. No `protocol` declaration is needed — the TLV carries it regardless of the route's application protocol. - Otherwise, symphony injects an **`X-JA3` / `X-JA4` HTTP header**. This requires `protocol: 'http'` on the route (see [Declaring the route protocol](#declaring-the-route-protocol)) and a plaintext HTTP/1 upstream (`terminateTls: true` and not `http2`); it is skipped for HTTP/2 upstreams (use `'proxyProtocolV2'` there). For that HTTP/1 case, any client-supplied `X-JA3`/`X-JA4` is stripped so the injected value is authoritative and can't be spoofed — **this guarantee does not extend to an h2-negotiated connection on an `http2: true` route**: injection and stripping are both skipped there, so a client-supplied `X-JA3`/`X-JA4` reaches the upstream unmodified. Use `'proxyProtocolV2'` wherever h2 is possible. -A config that requests a header-carried `forwardFingerprint` without declaring `protocol: 'http'` fails at construction — the same fail-loud rule as `sourceAddressHeader: 'xForwardedFor'`. A config that requests `forwardFingerprint` with no viable carrier at all — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — also fails at construction, with a distinct "no carrier" error: no `protocol` declaration could fix a passthrough route's inability to inject a header, so it isn't steered toward one. A route that could have carried the header but silently won't for some connections — `http2: true`, where ALPN negotiation is per-connection and some clients may still land on HTTP/1 — logs a startup warning instead, since that outcome isn't guaranteed. +A config that requests a header-carried `forwardFingerprint` without declaring `protocol: 'http'` is rejected (and isolated to just that route) — the same fail-loud rule as `sourceAddressHeader: 'xForwardedFor'`. A config that requests `forwardFingerprint` with no viable carrier at all — passthrough (`terminateTls: false`) without `sourceAddressHeader: 'proxyProtocolV2'`, where there's neither an HTTP request to inject a header into nor a v2 TLV — is also rejected, with a distinct "no carrier" error: no `protocol` declaration could fix a passthrough route's inability to inject a header, so it isn't steered toward one. A route that could have carried the header but silently won't for some connections — `http2: true`, where ALPN negotiation is per-connection and some clients may still land on HTTP/1 — logs a startup warning instead, since that outcome isn't guaranteed. ```typescript // TLV carrier — works for any upstream that speaks PROXY v2, including passthrough diff --git a/__test__/proxy-protocol-v2.spec.ts b/__test__/proxy-protocol-v2.spec.ts index 2a32c34..2e3cdef 100644 --- a/__test__/proxy-protocol-v2.spec.ts +++ b/__test__/proxy-protocol-v2.spec.ts @@ -10,7 +10,7 @@ import assert from 'node:assert/strict'; import * as tls from 'node:tls'; import { after, before, describe, it } from 'node:test'; import { SymphonyProxy } from '../ts/proxy.js'; -import { generateSelfSignedCert, getFreePort, startCaptureServer, sleep } from './util.js'; +import { generateSelfSignedCert, getFreePort, startCaptureServer, tlsRoundTrip, sleep } from './util.js'; const PROXY_V2_SIGNATURE = Buffer.from([0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49, 0x54, 0x0a]); const PP2_TYPE_JA3 = 0xe0; @@ -164,28 +164,36 @@ describe('PROXY protocol v2 + fingerprint forwarding', () => { // Passthrough forwards raw TLS bytes to a TLS upstream — there's no decrypted HTTP request to // splice a header into, and a header-carried fingerprint mode has no carrier at all here - // regardless of `protocol`. This used to build successfully and silently forward nothing; - // it now fails construction with a "no carrier" error instead (see route-protocol.spec.ts for - // focused coverage), so a passthrough + header-carried forwardFingerprint config can no longer - // look "working" while quietly forwarding no fingerprint. - it('rejects a header-carried fingerprint on a passthrough route at construction (no carrier, not a silent no-op)', async () => { - assert.throws( - () => - new SymphonyProxy({ - listeners: [{ host: '127.0.0.1', port: 0 }], - routes: [ - { - sni: 'localhost', - upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: 1 }], - terminateTls: false, - forwardFingerprint: 'ja3', - protocol: 'http', - }, - ], - }), - /no carrier/i, - 'passthrough + header-carried forwardFingerprint must fail construction, not silently drop the fingerprint at runtime' + // regardless of `protocol`. This used to build successfully and silently forward nothing; it + // now rejects the route with a "no carrier" error instead (see route-protocol.spec.ts for + // focused coverage of the rejection itself), so a passthrough + header-carried forwardFingerprint + // config can no longer look "working" while quietly forwarding no fingerprint. Isolated per-route + // (build_route, router.rs) rather than failing the whole construction — a bad route's SNI simply + // resolves to nothing, same as a bad cert. + it('rejects a header-carried fingerprint on a passthrough route by dropping it (no carrier, not a silent no-op)', async () => { + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'localhost', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: 1 }], + terminateTls: false, + forwardFingerprint: 'ja3', + protocol: 'http', + }, + ], + }); + await proxy.start(); + await sleep(50); + + await assert.rejects( + tlsRoundTrip({ port: proxyPort, servername: 'localhost', caCert: cert.cert, data: 'x' }), + /timeout|ECONNRESET|EPROTO|socket hang up|closed/i, + 'passthrough + header-carried forwardFingerprint has no carrier — the route must be dropped, not silently built to forward no fingerprint' ); + + await proxy.stop(); }); // Finding 1 (Critical — Slowloris): a client that completes the TLS handshake and then stalls diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index a960e76..39b4907 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -150,25 +150,41 @@ describe('SymphonyProxy – route protocol declaration', () => { await capture.close(); }); - it('rejects xForwardedFor without a protocol: "http" declaration at construction time (fail loud, not a silent no-op)', async () => { - assert.throws( - () => - new SymphonyProxy({ - listeners: [{ host: '127.0.0.1', port: 0 }], - routes: [ - { - sni: 'mqtt.example.com', - upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: 1 }], - terminateTls: true, - cert: { certChain: cert.cert, privateKey: cert.key }, - sourceAddressHeader: 'xForwardedFor', - // protocol left unset — defaults to 'opaque', must be rejected, not silently accepted. - }, - ], - }), - /protocol/i, - 'construction must throw a descriptive error, not silently build a route that never injects the header' + // Undeclared xForwardedFor, and passthrough + header-carried forwardFingerprint (below), are + // both rejected — but isolated per-route (build_route, router.rs) rather than failing the whole + // `new SymphonyProxy()`/`updateConfig()` call, exactly like a bad cert or the existing + // xForwardedFor+http2 check: the bad route is dropped (its SNI resolves to nothing) and the + // rejection is logged, but other routes on the same port-set are unaffected. This was a + // deliberate design decision (see PR #40 review discussion): failing the entire call for one + // route's config mistake would let a single tenant's typo freeze config updates — or block the + // port from binding at all on a cold start — for every other tenant on the same port-set. + it('rejects xForwardedFor without a protocol: "http" declaration by dropping just that route (not the whole construction)', async () => { + const upstream = await startEchoServer(); + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'mqtt.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: upstream.port }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + sourceAddressHeader: 'xForwardedFor', + // protocol left unset — defaults to 'opaque', must be rejected, not silently accepted. + }, + ], + }); + await proxy.start(); + await sleep(50); + + await assert.rejects( + tlsRoundTrip({ port: proxyPort, servername: 'mqtt.example.com', caCert: cert.cert, data: 'x' }), + /timeout|ECONNRESET|EPROTO|socket hang up|closed/i, + 'the undeclared route must be dropped (no route for its SNI), not silently built with the header uninjected' ); + + await proxy.stop(); + await upstream.close(); }); // A passthrough route (terminateTls: false, e.g. an MQTT-over-TLS route) never decrypts the @@ -176,34 +192,34 @@ describe('SymphonyProxy – route protocol declaration', () => { // declaration can fix that. This must be rejected as a distinct "no carrier" error, not // steered toward `protocol: 'http'` (which is both semantically wrong for an opaque // passthrough route and, since header injection genuinely can't happen without termination, - // would still not make the config work). + // would still not make the config work) — and, like the case above, isolated per-route rather + // than failing the whole construction. it('rejects forwardFingerprint on a passthrough route as having no carrier, regardless of protocol declaration', async () => { - const baseRoute = { - sni: 'mqtt.example.com', - upstreams: [{ kind: 'tcp' as const, host: '127.0.0.1', port: 1 }], - terminateTls: false, - forwardFingerprint: 'ja3' as const, - }; + const upstream = await startEchoServer(); + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'mqtt.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: upstream.port }], + terminateTls: false, + forwardFingerprint: 'ja3', + protocol: 'http', // declaring 'http' must not paper over a passthrough route with no carrier + }, + ], + }); + await proxy.start(); + await sleep(50); - assert.throws( - () => - new SymphonyProxy({ - listeners: [{ host: '127.0.0.1', port: 0 }], - routes: [baseRoute], - }), - /no carrier/i, - 'passthrough + header-carried forwardFingerprint must fail construction with a "no carrier" error' + await assert.rejects( + tlsRoundTrip({ port: proxyPort, servername: 'mqtt.example.com', caCert: cert.cert, data: 'x' }), + /timeout|ECONNRESET|EPROTO|socket hang up|closed/i, + 'passthrough + header-carried forwardFingerprint has no carrier — the route must be dropped regardless of a protocol: "http" declaration' ); - assert.throws( - () => - new SymphonyProxy({ - listeners: [{ host: '127.0.0.1', port: 0 }], - routes: [{ ...baseRoute, protocol: 'http' }], - }), - /no carrier/i, - 'declaring protocol: "http" must not paper over a passthrough route with no header carrier' - ); + await proxy.stop(); + await upstream.close(); }); // resolveConnection() parses and validates its `route` argument only once the id is confirmed diff --git a/src/proxy.rs b/src/proxy.rs index 7d1bfb8..fef06e3 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -783,23 +783,13 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { let protocol = parse_route_protocol(r.protocol.as_deref())?; let requires_http = requires_http_protocol(source_address_mode, forward_fingerprint); - // Passthrough (terminateTls=false) never decrypts the stream, so a header-carried mode has - // no carrier at all regardless of `protocol` — declaring 'http' wouldn't help. This is - // distinct from (and checked before) the declaration requirement below: it's not that the - // route mislabeled its protocol, it's that no protocol declaration could make this work. - if requires_http && !r.terminate_tls { - return Err(napi::Error::from_reason(format!( - "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), has no carrier when terminateTls=false (passthrough never decrypts the stream, so no header can be injected) — use sourceAddressHeader='proxyProtocolV2' (carries both source address and fingerprint), or remove forwardFingerprint", - r.sni - ))); - } - - if requires_http && protocol != RouteProtocol::Http { - return Err(napi::Error::from_reason(format!( - "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch away from the header-carried mode: 'proxyProtocol'/'proxyProtocolV2' both work on any protocol for source-address forwarding, but only 'proxyProtocolV2' carries a fingerprint (v1 does not)", - r.sni - ))); - } + // The "no carrier" and "declare protocol: 'http'" checks live in `build_route` (router.rs), + // not here: they need to fail the same way a bad cert or the xForwardedFor+h2 combination + // does — isolate the one bad route (drop the SNI, or carry the last-good route forward on a + // hot-swap) rather than aborting the entire `new SymphonyProxy()`/`updateConfig()` call and + // taking every other route on the port-set down with it. `parse_route_spec` runs inside a + // `.collect::>>()?` across all routes, so an error returned from here has no + // such isolation — only `build_route_table` provides it. let spec = RouteSpec { sni: r.sni.clone(), diff --git a/src/router.rs b/src/router.rs index 9a5aba2..8db7fe2 100644 --- a/src/router.rs +++ b/src/router.rs @@ -399,6 +399,28 @@ fn build_route( listener_tls: &ListenerTlsSpec, cache: &mut TlsConfigCache, ) -> crate::error::Result { + let requires_http = requires_http_protocol(spec.source_address_mode, spec.forward_fingerprint); + + // Passthrough (terminateTls=false) never decrypts the stream, so a header-carried mode has no + // carrier at all regardless of `protocol` — declaring 'http' wouldn't help. Checked before (and + // distinct from) the declaration requirement below: it's not that the route mislabeled its + // protocol, it's that no protocol declaration could make this work. Isolated here (rather than + // at parse time in proxy.rs) so one route's misconfiguration is dropped/last-good-carried like + // a bad cert, instead of aborting construction or updateConfig for every other route. + if requires_http && !spec.terminate_tls { + return Err(crate::error::SymphonyError::Config(format!( + "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), has no carrier when terminateTls=false (passthrough never decrypts the stream, so no header can be injected) — use sourceAddressHeader='proxyProtocolV2' (carries both source address and fingerprint), or remove forwardFingerprint", + spec.sni + ))); + } + + if requires_http && spec.protocol != RouteProtocol::Http { + return Err(crate::error::SymphonyError::Config(format!( + "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), requires protocol: 'http' — declare it explicitly, or switch away from the header-carried mode: 'proxyProtocol'/'proxyProtocolV2' both work on any protocol for source-address forwarding, but only 'proxyProtocolV2' carries a fingerprint (v1 does not)", + spec.sni + ))); + } + let tls_config = if spec.terminate_tls { let cert_pem = spec .cert_pem diff --git a/ts/types.ts b/ts/types.ts index 0d04034..0d0ca23 100644 --- a/ts/types.ts +++ b/ts/types.ts @@ -143,9 +143,12 @@ export interface RouteConfig { * ALPN cannot stand in for this declaration: a native non-HTTP client that offers no * ALPN is indistinguishable from an HTTPS client that simply didn't offer one, so a * route requesting a header-injection mode without declaring `protocol: 'http'` is a - * config error, not a route that quietly stops injecting the header. This is a breaking - * change for a hand-written route using `sourceAddressHeader: 'xForwardedFor'` (or a - * header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration. + * config error, not a route that quietly stops injecting the header — the route is + * rejected and dropped (isolated the same way a route with a bad cert is, not failing + * `new SymphonyProxy()`/`updateConfig()` for every other route on the port-set). This is + * a breaking change for a hand-written route using `sourceAddressHeader: 'xForwardedFor'` + * (or a header-carried `forwardFingerprint`) without `protocol: 'http'` — add the + * declaration, or that route will stop serving traffic on upgrade. */ protocol?: 'http' | 'opaque'; } From b65a1ca4fb2a027f47044bfc01955df649b3a5ad Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 13:51:10 -0600 Subject: [PATCH 11/12] Fix a fail-open route-isolation bug; fix removable-listener error hole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 7 (codex + gemini + grok + harper-domain) reviewed the isolation change Kris just directed and found a real, serious correctness bug it exposed: RouteTable::resolve() falls through exact → wildcard → default, and build_route_table's isolation path drops a rejected route with a plain `continue` — it's never inserted into `exact` or `wildcard`. A rejected route with an exact SNI shadowed by a wildcard (or a rejected wildcard itself) therefore does NOT resolve to nothing as documented; it silently misroutes to whatever wildcard/default would otherwise have caught it. This was always latent for cert failures, but this PR turns route rejection from a rare cert-rotation race into a routine outcome of a plain config omission, making it far more likely to actually be hit: a mistyped/missing `protocol: 'http'` on one tenant's route could send that tenant's traffic to an unrelated co-tenant's upstream with no visible error. Fixed by tracking rejected SNIs (exact and wildcard, separately, since a wildcard's suffix-match needs the same matching rule as valid wildcards) and checking them first in resolve() — a rejected route now returns None outright rather than falling through. Factored the suffix-match into a standalone `wildcard_suffix_matches` helper so both the valid and rejected paths share it. Added two Rust unit tests reproducing the exact scenario (dropped exact route shadowed by a valid wildcard; a dropped wildcard route itself) confirming both now fail closed. Also this round: - Replaced the constructor's permanent default 'error' listener (round 6) with a per-emission-site listenerCount check (ts/proxy.ts): the permanent listener could be removed by a consumer's own removeAllListeners('error'), reopening the exact crash it existed to prevent, and it logged every error even when a consumer's own listener already handled it. Checking at each emit call site closes both: emit('error', ...) can never be reached with zero listeners, and a consumer's own listener suppresses the log entirely rather than getting a redundant copy. - Updated the "no listener attached" regression test to drive a real resolveConnection() failure with zero listeners and assert on the logged output, since the previous version asserted on a since-removed constructor listener and called .emit() directly, bypassing the actual code path under test. - Added a two-route isolation test proving a healthy co-tenant route keeps serving traffic while a sibling route is rejected — the previous isolation tests only proved the bad route's own SNI was unreachable, which a regression that dropped the whole table would also satisfy. - Reworded the h2-fingerprint-warning comment (it previously conflated "not forwarded" with "forwarded but forgeable" — the latter is the actual risk) and the failingRoutes/symphony_routes_failing docs, which still described only cert failures after this PR added a second rejection class to the same counter. cargo build/clippy clean, cargo test 114/114, npm test 115/115. Co-Authored-By: Claude Sonnet 5 --- __test__/route-protocol.spec.ts | 47 +++++++++++++++ __test__/suspended.spec.ts | 68 +++++++++++++++++----- src/proxy.rs | 14 +++-- src/router.rs | 100 +++++++++++++++++++++++++++----- ts/addon.d.ts | 6 +- ts/admin.ts | 2 +- ts/proxy.ts | 30 ++++++---- ts/types.ts | 6 +- 8 files changed, 226 insertions(+), 47 deletions(-) diff --git a/__test__/route-protocol.spec.ts b/__test__/route-protocol.spec.ts index 39b4907..21838b0 100644 --- a/__test__/route-protocol.spec.ts +++ b/__test__/route-protocol.spec.ts @@ -222,6 +222,53 @@ describe('SymphonyProxy – route protocol declaration', () => { await upstream.close(); }); + // The two tests above prove the bad route's own SNI is unreachable, but that's also what a + // regression that dropped the *whole table* (defeating the isolation this PR exists to + // preserve) would look like. This test proves the isolation itself: a healthy co-tenant route + // on the same port-set must keep serving traffic while an unrelated sibling route is rejected. + it('a healthy co-tenant route keeps serving traffic when a sibling route is rejected', async () => { + const goodUpstream = await startEchoServer(); + const badUpstream = await startEchoServer(); + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'good.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: goodUpstream.port }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + // A plain opaque route — no header-carried mode configured, so it round-trips raw + // bytes and isn't itself subject to the check under test. The point of this route + // is just to prove it keeps working, not to re-exercise HTTP header injection. + }, + { + sni: 'bad.example.com', + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: badUpstream.port }], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + sourceAddressHeader: 'xForwardedFor', + // protocol left unset — rejected, but must not take good.example.com down with it. + }, + ], + }); + await proxy.start(); + await sleep(50); + + await assert.rejects( + tlsRoundTrip({ port: proxyPort, servername: 'bad.example.com', caCert: cert.cert, data: 'x' }), + /timeout|ECONNRESET|EPROTO|socket hang up|closed/i, + 'the undeclared sibling route must still be dropped' + ); + + const response = await tlsRoundTrip({ port: proxyPort, servername: 'good.example.com', caCert: cert.cert, data: 'x' }); + assert.deepEqual(response, Buffer.from('x'), 'the healthy co-tenant route must round-trip normally, unaffected by the rejected sibling'); + + await proxy.stop(); + await goodUpstream.close(); + await badUpstream.close(); + }); + // resolveConnection() parses and validates its `route` argument only once the id is confirmed // still live (an id that already timed out is a documented no-op — SuspendedRegistry::contains // — so it never reaches route validation at all). These tests therefore need a *real* suspended diff --git a/__test__/suspended.spec.ts b/__test__/suspended.spec.ts index b8c54d5..f923d4e 100644 --- a/__test__/suspended.spec.ts +++ b/__test__/suspended.spec.ts @@ -233,20 +233,60 @@ describe('Suspended routes – reject with null', () => { // Node's EventEmitter special-cases 'error': emitting it with zero listeners attached throws // synchronously instead of dropping the event. Every 'error' emission in SymphonyProxy happens -// inside the napi threadsafe-function callback, so that throw would otherwise escape into native -// code and crash the whole process — not just this instance — the very first time ANY native -// error fires (a resolveConnection() validation failure, a TLS error, anything) on a proxy whose -// owner hasn't gotten around to attaching an 'error' listener yet. The constructor installs a -// permanent default listener specifically to make this impossible; this test pins that a fresh -// proxy with zero listeners of its own survives an 'error' emission. -describe('SymphonyProxy never crashes from emit("error") with no listener attached', () => { - it('a proxy with no "error" listener at all does not throw when an error is emitted', () => { - const proxy = new SymphonyProxy({ listeners: [{ host: '127.0.0.1', port: 0 }], routes: [] }); - assert.equal(proxy.listenerCount('error'), 1, 'the constructor must install exactly one default listener'); - assert.doesNotThrow( - () => proxy.emit('error', new Error('simulated native error, no consumer listener attached')), - 'emit("error", ...) must never throw for lack of a listener — this is the crash the round-5 review flagged as a blocker' - ); +// inside the napi threadsafe-function callback (deferred one tick), so that throw would otherwise +// escape as an uncaught exception the very first time ANY native error fires (a resolveConnection() +// validation failure, a TLS error, anything) on a proxy whose owner hasn't gotten around to +// attaching an 'error' listener yet. Rather than a permanent listener (which a consumer's own +// `removeAllListeners('error')` would remove, reopening the hole), every 'error' emission site +// checks listenerCount first and logs directly when nothing is listening — this test drives a +// real validation failure through resolveConnection() with zero listeners attached and confirms +// it logs instead of crashing. +describe('SymphonyProxy never crashes emitting "error" with no listener attached', () => { + const cert = generateSelfSignedCert('localhost'); + + it('logs to stderr instead of throwing when a background error fires with no "error" listener', async () => { + const proxyPort = await getFreePort(); + const proxy = new SymphonyProxy({ + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + { + sni: 'localhost', + upstreams: [], + terminateTls: true, + cert: { certChain: cert.cert, privateKey: cert.key }, + suspended: true, + suspendTimeoutMs: 5000, + }, + ], + }); + assert.equal(proxy.listenerCount('error'), 0, 'no default listener should be pre-installed'); + + const originalConsoleError = console.error; + const logged: unknown[][] = []; + console.error = (...args: unknown[]) => logged.push(args); + + proxy.on('suspended', (conn) => { + // Undeclared xForwardedFor — rejected by parse_resolve_spec, no 'error' listener attached. + proxy.resolveConnection(conn.id, { + upstream: { kind: 'tcp', host: '127.0.0.1', port: 1 }, + terminateTls: true, + sourceAddressHeader: 'xForwardedFor', + }); + }); + + await proxy.start(); + await sleep(50); + + const socket = startTlsSocket(proxyPort, 'localhost', cert.cert); + socket.on('error', () => {}); + await sleep(200); + + console.error = originalConsoleError; + await proxy.stop(); + + assert.equal(logged.length, 1, 'the validation failure must be logged exactly once, not thrown'); + assert.match(String(logged[0][0]), /unhandled proxy error/i); + assert.match(String(logged[0][1]), /protocol/i); }); }); diff --git a/src/proxy.rs b/src/proxy.rs index fef06e3..ea6dd1d 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -202,7 +202,9 @@ pub struct JsProxyMetrics { pub suspended_unresolved: f64, /// Routes currently in the live table, including the default route. pub routes: f64, - /// Routes whose cert failed to build — dropped, or serving a carried-forward last-good cert. + /// Routes rejected at build time — a cert that failed to build, or a route that failed the + /// protocol/carrier validation — either dropped, or (cert failures only) serving a + /// carried-forward last-good cert. pub failing_routes: f64, pub listeners: Vec, } @@ -815,11 +817,11 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { // xForwardedFor + http2 is a hard error (build_route, router.rs) since it's unconditionally // unsafe — an h2 client's XFF would be neither injected nor stripped, forwarding an arbitrary // client-supplied value as if authoritative. A header-carried forwardFingerprint in the same - // spot is intentionally only a warning, not a hard error: unlike XFF, which no route needs at - // all if it drops h2, some deployments accept "an h2 client can choose to not have its - // fingerprint forwarded (or forward its own)" as a known best-effort limitation of a signal - // that's advisory in the first place. Name whichever mode actually triggered this so the - // message doesn't blame X-Forwarded-For when the route never configured it. + // spot is intentionally only a warning here, not a hard error, even though the risk is the + // same in kind (an h2 client can forge X-JA3/X-JA4, not merely go unforwarded) — see + // README.md's "Forwarding the fingerprint downstream" section for the tradeoff this leaves + // open and why it isn't a hard error like XFF. Name whichever mode actually triggered this so + // the message doesn't blame X-Forwarded-For when the route never configured it. if requires_http && spec.http2 { let mode_desc = if source_address_mode == SourceAddressMode::XForwardedFor { "xForwardedFor" diff --git a/src/router.rs b/src/router.rs index 8db7fe2..258a74e 100644 --- a/src/router.rs +++ b/src/router.rs @@ -180,6 +180,19 @@ pub struct Route { // ── Route table ─────────────────────────────────────────────────────────────── +/// True when `sni` matches exactly one left-most label against `suffix` (a wildcard's stored +/// suffix, without the `*.` prefix) — e.g. "foo.example.com" matches suffix "example.com" but +/// "a.b.example.com" does not. +fn wildcard_suffix_matches(sni: &str, suffix: &str) -> bool { + if sni.len() <= suffix.len() + 1 { + return false; + } + let dot_pos = sni.len() - suffix.len() - 1; + let rest = &sni[sni.len() - suffix.len()..]; + let prefix = &sni[..dot_pos]; + !prefix.contains('.') && rest == suffix && sni.as_bytes()[dot_pos] == b'.' +} + pub struct RouteTable { exact: HashMap, Route>, /// (suffix_without_star_dot, route) — "*.example.com" stored as "example.com" @@ -191,6 +204,18 @@ pub struct RouteTable { /// SNIs whose cert failed to build in this table. Carried across a hot-swap so a /// persistently-broken route is logged only on the good→bad transition, not every reconcile. failing_snis: HashSet>, + /// Exact SNIs dropped with no last-good route to carry forward (a subset of `failing_snis` — + /// this excludes SNIs whose last-good route IS still present in `exact`/`wildcard` and + /// serving traffic). `resolve()` must fail closed for these rather than falling through to a + /// wildcard or default route: a route absent from `exact` because it was rejected is not the + /// same as a route that was simply never configured, and letting the former silently match a + /// broader wildcard would misroute that tenant's traffic to an unrelated upstream instead of + /// refusing the connection. + dropped_exact: HashSet>, + /// Wildcard suffixes (without the `*.` prefix) dropped with no last-good route — same + /// fail-closed reasoning as `dropped_exact`, checked against incoming SNIs with the same + /// suffix-matching rule as `wildcard` itself. + dropped_wildcard: Vec>, } impl RouteTable { @@ -210,6 +235,14 @@ impl RouteTable { return self.default.as_ref(); }; + // A route rejected outright (no last-good to carry forward) must fail closed, not fall + // through to a broader wildcard or the default route: it was configured for this SNI, just + // rejected, which is not the same as "no route was ever configured here." Checked before + // any match below, exact or wildcard, so a poisoned entry always wins over a fallback. + if self.dropped_exact.contains(sni) || self.dropped_wildcard.iter().any(|suffix| wildcard_suffix_matches(sni, suffix)) { + return None; + } + // Exact match first if let Some(r) = self.exact.get(sni) { return Some(r); @@ -218,16 +251,8 @@ impl RouteTable { // Wildcard: match exactly one left-most label against stored suffixes. // e.g. "foo.example.com" matches "*.example.com" but "a.b.example.com" does not. for (suffix, route) in &self.wildcard { - if sni.len() > suffix.len() + 1 { - let dot_pos = sni.len() - suffix.len() - 1; - let rest = &sni[sni.len() - suffix.len()..]; - let prefix = &sni[..dot_pos]; - if !prefix.contains('.') - && rest == suffix.as_ref() - && sni.as_bytes()[dot_pos] == b'.' - { - return Some(route); - } + if wildcard_suffix_matches(sni, suffix) { + return Some(route); } } @@ -330,6 +355,8 @@ pub fn build_route_table( let mut wildcard: Vec<(Arc, Route)> = Vec::new(); let mut monitored_balancers: Vec> = Vec::new(); let mut failing_snis: HashSet> = HashSet::new(); + let mut dropped_exact: HashSet> = HashSet::new(); + let mut dropped_wildcard: Vec> = Vec::new(); for spec in specs { // Isolate per-route failures: a single route whose cert can't be built (e.g. a @@ -360,13 +387,21 @@ pub fn build_route_table( } prev.clone() } - // No prior route (initial build, or a newly-added route): drop this SNI. The - // missing route simply resolves to nothing — strictly better than a host-wide - // abort that would take down every other tenant on the listener. + // No prior route (initial build, or a newly-added route): drop this SNI. It must + // resolve to nothing, not silently fall through to a broader wildcard or the + // default route — this SNI *was* configured, just rejected, which is a different + // (and worse, if left unhandled) case than "no route was ever set up for it": a + // dropped exact route shadowed by an unrelated wildcard would otherwise misroute + // that tenant's traffic to a different upstream with no visible error. None => { if newly_failing { eprintln!("symphony: skipping route '{}': {}", spec.sni, e); } + if let Some(suffix) = spec.sni.strip_prefix("*.") { + dropped_wildcard.push(Arc::from(suffix)); + } else { + dropped_exact.insert(Arc::from(spec.sni.as_str())); + } continue; } } @@ -391,7 +426,7 @@ pub fn build_route_table( } } - Ok(RouteTable { exact, wildcard, default: None, monitored_balancers, failing_snis }) + Ok(RouteTable { exact, wildcard, default: None, monitored_balancers, failing_snis, dropped_exact, dropped_wildcard }) } fn build_route( @@ -739,6 +774,43 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== ); } + // A dropped route must fail closed, not silently fall through to a broader wildcard: it was + // configured for this SNI, just rejected — very different from "no route was ever set up + // here." Without this, a rejected exact route shadowed by a wildcard would misroute that + // tenant's traffic to the wildcard's (unrelated) upstream with no visible error. + #[test] + fn dropped_exact_route_does_not_fall_through_to_wildcard() { + let specs = vec![ + tls_route("api.acme.com", CERT_A, KEY_B), // KeyMismatch — dropped, no last-good + tls_route("*.acme.com", CERT_A, KEY_A), + ]; + + let table = build_route_table(&specs, &ListenerTlsSpec::empty(), None).expect("build"); + + assert!( + table.resolve(Some("api.acme.com")).is_none(), + "a dropped exact route must resolve to nothing, not the wildcard's upstream" + ); + assert!( + table.resolve(Some("other.acme.com")).is_some(), + "an unrelated subdomain must still reach the (valid) wildcard route" + ); + } + + // Same fail-closed requirement when the *wildcard* itself is the one that's dropped: a + // matching subdomain must not fall through to the default route either. + #[test] + fn dropped_wildcard_route_does_not_fall_through_to_default() { + let specs = vec![tls_route("*.acme.com", CERT_A, KEY_B)]; // KeyMismatch — dropped + + let table = build_route_table(&specs, &ListenerTlsSpec::empty(), None).expect("build"); + + assert!( + table.resolve(Some("api.acme.com")).is_none(), + "a subdomain matching a dropped wildcard must resolve to nothing, not fall through to default" + ); + } + // On a hot-swap, a route whose cert transiently fails to rebuild (the normal cert+key // non-atomic write window) must retain its last-good route from the live table rather // than dropping the SNI. diff --git a/ts/addon.d.ts b/ts/addon.d.ts index c6a620d..6f11f41 100644 --- a/ts/addon.d.ts +++ b/ts/addon.d.ts @@ -168,7 +168,11 @@ export interface JsProxyMetrics { suspendedUnresolved: number /** Routes currently in the live table, including the default route. */ routes: number - /** Routes whose cert failed to build — dropped, or serving a carried-forward last-good cert. */ + /** + * Routes rejected at build time — a cert that failed to build, or a route that failed the + * protocol/carrier validation — either dropped, or (cert failures only) serving a + * carried-forward last-good cert. + */ failingRoutes: number listeners: Array } diff --git a/ts/admin.ts b/ts/admin.ts index 8efe315..64b8efe 100644 --- a/ts/admin.ts +++ b/ts/admin.ts @@ -146,7 +146,7 @@ export function renderPrometheus(snapshot: MetricsSnapshot): string { out.sample( 'symphony_routes_failing', 'gauge', - 'Routes whose cert failed to build — dropped, or serving a carried-forward last-good cert.', + 'Routes rejected at build time (a cert that failed to build, or a route that failed protocol/carrier validation) — dropped, or (cert failures only) serving a carried-forward last-good cert.', metrics.failingRoutes, proxy ); diff --git a/ts/proxy.ts b/ts/proxy.ts index cc9b755..85b554f 100644 --- a/ts/proxy.ts +++ b/ts/proxy.ts @@ -165,14 +165,6 @@ export class SymphonyProxy extends EventEmitter { constructor(config: ProxyConfig) { super(); - // A default 'error' listener so a background failure (e.g. a resolveConnection() validation - // rejection — see resolve_connection in proxy.rs) is never silent for a consumer who hasn't - // gotten around to attaching their own listener yet. It logs rather than swallows: an - // operator should be able to tell a suspended route is failing without a proxy.on('error') - // wired up, and any listener the consumer does attach still fires normally alongside this one. - this.on('error', (err: Error, ctx?: { listener?: string }) => { - console.error(`symphony: unhandled proxy error${ctx?.listener ? ` [${ctx.listener}]` : ''}:`, err); - }); const { SymphonyProxyWrap: Wrap } = loadAddon(); const jsConfig = { @@ -202,7 +194,7 @@ export class SymphonyProxy extends EventEmitter { // general defense-in-depth grounds rather than a reproduced local crash. process.nextTick(() => { if (err) { - this.emit('error', err); + this._emitError(err); return; } const event = raw as ProxyEvent; @@ -226,13 +218,31 @@ export class SymphonyProxy extends EventEmitter { } satisfies SuspendedConnection); break; case 'error': - this.emit('error', new Error(event.message), { listener: event.listener }); + this._emitError(new Error(event.message), { listener: event.listener }); break; } }); }); } + // EventEmitter throws synchronously when 'error' is emitted with zero listeners attached — a + // permanent default listener installed in the constructor would guard against that only until + // a consumer calls removeAllListeners('error') (or with no args), which reopens the hole a + // pre-installed listener can't defend against from the inside. Checking listenerCount at each + // emission site instead means there is never a moment where emit('error', ...) can throw for + // lack of a listener, regardless of what the consumer does to their listeners in between. When + // nothing is listening, log directly rather than swallowing: an operator should be able to tell + // a background failure (e.g. a resolveConnection() validation rejection) happened without + // having wired up their own 'error' listener, and this never fires redundantly alongside a + // listener that already handles it — unlike a permanent listener, which logged every time. + private _emitError(err: Error, ctx?: { listener?: string }): void { + if (this.listenerCount('error') > 0) { + this.emit('error', err, ctx); + } else { + console.error(`symphony: unhandled proxy error${ctx?.listener ? ` [${ctx.listener}]` : ''}:`, err); + } + } + async start(): Promise { if (this._started) return; this._started = true; diff --git a/ts/types.ts b/ts/types.ts index 0d0ca23..30047df 100644 --- a/ts/types.ts +++ b/ts/types.ts @@ -341,7 +341,11 @@ export interface ProxyMetrics { suspendedUnresolved: number; /** Routes in the live table, including the default route. */ routes: number; - /** Routes whose cert failed to build — dropped, or serving a carried-forward last-good cert. */ + /** + * Routes rejected at build time — a cert that failed to build, or a route that failed the + * protocol/carrier validation — either dropped, or (cert failures only) serving a + * carried-forward last-good cert. + */ failingRoutes: number; /** Per-listener breakdown, in configuration order. */ listeners: ListenerMetrics[]; From 1ada5c03e1b9fbd2ae0c7bd2b2cfc4b2be36454e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 30 Jul 2026 14:14:14 -0600 Subject: [PATCH 12/12] Fix a real regression from my own isolation fix: dropped wildcard black-holed exact routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8 (codex + gemini + grok + harper-domain) reviewed the prior commit's route-isolation fix and found I'd introduced exactly the class of bug it was meant to prevent, just inverted: RouteTable::resolve() checked dropped_wildcard before the exact-match lookup, so any SNI matching a dropped wildcard's suffix returned None immediately — including a perfectly healthy, fully-built exact route at that same SNI. One tenant's wildcard misconfiguration (e.g. `*.acme.com` missing `protocol: 'http'`) would black-hole every co-tenant with an exact route under that suffix (`api.acme.com`, ACME challenge routes, etc.), which is worse than the fall-through bug being fixed. Root cause: I checked both dropped sets before any live lookup, on the theory that "a poison entry should always win." That's wrong — a poison entry must only suppress a fallback *broader* than itself, never something *more specific*. Fixed by checking exact (live, then dropped) before moving to wildcard (live, then dropped) before default — each dropped set immediately follows its same-specificity live counterpart, so an exact match always wins regardless of wildcard state. Added a Rust unit test reproducing the exact scenario (healthy exact route + dropped sibling wildcard) that fails against the previous ordering and passes against this one — verified by reverting and reconfirming the failure before restoring. Also from round 8: last-good carry-forward (designed for a transient cert-rotation race that heals on the next reconcile) was reaching the new protocol/no-carrier rejections too, which are permanent config errors that never heal — so a route could silently keep serving a stale, previous version indefinitely across every later reconcile, with the operator's `updateConfig` reporting success while their edit quietly never took effect. Extracted the two protocol/carrier checks into a standalone validate_route_protocol_declaration(), called before build_route in the build_route_table loop, and given its own never-carry-forward path — a permanent rejection now always drops the route on a hot-swap, same as on initial build. Added a test for this too. Also narrowed the http2 warning in parse_route_spec: it fired for both xForwardedFor and forwardFingerprint, but xForwardedFor+http2 is always a hard rejection (build_route), so that branch was dead and misleadingly implied the route degrades gracefully when it's actually dropped outright. cargo build/clippy clean, cargo test 116/116, npm test 115/115. Co-Authored-By: Claude Sonnet 5 --- src/proxy.rs | 21 ++++----- src/router.rs | 118 ++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 110 insertions(+), 29 deletions(-) diff --git a/src/proxy.rs b/src/proxy.rs index ea6dd1d..5b20416 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -816,18 +816,15 @@ fn parse_route_spec(r: &JsRouteConfig) -> Result { } // xForwardedFor + http2 is a hard error (build_route, router.rs) since it's unconditionally // unsafe — an h2 client's XFF would be neither injected nor stripped, forwarding an arbitrary - // client-supplied value as if authoritative. A header-carried forwardFingerprint in the same - // spot is intentionally only a warning here, not a hard error, even though the risk is the - // same in kind (an h2 client can forge X-JA3/X-JA4, not merely go unforwarded) — see - // README.md's "Forwarding the fingerprint downstream" section for the tradeoff this leaves - // open and why it isn't a hard error like XFF. Name whichever mode actually triggered this so - // the message doesn't blame X-Forwarded-For when the route never configured it. - if requires_http && spec.http2 { - let mode_desc = if source_address_mode == SourceAddressMode::XForwardedFor { - "xForwardedFor" - } else { - "a header-carried forwardFingerprint (X-JA3/X-JA4)" - }; + // client-supplied value as if authoritative — so it's excluded here: this warning would always + // be immediately followed by that hard rejection, misleadingly implying the route still works + // (minus the header) when the whole route is in fact dropped. A header-carried + // forwardFingerprint in the same spot is intentionally only a warning, not a hard error, even + // though the risk is the same in kind (an h2 client can forge X-JA3/X-JA4, not merely go + // unforwarded) — see README.md's "Forwarding the fingerprint downstream" section for the + // tradeoff this leaves open and why it isn't a hard error like XFF. + if requires_http && spec.http2 && source_address_mode != SourceAddressMode::XForwardedFor { + let mode_desc = "a header-carried forwardFingerprint (X-JA3/X-JA4)"; eprintln!( "symphony: route '{}': {mode_desc} has no effect for any client that negotiates h2 (http2=true) — injection and client-supplied-header stripping are both skipped, so an h2 client's own value reaches the upstream unmodified; consider sourceAddressHeader='proxyProtocolV2'", spec.sni diff --git a/src/router.rs b/src/router.rs index 258a74e..c9d96d6 100644 --- a/src/router.rs +++ b/src/router.rs @@ -236,25 +236,32 @@ impl RouteTable { }; // A route rejected outright (no last-good to carry forward) must fail closed, not fall - // through to a broader wildcard or the default route: it was configured for this SNI, just - // rejected, which is not the same as "no route was ever configured here." Checked before - // any match below, exact or wildcard, so a poisoned entry always wins over a fallback. - if self.dropped_exact.contains(sni) || self.dropped_wildcard.iter().any(|suffix| wildcard_suffix_matches(sni, suffix)) { - return None; - } - - // Exact match first + // through to a broader fallback: it was configured for this SNI, just rejected, which is + // not the same as "no route was ever configured here." But a poison entry must only ever + // suppress a fallback *broader* than itself — it must never shadow a route that is more + // specific than the poison. So each dropped set is checked immediately after its + // same-specificity live counterpart, not before it: a healthy exact route always wins over + // a dropped wildcard at the same suffix (checking dropped_wildcard first would incorrectly + // black-hole every exact route under a wildcard that happened to fail validation). + + // Exact, live or dropped — most specific, decided first. if let Some(r) = self.exact.get(sni) { return Some(r); } + if self.dropped_exact.contains(sni) { + return None; + } - // Wildcard: match exactly one left-most label against stored suffixes. + // Wildcard, live or dropped — match exactly one left-most label against stored suffixes. // e.g. "foo.example.com" matches "*.example.com" but "a.b.example.com" does not. for (suffix, route) in &self.wildcard { if wildcard_suffix_matches(sni, suffix) { return Some(route); } } + if self.dropped_wildcard.iter().any(|suffix| wildcard_suffix_matches(sni, suffix)) { + return None; + } self.default.as_ref() } @@ -359,6 +366,29 @@ pub fn build_route_table( let mut dropped_wildcard: Vec> = Vec::new(); for spec in specs { + // The protocol/no-carrier declaration checks are validated before any cert/TLS work, and + // deliberately never carried forward on a hot-swap even if a last-good route exists: unlike + // a cert failure (a transient race between two non-atomic file writes that heals on its own + // next reconcile), a missing declaration or a passthrough-with-no-carrier config is a + // permanent config error — it never heals without a human editing the config. Applying + // cert-style carry-forward to it would silently keep serving the *previous* route + // indefinitely, including ignoring any other change (a new upstream, a cert rotation) the + // same reconcile made to that route — the operator's `updateConfig` reports success while + // their edit quietly never took effect. + if let Err(e) = validate_route_protocol_declaration(spec) { + let newly_failing = previous.is_none_or(|p| !p.failing_snis.contains(spec.sni.as_str())); + failing_snis.insert(Arc::from(spec.sni.as_str())); + if newly_failing { + eprintln!("symphony: skipping route '{}': {}", spec.sni, e); + } + if let Some(suffix) = spec.sni.strip_prefix("*.") { + dropped_wildcard.push(Arc::from(suffix)); + } else { + dropped_exact.insert(Arc::from(spec.sni.as_str())); + } + continue; + } + // Isolate per-route failures: a single route whose cert can't be built (e.g. a // rotated key no longer matching an inlined chain → rustls KeyMismatch) must not // abort the whole table and take every other tenant on the port down with it. @@ -429,19 +459,16 @@ pub fn build_route_table( Ok(RouteTable { exact, wildcard, default: None, monitored_balancers, failing_snis, dropped_exact, dropped_wildcard }) } -fn build_route( - spec: &RouteSpec, - listener_tls: &ListenerTlsSpec, - cache: &mut TlsConfigCache, -) -> crate::error::Result { +/// The "no carrier" and "declare protocol: 'http'" requirements — checked in `build_route_table` +/// before `build_route` runs, and deliberately never carried forward on a hot-swap (see the call +/// site): unlike a cert-build failure, this is a permanent config error, not a transient race. +fn validate_route_protocol_declaration(spec: &RouteSpec) -> crate::error::Result<()> { let requires_http = requires_http_protocol(spec.source_address_mode, spec.forward_fingerprint); // Passthrough (terminateTls=false) never decrypts the stream, so a header-carried mode has no // carrier at all regardless of `protocol` — declaring 'http' wouldn't help. Checked before (and // distinct from) the declaration requirement below: it's not that the route mislabeled its - // protocol, it's that no protocol declaration could make this work. Isolated here (rather than - // at parse time in proxy.rs) so one route's misconfiguration is dropped/last-good-carried like - // a bad cert, instead of aborting construction or updateConfig for every other route. + // protocol, it's that no protocol declaration could make this work. if requires_http && !spec.terminate_tls { return Err(crate::error::SymphonyError::Config(format!( "route '{}': sourceAddressHeader 'xForwardedFor', or forwardFingerprint via an HTTP header (any mode other than 'proxyProtocolV2'), has no carrier when terminateTls=false (passthrough never decrypts the stream, so no header can be injected) — use sourceAddressHeader='proxyProtocolV2' (carries both source address and fingerprint), or remove forwardFingerprint", @@ -456,6 +483,14 @@ fn build_route( ))); } + Ok(()) +} + +fn build_route( + spec: &RouteSpec, + listener_tls: &ListenerTlsSpec, + cache: &mut TlsConfigCache, +) -> crate::error::Result { let tls_config = if spec.terminate_tls { let cert_pem = spec .cert_pem @@ -811,6 +846,30 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== ); } + // The inverse and more dangerous case: a dropped wildcard must NOT shadow a healthy, more + // specific exact route at the same suffix. A prior version of the fail-closed check tested + // `dropped_wildcard` before `exact`, which black-holed every exact route under any wildcard + // that happened to fail validation — turning one tenant's wildcard typo into an outage for + // every co-tenant sharing that parent domain. + #[test] + fn healthy_exact_route_survives_a_dropped_sibling_wildcard() { + let specs = vec![ + tls_route("*.acme.com", CERT_A, KEY_B), // KeyMismatch — dropped + tls_route("api.acme.com", CERT_A, KEY_A), // valid, more specific + ]; + + let table = build_route_table(&specs, &ListenerTlsSpec::empty(), None).expect("build"); + + assert!( + table.resolve(Some("api.acme.com")).is_some(), + "a healthy exact route must resolve normally even when a sibling wildcard at the same suffix was dropped" + ); + assert!( + table.resolve(Some("other.acme.com")).is_none(), + "a subdomain with no exact route of its own must still fail closed against the dropped wildcard" + ); + } + // On a hot-swap, a route whose cert transiently fails to rebuild (the normal cert+key // non-atomic write window) must retain its last-good route from the live table rather // than dropping the SNI. @@ -842,6 +901,31 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== ); } + // Unlike a cert-build failure (a transient race that heals on the next reconcile), a + // protocol-declaration/no-carrier rejection is a permanent config error — it never heals + // without a human editing the config. It must therefore never get cert-style last-good + // carry-forward: an operator who accidentally drops `protocol: 'http'` in the same edit that + // repoints a route to a new upstream must see that route stop serving, not have `updateConfig` + // silently keep the old route (old upstream included) running indefinitely. + #[test] + fn permanent_protocol_rejection_never_carries_forward_on_hot_swap() { + let mut good = tls_route("tenant.example.com", CERT_A, KEY_A); + good.source_address_mode = SourceAddressMode::XForwardedFor; + good.protocol = RouteProtocol::Http; + let live = build_route_table(&[good], &ListenerTlsSpec::empty(), None).expect("initial build"); + assert!(live.resolve(Some("tenant.example.com")).is_some()); + + // Hot-swap drops the protocol declaration — a permanent rejection, not a transient one. + let mut broken = tls_route("tenant.example.com", CERT_A, KEY_A); + broken.source_address_mode = SourceAddressMode::XForwardedFor; + // protocol left at its default (Opaque) — undeclared. + let swapped = build_route_table(&[broken], &ListenerTlsSpec::empty(), Some(&live)).expect("hot-swap must not fail"); + assert!( + swapped.resolve(Some("tenant.example.com")).is_none(), + "a permanent protocol-declaration rejection must drop the route, not silently carry the previous one forward" + ); + } + #[test] fn header_injection_detection() { // xForwardedFor always needs protocol: 'http', regardless of fingerprint.