Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/ws over h2 #220
base: develop
Are you sure you want to change the base?
Feature/ws over h2 #220
Changes from all commits
054175a
b07d7e3
9c38a02
eb4c7a9
d1965a2
3d67e7b
d60189c
3e34b9b
8af6a12
d026b3c
a4a6b7d
cab5a94
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are just tracking the presence and count of the pseudo-headers, so they can be checked in
decodeHeaders
. So we likely need to add a count forprotocol
and include in the check.Note we are validating the pseudo-headers themselves, not the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I misread this part of the spec in
rfc8441
:The :protocol pseudo-header field MUST be included in the CONNECT request, and it MUST have a value of websocket to initiate a WebSocket connection on an HTTP/2 stream
I now think it means that if we have a CONNECT request, before we create a websocket connection we must ensure that the request also contains the
:protocol
pseudo-header and it must have the valuewebsocket
.Maybe we don't need to do any validation in the http2 binding in relation to a connect request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we find a better place for it, I have added a check to allow a
:protocol
pseudo-header in a CONNECT request which is otherwise not allowed byrfc7540
. This seems to make sense to be in the http2 binding because the binding is always advertising theENABLE_CONNECT_PROTOCOL
setting.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, just make sure you are doing the simple count here, and the enforcement in
decodeHeaders
, aligned with approach for other checks.