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

Fetch failed when setting host: header correctly #2318

Closed
infinity0 opened this issue Oct 9, 2023 · 6 comments · Fixed by #2322
Closed

Fetch failed when setting host: header correctly #2318

infinity0 opened this issue Oct 9, 2023 · 6 comments · Fixed by #2322
Labels
bug Something isn't working

Comments

@infinity0
Copy link

Reproducible By

$ node
Welcome to Node.js v18.13.0.
Type ".help" for more information.
> const { fetch } = require('undici');
undefined
> await fetch("https://bing.com/translator", { headers: { host: 'bing.com' } });
Uncaught TypeError: fetch failed
    at fetch ($PWD/node_modules/undici/index.js:110:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async REPL2:1:33 {
  cause: Error: Client network socket disconnected before secure TLS connection was established
      at connResetException (node:internal/errors:718:14)
      at TLSSocket.onConnectEnd (node:_tls_wrap:1600:19)
      at TLSSocket.emit (node:events:525:35)
      at TLSSocket.emit (node:domain:552:15)
      at endReadableNT (node:internal/streams/readable:1359:12)
      at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
    code: 'ECONNRESET',
    path: undefined,
    host: 'www.bing.com',
    port: 443,
    localAddress: null
  }
}
> await fetch("https://bing.com/translator");
Response {
  [Symbol(realm)]: null,
[ .. all good without explicit host: .. ]

The bug happens with bing.com because it tries to redirect to www.bing.com; it doesn't happen for domains that don't redirect, such as fanyi.baidu.com.

Environment

software version
undici v5.25.4
node v18.13.0
Operating System Debian GNU/Linux trixie/sid
@infinity0 infinity0 added the bug Something isn't working label Oct 9, 2023
@KhafraDev
Copy link
Member

Setting the host header to www.bing.com works. I don't think this is an undici bug.

@infinity0
Copy link
Author

There's no way for the caller to predict www.bing.com since it's a dynamic redirect chosen by bing.com.

@infinity0
Copy link
Author

I don't think this is an undici bug.

It's an undici bug insofar as you allow setting forbidden headers. It works everywhere else (in the browser, on Cloudflare Workers) because the caller-supplied host is ignored as a forbidden header and the host: is filled in dynamically for each redirect.

@infinity0
Copy link
Author

Like #1462 this comes up when using undici fetch to implement a web proxy. The easiest way to do this is to copy/reuse the incoming request headers to/as the outgoing request headers.

With browser fetch and Cloudflare Workers fetch this Just Works. With undici (and node-fetch) you have to worry about explicitly deleting the Host header, wasting your time debugging the error. (Additionally because of #1462 it's impossible to pass-through compressed content, whereas with node-fetch you can at least pass compress: false.)

@KhafraDev
Copy link
Member

KhafraDev commented Oct 9, 2023

So to clear some things up, neither Deno or CFW implement forbidden headers, they simply disallow setting only the host header (edit: not really, but to simplify my explanation). Undici is going against RFC 9110 in this case: "A user agent MUST generate a Host header field in a request". I will have a fix for this shortly.

@paulrutter
Copy link

paulrutter commented Oct 20, 2023

@mcollina @KhafraDev I think there are valid usecases for setting the host header on a request in Node.js environment.
For example, if you have a multi-tenant application with specific hostnames, you want to able to supply the IP address + host header, not just the hostname in the url.

We are running into this issue when upgrading from Node 18.17.1 to Node 20.8.1, as the host header doesn't seem to be passed along to the application anymore (thus, breaking the integration).

Is this really the desired behavior?
It's not a backwards compatible change, and i think the RFC should not be followed when not in a browser environment.
I also don't read in the RFC that the host should not be overridable, just that the agent should generate one (if not available via the custom headers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants