ipn/ipnlocal: move URL validation to LocalBackend

Updates tailscale/corp#7948

Signed-off-by: Maisem Ali <maisem@tailscale.com>

control/controlclient/direct.go

@@ -999,9 +999,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool
 			metricMapResponsePings.Add(1)
 			go c.answerPing(pr)
 		}
-		if !c.validPopBrowserURL(resp.PopBrowserURL) {
-			resp.PopBrowserURL = ""
-		}
 		if u := resp.PopBrowserURL; u != "" && u != sess.lastPopBrowserURL {
 			sess.lastPopBrowserURL = u
 			if c.popBrowser != nil {
@@ -1705,28 +1702,6 @@ func (c *Direct) ReportHealthChange(sys health.Subsystem, sysErr error) {
 	res.Body.Close()
 }
 
-// validPopBrowserURL reports whether urlStr is a valid value for a
-// control server to send in a MapResponse.PopUpBrowserURL field.
-func (c *Direct) validPopBrowserURL(urlStr string) bool {
-	if urlStr == "" {
-		// Common case.
-		return true
-	}
-	u, err := url.Parse(urlStr)
-	if err != nil {
-		return false
-	}
-	switch u.Scheme {
-	case "https":
-		return true
-	case "http":
-		// If the control server is using plain HTTP (likely a dev server),
-		// then permit http://.
-		return strings.HasPrefix(c.serverURL, "http://")
-	}
-	return false
-}
-
 var (
 	metricMapRequestsActive = clientmetric.NewGauge("controlclient_map_requests_active")
 

ipn/ipnlocal/local.go

@@ -1726,8 +1726,34 @@ func (b *LocalBackend) popBrowserAuthNow() {
 	}
 }
 
+// validPopBrowserURL reports whether urlStr is a valid value for a
+// control server to send in a *URL field.
+// b.mu must *not* be held.
+func (b *LocalBackend) validPopBrowserURL(urlStr string) bool {
+	if urlStr == "" {
+		// Common case.
+		return true
+	}
+	u, err := url.Parse(urlStr)
+	if err != nil {
+		return false
+	}
+	switch u.Scheme {
+	case "https":
+		return true
+	case "http":
+		b.mu.Lock()
+		serverURL := b.serverURL
+		b.mu.Unlock()
+		// If the control server is using plain HTTP (likely a dev server),
+		// then permit http://.
+		return strings.HasPrefix(serverURL, "http://")
+	}
+	return false
+}
+
 func (b *LocalBackend) tellClientToBrowseToURL(url string) {
-	if url != "" {
+	if url != "" && b.validPopBrowserURL(url) {
 		b.send(ipn.Notify{BrowseToURL: &url})
 	}
 }

ipn/ipnlocal/state_test.go

@@ -404,7 +404,7 @@ func TestStateMachine(t *testing.T) {
 	// the user needs to visit a login URL.
 	t.Logf("\n\nLogin (url response)")
 	notifies.expect(1)
-	url1 := "http://localhost:1/1"
+	url1 := "https://localhost:1/1"
 	cc.send(nil, url1, false, nil)
 	{
 		cc.assertCalls()
@@ -453,7 +453,7 @@ func TestStateMachine(t *testing.T) {
 	// Provide a new interactive login URL.
 	t.Logf("\n\nLogin2 (url response)")
 	notifies.expect(1)
-	url2 := "http://localhost:1/2"
+	url2 := "https://localhost:1/2"
 	cc.send(nil, url2, false, nil)
 	{
 		cc.assertCalls()
@@ -811,7 +811,7 @@ func TestStateMachine(t *testing.T) {
 	t.Logf("\n\nLoginDifferent")
 	notifies.expect(1)
 	b.StartLoginInteractive()
-	url3 := "http://localhost:1/3"
+	url3 := "https://localhost:1/3"
 	cc.send(nil, url3, false, nil)
 	{
 		nn := notifies.drain(1)