This fixes parsing where the [ ] section gets merged into values

For example:

    cond %{SEND_RESPONSE_HDR_HOOK}
     set-header X-First "First"
     set-header X-Last "Last" [L]

would set the header X-Last: Last [L].

In addition, it also fixes a typo (I think?) in an unrelated test,
however, it's unclear as to why it's not failing that test.

    CHECK_EQ(p.get_tokens()[1], "%{METHOD}c”);

Author: zwoop

plugins/header_rewrite/header_rewrite.cc

@@ -184,8 +184,10 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook)
       continue;
     }
 
-    Parser p(line); // Tokenize and parse this line
-    if (p.empty()) {
+    Parser p;
+
+    // Tokenize and parse this line
+    if (!p.parse_line(line) || p.empty()) {
       continue;
     }
 

plugins/header_rewrite/header_rewrite_test.cc

@@ -44,7 +44,12 @@ TSError(const char *fmt, ...)
 class ParserTest : public Parser
 {
 public:
-  ParserTest(const std::string &line) : Parser(line), res(true) { std::cout << "Finished parser test: " << line << std::endl; }
+  ParserTest(const std::string &line) : res(true)
+  {
+    Parser::parse_line(line);
+    std::cout << "Finished parser test: " << line << std::endl;
+  }
+
   std::vector<std::string>
   getTokens() const
   {
@@ -353,12 +358,13 @@ test_parsing()
   }
 
   {
-    ParserTest p(R"(set-header Alt-Svc "quic=\":443\"; v=\"35\"")");
+    ParserTest p(R"(set-header Alt-Svc "quic=\":443\"; v=\"35\"" [L])");
 
-    CHECK_EQ(p.getTokens().size(), 3UL);
+    CHECK_EQ(p.getTokens().size(), 4UL);
     CHECK_EQ(p.getTokens()[0], "set-header");
     CHECK_EQ(p.getTokens()[1], "Alt-Svc");
     CHECK_EQ(p.getTokens()[2], R"(quic=":443"; v="35")");
+    CHECK_EQ(p.get_value(), R"(quic=":443"; v="35")");
 
     END_TEST();
   }
@@ -479,7 +485,7 @@ test_tokenizer()
     SimpleTokenizerTest p("A racoon's favorite tag is %{METHOD} in %{NOW:YEAR}!");
     CHECK_EQ(p.get_tokens().size(), 5UL);
     CHECK_EQ(p.get_tokens()[0], "A racoon's favorite tag is ");
-    CHECK_EQ(p.get_tokens()[1], "%{METHOD}c");
+    CHECK_EQ(p.get_tokens()[1], "%{METHOD}");
     CHECK_EQ(p.get_tokens()[2], " in ");
     CHECK_EQ(p.get_tokens()[3], "%{NOW:YEAR}");
     CHECK_EQ(p.get_tokens()[4], "!");

plugins/header_rewrite/parser.cc

@@ -30,7 +30,8 @@
 
 enum ParserState { PARSER_DEFAULT, PARSER_IN_QUOTE, PARSER_IN_REGEX, PARSER_IN_EXPANSION };
 
-Parser::Parser(const std::string &original_line) : _cond(false), _empty(false)
+bool
+Parser::parse_line(const std::string &original_line)
 {
   std::string line        = original_line;
   ParserState state       = PARSER_DEFAULT;
@@ -85,7 +86,7 @@ Parser::Parser(const std::string &original_line) : _cond(false), _empty(false)
         TSError("[%s] malformed line \"%s\", ignoring", PLUGIN_NAME, line.c_str());
         _tokens.clear();
         _empty = true;
-        return;
+        return false;
       }
     } else if (!extracting_token) {
       if (_tokens.empty() && line[i] == '#') {
@@ -114,21 +115,49 @@ Parser::Parser(const std::string &original_line) : _cond(false), _empty(false)
       TSError("[%s] malformed line, unterminated quotation: \"%s\", ignoring", PLUGIN_NAME, line.c_str());
       _tokens.clear();
       _empty = true;
-      return;
+      return false;
     }
   }
 
   if (_tokens.empty()) {
     _empty = true;
   } else {
-    preprocess(_tokens);
+    return preprocess(_tokens);
   }
+
+  return true;
 }
 
