diff --git a/spp_dci_server/middleware/signature.py b/spp_dci_server/middleware/signature.py index 54a6bb04..82032fef 100644 --- a/spp_dci_server/middleware/signature.py +++ b/spp_dci_server/middleware/signature.py @@ -177,12 +177,15 @@ async def verify_dci_signature( error_code="err.signature.missing", ) - # Look up sender in registry + # Look up sender in registry. Require an ACTIVE record: a deactivated + # sender must not authenticate (and downstream consent resolution also + # requires active, so accepting an inactive sender here would desync). # nosemgrep: odoo-sudo-without-context - sender_registry = env["spp.dci.sender.registry"].sudo().search([("sender_id", "=", sender_id)], limit=1) + SenderRegistry = env["spp.dci.sender.registry"].sudo() + sender_registry = SenderRegistry.search([("sender_id", "=", sender_id), ("active", "=", True)], limit=1) if not sender_registry: - _logger.warning("Unknown sender_id in DCI request: %s", sender_id) + _logger.warning("Unknown sender_id (inactive or unregistered) in DCI request: %s", sender_id) raise DCIHTTPException( status_code=status.HTTP_401_UNAUTHORIZED, error_message=f"Unknown sender_id: {sender_id}", diff --git a/spp_dci_server/models/transaction.py b/spp_dci_server/models/transaction.py index 95c22a4c..78fe24bc 100644 --- a/spp_dci_server/models/transaction.py +++ b/spp_dci_server/models/transaction.py @@ -210,6 +210,23 @@ def process_async_search(self): self.ensure_one() self.state = "processing" + # Fail closed: a search with no resolved (or no longer active) sender + # would disengage consent filtering (DCIConsentAdapter has no sender) + # and return unscoped PII. A Many2one still dereferences a deactivated + # record, and this job runs asynchronously, so the sender may have been + # deactivated after the transaction was created -- check active too. + # Every caller that reaches this sink (sync search is guarded at the + # route; async/bulk reach here) must have an active sender resolved. + if not self.sender_id or not self.sender_id.active: + self.state = "rejected" + self.error_code = SearchStatusReasonCode.SEARCH_CRITERIA_INVALID.value + self.error_message = "No active registered sender resolved for this transaction; search refused." + _logger.warning( + "DCI async search refused: transaction %s has no resolved sender", + self.transaction_id, + ) + return + try: # Parse request request_data = json.loads(self.request_payload) diff --git a/spp_dci_server/routers/async_router.py b/spp_dci_server/routers/async_router.py index 4de334a6..4bc2ecbf 100644 --- a/spp_dci_server/routers/async_router.py +++ b/spp_dci_server/routers/async_router.py @@ -146,10 +146,21 @@ async def async_search( ) from e # SECURITY: Use verified_sender_id from signature verification - # instead of reading from envelope.header to prevent spoofing - # Use sudo() for API access - authentication is handled by signature verification + # instead of reading from envelope.header to prevent spoofing. + # Fail closed: require an ACTIVE registered sender. Persisting + # sender_id=False here would later run the search with no sender, which + # disengages consent filtering and returns unscoped PII. # nosemgrep: odoo-sudo-without-context — DCI protocol handler with JWT/signature verification - sender = env["spp.dci.sender.registry"].sudo().search([("sender_id", "=", verified_sender_id)], limit=1) + sender = env["spp.dci.sender.registry"].sudo().get_by_sender_id(verified_sender_id) + if not sender: + _logger.warning( + "Async search rejected: sender %s is not an active registered sender", + verified_sender_id, + ) + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Sender is not an active registered DCI sender", + ) # Get callback URI from header callback_uri = envelope.header.sender_uri @@ -171,7 +182,7 @@ async def async_search( "correlation_id": correlation_id, "action": "search", "reg_type": "SOCIAL_REGISTRY", # Default, could be derived from request - "sender_id": sender.id if sender else False, + "sender_id": sender.id, "sender_uri": verified_sender_id, "callback_uri": callback_uri, "request_payload": json.dumps(envelope.model_dump(mode="json")), @@ -183,7 +194,7 @@ async def async_search( _logger.info( "Created async search transaction %s for sender_id=%s", transaction.transaction_id, - sender.id if sender else "unknown", + sender.id, ) # Queue the search job with job_worker diff --git a/spp_dci_server/routers/search.py b/spp_dci_server/routers/search.py index 1c031fb2..e597db88 100644 --- a/spp_dci_server/routers/search.py +++ b/spp_dci_server/routers/search.py @@ -99,19 +99,31 @@ async def search_registry( len(search_request.search_request), ) + # Resolve the authenticated sender to an ACTIVE registered sender record + # before returning any data. The consent adapter disengages (no filtering) + # when sender is None, so we must fail closed here rather than run a + # consent-bearing search with sender=None and leak unscoped PII. + # sudo: technical lookup of an already-verified sender id; the endpoint + # user (often public) has no read access to the registry. + SenderRegistry = env["spp.dci.sender.registry"].sudo() # nosemgrep: odoo-sudo-without-context + sender_registry = SenderRegistry.get_by_sender_id(verified_sender_id) + if not sender_registry: + _logger.warning( + "DCI search rejected: sender %s is not an active registered sender", + verified_sender_id, + ) + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Sender is not an active registered DCI sender", + ) + # Execute search using DCISocialSearchService try: from odoo.addons.spp_dci_server_social.services.search_service import ( DCISocialSearchService, ) - # Hand the verified sender to the service so consent filtering - # engages - the consent adapter disengages when sender is None. - # sudo: technical lookup of an already-verified sender id; the - # endpoint user (often public) has no read access to the registry. - SenderRegistry = env["spp.dci.sender.registry"].sudo() # nosemgrep: odoo-sudo-without-context - sender_registry = SenderRegistry.get_by_sender_id(verified_sender_id) - search_service = DCISocialSearchService(env, sender_registry=sender_registry or None) + search_service = DCISocialSearchService(env, sender_registry=sender_registry) search_response = search_service.execute_search(search_request) _logger.info( "DCI search completed - transaction_id: %s, items: %d", diff --git a/spp_dci_server/tests/test_async_router.py b/spp_dci_server/tests/test_async_router.py index a3d0164a..4c2eeae4 100644 --- a/spp_dci_server/tests/test_async_router.py +++ b/spp_dci_server/tests/test_async_router.py @@ -135,6 +135,64 @@ def test_unexpected_error_returns_500(self): self._call(envelope) self.assertEqual(ctx.exception.status_code, 500) + def test_unresolved_sender_rejected(self): + """A verified sender that doesn't resolve to an active registry record + must be rejected (403), not queued with sender_id=False (which would run + the background search unscoped and leak unscoped PII to the callback).""" + envelope = self._build_envelope() + with self.assertRaises(HTTPException) as ctx: + _run( + self.async_router.async_search( + envelope, + self.env, + _bearer_token="t", + verified_sender_id="ghost.sender.unregistered", + _rate_limit_check=None, + response=Response(), + ) + ) + self.assertEqual(ctx.exception.status_code, 403) + + def test_process_async_search_without_sender_is_refused(self): + """The shared async sink must refuse a transaction with no resolved + sender rather than run a consent-disengaged (unscoped) search. This + protects every caller of the sink (async + bulk).""" + txn = self.Transaction.sudo().create( + { + "transaction_id": f"txn-nosender-{uuid.uuid4()}", + "message_id": f"msg-{uuid.uuid4()}", + "correlation_id": str(uuid.uuid4()), + "action": "search", + "reg_type": "SOCIAL_REGISTRY", + "sender_id": False, + "request_payload": json.dumps({"message": {"transaction_id": "t", "search_request": []}}), + "state": "received", + } + ) + txn.process_async_search() + self.assertEqual(txn.state, "rejected") + self.assertIn("sender", (txn.error_message or "").lower()) + + def test_process_async_search_with_inactive_sender_is_refused(self): + """A sender deactivated after the transaction was created must also be + refused: a Many2one still dereferences the deactivated record, so a + plain truthiness check would let the unscoped search through.""" + txn = self.Transaction.sudo().create( + { + "transaction_id": f"txn-inactive-{uuid.uuid4()}", + "message_id": f"msg-{uuid.uuid4()}", + "correlation_id": str(uuid.uuid4()), + "action": "search", + "reg_type": "SOCIAL_REGISTRY", + "sender_id": self.test_sender.id, + "request_payload": json.dumps({"message": {"transaction_id": "t", "search_request": []}}), + "state": "received", + } + ) + self.test_sender.active = False + txn.process_async_search() + self.assertEqual(txn.state, "rejected") + # ============================================================================= # /subscribe diff --git a/spp_dci_server/tests/test_search_router.py b/spp_dci_server/tests/test_search_router.py index c4ba7e52..971e6d9f 100644 --- a/spp_dci_server/tests/test_search_router.py +++ b/spp_dci_server/tests/test_search_router.py @@ -83,7 +83,10 @@ def _build_response(self, statuses=("succ",)): search_response=items, ) - def _call(self, envelope, search_response, sender_id="external.test.gov"): + def _call(self, envelope, search_response, sender_id=None): + # Default to the active test sender so the route's fail-closed sender + # resolution passes; tests probing rejection pass an explicit bad id. + sender_id = sender_id or self.test_sender.sender_id with patch( "odoo.addons.spp_dci_server_social.services.search_service.DCISocialSearchService" ) as mock_service_cls: @@ -212,6 +215,40 @@ def test_sync_search_sender_lookup_survives_low_privilege_endpoint_user(self): ) self.assertEqual(result.header.status, "succ") + def test_unresolved_sender_is_rejected(self): + """A verified sender_id that does not resolve to an active registry + record must be rejected (403), never run with sender=None (which would + disengage consent filtering and return unscoped PII).""" + envelope = self._build_envelope() + with self.assertRaises(HTTPException) as ctx: + _run( + self.search_registry( + envelope, + self.env, + _bearer_token="t", + verified_sender_id="ghost-sender-not-registered", + _rate_limit_check=None, + ) + ) + self.assertEqual(ctx.exception.status_code, 403) + + def test_inactive_sender_is_rejected(self): + """A deactivated sender (which can still pass signature lookup if that + lookup omits the active filter) must not get data via the search path.""" + self.test_sender.active = False + envelope = self._build_envelope() + with self.assertRaises(HTTPException) as ctx: + _run( + self.search_registry( + envelope, + self.env, + _bearer_token="t", + verified_sender_id=self.test_sender.sender_id, + _rate_limit_check=None, + ) + ) + self.assertEqual(ctx.exception.status_code, 403) + # --- service errors ------------------------------------------------------- def test_search_service_exception_rejects_all_items(self): @@ -225,7 +262,7 @@ def test_search_service_exception_rejects_all_items(self): envelope, self.env, _bearer_token="t", - verified_sender_id="s", + verified_sender_id=self.test_sender.sender_id, _rate_limit_check=None, ) ) @@ -251,7 +288,7 @@ def test_social_module_import_error_falls_back_to_rjct(self): envelope, self.env, _bearer_token="t", - verified_sender_id="s", + verified_sender_id=self.test_sender.sender_id, _rate_limit_check=None, ) ) @@ -291,7 +328,7 @@ def test_signing_failure_is_tolerated(self): envelope, self.env, _bearer_token="t", - verified_sender_id="s", + verified_sender_id=self.test_sender.sender_id, _rate_limit_check=None, ) ) @@ -320,7 +357,7 @@ def test_unexpected_error_returns_500(self): envelope, self.env, _bearer_token="t", - verified_sender_id="s", + verified_sender_id=self.test_sender.sender_id, _rate_limit_check=None, ) ) diff --git a/spp_dci_server/tests/test_signature_middleware.py b/spp_dci_server/tests/test_signature_middleware.py index 4cccecc8..5c170987 100644 --- a/spp_dci_server/tests/test_signature_middleware.py +++ b/spp_dci_server/tests/test_signature_middleware.py @@ -86,6 +86,23 @@ def test_verify_missing_sender(self): self.assertEqual(exc.status_code, 401, "Should return 401 Unauthorized") self.assertIn("unknown sender", exc.detail.lower()) + def test_verify_inactive_sender_rejected(self): + """A deactivated sender must not authenticate, even with a valid + signature (the registry lookup requires active=True).""" + self.test_sender.active = False + envelope_data = self.create_signed_envelope( + sender_id=self.test_sender_id, + receiver_id="registry.test.gov", + action="search", + ) + + import asyncio + + with self.assertRaises(HTTPException) as context: + asyncio.run(self._verify_signature(envelope_data)) + + self.assertEqual(context.exception.status_code, 401, "Inactive sender should be rejected") + def test_verify_invalid_signature(self): """Test that bad signature returns 401 Unauthorized.""" # Create a properly signed envelope diff --git a/spp_dci_server_social/services/search_service.py b/spp_dci_server_social/services/search_service.py index 01980b17..4dd13aed 100644 --- a/spp_dci_server_social/services/search_service.py +++ b/spp_dci_server_social/services/search_service.py @@ -218,8 +218,20 @@ def _process_search_item(self, search_req: SearchRequestItem) -> SearchResponseI # Apply consent filtering if available domain = self._apply_consent_filter(domain) - # Get pagination parameters + # Get pagination parameters. Cap page_size so a single request cannot + # pull/enumerate the whole registry (the schema only enforces > 0). page_size = criteria.pagination.page_size if criteria.pagination else 20 + # nosemgrep: odoo-sudo-without-context + config = self.env["ir.config_parameter"].sudo() + try: + max_page_size = int(config.get_param("dci.max_page_size", 100)) + except (TypeError, ValueError): + max_page_size = 100 + # A non-positive value is a misconfiguration, not "no limit": fall back + # to the default so the cap can never be silently disabled. + if max_page_size <= 0: + max_page_size = 100 + page_size = min(page_size, max_page_size) page_number = criteria.pagination.page_number if criteria.pagination else 1 # OpenSPP extension: cursor-based pagination diff --git a/spp_dci_server_social/tests/test_search_service.py b/spp_dci_server_social/tests/test_search_service.py index d89d1bdb..1ff2d262 100644 --- a/spp_dci_server_social/tests/test_search_service.py +++ b/spp_dci_server_social/tests/test_search_service.py @@ -491,6 +491,63 @@ def test_search_pagination(self): self.assertLessEqual(len(response_item.data.reg_records), 2) self.assertGreaterEqual(response_item.pagination.total_count, 3) # At least our 3 individuals + def test_search_caps_page_size(self): + """page_size is clamped to dci.max_page_size so a single request cannot + enumerate the whole registry (the schema only enforces page_size > 0).""" + self.env["ir.config_parameter"].sudo().set_param("dci.max_page_size", "1") + criteria = SearchCriteria( + reg_type="SOCIAL_REGISTRY", + reg_event_type="ACTIVE", + query_type="expression", + query={"seq": []}, # match all registrants + pagination=PaginationRequest(page_size=1000, page_number=1), + ) + search_req = SearchRequestItem( + reference_id="test-ref-cap", + timestamp=datetime.now(UTC), + search_criteria=criteria, + ) + request = SearchRequest(transaction_id="test-txn-cap", search_request=[search_req]) + self.env.user.write({"group_ids": [(4, self.env.ref("spp_registry.group_registry_viewer").id)]}) + + response = self.search_service.execute_search(request) + + item = response.search_response[0] + self.assertEqual(item.status, "succ") + # Returned page and reported page_size are clamped to the cap (1), + # even though there are >= 3 matching registrants. + self.assertLessEqual(len(item.data.reg_records), 1) + self.assertEqual(item.pagination.page_size, 1) + self.assertGreaterEqual(item.pagination.total_count, 3) + + def test_search_non_positive_cap_falls_back_to_default(self): + """A non-positive dci.max_page_size (0 or negative) must NOT disable the + cap. Such a misconfiguration should fall back to the default (100), not + leave page_size unbounded at whatever the client requested.""" + self.env["ir.config_parameter"].sudo().set_param("dci.max_page_size", "0") + criteria = SearchCriteria( + reg_type="SOCIAL_REGISTRY", + reg_event_type="ACTIVE", + query_type="expression", + query={"seq": []}, # match all registrants + pagination=PaginationRequest(page_size=1000, page_number=1), + ) + search_req = SearchRequestItem( + reference_id="test-ref-cap0", + timestamp=datetime.now(UTC), + search_criteria=criteria, + ) + request = SearchRequest(transaction_id="test-txn-cap0", search_request=[search_req]) + self.env.user.write({"group_ids": [(4, self.env.ref("spp_registry.group_registry_viewer").id)]}) + + response = self.search_service.execute_search(request) + + item = response.search_response[0] + self.assertEqual(item.status, "succ") + # The client asked for 1000; a 0 cap must clamp to the default 100, + # never honor the unbounded request. + self.assertEqual(item.pagination.page_size, 100) + def test_search_pagination_second_page(self): """Test retrieving second page of results.""" criteria = SearchCriteria(