node:http2.createServer — fix h2c compatibility with strict peers

src/js/node/http2.ts

@@ -315,6 +315,7 @@ const bunHTTP2Native = Symbol.for("::bunhttp2native::");
 const bunHTTP2Socket = Symbol.for("::bunhttp2socket::");
 const bunHTTP2OriginSet = Symbol("::bunhttp2originset::");
 const bunHTTP2StreamFinal = Symbol.for("::bunHTTP2StreamFinal::");
+const bunHTTP2WaitForTrailers = Symbol("::bunhttp2waitfortrailers::");
 
 const bunHTTP2StreamStatus = Symbol.for("::bunhttp2StreamStatus::");
 
@@ -2050,7 +2051,17 @@ class Http2Stream extends Duplex {
         sensitiveNames[sensitives[i]] = true;
       }
     }
-    session[bunHTTP2Native]?.sendTrailers(this.#id, headers, sensitiveNames);
+    // RFC 9113 §8.1 doesn't explicitly forbid an empty trailer HEADERS frame,
+    // but strict peer implementations (nghttp2, used by curl and Node) reject
+    // a zero-length HPACK block as a callback failure. When the user passes an
+    // empty trailer object (which the compat Http2ServerResponse does
+    // unconditionally from onStreamTrailersReady), emit an empty DATA frame
+    // with END_STREAM instead — this matches Node's wire output.
+    if (ObjectKeys(headers).length === 0) {
+      session[bunHTTP2Native]?.noTrailers(this.#id);
+    } else {
+      session[bunHTTP2Native]?.sendTrailers(this.#id, headers, sensitiveNames);
+    }
     this.#sentTrailers = headers;
   }
 
@@ -2198,6 +2209,34 @@ class Http2Stream extends Duplex {
       const native = session[bunHTTP2Native];
       if (native) {
         this[bunHTTP2StreamStatus] |= StreamState.FinalCalled;
+        // When waitForTrailers is active, writing an empty DATA frame with
+        // close=true emits a bare empty DATA frame (flags=0) to the wire
+        // before the trailer/noTrailers path runs, which then emits ANOTHER
+        // empty DATA (with END_STREAM). Two consecutive empty DATA frames
+        // confuse strict peers (nghttp2 callback failure). Skip the empty
+        // writeStream and drive the wantTrailers path directly — the
+        // eventual `sendTrailers({})` → `noTrailers` call terminates the
+        // stream with a single empty DATA END_STREAM frame, matching Node.
+        if (this[bunHTTP2WaitForTrailers]) {
+          this[bunHTTP2WaitForTrailers] = false;
+          if ((this[bunHTTP2StreamStatus] & StreamState.WantTrailer) === 0) {
+            this[bunHTTP2StreamStatus] |= StreamState.WantTrailer;
+            if (this.listenerCount("wantTrailers") === 0) {
+              native.noTrailers(this.#id);
+              // Mark trailers as "sent" so a later stream.sendTrailers()
+              // call hits the ERR_HTTP2_TRAILERS_ALREADY_SENT guard instead
+              // of invoking native noTrailers() a second time on an
+              // already-half-closed stream. The emit("wantTrailers") path
+              // below reaches the same result via sendTrailers({}) which
+              // assigns #sentTrailers itself.
+              this.#sentTrailers = {};
+            } else {
+              this.emit("wantTrailers");
+            }
+          }
+          callback();
+          return;
+        }
         native.writeStream(this.#id, "", "ascii", true, callback);
         return;
       }
@@ -2628,7 +2667,16 @@ class ServerHttp2Stream extends Http2Stream {
       statusCode === HTTP_STATUS_NOT_MODIFIED ||
       this.headRequest === true
     ) {
-      options = { ...options, endStream: true };
+      // When endStream is true the HEADERS frame itself carries END_STREAM
+      // and the stream moves to HALF_CLOSED_LOCAL inside native request().
+      // If waitForTrailers is ALSO true the native layer dispatches
+      // onWantTrailers immediately after, whose JS handler calls
+      // noTrailers → sendData("", true) and emits a spurious DATA frame on
+      // the already-half-closed stream (RFC 9113 §5.1 violation). Strip
+      // waitForTrailers here so the native never fires that path; the JS
+      // guard further down (`options?.waitForTrailers && !endStream`) only
+      // covers the `_final` side and runs AFTER the native call.
+      options = { ...options, endStream: true, waitForTrailers: false };
       endStream = true;
     }
     const sendDate = options?.sendDate;
@@ -2643,6 +2691,17 @@ class ServerHttp2Stream extends Http2Stream {
       session[bunHTTP2Native]?.request(this.id, undefined, headers, sensitiveNames);
     } else {
       session[bunHTTP2Native]?.request(this.id, undefined, headers, sensitiveNames, options);
+      // Only track waitForTrailers when the HEADERS frame above did NOT end
+      // the stream. Status codes 204/205/304 and HEAD requests force
+      // endStream=true earlier in this method, which means the native
+      // request() already wrote END_STREAM on the HEADERS frame — driving
+      // the wantTrailers path from `_final` on such a stream would call
+      // `noTrailers`/`emit("wantTrailers")` on an already-half-closed
+      // stream and corrupt state. Use optional chaining: `options` may be
+      // `null` here (typeof null === "object" enters this else branch).
+      if (options?.waitForTrailers && !endStream) {
+        this[bunHTTP2WaitForTrailers] = true;
+      }
     }
     this.headersSent = true;
     this[bunHTTP2Headers] = headers;
@@ -2800,7 +2859,11 @@ class ServerHttp2Session extends Http2Session {
   #alpnProtocol: string | undefined = undefined;
   #localSettings: Settings | null = {
     headerTableSize: 4096,
-    enablePush: true,
+    // RFC 9113 §6.5.2: servers MUST NOT advertise ENABLE_PUSH != 0. The
+    // initial SETTINGS frame forces this to 0 in the constructor — keep the
+    // default here in sync so `session.localSettings.enablePush` agrees with
+    // the wire before the peer's SETTINGS ACK arrives.
+    enablePush: false,
     maxConcurrentStreams: 100,
     initialWindowSize: 65535,
     maxFrameSize: 16384,
@@ -2891,9 +2954,15 @@ class ServerHttp2Session extends Http2Session {
       if ((status & StreamState.StreamResponded) !== 0) {
         stream.emit("trailers", headers, flags, rawheaders);
       } else {
+        // Set the StreamResponded bit BEFORE dispatching the 'stream' event
+        // synchronously to user code. The user handler may call
+        // stream.respond()/stream.end() which set other bits (WantTrailer,
+        // FinalCalled, EndedCalled, WritableClosed). If we captured `status`
+        // and wrote it back AFTER the emit, we'd clobber any bits set by the
+        // user handler — in particular, losing WantTrailer/FinalCalled breaks
+        // any later `sendTrailers()` with ERR_HTTP2_TRAILERS_NOT_READY.
+        stream[bunHTTP2StreamStatus] |= StreamState.StreamResponded;
         self[kServer].emit("stream", stream, headers, flags, rawheaders);
-
-        stream[bunHTTP2StreamStatus] = status | StreamState.StreamResponded;
         self.emit("stream", stream, headers, flags, rawheaders);
       }
     },
@@ -3069,7 +3138,13 @@ class ServerHttp2Session extends Http2Session {
     this.#parser = new H2FrameParser({
       native: nativeSocket,
       context: this,
-      settings: { ...options, ...options?.settings },
+      // RFC 9113 §6.5.2: a server MUST NOT send SETTINGS_ENABLE_PUSH with a
+      // value other than 0 — any non-zero value is treated by a client as a
+      // PROTOCOL_ERROR (nghttp2 reports this as callback failure). This is
+      // unconditional at the protocol level, so `enablePush: false` is
+      // spread LAST to override any user-supplied setting and keep the
+      // server compliant regardless of caller configuration.
+      settings: { ...options, ...options?.settings, enablePush: false },
       type: 0, // server type
       handlers: ServerHttp2Session.#Handlers,
     });
@@ -3206,7 +3281,16 @@ class ServerHttp2Session extends Http2Session {
     if (callback !== undefined && typeof callback !== "function") {
       throw $ERR_INVALID_ARG_TYPE("callback", "function", callback);
     }
+    // Validate the caller-supplied object FIRST so null / arrays / primitives
+    // still throw ERR_INVALID_ARG_TYPE — spreading ({ ...null }) would hide
+    // these from the type guard in validateSettings.
     validateSettings(settings);
+    // RFC 9113 §6.5.2: a server MUST NOT advertise SETTINGS_ENABLE_PUSH != 0.
+    // Force-override whatever the caller passes so a mid-connection SETTINGS
+    // frame stays compliant (the initial SETTINGS frame already clamps this
+    // in ServerHttp2Session's constructor). Clients still accept `enablePush`
+    // via their own `settings()` method.
+    settings = { ...settings, enablePush: false };
     this.#pendingSettingsAck = true;
     this.#parser?.settings(settings);
     if (typeof callback === "function") {
@@ -3380,7 +3464,11 @@ class ClientHttp2Session extends Http2Session {
         if (header_status >= 100 && header_status < 200) {
           stream.emit("headers", headers, flags, rawheaders);
         } else {
-          stream[bunHTTP2StreamStatus] = status | StreamState.StreamResponded;
+          // Set the bit BEFORE dispatching synchronously to user code — a
+          // 'response' handler that mutates stream state would otherwise be
+          // clobbered by a stale read-modify-write (see the server-side note
+          // at the stream handler above).
+          stream[bunHTTP2StreamStatus] |= StreamState.StreamResponded;
           if (header_status === 421) {
             // 421 Misdirected Request
             removeOriginFromSet(self, stream);