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

petebacondarwin · 2024-02-07T22:48:39Z

In #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.

Author has addressed the following:

Tests
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- Included
- Not necessary because:
Associated docs
- Issue(s)/PR(s):
- Not necessary because: bug fix

changeset-bot · 2024-02-07T22:48:43Z

🦋 Changeset detected

Latest commit: ded262e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
miniflare	Patch
wrangler	Patch
@cloudflare/pages-shared	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-02-07T22:50:43Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7843359492/npm-package-wrangler-4950

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/4950/npm-package-wrangler-4950

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7843359492/npm-package-wrangler-4950 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7843359492/npm-package-create-cloudflare-4950 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7843359492/npm-package-miniflare-4950

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7843359492/npm-package-cloudflare-pages-shared-4950

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240129.1
`workerd`	1.20240129.0	1.20240129.0
`workerd --version`	1.20240129.0	2024-01-29

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

codecov · 2024-02-07T22:56:09Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ffafe8a) 70.43% compared to head (ded262e) 70.52%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4950      +/-   ##
==========================================
+ Coverage   70.43%   70.52%   +0.08%     
==========================================
  Files         295      295              
  Lines       15407    15419      +12     
  Branches     3948     3953       +5     
==========================================
+ Hits        10852    10874      +22     
+ Misses       4555     4545      -10

Files	Coverage Δ
packages/wrangler/src/api/startDevWorker/events.ts	`15.38% <ø> (ø)`
packages/wrangler/src/dev/remote.tsx	`7.92% <0.00%> (ø)`

... and 8 files with indirect coverage changes

petebacondarwin · 2024-02-08T09:16:06Z

packages/wrangler/src/dev/remote.tsx

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


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.

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

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).

petebacondarwin · 2024-02-08T09:16:30Z

packages/wrangler/src/dev/remote.tsx

+					hostname: props.host,
+					port: props.port.toString(),


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

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.

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

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.

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?~~

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

RamIdeas · 2024-02-08T14:33:19Z

packages/wrangler/src/dev/remote.tsx

+					hostname: props.host,
+					port: props.port.toString(),


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.

RamIdeas · 2024-02-08T15:01:38Z

packages/wrangler/src/dev/remote.tsx

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


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

RamIdeas · 2024-02-08T15:02:53Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

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


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

I'll tweak the description.

RamIdeas · 2024-02-08T15:04:37Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

