Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: setvar minor fix, tests, added warning when missing variable, deprecates usage of tx.LogData #892

Merged
merged 5 commits into from
Nov 3, 2023
Merged

chore: setvar minor fix, tests, added warning when missing variable, deprecates usage of tx.LogData #892

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f47a1c0
chore: setvar minor fix, tests, added warning when missing variable
M4tteoP Oct 22, 2023
955938a
adds tests with numerical operation starting with a negative number
M4tteoP Oct 22, 2023
2c955da
deprecating Logdata field in transaction
M4tteoP Oct 22, 2023
7c72491
updates test name
M4tteoP Oct 30, 2023
cc26c67
Merge branch 'main' into setvar_missing_key
M4tteoP Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/http-server/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/corazawaf/coraza/v3/examples/http-server

go 1.18
go 1.19

require github.com/corazawaf/coraza/v3 v3.0.0-20220914101451-05d352c89b24

Expand Down
2 changes: 2 additions & 0 deletions experimental/plugins/macro/macro.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ func expandToken(tx plugintypes.TransactionState, token macroToken) string {
}
}

// If the variable is known (e.g. TX) but the key is not found, we return the original text
tx.DebugLogger().Warn().Str("variable", token.variable.Name()).Str("key", token.key).Msg("key not found in collection, returning the original text")
return token.text
}

Expand Down
1 change: 0 additions & 1 deletion go.work.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
golang.org/x/crypto v0.14.0 h1:wBqGXzWJW6m1XrIKlAH0Hs1JJ7+9KBwnIO8v66Q9cHc=
golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4=
golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y=
golang.org/x/term v0.13.0/go.mod h1:LTmsnFJwVN6bCy1rVCoS+qHT1HhALEFxKncY3WNNh4U=
golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
2 changes: 1 addition & 1 deletion internal/actions/logdata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (a *logdataFn) Init(r plugintypes.RuleMetadata, data string) error {
}

func (a *logdataFn) Evaluate(r plugintypes.RuleMetadata, tx plugintypes.TransactionState) {
tx.(*corazawaf.Transaction).Logdata = r.(*corazawaf.Rule).LogData.Expand(tx)
// logdata macro expansion is performed after all other actions have been evaluated (and potentially all the needed variables have been set)
}

