fix: benchmark and propagate the status to not to swallow the failure (…

…#808)

* 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 <[email protected]>
Co-authored-by: Felipe Zipitría <[email protected]>

.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

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

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]

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,20 +26,27 @@ 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 {
 		tt := tc
 		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)
 			}
 		})
 	}

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"