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

[pull] master from gorilla:master #2

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Conversation

pull[bot]
Copy link

@pull pull bot commented Aug 23, 2019

See Commits and Changes for more details.


Created by pull[bot]. Want to support this open source service? Please star it : )

@pull pull bot added the ⤵️ pull label Aug 23, 2019
Copy link

@codelingo codelingo bot left a 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
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

dbaker-arch and others added 2 commits March 19, 2020 06:46
Fixes a couple of small typo's in the example test docs.
@vizipi
Copy link

vizipi bot commented Mar 19, 2020

Pull request analysis by VIZIPI

Below you will find who is the most qualified team member to review your code.
This analysis includes his/her work on the code included in this Pull request, in addition to their experience in code affected by these changes ( partly found within the list of potential missing files below )   Feedback always welcome

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 request

files commonly committed with a subset of this pr, but not committed this time. (click to collapse)
FilePercentilerate
examples/echo/README.md50.00 %1 out of 2 times
json.go50.00 %1 out of 2 times
x_net_proxy.go50.00 %1 out of 2 times

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]
  • Copy link

    @accesslint accesslint bot left a 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 />
    Copy link

    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.

    ferhatelmas and others added 6 commits March 19, 2020 06:52
    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.
    @squash-labs
    Copy link

    squash-labs bot commented Apr 24, 2021

    Manage this branch in Squash

    Test this branch here: https://gorillamaster-nhkf6.squash.io

    * Document allowed concurrency on Dialer.
    * Document allowed concurrency on Upgrader.
    @pull-request-size pull-request-size bot added size/XL and removed size/L labels Jan 1, 2022
    Alexander Emelin and others added 2 commits January 2, 2022 07:35
    * do not use cached PreparedMessage in broadcast benchmarks
    
    * pick better name for benchmark method
    - Update instructions to use docker.
    - Cleanup config file.
    garyburd and others added 4 commits January 2, 2022 12:16
    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>
    hirasawayuki and others added 6 commits February 15, 2022 17:15
    * add Sec-WebSocket-Key header verification
    
    * add testcase to Sec-WebSocket-Key header verification
    * 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>
    @pull-request-quantifier-deprecated

    This PR has 751 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!


    Quantification details

    Label      : Extra Large
    Size       : +481 -270
    Percentile : 91.7%
    
    Total files changed: 38
    
    Change summary by file extension:
    .yml : +67 -61
    .md : +11 -31
    .go : +384 -168
    .json : +17 -7
    .html : +1 -1
    .mod : +1 -0
    .sum : +0 -2
    

    Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

    Why proper sizing of changes matters

    Optimal pull request sizes drive a better predictable PR flow as they strike a
    balance between between PR complexity and PR review overhead. PRs within the
    optimal size (typical small, or medium sized PRs) mean:

    • Fast and predictable releases to production:
      • Optimal size changes are more likely to be reviewed faster with fewer
        iterations.
      • Similarity in low PR complexity drives similar review times.
    • Review quality is likely higher as complexity is lower:
      • Bugs are more likely to be detected.
      • Code inconsistencies are more likely to be detected.
    • Knowledge sharing is improved within the participants:
      • Small portions can be assimilated better.
    • Better engineering practices are exercised:
      • Solving big problems by dividing them in well contained, smaller problems.
      • Exercising separation of concerns within the code changes.

    What can I do to optimize my changes

    • Use the PullRequestQuantifier to quantify your PR accurately
      • Create a context profile for your repo using the context generator
      • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
      • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
      • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
    • Change your engineering behaviors
      • For PRs that fall outside of the desired spectrum, review the details and check if:
        • Your PR could be split in smaller, self-contained PRs instead
        • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

    How to interpret the change counts in git diff output

    • One line was added: +1 -0
    • One line was deleted: +0 -1
    • One line was modified: +1 -1 (git diff doesn't know about modified, it will
      interpret that line like one addition plus one deletion)
    • Change percentiles: Change characteristics (addition, deletion, modification)
      of this PR in relation to all other PRs within the repository.


    Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
    Customize PullRequestQuantifier for this repository.

    @coreydaley coreydaley deleted the branch reedhhw:master July 10, 2023 18:12
    @coreydaley coreydaley deleted the master branch July 10, 2023 18:12
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.