Verify-then-fix: device pairing/blocking security findings from #2233/#2238 (unauthenticated cap race, 200-on-no-admin, client-supplied device identity)

changelog.d/tsk-g5xc6k-device-pair-security.md

@@ -0,0 +1,3 @@
+### Fixed
+- Device pair-request creation: enforce the pending cap atomically so concurrent requests cannot bypass it.
+- Device pair-request creation: return 409 Conflict when no instance admin exists, instead of silently creating an unapprovable request.

docs/agent-coordination.md

@@ -600,6 +600,9 @@ each other's work.
 Route module `tinyagentos/routes/device_pair_requests.py`:
 
 - `POST /api/devices/pair-requests` creates a pairing request for a device.
+  Returns `409 Conflict` when no instance admin exists (the request can never be
+  approved). The pending cap is enforced atomically so concurrent requests cannot
+  exceed it.
 - `GET /api/devices/pair-requests/{pair_request_id}` returns its status.
 
 Approval or denial of a pair request is surfaced to the user through the Decisions app;

tests/routes/test_device_pair_security.py

@@ -0,0 +1,129 @@
+"""Security regression tests for device pairing and blocking (audit #2233/#2238).
+
+Red-first: each test is written against the post-merge code and must FAIL
+before the corresponding fix lands, then PASS after.
+"""
+import asyncio
+
+import pytest
+
+
+@pytest.mark.asyncio
+class TestDevicePairSecurity:
+    async def test_concurrent_creates_cannot_bypass_pending_cap(self, client, app):
+        """FINDING 1 (unauthenticated cap race): count_pending() and create()
+        are not atomic. Concurrent requests can exceed _PENDING_CAP."""
+        store = app.state.device_pair_requests
+        await store._db.execute("DELETE FROM device_pair_requests")
+        await store._db.commit()
+
+        await store.create(
+            platform="ios",
+            display_name="filler-1",
+            verify_code="123456",
+            requester_ip="10.0.0.1",
+        )
+        await store.create(
+            platform="ios",
+            display_name="filler-2",
+            verify_code="123456",
+            requester_ip="10.0.0.1",
+        )
+        await store.create(
+            platform="ios",
+            display_name="filler-3",
+            verify_code="123456",
+            requester_ip="10.0.0.1",
+        )
+        await store.create(
+            platform="ios",
+            display_name="filler-4",
+            verify_code="123456",
+            requester_ip="10.0.0.1",
+        )
+        pending_before = await store.count_pending()
+        assert pending_before == 4
+
+        async def _create():
+            return await client.post(
+                "/api/devices/pair-requests",
+                json={"platform": "ios", "display_name": "racer"},
+            )
+
+        # No return_exceptions: a server-side crash must fail the test, not
+        # empty the status list into a vacuous pass.
+        results = await asyncio.gather(*[_create() for _ in range(5)])
+        statuses = sorted(r.status_code for r in results)
+        assert statuses == [200, 429, 429, 429, 429], (
+            f"cap race: expected exactly one success at the cap, got {statuses}"
+        )
+
+    async def test_pair_request_requires_admin_presence(self, client, app):
+        """FINDING 2 (200-on-no-admin): when no admin exists, create_pair_request
+        must not return 200 because the request can never be approved."""
+        auth = app.state.auth
+        data = auth._read_users()
+        for u in data.get("users", []):
+            u["is_admin"] = False
+        auth._write_users(data)
+
+        resp = await client.post(
+            "/api/devices/pair-requests",
+            json={"platform": "ios", "display_name": "Orphan"},
+        )
+        assert resp.status_code == 409, (
+            "pair request must be rejected with 409 when no admin exists to "
+            f"approve it, got {resp.status_code}"
+        )
+
+    async def test_forged_display_name_cannot_impersonate_approved_device(
+        self, client, app
+    ):
+        """FINDING 3 (client-supplied device identity): a client must not be
+        able to forge identity attributes that cause the approved device to
+        impersonate an existing device."""
+        existing = (
+            await client.post(
+                "/api/devices/register",
+                json={"platform": "ios", "display_name": "Real Device"},
+            )
+        ).json()
+        existing_id = existing["device_id"]
+
+        body = await client.post(
+            "/api/devices/pair-requests",
+            json={"platform": "ios", "display_name": existing["display_name"]},
+        )
+        assert body.status_code == 200
+        pid = body.json()["pair_request_id"]
+
+        decision = await self._pairing_decision_for(client, pid)
+        resp = await client.post(
+            f"/api/decisions/{decision['id']}/answer", json={"value": "approve"}
+        )
+        assert resp.status_code == 200
+
+        poll = (await client.get(f"/api/devices/pair-requests/{pid}")).json()
+        assert poll["status"] == "accepted"
+        new_id = poll["device"]["device_id"]
+        assert new_id != existing_id, (
+            "forged display_name must not reuse an existing device_id"
+        )
+
+    async def test_display_name_respects_max_length(self, client):
+        """CreatePairRequest must reject over-length display_name."""
+        resp = await client.post(
+            "/api/devices/pair-requests",
+            json={"platform": "ios", "display_name": "x" * 201},
+        )
+        assert resp.status_code == 422
+
+    async def _pairing_decision_for(self, client, pair_request_id):
+        items = (await client.get("/api/decisions")).json()["items"]
+        matches = [
+            d for d in items
+            if (d.get("metadata") or {}).get("kind") == "device_pairing"
+            and (d.get("metadata") or {}).get("pair_request_id") == pair_request_id
+        ]
+        assert len(matches) == 1, f"expected exactly one pairing decision, got {len(matches)}"
+        return matches[0]

tinyagentos/device_pair_requests_store.py

@@ -16,13 +16,20 @@
 the Decision text can display it for the approving user, and is NEVER returned
 by ``get``/poll -- the route layer strips it. It is never server-checked and no
 endpoint accepts it as input.
+
+Concurrency scope: the pending-cap check-then-insert is serialized by an
+in-process lock (``_create_lock``), which is sound because taOS serves from a
+single process (``uvicorn.run`` with an app object -- no worker forking). If
+multi-process serving is ever introduced, the cap check must move into a
+single transactional INSERT ... WHERE (SELECT COUNT ...) statement.
 """
 
 import uuid
 from datetime import datetime, timedelta, timezone
 from typing import Optional
 
 import aiosqlite
+import asyncio
 
 from tinyagentos.base_store import BaseStore
 
@@ -98,6 +105,10 @@ class DevicePairRequestsStore(BaseStore):
 
     SCHEMA = SCHEMA
 
+    def __init__(self, db_path):
+        super().__init__(db_path)
+        self._create_lock = asyncio.Lock()
+
     async def init(self) -> None:
         await super().init()
         if self._db is not None:

tinyagentos/routes/device_pair_requests.py

@@ -31,7 +31,7 @@
 import secrets
 
 from fastapi import APIRouter, HTTPException, Request
-from pydantic import BaseModel
+from pydantic import BaseModel, Field
 
 from tinyagentos.device_pair_requests_store import (
     DevicePairRequestsStore,
@@ -45,11 +45,12 @@
 # F5: the store does not validate the platform, so the whitelist lives here.
 _VALID_PLATFORMS = frozenset({"ios", "watchos", "android"})
 _VERIFY_CODE_DIGITS = 6
+_MAX_DISPLAY_NAME = 200
 
 
 class CreatePairRequest(BaseModel):
     platform: str
-    display_name: str = ""
+    display_name: str = Field(default="", max_length=_MAX_DISPLAY_NAME)
 
 
 def _get_pair_requests_store(request: Request) -> DevicePairRequestsStore:
@@ -107,27 +108,35 @@ async def create_pair_request(request: Request, body: CreatePairRequest):
 
     store = _get_pair_requests_store(request)
 
-    # F4: cap TOTAL pending (not per-IP) -- mirrors the agent auth-request cap.
-    pending_count = await store.count_pending()
-    if pending_count >= _PENDING_CAP:
+    if not _admin_user_id(request):
         raise HTTPException(
-            status_code=429,
-            detail=(
-                f"too many pending pair requests ({pending_count} pending; "
-                f"resolve existing requests first)"
-            ),
+            status_code=409,
+            detail="no admin exists to approve pairing requests",
         )
 
     verify_code = _generate_verify_code()
     requester_ip = _requester_ip(request)
     display = (body.display_name or "").strip() or body.platform
 
-    record = await store.create(
-        platform=body.platform,
-        display_name=display,
-        verify_code=verify_code,
-        requester_ip=requester_ip,
-    )
+    # The lock is what makes the cap atomic; a store without it must fail
+    # loudly rather than fall back to the racy count-then-create this fix
+    # removed.
+    async with store._create_lock:
+        pending_count = await store.count_pending()
+        if pending_count >= _PENDING_CAP:
+            raise HTTPException(
+                status_code=429,
+                detail=(
+                    f"too many pending pair requests ({pending_count} pending; "
+                    f"resolve existing requests first)"
+                ),
+            )
+        record = await store.create(
+            platform=body.platform,
+            display_name=display,
+            verify_code=verify_code,
+            requester_ip=requester_ip,
+        )
     pair_request_id = record["id"]
 
     # Raise a Decision to the instance admin.  The metadata binds the approval to