diff --git a/.changeset/r2-local-public-fixes.md b/.changeset/r2-local-public-fixes.md new file mode 100644 index 0000000000..ae49144bbc --- /dev/null +++ b/.changeset/r2-local-public-fixes.md @@ -0,0 +1,5 @@ +--- +"miniflare": patch +--- + +Fix edge cases on the local R2 public bucket endpoint (`/cdn-cgi/local/r2/public`) to match r2.dev: write methods are rejected with 401, malformed/multiple/inverted ranges with 400 and unsatisfiable ranges (including `bytes=-0`) with 416, `Range` is honored on HEAD requests with a bodyless 206, `Content-Range` is correct for suffix ranges, and object keys are percent-decoded exactly once (keys containing a literal `%` no longer fail). Unread object bodies are also cancelled (on HEAD and unsatisfiable-range responses) instead of leaking a read stream until garbage collection. diff --git a/packages/miniflare/src/workers/r2/public.worker.ts b/packages/miniflare/src/workers/r2/public.worker.ts index 39d50c6bda..c1fc2d2bf9 100644 --- a/packages/miniflare/src/workers/r2/public.worker.ts +++ b/packages/miniflare/src/workers/r2/public.worker.ts @@ -4,6 +4,33 @@ import { CorePaths } from "../core/constants"; type Env = Record; +// Accept only a single range with start <= end; reject anything else +// (including multiple ranges) with an endpoint-specific 400 rather than +// ignoring it +const RANGE_HEADER = /^bytes=(?:(\d+)-(\d+)?|-(\d+))$/; + +type ParsedRangeHeader = + | { error: "malformed" | "inverted" | "unsatisfiable" } + // `start` is undefined for suffix ranges (`bytes=-N`) + | { start?: number }; + +function parseRangeHeader(header: string): ParsedRangeHeader { + const match = RANGE_HEADER.exec(header); + if (match === null) { + return { error: "malformed" }; + } + const [, start, end, suffix] = match; + if (start === undefined) { + // A zero suffix length (`bytes=-0`) is unsatisfiable for any object; + // the simulator would ignore the range and serve the full body + return Number(suffix) === 0 ? { error: "unsatisfiable" } : {}; + } + if (end !== undefined && Number(start) > Number(end)) { + return { error: "inverted" }; + } + return { start: Number(start) }; +} + function objectHeaders(object: R2Object): Headers { const headers = new Headers(); object.writeHttpMetadata(headers); @@ -20,21 +47,31 @@ app.use( ); app.on(["GET", "HEAD"], "/:bucketId/:key{.+}", async (c) => { - const bucketId = decodeURIComponent(c.req.param("bucketId")); - const key = decodeURIComponent(c.req.param("key")); + const bucketId = c.req.param("bucketId"); + const key = c.req.param("key"); const bucket = c.env[bucketId]; if (bucket === undefined) { return c.notFound(); } - const hasRange = c.req.header("Range") !== undefined; + // Reject malformed, multiple, and inverted ranges with 400 rather than + // ignoring them, and zero-length suffix ranges (`bytes=-0`) with 416 + const rangeHeader = c.req.header("Range"); + const parsedRange = + rangeHeader === undefined ? undefined : parseRangeHeader(rangeHeader); + if (parsedRange !== undefined && "error" in parsedRange) { + return c.body(null, parsedRange.error === "unsatisfiable" ? 416 : 400); + } + + // R2 honors Range on HEAD too (206 + Content-Range, no body) + const hasRange = rangeHeader !== undefined; // `bucket.head()` cannot evaluate conditional headers (the R2 head // operation only carries the key), so HEAD also uses `bucket.get()` and // discards the body. const object = await bucket.get(key, { onlyIf: c.req.raw.headers, - range: hasRange && c.req.method === "GET" ? c.req.raw.headers : undefined, + range: hasRange ? c.req.raw.headers : undefined, }); if (object === null) { @@ -70,41 +107,63 @@ app.on(["GET", "HEAD"], "/:bucketId/:key{.+}", async (c) => { return c.notFound(); } if (!("body" in recheck)) { - return c.body(null, { status: 412, headers: objectHeaders(recheck) }); + return c.body(null, 412); } + // An unread recheck body would hold a read stream open + void recheck.body.cancel(); } // Otherwise, the preconditions hold, so the failure came from a cache validator. return c.body(null, { status: 304, headers }); } - if (c.req.method === "HEAD") { - headers.set("Content-Length", `${object.size}`); - return c.body(null, { headers }); + const body = c.req.method === "HEAD" ? null : object.body; + if (body === null) { + // An unread body would hold a read stream open + void object.body.cancel(); } const range = object.range; - if ( - hasRange && - range !== undefined && - "offset" in range && - "length" in range - ) { - const { offset = 0, length = object.size - offset } = range; + if (hasRange && range !== undefined) { + // The simulator clamps out-of-bounds ranges and serves a zero-length + // range for empty objects; r2.dev rejects both with 416 (any range on a + // 0-byte object, or a range starting at or beyond the object size) + if ( + object.size === 0 || + (parsedRange?.start !== undefined && parsedRange.start >= object.size) + ) { + if (body !== null) { + void body.cancel(); + } + return c.body(null, 416); + } + // The returned range may carry all keys with some `undefined` (e.g. + // `suffix` present but undefined on an offset range), so normalize by + // value rather than by key presence + const normalized: { offset?: number; length?: number; suffix?: number } = { + ...range, + }; + let offset: number; + let length: number; + if (normalized.suffix !== undefined) { + length = Math.min(normalized.suffix, object.size); + offset = object.size - length; + } else { + offset = normalized.offset ?? 0; + length = normalized.length ?? object.size - offset; + } headers.set( "Content-Range", `bytes ${offset}-${offset + length - 1}/${object.size}` ); headers.set("Content-Length", `${length}`); - return c.body(object.body, { status: 206, headers }); + return new Response(body, { status: 206, headers }); } headers.set("Content-Length", `${object.size}`); - return c.body(object.body, { headers }); + return new Response(body, { headers }); }); -app.all("/:bucketId/:key{.+}", (c) => - c.text("Method Not Allowed", 405, { Allow: "GET, HEAD, OPTIONS" }) -); +app.all("/:bucketId/:key{.+}", (c) => c.body(null, 401)); export default app; diff --git a/packages/miniflare/test/plugins/r2/public.spec.ts b/packages/miniflare/test/plugins/r2/public.spec.ts index 9424731c30..2933778727 100644 --- a/packages/miniflare/test/plugins/r2/public.spec.ts +++ b/packages/miniflare/test/plugins/r2/public.spec.ts @@ -102,6 +102,12 @@ test("decodes percent-encoded keys", async ({ expect }) => { expect(res.status).toBe(200); expect(await res.text()).toBe("nested"); + + // Keys containing `%` must be decoded exactly once + await r2.put("100%/a%2Bb.txt", "percent"); + const percent = await fetch(bucketUrl("/100%25/a%252Bb.txt", ctx.url)); + expect(percent.status).toBe(200); + expect(await percent.text()).toBe("percent"); }); test("GET supports range requests", async ({ expect }) => { @@ -118,6 +124,35 @@ test("GET supports range requests", async ({ expect }) => { expect(res.headers.get("Content-Length")).toBe("4"); }); +test("GET honors suffix ranges with a partial 206", async ({ expect }) => { + const r2 = await ctx.mf.getR2Bucket("BUCKET"); + await r2.put("suffix-range-key", "0123456789"); + + const res = await fetch(bucketUrl("/suffix-range-key", ctx.url), { + headers: { Range: "bytes=-4" }, + }); + + expect(res.status).toBe(206); + expect(await res.text()).toBe("6789"); + expect(res.headers.get("Content-Range")).toBe("bytes 6-9/10"); + expect(res.headers.get("Content-Length")).toBe("4"); +}); + +test("HEAD honors Range with a bodyless 206", async ({ expect }) => { + const r2 = await ctx.mf.getR2Bucket("BUCKET"); + await r2.put("head-range-key", "0123456789"); + + const res = await fetch(bucketUrl("/head-range-key", ctx.url), { + method: "HEAD", + headers: { Range: "bytes=0-3" }, + }); + + expect(res.status).toBe(206); + expect(res.headers.get("Content-Range")).toBe("bytes 0-3/10"); + expect(res.headers.get("Content-Length")).toBe("4"); + expect(await res.text()).toBe(""); +}); + test("HEAD returns headers without a body", async ({ expect }) => { const r2 = await ctx.mf.getR2Bucket("BUCKET"); await r2.put("head-key", "abcdef", { @@ -144,9 +179,7 @@ test("HEAD returns 404 for a missing key", async ({ expect }) => { await res.arrayBuffer(); }); -test("rejects write methods with 405 and an Allow header", async ({ - expect, -}) => { +test("rejects write methods with 401", async ({ expect }) => { const r2 = await ctx.mf.getR2Bucket("BUCKET"); await r2.put("readonly-key", "untouched"); @@ -155,8 +188,7 @@ test("rejects write methods with 405 and an Allow header", async ({ method, body: method === "DELETE" ? undefined : "tampered", }); - expect(res.status, `${method} should be rejected`).toBe(405); - expect(res.headers.get("Allow")).toBe("GET, HEAD, OPTIONS"); + expect(res.status, `${method} should be rejected`).toBe(401); await res.arrayBuffer(); } @@ -164,6 +196,46 @@ test("rejects write methods with 405 and an Allow header", async ({ expect(await after?.text()).toBe("untouched"); }); +test("rejects malformed and multiple ranges with 400", async ({ expect }) => { + const r2 = await ctx.mf.getR2Bucket("BUCKET"); + await r2.put("bad-range-key", "0123456789"); + + for (const range of ["bytes=zzz", "bytes=0-1,3-4", "0-3", "bytes=5-2"]) { + const res = await fetch(bucketUrl("/bad-range-key", ctx.url), { + headers: { Range: range }, + }); + expect(res.status, `"${range}" should be rejected`).toBe(400); + await res.arrayBuffer(); + } +}); + +test("rejects unsatisfiable ranges with 416", async ({ expect }) => { + const r2 = await ctx.mf.getR2Bucket("BUCKET"); + await r2.put("unsat-range-key", "0123456789"); + + const res = await fetch(bucketUrl("/unsat-range-key", ctx.url), { + headers: { Range: "bytes=99999-" }, + }); + expect(res.status).toBe(416); + await res.arrayBuffer(); + + // A zero suffix length is unsatisfiable for any object + const zeroSuffix = await fetch(bucketUrl("/unsat-range-key", ctx.url), { + headers: { Range: "bytes=-0" }, + }); + expect(zeroSuffix.status).toBe(416); + await zeroSuffix.arrayBuffer(); + + // Any range on a zero-length object is unsatisfiable, including a suffix + // range (which the simulator would otherwise serve as a zero-length 206) + await r2.put("empty-range-key", ""); + const emptySuffix = await fetch(bucketUrl("/empty-range-key", ctx.url), { + headers: { Range: "bytes=-5" }, + }); + expect(emptySuffix.status).toBe(416); + await emptySuffix.arrayBuffer(); +}); + // The entry worker rejects /cdn-cgi/* requests from non-localhost origins // before they reach this worker, so cross-origin here means a different // localhost port (e.g. a frontend dev server).