Restrict DCI sync and cached value access#234
Conversation
![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 introduces access control restrictions for syncing DCI-backed CEL values. It disables default read access to spp.data.value for base users, restricts the sync action to CEL Domain Managers, and adds a permission check in sync_for_partners along with a corresponding unit test. Feedback on the changes suggests updating the access check in _check_dci_sync_access to bypass the group check when running in superuser mode (self.env.su), preventing unexpected AccessError exceptions during sudo() operations.
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #234 +/- ##
==========================================
+ Coverage 74.47% 74.60% +0.13%
==========================================
Files 1062 1077 +15
Lines 63833 64319 +486
==========================================
+ Hits 47540 47987 +447
- Misses 16293 16332 +39
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add an explicit superuser early-return to _check_dci_sync_access so the scheduled sync cron (user_id=base.user_root, su mode) passes its own gate, mirroring spp_cel_domain data_credential.py. Add positive tests for the CEL-manager allow path and the su/cron bypass.
gonzalesedwin1123
force-pushed
the
codex/propose-fix-for-dci-sync-vulnerability
branch
from
ea59ead to
ee403cf
Compare
Follow-up: a second outbound-fetch route is not covered by this gateThis PR gates The path:
Exploitability is unconfirmed. I found no button/action/RPC exposing Suggested follow-up:
Note: the new |
CI pre-commit prettier (plugin-xml) requires the long group_ids server-action field to be wrapped across lines, matching the file's existing field style.
gonzalesedwin1123
deleted the
codex/propose-fix-for-dci-sync-vulnerability
branch
…bles (#257) * security(dci): restrict Data API pull to non-sensitive provider variables 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. * security(dci): address review on Data API pull/list hardening 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.
…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.
2 participants
Motivation
sync_for_partners()entrypoint allowed arbitrary internal users to trigger outbound DCI lookups and populate sensitive disability/health indicators into the shared cachespp.data.valuewithout authorization.dr.dci.has_disability, severity flags) are sensitive and were readable bybase.group_uservia existing ACLs, creating a confidentiality risk.user_id, making scheduled execution ambiguous and potentially privileged operations less explicit.Description
_check_dci_sync_access()tospp.dci.cel.fetcherwhich raisesAccessErrorunless the caller has thespp_cel_domain.group_cel_domain_managergroup, and invoked it at the start ofsync_for_partners()to block low-privileged RPC/server calls.action_sync_dci_valuesto CEL Domain Managers by adding agroup_idsentry and set the scheduled cronuser_idtobase.user_rootso scheduled syncs remain explicit and privileged.base.group_userread permission formodel_spp_data_valueinspp_cel_domain/security/ir.model.access.csv(set read to0) so cached DCI values are no longer globally visible to basic internal users.base.group_user) receivesAccessErrorwhen callingsync_for_partners()directly (test_sync_for_partners_requires_cel_manager).Testing
python3 -m py_compileon the modified files (spp_dci_indicators/models/dci_cel_fetcher.pyand tests) and compilation succeeded.spp_dci_indicators/data/dci_sync.xml) withxml.etree.ElementTree.parseand performed a static ACL check onspp_cel_domain/security/ir.model.access.csv, both of which succeeded.git diff --checkand static checks with no reported issues.TransactionCasetest suite butimport odoofailed in this environment so full integration/unit test execution could not be performed here; a new unit test was added and should run in CI where Odoo is available.Codex Task