Copilot commented on code in PR #12672:
URL: https://github.com/apache/trafficserver/pull/12672#discussion_r2582738664
##########
plugins/experimental/cookie_remap/cookie_remap.cc:
##########
@@ -1268,12 +1344,101 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp,
TSRemapRequestInfo *rri)
TSError("can't parse substituted URL string");
goto error;
} else {
+ bool host_header_was_set = false;
+
+ // Set custom headers if configured and we took the sendto path.
+ if (!op->getSendtoHeaders().empty() && used_sendto) {
+ for (auto const &header_pair : op->getSendtoHeaders()) {
+ std::string header_name = header_pair.first;
+ std::string header_value = header_pair.second;
+
+ // Apply regex substitution to header value if we have regex
matches ($1, $2, etc.)
+ if (regex_ccount > 0 && !regex_match_strings.empty() &&
header_value.find('$') != std::string::npos) {
+ std::string::size_type pos = 0;
+ std::string::size_type ppos = 0;
+ std::string substituted_value;
+ substituted_value.reserve(header_value.size() * 2);
+
+ while (pos < header_value.length()) {
+ pos = header_value.find('$', ppos);
+ if (pos == std::string::npos) {
+ break;
+ }
+ // Check if there's a digit after the $
+ if (pos + 1 < header_value.length() &&
isdigit(header_value[pos + 1])) {
+ int const ix = header_value[pos + 1] - '0';
+ if (ix <= regex_ccount && ix <
static_cast<int>(regex_match_strings.size())) {
+ // Append everything before the $
+ substituted_value += header_value.substr(ppos, pos - ppos);
+ // Append the regex match string
+ substituted_value += regex_match_strings[ix];
+ // Move past the $N
+ ppos = pos + 2;
+ }
+ }
+ pos++;
+ }
+ // Append any remaining text
+ if (ppos < header_value.length()) {
+ substituted_value += header_value.substr(ppos);
+ }
+ header_value = substituted_value;
+ }
Review Comment:
The regex substitution logic for header values has a bug where unmatched `$`
characters are silently dropped. If a `$` in the header value is not followed
by a valid digit or doesn't match a regex capture group, the character is
skipped rather than preserved in the output.
For example, if `header_value = "test$invalid"` and there are no regex
matches, the output will be `"test"` instead of `"test$invalid"`.
The issue is that when the condition at line 1368-1377 is not met (e.g., `$`
not followed by a digit, or invalid capture group index), the loop increments
`pos++` at line 1379 but never appends the `$` character to
`substituted_value`. The `ppos` pointer is only updated when a successful
substitution occurs, so non-substituted `$` characters between `ppos` and `pos`
are lost.
The fix should either:
1. Append the `$` character when it's not part of a valid substitution
pattern (similar to how the original sendto URL substitution works at lines
745-765), or
2. Only increment `ppos` when a substitution is made, and handle the `$`
character explicitly when it's not part of a valid pattern.
##########
plugins/experimental/cookie_remap/cookie_remap.cc:
##########
@@ -1268,12 +1344,101 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp,
TSRemapRequestInfo *rri)
TSError("can't parse substituted URL string");
goto error;
} else {
+ bool host_header_was_set = false;
+
+ // Set custom headers if configured and we took the sendto path.
+ if (!op->getSendtoHeaders().empty() && used_sendto) {
+ for (auto const &header_pair : op->getSendtoHeaders()) {
+ std::string header_name = header_pair.first;
+ std::string header_value = header_pair.second;
+
+ // Apply regex substitution to header value if we have regex
matches ($1, $2, etc.)
+ if (regex_ccount > 0 && !regex_match_strings.empty() &&
header_value.find('$') != std::string::npos) {
+ std::string::size_type pos = 0;
+ std::string::size_type ppos = 0;
+ std::string substituted_value;
+ substituted_value.reserve(header_value.size() * 2);
+
+ while (pos < header_value.length()) {
+ pos = header_value.find('$', ppos);
+ if (pos == std::string::npos) {
+ break;
+ }
+ // Check if there's a digit after the $
+ if (pos + 1 < header_value.length() &&
isdigit(header_value[pos + 1])) {
+ int const ix = header_value[pos + 1] - '0';
+ if (ix <= regex_ccount && ix <
static_cast<int>(regex_match_strings.size())) {
+ // Append everything before the $
+ substituted_value += header_value.substr(ppos, pos - ppos);
+ // Append the regex match string
+ substituted_value += regex_match_strings[ix];
+ // Move past the $N
+ ppos = pos + 2;
+ }
+ }
+ pos++;
+ }
+ // Append any remaining text
+ if (ppos < header_value.length()) {
+ substituted_value += header_value.substr(ppos);
+ }
+ header_value = substituted_value;
+ }
+
+ // Apply cr_substitutions for variables like $path, $cr_req_url,
etc.
+ cr_substitutions(header_value, req_url);
+
+ Dbg(dbg_ctl, "Setting header: %s to value: %s",
header_name.c_str(), header_value.c_str());
+
+ // Find or create the header
+ TSMLoc field_loc = TSMimeHdrFieldFind(rri->requestBufp,
rri->requestHdrp, header_name.c_str(), header_name.length());
+
+ if (field_loc == TS_NULL_MLOC) {
+ // Header doesn't exist, create it
+ if (TS_SUCCESS == TSMimeHdrFieldCreateNamed(rri->requestBufp,
rri->requestHdrp, header_name.c_str(),
+
header_name.length(), &field_loc)) {
+ if (TS_SUCCESS ==
TSMimeHdrFieldValueStringSet(rri->requestBufp, rri->requestHdrp, field_loc, -1,
+
header_value.c_str(), header_value.length())) {
+ TSMimeHdrFieldAppend(rri->requestBufp, rri->requestHdrp,
field_loc);
+ Dbg(dbg_ctl, "Created and set header: %s",
header_name.c_str());
+ }
+ TSHandleMLocRelease(rri->requestBufp, rri->requestHdrp,
field_loc);
+ }
+ } else {
+ // Header exists, update it
+ TSMLoc tmp = TS_NULL_MLOC;
+ bool first = true;
+
+ while (field_loc != TS_NULL_MLOC) {
+ tmp = TSMimeHdrFieldNextDup(rri->requestBufp,
rri->requestHdrp, field_loc);
+ if (first) {
+ first = false;
+ if (TS_SUCCESS ==
TSMimeHdrFieldValueStringSet(rri->requestBufp, rri->requestHdrp, field_loc, -1,
+
header_value.c_str(), header_value.length())) {
+ Dbg(dbg_ctl, "Updated header: %s", header_name.c_str());
+ }
+ } else {
+ // Remove duplicate headers
+ TSMimeHdrFieldDestroy(rri->requestBufp, rri->requestHdrp,
field_loc);
+ }
+ TSHandleMLocRelease(rri->requestBufp, rri->requestHdrp,
field_loc);
+ field_loc = tmp;
+ }
+ }
+
+ // Check if we're setting the Host header (case-insensitive)
+ if (strcasecmp(header_name.c_str(), "Host") == 0) {
Review Comment:
Missing include for `strcasecmp`. The `strcasecmp` function requires
`<cstring>` to be included. While this may compile on some systems due to
transitive includes, it's not guaranteed by the C++ standard and should be
explicitly included.
Other files in the same plugin (cookiejar.cc, hash.cc, strip.cc) and other
plugins in the codebase (url_sig.cc) that use `strcasecmp` include `<cstring>`.
Add `#include <cstring>` to the includes at the top of the file (around line
34).
##########
doc/admin-guide/plugins/cookie_remap.en.rst:
##########
@@ -233,6 +234,110 @@ This option only affects the successful match
(``sendto``) path. The ``else``
path will continue to use the configured pristine host header setting
(typically
enabled in production environments).
+.. _set-sendto-headers:
+
+set_sendto_headers: [optional]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Sets arbitrary HTTP request headers when a rule matches and takes the
``sendto``
+path. This provides a flexible way to modify request headers, including the
+Host header, for redirected requests.
+
+Headers are only set when:
+
+* The operation matches successfully.
+* The ``sendto`` path is taken (not the ``else`` path).
+
+**Format:**
+
+The value must be a YAML sequence (list) where each item is a single-key map
+representing a header name and its value:
+
+.. code-block:: yaml
+
+ set_sendto_headers:
+ - Header-Name: header-value
+ - Another-Header: another-value
+
+**Header Value Substitution:**
+
+The special variables mentioned below for regex capture groups, path variables,
+URL variables, unmatched path, and URL encoding can be used in the header
value.
+
+* **Regex capture groups**: ``$1``, ``$2``, ... ``$9`` (from regex operations).
+* **Path variables**: ``$path``, ``$ppath`` (pre-remapped path).
+* **URL variables**: ``$cr_req_url``, ``$cr_req_purl`` (pre-remapped URL).
+* **Unmatched path**: ``$unmatched_path``, ``$unmatched_ppath``.
+* **URL encoding**: ``$cr_urlencode(...)``.
+
+**Special Behavior for Host Header:**
+
+When ``set_sendto_headers`` includes a ``Host`` header (case-insensitive), the
+pristine host header is automatically disabled for that transaction. This
allows
+the Host header to be updated to the specified value. You do not need to also
+set ``disable_pristine_host_hdr: true`` in this case.
+
+**Interaction with disable_pristine_host_hdr:**
+
+* If ``set_sendto_headers`` sets the Host header, pristine host header is
+ automatically disabled.
+* If ``set_sendto_headers`` does NOT set Host but ``disable_pristine_host_hdr``
+ is ``true``, pristine host header is still disabled
Review Comment:
Missing period at the end of the sentence. The sentence should end with a
period for consistency with the other bullet points.
```suggestion
is ``true``, pristine host header is still disabled.
```
--
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]
