From 8b30e703ca66094a33ab123f7a436ddf9e2cf802 Mon Sep 17 00:00:00 2001
From: Dan Fuhry <dan@fuhry.com>
Date: Tue, 8 Apr 2025 17:59:26 -0400
Subject: [PATCH] [http/saml] cookie bugfixes

- strip auth cookies before forwarding to origin
- when no session, send 401 instead of redirect for CORS and websocket requests
- delete unused relaystate cookies
---
 http/route_action_saml.go | 73 +++++++++++++++++++++++++++++++++++----
 1 file changed, 67 insertions(+), 6 deletions(-)

diff --git a/http/route_action_saml.go b/http/route_action_saml.go
index eb0d829..6594fce 100644
--- a/http/route_action_saml.go
+++ b/http/route_action_saml.go
@@ -41,9 +41,10 @@ type SAMLServiceProvider struct {
 }
 
 type samlAction struct {
-	sp             *SAMLServiceProvider
-	requireAuth    bool
-	usernameHeader string
+	sp              *SAMLServiceProvider
+	requireAuth     bool
+	usernameHeader  string
+	preserveCookies bool
 }
 
 var samlAttributeReplaceRegexp = regexp.MustCompile(`[^a-z0-9]+`)
@@ -85,6 +86,21 @@ func (sa *samlAction) Handle(w http.ResponseWriter, r *http.Request, next http.H
 	}
 
 	if sessionErr == samlsp.ErrNoSession && sa.requireAuth {
+		// cleanup previous saml cookies, because these really pile up
+		logger.V(3).Debugf("request tracker is a %T", provider.RequestTracker)
+		if t, ok := provider.RequestTracker.(*samlsp.CookieRequestTracker); ok {
+			sa.cleanCookies(w, r, t)
+		}
+		if r.Header.Get("sec-fetch-mode") == "cors" || r.Header.Get("sec-fetch-mode") == "websocket" {
+			// XHRs should not be redirected to auth as this creates a ton of cookie spam
+			errMsg := "SAML session required"
+			if r.Header.Get("accept") == "application/json" {
+				w.Header().Set("content-type", "application/json")
+				errMsg = `{"error": "` + errMsg + `"}`
+			}
+			http.Error(w, errMsg, http.StatusUnauthorized)
+			return
+		}
 		logger.V(3).Debugf("route requires a valid session, redirecting")
 
 		provider.HandleStartAuthFlow(w, r)
@@ -128,6 +144,20 @@ func (sa *samlAction) Handle(w http.ResponseWriter, r *http.Request, next http.H
 		r.Header.Set("x-saml-anonymous-auth", "1")
 	}
 
+	// don't forward saml cookies to the origin unless PreserveCookies is set
+	if !sa.preserveCookies {
+		newReq := r.Clone(r.Context())
+		newReq.Header.Del("Cookie")
+		for _, cookie := range r.Cookies() {
+			if cookie.Name != "token" && !strings.HasPrefix(cookie.Name, "saml_") {
+				newReq.AddCookie(cookie)
+			}
+		}
+
+		logger.V(3).Debugf("new Cookie header with saml cookies filtered: %+v", newReq.Cookies())
+		r = newReq
+	}
+
 	next(w, r)
 }
 
@@ -141,6 +171,35 @@ func (sa *samlAction) checkRequest(r *http.Request) error {
 	return nil
 }
 
+// cleanCookies deletes cookies set by crewjam/saml.
+//
+// The SP auth handler creates two cookies: one named "token" and one named "saml_" plus a
+// randomized suffix. The problem is, a new randomized-suffix cookie is generated each time an auth
+// redirect is performed. Apps like Grafana continue making requests in the background even after a
+// user's session expires, and this results in dozens or hundreds of cookies being set eventually
+// leading to a 431 (Requested Header Fields Too Large).
+//
+// To mitigate this, we clear any existing saml cookies whenever we send an auth redirect or reject
+// an XHR with a 401.
+func (sa *samlAction) cleanCookies(w http.ResponseWriter, r *http.Request, t *samlsp.CookieRequestTracker) {
+	for _, cookie := range r.Cookies() {
+		if strings.HasPrefix(cookie.Name, "saml_") {
+			logger := LoggerFromContext(r.Context())
+			logger.V(3).Debugf("deleting leftover relaystate cookie: %s", cookie.Name)
+
+			http.SetCookie(w, &http.Cookie{
+				Name:     cookie.Name,
+				Value:    "",
+				MaxAge:   -1,
+				HttpOnly: true,
+				SameSite: t.SameSite,
+				Secure:   t.ServiceProvider.AcsURL.Scheme == "https",
+				Path:     t.ServiceProvider.AcsURL.Path,
+			})
+		}
+	}
+}
+
 func (sp *SAMLServiceProvider) Metadata() (*saml.EntityDescriptor, error) {
 	var err error
 	sp.metadataOnce.Do(func() {
@@ -293,7 +352,8 @@ func samlActionFromRouteYaml(node *yaml.Node) (RouteAction, error) {
 	var rawNode struct {
 		SP *struct {
 			*SAMLServiceProvider
-			Require string `yaml:"require"`
+			Require         string `yaml:"require"`
+			PreserveCookies bool   `yaml:"preserve_cookies"`
 		} `yaml:"saml,omitempty"`
 		Auth string `yaml:"auth"`
 	}
@@ -309,8 +369,9 @@ func samlActionFromRouteYaml(node *yaml.Node) (RouteAction, error) {
 	}
 
 	sa := &samlAction{
-		sp:          rawNode.SP.SAMLServiceProvider,
-		requireAuth: require,
+		sp:              rawNode.SP.SAMLServiceProvider,
+		requireAuth:     require,
+		preserveCookies: rawNode.SP.PreserveCookies,
 	}
 
 	return sa, nil
-- 
2.50.1