diff --git a/src/runtime/api/bun/h2/connection.rs b/src/runtime/api/bun/h2/connection.rs index c8afea6e69b1..a03388d85f0d 100644 --- a/src/runtime/api/bun/h2/connection.rs +++ b/src/runtime/api/bun/h2/connection.rs @@ -13,6 +13,18 @@ use super::stream::{self, State}; use super::wire::{self, ErrorCode, FrameHeader, FrameType, SettingId}; use bun_collections::HashMap; +/// Pseudo-header presence bits shared by the per-field decode loop and the RFC 9113 §8.3.1 +/// request checks in `finish_header_block` (nghttp2's NGHTTP2_HTTP_FLAG__* equivalents). +mod pseudo { + pub(super) const METHOD: u8 = 1; + pub(super) const SCHEME: u8 = 2; + pub(super) const AUTHORITY: u8 = 4; + pub(super) const PATH: u8 = 8; + pub(super) const STATUS: u8 = 16; + pub(super) const PROTOCOL: u8 = 32; + pub(super) const UNKNOWN: u8 = 64; +} + /// Snapshot of the local-settings values carried by one sent-but-unACKed SETTINGS frame, so an /// inbound ACK is attributed to the submission it actually acknowledges (RFC 9113 §6.5.3) rather /// than to the latest submission. @@ -235,6 +247,11 @@ pub struct Connection { header_target: u32, /// 0 for a normal HEADERS block; the parent stream id when assembling a PUSH_PROMISE block. header_push_parent: u32, + /// The assembling block opens a new inbound stream (a request block on the server, a + /// PUSH_PROMISE request block on the client). False for a later HEADERS on the same stream + /// (a trailer section), which RFC 9113 §8.1 forbids from carrying pseudo-headers and which + /// must not be held to the request pseudo-header requirements of §8.3.1. + header_is_request: bool, /// HEADERS arrived on a closed/half-closed-remote stream: the block is still decoded so the /// connection-scoped HPACK table stays in sync (§4.3), then refused with RST_STREAM /// (STREAM_CLOSED) instead of being dispatched. @@ -286,6 +303,7 @@ impl Connection { header_flags: 0, header_target: 0, header_push_parent: 0, + header_is_request: false, header_stream_closed: false, header_stream_refused: false, terminated: false, @@ -986,6 +1004,10 @@ impl Connection { self.header_flags = hdr.flags; self.header_target = hdr.stream_id; self.header_push_parent = 0; + // Only a server receives request blocks via HEADERS; on a client every response block + // looks "new" (the engine only tracks inbound-created streams), so without this gate it + // would be misclassified as a request. PUSH_PROMISE sets the flag itself. + self.header_is_request = self.is_server && is_new; self.header_stream_closed = stream_closed; self.header_stream_refused = refused; if !end_headers { @@ -1056,9 +1078,14 @@ impl Connection { let mut malformed = is_trailer && !self.header_end_stream; let mut seen_regular = false; let mut seen_pseudo: u8 = 0; + // Request-block state for the RFC 9113 §8.3.1 checks below (nghttp2's + // nghttp2_http_on_request_headers): only an initial HEADERS block (or a PUSH_PROMISE + // block) is a request; a later HEADERS on the same stream is a trailer section. + let is_request = self.header_is_request; + let mut saw_connect = false; + let mut saw_host = false; let mut informational = false; let mut content_length: Option = None; - let mut connect = false; while off < block.len() { match self.hpack.decode(&block[off..]) { Ok(h) => { @@ -1090,13 +1117,13 @@ impl Connection { malformed = true; } else if let Some(rest) = name_b.strip_prefix(b":") { let bit: u8 = match rest { - b"method" => 1, - b"scheme" => 2, - b"authority" => 4, - b"path" => 8, - b"status" => 16, - b"protocol" => 32, - _ => 64, + b"method" => pseudo::METHOD, + b"scheme" => pseudo::SCHEME, + b"authority" => pseudo::AUTHORITY, + b"path" => pseudo::PATH, + b"status" => pseudo::STATUS, + b"protocol" => pseudo::PROTOCOL, + _ => pseudo::UNKNOWN, }; // 8.3.1: requests never carry :status - a server seeing it inbound is // a malformed block. (The client direction also constrains pseudo @@ -1112,12 +1139,16 @@ impl Connection { let protocol_disabled = self.is_server && rest == b"protocol" && self.local_settings.enable_connect_protocol == 0; + // nghttp2 (check_pseudo_header) treats an empty pseudo-header value as + // malformed, so `:path: ""` never counts as a present :path (§8.3.1: + // `:path` "MUST NOT be empty" for http/https). if seen_regular - || bit == 64 + || bit == pseudo::UNKNOWN || (seen_pseudo & bit) != 0 || wrong_direction || protocol_disabled || is_trailer + || value_b.is_empty() { malformed = true; } @@ -1126,13 +1157,14 @@ impl Connection { } seen_pseudo |= bit; if rest == b"method" && value_b == b"CONNECT" { - connect = true; + saw_connect = true; } } else { seen_regular = true; match name_b { b"connection" | b"keep-alive" | b"proxy-connection" | b"transfer-encoding" | b"upgrade" => malformed = true, + b"host" if is_request => saw_host = true, b"te" => { // RFC 9110 10.1.4: field values are case-insensitive. if !value_b.eq_ignore_ascii_case(b"trailers") { @@ -1186,9 +1218,25 @@ impl Connection { sink.on_stream_reset(target, ErrorCode::StreamClosed.as_u32()); return false; } + // RFC 9113 §8.3.1 (nghttp2_http_on_request_headers): a request block needs exactly one + // non-empty :method, :scheme and :path plus an :authority or Host; plain CONNECT omits + // :scheme/:path and carries :authority; extended CONNECT (:protocol, RFC 8441) requires + // :method CONNECT and :authority. Without this a block with an empty or missing :path + // reaches JS as a request with an empty url (no compliant peer can produce that shape). + if is_request && !rejected && !malformed { + use pseudo::{AUTHORITY, METHOD, PATH, PROTOCOL, SCHEME}; + let extended_connect = (seen_pseudo & PROTOCOL) != 0; + malformed = if saw_connect && !extended_connect { + (seen_pseudo & (SCHEME | PATH)) != 0 || (seen_pseudo & AUTHORITY) == 0 + } else { + (seen_pseudo & (METHOD | SCHEME | PATH)) != (METHOD | SCHEME | PATH) + || ((seen_pseudo & AUTHORITY) == 0 && !saw_host) + || (extended_connect && (!saw_connect || (seen_pseudo & AUTHORITY) == 0)) + }; + } if push_parent == 0 && self.is_server && !malformed && !rejected { if let Some(s) = self.streams.get_mut(&target) { - if !connect && s.content_length.is_none() { + if !saw_connect && s.content_length.is_none() { s.content_length = content_length; } if self.header_end_stream @@ -1654,6 +1702,7 @@ impl Connection { self.header_flags = 0; self.header_target = promised; self.header_push_parent = hdr.stream_id; + self.header_is_request = true; self.header_stream_closed = false; self.header_stream_refused = false; if !end_headers { @@ -2072,7 +2121,12 @@ mod tests { let sink = CaptureSink::default(); let mut c = Connection::new(true, Settings::default()); c.preface_received = wire::CONNECTION_PREFACE.len(); - let block = encode_block(&[(b":method", b"GET"), (b":path", b"/")]); + let block = encode_block(&[ + (b":method", b"GET"), + (b":scheme", b"http"), + (b":path", b"/"), + (b":authority", b"localhost"), + ]); let flags = wire::flags::END_HEADERS | wire::flags::END_STREAM; let f = frame(FrameType::Headers, flags, 1, &block); let fed = c.receive(&sink, &f); @@ -2104,7 +2158,12 @@ mod tests { let sink = CaptureSink::default(); let mut c = Connection::new(true, Settings::default()); c.preface_received = wire::CONNECTION_PREFACE.len(); - let block = encode_block(&[(b":method", b"POST"), (b":path", b"/")]); + let block = encode_block(&[ + (b":method", b"POST"), + (b":scheme", b"http"), + (b":path", b"/"), + (b":authority", b"localhost"), + ]); // HEADERS without END_STREAM -> stream stays open for DATA. let h = frame(FrameType::Headers, wire::flags::END_HEADERS, 1, &block); c.receive(&sink, &h); @@ -2144,7 +2203,9 @@ mod tests { let mut client = Connection::new(false, Settings::default()); client.begin_header_block(); assert!(client.encode_header(b":method", b"GET", false)); + assert!(client.encode_header(b":scheme", b"http", false)); assert!(client.encode_header(b":path", b"/x", false)); + assert!(client.encode_header(b":authority", b"localhost", false)); client.send_header_block(&csink, 1, true); let wire_bytes = csink.out.borrow().clone(); assert_eq!( @@ -2183,7 +2244,9 @@ mod tests { let mut server = Connection::new(true, Settings::default()); server.begin_header_block(); assert!(server.encode_header(b":method", b"GET", false)); + assert!(server.encode_header(b":scheme", b"http", false)); assert!(server.encode_header(b":path", b"/pushed", false)); + assert!(server.encode_header(b":authority", b"localhost", false)); server.send_push_promise(&ssink, 1, 2); let bytes = ssink.out.borrow().clone(); assert_eq!( diff --git a/test/js/node/http2/h2-conformance.test.ts b/test/js/node/http2/h2-conformance.test.ts index f5021a653c55..9b495f8f14a2 100644 --- a/test/js/node/http2/h2-conformance.test.ts +++ b/test/js/node/http2/h2-conformance.test.ts @@ -799,6 +799,248 @@ describe("request header and body framing (RFC 9113 §8.1)", () => { }); }); +describe("request pseudo-header requirements (RFC 9113 §8.3.1)", () => { + // HPACK "literal never indexed, new name" (0x10) so the wire shape is exactly what is written + // and no client library normalizes it away. + function hpackBlock(pairs: [string, string][]): Buffer { + return Buffer.concat(pairs.flatMap(([n, v]) => [Buffer.from([0x10]), hpackLiteral(n), hpackLiteral(v)])); + } + + async function probe( + headers: [string, string][], + opts?: http2.ServerOptions, + ): Promise<{ dispatched: boolean; frames: Frame[] }> { + const srv = http2.createServer(opts ?? {}); + srv.on("sessionError", () => {}); + let dispatched = false; + srv.on("stream", stream => { + dispatched = true; + stream.on("error", () => {}); + try { + stream.respond({ ":status": 200 }); + stream.end(); + } catch {} + }); + srv.listen(0, "127.0.0.1"); + await once(srv, "listening"); + const srvPort = (srv.address() as net.AddressInfo).port; + const c = await RawH2.connect(srvPort); + try { + c.sendPreface(); + c.sendEmptySettings(); + c.sendFrame(FrameType.HEADERS, 0x5, 1, hpackBlock(headers)); + // PING as a barrier: by the time the ACK (or a GOAWAY/close) arrives, the server has + // fully processed the HEADERS above. + c.sendFrame(FrameType.PING, 0, 0, Buffer.alloc(8)); + await Promise.race([ + c.waitFor(f => (f.type === FrameType.PING && (f.flags & 1) !== 0) || f.type === FrameType.GOAWAY), + c.waitClosed(), + ]); + return { dispatched, frames: c.frames.slice() }; + } finally { + c.destroy(); + srv.close(); + } + } + + function expectStreamProtocolError({ dispatched, frames }: { dispatched: boolean; frames: Frame[] }) { + expect(dispatched).toBe(false); + const rst = frames.find(f => f.type === FrameType.RST_STREAM && f.streamId === 1); + expect(rst?.payload.readUInt32BE(0)).toBe(ErrorCode.PROTOCOL_ERROR); + expect(frames.find(f => f.type === FrameType.HEADERS && f.streamId === 1)).toBeUndefined(); + } + + test.each([ + [ + "empty :path", + [ + [":method", "GET"], + [":scheme", "http"], + [":path", ""], + [":authority", "localhost"], + ], + ], + [ + "empty :method", + [ + [":method", ""], + [":scheme", "http"], + [":path", "/"], + [":authority", "localhost"], + ], + ], + [ + "empty :scheme", + [ + [":method", "GET"], + [":scheme", ""], + [":path", "/"], + [":authority", "localhost"], + ], + ], + [ + "empty :authority", + [ + [":method", "GET"], + [":scheme", "http"], + [":path", "/"], + [":authority", ""], + ], + ], + [ + "missing :path", + [ + [":method", "GET"], + [":scheme", "http"], + [":authority", "localhost"], + ], + ], + [ + "missing :method", + [ + [":scheme", "http"], + [":path", "/"], + [":authority", "localhost"], + ], + ], + [ + "missing :scheme", + [ + [":method", "GET"], + [":path", "/"], + [":authority", "localhost"], + ], + ], + [ + "no :authority or host", + [ + [":method", "GET"], + [":scheme", "http"], + [":path", "/"], + ], + ], + [ + "CONNECT with :path", + [ + [":method", "CONNECT"], + [":authority", "localhost"], + [":path", "/"], + ], + ], + [ + "CONNECT with :scheme", + [ + [":method", "CONNECT"], + [":authority", "localhost"], + [":scheme", "http"], + ], + ], + ["CONNECT without :authority", [[":method", "CONNECT"]]], + ] as const)("a request with %s is RST with PROTOCOL_ERROR and never dispatched", async (_, headers) => { + expectStreamProtocolError(await probe(headers as [string, string][])); + }); + + test.each([ + [ + ":protocol on non-CONNECT", + [ + [":method", "GET"], + [":scheme", "http"], + [":path", "/"], + [":authority", "localhost"], + [":protocol", "websocket"], + ], + ], + [ + "extended CONNECT without :scheme", + [ + [":method", "CONNECT"], + [":protocol", "websocket"], + [":path", "/"], + [":authority", "localhost"], + ], + ], + [ + "extended CONNECT without :path", + [ + [":method", "CONNECT"], + [":protocol", "websocket"], + [":scheme", "http"], + [":authority", "localhost"], + ], + ], + [ + "extended CONNECT without :authority", + [ + [":method", "CONNECT"], + [":protocol", "websocket"], + [":scheme", "http"], + [":path", "/"], + ], + ], + [ + "extended CONNECT with empty :path", + [ + [":method", "CONNECT"], + [":protocol", "websocket"], + [":scheme", "http"], + [":path", ""], + [":authority", "localhost"], + ], + ], + ] as const)("with enableConnectProtocol: %s is RST with PROTOCOL_ERROR and never dispatched", async (_, headers) => { + expectStreamProtocolError( + await probe(headers as [string, string][], { settings: { enableConnectProtocol: true } }), + ); + }); + + test("a valid request block is dispatched", async () => { + const { dispatched, frames } = await probe([ + [":method", "GET"], + [":scheme", "http"], + [":path", "/"], + [":authority", "localhost"], + ]); + expect(dispatched).toBe(true); + expect(frames.find(f => f.type === FrameType.RST_STREAM && f.streamId === 1)).toBeUndefined(); + }); + + test("a request with a host header in place of :authority is dispatched", async () => { + const { dispatched, frames } = await probe([ + [":method", "GET"], + [":scheme", "http"], + [":path", "/"], + ["host", "localhost"], + ]); + expect(dispatched).toBe(true); + expect(frames.find(f => f.type === FrameType.RST_STREAM && f.streamId === 1)).toBeUndefined(); + }); + + test("a plain CONNECT with only :authority is dispatched", async () => { + const { dispatched, frames } = await probe([ + [":method", "CONNECT"], + [":authority", "localhost"], + ]); + expect(dispatched).toBe(true); + expect(frames.find(f => f.type === FrameType.RST_STREAM && f.streamId === 1)).toBeUndefined(); + }); + + test("an extended CONNECT (:protocol) with :scheme/:path/:authority is dispatched", async () => { + const { dispatched, frames } = await probe( + [ + [":method", "CONNECT"], + [":protocol", "websocket"], + [":scheme", "http"], + [":path", "/"], + [":authority", "localhost"], + ], + { settings: { enableConnectProtocol: true } }, + ); + expect(dispatched).toBe(true); + expect(frames.find(f => f.type === FrameType.RST_STREAM && f.streamId === 1)).toBeUndefined(); + }); +}); + describe("inbound stream lifecycle", () => { test("releases server stream objects once the peer resets their streams", async () => { const total = 32;