From f6f15281bbbf4d2c455445fa02a0d076d7c15b73 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 20:14:59 +0800 Subject: [PATCH 1/2] 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 e2da4d5c7eb5f972563951eae6205f783c4a3971 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 23:04:22 +0800 Subject: [PATCH 2/2] 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()))