fix: miniflare now sets the "Host" header to match the upstream URL #4630

petebacondarwin · 2023-12-19T12:33:29Z

Fixes #4643
Fixes cloudflare/next-on-pages#588

It is good to review this PR commit by commit.

What this PR solves / how to test:

Some full-stack frameworks, such as Next.js, check that the Host header for a server
side action request matches the host where the application is expected to run.

In wrangler dev we have a Proxy Worker in between the browser and the actual User Worker.
This Proxy Worker is forwarding on the request from the browser, but then the actual User
Worker is running on a different host:port combination than that which the browser thinks
it should be on. This was causing the framework to think the request is malicious and blocking
it.

Now we update the request's Host header to that passed from the Proxy Worker in a custom MF-Original-Url
header, but only do this if the request also contains a shared secret between the Proxy Worker
and User Worker, which is passed via the MF-Proxy-Shared-Secret header. This last feature is to
prevent a malicious website from faking the Host header in a request directly to the User Worker.

The worker-app fixture has been updated to check that this is working as expected.

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: internal changes requiring no documentation

changeset-bot · 2023-12-19T12:33:33Z

🦋 Changeset detected

Latest commit: 66ea286

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 · 2023-12-19T12:37:26Z

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/7396783059/npm-package-wrangler-4630

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

wrangler@3.22.2 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20231218.0
`workerd`	1.20231218.0	1.20231218.0
`workerd --version`	1.20231218.0	2023-12-18

|

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

codecov · 2023-12-19T12:39:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9e03272) 75.58% compared to head (66ea286) 75.67%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4630      +/-   ##
==========================================
+ Coverage   75.58%   75.67%   +0.08%     
==========================================
  Files         243      243              
  Lines       13084    13087       +3     
  Branches     3368     3368              
==========================================
+ Hits         9890     9903      +13     
+ Misses       3194     3184      -10

Files	Coverage Δ
packages/wrangler/src/dev/local.tsx	`25.92% <ø> (ø)`
packages/wrangler/src/dev/miniflare.ts	`63.30% <100.00%> (+2.37%)`	⬆️
packages/wrangler/src/dev/start-server.ts	`78.03% <ø> (ø)`

... and 6 files with indirect coverage changes

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

packages/miniflare/test/index.spec.ts

mrbbot

Nice! I've added some comments, but LGTM! 👍

mrbbot · 2023-12-20T17:34:36Z

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

