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