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

Extend options param to accept options that can be forwarded to http/https request #724

Closed
jamiechong opened this issue Feb 11, 2020 · 18 comments

Comments

@jamiechong
Copy link

It would be great if the current options accepted another value that could forward options to the internal http.request call. Specifically I'm looking to use the insecureHTTPParser param to work around this issue.

So something like:

fetch(url, { extraHttpOptions: { insecureHTTPParser: true } });
@xxczaki
Copy link
Member

xxczaki commented Feb 20, 2020

@bitinn @Richienb What do you think? Should we allow users to pass options to the internal http.request call?

@willhoag
Copy link

willhoag commented Mar 9, 2020

I also have this need, trying to solve the same problem:
nodejs/node#27711

@jamiechong were you able to solve this (request specific insecure parsing) any another way?

@mehmetcc
Copy link

I also have this need, trying to solve the same problem:
nodejs/node#27711

@jamiechong were you able to solve this (request specific insecure parsing) any another way?

This is still a valid problem you guys

@NotMoni
Copy link
Member

NotMoni commented May 3, 2020

We should have the option to do insecure https.

@NotMoni
Copy link
Member

NotMoni commented May 3, 2020

@jamiechong Have you tried, this?

const https = require('https');
const fetch = require('node-fetch');

const agent = new https.Agent({
     rejectUnauthorized: false,
});

const uri = 'YOUR_URL';

const data = await fetch(uri, { agent })

@jamiechong
Copy link
Author

jamiechong commented May 3, 2020

@NotMoni
I don’t believe I have but that looks promising. It’s been quite a while since I’ve tried to resolve this (no longer at the organization where this problem exists). But for my own interest I’ll try to test and see if it handles the problem we had. Thanks!!

@NotMoni
Copy link
Member

NotMoni commented May 3, 2020

Np, let me know if it solves the problem 🦄

@roychri
Copy link

roychri commented May 27, 2020

Unfortunately, it does not help.

Here's a simple way to reproduce the problem...

Install latest node 12.x version

Create a file called test.js with this content:

const https = require('https');
const fetch = require('node-fetch');
async function main() {
    const agent = new https.Agent({
        rejectUnauthorized: false
    });
    const uri = 'https://cams.jacksonhole.com/webcam/SouthHoback.jpg?1590589817';
    const data = await fetch(uri, {
        agent,
	rejectUnauthorized: false // I do not think that's where it goes but just in case...
    });
}
main().catch(console.error);

Install node-fetch using npm install node-fetch

Running this with node test it generate this error:

FetchError: request to https://cams.jacksonhole.com/webcam/SouthHoback.jpg?1590589817 failed, reason: Parse Error: Invalid header value char
    at ClientRequest.<anonymous> (/home/christian/dev/yourtrainer/24go-server/node_modules/node-fetch/lib/index.js:1444:11)
    at ClientRequest.emit (events.js:311:20)
    at TLSSocket.socketOnData (_http_client.js:483:9)
    at TLSSocket.emit (events.js:311:20)
    at addChunk (_stream_readable.js:294:12)
    at readableAddChunk (_stream_readable.js:275:11)
    at TLSSocket.Readable.push (_stream_readable.js:209:10)
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:186:23) {
  message: 'request to https://cams.jacksonhole.com/webcam/SouthHoback.jpg?1590589817 failed, reason: Parse Error: Invalid header value char',
  type: 'system',
  errno: 'HPE_INVALID_HEADER_TOKEN',
  code: 'HPE_INVALID_HEADER_TOKEN'
}

However when I run it like this:

node --insecure-http-parser test

There's no errors.

If you run it with node v8.x or v10.x it generate no errors because it uses the insecure http parser by default.

If node-fetch allows us to pass any options to http/https requests, then this would work without having to pass --insecure-http-parser to node... It would allow node-fetch to support any future http/https options which would be good.

Thanks.

@NotMoni
Copy link
Member

NotMoni commented May 27, 2020

@roychri 2 Things.

1.) Why are you passing in rejectUnauthorized: false as a new Agent, then passing it in again?

2.) Related to #809

@roychri
Copy link

roychri commented May 27, 2020

@NotMoni Because in your previous message asking to try something (#724 (comment)) it was not clear where rejectUnauthorized should go and the code you provided had a couple of syntax error so it was clear you just wrote something quickly without checking. And the documentation suggest it should be passed to the agent but your code suggest it might be as part of the parameters to fetch().
I was not sure where you wanted us to put that to test, so I put it in two places with a comment explaining why its there twice.

@NotMoni
Copy link
Member

NotMoni commented May 27, 2020

@roychri Alright Bud. I wrote that on Github Mobile and, yes it was quick and I didn't check. I'm sorry about that. Maybe you could have notified me that the code was incorrect so I could have fixed it.

Now, the (#724 (comment)) has been updated. Take a look 🦄

@roychri
Copy link

roychri commented May 27, 2020

No worries, I still wanted to give you something that you can use to replicate the problem.

I think this line:

const data = fetch(uri, { agent })

Needs to be changed to this:

const data = await fetch(uri, { agent })

(or .then() equivalent) right?

@NotMoni
Copy link
Member

NotMoni commented May 27, 2020

yea 👍

I can't code without VSCode lol.

@roychri
Copy link

roychri commented May 27, 2020

Using your updated code, I was able to reproduce the problem, showing that the use of rejectUnauthorized: false does not solve the issue...

const https = require('https');
const fetch = require('node-fetch');
async function main() {
    const agent = new https.Agent({
        rejectUnauthorized: false
    });
    const uri = 'https://cams.jacksonhole.com/webcam/SouthHoback.jpg?1590589817';
    const data = await fetch(uri, { agent });
}
main().catch(console.error);

If you run this with node 12+ (I tried with 12.16.1) it will fail with HPE_INVALID_HEADER_TOKEN.

That's because the headers being returned by this one specific site (and many others, specially those using Incapsula CDN) is sending an invalid header, breaking the HTTP specs, and the newer version of node does not like that, unless we pass insecureHTTPParser: true to the https request method.

Hence the feature request of the OP to be able to forward options to http/https requests...

Not every website is affected by this. I read somewhere that Incapsula is doing that on purpose to detect bots... 🤷‍♂️

So we have no control over the other ends...

Does that clarifies the situation?

@c-kruse c-kruse mentioned this issue Jun 1, 2020
4 tasks
@c-kruse
Copy link
Contributor

c-kruse commented Jun 1, 2020

Hi! Just took a stab at PRing the suggestion @jamiechong had for this! As far as I can tell, there weren't any clever ways around this. #855

@c-kruse
Copy link
Contributor

c-kruse commented Jun 11, 2020

Think we’re okay to close this now?

https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md#v300-beta7

@jamiechong
Copy link
Author

Amazing. Thanks for the effort put into this!

@dobromyslov
Copy link

Note for those who's looking for insecureHTTPParser option in 2.x. This option is released in 3.x and requires your project to be updated to support ESM.

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

No branches or pull requests

8 participants