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

Impossible to make a HTTP request if response headers include control characters #30573

Closed
gajus opened this issue Nov 21, 2019 · 7 comments
Closed
Labels
feature request Issues that request new features to be added to Node.js. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem.

Comments

@gajus
Copy link

gajus commented Nov 21, 2019

Version

$ node -v
v13.1.0
$ uname -a                                                                                                                                                                                                                                         06:58:36
Darwin Admins-MacBook-Pro.local 19.0.0 Darwin Kernel Version 19.0.0: Wed Sep 25 20:18:50 PDT 2019; root:xnu-6153.11.26~2/RELEASE_X86_64 x86_64

Code

require('https')
  .get('https://www.megaplextheatres.com/api/theatres/all', (response) => {
    console.log('OK');
  })
  .on('error', (error) => {
    console.error(error);
  });

Actual

Error:

Error: Parse Error: Invalid header value char
    at TLSSocket.socketOnData (_http_client.js:454:22)
    at TLSSocket.emit (events.js:210:5)
    at addChunk (_stream_readable.js:326:12)
    at readableAddChunk (_stream_readable.js:301:11)
    at TLSSocket.Readable.push (_stream_readable.js:235:10)
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:182:23) {
  bytesParsed: 722,
  code: 'HPE_INVALID_HEADER_TOKEN',
  reason: 'Invalid header value char',
  rawPacket: <Buffer 48 54 54 50 2f 31 2e 31 20 32 30 30 20 4f 4b 0d 0a 54 72 61 6e 73 66 65 72 2d 45 6e 63 6f 64 69 6e 67 3a 20 63 68 75 6e 6b 65 64 0d 0a 43 6f 6e 74 65 ... 1402 more bytes>
}

Expected

"OK" or an error with an access to the raw HTTP response, so that I could at least somehow parse the response myself.

Workarounds

None known to me.

Previously it was possible to workaround it using --http-parser=legacy.

$ node -v
v12.11.1
$ node --http-parser=legacy test.js
OK

This stopped working in v13.

Previous it was possible to override the default HTTP parser.

process.binding('http_parser').HTTPParser = require('http-parser-js').HTTPParser;

require('https')
  .get('https://www.megaplextheatres.com/api/theatres/all', (response) => {
    console.log('OK');
  })
  .on('error', (error) => {
    console.error(error);
  });
$ node -v
v10.17.0
$ node test-using-http-parser-js.js
OK

This stopped working in v12.

This issue did not exist in Node.js v10.

Related issues

@gajus
Copy link
Author

gajus commented Nov 21, 2019

CC @addaleax as she is the author of #29589.

@devsnek devsnek added feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Nov 21, 2019
@devsnek
Copy link
Member

devsnek commented Nov 21, 2019

Does #30567 let in control characters? @indutny @sam-github

@indutny
Copy link
Member

indutny commented Nov 21, 2019

I believe it would help.

@gajus
Copy link
Author

gajus commented Nov 21, 2019

I don't particularly like --insecure-http as a solution to this.

It is a welcome workaround, though it is very much all or nothing.

I would much rather have Node.js throw an error and give me access to the raw HTTP response.

This way I could do whatever checks before proceeding to parse the response.

@devsnek
Copy link
Member

devsnek commented Nov 21, 2019

if you just want the raw response, why not use net, or the underlying socket from the http request?

@gajus
Copy link
Author

gajus commented Nov 21, 2019

That would break a number of things, starting with http.Agent/ HTTP proxy support.

Come to think about it, if Node.js cannot parse the HTTP response, then it couldn't provide the necessary data to the http.Agent anyway. It appears that llhttp is the way to go.

@gajus
Copy link
Author

gajus commented Feb 2, 2020

Solved by --insecure-http-parser Node.js attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants