Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Regression in Upgrade header handling #217

Closed
bnoordhuis opened this issue Jan 28, 2015 · 4 comments
Closed

Regression in Upgrade header handling #217

bnoordhuis opened this issue Jan 28, 2015 · 4 comments

Comments

@bnoordhuis
Copy link
Member

See nodejs/node#627 and nodejs/node#628 for background.

In a nutshell, 2.4.1 doesn't like uncommon Upgrade header formats. The regression test in nodejs/node#628 is a good example.

/cc @indutny
@bnoordhuis bnoordhuis mentioned this issue Jan 28, 2015
Merged
@mscdex
Copy link
Contributor

mscdex commented Apr 11, 2015

"Uncommon" here means "invalid" since both Connection: upgrade and Upgrade: ... are required according to RFC 7230 (second paragraph). Unless we're wanting to support broken clients.... but are there real cases of that out in the wild where clients only send Upgrade?

On a semi-related note, Upgrade headers must also be ignored if received from an HTTP 1.0 client. I'm guessing this should be handled at the parser layer too?

@indutny
Copy link
Member

indutny commented Apr 21, 2015

@mscdex all of your comment is correct, except that I have introduced an unintentional behavior change. This means that there could be some bug that I have overlooked.

@indutny
Copy link
Member

indutny commented Apr 21, 2015

Nah, I see now where this behavior was introduced 091ebb8 . @bnoordhuis on a second thought I think that @mscdex is right about it as the change was quite intentional.

@bnoordhuis Why do we want to support this?

indutny added a commit to indutny/http-parser that referenced this issue Apr 21, 2015
@indutny
src: handle Upgrade: without Conn: upgrade
43bdb52 
In strict mode this should not be treated as upgrade, in normal mode we
should be gentle and allow it.

Fix: nodejs#217
@indutny indutny mentioned this issue Apr 21, 2015
Closed
@indutny
Copy link
Member

indutny commented Apr 23, 2015

Not a regression.

@indutny indutny closed this as completed Apr 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: handle Upgrade: without Conn: upgrade indutny/http-parser
3 participants
@mscdex @indutny @bnoordhuis