OCPBUGS-22739: Properly handle rewrite-target annotation

images/router/haproxy/conf/haproxy-config.template

@@ -671,9 +671,9 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }}
         {{- with $pathRewriteTarget := firstMatch $pathRewriteTargetPattern (index $cfg.Annotations "haproxy.router.openshift.io/rewrite-target") }}
   # Path rewrite target
           {{- if eq $pathRewriteTarget "/" }}
-  http-request replace-path ^{{ $cfg.Path }}/?(.*)$ {{ $pathRewriteTarget }}\1
+  http-request replace-path ^{{ $cfg.Path }}/?(.*)$ '{{ processRewriteTarget $pathRewriteTarget }}'
           {{- else }}
-  http-request replace-path ^{{ $cfg.Path }}(.*)$ {{ $pathRewriteTarget }}\1
+  http-request replace-path ^{{ $cfg.Path }}(.*)$ '{{ processRewriteTarget $pathRewriteTarget }}'
           {{- end }}
         {{- end }}{{/* rewrite target */}}
 

pkg/router/template/template_helper.go

@@ -19,6 +19,7 @@ import (
 	templateutil "github.com/openshift/router/pkg/router/template/util"
 	haproxyutil "github.com/openshift/router/pkg/router/template/util/haproxy"
 	"github.com/openshift/router/pkg/router/template/util/haproxytime"
+	"github.com/openshift/router/pkg/router/template/util/rewritetarget"
 )
 
 const (
@@ -411,4 +412,6 @@ var helperFunctions = template.FuncMap{
 
 	"clipHAProxyTimeoutValue": clipHAProxyTimeoutValue, //clips extrodinarily high timeout values to be below the maximum allowed timeout value
 	"parseIPList":             parseIPList,             //parses the list of IPs/CIDRs (IPv4/IPv6)
+
+	"processRewriteTarget": rewritetarget.SanitizeInput, //sanitizes `haproxy.router.openshift.io/rewrite-target` annotation
 }

pkg/router/template/util/rewritetarget/rewritetarget.go

@@ -0,0 +1,159 @@
+package rewritetarget
+
+import (
+	"regexp"
+	"strings"
+)
+
+var (
+	// oneOrMorePercentSigns matches one or more literal percent
+	// signs.
+	oneOrMorePercentSigns = regexp.MustCompile(`%+`)
+
+	// oneOrMoreBackslashes matches one or more literal
+	// backslashes.
+	oneOrMoreBackslashes = regexp.MustCompile(`\\+`)
+)
+
+type runeResult struct {
+	value   string // The rune to append to the result.
+	escaped bool   // Whether the next rune is escaped.
+	skip    bool   // Whether to skip appending this rune.
+	stop    bool   // Whether to stop processing further runes.
+}
+
+type processFunc func(char rune, escaped bool) runeResult
+
+// processRunes iterates through the input string and calls processFn
+// for each rune. It uses the result of processFn to determine whether
+// to continue, skip the rune, or stop processing the input. It returns
+// a processed string.
+func processRunes(input string, processFn processFunc) string {
+	var sb strings.Builder
+
+	// Increase buffer capacity to improve performance by
+	// preallocating space for the string being built. The buffer
+	// is set to 125% of the input length to accommodate
+	// additional characters without the need for further
+	// allocation.
+	sb.Grow(len(input) + (len(input) / 4))
+
+	// escapeFlag tracks if the next rune is escaped.
+	escapeFlag := false
+
+	for _, char := range input {
+		result := processFn(char, escapeFlag)
+		escapeFlag = result.escaped
+		if result.stop {
+			break
+		}
+		if !result.skip {
+			sb.WriteString(result.value)
+		}
+	}
+
+	return sb.String()
+}
+
+// replacePercentSigns replaces each % to with %% if the current sequence
+// contains an odd number %. Previously, a single % produced a syntax error,
+// and users had to use %% to represent a literal %. To ensure compatibility
+// while resolving this problem, we must continue to replace single % with
+// double %%, but leave double %%  as is, so that existing users who manually
+// addressed the issue are not broken. Furthermore, even sequences, such as
+// %%%% or %%%%%% were also valid and should be left untouched, while odd
+// sequences such as %%% or %%%%% were invalid, and each % should be doubled
+// to ensure every % is escaped.
+func replacePercentSigns(val string) string {
+	return oneOrMorePercentSigns.ReplaceAllStringFunc(val, func(match string) string {
+		if len(match)%2 == 1 {
+			// For an odd count of %, replace each % with %%.
+			return strings.ReplaceAll(match, "%", "%%")
+		}
+		return match
+	})
+}
+
+// removeSingleBackslashes removes single backslashes \.
+func removeSingleBackslashes(val string) string {
+	return oneOrMoreBackslashes.ReplaceAllStringFunc(val, func(match string) string {
+		if len(match) == 1 {
+			return oneOrMoreBackslashes.ReplaceAllString(match, "")
+		}
+		return match
+	})
+}
+
+// convertDoubleBackslashes converts double backslashes \\ to single
+// backslashes \.
+func convertDoubleBackslashes(val string) string {
+	return strings.ReplaceAll(val, `\\`, `\`)
+}
+
+// newProcessHashCreator creates a function to process '#' runes in a
+// string. It returns a processFunc which will set
+// encounteredCommentMarker to true and stop processing if it
+// encounters a '#' that is not escaped.
+func newProcessHashCreator(encounteredCommentMarker *bool) processFunc {
+	return func(char rune, escaped bool) runeResult {
+		if char == '#' && !escaped {
+			*encounteredCommentMarker = true
+			return runeResult{stop: true}
+		}
+		return runeResult{value: string(char), escaped: char == '\\' && !escaped}
+	}
+}
+
+// processDoubleQuotes is a processFunc that processes double quote
+// characters. It skips the double quote if it's not escaped.
+func processDoubleQuotes(char rune, escaped bool) runeResult {
+	if char == '"' && !escaped {
+		return runeResult{skip: true}
+	}
+	return runeResult{value: string(char), escaped: char == '\\' && !escaped}
+}
+
+// processSingleQuotes is a processFunc that processes single quote
+// characters. It skips the single quote if it's not escaped. If it's
+// escaped, it adds an escaped single quote for later conversion.
+func processSingleQuotes(char rune, escaped bool) runeResult {
+	if char == '\'' && !escaped {
+		return runeResult{skip: true}
+	} else if char == '\'' && escaped {
+		// TRICKY: Because our later operation removes single \,
+		// we should double escape \\ it, so it will be converted
+		// to \ later.
+		return runeResult{value: `'\\''`}
+	}
+	return runeResult{value: string(char), escaped: char == '\\' && !escaped}
+}
+
+// SanitizeInput processes the `haproxy.router.openshift.io/rewrite-target`
+// annotation value for API compatibility while properly handling values
+// with spaces, backslashes, and other special characters. Because the
+// annotation value was initially introduced without being enclosed in
+// single quotes, certain characters were interpreted rather than being
+// treated literally. We later added the single quotes wrapping to resolve
+// OCPBUGS-22739. However, we must still maintain API compatibility after
+// this change: the annotation values MUST be interpreted to the same values
+// after updating to enclose the value in single quotes.
+func SanitizeInput(val string) string {
+	var encounteredCommentMarker bool
+
+	val = processRunes(val, newProcessHashCreator(&encounteredCommentMarker))
+	val = processRunes(val, processDoubleQuotes)
+	val = processRunes(val, processSingleQuotes)
+	val = replacePercentSigns(val)
+	val = removeSingleBackslashes(val)
+	val = convertDoubleBackslashes(val)
+
+	if !encounteredCommentMarker {
+		// The literal `\1` is appended to annotations without
+		// comments to comply with HAProxy rewrite rule
+		// expectations, which necessitate a capture group
+		// reference (i.e., `\1`) for dynamic substitutions.
+		val += `\1`
+	}
+
+	return val
+}

pkg/router/template/util/rewritetarget/rewritetarget_test.go

@@ -0,0 +1,94 @@
+package rewritetarget_test
+
+import (
+	"github.com/openshift/router/pkg/router/template/util/rewritetarget"
+	"testing"
+)
+
+func Test_SanitizeInput(t *testing.T) {
+	testCases := []struct {
+		name   string
+		input  string
+		output string
+	}{
+		{
+			name:   "single percent should be doubled",
+			input:  `%foo`,
+			output: `%%foo\1`,
+		},
+		{
+			name:   "double percent should be not changed",
+			input:  `%%foo`,
+			output: `%%foo\1`,
+		},
+		{
+			name:   "triple percent should be doubled entirely",
+			input:  `%%%foo`,
+			output: `%%%%%%foo\1`,
+		},
+		{
+			name:   "quad percent should be not changed",
+			input:  `%%%%foo`,
+			output: `%%%%foo\1`,
+		},
+		{
+			name:   "single quotes should be removed, unless escaped",
+			input:  `'foo'foo\'`,
+			output: `foofoo'\''\1`,
+		},
+		{
+			name:   "single backslash should be dropped",
+			input:  `\foo\`,
+			output: `foo\1`,
+		},
+		{
+			name:   "double backslash should be single",
+			input:  `\\foo\\`,
+			output: `\foo\\1`,
+		},
+		{
+			name:   "triple backslash should be double",
+			input:  `\\\foo\\\`,
+			output: `\\foo\\\1`,
+		},
+		{
+			name:   "quad backslash should be double",
+			input:  `\\\\foo\\\\`,
+			output: `\\foo\\\1`,
+		},
+		{
+			name:   "comment in beginning should be remove everything",
+			input:  `# foo # foo`,
+			output: ``,
+		},
+		{
+			name:   "don't remove escaped comments",
+			input:  `\# foo # foo`,
+			output: `# foo `,
+		},
+		{
+			name:   "comment in middle should remove everything following",
+			input:  `foo # foo`,
+			output: `foo `,
+		},
+		{
+			name:   "double quotes should get removed, unless escaped",
+			input:  `"foo"foo\"`,
+			output: `foofoo"\1`,
+		},
+		{
+			name:   "combination",
+			input:  `\\foo\"foo"\#foo\foo'foo\'#foo`,
+			output: `\foo"foo#foofoofoo'\''`,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := rewritetarget.SanitizeInput(tc.input)
+			if got != tc.output {
+				t.Errorf("Failure: expected %s, got %s", tc.output, got)
+			}
+		})
+	}
+}