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

[Miniflare 3] Assorted fixes #515

Merged
merged 4 commits into from
Feb 27, 2023
Merged

[Miniflare 3] Assorted fixes #515

merged 4 commits into from
Feb 27, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Feb 20, 2023

This PR includes fixes for a bunch of issues raised by @vlovich. See each commit description for a summary of the problem.

Previously, the `MF-Custom-Service` header was including on the
`request` parameter passed to custom service functions. This usually
wasn't a problem, but if the request ever made it back to the
loopback server (e.g. via `caches.default.put(request, ...)`), it
would be forwarded back to the custom service instead of the
appropriate plugin router.
In Miniflare 2, we removed `Transfer-Encoding`, `Connection`,
`Keep-Alive` and `Expect` headers from incoming Node HTTP requests,
before constructing a Node HTTP request and dispatching this to a
Worker. These headers are forbidden in `undici` `fetch` requests,
as they get added automatically.

We have a similar situation with custom service bindings:
requests may be passed straight through to another `fetch`, so
remove the same set of headers.

We also remove `Content-Length: 0` headers from `dispatchFetch()`
requests when a body is set to avoid `RequestContentLengthMismatch`
errors.
...allowing Miniflare to cleanly exit if the cache API is used.
Previously, the URL inside Worker code included the host of the
runtime, not what was passed to `dispatchFetch()`. Similarly, `fetch`
calls to custom service bindings, included the host of the loopback
server. This change passes through the original URLs in both cases,
via a new `MF-Original-URL` header.
@mrbbot mrbbot requested a review from penalosa February 20, 2023 19:22
@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2023

⚠️ No Changeset found

Latest commit: 5104cd7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@mrbbot mrbbot merged commit 7cdc393 into tre Feb 27, 2023
@mrbbot mrbbot deleted the tre-vlovich-fixes branch February 27, 2023 10:26
mrbbot added a commit that referenced this pull request Feb 27, 2023
Similar to #475, we weren't deleting the `MF-Original-Header` URL
from incoming requests, leading to incorrect URLs in the loopback
server when the incoming request was passed-through as the cache key.

Tests also needed to be updated as the storage location is now based
off the actual URL passed to `dispatchFetch()`.
mrbbot added a commit that referenced this pull request Feb 27, 2023
Similar to #475, we weren't deleting the `MF-Original-URL` header
from incoming requests, leading to incorrect URLs in the loopback
server when the incoming request was passed-through as the cache key.

Tests also needed to be updated as the storage location is now based
off the actual URL passed to `dispatchFetch()`.
mrbbot added a commit that referenced this pull request Feb 27, 2023
Similar to #475, we weren't deleting the `MF-Original-URL` header
from incoming requests, leading to incorrect URLs in the loopback
server when the incoming request was passed-through as the cache key.

Tests also needed to be updated as the storage location is now based
off the actual URL passed to `dispatchFetch()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants