From bb3514e31665298769f7c82433ce27cf86fd5ce2 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 21 May 2026 12:29:32 +0100 Subject: [PATCH] Require CSRF token regardless of Authorization header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CheckCSRF was skipping the CSRF check whenever the request carried Authorization: Bearer, Authorization: Basic, or X-API-Token, without validating the credential. An attacker on a cross-origin page could fetch() any state-changing endpoint with credentials: 'include' plus an arbitrary Authorization header — the browser would auto-attach the victim's pulse_session cookie, the server would skip CSRF, and the request would execute as the logged-in user. Full CSRF bypass for every session-authenticated user. CSRF protection exists because the browser auto-attaches the session cookie. That cookie is the only auto-attached credential we issue, so it is the only correct signal for whether CSRF applies. Header-based auth is set explicitly per request and is not CSRF-vulnerable, but its presence does not make a session-cookie-bearing request safe. Skip CSRF only when no session cookie is present; otherwise require the token regardless of any Authorization or X-API-Token header. Tests: the three "header bypasses CSRF" cases in security_test.go were passing only because they sent no session cookie (so the no-cookie path returned true). Renamed those to make the no-cookie precondition explicit. Added TestCheckCSRF_HeaderDoesNotBypassWhenSessionCookiePresent in security_regression_test.go covering X-API-Token, Authorization: Basic, Authorization: Bearer, and mixed-case Bearer with a session cookie present — each must require a valid CSRF token. --- internal/api/security.go | 33 +++++------- internal/api/security_regression_test.go | 65 ++++++++++++++++++++++++ internal/api/security_test.go | 20 ++++---- 3 files changed, 88 insertions(+), 30 deletions(-) diff --git a/internal/api/security.go b/internal/api/security.go index 51adad226..042786a4c 100644 --- a/internal/api/security.go +++ b/internal/api/security.go @@ -59,30 +59,21 @@ func CheckCSRF(w http.ResponseWriter, r *http.Request) bool { return true } - // Skip CSRF for API token auth (API clients don't have sessions) - if r.Header.Get("X-API-Token") != "" { - log.Debug().Str("path", r.URL.Path).Msg("CSRF check skipped: API token auth") - return true - } + // CSRF is a defence against browser-auto-attached credentials. The only + // auto-attached credential we issue is the session cookie, so the session + // cookie is the ONLY signal for whether CSRF applies. Explicit per-request + // credentials (X-API-Token, Authorization: Bearer, Authorization: Basic) + // are NOT a skip-signal: an attacker on a cross-origin page can attach + // those headers after a CORS preflight while the browser still + // auto-attaches the victim's session cookie, fully bypassing CSRF. The + // previous behaviour — early-returning on any Authorization header — is a + // full CSRF bypass for session-authenticated users and must not return. - // Skip CSRF only for explicit non-session auth schemes. - if authHeader := strings.TrimSpace(r.Header.Get("Authorization")); authHeader != "" { - lower := strings.ToLower(authHeader) - if strings.HasPrefix(lower, "basic ") { - log.Debug().Str("path", r.URL.Path).Msg("CSRF check skipped: Basic auth header present") - return true - } - if strings.HasPrefix(lower, "bearer ") { - log.Debug().Str("path", r.URL.Path).Msg("CSRF check skipped: Bearer auth header present") - return true - } - } - - // Get session from cookie + // Get session from cookie. No cookie => purely header-authenticated (or + // unauthenticated) request that downstream auth will accept or reject on + // its own merits; CSRF does not apply. cookie, err := readSessionCookie(r) if err != nil { - // No session cookie means no CSRF check needed - // (either no auth configured or using basic auth which doesn't use sessions) log.Debug().Str("path", r.URL.Path).Msg("CSRF check skipped: no session cookie") return true } diff --git a/internal/api/security_regression_test.go b/internal/api/security_regression_test.go index 71c9717db..7bf9f88bf 100644 --- a/internal/api/security_regression_test.go +++ b/internal/api/security_regression_test.go @@ -4603,3 +4603,68 @@ func TestSSHConfigRejectsSetupTokenQueryParam(t *testing.T) { t.Fatalf("expected 401 when setup token is only provided in query string, got %d", rec.Code) } } + +// TestCheckCSRF_HeaderDoesNotBypassWhenSessionCookiePresent regression-tests the +// fix for a CSRF bypass: CheckCSRF was skipping the entire CSRF check whenever +// the request carried Authorization: Basic, Authorization: Bearer, or +// X-API-Token, without validating the credential. A cross-origin attacker +// could fetch() with credentials: 'include' and an arbitrary Authorization +// header — the browser would still auto-attach the victim's session cookie +// and the server would skip CSRF, fully bypassing protection. The contract is +// now: a session cookie is the only signal for whether CSRF applies. If a +// session cookie is present, CSRF must be valid regardless of any other +// auth-style header. +func TestCheckCSRF_HeaderDoesNotBypassWhenSessionCookiePresent(t *testing.T) { + cases := []struct { + name string + setHeader func(*http.Request) + description string + }{ + { + name: "x_api_token", + setHeader: func(r *http.Request) { + r.Header.Set("X-API-Token", "some-api-token") + }, + description: "X-API-Token must not bypass CSRF when a session cookie is present", + }, + { + name: "authorization_basic", + setHeader: func(r *http.Request) { + r.Header.Set("Authorization", "Basic dXNlcjpwYXNz") + }, + description: "Authorization: Basic must not bypass CSRF when a session cookie is present", + }, + { + name: "authorization_bearer", + setHeader: func(r *http.Request) { + r.Header.Set("Authorization", "Bearer some-token") + }, + description: "Authorization: Bearer must not bypass CSRF when a session cookie is present", + }, + { + name: "authorization_bearer_mixed_case", + setHeader: func(r *http.Request) { + // Mixed-case scheme to ensure the old lower-case prefix check + // is not reintroduced as a guarded skip. + r.Header.Set("Authorization", "BeArEr some-token") + }, + description: "Mixed-case Bearer must not bypass CSRF when a session cookie is present", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + w := httptest.NewRecorder() + req := httptest.NewRequest("POST", "/api/test", nil) + tc.setHeader(req) + req.AddCookie(&http.Cookie{ + Name: "pulse_session", + Value: "test-session-id-1234567890", + }) + + if CheckCSRF(w, req) { + t.Fatal(tc.description) + } + }) + } +} diff --git a/internal/api/security_test.go b/internal/api/security_test.go index 01914528d..83b5d0b04 100644 --- a/internal/api/security_test.go +++ b/internal/api/security_test.go @@ -1369,39 +1369,41 @@ func TestCheckCSRF_SafeMethods(t *testing.T) { } } -func TestCheckCSRF_APITokenAuth(t *testing.T) { +func TestCheckCSRF_APITokenAuth_NoSessionCookie(t *testing.T) { w := httptest.NewRecorder() req := httptest.NewRequest("POST", "/api/test", nil) req.Header.Set("X-API-Token", "some-api-token") - // API token auth bypasses CSRF check + // No session cookie => no CSRF check needed (the API client is purely + // header-authenticated). Regression coverage for the header+cookie case + // lives in security_regression_test.go. result := CheckCSRF(w, req) if !result { - t.Error("CheckCSRF should return true when X-API-Token is present") + t.Error("CheckCSRF should return true when X-API-Token is present and no session cookie") } } -func TestCheckCSRF_BasicAuth(t *testing.T) { +func TestCheckCSRF_BasicAuth_NoSessionCookie(t *testing.T) { w := httptest.NewRecorder() req := httptest.NewRequest("POST", "/api/test", nil) req.Header.Set("Authorization", "Basic dXNlcjpwYXNz") - // Basic auth bypasses CSRF check + // No session cookie => no CSRF check needed. result := CheckCSRF(w, req) if !result { - t.Error("CheckCSRF should return true when Basic Authorization header is present") + t.Error("CheckCSRF should return true when Basic Authorization header is present and no session cookie") } } -func TestCheckCSRF_BearerAuth(t *testing.T) { +func TestCheckCSRF_BearerAuth_NoSessionCookie(t *testing.T) { w := httptest.NewRecorder() req := httptest.NewRequest("POST", "/api/test", nil) req.Header.Set("Authorization", "Bearer some-token") - // Bearer auth bypasses CSRF check for token-based API clients. + // No session cookie => no CSRF check needed. result := CheckCSRF(w, req) if !result { - t.Error("CheckCSRF should return true when Bearer Authorization header is present") + t.Error("CheckCSRF should return true when Bearer Authorization header is present and no session cookie") } }