diff --git a/docs/runtime/s3.mdx b/docs/runtime/s3.mdx index 9927d3260ff9..3d2139241043 100644 --- a/docs/runtime/s3.mdx +++ b/docs/runtime/s3.mdx @@ -294,6 +294,29 @@ Response (0 KB) { } ``` +The URL is signed for `GET` and is valid for 15 minutes. + +S3 checks the method of a presigned URL. When a `Bun.serve()` handler returns this response for a `HEAD` request, Bun signs the URL for `HEAD`. Every other request gets the `GET` URL. A copy made with `response.clone()` or `new Response(null, response)` behaves the same. + +```ts server.ts icon="/icons/typescript.svg" +import { s3, serve } from "bun"; + +serve({ + routes: { + "/report.pdf": () => new Response(s3.file("report.pdf")), + }, +}); +``` + +The `location` header that your code reads from the `Response` is always the `GET` URL. Code that sends the redirect in another way, for example through `server.fetch()` or with a response built from these headers, must sign for the method of the request: + +```ts s3.ts icon="/icons/typescript.svg" +function download(request: Request) { + const method = request.method === "HEAD" ? "HEAD" : "GET"; + return Response.redirect(s3file.presign({ method, expiresIn: 15 * 60 })); +} +``` + --- ## Support for S3-Compatible Services diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 813b8502a282..eda8183de46f 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -867,13 +867,8 @@ where self.response_root.set_rooted(value, global_this); if self.method == Method::HEAD { - if let Some(resp) = self.resp.get() { - let mut pair = HeaderResponsePair { - this: self, - response, - }; - resp.run_corked_with_type(Self::do_render_head_response, &raw mut pair); - } + // SAFETY: `response` is the live, protect()'d cell pointer. + unsafe { self.render_head(response) }; return; } @@ -2773,13 +2768,8 @@ where ctx.response_root.clear(); response_value.ensure_still_alive(); if ctx.method == Method::HEAD { - if let Some(resp) = ctx.resp.get() { - let mut pair = HeaderResponsePair { - this: ctx, - response, - }; - resp.run_corked_with_type(Self::do_render_head_response, &raw mut pair); - } + // SAFETY: `response` is the live, rooted cell pointer. + unsafe { ctx.render_head(response) }; return; } else { // SAFETY: `response` is the live, rooted cell pointer. @@ -2831,13 +2821,8 @@ where ctx.response_root.clear(); fulfilled_value.ensure_still_alive(); if ctx.method == Method::HEAD { - if let Some(resp) = ctx.resp.get() { - let mut pair = HeaderResponsePair { - this: ctx, - response, - }; - resp.run_corked_with_type(Self::do_render_head_response, &raw mut pair); - } + // SAFETY: `response` is the live, rooted cell pointer. + unsafe { ctx.render_head(response) }; return; } // SAFETY: `response` is the live, rooted cell pointer. @@ -3981,6 +3966,43 @@ where .set(unsafe { response::WeakRef::init_ref(response) }); } + /// Safety: the contract of [`Self::set_response`]. + #[inline(always)] + unsafe fn render_head(&self, response: *mut Response) { + // SAFETY: caller contract: `response` is live. + if unsafe { (*response).has_s3_redirect() } { + // SAFETY: caller contract. + return unsafe { self.render_s3_redirect_head(response) }; + } + if let Some(resp) = self.resp.get() { + let mut pair = HeaderResponsePair { + this: self, + response, + }; + resp.run_corked_with_type(Self::do_render_head_response, &raw mut pair); + } + } + + /// Safety: the contract of [`Self::set_response`]. + #[cold] + #[inline(never)] + unsafe fn render_s3_redirect_head(&self, response: *mut Response) { + let Some(resp) = self.resp.get() else { return }; + if let Some(server) = self.server.get() { + // SAFETY: caller contract: `response` is live. The borrow ends before the error handler runs. + let signed = unsafe { (*response).sign_s3_redirect_for_head(server.global_this()) }; + if let Err(err) = signed { + self.run_error_handler(err); + return; + } + } + let mut pair = HeaderResponsePair { + this: self, + response, + }; + resp.run_corked_with_type(Self::do_render_head_response, &raw mut pair); + } + /// # Safety /// Same contract as [`Self::set_response`]. pub(crate) unsafe fn render(&self, response: *mut Response) { @@ -3990,13 +4012,8 @@ where // handler path branches to `do_render_head_response` before reaching // here, but the `error()` handler paths call `render()` directly. if self.method == Method::HEAD { - if let Some(resp) = self.resp.get() { - let mut pair = HeaderResponsePair { - this: self, - response, - }; - resp.run_corked_with_type(Self::do_render_head_response, &raw mut pair); - } + // SAFETY: caller contract. + unsafe { self.render_head(response) }; return; } diff --git a/src/runtime/webcore/Response.rs b/src/runtime/webcore/Response.rs index cfb5970bc8fd..d6de4a41f06c 100644 --- a/src/runtime/webcore/Response.rs +++ b/src/runtime/webcore/Response.rs @@ -227,6 +227,10 @@ pub(crate) struct Response { abort_listener: JsCell>>, } +// A larger Response leaves the 320-byte size class of mimalloc for the 384-byte one. +#[cfg(not(debug_assertions))] +const _: () = assert!(mem::size_of::() <= 320); + impl Default for Response { fn default() -> Self { Self { @@ -404,6 +408,54 @@ impl Response { self.init.with_mut(|init| init.headers.take()) } + #[inline] + pub(crate) fn has_s3_redirect(&self) -> bool { + self.init.get().s3_redirect.is_some() + } + + /// Signs the `Location` of a `new Response(s3file)` for HEAD in a copy of the headers, while it is still the URL that Bun wrote. + #[cold] + #[inline(never)] + pub(crate) fn sign_s3_redirect_for_head(&self, global: &JSGlobalObject) -> Result<(), JSValue> { + let store = self.init.with_mut(|init| { + let state = init.s3_redirect.as_deref()?; + let location = init + .headers + .as_deref_mut()? + .fast_get(HTTPHeaderName::Location)?; + let written = state.location.latin1(); + let unchanged = if location.is_16bit() { + let units = location.utf16_slice(); + units.len() == written.len() + && units.iter().zip(written).all(|(&u, &b)| u == u16::from(b)) + } else { + location.slice() == written + }; + unchanged.then(|| state.store.clone()) + }); + let Some(store) = store else { + return Ok(()); + }; + let result = super::blob::store::presign_redirect(store.data.as_s3(), Method::HEAD) + .map_err(|err| crate::webcore::s3::client::get_js_sign_error(err.into(), global))?; + // A `Headers` object that JS got from this Response shares the map, and it keeps the GET URL. + let copy = self + .clone_init_headers(global) + .map_err(|err| global.take_exception(err))?; + let Some(mut headers) = copy else { + return Ok(()); + }; + headers + .put( + HTTPHeaderName::Location, + &BunString::ascii(&result.url), + global, + ) + .map_err(|err| global.take_exception(err))?; + self.set_init_headers(Some(headers)); + Ok(()) + } + #[inline] pub(crate) fn get_method(&self) -> Method { self.init.get().method @@ -1070,41 +1122,29 @@ impl Response { ..Default::default() }; - let s3 = blob.store.get().as_ref().unwrap().data.as_s3(); - let credentials = s3.get_credentials(); - - let result = match credentials.sign_request::( - &bun_s3_signing::SignOptions { - path: s3.path(), - method: Method::GET, - content_hash: None, - content_md5: None, - search_params: None, - content_disposition: None, - content_type: None, - content_encoding: None, - acl: None, - storage_class: None, - request_payer: false, - }, - Some(bun_s3_signing::SignQueryOptions { expires: 15 * 60 }), - ) { - Ok(r) => r, - Err(sign_err) => { - return Err(crate::webcore::s3::client::throw_sign_error( - sign_err.into(), - global_this, - )); - } - }; + let store = blob.store.get().as_ref().unwrap(); + let result = + match super::blob::store::presign_redirect(store.data.as_s3(), Method::GET) + { + Ok(r) => r, + Err(sign_err) => { + return Err(crate::webcore::s3::client::throw_sign_error( + sign_err.into(), + global_this, + )); + } + }; // `defer result.deinit()` — SignResult: Drop frees owned buffers at scope exit. response.redirected.set(true); let headers = response.get_or_create_headers(global_this)?; - headers.put( - HTTPHeaderName::Location, - &BunString::ascii(&result.url), - global_this, - )?; + let location = BunString::clone_latin1(&result.url); + headers.put(HTTPHeaderName::Location, &location, global_this)?; + response.init.with_mut(|init| { + init.s3_redirect = Some(Box::new(S3Redirect { + store: store.clone(), + location, + })); + }); return Ok(bun_core::heap::into_raw(Box::new(response))); } } @@ -1187,6 +1227,15 @@ pub(crate) struct Init { pub(crate) status_code: u16, pub(crate) status_text: BunString, pub method: Method, + /// Set by `new Response(s3file)`: Bun.serve signs that redirect for HEAD when the request is HEAD. + pub(crate) s3_redirect: Option>, +} + +#[derive(Clone)] +pub(crate) struct S3Redirect { + store: RefPtr, + /// The `Location` value that Bun wrote. Any other value is the user's and is sent as it is. + location: BunString, } impl Default for Init { @@ -1196,6 +1245,7 @@ impl Default for Init { status_code: 0, status_text: BunString::EMPTY, method: Method::GET, + s3_redirect: None, } } } @@ -1214,6 +1264,7 @@ impl Init { status_code: self.status_code, status_text: self.status_text.clone(), method: self.method, + s3_redirect: self.s3_redirect.clone(), }) } diff --git a/src/runtime/webcore/blob/Store.rs b/src/runtime/webcore/blob/Store.rs index 33a6aaa89aba..6c821040d1c6 100644 --- a/src/runtime/webcore/blob/Store.rs +++ b/src/runtime/webcore/blob/Store.rs @@ -19,6 +19,7 @@ use crate::webcore::s3::client::{ S3ListObjectsResult, }; use bun_core::strings; +use bun_http_types::Method::Method; use bun_http_types::MimeType::MimeType; use bun_ptr::RefPtr; @@ -426,6 +427,34 @@ impl S3Ext for S3 { } } +/// Signs the URL of the `new Response(s3file)` redirect: for HEAD when the request is HEAD, for GET in each other case. +#[inline(never)] +pub(crate) fn presign_redirect( + s3: &S3, + request_method: Method, +) -> Result { + s3.get_credentials().sign_request::( + &bun_s3_signing::SignOptions { + path: s3.path(), + method: if request_method == Method::HEAD { + Method::HEAD + } else { + Method::GET + }, + content_hash: None, + content_md5: None, + search_params: None, + content_disposition: None, + content_type: None, + content_encoding: None, + acl: None, + storage_class: None, + request_payer: false, + }, + Some(bun_s3_signing::SignQueryOptions { expires: 15 * 60 }), + ) +} + impl BytesExt for Bytes { /// Adopt an mmap'd region. `Drop` (`allocator.free`) will `munmap` it. #[cfg(unix)] diff --git a/test/js/bun/s3/s3.test.ts b/test/js/bun/s3/s3.test.ts index f0495d6b4e7b..1fad36b7599c 100644 --- a/test/js/bun/s3/s3.test.ts +++ b/test/js/bun/s3/s3.test.ts @@ -4,7 +4,7 @@ import { afterAll, describe, expect, it } from "bun:test"; import { createHash, createHmac, randomUUID } from "crypto"; import { bunEnv, bunExe, getSecret, isCI, tempDir, tempDirWithFiles } from "harness"; import path from "path"; -import { spawnServer } from "s3-server"; +import { serve, spawnServer } from "s3-server"; const s3 = (...args) => defaultS3.file(...args); const S3 = (...args) => new S3Client(...args); @@ -2169,3 +2169,218 @@ describe("presigned url signature", () => { } }); }); + +describe.concurrent("new Response(S3File) from a Bun.serve handler", () => { + const body = "0123456789"; + + // An origin in this process, so that a test reads what it answered. + async function startOrigin() { + const origin = serve({ buckets: ["redirect"] }); + const client = new S3Client(origin.clientOptions("redirect")); + await client.write("object.txt", body); + origin.requests.length = 0; + return { + client, + response: () => new Response(client.file("object.txt")), + /** The requests that the origin answered since the last call. */ + answered: () => origin.requests.splice(0).map(record => `${record.method} ${record.status}`), + [Symbol.asyncDispose]: () => origin.stop(), + }; + } + + async function follow(url: string | URL, method: string) { + const response = await fetch(url, { method }); + return { status: response.status, body: await response.text() }; + } + + async function redirectOf(url: string | URL, method: string) { + const response = await fetch(url, { method, redirect: "manual" }); + return { status: response.status, location: response.headers.get("location") }; + } + + const nextTask = () => new Promise(resolve => setImmediate(resolve)); + + describe("a followed GET and a followed HEAD reach the object", () => { + const handlers: [string, (make: () => Response) => object][] = [ + ["from a handler", make => ({ fetch: () => make() })], + ["from a handler that returns a fulfilled promise", make => ({ fetch: () => Promise.resolve(make()) })], + [ + "from a handler that returns a pending promise", + make => ({ + fetch: async () => { + await nextTask(); + return make(); + }, + }), + ], + ["from a route function", make => ({ routes: { "/": () => make() } })], + ["from a GET route function", make => ({ routes: { "/": { GET: () => make() } } })], + ["from a HEAD route function", make => ({ routes: { "/": { GET: () => make(), HEAD: () => make() } } })], + [ + "from error() after a throw", + make => ({ + fetch() { + throw new Error("answered by error()"); + }, + error: () => make(), + }), + ], + [ + "from error() after a rejection", + make => ({ + fetch: async () => { + await nextTask(); + throw new Error("answered by error()"); + }, + error: () => make(), + }), + ], + ["as response.clone()", make => ({ fetch: () => make().clone() })], + ["as new Response(null, response)", make => ({ fetch: () => new Response(null, make()) })], + ["as new Response(body, response)", make => ({ fetch: () => new Response("a body", make()) })], + ]; + it.each(handlers)("%s", async (_, options) => { + await using origin = await startOrigin(); + using server = Bun.serve({ port: 0, ...options(origin.response) } as any); + expect(await follow(server.url, "GET")).toEqual({ status: 200, body }); + expect(await follow(server.url, "HEAD")).toEqual({ status: 200, body: "" }); + expect(origin.answered()).toEqual(["GET 200", "HEAD 200"]); + }); + }); + + it("the URL is signed for HEAD only when the request is HEAD", async () => { + await using origin = await startOrigin(); + using server = Bun.serve({ port: 0, fetch: () => origin.response() }); + + const accepted: Record = {}; + for (const method of ["GET", "HEAD", "POST", "PUT", "DELETE"]) { + const { status, location } = await redirectOf(server.url, method); + expect(status).toBe(302); + accepted[method] = []; + for (const replay of ["GET", "HEAD", "PUT", "DELETE"]) { + const response = await fetch(location!, { + method: replay, + body: replay === "PUT" ? "overwritten" : undefined, + }); + await response.arrayBuffer(); + if (response.ok) accepted[method].push(replay); + } + } + expect(accepted).toEqual({ GET: ["GET"], HEAD: ["HEAD"], POST: ["GET"], PUT: ["GET"], DELETE: ["GET"] }); + expect(await origin.client.file("object.txt").text()).toBe(body); + }); + + it("a Location that the handler changed is sent as it is, and an unchanged one is signed", async () => { + await using origin = await startOrigin(); + // A string with 16-bit storage, as a string from a UTF-16 source has. + const wide = (text: string) => Buffer.from(text, "utf16le").toString("utf16le"); + const edits: Record void> = { + "/replaced": headers => headers.set("location", "https://example.com/elsewhere"), + "/replaced-wide": headers => headers.set("location", wide("https://example.com/elsewhere")), + "/deleted": headers => headers.delete("location"), + "/same": headers => headers.set("location", headers.get("location")!), + "/same-copy": headers => headers.set("location", Buffer.from(headers.get("location")!).toString()), + "/same-wide": headers => headers.set("location", wide(headers.get("location")!)), + "/other-header": headers => headers.set("x-added", "kept"), + }; + using server = Bun.serve({ + port: 0, + fetch(request) { + const response = origin.response(); + edits[new URL(request.url).pathname](response.headers); + return response; + }, + }); + + const sent: Record = {}; + for (const path of Object.keys(edits)) { + const response = await fetch(new URL(path, server.url), { method: "HEAD", redirect: "manual" }); + const location = response.headers.get("location"); + sent[path] = location?.startsWith("https://example.com/") + ? location + : location && (await fetch(location, { method: "HEAD" })).status; + if (path === "/other-header") expect(response.headers.get("x-added")).toBe("kept"); + } + expect(sent).toEqual({ + "/replaced": "https://example.com/elsewhere", + "/replaced-wide": "https://example.com/elsewhere", + "/deleted": null, + "/same": 200, + "/same-copy": 200, + "/same-wide": 200, + "/other-header": 200, + }); + }); + + it("the Headers object of the handler keeps the GET URL after a HEAD request", async () => { + await using origin = await startOrigin(); + let headers: Headers | undefined; + using server = Bun.serve({ + port: 0, + fetch() { + const response = origin.response(); + headers = response.headers; + return response; + }, + }); + expect(await follow(server.url, "HEAD")).toEqual({ status: 200, body: "" }); + expect(await follow(headers!.get("location")!, "GET")).toEqual({ status: 200, body }); + expect(origin.answered()).toEqual(["HEAD 200", "GET 200"]); + }); + + it("the Response that the handler sees is a 302 with a GET URL", async () => { + await using origin = await startOrigin(); + const response = origin.response(); + expect({ status: response.status, redirected: response.redirected, body: response.body }).toEqual({ + status: 302, + redirected: true, + body: null, + }); + expect(await follow(response.headers.get("location")!, "GET")).toEqual({ status: 200, body }); + expect(() => new Response(origin.client.file("object.txt"), { status: 200 })).toThrow( + "new Response(s3File) do not support ResponseInit options", + ); + expect(() => new Response(new S3Client({ bucket: "redirect" }).file("object.txt"))).toThrow( + expect.objectContaining({ code: "ERR_S3_MISSING_CREDENTIALS" }), + ); + }); + + it("a HEAD signature that cannot be made goes to error()", async () => { + // The signer writes the text it signs into a buffer of a fixed size. With the longest access + // key id for which the GET text fits, the HEAD text is one byte too long. + const client = (length: number) => + new S3Client({ + accessKeyId: Buffer.alloc(length, "A").toString(), + secretAccessKey: "secret", + bucket: "bucket", + endpoint: "http://127.0.0.1:1", + }); + let fits = 1; + let tooLong = 8192; + while (tooLong - fits > 1) { + const middle = (fits + tooLong) >> 1; + try { + new Response(client(middle).file("key")); + fits = middle; + } catch { + tooLong = middle; + } + } + expect(() => client(fits).presign("key", { method: "HEAD", expiresIn: 15 * 60 })).toThrow( + expect.objectContaining({ code: "ERR_S3_INVALID_SIGNATURE" }), + ); + + const codes: unknown[] = []; + using server = Bun.serve({ + port: 0, + fetch: () => new Response(client(fits).file("key")), + error(error: any) { + codes.push(error.code); + return new Response("no signature", { status: 500 }); + }, + }); + expect((await redirectOf(server.url, "GET")).status).toBe(302); + expect(await redirectOf(server.url, "HEAD")).toEqual({ status: 500, location: null }); + expect(codes).toEqual(["ERR_S3_INVALID_SIGNATURE"]); + }); +});