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.

Sign up for GitHub

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 we do not rewrite external Origin headers in wrangler dev #4950

Merged
merged 4 commits into from
Feb 9, 2024
Merged

fix: ensure we do not rewrite external Origin headers in wrangler dev #4950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2112775
fix: ensure we do not rewrite external Origin headers in wrangler dev
petebacondarwin Feb 7, 2024
ee8af06
fixup! fix: ensure we do not rewrite external Origin headers in wrang…
petebacondarwin Feb 8, 2024
7db6025
fixup! fix: ensure we do not rewrite external Origin headers in wrang…
petebacondarwin Feb 8, 2024
ded262e
fixup! fix: ensure we do not rewrite external Origin headers in wrang…
petebacondarwin Feb 8, 2024
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
12 changes: 12 additions & 0 deletions .changeset/curly-drinks-tell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"miniflare": patch
"wrangler": patch
---

fix: ensure we do not rewrite external Origin headers in wrangler dev

In https://github.com/cloudflare/workers-sdk/pull/4812 we tried to fix the Origin headers to match the Host header but were overzealous and rewrote Origin headers for external origins (outside of the proxy server's origin).

This is now fixed, and moreover we rewrite any headers that refer to the proxy server on the request with the configured host and vice versa on the response.

This should ensure that CORS is not broken in browsers when a different host is being simulated based on routes in the Wrangler configuration.
3 changes: 2 additions & 1 deletion fixtures/worker-app/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ export default {
async fetch(request) {
console.log("request log");

const { pathname } = new URL(request.url);
const { pathname, origin } = new URL(request.url);
if (pathname === "/random") return new Response(hexEncode(randomBytes(8)));
if (pathname === "/error") throw new Error("Oops!");
if (pathname === "/redirect") return Response.redirect(`${origin}/foo`);
if (request.headers.get("X-Test-URL") !== null) {
return new Response(request.url);
}
Expand Down
31 changes: 29 additions & 2 deletions fixtures/worker-app/tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe("'wrangler dev' correctly renders pages", () => {
expect(text).toBe(`https://prod.example.org//thing?a=1`);
});

it("updates the Host and Origin headers appropriately", async ({
it("rewrites the Host and Origin headers appropriately", async ({
expect,
}) => {
const response = await fetch(`http://${ip}:${port}/test`, {
Expand All @@ -104,12 +104,39 @@ describe("'wrangler dev' correctly renders pages", () => {
expect(text).toContain(`ORIGIN:https://prod.example.org`);
});

it("does not update Origin header if one is not passed by the client", async ({
it("does not rewrite 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`);
});

it("does not rewrite Origin header if it not the same origin as the proxy Worker", async ({
expect,
}) => {
const response = await fetch(`http://${ip}:${port}/test`, {
headers: { Origin: `http://foo.com` },
});
const text = await response.text();
console.log(text);
expect(text).toContain(`HOST:prod.example.org`);
expect(text).toContain(`ORIGIN:http://foo.com`);
});

it("rewrites response headers containing the emulated host", async ({
expect,
}) => {
// This /redirect request will add a Location header that points to prod.example.com/foo
// But we should rewrite this back to that of the proxy.
const response = await fetch(`http://${ip}:${port}/redirect`, {
redirect: "manual",
});
expect(response.status).toBe(302);
expect(await response.text()).toEqual("");
expect(response.headers.get("Location")).toEqual(
`http://${ip}:${port}/foo`
);
});
});
16 changes: 8 additions & 8 deletions packages/edge-preview-authenticated-proxy/tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ compatibility_date = "2023-01-01"
expect(
removeUUID(resp.headers.get("set-cookie") ?? "")
).toMatchInlineSnapshot(
'"token=00000000-0000-0000-0000-000000000000; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None"'
'"token=00000000-0000-0000-0000-000000000000; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None"'
);
tokenId = (resp.headers.get("set-cookie") ?? "")
.split(";")[0]
Expand All @@ -152,7 +152,7 @@ compatibility_date = "2023-01-01"
{
method: "GET",
headers: {
cookie: `token=${tokenId}; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
cookie: `token=${tokenId}; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
},
}
);
Expand Down Expand Up @@ -183,7 +183,7 @@ compatibility_date = "2023-01-01"
expect(
removeUUID(resp.headers.get("set-cookie") ?? "")
).toMatchInlineSnapshot(
'"token=00000000-0000-0000-0000-000000000000; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None"'
'"token=00000000-0000-0000-0000-000000000000; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None"'
);
tokenId = (resp.headers.get("set-cookie") ?? "")
.split(";")[0]
Expand Down Expand Up @@ -218,7 +218,7 @@ compatibility_date = "2023-01-01"
{
method: "GET",
headers: {
cookie: `token=${tokenId}; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
cookie: `token=${tokenId}; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
},
}
);
Expand All @@ -237,7 +237,7 @@ compatibility_date = "2023-01-01"
{
method: "GET",
headers: {
cookie: `token=${tokenId}; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
cookie: `token=${tokenId}; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
},
redirect: "manual",
}
Expand All @@ -255,7 +255,7 @@ compatibility_date = "2023-01-01"
{
method: "PUT",
headers: {
cookie: `token=${tokenId}; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
cookie: `token=${tokenId}; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
},
redirect: "manual",
}
Expand All @@ -270,7 +270,7 @@ compatibility_date = "2023-01-01"
method: "PUT",
headers: {
"X-Custom-Header": "custom",
cookie: `token=${tokenId}; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
cookie: `token=${tokenId}; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
},
redirect: "manual",
}
Expand All @@ -284,7 +284,7 @@ compatibility_date = "2023-01-01"
{
method: "PUT",
headers: {
cookie: `token=${tokenId}; Domain=preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
cookie: `token=${tokenId}; Domain=random-data.preview.devprod.cloudflare.dev; HttpOnly; Secure; SameSite=None`,
},
redirect: "manual",
}
Expand Down
4 changes: 0 additions & 4 deletions packages/miniflare/src/workers/core/entry.worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,6 @@ function getUserRequest(

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);
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/api/startDevWorker/events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export type ProxyData = {
userWorkerUrl: UrlOriginParts;
userWorkerInspectorUrl: UrlOriginAndPathnameParts;
userWorkerInnerUrlOverrides: Partial<UrlOriginParts>;
headers: Record<string, string | undefined>;
headers: Record<string, string>;
liveReload?: boolean;
proxyLogsToController?: boolean;
internalDurableObjects?: CfDurableObject[];
Expand Down
14 changes: 10 additions & 4 deletions packages/wrangler/src/dev/remote.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,13 @@
port: workerPreviewToken.inspectorUrl.port.toString(),
pathname: workerPreviewToken.inspectorUrl.pathname,
},
userWorkerInnerUrlOverrides: {}, // there is no analagous prop for this option because we did not permit overriding request.url in remote mode
userWorkerInnerUrlOverrides: {
hostname: props.host,
port: props.port.toString(),
},
headers: {
"cf-workers-preview-token": workerPreviewToken.value,
Cookie: accessToken && `CF_Authorization=${accessToken}`,
...(accessToken ? { Cookie: `CF_Authorization=${accessToken}` } : {}),

Check warning on line 332 in packages/wrangler/src/dev/remote.tsx

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/dev/remote.tsx#L332 

Added line #L332 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted that we had to make the headers record allow undefined values just to accommodate this Cookie assignment. But in reality we don't actually want to assign undefined to Cookie, since the code later one would result in the Cookie looking like: previous-cookies;undefined.

So I fixed this up here, removed the undefined from the type and was able to avoid a ! operator later on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fair enough. And is probably the "correct" solution. But this type will eventually be exposed to users to write themselves, and I believe allowing undefined will be more ergonomic and subtly improve DX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that allowing undefined here adds to user DX. If anything it creates a mental overhead because the user has to think about what setting to undefined actually means (when it actually means ignore this header and not remove the original header).

},
liveReload: false, // liveReload currently disabled in remote-mode, but will be supported with startDevWorker
proxyLogsToController: true,
Expand Down Expand Up @@ -436,10 +439,13 @@
port: previewToken.inspectorUrl.port.toString(),
pathname: previewToken.inspectorUrl.pathname,
},
userWorkerInnerUrlOverrides: {}, // there is no analagous prop for this option because we did not permit overriding request.url in remote mode
userWorkerInnerUrlOverrides: {
hostname: props.host,
port: props.port.toString(),
Comment on lines +443 to +444
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these here also fixes the remote dev mode Origin header rewriting!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit side-effecty... This will add a MF-Original-URL header which won't be interpreted and removed by miniflare in remote mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was a bit worried about that. Should we create new override properties that are specifically for header rewrites?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, as far as I can tell MF-Original-URL and MF-Disable-Pretty-Error are always set on every proxy worker request whether in local or remote mode. So I don't think that this change actually introduces any new side effect.

Whether those headers should be added in remote mode is a question for another PR I think.

Copy link
Contributor

@RamIdeas RamIdeas Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's follow up in another PR if needed.

I think in remote mode, the inner url overrides aren't set so the header wouldn't have been set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/templates/startDevWorker/ProxyWorker.ts#L125-L126

},
headers: {
"cf-workers-preview-token": previewToken.value,
Cookie: accessToken && `CF_Authorization=${accessToken}`,
...(accessToken ? { Cookie: `CF_Authorization=${accessToken}` } : {}),

Check warning on line 448 in packages/wrangler/src/dev/remote.tsx

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/dev/remote.tsx#L448 

Added line #L448 was not covered by tests
},
liveReload: false, // liveReload currently disabled in remote-mode, but will be supported with startDevWorker
proxyLogsToController: true,
Expand Down
25 changes: 24 additions & 1 deletion packages/wrangler/templates/startDevWorker/ProxyWorker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,11 @@ export class ProxyWorker implements DurableObject {
this.requestRetryQueue.delete(request);
this.requestQueue.delete(request);

const userWorkerUrl = new URL(request.url);
const outerUrl = new URL(request.url);
const headers = new Headers(request.headers);

// override url parts for proxying
const userWorkerUrl = new URL(request.url);
Object.assign(userWorkerUrl, proxyData.userWorkerUrl);

// set request.url in the UserWorker
Expand All @@ -125,6 +126,8 @@ export class ProxyWorker implements DurableObject {
headers.set("MF-Original-URL", innerUrl.href);
headers.set("MF-Disable-Pretty-Error", "true"); // disables the UserWorker miniflare instance from rendering the pretty error -- instead the ProxyWorker miniflare instance will intercept the json error response and render the pretty error page

rewriteUrlRelatedHeaders(headers, outerUrl, innerUrl);

// merge proxyData headers with the request headers
for (const [key, value] of Object.entries(proxyData.headers ?? {})) {
if (value === undefined) continue;
Expand All @@ -140,6 +143,9 @@ export class ProxyWorker implements DurableObject {
// explicitly NOT await-ing this promise, we are in a loop and want to process the whole queue quickly + synchronously
void fetch(userWorkerUrl, new Request(request, { headers }))
.then((res) => {
res = new Response(res.body, res);
rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl);

if (isHtmlResponse(res)) {
res = insertLiveReloadScript(request, res, this.env, proxyData);
}
Expand Down Expand Up @@ -290,3 +296,20 @@ function insertLiveReloadScript(

return htmlRewriter.transform(response);
}

/**
* Rewrite references to URLs in request/response headers.
*
* This function is used to map the URLs in headers like Origin and Access-Control-Allow-Origin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Access-Control-Allow-Origin is a response header so I assume you expect this function to be used in both directions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll tweak the description.

* so that this proxy is transparent to the Client Browser and User Worker.
*/
function rewriteUrlRelatedHeaders(headers: Headers, from: URL, to: URL) {
headers.forEach((value, key) => {
if (typeof value === "string" && value.includes(from.host)) {
headers.set(
key,
value.replaceAll(from.origin, to.origin).replaceAll(from.host, to.host)
);
}
});
}
Loading