Check for and report bad protocol in TLSClientConfig.NextProtos

[ChannyClaus]

Fixes #760

Summary of Changes

1. 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]

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

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

[ChannyClaus]

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

[garyburd]

Thank you for working on this.

The PR is what I was looking for when I wrote the issue, but the comment about false positives got me thinking. To avoid breaking code that happens to work today, move the test inside this if block. Return fmt.Errorf("<error text from PR>: %w", proto, err).

[ChannyClaus]

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

[garyburd]

Please wrap the original error with the fmt %w verb.

[ChannyClaus]

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"

[garyburd]

The %v verb is appropriate in the other examples because those examples format the error as text.

This PR creates a new error value. The %w verb should be used to wrap the original error with the new error.

Somebody should submit a separate PR to remove Go versions < 1.13 from the CircleCi config. I made an attempt at that and failed. I don't have time to continue. We can proceed here once that PR is merged.

[amustaque97]

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?

[ChannyClaus]

• 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)