Better error handling when loading configurations

CHANGES

@@ -1,6 +1,9 @@
 v3.x.y - YYYY-MMM-DD (to be released)
 ------------------------------------- 
  
+  - Added the basics for supporting better error/warning handling while
+    loading configurations.
+    [@zimmerle]
   - IMPORTANT: SecDefaultAction behaves changing: SecDefaultAction specified
     on a child configuration will overwrite the ones specified on the parent;
     Previously it was concatenating.

headers/modsecurity/modsecurity.h

@@ -78,6 +78,7 @@
 #include <iostream>
 #include <string>
 #include <memory>
+#include <vector>
 #endif
 
 
@@ -95,6 +96,10 @@ namespace modsecurity {
      */
     using ModSecString = std::string;
 
+    using RulesErrors = std::vector<std::unique_ptr<std::string>>;
+    using RulesWarnings = std::vector<std::unique_ptr<std::string>>;
+
+
     using RuleId = int64_t;
 
     /**

headers/modsecurity/rules.h

@@ -40,33 +40,35 @@ class Transformation;
 }
 }
 
+
 class Rules {
  public:
-    void dump() const;
+    using container=std::vector<std::shared_ptr<Rule>>;
+    using iterator=typename container::iterator;
+    using const_iterator=typename container::const_iterator;
 
-    int append(Rules *from,
-        const std::vector<RuleId> &ids,
-        std::ostringstream *err);
+    int append(Rules *from);
 
     bool insert(const std::shared_ptr<Rule> &rule);
 
-    bool insert(std::shared_ptr<Rule> rule,
-        const std::vector<RuleId> *ids,
-        std::ostringstream *err);
-
     size_t size() const;
 
     std::shared_ptr<Rule> operator[](int index) const;
     std::shared_ptr<Rule> at(int index) const;
 
-    void fixDefaultActions();
+    void fixDefaultActions(RulesWarnings *warnings, RulesErrors *errors);
 
     std::vector<std::shared_ptr<actions::Action> > m_defaultActions;
     std::vector<std::shared_ptr<actions::transformations::Transformation> > m_defaultTransformations;
 
-    std::vector<std::shared_ptr<Rule> > m_rules;
     void dump();
 
+    inline iterator begin() noexcept { return m_rules.begin(); }
+    inline const_iterator cbegin() const noexcept { return m_rules.cbegin(); }
+    inline iterator end() noexcept { return m_rules.end(); }
+    inline const_iterator cend() const noexcept { return m_rules.cend(); }
+ private:
+     container m_rules;
 };
 
 

headers/modsecurity/rules_set.h

@@ -22,6 +22,7 @@
 #include <string>
 #include <vector>
 #include <list>
+#include <memory>
 #endif
 
 
@@ -42,8 +43,6 @@ namespace Parser {
 class Driver;
 }
 
-
-
 /** @ingroup ModSecurity_CPP_API */
 class RulesSet : public RulesSetProperties {
  public:
@@ -68,12 +67,13 @@ class RulesSet : public RulesSetProperties {
     int load(const char *rules);
     int load(const char *rules, const std::string &ref);
 
-    void dump() const;
+    void dump();
 
     int merge(Parser::Driver *driver);
     int merge(RulesSet *rules);
 
     int evaluate(int phase, Transaction *transaction);
+
     std::string getParserError();
 
     void debug(int level, const std::string &id, const std::string &uri,
@@ -81,6 +81,7 @@ class RulesSet : public RulesSetProperties {
 
     RulesSetPhases m_rulesSetPhases;
  private:
+    bool containsDuplicatedIds(RulesWarnings *warnings, RulesErrors *errors);
 #ifndef NO_LOGS
     uint8_t m_secmarker_skipped;
 #endif

headers/modsecurity/rules_set_phases.h

@@ -23,6 +23,7 @@
 #include <string>
 #include <vector>
 #include <list>
+#include <array>
 #endif
 
 
@@ -42,18 +43,33 @@ class Driver;
 /** @ingroup ModSecurity_CPP_API */
 class RulesSetPhases {
  public:
+    using container = std::array<Rules, modsecurity::Phases::NUMBER_OF_PHASES>;
+    using iterator = typename container::iterator;
+    using const_iterator = typename container::const_iterator;
 
-    bool insert(std::shared_ptr<Rule> rule);
+    void insert(std::shared_ptr<Rule> rule);
+    void append(RulesSetPhases *from);
 
     int append(RulesSetPhases *from, std::ostringstream *err);
-    void dump() const;
+    void dump();
 
     Rules *operator[](int index);
     Rules *at(int index);
+    static size_t size() { return modsecurity::Phases::NUMBER_OF_PHASES; }
+
+    void fixDefaultActions(RulesWarnings *warnings, RulesErrors *errors) {
+        for (auto &phase : m_rulesAtPhase) {
+            phase.fixDefaultActions(warnings, errors);
+        }
+    }
+
+    inline iterator begin() noexcept { return m_rulesAtPhase.begin(); }
+    inline const_iterator cbegin() const noexcept { return m_rulesAtPhase.cbegin(); }
+    inline iterator end() noexcept { return m_rulesAtPhase.end(); }
+    inline const_iterator cend() const noexcept { return m_rulesAtPhase.cend(); }
 
  private:
-    Rules m_rulesAtPhase[8];
-
+    container m_rulesAtPhase;
 };
 
 

headers/modsecurity/rules_set_properties.h

@@ -330,7 +330,9 @@ class RulesSetProperties {
 
 
     static int mergeProperties(RulesSetProperties *from,
-        RulesSetProperties *to, std::ostringstream *err) {
+        RulesSetProperties *to,
+        RulesWarnings *warning,
+        RulesErrors *error) {
 
         merge_ruleengine_value(to->m_secRuleEngine, from->m_secRuleEngine,
                                PropertyNotSetRuleEngine);
@@ -401,10 +403,10 @@ class RulesSetProperties {
         }
 
         if (to->m_auditLog) {
-            std::string error;
-            to->m_auditLog->merge(from->m_auditLog, &error);
-            if (error.size() > 0) {
-                *err << error;
+            std::string error_;
+            to->m_auditLog->merge(from->m_auditLog, &error_);
+            if (error_.size() > 0) {
+                    error->push_back(std::unique_ptr<std::string>(new std::string(error_)));
                 return -1;
             }
         }
@@ -412,12 +414,12 @@ class RulesSetProperties {
         if (from->m_debugLog && to->m_debugLog &&
             from->m_debugLog->isLogFileSet()) {
             if (to->m_debugLog->isLogFileSet() == false) {
-                std::string error;
+                std::string error_;
                 to->m_debugLog->setDebugLogFile(
                     from->m_debugLog->getDebugLogFile(),
-                    &error);
-                if (error.size() > 0) {
-                    *err << error;
+                    &error_);
+                if (error_.size() > 0) {
+                    error->push_back(std::unique_ptr<std::string>(new std::string(error_)));
                     return -1;
                 }
             }

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();
     }
 }
 

test/test-cases/regression/config-include-bad.json

@@ -43,7 +43,7 @@
     "version_min":300000,
     "title":"Include - duplicate id",
     "expected":{
-      "parser_error": "Rule id: 40 is duplicated"
+      "parser_error": "There are multiple rules defined with same id. The ID 40 is defined at"
     },
     "rules":[
       "SecRuleEngine On",