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") } }