fix: ensure that Origin header is rewritten as necessary (#4812)

* fix: ensure that Origin header is rewritten as necessary

The `wrangler dev` command puts the Worker under test behind a proxy server.
This proxy server should be transparent to the client and the Worker, which
means that the `Request` arriving at the Worker with the correct `url` property,
and `Host` and `Origin` headers.
Previously we fixed the `Host` header but missed the `Origin` header which is
only added to a request under certain circumstances, such as cross-origin requests.

This change fixes the `Origin` header as well, so that it is rewritten, when it exists,
to use the `origin` of the `url` property.

Fixes #4761

* test: ensure that temp dirs used in tests do not collide

.changeset/selfish-starfishes-cry.md

@@ -0,0 +1,17 @@
+---
+"miniflare": patch
+---
+
+fix: ensure that Origin header is rewritten as necessary
+
+The `wrangler dev` command puts the Worker under test behind a proxy server.
+This proxy server should be transparent to the client and the Worker, which
+means that the `Request` arriving at the Worker with the correct `url` property,
+and `Host` and `Origin` headers.
+Previously we fixed the `Host` header but missed the `Origin` header which is
+only added to a request under certain circumstances, such as cross-origin requests.
+
+This change fixes the `Origin` header as well, so that it is rewritten, when it exists,
+to use the `origin` of the `url` property.
+
+Fixes #4761

fixtures/pages-d1-shim/tests/index.test.ts

@@ -1,5 +1,5 @@
 import { execSync } from "child_process";
-import { readFileSync } from "fs";
+import { mkdtempSync, readFileSync, realpathSync } from "fs";
 import { tmpdir } from "os";
 import * as path from "path";
 import { join } from "path";
@@ -9,8 +9,10 @@ describe("Pages D1 shim", () => {
 	it("builds functions with D1 binding, without the shim", async ({
 		expect,
 	}) => {
-		const dir = tmpdir();
-		const file = join(dir, "./d1-pages.js");
+		const tempDir = realpathSync(
+			mkdtempSync(join(tmpdir(), "pages-d1-shim-tests"))
+		);
+		const file = join(tempDir, "./d1-pages.js");
 
 		execSync(
 			`npx wrangler pages functions build --outfile ${file} --bindings="{\\"d1_databases\\":{\\"FOO\\":{}}}"`,

fixtures/pages-workerjs-directory/tests/index.test.ts

@@ -1,5 +1,5 @@
 import { execSync } from "node:child_process";
-import { existsSync, readFileSync } from "node:fs";
+import { existsSync, mkdtempSync, readFileSync, realpathSync } from "node:fs";
 import { tmpdir } from "node:os";
 import path, { join, resolve } from "node:path";
 import { fetch } from "undici";
@@ -8,7 +8,9 @@ import { runWranglerPagesDev } from "../../shared/src/run-wrangler-long-lived";
 
 describe("Pages _worker.js/ directory", () => {
 	it("should support non-bundling with 'dev'", async ({ expect }) => {
-		const tmpDir = join(tmpdir(), Math.random().toString(36).slice(2));
+		const tmpDir = realpathSync(
+			mkdtempSync(join(tmpdir(), "worker-directory-tests"))
+		);
 
 		const { ip, port, stop } = await runWranglerPagesDev(
 			resolve(__dirname, ".."),
@@ -67,8 +69,10 @@ describe("Pages _worker.js/ directory", () => {
 	});
 
 	it("should bundle", async ({ expect }) => {
-		const dir = tmpdir();
-		const file = join(dir, "_worker.bundle");
+		const tempDir = realpathSync(
+			mkdtempSync(join(tmpdir(), "worker-bundle-tests"))
+		);
+		const file = join(tempDir, "_worker.bundle");
 
 		execSync(
 			`npx wrangler pages functions build --build-output-directory public --outfile ${file} --bindings="{\\"d1_databases\\":{\\"D1\\":{}}}"`,

fixtures/worker-app/src/index.js

@@ -36,7 +36,9 @@ export default {
 
 		console.log("end of request");
 		return new Response(
-			`${request.url} ${now()} ${request.headers.get("Host")}`
+			`${request.url} ${now()} HOST:${request.headers.get(
+				"Host"
+			)} ORIGIN:${request.headers.get("Origin")}`
 		);
 	},
 

fixtures/worker-app/tests/index.test.ts

@@ -13,7 +13,12 @@ describe("'wrangler dev' correctly renders pages", () => {
 	beforeAll(async () => {
 		({ ip, port, stop, getOutput } = await runWranglerDev(
 			resolve(__dirname, ".."),
-			["--port=0", "--inspector-port=0", "--upstream-protocol=https"]
+			[
+				"--port=0",
+				"--inspector-port=0",
+				"--upstream-protocol=https",
+				"--host=prod.example.org",
+			]
 		));
 	});
 
@@ -26,8 +31,7 @@ describe("'wrangler dev' correctly renders pages", () => {
 		const response = await fetch(`http://${ip}:${port}/`);
 		const text = await response.text();
 		expect(response.status).toBe(200);
-		// Note that the upstream protocol defaults to https
-		expect(text).toContain(`https://${ip}:${port}/`);
+		expect(text).toContain(`https://prod.example.org/`);
 
 		// Wait up to 5s for all request logs to be flushed
 		for (let i = 0; i < 10; i++) {
@@ -41,7 +45,7 @@ describe("'wrangler dev' correctly renders pages", () => {
 		expect(output).toContain("request log");
 
 		// check host on request in the Worker is as expected
-		expect(output).toContain(`host' => '${ip}:${port}'`);
+		expect(output).toContain(`host' => 'prod.example.org`);
 
 		// Check logged strings are source mapped
 		expect(output).toMatch(
@@ -79,13 +83,33 @@ describe("'wrangler dev' correctly renders pages", () => {
 		expect(text).toMatch(/[0-9a-f]{16}/); // 8 hex bytes
 	});
 
-	it("passes through URL host and path unchanged", async ({ expect }) => {
-		const url = `http://${ip}:${port}//thing?a=1`;
+	it("passes through URL unchanged", async ({ expect }) => {
 		const response = await fetch(`http://${ip}:${port}//thing?a=1`, {
 			headers: { "X-Test-URL": "true" },
 		});
 		const text = await response.text();
-		// Note that the protocol changes due to the upstream_protocol configuration of Wrangler in the `beforeAll()`.
-		expect(text).toBe(`https://${ip}:${port}//thing?a=1`);
+		expect(text).toBe(`https://prod.example.org//thing?a=1`);
+	});
+
+	it("updates the Host and Origin headers appropriately", async ({
+		expect,
+	}) => {
+		const response = await fetch(`http://${ip}:${port}/test`, {
+			// Pass in an Origin header to trigger the rewriting
+			headers: { Origin: `http://${ip}:${port}` },
+		});
+		const text = await response.text();
+		console.log(text);
+		expect(text).toContain(`HOST:prod.example.org`);
+		expect(text).toContain(`ORIGIN:https://prod.example.org`);
+	});
+
+	it("does not update Origin header if one is not passed by the client", async ({
+		expect,
+	}) => {
+		const response = await fetch(`http://${ip}:${port}/test`, {});
+		const text = await response.text();
+		expect(text).toContain(`HOST:prod.example.org`);
+		expect(text).toContain(`ORIGIN:null`);
 	});
 });

packages/miniflare/src/workers/core/entry.worker.ts

@@ -45,11 +45,10 @@ function getUserRequest(
 	const originalUrl = request.headers.get(CoreHeaders.ORIGINAL_URL);
 	let url = new URL(originalUrl ?? request.url);
 
-	// The `upstreamHost` is used to override the `Host` header on the request being handled.
-	let upstreamHost: string | undefined;
+	let rewriteHeadersFromOriginalUrl = false;
 
-	// If the request is signed by an upstream proxy then we can use the one from the ORIGINAL_URL.
-	// The shared secret is required to prevent a malicious user being able to change the host header without permission.
+	// If the request is signed by a proxy server then we can use the Host and Origin from the ORIGINAL_URL.
+	// The shared secret is required to prevent a malicious user being able to change the headers without permission.
 	const proxySharedSecret = request.headers.get(
 		CoreHeaders.PROXY_SHARED_SECRET
 	);
@@ -60,7 +59,7 @@ function getUserRequest(
 			secretFromHeader.byteLength === configuredSecret?.byteLength &&
 			crypto.subtle.timingSafeEqual(secretFromHeader, configuredSecret)
 		) {
-			upstreamHost = url.host;
+			rewriteHeadersFromOriginalUrl = true;
 		} else {
 			throw new HttpError(
 				400,
@@ -77,7 +76,7 @@ function getUserRequest(
 		// Remove leading slash, so we resolve relative to `upstream`'s path
 		if (path.startsWith("/")) path = `./${path.substring(1)}`;
 		url = new URL(path, upstreamUrl);
-		upstreamHost = url.host;
+		rewriteHeadersFromOriginalUrl = true;
 	}
 
 	// Note when constructing new `Request`s from `request`, we must always pass
@@ -92,9 +91,15 @@ function getUserRequest(
 	if (request.cf === undefined) {
 		request = new Request(request, { cf: env[CoreBindings.JSON_CF_BLOB] });
 	}
-	if (upstreamHost !== undefined) {
-		request.headers.set("Host", upstreamHost);
+
+	if (rewriteHeadersFromOriginalUrl) {
+		request.headers.set("Host", url.host);
+		// Only rewrite Origin header if there is already one
+		if (request.headers.has("Origin")) {
+			request.headers.set("Origin", url.origin);
+		}
 	}
+
 	request.headers.delete(CoreHeaders.PROXY_SHARED_SECRET);
 	request.headers.delete(CoreHeaders.ORIGINAL_URL);
 	request.headers.delete(CoreHeaders.DISABLE_PRETTY_ERROR);