Repository navigation
Reject response header blocks that don’t include the :status field #311
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
Changes from all commits
ece998d
8fed89c
d343543
d74d28d
6fcca00
bceb701
371706f
92ff0e9
8cb0e39
e0edfb3
439d5dd
efb6450
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| RequestReceived, ResponseReceived, DataReceived, WindowUpdated, | ||
| StreamEnded, PushedStreamReceived, StreamReset, TrailersReceived, | ||
| InformationalResponseReceived, AlternativeServiceAvailable, | ||
| _HeadersSent, _TrailersSent | ||
| _HeadersSent, _ResponseHeadersSent, _TrailersSent | ||
| ) | ||
| from .exceptions import ( | ||
| ProtocolError, StreamClosedError, InvalidBodyLengthError | ||
|
|
@@ -146,7 +146,7 @@ def response_sent(self, previous_state): | |
| if self.client is True or self.client is None: | ||
| raise ProtocolError("Client cannot send responses.") | ||
| self.headers_sent = True | ||
| event = _HeadersSent() | ||
| event = _ResponseHeadersSent() | ||
| else: | ||
| assert not self.trailers_sent | ||
| self.trailers_sent = True | ||
|
|
@@ -1040,18 +1040,26 @@ def _build_hdr_validation_flags(self, events): | |
| is_trailer = isinstance( | ||
| events[0], (_TrailersSent, TrailersReceived) | ||
| ) | ||
| is_response_header = isinstance( | ||
| events[0], (_ResponseHeadersSent, ResponseReceived) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Trivial) Seeing these side-by-side, I wonder if we should drop
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than deal with this in this ticket, let's open a new one related to this issue and #314 that refactors the event names.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added as a comment on #314. |
||
| ) | ||
| except IndexError: | ||
| # Some state changes don't emit an internal event (for example, | ||
| # sending a push promise). We *always* emit an event for trailers, | ||
| # so the absence of an event means this definitely isn't a trailer. | ||
| # Similarly, we also emit an event whenever response headers are | ||
| # sent or received. So absence of those events means this is not an | ||
| # response header either. | ||
| # | ||
| # TODO: Find any places where we don't emit anything, and emit | ||
| # an internal event, so we can do away with this branch. | ||
| is_trailer = False | ||
| is_response_header = False | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you extend the comment above to mention that we always emit an event for responses, too? |
||
|
|
||
| return HeaderValidationFlags( | ||
| is_client=self.state_machine.client, | ||
| is_trailer=is_trailer | ||
| is_trailer=is_trailer, | ||
| is_response_header=is_response_header | ||
| ) | ||
|
|
||
| def _build_headers_frames(self, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,7 +171,7 @@ def authority_from_headers(headers): | |
| # should be applied to a given set of headers. | ||
| HeaderValidationFlags = collections.namedtuple( | ||
| 'HeaderValidationFlags', | ||
| ['is_client', 'is_trailer'] | ||
| ['is_client', 'is_trailer', 'is_response_header'] | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -331,6 +331,19 @@ def _reject_pseudo_header_fields(headers, hdr_validation_flags): | |
| seen_pseudo_header_fields | ||
| ) | ||
|
|
||
| # If ':status' pseudo-header is not there in a response header, reject it | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice if this comment had a reference to the exact RFC section – I know we‘ve not been super consistent on that point, but I think it’s helpful. To save you looking it up again: |
||
| # Relevant RFC section: RFC 7540 § 8.1.2.4 | ||
| # https://tools.ietf.org/html/rfc7540#section-8.1.2.4 | ||
| if hdr_validation_flags.is_response_header: | ||
| seen_status_field = ( | ||
| b':status' in seen_pseudo_header_fields or | ||
| u':status' in seen_pseudo_header_fields | ||
| ) | ||
| if not seen_status_field: | ||
| raise ProtocolError( | ||
| "Response header block does not have a :status header" | ||
| ) | ||
|
|
||
|
|
||
| def _validate_host_authority_header(headers): | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -605,7 +605,7 @@ def test_reject_trailers_not_ending_stream(self, frame_factory): | |
| c = h2.connection.H2Connection() | ||
| c.initiate_connection() | ||
| c.send_headers(1, self.example_request_headers) | ||
| f = frame_factory.build_headers_frame(self.example_request_headers) | ||
| f = frame_factory.build_headers_frame(self.example_response_headers) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, nice catch! That's a good validation that the new code is working as expected! |
||
| c.receive_data(f.serialize()) | ||
|
|
||
| # Send in trailers. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
| This module contains tests that use invalid header blocks, and validates that | ||
| they fail appropriately. | ||
| """ | ||
| import itertools | ||
|
|
||
| import pytest | ||
|
|
||
| import h2.connection | ||
|
|
@@ -239,21 +241,22 @@ class TestFilter(object): | |
| ] | ||
|
|
||
| hdr_validation_combos = [ | ||
| h2.utilities.HeaderValidationFlags(is_client, is_trailer) | ||
| for is_client, is_trailer in [ | ||
| (True, True), | ||
| (True, False), | ||
| (False, True), | ||
| (False, False) | ||
| ] | ||
| h2.utilities.HeaderValidationFlags( | ||
| is_client, is_trailer, is_response_header | ||
| ) | ||
| for is_client, is_trailer, is_response_header in ( | ||
| itertools.product([True, False], repeat=3) | ||
| ) | ||
| ] | ||
|
|
||
| hdr_validation_no_trailers = [ | ||
| h2.utilities.HeaderValidationFlags(is_client, is_trailer) | ||
| for is_client, is_trailer in [ | ||
| (True, False), | ||
| (False, False) | ||
| ] | ||
| hdr_validation_response_headers = [ | ||
| flags for flags in hdr_validation_combos | ||
| if flags.is_response_header | ||
| ] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @Lukasa,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I responded somewhat hastily here without reading @Lukasa's comment clearly. @Lukasa this has the same issue that I hit in #305, where testing on specifically request headers we end up with a lot of combinations that don't work. Part of me kind of feels like this test doesn't belong in the filter class, but I didn't know where to move it while working on #305. We'll have to continually keep tweaking
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot safely ignore response headers: we still need to confirm that they don't explode. =)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand or whether we're talking about the same thing. So let me just push my changes. May be it'll be easier then.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the latest build shows the issue a bit clearer I think. @Lukasa I know we want to be checking for the broadest set of options here in Since we're already testing that the failure happens in I completely agree that we should be testing for all cases, I'm just afraid I may have accidentally started a trend of building unnecessary infrastructure (see:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hrm. Are we able to filter out inconsistent combinations effectively? That is, can we run a filter that requires that we never have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can modify (
(is_client, is_trailer, is_response_header'),
(True, False, False)
)So we lose any point of having this in the TestFilter class to begin with. We're testing all of these other combinations elsewhere in the test base.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, in that case let's just simplify that down. =)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the latest change I'm considering both the cases for |
||
|
|
||
| hdr_validation_request_headers_no_trailer = [ | ||
| flags for flags in hdr_validation_combos | ||
| if not (flags.is_trailer or flags.is_response_header) | ||
| ] | ||
|
|
||
| @pytest.mark.parametrize('validation_function', validation_functions) | ||
|
|
@@ -281,7 +284,7 @@ def test_invalid_pseudo_headers(self, hdr_validation_flags): | |
|
|
||
| @pytest.mark.parametrize('validation_function', validation_functions) | ||
| @pytest.mark.parametrize( | ||
| 'hdr_validation_flags', hdr_validation_no_trailers | ||
| 'hdr_validation_flags', hdr_validation_request_headers_no_trailer | ||
| ) | ||
| def test_matching_authority_host_headers(self, | ||
| validation_function, | ||
|
|
@@ -298,7 +301,16 @@ def test_matching_authority_host_headers(self, | |
| (b'host', b'example.com'), | ||
| ] | ||
| assert headers == h2.utilities.validate_headers( | ||
| headers, hdr_validation_flags) | ||
| headers, hdr_validation_flags | ||
| ) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| 'hdr_validation_flags', hdr_validation_response_headers | ||
| ) | ||
| def test_response_header_without_status(self, hdr_validation_flags): | ||
| headers = [(b'content-length', b'42')] | ||
| with pytest.raises(h2.exceptions.ProtocolError): | ||
| h2.utilities.validate_headers(headers, hdr_validation_flags) | ||
|
|
||
|
|
||
| class TestOversizedHeaders(object): | ||
|
|
||
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'd like at this point to also add a
_RequestHeadersSentfield. We don't have to do it as part of this PR, but can you open a new issue to track adding the new internal event? The lack of symmetry is making me sad. ;)