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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
