Receiving a PUSH_PROMISE from a client is an error #315
Conversation
This is explicitly forbidden in the spec (RFC 7540 § 8.2). In practice hyper-h2 was already raising a ProtocolError, because local_settings.enable_push is False for servers. This commit: * Tweaks the error message to make it clear that receiving it from the client is important. * Add an explicit test case.
| expected_frame = frame_factory.build_goaway_frame( | ||
| 1, h2.errors.ErrorCodes.PROTOCOL_ERROR | ||
| ) | ||
| assert c.data_to_send() == expected_frame.serialize() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The test now checks the exception message.
| # 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") |
There was a problem hiding this comment.
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.
|
Closing due to inactivity (from me). |
This is explicitly forbidden in the spec (RFC 7540 § 8.2).
In practice hyper-h2 was already raising a ProtocolError, because
local_settings.enable_push is False for servers. This commit:
the client is important.