Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 8 additions & 5 deletions packages/bun-uws/src/HttpParser.h
Original file line number Diff line number Diff line change
Expand Up @@ -595,12 +595,15 @@ namespace uWS
}


/* RFC 9112 3: exactly one SP separates method and request-target */
bool isHTTPMethod = (__builtin_expect(data[0] == 32 && data[1] == '/', 1));
bool isConnect = !isHTTPMethod && ((data - start) == 7 && data[0] == 32 && memcmp(start, "CONNECT", 7) == 0);
/* RFC 9112 3: exactly one SP separates method and request-target.
* Accept origin-form ("/...") and asterisk-form ("*", RFC 9112 3.2.4,
* e.g. `OPTIONS * HTTP/1.1`). Like Node.js (llhttp), any method may
* carry an asterisk-form target and it is delivered verbatim. */
bool isOriginOrAsteriskForm = (__builtin_expect(data[0] == 32 && (data[1] == '/' || data[1] == '*'), 1));
Comment thread
robobun marked this conversation as resolved.
Resolved
bool isConnect = !isOriginOrAsteriskForm && ((data - start) == 7 && data[0] == 32 && memcmp(start, "CONNECT", 7) == 0);
Comment thread
robobun marked this conversation as resolved.
Resolved
/* Also accept proxy-style absolute URLs (http://... or https://...) as valid request targets */
bool isProxyStyleURL = !isHTTPMethod && !isConnect && data[0] == 32 && isHTTPorHTTPSPrefixForProxies(data + 1, end) == 1;
if (isHTTPMethod || isConnect || isProxyStyleURL) [[likely]] {
bool isProxyStyleURL = !isOriginOrAsteriskForm && !isConnect && data[0] == 32 && isHTTPorHTTPSPrefixForProxies(data + 1, end) == 1;
if (isOriginOrAsteriskForm || isConnect || isProxyStyleURL) [[likely]] {
header.key = {start, (size_t) (data - start)};
data++;
if(!isValidMethod(header.key, useStrictMethodValidation)) {
Expand Down
79 changes: 79 additions & 0 deletions test/js/node/http/node-http-asterisk-form-target.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { describe, expect, test } from "bun:test";
import { once } from "node:events";
import http from "node:http";
import net from "node:net";

// RFC 9112 3.2.4: the asterisk-form request-target ("*") identifies the
// server as a whole and is sent by `OPTIONS * HTTP/1.1`. The request-line
// parser only accepted targets beginning with "/", so asterisk-form requests
// were answered with 400 Bad Request before ever reaching the handler. Node
// (llhttp) delivers them with `req.url === "*"`, for any method, so Bun does
// the same.

Check warning on line 11 in test/js/node/http/node-http-asterisk-form-target.test.ts

View check run for this annotation

Claude / Claude Code Review

Test comments exceed 3-line max and narrate bug history

nit: per CLAUDE.md rule 13 ("Keep code comments to 3 lines max") and the Code style section ("no bug history — that belongs in the PR description"), this 6-line header narrates bug history that's already in the PR description. Same for lines 63-64 ("so it was rejecting asterisk-form targets too"). Trim both to ≤3 lines stating only the durable invariant — e.g. the RFC 9112 3.2.4 reference and that Node (llhttp) delivers `req.url === "*"` for any method.
Comment thread
robobun marked this conversation as resolved.
Resolved

async function writeAndCollect(port: number, requestLine: string) {
const client = net.connect(port);
try {
client.on("error", () => {});
const chunks: Buffer[] = [];
client.on("data", chunk => chunks.push(chunk));
const closed = once(client, "close");
client.write(`${requestLine}\r\nHost: localhost\r\nConnection: close\r\n\r\n`);
await closed;
return Buffer.concat(chunks).toString();
} finally {
client.destroy();
}
}

describe("node:http server", () => {
// llhttp accepts asterisk-form for any method, not only OPTIONS.
test.each(["OPTIONS", "GET"])("delivers `%s * HTTP/1.1` to the handler with url '*'", async method => {
const { promise: handled, resolve } = Promise.withResolvers<{ method: string; url: string }>();
await using server = http
.createServer((req, res) => {
resolve({ method: req.method!, url: req.url! });
res.end("hi");
})
.listen(0);
await once(server, "listening");

const wire = await writeAndCollect((server.address() as net.AddressInfo).port, `${method} * HTTP/1.1`);
expect(wire).toStartWith("HTTP/1.1 200 OK\r\n");
expect(wire).toEndWith("\r\n\r\nhi");
expect(await handled).toEqual({ method, url: "*" });
});

test("an asterisk-form target with a query is delivered verbatim, like Node", async () => {
const { promise: handled, resolve } = Promise.withResolvers<string>();
await using server = http
.createServer((req, res) => {
resolve(req.url!);
res.end();
})
.listen(0);
await once(server, "listening");

const wire = await writeAndCollect((server.address() as net.AddressInfo).port, "OPTIONS *?a=1 HTTP/1.1");
expect(wire).toStartWith("HTTP/1.1 200 OK\r\n");
expect(await handled).toBe("*?a=1");
});
});

describe("Bun.serve", () => {
// Bun.serve shares the same request-line parser, so it was rejecting
// asterisk-form targets too.
test("delivers `OPTIONS * HTTP/1.1` to fetch()", async () => {
const { promise: handled, resolve } = Promise.withResolvers<string>();
await using server = Bun.serve({
port: 0,
fetch(req) {
resolve(req.method);
return new Response("hi");
},
});

const wire = await writeAndCollect(server.port, "OPTIONS * HTTP/1.1");
expect(wire).toStartWith("HTTP/1.1 200 OK\r\n");
expect(await handled).toBe("OPTIONS");
});
});
Loading