-
Notifications
You must be signed in to change notification settings - Fork 652
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
Forbid HTTP protocols other than 1. #283
Forbid HTTP protocols other than 1. #283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that looks great, one minor issue
Sources/NIOHTTP1/HTTPDecoder.swift
Outdated
@@ -25,6 +25,7 @@ private struct HTTPParserState { | |||
var readerIndexAdjustment = 0 | |||
// This is set before http_parser_execute(...) is called and set to nil again after it finish | |||
var baseAddress: UnsafePointer<UInt8>? | |||
var currentError: Error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can save an existential here if you make the type HTTPParserError
. That probably also save an allocation, maybe ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, only potentially saves an allocation on assignment (which doesn't matter as this never happens). But definitely saves a couple of words in this structure if we don't make that an existential type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, addressed.
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestDecoder()).wait()) | ||
|
||
// We tolerate higher versions of HTTP/1 than we know about because RFC 7230 | ||
// says that these should be treated like HTTP/1.1 by our users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omg, what's wrong with RFC7230?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, the right thing for our users to do is to send HTTP/1.1 responses in response to HTTP/1.2 requests. But we can't really enforce that sensibly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation: Our HTTP code handles only HTTP/1.X. There is no reason to support HTTP/0.9, and we cannot safely handle a major protocol higher than 1 in this code, so we should simply treat requests/responses claiming to be of those protocols as errors. Modifications: HTTPDecoder now checks the major version is equal to 1 before it continues with parsing. If it hits an error, that error will be propagated out to the user. Result: Better resilience against bad HTTP messages.
e4d1e08
to
937c458
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
@swift-nio-bot test this please |
Gah, this hit |
@swift-nio-bot test this please |
Motivation:
Our HTTP code handles only HTTP/1.X. There is no reason to support
HTTP/0.9, and we cannot safely handle a major protocol higher than 1 in
this code, so we should simply treat requests/responses claiming to be
of those protocols as errors.
Modifications:
HTTPDecoder now checks the major version is equal to 1 before it
continues with parsing. If it hits an error, that error will be propagated
out to the user.
Result:
Better resilience against bad HTTP messages.
Resolves #281