Skip to content

Commit 799d3a9

Browse files
kriszypclaude
andcommitted
Address PR #1985 review feedback on connection-info TLVs
- Gate the SSL TLV branch on PP2_CLIENT_SSL (0x01), not just presence: a proxy can legally emit an SSL TLV with client=0x00 (TLS not used), and connectionInfo.tls was truthy regardless, so request.protocol reported "https" for a cleartext client (hdbjeff, blocking). - Document that alpn/authority/ja3/ja4 are raw, unvalidated TLV payloads (hdbjeff, non-blocking). - Add explicit connectionInfo getters (returning undefined) on BunRequest/UwsRequest for API-surface consistency with the existing authorized/peerCertificate getters (Ethan-Arrowood, non-blocking). - Regression tests for an SSL TLV with client=0x00, with and without the CERT_CONN bit set alongside it. Co-Authored-By: Claude Opus <noreply@anthropic.com>
1 parent ba2a5db commit 799d3a9

3 files changed

Lines changed: 49 additions & 10 deletions

File tree

server/serverHelpers/Request.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,10 @@ export class BunRequest {
419419
get mtlsConfig() {
420420
return undefined;
421421
}
422+
get connectionInfo() {
423+
// No PROXY v2 decoding on the Bun.serve() path
424+
return undefined;
425+
}
422426
get body() {
423427
return this.#body || (this.#body = new BunRequestBody(this._webRequest));
424428
}
@@ -535,6 +539,10 @@ export class UwsRequest {
535539
get mtlsConfig() {
536540
return undefined;
537541
}
542+
get connectionInfo() {
543+
// TLS terminated upstream — no PROXY v2 decoding at this hop.
544+
return undefined;
545+
}
538546
get body() {
539547
// Bodyless requests get an already-ended empty stream so consumers can still read it uniformly.
540548
if (!this.#body) {

server/serverHelpers/proxyProtocol.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ const PP2_SUBTYPE_SSL_CIPHER = 0x23;
4343
const PP2_TYPE_JA3 = 0xe0;
4444
const PP2_TYPE_JA4 = 0xe1;
4545
const PP2_TYPE_CLIENT_CERT = 0xe2;
46+
const PP2_CLIENT_SSL = 0x01;
4647
const PP2_CLIENT_CERT_CONN = 0x02;
4748

4849
/** TLS facts the fronting proxy observed on the terminated connection. */
@@ -65,17 +66,22 @@ export interface ForwardedTlsInfo {
6566
* Connection-level facts forwarded by a trusted proxy over a PROXY v2 header.
6667
* Present on `request.connectionInfo` only when a v2 header was decoded; every
6768
* field is optional since a given route forwards only what it's configured to.
69+
*
70+
* `alpn`, `authority`, `ja3`, and `ja4` are raw TLV payloads: decoded as bytes
71+
* (latin1/UTF-8) with no grammar or length validation beyond the enclosing
72+
* PROXY header's bounds. Validate before placing them in headers, logs, cache
73+
* keys, or other security-sensitive contexts.
6874
*/
6975
export interface ConnectionInfo {
70-
/** Negotiated ALPN protocol, e.g. "h2" (PP2 TLV 0x01). */
76+
/** Negotiated ALPN protocol, e.g. "h2" (PP2 TLV 0x01). Raw, unvalidated. */
7177
alpn?: string;
72-
/** SNI hostname from the ClientHello (PP2 TLV 0x02). */
78+
/** SNI hostname from the ClientHello (PP2 TLV 0x02). Raw, unvalidated. */
7379
authority?: string;
7480
/** TLS facts from the SSL TLV (0x20); present when the proxy terminated TLS. */
7581
tls?: ForwardedTlsInfo;
76-
/** Client JA3 fingerprint, 32-char MD5 hex (PP2 TLV 0xE0). */
82+
/** Client JA3 fingerprint, 32-char MD5 hex (PP2 TLV 0xE0). Raw, unvalidated. */
7783
ja3?: string;
78-
/** Client JA4 fingerprint (PP2 TLV 0xE1). */
84+
/** Client JA4 fingerprint (PP2 TLV 0xE1). Raw, unvalidated. */
7985
ja4?: string;
8086
/** Verified mTLS client certificate chain (DER, leaf first) from PP2 0xE2 TLVs. */
8187
clientCertChain?: Buffer[];
@@ -170,10 +176,16 @@ function decodeV2(buffer: Buffer): ProxyHeaderDecision {
170176
info.authority = buffer.toString('utf8', valueStart, valueEnd);
171177
} else if (type === PP2_TYPE_SSL && valueLength >= 5) {
172178
// struct pp2_tlv_ssl: client(1) verify(4, 0 = verified ok), then sub-TLVs.
173-
certPresented = (buffer[valueStart] & PP2_CLIENT_CERT_CONN) !== 0;
174-
const verifyOk = buffer.readUInt32BE(valueStart + 1) === 0;
175-
tls = { verified: certPresented && verifyOk };
176-
parseSslSubTlvs(buffer, valueStart + 5, valueEnd, tls);
179+
// Only PP2_CLIENT_SSL means the client actually connected over TLS; a
180+
// CERT_CONN bit without it is internally inconsistent (a client cert
181+
// is a TLS construct) and reported alongside, not on its own.
182+
const clientBits = buffer[valueStart];
183+
if ((clientBits & PP2_CLIENT_SSL) !== 0) {
184+
certPresented = (clientBits & PP2_CLIENT_CERT_CONN) !== 0;
185+
const verifyOk = buffer.readUInt32BE(valueStart + 1) === 0;
186+
tls = { verified: certPresented && verifyOk };
187+
parseSslSubTlvs(buffer, valueStart + 5, valueEnd, tls);
188+
}
177189
} else if (type === PP2_TYPE_JA3 && valueLength > 0) {
178190
info.ja3 = buffer.toString('latin1', valueStart, valueEnd);
179191
} else if (type === PP2_TYPE_JA4 && valueLength > 0) {

unitTests/server/serverHelpers/proxyProtocol.test.js

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,9 @@ function tlv(type, value) {
7272
return Buffer.concat([header, value]);
7373
}
7474

75-
function sslTlv({ certPresented = true, verify = 0 } = {}) {
75+
function sslTlv({ certPresented = true, verify = 0, clientSsl = true } = {}) {
7676
const value = Buffer.alloc(5);
77-
value[0] = 0x01 | (certPresented ? 0x02 : 0); // PP2_CLIENT_SSL | PP2_CLIENT_CERT_CONN
77+
value[0] = (clientSsl ? 0x01 : 0) | (certPresented ? 0x02 : 0); // PP2_CLIENT_SSL | PP2_CLIENT_CERT_CONN
7878
value.writeUInt32BE(verify, 1);
7979
return tlv(0x20, value);
8080
}
@@ -187,6 +187,25 @@ describe('proxyProtocol decodeProxyHeader', () => {
187187
assert.strictEqual(info.clientCertChain, undefined);
188188
});
189189

190+
it('ignores an SSL TLV whose client field does not report PP2_CLIENT_SSL', () => {
191+
// client = 0x00: no SSL, no cert-conn — the proxy is not claiming TLS was used.
192+
// With no other TLVs, no connectionInfo fields are populated at all.
193+
const header = buildV2Header({ tlvs: [sslTlv({ clientSsl: false, certPresented: false }), tlv(0xe2, CLIENT_DER)] });
194+
assert.strictEqual(decodeProxyHeader(header).connectionInfo, undefined);
195+
});
196+
197+
it('ignores CERT_CONN without PP2_CLIENT_SSL as internally inconsistent', () => {
198+
// client = CERT_CONN only, no SSL bit: asserts a client cert without asserting TLS.
199+
// JA3 alongside proves the TLV loop keeps parsing past the ignored SSL TLV.
200+
const header = buildV2Header({
201+
tlvs: [sslTlv({ clientSsl: false, certPresented: true }), tlv(0xe2, CLIENT_DER), tlv(0xe0, Buffer.from('0123456789abcdef0123456789abcdef'))],
202+
});
203+
const info = decodeProxyHeader(header).connectionInfo;
204+
assert.strictEqual(info.tls, undefined);
205+
assert.strictEqual(info.clientCertChain, undefined);
206+
assert.strictEqual(info.ja3, '0123456789abcdef0123456789abcdef');
207+
});
208+
190209
it('captures ALPN, authority, JA3, JA4, and SSL version/cipher TLVs', () => {
191210
const sslWithSubTlvs = Buffer.concat([
192211
Buffer.from([0x01 | 0x02]), // client: SSL | CERT_CONN

0 commit comments

Comments
 (0)