security(cel): key metric cache lookups strictly by requested params#272
security(cel): key metric cache lookups strictly by requested params#272gonzalesedwin1123 wants to merge 2 commits into
Conversation
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CEL Domain Query Builder to prevent parameterized queries from falling back to unparameterized cache rows. This is achieved by adjusting the query combinations in _provider_clause to avoid appending fallback combinations with empty parameter hashes when a parameter hash is requested. Additionally, the params attribute is now passed to svc.evaluate, and corresponding unit tests have been added. The feedback points out a potential compatibility issue where svc.evaluate might not support the params keyword argument in older environments, which could lead to a runtime TypeError. A dynamic signature check is suggested to ensure backward compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #272 +/- ##
=======================================
Coverage 75.31% 75.31%
=======================================
Files 1098 1098
Lines 65317 65314 -3
=======================================
- Hits 49191 49189 -2
+ Misses 16126 16125 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
1 participant
Problem
Commit
686702a4made namedmetric()args part ofMetricCompare.paramsand hashed them(
params_hash) before cache lookup. But two paths still crossed the parameter boundary:_provider_clause()fallback (cel_executor.py) always appended combos withparams_hash=""((provider, "")and("", "")), even when a non-empty params_hash wasrequested. So
metric('x', me, arg='Hearing')could match a cache row for metricxwithno params if that row satisfied the comparison. This clause feeds both the freshness
preflight and the SQL fast path.
svc.evaluate(p.metric, subject_model, batch_ids, period_key, mode=...)withoutp.params,so parameterized cache misses/refreshes were computed as unparameterized.
Where CEL predicates drive DCI/social-search or eligibility/targeting, this could make
parameter-specific filters include the wrong beneficiaries or leak records selected by a
less-specific cached value. Severity: High.
Fix
_provider_clause: relax the provider (a routing/registry detail) only — never therequested
params_hash. Removed the(provider, "")and("", "")fallback combos. Whenparams_hash == ""the retained combos already cover unparameterized rows, so behavior forlegacy/unparameterized metrics is unchanged. The
allow_any_providerclause already keyed onthe requested
params_hash, so it was left as-is.params=p.paramsthrough tosvc.evaluate(...)so refreshes/misses ofa parameterized metric are computed (and cached) with the same params the read is keyed on. The
aggregate call site is unchanged —
AggMetricComparecarries noparams.Tests
The shipped test (
spp_dci_indicators/tests/test_dci_cel_params.py) only seeded parameterizedrows, so it could not catch pollution from a legacy/default unparameterized row. Added:
test_param_query_ignores_unparameterized_row— seeds aHearingrow (value 1) and anunparameterized row (value 4) for the same metric;
metric(..., arg='Hearing') >= 3mustexclude the subject. Failed before the fix (the value=4 row leaked in), passes after.
test_unparameterized_query_still_matches_unparameterized_row— no regression for legacymetrics: an unparameterized query still matches an unparameterized row.
Written test-first (red → green).
./spp test spp_cel_domain→ 621 passed;./spp test spp_dci_indicators→ 84 passed;./spp lintclean.Note
svc.evaluate(spp.indicator) has no in-repo definition (legacy/optional service), so theparams=addition on that call is not exercised by in-repo tests. It is additive and consistentwith the write path (
spp.data.value.upsert_valuesalready takesparams); it cannot regress apath that has no working evaluator in this repo. The primary parameterized-metric flow
(
spp_dci_indicatorsfetcher →upsert_values→ SQL fast path) is fully covered by the testsabove, and that is the path the
_provider_clausefix governs.