From ece998d9165848d74875e4dd4f1fe4edfe94cdac Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Mon, 12 Sep 2016 20:37:45 +0530 Subject: [PATCH 01/11] =?UTF-8?q?Reject=20response=20header=20blocks=20tha?= =?UTF-8?q?t=20don=E2=80=99t=20include=20the=20=20field?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- h2/stream.py | 10 ++++++--- h2/utilities.py | 9 +++++++- test/test_invalid_headers.py | 40 ++++++++++++++++++++++++++---------- 3 files changed, 44 insertions(+), 15 deletions(-) diff --git a/h2/stream.py b/h2/stream.py index 940849304..f3d74be39 100644 --- a/h2/stream.py +++ b/h2/stream.py @@ -766,7 +766,8 @@ def send_headers(self, headers, encoder, end_stream=False): events = self.state_machine.process_input(input_) hf = HeadersFrame(self.stream_id) - hdr_validation_flags = self._build_hdr_validation_flags(events) + hdr_validation_flags = self._build_hdr_validation_flags( + events, is_response=not self.state_machine.client) frames = self._build_headers_frames( headers, encoder, hf, hdr_validation_flags ) @@ -1031,7 +1032,7 @@ def stream_reset(self, frame): return [], events - def _build_hdr_validation_flags(self, events): + def _build_hdr_validation_flags(self, events, is_response=False): """ Constructs a set of header validation flags for use when normalizing and validating header blocks. @@ -1049,9 +1050,12 @@ def _build_hdr_validation_flags(self, events): # an internal event, so we can do away with this branch. is_trailer = False + is_response_header = is_response and not is_trailer + 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, diff --git a/h2/utilities.py b/h2/utilities.py index 3b3ff26d0..6c5f7ae2d 100644 --- a/h2/utilities.py +++ b/h2/utilities.py @@ -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'] ) @@ -329,6 +329,13 @@ 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 + if (hdr_validation_flags.is_response_header and + not any(field in seen_pseudo_header_fields + for field in (b':status', u':status'))): + raise ProtocolError( + "Couldn't find :status pseudo-header field in response header") + def _validate_host_authority_header(headers): """ diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index 4d323810b..72e5b1ab0 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -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,20 +241,29 @@ 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) + h2.utilities.HeaderValidationFlags( + is_client, is_trailer, is_response_header) + for is_client, is_trailer, is_response_header in [ + (True, False, False), + (False, False, False), + ] + ] + + hdr_validation_response_headers = [ + h2.utilities.HeaderValidationFlags( + is_client, is_trailer, is_response_header) + for is_client, is_trailer, is_response_header in [ + (True, False, True), + (False, True, True), + (True, True, True), + (False, False, True) ] ] @@ -300,6 +311,13 @@ def test_matching_authority_host_headers(self, assert headers == h2.utilities.validate_headers( 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): """ From 8fed89cb58ddea6aa536da8f55105da2bd10f32a Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Tue, 13 Sep 2016 12:58:44 +0530 Subject: [PATCH 02/11] Get rid of the unnecessary is_response flag. And some code style fixes --- h2/stream.py | 11 ++++++----- h2/utilities.py | 11 +++++++---- test/test_invalid_headers.py | 9 ++++++--- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/h2/stream.py b/h2/stream.py index f3d74be39..7ee9cf249 100644 --- a/h2/stream.py +++ b/h2/stream.py @@ -766,8 +766,7 @@ def send_headers(self, headers, encoder, end_stream=False): events = self.state_machine.process_input(input_) hf = HeadersFrame(self.stream_id) - hdr_validation_flags = self._build_hdr_validation_flags( - events, is_response=not self.state_machine.client) + hdr_validation_flags = self._build_hdr_validation_flags(events) frames = self._build_headers_frames( headers, encoder, hf, hdr_validation_flags ) @@ -1032,7 +1031,7 @@ def stream_reset(self, frame): return [], events - def _build_hdr_validation_flags(self, events, is_response=False): + def _build_hdr_validation_flags(self, events): """ Constructs a set of header validation flags for use when normalizing and validating header blocks. @@ -1041,6 +1040,9 @@ def _build_hdr_validation_flags(self, events, is_response=False): is_trailer = isinstance( events[0], (_TrailersSent, TrailersReceived) ) + is_response_header = isinstance( + events[0], ResponseReceived + ) and not self.state_machine.client except IndexError: # Some state changes don't emit an internal event (for example, # sending a push promise). We *always* emit an event for trailers, @@ -1049,8 +1051,7 @@ def _build_hdr_validation_flags(self, events, is_response=False): # 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 = is_response and not is_trailer + is_response_header = False return HeaderValidationFlags( is_client=self.state_machine.client, diff --git a/h2/utilities.py b/h2/utilities.py index 6c5f7ae2d..0db512c4c 100644 --- a/h2/utilities.py +++ b/h2/utilities.py @@ -330,11 +330,14 @@ def _reject_pseudo_header_fields(headers, hdr_validation_flags): ) # If ':status' pseudo-header is not there in a response header, reject it - if (hdr_validation_flags.is_response_header and - not any(field in seen_pseudo_header_fields - for field in (b':status', u':status'))): + if ( + hdr_validation_flags.is_response_header and + b':status' not in seen_pseudo_header_fields and + u':status' not in seen_pseudo_header_fields + ): raise ProtocolError( - "Couldn't find :status pseudo-header field in response header") + "Response header block does not have a :status header" + ) def _validate_host_authority_header(headers): diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index 72e5b1ab0..045cca482 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -242,14 +242,16 @@ class TestFilter(object): hdr_validation_combos = [ h2.utilities.HeaderValidationFlags( - is_client, is_trailer, is_response_header) + 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, is_response_header) + is_client, is_trailer, is_response_header + ) for is_client, is_trailer, is_response_header in [ (True, False, False), (False, False, False), @@ -258,7 +260,8 @@ class TestFilter(object): hdr_validation_response_headers = [ h2.utilities.HeaderValidationFlags( - is_client, is_trailer, is_response_header) + is_client, is_trailer, is_response_header + ) for is_client, is_trailer, is_response_header in [ (True, False, True), (False, True, True), From d3435434ac551e58401107d190ae382f77fee97f Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Tue, 13 Sep 2016 13:05:18 +0530 Subject: [PATCH 03/11] A couple more style fixes --- test/test_invalid_headers.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index 045cca482..b9d557fad 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -245,7 +245,8 @@ class TestFilter(object): is_client, is_trailer, is_response_header ) for is_client, is_trailer, is_response_header in ( - itertools.product([True, False], repeat=3)) + itertools.product([True, False], repeat=3) + ) ] hdr_validation_no_trailers = [ @@ -314,8 +315,9 @@ def test_matching_authority_host_headers(self, assert headers == h2.utilities.validate_headers( headers, hdr_validation_flags) - @pytest.mark.parametrize('hdr_validation_flags', - hdr_validation_response_headers) + @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): From d74d28db77e87d61e861e58b8a03c4e0bfc775c9 Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Thu, 15 Sep 2016 22:23:58 +0530 Subject: [PATCH 04/11] Introduce _ResponseHeadersSent event --- h2/events.py | 11 +++++++++++ h2/stream.py | 11 +++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/h2/events.py b/h2/events.py index 6d6aa1f42..9ef9369e7 100644 --- a/h2/events.py +++ b/h2/events.py @@ -146,6 +146,17 @@ class _HeadersSent(object): pass +class _ResponseHeadersSent(_HeadersSent): + """ + The _ResponseHeadersSent event is fired whenever response headers are sent + on a stream. + + This is an internal event, used to determine validation steps on + outgoing header blocks. + """ + pass + + class _TrailersSent(_HeadersSent): """ The _TrailersSent event is fired whenever trailers are sent on a diff --git a/h2/stream.py b/h2/stream.py index 7ee9cf249..b6d4a7c8f 100644 --- a/h2/stream.py +++ b/h2/stream.py @@ -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 @@ -1041,12 +1041,15 @@ def _build_hdr_validation_flags(self, events): events[0], (_TrailersSent, TrailersReceived) ) is_response_header = isinstance( - events[0], ResponseReceived - ) and not self.state_machine.client + events[0], (_ResponseHeadersSent, ResponseReceived) + ) 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 reponse headers are + # sent or received. So absent 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. From 6fcca0097a312cb2994fdf55e7b7e9cb4b067d37 Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Fri, 16 Sep 2016 21:02:33 +0530 Subject: [PATCH 05/11] Use filter to generate validation flags for testing --- test/test_basic_logic.py | 2 +- test/test_invalid_headers.py | 26 ++++++-------------------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/test/test_basic_logic.py b/test/test_basic_logic.py index 1664c2408..caa1303a2 100644 --- a/test/test_basic_logic.py +++ b/test/test_basic_logic.py @@ -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) c.receive_data(f.serialize()) # Send in trailers. diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index b9d557fad..41c3f8e12 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -249,27 +249,13 @@ class TestFilter(object): ) ] - hdr_validation_no_trailers = [ - h2.utilities.HeaderValidationFlags( - is_client, is_trailer, is_response_header - ) - for is_client, is_trailer, is_response_header in [ - (True, False, False), - (False, False, False), - ] - ] + hdr_validation_response_headers = list( + filter(lambda flags: flags.is_response_header, hdr_validation_combos) + ) - hdr_validation_response_headers = [ - h2.utilities.HeaderValidationFlags( - is_client, is_trailer, is_response_header - ) - for is_client, is_trailer, is_response_header in [ - (True, False, True), - (False, True, True), - (True, True, True), - (False, False, True) - ] - ] + hdr_validation_no_trailers = list( + filter(lambda flags: not flags.is_trailer, hdr_validation_combos) + ) @pytest.mark.parametrize('validation_function', validation_functions) @pytest.mark.parametrize('hdr_validation_flags', hdr_validation_combos) From bceb7019d30f1d6be8b866d216456a952e717c5b Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Fri, 16 Sep 2016 21:02:59 +0530 Subject: [PATCH 06/11] Refactoring some ugly code --- h2/utilities.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/h2/utilities.py b/h2/utilities.py index 0db512c4c..436223318 100644 --- a/h2/utilities.py +++ b/h2/utilities.py @@ -330,11 +330,11 @@ def _reject_pseudo_header_fields(headers, hdr_validation_flags): ) # If ':status' pseudo-header is not there in a response header, reject it - if ( - hdr_validation_flags.is_response_header and - b':status' not in seen_pseudo_header_fields and - u':status' not in seen_pseudo_header_fields - ): + seen_status_field = ( + b':status' in seen_pseudo_header_fields or + u':status' in seen_pseudo_header_fields + ) + if hdr_validation_flags.is_response_header and not seen_status_field: raise ProtocolError( "Response header block does not have a :status header" ) From 371706f23e10b64bf057f4ea48e9ac6329c0dbeb Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Fri, 16 Sep 2016 22:40:58 +0530 Subject: [PATCH 07/11] Refactored the host-authority matching test --- test/test_invalid_headers.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index 41c3f8e12..be657802f 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -253,8 +253,11 @@ class TestFilter(object): filter(lambda flags: flags.is_response_header, hdr_validation_combos) ) - hdr_validation_no_trailers = list( - filter(lambda flags: not flags.is_trailer, hdr_validation_combos) + hdr_validation_request_headers_no_trailer = list( + filter( + lambda flags: not (flags.is_trailer or flags.is_response_header), + hdr_validation_combos + ) ) @pytest.mark.parametrize('validation_function', validation_functions) @@ -282,11 +285,11 @@ 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, - hdr_validation_flags): + def test_matching_authority_host_headers( + self, validation_function, hdr_validation_flags + ): """ If a header block has :authority and Host headers and they match, the headers should pass through unchanged. @@ -299,7 +302,8 @@ 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 From 92ff0e9c64b0ba4c13157183c6bdc0b96421a682 Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Fri, 16 Sep 2016 23:05:32 +0530 Subject: [PATCH 08/11] Fix code style --- test/test_invalid_headers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index be657802f..a64c6d33b 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -287,9 +287,9 @@ def test_invalid_pseudo_headers(self, hdr_validation_flags): @pytest.mark.parametrize( 'hdr_validation_flags', hdr_validation_request_headers_no_trailer ) - def test_matching_authority_host_headers( - self, validation_function, hdr_validation_flags - ): + def test_matching_authority_host_headers(self, + validation_function, + hdr_validation_flags): """ If a header block has :authority and Host headers and they match, the headers should pass through unchanged. From 8cb0e39c040858a6eccdbe73ff86cbb7f40fc9af Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Sat, 17 Sep 2016 00:39:06 +0530 Subject: [PATCH 09/11] fix typo in comment --- h2/stream.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/h2/stream.py b/h2/stream.py index b6d4a7c8f..ac8b05e33 100644 --- a/h2/stream.py +++ b/h2/stream.py @@ -1047,8 +1047,8 @@ def _build_hdr_validation_flags(self, events): # 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 reponse headers are - # sent or received. So absent of those events means this is not an + # 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 From e0edfb3228cb252b7a94a569c342480926183a47 Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Tue, 20 Sep 2016 23:42:57 +0530 Subject: [PATCH 10/11] add reference to the relevant RFC section --- h2/utilities.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/h2/utilities.py b/h2/utilities.py index 436223318..187e339b1 100644 --- a/h2/utilities.py +++ b/h2/utilities.py @@ -330,6 +330,8 @@ def _reject_pseudo_header_fields(headers, hdr_validation_flags): ) # If ':status' pseudo-header is not there in a response header, reject it + # Relevant RFC section: RFC 7540 § 8.1.2.4 + # https://tools.ietf.org/html/rfc7540#section-8.1.2.4 seen_status_field = ( b':status' in seen_pseudo_header_fields or u':status' in seen_pseudo_header_fields From 439d5dd86895576ffc8f252346f7349574fc744b Mon Sep 17 00:00:00 2001 From: Tarashish Mishra Date: Tue, 20 Sep 2016 23:48:03 +0530 Subject: [PATCH 11/11] Use list comprehension instead of filter --- h2/utilities.py | 15 ++++++++------- test/test_invalid_headers.py | 17 ++++++++--------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/h2/utilities.py b/h2/utilities.py index 187e339b1..af64ef6e2 100644 --- a/h2/utilities.py +++ b/h2/utilities.py @@ -332,14 +332,15 @@ def _reject_pseudo_header_fields(headers, hdr_validation_flags): # If ':status' pseudo-header is not there in a response header, reject it # Relevant RFC section: RFC 7540 § 8.1.2.4 # https://tools.ietf.org/html/rfc7540#section-8.1.2.4 - seen_status_field = ( - b':status' in seen_pseudo_header_fields or - u':status' in seen_pseudo_header_fields - ) - if hdr_validation_flags.is_response_header and not seen_status_field: - raise ProtocolError( - "Response header block does not have a :status header" + 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): diff --git a/test/test_invalid_headers.py b/test/test_invalid_headers.py index a64c6d33b..b819f9ac7 100644 --- a/test/test_invalid_headers.py +++ b/test/test_invalid_headers.py @@ -249,16 +249,15 @@ class TestFilter(object): ) ] - hdr_validation_response_headers = list( - filter(lambda flags: flags.is_response_header, hdr_validation_combos) - ) + hdr_validation_response_headers = [ + flags for flags in hdr_validation_combos + if flags.is_response_header + ] - hdr_validation_request_headers_no_trailer = list( - filter( - lambda flags: not (flags.is_trailer or flags.is_response_header), - hdr_validation_combos - ) - ) + 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) @pytest.mark.parametrize('hdr_validation_flags', hdr_validation_combos)