client/web: fix CSRF handler order in web UIpatrickod/reverse-web-handler-order-csrf

Fix the order of the CSRF handlers (HTTP plaintext context setting,
_then_ enforcement) in the construction of the web UI server. This
resolves false-positive "invalid Origin" 403 exceptions when attempting
to update settings in the web UI.

Add unit test to exercise the CSRF protection failure and success cases
for our web UI configuration.

Updates #14822
Updates #14872

Signed-off-by: Patrick O'Doherty <patrick@tailscale.com>

1 files changed, 30 insertions, 23 deletions

diff --git a/client/web/web.go b/client/web/web.go
index 6203b4c18..e9810ccd0 100644
--- a/client/web/web.go
+++ b/client/web/web.go
@@ -203,15 +203,25 @@ func NewServer(opts ServerOpts) (s *Server, err error) {
 	}
 	s.assetsHandler, s.assetsCleanup = assetsHandler(s.devMode)
 
-	var metric string // clientmetric to report on startup
+	var metric string
+	s.apiHandler, metric = s.modeAPIHandler(s.mode)
+	s.apiHandler = s.withCSRF(s.apiHandler)
 
-	// Create handler for "/api" requests with CSRF protection.
-	// We don't require secure cookies, since the web client is regularly used
-	// on network appliances that are served on local non-https URLs.
-	// The client is secured by limiting the interface it listens on,
-	// or by authenticating requests before they reach the web client.
+	// Don't block startup on reporting metric.
+	// Report in separate go routine with 5 second timeout.
+	go func() {
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		s.lc.IncrementCounter(ctx, metric, 1)
+	}()
+
+	return s, nil
+}
+
+func (s *Server) withCSRF(h http.Handler) http.Handler {
 	csrfProtect := csrf.Protect(s.csrfKey(), csrf.Secure(false))
 
+	// ref https://github.com/tailscale/tailscale/pull/14822
 	// signal to the CSRF middleware that the request is being served over
 	// plaintext HTTP to skip TLS-only header checks.
 	withSetPlaintext := func(h http.Handler) http.Handler {
@@ -221,27 +231,24 @@ func NewServer(opts ServerOpts) (s *Server, err error) {
 		})
 	}
 
-	switch s.mode {
+	// NB: the order of the withSetPlaintext and csrfProtect calls is important
+	// to ensure that we signal to the CSRF middleware that the request is being
+	// served over plaintext HTTP and not over TLS as it presumes by default.
+	return withSetPlaintext(csrfProtect(h))
+}
+
+func (s *Server) modeAPIHandler(mode ServerMode) (http.Handler, string) {
+	switch mode {
 	case LoginServerMode:
-		s.apiHandler = csrfProtect(withSetPlaintext(http.HandlerFunc(s.serveLoginAPI)))
-		metric = "web_login_client_initialization"
+		return http.HandlerFunc(s.serveLoginAPI), "web_login_client_initialization"
 	case ReadOnlyServerMode:
-		s.apiHandler = csrfProtect(withSetPlaintext(http.HandlerFunc(s.serveLoginAPI)))
-		metric = "web_readonly_client_initialization"
+		return http.HandlerFunc(s.serveLoginAPI), "web_readonly_client_initialization"
 	case ManageServerMode:
-		s.apiHandler = csrfProtect(withSetPlaintext(http.HandlerFunc(s.serveAPI)))
-		metric = "web_client_initialization"
+		return http.HandlerFunc(s.serveAPI), "web_client_initialization"
+	default: // invalid mode
+		log.Fatalf("invalid mode: %v", mode)
 	}
-
-	// Don't block startup on reporting metric.
-	// Report in separate go routine with 5 second timeout.
-	go func() {
-		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-		defer cancel()
-		s.lc.IncrementCounter(ctx, metric, 1)
-	}()
-
-	return s, nil
+	return nil, ""
 }
 
 func (s *Server) Shutdown() {