tsweb: allow empty redirect URL in CleanRedirectURL (#11295)

Updates #cleanup

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

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.

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