From d9f23ee2e71bd1d10c9814039d0b4ab32ba30dc3 Mon Sep 17 00:00:00 2001 From: Nathan Byrd Date: Fri, 3 Jan 2025 22:53:39 +0000 Subject: [PATCH 1/6] Created failing test for issue --- internal/operators/restpath_test.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/operators/restpath_test.go b/internal/operators/restpath_test.go index f54e9f4b2..619daf59b 100644 --- a/internal/operators/restpath_test.go +++ b/internal/operators/restpath_test.go @@ -25,6 +25,25 @@ func TestRestPath(t *testing.T) { t.Errorf("Expected %s to match %s", exp, path) } if tx.Variables().ArgsPath().Get("id")[0] != "123" { - t.Errorf("Expected 123, got %s", tx.Variables().ArgsPath().Get("id")) + t.Errorf("Expected id to be 123, got %s", tx.Variables().ArgsPath().Get("id")) + } +} + +func TestRestPathQueryShouldNotBeIncluded(t *testing.T) { + waf := corazawaf.NewWAF() + tx := waf.NewTransaction() + exp := "/some-random/url/{id}" + path := "/some-random/url/123?name=foo" + rp, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err != nil { + t.Error(err) + } + if !rp.Evaluate(tx, path) { + t.Errorf("Expected %s to match %s", exp, path) + } + if tx.Variables().ArgsPath().Get("id")[0] != "123" { + t.Errorf("Expected id value of 123, got %s", tx.Variables().ArgsPath().Get("id")) } } From 497cac1d5d51d335e13cc7cf008f8072c77d16e6 Mon Sep 17 00:00:00 2001 From: Nathan Byrd Date: Sat, 4 Jan 2025 00:02:03 +0000 Subject: [PATCH 2/6] Update regex to prevent greedy matching in REST path and add corresponding test --- internal/operators/restpath.go | 2 +- internal/operators/restpath_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/internal/operators/restpath.go b/internal/operators/restpath.go index f1e4a8911..19ff51b6f 100644 --- a/internal/operators/restpath.go +++ b/internal/operators/restpath.go @@ -29,7 +29,7 @@ var _ plugintypes.Operator = (*restpath)(nil) func newRESTPath(options plugintypes.OperatorOptions) (plugintypes.Operator, error) { data := strings.ReplaceAll(options.Arguments, "/", "\\/") for _, token := range rePathTokenRe.FindAllStringSubmatch(data, -1) { - data = strings.Replace(data, token[0], fmt.Sprintf("(?P<%s>.*)", token[1]), 1) + data = strings.Replace(data, token[0], fmt.Sprintf("(?P<%s>[^?/]*)", token[1]), 1) } re, err := memoize.Do(data, func() (interface{}, error) { return regexp.Compile(data) }) diff --git a/internal/operators/restpath_test.go b/internal/operators/restpath_test.go index 619daf59b..1810f03a7 100644 --- a/internal/operators/restpath_test.go +++ b/internal/operators/restpath_test.go @@ -47,3 +47,30 @@ func TestRestPathQueryShouldNotBeIncluded(t *testing.T) { t.Errorf("Expected id value of 123, got %s", tx.Variables().ArgsPath().Get("id")) } } + +func TestRestPathQueryShouldNotBeGreedy(t *testing.T) { + waf := corazawaf.NewWAF() + tx := waf.NewTransaction() + + exp := "/some-random/url/{id}" + testCases := map[string]string{ + "/some-random/url/123?q=test": "123", // ?q=test is query info + "/some-random/url/456/test": "456", // /test is extra path info + } + + for path, want := range testCases { + + rp, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err != nil { + t.Error(err) + } + if !rp.Evaluate(tx, path) { + t.Errorf("Expected %s to match %s", exp, path) + } + if tx.Variables().ArgsPath().Get("id")[0] != want { + t.Errorf("Expected id value of %s, got %s", want, tx.Variables().ArgsPath().Get("id")) + } + } +} From 37b3372f6c24ce685ff70029b9ba7bc6c39c97c1 Mon Sep 17 00:00:00 2001 From: Nathan Byrd Date: Sat, 4 Jan 2025 00:03:03 +0000 Subject: [PATCH 3/6] Remove redundant test for query parameters in REST path --- internal/operators/restpath_test.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/internal/operators/restpath_test.go b/internal/operators/restpath_test.go index 1810f03a7..37a277571 100644 --- a/internal/operators/restpath_test.go +++ b/internal/operators/restpath_test.go @@ -29,25 +29,6 @@ func TestRestPath(t *testing.T) { } } -func TestRestPathQueryShouldNotBeIncluded(t *testing.T) { - waf := corazawaf.NewWAF() - tx := waf.NewTransaction() - exp := "/some-random/url/{id}" - path := "/some-random/url/123?name=foo" - rp, err := newRESTPath(plugintypes.OperatorOptions{ - Arguments: exp, - }) - if err != nil { - t.Error(err) - } - if !rp.Evaluate(tx, path) { - t.Errorf("Expected %s to match %s", exp, path) - } - if tx.Variables().ArgsPath().Get("id")[0] != "123" { - t.Errorf("Expected id value of 123, got %s", tx.Variables().ArgsPath().Get("id")) - } -} - func TestRestPathQueryShouldNotBeGreedy(t *testing.T) { waf := corazawaf.NewWAF() tx := waf.NewTransaction() From 5173d944f892649d5f9ad582bb5aa3be797eaa73 Mon Sep 17 00:00:00 2001 From: Nathan Byrd Date: Sat, 4 Jan 2025 16:38:27 +0000 Subject: [PATCH 4/6] Added additional tests and fixed additional found edge case with ending parameter --- internal/operators/restpath.go | 3 +- internal/operators/restpath_test.go | 70 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/internal/operators/restpath.go b/internal/operators/restpath.go index 19ff51b6f..d97961289 100644 --- a/internal/operators/restpath.go +++ b/internal/operators/restpath.go @@ -29,7 +29,7 @@ var _ plugintypes.Operator = (*restpath)(nil) func newRESTPath(options plugintypes.OperatorOptions) (plugintypes.Operator, error) { data := strings.ReplaceAll(options.Arguments, "/", "\\/") for _, token := range rePathTokenRe.FindAllStringSubmatch(data, -1) { - data = strings.Replace(data, token[0], fmt.Sprintf("(?P<%s>[^?/]*)", token[1]), 1) + data = strings.Replace(data, token[0], fmt.Sprintf("(?P<%s>[^?/]+)", token[1]), 1) } re, err := memoize.Do(data, func() (interface{}, error) { return regexp.Compile(data) }) @@ -43,6 +43,7 @@ func (o *restpath) Evaluate(tx plugintypes.TransactionState, value string) bool // we use the re regex to match the path and match named captured groups // to the ARGS_PATH match := o.re.FindStringSubmatch(value) + if len(match) == 0 { return false } diff --git a/internal/operators/restpath_test.go b/internal/operators/restpath_test.go index 37a277571..88ce24556 100644 --- a/internal/operators/restpath_test.go +++ b/internal/operators/restpath_test.go @@ -27,6 +27,9 @@ func TestRestPath(t *testing.T) { if tx.Variables().ArgsPath().Get("id")[0] != "123" { t.Errorf("Expected id to be 123, got %s", tx.Variables().ArgsPath().Get("id")) } + if tx.Variables().ArgsPath().Get("name")[0] != "juan" { + t.Errorf("Expected name to be juan, got %s", tx.Variables().ArgsPath().Get("name")) + } } func TestRestPathQueryShouldNotBeGreedy(t *testing.T) { @@ -55,3 +58,70 @@ func TestRestPathQueryShouldNotBeGreedy(t *testing.T) { } } } + +func TestRestPathShouldNotBeGreedyOnMultiMatch(t *testing.T) { + waf := corazawaf.NewWAF() + tx := waf.NewTransaction() + exp := "/some-random/url-{id}/{expression}/{name}" + path := "/some-random/url-123/foo/juan?q=test" + rp, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err != nil { + t.Error(err) + } + if !rp.Evaluate(tx, path) { + t.Errorf("Expected %s to match %s", exp, path) + } + if tx.Variables().ArgsPath().Get("id")[0] != "123" { + t.Errorf("Expected id to be 123, got %s", tx.Variables().ArgsPath().Get("id")) + } + if tx.Variables().ArgsPath().Get("expression")[0] != "foo" { + t.Errorf("Expected expression to be foo, got %s", tx.Variables().ArgsPath().Get("expression")) + } + if tx.Variables().ArgsPath().Get("name")[0] != "juan" { + t.Errorf("Expected name to be juan, got %s", tx.Variables().ArgsPath().Get("name")) + } +} + +func TestRestPathWithBadExpressionShouldError(t *testing.T) { + exp := "/some-random/url-{id/{name}" + _, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err == nil { + t.Error("Expected error not to be nil with a bad expression") + } +} + +func TestRestPathShouldNotMatchOnIncompleteURL(t *testing.T) { + waf := corazawaf.NewWAF() + tx := waf.NewTransaction() + exp := "/some-random/url-{id}/foo" + path := "/some-random/url-123/" + rp, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err != nil { + t.Error(err) + } + if rp.Evaluate(tx, path) { + t.Errorf("Expected %s to NOT match %s", exp, path) + } +} + +func TestRestPathShouldNotMatchOnIncompleteURLWithEndingParam(t *testing.T) { + waf := corazawaf.NewWAF() + tx := waf.NewTransaction() + exp := "/some-random/url-{id}/{name}" + path := "/some-random/url-123/" + rp, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err != nil { + t.Error(err) + } + if rp.Evaluate(tx, path) { + t.Errorf("Expected %s to NOT match %s", exp, path) + } +} From fe2448373f6eae0546a4d8f961ccca703876d0c1 Mon Sep 17 00:00:00 2001 From: Nathan Byrd Date: Sat, 4 Jan 2025 10:47:54 -0600 Subject: [PATCH 5/6] Added additional test for empty elements --- internal/operators/restpath_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/internal/operators/restpath_test.go b/internal/operators/restpath_test.go index 88ce24556..dc31647ac 100644 --- a/internal/operators/restpath_test.go +++ b/internal/operators/restpath_test.go @@ -125,3 +125,19 @@ func TestRestPathShouldNotMatchOnIncompleteURLWithEndingParam(t *testing.T) { t.Errorf("Expected %s to NOT match %s", exp, path) } } + +func TestRestPathShouldNotMatchOnEmptyPathElement(t *testing.T) { + waf := corazawaf.NewWAF() + tx := waf.NewTransaction() + exp := "/some-random/{id}/{name}" + path := "/some-random//test" + rp, err := newRESTPath(plugintypes.OperatorOptions{ + Arguments: exp, + }) + if err != nil { + t.Error(err) + } + if rp.Evaluate(tx, path) { + t.Errorf("Expected %s to NOT match %s", exp, path) + } +} From 04e51fe4a2c7d2fec3f3b7222358a5b34c9910de Mon Sep 17 00:00:00 2001 From: Nathan Byrd Date: Sat, 4 Jan 2025 21:06:28 -0600 Subject: [PATCH 6/6] Update internal/operators/restpath.go from suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> --- internal/operators/restpath.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/operators/restpath.go b/internal/operators/restpath.go index d97961289..45ce6d76c 100644 --- a/internal/operators/restpath.go +++ b/internal/operators/restpath.go @@ -43,7 +43,6 @@ func (o *restpath) Evaluate(tx plugintypes.TransactionState, value string) bool // we use the re regex to match the path and match named captured groups // to the ARGS_PATH match := o.re.FindStringSubmatch(value) - if len(match) == 0 { return false }