From 501f1a9ba46381c2879c13917b2263a5e61efe44 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Sun, 16 Aug 2026 22:00:54 +0000 Subject: [PATCH 1/3] fix(device-pair): enforce pending cap atomically and reject requests when no admin exists - Add asyncio.Lock to DevicePairRequestsStore so count_pending() and create() are serialized, preventing concurrent requests from bypassing _PENDING_CAP. - Return 409 Conflict from POST /api/devices/pair-requests when no instance admin exists, instead of silently creating an unapprovable request. - Add max_length validation to CreatePairRequest.display_name, matching the existing RegisterIn constraint. - Add security regression tests covering the cap race, missing-admin case, forged display-name impersonation, and display-name length limit. - Update docs/agent-coordination.md to document the new 409 behaviour. Docs-Reviewed: device pair-request route contract changed (409 on missing admin, atomic cap enforcement), agent-coordination doc updated to match. Stale findings (no code change needed): - Client-supplied device identity: current code generates device_id and scoped_token server-side, so a forged display_name cannot impersonate an approved device (verified by test_forged_display_name_cannot_impersonate_approved_device). --- .../tsk-g5xc6k-device-pair-security.md | 3 + docs/agent-coordination.md | 3 + tests/routes/test_device_pair_security.py | 129 ++++++++++++++++++ tinyagentos/device_pair_requests_store.py | 5 + tinyagentos/routes/device_pair_requests.py | 56 +++++--- 5 files changed, 180 insertions(+), 16 deletions(-) create mode 100644 changelog.d/tsk-g5xc6k-device-pair-security.md create mode 100644 tests/routes/test_device_pair_security.py 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..275ff7cd8 --- /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"}, + ) + + results = await asyncio.gather(*[_create() for _ in range(5)], return_exceptions=True) + statuses = [ + r.status_code for r in results if not isinstance(r, Exception) + ] + successes = [s for s in statuses if s == 200] + assert len(successes) <= 1, ( + f"cap race: {len(successes)} requests succeeded past the cap" + ) + + 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 != 200, ( + "pair request must not succeed when no admin exists to approve it" + ) + + 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..003025236 100644 --- a/tinyagentos/device_pair_requests_store.py +++ b/tinyagentos/device_pair_requests_store.py @@ -23,6 +23,7 @@ from typing import Optional import aiosqlite +import asyncio from tinyagentos.base_store import BaseStore @@ -98,6 +99,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..198379b4f 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,50 @@ 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, - ) + lock = getattr(store, "_create_lock", None) + if lock is not None: + async with 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, + ) + else: + 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 From 1e735b0d2a41e91169265a858a1b737cb1963ac5 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Sun, 16 Aug 2026 23:16:00 +0000 Subject: [PATCH 2/3] review fixes: fail-loud cap lock, two-sided race assert, exact 409 The getattr fallback silently degraded to the exact count-then-create race this PR removes; the lock is now mandatory. The race test asserted only the refusing half (an all-crash run passed vacuously via return_exceptions) and the no-admin test accepted any non-200; both now assert the exact expected statuses. --- tests/routes/test_device_pair_security.py | 18 ++++++++--------- tinyagentos/routes/device_pair_requests.py | 23 ++++------------------ 2 files changed, 13 insertions(+), 28 deletions(-) diff --git a/tests/routes/test_device_pair_security.py b/tests/routes/test_device_pair_security.py index 275ff7cd8..75927afa0 100644 --- a/tests/routes/test_device_pair_security.py +++ b/tests/routes/test_device_pair_security.py @@ -50,13 +50,12 @@ async def _create(): json={"platform": "ios", "display_name": "racer"}, ) - results = await asyncio.gather(*[_create() for _ in range(5)], return_exceptions=True) - statuses = [ - r.status_code for r in results if not isinstance(r, Exception) - ] - successes = [s for s in statuses if s == 200] - assert len(successes) <= 1, ( - f"cap race: {len(successes)} requests succeeded past the cap" + # 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): @@ -72,8 +71,9 @@ async def test_pair_request_requires_admin_presence(self, client, app): "/api/devices/pair-requests", json={"platform": "ios", "display_name": "Orphan"}, ) - assert resp.status_code != 200, ( - "pair request must not succeed when no admin exists to approve it" + 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( diff --git a/tinyagentos/routes/device_pair_requests.py b/tinyagentos/routes/device_pair_requests.py index 198379b4f..fe0890b37 100644 --- a/tinyagentos/routes/device_pair_requests.py +++ b/tinyagentos/routes/device_pair_requests.py @@ -118,25 +118,10 @@ async def create_pair_request(request: Request, body: CreatePairRequest): requester_ip = _requester_ip(request) display = (body.display_name or "").strip() or body.platform - lock = getattr(store, "_create_lock", None) - if lock is not None: - async with 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, - ) - else: + # 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( From 695744b39659c8232cef2455970f12994350fbe8 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Mon, 17 Aug 2026 02:20:13 +0000 Subject: [PATCH 3/3] document the single-process scope of the pending-cap lock The in-process _create_lock is sound only because taOS serves from one process (uvicorn.run with an app object; no worker forking). Name that assumption and the migration path (transactional insert) if multi-process serving ever arrives. --- tinyagentos/device_pair_requests_store.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tinyagentos/device_pair_requests_store.py b/tinyagentos/device_pair_requests_store.py index 003025236..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