Repository navigation
Conversation
|
How this was reproduced: On a release build of main at 13a98b0: On a release build of this change: The other rows of #44644 (a static route, a |
…equest is HEAD `new Response(s3file)` is a 302 whose Location is a presigned URL. The constructor signs that URL for GET before a request exists. SigV4 signs the method, and a client keeps HEAD across a 302, so S3 answers a followed HEAD request with 403 SignatureDoesNotMatch. The Response now keeps its S3 store and the URL that it wrote. When a handler returns it for a HEAD request, Bun.serve signs the URL for HEAD before it writes the headers. A Location that the handler changed is sent as it is. `presign_redirect` is the one place that signs this redirect, and only a HEAD request gets a HEAD signature. Static routes are not changed.
3ac420d to
1759627
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughS3 responses now use 15-minute presigned URLs. Bun.serve() signs HEAD requests for HEAD and other requests for GET. HEAD rendering re-signs an unchanged S3 redirect location and routes signing errors to the error handler. Tests and documentation cover method selection and response behavior. ChangesS3 response redirects
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change’s HEAD redirect behavior and documented edge cases are supported by the inspected implementation and tests. No new material merge risk was identified; proceed with normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the S3Redirect ownership through Init::clone/Drop (the RefPtr<Store> and the BunString are both RAII, so response.clone() and new Response(null, res) balance their refs), the detached-server branch of render_s3_redirect_head (it falls through to the normal HEAD render with the GET-signed Location, no crash), and the Latin-1 vs 16-bit Location comparison in sign_s3_redirect_for_head (compares code units, not bytes, so a user-set 16-bit header is left untouched) — none of those turned up a problem.
Extended reasoning...
The change touches the Bun.serve HEAD rendering path in RequestContext.rs, Response.rs construction and Init state, and the S3 presigning helper in Store.rs, plus tests and docs. The security-relevant surface is SigV4 presigned URL generation for S3 redirects. Four inline findings are posted, so a human look is already signalled; this note only records the adjacent concerns examined and ruled out.
…ders The HEAD signature was written into the header map of the Response. A `Headers` object that the handler got from `response.headers` shares that map, so it showed the HEAD URL after the send. `sign_s3_redirect_for_head` now signs into a copy of the map, and the Response sends the copy. The `Headers` object of the handler keeps the GET URL. The debug assertion after the send is removed. A Response that has an S3 file in its body answers HEAD after a request to S3, so its headers are still there when the assertion runs.
Part of #44644 (row 1)
Problem
Bun.servehandler returnsnew Response(s3file). GET works. HEAD ends in403 SignatureDoesNotMatchfrom S3. No user reported it.src/runtime/webcore/Response.rs:1076). SigV4 signs the method, and a client keeps HEAD across a 302.Fix
Responsekeeps its S3 store and the URL it wrote (S3RedirectinInit). On HEAD,Bun.servesigns for HEAD into a copy of the headers and sends the copy.presign_redirectis the one signer. PUT, DELETE and POST keep the GET URL.new Response(s3file): a static route and some copies answer a URL that S3 refuses #44644).test/js/bun/s3/s3.test.ts(16 new tests, 15 fail on main). Self-reviewed: 30 concerns raised, 27 addressed (Notes).Background
Initis whatclone()andnew Response(null, res)copy. Hono answers HEAD with the second form.Init, so a response rebuilt fromres.headersloses it.Downsides
Responsegrows from 280 to 288 bytes (same 320-byte mimalloc block). Without S3: HEAD +2 instructions, finalize +8.mi_malloc.Location, worked before and gets 403 now.Notes
History. #35584 was the first attempt. It signed a second time on HEAD and kept the store in a field of
Response. A cleanup of stale pull requests closed it, and no person reviewed the approach. This PR keeps its idea. The state is now inInit, so the two copies thatInit::clonemakes carry it.What the tests do. They start the S3 server of
test/packages/s3-serverin the test process. That server checks the signature of a presigned URL, the method included, and it records each request.{ GET: fn },{ GET, HEAD },error()after a throw and after a rejection,response.clone(),new Response(null, response),new Response(body, response). Each one asserts that a followed GET and a followed HEAD get 200 and that the origin sawGET 200,HEAD 200.Locationthat the handler replaced (8-bit and 16-bit string) or deleted is sent as it is. ALocationthat the handler set again to the same value is still signed for HEAD. An added header is kept.error(). The signer writes the text it signs into a buffer of 4096 bytes. The test finds the longest access key id for which the GET text fits. The HEAD text is then one byte too long.Headersobject that the handler took fromresponse.headersstill has the GET URL.Responsethat code sees does not change: a 302, a GET URL inLocation, a throw for missing credentials and for aResponseInit. This test passes on main too.Suites on the debug build of this branch (main at bbdc5a5).
test/js/bun/s3/s3.test.ts323 pass.test/js/bun/http/:bun-server80,bun-serve-static46,bun-serve-routes61,serve-reused-response11,bun-serve-headers9,serve.test.ts -t HEAD29.test/js/web/fetch/body-clone100.test/js/web/fetch/response.test.ts: 22 pass, andhandle stack overflowexceeds its 5 s limit on the debug build (5.7 s). It passes on the release build of this change. Clippy onbun_runtimeandtest/internal/source-lintspass.Measurements. Release builds of main at 13a98b0 and of this change on that commit. The later rebase onto bbdc5a5 changes no file of this diff. Instruction counts are from a ptrace single-step counter over one function, callees included.
perfand valgrind are not available in the container.size_of::<Response>()(objdump, the allocation inResponseClass__constructError)mi_usable_size)estimateShallowMemoryUsageOf(new Response())new Response(s3file), allocator calls per construction (gdbbreakpoint counts, N = 1000 and 3000)mi_malloc, 23.02mi_reallocgdbbreakpoint onsign_request)on_response, GET with a bodymi_mallocand 23mi_realloccallsResponse::destroyStaticRoute::on, a plain static routesize)render_metadata,do_render_head_response,do_render_with_body,AnyRoute::from_jsandStaticRoute::{on, on_head, on_request, do_write_headers, do_render_blob_corked}have the same instructions on both builds (objdump, addresses masked).nm:sign_s3_redirect_for_head801,presign_redirect172,render_s3_redirect_head8 x 149,Init::clone339 (it is no longer inlined),Response::constructor+143,Response::destroy+92.Which copies keep the state. A HEAD request, this change:
Locationis signed fornew Response(s3file), also from a subclass ofResponseresponse.clone()new Response(null, response),new Response(body, response)new Response(null, response)whereresponsehas an own property, is frozen, or is a subclass instancenew Response(null, { status: response.status, headers: response.headers })Response.redirect(response.headers.get("location"))new HTMLRewriter().transform(response)The GET rows are as on main. The docs now give
Response.redirect(s3file.presign({ method }))for code that does not send theResponseof the constructor.The choice. The state is on
Init. That costs 8 bytes for eachResponseand followsclone()andnew Response(null, res). AFetchHeadersmember would also follow a response that code built again fromres.headers. It changes the class behind eachHeaders,RequestandResponse, and the state would follow a copy of the headers into objects that are not this redirect.The HEAD URL goes to the wire only.
sign_s3_redirect_for_headcopies the header map, puts the HEAD URL into the copy, and theResponsesends the copy. AHeadersobject that the handler took fromresponse.headersshares the first map, so it keeps the GET URL. The first push wrote into the shared map and asserted in debug builds that the send took the headers. Review found two faults in that: theHeadersobject of the handler showed the HEAD URL, andnew Response([s3file], response)failed the assertion, because that body answers HEAD after a request to S3. That shape is the one case where the copy stays on theResponseafterrender_s3_redirect_headreturns: it is there until S3 answers. Each other body is sent in that call, and the send takes the copy.Self-review. 30 concerns, 27 addressed. The main ones: the static route kind and the signature at send for GET are out of this PR, the state does not change after the constructor, the URL is one string that the state and the header map share, the HEAD signature is made only when a response is written, the docs example has a fixed key and shows the
presign()form. Not taken:presign().presign()signs other sets of parameters, and S3: some paths droppartSize,acl,storageClassorrequestPayer#44289 has an open decision on that.Left for later.
new Response(s3file): a static route and some copies answer a URL that S3 refuses #44644: static routes (rows 2 and 3), aResponsethat is older than its signature (row 4), aResponsethat code built again from the headers (row 5).new Response(s3file.slice(a, b))redirects to the whole object #43930: a slice redirects to the whole object. It is not part of this change.request_payer: falseis as on main. S3Client.list and new Response(s3File): send requestPayer #44311 owns it. After this PR it is one line inpresign_redirect, and it then covers the HEAD signature too. feat(s3): sign content-length into a presigned URL #37148 adds a field to the sameSignOptionsliteral.presign_redirect, which now has two callers.Response.rsandRequestContext.rs. The new code reachesinitthroughinit.with_mut,clone_init_headersandset_init_headers.Responsewhose body is an S3 blob can panic (new Response([s3file])).new Response(s3file)does not reach that arm.new Response([s3file], response)does, and this PR does not change that arm.[human-review] gate passed · iteration 4 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 4
evidence per changed file