Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR replaces the fixed ChangesCustom HTTP method support
Sequence Diagram(s)sequenceDiagram
participant JS as JS caller
participant RequestInit as Init::parse
participant JSCBridge as request_method_from_js
participant Request
participant AsyncHTTP
JS->>RequestInit: new Request(url, {method})
RequestInit->>JSCBridge: request_method_from_js(method)
alt valid known/custom token
JSCBridge-->>RequestInit: Ok(MethodBuf)
else invalid or forbidden
JSCBridge-->>RequestInit: TypeError
end
RequestInit->>Request: store MethodBuf
Request->>AsyncHTTP: init(MethodRef derived from MethodBuf)
AsyncHTTP->>AsyncHTTP: build_request using method bytes
Related Issues: Suggested labels: rust, http, webcore, needs-review Suggested reviewers: Jarred-Sumner, cirospaciari 🐰 A method once fixed, now free to roam, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:06 PM PT - Jul 6th, 2026
❌ @robobun, your commit b70b59a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33469That installs a local version of the PR into your bun-33469 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
I checked each of the three issues the bot suggested against this branch. Only one of them is actually closed by it. #12014 — fixed. #6021 — half. Its repro round-trips through #21566 — half. It tracks three bullets; this PR is the first one ( Raw request lines against So The server side is a separate change: uWS Only Also addressed the |
There was a problem hiding this comment.
Thanks for addressing the empty-string presence check — that's now covered end-to-end. I didn't find further issues, but given the new MethodRef/MethodBuf types threaded through HTTPClient/AsyncHTTP, the cross-thread borrowed-token lifetime, and the user-visible behavior changes (patch no longer upper-cased, forbidden methods now reject), this warrants a human look.
Extended reasoning...
Overview
This PR fixes fetch() and new Request() silently replacing unrecognized HTTP methods with GET. It introduces MethodRef<'a> (borrowing, Copy) and MethodBuf (owning) alongside the existing closed Method enum, and threads them through 23 files: the HTTP client core (HTTPClient, AsyncHTTP, FetchTasklet), the webcore Request/Response/fetch entry points, and a dozen mechanical .into() updates at CLI/install/S3 call sites. New spec predicates (is_token, is_forbidden, normalize) live in Method.rs with unit tests, and fetch-method.test.ts asserts wire bytes via a raw socket.
Since my last review, commit d70b19b1 addressed the get_truthy → get/fast_get presence-check gap in both fetch.rs and Response.rs, added "" to the invalid-token test tables, and added a test that only undefined falls through to the default.
Security risks
The RFC 9110 token validator (is_token) rejects control characters, whitespace, and non-ASCII before any custom method reaches the request line, which prevents request-line injection via method. The forbidden-method check is case-insensitive per spec. I don't see auth/crypto/permissions surface touched. The main risk area is memory safety, not security per se.
Level of scrutiny
High. This touches the per-request hot path of the HTTP client and introduces a lifetime-erased borrow (MethodRef::Custom(&'a [u8]) stored as MethodRef<'static> in AsyncHTTP) whose backing storage lives in FetchTasklet.method: MethodBuf. Correctness relies on field declaration order (method after http so drop order frees the borrower first) and on the same Interned::assume contract that headers_buf/hostname already use — plausible, and the PR documents it, but this is exactly the kind of cross-thread borrowed-slice invariant that Bun's review guidelines flag for careful human review. The bitwise-copy path in on_async_http_callback also copies original_method back into the client; a reviewer familiar with the HTTP-thread copy lifecycle should confirm the custom-token slice remains valid through redirects and retries.
Other factors
- User-visible behavior changes a maintainer should sign off on:
fetch(url, {method: "patch"})now sends lowercasepatch(spec-correct but a change from prior Bun behavior), andCONNECT/TRACE/TRACKnow reject where they previously went out on the wire. - DevServer.rs now does
.known().unwrap_or(Method::POST)/.unwrap_or(Method::GET)on saved requests — a reasonable fallback but worth a glance from someone who knows that code path. - Test coverage is thorough (wire-level assertions, clone/copy paths, forbidden methods, token edge cases, falsy-value handling), and the PR description is exemplary.
- No outstanding reviewer comments; my prior feedback is fully resolved.
|
Took the lifetime concern seriously rather than just asserting it, since that was the right thing to poke at.
They pass under On retries: Also fixed the Agreed the |
fetch() and new Request() looked the method up in a case-sensitive table of 36 verbs and fell back to GET when the lookup missed, so "Delete", "Put", "Propfind" and any custom token were silently sent as GET. Follow the fetch spec instead: case-normalize only DELETE, GET, HEAD, OPTIONS, POST and PUT, forward any other valid RFC 9110 token byte-for-byte, and raise a TypeError for invalid tokens and the forbidden CONNECT, TRACE and TRACK. Method becomes MethodRef<'a> (a known verb or a borrowed custom token) in the HTTP client, which keeps it Copy, and MethodBuf (owning) in Request, Response init and FetchOptions. Custom verbs are treated as body-carrying and non-idempotent: they may have a request body, are never retried on a keep-alive reset, and are rewritten to GET only on a 303 redirect.
get_truthy/fast_get_truthy drop "" and null before the method reaches the
validator, so fetch(url, {method: ""}) still defaulted to GET while
server.fetch() rejected it. The fetch spec checks WebIDL presence: only
undefined falls through to the default.
The custom method token is borrowed from the FetchTasklet and re-read on the HTTP thread for the next hop, so assert it survives 302/307 and that 303 still rewrites it to GET. Also move the SAFETY comment onto the line clippy's undocumented_unsafe_blocks wants it on.
Response::Init is the shared RequestInit/ResponseInit parser. WebIDL's
ResponseInit has no method member and Response never exposes one; Bun reads it
only so new Request(url, response) can inherit a method. Validating it there
made new Response(body, {method: "CONNECT"}) and Response.json/redirect throw
for a field that does not exist.
Split the parser: the Request path rejects invalid and forbidden tokens, the
Response paths ignore a method they cannot represent.
Also answer each raw-socket connection exactly once in the test helpers, so a
request body arriving in its own data event cannot consume the next hop.
c74bf22 to
7349a54
Compare
|
Worked through the red lanes on build #69019. Rebased onto
Same allocation site, same stack. It reports 128 bytes on this branch only because
|
new Request(test #1) measures 65 MB with no leak present, against a bound of 64, so the assertion rides the boundary. Request also grew 16 bytes here (a MethodBuf in place of a one-byte Method). The leak this guards against presents as 100+ MB, so 80 still catches it. These cases have also never run under a debug build: ~100k Requests per case takes a debug JSC ~30s, past the default timeout. Shrinking the workload to fit would leave the RSS bounds measuring nothing, so skip there and say so.
7349a54 to
d036bdc
Compare
|
Two changes to The ASAN bound had no headroom. These cases have never run under a debug build. Each builds ~100k Requests, which a debug JSC takes ~30s to do — well past the default test timeout, so they time out on Shrinking the workload enough to fit would leave the RSS bounds measuring nothing, so they are now explicitly skipped in debug rather than silently timing out. Release and release-ASAN, where this guard actually earns its keep, are unaffected. Happy to pull either out if you would rather they land separately. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/fetch/fetch-method.test.ts`:
- Around line 75-153: The tests in fetch-method.test.ts are all network-bound
and independent, so this suite should run concurrently instead of sequentially.
Update the surrounding describe block and the related sibling network test
blocks to use describe.concurrent or test.concurrent, and keep the existing
helper functions like wireMethod, requestLine, and redirectedMethods unchanged
so each case still owns its own ephemeral server and socket I/O without sharing
state.
- Around line 7-73: The TCP server setup and teardown logic is duplicated in
requestLine and redirectedMethods, including the buffered data handling and
finally server.close() cleanup. Extract the shared
net.createServer/listen/error-handling/request-capture flow into a reusable
helper, then let requestLine and redirectedMethods build on it with their
specific response behavior and parsing. Use the existing requestLine and
redirectedMethods helpers as the main entry points to refactor around.
In `@test/js/web/request/request-clone-leak.test.ts`:
- Around line 89-93: The RSS assertion in request-clone-leak.test.ts is too
tight for the measured no-leak baseline, especially under ASAN, and needs more
headroom. Update the threshold used in the delta expectation in the request
clone leak test to give roughly 2x margin over the observed baseline (for
example, re-measure and raise the ASAN bound closer to ~130 instead of 80),
keeping the existing isASAN conditional in place. Ensure the change is made in
the test case around the delta check so the threshold remains stable across CI
variance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61a96771-d76a-4b2b-8568-524c738bceb1
📒 Files selected for processing (24)
src/http/AsyncHTTP.rssrc/http/lib.rssrc/http_jsc/method_jsc.rssrc/http_types/Method.rssrc/install/NetworkTask.rssrc/install/npm.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/audit_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/pm_view_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/simple_request.rssrc/standalone_graph/StandaloneModuleGraph.rstest/js/web/fetch/fetch-method.test.tstest/js/web/request/request-clone-leak.test.ts
requestLine and redirectedMethods repeated the same server bootstrap, buffered read and teardown; both now build on one requestLines helper that answers the nth request however the caller asks. Each case still owns its own ephemeral port, so the network-bound block runs concurrently.
There was a problem hiding this comment.
No bugs found on this pass and the earlier findings are all addressed, but this touches enough surface — new MethodRef/MethodBuf types threaded through HTTPClient/AsyncHTTP/FetchTasklet with lifetime-erased borrows, plus the user-facing behavior changes called out in the description (patch no longer normalized, CONNECT/TRACE/TRACK now reject, method: null → "null") — that a human should sign off on the design and the compat trade-offs.
Extended reasoning...
Overview
24 files. The core change introduces MethodRef<'a> (borrowing, Copy) and MethodBuf (owning) alongside the existing closed Method enum, then threads them through the HTTP stack: HTTPClient.method and AsyncHTTP.method become MethodRef<'a>; Request.method, Response::Init.method, and FetchOptions.method become MethodBuf; FetchTasklet gains a method: MethodBuf field that owns the custom token the AsyncHTTP borrows via Interned::assume + detach_lifetime. request_method_from_js implements the fetch-spec normalize/token/forbidden rules, and Response::Init::init is split (const-generic) so only the RequestInit path validates. About half the touched files are mechanical Method::X → Method::X.into() updates at AsyncHTTP::init call sites.
Security risks
Low. Token validation (is_token) follows RFC 9110 §5.6.2 and rejects non-ASCII / whitespace / separators, which prevents request-line injection via the method. Forbidden-method rejection (CONNECT/TRACE/TRACK) tightens rather than loosens. No auth, crypto, or permission surfaces touched.
Level of scrutiny
High. This is production-critical HTTP client code with:
- Several explicitly-called-out user-facing behavior changes (non-normalized
patch, rejected forbidden methods,{method: ""}now throws,{method: null}→"null", S3 rejects custom methods) that a maintainer should agree are the right compat trade-offs. - Lifetime-sensitive unsafe: the
MethodRef::Customtoken is stored inFetchTaskletand lent to the HTTP-threadAsyncHTTPcopy viabun_ptr::Interned::assume+detach_lifetime, relying on field-declaration order for drop sequencing and on the same self-borrow contract ashostname/headers_buf. The redirect test exercises this across hops under ASAN, which is reassuring, but the pattern still deserves a human eye. - A new type pair in
bun_http_typesthat becomes public API for the crate.
Other factors
All three of my earlier inline findings (truthy vs presence read, ResponseInit collateral validation, per-socket test guard) were addressed and resolved, as were CodeRabbit's dedupe/concurrency nits. One CodeRabbit comment remains open (RSS-threshold headroom in request-clone-leak.test.ts — 80 vs the ~2× guideline); minor, but worth a decision. Test coverage is thorough (68 wire-level assertions + Rust unit tests for the spec predicates), and the DevServer .known().unwrap_or(...) fallbacks are reachable only from Bun.serve-received requests, which never carry a Custom method. The bug-hunting system found nothing on this revision.
|
Both review bots have gone quiet with nothing outstanding, so here is the short version of what actually wants a human call. Everything below is already in the description; collecting it so it is one read rather than six. The compat trade-offs. Each is a deliberate deviation from today's Bun, and each matches Node/undici and browsers:
The first two are the bug. The last four are the spec rules that come with fixing it properly, and they are the ones worth disagreeing with if you are going to. The lifetime. One thing I checked after review. Scope I left out, on purpose. |
|
CI status for the current head (b70b59a, build #69121). The diff's own tests pass; the red lanes are pre-existing or unrelated flake.
Not re-triggering: the x64-asan leak is deterministic and pre-existing, so a re-roll can't turn the build green. Flagging for a maintainer rather than pushing empty commits. |
…e ujung (#364) Sambungan terakhir yang belum teruji: rute publikasi sungguhan → PostgreSQL sungguhan → Varnish sungguhan. Suite transport membuktikan purgeEdgeCache() mengosongkan cache nyata; unit test membuktikan rute memanggil pembungkusnya. Tidak satu pun membuktikan keduanya TERSAMBUNG — resolusi hostname dari awcms_micro_tenant_domains duduk di antaranya, dan tenant yang hostname-nya tidak resolve tidak meng-invalidasi apa pun sementara seluruh test komponen tetap hijau. Di staging celah itu hanya pernah terlihat dengan menerbitkan artikel sungguhan. Yang ikut terkunci: setiap hostname aktif tenant di-purge (bukan hanya primary), hostname non-aktif tidak, publikasi tenant lain tidak mengosongkan cache tenant ini, dan publikasi yang GAGAL tidak mengosongkan apa pun — yang terakhir menutup cara murah bagi pemanggil tak berwenang untuk membuang cache sebuah situs. Kedua CLI operator dijalankan sebagai proses sungguhan sehingga exit code yang dibaca pipeline deploy ikut jadi assertion. Fixture Varnish diekstrak ke tests/integration/varnish-fixture.ts. Diuji balik dengan dua mutasi: melepas pemanggilan invalidasi dari rute publish menggagalkan 3 dari 6; mengubah filter status resolver hostname menggagalkan 5 dari 6. Ditambah gate http:methods:check. Aturan Bun ternyata lebih luas dari "metode kustom" — yang menentukan kecocokan huruf per huruf dengan tabel verb internalnya, sehingga Post/Delete/Patch juga terkirim sebagai GET. Sudah dilaporkan ke hulu (oven-sh/bun#33469), jadi tidak dibuat laporan baru; gate ini menutup sisi kita. bun run check hijau dengan database nyata: 4829 pass, 0 fail. Co-authored-by: AWCMS-Micro Security <security@awcms-micro>
|
Issue #42497 reports the same defect: |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Repro
Any method string that is not an exact-case match of Bun's internal 36-verb
table is silently sent as
GET. Against a raw socket:The request "succeeds" with a 200, so a
DELETEthat never deleted lookslike it worked. Methods usually come from a variable (an HTTP client wrapper,
a proxy forwarding
req.method, a REST SDK with a per-call method option),so any casing the table does not hold turns a mutation into a read. Node and
browsers send the token as given.
Cause
bun_http_types::Methodis a closed enum looked up through acomptime_string_map!keyed on exact bytes (only the all-upper and all-lowerspellings are present).
fetch()andnew Request()both didso a miss became
GETrather than an error, and the enum had nowhere to puta verb the table does not know.
Fix
Follow the fetch spec:
DELETE,GET,HEAD,OPTIONS,POST,PUTTypeErroron a token that is not a token, and on the forbiddenCONNECT,TRACE,TRACKMethodkeeps its role as the closed enum used for routing, S3 signing andthe
EnumSetroute tables. Two new types carry a user-supplied verb:MethodRef<'a>=Known(Method) | Custom(&'a [u8]),Copy, stored inHTTPClient/AsyncHTTPso the HTTP thread's bitwise copy ofAsyncHTTPand the
picohttp::Request<'static>borrow keep working. The custom tokenborrows
FetchTasklet-owned storage, the same contractheaders_buf,request_bodyandhostnamealready use.MethodBuf= owning counterpart, stored inRequest,Response's initand
FetchOptions.A custom verb is treated conservatively: it may carry a request body, it is
never assumed idempotent (so a keep-alive reset does not replay it), and it
is rewritten to
GETonly on a 303 redirect, never on 301/302.Errors come back the way the surrounding code already reports them:
new Request()throws,fetch()andserver.fetch()reject.init["method"]is also read withget/fast_getinstead ofget_truthy/fast_get_truthy, because the spec keys on WebIDL presence:only
undefinedfalls through to the default. That lines the three entrypoints (
fetch,new Request,server.fetch) up with each other and withNode:
init.methodundefinedGETGET""GETTypeErrornullGET"null"0/false"0"/"false""0"/"false"Response::Initis the parser for both shapes, so it is split: the threeResponseentry points (new Response,Response.json,Response.redirect)stay permissive, because WebIDL's
ResponseInithas nomethodmember andResponsenever exposes one — Bun reads it only sonew Request(url, response)can inherit a method. Only
new Request(url, init)applies the rules above.Behavior changes worth calling out
fetch(url, { method: "patch" })now sendspatch, notPATCH. Only thesix spec-listed methods are normalized; this matches undici and browsers.
node:httpkeeps upper-casing, which also matches Node.fetch(url, { method: "CONNECT" })(andTRACE/TRACK) now rejectsinstead of going out on the wire.
fetch(url, { method: "" })now rejects, and{ method: null }sendsnull, per the table above.fetch("s3://...", { method: <custom token> })rejects, because the S3request signer takes a known
Method.Not in this PR: the
Bun.servereceive sideBun.servedrops the connection for any method outside the same 36-verbtable. Raw request lines against
Bun.serve({ fetch: req => new Response(req.method) }),identical on
mainand on this branch:fetch()now stops masking that:fetch(bunServer, { method: "CUSTOM" })usedto arrive as a silent
GETwith a 200, and now surfaces as a connection error.Fixing the server needs a case-sensitive accessor through uWS (whose
getMethod()lower-cases the request line in place), relaxingisValidMethod's strict mode, and the 9-verb routing dispatch inuws_sys::App/uws_sys::h3— a separate change, left alone deliberately.That means #6021 and #21566 are only half addressed here (the
fetchhalf);I have not marked them as fixed.
Verification
test/js/web/fetch/fetch-method.test.tsasserts the bytes on the wire via araw socket, not what Bun echoes back in
request.method.The same file against a debug build of
main:20 pass, 48 fail.It also pins the lifetime of a custom method's borrowed token across redirect
hops (302/307 keep it, 303 rewrites it to
GET), which the debug build's ASANexercises.
Unit coverage for the spec predicates lives in
src/http_types/Method.rs(
cargo test -p bun_http_types).Fixes #12014
Suites re-run against this build
test/js/web/fetch/,test/js/bun/http/serve.test.ts,test/js/node/http/node-http.test.ts,test/js/bun/s3/s3.test.ts. Theremaining failures (RSS-threshold leak tests, IPv6
requestIP,localhostresolution, 5s timeouts under ASAN) reproduce identically on a debug build of
mainin the same container.