security(dci): restrict Data API pull to non-sensitive provider variables#257
Merged
Conversation
…bles The Data API GET /Data/pull required only the generic data:read scope, read spp.data.value with sudo (bypassing the #234 ORM ACL), and matched any variable_name with no provider/variable check (unlike push/invalidate). Once eligibility precompute/refresh cached DCI-backed values, a data:read client could pull sensitive CRVS facts (r.dci.crvs.is_alive, birth_verified) and disability metrics for any subject. Scoring/PMT values were exposed the same way, and /variables enumerated all of them. Add a positive, default-deny allowlist on the resolved variable: - spp.cel.variable.is_data_api_pullable() — true only for ordinary external-provider variables; computed/scoring/aggregate are not. - spp_dci_indicators overrides it to also exclude DCI-backed providers (inter-registry data has its own consent/provider boundary). - /pull resolves the requested name by cel_accessor (the cache key), denies with a uniform 403 if it resolves to nothing (fail-closed, covers orphan rows) or if any match is not pullable; also adds the missing company_id filter (cross-company read under sudo). - /variables lists only pullable external-provider variables. The internal ORM/RPC read by base.group_user was already closed by #234; this covers the FastAPI sudo path it did not. Sibling oracles (Simulation metric() expressions; subject-existence via sudo resolve) are tracked separately. Tests: base rule (spp_cel_domain), DCI override (spp_dci_indicators), endpoint behavior incl. unknown/non-pullable -> 403 and company isolation (spp_api_v2_data). spp_api_v2_data 57/57, spp_cel_domain 620/620, spp_dci_indicators 82/82.
![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 restricts the generic external Data API to only expose ordinary external-provider variables, preventing computed, scoring, and DCI-backed variables from being retrieved. It also adds company isolation to the pull endpoint and includes comprehensive tests. The review feedback highlights a critical issue in the list_variables endpoint where Python-side filtering of variables causes a mismatch with database-side total counts and pagination. To resolve this, the reviewer suggests implementing a model method _get_data_api_pullable_domain to perform the filtering directly at the database level.
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 #257 +/- ##
==========================================
+ Coverage 74.75% 74.80% +0.05%
==========================================
Files 1090 1092 +2
Lines 64813 64836 +23
==========================================
+ Hits 48453 48503 +50
+ Misses 16360 16333 -27
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Code review (Gemini) + Codecov follow-ups on the pull/list allowlist: - list_variables: filter at the DB level via a new model method spp.cel.variable._get_data_api_pullable_domain() (overridden in spp_dci_indicators to also exclude DCI-backed providers), instead of a Python post-filter. This fixes the total count and cursor pagination, which were computed on the unfiltered domain. - Real, executing endpoint tests: the existing Data API endpoint tests are `async def` on a plain TransactionCase, which unittest never awaits (silent no-ops) -- which is why the router changes showed as uncovered. Add test_data_api_pull_security.py driving the async endpoints via asyncio.run so they actually run: pull rejects unknown (fail-closed) and non-pullable variables with 403, enforces company isolation, returns ordinary values; list excludes non-external with a consistent total. Remove the 3 no-op async pull tests added earlier (superseded by these). These running tests surfaced two pre-existing latent bugs in the endpoints (both in functions touched here), now fixed: - pull_values crashed (pydantic) on any cached value without a TTL, because an unset Odoo Datetime returns False, not None -> serialize expires_at as `rec.expires_at or None`. - list_variables crashed (AttributeError) when spp_studio is not installed, since `description` is contributed by spp_studio -> use getattr. Tests: spp_api_v2_data 59/59, spp_cel_domain 621/621, spp_dci_indicators 82/82. NOTE: the repo-wide async-test-no-op problem (async def endpoint tests across several spp_api_v2_* modules never execute) is pre-existing and tracked separately; only this module's relevant tests are made real here.
gonzalesedwin1123
added a commit
that referenced
this pull request
Jun 30, 2026
GET /studio/variables/{resource_type}/{identifier} (and the underlying
VariableValueService.get_values_for_subject[s]) read spp.data.value with sudo,
no allowlist, no company filter, and with the default variables="*" returned
EVERY cached value for a subject -- including DCI/CRVS (inter-registry
health/vital) and scoring/PMT values -- to any client holding the generic
studio:read scope. Same exposure class as the Data API /Data/pull leak fixed
in #257, on a sibling endpoint/scope (found by #257's post-implementation
review).
Reuse the #257 allowlist: restrict both service methods to API-pullable
variables (spp.cel.variable._get_data_api_pullable_domain -> ordinary
external-provider only; DCI-backed/computed/scoring excluded) by intersecting
on cel_accessor, and add the missing company_id filter. The pullable-variable
lookup is sudo (system config; API authz is the scope check).
Tests: service-level (excludes non-pullable + scoring, company isolation) and
the existing endpoint/value tests updated to use external-provider fixtures
(the only pullable kind under the new policy). spp_studio_api_v2 162/162.
Stacked on #257 (security-dci-crvs-cache) for the shared allowlist primitive;
retarget PR base to 19.0 after #257 merges.
gonzalesedwin1123
added a commit
that referenced
this pull request
Jun 30, 2026
GET /studio/variables/{resource_type}/{identifier} (and the underlying
VariableValueService.get_values_for_subject[s]) read spp.data.value with sudo,
no allowlist, no company filter, and with the default variables="*" returned
EVERY cached value for a subject -- including DCI/CRVS (inter-registry
health/vital) and scoring/PMT values -- to any client holding the generic
studio:read scope. Same exposure class as the Data API /Data/pull leak fixed
in #257, on a sibling endpoint/scope (found by #257's post-implementation
review).
Reuse the #257 allowlist: restrict both service methods to API-pullable
variables (spp.cel.variable._get_data_api_pullable_domain -> ordinary
external-provider only; DCI-backed/computed/scoring excluded) by intersecting
on cel_accessor, and add the missing company_id filter. The pullable-variable
lookup is sudo (system config; API authz is the scope check).
Tests: service-level (excludes non-pullable + scoring, company isolation) and
the existing endpoint/value tests updated to use external-provider fixtures
(the only pullable kind under the new policy). spp_studio_api_v2 162/162.
Stacked on #257 (security-dci-crvs-cache) for the shared allowlist primitive;
retarget PR base to 19.0 after #257 merges.
gonzalesedwin1123
added a commit
that referenced
this pull request
Jun 30, 2026
* security(studio): allowlist Studio variable-values endpoint
GET /studio/variables/{resource_type}/{identifier} (and the underlying
VariableValueService.get_values_for_subject[s]) read spp.data.value with sudo,
no allowlist, no company filter, and with the default variables="*" returned
EVERY cached value for a subject -- including DCI/CRVS (inter-registry
health/vital) and scoring/PMT values -- to any client holding the generic
studio:read scope. Same exposure class as the Data API /Data/pull leak fixed
in #257, on a sibling endpoint/scope (found by #257's post-implementation
review).
Reuse the #257 allowlist: restrict both service methods to API-pullable
variables (spp.cel.variable._get_data_api_pullable_domain -> ordinary
external-provider only; DCI-backed/computed/scoring excluded) by intersecting
on cel_accessor, and add the missing company_id filter. The pullable-variable
lookup is sudo (system config; API authz is the scope check).
Tests: service-level (excludes non-pullable + scoring, company isolation) and
the existing endpoint/value tests updated to use external-provider fixtures
(the only pullable kind under the new policy). spp_studio_api_v2 162/162.
Stacked on #257 (security-dci-crvs-cache) for the shared allowlist primitive;
retarget PR base to 19.0 after #257 merges.
* style: ruff-format empty-allowlist test call
gonzalesedwin1123
added a commit
that referenced
this pull request
Jul 2, 2026
…SQL CASE (from #76) Re-lands the spp_cel_domain portion of reverted PR #76: - SQL CASE support: to_sql_case compiler for CEL ternaries, case_when/ comparison builders, right-associative ternary parsing fix - Read-only smart operator label lookup (no record creation, no sudo during compilation) - Tests for the translation cache helpers tests/__init__.py was merged by hand to keep imports added by later security PRs (#251/#257/#223). Version bumped to 19.0.2.1.0 with a HISTORY entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Data API pull endpoint exposed sensitive cached values to any client with the generic
data:readscope.GET /Data/pull(spp_api_v2_data/routers/data.py) readspp.data.valuewithsudo()(bypassing the ORM ACL #234 added) and matched anyvariable_namewith no provider/variable-ownership check — unlikepush/invalidate, which call_validate_provider_access. Once eligibility precompute/refresh cached DCI-backed values, adata:readclient could pull CRVS facts (r.dci.crvs.is_alive,birth_verified) and disability metrics for any subject. Scoring/PMT values leaked the same way, andGET /Data/variablesenumerated all of them.The internal ORM/RPC read by
base.group_userwas already closed by #234; this PR covers the FastAPIsudopath that #234 did not.Fix — positive, default-deny allowlist on the resolved variable
spp_cel_domain:spp.cel.variable.is_data_api_pullable()— true only for ordinary external-provider variables; computed/scoring/aggregate are not.spp_dci_indicators: overrides it to also exclude DCI-backed providers (inter-registry data has its own consent/provider boundary).spp_api_v2_data/routers/data.py:/pullresolves the requested name bycel_accessor(the cache key), and returns a uniform 403 if it resolves to nothing (fail-closed — covers orphan rows from a deleted/de-provisioned variable) or if any match isn't pullable. Adds the missingcompany_idfilter (cross-company read undersudo)./variableslists only pullable external-provider variables (no longer enumerates computed/scoring/DCI accessors).Why an allowlist resolved by
cel_accessor, fail-closedA staff review showed a DCI-only denylist was insufficient: cache rows are keyed by
cel_accessor(notname, which is whatis_dci_backed-by-name would miss);cel_accessoris unique only perapplies_to(so all matches must be checked, deny-on-any); scoring/PMT values aren'tis_dci_backedbut leak identically; and orphaned rows must fail closed.Scope / non-impact
metric('r.dci.crvs.is_alive', me)==1→beneficiary_countoracle; separatesimulation:executescope) and a subject-existence oracle viasudo_resolve_subject_id.Tests
spp_cel_domain: baseis_data_api_pullablerule (external/computed/scoring/no-provider).spp_dci_indicators: DCI-backed override (DCI → not pullable; ordinary → pullable).spp_api_v2_data: endpoint behavior — unknown variable → 403, non-pullable (computed) → 403, company isolation; updated existingno_results/list_variablestests for the deliberate behavior change.All green:
spp_api_v2_data57/57 ·spp_cel_domain620/620 ·spp_dci_indicators82/82. Lint-clean (ruff/ruff-format).