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

Is this valid according to rfc2616? #301

Open
vinniefalco opened this issue Apr 25, 2016 · 9 comments
Open

Is this valid according to rfc2616? #301

vinniefalco opened this issue Apr 25, 2016 · 9 comments

Comments

@vinniefalco
Copy link
Contributor

vinniefalco commented Apr 25, 2016

My reading of rfc2616 is that both spaces and horizontal tabs are allowed as LWS in field values:

 LWS = [CRLF] 1*( SP | HT )

But in http_parser.c where there should be checks for both SP and HT I see only a check for SP:
https://github.com/nodejs/http-parser/blob/master/http_parser.c#L1660

That means the following HTTP message is malformed:

GET / HTTP/1.1\r\n
Host: example.com:80\r\n
Connection: keep-alive\t\r\n
\r\n

Am I reading this correctly?

@indutny
Copy link
Member

indutny commented Apr 25, 2016

Hm.. it really looks like we should support this. I think you are reading it correctly.

cc @mscdex and @jasnell just in case

@mscdex
Copy link
Contributor

mscdex commented Apr 25, 2016

Yes, IIRC both characters should be allowed.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2016

For header field values only yes, tabs should be allowed albeit encountering them in the wild appears to be quite rare.

@mscdex
Copy link
Contributor

mscdex commented Apr 25, 2016

Also FWIW rfc7230 obsoletes rfc2616.

@indutny
Copy link
Member

indutny commented Apr 25, 2016

@mscdex yeah, I checked it first, and this thing was the same there too.

@socketpair
Copy link

Yes, this message is malformed since host: header is required for HTTP/1.1

@vinniefalco
Copy link
Contributor Author

@socketpair Sorry, yes the header is missing but that's not the problem. Even re-adding the header makes the message malformed according to the parser. I will edit the post

@vinipsmaker
Copy link

Also FWIW rfc7230 obsoletes rfc2616.

I too much prefer references to the updated document. However, the rules don't change regarding to optional whitespace (OWS in RFC7230) being allowed in field values.

I use this http parser in my project currently and I had to add more code to remove the ending whitespace from the field value before forwarding it to upper layers because this parser erroneously[1] include ending OWS as part of the field value. I agree about the project decision as it keeps the parser simpler and more performant. However I completely disagree about such poor documentation that doesn't warn users on how to properly use this parser. I'd point people to the implementation of my lib rather than this page itself because of the very very very poor documentation around it. I went just fine because of RFCs and a high load of unit tests. HTTP is the domain I'm implementing and I don't expect to get away not knowing the RFCs. However, for other users, the situation may be very different and they're using this parser exactly because they don't need to know the protocol details.

[1] "The field value does not include any leading or trailing whitespace: OWS occurring before the first non-whitespace octet of the field value or after the last non-whitespace octet of the field value ought to be excluded by parsers when extracting the field value from a header field.", section 3.2.4 of RFC7230

@vinipsmaker
Copy link

The RFC7230 (section 3.2) ABNF rules for header fields:

header‑field   = field‑name ":" OWS field‑value OWS
OWS            = *( SP / HTAB )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants