From 68da15516fe1c152fb7add475048200e65b6cc5c Mon Sep 17 00:00:00 2001
From: Sonia Appasamy <sonia@tailscale.com>
Date: Fri, 20 Oct 2023 13:24:29 -0400
Subject: [PATCH] ipn/localapi,client/web: clean up auth error handling

This commit makes two changes to the web client auth flow error
handling:

1. Properly passes back the error code from the noise request from
   the localapi. Previously we were using io.Copy, which was always
   setting a 200 response status code.
2. Clean up web client browser sessions on any /wait endpoint error.
   This avoids the user getting in a stuck state if something goes
   wrong with their auth path.

Updates tailscale/corp#14335

Signed-off-by: Sonia Appasamy <sonia@tailscale.com>
---
 client/web/web.go        | 19 +++++++------------
 ipn/localapi/localapi.go |  9 +++++----
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/client/web/web.go b/client/web/web.go
index 0cf4acfc9..7cbd413f0 100644
--- a/client/web/web.go
+++ b/client/web/web.go
@@ -292,7 +292,6 @@ func (s *Server) serveLoginAPI(w http.ResponseWriter, r *http.Request) {
 	errNotUsingTailscale = errors.New("not-using-tailscale")
 	errTaggedSource      = errors.New("tagged-source")
 	errNotOwner          = errors.New("not-owner")
-	errFailedAuth        = errors.New("failed-auth")
 )
 
 // getTailscaleBrowserSession retrieves the browser session associated with
@@ -413,12 +412,12 @@ func (s *Server) serveTailscaleAuth(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Query().Get("wait") == "true" {
 			// Client requested we block until user completes auth.
 			d, err := s.getOrAwaitAuth(r.Context(), session.AuthID, whois.Node.ID)
-			if errors.Is(err, errFailedAuth) {
-				http.Error(w, "user is unauthorized", http.StatusUnauthorized)
-				s.browserSessions.Delete(session.ID) // clean up the failed session
-				return
-			} else if err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
+			if err != nil {
+				http.Error(w, err.Error(), http.StatusUnauthorized)
+				// Clean up the session. Doing this on any error from control
+				// server to avoid the user getting stuck with a bad session
+				// cookie.
+				s.browserSessions.Delete(session.ID)
 				return
 			}
 			if d.Complete {
@@ -485,11 +484,7 @@ type data struct {
 	}
 	body, _ := io.ReadAll(resp.Body)
 	resp.Body.Close()
-	if resp.StatusCode == http.StatusUnauthorized {
-		// User completed auth, but control server reported
-		// them unauthorized to manage this node.
-		return nil, errFailedAuth
-	} else if resp.StatusCode != http.StatusOK {
+	if resp.StatusCode != http.StatusOK {
 		return nil, fmt.Errorf("failed request: %s", body)
 	}
 	var authResp *tailcfg.WebClientAuthResponse
diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go
index 4151d94e7..ef820537f 100644
--- a/ipn/localapi/localapi.go
+++ b/ipn/localapi/localapi.go
@@ -2172,12 +2172,13 @@ type reqData struct {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
-	defer resp.Body.Close()
-
-	if _, err := io.Copy(w, resp.Body); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+	body, _ := io.ReadAll(resp.Body)
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		http.Error(w, string(body), resp.StatusCode)
 		return
 	}
+	w.Write(body)
 	w.Header().Set("Content-Type", "application/json")
 }