Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure that the Pages dev proxy server does not change the Host header #4888

Merged
merged 4 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/orange-emus-mate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"wrangler": patch
---

fix: ensure that the Pages dev proxy server does not change the Host header

Previously, when configuring `wrangler pages dev` to use a proxy to a 3rd party dev server,
the proxy would replace the Host header, resulting in problems at the dev server if it was
checking for cross-site scripting attacks.

Now the proxy server passes through the Host header unaltered making it invisible to the
3rd party dev server.

Fixes #4799
25 changes: 25 additions & 0 deletions fixtures/pages-proxy-app/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"name": "pages-proxy-app",
"version": "0.1.2",
"private": true,
"sideEffects": false,
"main": "server/index.js",
"scripts": {
"build": "esbuild --bundle --platform=node server/index.ts --outfile=dist/index.js",
"check:type": "tsc",
"dev": "npx wrangler pages dev --compatibility-date=2024-01-17 --port 8790 --proxy 8791 -- pnpm run server",
"server": "node dist/index.js",
"test": "vitest run",
"test:watch": "vitest",
"type:tests": "tsc -p ./tests/tsconfig.json"
},
"devDependencies": {
"@cloudflare/workers-tsconfig": "workspace:*",
"miniflare": "workspace:*",
"undici": "^5.28.2",
"wrangler": "workspace:*"
},
"engines": {
"node": ">=14"
}
}
10 changes: 10 additions & 0 deletions fixtures/pages-proxy-app/server/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { createServer } from "http";

const server = createServer();

server.on("request", (req, res) => {
res.write("Host:" + req.headers.host);
res.end();
});

server.listen(8791);
36 changes: 36 additions & 0 deletions fixtures/pages-proxy-app/tests/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { fork } from "node:child_process";
import { resolve } from "node:path";
import { fetch } from "undici";
import { afterAll, beforeAll, describe, it } from "vitest";
import { runWranglerPagesDev } from "../../shared/src/run-wrangler-long-lived";
import type { ChildProcess } from "node:child_process";

describe("pages-proxy-app", async () => {
let ip: string, port: number, stop: (() => Promise<unknown>) | undefined;
let devServer: ChildProcess;

beforeAll(async () => {
devServer = fork(resolve(__dirname, "../dist/index.js"), {
stdio: "ignore",
});

debugger;
({ ip, port, stop } = await runWranglerPagesDev(
resolve(__dirname, ".."),
undefined,
["--port=0", "--inspector-port=0", "--proxy=8791"]
));
});

afterAll(async () => {
await stop?.();
devServer.kill();
});

it("receives the correct Host header", async ({ expect }) => {
debugger;
const response = await fetch(`http://${ip}:${port}/`);
const text = await response.text();
expect(text).toContain(`Host:${ip}:${port}`);
});
});
7 changes: 7 additions & 0 deletions fixtures/pages-proxy-app/tests/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"extends": "@cloudflare/workers-tsconfig/tsconfig.json",
"compilerOptions": {
"types": ["node"]
},
"include": ["**/*.ts", "../../../node-types.d.ts"]
}
13 changes: 13 additions & 0 deletions fixtures/pages-proxy-app/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"include": ["server"],
"compilerOptions": {
"target": "ES2020",
"module": "CommonJS",
"lib": ["ES2020"],
"types": ["node"],
"moduleResolution": "node",
"esModuleInterop": true,
"noEmit": true,
"skipLibCheck": true
}
}
9 changes: 9 additions & 0 deletions fixtures/pages-proxy-app/turbo.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"$schema": "http://turbo.build/schema.json",
"extends": ["//"],
"pipeline": {
"build": {
"outputs": ["dist/**"]
}
}
}
9 changes: 9 additions & 0 deletions fixtures/pages-proxy-app/vitest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { defineProject, mergeConfig } from "vitest/config";
import configShared from "../../vitest.shared";

