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.

Sign up for GitHub

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

Receiving a PUSH_PROMISE from a client is an error #315

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4afd59c
Receiving a PUSH_PROMISE from a client is an error
alexwlchan Sep 19, 2016
797772f
That full stop annoys me
alexwlchan Sep 19, 2016
934d013
Make sure the test catches the correct exception message
alexwlchan Sep 19, 2016
3bd24b8
Add a changelog entry
alexwlchan Sep 19, 2016
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
4 changes: 4 additions & 0 deletions h2/connection.py
View file
Open in desktop
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
29 changes: 29 additions & 0 deletions test/test_basic_logic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1523,3 +1523,32 @@ 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):
c.receive_data(f2.serialize())

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 •
edited
Loading

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.