Fix collection cache ephemerality

CMakeLists.txt

@@ -76,7 +76,6 @@ set(LIBDDWAF_SOURCE
     ${libddwaf_SOURCE_DIR}/src/object_store.cpp
     ${libddwaf_SOURCE_DIR}/src/collection.cpp
     ${libddwaf_SOURCE_DIR}/src/expression.cpp
-    ${libddwaf_SOURCE_DIR}/src/rule.cpp
     ${libddwaf_SOURCE_DIR}/src/ruleset_info.cpp
     ${libddwaf_SOURCE_DIR}/src/ip_utils.cpp
     ${libddwaf_SOURCE_DIR}/src/processor.cpp

src/collection.cpp

@@ -83,16 +83,23 @@ void base_collection<Derived>::match(memory::vector<event> &events, const object
     ddwaf::timer &deadline) const
 {
     if (cache.result >= Derived::type()) {
-        return;
+        // If the result was cached but ephemeral, clear it. Note that this is
+        // just a workaround taking advantage of the order of evaluation of
+        // collections. Collections might be removed in the future altogether.
+        if (cache.result == Derived::type() && cache.ephemeral) {
+            cache.result = collection_type::none;
+            cache.ephemeral = false;
+        } else {
+            return;
+        }
     }
 
     for (auto *rule : rules_) {
         auto event = match_rule(rule, store, cache.rule_cache, rules_to_exclude, objects_to_exclude,
             dynamic_matchers, deadline);
         if (event.has_value()) {
-            if (!event->ephemeral) {
-                cache.result = Derived::type();
-            }
+            cache.result = Derived::type();
+            cache.ephemeral = event->ephemeral;
 
             events.emplace_back(std::move(*event));
             DDWAF_DEBUG("Found event on rule %s", rule->get_id().c_str());

src/collection.hpp

@@ -26,11 +26,11 @@ enum class collection_type : uint8_t { none = 0, regular = 1, priority = 2 };
 // how they interact with the cache, e.g. a priority collection will try to match even if there has
 // already been a match in a regular collection.
 
-// The collection cache is shared by both priority and regular collections,
-// this ensures that regular collections for which there is an equivalent
-// priority collection of the same type, aren't processed when the respective
-// priority collection has already had a match.
+// The collection cache is shared by both priority and regular collections, this ensures that
+// regular collections for which there is an equivalent priority collection of the same type,
+// aren't processed when the respective priority collection has already had a match.
 struct collection_cache {
+    bool ephemeral{false};
     collection_type result{collection_type::none};
     memory::unordered_map<rule *, rule::cache_type> rule_cache;
 };
@@ -49,8 +49,7 @@ template <typename Derived> class base_collection {
 
     void insert(const std::shared_ptr<rule> &rule) { rules_.emplace_back(rule.get()); }
 
-    void match(memory::vector<event> &events /* output */, const object_store &store,
-        collection_cache &cache,
+    void match(memory::vector<event> &events, const object_store &store, collection_cache &cache,
         const memory::unordered_map<ddwaf::rule *, exclusion::filter_mode> &rules_to_exclude,
         const memory::unordered_map<ddwaf::rule *, exclusion::object_set> &objects_to_exclude,
         const std::unordered_map<std::string, std::shared_ptr<matcher::base>> &dynamic_matchers,

src/expression.cpp

@@ -118,7 +118,6 @@ const matcher::base *expression::evaluator::get_matcher(const condition &cond) c
     return it->second.get();
 }
 
-// NOLINTNEXTLINE(misc-no-recursion)
 expression::eval_result expression::evaluator::eval_condition(
     const condition &cond, condition::cache_type &cache)
 {
@@ -183,7 +182,7 @@ expression::eval_result expression::evaluator::eval()
             continue;
         }
 
-        auto [res, ephemeral] = eval_condition(*cond, cond_cache);
+        auto [res, ephemeral] = eval_condition(cond, cond_cache);
         if (!res) {
             return {false, false};
         }
@@ -224,7 +223,7 @@ void expression_builder::add_target(std::string name, std::vector<std::string> k
     target.source = source;
 
     auto &cond = conditions_.back();
-    cond->targets.emplace_back(std::move(target));
+    cond.targets.emplace_back(std::move(target));
 }
 
 } // namespace ddwaf

src/expression.hpp

@@ -77,7 +77,7 @@ class expression {
 
         ddwaf::timer &deadline;
         const ddwaf::object_limits &limits;
-        const std::vector<std::unique_ptr<condition>> &conditions;
+        const std::vector<condition> &conditions;
         const object_store &store;
         const exclusion::object_set &objects_excluded;
         const std::unordered_map<std::string, std::shared_ptr<matcher::base>> &dynamic_matchers;
@@ -86,8 +86,7 @@ class expression {
 
     expression() = default;
 
-    explicit expression(
-        std::vector<std::unique_ptr<condition>> &&conditions, ddwaf::object_limits limits = {})
+    explicit expression(std::vector<condition> &&conditions, ddwaf::object_limits limits = {})
         : limits_(limits), conditions_(std::move(conditions))
     {}
 
@@ -99,9 +98,7 @@ class expression {
     void get_addresses(std::unordered_map<target_index, std::string> &addresses) const
     {
         for (const auto &cond : conditions_) {
-            for (const auto &target : cond->targets) {
-                addresses.emplace(target.root, target.name);
-            }
+            for (const auto &target : cond.targets) { addresses.emplace(target.root, target.name); }
         }
     }
 
@@ -125,7 +122,7 @@ class expression {
 
 protected:
     ddwaf::object_limits limits_;
-    std::vector<std::unique_ptr<condition>> conditions_;
+    std::vector<condition> conditions_;
 };
 
 class expression_builder {
@@ -136,37 +133,37 @@ class expression_builder {
         conditions_.reserve(num_conditions);
     }
 
-    void start_condition() { conditions_.emplace_back(std::make_unique<expression::condition>()); }
+    void start_condition() { conditions_.emplace_back(expression::condition{}); }
     template <typename T, typename... Args> void start_condition(Args... args)
     {
-        auto cond = std::make_unique<expression::condition>();
-        cond->matcher = std::make_unique<T>(std::forward<Args>(args)...);
+        expression::condition cond;
+        cond.matcher = std::make_unique<T>(std::forward<Args>(args)...);
         conditions_.emplace_back(std::move(cond));
     }
 
     void start_condition(std::string data_id)
     {
-        auto cond = std::make_unique<expression::condition>();
-        cond->data_id = std::move(data_id);
+        expression::condition cond;
+        cond.data_id = std::move(data_id);
         conditions_.emplace_back(std::move(cond));
     }
 
     void set_data_id(std::string data_id)
     {
         auto &cond = conditions_.back();
-        cond->data_id = std::move(data_id);
+        cond.data_id = std::move(data_id);
     }
 
     template <typename T, typename... Args> void set_matcher(Args... args)
     {
         auto &cond = conditions_.back();
-        cond->matcher = std::make_unique<T>(args...);
+        cond.matcher = std::make_unique<T>(args...);
     }
 
     void set_matcher(std::unique_ptr<matcher::base> &&matcher)
     {
         auto &cond = conditions_.back();
-        cond->matcher = std::move(matcher);
+        cond.matcher = std::move(matcher);
     }
 
     void add_target(std::string name, std::vector<std::string> key_path = {},
@@ -180,7 +177,7 @@ class expression_builder {
 
 protected:
     ddwaf::object_limits limits_{};
-    std::vector<std::unique_ptr<expression::condition>> conditions_{};
+    std::vector<expression::condition> conditions_{};
 };
 
 } // namespace ddwaf

src/rule.hpp

@@ -65,7 +65,20 @@ class rule {
     virtual std::optional<event> match(const object_store &store, cache_type &cache,
         const exclusion::object_set &objects_excluded,
         const std::unordered_map<std::string, std::shared_ptr<matcher::base>> &dynamic_matchers,
-        ddwaf::timer &deadline) const;
+        ddwaf::timer &deadline) const
+    {
+        if (expression::get_result(cache)) {
+            // An event was already produced, so we skip the rule
+            return std::nullopt;
+        }
+
+        auto res = expr_->eval(cache, store, objects_excluded, dynamic_matchers, deadline);
+        if (!res.outcome) {
+            return std::nullopt;
+        }
+
+        return {ddwaf::event{this, expression::get_matches(cache), res.ephemeral}};
+    }
 
     [[nodiscard]] bool is_enabled() const { return enabled_; }
     void toggle(bool value) { enabled_ = value; }

tests/rule_test.cpp

@@ -15,6 +15,7 @@ using namespace ddwaf;
 using namespace std::literals;
 
 namespace {
+
 TEST(TestRule, Match)
 {
     expression_builder builder(1);
@@ -49,6 +50,7 @@ TEST(TestRule, Match)
         std::vector<std::string> expected_actions{"update", "block", "passlist"};
         EXPECT_EQ(event->rule->get_actions(), expected_actions);
         EXPECT_EQ(event->matches.size(), 1);
+        EXPECT_FALSE(event->ephemeral);
 
         auto &match = event->matches[0];
         EXPECT_STREQ(match.resolved.c_str(), "192.168.0.1");
@@ -57,6 +59,7 @@ TEST(TestRule, Match)
         EXPECT_STREQ(match.operator_value.data(), "");
         EXPECT_STREQ(match.address.data(), "http.client_ip");
         EXPECT_TRUE(match.key_path.empty());
+        EXPECT_FALSE(match.ephemeral);
     }
 
     {
@@ -67,6 +70,8 @@ TEST(TestRule, Match)
         EXPECT_FALSE(event.has_value());
     }
 
+    EXPECT_TRUE(cache.result);
+
     ddwaf_object_free(&root);
 }
 
@@ -108,6 +113,8 @@ TEST(TestRule, EphemeralMatch)
         EXPECT_TRUE(event->ephemeral);
     }
 
+    EXPECT_FALSE(cache.result);
+
     ddwaf_object_free(&root);
 }