http: send 400 bad request on parse error#15324

mog422

A web server such as nginx assumes that upstream is dead
if upstream closes the socket without any response.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http

mscdex

-1 I would prefer to have this explicitly done by userland. Also, this could cause some backwards compatibility issues...

FWIW code-wise the response written to the socket could be a static Buffer for better performance.

mog422

I clearly think that this should be done in the framework.
I have never seen user need to implement this in any framework I know.

apapirovski

Not sure about compatibility but it does feel weird that this is in user-land when comparing it to http2. Seems like node is potentially encouraging a lot of badly behaved servers out there. Just looking at popular user-land implementations, only fastify behaves well.

jasnell

Preferably, this would have been the behavior from the beginning, for backwards compatibility reasons now, I'd say that this is not worth changing and should be kept to userland. The http/2 implementation was intentionally made stricter in these kinds of cases to avoid having this kind of problem. I'm -1 on this change.

BridgeAR

@jasnell what do you think about just running CITGM on it and see how bad things break? This would be semver-major anyway and I do see the point that it makes sense to have it in the framework. Especially as a lot of servers will likely not handle this properly because they did not think about it.

jasnell

I would be extremely surprised if anything in CITGM would exercise this case so I'm not sure if that would help.

apapirovski

Some of the libraries likely have tests around this behaviour, no? Feels like at the very least this would be interesting to test & see what it does with CITGM. Couldn't Node ultimately emit a warning and then semver-major release it, or something?

The fact that this only affects situations where the end-users haven't bound a listener for 'clientError' should make the risk of this at least a little bit smaller, no? If they're not doing any handling of their own, is it that likely to break their code?

jasnell

@nodejs/tsc thoughts?

jasnell

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/986/

dougwilson

I think this should, one day, be the default behavior when there is no registered listener. No one will really notice.

mcollina

This is a semver-major change.

The major problem is that the top frameworks are not implementing it. I think that adding it to core it's the best solution to fix this issue.

@dougwilson are you ok with this one? I'm 👍 if it's ok for you.

jasnell

Ok, I'm convinced ;-) ...

dougwilson

At least for frameworks like Connect / Express / Koa they all sit abstracted higher than the server instance, so they don't have the server reference to add a listener, which is why none of those frameworks set up this kind of listener.

But, if these frameworks ever wanted to, doing so would replace this default handler, so there doesn't seem like much downside to me.

mog422

I fixed it.

mog422

@cjihrig fixed.

cjihrig

Code LGTM.

I do see the potential for backwards compatibility issues too though. @mscdex, you might want to use the big red X to prevent your -1 from getting accidentally overlooked (if you feel strongly enough about it).

mscdex

Making it more explicit

jasnell

Marking for tsc-review given the objections. Ping @nodejs/tsc

Trott

11 TSC members have approved. At the TSC meeting today, we agreed to take that as a vote count, so this can land.

ofrobots

+1 on the change landing. Didn't review the code.

addaleax

Old CIs are inaccessible, so here’s a fresh one: https://ci.nodejs.org/job/node-test-commit/13272/

(Looks like AIX already broke down due to build system changes? @nodejs/build)

This should be ready.

BridgeAR

The CI is very red. Rerun https://ci.nodejs.org/job/node-test-pull-request/10846/

Dismissing the blocking review due to the TSC vote.

mcollina

CI again: https://ci.nodejs.org/job/node-test-pull-request/10851/

It seemed very red for some infra problem.

addaleax

I looked through the the CI runs, nothing in there is related to this PR.

Landed in f2f391e

A web server such as nginx assumes that upstream is dead if upstream closes the socket without any response. PR-URL: #15324 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

A web server such as nginx assumes that upstream is dead if upstream closes the socket without any response. PR-URL: nodejs/node#15324 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Default behavior is to send a '400 Bad Request' response if the socket is writable. Refs: nodejs#15324

