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

Sending a FormData body and manually providing a content-type header #2736

Open
KhafraDev opened this issue Feb 12, 2024 · 4 comments
Open

Comments

@KhafraDev
Copy link
Member

Should undici throw an error when sending a FormData body, either via request or fetch, when providing a content-type header?

The following will "work", but will cause the server to be unable to parse the formdata:

    const data = new FormData();
    data.append("url", url);
    data.append("capture_all", "on");

    const res = await fetch(`${process.env.WEB_ARCHIVE_BASE!}save/${url}`, {
        method: "POST",
        headers: {
            "Content-Type": "multipart/form-data",
        },
        body: data,
    })

note: this is an actual sample of code someone asked me to help them fix, as the endpoint was giving them a 400 status code (but working with curl)

should i be able to just throw a FormData instance at the undici fetch under the body key with a Content-Type: multipart/form-data header in node 21.2.0?
the result i get is not what i'd expect and my current guess is that the form data is wrong :firHmm:

the curl data to emulate would be

curl --request POST \
  --url https://web.archive.org/save/<snip> \
  --header 'Content-Type: multipart/form-data' \
  --form url=<snip> \
  --form capture_all=on

There are two different issues here:

  • fetch doesn't implement forbidden headers, allowing a content-type to be provided.
  • request will only append a content-type header if one does not exist.

The solutions are:

  • throw an error, which is the easiest & would allow people to fix their code, rather than covering it up
  • override the custom content-type header, which curl seems to do?

IMO when sending a FormData body there should never be an expectation of being able to send any other content-type, as without the undici-generated header, it literally can't be parsed by the server. I'd prefer throwing an error because there shouldn't be an expectation that these issues will get fixed by undici.

@metcoder95
Copy link
Member

Agree with the statement that these scenarios should be handled by the users rather than undici or fetch doing its best guest to cover that up.

However, I'm a little over the fence if this is something that should be changed within undici or maybe a documentation improvement to be done. Especially as it feels pretty unique to multipart/form-data (at least for now).

Should we consider adding a documentation improvement and a possible warning before considering it to throw?
As said earlier, the only thing that it worries me is that this is becomes pretty specific to multipart/form-data only

@mcollina
Copy link
Member

I would keep supporting it in request() and override it in fetch().

@KhafraDev
Copy link
Member Author

Especially as it feels pretty unique to multipart/form-data (at least for now).

It is unique to multipart/form-data, as without the boundary type provided in the content-type header, it can't be parsed.
https://datatracker.ietf.org/doc/html/rfc7578

"boundary" is now a required parameter in
Content-Type.

I would keep supporting it in request() and override it in fetch().

Here's where we'd implement the logic. I wonder if we should take a look at reimplementing a subset of forbidden headers as we've had nothing but issues (host header) and security vulnerabilities (cookie, authorization, proxy-authorization) by not implementing them. Originally I wanted to follow Deno's behavior, but I'm second guessing my judgement... I know ronag's disapproved removing them too, way back when.

https://github.com/nodejs/undici/blob/ae9587193403bccac4fdfe9ebc882c5240538cf3/lib/fetch/request.js#L507-509

@mcollina
Copy link
Member

IMHO those are needed for compat with the rest of the Node.js ecosystem.

SGTM on the implementation of in request.

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

No branches or pull requests

3 participants