From 99835d8e27b3e6af69825b833b9bfae09fdf2a2c Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 20:03:18 +0800 Subject: [PATCH 1/3] security(gis): restrict spatial stats to GIS-published indicators 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). --- spp_api_v2_gis/__manifest__.py | 2 +- spp_api_v2_gis/readme/HISTORY.md | 11 +++ .../services/spatial_query_service.py | 25 +++-- spp_api_v2_gis/tests/__init__.py | 1 + .../tests/test_gis_published_variables.py | 95 +++++++++++++++++++ 5 files changed, 125 insertions(+), 9 deletions(-) create mode 100644 spp_api_v2_gis/tests/test_gis_published_variables.py diff --git a/spp_api_v2_gis/__manifest__.py b/spp_api_v2_gis/__manifest__.py index 2aad21224..e8616fc42 100644 --- a/spp_api_v2_gis/__manifest__.py +++ b/spp_api_v2_gis/__manifest__.py @@ -2,7 +2,7 @@ { "name": "OpenSPP GIS API", "category": "OpenSPP/Integration", - "version": "19.0.2.0.0", + "version": "19.0.2.0.1", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_api_v2_gis/readme/HISTORY.md b/spp_api_v2_gis/readme/HISTORY.md index 4aaf9afef..ba46a97d0 100644 --- a/spp_api_v2_gis/readme/HISTORY.md +++ b/spp_api_v2_gis/readme/HISTORY.md @@ -1,3 +1,14 @@ +### 19.0.2.0.1 + +- fix(security): restrict spatial/proximity statistics to GIS-published + indicators. Client-supplied ``variables`` were passed straight to the + aggregation service, which resolves names via ``sudo()`` against all + indicators and CEL variables, so a caller with only GIS/statistics read could + request unpublished indicator or raw CEL variable names and receive their + aggregates. Supplied variables are now filtered to the same GIS publication + allowlist (``spp.indicator.is_published_gis``) already used for the default + path; unpublished names are dropped before aggregation. + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_api_v2_gis/services/spatial_query_service.py b/spp_api_v2_gis/services/spatial_query_service.py index 6c49aba85..84397648a 100644 --- a/spp_api_v2_gis/services/spatial_query_service.py +++ b/spp_api_v2_gis/services/spatial_query_service.py @@ -354,14 +354,23 @@ def _compute_via_aggregation_service(self, registrant_ids, variables): # Create an explicit scope for the registrant IDs scope = build_explicit_scope(registrant_ids) - # Determine statistics to compute - statistics_to_compute = variables - if not statistics_to_compute: - # Use GIS-published statistics - # nosemgrep: odoo-sudo-without-context - Statistic = self.env["spp.indicator"].sudo() - gis_stats = Statistic.get_published_for_context("gis") - statistics_to_compute = [stat.name for stat in gis_stats] if gis_stats else None + # Enforce the GIS publication boundary as a positive allowlist: only + # indicators an admin published to GIS may be computed here. Client- + # supplied `variables` are otherwise resolved by the aggregation + # registry via sudo() against ALL indicators and CEL variables, which + # would let a caller with only gis/statistics read pull aggregates for + # names never published to GIS. Restrict both the supplied and default + # paths to the same published set; unknown/unpublished names are dropped + # (matching the existing "unknown variables -> valid response" contract + # and avoiding an existence oracle). + # nosemgrep: odoo-sudo-without-context + Statistic = self.env["spp.indicator"].sudo() + published_gis_names = set(Statistic.get_published_for_context("gis").mapped("name")) + + if variables: + statistics_to_compute = [name for name in variables if name in published_gis_names] + else: + statistics_to_compute = list(published_gis_names) if not statistics_to_compute: return { diff --git a/spp_api_v2_gis/tests/__init__.py b/spp_api_v2_gis/tests/__init__.py index 06c22da07..f8c293e86 100644 --- a/spp_api_v2_gis/tests/__init__.py +++ b/spp_api_v2_gis/tests/__init__.py @@ -9,4 +9,5 @@ from . import test_spatial_query_service from . import test_statistics_endpoint from . import test_batch_query +from . import test_gis_published_variables from . import test_proximity_query diff --git a/spp_api_v2_gis/tests/test_gis_published_variables.py b/spp_api_v2_gis/tests/test_gis_published_variables.py new file mode 100644 index 000000000..5663c548b --- /dev/null +++ b/spp_api_v2_gis/tests/test_gis_published_variables.py @@ -0,0 +1,95 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: GIS spatial stats must honor the GIS publication boundary. + +`request.variables` is client-supplied and flows to the aggregation service, +which resolves names via sudo() against all indicators and all CEL variables. +Only names an admin published to GIS (`spp.indicator.is_published_gis`) may be +computed — supplying an unpublished indicator name, or a raw CEL variable name, +must not return an aggregate for it. +""" + +from odoo.tests.common import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestGisPublishedVariables(TransactionCase): + """Supplied variables are restricted to the GIS-published allowlist.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + Partner = cls.env["res.partner"] + Indicator = cls.env["spp.indicator"] + CelVariable = cls.env["spp.cel.variable"] + + cls.group = Partner.create({"name": "GIS Pub Group", "is_registrant": True, "is_group": True}) + cls.member = Partner.create({"name": "GIS Pub Member", "is_registrant": True, "is_group": False}) + if "spp.group.membership" in cls.env: + cls.env["spp.group.membership"].create({"group": cls.group.id, "individual": cls.member.id}) + cls.registrant_ids = [cls.group.id, cls.member.id] + + def _make_var(name): + return CelVariable.create( + { + "name": name, + "cel_accessor": name, + "source_type": "aggregate", + "aggregate_type": "count", + "aggregate_target": "members", + "aggregate_filter": "true", + "value_type": "number", + "applies_to": "group", + "state": "active", + } + ) + + # Published-to-GIS indicator (allowed). + cls.published_var = _make_var("gis_pub_var") + cls.published = Indicator.create( + { + "name": "gis_published_stat", + "label": "GIS Published Stat", + "variable_id": cls.published_var.id, + "format": "count", + "is_published_gis": True, + } + ) + # Indicator NOT published to GIS (must be denied). + cls.unpublished_var = _make_var("gis_unpub_var") + cls.unpublished = Indicator.create( + { + "name": "gis_unpublished_stat", + "label": "GIS Unpublished Stat", + "variable_id": cls.unpublished_var.id, + "format": "count", + "is_published_gis": False, + } + ) + # Raw CEL variable, never surfaced as a GIS indicator (must be denied). + cls.raw_var = _make_var("gis_raw_cel_var") + + def _service(self): + from ..services.spatial_query_service import SpatialQueryService + + return SpatialQueryService(self.env) + + def _flat_stat_keys(self, result): + return {k for k in result.get("statistics", {}) if k != "_grouped"} + + def test_unpublished_indicator_name_is_not_computed(self): + result = self._service()._compute_statistics(self.registrant_ids, ["gis_unpublished_stat"]) + self.assertNotIn("gis_unpublished_stat", self._flat_stat_keys(result)) + + def test_raw_cel_variable_name_is_not_computed(self): + result = self._service()._compute_statistics(self.registrant_ids, ["gis_raw_cel_var"]) + self.assertNotIn("gis_raw_cel_var", self._flat_stat_keys(result)) + + def test_published_indicator_name_is_computed(self): + result = self._service()._compute_statistics(self.registrant_ids, ["gis_published_stat"]) + self.assertIn("gis_published_stat", self._flat_stat_keys(result)) + + def test_default_path_uses_published_only(self): + result = self._service()._compute_statistics(self.registrant_ids, []) + keys = self._flat_stat_keys(result) + self.assertIn("gis_published_stat", keys) + self.assertNotIn("gis_unpublished_stat", keys) From a4b42f25ea5bd5303589dd45672b066f91a2d3e5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 20:12:37 +0800 Subject: [PATCH 2/3] docs(spp_api_v2_gis): regenerate README/index for 19.0.2.0.1 changelog Applies CI's pinned oca-gen output verbatim for the new HISTORY fragment. --- spp_api_v2_gis/README.rst | 13 ++++++++++ spp_api_v2_gis/static/description/index.html | 27 +++++++++++++++----- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/spp_api_v2_gis/README.rst b/spp_api_v2_gis/README.rst index 961e531d3..f6113bb15 100644 --- a/spp_api_v2_gis/README.rst +++ b/spp_api_v2_gis/README.rst @@ -156,6 +156,19 @@ Dependencies Changelog ========= +19.0.2.0.1 +~~~~~~~~~~ + +- fix(security): restrict spatial/proximity statistics to GIS-published + indicators. Client-supplied ``variables`` were passed straight to the + aggregation service, which resolves names via ``sudo()`` against all + indicators and CEL variables, so a caller with only GIS/statistics + read could request unpublished indicator or raw CEL variable names and + receive their aggregates. Supplied variables are now filtered to the + same GIS publication allowlist (``spp.indicator.is_published_gis``) + already used for the default path; unpublished names are dropped + before aggregation. + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_api_v2_gis/static/description/index.html b/spp_api_v2_gis/static/description/index.html index b7c6f7e27..1a51a22be 100644 --- a/spp_api_v2_gis/static/description/index.html +++ b/spp_api_v2_gis/static/description/index.html @@ -557,24 +557,39 @@

Dependencies

Changelog

-

19.0.2.0.0

+

19.0.2.0.1

+
    +
  • fix(security): restrict spatial/proximity statistics to GIS-published +indicators. Client-supplied variables were passed straight to the +aggregation service, which resolves names via sudo() against all +indicators and CEL variables, so a caller with only GIS/statistics +read could request unpublished indicator or raw CEL variable names and +receive their aggregates. Supplied variables are now filtered to the +same GIS publication allowlist (spp.indicator.is_published_gis) +already used for the default path; unpublished names are dropped +before aggregation.
    • +
+
+
+

19.0.2.0.0

  • Initial migration to OpenSPP2
-

Bug Tracker

+

Bug Tracker

Bugs are tracked on GitHub Issues. In case of trouble, please check there if your issue has already been reported. If you spotted it first, help us to smash it by providing a detailed and welcomed @@ -582,7 +597,7 @@

Bug Tracker

Do not contact contributors directly about support or help with technical issues.

From 004a08151dcc4a9536c5e13e07a3cb1af9d8539e Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 24 Jul 2026 20:50:56 +0800 Subject: [PATCH 3/3] security(gis): address staff-review nits on published-variable allowlist - 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. --- spp_api_v2_gis/services/spatial_query_service.py | 10 ++++++++-- spp_api_v2_gis/tests/test_gis_published_variables.py | 11 +++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/spp_api_v2_gis/services/spatial_query_service.py b/spp_api_v2_gis/services/spatial_query_service.py index 84397648a..52f2b6f1a 100644 --- a/spp_api_v2_gis/services/spatial_query_service.py +++ b/spp_api_v2_gis/services/spatial_query_service.py @@ -365,12 +365,15 @@ def _compute_via_aggregation_service(self, registrant_ids, variables): # and avoiding an existence oracle). # nosemgrep: odoo-sudo-without-context Statistic = self.env["spp.indicator"].sudo() - published_gis_names = set(Statistic.get_published_for_context("gis").mapped("name")) + # Keep the recordset order (spp.indicator _order) for a deterministic + # default result; use the set only for the supplied-path membership test. + published_gis = Statistic.get_published_for_context("gis").mapped("name") + published_gis_names = set(published_gis) if variables: statistics_to_compute = [name for name in variables if name in published_gis_names] else: - statistics_to_compute = list(published_gis_names) + statistics_to_compute = published_gis if not statistics_to_compute: return { @@ -409,6 +412,9 @@ def _convert_aggregation_result(self, agg_result, registrant_ids=None): result = {} grouped_stats = {} + # These names are already restricted to GIS-published indicators by + # _compute_via_aggregation_service, so this metadata lookup only ever + # sees published names (no is_published_gis filter needed here). # nosemgrep: odoo-sudo-without-context Statistic = self.env["spp.indicator"].sudo() statistic_by_name = {stat.name: stat for stat in Statistic.search([("name", "in", list(statistics.keys()))])} diff --git a/spp_api_v2_gis/tests/test_gis_published_variables.py b/spp_api_v2_gis/tests/test_gis_published_variables.py index 5663c548b..134c027ae 100644 --- a/spp_api_v2_gis/tests/test_gis_published_variables.py +++ b/spp_api_v2_gis/tests/test_gis_published_variables.py @@ -93,3 +93,14 @@ def test_default_path_uses_published_only(self): keys = self._flat_stat_keys(result) self.assertIn("gis_published_stat", keys) self.assertNotIn("gis_unpublished_stat", keys) + + def test_mixed_list_keeps_published_drops_unpublished(self): + # Smuggling an unpublished name alongside a published one must not leak + # the unpublished aggregate; the published one is still returned. + result = self._service()._compute_statistics( + self.registrant_ids, ["gis_published_stat", "gis_unpublished_stat", "gis_raw_cel_var"] + ) + keys = self._flat_stat_keys(result) + self.assertIn("gis_published_stat", keys) + self.assertNotIn("gis_unpublished_stat", keys) + self.assertNotIn("gis_raw_cel_var", keys)