From 231471be93fb3cd76a48e78598ae43e6deebe575 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 14:01:03 +0100 Subject: [PATCH] Require POST for Assistant session mutation routes The session router dispatches /api/ai/sessions/{id}/abort, summarize, fork, undo and redo by path alone, and none of those handlers checked the method, so a GET or HEAD cancelled a running turn, compacted or rewound a transcript, or persisted a fork. The demo-mode guard admits every GET and HEAD as a read, so a public-demo visitor could mutate sessions despite the read-only mode. The CSRF check also skips GET and HEAD, and SameSite=Lax session cookies ride cross-site top-level GET navigations, so a link carrying a session ID could do the same to a signed-in user. Each mutating handler now admits only POST and answers anything else with 405 and Allow: POST before resolving the Assistant service. Steer already refused non-POST but sent no Allow header; it now shares the same guard. The checks sit in the handlers, where steer, chat and the approval handlers already own their method contracts. messages (GET) and the unsupported diff/revert/unrevert routes are unchanged. Every caller (aiChat.ts, the mobile app and its OTA abort probe) already sends POST. --- .../v6/internal/subsystems/agent-lifecycle.md | 6 + .../v6/internal/subsystems/ai-runtime.md | 6 + .../v6/internal/subsystems/api-contracts.md | 14 ++ .../internal/subsystems/storage-recovery.md | 6 + internal/api/ai_handler.go | 32 ++- internal/api/ai_handlers_more_test.go | 204 ++++++++++++++++++ 6 files changed, 266 insertions(+), 2 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index b8ed3f012b..2705524f45 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -4703,6 +4703,12 @@ agent-lifecycle operations. If those routes are called directly, the API must fail them as unsupported rather than presenting file diffs or reverts as agent command rollback, enrollment repair, update rollback, or fleet-control authority. +Assistant session mutations (`/abort`, `/summarize`, `/fork`, `/undo`, +`/redo`, `/steer` under `/api/ai/sessions/{id}`) accept only `POST`; any other +method that reaches those handlers gets `405` before touching session state. +That method guard +is AI-runtime transport hardening in `internal/api/ai_handler.go`; it adds no +agent command, enrollment, update, or fleet-control path. That same shared dependency now also assumes hosted cloud handoff authorizes tenant org access before browser lifecycle continues. Lifecycle-adjacent opens into hosted workspaces may depend on `internal/api/cloud_handoff_handlers.go`, diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 48b93c4c1a..31337fb6a3 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -3556,6 +3556,12 @@ query...`, and `Reading storage...` before streamed tool arguments are answer (nothing streaming, no error block, no pending approval or question, and a user prompt available to re-send) because the undo boundary only operates on the last durable turn. + Session mutations stay POST-only at the handler: abort, summarize, fork, + undo, redo, and steer refuse every other method with `405` and + `Allow: POST` before resolving the Assistant service. A `GET` or `HEAD` + that the demo-mode read-only guard or the CSRF check admits as a read + therefore cannot cancel a running turn, compact or rewind a transcript, + or persist a fork. The empty Assistant drawer may surface recent non-empty sessions as direct resume actions using the backend session list already owned by the drawer; it must not create a parallel recent-chat store or product-authored prompt diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 978b841439..d7a3f245b5 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -2088,6 +2088,20 @@ payload shape change when the portal presents compact client rows. index; clients must treat the endpoint's `accepted:true` as inbox receipt, not delivery. Browser clients must use the shared `AIChatAPI.steerSession(sessionId, { prompt, clientMessageId })` helper. + Every Assistant session mutation sub-resource under + `/api/ai/sessions/{id}` (`/abort`, `/summarize`, `/fork`, `/undo`, + `/redo`, `/steer`) accepts only `POST`. Any other method that reaches the + handler returns `405` with `Allow: POST` through + `requireAssistantSessionMutationMethod` in `internal/api/ai_handler.go`, + before the session service is resolved (earlier layers still answer + first: `Router.ServeHTTP` returns `200` for `OPTIONS` preflight, and + auth, scope, CSRF, and demo-mode guards keep their own refusals). The + session router dispatches these sub-resources by path alone, and the + demo-mode read-only guard and the CSRF check both treat `GET` and `HEAD` + as reads, so the method contract belongs to the mutating handler rather + than to its callers. `GET /api/ai/sessions/{id}/messages` stays the read + path, and the unsupported `/diff`, `/revert`, and `/unrevert` handlers + keep answering `501` for every method that reaches them. OpenCode-style file diff/revert session routes are deliberately not part of Pulse's supported Assistant session contract: Pulse sessions do not own local code-file edits, and infrastructure mutations must be reviewed diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 53b6bd3c34..7f2506f6ab 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -1404,6 +1404,12 @@ state only. A restored prompt or restored message count may help an operator continue a protected-item investigation, but it must not become backup coverage evidence, recovery freshness, restore entitlement, storage-owner identity, approval policy, or a provider-local recovery command handoff. +The POST-only guard on Assistant session mutations (`/abort`, +`/summarize`, `/fork`, `/undo`, `/redo`, `/steer`; `405` with `Allow: POST` +for any other method that reaches the handler) is transport hardening in +`internal/api/ai_handler.go`. +It opens no storage, backup, or recovery read or write path, and a rejected +request never reaches session state. Approved Assistant tool execution through `internal/api/router_routes_ai_relay.go` is also adjacent API/AI action plumbing for storage/recovery consumers. `AssistantToolExecutor` / `ApprovedAssistantToolExecutor` may execute an already diff --git a/internal/api/ai_handler.go b/internal/api/ai_handler.go index aff2273af3..d494e86374 100644 --- a/internal/api/ai_handler.go +++ b/internal/api/ai_handler.go @@ -2971,8 +2971,25 @@ func (h *AIHandler) HandleMessages(w http.ResponseWriter, r *http.Request, sessi json.NewEncoder(w).Encode(messages) } +// requireAssistantSessionMutationMethod admits only POST to an Assistant +// session mutation (abort, summarize, fork, undo, redo, steer). The session +// router dispatches these sub-resources by path alone, and GET/HEAD pass both +// the demo-mode read-only guard and the CSRF check while SameSite=Lax cookies +// ride cross-site top-level GET navigations, so each mutation owns its method. +func requireAssistantSessionMutationMethod(w http.ResponseWriter, r *http.Request) bool { + if r.Method == http.MethodPost { + return true + } + w.Header().Set("Allow", http.MethodPost) + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return false +} + // HandleAbort handles POST /api/ai/sessions/{id}/abort func (h *AIHandler) HandleAbort(w http.ResponseWriter, r *http.Request, sessionID string) { + if !requireAssistantSessionMutationMethod(w, r) { + return + } ctx := r.Context() if !h.IsRunning(ctx) { http.Error(w, "Pulse Assistant is not running", http.StatusServiceUnavailable) @@ -3187,6 +3204,9 @@ func (h *AIHandler) recordFirstPartyWorkflowPromptActivity(ctx context.Context, // HandleSummarize handles POST /api/ai/sessions/{id}/summarize // Compresses context when nearing model limits func (h *AIHandler) HandleSummarize(w http.ResponseWriter, r *http.Request, sessionID string) { + if !requireAssistantSessionMutationMethod(w, r) { + return + } ctx := r.Context() if !h.IsRunning(ctx) { http.Error(w, "Pulse Assistant is not running", http.StatusServiceUnavailable) @@ -3218,6 +3238,9 @@ func (h *AIHandler) HandleDiff(w http.ResponseWriter, r *http.Request, sessionID // HandleFork handles POST /api/ai/sessions/{id}/fork // Creates a branch point in the conversation func (h *AIHandler) HandleFork(w http.ResponseWriter, r *http.Request, sessionID string) { + if !requireAssistantSessionMutationMethod(w, r) { + return + } ctx := r.Context() if !h.IsRunning(ctx) { http.Error(w, "Pulse Assistant is not running", http.StatusServiceUnavailable) @@ -3244,6 +3267,9 @@ func (h *AIHandler) HandleFork(w http.ResponseWriter, r *http.Request, sessionID // The body is optional; retry/regenerate sends SessionTurnUndoOptions with the // prompt being re-run so a stale retry can never remove a different turn. func (h *AIHandler) HandleUndoLastTurn(w http.ResponseWriter, r *http.Request, sessionID string) { + if !requireAssistantSessionMutationMethod(w, r) { + return + } ctx := r.Context() if !h.IsRunning(ctx) { http.Error(w, "Pulse Assistant is not running", http.StatusServiceUnavailable) @@ -3276,6 +3302,9 @@ func (h *AIHandler) HandleUndoLastTurn(w http.ResponseWriter, r *http.Request, s // HandleRedoLastTurn handles POST /api/ai/sessions/{id}/redo. func (h *AIHandler) HandleRedoLastTurn(w http.ResponseWriter, r *http.Request, sessionID string) { + if !requireAssistantSessionMutationMethod(w, r) { + return + } ctx := r.Context() if !h.IsRunning(ctx) { http.Error(w, "Pulse Assistant is not running", http.StatusServiceUnavailable) @@ -3303,8 +3332,7 @@ func (h *AIHandler) HandleRedoLastTurn(w http.ResponseWriter, r *http.Request, s // loop; accepted=false with a reason is a normal outcome and the client // falls back to the ordinary follow-up queue. func (h *AIHandler) HandleSteerSession(w http.ResponseWriter, r *http.Request, sessionID string) { - if r.Method != http.MethodPost { - http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + if !requireAssistantSessionMutationMethod(w, r) { return } ctx := r.Context() diff --git a/internal/api/ai_handlers_more_test.go b/internal/api/ai_handlers_more_test.go index 52339f4893..3e81a3e649 100644 --- a/internal/api/ai_handlers_more_test.go +++ b/internal/api/ai_handlers_more_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "reflect" "strings" + "sync" "testing" "time" "unsafe" @@ -14,6 +15,7 @@ import ( "github.com/gorilla/websocket" "github.com/rcourtman/pulse-go-rewrite/internal/agentexec" "github.com/rcourtman/pulse-go-rewrite/internal/ai" + "github.com/rcourtman/pulse-go-rewrite/internal/ai/chat" "github.com/rcourtman/pulse-go-rewrite/internal/ai/cost" "github.com/rcourtman/pulse-go-rewrite/internal/ai/memory" "github.com/rcourtman/pulse-go-rewrite/internal/alerts" @@ -22,6 +24,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{} @@ -795,3 +798,204 @@ func TestHandleExportAICostHistoryCarriesPromptCacheBuckets(t *testing.T) { t.Fatalf("csv row missing cache buckets or cache-rate pricing: %s", lines[1]) } } + +// sessionMutationRecordingAIService records every Assistant session mutation +// and every running check that reaches the service, so a rejected request can +// be proven to stop before the handler touches the service at all. +type sessionMutationRecordingAIService struct { + capturingAIService + mu sync.Mutex + mutations []string + runningCalls int +} + +func newSessionMutationRecordingAIService() *sessionMutationRecordingAIService { + return &sessionMutationRecordingAIService{capturingAIService: capturingAIService{running: true}} +} + +func (s *sessionMutationRecordingAIService) record(op, sessionID string) { + s.mu.Lock() + defer s.mu.Unlock() + s.mutations = append(s.mutations, op+":"+sessionID) +} + +func (s *sessionMutationRecordingAIService) recorded() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.mutations...) +} + +func (s *sessionMutationRecordingAIService) IsRunning() bool { + s.mu.Lock() + defer s.mu.Unlock() + s.runningCalls++ + return s.capturingAIService.running +} + +func (s *sessionMutationRecordingAIService) runningChecks() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.runningCalls +} + +func (s *sessionMutationRecordingAIService) AbortSession(ctx context.Context, sessionID string) error { + s.record("abort", sessionID) + return nil +} + +func (s *sessionMutationRecordingAIService) SummarizeSession(ctx context.Context, sessionID string) (map[string]interface{}, error) { + s.record("summarize", sessionID) + return map[string]interface{}{"success": true}, nil +} + +func (s *sessionMutationRecordingAIService) ForkSession(ctx context.Context, sessionID string) (*chat.Session, error) { + s.record("fork", sessionID) + return &chat.Session{ID: sessionID + "-fork"}, nil +} + +func (s *sessionMutationRecordingAIService) UndoLastTurn(ctx context.Context, sessionID string, opts chat.SessionTurnUndoOptions) (*chat.SessionTurnUndoResult, error) { + s.record("undo", sessionID) + return &chat.SessionTurnUndoResult{Success: true, SessionID: sessionID}, nil +} + +func (s *sessionMutationRecordingAIService) RedoLastTurn(ctx context.Context, sessionID string) (*chat.SessionTurnRedoResult, error) { + s.record("redo", sessionID) + return &chat.SessionTurnRedoResult{Success: true, SessionID: sessionID}, nil +} + +func (s *sessionMutationRecordingAIService) SteerSession(ctx context.Context, sessionID string, req chat.SessionSteerRequest) (*chat.SessionSteerResult, error) { + s.record("steer", sessionID) + return &chat.SessionSteerResult{Accepted: true, SessionID: sessionID}, nil +} + +var assistantSessionMutationSubresources = []string{"abort", "summarize", "fork", "undo", "redo", "steer"} + +func TestAssistantSessionMutationsRequirePost(t *testing.T) { + for _, sub := range assistantSessionMutationSubresources { + for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodPut, http.MethodDelete} { + t.Run(sub+"/"+method, func(t *testing.T) { + svc := newSessionMutationRecordingAIService() + handler := &AIHandler{} + setUnexportedField(t, handler, "defaultService", svc) + router := &Router{aiHandler: handler} + + req := httptest.NewRequest(method, "/api/ai/sessions/session-1/"+sub, nil) + rec := httptest.NewRecorder() + router.routeAISessions(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("%s %s status = %d, want %d", method, sub, rec.Code, http.StatusMethodNotAllowed) + } + if allow := rec.Header().Get("Allow"); allow != http.MethodPost { + t.Fatalf("%s %s Allow = %q, want %q", method, sub, allow, http.MethodPost) + } + if got := svc.recorded(); len(got) != 0 { + t.Fatalf("%s %s reached the service: %v", method, sub, got) + } + if checks := svc.runningChecks(); checks != 0 { + t.Fatalf("%s %s resolved the service (%d running checks) before rejecting the method", method, sub, checks) + } + }) + } + + t.Run(sub+"/POST", func(t *testing.T) { + svc := newSessionMutationRecordingAIService() + handler := &AIHandler{} + setUnexportedField(t, handler, "defaultService", svc) + router := &Router{aiHandler: handler} + + body := "" + if sub == "steer" { + body = `{"prompt":"check the backup job too"}` + } + req := httptest.NewRequest(http.MethodPost, "/api/ai/sessions/session-1/"+sub, strings.NewReader(body)) + rec := httptest.NewRecorder() + router.routeAISessions(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("POST %s status = %d, want %d: %s", sub, rec.Code, http.StatusOK, rec.Body.String()) + } + if got := svc.recorded(); !reflect.DeepEqual(got, []string{sub + ":session-1"}) { + t.Fatalf("POST %s mutations = %v, want [%s:session-1]", sub, got, sub) + } + }) + } +} + +func TestAssistantSessionReadSubresourcesKeepTheirMethods(t *testing.T) { + svc := newSessionMutationRecordingAIService() + handler := &AIHandler{} + setUnexportedField(t, handler, "defaultService", svc) + router := &Router{aiHandler: handler} + + messagesReq := httptest.NewRequest(http.MethodGet, "/api/ai/sessions/session-1/messages", nil) + messagesRec := httptest.NewRecorder() + router.routeAISessions(messagesRec, messagesReq) + if messagesRec.Code != http.StatusOK { + t.Fatalf("GET messages status = %d, want %d: %s", messagesRec.Code, http.StatusOK, messagesRec.Body.String()) + } + + diffReq := httptest.NewRequest(http.MethodGet, "/api/ai/sessions/session-1/diff", nil) + diffRec := httptest.NewRecorder() + router.routeAISessions(diffRec, diffReq) + if diffRec.Code != http.StatusNotImplemented { + t.Fatalf("GET diff status = %d, want %d: %s", diffRec.Code, http.StatusNotImplemented, diffRec.Body.String()) + } + + if got := svc.recorded(); len(got) != 0 { + t.Fatalf("read sub-resources mutated the session: %v", got) + } +} + +// TestDemoModeRouterRejectsAssistantSessionMutationsOverSafeMethods drives the +// production handler chain: the demo-mode guard admits GET and HEAD as reads, +// so the session mutation itself must refuse them. +func TestDemoModeRouterRejectsAssistantSessionMutationsOverSafeMethods(t *testing.T) { + tempDir := t.TempDir() + hashed, err := internalauth.HashPassword("Password!1") + if err != nil { + t.Fatalf("hash password: %v", err) + } + cfg := &config.Config{ + DataPath: tempDir, + ConfigPath: tempDir, + AuthUser: "admin", + AuthPass: hashed, + DemoMode: true, + } + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + t.Cleanup(router.shutdownBackgroundWorkers) + svc := newSessionMutationRecordingAIService() + setUnexportedField(t, router.aiHandler, "defaultService", svc) + + serve := func(method, path string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, nil) + req.SetBasicAuth("admin", "Password!1") + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + return rec + } + + for _, sub := range assistantSessionMutationSubresources { + for _, method := range []string{http.MethodGet, http.MethodHead} { + rec := serve(method, "/api/ai/sessions/session-1/"+sub) + if rec.Header().Get("X-Demo-Mode") != "true" { + t.Fatalf("%s %s did not pass through the demo-mode guard", method, sub) + } + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("demo %s %s status = %d, want %d: %s", method, sub, rec.Code, http.StatusMethodNotAllowed, rec.Body.String()) + } + } + if rec := serve(http.MethodPost, "/api/ai/sessions/session-1/"+sub); rec.Code != http.StatusForbidden { + t.Fatalf("demo POST %s status = %d, want %d: %s", sub, rec.Code, http.StatusForbidden, rec.Body.String()) + } + } + if got := svc.recorded(); len(got) != 0 { + t.Fatalf("demo-mode requests mutated Assistant sessions: %v", got) + } + + if rec := serve(http.MethodGet, "/api/ai/sessions/session-1/messages"); rec.Code != http.StatusOK { + t.Fatalf("demo GET messages status = %d, want %d: %s", rec.Code, http.StatusOK, rec.Body.String()) + } +}