From 433f617db2b3f4d675e99abfd9a3dbb751fa68a6 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sat, 24 Oct 2026 05:21:01 +0100 Subject: [PATCH] Preserve explicit audit selections before reads and webhook updates Reject damaged query encoding and repeated singleton selectors in the canonical list, export and summary handlers before reading events. Require an explicit webhook urls array so absent selections cannot silently disable forwarding; intentional empty arrays, URL validation and tenant boundaries remain intact. Add executable handler/exporter and mutation preservation controls and update the owning contracts. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 8 + .../v6/internal/subsystems/api-contracts.md | 20 ++ .../internal/subsystems/storage-recovery.md | 8 + internal/api/activity_audit_handlers.go | 44 ++- internal/api/audit_handlers_test.go | 23 ++ .../api/audit_selection_validation_test.go | 269 ++++++++++++++++++ 6 files changed, 368 insertions(+), 4 deletions(-) create mode 100644 internal/api/audit_selection_validation_test.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 64ec02aa7f..ca50cb0bc7 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -4011,6 +4011,14 @@ existing export/summary non-pagination semantics. This changes only audit request validation, not agent identity, enrolment, collection or recovery. Fake-logger handler controls live in `internal/api/audit_filter_validation_test.go`. +The canonical audit read handlers also refuse damaged raw query encoding and +repeated named singleton selectors before any event read, rather than silently +losing a filter or taking its first value. Absent webhook URL selections are +not deliberate removal: POST/PUT require an explicit array, with `[]` retaining +its intentional clear semantics. `internal/api/audit_selection_validation_test.go` +pins read non-execution, selected wire results, tenant isolation and mutation +preservation/clear/restore. Agent operations and storage internals are unchanged. + ## Forbidden Paths 1. New install or update continuity behavior hidden only inside broad monitoring ownership. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index a34c4c68b9..8ec0e85cfa 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4082,6 +4082,26 @@ exporter are pinned by `internal/api/audit_filter_validation_test.go` with fake persistent loggers, including query non-execution and tenant isolation; `internal/api/audit_handlers_test.go` pins valid export/summary filter payloads. +List/export/summary parse the entire raw query with `url.ParseQuery`; damaged +encoding (including an invalid unknown pair) returns `400 invalid_query`, never +a partial selection. The named singleton parameters `event`, `user`, +`startTime`, `endTime`, `success`, `limit`, `offset`, `format` and `verify` may +appear at most once, including under encoded keys; repeated values return +`400 ambiguous_query`, even when identical. Valid encoded values, unknown +well-formed parameters and existing per-endpoint filter semantics remain. +Rejections occur before Query/Count/signature checks or export attachment +headers. `internal/api/audit_selection_validation_test.go` exercises the actual +handlers/exporter, scoped wire results and tenant isolation without a database. + +POST/PUT `/api/admin/webhooks/audit` require an explicit `urls` array. +`null`, `{}` and `{"urls":null}` return `400 invalid_request` without updating +existing destinations. `{"urls":[]}` is intentional removal of every endpoint; +non-empty selections still undergo all existing URL/SSRF validation before one +update. Body bounds, organisation/access/entitlement gates and best-effort +forwarding remain unchanged; these controls establish no receiver delivery. +The same test file pins both methods, unchanged state after refusal, deliberate +clear/restore, invalid-URL atomicity and tenant-scoped mutation. + ## Forbidden Paths 1. Handler-local payload shape drift without a contract test diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index bc0fcaf345..6c016bfeb9 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -2744,6 +2744,14 @@ This boundary adds no database/schema, retention, signing, quota or recovery change. Fake persistent-logger controls in `internal/api/audit_filter_validation_test.go` prove rejection without a store. +The canonical audit read handlers also refuse damaged raw query encoding and +repeated named singleton selectors before any event read, rather than silently +losing a filter or taking its first value. Absent webhook URL selections are +not deliberate removal: POST/PUT require an explicit array, with `[]` retaining +its intentional clear semantics. `internal/api/audit_selection_validation_test.go` +pins read non-execution, selected wire results, tenant isolation and mutation +preservation/clear/restore. Agent operations and storage internals are unchanged. + ## Forbidden Paths 1. Reintroducing storage or recovery product logic as ad hoc dashboard-only summaries without a canonical page-surface owner diff --git a/internal/api/activity_audit_handlers.go b/internal/api/activity_audit_handlers.go index fc914454b1..9c5146df97 100644 --- a/internal/api/activity_audit_handlers.go +++ b/internal/api/activity_audit_handlers.go @@ -99,7 +99,10 @@ type auditQueryValidationError struct { } func auditEventListFilterFromRequest(r *http.Request) (audit.QueryFilter, *auditQueryValidationError) { - query := r.URL.Query() + query, validationErr := auditEventQueryFromRequest(r) + if validationErr != nil { + return audit.QueryFilter{}, validationErr + } filter := audit.QueryFilter{ EventType: query.Get("event"), User: query.Get("user"), @@ -136,6 +139,27 @@ func auditEventListFilterFromRequest(r *http.Request) (audit.QueryFilter, *audit return filter, nil } +// URL.Query silently discards damaged pairs. An audit selection must be parsed +// completely, and singleton selectors must not depend on which value came first. +func auditEventQueryFromRequest(r *http.Request) (url.Values, *auditQueryValidationError) { + query, err := url.ParseQuery(r.URL.RawQuery) + if err != nil { + return nil, &auditQueryValidationError{ + code: "invalid_query", + message: "Invalid audit query encoding", + } + } + for _, key := range []string{"event", "user", "startTime", "endTime", "success", "limit", "offset", "format", "verify"} { + if len(query[key]) > 1 { + return nil, &auditQueryValidationError{ + code: "ambiguous_query", + message: fmt.Sprintf("Audit query parameter %s must be supplied at most once", key), + } + } + } + return query, nil +} + // A supplied malformed or empty bound must not become an unfiltered read. // Keep list, export and summary on the same time-window validation boundary. func auditTimeFilterFromQuery(query url.Values, filter *audit.QueryFilter) *auditQueryValidationError { @@ -283,9 +307,13 @@ func (h *AuditHandlers) HandleUpdateWebhooks(w http.ResponseWriter, r *http.Requ writeErrorResponse(w, http.StatusBadRequest, "invalid_request", "Invalid request body", nil) return } + if req.URLs == nil { + writeErrorResponse(w, http.StatusBadRequest, "invalid_request", "An explicit urls array is required; use [] to remove all audit webhooks", nil) + return + } // Validate all webhook URLs - var validatedURLs []string + validatedURLs := make([]string, 0, len(req.URLs)) for _, rawURL := range req.URLs { if err := validateWebhookURL(rawURL); err != nil { writeErrorResponse(w, http.StatusBadRequest, "invalid_url", fmt.Sprintf("Invalid webhook URL: %s", err.Error()), map[string]string{"url": rawURL}) @@ -443,7 +471,11 @@ func (h *AuditHandlers) HandleExportAuditEvents(w http.ResponseWriter, r *http.R return } - query := r.URL.Query() + query, validationErr := auditEventQueryFromRequest(r) + if validationErr != nil { + writeErrorResponse(w, http.StatusBadRequest, validationErr.code, validationErr.message, nil) + return + } // Parse format format := audit.ExportFormatJSON @@ -509,7 +541,11 @@ func (h *AuditHandlers) HandleAuditSummary(w http.ResponseWriter, r *http.Reques return } - query := r.URL.Query() + query, validationErr := auditEventQueryFromRequest(r) + if validationErr != nil { + writeErrorResponse(w, http.StatusBadRequest, validationErr.code, validationErr.message, nil) + return + } // Parse filter filter := audit.QueryFilter{ diff --git a/internal/api/audit_handlers_test.go b/internal/api/audit_handlers_test.go index f6bc728b4e..022d8075ce 100644 --- a/internal/api/audit_handlers_test.go +++ b/internal/api/audit_handlers_test.go @@ -407,6 +407,29 @@ func TestHandleUpdateWebhooks(t *testing.T) { } } +func TestHandleUpdateWebhooksRejectsNonArrayAndOversizedSelections(t *testing.T) { + for _, body := range []string{ + `{"urls":"https://8.8.8.8/audit"}`, + `{"urls":false}`, + `{"urls":[{}]}`, + strings.Repeat(" ", 64*1024) + `{"urls":[]}`, + } { + logger := newAuditSelectionLogger() + logger.urls = []string{"https://8.8.8.8/audit"} + setAuditLogger(t, logger) + rec := httptest.NewRecorder() + NewAuditHandlers().HandleUpdateWebhooks(rec, httptest.NewRequest(http.MethodPut, + "/api/admin/webhooks/audit", strings.NewReader(body))) + var payload APIError + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusBadRequest || payload.Code != "invalid_request" || logger.updates != 0 || len(logger.urls) != 1 { + t.Fatalf("invalid selection changed destination: status/code/updates = %d/%q/%d", rec.Code, payload.Code, logger.updates) + } + } +} + func TestHandleExportAuditEvents_NotPersistent(t *testing.T) { setAuditLogger(t, audit.NewConsoleLogger()) handler := NewAuditHandlers() diff --git a/internal/api/audit_selection_validation_test.go b/internal/api/audit_selection_validation_test.go new file mode 100644 index 0000000000..92a51d0d70 --- /dev/null +++ b/internal/api/audit_selection_validation_test.go @@ -0,0 +1,269 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/pkg/audit" +) + +// Exercise the canonical handlers and exporter without a database or receiver. +func TestAuditSelectionRejectsDamagedQuery(t *testing.T) { + queries := []string{ + "user=%zz", "event=%", "success=%zz", "startTime=%zz", "endTime=%zz", + "limit=%zz", "offset=%zz", "format=%zz", "verify=%zz", "%zz=operator", + "user=operator;event=login", "user=operator&user=%zz", "event=login&unknown=%zz", + } + for _, endpoint := range auditScopeEndpoints() { + for _, query := range queries { + t.Run(endpoint.name+"/"+query, func(t *testing.T) { + assertAuditSelectionRejected(t, endpoint.handle, query, "invalid_query") + }) + } + } +} + +func TestAuditSelectionRejectsRepeatedParameters(t *testing.T) { + values := map[string]string{ + "event": "login", "user": "operator", "success": "true", "startTime": "2026-01-01T00:00:00Z", + "endTime": "2026-01-02T00:00:00Z", "limit": "10", "offset": "0", "format": "json", "verify": "true", + } + for _, endpoint := range auditScopeEndpoints() { + for _, key := range []string{"event", "user", "success", "startTime", "endTime", "limit", "offset", "format", "verify"} { + for _, variant := range []string{"identical", "conflicting", "encoded_key"} { + t.Run(endpoint.name+"/"+key+"/"+variant, func(t *testing.T) { + first, second := key, url.QueryEscape(values[key]) + if variant == "conflicting" { + second = "different" + } + if variant == "encoded_key" { + first = fmt.Sprintf("%%%02x%s", key[0], key[1:]) + } + query := first + "=" + url.QueryEscape(values[key]) + "&" + key + "=" + second + assertAuditSelectionRejected(t, endpoint.handle, query, "ambiguous_query") + }) + } + } + } +} + +func assertAuditSelectionRejected(t *testing.T, handle http.HandlerFunc, query, code string) { + t.Helper() + logger := &auditScopeLogger{testAuditLogger: testAuditLogger{events: []audit.Event{{ID: "unselected-event", Signature: "synthetic"}}}} + setAuditLogger(t, logger) + rec := httptest.NewRecorder() + handle(rec, httptest.NewRequest(http.MethodGet, "/api/audit?"+query, nil)) + var body APIError + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusBadRequest || body.Code != code { + t.Errorf("status/code = %d/%q, want 400/%q", rec.Code, body.Code, code) + } + if logger.queries != 0 || logger.counts != 0 || logger.verifications != 0 { + t.Errorf("query/count/verify = %d/%d/%d, want no event read", logger.queries, logger.counts, logger.verifications) + } + if strings.Contains(rec.Body.String(), "unselected-event") || rec.Header().Get("Content-Disposition") != "" || rec.Header().Get("X-Event-Count") != "" { + t.Error("rejected selection returned event/export content") + } +} + +// A filter-backed, in-memory logger makes widening visible in the wire result. +type auditSelectionLogger struct { + auditScopeLogger + urls []string + updates int +} + +func (l *auditSelectionLogger) selected(f audit.QueryFilter) []audit.Event { + events := []audit.Event{} + for _, e := range l.events { + if f.EventType != "" && e.EventType != f.EventType || f.User != "" && e.User != f.User || f.Success != nil && e.Success != *f.Success || f.StartTime != nil && e.Timestamp.Before(*f.StartTime) || f.EndTime != nil && !e.Timestamp.Before(*f.EndTime) { + continue + } + events = append(events, e) + } + return events +} +func (l *auditSelectionLogger) Query(f audit.QueryFilter) ([]audit.Event, error) { + l.queries++ + l.lastQuery = f + events := l.selected(f) + if f.Offset >= len(events) { + return []audit.Event{}, nil + } + events = events[f.Offset:] + if f.Limit > 0 && len(events) > f.Limit { + events = events[:f.Limit] + } + return events, nil +} +func (l *auditSelectionLogger) Count(f audit.QueryFilter) (int, error) { + l.counts++ + l.lastCount = f + return len(l.selected(f)), nil +} +func (l *auditSelectionLogger) GetWebhookURLs() []string { return append([]string{}, l.urls...) } +func (l *auditSelectionLogger) UpdateWebhookURLs(urls []string) error { + l.updates++ + l.urls = append([]string{}, urls...) + return l.updateErr +} + +func newAuditSelectionLogger() *auditSelectionLogger { + at := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + return &auditSelectionLogger{auditScopeLogger: auditScopeLogger{testAuditLogger: testAuditLogger{verifyResult: true, events: []audit.Event{ + {ID: "selected-event", EventType: "login", User: "operator+é@example.invalid", Timestamp: at, Success: true, Signature: "synthetic"}, + {ID: "other-user", EventType: "login", User: "another", Timestamp: at, Success: true}, + {ID: "other-event", EventType: "logout", User: "operator+é@example.invalid", Timestamp: at, Success: true}, + {ID: "outside-window", EventType: "login", User: "operator+é@example.invalid", Timestamp: at.Add(-48 * time.Hour), Success: true}, + }}}} +} + +func TestAuditSelectionPreservesEncodedWireResults(t *testing.T) { + for _, endpoint := range auditScopeEndpoints() { + for _, format := range []string{"json", "csv"} { + t.Run(endpoint.name+"/"+format, func(t *testing.T) { + logger := newAuditSelectionLogger() + setAuditLogger(t, logger) + q := url.Values{"user": {"operator+é@example.invalid"}, "event": {"login"}, "startTime": {"2026-01-01T01:00:00+01:00"}, "endTime": {"2026-01-02T00:00:00Z"}, "verify": {"true"}, "format": {format}, "unknown": {"one", "two"}} + rec := httptest.NewRecorder() + endpoint.handle(rec, httptest.NewRequest(http.MethodGet, "/api/audit?"+q.Encode(), nil)) + if rec.Code != 200 || logger.queries != 1 || logger.lastQuery.User != q.Get("user") || logger.lastQuery.EventType != "login" { + t.Fatalf("status/query/filter = %d/%d/%+v", rec.Code, logger.queries, logger.lastQuery) + } + for _, id := range []string{"other-user", "other-event", "outside-window"} { + if strings.Contains(rec.Body.String(), id) { + t.Fatalf("widened selection contains %s", id) + } + } + if endpoint.name == "summary" { + var summary audit.ExportSummary + if err := json.Unmarshal(rec.Body.Bytes(), &summary); err != nil { + t.Fatal(err) + } + if summary.TotalEvents != 1 { + t.Fatalf("summary count = %d", summary.TotalEvents) + } + } else if !strings.Contains(rec.Body.String(), "selected-event") { + t.Fatal("selected event missing") + } + if endpoint.name == "export" && rec.Header().Get("X-Event-Count") != "1" { + t.Fatal("export count not scoped") + } + }) + } + } +} + +func TestAuditWebhookSelectionRequiresExplicitArray(t *testing.T) { + for _, method := range []string{http.MethodPost, http.MethodPut} { + for _, body := range []string{"null", "{}", `{"urls":null}`} { + t.Run(method+"/"+body, func(t *testing.T) { + logger := newAuditSelectionLogger() + logger.urls = []string{"https://8.8.8.8/audit"} + setAuditLogger(t, logger) + rec := httptest.NewRecorder() + NewAuditHandlers().HandleUpdateWebhooks(rec, httptest.NewRequest(method, "/api/admin/webhooks/audit", strings.NewReader(body))) + var payload APIError + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatal(err) + } + if rec.Code != 400 || payload.Code != "invalid_request" || logger.updates != 0 || !reflect.DeepEqual(logger.urls, []string{"https://8.8.8.8/audit"}) { + t.Fatalf("status/code/updates/urls = %d/%q/%d/%v", rec.Code, payload.Code, logger.updates, logger.urls) + } + }) + } + } +} + +func TestAuditWebhookSelectionExplicitClearRestoreAndValidation(t *testing.T) { + for _, method := range []string{http.MethodPost, http.MethodPut} { + t.Run(method, func(t *testing.T) { + logger := newAuditSelectionLogger() + logger.urls = []string{"https://8.8.8.8/audit"} + setAuditLogger(t, logger) + for _, tc := range []struct { + body string + status, updates int + urls []string + }{ + {`{"urls":["http://127.0.0.1/private"]}`, 400, 0, []string{"https://8.8.8.8/audit"}}, + {`{"urls":[]}`, 204, 1, []string{}}, + {`{"urls":["https://8.8.8.8/audit"]}`, 204, 2, []string{"https://8.8.8.8/audit"}}, + {`{"urls":["https://8.8.8.8/changed","http://127.0.0.1/private"]}`, 400, 2, []string{"https://8.8.8.8/audit"}}, + } { + rec := httptest.NewRecorder() + NewAuditHandlers().HandleUpdateWebhooks(rec, httptest.NewRequest(method, "/api/admin/webhooks/audit", strings.NewReader(tc.body))) + if rec.Code != tc.status || logger.updates != tc.updates || !reflect.DeepEqual(logger.urls, tc.urls) { + t.Fatalf("status/updates/urls = %d/%d/%v", rec.Code, logger.updates, logger.urls) + } + } + rec := httptest.NewRecorder() + NewAuditHandlers().HandleUpdateWebhooks(rec, httptest.NewRequest(http.MethodGet, "/api/admin/webhooks/audit", strings.NewReader(`{"urls":[]}`))) + if rec.Code != 405 || logger.updates != 2 { + t.Fatal("safe method mutated selection") + } + }) + } +} + +// Tenant selection still comes only from the authenticated request context. +type auditSelectionFactory map[string]*auditSelectionLogger + +func (f auditSelectionFactory) CreateLogger(path string) (audit.Logger, error) { + for org, l := range f { + if strings.HasSuffix(path, "/"+org+"/audit.db") { + return l, nil + } + } + return nil, context.Canceled +} + +func TestAuditSelectionTenantIsolation(t *testing.T) { + previous := GetTenantAuditManager() + factory := auditSelectionFactory{"org-a": newAuditSelectionLogger(), "org-b": newAuditSelectionLogger()} + for org, l := range factory { + l.urls = []string{"https://8.8.8.8/" + org} + l.events[0].ID = org + } + manager := audit.NewTenantLoggerManager(t.TempDir(), factory) + SetTenantAuditManager(manager) + t.Cleanup(func() { manager.Close(); SetTenantAuditManager(previous) }) + for _, org := range []string{"org-a", "org-b"} { + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/audit?user=operator%2B%C3%A9%40example.invalid&event=login&startTime=2026-01-01T00:00:00Z", nil) + req = req.WithContext(context.WithValue(req.Context(), OrgIDContextKey, org)) + NewAuditHandlers().HandleListAuditEvents(rec, req) + other := "org-a" + if org == other { + other = "org-b" + } + if rec.Code != 200 || !strings.Contains(rec.Body.String(), org) || strings.Contains(rec.Body.String(), other) { + t.Fatalf("tenant list: %d/%s", rec.Code, rec.Body.String()) + } + req = httptest.NewRequest(http.MethodPut, "/api/admin/webhooks/audit", strings.NewReader(`{"urls":null}`)) + req = req.WithContext(context.WithValue(req.Context(), OrgIDContextKey, org)) + rec = httptest.NewRecorder() + NewAuditHandlers().HandleUpdateWebhooks(rec, req) + if rec.Code != 400 || factory[org].updates != 0 || factory[other].updates != 0 { + t.Fatal("absent selection mutated tenant receivers") + } + req = httptest.NewRequest(http.MethodPut, "/api/admin/webhooks/audit", strings.NewReader(`{"urls":[]}`)) + req = req.WithContext(context.WithValue(req.Context(), OrgIDContextKey, org)) + rec = httptest.NewRecorder() + before := factory[other].updates + NewAuditHandlers().HandleUpdateWebhooks(rec, req) + if rec.Code != 204 || len(factory[org].urls) != 0 || factory[other].updates != before { + t.Fatal("explicit clear crossed tenant boundary") + } + } +}