safeweb: replace gorilla with Sec-Fetch-Site checkpatrickod/safeweb-sec-fetch-site

Require that all non-(GET|OPTIONS|HEAD) requests to the browser mux
specify Sec-Fetch-Site=same-origin to prohibit cross-origin requests.

Optionally allow for requests to specify "same-site" indicating a
cross-origin request from an origin that shares a root domain with the
application's own.

Updates tailscale/corp#25340

2 files changed, 108 insertions, 128 deletions

diff --git a/safeweb/http.go b/safeweb/http.go
index 143c4dcee..4d4d38874 100644
--- a/safeweb/http.go
+++ b/safeweb/http.go
@@ -15,11 +15,9 @@
 //
 // # Browser Routes
 //
-// All routes in the browser mux enforce CSRF protection using the gorilla/csrf
-// package. The application must template the CSRF token into its forms using
-// the [TemplateField] and [TemplateTag] APIs. Applications that are served in a
-// secure context (over HTTPS) should also set the SecureContext field to true
-// to ensure that the the CSRF cookies are marked as Secure.
+// All routes in the browser mux are protected against CSRF attacks by
+// requiring non-(GET|HEAD|OPTIONS) requests to specify the Sec-Fetch-Site header
+// with the value "same-origin".
 //
 // In addition, browser routes will also have the following applied:
 //   - Content-Security-Policy header that disallows inline scripts, framing, and third party resources.
@@ -64,15 +62,11 @@
 //	if err := s.Serve(ln); err != nil && err != http.ErrServerClosed {
 //		log.Fatalf("failed to serve: %v", err)
 //	}
