Normalize workers assets path (#7318)

* Normalize double slashes into single in Workers Assets

* Remove multiple slashes from asset pathing

---------

Co-authored-by: Daniel Walsh <[email protected]>

.changeset/cyan-laws-mate.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/workers-shared": minor
+---
+
+Prevent same-schema attacks

fixtures/asset-config/redirects.test.ts

@@ -0,0 +1,89 @@
+import { SELF } from "cloudflare:test";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { applyConfigurationDefaults } from "../../packages/workers-shared/asset-worker/src/configuration";
+import Worker from "../../packages/workers-shared/asset-worker/src/index";
+import { getAssetWithMetadataFromKV } from "../../packages/workers-shared/asset-worker/src/utils/kv";
+import type { AssetMetadata } from "../../packages/workers-shared/asset-worker/src/utils/kv";
+
+const IncomingRequest = Request<unknown, IncomingRequestCfProperties>;
+
+vi.mock("../../packages/workers-shared/asset-worker/src/utils/kv.ts");
+vi.mock("../../packages/workers-shared/asset-worker/src/configuration");
+const existsMock = (fileList: Set<string>) => {
+	vi.spyOn(Worker.prototype, "unstable_exists").mockImplementation(
+		async (pathname: string) => {
+			if (fileList.has(pathname)) {
+				return pathname;
+			}
+		}
+	);
+};
+const BASE_URL = "http://example.com";
+
+describe("[Asset Worker] `test url normalization`", () => {
+	afterEach(() => {
+		vi.mocked(getAssetWithMetadataFromKV).mockRestore();
+	});
+	beforeEach(() => {
+		vi.mocked(getAssetWithMetadataFromKV).mockImplementation(
+			() =>
+				Promise.resolve({
+					value: "no-op",
+					metadata: {
+						contentType: "no-op",
+					},
+				}) as unknown as Promise<
+					KVNamespaceGetWithMetadataResult<ReadableStream, AssetMetadata>
+				>
+		);
+
+		vi.mocked(applyConfigurationDefaults).mockImplementation(() => {
+			return {
+				html_handling: "none",
+				not_found_handling: "none",
+			};
+		});
+	});
+
+	it("returns 404 for non matched encoded url", async () => {
+		const files = ["/christmas/starts/november/first.html"];
+		const requestPath = "/%2f%2fbad.example.com%2f";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request, { redirect: "manual" });
+		expect(response.status).toBe(404);
+	});
+
+	it("returns 200 for matched non encoded url", async () => {
+		const files = ["/you/lost/the/game.bin"];
+		const requestPath = "/you/lost/the/game.bin";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request, { redirect: "manual" });
+		expect(response.status).toBe(200);
+	});
+
+	it("returns redirect for matched encoded url", async () => {
+		const files = ["/awesome/file.bin"];
+		const requestPath = "/awesome/file%2ebin";
+		const finalPath = "/awesome/file.bin";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request, { redirect: "manual" });
+		expect(response.status).toBe(307);
+		expect(response.headers.get("location")).toBe(finalPath);
+	});
+
+	it("returns 200 for matched non encoded url", async () => {
+		const files = ["/mylittlepony.png"];
+		const requestPath = "/mylittlepony.png";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request, { redirect: "manual" });
+		expect(response.status).toBe(200);
+	});
+});

fixtures/asset-config/url-normalization.test.ts

@@ -0,0 +1,77 @@
+import { SELF } from "cloudflare:test";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { applyConfigurationDefaults } from "../../packages/workers-shared/asset-worker/src/configuration";
+import Worker from "../../packages/workers-shared/asset-worker/src/index";
+import { getAssetWithMetadataFromKV } from "../../packages/workers-shared/asset-worker/src/utils/kv";
+import type { AssetMetadata } from "../../packages/workers-shared/asset-worker/src/utils/kv";
+
+const IncomingRequest = Request<unknown, IncomingRequestCfProperties>;
+
+vi.mock("../../packages/workers-shared/asset-worker/src/utils/kv.ts");
+vi.mock("../../packages/workers-shared/asset-worker/src/configuration");
+const existsMock = (fileList: Set<string>) => {
+	vi.spyOn(Worker.prototype, "unstable_exists").mockImplementation(
+		async (pathname: string) => {
+			if (fileList.has(pathname)) {
+				return pathname;
+			}
+		}
+	);
+};
+const BASE_URL = "http://example.com";
+
+describe("[Asset Worker] `test redirects`", () => {
+	afterEach(() => {
+		vi.mocked(getAssetWithMetadataFromKV).mockRestore();
+	});
+	beforeEach(() => {
+		vi.mocked(getAssetWithMetadataFromKV).mockImplementation(
+			() =>
+				Promise.resolve({
+					value: "no-op",
+					metadata: {
+						contentType: "no-op",
+					},
+				}) as unknown as Promise<
+					KVNamespaceGetWithMetadataResult<ReadableStream, AssetMetadata>
+				>
+		);
+
+		vi.mocked(applyConfigurationDefaults).mockImplementation(() => {
+			return {
+				html_handling: "none",
+				not_found_handling: "none",
+			};
+		});
+	});
+
+	it("returns 200 leading encoded double slash", async () => {
+		const files = ["/blog/index.html"];
+		const requestPath = "/%2fblog/index.html";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request);
+		expect(response.status).toBe(200);
+	});
+
+	it("returns 200 leading non encoded double slash", async () => {
+		const files = ["/blog/index.html"];
+		const requestPath = "//blog/index.html";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request);
+		expect(response.status).toBe(200);
+	});
+
+	it("returns 404 for non matched url", async () => {
+		const files = ["/blog/index.html"];
+		const requestPath = "/%2fexample.com/";
+
+		existsMock(new Set(files));
+		const request = new IncomingRequest(BASE_URL + requestPath);
+		let response = await SELF.fetch(request);
+		expect(response.status).toBe(404);
+	});
+});

packages/workers-shared/asset-worker/src/handler.ts

@@ -18,7 +18,10 @@ export const handleRequest = async (
 ) => {
 	const { pathname, search } = new URL(request.url);
 
-	const decodedPathname = decodePath(pathname);
+	let decodedPathname = decodePath(pathname);
+	// normalize the path; remove multiple slashes which could lead to same-schema redirects
+	decodedPathname = decodedPathname.replace(/\/+/g, "/");
+
 	const intent = await getIntent(decodedPathname, configuration, exists);
 
 	if (!intent) {
@@ -41,7 +44,7 @@ export const handleRequest = async (
 	 * Thus we need to redirect if that is different from the decoded version.
 	 * We combine this with other redirects (e.g. for html_handling) to avoid multiple redirects.
 	 */
-	if (encodedDestination !== pathname || intent.redirect) {
+	if ((encodedDestination !== pathname && intent.asset) || intent.redirect) {
 		return new TemporaryRedirectResponse(encodedDestination + search);
 	}