Skip to content

node:http: convert write()/end() arguments before reading response state #39386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jarred-Sumner wants to merge 4 commits into
base: main
Choose a base branch
from
+99 −67
Open

node:http: convert write()/end() arguments before reading response state #39386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
938c181
NodeHTTPResponse: re-check response state after converting write()/en…
Jarred-Sumner Aug 17, 2026
c907de2
NodeHTTPResponse: convert write()/end() arguments before reading resp…
Jarred-Sumner Aug 17, 2026
abb85e6
NodeHTTPResponse: convert writeHead/writeInformational args before re…
Jarred-Sumner Aug 17, 2026
df8ca56
write_or_end: a destroyed socket returns false before the write-after…
Jarred-Sumner Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 56 additions & 67 deletions src/runtime/server/NodeHTTPResponse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,27 +909,8 @@ impl NodeHTTPResponse {
auto_header_bits: u32,
keep_alive_timeout_secs: u32,
) -> JsResult<JSValue> {
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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1219,16 +1221,6 @@ impl NodeHTTPResponse {
global_object: &JSGlobalObject,
callframe: &CallFrame,
) -> JsResult<JSValue> {
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() {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -1910,41 +1907,8 @@ impl NodeHTTPResponse {
arguments: &[JSValue],
this_value: JSValue,
) -> JsResult<JSValue> {
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 {
Expand Down Expand Up @@ -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 {
Expand Down
43 changes: 43 additions & 0 deletions test/js/node/http/node-http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2281,6 +2281,49 @@
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"],
])(

Check warning on line 2287 in test/js/node/http/node-http.test.ts

View check run for this annotation

Claude / Claude Code Review

write_head_impl and write_informational fixes lack test coverage 

The `it.each` only passes a hostile *encoding* to `res.write()`/`res.end()` — both route into `write_or_end`. Neither row sets a hostile `res.statusMessage` (so `write_head_impl`'s `to_bun_string` at :948 stays on the `is_undefined()` short-circuit) and neither reaches `write_informational`, so reverting either of those two reorderings would not break any test in this PR (REVIEW.md: "Confirm deleting each load-bearing clause of your fix breaks at least one test"). Consider a third row, e.g. `["w
Comment on lines +2284 to +2287

@claude claude Bot Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The it.each only passes a hostile encoding to res.write()/res.end() — both route into write_or_end. Neither row sets a hostile res.statusMessage (so write_head_impl's to_bun_string at :948 stays on the is_undefined() short-circuit) and neither reaches write_informational, so reverting either of those two reorderings would not break any test in this PR (REVIEW.md: "Confirm deleting each load-bearing clause of your fix breaks at least one test"). Consider a third row, e.g. ["writeHead via statusMessage", 'res.statusMessage = enc; result = res.write("x");', "threw ERR_STREAM_ALREADY_FINISHED"], to pin the write_head_impl fix.

Extended reasoning...

What the gap is

This PR now applies the same "convert arguments before reading response state" reordering to three functions in NodeHTTPResponse.rs:

  1. write_or_end<IS_END> — the original crash fix.
  2. write_head_impl — state checks (is_requested_completed_or_ended(), SOCKET_CLOSED, raw_response, handle_ended_if_necessary) moved from :912 down to :959–978, after status_message_value.to_bun_string(global_object)? at :948.
  3. write_informational — is_done() / raw_response / handle_ended_if_necessary moved to :1251–1259, after Encoding::from_js / from_js_with_encoding_into.

The last two were added in response to the earlier "fix the whole class in the same PR" review comment. But the new it.each at node-http.test.ts:2284-2325 still only exercises the first.

Step-by-step: why neither row reaches the sibling fixes

["write", …] row: res.write(payload, enc) enters the JS write path → handle.cork(() => { handle.writeHead(this.statusCode, this[kSnapshotStatusMessage] ?? this.statusMessage, headers); handle.write(chunk, enc, …) }). The fixture never assigns res.statusMessage, so it is undefined and kSnapshotStatusMessage is unset (only set inside an explicit writeHead()). Native write_head_impl receives status_message_value = undefined, and at :946 !status_message_value.is_undefined() is false — to_bun_string() at :948 never runs, so the moved re-check at :959–978 is never the thing that observes the destroyed state. The hostile enc only fires later inside handle.write → write_or_end<false>, which is the covered path.

["end", …] row: res.flushHeaders() calls writeHead with the same statusMessage = undefined (benign), then res.end(payload, enc) sees headersSent === true and goes straight to write_or_end<true> without re-entering write_head_impl. Again only write_or_end is exercised.

Neither row calls anything that reaches write_informational (that is only entered via res._writeRaw for 1xx responses).

Why REVIEW.md flags this

REVIEW.md, Tests reviewers reject: "Every behavioral change ships an automated test in the same PR" and "Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test." The reordering in write_head_impl and write_informational is a behavioral change (it turns a use-of-stale-raw_response into a clean early-return/throw), but reverting either block to its pre-PR position would leave every test in this PR green.

This is not a duplicate of the existing timeline comments: the earlier sibling-site comment asked for the fix (now applied), and the earlier "cover end()" comment asked for the IS_END = true arm (now the second row). This is the remaining gap: tests for the two applied sibling fixes.

Suggested fix

The fixture already builds a hostile enc = Object.assign(new String("hex"), { [Symbol.toPrimitive]() { res.destroy(); Bun.gc(true); return "hex"; } }), so a third row can reuse it as the status message:

["writeHead via statusMessage", `res.statusMessage = enc; result = res.write("x");`, "threw ERR_STREAM_ALREADY_FINISHED"],

Trace: res.statusMessage = enc (plain data property, no validating setter in http1) → res.write("x") → handle.cork → handle.writeHead(200, enc, …) → write_head_impl :946 !is_undefined() is true → :948 to_bun_string invokes Symbol.toPrimitive → res.destroy() → abort() sets SOCKET_CLOSED and, via on_request_complete(), REQUEST_HAS_COMPLETED → back at the moved :959 is_requested_completed_or_ended() is true → throws ERR_STREAM_ALREADY_FINISHED → propagates out of cork → fixture's catch prints threw ERR_STREAM_ALREADY_FINISHED. (Confirm the exact expected string with bun bd test; if the SOCKET_CLOSED branch at :972 wins instead, the expected becomes "returned boolean" — either way the row pins the reordering.)

Covering write_informational from public API is harder (it's reached via internal _writeRaw for 1xx); if the author considers it not publicly reachable with a hostile object, saying so in the PR description satisfies REVIEW.md's "if a site is intentionally excluded, say so in the PR."

Severity

Nit. The primary crash (write_or_end) is properly tested for both write() and end(), and the two sibling reorderings are structurally identical moves of the same guard block — the risk of one being wrong while write_or_end is right is low. The PR is strictly safer than before; this only tightens the mutation-testing bar the repo's own review guide sets.

"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);
Comment thread
claude[bot] marked this conversation as resolved.
}
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,
Expand Down
Loading