From b4659959cd2fe9fa38cf95e4065daf06c24a3c59 Mon Sep 17 00:00:00 2001
From: gberkes <gabor@berkes.me>
Date: Sun, 4 Aug 2024 22:04:07 +0200
Subject: [PATCH] Refactor: Ensure safe error handling by removing isolated
 throw; statements.

- SonarCloud analysis identified standalone `throw;` calls without accompanying `try-catch` blocks, used inconsistently as placeholders or for premature termination under specific conditions.
- Removed these `throw;` instances to prevent potential runtime issues in future development phases, where such configurations might inadvertently be created.
- Introduced `assert` statements as a more appropriate mechanism for asserting preconditions in the affected class member functions, ensuring clearer intent and safer code behavior during development.
- Refactor action_kind processing to use switch() instead of if-else chains; add assertion in default case.
- Fix SonarCloud issue: Make this variable a const reference.
https://sonarcloud.io/project/issues?resolved=false&pullRequest=3104&id=owasp-modsecurity_ModSecurity&open=AY8Vpgy4f6U6E7VKL4Cn
---
 headers/modsecurity/transaction.h |  8 ++-
 src/rule_with_actions.cc          | 83 +++++++++++++++++--------------
 2 files changed, 48 insertions(+), 43 deletions(-)

diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h
index 9caace2c..e35ed6eb 100644
--- a/headers/modsecurity/transaction.h
+++ b/headers/modsecurity/transaction.h
@@ -14,6 +14,7 @@
  */
 
 #ifdef __cplusplus
+#include <cassert>
 #include <ctime>
 #include <fstream>
 #include <iomanip>
@@ -307,11 +308,8 @@ class TransactionSecMarkerManagement {
     }
 
     std::shared_ptr<std::string> getCurrentMarker() const {
-        if (m_marker) {
-            return m_marker;
-        } else {
-            throw; // cppcheck-suppress rethrowNoCurrentException
-        }
+        assert((m_marker != nullptr) && "You might have forgotten to call and evaluate isInsideAMarker() before calling getCurrentMarker().");
+        return m_marker;
     }
 
     void removeMarker() {
diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc
index 5ae7d407..7314b5c8 100644
--- a/src/rule_with_actions.cc
+++ b/src/rule_with_actions.cc
@@ -17,6 +17,7 @@
 
 #include <stdio.h>
 
+#include <cassert>
 #include <algorithm>
 #include <iostream>
 #include <string>
@@ -86,45 +87,51 @@ RuleWithActions::RuleWithActions(
 
     if (actions) {
         for (Action *a : *actions) {
-            if (a->action_kind == Action::ConfigurationKind) {
-                a->evaluate(this, NULL);
-                delete a;
-
-            } else if (a->action_kind == Action::RunTimeOnlyIfMatchKind) {
-                if (dynamic_cast<actions::Capture *>(a)) {
-                    m_containsCaptureAction = true;
+            switch (a->action_kind) {
+                case Action::ConfigurationKind:
+                    a->evaluate(this, NULL);
                     delete a;
-                } else if (dynamic_cast<actions::MultiMatch *>(a)) {
-                    m_containsMultiMatchAction = true;
-                    delete a;
-                } else if (dynamic_cast<actions::Severity *>(a)) {
-                    m_severity = dynamic_cast<actions::Severity *>(a);
-                } else if (dynamic_cast<actions::LogData *>(a)) {
-                    m_logData = dynamic_cast<actions::LogData*>(a);
-                } else if (dynamic_cast<actions::Msg *>(a)) {
-                    m_msg = dynamic_cast<actions::Msg*>(a);
-                } else if (dynamic_cast<actions::SetVar *>(a)) {
-                    m_actionsSetVar.push_back(
-                        dynamic_cast<actions::SetVar *>(a));
-                } else if (dynamic_cast<actions::Tag *>(a)) {
-                    m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
-                } else if (dynamic_cast<actions::Block *>(a)) {
-                    m_actionsRuntimePos.push_back(a);
-                    m_containsStaticBlockAction = true;
-                } else if (a->isDisruptive() == true) {
-                    if (m_disruptiveAction != nullptr) {
-                        delete m_disruptiveAction;
-                        m_disruptiveAction = nullptr;
+                    break;
+                case Action::RunTimeOnlyIfMatchKind:
+                    if (dynamic_cast<actions::Capture *>(a)) {
+                        m_containsCaptureAction = true;
+                        delete a;
+                    } else if (dynamic_cast<actions::MultiMatch *>(a)) {
+                        m_containsMultiMatchAction = true;
+                        delete a;
+                    } else if (dynamic_cast<actions::Severity *>(a)) {
+                        m_severity = dynamic_cast<actions::Severity *>(a);
+                    } else if (dynamic_cast<actions::LogData *>(a)) {
+                        m_logData = dynamic_cast<actions::LogData*>(a);
+                    } else if (dynamic_cast<actions::Msg *>(a)) {
+                        m_msg = dynamic_cast<actions::Msg*>(a);
+                    } else if (dynamic_cast<actions::SetVar *>(a)) {
+                        m_actionsSetVar.push_back(
+                            dynamic_cast<actions::SetVar *>(a));
+                    } else if (dynamic_cast<actions::Tag *>(a)) {
+                        m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
+                    } else if (dynamic_cast<actions::Block *>(a)) {
+                        m_actionsRuntimePos.push_back(a);
+                        m_containsStaticBlockAction = true;
+                    } else if (a->isDisruptive() == true) {
+                        if (m_disruptiveAction != nullptr) {
+                            delete m_disruptiveAction;
+                            m_disruptiveAction = nullptr;
+                        }
+                        m_disruptiveAction = a;
+                    } else {
+                        m_actionsRuntimePos.push_back(a);
                     }
-                    m_disruptiveAction = a;
-                } else {
-                    m_actionsRuntimePos.push_back(a);
-                }
-            } else {
-                delete a;
-                std::cout << "General failure, action: " << a->m_name;
-                std::cout << " has an unknown type." << std::endl;
-                throw; // cppcheck-suppress rethrowNoCurrentException
+                    break;
+                default:
+                    std::cout << "General failure, action: " << a->m_name;
+                    std::cout << " has an unknown type." << std::endl;
+                    delete a;
+                    #ifdef NDEBUG
+                    break;
+                    #else
+                    assert(false);
+                    #endif
             }
         }
         delete actions;
@@ -239,7 +246,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
     bool containsBlock, std::shared_ptr<RuleMessage> ruleMessage) {
     bool disruptiveAlreadyExecuted = false;
 
-    for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer
+    for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer
         if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) {
             continue;
         }