security(gis): restrict spatial stats to GIS-published indicators#359
Draft
gonzalesedwin1123 wants to merge 3 commits into
Draft
security(gis): restrict spatial stats to GIS-published indicators#359gonzalesedwin1123 wants to merge 3 commits into
gonzalesedwin1123 wants to merge 3 commits into
Conversation
Client-supplied request.variables were passed straight to the aggregation
service, which resolves names via sudo() against ALL spp.indicator and ALL
spp.cel.variable records with no publication check. Only the default (no
variables) path limited to get_published_for_context('gis'). So a caller with
only gis:read/statistics:read could request unpublished indicator names, or raw
CEL variable names, and receive their aggregates - bypassing the intended GIS
publication boundary.
Enforce the boundary as a positive allowlist at the single chokepoint
(_compute_via_aggregation_service): filter supplied variables to the
GIS-published indicator names (is_published_gis), same set as the default path;
drop unknown/unpublished names before aggregation (matching the existing
'unknown variables -> valid response' contract, oracle-free). The registry
sudo-resolution and downstream metadata lookup therefore only ever see
published names.
Bump 19.0.2.0.0 -> 19.0.2.0.1 + HISTORY. No migration (behavior fix, no schema
change). 195 tests (4 new).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Member
Author
|
Promised sweep done: the same pattern (user-supplied statistic names → |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #359 +/- ##
==========================================
- Coverage 74.28% 69.08% -5.21%
==========================================
Files 372 133 -239
Lines 25385 12174 -13211
==========================================
- Hits 18857 8410 -10447
+ Misses 6528 3764 -2764
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Applies CI's pinned oca-gen output verbatim for the new HISTORY fragment.
- Preserve deterministic default-path ordering: use the ordered indicator recordset for the default statistics list, and the set only for the supplied-path membership test (was list(set(...)) -> non-deterministic across processes). - Document the invariant that _convert_aggregation_result's metadata search only sees already-published names. - Add a mixed published+unpublished list test (the realistic exfil attempt): the published name is kept, unpublished indicator and raw CEL var are dropped. 196 tests (was 195). Both staff reviews: SHIP.
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 GIS spatial/proximity statistics path accepts
request.variablesfrom theAPI and passes it straight to the analytics aggregation service, which resolves
the supplied names via
sudo()against allspp.indicatorrecords and thenall
spp.cel.variablerecords — with no publication check. Only the default(no-variables) path limited computation to
get_published_for_context("gis").So a caller with only
gis:read/statistics:readcould request unpublishedindicator names, or raw CEL variable names never surfaced as a GIS indicator,
and receive their aggregate counts/values — bypassing the intended GIS
publication boundary and potentially exposing sensitive beneficiary aggregates
(k-anonymity mitigates but does not close this).
Fix
Enforce the publication boundary as a positive allowlist (default-deny) at
the single chokepoint every query path funnels through
(
spatial_query_service._compute_via_aggregation_service): compute theGIS-published indicator names (
spp.indicator.is_published_gis, viaget_published_for_context("gis")) and restrict both the supplied anddefault paths to that set. Unknown/unpublished names — including raw CEL
variable names — are dropped before
compute_aggregation, so the registry'ssudo resolution and the downstream metadata lookup only ever see published
names.
Dropping (not erroring) matches the existing "unknown variables → valid
response" contract (
tests/test_batch_query.py) and avoids an existence oracle(a client can't distinguish unpublished-but-existing from nonexistent).
total_countis a separate response field, so the basic area count isunaffected.
Testing
./spp t spp_api_v2_gis→ 195 passed, 0 failed, 0 errors (4 new).computed (both fail before the fix); a published-GIS indicator name IS
computed; the default path still returns the published set only.
Scope / follow-up
spp_analyticsregistryresolves names context-agnostically and is used by other contexts
(dashboard/api/report) that may intentionally compute non-GIS stats, so the
GIS boundary belongs in the GIS caller (mirrors the default path). I'll sweep
the other API contexts for the same "user variables → compute_aggregation
without an allowlist" pattern and file a follow-up if found.
at 19.0.2.0.0, but access/compute logic changes need no data migration).
Draft: awaiting human security review before undraft/merge.