diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index fc5508d0c533..4d7be9b5b3a0 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -250,8 +250,8 @@ pub trait BlobExt { fn get_slice_from( &self, global_this: &JSGlobalObject, - relative_start: i64, - relative_end: i64, + offset: SizeType, + size: SizeType, content_type: BlobContentType, ) -> JSValue; fn get_slice(&self, global_this: &JSGlobalObject, callframe: &CallFrame) -> JsResult; @@ -2000,21 +2000,15 @@ impl BlobExt for Blob { fn get_slice_from( &self, global_this: &JSGlobalObject, - relative_start: i64, - relative_end: i64, + offset: SizeType, + size: SizeType, content_type: BlobContentType, ) -> JSValue { - let offset = self - .offset - .get() - .saturating_add(SizeType::try_from(relative_start).expect("int cast")); - let len = SizeType::try_from((relative_end.saturating_sub(relative_start)).max(0)).unwrap(); - // This copies over the charset field // which is okay because this will only be a <= slice let blob = self.dupe(); blob.offset.set(offset); - blob.size.set(len); + blob.size.set(size); let content_type_was_allocated = content_type.is_owned() && !content_type.is_empty(); // infer the content type if it was not specified @@ -2049,10 +2043,7 @@ impl BlobExt for Blob { return Ok(unsafe { BlobExt::to_js(&*ptr, global_this) }); } - // If the optional start parameter is not used as a parameter, let relativeStart be 0. - let mut relative_start: i64 = 0; - // If the optional end parameter is not used, let relativeEnd be size. - let mut relative_end: i64 = i64::try_from(self.size.get()).expect("int cast"); + let this_size = i64::try_from(self.size.get()).expect("int cast"); // Mutate the fixed-3 args array in place to shift the string arg into [2]. if args[0].is_string() { @@ -2065,32 +2056,84 @@ impl BlobExt for Blob { } let mut args_iter = jsc::ArgumentsSlice::init(global_this.bun_vm(), &arguments_.ptr[..3]); + let mut start_raw: Option = None; if let Some(start_) = args_iter.next_eat() { if start_.is_number() { - let start = start_.to_int64(); - if start < 0 { - relative_start = (start - .wrapping_add(i64::try_from(self.size.get()).expect("int cast"))) - .max(0); - } else { - relative_start = start.min(i64::try_from(self.size.get()).expect("int cast")); - } + start_raw = Some(start_.to_int64()); } } - + let mut end_raw: Option = None; if let Some(end_) = args_iter.next_eat() { if end_.is_number() { - let end = end_.to_int64(); - if end < 0 { - relative_end = (end - .wrapping_add(i64::try_from(self.size.get()).expect("int cast"))) - .max(0); - } else { - relative_end = end.min(i64::try_from(self.size.get()).expect("int cast")); - } + end_raw = Some(end_.to_int64()); } } + // If the optional start parameter is not used as a parameter, let relativeStart be 0. + let relative_start = match start_raw { + Some(s) if s < 0 => s.wrapping_add(this_size).max(0), + Some(s) => s.min(this_size), + None => 0, + }; + // If the optional end parameter is not used, let relativeEnd be size. + let relative_end = match end_raw { + Some(e) if e < 0 => e.wrapping_add(this_size).max(0), + Some(e) => e.min(this_size), + None => this_size, + }; + + let span = SizeType::try_from(relative_end.saturating_sub(relative_start).max(0)).unwrap(); + // Cap below `MAX_SIZE` so ordinary arithmetic can never land exactly on + // the suffix sentinel (e.g. `slice(F).slice(-F)` would otherwise sum to + // `F + (MAX_SIZE - F)` and be misread as a whole-object suffix). + let default_offset = self + .offset + .get() + .saturating_add(SizeType::try_from(relative_start).expect("int cast")) + .min(MAX_SIZE - 1); + + // For an S3 blob whose real size is still unknown, don't serialize the + // `MAX_SIZE` placeholder into a `Range` header. HTTP supports the two + // cases that need no size: `slice(-n)` → `bytes=-n` (encoded as + // `offset == MAX_SIZE, size == n`), and `slice(a)` → `bytes=a-` + // (encoded as `size == MAX_SIZE`). + let (offset, size) = if self.size.get() == MAX_SIZE && end_raw.is_none() && self.is_s3() { + match start_raw { + // A suffix range is only unambiguous on a top-level file; on a + // chained open-ended parent (`offset > 0`) or an oversized `n` + // (e.g. `-Infinity`) fall through to the generic arithmetic, + // which matches what happened before this encoding existed. + Some(s) if s < 0 && self.offset.get() == 0 && s.unsigned_abs() < MAX_SIZE => { + (MAX_SIZE, s.unsigned_abs()) + } + // Cap below `MAX_SIZE` so a huge positive start cannot collide + // with the suffix sentinel and turn into a whole-object fetch. + Some(s) if s >= 0 => ( + self.offset + .get() + .saturating_add(SizeType::try_from(s).expect("int cast")) + .min(MAX_SIZE - 1), + MAX_SIZE, + ), + None => (self.offset.get(), MAX_SIZE), + _ => (default_offset, span), + } + } else if self.offset.get() == MAX_SIZE && self.is_s3() { + // Re-slicing a suffix slice of length `this_size`. A non-empty + // re-slice that still ends at the parent's end is itself a suffix; + // one that is empty or stops short cannot be expressed as an + // RFC 7233 range without the total length, so push the offset past + // the sentinel and let the server reject it with 416 rather than + // return the wrong bytes. + if relative_end == this_size && span > 0 { + (MAX_SIZE, span) + } else { + (MAX_SIZE + 1, span) + } + } else { + (default_offset, span) + }; + let mut content_type = BlobContentType::default(); if let Some(content_type_) = args_iter.next_eat() { 'inner: { @@ -2109,7 +2152,7 @@ impl BlobExt for Blob { } } - Ok(self.get_slice_from(global_this, relative_start, relative_end, content_type)) + Ok(self.get_slice_from(global_this, offset, size, content_type)) } fn get_mime_type(&self) -> Option { @@ -4863,7 +4906,12 @@ pub fn write_file_with_source_destination( } else if destination_type == store::DataTag::Bytes && (source_type == store::DataTag::File || source_type == store::DataTag::S3) { - let blob_value = source_blob.get_slice_from(ctx, 0, 0, BlobContentType::default()); + let blob_value = source_blob.get_slice_from( + ctx, + source_blob.offset.get(), + 0, + BlobContentType::default(), + ); return Ok(JSPromise::resolved_promise_value(ctx, blob_value)); } else if destination_type == store::DataTag::S3 { let s3 = destination_store.data.as_s3(); diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index 26bfaa497f63..229f6b6c2e3b 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -107,6 +107,35 @@ pub(crate) fn download( ) } +/// Build an RFC 7233 `Range` header value from a Blob's (offset, size) pair. +/// `offset == blob::MAX_SIZE` encodes a suffix slice (`Blob.slice(-n)`), and +/// `size == None` encodes an open-ended slice (`Blob.slice(a)`). +fn range_header(offset: usize, size: Option) -> Option> { + let mut v = Vec::new(); + if offset as u64 == crate::webcore::blob::MAX_SIZE { + match size { + None | Some(0) => return None, + Some(n) => { + write!(&mut v, "bytes=-{}", n).expect("infallible: in-memory write"); + return Some(v); + } + } + } + if let Some(size_) = size { + let mut end = offset + size_; + if size_ > 0 { + end -= 1; + } + write!(&mut v, "bytes={}-{}", offset, end).expect("infallible: in-memory write"); + return Some(v); + } + if offset == 0 { + return None; + } + write!(&mut v, "bytes={}-", offset).expect("infallible: in-memory write"); + Some(v) +} + pub(crate) fn download_slice( this: &S3Credentials, path: &[u8], @@ -117,23 +146,7 @@ pub(crate) fn download_slice( proxy_url: Option<&[u8]>, request_payer: bool, ) -> JsTerminatedResult<()> { - let range: Option> = 'brk: { - if let Some(size_) = size { - let mut end = offset + size_; - if size_ > 0 { - end -= 1; - } - let mut v = Vec::new(); - write!(&mut v, "bytes={}-{}", offset, end).expect("infallible: in-memory write"); - break 'brk Some(v); - } - if offset == 0 { - break 'brk None; - } - let mut v = Vec::new(); - write!(&mut v, "bytes={}-", offset).expect("infallible: in-memory write"); - Some(v) - }; + let range = range_header(offset, size); s3_simple_request::execute_simple_s3_request( this, @@ -933,23 +946,7 @@ pub(crate) fn download_stream( ), callback_context: *mut c_void, ) -> *mut S3HttpDownloadStreamingTask { - let range: Option> = 'brk: { - if let Some(size_) = size { - let mut end = offset + size_; - if size_ > 0 { - end -= 1; - } - let mut v = Vec::new(); - write!(&mut v, "bytes={}-{}", offset, end).expect("infallible: in-memory write"); - break 'brk Some(v); - } - if offset == 0 { - break 'brk None; - } - let mut v = Vec::new(); - write!(&mut v, "bytes={}-", offset).expect("infallible: in-memory write"); - Some(v) - }; + let range = range_header(offset, size); let result = match this.sign_request::( &bun_s3_signing::SignOptions { diff --git a/test/js/bun/s3/s3-slice-range.test.ts b/test/js/bun/s3/s3-slice-range.test.ts new file mode 100644 index 000000000000..c3c12f898010 --- /dev/null +++ b/test/js/bun/s3/s3-slice-range.test.ts @@ -0,0 +1,198 @@ +import { describe, expect, it } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// A 1000-byte object on a mock endpoint that implements Range per RFC 7233: +// - `bytes=-n` returns the last n bytes (suffix range) +// - `bytes=a-` returns from a to the end +// - `bytes=a-b` returns [a, b] inclusive, clamping b to the last byte +// - a first-byte-pos past the last byte is 416 Range Not Satisfiable +// AWS S3, MinIO and Cloudflare R2 all behave this way. +// +// Spawned as a subprocess because the S3 client picks up HTTP_PROXY without +// consulting NO_PROXY, so an inherited proxy would hijack the request. +const fixture = /* ts */ ` +const OBJ = new Uint8Array(1000).map((_, i) => i & 0x7f); +using server = Bun.serve({ + port: 0, + fetch(req) { + const range = req.headers.get("range"); + process.stdout.write(JSON.stringify({ range }) + "\\n"); + if (!range) return new Response(OBJ, { headers: { ETag: '"x"' } }); + const m = /^bytes=(\\d*)-(\\d*)$/.exec(range); + let a = 0, b = OBJ.length - 1, status = 206; + if (!m) { + status = 416; + } else if (m[1] === "") { + a = Math.max(0, OBJ.length - Number(m[2])); + } else { + a = Number(m[1]); + b = m[2] === "" ? b : Math.min(Number(m[2]), b); + } + if (status !== 416 && a >= OBJ.length) status = 416; + if (status === 416) + return new Response( + 'InvalidRangeThe requested range is not satisfiable', + { status: 416, headers: { "Content-Range": "bytes */" + OBJ.length } }, + ); + return new Response(OBJ.subarray(a, b + 1), { + status: 206, + headers: { ETag: '"x"', "Content-Range": "bytes " + a + "-" + b + "/" + OBJ.length }, + }); + }, +}); +const c = new Bun.S3Client({ + endpoint: server.url.href, + bucket: "b", + accessKeyId: "AK", + secretAccessKey: "SK", + region: "us-east-1", +}); +const [mode, ...args] = process.argv.slice(1); +let slice = c.file("k"); +for (const group of args.join(" ").split("/")) + slice = slice.slice(...group.split(" ").filter(Boolean).map(Number)); +let got; +if (mode === "bytes") got = await slice.bytes(); +else if (mode === "text") got = new TextEncoder().encode(await slice.text()); +else if (mode === "arrayBuffer") got = new Uint8Array(await slice.arrayBuffer()); +else if (mode === "stream") got = new Uint8Array(await Bun.readableStreamToArrayBuffer(slice.stream())); +else throw new Error("bad mode"); +process.stdout.write(JSON.stringify({ len: got.length, first: got[0] ?? null }) + "\\n"); +`; + +type Result = { range: string | null; len: number; first: number | null }; + +async function run(mode: "bytes" | "text" | "arrayBuffer" | "stream", ...args: (number | "/")[]): Promise { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture, mode, ...args.map(String)], + env: { + ...bunEnv, + HTTP_PROXY: undefined, + HTTPS_PROXY: undefined, + http_proxy: undefined, + https_proxy: undefined, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + if (exitCode !== 0) { + throw new Error(`exit ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`); + } + const lines = stdout + .trim() + .split("\n") + .map(l => JSON.parse(l)); + return { range: lines[0].range, len: lines[1].len, first: lines[1].first }; +} + +describe("S3File.slice() Range header", () => { + describe.each(["bytes", "text", "arrayBuffer", "stream"] as const)("via .%s()", mode => { + it.concurrent.each([1, 5, 500])("slice(-%d) sends a suffix range", async n => { + expect(await run(mode, -n)).toEqual({ range: `bytes=-${n}`, len: n, first: (1000 - n) & 0x7f }); + }); + + it.concurrent("slice(200) with no end sends an open-ended range", async () => { + expect(await run(mode, 200)).toEqual({ range: "bytes=200-", len: 800, first: 200 & 0x7f }); + }); + + it.concurrent("slice(6, 10) still sends an absolute range", async () => { + expect(await run(mode, 6, 10)).toEqual({ range: "bytes=6-9", len: 4, first: 6 }); + }); + + it.concurrent("slice(0, 5) still sends an absolute range", async () => { + expect(await run(mode, 0, 5)).toEqual({ range: "bytes=0-4", len: 5, first: 0 }); + }); + + it.concurrent("slice(0) with no end fetches the whole object (no Range header)", async () => { + expect(await run(mode, 0)).toEqual({ range: null, len: 1000, first: 0 }); + }); + + it.concurrent("slice() with no args fetches the whole object (no Range header)", async () => { + expect(await run(mode)).toEqual({ range: null, len: 1000, first: 0 }); + }); + }); + + it.concurrent("slice(-10).slice(3) re-slices a suffix as a shorter suffix", async () => { + expect(await run("bytes", -10, "/", 3)).toEqual({ range: "bytes=-7", len: 7, first: 993 & 0x7f }); + }); + + it.concurrent("slice(-10).slice(-5) re-slices a suffix as a shorter suffix", async () => { + expect(await run("bytes", -10, "/", -5)).toEqual({ range: "bytes=-5", len: 5, first: 995 & 0x7f }); + }); + + it.concurrent("slice(-10).slice(3, 10) reaching the end stays a suffix", async () => { + expect(await run("bytes", -10, "/", 3, 10)).toEqual({ range: "bytes=-7", len: 7, first: 993 & 0x7f }); + }); + + it.concurrent("slice(-10).slice(0, 3) does not silently return a wrong suffix", async () => { + // [len-10, len-7) cannot be expressed as an RFC 7233 range without the + // total length, so this must fail loudly rather than return `bytes=-3`. + const err = await run("bytes", -10, "/", 0, 3).then( + () => null, + e => String(e), + ); + expect(err).toContain("InvalidRange"); + }); + + it.concurrent("slice(-10).slice(3, 7) does not silently return a wrong suffix", async () => { + const err = await run("bytes", -10, "/", 3, 7).then( + () => null, + e => String(e), + ); + expect(err).toContain("InvalidRange"); + }); + + it.concurrent.each([[10], [20], [10, 10]])( + "slice(-10).slice(%p) is an empty re-slice and must not download the whole object", + async (...args) => { + const got = await run("bytes", -10, "/", ...(args as number[])).then( + r => ({ ok: true, ...r }), + e => ({ ok: false, err: String(e) }), + ); + if (got.ok) { + expect(got).toEqual({ ok: true, range: null, len: 0, first: null }); + } else { + expect(got.err).toContain("InvalidRange"); + } + }, + ); + + it.concurrent("slice(200).slice(50) chains open-ended offsets", async () => { + expect(await run("bytes", 200, "/", 50)).toEqual({ range: "bytes=250-", len: 750, first: 250 & 0x7f }); + }); + + // A negative re-slice of an open-ended parent cannot be expressed as an + // RFC 7233 range without the total length; it must 416 rather than emit a + // whole-object suffix. `F/-F` is the exact-collision case where the generic + // arithmetic lands on the sentinel. + it.concurrent.each([ + [700, -500], + [700, -700], + ])("slice(%d).slice(%d) does not return bytes before the parent's start", async (a, b) => { + const err = await run("bytes", a, "/", b).then( + r => `ok len=${r.len}`, + e => String(e), + ); + expect(err).toContain("InvalidRange"); + }); + + it.concurrent("slice(-Infinity) fetches the whole object and can be re-sliced without panicking", async () => { + expect(await run("bytes", -Infinity)).toEqual({ range: null, len: 1000, first: 0 }); + expect(await run("bytes", -Infinity, "/", 0, 5)).toEqual({ range: "bytes=0-4", len: 5, first: 0 }); + }); + + for (const huge of [2 ** 52 - 1, Number.MAX_SAFE_INTEGER]) { + it.concurrent(`slice(${huge}) does not collide with the suffix sentinel`, async () => { + // A start at or past 2^52-1 must not be encoded as the suffix sentinel + // (which would drop the Range header and download the whole object). + const { range, len } = await run("bytes", huge).catch(e => { + const m = /"range":("[^"]*"|null)/.exec(String(e)); + return { range: m ? JSON.parse(m[1]) : undefined, len: undefined }; + }); + expect(range).not.toBeNull(); + expect(range).not.toMatch(/^bytes=-/); + expect(len).not.toBe(1000); + }); + } +});