Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
6 changes: 5 additions & 1 deletion src/jsc/FetchHeaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,11 @@
value: &BunString,
global: &JSGlobalObject,
) -> JsResult<()> {
if self.fast_has(name_) {
// `fast_get` returns None for a zero-length value, so an empty entry is
// treated as absent and overwritten with the default (`Response.json`'s
// `application/json`, StaticRoute's was-string `text/plain`). `fast_has`
// would leave the empty value in place.
if self.fast_get(name_).is_some() {

Check warning on line 150 in src/jsc/FetchHeaders.rs

View check run for this annotation

Claude / Claude Code Review

put_default change makes Response.json diverge from Fetch spec for empty init content-type

Unlike the other changes in this PR (which are scoped to the wire serializer), `put_default` is called at **Response construction time** by `Response.json` (Response.rs:1015), so switching it from `fast_has` to `fast_get(...).is_some()` mutates the JS-observable `Headers`: `Response.json({a:1}, {headers:{'content-type':''}}).headers.get('content-type')` now returns `'application/json;charset=utf-8'` instead of `''` as the Fetch spec (and Node/Chrome/Firefox) require. Not blocking — nobody realis
Comment thread
robobun marked this conversation as resolved.
Outdated
return Ok(());
}

Expand Down
19 changes: 18 additions & 1 deletion src/jsc/bindings/NodeHTTP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,15 @@
const auto& name = WebCore::httpHeaderNameString(header.key);
const auto& value = header.value;

// FetchHeaders strips leading/trailing HTTP whitespace, so an empty value here
// covers `""` and whitespace-only. Treat it as absent: don't write the line and
// don't set the wrote-this-header state bits, so the matching auto-header (Date,
// Content-Type from render_metadata) is emitted exactly once instead of alongside
// an empty duplicate.
if (value.isEmpty()) {
continue;
}
Comment thread
robobun marked this conversation as resolved.

// We have to tell uWS not to automatically insert a TransferEncoding or Date header.
// Otherwise, you get this when using Fastify;
//
Expand Down Expand Up @@ -843,7 +852,9 @@
for (auto& header : internalHeaders.uncommonHeaders()) {
const auto& name = header.key;
const auto& value = header.value;

if (value.isEmpty()) {
continue;
}

Check warning on line 857 in src/jsc/bindings/NodeHTTP.cpp

View check run for this annotation

Claude / Claude Code Review

Set-Cookie loop not given isEmpty() skip → dynamic and static paths still diverge

The `getSetCookieHeaders()` loop at the top of `writeFetchHeadersToUWSResponse` (line 780) and `writeFetchHeadersToH3Response` (line 1536) is a third sibling that didn't get the `isEmpty()` skip, so `{headers:{'set-cookie':''}}` on the dynamic path still emits an empty `set-cookie: ` line while the static/file path (whose `copyTo` snapshot includes Set-Cookie entries and now hits the `value.length == 0` skip) drops it — the exact dynamic/static divergence 3d70b967 was closing. Set-Cookie has no
Comment thread
robobun marked this conversation as resolved.
writeResponseHeader<isSSL>(res, name, value);
}
}
Expand Down Expand Up @@ -1533,6 +1544,9 @@
}

