Add httpOptions option

[c-kruse]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• New feature
• Other, please explain:

What changes did you make? (provide an overview)
Added extraHTTPOptions option that gets passed through to the http(s).request options here https://nodejs.org/docs/latest-v12.x/api/http.html#http_http_request_options_callback.

It looks like there was recently a similar discussion around this for TLS Options, which can be configured on the agent (undocumented). The impetus for this PR is the ability to pass the insecureHTTPParser option, which as far as I can tell can't be set on the agent.

Which issue (if any) does this pull request address?
#724

Is there anything you'd like reviewers to know?

Here's some background reading: nodejs/node#27711
Sounds like it's a fairly common issue with the Incapsula CDN.

Why not just use the Agent?

const https = require('https');
const fetch = require('node-fetch');
async function main() {
    const agent = new https.Agent({
        insecureHTTPParser: true // not a valid option in https://nodejs.org/api/http.html#http_new_agent_options
    });
    const uri = 'https://cams.jacksonhole.com/webcam/SouthHoback.jpg?1590589817';
    const data = await fetch(uri, {
        agent
    });
    console.log(data)
}

main().catch(console.error);

Running this on node 12.x LTS will get you this:

➜  node test.js
FetchError: request to https://www.dezeen.com/robots.txt failed, reason: Parse Error: Invalid header value char
    at ClientRequest.<anonymous> (/path/to/project/node_modules/node-fetch/lib/index.js:1455:11)
    at ClientRequest.emit (events.js:315:20)
    at TLSSocket.socketOnData (_http_client.js:476:9)
    at TLSSocket.emit (events.js:315:20)
    at addChunk (_stream_readable.js:295:12)
    at readableAddChunk (_stream_readable.js:271:9)
    at TLSSocket.Readable.push (_stream_readable.js:212:10)
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:186:23) {
  type: 'system',
  errno: 'HPE_INVALID_HEADER_TOKEN',
  code: 'HPE_INVALID_HEADER_TOKEN'
}

We need the insecureHTTPParser option on the request to override this on a per-call basis. https://nodejs.org/api/http.html#http_http_request_options_callback

[Richienb]

I think we can just call the parameter httpOptions.

[c-kruse]

I think we can just call the parameter httpOptions.

Agreed!

[jimmywarting]

fix indentation, replace tab for space to be consistent

[jimmywarting]

would have been nice with at least some test

[jimmywarting]

wouldn't the new recommended way to pass a Agent be thought the new httpOptions? now we essentially got two ways of adding Agent, but i guess that could be fine too...

[c-kruse]

@jimmywarting I’ll fix the whitespace and mull over a way to test this - it’s not obvious to me how I could do that without stubbing out the http package. Any ideas?

As far as the agent goes, I’m conflicted. The other idea I’ve entertained is only adding the insecureHTTPParser Parameter - I didn’t see anyone looking for any of the other options here https://nodejs.org/docs/latest-v12.x/api/http.html#http_http_request_options_callback

[bitinn]

Please demostrate what's not possible via Agent?

[bitinn]

ok looks like nodejs did not make it easy for us.

nodejs/node@0082f62d9c

My preference is still not to introduce an open httpOptions that just pass everything in http.request (as most options should be done via Agent), but rather to handle this specific case via a custom insecureHTTPParser option.

But if other team members are into httpOptions, which is essentially everything is possible but you are fully responsible for breaking fetch, then I rest my case.

[c-kruse]

@bitinn sounds like you beat me to the punch, but updated the PR description with an example of what the agent cannot do. (Thanks @roychri for writing that up in #724)

I'll wait for other maintainers to chime in.

[tinovyatkin]

How insecure HTTP parser (and wider allowance in HTTP headers) will then work with our Headers that will do validation too?

[c-kruse]

How insecure HTTP parser (and wider allowance in HTTP headers) will then work with our Headers that will do validation too?

Thanks for pointing that out @tinovyatkin - it is filtering (silently dropping) invalid headers here:

node-fetch/src/headers.js

Line 250 in e6bfe4d

.filter(([name, value]) => !(invalidTokenRegex.test(name) || invalidHeaderCharRegex.test(value)))

I think that is probably acceptable behavior.

[tinovyatkin]

But, I mean, what is the point if you will not be able to access those headers anyway?
The only difference of insecure parser is headers: Use an insecure HTTP parser that accepts invalid HTTP headers when true, but node-fetch Headers will not expose them anyway... So, why this feature should be inside node-fetch?

[tinovyatkin]

Should setHost option be handled specially in this case?

[jimmywarting]

it’s not obvious to me how I could do that without stubbing out the http package. Any ideas?

@c-kruse in https://github.com/node-fetch/node-fetch/blob/master/test/utils/server.js you got a test server, you could perhaps create a own response with invalid headers, if node don't allow you to respond with inaccurate headers then maybe you could test other httpOptions in other cases it would also be possible to create a server using net and craft a really low level http server/response. then in https://github.com/node-fetch/node-fetch/blob/master/test/request.js you would have to do the request to the local server.

But, I mean, what is the point if you will not be able to access those headers anyway?
The only difference of insecure parser is headers: Use an insecure HTTP parser that accepts invalid HTTP headers when true, but node-fetch Headers will not expose them anyway... So, why this feature should be inside node-fetch?

maybe so you at least got a graceful degration instead of flat out get rejected with an error of why you where not able to get a response?

[jimmywarting]

would a annoying, nagging console.warn be useful? saying something like "hey, example.com/hello-world responded with invalid header token 'x-foobar' you should report this to example.com" 😆

[c-kruse]

what is the point if you will not be able to access those headers anyway?

To provide browser-like ability to ignore junk http headers in the node environment for anyone who wishes to opt-in. Today it raises the HPE_INVALID_HEADER_TOKEN error. If you force your browser to use http/1.1 you can catch it dropping invalid http headers from most sites running behind an Incapsula CDN.

why this feature should be inside node-fetch?

I could be convinced either way. It seems like a relatively small number of us who are blessed with this problem. Selfishly, I don't want to maintain a fork of node-fetch or refactor the handful of projects we have using it to another client.

[tinovyatkin]

@jimmywarting
If it's browser-like behavior, maybe we should just enable insecureHttpParser for all requests by default? node-fetch filters them anyway and we will be on par with browser fetch...

[jimmywarting]

maybe we should just enable insecureHttpParser for all requests by default

nah, isn't the world going to a bit better if everyone just instead fix there bugs? i don't expect that the insecure http parser will be around for much long, insecure is... well, insecure

[c-kruse]

Thanks everyone for the direction! Closing this for #856 as I think we're in agreement on the suggestion @bitinn made.