From 21d330530f80ba94da6b58aa91816ac54074ce34 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Mon, 29 Sep 2025 11:39:49 -0600 Subject: [PATCH] HRW: Bug where after an elif an else may be required This also fixes a small bug in run-plugin --- plugins/header_rewrite/conditions.h | 6 +++ plugins/header_rewrite/header_rewrite.cc | 62 ++++++++++++++++++------ plugins/header_rewrite/operators.cc | 2 +- plugins/header_rewrite/parser.cc | 4 +- 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/plugins/header_rewrite/conditions.h b/plugins/header_rewrite/conditions.h index 10144a51fee..28e888d4ed6 100644 --- a/plugins/header_rewrite/conditions.h +++ b/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; diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc index c3545dd8e2b..b047fe0bcbf 100644 --- a/plugins/header_rewrite/header_rewrite.cc +++ b/plugins/header_rewrite/header_rewrite.cc @@ -137,6 +137,39 @@ RulesConfig::add_rule(std::unique_ptr 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 diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index cb7ac53c12f..42c00027c63 100644 --- a/plugins/header_rewrite/operators.cc +++ b/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(tokens[i].c_str()); } diff --git a/plugins/header_rewrite/parser.cc b/plugins/header_rewrite/parser.cc index 970da3a7258..a0f93afb774 100644 --- a/plugins/header_rewrite/parser.cc +++ b/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 tokens) { @@ -178,6 +178,7 @@ Parser::preprocess(std::vector 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 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; }