-// This is the core "parser", parsing rule sets
-void
+// This is the main "parser", a helper function to the above tokenizer. NOTE: this modifies (possibly) the tokens list,
+// therefore, we pass in a copy of the parsers tokens here, such that the original token list is retained (useful for tests etc.).
+bool
 Parser::preprocess(std::vector<std::string> tokens)
 {
+  // The last token might be the "flags" section, lets consume it if it is
+  if (tokens.size() > 0) {
+    std::string m = tokens[tokens.size() - 1];
+
+    if (!m.empty() && (m[0] == '[')) {
+      if (m[m.size() - 1] == ']') {
+        m = m.substr(1, m.size() - 2);
+        if (m.find_first_of(',') != std::string::npos) {
+          std::istringstream iss(m);
+          std::string t;
+          while (getline(iss, t, ',')) {
+            _mods.push_back(t);
+          }
+        } else {
+          _mods.push_back(m);
+        }
+        tokens.pop_back(); // consume it, so we don't concatenate it into the value
+      } else {
+        // Syntax error
+        TSError("[%s] mods have to be enclosed in []", PLUGIN_NAME);
+        return false;
+      }
+    }
+  }
+
   // Special case for "conditional" values
   if (tokens[0].substr(0, 2) == "%{") {
     _cond = true;
@@ -155,7 +184,7 @@ Parser::preprocess(std::vector<std::string> tokens)
       }
     } else {
       TSError("[%s] conditions must be embraced in %%{}", PLUGIN_NAME);
-      return;
+      return false;
     }
   } else {
     // Operator has no qualifiers, but could take an optional second argument
@@ -179,29 +208,7 @@ Parser::preprocess(std::vector<std::string> tokens)
     }
   }
 
-  // The last token might be the "flags" section
-  if (tokens.size() > 0) {
-    std::string m = tokens[tokens.size() - 1];
-
-    if (!m.empty() && (m[0] == '[')) {
-      if (m[m.size() - 1] == ']') {
-        m = m.substr(1, m.size() - 2);
-        if (m.find_first_of(',') != std::string::npos) {
-          std::istringstream iss(m);
-          std::string t;
-          while (getline(iss, t, ',')) {
-            _mods.push_back(t);
-          }
-        } else {
-          _mods.push_back(m);
-        }
-      } else {
-        // Syntax error
-        TSError("[%s] mods have to be enclosed in []", PLUGIN_NAME);
-        return;
-      }
-    }
-  }
+  return true;
 }
 
 // Check if the operator is a condition, a hook, and if so, which hook. If the cond is not a hook
@@ -241,12 +248,6 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
   return false;
 }
 
-const std::vector<std::string> &
-Parser::get_tokens() const
-{
-  return _tokens;
-}
-
 SimpleTokenizer::SimpleTokenizer(const std::string &original_line)
 {
   std::string line        = original_line;
@@ -294,9 +295,3 @@ SimpleTokenizer::SimpleTokenizer(const std::string &original_line)
     _tokens.push_back(line.substr(cur_token_start));
   }
 }
-
-const std::vector<std::string> &
-SimpleTokenizer::get_tokens() const
-{
-  return _tokens;
-}

plugins/header_rewrite/parser.h

@@ -33,7 +33,7 @@
 class Parser
 {
 public:
-  explicit Parser(const std::string &line);
+  Parser(){};
 
   // noncopyable
   Parser(const Parser &) = delete;
@@ -76,13 +76,20 @@ class Parser
   }
 
   bool cond_is_hook(TSHttpHookID &hook) const;
-  const std::vector<std::string> &get_tokens() const;
+
+  const std::vector<std::string> &
+  get_tokens() const
+  {
+    return _tokens;
+  }
+
+  bool parse_line(const std::string &original_line);
 
 private:
-  void preprocess(std::vector<std::string> tokens);
+  bool preprocess(std::vector<std::string> tokens);
 
-  bool _cond;
-  bool _empty;
+  bool _cond  = false;
+  bool _empty = false;
   std::vector<std::string> _mods;
   std::string _op;
   std::string _arg;
@@ -101,7 +108,11 @@ class SimpleTokenizer
   SimpleTokenizer(const SimpleTokenizer &) = delete;
   void operator=(const SimpleTokenizer &) = delete;
 
-  const std::vector<std::string> &get_tokens() const;
+  const std::vector<std::string> &
+  get_tokens() const
+  {
+    return _tokens;
+  }
 
 protected:
   std::vector<std::string> _tokens;

plugins/header_rewrite/ruleset.cc

@@ -76,8 +76,7 @@ RuleSet::add_operator(Parser &p, const char *filename, int lineno)
   Operator *o = operator_factory(p.get_op());
 
   if (nullptr != o) {
-    // TODO: This should be extended to show both the "argument" and the "value" (if both are used)
-    TSDebug(PLUGIN_NAME, "   Adding operator: %s(%s)", p.get_op().c_str(), p.get_arg().c_str());
+    TSDebug(PLUGIN_NAME, "   Adding operator: %s(%s)=\"%s\"", p.get_op().c_str(), p.get_arg().c_str(), p.get_value().c_str());
     o->initialize(p);
     if (!o->set_hook(_hook)) {
       delete o;

plugins/header_rewrite/value.cc

@@ -54,9 +54,13 @@ Value::set_value(const std::string &val)
         std::string cond_token = token.substr(2, token.size() - 3);
 
         if ((tcond_val = condition_factory(cond_token))) {
-          Parser parser(_value);
+          Parser parser;
 
-          tcond_val->initialize(parser);
+          if (parser.parse_line(_value)) {
+            tcond_val->initialize(parser);
+          } else {
+            // TODO: should we produce error here?
+          }
         }
       } else {
         tcond_val = new ConditionStringLiteral(token);