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

Add insecureHTTPParser Parameter #856

Merged
merged 1 commit into from Jun 4, 2020

Conversation

c-kruse
Copy link
Contributor

@c-kruse c-kruse commented Jun 3, 2020

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 option to pass insecureHTTPParser parameter to the http(s).request 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?

See the previous discussion over here: #855

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://www.dezeen.com/robots.txt';
    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

Keep whitespace consistent
@c-kruse c-kruse mentioned this pull request Jun 3, 2020
4 tasks
@tinovyatkin tinovyatkin merged commit af7e67f into node-fetch:master Jun 4, 2020
@c-kruse c-kruse deleted the add-insecurehttpparser branch June 4, 2020 17:06
rnons added a commit to rnons/meta-proxy that referenced this pull request Jun 17, 2020
@NSExceptional
Copy link

NSExceptional commented Jul 17, 2021

Did this ever make it into a release…? cc @tinovyatkin

I'm guessing not since the @types file was not updated either. I would really love to use this…

@dobromyslov
Copy link

@NSExceptional yes. It's released in 3.x wich requires ESM. And it's not backported to the 2.x with CJS support.

@NSExceptional
Copy link

NSExceptional commented Jun 5, 2022

@dobromyslov Could this be backported to 2.x? The README says critical fixes are still being backported... this is pretty critical imo. 2.x is largely useless without it for several websites.

On top of that, TypeScript doesn't fully support ESM yet, and I'm using TypeScript. Gating this behind ESM so soon seems cruel.

@esadalez
Copy link

I need this in 2.x also. I'm using TypeScript and can't use ESM right now.

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

Successfully merging this pull request may close these issues.

None yet

8 participants