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.

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

Fix how the client checks for presence of Upgrade: websocket, Connection: upgrade #604

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

bluetech
Copy link
Contributor

The values of the Upgrade and Connection response headers can
contain multiple tokens, for example

Connection: upgrade, keep-alive

The WebSocket RFC describes the checking of these as follows:

2. If the response lacks an |Upgrade| header field or the |Upgrade|
   header field contains a value that is not an ASCII case-
   insensitive match for the value "websocket", the client MUST
   _Fail the WebSocket Connection_.

3. If the response lacks a |Connection| header field or the
   |Connection| header field doesn't contain a token that is an
   ASCII case-insensitive match for the value "Upgrade", the client
   MUST _Fail the WebSocket Connection_.

It is careful to note "contains a value", "contains a token".

Previously, the client would reject with "bad handshake" if the header
doesn't contain exactly the value it looks for.

Change the checks to use tokenListContainsValue instead, which is
incidentally what the server is already doing for similar checks.

…ion: upgrade

The values of the `Upgrade` and `Connection` response headers can
contain multiple tokens, for example

    Connection: upgrade, keep-alive

The WebSocket RFC describes the checking of these as follows:

   2.  If the response lacks an |Upgrade| header field or the |Upgrade|
       header field contains a value that is not an ASCII case-
       insensitive match for the value "websocket", the client MUST
       _Fail the WebSocket Connection_.

   3.  If the response lacks a |Connection| header field or the
       |Connection| header field doesn't contain a token that is an
       ASCII case-insensitive match for the value "Upgrade", the client
       MUST _Fail the WebSocket Connection_.

It is careful to note "contains a value", "contains a token".

Previously, the client would reject with "bad handshake" if the header
doesn't contain exactly the value it looks for.

Change the checks to use `tokenListContainsValue` instead, which is
incidentally what the server is already doing for similar checks.
@bluetech
Copy link
Contributor Author

We ran into this issue where some MITM proxy adds Connection: keep-alive to our responses.

The test I added fails before and passes after.

@elithrar elithrar self-requested a review August 20, 2020 13:41
@elithrar elithrar self-assigned this Aug 20, 2020
@elithrar elithrar added the bug label Aug 20, 2020
Copy link
Contributor

@elithrar elithrar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, and for including the relevant part of the RFCs.

@elithrar elithrar merged commit 873e67e into gorilla:master Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants