Reject client enable-push settings - #1318
Conversation
Sorry, something went wrong.
| ): | ||
| invalid = ErrorCodes.PROTOCOL_ERROR | ||
|
|
||
| if invalid: |
There was a problem hiding this comment.
_validate_setting returns an ErrorCodes and should be explicitly compared against ErrorCodes.NO_ERROR.
Sorry, something went wrong.
| and self._client | ||
| and setting == SettingCodes.ENABLE_PUSH | ||
| and value != 0 | ||
| ): |
There was a problem hiding this comment.
This check should probably live in the _validate_setting function.
Sorry, something went wrong.
| c.initiate_connection() | ||
| c.receive_data(frame_factory.preamble()) | ||
|
|
||
| f = frame_factory.build_settings_frame(settings={0x2: 1}) |
There was a problem hiding this comment.
use SettingCodes.ENABLE_PUSH instead of 0x2.
Sorry, something went wrong.
| """ | ||
| if setting == SettingCodes.ENABLE_PUSH: | ||
| if value not in (0, 1): | ||
| if value not in (0, 1) or (client and value != 0): |
There was a problem hiding this comment.
add a quote of https://datatracker.ietf.org/doc/html/rfc9113#name-defined-settings
section 6.5.2
A client MUST treat receipt of a SETTINGS frame with SETTINGS_ENABLE_PUSH set to 1 as a connection error (Section 5.4.1) of type PROTOCOL_ERROR.
Sorry, something went wrong.
|
Thanks — addressed the review feedback in the latest commits. I replaced the raw setting code in the regression with h2.settings.SettingCodes.ENABLE_PUSH, added the RFC 9113 §6.5.2 quote near the validation, and corrected the regression to model a client receiving the server setting so it matches the quoted rule. No local tests were run; I only did static diff review / git diff --check here. |
Sorry, something went wrong.
|
Pushed one follow-up commit (b9520e8) after reading the remote tox (3.14) failure. The previous revision applied the role-specific ENABLE_PUSH rule to local Settings(...) construction and setters as well; this now keeps ordinary setting validation to the generic 0 / 1 rule and applies the server-peer restriction only to received settings. Not run locally; this was based on the remote failure and static review. |
Sorry, something went wrong.
|
Pushed one small wording follow-up (2ea6551) after re-reading the final diff against RFC 9113 section 6.5.2. The code was already applying the role-specific rule to a server peer, but the changelog and helper docstring still said clients; those now say servers to match the tested case and the quoted RFC text. Not run locally; this was a static wording-only review. |
Sorry, something went wrong.
Summary
Why
RFC 7540 requires clients to be rejected if they attempt to change SETTINGS_ENABLE_PUSH to a non-zero value. Issue #316 notes that h2 did not enforce that rule for received SETTINGS frames.
The implementation keeps this validation on the received-settings path rather than changing the broader Settings mutation semantics, so it closes the protocol gap without altering existing local-setting behavior.
Validation