You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2026/08/25 23:01:21 UTC
[PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
zwoop opened a new pull request, #13587:
URL: https://github.com/apache/trafficserver/pull/13587
A client can evade a string or regex condition by escaping any character of the pattern, so [NORM] normalizes the condition's value before it is matched. Normalization is percent-decoding for now, but the name leaves room for more.
Applying it in a non-virtual append_value() wrapper means it also covers regexes and %{} expansions inside operator values, neither of which the matcher-level modifiers (PRE/SUF/MID/EXT) reach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4232129582
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +76,36 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto in = s.find('%', start);
+
+ if (in == std::string::npos) {
+ return;
+ }
+
+ auto out = in;
+
+ while (in < s.size()) {
+ if (s[in] == '%' && s.size() - in > 2) {
+ const char *hex = s.data() + in + 1;
+ unsigned char c = 0;
+ auto [end, ec] = std::from_chars(hex, hex + 2, c, 16);
+
+ // Escaped controls stay encoded, since a decoded CR/LF in an expansion would inject a header field.
+ if (ec == std::errc{} && end == hex + 2 && c >= 0x20 && c != 0x7f) {
+ s[out++] = static_cast<char>(c);
+ in += 3;
+ continue;
Review Comment:
`std::from_chars` is specified for every integer type, `unsigned char` included, and this code builds on every CI platform (CentOS, Rocky, Debian, Fedora, Ubuntu, macOS). Leaving it as is.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -414,6 +415,20 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
Review Comment:
Updated the docstring in 11951df717.
##########
tools/hrw4u/src/common.py:
##########
@@ -43,20 +43,57 @@ class RegexPatterns:
SUBSTITUTE_PATTERN: Final = re.compile(
r"""(?P<escaped>\{\{.*?\}\})
|
- (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)\s*\}
+ (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)
+ (?:\s+with\s+(?P<func_mods>[A-Za-z][A-Za-z0-9,\s]*?))?\s*\}
|
(?<!%)\{(?P<var>[^{}()]+)\}
""",
re.VERBOSE | re.DOTALL,
)
+ # An interpolated symbol may carry modifiers, spelled as on a condition: {inbound.url.path with NORM}
+ INTERPOLATION_MODS: Final = re.compile(r'^\s*(?P<name>\S+?)\s+with\s+(?P<mods>[A-Za-z][A-Za-z0-9,\s]*?)\s*$')
+
+ # Trailing [MODS] inside a %{} block, which u4wrh turns back into a "with" clause
+ PERCENT_MODS: Final = re.compile(r'^(?P<body>.*?)\s+\[(?P<mods>[A-Za-z][A-Za-z0-9,\s]*)\]$')
+
# Additional performance patterns
IDENTIFIER: Final = re.compile(r'^[a-zA-Z_][a-zA-Z0-9_]*$')
WHITESPACE: Final = re.compile(r'\s+')
COMMENT_BLOCK: Final = re.compile(r'/\*.*?\*/', re.DOTALL)
STRING_INTERPOLATION: Final = re.compile(r'\{([a-zA-Z_][a-zA-Z0-9_.-]*(?:\([^)]*\))?)\}', re.MULTILINE)
+def parse_mods(raw: str | None) -> list[str]:
+ """Parse 'MOD, mod,MOD' into ['MOD', 'MOD', 'MOD']."""
+ return [mod.strip().upper() for mod in raw.split(",") if mod.strip()] if raw else []
+
+
Review Comment:
Fixed in 11951df717: `parse_mods()` collapses repeats, as condition modifiers already did. `with NORM,NORM` emitted `[NORM,NORM]`, and header_rewrite rejects the second as an unknown modifier.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -414,6 +415,20 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
Review Comment:
I dropped the change I made for this, because its output doesn't compile either: `int8_0 = "{inbound.url.path with NORM}"` fails with `Expected integer for int8 variable 'int8_0'`. Without NORM it's the same on master: u4wrh turns `set-state-int8 0 %{HEADER:X-Input}` into `int8_0 = inbound.req.X-Input;`, which hrw4u rejects the same way, since a state assignment only takes an integer literal or another state variable. So no `%{}` value in a state or session assignment has an hrw4u form, NORM or not. That's an existing u4wrh gap, and I'd rather fix it separately than add a special case here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#issuecomment-6088464294
[approve ci autest 1]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4234176031
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -413,7 +414,21 @@ def negate_expression(self, term: str) -> str:
return f"!({t})"
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
- """Convert percent block to identifier or function call."""
+ """Convert a %{} block to an identifier or function call; trailing [MODS] become a "with" clause."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
+
Review Comment:
Only NORM applies to a value expansion. header_rewrite tolerates the other modifiers there, but they do nothing, and hrw4u never generates them. If a hand-written config has one, u4wrh carrying it into the `with` clause and hrw4u rejecting it with "Not a value modifier, expected one of NORM" is the right outcome: it points at the mistake, where filtering would silently drop it and keeping it raw would pass a no-op through. Leaving it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4148457093
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -414,6 +415,20 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
Review Comment:
Fixed: modifier-bearing state/session values now reverse to a quoted interpolation, like the header path (`int8_0 = "{inbound.req.X-Input with NORM}";`), with a unit test in `test_coverage.py`.
A full round-trip test isn't possible yet: the forward's `validate_assignment()` accepts only literals or same-typed variables for int8/int16/bool, so `int8_0 = inbound.req.X-Input;` fails today without NORM too, although header_rewrite evaluates `%{...}` values for int8/int16. I'd rather close that pre-existing gap in a follow-up than grow this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4148622121
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +76,36 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto in = s.find('%', start);
+
+ if (in == std::string::npos) {
+ return;
+ }
+
+ auto out = in;
+
+ while (in < s.size()) {
+ if (s[in] == '%' && s.size() - in > 2) {
+ const char *hex = s.data() + in + 1;
+ unsigned char c = 0;
+ auto [end, ec] = std::from_chars(hex, hex + 2, c, 16);
+
+ // Escaped controls stay encoded, since a decoded CR/LF in an expansion would inject a header field.
+ if (ec == std::errc{} && end == hex + 2 && c >= 0x20 && c != 0x7f) {
+ s[out++] = static_cast<char>(c);
+ in += 3;
+ continue;
Review Comment:
`std::from_chars` support for `unsigned char` is less portable across standard library implementations than parsing into a wider integer type (e.g., `unsigned int`) and then casting. To reduce build/portability risk, parse into an `unsigned int` (or similar) and range-check before converting to `char`.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -414,6 +415,20 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
Review Comment:
The docstring for `percent_to_ident_or_func()` is now incomplete/misleading: it no longer only “converts percent block to identifier or function call”, it also parses trailing `[MODS]` and may return either `expr with ...` or a rewritten `%{... [MODS]}` when unmapped. Update the docstring to describe the modifier-aware behavior so callers understand the new contract.
##########
tools/hrw4u/src/common.py:
##########
@@ -43,20 +43,57 @@ class RegexPatterns:
SUBSTITUTE_PATTERN: Final = re.compile(
r"""(?P<escaped>\{\{.*?\}\})
|
- (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)\s*\}
+ (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)
+ (?:\s+with\s+(?P<func_mods>[A-Za-z][A-Za-z0-9,\s]*?))?\s*\}
|
(?<!%)\{(?P<var>[^{}()]+)\}
""",
re.VERBOSE | re.DOTALL,
)
+ # An interpolated symbol may carry modifiers, spelled as on a condition: {inbound.url.path with NORM}
+ INTERPOLATION_MODS: Final = re.compile(r'^\s*(?P<name>\S+?)\s+with\s+(?P<mods>[A-Za-z][A-Za-z0-9,\s]*?)\s*$')
+
+ # Trailing [MODS] inside a %{} block, which u4wrh turns back into a "with" clause
+ PERCENT_MODS: Final = re.compile(r'^(?P<body>.*?)\s+\[(?P<mods>[A-Za-z][A-Za-z0-9,\s]*)\]$')
+
# Additional performance patterns
IDENTIFIER: Final = re.compile(r'^[a-zA-Z_][a-zA-Z0-9_]*$')
WHITESPACE: Final = re.compile(r'\s+')
COMMENT_BLOCK: Final = re.compile(r'/\*.*?\*/', re.DOTALL)
STRING_INTERPOLATION: Final = re.compile(r'\{([a-zA-Z_][a-zA-Z0-9_.-]*(?:\([^)]*\))?)\}', re.MULTILINE)
+def parse_mods(raw: str | None) -> list[str]:
+ """Parse 'MOD, mod,MOD' into ['MOD', 'MOD', 'MOD']."""
+ return [mod.strip().upper() for mod in raw.split(",") if mod.strip()] if raw else []
+
+
Review Comment:
`parse_mods()` preserves duplicates, so an input like `with NORM,NORM` will flow through and get re-emitted as duplicate modifiers (and may trigger duplicate warnings). Consider de-duplicating while preserving order (e.g., first occurrence wins) to keep output stable and error reporting cleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4232279978
##########
plugins/header_rewrite/condition.cc:
##########
@@ -19,6 +19,7 @@
// condition.cc: Implementation of the condition base class
//
//
+#include <charconv>
#include <string>
Review Comment:
`Condition::normalize()` compares `ec` against `std::errc{}`, but this file does not explicitly include the header that declares `std::errc` (`<system_error>`). This can fail to compile depending on standard library/header transitive includes. Add `#include <system_error>` here (or otherwise ensure the translation unit includes it directly) to make the dependency explicit.
##########
tools/hrw4u/src/common.py:
##########
@@ -43,20 +43,57 @@ class RegexPatterns:
SUBSTITUTE_PATTERN: Final = re.compile(
r"""(?P<escaped>\{\{.*?\}\})
|
- (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)\s*\}
+ (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)
+ (?:\s+with\s+(?P<func_mods>[A-Za-z][A-Za-z0-9,\s]*?))?\s*\}
|
(?<!%)\{(?P<var>[^{}()]+)\}
""",
re.VERBOSE | re.DOTALL,
)
+ # An interpolated symbol may carry modifiers, spelled as on a condition: {inbound.url.path with NORM}
+ INTERPOLATION_MODS: Final = re.compile(r'^\s*(?P<name>\S+?)\s+with\s+(?P<mods>[A-Za-z][A-Za-z0-9,\s]*?)\s*$')
+
+ # Trailing [MODS] inside a %{} block, which u4wrh turns back into a "with" clause
+ PERCENT_MODS: Final = re.compile(r'^(?P<body>.*?)\s+\[(?P<mods>[A-Za-z][A-Za-z0-9,\s]*)\]$')
+
# Additional performance patterns
IDENTIFIER: Final = re.compile(r'^[a-zA-Z_][a-zA-Z0-9_]*$')
WHITESPACE: Final = re.compile(r'\s+')
COMMENT_BLOCK: Final = re.compile(r'/\*.*?\*/', re.DOTALL)
STRING_INTERPOLATION: Final = re.compile(r'\{([a-zA-Z_][a-zA-Z0-9_.-]*(?:\([^)]*\))?)\}', re.MULTILINE)
+def parse_mods(raw: str | None) -> list[str]:
+ """Parse 'MOD, mod,NOCASE' into ['MOD', 'NOCASE']; a repeat would fail header_rewrite's modifier check."""
Review Comment:
The docstring says repeated modifiers “would fail header_rewrite's modifier check”, but the implementation explicitly deduplicates repeats via `dict.fromkeys(...)` (so repeats do not fail). Update the docstring to match the behavior (dedup/collapse), or change the implementation to raise on duplicates if that’s the intended contract for hrw4u diagnostics.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -413,7 +414,21 @@ def negate_expression(self, term: str) -> str:
return f"!({t})"
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
- """Convert percent block to identifier or function call."""
+ """Convert a %{} block to an identifier or function call; trailing [MODS] become a "with" clause."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
+
Review Comment:
`percent_to_ident_or_func()` emits `with ...` modifiers in the user-provided order from `[MODS]`. Elsewhere (`CondState.to_with_modifiers`) modifiers are sorted into a canonical order (`WITH_MODIFIER_ORDER`). For stable, predictable round-trips (u4wrh → hrw4u) and fewer noisy diffs, consider normalizing `mods` to the same canonical ordering here before joining.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4232597847
##########
plugins/header_rewrite/condition.cc:
##########
@@ -19,6 +19,7 @@
// condition.cc: Implementation of the condition base class
//
//
+#include <charconv>
#include <string>
Review Comment:
`from_chars_result::ec` is a `std::errc`, so `<charconv>` has to provide the type, and ATS relies on that elsewhere (`cidr.h` in this plugin, `InkAPI.cc`, `MIME.cc`, `HTTP.cc`). Leaving it.
##########
tools/hrw4u/src/common.py:
##########
@@ -43,20 +43,57 @@ class RegexPatterns:
SUBSTITUTE_PATTERN: Final = re.compile(
r"""(?P<escaped>\{\{.*?\}\})
|
- (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)\s*\}
+ (?<!%)\{\s*(?P<func>[a-zA-Z_][a-zA-Z0-9_-]*)\s*\((?P<args>[^)]*)\)
+ (?:\s+with\s+(?P<func_mods>[A-Za-z][A-Za-z0-9,\s]*?))?\s*\}
|
(?<!%)\{(?P<var>[^{}()]+)\}
""",
re.VERBOSE | re.DOTALL,
)
+ # An interpolated symbol may carry modifiers, spelled as on a condition: {inbound.url.path with NORM}
+ INTERPOLATION_MODS: Final = re.compile(r'^\s*(?P<name>\S+?)\s+with\s+(?P<mods>[A-Za-z][A-Za-z0-9,\s]*?)\s*$')
+
+ # Trailing [MODS] inside a %{} block, which u4wrh turns back into a "with" clause
+ PERCENT_MODS: Final = re.compile(r'^(?P<body>.*?)\s+\[(?P<mods>[A-Za-z][A-Za-z0-9,\s]*)\]$')
+
# Additional performance patterns
IDENTIFIER: Final = re.compile(r'^[a-zA-Z_][a-zA-Z0-9_]*$')
WHITESPACE: Final = re.compile(r'\s+')
COMMENT_BLOCK: Final = re.compile(r'/\*.*?\*/', re.DOTALL)
STRING_INTERPOLATION: Final = re.compile(r'\{([a-zA-Z_][a-zA-Z0-9_.-]*(?:\([^)]*\))?)\}', re.MULTILINE)
+def parse_mods(raw: str | None) -> list[str]:
+ """Parse 'MOD, mod,NOCASE' into ['MOD', 'NOCASE']; a repeat would fail header_rewrite's modifier check."""
Review Comment:
Reworded in 5daff23b5d: repeats are dropped because header_rewrite would reject them.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -413,7 +414,21 @@ def negate_expression(self, term: str) -> str:
return f"!({t})"
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
- """Convert percent block to identifier or function call."""
+ """Convert a %{} block to an identifier or function call; trailing [MODS] become a "with" clause."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
+
Review Comment:
NORM is the only modifier valid on a value (`VALUE_MODIFIERS`), and hrw4u rejects any other in an interpolation, so there's never more than one to order. Leaving it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r3858309974
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +75,27 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto pos = s.find('%', start);
+
+ // Percent-decoding is the only normalization so far, hence the cheap bail-out.
+ if (pos == std::string::npos) {
+ return;
+ }
+
+ size_t len = s.size() - pos;
+ size_t written = 0;
+
+ // Decoding only shrinks, so it's safe in place; the +1 is room for the NUL it appends.
+ if (TSStringPercentDecode(s.data() + pos, len, s.data() + pos, len + 1, &written) == TS_SUCCESS) {
+ s.resize(pos + written);
+ } else {
+ Dbg(pi_dbg_ctl, "Failed to percent-decode, leaving the value untouched");
+ }
Review Comment:
Fixed in f687068a67.
The write itself is actually well-defined in C++20 — `[string.access]` says `operator[](size())` returns a reference to a `charT` object where *modifying it to any value other than `charT()`* is UB, so storing `'\0'` there is permitted, and `data()` is guaranteed to alias `&operator[](i)` for `i` in `[0, size()]`.
That said, you're right that it's reachable at `pos+len` (an invalid escape consumes nothing, so `data_written == len`), and relying on that guarantee isn't worth the reader's time. `normalize()` now grows the string by one scratch byte for the call and resizes it away afterwards, so nothing depends on the terminator slot.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -408,6 +409,16 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ # An unresolved block comes back verbatim, so re-attaching mods would duplicate them.
+ if mods and expr != stripped:
+ expr = f"{expr} with {','.join(mods)}"
+
Review Comment:
Good catch, fixed in f687068a67 — this one was a real bug, and my comment on the guard was wrong.
Since `_percent_to_ident_or_func()` is handed the *stripped* block, a verbatim return comes back without the mods, so re-attaching could never have duplicated them; it just dropped them. `%{READ_RESPONSE_HDR_HOOK [NORM]}` reversed to `%{READ_RESPONSE_HDR_HOOK}`.
Now the mods go back inside the braces when the block has no DSL equivalent, and into a `with` clause when it does. Added regression tests for both paths in `tests/test_coverage.py`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4232745236
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -413,7 +414,21 @@ def negate_expression(self, term: str) -> str:
return f"!({t})"
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
- """Convert percent block to identifier or function call."""
+ """Convert a %{} block to an identifier or function call; trailing [MODS] become a "with" clause."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
+
Review Comment:
`percent_to_ident_or_func()` converts any trailing `[MODS]` on a mapped `%{...}` into a DSL `with ...` clause, but HRW4U interpolation/value contexts only allow value-modifiers (currently just `NORM`). This can make u4wrh generate HRW4U that later fails to parse (e.g., `%{CLIENT-URL:PATH [NOCASE]}` would become `inbound.url.path with NOCASE`, which is rejected by the value-modifier validation). Consider filtering `mods` to `VALUE_MODIFIERS` when producing the `with ...` clause, and either (a) keep non-value modifiers inside the `%{...}` via `apply_percent_mods`, or (b) emit a clear error/warning during conversion when unsupported value modifiers are present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4066452454
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +75,30 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto pos = s.find('%', start);
+
+ // Percent-decoding is the only normalization so far.
+ if (pos == std::string::npos) {
+ return;
+ }
+
+ size_t len = s.size() - pos;
+ size_t written = 0;
+
+ // Decoding shrinks, so in place is safe; the extra byte is for the NUL it appends.
+ s.push_back('\0');
+
+ if (TSStringPercentDecode(s.data() + pos, len, s.data() + pos, len + 1, &written) != TS_SUCCESS) {
+ written = len;
+ Dbg(pi_dbg_ctl, "Failed to percent-decode, leaving the value untouched");
+ }
Review Comment:
This decoder is also used for `%{... [NORM]}` operator values, so a client-controlled path such as `%0d%0aX-Evil:%20yes` becomes raw CR/LF in the expanded string. `set-header` passes that string (with its explicit length) to the MIME field setter, which stores and serializes the bytes without filtering, allowing a configured response/request header to inject additional lines. Please reject or sanitize decoded control bytes for value expansions before they reach header/body sinks, or otherwise constrain NORM's output alphabet.
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +75,30 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto pos = s.find('%', start);
+
+ // Percent-decoding is the only normalization so far.
+ if (pos == std::string::npos) {
+ return;
+ }
+
+ size_t len = s.size() - pos;
+ size_t written = 0;
+
+ // Decoding shrinks, so in place is safe; the extra byte is for the NUL it appends.
+ s.push_back('\0');
+
+ if (TSStringPercentDecode(s.data() + pos, len, s.data() + pos, len + 1, &written) != TS_SUCCESS) {
+ written = len;
+ Dbg(pi_dbg_ctl, "Failed to percent-decode, leaving the value untouched");
+ }
Review Comment:
`TSStringPercentDecode()` does not report malformed input as `TS_ERROR` (the implementation always returns `TS_SUCCESS`), and its `unescape_str()` path drops an incomplete trailing escape. Consequently the fallback at lines 95-96 is unreachable: with `[NORM]`, a value ending in `%` or `%4` is shortened instead of preserved, which can create false matches and contradicts the malformed-escape behavior tested in the new gold test. Preserve incomplete escapes or explicitly reject/leave the value unchanged before calling this decoder.
##########
plugins/header_rewrite/value.cc:
##########
@@ -51,9 +54,19 @@ Value::set_value(const std::string &val, Statement *owner)
Condition *tcond_val = nullptr;
if (token.substr(0, 2) == "%{") {
- std::string cond_token = token.substr(2, token.size() - 3);
+ std::string cond_token = token.substr(2, token.size() - 3);
+ swoc::TextView cond_name{cond_token};
+
+ // The factory only wants the condition and its qualifier, so hide any trailing
+ // [MODS] from it. The Parser still sees them, and initialize() consumes them.
+ if (cond_name.ends_with(']')) {
+ if (auto pos = cond_name.rfind('['); pos != swoc::TextView::npos) {
Review Comment:
The new stripping logic treats any qualifier that ends in `]` as if it had a modifier suffix. This regresses valid query-parameter expansions such as `%{CLIENT-URL:QUERY:foo[]}`: `rfind('[')` removes `[]` before `condition_factory()` sees it, so the lookup is performed for `foo` instead of `foo[]`. Require whitespace before the `[` (the actual `%{} [MODS]` syntax) before removing this suffix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#issuecomment-5854741758
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r3858239823
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -408,6 +409,16 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ # An unresolved block comes back verbatim, so re-attaching mods would duplicate them.
+ if mods and expr != stripped:
+ expr = f"{expr} with {','.join(mods)}"
+
Review Comment:
percent_to_ident_or_func() strips trailing [MODS] via split_percent_mods(), but only re-attaches modifiers when the %{} block was successfully mapped to a DSL symbol/function (expr != stripped). If the block is left as a raw %{} expression (e.g., section-label tags or any unrecognized tag), modifiers are silently dropped during reverse conversion.
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +75,27 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto pos = s.find('%', start);
+
+ // Percent-decoding is the only normalization so far, hence the cheap bail-out.
+ if (pos == std::string::npos) {
+ return;
+ }
+
+ size_t len = s.size() - pos;
+ size_t written = 0;
+
+ // Decoding only shrinks, so it's safe in place; the +1 is room for the NUL it appends.
+ if (TSStringPercentDecode(s.data() + pos, len, s.data() + pos, len + 1, &written) == TS_SUCCESS) {
+ s.resize(pos + written);
+ } else {
+ Dbg(pi_dbg_ctl, "Failed to percent-decode, leaving the value untouched");
+ }
Review Comment:
Condition::normalize() passes dst_size=len+1 to TSStringPercentDecode(), which always writes a NUL terminator at dst[data_written]. With a std::string buffer this can write past the string’s current size (UB) unless the string is temporarily extended to provide that extra byte. This is especially relevant for invalid %-escapes where the decoded length may equal the input length (so the NUL would land at pos+len).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r3858309974
##########
plugins/header_rewrite/condition.cc:
##########
@@ -75,6 +75,27 @@ parse_matcher_op(std::string &arg)
}
}
+void
+Condition::normalize(std::string &s, size_t start)
+{
+ auto pos = s.find('%', start);
+
+ // Percent-decoding is the only normalization so far, hence the cheap bail-out.
+ if (pos == std::string::npos) {
+ return;
+ }
+
+ size_t len = s.size() - pos;
+ size_t written = 0;
+
+ // Decoding only shrinks, so it's safe in place; the +1 is room for the NUL it appends.
+ if (TSStringPercentDecode(s.data() + pos, len, s.data() + pos, len + 1, &written) == TS_SUCCESS) {
+ s.resize(pos + written);
+ } else {
+ Dbg(pi_dbg_ctl, "Failed to percent-decode, leaving the value untouched");
+ }
Review Comment:
Fixed in the latest push.
The write itself is actually well-defined in C++20 — `[string.access]` says `operator[](size())` returns a reference to a `charT` object where *modifying it to any value other than `charT()`* is UB, so storing `'\0'` there is permitted, and `data()` is guaranteed to alias `&operator[](i)` for `i` in `[0, size()]`.
That said, you're right that it's reachable at `pos+len` (an invalid escape consumes nothing, so `data_written == len`), and relying on that guarantee isn't worth the reader's time. `normalize()` now grows the string by one scratch byte for the call and resizes it away afterwards, so nothing depends on the terminator slot.
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -408,6 +409,16 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ # An unresolved block comes back verbatim, so re-attaching mods would duplicate them.
+ if mods and expr != stripped:
+ expr = f"{expr} with {','.join(mods)}"
+
Review Comment:
Good catch, fixed in the latest push — this one was a real bug, and my comment on the guard was wrong.
Since `_percent_to_ident_or_func()` is handed the *stripped* block, a verbatim return comes back without the mods, so re-attaching could never have duplicated them; it just dropped them. `%{READ_RESPONSE_HDR_HOOK [NORM]}` reversed to `%{READ_RESPONSE_HDR_HOOK}`.
Now the mods go back inside the braces when the block has no DSL equivalent, and into a `with` clause when it does. Added regression tests for both paths in `tests/test_coverage.py`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HRW: Adds a NORM normalization modifier for strings [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13587:
URL: https://github.com/apache/trafficserver/pull/13587#discussion_r4127410454
##########
tools/hrw4u/src/hrw_symbols.py:
##########
@@ -414,6 +415,20 @@ def negate_expression(self, term: str) -> str:
def percent_to_ident_or_func(self, percent: str, section: SectionType | None) -> tuple[str, bool]:
"""Convert percent block to identifier or function call."""
+ stripped, mods = split_percent_mods(percent)
+ expr, is_func = self._percent_to_ident_or_func(stripped, section)
+
+ if not mods:
+ return expr, is_func
+
+ # A block with no DSL equivalent comes back as %{...}, where the modifiers belong
+ # inside the braces rather than in a "with" clause.
+ if expr == stripped:
+ return apply_percent_mods(expr, mods), is_func
+
+ return f"{expr} with {','.join(mods)}", is_func
Review Comment:
[P2] Preserve interpolation syntax for normalized state assignments
This new return value also reaches the state/session-variable assignment branch in `op_to_hrw4u()`, which inserts it directly after `=` rather than wrapping it as an interpolation. For example, `set-state-int8 0 %{HEADER:X-Input [NORM]}` now converts to `int8_0 = inbound.req.X-Input with NORM;`. The forward grammar does not accept a `with` clause on assignment values, so the generated configuration cannot be compiled back to header_rewrite. Please update that caller to emit `int8_0 = "{inbound.req.X-Input with NORM}";` (and cover the state/session assignment path with a round-trip test). I reproduced this with the current `InverseSymbolResolver.op_to_hrw4u()`; the header-assignment path already emits the correct quoted interpolation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org