From 32a30434b11ac09f4e0d3f24c37ecdbd1fe4765d Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Sun, 5 Apr 2026 02:51:04 +0000 Subject: [PATCH] fix(security): prevent client injection of enrichment fields on decisions --- .../internal/api/handlers/security_handler.go | 36 +++++++++++----- .../security_handler_coverage_test.go | 41 +++++++++++++++++++ backend/internal/models/security_decision.go | 4 +- 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/backend/internal/api/handlers/security_handler.go b/backend/internal/api/handlers/security_handler.go index 20dafef9..591c2f2b 100644 --- a/backend/internal/api/handlers/security_handler.go +++ b/backend/internal/api/handlers/security_handler.go @@ -34,6 +34,17 @@ type WAFExclusion struct { Description string `json:"description,omitempty"` } +// CreateDecisionRequest is the client-facing DTO for manual decision creation. +// Enrichment fields (Scenario, Country, ExpiresAt) are system-populated and +// deliberately excluded to prevent clients from injecting arbitrary values. +type CreateDecisionRequest struct { + IP string `json:"ip" binding:"required"` + Action string `json:"action" binding:"required"` + Host string `json:"host,omitempty"` + RuleID string `json:"rule_id,omitempty"` + Details string `json:"details,omitempty"` +} + // SecurityHandler handles security-related API requests. type SecurityHandler struct { cfg config.SecurityConfig @@ -328,19 +339,19 @@ func (h *SecurityHandler) CreateDecision(c *gin.Context) { return } - var payload models.SecurityDecision - if err := c.ShouldBindJSON(&payload); err != nil { + var req CreateDecisionRequest + if err := c.ShouldBindJSON(&req); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) return } - if payload.IP == "" || payload.Action == "" { + if req.IP == "" || req.Action == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "ip and action are required"}) return } // CRITICAL: Validate IP format to prevent SQL injection via IP field // Must accept both single IPs and CIDR ranges - if !isValidIP(payload.IP) && !isValidCIDR(payload.IP) { + if !isValidIP(req.IP) && !isValidCIDR(req.IP) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid IP address format"}) return } @@ -348,16 +359,21 @@ func (h *SecurityHandler) CreateDecision(c *gin.Context) { // CRITICAL: Validate action enum // Only accept known action types to prevent injection via action field validActions := []string{"block", "allow", "captcha"} - if !contains(validActions, payload.Action) { + if !contains(validActions, req.Action) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid action"}) return } - // Sanitize details field (limit length, strip control characters) - payload.Details = sanitizeString(payload.Details, 1000) - - // Populate source - payload.Source = "manual" + // Map DTO to model — enrichment fields (Scenario, Country, ExpiresAt) + // are intentionally excluded; they are system-populated only. + payload := models.SecurityDecision{ + IP: req.IP, + Action: req.Action, + Host: req.Host, + RuleID: req.RuleID, + Details: sanitizeString(req.Details, 1000), + Source: "manual", + } if err := h.svc.LogDecision(&payload); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to log decision"}) return diff --git a/backend/internal/api/handlers/security_handler_coverage_test.go b/backend/internal/api/handlers/security_handler_coverage_test.go index f3be817d..71ee9415 100644 --- a/backend/internal/api/handlers/security_handler_coverage_test.go +++ b/backend/internal/api/handlers/security_handler_coverage_test.go @@ -1005,3 +1005,44 @@ func TestLatestConfigApplyState_Helper(t *testing.T) { require.Equal(t, true, state["available"]) require.Equal(t, "applied", state["status"]) } + +// TestSecurityHandler_CreateDecision_StripsEnrichmentFields verifies that +// clients cannot inject system-populated enrichment fields (Scenario, Country, +// ExpiresAt) via the CreateDecision endpoint. +func TestSecurityHandler_CreateDecision_StripsEnrichmentFields(t *testing.T) { + db := setupTestDB(t) + require.NoError(t, db.AutoMigrate(&models.SecurityDecision{}, &models.SecurityAudit{})) + + handler := NewSecurityHandler(config.SecurityConfig{}, db, nil) + router := gin.New() + router.Use(func(c *gin.Context) { + c.Set("role", "admin") + c.Next() + }) + router.POST("/security/decisions", handler.CreateDecision) + + payload := map[string]any{ + "ip": "10.0.0.1", + "action": "block", + "details": "test", + "scenario": "injected-scenario", + "country": "XX", + "expires_at": "2099-01-01T00:00:00Z", + } + body, _ := json.Marshal(payload) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/security/decisions", bytes.NewBuffer(body)) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + + // Verify the stored decision has empty enrichment fields + var stored models.SecurityDecision + require.NoError(t, db.First(&stored).Error) + assert.Empty(t, stored.Scenario, "Scenario should not be settable by client") + assert.Empty(t, stored.Country, "Country should not be settable by client") + assert.Nil(t, stored.ExpiresAt, "ExpiresAt should not be settable by client") + assert.Equal(t, "manual", stored.Source, "Source must be forced to manual") +} diff --git a/backend/internal/models/security_decision.go b/backend/internal/models/security_decision.go index ff22e15d..7f9ff139 100644 --- a/backend/internal/models/security_decision.go +++ b/backend/internal/models/security_decision.go @@ -18,7 +18,7 @@ type SecurityDecision struct { CreatedAt time.Time `json:"created_at" gorm:"index;compositeIndex:idx_sd_source_created,sort:desc;compositeIndex:idx_sd_source_scenario_created,sort:desc;compositeIndex:idx_sd_source_ip_created,sort:desc"` // Dashboard enrichment fields (Issue #26, PR-1) - Scenario string `json:"scenario" gorm:"index;compositeIndex:idx_sd_source_scenario_created"` - Country string `json:"country" gorm:"index;size:2"` + Scenario string `json:"scenario,omitempty" gorm:"index;compositeIndex:idx_sd_source_scenario_created"` + Country string `json:"country,omitempty" gorm:"index;size:2"` ExpiresAt *time.Time `json:"expires_at,omitempty" gorm:"index"` }