fix(security): prevent client injection of enrichment fields on decisions

This commit is contained in:
GitHub Actions
2026-04-05 02:51:04 +00:00
parent 138426311f
commit 32a30434b1
3 changed files with 69 additions and 12 deletions

View File

@@ -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

View File

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

View File

@@ -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"`
}