From 081f4837f73c9bd4e0fa4227932f10367c172337 Mon Sep 17 00:00:00 2001 From: wbt Date: Mon, 24 Aug 2020 16:43:18 -0400 Subject: [PATCH 1/2] Only compare hostnames in ws.origins Also using a helper function for ToLower consolidates all preparation steps in one function for more maintainable consistency. Spaces => tabs Remove a semicolon Add space at start of comment Remove parens around conditional Handle case wehre parsed hostname is empty When passing a single word like "localhost" the parsed hostname is an empty string. Handle this and the error-parsing case together as default, and the nonempty hostname case in the conditional. Refactor with new originIsAllowed functions Adds originIsAllowed() & ruleAllowsOrigin(); removes prepOriginForComparison Remove blank line Added tests for simple allowed-orign rule which does not specify a protocol or port, just a hostname Fix copy-paste: `:=` => `=` Remove parens around conditional Remove autoadded whitespace on blank lines Compare scheme, hostname, and port with rule if the rule specifies those portions. Remove one autoadded trailing whitespace Better handle case where only origin host is given e.g. "localhost" Remove parens around conditional Refactor: attemptWebsocketConnectionFromOrigin DRY Include return type on helper function Provide srv obj in helper fn Provide srv to helper fn Remove stray underscore Remove blank line parent 93e666b4c1e7e49b8406dc83ed93f4a02ea49ac1 author wbt 1598559718 -0400 committer Martin Holst Swende 1605602257 +0100 gpgsig -----BEGIN PGP SIGNATURE----- iQFFBAABCAAvFiEEypmrtbNuJK1doP1AaDtDjAWl3fAFAl+zi9ARHG1hcnRpbkBz d2VuZGUuc2UACgkQaDtDjAWl3fDRiwgAoMtzU8dwRV7Q9xkCwWEx9Wz2f3n6jUr2 VWBycDKGKwRkPPOER3oc9kzjGU/P1tFlK07PjfnAKZ9KWzxpDcJZwYM3xCBurG7A 16y4YsQnzgPNONv3xIkdi3RZtDBIiPFFEmdZFFvZ/jKexfI6JIYPngCAoqdTIFb9 On/aPvvVWQn1ExfmarsvvJ7kUDUG77tZipuacEH5FfFsfelBWOEYPe+I9ToUHskv +qO6rOkV1Ojk8eBc6o0R1PnApwCAlEhJs7aM/SEOg4B4ZJJneiFuEXBIG9+0yS2I NOicuDPLGucOB5nBsfIKI3USPeE+3jxdT8go2lN5Nrhm6MimoILDsQ== =sgUp -----END PGP SIGNATURE----- Refactor: drop err var for more concise test lines Add several tests for new WebSocket origin checks Remove autoadded whitespace on blank lines Restore TestWebsocketOrigins originally-named test and rename the others to be helpers rather than full tests Remove autoadded whitespace on blank line Temporarily comment out new test sets Uncomment test around origin rule with scheme Remove tests without scheme on browser origin per https://github.com/ethereum/go-ethereum/pull/21481/files#r479371498 Uncomment tests with port; remove some blank lines Handle when browser does not specify scheme/port Uncomment test for including scheme & port in rule Add IP tests --- node/rpcstack_test.go | 121 +++++++++++++++++++++++++++++++++++++----- rpc/websocket.go | 56 +++++++++++++++++-- 2 files changed, 161 insertions(+), 16 deletions(-) diff --git a/node/rpcstack_test.go b/node/rpcstack_test.go index 0ee120efd7aa..688b08cca14b 100644 --- a/node/rpcstack_test.go +++ b/node/rpcstack_test.go @@ -54,23 +54,107 @@ func TestVhosts(t *testing.T) { // TestWebsocketOrigins makes sure the websocket origins are properly handled on the websocket server. func TestWebsocketOrigins(t *testing.T) { + tryWebsocketOriginsSimpleRule(t) + tryWebsocketOriginsRuleWithScheme(t) + tryWebsocketOriginsIPRuleWithScheme(t) + tryWebsocketOriginsRuleWithPort(t) + tryWebsocketOriginsRuleWithSchemeAndPort(t) +} + +func tryWebsocketOriginsSimpleRule(t *testing.T) { srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"test"}}) defer srv.stop() - dialer := websocket.DefaultDialer - _, _, err := dialer.Dial("ws://"+srv.listenAddr(), http.Header{ - "Content-type": []string{"application/json"}, - "Sec-WebSocket-Version": []string{"13"}, - "Origin": []string{"test"}, - }) - assert.NoError(t, err) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) - _, _, err = dialer.Dial("ws://"+srv.listenAddr(), http.Header{ - "Content-type": []string{"application/json"}, - "Sec-WebSocket-Version": []string{"13"}, - "Origin": []string{"bad"}, - }) - assert.Error(t, err) + // Host mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) +} + +func tryWebsocketOriginsRuleWithScheme(t *testing.T) { + srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"https://test"}}) + defer srv.stop() + + // Scheme mismatch: + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) + + // Scheme mismatch: + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) + + // Host mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) +} + +func tryWebsocketOriginsIPRuleWithScheme(t *testing.T) { + srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"https://12.34.56.78"}}) + defer srv.stop() + + // Scheme mismatch: + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://12.34.56.78")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://12.34.56.78")) + + // Scheme mismatch: + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://12.34.56.78:8540")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://12.34.56.78:8540")) + + // Host mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://87.65.43.21")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://87.65.43.21")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://87.65.43.21:8540")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://87.65.43.21:8540")) +} + +func tryWebsocketOriginsRuleWithPort(t *testing.T) { + srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"test:8540"}}) + defer srv.stop() + + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) + + // Port mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8541")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8541")) + + // Host mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) +} + +func tryWebsocketOriginsRuleWithSchemeAndPort(t *testing.T) { + srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"https://test:8540"}}) + defer srv.stop() + + // Scheme mismatch: + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) + // Scheme mismatch: + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) + assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) + + // Port mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8541")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8541")) + + // Host mismatch (set): + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) + assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) } // TestIsWebsocket tests if an incoming websocket upgrade request is handled properly. @@ -103,6 +187,17 @@ func createAndStartServer(t *testing.T, conf httpConfig, ws bool, wsConf wsConfi return srv } +func attemptWebsocketConnectionFromOrigin(t *testing.T, srv *httpServer, browserOrigin string) error { + t.Helper() + dialer := websocket.DefaultDialer + _, _, err := dialer.Dial("ws://"+srv.listenAddr(), http.Header{ + "Content-type": []string{"application/json"}, + "Sec-WebSocket-Version": []string{"13"}, + "Origin": []string{browserOrigin}, + }) + return err +} + func testRequest(t *testing.T, key, value, host string, srv *httpServer) *http.Response { t.Helper() diff --git a/rpc/websocket.go b/rpc/websocket.go index a716383be91a..de62798b46cb 100644 --- a/rpc/websocket.go +++ b/rpc/websocket.go @@ -75,14 +75,14 @@ func wsHandshakeValidator(allowedOrigins []string) func(*http.Request) bool { allowAllOrigins = true } if origin != "" { - origins.Add(strings.ToLower(origin)) + origins.Add(origin) } } // allow localhost if no allowedOrigins are specified. if len(origins.ToSlice()) == 0 { origins.Add("http://localhost") if hostname, err := os.Hostname(); err == nil { - origins.Add("http://" + strings.ToLower(hostname)) + origins.Add("http://" + hostname) } } log.Debug(fmt.Sprintf("Allowed origin(s) for WS RPC interface %v", origins.ToSlice())) @@ -97,7 +97,7 @@ func wsHandshakeValidator(allowedOrigins []string) func(*http.Request) bool { } // Verify origin against whitelist. origin := strings.ToLower(req.Header.Get("Origin")) - if allowAllOrigins || origins.Contains(origin) { + if allowAllOrigins || originIsAllowed(origins, origin) { return true } log.Warn("Rejected WebSocket connection", "origin", origin) @@ -120,6 +120,56 @@ func (e wsHandshakeError) Error() string { return s } +func originIsAllowed(allowedOrigins mapset.Set, browserOrigin string) bool { + it := allowedOrigins.Iterator() + for origin := range it.C { + if ruleAllowsOrigin(origin.(string), browserOrigin) { + return true + } + } + return false +} + +func ruleAllowsOrigin(allowedOrigin string, browserOrigin string) bool { + allowedScheme, allowedHostname, allowedPort, parsedAllowedErr := parseOriginURL(allowedOrigin) + browserScheme, browserHostname, browserPort, browserOriginErr := parseOriginURL(browserOrigin) + if parsedAllowedErr == nil && browserOriginErr == nil { + if allowedScheme != "" && browserScheme != "" && allowedScheme != browserScheme { + return false + } + if allowedHostname != "" && allowedHostname != browserHostname { + return false + } + if allowedPort != "" && browserPort != "" && allowedPort != browserPort { + return false + } + return true + } else { // parse error + log.Warn("Error parsing Origin URL in comparison for match.", "allowedOrigin", allowedOrigin, "browserOrigin", browserOrigin) + return false + } +} + +func parseOriginURL(origin string) (string, string, string, error) { + parsedURL, parseError := url.Parse(strings.ToLower(origin)) + var scheme, hostname, port string + if parseError == nil { + if strings.Contains(origin, "://") { + scheme = parsedURL.Scheme + hostname = parsedURL.Hostname() + port = parsedURL.Port() + } else { + scheme = "" + hostname = parsedURL.Scheme + port = parsedURL.Opaque + if hostname == "" { + hostname = origin + } + } + } + return scheme, hostname, port, parseError +} + // DialWebsocketWithDialer creates a new RPC client that communicates with a JSON-RPC server // that is listening on the given endpoint using the provided dialer. func DialWebsocketWithDialer(ctx context.Context, endpoint, origin string, dialer websocket.Dialer) (*Client, error) { From 8430f92e883a4b446ad2f8b58c3e1fb05abc62ed Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Fri, 11 Sep 2020 10:57:28 +0200 Subject: [PATCH 2/2] node: more tests + table-driven, ws origin changes --- node/rpcstack_test.go | 194 +++++++++++++++++++++--------------------- rpc/websocket.go | 67 ++++++++------- 2 files changed, 133 insertions(+), 128 deletions(-) diff --git a/node/rpcstack_test.go b/node/rpcstack_test.go index 688b08cca14b..8267fb2f1ddb 100644 --- a/node/rpcstack_test.go +++ b/node/rpcstack_test.go @@ -19,6 +19,7 @@ package node import ( "bytes" "net/http" + "strings" "testing" "github.com/ethereum/go-ethereum/internal/testlog" @@ -52,109 +53,104 @@ func TestVhosts(t *testing.T) { assert.Equal(t, resp2.StatusCode, http.StatusForbidden) } -// TestWebsocketOrigins makes sure the websocket origins are properly handled on the websocket server. -func TestWebsocketOrigins(t *testing.T) { - tryWebsocketOriginsSimpleRule(t) - tryWebsocketOriginsRuleWithScheme(t) - tryWebsocketOriginsIPRuleWithScheme(t) - tryWebsocketOriginsRuleWithPort(t) - tryWebsocketOriginsRuleWithSchemeAndPort(t) -} - -func tryWebsocketOriginsSimpleRule(t *testing.T) { - srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"test"}}) - defer srv.stop() - - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) - - // Host mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) -} - -func tryWebsocketOriginsRuleWithScheme(t *testing.T) { - srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"https://test"}}) - defer srv.stop() - - // Scheme mismatch: - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) - - // Scheme mismatch: - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) - - // Host mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) -} - -func tryWebsocketOriginsIPRuleWithScheme(t *testing.T) { - srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"https://12.34.56.78"}}) - defer srv.stop() - - // Scheme mismatch: - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://12.34.56.78")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://12.34.56.78")) - - // Scheme mismatch: - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://12.34.56.78:8540")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://12.34.56.78:8540")) - - // Host mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://87.65.43.21")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://87.65.43.21")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://87.65.43.21:8540")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://87.65.43.21:8540")) +type originTest struct { + spec string + expOk []string + expFail []string } -func tryWebsocketOriginsRuleWithPort(t *testing.T) { - srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"test:8540"}}) - defer srv.stop() - - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) - - // Port mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8541")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8541")) - - // Host mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) +// splitAndTrim splits input separated by a comma +// and trims excessive white space from the substrings. +// Copied over from flags.go +func splitAndTrim(input string) (ret []string) { + l := strings.Split(input, ",") + for _, r := range l { + r = strings.TrimSpace(r) + if len(r) > 0 { + ret = append(ret, r) + } + } + return ret } -func tryWebsocketOriginsRuleWithSchemeAndPort(t *testing.T) { - srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"https://test:8540"}}) - defer srv.stop() - - // Scheme mismatch: - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test")) - // Scheme mismatch: - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8540")) - assert.NoError(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8540")) - - // Port mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://test:8541")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://test:8541")) - - // Host mismatch (set): - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "http://bad:8540")) - assert.Error(t, attemptWebsocketConnectionFromOrigin(t, srv, "https://bad:8540")) +// TestWebsocketOrigins makes sure the websocket origins are properly handled on the websocket server. +func TestWebsocketOrigins(t *testing.T) { + tests := []originTest{ + { + spec: "*", // allow all + expOk: []string{"", "http://test", "https://test", "http://test:8540", "https://test:8540", + "http://test.com", "https://foo.test", "http://testa", "http://atestb:8540", "https://atestb:8540"}, + }, + { + spec: "test", + expOk: []string{"http://test", "https://test", "http://test:8540", "https://test:8540"}, + expFail: []string{"http://test.com", "https://foo.test", "http://testa", "http://atestb:8540", "https://atestb:8540"}, + }, + // scheme tests + { + spec: "https://test", + expOk: []string{"https://test", "https://test:9999"}, + expFail: []string{ + "test", // no scheme, required by spec + "http://test", // wrong scheme + "http://test.foo", "https://a.test.x", // subdomain variatoins + "http://testx:8540", "https://xtest:8540"}, + }, + // ip tests + { + spec: "https://12.34.56.78", + expOk: []string{"https://12.34.56.78", "https://12.34.56.78:8540"}, + expFail: []string{ + "http://12.34.56.78", // wrong scheme + "http://12.34.56.78:443", // wrong scheme + "http://1.12.34.56.78", // wrong 'domain name' + "http://12.34.56.78.a", // wrong 'domain name' + "https://87.65.43.21", "http://87.65.43.21:8540", "https://87.65.43.21:8540"}, + }, + // port tests + { + spec: "test:8540", + expOk: []string{"http://test:8540", "https://test:8540"}, + expFail: []string{ + "http://test", "https://test", // spec says port required + "http://test:8541", "https://test:8541", // wrong port + "http://bad", "https://bad", "http://bad:8540", "https://bad:8540"}, + }, + // scheme and port + { + spec: "https://test:8540", + expOk: []string{"https://test:8540"}, + expFail: []string{ + "https://test", // missing port + "http://test", // missing port, + wrong scheme + "http://test:8540", // wrong scheme + "http://test:8541", "https://test:8541", // wrong port + "http://bad", "https://bad", "http://bad:8540", "https://bad:8540"}, + }, + // several allowed origins + { + spec: "localhost,http://127.0.0.1", + expOk: []string{"localhost", "http://localhost", "https://localhost:8443", + "http://127.0.0.1", "http://127.0.0.1:8080"}, + expFail: []string{ + "https://127.0.0.1", // wrong scheme + "http://bad", "https://bad", "http://bad:8540", "https://bad:8540"}, + }, + } + for _, tc := range tests { + srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: splitAndTrim(tc.spec)}) + for _, origin := range tc.expOk { + if err := attemptWebsocketConnectionFromOrigin(t, srv, origin); err != nil { + t.Errorf("spec '%v', origin '%v': expected ok, got %v", tc.spec, origin, err) + } + } + for _, origin := range tc.expFail { + if err := attemptWebsocketConnectionFromOrigin(t, srv, origin); err == nil { + t.Errorf("spec '%v', origin '%v': expected not to allow, got ok", tc.spec, origin) + } + } + srv.stop() + } } // TestIsWebsocket tests if an incoming websocket upgrade request is handled properly. diff --git a/rpc/websocket.go b/rpc/websocket.go index de62798b46cb..cd60eeb613cd 100644 --- a/rpc/websocket.go +++ b/rpc/websocket.go @@ -131,43 +131,52 @@ func originIsAllowed(allowedOrigins mapset.Set, browserOrigin string) bool { } func ruleAllowsOrigin(allowedOrigin string, browserOrigin string) bool { - allowedScheme, allowedHostname, allowedPort, parsedAllowedErr := parseOriginURL(allowedOrigin) - browserScheme, browserHostname, browserPort, browserOriginErr := parseOriginURL(browserOrigin) - if parsedAllowedErr == nil && browserOriginErr == nil { - if allowedScheme != "" && browserScheme != "" && allowedScheme != browserScheme { - return false - } - if allowedHostname != "" && allowedHostname != browserHostname { - return false - } - if allowedPort != "" && browserPort != "" && allowedPort != browserPort { - return false - } - return true - } else { // parse error - log.Warn("Error parsing Origin URL in comparison for match.", "allowedOrigin", allowedOrigin, "browserOrigin", browserOrigin) + var ( + allowedScheme, allowedHostname, allowedPort string + browserScheme, browserHostname, browserPort string + err error + ) + allowedScheme, allowedHostname, allowedPort, err = parseOriginURL(allowedOrigin) + if err != nil { + log.Warn("Error parsing allowed origin specification", "spec", allowedOrigin, "error", err) + return false + } + browserScheme, browserHostname, browserPort, err = parseOriginURL(browserOrigin) + if err != nil { + log.Warn("Error parsing browser 'Origin' field", "Origin", browserOrigin, "error", err) + return false + } + if allowedScheme != "" && allowedScheme != browserScheme { + return false + } + if allowedHostname != "" && allowedHostname != browserHostname { + return false + } + if allowedPort != "" && allowedPort != browserPort { return false } + return true } func parseOriginURL(origin string) (string, string, string, error) { - parsedURL, parseError := url.Parse(strings.ToLower(origin)) + parsedURL, err := url.Parse(strings.ToLower(origin)) + if err != nil { + return "", "", "", err + } var scheme, hostname, port string - if parseError == nil { - if strings.Contains(origin, "://") { - scheme = parsedURL.Scheme - hostname = parsedURL.Hostname() - port = parsedURL.Port() - } else { - scheme = "" - hostname = parsedURL.Scheme - port = parsedURL.Opaque - if hostname == "" { - hostname = origin - } + if strings.Contains(origin, "://") { + scheme = parsedURL.Scheme + hostname = parsedURL.Hostname() + port = parsedURL.Port() + } else { + scheme = "" + hostname = parsedURL.Scheme + port = parsedURL.Opaque + if hostname == "" { + hostname = origin } } - return scheme, hostname, port, parseError + return scheme, hostname, port, nil } // DialWebsocketWithDialer creates a new RPC client that communicates with a JSON-RPC server