Repository: gorilla/websocket
Pull request #752 (2022-01-02 16:43:10)

Add check for Sec-WebSocket-Key header
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](https://datatracker.ietf.org/doc/html/rfc6455#section-4.2.1) states the following:
> A |Sec-WebSocket-Key| header field with a base64-encoded value that, when decoded, is 16 bytes in length.

garyburd: 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](https://github.com/gorilla/websocket/blob/2d6ee4c55cc9e9dc0eb5929f32a999213e25256f/util.go#L33).  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: @garyburd 
Thanks for the review! I checked the [ws(nodejs library)](https://github.com/websockets/ws), and it seems to return HTTP Status 400 if it doesn't match `/^[+/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

garyburd: 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](https://github.com/crossbario/autobahn-testsuite)  does not test invalid keys.
plasticsax: Simplify: `return err == nil && len(decoded) == 16`

hirasawayuki: @garyburd
I'll let you be the judge! 😃 

@plasticsax
Thankyou! I fixed it.(3ae5d9c)

Patch:
diff --git a/server.go b/server.go
index 24d53b3..bb33597 100644
--- a/server.go
+++ b/server.go
@@ -154,8 +154,8 @@ func (u *Upgrader) Upgrade(w http.ResponseWriter, r *http.Request, responseHeade
 	}
 
 	challengeKey := r.Header.Get("Sec-Websocket-Key")
-	if challengeKey == "" {
-		return u.returnError(w, r, http.StatusBadRequest, "websocket: not a websocket handshake: 'Sec-WebSocket-Key' header is missing or blank")
+	if !isValidChallengeKey(challengeKey) {
+		return u.returnError(w, r, http.StatusBadRequest, "websocket: not a websocket handshake: 'Sec-WebSocket-Key' header must be Base64 encoded value of 16-byte in length")
 	}
 
 	subprotocol := u.selectSubprotocol(r, responseHeader)
diff --git a/util.go b/util.go
index 7bf2f66..31a5dee 100644
--- a/util.go
+++ b/util.go
@@ -281,3 +281,18 @@ headers:
 	}
 	return result
 }
+
+// isValidChallengeKey checks if the argument meets RFC6455 specification.
+func isValidChallengeKey(s string) bool {
+	// From RFC6455:
+	//
+	// A |Sec-WebSocket-Key| header field with a base64-encoded (see
+	// Section 4 of [RFC4648]) value that, when decoded, is 16 bytes in
+	// length.
+
+	if s == "" {
+		return false
+	}
+	decoded, err := base64.StdEncoding.DecodeString(s)
+	return err == nil && len(decoded) == 16
+}


Test patch:
diff --git a/util_test.go b/util_test.go
index af710ba..f14d69a 100644
--- a/util_test.go
+++ b/util_test.go
@@ -53,6 +53,25 @@ func TestTokenListContainsValue(t *testing.T) {
 	}
 }
 
+var isValidChallengeKeyTests = []struct {
+	key string
+	ok  bool
+}{
+	{"dGhlIHNhbXBsZSBub25jZQ==", true},
+	{"", false},
+	{"InvalidKey", false},
+	{"WHQ4eXhscUtKYjBvOGN3WEdtOEQ=", false},
+}
+
+func TestIsValidChallengeKey(t *testing.T) {
+	for _, tt := range isValidChallengeKeyTests {
+		ok := isValidChallengeKey(tt.key)
+		if ok != tt.ok {
+			t.Errorf("isValidChallengeKey returns %v, want %v", ok, tt.ok)
+		}
+	}
+}
+
 var parseExtensionTests = []struct {
 	value      string
 	extensions []map[string]string