🐛 Bug Report — Runtime APIs: workerd asserts on GET/HEAD request with 'Content-Length: 0'

[janjongboom]

GET/HEAD requests that include a Content-Length: 0 header assert when running CF workers locally in miniflare (ran through wrangler 3.0.0). E.g. take any worker, and make a GET request to it with this header:

$ curl -H 'Content-Length: 0' http://localhost:8001/

Yields:

workerd/server/server.c++:2528: error: Uncaught exception: workerd/jsg/_virtual_includes/jsg/workerd/jsg/value.h:1343: failed: remote.jsg.TypeError: Request with a GET or HEAD method cannot have a body.
stack: /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@19e138c /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@19e1ff7 /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@18b4880 /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@18b4de4 /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@18b5c14 /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@18b6198 /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@1faf7b4 /app/dashboard/node_modules/workerd/lib/downloaded-@cloudflare-workerd-linux-arm64-workerd@1fb2284

Doing the same request against the CF workers playground is fine (omitted some headers for clarity here):

$ curl 'https://29cb9a0e78771a2831a65e4f7065da77.cloudflareworkers.com/' -H 'Content-Length: 0'
Hello world

My guess is this pointer should not be set when Content-Length: 0 is passed in (or there should be another check for headers here):

workerd/src/workerd/api/http.c++

Line 854 in d28b0e1

KJ_IF_MAYBE(b, kj::mv(initDict.body).orDefault(nullptr)) {

Setting the Content-Length header on GET requests is f.e. used by Docker whenever it pulls manifests from Docker Hub (found this behavior when proxying these requests through a worker).

[kentonv]

The error in question is thrown by the Request constructor (or fetch()) when called from JavaScript and given an initializer object which specifies method: "GET" and a non-null body. The system does not call this implicitly; it must be invoked from JavaScript.

@mrbbot does Miniflare inject an ingress worker that handles requests before passing them on to the user's worker? If so I think this may be a bug in that ingress worker.

[kentonv]

For a little background here: The fetch() and Request API spec say that GET requests cannot have a body. But, the HTTP spec actually does allow GET requests to have a body, and people do use this. Specifying Content-Length or Transfer-Encoding headers in a GET request causes it to have a body (even if zero-size).

The Workers Runtime, as a compromise, permits bodied GET requests to proxy through, but does not allow a bodied GET to be constructed directly from JavaScript. This pass-through works even if you do new Request(newUrl, oldRequest) to rewrite the URL of the request -- in this case, theoretically, oldRequest is being used as the options object to the Request constructor, which "just works" because all the defined options that this structure can have correspond to same-named properties of Request. Theoretically, though, this should fail in the case of a bodied GET because it's specifying method: "GET" and non-null body. However, the runtime explicitly notices when you are using a full Request object as the second parameter and treats this specially, so that the bodied GET can pass through after all.

It looks like miniflare might be deconstructing a Request object somewhere into its component properties, and then constructing a new initializer dict from that. This defeats the special handling of Request objects used as an options object, so the bodied GET is not permitted through. Ideally miniflare's code should be adjusted to avoid decomposing the Request object in this way. Note that individual properties can be modified if needed by doing new Request(oldRequest, {...options...}) -- the options will update specific properties of the request while other properties are copied over from oldRequest.

[mrbbot]

Hey! 👋 Yep, Miniflare has an ingress worker that does exactly that:

request = new Request(url, {
  method: request.method,
  headers: request.headers,
  cf: request.cf ?? env[CoreBindings.JSON_CF_BLOB],
  redirect: request.redirect,
  body: request.body,
});

(https://github.com/cloudflare/miniflare/blob/6ec2eaade4014bff3aa7e3df0e98034b650fdc46/packages/miniflare/src/workers/core/entry.worker.ts#L49-L55)

The reason we decompose the request like this is so we can rewrite the URL, and add the fallback cf object in the same call to new Request(). I guess we'll need to have 2 new Request()s now: one to rewrite the URL, and one for cf:

request = new Request(url, request);
if (request.cf === undefined) {
  request = new Request(request, { cf: env[CoreBindings.JSON_CF_BLOB] });
}

Something sneaky with Object.defineProperty(request, "cf", { get() { ... }} before this doesn't seem to work. 😅

Anyways, put a fix up here cloudflare/miniflare#677. Thanks for the explanation. 👍

[kentonv]

Yep, invoking the Request constructor twice is the right solution here, as silly as it may appear.

[janjongboom]

Thanks for the quick response all, and great to see the quick patch upstream!

For anyone running into this before the next miniflare release (I see the patch hasn't landed in master yet): I've put a Node proxy in front of my local workers that stripped off the header to workaround this:

import http from 'http';

const PROXY_HOST = process.env.PROXY_HOST || 'localhost';
const PROXY_PORT = process.env.PROXY_PORT || '4502';
const PORT = process.env.PORT || '4501';

http.createServer((clientReq, clientRes) => {
    let strippedContentLength = false;
    if (clientReq.method === 'GET') {
        if (clientReq.headers['content-length']) {
            delete clientReq.headers['content-length'];
            strippedContentLength = true;
        }
    }

    if (strippedContentLength) {
        console.log('serving ' + clientReq.method + ' ' + clientReq.url + ' (stripped Content-Length header)');
    } else {
        console.log('serving ' + clientReq.method + ' ' + clientReq.url);
    }

    const options = {
        hostname: PROXY_HOST,
        port: Number(PROXY_PORT),
        path: clientReq.url,
        method: clientReq.method,
        headers: clientReq.headers
    };

    const proxy = http.request(options, (res) => {
        clientRes.writeHead(res.statusCode || 500, res.headers);
        res.pipe(clientRes, {
            end: true
        });
    });

    clientReq.pipe(proxy, {
        end: true
    });

    proxy.on('error', err => {
        console.log('error proxying ' + clientReq.method + ' ' + clientReq.url + ': ' + err);
        clientRes.writeHead(500, { 'Content-Type': 'text/plain' });
        clientRes.write('error proxying ' + clientReq.method + ' ' + clientReq.url + ': ' + err + '\n');
        clientRes.end();
    });
}).listen(Number(PORT), '0.0.0.0');

console.log(`Proxying 0.0.0.0:${PORT} -> ${PROXY_HOST}:${PROXY_PORT}`);