Better error handling when loading configurations

Author: zimmerle

CHANGES

@@ -1,7 +1,10 @@
 v3.x.y - YYYY-MMM-DD (to be released)
--------------------------------------
-
-   - IMPORTANT: SecDefaultAction behaves changing: SecDefaultAction specified
+------------------------------------- 
+ 
+  - 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.
     [@zimmerle]

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,19 +67,21 @@ 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,
         const std::string &msg);
 
     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; }
 
- private:
-    Rules m_rulesAtPhase[8];
+    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:
+    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,23 +403,23 @@ 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;
             }
         }
 
         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;