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

Receiving a PUSH_PROMISE from a client is an error #315

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions HISTORY.rst
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ Bugfixes
7540 Section 8.1.2.1.
- Correctly refuse to send trailers that contain HTTP/2 pseudo-header fields,
per RFC 7540 Section 8.1.2.1.
- Correctly reject a PUSH_PROMISE frame sent by a client, per RFC 7540
Section 8.2.


2.4.0 (2016-07-01)
Expand Down
4 changes: 4 additions & 0 deletions h2/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,10 @@ def _receive_push_promise_frame(self, frame):
"""
Receive a push-promise frame on the connection.
"""
# A client cannot push - RFC 7540 § 8.2
if not self.config.client_side:
raise ProtocolError("Received pushed stream from a client")

if not self.local_settings.enable_push:
raise ProtocolError("Received pushed stream")
Copy link
Member

Choose a reason for hiding this comment

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

So, here's my question: is this worth it? I think, ordinarily, an invalid push would hit this code block here. That means that we'd still raise a ProtocolError. It also means that now, on push promise receipt, we have an extra if test that always fires in order to check for an error that we'll then immediately look for.

Should we consider whether it'd be better to put the new check into this if block? Or, even more aggressively, should we just implement #316? If we implement #316 then, definitionally, a server will be forbidden from setting ENABLE_PUSH to 1 (because we'll tell it not to), which means that all servers will automatically fall into the if block here.


Expand Down
30 changes: 30 additions & 0 deletions test/test_basic_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -1523,3 +1523,33 @@ def test_receiving_goaway_frame_with_unknown_error(self, frame_factory):
assert c.state_machine.state == h2.connection.ConnectionState.CLOSED

assert not c.data_to_send()

def test_receiving_push_promise_from_client_is_error(self, frame_factory):
"""
If we are a server, receiving PUSH_PROMISE frames from a client
causes the connection to be closed.
"""
c = h2.connection.H2Connection(client_side=False)
c.initiate_connection()
c.receive_data(frame_factory.preamble())

f1 = frame_factory.build_headers_frame(
self.example_request_headers
)
f2 = frame_factory.build_push_promise_frame(
stream_id=1,
promised_stream_id=2,
headers=self.example_request_headers,
flags=['END_HEADERS'],
)
c.receive_data(f1.serialize())
c.clear_outbound_data_buffer()

with pytest.raises(h2.exceptions.ProtocolError) as excinfo:
c.receive_data(f2.serialize())
excinfo.match('Received pushed stream from a client')

expected_frame = frame_factory.build_goaway_frame(
1, h2.errors.ErrorCodes.PROTOCOL_ERROR
)
assert c.data_to_send() == expected_frame.serialize()
Copy link
Member

Choose a reason for hiding this comment

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

Running this on my machine shows that this test would have passed before the code change above. Can you check the exception message, which is the substantive part of this change?

Copy link
Contributor Author

@alexwlchan alexwlchan Sep 19, 2016

Choose a reason for hiding this comment

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

The test now checks the exception message.