http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect

src/codegen/bake-codegen.ts

@@ -53,7 +53,13 @@ async function run() {
           side: JSON.stringify(side),
           IS_ERROR_RUNTIME: String(file === "error"),
           IS_BUN_DEVELOPMENT: String(!!debug),
-          OVERLAY_CSS: css("../runtime/bake/client/overlay.css", !!debug),
+          // JSON.stringify so the define: value is a proper JSON string.
+          // Raw CSS starts with `*{…}`, which the JSON lexer only tokenises
+          // as a recoverable SyntaxError in bun >= 1.3.14-canary (commit
+          // 314d044c0a); older bootstrap bins reject it before auto-quote
+          // can kick in. Quoting here keeps bake-codegen.ts forward- and
+          // backward-compatible with the lexer change.
+          OVERLAY_CSS: JSON.stringify(css("../runtime/bake/client/overlay.css", !!debug)),
         },
         minify: {
           syntax: !debug,

src/http_jsc/websocket_client/WebSocketUpgradeClient.rs

@@ -198,24 +198,36 @@
 
     /// On error, this returns null.
     /// Returning null signals to the parent function that the connection failed.
+    ///
+    /// The signature is safe: `websocket` is an opaque back-reference this
+    /// function stores on the client — `connect` never dereferences it.
+    /// Taking `NonNull` (instead of the raw `*mut` the extern-C shim
+    /// receives) encodes the non-nullness invariant in the type; the C++
+    /// caller in WebSocket.cpp always passes `this` cast from a live
+    /// object. The remaining FFI pointers have been lifted to
+    /// `&[BunString]` slices by the extern-C shim below. The focused
+    /// `unsafe {}` blocks inside the body cover the genuinely unsafe
+    /// operations (reads through the live VM pointer, pre-connect
+    /// `&mut *client` derivations, `Self::deref` on the error paths,
+    /// raw `SSL_CTX*` creation). Later callbacks that dereference the
+    /// stored pointer (`handle_connect_error`, `did_connect`, etc.) still
+    /// carry their own SAFETY comments at the deref sites.
     #[allow(clippy::too_many_arguments)]
-    pub unsafe fn connect(
+    pub fn connect(
         global: &JSGlobalObject,
-        websocket: *mut CppWebSocket,
+        websocket: core::ptr::NonNull<CppWebSocket>,
         host: &BunString,
         port: u16,
         pathname: &BunString,
         client_protocol: &BunString,
-        header_names: *const BunString,
-        header_values: *const BunString,
-        header_count: usize,
+        header_names: &[BunString],
+        header_values: &[BunString],
         // Proxy parameters
         proxy_host: Option<&BunString>,
         proxy_port: u16,
         proxy_authorization: Option<&BunString>,
-        proxy_header_names: *const BunString,
-        proxy_header_values: *const BunString,
-        proxy_header_count: usize,
+        proxy_header_names: &[BunString],
+        proxy_header_values: &[BunString],
         // TLS options (full SSLConfig for complete TLS customization)
         ssl_config: Option<Box<SSLConfig>>,
         // Whether the target URL is wss:// (separate from ssl template parameter)
@@ -244,8 +256,7 @@
 
         // Headers8Bit::init only returns AllocError; handle OOM as a crash per
         // the OOM contract instead of masking it as a connection failure.
-        // SAFETY: header_names/header_values point to header_count live BunStrings per extern-C contract.
-        let extra_headers = unsafe { Headers8Bit::init(header_names, header_values, header_count) };
+        let extra_headers = Headers8Bit::init(header_names, header_values);
 
         let proxy_host_slice: Option<Utf8Slice> = proxy_host.map(|ph| ph.to_utf8());
         let target_authorization_slice: Option<Utf8Slice> =
@@ -293,12 +304,9 @@
             // Parse proxy headers (temporary, freed after building CONNECT request)
             // Headers8Bit::init / to_headers only return AllocError; OOM should
             // crash, not silently become a connection failure.
-            // SAFETY: proxy_header_names/values point to proxy_header_count live BunStrings per extern-C contract.
-            let proxy_extra_headers = unsafe {
-                Headers8Bit::init(proxy_header_names, proxy_header_values, proxy_header_count)
-            };
+            let proxy_extra_headers = Headers8Bit::init(proxy_header_names, proxy_header_values);
 
-            let proxy_hdrs: Option<Headers> = if proxy_header_count > 0 {
+            let proxy_hdrs: Option<Headers> = if !proxy_header_names.is_empty() {
                 Some(proxy_extra_headers.to_headers())
             } else {
                 None
@@ -340,7 +348,7 @@
         let client: *mut Self = bun_core::heap::into_raw(Box::new(HTTPClient::<SSL> {
             ref_count: Cell::new(1),
             tcp: Socket::<SSL>::detached(),
-            outgoing_websocket: Some(websocket),
+            outgoing_websocket: Some(websocket.as_ptr()),
             input_body_buf,
             to_send_len: 0,
             headers_buf: [picohttp::Header::ZERO; 128],
@@ -1863,25 +1871,31 @@
 }
 
 impl<'a> Headers8Bit<'a> {
-    /// # Safety
-    /// `names_ptr` and `values_ptr` must each be null or point to `len` valid
-    /// `BunString`s alive for `'a`.
-    unsafe fn init(names_ptr: *const BunString, values_ptr: *const BunString, len: usize) -> Self {
-        if len == 0 {
+    /// Decode parallel name/value `BunString` slices into interleaved UTF-8
+    /// slices. Callers that reach this from the C ABI should convert their
+    /// `(ptr, len)` pairs with `bun_core::ffi::slice` inside an `unsafe {}`
+    /// block before calling.
+    ///
+    /// `names` and `values` are expected to be the same length — the
+    /// extern-C shim derives both from a single `header_count`, so a
+    /// mismatch is a programmer bug. The `debug_assert_eq!` catches that
+    /// loudly in debug; `zip` stops at the shorter slice so a mismatch in
+    /// release drops the trailing unpaired entries instead of panicking
+    /// on an OOB index.
+    fn init(names: &'a [BunString], values: &'a [BunString]) -> Self {
+        debug_assert_eq!(names.len(), values.len());
+        let pair_count = names.len().min(values.len());
+        if pair_count == 0 {
             return Self {
                 slices: Vec::new(),
                 _marker: core::marker::PhantomData,
             };
         }
-        // SAFETY: per fn contract.
-        let names_in = unsafe { bun_core::ffi::slice(names_ptr, len) };
-        // SAFETY: per fn contract — `values_ptr` points to `len` live `BunString`s.
-        let values_in = unsafe { bun_core::ffi::slice(values_ptr, len) };
-
-        let mut slices: Vec<Utf8Slice> = Vec::with_capacity(len * 2);
-        for i in 0..len {
-            slices.push(names_in[i].to_utf8());
-            slices.push(values_in[i].to_utf8());
+
+        let mut slices: Vec<Utf8Slice> = Vec::with_capacity(pair_count * 2);
+        for (name, value) in names.iter().zip(values.iter()) {
+            slices.push(name.to_utf8());
+            slices.push(value.to_utf8());
         }
 
         Self {
@@ -2230,32 +2244,43 @@
             ) -> *mut HTTPClient<$ssl> {
                 // SAFETY: extern-C contract — caller (WebCore::WebSocket C++)
                 // guarantees `header_names`/`header_values` point to
-                // `header_count` live `BunString`s (and likewise for the proxy
-                // header arrays), and that `websocket` is a live back-ref.
-                match unsafe {
-                    HTTPClient::<$ssl>::connect(
-                        global,
-                        websocket,
-                        host,
-                        port,
-                        pathname,
-                        client_protocol,
-                        header_names,
-                        header_values,
-                        header_count,
-                        proxy_host,
-                        proxy_port,
-                        proxy_authorization,
-                        proxy_header_names,
-                        proxy_header_values,
-                        proxy_header_count,
-                        ssl_config,
-                        target_is_secure,
-                        target_authorization,
-                        unix_socket_path,
-                        offer_permessage_deflate,
-                    )
-                } {
+                // `header_count` live `BunString`s (and likewise for the
+                // proxy header arrays). Lift the `(ptr, len)` pairs to
+                // `&[BunString]` here so the inner `connect` can stay
+                // safe. `ffi::slice` tolerates `(null, 0)`.
+                let header_names = unsafe { bun_core::ffi::slice(header_names, header_count) };
+                let header_values = unsafe { bun_core::ffi::slice(header_values, header_count) };
+                let proxy_header_names =
+                    unsafe { bun_core::ffi::slice(proxy_header_names, proxy_header_count) };
+                let proxy_header_values =
+                    unsafe { bun_core::ffi::slice(proxy_header_values, proxy_header_count) };
+                // `websocket` is a live back-ref to the C++ WebCore::WebSocket;
+                // WebSocket.cpp always passes `this`. Encode non-nullness at
+                // the boundary and return null on the defensive-unreachable
+                // branch so the inner `connect` can take a `NonNull`.
+                let Some(websocket) = core::ptr::NonNull::new(websocket) else {
+                    return ptr::null_mut();
+                };
+                match HTTPClient::<$ssl>::connect(
+                    global,
+                    websocket,
+                    host,
+                    port,
+                    pathname,
+                    client_protocol,
+                    header_names,
+                    header_values,
+                    proxy_host,
+                    proxy_port,
+                    proxy_authorization,
+                    proxy_header_names,
+                    proxy_header_values,
+                    ssl_config,
+                    target_is_secure,
+                    target_authorization,
+                    unix_socket_path,
+                    offer_permessage_deflate,
+                ) {
                     Some(p) => p,
                     None => ptr::null_mut(),
                 }

test/js/web/websocket/websocket-custom-headers.test.ts

@@ -379,3 +379,60 @@
     ws.close();
   });
 });
+
+// Regression for #30777: the connect() path was refactored to take
+// `&[BunString]` slices (with the `(ptr, len)` → slice conversion lifted
+// into the extern-C shim). Cover both ends of that slice parameter:
+// the empty-headers case (`(ptr, 0)`) and the many-headers case
+// (drives the `Headers8Bit::init` iteration).
+describe("WebSocket connect() header slice boundaries (#30777)", () => {
+  async function openAndEchoHeaders(opts: { headers?: Record<string, string> }): Promise<Record<string, string>> {
+    const { promise: headersSeen, resolve: resolveHeaders } = Promise.withResolvers<Record<string, string>>();
+    await using server = Bun.serve({
+      hostname: "127.0.0.1",
+      port: 0,
+      websocket: {
+        open(_ws) {},
+        message(_ws, _msg) {},
+      },
+      fetch(req, server) {
+        const headers: Record<string, string> = {};
+        for (const [k, v] of req.headers.entries()) headers[k.toLowerCase()] = v;
+        resolveHeaders(headers);
+        if (server.upgrade(req)) return;
+        return new Response("no upgrade", { status: 400 });
+      },
+    });
+    const { promise: opened, resolve: resolveOpen, reject: rejectOpen } = Promise.withResolvers<void>();
+    const ws = new WebSocket(`ws://127.0.0.1:${server.port}/`, opts);
+    ws.onopen = () => resolveOpen();
+    ws.onerror = e => rejectOpen((e as any).error ?? new Error((e as any).message ?? "ws error"));
+    const [, headers] = await Promise.all([opened, headersSeen]);
+    ws.close();
+    return headers;
+  }
+
+  it("connects with zero user headers (ffi::slice(ptr, 0) path)", async () => {
+    const seen = await openAndEchoHeaders({});
+    // Core upgrade headers still arrive — the connect path must not skip
+    // them when the user-headers slice is empty.
+    expect(seen["connection"]?.toLowerCase()).toContain("upgrade");
+    expect(seen["upgrade"]?.toLowerCase()).toBe("websocket");
+    expect(seen["sec-websocket-key"]).toBeString();
+    expect(seen["sec-websocket-version"]).toBe("13");
+  });
+
+  it("connects with many user headers (Headers8Bit slice iteration)", async () => {
+    const custom: Record<string, string> = {};
+    for (let i = 0; i < 12; i++) custom[`X-Custom-${i}`] = `value-${i}`;
+    const seen = await openAndEchoHeaders({ headers: custom });
+    // Every custom header must round-trip: the refactor interleaves
+    // names/values via `chunks_exact(2)`, so a drop-one or pair-off-by-one
+    // bug would show up as a missing or swapped value here.
+    for (let i = 0; i < 12; i++) {
+      expect(seen[`x-custom-${i}`]).toBe(`value-${i}`);
+    }
+    expect(seen["connection"]?.toLowerCase()).toContain("upgrade");
+    expect(seen["upgrade"]?.toLowerCase()).toBe("websocket");
+  });
+});