HRW: Allow sets to have quoted strings (#12256)

Author: zwoop

doc/admin-guide/plugins/header_rewrite.en.rst

@@ -739,6 +739,11 @@ The absence of an operand for conditions which accept them simply requires that
 a value exists (e.g. the content of the header is not an empty string) for the
 condition to be considered true.
 
+.. note::
+    Strings within a set can be quoted, but the quotes are not necessary. This
+    can be important if a matching string can include spaces or commas,
+    e.g. ``(foo,"bar,fie",baz)``.
+
 Condition Flags
 ---------------
 
@@ -764,9 +769,10 @@ EXT    The substring match only applies to the file extension following a dot.
        This is generally mostly useful for the ``URL:PATH`` part.
 ====== ========================================================================
 
-**Note**: At most, one of ``[PRE]``, ``[SUF]``, ``[MID]``, or ``[EXT]`` may be
-used at any time. They can however be used together with ``[NOCASE]`` and the
-other flags.
+.. note::
+    At most, one of ``[PRE]``, ``[SUF]``, ``[MID]``, or ``[EXT]`` may be
+    used at any time. They can however be used together with ``[NOCASE]`` and the
+    other flags.
 
 Operators
 ---------
@@ -898,7 +904,9 @@ Will call ``<URL>`` (see URL in `URL Parts`_) to retrieve a custom error respons
 and set the body with the result. Triggering this rule on an OK transaction will
 send a 500 status code to the client with the desired response. If this is triggered
 on any error status code, that original status code will be sent to the client.
-**Note**: This config should only be set using READ_RESPONSE_HDR_HOOK
+
+.. note::
+    This config should only be set using READ_RESPONSE_HDR_HOOK
 
 An example config would look like::
 
@@ -958,8 +966,9 @@ the appropriate logs even when the debug tag has not been enabled. For
 additional information on |TS| debugging statements, refer to
 :ref:`developer-debug-tags` in the developer's documentation.
 
-**Note**: This operator is deprecated, use the `set-http-cntl`_ operator instead,
-with the ``TXN_DEBUG`` control.
+.. note::
+    This operator is deprecated, use the `set-http-cntl`_ operator instead,
+    with the ``TXN_DEBUG`` control.
 
 set-destination
 ~~~~~~~~~~~~~~~
@@ -1074,8 +1083,9 @@ When invoked, and when ``<value>`` is any of ``1``, ``true``, or ``TRUE``, this
 operator causes |TS| to abort further request remapping. Any other value and
 the operator will effectively be a no-op.
 
-**Note**: This operator is deprecated, use the `set-http-cntl`_ operator instead,
-with the ``SKIP_REMAP`` control.
+.. note::
+    This operator is deprecated, use the `set-http-cntl`_ operator instead,
+    with the ``SKIP_REMAP`` control.
 
 set-cookie
 ~~~~~~~~~~

plugins/header_rewrite/matcher.h

@@ -194,13 +194,39 @@ template <class T> class Matchers : public Matcher
     // MATCH_SET (allowed for any T)
     if (_op == MATCH_SET) {
       _data.template emplace<std::set<T>>();
+      auto          &values = std::get<std::set<T>>(_data);
+      swoc::TextView src{s};
+      bool           in_quotes = false;
+      size_t         start = 0, cur = 0, skip_quotes = 0;
+
+      while (cur < src.size()) {
+        if (src[cur] == '"') {
+          skip_quotes = 1;
+          in_quotes   = !in_quotes;
+          ++cur;
+        } else if (src[cur] == ',' && !in_quotes) {
+          swoc::TextView field = src.substr(start + skip_quotes, cur - start - skip_quotes * 2);
+
+          field.ltrim_if(&isspace).rtrim_if(&isspace);
+          values.insert(convert(std::string(field)));
+          start       = ++cur + skip_quotes;
+          skip_quotes = 0;
+        } else {
+          ++cur;
+        }
+      }
+
+      if (in_quotes) {
+        Dbg(pi_dbg_ctl, "Invalid set: unmatched quotes in: %s", s.c_str());
+        throw std::runtime_error("Malformed set, unmatched quotes");
+      }
 
-      auto              &values = std::get<std::set<T>>(_data);
-      std::istringstream stream(s);
-      std::string        part;
+      // Last field (if any)
+      if (start < src.size()) {
+        swoc::TextView field = src.substr(start + skip_quotes, src.size() - start - skip_quotes * 2);
 
-      while (std::getline(stream, part, ',')) {
-        values.insert(convert(part));
+        field.ltrim_if(&isspace).rtrim_if(&isspace);
+        values.insert(convert(std::string(field)));
       }
 
       if (!values.empty()) {

plugins/header_rewrite/parser.cc

@@ -28,7 +28,7 @@
 
 #include "parser.h"
 
-enum ParserState { PARSER_DEFAULT, PARSER_IN_QUOTE, PARSER_IN_REGEX, PARSER_IN_EXPANSION, PARSER_IN_BRACE };
+enum ParserState { PARSER_DEFAULT, PARSER_IN_QUOTE, PARSER_IN_REGEX, PARSER_IN_EXPANSION, PARSER_IN_BRACE, PARSER_IN_PAREN };
 
 bool
 Parser::parse_line(const std::string &original_line)
@@ -71,7 +71,7 @@ Parser::parse_line(const std::string &original_line)
         cur_token_start  = i;
       }
       line.erase(i, 1);
-    } else if ((state != PARSER_IN_REGEX) && (line[i] == '"')) {
+    } else if ((state != PARSER_IN_REGEX) && (state != PARSER_IN_PAREN) && (line[i] == '"')) {
       if ((state != PARSER_IN_QUOTE) && !extracting_token) {
         state            = PARSER_IN_QUOTE;
         extracting_token = true;
@@ -97,6 +97,15 @@ Parser::parse_line(const std::string &original_line)
       _tokens.push_back(line.substr(cur_token_start, cur_token_length));
       state            = PARSER_DEFAULT;
       extracting_token = false;
+    } else if ((state == PARSER_DEFAULT) && (line[i] == '(')) {
+      state            = PARSER_IN_PAREN;
+      extracting_token = true;
+      cur_token_start  = i;
+    } else if ((state == PARSER_IN_PAREN) && (line[i] == ')')) {
+      cur_token_length = i - cur_token_start + 1;
+      _tokens.push_back(line.substr(cur_token_start, cur_token_length));
+      state            = PARSER_DEFAULT;
+      extracting_token = false;
     } else if (!extracting_token) {
       if (_tokens.empty() && line[i] == '#') {
         // this is a comment line (it may have had leading whitespace before the #)

plugins/header_rewrite/parser.h

@@ -109,6 +109,7 @@ class Parser
 
   bool parse_line(const std::string &original_line);
 
+  // We chose to have this take a std::string, since some of these conversions can not take a TextView easily
   template <typename NumericT>
   static NumericT
   parseNumeric(const std::string &s)

tests/gold_tests/pluginTest/header_rewrite/gold/ext-sets.gold

@@ -7,4 +7,5 @@
 < Date: ``
 ``
 < X-Extension: Yes
+< X-Testing: Yes
 ``

tests/gold_tests/pluginTest/header_rewrite/gold/header_rewrite-client.gold

@@ -10,4 +10,5 @@
 < Server: ATS/``
 < Cache-Control: no-store
 < X-Pre-Else: Yes
+< X-Testing: No
 ``

tests/gold_tests/pluginTest/header_rewrite/header_rewrite_url.test.py

@@ -90,7 +90,8 @@
 tr = Test.AddTestRun()
 tr.MakeCurlCommand(
     '--proxy 127.0.0.1:{0} "http://www.example.com/from_path/hrw-sets.png" '
-    '-H "Proxy-Connection: keep-alive" --verbose'.format(ts.Variables.port))
+    '-H "Proxy-Connection: keep-alive" -H "X-Testing: foo,bar" '
+    '--verbose'.format(ts.Variables.port))
 tr.Processes.Default.ReturnCode = 0
 tr.Processes.Default.Streams.stderr = "gold/ext-sets.gold"
 tr.StillRunningAfter = server

tests/gold_tests/pluginTest/header_rewrite/rules/rule_client.conf

@@ -30,3 +30,9 @@ cond %{CLIENT-URL:PATH} (hrw,foo) [MID,NOCASE]
   no-op
 else
   set-header X-Pre-Else "Yes"
+
+cond %{SEND_RESPONSE_HDR_HOOK}
+cond %{CLIENT-HEADER:X-Testing} (foo,bar,"foo,bar")
+  set-header X-Testing "Yes"
+else
+  set-header X-Testing "No"