-//
-// [TemplateField]: https://pkg.go.dev/github.com/gorilla/csrf#TemplateField
-// [TemplateTag]: https://pkg.go.dev/github.com/gorilla/csrf#TemplateTag
 package safeweb
 
 import (
 	"cmp"
 	"context"
-	crand "crypto/rand"
 	"fmt"
 	"log"
 	"maps"
@@ -82,8 +76,6 @@ import (
 	"path"
 	"slices"
 	"strings"
-
-	"github.com/gorilla/csrf"
 )
 
 // CSP is the value of a Content-Security-Policy header. Keys are CSP
@@ -150,10 +142,6 @@ var DefaultStrictTransportSecurityOptions = "max-age=31536000"
 
 // Config contains the configuration for a safeweb server.
 type Config struct {
-	// SecureContext specifies whether the Server is running in a secure (HTTPS) context.
-	// Setting this to true will cause the Server to set the Secure flag on CSRF cookies.
-	SecureContext bool
-
 	// BrowserMux is the HTTP handler for any routes in your application that
 	// should only be served to browsers in a primary origin context. These
 	// requests will be subject to CSRF protection and will have
@@ -174,12 +162,6 @@ type Config struct {
 	// No headers will be sent if no methods are provided.
 	AccessControlAllowMethods []string
 
-	// CSRFSecret is the secret used to sign CSRF tokens. It must be 32 bytes long.
-	// This should be considered a sensitive value and should be kept secret.
-	// If this is not provided, the Server will generate a random CSRF secret on
-	// startup.
-	CSRFSecret []byte
-
 	// CSP is the Content-Security-Policy header to return with BrowserMux
 	// responses.
 	CSP CSP
@@ -188,10 +170,6 @@ type Config struct {
 	// inline CSS.
 	CSPAllowInlineStyles bool
 
-	// CookiesSameSiteLax specifies whether to use SameSite=Lax in cookies. The
-	// default is to set SameSite=Strict.
-	CookiesSameSiteLax bool
-
 	// StrictTransportSecurityOptions specifies optional directives for the
 	// Strict-Transport-Security header sent in response to requests made to the
 	// BrowserMux when SecureContext is true.
@@ -203,6 +181,16 @@ type Config struct {
 	// Do not use the Handler field of http.Server, as it will be ignored.
 	// Instead, set your handlers using APIMux and BrowserMux.
 	HTTPServer *http.Server
+
+	// ServeHSTS specifies whether to serve the Strict-Transport-Security header
+	// in response to requests made to the BrowserMux.
+	ServeHSTS bool
+
+	// AllowSecFetchSiteSameSite specifies whether to allow requests with the
+	// Sec-Fetch-Site header set to "same-site " indicating that they are
+	// cross-origin but that their origin shares the same site (gTLD+1) with
+	// that of the request.
+	AllowSecFetchSiteSameSite bool
 }
 
 func (c *Config) setDefaults() error {
@@ -214,13 +202,6 @@ func (c *Config) setDefaults() error {
 		c.APIMux = &http.ServeMux{}
 	}
 
-	if c.CSRFSecret == nil || len(c.CSRFSecret) == 0 {
-		c.CSRFSecret = make([]byte, 32)
-		if _, err := crand.Read(c.CSRFSecret); err != nil {
-			return fmt.Errorf("failed to generate CSRF secret: %w", err)
-		}
-	}
-
 	if c.CSP == nil {
 		c.CSP = DefaultCSP()
 	}
@@ -231,9 +212,8 @@ func (c *Config) setDefaults() error {
 // Server is a safeweb server.
 type Server struct {
 	Config
-	h           *http.Server
-	csp         string
-	csrfProtect func(http.Handler) http.Handler
+	h   *http.Server
+	csp string
 }
 
 // NewServer creates a safeweb server with the provided configuration. It will
@@ -252,10 +232,6 @@ func NewServer(config Config) (*Server, error) {
 		return nil, fmt.Errorf("failed to set defaults: %w", err)
 	}
 
-	sameSite := csrf.SameSiteStrictMode
-	if config.CookiesSameSiteLax {
-		sameSite = csrf.SameSiteLaxMode
-	}
 	if config.CSPAllowInlineStyles {
 		if _, ok := config.CSP["style-src"]; ok {
 			config.CSP.Add("style-src", "unsafe-inline")
@@ -266,9 +242,6 @@ func NewServer(config Config) (*Server, error) {
 	s := &Server{
 		Config: config,
 		csp:    config.CSP.String(),
-		// only set Secure flag on CSRF cookies if we are in a secure context
-		// as otherwise the browser will reject the cookie
-		csrfProtect: csrf.Protect(config.CSRFSecret, csrf.Secure(config.SecureContext), csrf.SameSite(sameSite)),
 	}
 	s.h = cmp.Or(config.HTTPServer, &http.Server{})
 	if s.h.Handler != nil {
@@ -318,12 +291,6 @@ func checkHandlerType(apiPattern, browserPattern string) handlerType {
 }
 
 func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	// if we are not in a secure context, signal to the CSRF middleware that
-	// TLS-only header checks should be skipped
-	if !s.Config.SecureContext {
-		r = csrf.PlaintextHTTPRequest(r)
-	}
-
 	_, bp := s.BrowserMux.Handler(r)
 	_, ap := s.APIMux.Handler(r)
 	switch {
@@ -376,10 +343,38 @@ func (s *Server) serveBrowser(w http.ResponseWriter, r *http.Request) {
 	w.Header().Set("Content-Security-Policy", s.csp)
 	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.Header().Set("Referer-Policy", "same-origin")
-	if s.SecureContext {
+	if s.ServeHSTS {
 		w.Header().Set("Strict-Transport-Security", cmp.Or(s.StrictTransportSecurityOptions, DefaultStrictTransportSecurityOptions))
 	}
-	s.csrfProtect(s.BrowserMux).ServeHTTP(w, r)
+
+	// Allow GET, HEAD, and OPTIONS requests to the browser mux without
+	// Sec-Fetch-Site header checks.
+	switch r.Method {
+	case "GET", "HEAD", "OPTIONS":
+		s.BrowserMux.ServeHTTP(w, r)
+		return
+	default:
+	}
+
+	switch r.Header.Get("Sec-Fetch-Site") {
+	case "same-origin":
+		// allow same-origin requests
+	case "same-site":
+		// allow cross-origin, but same-site requests if configured
+		if !s.AllowSecFetchSiteSameSite {
+			http.Error(w, "forbidden cross-origin request", http.StatusForbidden)
+			return
+		}
+	case "cross-site", "none":
+		// deny cross-site requests or direct navigation non-GET requests.
+		http.Error(w, "forbidden cross-origin request", http.StatusForbidden)
+		return
+	default:
+		// deny requests with no Sec-Fetch-Site header
+		http.Error(w, "missing required Sec-Fetch-Site header. You might need to update your browser.", http.StatusForbidden)
+		return
+	}
+	s.BrowserMux.ServeHTTP(w, r)
 }
 
 // ServeRedirectHTTP serves a single HTTP handler on the provided listener that
diff --git a/safeweb/http_test.go b/safeweb/http_test.go
index 852ce326b..7644eb81d 100644
--- a/safeweb/http_test.go
+++ b/safeweb/http_test.go
@@ -13,7 +13,6 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/gorilla/csrf"
 )
 
 func TestCompleteCORSConfig(t *testing.T) {
@@ -156,28 +155,62 @@ func TestAPIMuxCrossOriginResourceSharingHeaders(t *testing.T) {
 
 func TestCSRFProtection(t *testing.T) {
 	tests := []struct {
-		name          string
-		apiRoute      bool
-		passCSRFToken bool
-		wantStatus    int
+		name                   string
+		httpMethod             string
+		apiRoute               bool
+		secFetchSiteNone       bool
+		secFetchSiteSameOrigin bool
+		secFetchSiteSameSite   bool
+		secFetchSiteCrossSite  bool
+		permitSameSite         bool
+		wantStatus             int
 	}{
 		{
-			name:          "POST requests to non-API routes require CSRF token and fail if not provided",
-			apiRoute:      false,
-			passCSRFToken: false,
-			wantStatus:    http.StatusForbidden,
+			name:       "GET requests to browser routes do not require Sec-Fetch-Site header",
+			httpMethod: http.MethodGet,
+			apiRoute:   false,
+			wantStatus: http.StatusOK,
+		},
+		{
+			name:       "POST requests to browser routes require Sec-Fetch-Site=same-origin and fail if not provided",
+			httpMethod: http.MethodPost,
+			apiRoute:   false,
+			wantStatus: http.StatusForbidden,
 		},
 		{
-			name:          "POST requests to non-API routes require CSRF token and pass if provided",
-			apiRoute:      false,
-			passCSRFToken: true,
-			wantStatus:    http.StatusOK,
+			name:                   "POST requests to browser routes require Sec-Fetch-Site=same-origin and pass if provided",
+			secFetchSiteSameOrigin: true,
+			httpMethod:             http.MethodPost,
+			apiRoute:               false,
+			wantStatus:             http.StatusOK,
 		},
 		{
-			name:          "POST requests to /api/ routes do not require CSRF token",
-			apiRoute:      true,
-			passCSRFToken: false,
-			wantStatus:    http.StatusOK,
+			name:             "POST requests to browser routes with Sec-Fetch-Site=none fail",
+			secFetchSiteNone: true,
+			httpMethod:       http.MethodPost,
+			apiRoute:         false,
+			wantStatus:       http.StatusForbidden,
+		},
+		{
+			name:                 "POST requests to browser routes with Sec-Fetch-Site=same-site fail by default",
+			secFetchSiteSameSite: true,
+			httpMethod:           http.MethodPost,
+			apiRoute:             false,
+			wantStatus:           http.StatusForbidden,
+		},
+		{
+			name:                 "POST requests to browser routes with Sec-Fetch-Site=same-site pass if configured",
+			secFetchSiteSameSite: true,
+			permitSameSite:       true,
+			httpMethod:           http.MethodPost,
+			apiRoute:             false,
+			wantStatus:           http.StatusOK,
+		},
+		{
+			name:       "POST requests to API routes do not require Sec-Fetch-Site header",
+			httpMethod: http.MethodPost,
+			apiRoute:   true,
+			wantStatus: http.StatusOK,
 		},
 	}
 	for _, tt := range tests {
@@ -199,7 +232,7 @@ func TestCSRFProtection(t *testing.T) {
 			defer s.Close()
 
 			// construct the test request
-			req := httptest.NewRequest("POST", "/", nil)
+			req := httptest.NewRequest(tt.httpMethod, "/", nil)
 
 			// send JSON for API routes, form data for browser routes
 			if tt.apiRoute {
@@ -208,25 +241,19 @@ func TestCSRFProtection(t *testing.T) {
 				req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 			}
 
-			// retrieve CSRF cookie & pass it in the test request
-			// ref: https://github.com/gorilla/csrf/blob/main/csrf_test.go#L344-L347
-			var token string
-			if tt.passCSRFToken {
-				h.Handle("/csrf", http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
-					token = csrf.Token(r)
-				}))
-				get := httptest.NewRequest("GET", "/csrf", nil)
-				w := httptest.NewRecorder()
-				s.h.Handler.ServeHTTP(w, get)
-				resp := w.Result()
-
-				// pass the token & cookie in our subsequent test request
-				req.Header.Set("X-CSRF-Token", token)
-				for _, c := range resp.Cookies() {
-					req.AddCookie(c)
-				}
+			if tt.permitSameSite {
+				s.Config.AllowSecFetchSiteSameSite = true
 			}
 
+			if tt.secFetchSiteNone {
+				req.Header.Set("Sec-Fetch-Site", "none")
+			} else if tt.secFetchSiteSameOrigin {
+				req.Header.Set("Sec-Fetch-Site", "same-origin")
+			} else if tt.secFetchSiteSameSite {
+				req.Header.Set("Sec-Fetch-Site", "same-site")
+			} else if tt.secFetchSiteCrossSite {
+				req.Header.Set("Sec-Fetch-Site", "cross-site")
+			}
 			w := httptest.NewRecorder()
 			s.h.Handler.ServeHTTP(w, req)
 			resp := w.Result()
@@ -294,48 +321,6 @@ func TestContentSecurityPolicyHeader(t *testing.T) {
 	}
 }
 
-func TestCSRFCookieSecureMode(t *testing.T) {
-	tests := []struct {
-		name       string
-		secureMode bool
-		wantSecure bool
-	}{
-		{
-			name:       "CSRF cookie should be secure when server is in secure context",
-			secureMode: true,
-			wantSecure: true,
-		},
-		{
-			name:       "CSRF cookie should not be secure when server is not in secure context",
-			secureMode: false,
-			wantSecure: false,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			h := &http.ServeMux{}
-			h.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				w.Write([]byte("ok"))
-			}))
-			s, err := NewServer(Config{BrowserMux: h, SecureContext: tt.secureMode})
-			if err != nil {
-				t.Fatal(err)
-			}
-			defer s.Close()
-
-			req := httptest.NewRequest("GET", "/", nil)
-			w := httptest.NewRecorder()
-			s.h.Handler.ServeHTTP(w, req)
-			resp := w.Result()
-
-			cookie := resp.Cookies()[0]
-			if (cookie.Secure == tt.wantSecure) == false {
-				t.Fatalf("csrf cookie secure flag want: %v; got: %v", tt.wantSecure, cookie.Secure)
-			}
-		})
-	}
-}
-
 func TestRefererPolicy(t *testing.T) {
 	tests := []struct {
 		name              string
@@ -607,7 +592,7 @@ func TestStrictTransportSecurityOptions(t *testing.T) {
 			h.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				w.Write([]byte("ok"))
 			}))
-			s, err := NewServer(Config{BrowserMux: h, SecureContext: tt.secureContext, StrictTransportSecurityOptions: tt.options})
+			s, err := NewServer(Config{BrowserMux: h, ServeHSTS: tt.secureContext, StrictTransportSecurityOptions: tt.options})
 			if err != nil {
 				t.Fatal(err)
 			}