Default behavior is to send a '400 Bad Request' response if the socket is writable. PR-URL: #18885 Refs: #15324 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Default behavior is to send a '400 Bad Request' response if the socket is writable. PR-URL: nodejs#18885 Refs: nodejs#15324 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Default behavior is to send a '400 Bad Request' response if the socket is writable. PR-URL: #18885 Refs: #15324 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Default behavior is to send a '400 Bad Request' response if the socket is writable. PR-URL: nodejs#18885 Refs: nodejs#15324 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Default behavior is to send a '400 Bad Request' response if the socket is writable. PR-URL: #18885 Refs: #15324 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 11, 2017

mog422 force-pushed the http.send400.on.parser.error branch 2 times, most recently from 5147084 to 1a7fd84 Compare September 11, 2017 02:09

mog422 force-pushed the http.send400.on.parser.error branch from 1a7fd84 to c3cbc17 Compare September 11, 2017 04:34

mcollina added the semver-major label Sep 18, 2017

BridgeAR approved these changes Sep 19, 2017

View reviewed changes

mog422 force-pushed the http.send400.on.parser.error branch 2 times, most recently from f163e76 to 24325ee Compare September 19, 2017 22:51

cjihrig reviewed Sep 20, 2017

View reviewed changes

mog422 force-pushed the http.send400.on.parser.error branch from 24325ee to 71e702f Compare September 20, 2017 00:15

cjihrig approved these changes Sep 20, 2017

View reviewed changes

mscdex previously requested changes Sep 20, 2017

View reviewed changes

jasnell added the tsc-review label Sep 20, 2017

jasnell requested a review from a team September 20, 2017 07:40

Trott removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. tsc-review labels Oct 18, 2017

ofrobots approved these changes Oct 18, 2017

View reviewed changes

jasnell added this to the 9.0.0 milestone Oct 18, 2017

addaleax closed this Oct 19, 2017

hueniverse mentioned this pull request Oct 19, 2017

Send 400 bad request on parse error hapijs/hapi#3624

Closed

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

cristianbote mentioned this pull request Feb 2, 2018

http server. empty reply on GET request with not url encoded GET-param #6295

Closed

lpinca mentioned this pull request Feb 20, 2018

doc: update description of 'clientError' event #18885

Closed

3 tasks

lpinca added a commit to lpinca/node that referenced this pull request Feb 20, 2018

doc: update description of 'clientError' event …

e733af6

Default behavior is to send a '400 Bad Request' response if the socket is writable. Refs: nodejs#15324

apapirovski mentioned this pull request Apr 12, 2018

HTTP connection prematurely closed when unescaped space present in query string #13407

Closed

dougwilson mentioned this pull request Jul 21, 2020

http: don't write error to socket #34465

Closed

4 tasks

Conversation

mog422 commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mog422 commented Sep 11, 2017

Uh oh!

apapirovski commented Sep 11, 2017

Uh oh!

jasnell commented Sep 12, 2017

Uh oh!

BridgeAR commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 14, 2017

Uh oh!

apapirovski commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

dougwilson commented Sep 15, 2017

Uh oh!

mcollina commented Sep 18, 2017

Uh oh!

jasnell commented Sep 19, 2017

Uh oh!

dougwilson commented Sep 19, 2017

Uh oh!

mog422 commented Sep 19, 2017

Uh oh!

mog422 commented Sep 20, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 20, 2017

Uh oh!

Trott commented Oct 18, 2017

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 18, 2017

Uh oh!

BridgeAR commented Oct 19, 2017

Uh oh!

mcollina commented Oct 19, 2017

Uh oh!

addaleax commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

mog422 commented Sep 11, 2017 •

edited

Loading

mscdex commented Sep 11, 2017 •

edited

Loading

BridgeAR commented Sep 13, 2017 •

edited

Loading

apapirovski commented Sep 14, 2017 •

edited

Loading

addaleax commented Oct 19, 2017 •

edited

Loading