feat: add SecRuleUpdateActionById directive (#1071)

* feat: add SecRuleUpdateActionById directive Signed-off-by: Felipe Zipitria <[email protected]> * fix: testing rule e2e Signed-off-by: Felipe Zipitria <[email protected]> * fix: apply code review suggestions Signed-off-by: Felipe Zipitria <[email protected]> --------- Signed-off-by: Felipe Zipitria <[email protected]>
corazawaf · Oct 31, 2024 · 42e97ac · 42e97ac
1 parent 3719dce
commit 42e97ac
Show file tree

Hide file tree

Showing 5 changed files with 187 additions and 3 deletions.
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
@@ -357,7 +357,7 @@ func directiveSecRuleRemoveByID(options *DirectiveOptions) error {
 			options.WAF.Rules.DeleteByID(id)
 		} else {
 			if idx == 0 {
-				return fmt.Errorf("SecRuleUpdateTargetById: invalid negative id: %s", idOrRange)
+				return fmt.Errorf("SecRuleRemoveById: invalid negative id: %s", idOrRange)
 			}
 			start, err := strconv.Atoi(idOrRange[:idx])
 			if err != nil {
@@ -1046,6 +1046,88 @@ func updateTargetBySingleID(id int, variables string, options *DirectiveOptions)
 	return rp.ParseVariables(strings.Trim(variables, "\""))
 }
 
+// Description: Updates the action list of the specified rule(s).
+// Syntax: SecRuleUpdateActionById ID ACTIONLIST
+// ---
+// This directive will overwrite the action list of the specified rule with the actions provided in the second parameter.
+// It has two limitations: it cannot be used to change the ID or phase of a rule.
+// Only the actions that can appear only once are overwritten.
+// The actions that are allowed to appear multiple times in a list, will be appended to the end of the list.
+// The following example demonstrates how `SecAuditEngine` is used:
+// ```apache
+// SecRuleUpdateActionById 12345 "deny,status:403"
+// ```
+func directiveSecRuleUpdateActionByID(options *DirectiveOptions) error {
+	if len(options.Opts) == 0 {
+		return errEmptyOptions
+	}
+
+	idsOrRanges := strings.Fields(options.Opts)
+	idsOrRangesLen := len(idsOrRanges)
+	if idsOrRangesLen < 2 {
+		return errors.New("syntax error: SecRuleUpdateActionById id \"ACTION1,ACTION2,...\"")
+	}
+	// The last element is expected to be the actions(s)
+	actions := idsOrRanges[idsOrRangesLen-1]
+	for _, idOrRange := range idsOrRanges[:idsOrRangesLen-1] {
+		if idx := strings.Index(idOrRange, "-"); idx == -1 {
+			id, err := strconv.Atoi(idOrRange)
+			if err != nil {
+				return err
+			}
+			return updateActionBySingleID(id, actions, options)
+		} else {
+			if idx == 0 {
+				return fmt.Errorf("SecRuleUpdateActionById: invalid negative id: %s", idOrRange)
+			}
+			start, err := strconv.Atoi(idOrRange[:idx])
+			if err != nil {
+				return err
+			}
+
+			end, err := strconv.Atoi(idOrRange[idx+1:])
+			if err != nil {
+				return err
+			}
+			if start == end {
+				return updateActionBySingleID(start, actions, options)
+			}
+			if start > end {
+				return fmt.Errorf("invalid range: %s", idOrRange)
+			}
+
+			for _, rule := range options.WAF.Rules.GetRules() {
+				if rule.ID_ < start && rule.ID_ > end {
+					continue
+				}
+				rp := RuleParser{
+					rule:           &rule,
+					options:        RuleOptions{},
+					defaultActions: map[types.RulePhase][]ruleAction{},
+				}
+				if err := rp.ParseActions(strings.Trim(actions, "\"")); err != nil {
+					return err
+				}
+			}
+		}
+	}
+	return nil
+}
+
+func updateActionBySingleID(id int, actions string, options *DirectiveOptions) error {
+
+	rule := options.WAF.Rules.FindByID(id)
+	if rule == nil {
+		return fmt.Errorf("SecRuleUpdateActionById: rule \"%d\" not found", id)
+	}
+	rp := RuleParser{
+		rule:           rule,
+		options:        RuleOptions{},
+		defaultActions: map[types.RulePhase][]ruleAction{},
+	}
+	return rp.ParseActions(strings.Trim(actions, "\""))
+}
+
 // Description: Updates the target (variable) list of the specified rule(s) by tag.
 // Syntax: SecRuleUpdateTargetByTag TAG TARGET1[|TARGET2|TARGET3]
 // ---

diff --git a/internal/seclang/directives_test.go b/internal/seclang/directives_test.go
@@ -36,6 +36,31 @@ func Test_NonImplementedDirective(t *testing.T) {
 	}
 }
 
+func TestSecRuleUpdateActionByID(t *testing.T) {
+	waf := corazawaf.NewWAF()
+	rule, err := ParseRule(RuleOptions{
+		Data:         "REQUEST_URI \"^/test\" \"id:181,log\"",
+		WAF:          waf,
+		WithOperator: true,
+	})
+	if err != nil {
+		t.Error(err)
+	}
+	if err := waf.Rules.Add(rule); err != nil {
+		t.Error(err)
+	}
+	if waf.Rules.Count() != 1 {
+		t.Error("Failed to add rule")
+	}
+	if err := directiveSecRuleUpdateActionByID(&DirectiveOptions{
+		WAF:  waf,
+		Opts: "181 \"nolog\"",
+	}); err != nil {
+		t.Error(err)
+	}
+
+}
+
 func TestSecRuleUpdateTargetByID(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	rule, err := ParseRule(RuleOptions{
@@ -179,6 +204,19 @@ func TestDirectives(t *testing.T) {
 			{"1 2", expectNoErrorOnDirective},
 			{"1 2 3-4", expectNoErrorOnDirective},
 		},
+		"SecRuleUpdateActionById": {
+			{"", expectErrorOnDirective},
+			{"a", expectErrorOnDirective},
+			{"1-a", expectErrorOnDirective},
+			{"a-2", expectErrorOnDirective},
+			{"2-1", expectErrorOnDirective},
+			{"1-a \"status:403\"", expectErrorOnDirective},
+			{"a-2 \"status:403\"", expectErrorOnDirective},
+			{"2-1 \"status:403\"", expectErrorOnDirective},
+			{"-1 \"status:403\"", expectErrorOnDirective},
+			{"1 2 3-4 \"status:403\"", expectNoErrorOnDirective},
+			{"1 2 3-4 \"status:403,nolog\"", expectNoErrorOnDirective},
+		},
 		"SecRuleUpdateTargetById": {
 			{"", expectErrorOnDirective},
 			{"a", expectErrorOnDirective},

diff --git a/internal/seclang/directivesmap.gen.go b/internal/seclang/directivesmap.gen.go
diff --git a/internal/seclang/generator/directivesmap.go.tmpl b/internal/seclang/generator/directivesmap.go.tmpl
@@ -17,7 +17,6 @@ var directivesMap = map[string]directive{
 	"secargumentseparator":     directiveUnsupported,
 	"seccookieformat":          directiveUnsupported,
 	"secruleupdatetargetbymsg": directiveUnsupported,
-	"secruleupdateactionbyid":  directiveUnsupported,
 	"secrulescript":            directiveUnsupported,
 	"secruleperftime":          directiveUnsupported,
 	"secunicodemap":            directiveUnsupported,

diff --git a/testing/engine/directives_updateactions.go b/testing/engine/directives_updateactions.go
@@ -0,0 +1,64 @@
+// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors
+// SPDX-License-Identifier: Apache-2.0
+
+package engine
+
+import (
+	"github.com/corazawaf/coraza/v3/testing/profile"
+)
+
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "fzipi",
+		Description: "Test SecRuleUpdateActionById directives",
+		Enabled:     true,
+		Name:        "SecRuleUpdateActionById.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "SecRuleUpdateActionById to pass",
+			Stages: []profile.Stage{
+				// Phase 1
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/phase1?param1=value1",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								1004,
+							},
+						},
+					},
+				},
+				// Phase 2
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/phase2?param2=value2",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								1014,
+							},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 302,
+								Data:   "https://www.example.com/",
+								RuleID: 1014,
+								Action: "redirect",
+							},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+	SecRule ARGS "@contains value1" "phase:1,id:1004,deny"
+    SecRule ARGS "@contains value1" "phase:1,id:1005,log"
+    SecRuleUpdateActionById 1004 "pass"
+
+    SecRule ARGS "@contains value2" "phase:2,id:1014,block,deny"
+	SecRuleUpdateActionById 1014 "redirect:'https://www.example.com/',status:302"
+	`,
+})