diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 4ad0968ce..b889a8fe3 100644 --- a/control/controlclient/direct.go +++ b/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") diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index fc5f87597..c0d0c6d09 100644 --- a/ipn/ipnlocal/local.go +++ b/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}) } } diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 980039800..5fc2a2cd2 100644 --- a/ipn/ipnlocal/state_test.go +++ b/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)