for (const auto& header : internalHeaders.commonHeaders()) {
if (header.value.isEmpty()) {
continue;
}
if (header.key == WebCore::HTTPHeaderName::ContentLength) {
if (!(data->state & uWS::Http3ResponseData::HTTP_WROTE_CONTENT_LENGTH_HEADER)) {
data->state |= uWS::Http3ResponseData::HTTP_WROTE_CONTENT_LENGTH_HEADER;
Expand All @@ -1548,6 +1562,9 @@
}

for (auto& header : internalHeaders.uncommonHeaders()) {
if (header.value.isEmpty()) {
continue;
}
writeOne(header.key, header.value);
}
}
Expand Down
33 changes: 25 additions & 8 deletions src/runtime/server/FileRoute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,12 @@ impl FileRoute {
bun_core::heap::into_raw(Box::new(FileRoute {
ref_count: Cell::new(1),
server: Cell::new(opts.server),
has_last_modified_header: headers.get(b"last-modified").is_some(),
has_content_length_header: headers.get(b"content-length").is_some(),
has_content_range_header: headers.get(b"content-range").is_some(),
has_date_header: headers.get(b"date").is_some(),
has_last_modified_header: headers.get(b"last-modified").is_some_and(|v| !v.is_empty()),
has_content_length_header: headers
.get(b"content-length")
.is_some_and(|v| !v.is_empty()),
has_content_range_header: headers.get(b"content-range").is_some_and(|v| !v.is_empty()),
has_date_header: headers.get(b"date").is_some_and(|v| !v.is_empty()),
blob,
headers,
status_code: opts.status_code,
Expand Down Expand Up @@ -180,10 +182,16 @@ impl FileRoute {
return Ok(Some(bun_core::heap::into_raw(Box::new(FileRoute {
ref_count: Cell::new(1),
server: Cell::new(None),
has_last_modified_header: headers.get(b"last-modified").is_some(),
has_content_length_header: headers.get(b"content-length").is_some(),
has_content_range_header: headers.get(b"content-range").is_some(),
has_date_header: headers.get(b"date").is_some(),
has_last_modified_header: headers
.get(b"last-modified")
.is_some_and(|v| !v.is_empty()),
has_content_length_header: headers
.get(b"content-length")
.is_some_and(|v| !v.is_empty()),
has_content_range_header: headers
.get(b"content-range")
.is_some_and(|v| !v.is_empty()),
has_date_header: headers.get(b"date").is_some_and(|v| !v.is_empty()),
blob,
headers,
status_code,
Expand Down Expand Up @@ -230,6 +238,9 @@ impl FileRoute {
AnyResponse::SSL(s) => {
let s = bun_opaque::opaque_deref_mut(s);
for (name, value) in names.iter().zip(values) {
if value.length == 0 {
continue;
}
s.write_header(sp_slice(*name, buf), sp_slice(*value, buf));
}
if let Some(srv) = self.server.get() {
Expand All @@ -241,6 +252,9 @@ impl FileRoute {
AnyResponse::TCP(s) => {
let s = bun_opaque::opaque_deref_mut(s);
for (name, value) in names.iter().zip(values) {
if value.length == 0 {
continue;
}
s.write_header(sp_slice(*name, buf), sp_slice(*value, buf));
}
if let Some(srv) = self.server.get() {
Expand All @@ -252,6 +266,9 @@ impl FileRoute {
AnyResponse::H3(s) => {
let s = bun_opaque::opaque_deref_mut(s);
for (name, value) in names.iter().zip(values) {
if value.length == 0 {
continue;
}
s.write_header(sp_slice(*name, buf), sp_slice(*value, buf));
}
// tag == .H3 → no alt-svc header
Expand Down
7 changes: 5 additions & 2 deletions src/runtime/server/StaticRoute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@
}

let cached_blob_size = blob.size();
let has_date = headers.get(b"date").is_some();
let has_date = headers.get(b"date").is_some_and(|v| !v.is_empty());
bun_core::heap::into_raw(Box::new(StaticRoute {
ref_count: Cell::new(1),
blob,
Expand Down Expand Up @@ -241,10 +241,10 @@
if !blob.slice().is_empty() {
append_etag(blob.slice(), &mut headers);
}
}

let cached_blob_size = blob.size();
let has_date = headers.get(b"date").is_some();
let has_date = headers.get(b"date").is_some_and(|v| !v.is_empty());

Check warning on line 247 in src/runtime/server/StaticRoute.rs

View check run for this annotation

Claude / Claude Code Review

Remaining key-presence auto-header gates not converted to value-non-empty (etag, Content-Disposition, Content-Range)

A few sibling presence checks that gate other auto-headers weren't converted from key-presence to value-non-empty, so with the new empty-value serializer skip an empty user value now suppresses the auto-header *and* is dropped from the wire, leaving the response with neither: `headers.get(b"etag").is_none()` at `StaticRoute.rs:240` and `:98` (auto-ETag), and `fast_has(ContentDisposition/ContentRange)` at `RequestContext.rs:3633-3634` / `:1834` (auto `filename=…` and auto `Content-Range` on the d
Comment thread
robobun marked this conversation as resolved.
return Ok(Some(bun_core::heap::into_raw(Box::new(StaticRoute {
ref_count: Cell::new(1),
blob,
Expand Down Expand Up @@ -494,6 +494,9 @@

debug_assert_eq!(names.len(), values.len());
for (name, value) in names.iter().zip(values) {
if value.length == 0 {
continue;
}
Comment thread
robobun marked this conversation as resolved.
resp.write_header(
&buf[name.offset as usize..][..name.length as usize],
&buf[value.offset as usize..][..value.length as usize],
Expand Down
155 changes: 155 additions & 0 deletions test/js/bun/http/bun-serve-headers.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,161 @@
import { describe, expect, test } from "bun:test";
import { tempDir } from "harness";
import { once } from "node:events";
import * as net from "node:net";
import * as path from "node:path";

// An empty (or whitespace-only) header value must be treated as absent for the
// well-known headers Bun fills in automatically, so the response head carries
// exactly one Content-Type and exactly one Date (RFC 9110 forbids duplicate
// Content-Type; an origin server MUST send a valid Date).
describe("empty header value does not duplicate auto-headers", () => {
async function readHead(port: number): Promise<string> {
const socket = net.connect(port, "127.0.0.1");
try {
await once(socket, "connect");
socket.write("GET / HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n");
let raw = "";
await new Promise<void>((resolve, reject) => {
socket.on("data", c => (raw += c.toString("latin1")));
socket.on("error", reject);
socket.on("close", resolve);
});
return raw.split("\r\n\r\n")[0];
} finally {
socket.destroy();
}
}

async function rawHead(makeResponse: () => Response): Promise<string> {
using server = Bun.serve({
port: 0,
hostname: "127.0.0.1",
development: false,
fetch: makeResponse,
});
return await readHead(server.port);
}

const lines = (head: string, name: string) => head.split("\r\n").filter(l => l.toLowerCase().startsWith(name + ":"));

for (const [label, value] of [
["empty", ""],
["whitespace", " \t "],
] as const) {
test(`content-type: ${label}`, async () => {
const head = await rawHead(() => new Response("x", { headers: { "content-type": value } }));
const ct = lines(head, "content-type");
expect(ct).toHaveLength(1);
expect(ct[0].toLowerCase()).toBe("content-type: text/plain;charset=utf-8");
});

test(`date: ${label}`, async () => {
const head = await rawHead(() => new Response("x", { headers: { date: value } }));
const date = lines(head, "date");
expect(date).toHaveLength(1);
// auto Date is a valid IMF-fixdate, never an empty value
expect(date[0]).toMatch(/^Date: \S/);
expect(Number.isFinite(new Date(date[0].slice(6)).getTime())).toBe(true);
});
}

test("headers.set('content-type', '')", async () => {
const head = await rawHead(() => {
const r = new Response("x");
r.headers.set("content-type", "");
return r;
});
const ct = lines(head, "content-type");
expect(ct).toHaveLength(1);
expect(ct[0].toLowerCase()).toBe("content-type: text/plain;charset=utf-8");
});

test("Response.json with empty content-type", async () => {
const head = await rawHead(() => Response.json({ a: 1 }, { headers: { "content-type": "" } }));
const ct = lines(head, "content-type");
expect(ct).toHaveLength(1);
expect(ct[0].toLowerCase()).toBe("content-type: application/json;charset=utf-8");
});

test("both empty at once: one of each", async () => {
const head = await rawHead(() => new Response("x", { headers: { "content-type": "", "date": "" } }));
expect(lines(head, "content-type")).toHaveLength(1);
expect(lines(head, "date")).toHaveLength(1);
});

async function rawHeadStatic(response: Response): Promise<string> {
using server = Bun.serve({
port: 0,
hostname: "127.0.0.1",
development: false,
static: { "/": response },
fetch() {
return new Response("unreachable");
},
});
return await readHead(server.port);
}
Comment thread
robobun marked this conversation as resolved.

test("static route: content-type empty", async () => {
const head = await rawHeadStatic(new Response("x", { headers: { "content-type": "" } }));
const ct = lines(head, "content-type");
expect(ct).toHaveLength(1);
expect(ct[0].toLowerCase()).toBe("content-type: text/plain;charset=utf-8");
});

test("static route: date empty", async () => {
const head = await rawHeadStatic(new Response("x", { headers: { date: "" } }));
const date = lines(head, "date");
expect(date).toHaveLength(1);
expect(date[0]).toMatch(/^Date: \S/);
expect(Number.isFinite(new Date(date[0].slice(6)).getTime())).toBe(true);
});

test("static route: both empty", async () => {
const head = await rawHeadStatic(new Response("x", { headers: { "content-type": "", "date": "" } }));
const ct = lines(head, "content-type");
const date = lines(head, "date");
expect({ ct: ct.length, date: date.length }).toEqual({ ct: 1, date: 1 });
expect(ct[0].toLowerCase()).toBe("content-type: text/plain;charset=utf-8");
expect(date[0]).toMatch(/^Date: \S/);
});

test("static route: non-empty user values preserved", async () => {
const head = await rawHeadStatic(
new Response("x", { headers: { "content-type": "text/html", "date": "Sun, 06 Oct 2024 13:37:01 GMT" } }),
);
expect(lines(head, "content-type")).toEqual(["Content-Type: text/html"]);
expect(lines(head, "date")).toEqual(["Date: Sun, 06 Oct 2024 13:37:01 GMT"]);
});

test("file route: date empty", async () => {
using dir = tempDir("serve-file-empty-date", { "a.txt": "x" });
const head = await rawHeadStatic(
new Response(Bun.file(path.join(String(dir), "a.txt")), { headers: { date: "" } }),
);
const date = lines(head, "date");
expect(date).toHaveLength(1);
expect(date[0]).toMatch(/^Date: \S/);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test("non-empty user values are still preserved", async () => {
const head = await rawHead(
() => new Response("x", { headers: { "content-type": "text/html", "date": "Sun, 06 Oct 2024 13:37:01 GMT" } }),
);
expect(lines(head, "content-type")).toEqual(["Content-Type: text/html"]);
expect(lines(head, "date")).toEqual(["Date: Sun, 06 Oct 2024 13:37:01 GMT"]);
});

test("empty custom header is dropped on both paths", async () => {
for (const head of [
await rawHead(() => new Response("x", { headers: { "x-custom": "", "content-type": "text/html" } })),
await rawHeadStatic(new Response("x", { headers: { "x-custom": "", "content-type": "text/html" } })),
]) {
expect(lines(head, "x-custom")).toEqual([]);
expect(lines(head, "content-type")).toEqual(["Content-Type: text/html"]);
}
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

// https://github.com/oven-sh/bun/issues/9180
test("weird headers", async () => {
Expand Down
Loading