@@ -127,6 +130,8 @@ export class ProxyWorker implements DurableObject {
 			// if we decide to await, we should include a timeout (~100ms) in case the user worker has long-running/parellel requests
 			void fetch(userWorkerUrl, new Request(request, { headers }))
 				.then((res) => {
+					rewriteUrlRelatedHeaders(headers, innerUrl, outerUrl);


Ah yes both directions! This is clever. Now the browser will have no idea that the URL was faked inside the worker either 😄

Small bug/typo:

Suggested change

rewriteUrlRelatedHeaders(headers, innerUrl, outerUrl);

rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl);

But you might not be able to mutate response headers without:

Suggested change

rewriteUrlRelatedHeaders(headers, innerUrl, outerUrl);

res = new Response(res);

rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl);

That's not a typo! It's a bona fide mistake! Thanks for spotting that.

I also think this is a "new"-ish feature that should get its own changeset and documentation. It's really cool that a dev could write Response.redirect(`${url.origin}/redirect/path`) and that would "just work"™️ in wrangler dev [--remote] – we should shout from the rooftops!

Strictly this was already part of the old proxy:

workers-sdk/packages/wrangler/src/dev/proxy.ts

Line 70 in d637bd5

function rewriteRemoteHostToLocalHostInHeaders(

Fixed this problem in a fixup commit and added a test to prove it.

petebacondarwin

Thanks for the review

petebacondarwin · 2024-02-08T15:12:58Z

packages/wrangler/src/dev/remote.tsx

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


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).

petebacondarwin · 2024-02-08T15:13:33Z

packages/wrangler/src/dev/remote.tsx

+					hostname: props.host,
+					port: props.port.toString(),


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

petebacondarwin · 2024-02-08T15:14:31Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

@@ -127,6 +130,8 @@ export class ProxyWorker implements DurableObject {
 			// if we decide to await, we should include a timeout (~100ms) in case the user worker has long-running/parellel requests
 			void fetch(userWorkerUrl, new Request(request, { headers }))
 				.then((res) => {
+					rewriteUrlRelatedHeaders(headers, innerUrl, outerUrl);


That's not a typo! It's a bona fide mistake! Thanks for spotting that.

petebacondarwin · 2024-02-08T15:14:50Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

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


I'll tweak the description.

In #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.

…ler dev

RamIdeas

Happy to merge. Might want to hold off merging since I believe we want to get a hot fix out today.

One remaining point (you decide whether to address or not): should we print something to the console to let the user know the origin/host is being rewritten across all request/response headers inc Set-Cookie (and not the body) and let them know they can opt-out by...?

petebacondarwin · 2024-02-09T11:25:14Z

Happy to merge. Might want to hold off merging since I believe we want to get a hot fix out today.

One remaining point (you decide whether to address or not): should we print something to the console to let the user know the origin/host is being rewritten across all request/response headers inc Set-Cookie (and not the body) and let them know they can opt-out by...?

Yeah good question... we have not previously had a message for rewriting the request.url, and prior to 3.18, I believe that we would have been rewriting response headers in this way anyway.

~~I've put some debug logging messages in, so at least if the user is getting confused it could be analysed via increasing the logging level.~~ I forgot this is workerd and can't use the Wrangler logger here...

penalosa

The edge-preview-authenticated-proxy stuff here looks good!

RamIdeas · 2024-02-09T11:56:02Z

~~I've put some debug logging messages in, so at least if the user is getting confused it could be analysed via increasing the logging level.~~ I forgot this is workerd and can't use the Wrangler logger here...

In remote mode, yeah no, but locally workerd logs now reach the log file (not the terminal but that's what you were going for anyway)

petebacondarwin · 2024-02-09T12:18:13Z

In remote mode, yeah no, but locally workerd logs now reach the log file (not the terminal but that's what you were going for anyway)

Hmm I tried adding console.debug() logs to the ProxyWorker but these always appeared in the output of wrangler dev (whatever the logging level). So I think this is too much noise. Perhaps we need to tweak the Proxy Worker's Miniflare configuration?

I think we can leave this to another time, or if someone gets confused by this rewriting.

jfnault-seedbox · 2024-03-14T00:02:20Z

@petebacondarwin Can you please take a look at
#5221

I think this PR is related, I had couple of notes:
#5221 (comment)

petebacondarwin requested a review from a team as a code owner February 7, 2024 22:48

petebacondarwin commented Feb 8, 2024

View reviewed changes

petebacondarwin force-pushed the fix-host-origin-header branch from e904944 to 3c37d20 Compare February 8, 2024 10:26

RamIdeas suggested changes Feb 8, 2024

View reviewed changes

petebacondarwin commented Feb 8, 2024

View reviewed changes

petebacondarwin added 2 commits February 9, 2024 10:14

fixup! fix: ensure we do not rewrite external Origin headers in wrang…

ee8af06

…ler dev

petebacondarwin force-pushed the fix-host-origin-header branch from 3c37d20 to 0e5e856 Compare February 9, 2024 10:15

fixup! fix: ensure we do not rewrite external Origin headers in wrang…

7db6025

…ler dev

petebacondarwin force-pushed the fix-host-origin-header branch from 0e5e856 to 7db6025 Compare February 9, 2024 10:18

fixup! fix: ensure we do not rewrite external Origin headers in wrang…

ded262e

…ler dev

petebacondarwin force-pushed the fix-host-origin-header branch from 6eb14e7 to ded262e Compare February 9, 2024 10:45

petebacondarwin requested a review from RamIdeas February 9, 2024 11:09

RamIdeas approved these changes Feb 9, 2024

View reviewed changes

petebacondarwin requested a review from penalosa February 9, 2024 11:30

petebacondarwin force-pushed the fix-host-origin-header branch from c621c64 to ded262e Compare February 9, 2024 11:33

penalosa approved these changes Feb 9, 2024

View reviewed changes

petebacondarwin merged commit 05360e4 into main Feb 9, 2024
31 checks passed

petebacondarwin deleted the fix-host-origin-header branch February 9, 2024 12:18

workers-devprod mentioned this pull request Feb 9, 2024

Version Packages #4964

Merged

petebacondarwin mentioned this pull request Feb 12, 2024

🐛 BUG: Origin header is ignored in 3.25.0 - 3.27.0 versions #4849

Closed

penalosa mentioned this pull request Feb 15, 2024

[quick-edit] Partitioned cookies #5016

Merged

6 tasks

petebacondarwin mentioned this pull request Feb 19, 2024

[quick-edit] Deploy Partitioned Cookies #5058

Merged

workers-devprod mentioned this pull request Feb 20, 2024

Version Packages #5061

Merged

jfnault-seedbox mentioned this pull request Mar 14, 2024

🐛 Bug Report — Runtime APIs: Get an incorrect Location param when using Response.redirect #5221

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petebacondarwin commented Feb 7, 2024

changeset-bot bot commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

petebacondarwin Feb 8, 2024

RamIdeas Feb 8, 2024

petebacondarwin Feb 8, 2024

petebacondarwin Feb 8, 2024

RamIdeas Feb 8, 2024

petebacondarwin Feb 8, 2024

petebacondarwin Feb 9, 2024

RamIdeas Feb 9, 2024 •

edited

Loading

petebacondarwin Feb 9, 2024

RamIdeas Feb 8, 2024

RamIdeas Feb 8, 2024

RamIdeas Feb 8, 2024

petebacondarwin Feb 8, 2024

RamIdeas Feb 8, 2024

petebacondarwin Feb 8, 2024

RamIdeas Feb 8, 2024 •

edited

Loading

petebacondarwin Feb 8, 2024

petebacondarwin Feb 9, 2024

petebacondarwin left a comment

petebacondarwin Feb 8, 2024

petebacondarwin Feb 8, 2024

petebacondarwin Feb 8, 2024

petebacondarwin Feb 8, 2024

RamIdeas left a comment

petebacondarwin commented Feb 9, 2024 •

edited

Loading

penalosa left a comment

RamIdeas commented Feb 9, 2024

petebacondarwin commented Feb 9, 2024

jfnault-seedbox commented Mar 14, 2024

	rewriteUrlRelatedHeaders(headers, innerUrl, outerUrl);
	rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl);

	rewriteUrlRelatedHeaders(headers, innerUrl, outerUrl);
	res = new Response(res);
	rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl);

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

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

Conversation

petebacondarwin commented Feb 7, 2024

changeset-bot bot commented Feb 7, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 7, 2024 • edited Loading

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas left a comment

Choose a reason for hiding this comment

petebacondarwin commented Feb 9, 2024 • edited Loading

penalosa left a comment

Choose a reason for hiding this comment

RamIdeas commented Feb 9, 2024

petebacondarwin commented Feb 9, 2024

jfnault-seedbox commented Mar 14, 2024

changeset-bot bot commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

RamIdeas Feb 9, 2024 •

edited

Loading

RamIdeas Feb 8, 2024 •

edited

Loading

petebacondarwin commented Feb 9, 2024 •

edited

Loading