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: send 400 bad request on parse error #15324

Closed
wants to merge 1 commit into
base: master
from

Conversation

@mog422
Contributor

mog422 commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Sep 11, 2017

Contributor

-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.

Contributor

mscdex commented Sep 11, 2017

-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

This comment has been minimized.

Show comment
Hide comment
@mog422

mog422 Sep 11, 2017

Contributor

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.

Contributor

mog422 commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 11, 2017

Member

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.

Member

apapirovski commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 12, 2017

Member

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.

Member

jasnell commented Sep 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 13, 2017

Member

@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.

Member

BridgeAR commented Sep 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 14, 2017

Member

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

Member

jasnell commented Sep 14, 2017

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

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Sep 14, 2017

Member

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?

Member

apapirovski commented Sep 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 15, 2017

Member

@nodejs/tsc thoughts?

Member

jasnell commented Sep 15, 2017

@nodejs/tsc thoughts?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 15, 2017

Member

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

Member

dougwilson commented Sep 15, 2017

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

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Sep 18, 2017

Member

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.

Member

mcollina commented Sep 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 19, 2017

Member

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

Member

jasnell commented Sep 19, 2017

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

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Sep 19, 2017

Member

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.

Member

dougwilson commented Sep 19, 2017

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.

Show outdated Hide outdated lib/_http_server.js
@mog422

This comment has been minimized.

Show comment
Hide comment
@mog422

mog422 Sep 19, 2017

Contributor

I fixed it.

Contributor

mog422 commented Sep 19, 2017

I fixed it.

@mog422

This comment has been minimized.

Show comment
Hide comment
@mog422

mog422 Sep 20, 2017

Contributor

@cjihrig fixed.

Contributor

mog422 commented Sep 20, 2017

@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 jasnell added the tsc-review label Sep 20, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 20, 2017

Member

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

Member

jasnell commented Sep 20, 2017

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

@jasnell jasnell requested a review from nodejs/tsc Sep 20, 2017

@mog422

This comment has been minimized.

Show comment
Hide comment
@mog422

mog422 Sep 20, 2017

Contributor

@mscdex What does it mean?

Contributor

mog422 commented Sep 20, 2017

@mscdex What does it mean?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Sep 20, 2017

Contributor

@mog422 What does what mean?

Contributor

mscdex commented Sep 20, 2017

@mog422 What does what mean?

@mog422

This comment has been minimized.

Show comment
Hide comment
@mog422

mog422 Sep 20, 2017

Contributor

@mscdex I can not understand what you said "Making it more explicit". Can you explain in detail?

Contributor

mog422 commented Sep 20, 2017

@mscdex I can not understand what you said "Making it more explicit". Can you explain in detail?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Sep 20, 2017

Contributor

@mog422 This:

@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).

^

Contributor

mscdex commented Sep 20, 2017

@mog422 This:

@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).

^

@mog422

This comment has been minimized.

Show comment
Hide comment
@mog422

mog422 Sep 20, 2017

Contributor

@mscdex So what is it?

mscdex requested changes on this pull request

Contributor

mog422 commented Sep 20, 2017

@mscdex So what is it?

mscdex requested changes on this pull request

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Sep 20, 2017

Contributor

@mscdex So what is it?

mscdex requested changes on this pull request

@mog422 See: #15324 (comment)

Contributor

mscdex commented Sep 20, 2017

@mscdex So what is it?

mscdex requested changes on this pull request

@mog422 See: #15324 (comment)

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 18, 2017

Member

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

Member

Trott commented Oct 18, 2017

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.

@jasnell jasnell added this to the 9.0.0 milestone Oct 18, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 18, 2017

Member

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.

Member

addaleax commented Oct 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BridgeAR
Member

BridgeAR commented Oct 19, 2017

Dismissing the blocking review due to the TSC vote.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Oct 19, 2017

Member

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

It seemed very red for some infra problem.

Member

mcollina commented Oct 19, 2017

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

It seemed very red for some infra problem.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 19, 2017

Member

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

Landed in f2f391e

Member

addaleax commented Oct 19, 2017

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

Landed in f2f391e

@addaleax addaleax closed this Oct 19, 2017

addaleax added a commit that referenced this pull request Oct 19, 2017

http: send 400 bad request on parse error
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>

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

http: send 400 bad request on parse error
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>

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

http: send 400 bad request on parse error
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>

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

doc: update description of 'clientError' event
Default behavior is to send a '400 Bad Request' response if the socket
is writable.

Refs: nodejs#15324

@lpinca lpinca referenced this pull request Feb 20, 2018

Closed

doc: update description of 'clientError' event #18885

3 of 3 tasks complete

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

doc: update description of 'clientError' event
Default behavior is to send a '400 Bad Request' response if the socket
is writable.

Refs: nodejs#15324

benjamingr added a commit that referenced this pull request Feb 23, 2018

doc: update description of 'clientError' event
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>

addaleax added a commit to addaleax/node that referenced this pull request Feb 26, 2018

doc: update description of 'clientError' event
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>

addaleax added a commit that referenced this pull request Feb 26, 2018

doc: update description of 'clientError' event
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>

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

doc: update description of 'clientError' event
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>

MylesBorins added a commit that referenced this pull request Aug 8, 2018

doc: update description of 'clientError' event
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>

MylesBorins added a commit that referenced this pull request Aug 9, 2018

doc: update description of 'clientError' event
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>

rvagg added a commit that referenced this pull request Aug 16, 2018

doc: update description of 'clientError' event
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>

MylesBorins added a commit that referenced this pull request Aug 16, 2018

doc: update description of 'clientError' event
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment