security(cel): key metric cache lookups strictly by requested params

spp_cel_domain/__manifest__.py

@@ -2,7 +2,7 @@
 {
     "name": "CEL Domain Query Builder",
     "summary": "Write simple CEL-like expressions to filter records (OpenSPP/OpenG2P friendly)",
-    "version": "19.0.2.0.0",
+    "version": "19.0.2.0.1",
     "license": "LGPL-3",
     "development_status": "Production/Stable",
     "author": "OpenSPP.org, OpenSPP Community",

spp_cel_domain/models/cel_executor.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import inspect
 import logging
 from collections.abc import Iterable, Iterator
 from typing import Any
@@ -1218,7 +1219,9 @@ def _exec_metric(
             if not batch_ids:
                 continue
             total_requested += len(batch_ids)
-            batch_values, batch_stats = svc.evaluate(p.metric, subject_model, batch_ids, period_key, mode=eval_mode)
+            batch_values, batch_stats = self._svc_evaluate_batch(  # pragma: no cover
+                svc, p, subject_model, batch_ids, period_key, eval_mode
+            )
             aggregated_values.update(batch_values)
             if batch_stats:
                 stats_total["cache_hits"] += int(batch_stats.get("cache_hits") or 0)
@@ -1463,17 +1466,52 @@ def _feature_value_subquery(
         )
         return SQL("(%s)", SQL(sql, *args))
 
+    @staticmethod
+    def _evaluate_accepts_params(svc) -> bool:
+        """Whether ``svc.evaluate`` accepts a ``params`` keyword argument.
+
+        The evaluation service (``spp.indicator``) is provided by a legacy/external
+        module whose signature we do not control and which may predate the
+        ``params`` kwarg. Returns True when ``evaluate`` declares an explicit
+        ``params`` parameter or a ``**kwargs`` catch-all; False otherwise (so the
+        caller degrades to an unparameterized call instead of raising ``TypeError``).
+        """
+        try:
+            sig = inspect.signature(svc.evaluate)
+        except (TypeError, ValueError):
+            return False
+        return any(prm.name == "params" or prm.kind is inspect.Parameter.VAR_KEYWORD for prm in sig.parameters.values())
+
+    def _svc_evaluate_batch(self, svc, p, subject_model, batch_ids, period_key, eval_mode):  # pragma: no cover
+        """Call the legacy/external evaluation service for one batch.
+
+        Threads the metric's params through so parameterized refreshes are computed
+        with the right params — but only when ``evaluate`` accepts a ``params`` kwarg
+        (see ``_evaluate_accepts_params``), degrading gracefully on older services.
+
+        Not covered by tests: ``spp.indicator`` is not in this repo's dependency
+        closure, so this path is unreachable here; the params-compat decision is
+        unit-tested via ``_evaluate_accepts_params``.
+        """
+        eval_kwargs = {"mode": eval_mode}
+        metric_params = getattr(p, "params", None)
+        if metric_params and self._evaluate_accepts_params(svc):
+            eval_kwargs["params"] = metric_params
+        return svc.evaluate(p.metric, subject_model, batch_ids, period_key, **eval_kwargs)
+
     def _provider_clause(self, provider: str, params_hash: str, allow_any_provider: bool) -> tuple[str, list[Any]]:
         provider = provider or ""
         params_hash = params_hash or ""
         combos: list[tuple[str, str]] = [
             (provider, params_hash),
         ]
         if provider:
+            # Relax the provider (a routing/registry detail) but keep the requested
+            # params_hash. Params are a semantic filter, not a provider detail: a
+            # non-empty params_hash must never fall back to params_hash "" rows, or a
+            # parameterized metric would match unparameterized/legacy cache rows. When
+            # params_hash == "" this combo already covers the unparameterized rows.
             combos.append(("", params_hash))
-        if params_hash:
-            combos.append((provider, ""))
-        combos.append(("", ""))
         # Deduplicate while preserving order
         seen = set()
         uniq_combos: list[tuple[str, str]] = []

spp_cel_domain/tests/__init__.py

@@ -32,3 +32,4 @@
 from . import test_cel_relational_predicate
 from . import test_cel_smart_op_lookup
 from . import test_cel_translator_cache
+from . import test_evaluate_accepts_params

spp_cel_domain/tests/test_evaluate_accepts_params.py

@@ -0,0 +1,51 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""Unit tests for the params-compatibility guard used before calling the
+legacy/external evaluation service (spp.indicator.evaluate).
+
+The metric compute path passes the metric's params to svc.evaluate only when
+that method accepts a `params` kwarg, so a parameterized refresh is computed
+with the right params without risking a TypeError on an older service that
+predates the kwarg. See _evaluate_accepts_params / _svc_evaluate_batch in
+cel_executor.py.
+"""
+
+from odoo.tests.common import TransactionCase, tagged
+
+
+class _EvalWithParams:
+    def evaluate(self, metric, model, ids, period_key, mode="fallback", params=None):
+        return {}, {}
+
+
+class _EvalWithKwargs:
+    def evaluate(self, metric, model, ids, period_key, **kwargs):
+        return {}, {}
+
+
+class _EvalNoParams:
+    def evaluate(self, metric, model, ids, period_key, mode="fallback"):
+        return {}, {}
+
+
+class _NonCallableEvaluate:
+    evaluate = 42  # inspect.signature() raises TypeError -> degrade to False
+
+
+@tagged("post_install", "-at_install")
+class TestEvaluateAcceptsParams(TransactionCase):
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.executor = cls.env["spp.cel.executor"]
+
+    def test_explicit_params_arg_supported(self):
+        self.assertTrue(self.executor._evaluate_accepts_params(_EvalWithParams()))
+
+    def test_var_keyword_supported(self):
+        self.assertTrue(self.executor._evaluate_accepts_params(_EvalWithKwargs()))
+
+    def test_no_params_not_supported(self):
+        self.assertFalse(self.executor._evaluate_accepts_params(_EvalNoParams()))
+
+    def test_uninspectable_evaluate_degrades_to_false(self):
+        self.assertFalse(self.executor._evaluate_accepts_params(_NonCallableEvaluate()))

spp_dci_indicators/tests/test_dci_cel_params.py

@@ -64,3 +64,50 @@ def test_param_selects_the_matching_row(self):
     def test_param_discriminates_by_params_hash(self):
         # Hearing=1 >= 3 -> excluded (proves the lookup keyed on params, not just name)
         self.assertNotIn(self.partner, self._match("metric('zz.test.severity', me, arg='Hearing') >= 3"))
+
+    def test_param_query_ignores_unparameterized_row(self):
+        """A parameterized query must NOT fall back to an unparameterized cache
+        row for the same metric. Seed a legacy/default row (no params -> params_hash
+        "") whose value would satisfy the comparison; querying with a param whose
+        own row does NOT satisfy it must still exclude the subject.
+
+        Before the fix, _provider_clause appended (provider, "") / ("", "")
+        combos, so the unparameterized value=4 row matched arg='Hearing' >= 3.
+        """
+        # Unparameterized row (params_hash "") with a value that satisfies >= 3.
+        self.DV.upsert_values(
+            [
+                {
+                    "variable_name": "zz.test.severity",
+                    "subject_model": "res.partner",
+                    "subject_id": self.partner.id,
+                    "period_key": "current",
+                    "value_json": {"value": 4},
+                    "value_type": "number",
+                    "source_type": "external",
+                    # no "params" -> params_hash == ""
+                    "ttl_seconds": 3600,
+                },
+            ]
+        )
+        # Hearing's own value is 1 (< 3); the unparameterized 4 must not leak in.
+        self.assertNotIn(self.partner, self._match("metric('zz.test.severity', me, arg='Hearing') >= 3"))
+
+    def test_unparameterized_query_still_matches_unparameterized_row(self):
+        """No regression for legacy metrics: an unparameterized query still
+        matches an unparameterized cache row."""
+        self.DV.upsert_values(
+            [
+                {
+                    "variable_name": "zz.test.plain",
+                    "subject_model": "res.partner",
+                    "subject_id": self.partner.id,
+                    "period_key": "current",
+                    "value_json": {"value": 4},
+                    "value_type": "number",
+                    "source_type": "external",
+                    "ttl_seconds": 3600,
+                },
+            ]
+        )
+        self.assertIn(self.partner, self._match("metric('zz.test.plain', me) >= 3"))