export default mergeConfig(
configShared,
defineProject({
test: {},
})
);
8 changes: 6 additions & 2 deletions fixtures/shared/src/run-wrangler-long-lived.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@ export const wranglerEntryPath = path.resolve(
*/
export async function runWranglerPagesDev(
cwd: string,
publicPath: string,
publicPath: string | undefined,
options: string[]
) {
return runLongLivedWrangler(["pages", "dev", publicPath, ...options], cwd);
if (publicPath) {
return runLongLivedWrangler(["pages", "dev", publicPath, ...options], cwd);
} else {
return runLongLivedWrangler(["pages", "dev", ...options], cwd);
}
}

/**
Expand Down
65 changes: 64 additions & 1 deletion packages/wrangler/src/miniflare-cli/assets.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import assert from "node:assert";
import { existsSync, lstatSync, readFileSync } from "node:fs";
import { join, resolve } from "node:path";
import { createMetadataObject } from "@cloudflare/pages-shared/metadata-generator/createMetadataObject";
Expand All @@ -6,6 +7,7 @@ import { parseRedirects } from "@cloudflare/pages-shared/metadata-generator/pars
import { watch } from "chokidar";
import { getType } from "mime";
import { fetch, Request, Response } from "miniflare";
import { Dispatcher, getGlobalDispatcher } from "undici";
import { hashFile } from "../pages/hash";
import type { Logger } from "../logger";
import type { Metadata } from "@cloudflare/pages-shared/asset-server/metadata";
Expand All @@ -15,6 +17,7 @@ import type {
} from "@cloudflare/pages-shared/metadata-generator/types";
import type { Request as WorkersRequest } from "@cloudflare/workers-types/experimental";
import type { RequestInit } from "miniflare";
import type { IncomingHttpHeaders } from "undici/types/header";

export interface Options {
log: Logger;
Expand All @@ -38,7 +41,9 @@ export default async function generateASSETSBinding(options: Options) {
proxyRequest.headers.delete("Sec-WebSocket-Accept");
proxyRequest.headers.delete("Sec-WebSocket-Key");
}
return await fetch(proxyRequest);
return await fetch(proxyRequest, {
dispatcher: new ProxyDispatcher(miniflareRequest.headers.get("Host")),
});
} catch (thrown) {
options.log.error(new Error(`Could not proxy request: ${thrown}`));

Expand All @@ -63,6 +68,64 @@ export default async function generateASSETSBinding(options: Options) {
};
}

/**
* An Undici custom Dispatcher that is used for the fetch requests
* of the Pages dev server proxy.
*
* Notably, the ProxyDispatcher reinstates the Host header that was removed by the
* Undici `fetch` function call. Undici removes the Host header as a security precaution,
* but this is not relevant for an internal Proxy.
*
* The ProxyDispatcher will delegate through to the current global `Dispatcher`,
* ensuring that the request is routed correctly in case a developer has changed the
* global Dispatcher by a call to `setGlobalDispatcher()`.
*/
class ProxyDispatcher extends Dispatcher {
private dispatcher: Dispatcher = getGlobalDispatcher();

constructor(private host: string | null) {
super();
}

dispatch(
options: Dispatcher.DispatchOptions,
handler: Dispatcher.DispatchHandlers
): boolean {
if (this.host !== null) {
ProxyDispatcher.reinstateHostHeader(options.headers, this.host);
}
return this.dispatcher.dispatch(options, handler);
}

close() {
return this.dispatcher.close();
}

destroy() {
return this.dispatcher.destroy();
}

/**
* Ensure that the request contains a Host header, which would have been deleted
* by the `fetch()` function before calling `dispatcher.dispatch()`.
*/
private static reinstateHostHeader(
headers: string[] | IncomingHttpHeaders | null | undefined,
host: string
) {
assert(headers, "Expected all proxy requests to contain headers.");
assert(
petebacondarwin marked this conversation as resolved.
Show resolved Hide resolved
!Array.isArray(headers),
"Expected proxy request headers to be a hash object"
);
assert(
Object.keys(headers).every((h) => h.toLowerCase() !== "host"),
"Expected Host header to have been deleted."
);
headers["Host"] = host;
}
}

async function generateAssetsFetch(
directory: string,
log: Logger
Expand Down
15 changes: 15 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading