Check for and report bad protocol in TLSClientConfig.NextProtos by ChannyClaus · Pull Request #788 · gorilla/websocket

ChannyClaus · 2022-06-12T04:44:27Z

Fixes #760

Summary of Changes

Adds an explicit check for unsupported protocol (seems to be only HTTP/1.1 for the time being as shown here)

websocket/client.go

Line 191 in 78cf1bc

Proto: "HTTP/1.1",

and returns an error more helpful for application developer.

PS: Make sure your PR includes/updates tests! If you need help with this part, just ask!

did run the unit test included in the issue and got

$ go test -run TestNextProto -v
=== RUN   TestNextProto
    client_server_test.go:1125: Dial: protocol "h2" was given but is not supported;
                                        sharing tls.Config with net/http Transport can cause this error
--- FAIL: TestNextProto (0.00s)
FAIL
exit status 1
FAIL    github.com/gorilla/websocket    0.176s

which is expected; can add this or some other form of unit test to assert on the error message if desired (seemed somewhat unnatural to assert on the error message, hence the omission)

amustaque97

@ChannyClaus thank you for taking this forward. Changes look good to me.

cc @garyburd

ghost · 2022-06-12T21:14:30Z

Testing for the known bad case "h2" reduces the likelihood of a false positive.
Error strings with embedded newlines are not conventional in Go. Use string concatenation to construct a single line error string on multiple source lines.
Many of the error strings in this package have the prefix "websocket:". This one should as well.
There should be a test that ensures that an error is returned when configs are shared with net/http.

ChannyClaus · 2022-06-14T02:11:41Z

client_server_http2_test.go

+// license that can be found in the LICENSE file.
+
+//go:build go1.14
+// +build go1.14


this was necessary because https://github.com/gorilla/websocket/pull/788/files#diff-647b905ba2feaa4a134811cd904e1fd3af5d28129e9b9031cfa4fc127f5c55fdR20 was not present prior to go version 1.14

ChannyClaus · 2022-06-14T02:12:42Z

thanks for the quick turnaround @amustaque97 @easterf - made the suggested changes, let me know if there's anything else!

ChannyClaus · 2022-06-19T19:09:36Z

@garyburd done! can also move the check into a separate function if need be (the indenting gets a little crazy now)

ChannyClaus · 2022-06-19T19:20:35Z

seems like using %w breaks compatibility with older versions of go + the rest of the code base seems to be using %v for formatting the error object:

$ git grep -rF "%v" | head
client.go:							"sharing tls.Config with net/http Transport can cause this error: %v",
client_server_http2_test.go:		t.Fatalf("Get: %v", err)
client_server_test.go:		t.Logf("path=%v, want %v", r.URL.Path, cstPath)
client_server_test.go:		t.Logf("query=%v, want %v", r.URL.RawQuery, cstRawQuery)
client_server_test.go:		t.Logf("subprotols=%v, want %v", subprotos, cstDialer.Subprotocols)
client_server_test.go:		t.Logf("Upgrade: %v", err)
client_server_test.go:		t.Logf("NextReader: %v", err)
client_server_test.go:		t.Logf("NextWriter: %v", err)
client_server_test.go:		t.Logf("NextWriter: %v", err)
client_server_test.go:		t.Logf("Close: %v", err)
chankang@Chans-MBP ~/websocket -  (master)
$ git grep -rF "%w"

amustaque97 · 2022-06-19T20:32:12Z

I’m checking on Circle CI from past few days because of some issues I’m facing in other project gorilla mux. CI doesn’t trigger all the time if we create branch n PR at the same time. Here is the link of same issue mentioned by Circle CI https://support.circleci.com/hc/en-us/articles/360008097173-Why-aren-t-pull-requests-triggering-jobs-on-my-organization-

If we all agree we can move from circle ci to GitHub actions else I can raise a PR to remove golang versions in circle ci.

Let me know what do you think?

…tocol that is not http/1.1

… the check

…onditionally on go versions >= 1.14

… reduce the possibility of false positives

…of go" This reverts commit d34dd94.

…d constraint is no longer necessary

ChannyClaus · 2022-06-21T00:41:40Z

rebase-pulled the change from remove all versions < 1.16 and add 1.18 #793
swapped in %w per Check for and report bad protocol in TLSClientConfig.NextProtos #788 (comment)
moved the unit test back into the existing file since golang build constraint is no longer necessary (since the test does not need to pass for go versions < 1.16 after the PR 793)

amustaque97 approved these changes Jun 12, 2022

View reviewed changes

ChannyClaus commented Jun 14, 2022

View reviewed changes

ChannyClaus force-pushed the master branch from 75b93bb to d210a40 Compare June 19, 2022 19:21

ghost mentioned this pull request Jun 19, 2022

Update Go versions used in CI #792

Closed

Chan Kang added 9 commits June 20, 2022 20:36

return an error when Dialer.TLSClientConfig.NextProtos contains a pro…

8a0b8d4

…tocol that is not http/1.1

include the likely cause of the error in the error message

464f255

check for nil-ness of Dialer.TLSClientConfig before attempting to run…

a410921

… the check

addressing the review

1da1d61

move the NextProtos test into a separate file so that it can be run c…

b67992c

…onditionally on go versions >= 1.14

moving the new error check into existing http response error block to…

fc45505

… reduce the possibility of false positives

wrapping the error in %w

d926999

using %v instead of %w for compatibility with older versions of go

d34dd94

Revert "using %v instead of %w for compatibility with older versions …

0cd10fc

…of go" This reverts commit d34dd94.

ChannyClaus force-pushed the master branch from d210a40 to 0cd10fc Compare June 21, 2022 00:37

move the unit test back into the existing test code since golang buil…

b7f4791

…d constraint is no longer necessary

ghost merged commit bc7ce89 into gorilla:master Jun 21, 2022

ghost mentioned this pull request Aug 4, 2024

Restore trust in the project #923

Closed

1 task

yashuatla mentioned this pull request Jun 25, 2025

Feature implementation from commits e8629af..af47554 yashuatla/websocket#2

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for and report bad protocol in TLSClientConfig.NextProtos#788

Check for and report bad protocol in TLSClientConfig.NextProtos#788
10 commits merged intogorilla:masterfrom
ChannyClaus:master

ChannyClaus commented Jun 12, 2022 •

edited

Loading

Uh oh!

amustaque97 left a comment

Uh oh!

ghost commented Jun 12, 2022

Uh oh!

ChannyClaus Jun 14, 2022

Uh oh!

ChannyClaus commented Jun 14, 2022

Uh oh!

ChannyClaus commented Jun 19, 2022

Uh oh!

ChannyClaus commented Jun 19, 2022

Uh oh!

amustaque97 commented Jun 19, 2022

Uh oh!

ChannyClaus commented Jun 21, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ChannyClaus commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amustaque97 left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 12, 2022

Uh oh!

ChannyClaus Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

ChannyClaus commented Jun 14, 2022

Uh oh!

ChannyClaus commented Jun 19, 2022

Uh oh!

ChannyClaus commented Jun 19, 2022

Uh oh!

amustaque97 commented Jun 19, 2022

Uh oh!

ChannyClaus commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChannyClaus commented Jun 12, 2022 •

edited

Loading

ChannyClaus commented Jun 21, 2022 •

edited

Loading