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

Can't remove default headers. Can't overwrite "sec-fetch-mode", "connection" headers. #1305

Closed
AlttiRi opened this issue Mar 28, 2022 · 27 comments · Fixed by #1829
Closed

Can't remove default headers. Can't overwrite "sec-fetch-mode", "connection" headers. #1305

AlttiRi opened this issue Mar 28, 2022 · 27 comments · Fixed by #1829
Labels
bug Something isn't working enhancement New feature or request

Comments

@AlttiRi
Copy link

AlttiRi commented Mar 28, 2022

  • I can't remove any default header.
  • Setting "connection": "keep-alive" throws an error.
  • I can't overwrite default "sec-fetch-mode" header correctly.

1. I can't remove the default headers

I can't remove the default headers: host, connection, accept, accept-encoding, accept-language, sec-fetch-mode, user-agent.

For example, curl allows to remove any header (even host header):
curl http://127.0.0.1:3000 -H 'User-Agent:' -H 'Accept:' -H 'Host:'
And yes, it works.

Using of "" will send "" as the header's value, using of null will send "null" as the header's value.

2. "connection": "keep-alive"

Also, if I manually set connection: "keep-alive" I get the error (code: 'UND_ERR_INVALID_ARG').

const resp = await fetch(url, {
    headers: {
        "connection": "keep-alive"
    }
});

It's important for code compatibility reason — since node-fetch uses connection: close by default.

3. "sec-fetch-mode"

It uses "sec-fetch-mode": "cors" by default.

As I said above, I can't remove this header (as well as any other default header).

More over, I can't change it correctly.

For example, using of:

await fetch(url, {
    headers: {
        "sec-fetch-mode": "navigate"
    }
});

Will produce a request with sec-fetch-mode: navigate, cors, while it should be sec-fetch-mode: navigate.


"undici": "4.16.0"

@AlttiRi AlttiRi added the bug Something isn't working label Mar 28, 2022
@ronag
Copy link
Member

ronag commented Mar 28, 2022

Can you check what Chrome does? I think most of the behavior you are seeing is regulated by the spec.

@AlttiRi
Copy link
Author

AlttiRi commented Mar 28, 2022

2 and 3 are obviously bugs.

For 1, for me:

  • Either add a way to remove the default headers, for example, by setting its value to null (not "null"). It's the best choose especially if it will be a part of Node.js. (UPD: However it will not be compatible with native fetch, so no.)
  • Or at least remove sec-fetch-mode from the default headers (and maybe accept-language too).
    (For me it would be enough. Why you have even added it to default?)

Can you check what Chrome does? I think most of the behavior you are seeing is regulated by the spec.

What does Chrome have to do with it? Chrome, as a browser, also follows CORS limitations, for example.
But even Chrome:

  • Does not throws errors if there is "connection": "keep-alive" in headers of fetch.
  • Correctly set any custom sec-fetch-mode header value (with Web Extension API).

HTTP specification does not say that these headers are mandatory.
But undici forces to use them without any way not to do.


So, removing sec-fetch-mode from default headers is most simple and the correct thing.

If someone want to emulate a browser's behaviour he will add both sec-fetch-mode and sec-fetch-dest, sec-fetch-site: none, sec-fetch-user headers. As well as sec-ch-* headers.

Why a tool that going to be a part of Node.js have too much own vision of which headers are mandatory to use?

@ronag
Copy link
Member

ronag commented Mar 28, 2022

2 and 3 are obviously bugs.

For 1, for me:

  • Either add a way to remove the default headers, for example, by setting its value to null (not "null"). It's the best > choose especially if it will be a part of Node.js. (UPD: However it will not be compatible with native fetch, so no.)
  • Or at least remove sec-fetch-mode from the default headers

https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

Step 13

Append the Fetch metadata headers for httpRequest. [FETCH-METADATA]

https://w3c.github.io/webappsec-fetch-metadata/#sec-fetch-mode-header

(and maybe accept-language too).

https://fetch.spec.whatwg.org/#fetching

Step 13:

