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

Behaviour mismatch: Fetch with content-length: 0 fails with undici error #193

Closed
orls opened this issue Feb 28, 2022 · 4 comments · Fixed by #204
Closed

Behaviour mismatch: Fetch with content-length: 0 fails with undici error #193

orls opened this issue Feb 28, 2022 · 4 comments · Fixed by #204
Labels
behaviour mismatch Different behaviour to Workers runtime
Milestone

Comments

@orls
Copy link
Contributor

orls commented Feb 28, 2022

Attempting to fetch with content-length: 0 results in an error from undici. This is tolerated in real Workers env:

Steps to reproduce:

addEventListener("fetch", event => {
  event.respondWith(handleRequest(event.request))
})

async function handleRequest(request) {
  return fetch('https://httpbin.org/delete', {
    method: 'DELETE',
    headers: {
      'Accept': 'application/json',
      'Content-length': '0'
    }
  });
}

expected behaviour:

Request succeeds (worker returns the httpbin 200 response).

Actual behaviour

[mf:err] GET /: TypeError: fetch failed
    at Object.processResponse (/somedir/node_modules/undici/lib/fetch/index.js:176:23)
    at fetchFinale (/somedir/node_modules/undici/lib/fetch/index.js:737:17)
    at Fetch.mainFetch (/somedir/node_modules/undici/lib/fetch/index.js:712:5)
[mf:err] Cause: RequestContentLengthMismatchError: Request body length does not match content-length header
    at write (/somedir/node_modules/undici/lib/client.js:1433:37)
    at _resume (/somedir/node_modules/undici/lib/client.js:1385:29)
    at resume (/somedir/node_modules/undici/lib/client.js:1244:3)
    at connect (/somedir/node_modules/undici/lib/client.js:1229:3)
GET / 500 Internal Server Error (329.42ms)

This originates from undici's strictness; they deliberately treat the req as invalid, it's not a bug from their POV. See undici issue 649 for example. They offer a non-strict mode to turn this and other behaviours into warnings, but I don't think miniflare supports enabling this mode?

This does not match behaviour of CF deployed workers. Moreover, there is code in the wild that explicitly and deliberately adds content-length: 0 to certain requests. For example, popular NPM lib node-http-proxy; I believe Envoy proxy does similar.


(this shares some DNA with recent miniflare issues like #183 and #182: undici's design goals, while reasonable in some contexts, don't always seem to align with CF Workers' server-side-oriented fetch impl)

@mrbbot
Copy link
Contributor

mrbbot commented Mar 4, 2022

Hey! 👋 Thanks for raising this. Should be possible to remove the Content-Length header if it's 0 in fetch:

req.headers[fetchSymbols.kGuard] = "none";
// Delete the "Host" header, the correct value will be added by undici
req.headers.delete("host");
// Delete the "CF-Connecting-IP" header, if we didn't do this, we'd get a 403
// response when attempting to make requests to sites behind Cloudflare
req.headers.delete("cf-connecting-ip");
// Add "MF-Loop" header for loop detection
req.headers.set(_kLoopHeader, String(ctx?.requestDepth ?? 1));

@mrbbot mrbbot added bug Something isn't working behaviour mismatch Different behaviour to Workers runtime and removed bug Something isn't working labels Mar 4, 2022
orls pushed a commit to orls/miniflare that referenced this issue Mar 7, 2022
orls pushed a commit to orls/miniflare that referenced this issue Mar 8, 2022
mrbbot pushed a commit that referenced this issue Mar 8, 2022
Co-authored-by: Owen Smith <osmith@cloudflare.com>
@mrbbot mrbbot added this to the 2.4.0 milestone Mar 8, 2022
@dpeek
Copy link

dpeek commented Mar 23, 2022

Any chance this could be released as a patch version? I ran into the Stripe SDK issue so I'm using a yarn link version... looks like 2.4.0 is a ways off 😅

@CraigglesO
Copy link
Contributor

I second @dpeek , I've been running a personal duplicate of Stripe that removes the header if 0 for now, but would be nice to be able to keep up with Stripe updates.

@mrbbot
Copy link
Contributor

mrbbot commented Apr 2, 2022

Hey! 👋 I've just released version 2.4.0 including the fix for this. You can find the full changelog here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviour mismatch Different behaviour to Workers runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants