Better error handling when loading configurations

src/parser/driver.cc

@@ -176,18 +176,6 @@ int Driver::addSecRule(std::unique_ptr<RuleWithActions> r) {
         return false;
     }
 
-    for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
-        Rules *rules = m_rulesSetPhases[i];
-        for (int j = 0; j < rules->size(); j++) {
-            RuleWithOperator *lr = dynamic_cast<RuleWithOperator *>(rules->at(j).get());
-            if (lr && lr->getId() == rule->getId()) {
-                m_parserError << "Rule id: " << std::to_string(rule->getId()) \
-                    << " is duplicated" << std::endl;
-                return false;
-            }
-        }
-    }
-
     m_lastRule = rule.get();
 
     m_rulesSetPhases.insert(rule);

src/rules.cc

@@ -20,38 +20,23 @@
 namespace modsecurity {
 
 
-int Rules::append(Rules *from, const std::vector<RuleId> &ids, std::ostringstream *err) {
-    size_t j = 0;
-    for (; j < from->size(); j++) {
-        RuleWithActions *rule = dynamic_cast<RuleWithActions*>(from->at(j).get());
-        if (rule && std::binary_search(ids.begin(), ids.end(), rule->getId())) {
-            if (err != NULL) {
-                *err << "Rule id: " << std::to_string(rule->getId()) \
-                     << " is duplicated" << std::endl;
-            }
-            return -1;
+int Rules::append(Rules *from) {
+    m_rules.insert(m_rules.end(), from->m_rules.begin(), from->m_rules.end());
+    if (!from->m_defaultActions.empty() || !from->m_defaultTransformations.empty()) {
+        m_defaultActions.clear();
+        m_defaultTransformations.clear();
+        for (auto &a : from->m_defaultActions) {
+            m_defaultActions.push_back(a);
+        }
+        for (auto &a : from->m_defaultTransformations) {
+            m_defaultTransformations.push_back(a);
         }
     }
-    m_rules.insert(m_rules.end(), from->m_rules.begin(), from->m_rules.end());
-    return j;
+    return from->size();
 }
 
 
 bool Rules::insert(const std::shared_ptr<Rule> &rule) {
-    return insert(rule, nullptr, nullptr);
-}
-
-
-bool Rules::insert(std::shared_ptr<Rule> rule, const std::vector<RuleId> *ids, std::ostringstream *err) {
-    RuleWithActions*r = dynamic_cast<RuleWithActions*>(rule.get());
-    if (r && ids != nullptr
-        && std::binary_search(ids->begin(), ids->end(), r->getId())) {
-        if (err != NULL) {
-            *err << "Rule id: " << std::to_string(r->getId()) \
-                << " is duplicated" << std::endl;
-        }
-        return false;
-    }
     m_rules.push_back(rule);
     return true;
 }
@@ -72,7 +57,7 @@ std::shared_ptr<Rule> Rules::at(int index) const {
 }
 
 
-void Rules::dump() const {
+void Rules::dump() {
     for (int j = 0; j < m_rules.size(); j++) {
         std::cout << "    Rule ID: " << m_rules.at(j)->getReference();
         std::cout << "--" << m_rules.at(j) << std::endl;

src/rules_set.cc

@@ -18,6 +18,7 @@
 #include <fstream>
 #include <string>
 #include <vector>
+#include <memory>
 
 #include "modsecurity/rules_set.h"
 #include "src/rule_marker.h"
@@ -32,7 +33,7 @@ using modsecurity::Utils::HttpsClient;
 
 namespace modsecurity {
 
-    void Rules::fixDefaultActions() {
+    void Rules::fixDefaultActions(RulesWarnings *warnings, RulesErrors *errors) {
         for (size_t i = 0; i < m_rules.size(); i++) {
             auto &rule = m_rules[i];
 
@@ -142,6 +143,52 @@ int RulesSet::load(const char *plainRules) {
 }
 
 
+bool RulesSet::containsDuplicatedIds(RulesWarnings *warning, RulesErrors *error) {
+    std::multimap<RuleId, Rule *> allIds;
+    std::set<RuleId> duplicatedIds;
+    for (auto &rules : m_rulesSetPhases) {
+        for (auto &j : rules) {
+            RuleWithActions *rule = dynamic_cast<RuleWithActions *>(j.get());
+            if (rule) {
+                allIds.insert(std::pair<RuleId, Rule *>(rule->getId(), rule));
+            }
+        }
+    }
+
+    auto id = allIds.begin();
+    auto next = id;
+    if (id != allIds.end()) {
+        next++;
+    }
+    while (id != allIds.end() && next != allIds.end()) {
+        if (id->first == next->first) {
+            duplicatedIds.insert(id->first);
+        }
+        id++;
+        next++;
+    }
+
+    for (auto i : duplicatedIds) {
+        auto ret = allIds.equal_range(i);
+        std::stringstream ss;
+
+        ss << "There are multiple rules defined with ";
+        ss << "same id. The ID " << i << " is defined at: " << std::endl;
+        for (auto it = ret.first; it != ret.second; ++it) {
+            auto rule = it->second;
+            ss << "  " << *rule->getFileName() << ":";
+            ss << rule->getLineNumber() << std::endl;
+        }
+
+        std::unique_ptr<std::string> e(new std::string(ss.str()));
+        error->push_back(std::move(e));
+    }
+
+    return !duplicatedIds.empty();
+}
+
+
+
 std::string RulesSet::getParserError() {
     return this->m_parserError.str();
 }
@@ -265,13 +312,24 @@ int RulesSet::evaluate(int phase, Transaction *t) {
 
 int RulesSet::merge(Driver *from) {
     int amount_of_rules = 0;
+    RulesErrors errors;
+    RulesWarnings warnings;
 
-    amount_of_rules = m_rulesSetPhases.append(&from->m_rulesSetPhases,
-        &m_parserError);
+    m_rulesSetPhases.append(&from->m_rulesSetPhases);
     mergeProperties(
         dynamic_cast<RulesSetProperties *>(from),
         dynamic_cast<RulesSetProperties *>(this),
-        &m_parserError);
+        &warnings, &errors);
+
+    m_rulesSetPhases.fixDefaultActions(&warnings, &errors);
+    containsDuplicatedIds(&warnings, &errors);
+
+    if (!errors.empty()) {
+        for (auto &i : errors) {
+            m_parserError << "*** Error: " << *i << std::endl;
+        }
+        return -1;
+    }
 
     return amount_of_rules;
 }
@@ -279,13 +337,24 @@ int RulesSet::merge(Driver *from) {
 
 int RulesSet::merge(RulesSet *from) {
     int amount_of_rules = 0;
+    RulesErrors errors;
+    RulesWarnings warnings;
 
-    amount_of_rules = m_rulesSetPhases.append(&from->m_rulesSetPhases,
-        &m_parserError);
+    m_rulesSetPhases.append(&from->m_rulesSetPhases);
     mergeProperties(
         dynamic_cast<RulesSetProperties *>(from),
         dynamic_cast<RulesSetProperties *>(this),
-        &m_parserError);
+        &warnings, &errors);
+
+    m_rulesSetPhases.fixDefaultActions(&warnings, &errors);
+    containsDuplicatedIds(&warnings, &errors);
+
+    if (!errors.empty()) {
+        for (auto &i : errors) {
+            m_parserError << "*** Error: " << *i << std::endl;
+        }
+        return -1;
+    }
 
     return amount_of_rules;
 }
@@ -299,7 +368,7 @@ void RulesSet::debug(int level, const std::string &id,
 }
 
 
-void RulesSet::dump() const {
+void RulesSet::dump() {
     m_rulesSetPhases.dump();
 }
 

src/rules_set_phases.cc

@@ -15,78 +15,34 @@
 
 #include "modsecurity/rules_set_phases.h"
 #include "src/rule_with_operator.h"
-
+#include <iterator>
 
 namespace modsecurity {
 
 
-bool RulesSetPhases::insert(std::shared_ptr<Rule> rule) {
-    if (rule->getPhase() >= modsecurity::Phases::NUMBER_OF_PHASES) {
-        return false;
+void RulesSetPhases::insert(std::shared_ptr<Rule> rule) {
+    if (rule->getPhase() >= size()) {
+        return;
     }
     m_rulesAtPhase[rule->getPhase()].insert(rule);
-
-    return true;
 }
 
 
-int RulesSetPhases::append(RulesSetPhases *from, std::ostringstream *err) {
-    int amount_of_rules = 0;
-    std::vector<int64_t> v;
-
-    for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
-        v.reserve(m_rulesAtPhase[i].size());
-        for (size_t z = 0; z < m_rulesAtPhase[i].size(); z++) {
-            RuleWithOperator *rule_ckc = dynamic_cast<RuleWithOperator *>(m_rulesAtPhase[i].at(z).get());
-            //RuleWithOperator *rule_ckc = dynamic_cast<RuleWithOperator *>(m_rulesAtPhase->at(i).get());
-            if (!rule_ckc) {
-                continue;
-            }
-            v.push_back(rule_ckc->getId());
-        }
+void RulesSetPhases::append(RulesSetPhases *from) {
+    int phase = 0;
+    for (auto &a : *from) {
+         m_rulesAtPhase[phase++].append(&a);
     }
-    std::sort (v.begin(), v.end());
-
-    for (int phase = 0; phase < modsecurity::Phases::NUMBER_OF_PHASES; phase++) {
-        int res = m_rulesAtPhase[phase].append(from->at(phase), v, err);
-        if (res < 0) {
-            return res;
-        }
-        amount_of_rules = amount_of_rules + res;
-
-        /**
-         * An action set in a child will overwrite an action set on a parent.
-         *
-         */
-        std::vector<std::shared_ptr<actions::Action> > *actions_to = &at(phase)->m_defaultActions;
-        std::vector<std::shared_ptr<actions::transformations::Transformation> > *actions_t_to = &at(phase)->m_defaultTransformations;
-        if (actions_to->size() == 0 || actions_t_to->size() == 0) {
-            std::vector<std::shared_ptr<actions::Action> > *actions_from = &from->at(phase)->m_defaultActions;
-
-            actions_to->clear();
-            for (size_t j = 0; j < actions_from->size(); j++) {
-                actions_to->push_back(actions_from->at(j));
-            }
-
-            std::vector<std::shared_ptr<actions::transformations::Transformation> > *actions_t_from = &from->at(phase)->m_defaultTransformations;
-            actions_t_to->clear();
-            for (size_t j = 0; j < actions_t_from->size(); j++) {
-                actions_t_to->push_back(actions_t_from->at(j));
-            }
-            at(phase)->fixDefaultActions();
-        }
-    }
-
-    return amount_of_rules;
 }
 
-void RulesSetPhases::dump() const {
-    for (int i = 0; i <= modsecurity::Phases::NUMBER_OF_PHASES; i++) {
-        const Rules *rules = &m_rulesAtPhase[i];
-        std::cout << "Phase: " << std::to_string(i);
-        std::cout << " (" << std::to_string(m_rulesAtPhase[i].size());
+
+void RulesSetPhases::dump() {
+    int phase = 0;
+    for (auto &rules : m_rulesAtPhase) {
+        std::cout << "Phase: " << std::to_string(phase++);
+        std::cout << " (" << std::to_string(rules.size());
         std::cout << " rules)" << std::endl;
-        m_rulesAtPhase[i].dump();
+        rules.dump();
     }
 }