diff --git a/tsweb/tsweb.go b/tsweb/tsweb.go index ebe4976fa..856bb3efa 100644 --- a/tsweb/tsweb.go +++ b/tsweb/tsweb.go @@ -524,6 +524,9 @@ func VarzHandler(w http.ResponseWriter, r *http.Request) { // current server, or one of allowedHosts. Returns the cleaned URL or // a validation error. func CleanRedirectURL(urlStr string, allowedHosts []string) (*url.URL, error) { + if urlStr == "" { + return &url.URL{}, nil + } // In some places, we unfortunately query-escape the redirect URL // too many times, and end up needing to redirect to a URL that's // still escaped by one level. Try to unescape the input. diff --git a/tsweb/tsweb_test.go b/tsweb/tsweb_test.go index cb9ea100a..11f68051f 100644 --- a/tsweb/tsweb_test.go +++ b/tsweb/tsweb_test.go @@ -626,44 +626,49 @@ func TestCleanRedirectURL(t *testing.T) { localHost := []string{"127.0.0.1", "localhost"} myServer := []string{"myserver"} cases := []struct { - url string - hosts []string - want string + url string + hosts []string + want string + wantErr bool }{ - {"http://tailscale.com/foo", tailscaleHost, "http://tailscale.com/foo"}, - {"http://tailscale.com/foo", tailscaleAndOtherHost, "http://tailscale.com/foo"}, - {"http://microsoft.com/foo", tailscaleAndOtherHost, "http://microsoft.com/foo"}, - {"https://tailscale.com/foo", tailscaleHost, "https://tailscale.com/foo"}, - {"/foo", tailscaleHost, "/foo"}, - {"//tailscale.com/foo", tailscaleHost, "//tailscale.com/foo"}, - - {"/a/foobar", tailscaleHost, "/a/foobar"}, - {"http://127.0.0.1/a/foobar", localHost, "http://127.0.0.1/a/foobar"}, - {"http://127.0.0.1:123/a/foobar", localHost, "http://127.0.0.1:123/a/foobar"}, - {"http://127.0.0.1:31544/a/foobar", localHost, "http://127.0.0.1:31544/a/foobar"}, - {"http://localhost/a/foobar", localHost, "http://localhost/a/foobar"}, - {"http://localhost:123/a/foobar", localHost, "http://localhost:123/a/foobar"}, - {"http://localhost:31544/a/foobar", localHost, "http://localhost:31544/a/foobar"}, - {"http://myserver/a/foobar", myServer, "http://myserver/a/foobar"}, - {"http://myserver:123/a/foobar", myServer, "http://myserver:123/a/foobar"}, - {"http://myserver:31544/a/foobar", myServer, "http://myserver:31544/a/foobar"}, - {"http://evil.com/foo", tailscaleHost, ""}, - {"//evil.com", tailscaleHost, ""}, - {"HttP://tailscale.com", tailscaleHost, "http://tailscale.com"}, - {"http://TaIlScAlE.CoM/spongebob", tailscaleHost, "http://TaIlScAlE.CoM/spongebob"}, - {"ftp://tailscale.com", tailscaleHost, ""}, - {"https:/evil.com", tailscaleHost, ""}, // regression test for tailscale/corp#892 - {"%2Fa%2F44869c061701", tailscaleHost, "/a/44869c061701"}, // regression test for tailscale/corp#13288 - {"https%3A%2Ftailscale.com", tailscaleHost, ""}, // escaped colon-single-slash malformed URL + {"http://tailscale.com/foo", tailscaleHost, "http://tailscale.com/foo", false}, + {"http://tailscale.com/foo", tailscaleAndOtherHost, "http://tailscale.com/foo", false}, + {"http://microsoft.com/foo", tailscaleAndOtherHost, "http://microsoft.com/foo", false}, + {"https://tailscale.com/foo", tailscaleHost, "https://tailscale.com/foo", false}, + {"/foo", tailscaleHost, "/foo", false}, + {"//tailscale.com/foo", tailscaleHost, "//tailscale.com/foo", false}, + + {"/a/foobar", tailscaleHost, "/a/foobar", false}, + {"http://127.0.0.1/a/foobar", localHost, "http://127.0.0.1/a/foobar", false}, + {"http://127.0.0.1:123/a/foobar", localHost, "http://127.0.0.1:123/a/foobar", false}, + {"http://127.0.0.1:31544/a/foobar", localHost, "http://127.0.0.1:31544/a/foobar", false}, + {"http://localhost/a/foobar", localHost, "http://localhost/a/foobar", false}, + {"http://localhost:123/a/foobar", localHost, "http://localhost:123/a/foobar", false}, + {"http://localhost:31544/a/foobar", localHost, "http://localhost:31544/a/foobar", false}, + {"http://myserver/a/foobar", myServer, "http://myserver/a/foobar", false}, + {"http://myserver:123/a/foobar", myServer, "http://myserver:123/a/foobar", false}, + {"http://myserver:31544/a/foobar", myServer, "http://myserver:31544/a/foobar", false}, + {"http://evil.com/foo", tailscaleHost, "", true}, + {"//evil.com", tailscaleHost, "", true}, + {"HttP://tailscale.com", tailscaleHost, "http://tailscale.com", false}, + {"http://TaIlScAlE.CoM/spongebob", tailscaleHost, "http://TaIlScAlE.CoM/spongebob", false}, + {"ftp://tailscale.com", tailscaleHost, "", true}, + {"https:/evil.com", tailscaleHost, "", true}, // regression test for tailscale/corp#892 + {"%2Fa%2F44869c061701", tailscaleHost, "/a/44869c061701", false}, // regression test for tailscale/corp#13288 + {"https%3A%2Ftailscale.com", tailscaleHost, "", true}, // escaped colon-single-slash malformed URL + {"", nil, "", false}, } for _, tc := range cases { gotURL, err := CleanRedirectURL(tc.url, tc.hosts) if err != nil { - if tc.want != "" { + if !tc.wantErr { t.Errorf("CleanRedirectURL(%q, %v) got error: %v", tc.url, tc.hosts, err) } } else { + if tc.wantErr { + t.Errorf("CleanRedirectURL(%q, %v) got %q, want an error", tc.url, tc.hosts, gotURL) + } if got := gotURL.String(); got != tc.want { t.Errorf("CleanRedirectURL(%q, %v) = %q, want %q", tc.url, tc.hosts, got, tc.want) }