Reject client enable-push settings#1318
Conversation
| ): | ||
| 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.
| 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.
| 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.
| """ | ||
| 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.
|
Thanks — addressed the review feedback in the latest commits. I replaced the raw setting code in the regression with No local tests were run; I only did static diff review / |
|
Pushed one follow-up commit ( No local tests were run, per my coordination-workspace policy; this was based on the remote failure and static review. |
|
Pushed one small wording follow-up (2ea6551) after re-reading the final diff against RFC 9113 §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. No local tests were run, per my coordination-workspace policy; this was a static wording-only review. |
2 participants
Summary
SETTINGS_ENABLE_PUSHvalues received from clientsWhy
RFC 7540 requires clients to be rejected if they attempt to change
SETTINGS_ENABLE_PUSHto a non-zero value. Issue #316 notes thath2did not enforce that rule for received SETTINGS frames.The implementation keeps this validation on the received-settings path rather than changing the broader
Settingsmutation semantics, so it closes the protocol gap without altering existing local-setting behavior.Validation