From 0700bb01d4e656406a407b4944fd7d47483eda6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 27 Jun 2023 10:22:31 +0200 Subject: [PATCH] fix: benchmark and propagate the status to not to swallow the failure (#808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore: propagates the pipe status after tee. * Refactor escape_seq_decode * docs: adds more comments. * tests: tweaks the code. * tests: adds test for TestEscapeSeqDecode * chore: simplify the doEscapeSeqDecode function. * chore: aligns go version of benchmarks with tests --------- Co-authored-by: Anuraag Agrawal Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> --- .github/workflows/benchmark.yml | 6 +- internal/transformations/css_decode_test.go | 13 ++-- internal/transformations/escape_seq_decode.go | 76 ++++++++----------- .../transformations/escape_seq_decode_test.go | 17 +++-- types/waf_test.go | 3 + 5 files changed, 60 insertions(+), 55 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 2869461bf..00d6793e2 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -23,12 +23,14 @@ jobs: - uses: actions/checkout@v3 - uses: actions/setup-go@v4 with: - go-version: '>=1.18.0' + go-version: '>=1.19.0' - name: Install dependencies run: go get ./... # Run benchmark with `go test -bench` and stores the output to a file - name: Run benchmark - run: go test -bench=. ./... | tee output.txt + run: | + go test -bench=. ./... | tee output.txt + exit ${PIPESTATUS[0]} # Download previous benchmark result from cache (if exists) - name: Download previous benchmark data uses: actions/cache@v3 diff --git a/internal/transformations/css_decode_test.go b/internal/transformations/css_decode_test.go index bc9f2dda7..400cc8bfe 100644 --- a/internal/transformations/css_decode_test.go +++ b/internal/transformations/css_decode_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 package transformations @@ -29,13 +29,16 @@ func TestCSSDecode(t *testing.T) { t.Run(tt.input, func(t *testing.T) { have, changed, err := cssDecode(tt.input) if err != nil { - t.Error(err) + t.Fatal(err) } - if tt.input == tt.want && changed || tt.input != tt.want && !changed { - t.Errorf("input %q, have %q with changed %t", tt.input, have, changed) + + shouldChange := tt.input != tt.want + if changed != shouldChange { + t.Errorf("unexpected changed value, want %t, have %t", shouldChange, changed) } + if have != tt.want { - t.Errorf("have %q, want %q", have, tt.want) + t.Errorf("unexpected value, want %q, have %q", tt.want, have) } }) } diff --git a/internal/transformations/escape_seq_decode.go b/internal/transformations/escape_seq_decode.go index b736d672b..f740b683b 100644 --- a/internal/transformations/escape_seq_decode.go +++ b/internal/transformations/escape_seq_decode.go @@ -29,7 +29,10 @@ func doEscapeSeqDecode(input string, pos int) (string, bool) { for i < inputLen { if (input[i] == '\\') && (i+1 < inputLen) { - c := int8(-1) + var ( + c byte + ok = true + ) switch input[i+1] { case 'a': @@ -54,59 +57,46 @@ func doEscapeSeqDecode(input string, pos int) (string, bool) { c = '\'' case '"': c = '"' + default: + ok = false } - if c != -1 { + if ok { + data[d] = c + d += 1 i += 2 + changed = true + continue } /* Hexadecimal or octal? */ - if c == -1 { - if (input[i+1] == 'x') || (input[i+1] == 'X') { - /* Hexadecimal. */ - if (i+3 < inputLen) && (utils.ValidHex((input[i+2]))) && (utils.ValidHex((input[i+3]))) { - /* Two digits. */ - c = int8(utils.X2c(input[i+2:])) - i += 4 - } - /* Else Invalid encoding, do nothing. */ - - } else { - if isODigit(input[i+1]) { /* Octal. */ - buf := make([]byte, 4) - j := 0 - - for (i+1+j < inputLen) && (j < 3) { - buf[j] = input[i+1+j] - j++ - if (len(input) > (i + 1 + j)) && !isODigit(input[i+1+j]) { - break - } - } - // buf[j] = '\x00' - // This line actually means that the string ends here so: - buf = buf[:j] + if (input[i+1] == 'x' || input[i+1] == 'X') && i+3 < inputLen && utils.ValidHex(input[i+2]) && utils.ValidHex(input[i+3]) { + /* Two digits. */ + data[d] = utils.X2c(input[i+2:]) + d += 1 + i += 4 + changed = true + continue + } - if j > 0 { - bc, _ := strconv.ParseUint(string(buf), 8, 8) - c = int8(bc) - i += 1 + j - } - } + if isODigit(input[i+1]) { /* Octal. */ + j := 2 + for j < 4 && i+j < inputLen && isODigit(input[i+j]) { + j += 1 } - } - if c == -1 { - /* Didn't recognise encoding, copy raw bytes. */ - data[d] = input[i+1] - d++ - i += 2 - } else { - /* Converted the encoding. */ - data[d] = byte(c) + bc, _ := strconv.ParseUint(input[i+1:i+j], 8, 8) + data[d] = byte(bc) + d += 1 + i += j changed = true - d++ + continue } + + /* Didn't recognise encoding, copy raw bytes. */ + data[d] = input[i+1] + d++ + i += 2 } else { /* Input character not a backslash, copy it. */ data[d] = input[i] diff --git a/internal/transformations/escape_seq_decode_test.go b/internal/transformations/escape_seq_decode_test.go index 95e0f5c61..d9a62f891 100644 --- a/internal/transformations/escape_seq_decode_test.go +++ b/internal/transformations/escape_seq_decode_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 package transformations @@ -26,6 +26,10 @@ func TestEscapeSeqDecode(t *testing.T) { input: "\\\\u0000", want: "\\u0000", }, + { + input: "\\a\\b\\f\\n\\r\\t\\v\\u0000\\?\\'\\\"\\0\\12\\123\\x00\\xff", + want: "\a\b\f\n\r\t\vu0000?'\"\x00\nS\x00\xff", + }, } for _, tc := range tests { @@ -33,13 +37,16 @@ func TestEscapeSeqDecode(t *testing.T) { t.Run(tt.input, func(t *testing.T) { have, changed, err := escapeSeqDecode(tt.input) if err != nil { - t.Error(err) + t.Fatal(err) } - if tt.input == tt.want && changed || tt.input != tt.want && !changed { - t.Errorf("input %q, have %q with changed %t", tt.input, have, changed) + + shouldChange := tt.input != tt.want + if changed != shouldChange { + t.Errorf("unexpected changed value, want %t, have %t", shouldChange, changed) } + if have != tt.want { - t.Errorf("have %q, want %q", have, tt.want) + t.Errorf("unexpected value, want %q, have %q", tt.want, have) } }) } diff --git a/types/waf_test.go b/types/waf_test.go index 1a0dc3fba..118c86825 100644 --- a/types/waf_test.go +++ b/types/waf_test.go @@ -1,3 +1,6 @@ +// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + package types import "testing"