From f6f15281bbbf4d2c455445fa02a0d076d7c15b73 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 20:14:59 +0800 Subject: [PATCH 1/3] security(cel): key metric cache lookups strictly by requested params metric() named args are hashed into params_hash for cache lookup, but two paths still crossed the parameter boundary: - _provider_clause() always appended fallback combos with params_hash="" ((provider, "") and ("", "")) even when a non-empty params_hash was requested, so metric('x', me, arg='Hearing') could match an unparameterized cache row for metric x. Params are a semantic filter, not a provider-routing detail: relax the provider only, never the requested params_hash. When params_hash is empty the retained combos already cover unparameterized rows. - The non-fresh compute path called svc.evaluate() without params, so parameterized cache misses/refreshes were computed as unparameterized. Pass p.params through. Where CEL predicates drive DCI/social-search or eligibility/targeting, the fallback could include the wrong beneficiaries or leak records selected by a less-specific cached value. Add a regression test for the gap the shipped test missed: a parameterized query must ignore an unparameterized cache row for the same metric, while an unparameterized query still matches unparameterized rows. spp_cel_domain 621/621, spp_dci_indicators 84/84. --- spp_cel_domain/__manifest__.py | 2 +- spp_cel_domain/models/cel_executor.py | 12 +++-- .../tests/test_dci_cel_params.py | 47 +++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/spp_cel_domain/__manifest__.py b/spp_cel_domain/__manifest__.py index eedd10320..955a58f01 100644 --- a/spp_cel_domain/__manifest__.py +++ b/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", diff --git a/spp_cel_domain/models/cel_executor.py b/spp_cel_domain/models/cel_executor.py index b9fb36003..1ef54652e 100644 --- a/spp_cel_domain/models/cel_executor.py +++ b/spp_cel_domain/models/cel_executor.py @@ -1218,7 +1218,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 = svc.evaluate( + p.metric, subject_model, batch_ids, period_key, mode=eval_mode, params=getattr(p, "params", None) + ) aggregated_values.update(batch_values) if batch_stats: stats_total["cache_hits"] += int(batch_stats.get("cache_hits") or 0) @@ -1470,10 +1472,12 @@ def _provider_clause(self, provider: str, params_hash: str, allow_any_provider: (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]] = [] diff --git a/spp_dci_indicators/tests/test_dci_cel_params.py b/spp_dci_indicators/tests/test_dci_cel_params.py index fada96298..5f495618a 100644 --- a/spp_dci_indicators/tests/test_dci_cel_params.py +++ b/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")) From f6149e73fdb70d140a56c1b9b889a00020522bed Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 23:04:22 +0800 Subject: [PATCH 2/3] security(cel): guard params kwarg against legacy evaluate signature Address review on PR #272: svc.evaluate lives in a legacy/external service (spp.indicator) whose signature we do not control and which may predate the params kwarg, so passing params unconditionally could raise TypeError at runtime. - Add _evaluate_accepts_params(): detect (once) whether evaluate declares an explicit params parameter or a **kwargs catch-all; unit-tested directly. - Move the service call into _svc_evaluate_batch(), which passes params only when the metric is parameterized AND the signature accepts it, degrading to an unparameterized call otherwise. This path is unreachable in-repo (spp.indicator is not in the dependency closure), so it is marked '# pragma: no cover'; the compat decision itself is covered by unit tests. --- spp_cel_domain/models/cel_executor.py | 38 +++++++++++++- spp_cel_domain/tests/__init__.py | 1 + .../tests/test_evaluate_accepts_params.py | 51 +++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 spp_cel_domain/tests/test_evaluate_accepts_params.py diff --git a/spp_cel_domain/models/cel_executor.py b/spp_cel_domain/models/cel_executor.py index 1ef54652e..a0241e02e 100644 --- a/spp_cel_domain/models/cel_executor.py +++ b/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,8 +1219,8 @@ 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, params=getattr(p, "params", None) + 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: @@ -1465,6 +1466,39 @@ 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 "" diff --git a/spp_cel_domain/tests/__init__.py b/spp_cel_domain/tests/__init__.py index f9eb6bff3..6733b1424 100644 --- a/spp_cel_domain/tests/__init__.py +++ b/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 diff --git a/spp_cel_domain/tests/test_evaluate_accepts_params.py b/spp_cel_domain/tests/test_evaluate_accepts_params.py new file mode 100644 index 000000000..62d47230a --- /dev/null +++ b/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())) From ee00685970352ea62e8b3c432135213d106cfe12 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 21 Jul 2026 11:55:45 +0800 Subject: [PATCH 3/3] docs(spp_cel_domain): apply CI-generated README for 19.0.2.1.1 --- spp_cel_domain/README.rst | 9 +++++++++ spp_cel_domain/static/description/index.html | 12 +++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/spp_cel_domain/README.rst b/spp_cel_domain/README.rst index 9a72fa2b4..44ebf3975 100644 --- a/spp_cel_domain/README.rst +++ b/spp_cel_domain/README.rst @@ -142,6 +142,15 @@ Dependencies Changelog ========= +19.0.2.1.1 +~~~~~~~~~~ + +- fix(security): metric cache lookups are keyed strictly by the + requested params — a parameterized metric request no longer falls back + to unparameterized (or differently-parameterized) cache rows, which + could return values computed for other parameters. ``evaluate()`` also + guards the ``params`` kwarg against legacy signatures. + 19.0.2.1.0 ~~~~~~~~~~ diff --git a/spp_cel_domain/static/description/index.html b/spp_cel_domain/static/description/index.html index f280ce2a1..99156805f 100644 --- a/spp_cel_domain/static/description/index.html +++ b/spp_cel_domain/static/description/index.html @@ -522,6 +522,16 @@

Changelog

+

19.0.2.1.1

+
    +
  • fix(security): metric cache lookups are keyed strictly by the +requested params — a parameterized metric request no longer falls back +to unparameterized (or differently-parameterized) cache rows, which +could return values computed for other parameters. evaluate() also +guards the params kwarg against legacy signatures.
    • +
+
+

19.0.2.1.0

  • feat(sql): compile CEL ternary expressions to SQL CASE via @@ -533,7 +543,7 @@

    19.0.2.1.0

  • test(translator): add coverage for the CEL translation cache helpers
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2