diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index a2ee20031e..6f04936296 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8927,6 +8927,15 @@ their own heartbeat cutoff (`fleethealth.AgentStaleThreshold`, which is separate from the monitoring reporting lease). Agent registration, enrolment, install, update, removal and report identity are unchanged. +### Safe methods refused on discovery settings and SAML logout + +`internal/api/method_guard.go`, `internal/api/discovery_handlers.go` and +`internal/api/saml_handlers.go` changed only so `/api/discovery/settings` and +`/api/saml/{id}/logout` answer `405` to every method outside their allowed set, +`GET` and `HEAD` included. The settings write still takes `PUT` or `POST` and +logout still takes `POST`. No agent registration, enrolment, install, update or +removal path changed. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index f415badc7d..361bbedaa8 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -3896,6 +3896,13 @@ counters exist to measure. onboarding telemetry. Customer diagnostics may expose runtime health, supportability, and sanitized troubleshooting state; admin analytics must stay behind admin-owned metrics routes. +25. Handlers that perform a caller-requested write (settings, sessions, + credentials, workflow or resource state) while answering `GET` or `HEAD`. + A route registered without a mux method pattern, or a sub-resource + dispatched by path, must refuse safe methods in the mutating handler + through `requireRequestMethod` in `internal/api/method_guard.go` (or an + equivalent explicit check) instead of relying on its callers, the + demo-mode read-only guard, or the CSRF check. ## Completion Obligations @@ -5136,6 +5143,33 @@ pass the bounded non-symlink regular-file boundary in `internal/api/sso_outbound.go`. Cloud handoff redirects consume the shared host-local redirect validator rather than carrying route-local prefix checks. +A handler that performs a caller-requested write behind a route whose mux +pattern carries no method must refuse `GET` and `HEAD` itself. +`requireRequestMethod` in `internal/api/method_guard.go` is the shared +handler-level guard: it admits only the listed methods and answers any other +method that reaches the handler with `405` and an `Allow` header naming them. +The demo-mode read-only guard and the CSRF check both admit `GET` and `HEAD` as +reads, and the `SameSite=Lax` session cookie rides cross-site top-level `GET` +navigations, so the method contract belongs to the mutating handler rather than +to its callers. `/api/discovery/settings` accepts `PUT`, its documented method, +and `POST`, which it took before the guard existed. SP-initiated SAML logout at +`/api/saml/{id}/logout` accepts only `POST`, matching the `/api/logout` +fallback it delegates to. The IdP's LogoutResponse keeps returning to +`/api/saml/{id}/slo`, which must accept the HTTP-Redirect binding's `GET` and +validates the signed response before it touches the session. +`TestDemoModeSafeMethodsCannotReachRouteMutations` in +`internal/api/ai_handlers_more_test.go` drives both routes through the full +`DEMO_MODE` router and proves the threshold and the session survive. +The rule does not cover `GET` flows the protocol or the browser handoff +requires (OIDC login and callback, SAML login initiation and SLO, magic-link +verify, cloud handoff, the checkout start and activation bridges, websocket +handshakes), or reads that run one-time lazy initialization or idempotent +reconciliation, such as the license and onboarding-overflow bootstrap behind +the entitlement reads and the action-expiry sweep behind the action reads. +Single-resource report generation at `GET /api/admin/reports/generate` still +runs the configured AI narrator, a paid provider call recorded in the cost +ledger; that is a known open exception, not part of this contract. + Alert delivery diagnosis is a read-only monitoring API contract. `GET /api/alerts/delivery-diagnosis?alertIdentifier=` returns the alert manager's current delivery-policy projection for one active alert, including diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index d1199c9224..d5112ce305 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6867,6 +6867,15 @@ The performance report and reporting runtime snapshot handlers apply the same test before tabulating a disk temperature. That changes only which held value a report shows, not any storage or recovery path. +### Safe methods refused on discovery settings and SAML logout + +`internal/api/method_guard.go` adds `requireRequestMethod`, and +`internal/api/discovery_handlers.go` and `internal/api/saml_handlers.go` use it +so `/api/discovery/settings` (`PUT` or `POST`) and `/api/saml/{id}/logout` +(`POST`) answer `405` to any other method, `GET` and `HEAD` included, instead +of changing the discovery staleness threshold or clearing a session. No +storage, retention, backup, migration or recovery path is added or moved. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` no longer exempts every request carrying diff --git a/internal/api/ai_handlers_more_test.go b/internal/api/ai_handlers_more_test.go index 52339f4893..d4838850f2 100644 --- a/internal/api/ai_handlers_more_test.go +++ b/internal/api/ai_handlers_more_test.go @@ -22,6 +22,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/monitoring" "github.com/rcourtman/pulse-go-rewrite/internal/servicediscovery" unifiedresources "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" + internalauth "github.com/rcourtman/pulse-go-rewrite/pkg/auth" ) type stubMetadataProvider struct{} @@ -521,6 +522,78 @@ func TestAISettingsHandler_DiscoveryStoreAccessors(t *testing.T) { } } +// In DEMO_MODE the demo middleware admits every GET and HEAD as a read, and +// the CSRF check skips them, so a mutating handler reachable by a safe method +// is a write any demo visitor can make. These routes dispatch without a mux +// method pattern, so each handler must refuse GET and HEAD itself. +func TestDemoModeSafeMethodsCannotReachRouteMutations(t *testing.T) { + dataPath := t.TempDir() + hashed, err := internalauth.HashPassword("demo-router-test-password") + if err != nil { + t.Fatalf("HashPassword: %v", err) + } + cfg := &config.Config{ + DataPath: dataPath, + ConfigPath: dataPath, + DemoMode: true, + AuthUser: "demo", + AuthPass: hashed, + } + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + cleanupTestRouter(t, router) + handler := router.Handler() + + discovery := servicediscovery.NewService(nil, nil, servicediscovery.DefaultConfig()) + router.SetDiscoveryService(discovery) + ageBefore := discovery.GetMaxDiscoveryAge() + + router.samlManager.services["okta"] = newTestSAMLService(t, "okta", ` + + + + + +`) + samlSession := generateSessionToken() + GetSessionStore().CreateSAMLSession(samlSession, time.Hour, "agent", "127.0.0.1", "demo", &SAMLTokenInfo{ + ProviderID: "okta", + NameID: "name-id", + SessionIndex: "sess-1", + }) + + cases := []struct { + path string + body string + allow string + }{ + {path: "/api/discovery/settings", body: `{"max_discovery_age_days":10}`, allow: "PUT, POST"}, + {path: "/api/saml/okta/logout", allow: http.MethodPost}, + } + for _, tc := range cases { + for _, method := range []string{http.MethodGet, http.MethodHead} { + req := httptest.NewRequest(method, tc.path, strings.NewReader(tc.body)) + req.SetBasicAuth("demo", "demo-router-test-password") + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: samlSession}) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("%s %s status = %d, want %d: %s", method, tc.path, rec.Code, http.StatusMethodNotAllowed, rec.Body.String()) + } + if allow := rec.Header().Get("Allow"); allow != tc.allow { + t.Fatalf("%s %s Allow = %q, want %q", method, tc.path, allow, tc.allow) + } + } + } + + if got := discovery.GetMaxDiscoveryAge(); got != ageBefore { + t.Fatalf("safe methods changed the max discovery age: %s -> %s", ageBefore, got) + } + if !ValidateSession(samlSession) { + t.Fatal("safe methods cleared the SAML session") + } +} + func TestAISettingsHandler_GetConfig_NonDefaultFallsBackWhenMultiTenantUnavailable(t *testing.T) { handler := newTestAISettingsHandler(&config.Config{APIToken: "token"}, config.NewConfigPersistence(t.TempDir()), nil) ctx := context.WithValue(context.Background(), OrgIDContextKey, "acme") diff --git a/internal/api/discovery_handlers.go b/internal/api/discovery_handlers.go index 54dff9e5c7..f77923bb17 100644 --- a/internal/api/discovery_handlers.go +++ b/internal/api/discovery_handlers.go @@ -881,7 +881,12 @@ func (h *DiscoveryHandlers) HandleRunDiscovery(w http.ResponseWriter, r *http.Re // HandleUpdateSettings handles PUT /api/discovery/settings // Allows updating discovery settings like the staleness threshold. +// PUT is the documented method. POST stays accepted because the route took any +// method before; GET and HEAD are refused so a read can never write. func (h *DiscoveryHandlers) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { + if !requireRequestMethod(w, r, http.MethodPut, http.MethodPost) { + return + } if h.service == nil { writeDiscoveryError(w, http.StatusServiceUnavailable, "discovery service not configured") return diff --git a/internal/api/discovery_handlers_test.go b/internal/api/discovery_handlers_test.go index 4f099eefb2..0fa57d3fce 100644 --- a/internal/api/discovery_handlers_test.go +++ b/internal/api/discovery_handlers_test.go @@ -476,6 +476,41 @@ func TestHandleUpdateSettings(t *testing.T) { // We can't check service private field easily, but we check 200 OK. } +// GET and HEAD pass the demo-mode read-only guard and the CSRF check, so the +// settings write must refuse them even when they carry a JSON body. +func TestHandleUpdateSettingsRefusesSafeMethods(t *testing.T) { + h, service, _ := setupDiscoveryHandlers(t) + before := service.GetMaxDiscoveryAge() + reqBody := `{"max_discovery_age_days": 10}` + if before == 10*24*time.Hour { + t.Fatal("test body must change the default max discovery age") + } + + for _, method := range []string{http.MethodGet, http.MethodHead} { + req := httptest.NewRequest(method, "/api/discovery/settings", bytes.NewBufferString(reqBody)) + req.SetBasicAuth("admin", "admin") + w := httptest.NewRecorder() + + h.HandleUpdateSettings(w, req) + + assert.Equal(t, http.StatusMethodNotAllowed, w.Code, method) + assert.Equal(t, "PUT, POST", w.Header().Get("Allow"), method) + assert.Equal(t, before, service.GetMaxDiscoveryAge(), method) + } + + for _, method := range []string{http.MethodPut, http.MethodPost} { + service.SetMaxDiscoveryAge(before) + req := httptest.NewRequest(method, "/api/discovery/settings", bytes.NewBufferString(reqBody)) + req.SetBasicAuth("admin", "admin") + w := httptest.NewRecorder() + + h.HandleUpdateSettings(w, req) + + assert.Equal(t, http.StatusOK, w.Code, method) + assert.Equal(t, 10*24*time.Hour, service.GetMaxDiscoveryAge(), method) + } +} + func TestHandleListByType(t *testing.T) { h, _, store := setupDiscoveryHandlers(t) diff --git a/internal/api/method_guard.go b/internal/api/method_guard.go new file mode 100644 index 0000000000..9e73728668 --- /dev/null +++ b/internal/api/method_guard.go @@ -0,0 +1,23 @@ +package api + +import ( + "net/http" + "strings" +) + +// requireRequestMethod admits only the listed methods to a handler that +// changes state, answering anything else with 405 and an Allow header. Bare +// mux patterns and path-dispatched routes do not filter methods, and GET/HEAD +// pass both the demo-mode read-only guard and the CSRF check while +// SameSite=Lax session cookies ride cross-site top-level GET navigations, so a +// mutating handler must refuse safe methods itself. +func requireRequestMethod(w http.ResponseWriter, r *http.Request, allowed ...string) bool { + for _, method := range allowed { + if r.Method == method { + return true + } + } + w.Header().Set("Allow", strings.Join(allowed, ", ")) + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return false +} diff --git a/internal/api/saml_handlers.go b/internal/api/saml_handlers.go index f4706ec4a8..6ce686e585 100644 --- a/internal/api/saml_handlers.go +++ b/internal/api/saml_handlers.go @@ -377,8 +377,13 @@ func (r *Router) handleSAMLMetadata(w http.ResponseWriter, req *http.Request) { w.Write(metadata) } -// handleSAMLLogout handles SAML Single Logout +// handleSAMLLogout handles SP-initiated SAML Single Logout. It is POST-only, +// like the /api/logout fallback: the IdP's LogoutResponse returns to /slo, not +// here, so a GET to this path could only be a forced logout. func (r *Router) handleSAMLLogout(w http.ResponseWriter, req *http.Request) { + if !requireRequestMethod(w, req, http.MethodPost) { + return + } providerID := extractSAMLProviderID(req.URL.Path, "logout") if providerID == "" { // Fall back to regular logout diff --git a/internal/api/saml_handlers_more_test.go b/internal/api/saml_handlers_more_test.go index ff7ac9f9ac..10c04637bd 100644 --- a/internal/api/saml_handlers_more_test.go +++ b/internal/api/saml_handlers_more_test.go @@ -247,7 +247,9 @@ func TestHandleSAMLSLO_RejectsIDPInitiatedLogoutRequest(t *testing.T) { } func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { - InitSessionStore(t.TempDir()) + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} metadataXML := ` @@ -265,7 +267,7 @@ func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { SessionIndex: "sess-1", }) - req := httptest.NewRequest(http.MethodGet, "/api/saml/okta/logout", nil) + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) rec := httptest.NewRecorder() @@ -280,7 +282,9 @@ func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { } func TestHandleSAMLLogout_SLOSuccess(t *testing.T) { - InitSessionStore(t.TempDir()) + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} metadataXML := ` @@ -299,7 +303,7 @@ func TestHandleSAMLLogout_SLOSuccess(t *testing.T) { SessionIndex: "sess-1", }) - req := httptest.NewRequest(http.MethodGet, "/api/saml/okta/logout", nil) + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) rec := httptest.NewRecorder() @@ -314,6 +318,64 @@ func TestHandleSAMLLogout_SLOSuccess(t *testing.T) { } } +// A cross-site top-level GET carries the SameSite=Lax session cookie and skips +// the CSRF check, so SP-initiated logout must not clear the session on GET or +// HEAD. +func TestHandleSAMLLogout_RefusesSafeMethods(t *testing.T) { + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) + + router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} + metadataXML := ` + + + + + +` + router.samlManager.services["okta"] = newTestSAMLService(t, "okta", metadataXML) + + token := generateSessionToken() + GetSessionStore().CreateSAMLSession(token, time.Hour, "agent", "127.0.0.1", "user", &SAMLTokenInfo{ + ProviderID: "okta", + NameID: "name-id", + SessionIndex: "sess-1", + }) + + for _, method := range []string{http.MethodGet, http.MethodHead} { + req := httptest.NewRequest(method, "/api/saml/okta/logout", nil) + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) + rec := httptest.NewRecorder() + + router.handleSAMLLogout(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("%s: expected status %d, got %d", method, http.StatusMethodNotAllowed, rec.Code) + } + if allow := rec.Header().Get("Allow"); allow != http.MethodPost { + t.Fatalf("%s: expected Allow %q, got %q", method, http.MethodPost, allow) + } + if loc := rec.Header().Get("Location"); loc != "" { + t.Fatalf("%s: expected no SLO redirect, got %q", method, loc) + } + if !ValidateSession(token) { + t.Fatalf("%s: SAML logout cleared the session", method) + } + } + + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) + rec := httptest.NewRecorder() + router.handleSAMLLogout(rec, req) + if rec.Code != http.StatusFound { + t.Fatalf("POST: expected status %d, got %d", http.StatusFound, rec.Code) + } + if ValidateSession(token) { + t.Fatal("POST: expected SAML logout to clear the session") + } +} + func TestExtractSAMLProviderID(t *testing.T) { if got := extractSAMLProviderID("/api/saml/okta/login", "login"); got != "okta" { t.Fatalf("expected okta, got %q", got) diff --git a/internal/api/saml_runtime_license_gating_test.go b/internal/api/saml_runtime_license_gating_test.go index 41e4ad6e0e..6a023b5dda 100644 --- a/internal/api/saml_runtime_license_gating_test.go +++ b/internal/api/saml_runtime_license_gating_test.go @@ -26,7 +26,7 @@ func TestSAMLRuntimeEndpointsUseCommunitySSOFeature(t *testing.T) { {http.MethodGet, "/api/saml/test-provider/login"}, {http.MethodPost, "/api/saml/test-provider/acs"}, {http.MethodGet, "/api/saml/test-provider/metadata"}, - {http.MethodGet, "/api/saml/test-provider/logout"}, + {http.MethodPost, "/api/saml/test-provider/logout"}, {http.MethodPost, "/api/saml/test-provider/slo"}, }