Skip to content
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

Merged
merged 2 commits into from
Apr 6, 2018

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Apr 5, 2018

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

@Lukasa Lukasa requested review from normanmaurer and weissi April 5, 2018 16:37
Copy link
Member

@weissi weissi left a 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

@@ -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?
Copy link
Member

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 ;)

Copy link
Member

@weissi weissi Apr 5, 2018

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

Copy link
Contributor Author

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.
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway it's largely theoretical as HTTP/1.2 is never gonna happen.

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.
@Lukasa Lukasa force-pushed the cb-refuse-http-versions-other-than-1 branch from e4d1e08 to 937c458 Compare April 5, 2018 16:49
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@tomerd
Copy link
Member

tomerd commented Apr 5, 2018

@swift-nio-bot test this please

@Lukasa
Copy link
Contributor Author

Lukasa commented Apr 6, 2018

Gah, this hit testWeDontCrashIfChannelReleasesBeforePipeline flakiness. I think there is a real timing issue here, probably best addressed by just sleeping the main thread.

@Lukasa
Copy link
Contributor Author

Lukasa commented Apr 6, 2018

@swift-nio-bot test this please

@Lukasa Lukasa merged commit d6ade14 into apple:master Apr 6, 2018
@Lukasa Lukasa deleted the cb-refuse-http-versions-other-than-1 branch April 6, 2018 10:26
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Apr 6, 2018
@Lukasa Lukasa added this to the 1.4.0 milestone Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants