security(dci): fail closed on unresolved search sender + cap page_size (#260)

* security(dci): fail closed on unresolved sender (search) + cap page_size

DCI search reached an unscoped sudo res.partner search whenever no active
sender was resolved: with sender=None/False, DCIConsentAdapter disengages
(build_consented_domain returns the base domain unchanged), so full PII is
returned. Three routes funnel into this: the sync search route, the async
search route, and bulk upload (all via DCISocialSearchService /
transaction.process_async_search). page_size was also unbounded (schema only
enforces > 0), enabling whole-registry enumeration in one request.

Fail closed across the shared sink and the request paths:
- routers/search.py: reject (403) when the verified sender doesn't resolve to
  an active registered sender; never run with sender=None (removed `or None`).
- middleware/signature.py: verify_dci_signature requires an active sender, so a
  deactivated sender cannot authenticate (keeps verify and resolution in sync).
- models/transaction.py: process_async_search refuses (rejected, no search)
  when the transaction has no resolved sender -- the shared sink guard that
  protects the async and bulk paths even before their route-level checks.
- routers/async_router.py: async_search rejects (403) an unresolved sender
  instead of persisting sender_id=False and queuing an unscoped job.
- search_service.py: clamp page_size to dci.max_page_size (default 100).

The report's literal claim (service built without a sender) was already fixed
by 0944b7f; this closes the residual fail-open resolution + unbounded
page_size. Separately tracked follow-ups: bulk_upload accepts an UNSIGNED,
caller-asserted sender_id (impersonation -- needs signature binding), and the
dead consent domain (status=="active" matches nothing for require_consent
senders; functional, fail-closed).

Tests: unresolved/inactive sender -> 403 (sync + async) / 401 (signature);
sink refuses sender-less transaction; page_size clamped. spp_dci_server
325/325, spp_dci_server_social 75/75.

* fix(dci): address CI/review on the search consent hardening

- pre-commit (semgrep): ruff-format had wrapped the signature.py sudo lookup
  across lines, pushing the nosemgrep off the matched `.sudo()` line. Keep
  `.sudo()` on a single line with the nosemgrep directly above (and likewise
  for the new search_service config read). Semgrep passes locally.
- transaction.py (Gemini security-high): a Many2one still dereferences a
  DEACTIVATED record, and process_async_search runs asynchronously, so a sender
  deactivated after the transaction was created would slip past `if not
  self.sender_id`. Check `not self.sender_id.active` too (fail closed).
- search_service.py (Gemini medium): a misconfigured dci.max_page_size
  (empty/non-integer) would crash int(); wrap in try/except, fall back to 100.

Tests: add a sink test for a deactivated sender -> refused.
spp_dci_server 326/326, spp_dci_server_social 75/75.

* security(dci): treat non-positive max_page_size as default, not no-cap

A non-positive dci.max_page_size (0 or negative) previously skipped the
page_size clamp entirely, silently disabling the cap and letting a client
pull an arbitrarily large page. Treat such a misconfiguration as the
default (100) so the cap can never be turned off by accident.

Addresses staff-review nit on PR #260.

Author: gonzalesedwin1123

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}",

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)

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

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",

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

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,
                     )
                 )

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

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

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(