@@ -33,15 +34,39 @@ function getUserRequest(
 	request: Request<unknown, IncomingRequestCfProperties>,
 	env: Env
 ) {
+	// The original URL is a header that is passed by Miniflare to the user worker
+	// in case the request was rewritten on its way to the user worker.


Could you update this comment to clarify that ORIGINAL_URL is also set when calling Miniflare#dispatchFetch(...) with the passed URL?

I updated the comment, but I am not 100% it is what you wanted.

mrbbot · 2023-12-20T17:36:17Z

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

+		CoreHeaders.PROXY_SHARED_SECRET
+	);
+	if (proxySharedSecret) {
+		if (proxySharedSecret === env[CoreBindings.TEXT_PROXY_SHARED_SECRET]) {


Unlikely to be a problem, but really this should be crypto.subtle.timingSafeEqual() to prevent timing attacks. This would require the shared secret to be an encoding of some uniformly distributed byte array (e.g. random hex string).

workers-sdk/packages/miniflare/src/workers/core/proxy.worker.ts

Line 155 in 63708a9

!crypto.subtle.timingSafeEqual(secretBuffer, expectedSecret)

Updated to use this approach.

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

…ating host from original URL header

…local mode Some full-stack frameworks, such as Next.js, check that the Host header for a server side action request matches the host where the application is expected to run. In `wrangler dev` we have a Proxy Worker in between the browser and the actual User Worker. This Proxy Worker is forwarding on the request from the browser, but then the actual User Worker is running on a different host:port combination than that which the browser thinks it should be on. This was causing the framework to think the request is malicious and blocking it. Now we update the request's Host header to that passed from the Proxy Worker in a custom `MF-Original-Url` header, but only do this if the request also contains a shared secret between the Proxy Worker and User Worker, which is passed via the `MF-Proxy-Shared-Secret` header. This last feature is to prevent a malicious website from faking the Host header in a request directly to the User Worker. Fixes cloudflare/next-on-pages#588

This avoids prop drilling.

This helps to avoid an attacker guessing the secret via the timing of the comparison.

notes: - this is needed so that we can add e2e tests for server actions: cloudflare/workers-sdk#4630 - we the commit also updates node from 18 to 20 in e2e github acitons testing, this is necessary for using the latest wrangler release, see: cloudflare/workers-sdk#4563 (comment)

* add vitest.config.ts file with retry for better tests stability * add new e2e features to test wasm * avoid inconsistent kebab-case * use latest wrangler release notes: - this is needed so that we can add e2e tests for server actions: cloudflare/workers-sdk#4630 - we the commit also updates node from 18 to 20 in e2e github acitons testing, this is necessary for using the latest wrangler release, see: cloudflare/workers-sdk#4563 (comment) * add app server-actions e2e test * remove unnecessary existsSync assertion in processVercelOutput unit tests

petebacondarwin requested a review from a team as a code owner December 19, 2023 12:33

petebacondarwin assigned RamIdeas and mrbbot Dec 19, 2023

petebacondarwin added bug Something that isn't working miniflare Relating to Miniflare wrangler Relating to the Wrangler CLI tool e2e Run wrangler e2e tests on a PR labels Dec 19, 2023

petebacondarwin mentioned this pull request Dec 19, 2023

[🐛 Bug]: Server actions don't work with wrangler pages dev cloudflare/next-on-pages#588

Closed

RamIdeas reviewed Dec 20, 2023

View reviewed changes

packages/miniflare/src/workers/core/entry.worker.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Dec 20, 2023

View reviewed changes

packages/miniflare/test/index.spec.ts Outdated Show resolved Hide resolved

petebacondarwin force-pushed the miniflare-upstream-host branch 6 times, most recently from 86029d3 to b3337e9 Compare December 20, 2023 14:44

RamIdeas approved these changes Dec 20, 2023

View reviewed changes

mrbbot approved these changes Dec 20, 2023

View reviewed changes

petebacondarwin mentioned this pull request Dec 21, 2023

🐛 BUG: --host flag not respected #4643

Closed

petebacondarwin force-pushed the miniflare-upstream-host branch from 5797645 to d98e0f8 Compare December 21, 2023 11:34

petebacondarwin requested a review from mrbbot December 21, 2023 16:47

mrbbot reviewed Jan 3, 2024

View reviewed changes

packages/miniflare/src/workers/core/entry.worker.ts Show resolved Hide resolved

packages/miniflare/src/workers/core/entry.worker.ts Outdated Show resolved Hide resolved

petebacondarwin and others added 5 commits January 3, 2024 11:23

fix: miniflare now sets the "Host" header to match the upstream URL

e1b1351

feat: miniflare exposes unsafeProxySignature config that controls upd…

ed8a4fc

…ating host from original URL header

rename "proxy signature" to "proxy shared secret"

feaeed8

Move proxy shared secret to where it is needed

668655f

This avoids prop drilling.

petebacondarwin added 2 commits January 3, 2024 11:23

Use timingSafeEqual for secret comparison

5e3dc36

This helps to avoid an attacker guessing the secret via the timing of the comparison.

fixup! Use timingSafeEqual for secret comparison

66ea286

petebacondarwin force-pushed the miniflare-upstream-host branch from d98e0f8 to 66ea286 Compare January 3, 2024 11:37

petebacondarwin merged commit 037de5e into main Jan 3, 2024
19 checks passed

petebacondarwin deleted the miniflare-upstream-host branch January 3, 2024 12:28

workers-devprod mentioned this pull request Jan 3, 2024

Version Packages #4691

Merged

Le0Developer mentioned this pull request Jan 17, 2024

🐛 BUG MF: host & origin header out of sync #4761

Closed

workers-devprod mentioned this pull request Feb 20, 2024

Version Packages #5061

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: miniflare now sets the "Host" header to match the upstream URL #4630

fix: miniflare now sets the "Host" header to match the upstream URL #4630

petebacondarwin commented Dec 19, 2023 •

edited

Loading

changeset-bot bot commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading

codecov bot commented Dec 19, 2023 •

edited

Loading

mrbbot left a comment

mrbbot Dec 20, 2023

petebacondarwin Dec 21, 2023

mrbbot Dec 20, 2023

petebacondarwin Dec 21, 2023

fix: miniflare now sets the "Host" header to match the upstream URL #4630

fix: miniflare now sets the "Host" header to match the upstream URL #4630

Conversation

petebacondarwin commented Dec 19, 2023 • edited Loading

changeset-bot bot commented Dec 19, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Dec 19, 2023 • edited Loading

codecov bot commented Dec 19, 2023 • edited Loading

Codecov Report

mrbbot left a comment

Choose a reason for hiding this comment

mrbbot Dec 20, 2023

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

mrbbot Dec 20, 2023

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

petebacondarwin commented Dec 19, 2023 •

edited

Loading

changeset-bot bot commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading

codecov bot commented Dec 19, 2023 •

edited

Loading