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

Add check for Sec-WebSocket-Key header #752

Merged
merged 2 commits into from Feb 16, 2022

Conversation

hirasawayuki
Copy link
Contributor

@hirasawayuki hirasawayuki commented Jan 2, 2022

Fixes issue: #617

Added the following two fixes

  • Check that Sec-websocket-key is encoded as base64
  • Check that Sec-Websocket-Key is encoded from 16 bytes long

RFC6455 states the following:

A |Sec-WebSocket-Key| header field with a base64-encoded value that, when decoded, is 16 bytes in length.

@garyburd
Copy link
Contributor

garyburd commented Jan 2, 2022

Do popular libraries for other programming languages reject invalid keys? I don't want this package to the only library that rejects invalid keys.

Assuming that it's common practice to reject invalid keys, please move the logic to new function isValidChallengeKey(s string) bool { } in util.go. I want to keep the functionality related to keys in one file.

It's OK to return a single error message for invalid keys.

@hirasawayuki
Copy link
Contributor Author

hirasawayuki commented Jan 3, 2022

@garyburd
Thanks for the review! I checked the ws(nodejs library), and it seems to return HTTP Status 400 if it doesn't match regular expression of encoded 16-byte string. (/^[+/0-9A-Za-z]{22}==$/)
So I saw no problem in rejecting an invalid key.

https://github.com/websockets/ws/blob/5edf1f4a1b1750109c1bb56eff7ad78902eee7dc/lib/websocket-server.js#L18
https://github.com/websockets/ws/blob/5edf1f4a1b1750109c1bb56eff7ad78902eee7dc/lib/websocket-server.js#L236-L245

@hirasawayuki hirasawayuki changed the title [bug] Add check for Sec-WebSocket-Key header Add check for Sec-WebSocket-Key header Jan 3, 2022
@garyburd
Copy link
Contributor

garyburd commented Jan 4, 2022

LGTM.

The nodejs library provides strong evidence that the check is OK, but I want check more libraries before accepting the PR. One cause for concern is that the Autobahn|Testsuite does not test invalid keys.

@hirasawayuki
Copy link
Contributor Author

hirasawayuki commented Jan 4, 2022

@garyburd
I'll let you be the judge! 😃

@garyburd garyburd merged commit 69d0eb9 into gorilla:master Feb 16, 2022
@hirasawayuki hirasawayuki deleted the check-sec-websocket-key branch February 23, 2022 13:36
@andrewhodel
Copy link

There is a maximum limit of the incoming data per the base64 limit.

You should add an error from the upgrader if the size is exceeded for two reasons:

  1. The response isn't unknowingly handled in the module code like go's http 301 when there is a .. in the url path.

  2. The developer gets to decide how strict to be instead of a lower level structure that doesn't participate in these cost decisions.

@andrewhodel
Copy link

The base64 alphabet has a limit per byte with regards to representation size.

