diff --git a/src/runtime/server/NodeHTTPResponse.rs b/src/runtime/server/NodeHTTPResponse.rs index bfcf97a09630..9ad0d80c5267 100644 --- a/src/runtime/server/NodeHTTPResponse.rs +++ b/src/runtime/server/NodeHTTPResponse.rs @@ -909,27 +909,8 @@ impl NodeHTTPResponse { auto_header_bits: u32, keep_alive_timeout_secs: u32, ) -> JsResult { - if self.is_requested_completed_or_ended() { - return err_throw( - global_object, - ErrorCode::ERR_STREAM_ALREADY_FINISHED, - "Stream is already ended", - ); - } - - let flags = self.flags.get(); - let Some(raw_response) = self.raw_response.get() else { - // We haven't emitted the "close" event yet. - return Ok(JSValue::UNDEFINED); - }; - if flags.contains(Flags::SOCKET_CLOSED) || flags.contains(Flags::UPGRADED) { - // We haven't emitted the "close" event yet. - return Ok(JSValue::UNDEFINED); - } - - let state = raw_response.state(); - handle_ended_if_necessary(state, global_object)?; - + // Arguments are converted before any response state is read: ToString on + // `statusMessage` can run user JS that destroys or ends the response. let status_code_value: JSValue = arguments.first().copied().unwrap_or(JSValue::UNDEFINED); let status_message_value: JSValue = match arguments.get(1).copied() { Some(v) if v != JSValue::NULL => v, @@ -975,6 +956,27 @@ impl NodeHTTPResponse { return Err(jsc::JsError::Thrown); } + if self.is_requested_completed_or_ended() { + return err_throw( + global_object, + ErrorCode::ERR_STREAM_ALREADY_FINISHED, + "Stream is already ended", + ); + } + + let flags = self.flags.get(); + let Some(raw_response) = self.raw_response.get() else { + // We haven't emitted the "close" event yet. + return Ok(JSValue::UNDEFINED); + }; + if flags.contains(Flags::SOCKET_CLOSED) || flags.contains(Flags::UPGRADED) { + // We haven't emitted the "close" event yet. + return Ok(JSValue::UNDEFINED); + } + + let state = raw_response.state(); + handle_ended_if_necessary(state, global_object)?; + if state.is_http_status_called() { return err_throw( global_object, @@ -1219,16 +1221,6 @@ impl NodeHTTPResponse { global_object: &JSGlobalObject, callframe: &CallFrame, ) -> JsResult { - if self.is_done() { - return Ok(JSValue::UNDEFINED); - } - { - let Some(raw_response) = self.raw_response.get() else { - return Ok(JSValue::UNDEFINED); - }; - handle_ended_if_necessary(raw_response.state(), global_object)?; - } - let arguments = callframe.arguments(); let input_value = arguments.first().copied().unwrap_or(JSValue::UNDEFINED); if input_value.is_undefined_or_null() { @@ -1256,10 +1248,15 @@ impl NodeHTTPResponse { )); } - // Re-read after the JS-capable coercion above (R-2: re-entry may clear it). + // Response state is read only after the JS-capable coercion above + // (R-2: re-entry may destroy or end the response). + if self.is_done() { + return Ok(JSValue::UNDEFINED); + } let Some(raw_response) = self.raw_response.get() else { return Ok(JSValue::UNDEFINED); }; + handle_ended_if_necessary(raw_response.state(), global_object)?; raw_response.write_informational(string_or_buffer.slice()); Ok(JSValue::UNDEFINED) } @@ -1910,41 +1907,8 @@ impl NodeHTTPResponse { arguments: &[JSValue], this_value: JSValue, ) -> JsResult { - if self.is_requested_completed_or_ended() { - return err_throw( - global_object, - ErrorCode::ERR_STREAM_WRITE_AFTER_END, - "Stream already ended", - ); - } - - // Loosely mimicking this code: - // function _writeRaw(data, encoding, callback, size) { - // const conn = this[kSocket]; - // if (conn?.destroyed) { - // // The socket was destroyed. If we're still trying to write to it, - // // then we haven't gotten the 'close' event yet. - // return false; - // } - if self.flags.get().contains(Flags::SOCKET_CLOSED) || self.raw_response.get().is_none() { - return Ok(if IS_END { - JSValue::UNDEFINED - } else { - JSValue::js_number_from_int32(0) - }); - } - - // Re-read raw_response at each use site (R-2: methods that - // re-enter may clear it). - let state = self.raw_response.get().unwrap().state(); - if !state.is_response_pending() { - return err_throw( - global_object, - ErrorCode::ERR_STREAM_WRITE_AFTER_END, - "Stream already ended", - ); - } - + // Arguments are converted before any response state is read: ToString on + // `input` / `encoding` can run user JS that destroys or ends the response. let input_value: JSValue = if arguments.len() > 0 { arguments[0] } else { @@ -2027,6 +1991,31 @@ impl NodeHTTPResponse { return Err(jsc::JsError::Thrown); } + // Loosely mimicking this code: + // function _writeRaw(data, encoding, callback, size) { + // const conn = this[kSocket]; + // if (conn?.destroyed) { + // // The socket was destroyed. If we're still trying to write to it, + // // then we haven't gotten the 'close' event yet. + // return false; + // } + if self.flags.get().contains(Flags::SOCKET_CLOSED) || self.raw_response.get().is_none() { + return Ok(if IS_END { + JSValue::UNDEFINED + } else { + JSValue::js_number_from_int32(0) + }); + } + + let state = self.raw_response.get().unwrap().state(); + if self.is_requested_completed_or_ended() || !state.is_response_pending() { + return err_throw( + global_object, + ErrorCode::ERR_STREAM_WRITE_AFTER_END, + "Stream already ended", + ); + } + let bytes = string_or_buffer.slice(); if IS_END { diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index b9edb1f12756..e277bc6f4809 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -2281,6 +2281,49 @@ it("socket handle write keeps buffered data intact when encoding coercion re-ent expect(exitCode).toBe(0); }, 30_000); +it.each([ + ["write", `result = res.write(payload, enc); res.end();`, "returned boolean"], + ["end", `res.flushHeaders(); result = res.end(payload, enc);`, "returned object"], +])( + "ServerResponse.%s() with an encoding whose toPrimitive destroys the response does not crash", + async (_method, call, expected) => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const server = http.createServer((req, res) => { + const enc = Object.assign(new String("hex"), { + [Symbol.toPrimitive]() { res.destroy(); Bun.gc(true); return "hex"; }, + }); + const payload = Buffer.alloc(40000, "41").toString(); + let result; + try { + ${call} + result = "returned " + typeof result; + } catch (e) { + result = "threw " + (e.code || e.message); + } + console.log(result); + setImmediate(() => { server.close(); process.exit(0); }); + }); + server.listen(0, "127.0.0.1", () => { + fetch("http://127.0.0.1:" + server.address().port + "/").then(r => r.text()).catch(() => {}); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toBe(expected + "\n"); + expect(exitCode).toBe(0); + }, +); + it("client request path that does not begin with a slash stays on the configured host", async () => { // `options.path` must only ever influence the request target that is written // on the wire; it must never change which server the client connects to,