func (a *logdataFn) Type() plugintypes.ActionType {
Expand Down
30 changes: 13 additions & 17 deletions internal/actions/setvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,19 @@
col.Remove(key)
return
}
res := ""
currentVal := ""
if r := col.Get(key); len(r) > 0 {
res = r[0]
currentVal = r[0]
}
var err error
switch {
case len(value) == 0:
// if nothing to input
col.Set(key, []string{""})
case value[0] == '+':
// if we want to sum
sum := 0
case value[0] == '+', value[0] == '-': // Increment or decrement, arithmetical operations
val := 0
if len(value) > 1 {
sum, err = strconv.Atoi(value[1:])
val, err = strconv.Atoi(value[1:])
if err != nil {
tx.DebugLogger().Error().
Str("var_value", value).
Expand All @@ -159,26 +158,23 @@
return
}
}
val := 0
if res != "" {
val, err = strconv.Atoi(res)
currentValInt := 0
if currentVal != "" {
currentValInt, err = strconv.Atoi(currentVal)
if err != nil {
tx.DebugLogger().Error().
Str("var_key", res).
Str("var_key", currentVal).

Check warning on line 166 in internal/actions/setvar.go

View check run for this annotation

Codecov / codecov/patch

internal/actions/setvar.go#L166 

Added line #L166 was not covered by tests
Int("rule_id", r.ID()).
Err(err).
Msg("Invalid value")
return
}
}
col.Set(key, []string{strconv.Itoa(sum + val)})
case value[0] == '-':
me, _ := strconv.Atoi(value[1:])
txv, err := strconv.Atoi(res)
if err != nil {
return
if value[0] == '+' {
col.Set(key, []string{strconv.Itoa(currentValInt + val)})
} else {
col.Set(key, []string{strconv.Itoa(currentValInt - val)})
}
col.Set(key, []string{strconv.Itoa(txv - me)})
default:
col.Set(key, []string{value})
}
Expand Down
125 changes: 122 additions & 3 deletions internal/actions/setvar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,26 @@
package actions

import (
"bytes"
"strings"
"testing"

"github.com/corazawaf/coraza/v3/collection"
"github.com/corazawaf/coraza/v3/debuglog"
"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
"github.com/corazawaf/coraza/v3/internal/corazawaf"
)

type md struct {
}

func (_ md) ID() int {
func (md) ID() int {
return 0
}
func (_ md) ParentID() int {
func (md) ParentID() int {
return 0
}
func (_ md) Status() int {
func (md) Status() int {
return 0
}

Expand Down Expand Up @@ -46,3 +53,115 @@ func TestSetvarInit(t *testing.T) {
}
})
}

var invalidSyntaxAtoiError = "invalid syntax"
var warningKeyNotFoundInCollection = "key not found in collection"

func TestSetvarEvaluateErrors(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, there is a fix related to - should there be a new test for it?

Copy link
Member Author

@M4tteoP M4tteoP Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Numerical operation - with existing negative variable and Numerical operation - with existing variable are the two test cases added to this new test to ensure that the fix related to - is working as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think I got confused by the test function name, it looks like not all of these are errors

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sorry about that. The test evolved over time and is not anymore just about errors. Renamed it to TestSetvarEvaluate

tests := []struct {
name string
init string
init2 string
expectInvalidSyntaxError bool
expectNewVarValue string
}{
{
name: "Numerical operation + with existing variable",
init: "TX.var=5",
init2: "TX.newvar=+%{tx.var}",
expectInvalidSyntaxError: false,
expectNewVarValue: "5",
},
{
name: "Numerical operation - with existing variable",
init: "TX.var=5",
init2: "TX.newvar=-%{tx.var}",
expectInvalidSyntaxError: false,
expectNewVarValue: "-5",
},
{
name: "Numerical operation - with existing negative variable",
init: "TX.newvar=-5",
init2: "TX.newvar=+5",
expectInvalidSyntaxError: false,
expectNewVarValue: "0",
},
{
name: "Numerical operation + with missing (or non-numerical) variable",
init: "TX.newvar=+%{tx.missingvar}",
expectInvalidSyntaxError: true,
},
{
name: "Numerical operation - with missing (or non-numerical) variable",
init: "TX.newvar=-%{tx.missingvar}",
expectInvalidSyntaxError: true,
},
}

for _, tt := range tests {
logsBuf := &bytes.Buffer{}

logger := debuglog.Default().WithLevel(debuglog.LevelWarn).WithOutput(logsBuf)

t.Run(tt.name, func(t *testing.T) {
defer logsBuf.Reset()
a := setvar()
metadata := &md{}
if err := a.Init(metadata, tt.init); err != nil {
t.Error("unexpected error during setvar init")
}
waf := corazawaf.NewWAF()
waf.Logger = logger
tx := waf.NewTransaction()
a.Evaluate(metadata, tx)
if tt.expectInvalidSyntaxError {
if logsBuf.Len() == 0 {
t.Fatal("expected error")
}
if !strings.Contains(logsBuf.String(), invalidSyntaxAtoiError) {
t.Errorf("expected error containing %q, got %q", invalidSyntaxAtoiError, logsBuf.String())
}
if !strings.Contains(logsBuf.String(), warningKeyNotFoundInCollection) {
t.Errorf("expected error containing %q, got %q", warningKeyNotFoundInCollection, logsBuf.String())
}
}
if logsBuf.Len() != 0 && !tt.expectInvalidSyntaxError {
t.Fatalf("unexpected error: %s", logsBuf.String())
}

if tt.init2 != "" {
if err := a.Init(metadata, tt.init2); err != nil {
t.Fatal("unexpected error during setvar init")
}
a.Evaluate(metadata, tx)
if logsBuf.Len() != 0 && !tt.expectInvalidSyntaxError {
t.Fatalf("unexpected error: %s", logsBuf.String())
}
}
if tt.expectNewVarValue != "" {
checkCollectionValue(t, a.(*setvarFn), tx, "newvar", tt.expectNewVarValue)
}
})
}
}

func checkCollectionValue(t *testing.T, a *setvarFn, tx plugintypes.TransactionState, key string, expected string) {
t.Helper()
var col collection.Map
if c, ok := tx.Collection(a.collection).(collection.Map); !ok {
t.Fatal("collection in setvar is not a map")
return
} else {
col = c
}
if col == nil {
t.Fatal("collection in setvar is nil")
return
}
if col == nil {
t.Fatal("collection is nil")
}
if col.Get(key)[0] != expected {
t.Errorf("key %q: expected %q, got %q", key, expected, col.Get(key))
}
}
2 changes: 1 addition & 1 deletion internal/corazawaf/rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ func (r *Rule) doEvaluate(phase types.RulePhase, tx *Transaction, collectiveMatc
}

// Expansion of Msg and LogData is postponed here. It allows to run it only if the whole rule/chain
// matches and to rely on MATCHED_* variables updated by the chain, not just by the fist rule.
// matches and to rely on MATCHED_* variables updated by the chain, not just by the first rule.
if !r.MultiMatch {
if r.Msg != nil {
matchedValues[0].(*corazarules.MatchData).Message_ = r.Msg.Expand(tx)
Expand Down
9 changes: 5 additions & 4 deletions internal/corazawaf/rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ func (*dummyFlowAction) Init(_ plugintypes.RuleMetadata, _ string) error {
}

func (*dummyFlowAction) Evaluate(_ plugintypes.RuleMetadata, tx plugintypes.TransactionState) {
tx.(*Transaction).Logdata = "flow action triggered"
// SkipAfter is used in a improper way, just for testing purposes ensuring that the action has been enforced
tx.(*Transaction).SkipAfter = "flow action triggered"
}

func (*dummyFlowAction) Type() plugintypes.ActionType {
Expand All @@ -116,7 +117,7 @@ func TestFlowActionIfDetectionOnlyEngine(t *testing.T) {
if len(matchdata) != 1 {
t.Errorf("Expected 1 matchdata, got %d", len(matchdata))
}
if tx.Logdata != "flow action triggered" {
if tx.SkipAfter != "flow action triggered" {
t.Errorf("Expected flow action triggered with DetectionOnly engine")
}
}
Expand All @@ -128,7 +129,7 @@ func (*dummyNonDisruptiveAction) Init(_ plugintypes.RuleMetadata, _ string) erro
}

func (*dummyNonDisruptiveAction) Evaluate(_ plugintypes.RuleMetadata, tx plugintypes.TransactionState) {
tx.(*Transaction).Logdata = "action enforced"
tx.(*Transaction).SkipAfter = "action enforced"
}

func (*dummyNonDisruptiveAction) Type() plugintypes.ActionType {
Expand All @@ -142,7 +143,7 @@ func TestMatchVariableRunsActionTypeNondisruptive(t *testing.T) {
action := &dummyNonDisruptiveAction{}
_ = rule.AddAction("dummyNonDisruptiveAction", action)
rule.matchVariable(tx, md)
if tx.Logdata != "action enforced" {
if tx.SkipAfter != "action enforced" {
t.Errorf("Expected non disruptive action to be enforced during matchVariable")
}
}
Expand Down
1 change: 1 addition & 0 deletions internal/corazawaf/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type Transaction struct {
interruption *types.Interruption

// This is used to store log messages
// Deprecated since Coraza 3.0.5: this variable is not used, logdata values are stored in the matched rules
Logdata string

// Rules will be skipped after a rule with this SecMarker is found
Expand Down
2 changes: 1 addition & 1 deletion internal/corazawaf/waf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func (w *WAF) newTransactionWithID(id string) *Transaction {
tx.id = id
tx.matchedRules = []types.MatchedRule{}
tx.interruption = nil
tx.Logdata = ""
tx.Logdata = "" // Deprecated, this variable is not used. Logdata for each matched rule is stored in the MatchData field.
tx.SkipAfter = ""
tx.AuditEngine = w.AuditEngine
tx.AuditLogParts = w.AuditLogParts
Expand Down
6 changes: 3 additions & 3 deletions testing/coreruleset/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ require (
github.com/tidwall/match v1.1.1 // indirect
github.com/tidwall/pretty v1.2.1 // indirect
github.com/yargevad/filepathx v1.0.0 // indirect
golang.org/x/crypto v0.12.0 // indirect
golang.org/x/net v0.14.0 // indirect
golang.org/x/sys v0.11.0 // indirect
golang.org/x/crypto v0.14.0 // indirect
golang.org/x/net v0.17.0 // indirect
golang.org/x/sys v0.13.0 // indirect
golang.org/x/tools v0.6.0 // indirect
golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
Expand Down
12 changes: 6 additions & 6 deletions testing/coreruleset/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,8 +314,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk
golang.org/x/crypto v0.0.0-20190923035154-9ee001bba392/go.mod h1:/lpIB1dKB+9EgE3H3cr1v9wB50oz8l4C4h62xy7jSTY=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
golang.org/x/crypto v0.12.0 h1:tFM/ta59kqch6LlvYnPa0yx5a83cL2nHflFhYKvv9Yk=
golang.org/x/crypto v0.12.0/go.mod h1:NF0Gs7EO5K4qLn+Ylc+fih8BSTeIjAP05siRnAh98yw=
golang.org/x/crypto v0.14.0 h1:wBqGXzWJW6m1XrIKlAH0Hs1JJ7+9KBwnIO8v66Q9cHc=
golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4=
golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU=
Expand Down Expand Up @@ -343,8 +343,8 @@ golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwY
golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
golang.org/x/net v0.0.0-20210410081132-afb366fc7cd1/go.mod h1:9tjilg8BloeKEkVJvy7fQ90B1CfIiPueXVOjqfkSzI8=
golang.org/x/net v0.14.0 h1:BONx9s002vGdD9umnlX1Po8vOZmrgH34qlHcD1MfK14=
golang.org/x/net v0.14.0/go.mod h1:PpSgVXXLK0OxS0F31C1/tv6XNguvCrnXIDrFMspZIUI=
golang.org/x/net v0.17.0 h1:pVaXccu2ozPjCXewfr1S7xza/zcXTity9cCdXQYSjIM=
golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE=
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
Expand Down Expand Up @@ -391,8 +391,8 @@ golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM=
golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE=
golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.1-0.20181227161524-e6919f6577db/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
Expand Down
Loading