Repository navigation
RFC 9110: reject header field names that are not a token - #732
Closed
arturobernalg wants to merge 1 commit into
Closed
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
The name of a received header was taken as is, up to the colon, so names with spaces, control characters or delimiters were accepted and exposed to the application. A recipient that parses names differently, such as a proxy in front of or behind HttpCore, can be made to disagree with it about which header was sent. BufferedHeader and BasicLineParser now reject a name that is not a token with the ParseException they already throw for a malformed header, which the message parsers turn into a 400 or a failed message. The check is a single pass over the name.
Member
|
@arturobernalg I am not in favor of this change. There can always be recipients that parse things differently. This does not justify adding a full god damn scan to each and every header token. |
Member
|
@arturobernalg A proper solution to the problem would be passing a char validation lambda or |
Member
Author
|
Thanks, Oleg. Agreed, the check should not cost a pass over every header name. I am closing this one.
I will keep that in mind and I am not pursuing it now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The name of a received header was taken as is, up to the colon. Against real classic and async servers, master answered
200 OKand exposed the namesFoo Bar,X\x01Y,Foo\x7f,Foo\x09BarandFoo;x. A recipient that parses names differently, such as a proxy in front of or behind HttpCore, can be made to disagree with it about which header was sent.BufferedHeaderandBasicLineParsernow reject a name that is not atoken(RFC 9110 section 5.6.2) with theParseExceptionthey already throw for a malformed header, which both message parsers turn into a 400 or a failed message. The check is one pass over the name. With the change the same requests get400 Bad RequestandX-Good_1.astill gets 200.Whitespace before the colon and obs-fold are handled as before.
Compatibility: a peer that sends names with characters such as
@,[or spaces now gets a 400 from a server, and a client now fails a response carrying such a name, where it used to be accepted. Dropping the offending header instead of failing the message on the lax parser would be the alternative.