diff --git a/changelog.d/tsk-g5xc6k-device-pair-security.md b/changelog.d/tsk-g5xc6k-device-pair-security.md new file mode 100644 index 000000000..94c78c8d6 --- /dev/null +++ b/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. diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index bfc92bdc1..9fb7a461f 100644 --- a/docs/agent-coordination.md +++ b/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; diff --git a/tests/routes/test_device_pair_security.py b/tests/routes/test_device_pair_security.py new file mode 100644 index 000000000..75927afa0 --- /dev/null +++ b/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] diff --git a/tinyagentos/device_pair_requests_store.py b/tinyagentos/device_pair_requests_store.py index dac7da9e6..050c5bf2f 100644 --- a/tinyagentos/device_pair_requests_store.py +++ b/tinyagentos/device_pair_requests_store.py @@ -16,6 +16,12 @@ 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 @@ -23,6 +29,7 @@ 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: diff --git a/tinyagentos/routes/device_pair_requests.py b/tinyagentos/routes/device_pair_requests.py index b47908330..fe0890b37 100644 --- a/tinyagentos/routes/device_pair_requests.py +++ b/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