nono added a commit to cozy/cozy-stack that referenced this pull request Nov 6, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/gorilla/websocket](https://togithub.com/gorilla/websocket)
| require | patch | `v1.5.0` -> `v1.5.1` |

---

### Release Notes

<details>
<summary>gorilla/websocket (github.com/gorilla/websocket)</summary>

###
[`v1.5.1`](https://togithub.com/gorilla/websocket/releases/tag/v1.5.1)

[Compare
Source](https://togithub.com/gorilla/websocket/compare/v1.5.0...v1.5.1)

#### What's Changed

- Add check for Sec-WebSocket-Key header by
[@&#8203;hirasawayuki](https://togithub.com/hirasawayuki) in
[gorilla/websocket#752
- Changed the method name UnderlyingConn to NetConn by
[@&#8203;JWSong](https://togithub.com/JWSong) in
[gorilla/websocket#773
- remove all versions < 1.16 and add 1.18 by
[@&#8203;ChannyClaus](https://togithub.com/ChannyClaus) in
[gorilla/websocket#793
- Check for and report bad protocol in TLSClientConfig.NextProtos by
[@&#8203;ChannyClaus](https://togithub.com/ChannyClaus) in
[gorilla/websocket#788
- check err before GotConn for trace by
[@&#8203;junnplus](https://togithub.com/junnplus) in
[gorilla/websocket#798
- Update README.md by
[@&#8203;coreydaley](https://togithub.com/coreydaley) in
[gorilla/websocket#839
- Correct way to save memory using write buffer pool and freeing
net.http default buffers by [@&#8203;FMLS](https://togithub.com/FMLS) in
[gorilla/websocket#761
- Update go version & add verification/testing tools by
[@&#8203;coreydaley](https://togithub.com/coreydaley) in
[gorilla/websocket#840
- update golang.org/x/net by
[@&#8203;coreydaley](https://togithub.com/coreydaley) in
[gorilla/websocket#856
- update GitHub workflows by
[@&#8203;coreydaley](https://togithub.com/coreydaley) in
[gorilla/websocket#857

#### New Contributors

- [@&#8203;hirasawayuki](https://togithub.com/hirasawayuki) made their
first contribution in
[gorilla/websocket#752
- [@&#8203;JWSong](https://togithub.com/JWSong) made their first
contribution in
[gorilla/websocket#773
- [@&#8203;ChannyClaus](https://togithub.com/ChannyClaus) made their
first contribution in
[gorilla/websocket#793
- [@&#8203;junnplus](https://togithub.com/junnplus) made their first
contribution in
[gorilla/websocket#798
- [@&#8203;coreydaley](https://togithub.com/coreydaley) made their first
contribution in
[gorilla/websocket#839
- [@&#8203;FMLS](https://togithub.com/FMLS) made their first
contribution in
[gorilla/websocket#761

**Full Changelog**:
gorilla/websocket@v1.5.0...v1.5.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone
Europe/Paris, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/cozy/cozy-stack).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->
algitbot pushed a commit to alpinelinux/build-server-status that referenced this pull request May 5, 2024
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/gorilla/websocket](https://github.com/gorilla/websocket) | require | patch | `v1.5.0` -> `v1.5.1` |

---

### Release Notes

<details>
<summary>gorilla/websocket (github.com/gorilla/websocket)</summary>

### [`v1.5.1`](https://github.com/gorilla/websocket/releases/tag/v1.5.1)

[Compare Source](gorilla/websocket@v1.5.0...v1.5.1)

#### What's Changed

-   Add check for Sec-WebSocket-Key header by [@&#8203;hirasawayuki](https://github.com/hirasawayuki) in gorilla/websocket#752
-   Changed the method name UnderlyingConn to NetConn by [@&#8203;JWSong](https://github.com/JWSong) in gorilla/websocket#773
-   remove all versions < 1.16 and add 1.18 by [@&#8203;ChannyClaus](https://github.com/ChannyClaus) in gorilla/websocket#793
-   Check for and report bad protocol in TLSClientConfig.NextProtos by [@&#8203;ChannyClaus](https://github.com/ChannyClaus) in gorilla/websocket#788
-   check err before GotConn for trace by [@&#8203;junnplus](https://github.com/junnplus) in gorilla/websocket#798
-   Update README.md by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#839
-   Correct way to save memory using write buffer pool and freeing net.http default buffers by [@&#8203;FMLS](https://github.com/FMLS) in gorilla/websocket#761
-   Update go version & add verification/testing tools by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#840
-   update golang.org/x/net by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#856
-   update GitHub workflows by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#857

#### New Contributors

-   [@&#8203;hirasawayuki](https://github.com/hirasawayuki) made their first contribution in gorilla/websocket#752
-   [@&#8203;JWSong](https://github.com/JWSong) made their first contribution in gorilla/websocket#773
-   [@&#8203;ChannyClaus](https://github.com/ChannyClaus) made their first contribution in gorilla/websocket#793
-   [@&#8203;junnplus](https://github.com/junnplus) made their first contribution in gorilla/websocket#798
-   [@&#8203;coreydaley](https://github.com/coreydaley) made their first contribution in gorilla/websocket#839
-   [@&#8203;FMLS](https://github.com/FMLS) made their first contribution in gorilla/websocket#761

**Full Changelog**: gorilla/websocket@v1.5.0...v1.5.1

</details>

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

&nbsp;
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yODYuMSIsInVwZGF0ZWRJblZlciI6IjM3LjI4Ni4xIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbXX0=-->

See merge request alpine/infra/build-server-status!9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants