Copilot commented on code in PR #12822:
URL: https://github.com/apache/trafficserver/pull/12822#discussion_r2700317384
##########
plugins/header_rewrite/conditions.cc:
##########
@@ -280,7 +282,26 @@ ConditionUrl::set_qualifier(const std::string &q)
Condition::set_qualifier(q);
Dbg(pi_dbg_ctl, "\tParsing %%{URL:%s}", q.c_str());
- _url_qual = parse_url_qualifier(q);
+
+ std::string::size_type pos = q.find(':');
+
+ if (pos != std::string::npos) {
+ std::string qual_part = q.substr(0, pos);
+ std::string sub_qual = q.substr(pos + 1);
+
+ _url_qual = parse_url_qualifier(qual_part);
+
+ if (_url_qual == URL_QUAL_QUERY) {
+ if (!sub_qual.empty()) {
+ _query_param = sub_qual;
+ Dbg(pi_dbg_ctl, "\tQuery parameter sub-key: %s", _query_param.c_str());
+ }
+ } else {
+ TSError("[%s] Sub-qualifier not supported for URL component: %s",
PLUGIN_NAME, qual_part.c_str());
Review Comment:
The error message should be more specific about which URL component was
specified and clarify that sub-qualifiers are only supported for QUERY.
Consider changing the message to: 'Sub-qualifier syntax (component:subkey) is
only supported for QUERY component, got: %s' to help users understand the
limitation.
```suggestion
TSError("[%s] Sub-qualifier syntax (component:subkey) is only
supported for QUERY component, got: %s", PLUGIN_NAME,
qual_part.c_str());
```
##########
plugins/header_rewrite/resources.cc:
##########
@@ -181,3 +181,32 @@ Resources::destroy()
_ready = false;
}
+
+swoc::TextView
+Resources::get_query_param(const std::string &name, const char *query_str, int
query_len) const
+{
+ if (!_extended_info.query_parsed) {
+ if (query_str && query_len > 0) {
+ swoc::TextView query_view(query_str, query_len);
+
+ while (!query_view.empty()) {
+ swoc::TextView param = query_view.take_prefix_at('&');
+ swoc::TextView param_name = param.take_prefix_at('=');
+ swoc::TextView param_value = param;
+
+ if (!param_name.empty()) {
+ _extended_info.query_params[param_name] = param_value;
Review Comment:
The query parameter parsing does not handle duplicate query parameters
correctly. When a parameter appears multiple times in the query string (e.g.,
`?tag=foo&tag=bar`), the current implementation will overwrite earlier values
with later ones, storing only the last occurrence. This could lead to incorrect
behavior when conditions expect to match against the first occurrence or when
all values should be considered. Consider documenting this limitation or
implementing support for multiple values (e.g., by storing the first occurrence
or concatenating values).
```suggestion
// Preserve the first occurrence of each parameter name.
_extended_info.query_params.emplace(param_name, param_value);
```
##########
doc/admin-guide/plugins/header_rewrite.en.rst:
##########
@@ -2000,3 +2004,17 @@ Those will pick the address provided by PROXY protocol,
instead of the peer's ad
cond %{SEND_RESPONSE_HDR_HOOK}
set-header real-ip %{INBOUND:REMOTE-ADDR}
+
+Route Based on Query Parameter Value
+------------------------------------
+
+This rule extracts a specific query parameter value and uses it to set a custom
+header or make routing decisions. The ``QUERY:<param_name>`` sub-key syntax
+allows extracting individual query parameter values::
+
+ cond %{REMAP_PSEUDO_HOOK} [AND]
+ cond %{CLIENT-URL:QUERY:version} ="v2"
Review Comment:
The documentation example uses a loose equality operator `=` which may not
match the exact string 'v2' if there are additional characters. Consider using
the exact match operator or clarifying in the documentation whether partial
matches are intended. The example should demonstrate best practices for
matching query parameter values.
```suggestion
cond %{CLIENT-URL:QUERY:version} =="v2"
```
##########
plugins/header_rewrite/conditions.cc:
##########
@@ -347,8 +368,15 @@ ConditionUrl::append_value(std::string &s, const Resources
&res)
break;
case URL_QUAL_QUERY:
q_str = TSUrlHttpQueryGet(bufp, url, &i);
- s.append(q_str, i);
- Dbg(pi_dbg_ctl, " Query parameters to match is: %.*s", i, q_str);
+ if (_query_param.empty()) {
+ s.append(q_str, i);
+ Dbg(pi_dbg_ctl, " Query parameters to match is: %.*s", i, q_str);
+ } else {
+ swoc::TextView value = res.get_query_param(_query_param, q_str, i);
+
+ s.append(value.data(), value.size());
+ Dbg(pi_dbg_ctl, " Query parameter %s value is: %.*s",
_query_param.c_str(), static_cast<int>(value.size()), value.data());
Review Comment:
If `value.data()` returns nullptr (when the parameter is not found and an
empty swoc::TextView is returned), appending it to the string could lead to
undefined behavior. While swoc::TextView likely handles this safely by
returning a valid pointer with size 0, this should be verified or explicitly
checked for defensive programming.
```suggestion
if (value.data() != nullptr && value.size() > 0) {
s.append(value.data(), value.size());
Dbg(pi_dbg_ctl, " Query parameter %s value is: %.*s",
_query_param.c_str(), static_cast<int>(value.size()), value.data());
} else {
Dbg(pi_dbg_ctl, " Query parameter %s is empty or not present",
_query_param.c_str());
}
```
##########
plugins/header_rewrite/resources.cc:
##########
@@ -181,3 +181,32 @@ Resources::destroy()
_ready = false;
}
+
+swoc::TextView
+Resources::get_query_param(const std::string &name, const char *query_str, int
query_len) const
+{
+ if (!_extended_info.query_parsed) {
+ if (query_str && query_len > 0) {
+ swoc::TextView query_view(query_str, query_len);
+
+ while (!query_view.empty()) {
+ swoc::TextView param = query_view.take_prefix_at('&');
+ swoc::TextView param_name = param.take_prefix_at('=');
+ swoc::TextView param_value = param;
+
+ if (!param_name.empty()) {
+ _extended_info.query_params[param_name] = param_value;
+ }
+ }
Review Comment:
The query parameter parsing logic lacks documentation explaining that
URL-encoded parameter names and values are not decoded. Users may expect
`%{CLIENT-URL:QUERY:my%20param}` to match a parameter named 'my param', but it
will only match the literal 'my%20param'. Add a comment documenting this
behavior or consider adding URL decoding support.
##########
plugins/header_rewrite/resources.cc:
##########
@@ -181,3 +181,32 @@ Resources::destroy()
_ready = false;
}
+
+swoc::TextView
+Resources::get_query_param(const std::string &name, const char *query_str, int
query_len) const
+{
+ if (!_extended_info.query_parsed) {
+ if (query_str && query_len > 0) {
+ swoc::TextView query_view(query_str, query_len);
+
+ while (!query_view.empty()) {
+ swoc::TextView param = query_view.take_prefix_at('&');
+ swoc::TextView param_name = param.take_prefix_at('=');
+ swoc::TextView param_value = param;
+
+ if (!param_name.empty()) {
+ _extended_info.query_params[param_name] = param_value;
+ }
+ }
+ }
+ _extended_info.query_parsed = true;
+ }
+
+ auto it = _extended_info.query_params.find(swoc::TextView(name));
+
+ if (it != _extended_info.query_params.end()) {
+ return it->second;
+ }
+
Review Comment:
The query parameter cache stores swoc::TextView objects that reference the
original `query_str` pointer, but there's no guarantee that the buffer remains
valid for the lifetime of the cache. The query string returned by
TSUrlHttpQueryGet points into a TSMBuffer that could potentially be modified or
freed. If the URL is changed after caching but before a subsequent access, the
cached TextView objects could reference freed memory, leading to use-after-free
issues. The cache should either copy the string data or be invalidated when the
underlying buffer could be affected.
```suggestion
if (!(query_str && query_len > 0)) {
return swoc::TextView();
}
swoc::TextView query_view(query_str, query_len);
swoc::TextView target_name(name);
while (!query_view.empty()) {
swoc::TextView param = query_view.take_prefix_at('&');
swoc::TextView param_name = param.take_prefix_at('=');
if (!param_name.empty() && param_name == target_name) {
// 'param' now contains the value portion of the "name=value" pair.
return param;
}
}
```
--
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]
