From e496d934a83caff3653765194de99283016268e5 Mon Sep 17 00:00:00 2001
From: David Hartunian <davidh@cockroachlabs.com>
Date: Thu, 9 Feb 2023 11:11:33 -0500
Subject: [PATCH] servercontroller: serve http from default tenant on error

Previously, when an HTTP request arrived with a tenant cookie attached, we
would attempt to connect to that tenant and fail if the tenant couldn't be
started or if it didn't exist. This causes a bad user-facing experience when
someone's browser contains stale tenant cookies and they attempt to load DB
Console. The error returns a 500 to the browser and user sees no UI, can't try
and login again, and is basically stuck until they clear their cookies.

This change falls back to serving HTTP from the default tenant when the
requested one can't be reached. This ensures that *something* is served to
ensure that unauthenticated endpoints can still be loaded in these scenarios
and users can attempt to login again.

Epic: CRDB-12100

Release note: None
---
 pkg/ccl/serverccl/server_controller_test.go | 42 +++++++++++++++++++++
 pkg/server/server_controller_http.go        | 16 +++++++-
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/pkg/ccl/serverccl/server_controller_test.go b/pkg/ccl/serverccl/server_controller_test.go
index 92d1b204bc72..9599e7b5d650 100644
--- a/pkg/ccl/serverccl/server_controller_test.go
+++ b/pkg/ccl/serverccl/server_controller_test.go
@@ -181,6 +181,48 @@ VALUES($1, $2, $3, $4, $5)`, id, secret, username, created, expires)
 	require.Equal(t, body.Sessions[0].ApplicationName, "hello system")
 }
 
+// TestServerControllerBadHTTPCookies tests the controller's proxy
+// layer for correct behavior under scenarios where the client has
+// stale or invalid cookies. This helps ensure that we continue to
+// serve static assets even when the browser is referencing an
+// unknown tenant, or bad sessions.
+func TestServerControllerBadHTTPCookies(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	client, err := s.GetUnauthenticatedHTTPClient()
+	require.NoError(t, err)
+
+	c := &http.Cookie{
+		Name:     server.TenantSelectCookieName,
+		Value:    "some-nonexistent-tenant",
+		Path:     "/",
+		HttpOnly: true,
+		Secure:   true,
+	}
+
+	req, err := http.NewRequest("GET", s.AdminURL()+"/", nil)
+	require.NoError(t, err)
+	req.AddCookie(c)
+	resp, err := client.Do(req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	require.Equal(t, 200, resp.StatusCode)
+
+	req, err = http.NewRequest("GET", s.AdminURL()+"/bundle.js", nil)
+	require.NoError(t, err)
+	req.AddCookie(c)
+	resp, err = client.Do(req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	require.Equal(t, 200, resp.StatusCode)
+}
+
 func TestServerStartStop(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
diff --git a/pkg/server/server_controller_http.go b/pkg/server/server_controller_http.go
index 2a8b7be4ddd6..3eec8dcbe06e 100644
--- a/pkg/server/server_controller_http.go
+++ b/pkg/server/server_controller_http.go
@@ -64,7 +64,7 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {
 	s, err := c.getServer(ctx, tenantName)
 	if err != nil {
 		log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err)
-		// Clear session and tenant cookies after all logouts have completed.
+		// Clear session and tenant cookies since it appears they reference invalid state.
 		http.SetCookie(w, &http.Cookie{
 			Name:     MultitenantSessionCookieName,
 			Value:    "",
@@ -79,7 +79,19 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {
 			HttpOnly: false,
 			Expires:  timeutil.Unix(0, 0),
 		})
-		w.WriteHeader(http.StatusInternalServerError)
+		// Fall back to serving requests from the default tenant. This helps us serve
+		// the root path along with static assets even when the browser contains invalid
+		// tenant names or sessions (common during development). Otherwise the user can
+		// get into a state where they cannot load DB Console assets at all due to invalid
+		// cookies.
+		defaultTenantName := roachpb.TenantName(defaultTenantSelect.Get(&c.st.SV))
+		s, err = c.getServer(ctx, defaultTenantName)
+		if err != nil {
+			log.Warningf(ctx, "unable to find server for default tenant %q: %v", defaultTenantName, err)
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+		s.getHTTPHandlerFn()(w, r)
 		return
 	}
 	s.getHTTPHandlerFn()(w, r)