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

HTTP response corrupted with status 400 when URL includes pipe character (|) #27584

Closed
hoangsetup opened this issue May 6, 2019 · 5 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. 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

@hoangsetup
Copy link

  • Version: v12.1.0 v12.0.0
  • Platform: MacOS 10.14.2 - Darwin Kernel Version 18.5.0: Mon Mar 11 20:40:32 PDT 2019; root:xnu-4903.251.3~3/RELEASE_X86_64 x8
    6_64

My code:

index.js

var http = require('http');
var url = require('url');

http.createServer(function (req, res) {
  res.writeHead(200, { 'Content-Type': 'text/html' });
  var q = url.parse(req.url, true).query;
  var search = q.search;
  res.end(search);
}).listen(8080);

Starting command: node index.js

Chrome: Version 74.0.3729.131 (Official Build) (64-bit)

When I try to request to my http service, it is working fine with normal url like
http://localhost:8080/?search=example

But, when the url come to http://localhost:8080/?search=example| or http://localhost:8080/?sea|rch=example . My browser gets back This page isn’t working and HTTP ERROR 400 (the request did not appeared on Network tab of Chrome devtool) .

Try again with curl or Postman, these urls are working fine 🤔

=> When I use older node version like v11.11.0 I don't get any errors.

@Trott Trott added http Issues or PRs related to the http subsystem. url Issues and PRs related to the legacy built-in url module. labels May 6, 2019
@addaleax addaleax added http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. and removed url Issues and PRs related to the legacy built-in url module. labels May 6, 2019
@addaleax
Copy link
Member

addaleax commented May 6, 2019

Thank you for reporting this bug! It’s an issue that seems to occur because we switched out the HTTP parser library default in Node 12. The good news is that you can use --http-parser=legacy to work around this issue for now, and get consistent behaviour on both v11.x and v12.x.

@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label May 6, 2019
@addaleax
Copy link
Member

addaleax commented May 6, 2019

Yes, this seems like something that should be fixed in llhttp to me. At least browsers and curl do not percent-encode | in the given example URLs.

/cc @indutny @nodejs/http

@indutny
Copy link
Member

indutny commented May 6, 2019

/me is on it

@indutny
Copy link
Member

indutny commented May 6, 2019

🤦 I literally missed this single character. Thank you @hoangsetup for catching this!

indutny added a commit to nodejs/llhttp that referenced this issue May 6, 2019
The vertical bar character `|` wasn't ported from http-parser during
rewrite. This commit introduces it back.

See: https://github.com/nodejs/http-parser/blob/5c17dad400e45c5a442a63f250fff2638d144682/http_parser.c#L275
See: nodejs/node#27584
@indutny
Copy link
Member

indutny commented May 6, 2019

See: nodejs/llhttp#24

indutny added a commit to nodejs/llhttp that referenced this issue May 7, 2019
The vertical bar character `|` wasn't ported from http-parser during
rewrite. This commit introduces it back.

See: https://github.com/nodejs/http-parser/blob/5c17dad400e45c5a442a63f250fff2638d144682/http_parser.c#L275
See: nodejs/node#27584

PR-URL: #24
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos closed this as completed in c476daf May 7, 2019
targos pushed a commit that referenced this issue May 7, 2019
Fixes: #27584
PR-URL: #27595
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. 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

4 participants