-
Notifications
You must be signed in to change notification settings - Fork 1
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
[pull] master from gorilla:master #2
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM
This fix addresses a potential denial-of-service (DoS) vector that can cause an integer overflow in the presence of malicious WebSocket frames. The fix adds additional checks against the remaining bytes on a connection, as well as a test to prevent regression. Credit to Max Justicz (https://justi.cz/) for discovering and reporting this, as well as providing a robust PoC and review. * build: go.mod to go1.12 * bugfix: fix DoS vector caused by readLimit bypass * test: update TestReadLimit sub-test * bugfix: payload length 127 should read bytes as uint64 * bugfix: defend against readLength overflows
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.
LGTM
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.
LGTM
Pull request analysis by VIZIPI Below you will find who is the most qualified team member to review your code. No other active qualified developers found to review these specific changes. You might consider involving more team members with these code segments. Potential missing files from this Pull requestfiles commonly committed with a subset of this pr, but not committed this time. (click to collapse)
Committed file ranks(click to expand)88.14% [README.md] 93.22% [client.go] 88.14% [client_server_test.go] 0.00% [.circleci/config.yml] 96.61% [conn.go] 86.44% [conn_test.go] 55.93% [server_test.go] 64.41% [util.go] 94.92% [server.go] 55.93% [util_test.go] |
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.
There are accessibility issues in these changes.
@@ -92,7 +92,7 @@ | |||
<div id="log"></div> | |||
<form id="form"> | |||
<input type="submit" value="Send" /> | |||
<input type="text" id="msg" size="64"/> | |||
<input type="text" id="msg" size="64" autofocus /> |
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.
Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.
Using empty struct for signaling is more idiomatic compared to booleans because users might wonder what happens on false or true. Empty struct removes this problem. There is also a side benefit of occupying less memory but it should be negligible in this case.
…ion: upgrade (#604) 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.
Manage this branch in SquashTest this branch here: https://gorillamaster-nhkf6.squash.io |
* Document allowed concurrency on Dialer. * Document allowed concurrency on Upgrader.
* do not use cached PreparedMessage in broadcast benchmarks * pick better name for benchmark method
- Update instructions to use docker. - Cleanup config file.
To aid protocol error debugging, report all errors found in the first two bytes of a message header.
- Note that a new maintainer is needed. - Remove comparison with x/net/websocket. There's no need to describe the issues with that package now that the package's documentation points people here and elsewhere.
Fixes issue: #745 With the previous interface, NetDial and NetDialContext were used for both TLS and non-TLS TCP connections, and afterwards TLSClientConfig was used to do the TLS handshake. While this API works for most cases, it prevents from using more advance authentication methods during the TLS handshake, as this is out of the control of the user. This commits introduces another a new dial method, NetDialTLSContext, which is used when dialing for TLS/TCP. The code then assumes that the handshake is done there and TLSClientConfig is not used. This API change is fully backwards compatible and it better aligns with net/http.Transport API, which has these two dial flavors. See: https://pkg.go.dev/net/http#Transport Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
* add Sec-WebSocket-Key header verification * add testcase to Sec-WebSocket-Key header verification
…s names with Go 1.18 standard library (#773)
* return an error when Dialer.TLSClientConfig.NextProtos contains a protocol that is not http/1.1 * include the likely cause of the error in the error message * check for nil-ness of Dialer.TLSClientConfig before attempting to run the check * addressing the review * move the NextProtos test into a separate file so that it can be run conditionally on go versions >= 1.14 * moving the new error check into existing http response error block to reduce the possibility of false positives * wrapping the error in %w * using %v instead of %w for compatibility with older versions of go * Revert "using %v instead of %w for compatibility with older versions of go" This reverts commit d34dd94. * move the unit test back into the existing test code since golang build constraint is no longer necessary Co-authored-by: Chan Kang <chankang@chankang17@gmail.com>
Signed-off-by: Ye Sijun <junnplus@gmail.com>
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
See Commits and Changes for more details.
Created by pull[bot]. Want to support this open source service? Please star it : )