HRW: Bug where after an elif an else may be required (#12536)

This also fixes a small bug in run-plugin

Author: zwoop

plugins/header_rewrite/conditions.h

@@ -786,6 +786,12 @@ class ConditionGroup : public Condition
     }
   }
 
+  bool
+  has_conditions() const
+  {
+    return _cond != nullptr;
+  }
+
 private:
   Condition *_cond = nullptr; // First pre-condition (linked list)
   bool       _end  = false;

plugins/header_rewrite/header_rewrite.cc

@@ -137,6 +137,39 @@ RulesConfig::add_rule(std::unique_ptr<RuleSet> rule)
   }
 }
 
+// Helper function to validate rule completion
+static bool
+validate_rule_completion(RuleSet *rule, const std::string &fname, int lineno)
+{
+  // Early return if rule has operators - no validation errors possible
+  if (!rule || rule->has_operator()) {
+    return true;
+  }
+
+  switch (rule->get_clause()) {
+  case Parser::CondClause::ELIF:
+    if (rule->cur_section()->group.has_conditions()) {
+      TSError("[%s] ELIF conditions without operators are not allowed in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
+      return false;
+    }
+    break;
+
+  case Parser::CondClause::ELSE:
+    TSError("[%s] conditions not allowed in ELSE clause in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
+    return false;
+
+  case Parser::CondClause::OPER:
+    TSError("[%s] conditions without operators are not allowed in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
+    return false;
+
+  case Parser::CondClause::COND:
+    // COND clause without operators - potentially valid in some cases
+    break;
+  }
+
+  return true;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // Config parser, use to parse both the global, and per-remap, configurations.
 //
@@ -220,21 +253,14 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
     }
 
     // If we are at the beginning of a new condition, save away the previous rule (but only if it has operators).
-    if (p.is_cond() && rule) {
-      bool transfer    = rule->cur_section()->has_operator();
-      auto rule_clause = rule->get_clause();
+    if (p.is_cond() && rule && is_hook) {
+      // Only validate and save when starting a NEW rule (hook condition)
+      bool transfer = rule->cur_section()->has_operator();
 
-      if (rule_clause == Parser::CondClause::ELIF) {
-        if (is_hook) {
-          TSError("[%s] ELIF without operators are not allowed in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
-          return false;
-        }
-      } else if (rule_clause == Parser::CondClause::ELSE) {
-        if (!transfer) {
-          TSError("[%s] conditions not allowed in ELSE clause in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
-          return false;
-        }
+      if (!validate_rule_completion(rule.get(), fname, lineno)) {
+        return false;
       }
+
       if (transfer) {
         add_rule(std::move(rule));
       }
@@ -327,8 +353,14 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
   }
 
   // Add the last rule (possibly the only rule)
-  if (rule && rule->has_operator()) {
-    add_rule(std::move(rule));
+  if (rule) {
+    if (!validate_rule_completion(rule.get(), fname, lineno)) {
+      return false;
+    }
+
+    if (rule->has_operator()) {
+      add_rule(std::move(rule));
+    }
   }
 
   // Collect all resource IDs that we need

plugins/header_rewrite/operators.cc

@@ -1289,7 +1289,7 @@ OperatorRunPlugin::initialize(Parser &p)
   argv[0] = p.from_url();
   argv[1] = p.to_url();
 
-  for (int i = 0; i < argc; ++i) {
+  for (size_t i = 0; i < tokens.size(); ++i) {
     argv[i + 2] = const_cast<char *>(tokens[i].c_str());
   }
 

plugins/header_rewrite/parser.cc

@@ -164,7 +164,7 @@ Parser::parse_line(const std::string &original_line)
 }
 
 // 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.).
+// therefore, we pass in a copy of the parser's tokens here, such that the original token list is retained.
 bool
 Parser::preprocess(std::vector<std::string> tokens)
 {
@@ -178,6 +178,7 @@ Parser::preprocess(std::vector<std::string> tokens)
         if (m.find_first_of(',') != std::string::npos) {
           std::istringstream iss(m);
           std::string        t;
+
           while (getline(iss, t, ',')) {
             _mods.push_back(t);
           }
@@ -186,7 +187,6 @@ Parser::preprocess(std::vector<std::string> tokens)
         }
         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;
       }