diff --git a/.changeset/selfish-starfishes-cry.md b/.changeset/selfish-starfishes-cry.md new file mode 100644 index 000000000000..c151f6eb56e6 --- /dev/null +++ b/.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 diff --git a/fixtures/pages-d1-shim/tests/index.test.ts b/fixtures/pages-d1-shim/tests/index.test.ts index 79f57498cd8b..6f8b61288bf8 100644 --- a/fixtures/pages-d1-shim/tests/index.test.ts +++ b/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\\":{}}}"`, diff --git a/fixtures/pages-workerjs-directory/tests/index.test.ts b/fixtures/pages-workerjs-directory/tests/index.test.ts index bb7dbf2e3d06..3be3c780e2d1 100644 --- a/fixtures/pages-workerjs-directory/tests/index.test.ts +++ b/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\\":{}}}"`, diff --git a/fixtures/worker-app/src/index.js b/fixtures/worker-app/src/index.js index c18f9f5ac86c..c28a563063a6 100644 --- a/fixtures/worker-app/src/index.js +++ b/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")}` ); }, diff --git a/fixtures/worker-app/tests/index.test.ts b/fixtures/worker-app/tests/index.test.ts index fbe98fb578a1..d98c883b3fe8 100644 --- a/fixtures/worker-app/tests/index.test.ts +++ b/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`); }); }); diff --git a/packages/miniflare/src/workers/core/entry.worker.ts b/packages/miniflare/src/workers/core/entry.worker.ts index 1a2af03877de..fe0431ca06dc 100644 --- a/packages/miniflare/src/workers/core/entry.worker.ts +++ b/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);