[miniflare] R2 local public bucket fixes

.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.

packages/miniflare/src/workers/r2/public.worker.ts

@@ -4,6 +4,33 @@ import { CorePaths } from "../core/constants";
 
 type Env = Record<string, R2Bucket>;
 
+// 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;

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,15 +188,54 @@ 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();
 	}
 
 	const after = await r2.get("readonly-key");
 	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).