If request’s header list does not contain Accept-Language, then user agents should append (`Accept-Language, an appropriate header value) to request’s header list.


What does Chrome have to do with it? Chrome, as a browser, also follows CORS limitations, for example.

We try to match the behaviour of Chrome. Basically we use it as our reference implementation.

  • Does not throws errors if there is "connection": "keep-alive" in headers of fetch.

That is something we might want to revisit. It just don't make much sense to provide that header. Why do you do that? Depending on the settings of the undici client it might or might not be correct and might need to be stripped anyway if keep alive is disabled on the connection.

  • Correctly set any custom sec-fetch-mode header value (with Web Extension API).

I'm not sure why you think you can't do that today?

HTTP specification does not say that these headers are mandatory.
But undici forces to use them without any way not to do.

I don't quite agree with this. You can disable the keep-alive header by disabling keep-alive and the other headers are mandated by the spec, as for as I know at least.

@ronag
Copy link
Member

ronag commented Mar 28, 2022

Sorry I accidentally edited your response instead of quoting it. Sorry. Tried to restore it.

@AlttiRi
Copy link
Author

AlttiRi commented Mar 28, 2022

So, what about "sec-fetch-mode"?

It's a pure browser's thing, like CORS.
Why Node.js aimed undici uses it by default (and there is no way to disable it)?


Why do you do that?

I have already written it:

It's important for code compatibility reason — since node-fetch uses connection: close by default.

And because it's a pure bug — I set the correct, valid HTTP header (even it used by default), but undici fails. Also it works without errors in the browser and in any other HTTP client.


I'm not sure why you think you can't do that today?

Because of the bug №3:

await fetch(url, {
   headers: {
       "sec-fetch-mode": "navigate"
   }
});

Will produce a request with sec-fetch-mode: navigate, cors, while it should be sec-fetch-mode: navigate.

@AlttiRi
Copy link
Author

AlttiRi commented Mar 28, 2022

You can disable the keep-alive header by disabling keep-alive and the other headers

Wait.
Could you show me the example?

@ronag
Copy link
Member

ronag commented Mar 28, 2022

You can disable the keep-alive header by disabling keep-alive and the other headers

Wait. Could you show me the example?

setGlobalDispatcher(new Agent({ pipelining: 0 }))

@ronag
Copy link
Member

ronag commented Mar 28, 2022

Regarding the "sec-fetch-mode": "navigate"... as far as I can tell the spec doesn't allow it and neither does Chrome. We could bypass the spec to make more sense in a node environment but I'm not familiar enough with these things to have confidence in making a good decision. Maybe @annevk can provide some guidance?

@ronag
Copy link
Member

ronag commented Mar 28, 2022

Actually we were doing it slight wrong, we should set, not append the header. 5b9acf2

@ronag
Copy link
Member

ronag commented Mar 28, 2022

One way to do what you want would be:

await fetch(url, {
   mode: 'navigate'
});

But again... the spec explicitly disallows it. But maybe it would make sense to bypass that? I don't know.

@AlttiRi
Copy link
Author

AlttiRi commented Mar 28, 2022

setGlobalDispatcher(new Agent({ pipelining: 0 }))

It just changes connection: keep-alive to connection: close.

It does not remove any default header.


neither does Chrome.

As I already said, Chrome allows to do it with web extension.

Why it's not possible to do from a web page context? Because it's a browser. Web browsers have a lot of limitations.

Browsers do not allow:

  • CORS,
  • CORB,
  • changing/removing "sec-fetch-*", <--
  • reading "cookies" HTTP header
  • and other things.

undici is not a browser. Don't overengineer it.

@ronag
Copy link
Member

ronag commented Mar 28, 2022

It does not remove any default header.

We need to set one of the headers. I'm not sure what you want here? #1307 PR welcome.

As I already said, Chrome allows to do it with web extension.

How? I'm curious. Can you show an example. I'm not familiar with web extension.

EDIT: Ah, yea doing it with web extensions is just a hole to get around the spec.

@ronag
Copy link
Member

ronag commented Mar 28, 2022

undici is not a browser. Don't overengineer it.

Easier said than done. Knowing what parts of the spec to ignore or not is not that easy. To keep things simple with the limited development time we have (I'm doing this work for free) it's much easier to just follow the spec verbatim, i.e. the aim here is to match the browser as closely as possible and not bike-shed over what is right or wrong; the spec/Chrome is right (with few well discussed exceptions).

undici is not a browser. Don't overengineer it.

fetch is a browser API. If you don't want/need the browser behavior then don't use the fetch API. We have much better API's for node, e.g. undici.request.

@AlttiRi
Copy link
Author

AlttiRi commented Mar 28, 2022

ModHeader allows to edit and remove entirely any header (except "host", "connection", "cache-control" (on a page reload), "pragma" (on "Disable cache" with DevTools)).


fetch is a browser API.

So, I assume the next things will be implementing of CORS, CORB limitations, restricting of the access to "cookie" header since it's a browser API? [sarcasm]


I want to use Fetch API because it's well know and pretty convenient API.

Since it's a Node.js library I expect that it will have no limitations/additional headers which browsers applies due to security reason. In particular, no sec-fetch-* headers.


Or just add a way to remove the default headers.

With some option for setGlobalDispatcher, for example.


curl http://example.com/ -H 'User-Agent:' -H 'Accept:' -H 'Host:' no one header, but it works (However, with 400 - Bad Request error)

curl https://example.com/ -H 'User-Agent:' -H 'Accept:' only host header and it works with no error.

@ronag
Copy link
Member

ronag commented Mar 28, 2022

ModHeader allows to edit and remove entirely any header (except "host", "connection", "cache-control" (on a page reload), "pragma" (on "Disable cache" with DevTools)).

That's kind of non-standard. I wouldn't use that as a point of reference.

I want to use Fetch API because it's well know and pretty convenient API.

Then you kind of have to live with its limitations. You are kind of asking for fetch but not quite fetch.

Or just add a way to remove the default headers.

Feel free to open PR with a suggestion on how that would look/work.

@benjamingr
Copy link
Member

So, I assume the next things will be implementing of CORS, CORB limitations, restricting of the access to "cookie" header since it's a browser API? [sarcasm]

You know, there are people out there asking Node to do all these things (like CORS support) and some server runtimes like Deno with the concept of an origin are in fact implementing some of this stuff.

So this is not as out of the question as you may assume.


Also, please avoid sarcasm/snark in the tracker. We don't know you well (yet) and it's super easy to take things the wrong way in these situations without a lot of prior context (which we don't have yet!) and it's important to me that your contribution (feedback) on what node does here doesn't get overlooked because of that.


That said I tend to agree node's fetch should allow overriding these things unlike the browser's. I also absolutely agree with Robert it's not clear cut. I can ask in a forum of server runtimes I participate in but if you want to move this ahead it'd be great to do what server runtimes that implement fetch do (Deno/cloudflare/shopify etc) as well as what user-land implementations (like node-fetch) do.

@benjamingr benjamingr added the enhancement New feature or request label Mar 28, 2022
@AlttiRi
Copy link
Author

AlttiRi commented Mar 28, 2022

Yeah, I also would like if Node.js have some permission list in package.json like it is in Web Extensions, Android Applications:
My comment in the topic about the recent RIAEvangelist/node-ipc case.


While sec-fetch-* contains "fetch" word, it's not a part of Fetch API.
sec-fetch-* headers were added in browsers much later than Fetch API.
The modern browser send sec-fetch-* headers with any request, not only with XHR made with fetch function.

Also, why it sends sec-fetch-mode: cors?

cors

What if I do a request to the server from the same process where I host that HTTP server? Is it still "cors"?
What even CORS means in a Node.js application?
What about other sec-fetch-* headers? The browsers currently send 3-4 "sec-fetch" headers with every request.


I see default adding sec-fetch-mode: cors header is meaningless, and even annoying if I want to simulate a fetch request from a browser before 2019.08 (Chrome 76).

If any one want to simulate a request from the modern browser (with absolutely the same headers, and the same header's order), he can (and should) add all required headers by yourself.


About manual removing the other default headers:

how that would look

Somehow. It can be a property with the array of string (header names) which should not be added by default, for example.

UPD (2022.04.08):

For example, to export a setDefaultHeaders function. It would pretty convenient.
Note, it would have the global effect. (As well as using setGlobalDispatcher currently)


I think for people Fetch API is about "promises" and "streaming", it's about Request, Response, ReadableStream and Headers objects. But it's not about which headers a browser adds.

@ronag
Copy link
Member

ronag commented Mar 28, 2022

Yeah, I also would like if Node.js have some permission list in package.json like it is in Web Extensions, Android Applications: My comment in the topic about the recent RIAEvangelist/node-ipc case.

While sec-fetch-* contains "fetch" word, it's not a part of Fetch API. sec-fetch-* headers were added in browsers much later than Fetch API. The modern browser send sec-fetch-* headers with any request, not only with XHR made with fetch function.

It’s in the fetch spec.

Also, why it sends sec-fetch-mode: cors?

Because the spec says so.

cors

What if I do a request to the server from the same process where I host that HTTP server? Is it still "cors"? What even CORS means in a Node.js application? What about other sec-fetch-* headers? The browsers currently send 3-4 "sec-fetch" headers with every request.

We haven’t fully implemented the spec yet so the other headers are missing.

I see default adding sec-fetch-mode: cors header is meaningless, and even annoying if I want to simulate a fetch request from a browser before 2019.08 (Chrome 76).

We only target latest version of spec. Old browsers are out of scope.

If any one want to simulate a request from the modern browser (with absolutely the same headers, and the same header's order), he can (and should) add all required headers by yourself.

About manual removing the other default headers:

how that would look

Somehow. It can be a property with the array of string (header names) which should not be added by default, for example.

PR welcome

I think for people Fetch API is about "promises" and "streaming", it's about Request, Response, ReadableStream and Headers objects. But it's not about which headers a browser adds.

You a maybe right but that is currently not the priority here. We have chosen to focus on fully implementing the spec. I don’t know enough about Cors and what not the determine what is relevant or not. If you have any specific, constructive and articulated suggestions we are open to discuss them. Even better open a PR.

@benjamingr
Copy link
Member

Yeah, I also would like if Node.js have some permission list in package.json like it is in Web Extensions, Android Applications:

Those are called policies and already exist in Node.js! if you'd like to work on them (improving them etc) please do! They are mostly blocked on feedback and people actually using them to make progress.

@benjamingr
Copy link
Member

@ronag I think being able to remove the sec-fetch-mode stuff is probably fine? @AlttiRi did you check what other environments like cloudflare workers or deno do?

@AlttiRi
Copy link
Author

AlttiRi commented Mar 29, 2022

I don't know about Cloudflare Workers, but Deno's fetch default headers are:

accept: */*
user-agent: Deno/1.20.3
accept-encoding: gzip, br
host: 127.0.0.1:3000

That's all.
That looks fine. Need additional headers? Add them by yourself.

Also look at the headers order.


While the article about sec-fetch-* names "Fetch Metadata Request Headers", but.
It's not about only Fetch API requests. It's about every request in browsers, made from https origin: navigate (address bar and link clicks), parser (script/image loading), xhr, fetch.

https://fetch.spec.whatwg.org/#http-network-or-cache-fetch is also written in mind to be for use in browsers.


Using in Node.js sec-fetch-* by default without way to disable it for what? It's meaningless.
Node.js applications don't have origin, so it's not possible to say that some request was "cors" (cross-origin), or "same-site".
Node.js applications don't have navigation bar, global location object, it's not possible to say what some request was "navigate".


I like Fetch API (streaming and convenient), but not the browsers limitations (with the corresponding additional "features"). Separate them.

I need a Node.js tool to do network requests with well know and convenient API.
I think, people that asked to add fetch to undici also meant that.
A Fetch API compatible function.

BTW, response.body of undici is "async iterable" (I can use it in for-await-of), while the native one is not. And it's okay.

Mimic browsers requests by adding additional headers (sec-fetch-*, sec-ch-ua-*, upgrade-insecure-requests, dnt) (and reordering of them*) is more work for a plugin/other lib that will modify the headers object the appropriate way.


* Wait, I can't reorder them. It looks that undici just sorts them alphabetically (after host and connection headers). Neither Chrome, not Firefox don't do that. Both browsers use a custom headers order.

In particular, it's the addiction case why I need to set "connection": "keep-alive" manually. To reorder the headers.

@RafaelGSS
Copy link
Member

Just FYI

  • Wait, I can't reorder them. It looks that undici just sorts them alphabetically (after host and connection headers). Neither Chrome, not Firefox don't do that. Both browsers use a custom headers order.

We are going to discuss the sorting in: #1309 (comment)

@issuefiler
Copy link

issuefiler commented May 26, 2022

I wish Node.js’ fetch were freer (“more capable”) than browser’s, while being compatible with browser’s.

This is globalThis.fetch, which has been shipped into Node.js. I want to fully utilize Node.js’ capability using the elegant dependency-less API, fetch.

Nothing fancy is needed; just a few less-restrictive options to customize requests, the spec being the default. So that I don’t need to introduce any unnecessary dependencies (“external packages”) that do the same thing as what Node.js’ fetch does.

@mcollina
Copy link
Member

@issuefiler essentially you want a non-spec compliant fetch(). I think the venue to discuss this is https://github.com/wintercg/fetch (read more about this initiative in https://blog.cloudflare.com/introducing-the-wintercg/).

The goal for fetch() is to be as close to the WHATWG spec as it's possible right now. If you need lower-level APIs use undici.request().

@adam-arthur
Copy link

adam-arthur commented Jul 10, 2022

Enforcing CORS/browser specific rules in a serverside request context is non-sensical. These rules exist in browsers for security reasons to protect the user.

Fine to default to them if the spec requires that, but there definitely needs to be a mechanism to override the default headers. Core constructs should be configurable to enable a wide variety of use cases.

I can simply use http or curl to set whatever headers I want. But why would we handicap the standard lib and require dropping into lower level constructs for this? There is 0 security benefit to having this in fetch for serverside contexts.

Having a node specific mechanism to override default headers does not violate the fetch spec/api or prevent isomorphic fetch code.

@robhogan
Copy link

robhogan commented Nov 1, 2023

Could this be reopened? It was incorrectly closed by #1829, which only addresses connection. sec-fetch-mode still can't be unset or changed, as far as I can tell.

@matt-way
Copy link
Contributor

matt-way commented Nov 29, 2023

I agree that this should be reopened.

I just spent an entire day narrowing in on why my proxy requests were returning 400, but working with node-fetch. Turns out removing the sec-fetch-mode (or changing it to navigate) fixed my issue.

The goal for fetch() is to be as close to the WHATWG spec as it's possible right now. If you need lower-level APIs use undici.request().

Then what's the point? The only reason for using this lib is because its now native.

You can get around this whole thing with something like the following:

import { ProxyAgent } from 'undici'

class CustomProxyAgent extends ProxyAgent {
  constructor(opts) {
    super(opts)
  }

  dispatch(opts, handler) {
    delete opts.headers['sec-fetch-mode']    
    return super.dispatch(opts, handler)
  }
}

export default CustomProxyAgent

Such a weird goal to be 100% spec compliant for a browser fetch.

oplik0 added a commit to oplik0/NodeBB that referenced this issue Jan 22, 2024
julianlam pushed a commit to NodeBB/NodeBB that referenced this issue Jan 22, 2024
facebook-github-bot pushed a commit to facebook/metro that referenced this issue Aug 30, 2024
Summary:
Fixing node 18 fetch error due to attempt to set the "Connection" header which is not supported on that version of node (nodejs/undici#1305).
However "Connection" is set to "close" by default in node 18 anyway comparing with later version so we just don't touch it on that version.

Pull Request resolved: #1341

Test Plan:
`yarn jest packages/metro/src/integration_tests/__tests__/server-test.js`

Tested ad hoc on Node JS versions:
* `18.0.0`
* `18.14.1`
* `18.14.2`
Plus the versions in the github actions

Reviewed By: robhogan

Differential Revision: D62026647

Pulled By: vzaidman

fbshipit-source-id: fe99cfd0a7cdedc0119b1c68c38125fe464b4742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants