diff --git a/.env.example b/.env.example index 83a214e30..1b0e74131 100644 --- a/.env.example +++ b/.env.example @@ -31,6 +31,22 @@ XAI_API_KEY=your-xai-api-key # SOS_TOOLS_PORT=6063 # SOS_IDENTITY_PORT=6064 +# ============================================ +# REQUIRED for the Squad service — system-tier auth +# ============================================ + +# Bearer token that authenticates as the SYSTEM identity (is_system=True, +# unrestricted cross-tenant access) against the Squad service. Used by +# /auth/verify, /auth/revoke, /api-keys, and the auth CLI's revoke path. +# +# unset = system tier DISABLED (fail closed). Do not leave this blank in +# any deployed environment — sos/services/squad/auth.py only grants +# system-tier access when this var is both set AND matched via a +# constant-time compare; an unset var used to be silently treated as an +# empty-string token that matched an empty/no Authorization header (P0-A, +# sos-205-47f5f8c2 gate-3). Generate with e.g. `openssl rand -hex 32`. +SOS_SYSTEM_TOKEN= + # ============================================ # OPTIONAL - Agent Configuration # ============================================ diff --git a/docker-compose.yml b/docker-compose.yml index f594b0d7f..63f3fe223 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -197,6 +197,10 @@ services: - .env environment: - SOS_REDIS_URL=redis://:${REDIS_PASSWORD:-changeme}@redis:6379/0 + # P0-A fix (sos-205-47f5f8c2 gate-3): fails the compose run rather than + # silently booting with system-tier auth disabled (fail-closed) or, + # pre-fix, fail-OPEN to an unauthenticated system:sos identity. + - SOS_SYSTEM_TOKEN=${SOS_SYSTEM_TOKEN:?set explicitly} depends_on: redis: condition: service_healthy diff --git a/sos/kernel/auth.py b/sos/kernel/auth.py index cc3b57b88..7ae3a59bb 100644 --- a/sos/kernel/auth.py +++ b/sos/kernel/auth.py @@ -176,7 +176,14 @@ def _check_env_tokens(raw_token: str) -> AuthContext | None: """Return an AuthContext if *raw_token* matches any configured env-var token.""" for env_var, is_admin, scopes in _ENV_TOKENS: env_val = os.environ.get(env_var, "") - if env_val and hmac.compare_digest(env_val, raw_token): + # BLOCK-A fix (sos-205-790a2a63 gate-4): `hmac.compare_digest` raises + # `TypeError` on `str` arguments containing a non-ASCII codepoint; + # `raw_token` comes straight off the wire (a bearer header), so any + # non-ASCII byte in a presented token turned this into an + # unauthenticated 500 instead of a 401. Compare bytes — no ASCII + # restriction — same fix as sos/services/squad/auth.py:337 (see that + # comment for the full TestClient-blindspot rationale). + if env_val and hmac.compare_digest(env_val.encode("utf-8"), raw_token.encode("utf-8")): return AuthContext( agent=None, project=None, diff --git a/sos/services/squad/app.py b/sos/services/squad/app.py index 54cde7b59..520241e6c 100644 --- a/sos/services/squad/app.py +++ b/sos/services/squad/app.py @@ -4,6 +4,7 @@ import dataclasses import json import os +import threading import time from dataclasses import asdict from typing import Any, Optional @@ -36,8 +37,15 @@ TrustTier, ) from fastapi import Header -from sos.services.squad.auth import AuthContext, create_api_key as _create_api_key, require_capability -from sos.services.squad.auth import _lookup_token as _squad_lookup_token +from sos.services.squad.auth import ( + AuthContext, + create_api_key as _create_api_key, + lookup_token, + require_capability, + revoke_api_key as _revoke_api_key, + _token_cache_clear, + _TOKEN_CACHE_POSITIVE_TTL_S, +) from sos.services.squad.service import SquadDB, LeagueService from sos.services.squad import PipelineService, SquadService, SquadSkillService, SquadStateService, SquadTaskService from sos.services.squad.tasks import ClaimTokenMismatchError, InsufficientFundsError, NotAllDoneError @@ -334,6 +342,58 @@ def _require_system_bearer(authorization: Optional[str]) -> None: raise HTTPException(status_code=403, detail="system_bearer_required") +# P2-C rider (sos-205-47f5f8c2 gate-3): revoke_api_key() clears the ENTIRE +# in-process token cache (both pools, every tenant) on every call — there is +# no per-tenant invalidation because raw tokens are never stored (see +# revoke_api_key's docstring in auth.py). Measured: a single ~6ms revoke call +# forces every OTHER live client's next lookup back to a full bcrypt table +# scan (~7757x cost amplification observed), and one observed side effect was +# the brain's colony capability-gate roster fetch (5s hardcoded timeout) +# racing that cold scan and losing, degrading to its static fallback list. +# System-bearer gated already, so this is a privileged-caller DoS knob, not +# an open one — but a min-interval guard costs nothing and bounds it. This is +# a blunt, honest mitigation, NOT the durable fix: the durable fix is +# per-tenant cache invalidation via an indexed token-fingerprint column +# (tracked: Mumega-com/sos#206), which would let a revoke drop exactly the +# revoked tenant's entries instead of the whole cache. +# +# P2-G fix (sos-205-790a2a63 gate-4): this guard used to be a single GLOBAL +# timestamp checked BEFORE the DB delete — so a 429 for tenant B's revoke, +# fired purely because tenant A revoked 3s earlier, aborted the ENTIRE +# request and left tenant B's compromised key rows fully present in +# `api_keys` (confirmed live: "'second-tenant' token STILL VALID after its +# 429'd revoke: True"). A security control whose failure mode is "the revoke +# silently did not happen" is worse than no rate limit at all. Two changes: +# (1) the clock is now PER TENANT (a dict, not one float) so throttling one +# tenant's cache flush can never look like it aborted a different tenant's +# revoke; (2) the DB delete (see the route below) now runs UNCONDITIONALLY, +# before the throttle decision — only the expensive whole-process cache +# flush is ever rate-limited, and a throttled flush returns an honest +# `cache_flushed: false` + `retry_after` + TTL warning, never a bare 429 +# that could be mistaken for "nothing happened." +# +# F2 fix (sos-205-769a2651, Cursor Grok 4.5 diverse-correctness gate): change +# (1) above was wrong in a way change (2) hid. Making the clock per-tenant +# fixed the cross-tenant abort, but the THROTTLED RESOURCE is process-global — +# `_token_cache_clear()` empties every tenant's entries no matter whose revoke +# triggered it. Keying a global resource by tenant means the limit does not +# bind: vary `tenant_id` (which need not even exist) and every request is a +# first request. Measured 20/20 whole-cache flushes in one instant against a +# nominal 5s interval, defeating the ~7757x amplification argument the throttle +# was added to answer. The unbounded dict was the memory face of the same +# key-does-not-match-resource mismatch (gate-5 LOW-N). +# +# So: back to ONE clock, because there is ONE thing being rate-limited — but +# keeping change (2), which is what actually made the P2-G failure safe. The +# delete still runs unconditionally per tenant and is never throttled; only the +# shared flush is, and a throttled flush still reports itself honestly. Per +# tenant invalidation (sos#206) would let both the resource and the key be +# per-tenant, at which point this becomes a per-tenant clock again. +_REVOKE_MIN_INTERVAL_S = 5.0 +_LAST_REVOKE_FLUSH_TS = 0.0 +_REVOKE_FLUSH_LOCK = threading.Lock() + + class AuthVerifyRequest(BaseModel): token: str @@ -350,7 +410,7 @@ async def auth_verify( shape is uniform for clients; SOSClientError is not raised for 401. """ _require_system_bearer(authorization) - ctx = _squad_lookup_token(payload.token, SquadDB()) + ctx = await lookup_token(payload.token, SquadDB()) if ctx is None: return {"ok": False} return { @@ -362,6 +422,75 @@ async def auth_verify( } +class AuthRevokeRequest(BaseModel): + tenant_id: str + + +@app.post("/auth/revoke") +async def auth_revoke( + payload: AuthRevokeRequest, + authorization: Optional[str] = Header(default=None), +) -> dict[str, Any]: + """Revoke every api_key row for a tenant AND (subject to a per-tenant + throttle) clear THIS process's token cache. System-bearer gated. + + BLOCK-2 fix (sos-205-b5307dd7 re-gate): ``sos.services.squad.auth``'s CLI + ``revoke`` subcommand runs in a separate OS process from the uvicorn + worker(s) actually serving requests, so it can delete the DB rows but has + no way to reach the serving process's in-memory token cache — a revoked + token kept authenticating here for up to the positive-cache TTL (30s). + This route runs the deletion + cache clear IN this serving process, so + it is the only path that can make revocation actually immediate. The CLI + now calls this route first and only falls back to a local-only DB delete + (with an honest, non-``true`` receipt) when it can't reach it. + + P2-G fix (sos-205-790a2a63 gate-4): the DB delete now ALWAYS runs first — + a revoked key must never stay valid because a cache flush got throttled. + Only the whole-process cache clear is rate-limited, per tenant (see the + constants' docstring above), and never returns a bare 429 for it: a + throttled flush is still a 200 with ``cache_flushed: false``, a + ``retry_after`` seconds, and an explicit TTL warning — an honest receipt, + not a silent skip. + """ + _require_system_bearer(authorization) + tenant_id = payload.tenant_id + + # The delete must always happen, independent of the flush throttle below. + deleted = _revoke_api_key(tenant_id, SquadDB(), flush_cache=False) + + # Claim the flush slot and stamp the clock in ONE critical section. Reading + # the timestamp, deciding, then writing it would let two concurrent revokes + # both observe the same stale `last` and both flush — the check would bound + # nothing under exactly the concurrent load it exists to bound. + global _LAST_REVOKE_FLUSH_TS + with _REVOKE_FLUSH_LOCK: + now = time.monotonic() + elapsed = now - _LAST_REVOKE_FLUSH_TS + may_flush = elapsed >= _REVOKE_MIN_INTERVAL_S + if may_flush: + _LAST_REVOKE_FLUSH_TS = now + if not may_flush: + retry_after = round(_REVOKE_MIN_INTERVAL_S - elapsed, 3) + return { + "tenant_id": tenant_id, + "revoked_rows": deleted, + "cache_flushed": False, + "retry_after": retry_after, + "warning": ( + f"cache flush rate-limited ({_REVOKE_MIN_INTERVAL_S}s min " + f"interval; the flush is process-wide, so the limit is too — " + f"another tenant's revoke may have consumed the slot). DB rows " + f"for this tenant are deleted, but entries already cached in " + f"THIS process may continue to authenticate for up to " + f"{_TOKEN_CACHE_POSITIVE_TTL_S}s (the positive-cache TTL). " + f"Retry after {retry_after}s to force a flush, or wait out the " + f"TTL." + ), + } + _token_cache_clear() + return {"tenant_id": tenant_id, "revoked_rows": deleted, "cache_flushed": True} + + class ApiKeyCreateRequest(BaseModel): tenant_id: str identity_type: str = "user" @@ -1302,14 +1431,14 @@ async def _daily_kpi_snapshot() -> None: async def _kpi_cron_loop() -> None: """Background task: sleep until 00:05 UTC, run snapshot, repeat daily.""" import math - from datetime import datetime, timezone + from datetime import datetime, timedelta, timezone while True: now = datetime.now(timezone.utc) # Next 00:05 UTC target = now.replace(hour=0, minute=5, second=0, microsecond=0) if target <= now: - target = target.replace(day=target.day + 1) + target = target + timedelta(days=1) wait_seconds = (target - now).total_seconds() await asyncio.sleep(wait_seconds) await _daily_kpi_snapshot() @@ -1317,7 +1446,7 @@ async def _kpi_cron_loop() -> None: async def _league_weekly_snapshot_loop() -> None: """Background task: every Monday at 01:00 UTC, snapshot league scores.""" - from datetime import datetime, timezone + from datetime import datetime, timedelta, timezone while True: now = datetime.now(timezone.utc) @@ -1325,10 +1454,10 @@ async def _league_weekly_snapshot_loop() -> None: days_until_monday = (7 - now.weekday()) % 7 # 0 if today is Monday target = now.replace(hour=1, minute=0, second=0, microsecond=0) if days_until_monday > 0: - target = target.replace(day=target.day + days_until_monday) + target = target + timedelta(days=days_until_monday) elif target <= now: # It's Monday but we've already passed 01:00 — skip to next Monday - target = target.replace(day=target.day + 7) + target = target + timedelta(days=7) wait_seconds = (target - now).total_seconds() await asyncio.sleep(wait_seconds) try: @@ -1341,14 +1470,14 @@ async def _league_weekly_snapshot_loop() -> None: async def _league_daily_season_loop() -> None: """Background task: every day at 00:01 UTC, ensure an active season exists.""" - from datetime import datetime, timezone + from datetime import datetime, timedelta, timezone while True: now = datetime.now(timezone.utc) # Next 00:01 UTC target = now.replace(hour=0, minute=1, second=0, microsecond=0) if target <= now: - target = target.replace(day=target.day + 1) + target = target + timedelta(days=1) wait_seconds = (target - now).total_seconds() await asyncio.sleep(wait_seconds) try: @@ -2145,7 +2274,7 @@ async def create_project_role( authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: """Create a named role for a project. Owner-level auth.""" - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: raise HTTPException(status_code=401, detail="invalid_token") try: @@ -2163,7 +2292,7 @@ async def list_project_roles( project_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: raise HTTPException(status_code=401, detail="invalid_token") roles = _role_svc.list_roles(project_id, tenant_id=auth.tenant_scope or "default") @@ -2176,9 +2305,20 @@ async def add_role_permission( body: _PermissionBody, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - _squad_lookup_token(_parse_bearer(authorization), SquadDB()) or _raise_401() + # P0-B fix (sos-205-47f5f8c2 gate-3): this route used to discard the + # AuthContext entirely (`await lookup_token(...) or _raise_401()`) after + # proving the caller held SOME valid api key — any tenant's key could + # then mutate ANY other tenant's role permissions. Bind `auth` and scope + # by `auth.tenant_scope`, same pattern as the sibling create_project_role + # / list_project_roles routes above (None for system = cross-tenant by + # design; a tenant's own scope otherwise). A foreign-tenant role_id now + # 404s exactly like a nonexistent one — the route was never meant to + # disclose which is which to a caller who doesn't own it. + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) + if not auth: + _raise_401() try: - return _role_svc.add_permission(role_id, body.permission) + return _role_svc.add_permission(role_id, body.permission, tenant_id=auth.tenant_scope) except RoleNotFoundError: raise HTTPException(status_code=404, detail="role_not_found") @@ -2189,8 +2329,14 @@ async def remove_role_permission( permission: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - _squad_lookup_token(_parse_bearer(authorization), SquadDB()) or _raise_401() - _role_svc.remove_permission(role_id, permission) + # P0-B fix — see add_role_permission above for the full rationale. + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) + if not auth: + _raise_401() + try: + _role_svc.remove_permission(role_id, permission, tenant_id=auth.tenant_scope) + except RoleNotFoundError: + raise HTTPException(status_code=404, detail="role_not_found") return {"deleted": True} @@ -2200,13 +2346,20 @@ async def assign_role( body: _AssignBody, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + # BLOCK-B fix (sos-205-790a2a63 gate-4): this was the 6th RBAC route on + # this surface — the P0-B fix (sos-205-47f5f8c2) scoped five siblings but + # missed this one, so any tenant's valid api key could plant a role + # assignment into ANOTHER tenant's role (and, because revoke_assignment + # IS scoped, could not even undo it). Bind `auth` and scope by + # `auth.tenant_scope`, same pattern as the five siblings. + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() caller_id = auth.identity.id if auth.identity else "system" try: return _role_svc.assign_role( role_id, body.assignee_id, + tenant_id=auth.tenant_scope, assignee_type=body.assignee_type, assigned_by=body.assigned_by, caller_id=caller_id, @@ -2223,8 +2376,14 @@ async def revoke_role_assignment( assignee_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - _squad_lookup_token(_parse_bearer(authorization), SquadDB()) or _raise_401() - _role_svc.revoke_assignment(role_id, assignee_id) + # P0-B fix — see add_role_permission above for the full rationale. + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) + if not auth: + _raise_401() + try: + _role_svc.revoke_assignment(role_id, assignee_id, tenant_id=auth.tenant_scope) + except RoleNotFoundError: + raise HTTPException(status_code=404, detail="role_not_found") return {"revoked": True} @@ -2233,8 +2392,14 @@ async def list_role_assignments( role_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - _squad_lookup_token(_parse_bearer(authorization), SquadDB()) or _raise_401() - assignments = _role_svc.list_assignments(role_id) + # P0-B fix — see add_role_permission above for the full rationale. + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) + if not auth: + _raise_401() + try: + assignments = _role_svc.list_assignments(role_id, tenant_id=auth.tenant_scope) + except RoleNotFoundError: + raise HTTPException(status_code=404, detail="role_not_found") return {"assignments": assignments} @@ -2271,8 +2436,15 @@ async def get_agent_roles( agent_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - _squad_lookup_token(_parse_bearer(authorization), SquadDB()) or _raise_401() - roles = _role_svc.get_agent_roles(agent_id) + # P0-B fix — see add_role_permission above for the full rationale. This + # route enumerates ALL roles held by agent_id across every project; an + # unscoped call let any tenant's key discover which roles ANY agent + # holds in ANY tenant. Scoping filters the join to the caller's own + # tenant (system unrestricted, per auth.tenant_scope). + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) + if not auth: + _raise_401() + roles = _role_svc.get_agent_roles(agent_id, tenant_id=auth.tenant_scope) return {"agent_id": agent_id, "roles": roles} @@ -2280,7 +2452,7 @@ async def get_agent_roles( async def get_my_roles( authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: raise HTTPException(status_code=401, detail="invalid_token") roles = _role_svc.get_token_roles(auth.tenant_id or "") @@ -2365,7 +2537,7 @@ async def create_contact( body: _ContactCreate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2387,7 +2559,7 @@ async def list_contacts( tier: Optional[str] = None, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() contacts = _contacts_svc.list( @@ -2402,7 +2574,7 @@ async def get_contact_by_email( email: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() contact = _contacts_svc.get_by_email(_workspace(auth), email) @@ -2416,7 +2588,7 @@ async def get_contact( contact_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2431,7 +2603,7 @@ async def update_contact( body: _ContactUpdate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2449,7 +2621,7 @@ async def touch_contact( body: _TouchBody, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2463,7 +2635,7 @@ async def delete_contact( contact_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2509,7 +2681,7 @@ async def create_partner( body: _PartnerCreate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2529,7 +2701,7 @@ async def list_partners( status: Optional[str] = None, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() partners = _partners_svc.list(_workspace(auth), type=type, active_only=active_only, status=status) @@ -2541,7 +2713,7 @@ async def get_partner( partner_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2556,7 +2728,7 @@ async def update_partner( body: _PartnerUpdate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2573,7 +2745,7 @@ async def get_partner_contacts( partner_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() contacts = _partners_svc.get_contacts(partner_id, _workspace(auth)) @@ -2585,7 +2757,7 @@ async def get_partner_opportunities( partner_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() opps = _partners_svc.get_opportunities(partner_id, _workspace(auth)) @@ -2631,7 +2803,7 @@ async def create_opportunity( body: _OppCreate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2652,7 +2824,7 @@ async def list_opportunities( archived: bool = False, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() opps = _opps_svc.list( @@ -2666,7 +2838,7 @@ async def list_opportunities( async def pipeline_summary( authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() return {"pipeline": _opps_svc.pipeline_summary(_workspace(auth))} @@ -2677,7 +2849,7 @@ async def get_opportunity( opp_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2692,7 +2864,7 @@ async def transition_opportunity_stage( body: _StageTransition, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2707,7 +2879,7 @@ async def update_opportunity( body: _OppUpdate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2746,7 +2918,7 @@ async def create_referral( body: _ReferralCreate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2769,7 +2941,7 @@ async def list_referrals( target_type: Optional[str] = None, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() refs = _refs_svc.list( @@ -2785,7 +2957,7 @@ async def referral_network( hops: int = Query(default=2, ge=1, le=5), authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() return _refs_svc.network(entity_id, _workspace(auth), hops=hops) @@ -2797,7 +2969,7 @@ async def update_referral( body: _ReferralUpdate, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() try: @@ -2814,7 +2986,7 @@ async def delete_referral( ref_id: str, authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() _refs_svc.delete(ref_id, _workspace(auth), _actor(auth)) @@ -2830,7 +3002,7 @@ async def ghl_sync_contact( authorization: Optional[str] = Header(default=None), ) -> dict[str, Any]: """Upsert contact from GHL lead payload. Keyed by email.""" - auth = _squad_lookup_token(_parse_bearer(authorization), SquadDB()) + auth = await lookup_token(_parse_bearer(authorization), SquadDB()) if not auth: _raise_401() email = payload.get("email") or payload.get("contact", {}).get("email") diff --git a/sos/services/squad/auth.py b/sos/services/squad/auth.py index c171e697d..6c7759d1a 100644 --- a/sos/services/squad/auth.py +++ b/sos/services/squad/auth.py @@ -1,15 +1,21 @@ from __future__ import annotations import argparse +import concurrent.futures import hmac import hashlib +import logging import os import secrets import sqlite3 +import threading +import time from dataclasses import dataclass -from typing import Callable +from typing import Any, Callable +import anyio import bcrypt +import requests from fastapi import Depends, HTTPException, Request from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer @@ -18,7 +24,22 @@ from sos.services.squad.service import DEFAULT_TENANT_ID, SquadDB, now_iso +logger = logging.getLogger("sos.services.squad.auth") + SYSTEM_TOKEN = os.getenv("SOS_SYSTEM_TOKEN", "") +if not SYSTEM_TOKEN: + # P0-A fix (sos-205-47f5f8c2 gate-3): loud, not silent. The compose + # deploy path now hard-fails on a missing SOS_SYSTEM_TOKEN + # (docker-compose.yml `squad:` service, `${SOS_SYSTEM_TOKEN:?set + # explicitly}`); this covers every other entry point (bare uvicorn, + # tests that don't need the system tier, local dev) where that guard + # doesn't run. System-tier auth is simply unreachable while this is + # unset — see the fail-closed guard in _lookup_token below — this is + # informational, not an additional gate. + logger.warning( + "SOS_SYSTEM_TOKEN is unset — system-tier auth is DISABLED (fail-closed, " + "not fail-open). Set it before deploying; see .env.example." + ) security = HTTPBearer(auto_error=False) OPERATION_MAP: dict[tuple[str, str], CapabilityAction] = { @@ -67,6 +88,25 @@ def _verify_token(token: str, stored_hash: str) -> bool: return hmac.compare_digest(legacy_hash, stored_hash) +def _identity_from_snapshot(snapshot: dict) -> Identity: + """Rebuild an Identity from a cached {tenant_id, identity_type} snapshot. + + Same construction as _identity_from_row — kept in one place so the cache + cannot drift from the row path. + """ + tenant_id = snapshot["tenant_id"] + identity_type = snapshot["identity_type"] + if identity_type == "agent": + identity = AgentIdentity(name=tenant_id, model="api-key") + elif identity_type == "service": + identity = Identity(id=f"service:{tenant_id}", type=IdentityType.SERVICE, name=tenant_id) + else: + identity = UserIdentity(name=tenant_id) + identity.metadata["tenant_id"] = tenant_id + identity.metadata["identity_type"] = identity_type + return identity + + def _identity_from_row(row: sqlite3.Row) -> Identity: tenant_id = row["tenant_id"] identity_type = row["identity_type"] @@ -95,9 +135,243 @@ def _capability_for(identity: Identity, tenant_id: str | None, action: Capabilit ) -def _lookup_token(token: str, db: SquadDB) -> AuthContext | None: - if token == SYSTEM_TOKEN: - return AuthContext(token=token, identity=SYSTEM_IDENTITY, tenant_id=None, is_system=True) +# Token-verification cache — 2026-07-27 incident fix, hardened 2026-07-27 +# against sos-205-a7c2fc44 adversarial gate findings (BLOCK-1/2/3, WARN-2/3). +# +# _lookup_token bcrypt-checks the presented token against EVERY api_keys row +# (17 bcrypt rows ≈ 5s of CPU) synchronously. Callers that retry on timeout +# (loop.py skill registration, hermes check-in) turned one slow verify into a +# congestion collapse: the service pegged a core and stopped answering while +# healthy clients piled on more verifies. The cache fixes the ACCIDENTAL case +# (same bad token replayed). It does NOT fix the adversarial case (unique-token +# spray, one full scan each) — require_capability now offloads the scan to a +# worker thread (BLOCK-1) so a spray blocks a thread-pool slot, not the event +# loop; the durable fix is still an indexed token-fingerprint column written at +# mint time (follow-up: "squad auth: indexed token fingerprint column at mint +# time", tracked on Mumega-com/sos). +# +# Cache keyed by a domain-separated hash of the token (WARN-3 — the previous +# bare sha256(token) was byte-identical to the legacy stored-credential format, +# so a cache key doubled as a credential against any row still on that legacy +# hash) so raw tokens never sit in memory beyond the request. +# +# Positive and negative hits live in SEPARATE bounded pools (BLOCK-3 — a single +# shared pool let an attacker spray unique bad tokens to evict live positive +# entries, since eviction picked oldest-by-expiry and every positive lives +# longer than a negative). A spray can now only evict other negatives. +# +# Positive TTL dropped 300s -> 30s (BLOCK-2). 300s on a surface with NO +# revocation mechanism was an unbounded trade-off, not a bounded one: a +# revoked-but-cached token could keep registering/executing skills for the +# full window. 30s still amortizes the ~1.2s bcrypt cost at real request +# rates while killing most of the stale-validity blast radius; the remaining +# window is closed on-demand by revoke_api_key(), which clears the cache +# outright. Negative TTL stays 60s (one bad-token client still costs one scan +# per minute, not one per request). +# +# _TOKEN_CACHE_LOCK guards ALL reads/writes to both pools. This was +# incidentally safe before because _lookup_token was fully synchronous and +# uvicorn ran a single worker — no interleaved dict mutation was possible. +# Offloading the scan to a thread pool (BLOCK-1) makes the cache genuinely +# shared mutable state across threads, so the lock is now load-bearing, not +# defensive. +_TOKEN_CACHE_POSITIVE: dict[str, tuple[float, dict]] = {} +_TOKEN_CACHE_NEGATIVE: dict[str, tuple[float, None]] = {} +_TOKEN_CACHE_LOCK = threading.Lock() +_TOKEN_CACHE_POSITIVE_TTL_S = 30.0 +_TOKEN_CACHE_NEGATIVE_TTL_S = 60.0 +_TOKEN_CACHE_POSITIVE_MAX = 64 +_TOKEN_CACHE_NEGATIVE_MAX = 192 + +# Cache generation, bumped on every whole-cache clear. Guarded by +# _TOKEN_CACHE_LOCK like the pools themselves. +# +# F1 (sos-205-769a2651, Cursor Grok 4.5 diverse-correctness gate): clearing +# the pools is not enough, because a scan that has ALREADY read its rows can +# still be running. It finishes bcrypt, calls _token_cache_put, and republishes +# a snapshot of the pre-revoke world into the just-emptied cache. Measured: DB +# rows for the tenant = 0, flush reported success, and the revoked token still +# authenticated. The flush was honest and the property was still false — the +# window is the width of a full bcrypt table scan (seconds). +# +# The generation makes the staleness detectable: a scan snapshots the epoch +# before touching the DB, and any write carrying a superseded epoch is refused. +_TOKEN_CACHE_EPOCH = 0 + +# Backward-compat alias for external/test code that still refers to the old +# single-cap constant name; both pools are individually bounded above. +_TOKEN_CACHE_MAX = _TOKEN_CACHE_POSITIVE_MAX + _TOKEN_CACHE_NEGATIVE_MAX + +# Single-flight in-flight map (BLOCK-1b — sos-205-b5307dd7 re-gate). Keyed +# identically to the cache pools. Concurrent replays of the SAME cold token +# used to each run their own full-table bcrypt scan (measured: 8 concurrent +# lookups of one token -> 8 scans, vs 1 scan when sequential) because the +# cache-miss check and the scan were not atomic with respect to each other. +# The first caller for a cache_key registers a Future here (under +# _TOKEN_CACHE_LOCK, so registration is atomic with the miss check) and does +# the scan; every other caller for the same key waits on that Future instead +# of scanning again. Evicted as soon as the leader resolves it. +_TOKEN_CACHE_INFLIGHT: dict[str, "concurrent.futures.Future[dict | None]"] = {} + +# LOW-1 (sos-205-47f5f8c2 gate-3): a follower's `future.result()` used to +# have no timeout — it waited exactly as long as the leader did, unbounded. +# Measured: a leader artificially stuck for 8s left a follower blocked past +# 2s and it only returned at 4.7s (i.e. genuinely followed the leader, not a +# fixed delay). Not worse than pre-single-flight behaviour (everyone +# scanned) and it still fails closed, but nothing bounded it. 30s is well +# above the worst-case observed full-table scan (~10s at 21 rows). +_TOKEN_CACHE_INFLIGHT_TIMEOUT_S = 30.0 + + +def _token_cache_key(token: str, db: SquadDB) -> str: + # Domain-separated (WARN-3): must never collide with the legacy stored + # hash format (bare sha256(token)) used by _verify_token's legacy path. + # WARN-1 (sos-205-b5307dd7 re-gate): scoped to `db` too — the cache used + # to be keyed on the token alone, so two distinct SquadDB instances + # (e.g. a future per-tenant DB split) could share cache entries and a + # token valid only in DB A would authenticate against DB B without ever + # touching it. `_lookup_token(token, db)` and `revoke_api_key(tenant, + # db=...)` both already advertise a per-db contract; this makes the + # cache honor it instead of ignoring the db parameter. + return hashlib.sha256( + b"squad-authcache-v1:" + str(db.db_path).encode("utf-8") + b":" + token.encode("utf-8") + ).hexdigest() + + +def _token_cache_get(key: str) -> tuple[bool, dict | None]: + """Returns (hit, row_snapshot_or_None). Expired entries count as miss.""" + with _TOKEN_CACHE_LOCK: + for pool in (_TOKEN_CACHE_POSITIVE, _TOKEN_CACHE_NEGATIVE): + entry = pool.get(key) + if entry is None: + continue + expires_at, snapshot = entry + if time.monotonic() >= expires_at: + pool.pop(key, None) + return False, None + return True, snapshot + return False, None + + +def _token_cache_epoch() -> int: + """Current cache generation. Callers snapshot this BEFORE reading rows + from the DB and hand it back to _token_cache_put, which refuses to write + a result computed against a generation that has since been invalidated + (F1, sos-205-769a2651 diverse-correctness gate).""" + with _TOKEN_CACHE_LOCK: + return _TOKEN_CACHE_EPOCH + + +def _token_cache_put(key: str, snapshot: dict | None, epoch: int | None = None) -> bool: + """Write a scan result into the cache. Returns True if it was stored, + False if it was DROPPED as stale. + + ``epoch`` is the generation observed before the DB rows behind + ``snapshot`` were read. If the cache has been cleared since (a revoke or + rotation landed mid-scan), the snapshot describes a world that no longer + exists and must not be persisted — otherwise a revoked credential gets + re-published into a freshly-flushed cache and authenticates for the rest + of the positive TTL. Passing ``None`` skips the check (callers that hold + no meaningful generation). + + The comparison happens INSIDE _TOKEN_CACHE_LOCK, together with the write. + Checking the epoch outside the lock would reintroduce the same race one + level up. + """ + is_positive = snapshot is not None + ttl = _TOKEN_CACHE_POSITIVE_TTL_S if is_positive else _TOKEN_CACHE_NEGATIVE_TTL_S + pool = _TOKEN_CACHE_POSITIVE if is_positive else _TOKEN_CACHE_NEGATIVE + other_pool = _TOKEN_CACHE_NEGATIVE if is_positive else _TOKEN_CACHE_POSITIVE + cap = _TOKEN_CACHE_POSITIVE_MAX if is_positive else _TOKEN_CACHE_NEGATIVE_MAX + with _TOKEN_CACHE_LOCK: + if epoch is not None and epoch != _TOKEN_CACHE_EPOCH: + return False + # A key can only ever be positive or negative at once — drop it from + # the other pool first (e.g. a token that was negative-cached and has + # just verified). + other_pool.pop(key, None) + if len(pool) >= cap: + oldest = min(pool, key=lambda k: pool[k][0]) + pool.pop(oldest, None) + pool[key] = (time.monotonic() + ttl, snapshot) + return True + + +def _token_cache_forget(key: str) -> None: + """Drop a single key from both pools, wherever it lives.""" + with _TOKEN_CACHE_LOCK: + _TOKEN_CACHE_POSITIVE.pop(key, None) + _TOKEN_CACHE_NEGATIVE.pop(key, None) + + +def _token_cache_clear() -> None: + """Drop the ENTIRE cache — both pools. + + Used by revoke_api_key() and rotation. Raw tokens are never stored (only + the domain-separated hash), so revocation cannot target the specific + cache entry for a revoked token — a whole-cache clear is the only sound + option at this scale (<=256 entries total, cheap to rebuild on the next + lookup). + + Also bumps _TOKEN_CACHE_EPOCH so that any scan currently in flight — one + that read its rows BEFORE this clear — cannot republish its now-stale + result after we return (F1). Emptying the pools alone left a window as wide + as a full bcrypt scan in which a revoked credential got re-cached. + """ + global _TOKEN_CACHE_EPOCH + with _TOKEN_CACHE_LOCK: + _TOKEN_CACHE_POSITIVE.clear() + _TOKEN_CACHE_NEGATIVE.clear() + _TOKEN_CACHE_EPOCH += 1 + + +def _context_from_snapshot(token: str, snapshot: dict | None) -> AuthContext | None: + if snapshot is None: + return None + return AuthContext( + token=token, + identity=_identity_from_snapshot(snapshot), + tenant_id=snapshot["tenant_id"], + is_system=False, + ) + + +def _inflight_release( + cache_key: str, future: "concurrent.futures.Future[dict | None]" +) -> None: + """Deregister ``future`` as the in-flight scan for ``cache_key`` — but ONLY + if it is still the registered one. + + F5 (sos-205-769a2651 diverse-correctness gate): the leader used to pop + unconditionally. After a follower timed out and evicted a stalled leader + (see LOW-1 handling below), a NEW leader registers its own Future under the + same key. When the original leader finally finished, its unconditional pop + removed the *successor's* registration, so every subsequent caller became a + fresh leader and BLOCK-1b's single-flight guarantee quietly stopped holding + — precisely under the stall conditions that motivated the timeout. Identity + check, same shape the follower timeout path already used. + """ + with _TOKEN_CACHE_LOCK: + if _TOKEN_CACHE_INFLIGHT.get(cache_key) is future: + _TOKEN_CACHE_INFLIGHT.pop(cache_key, None) + + +def _scan_and_cache( + token: str, db: SquadDB, cache_key: str +) -> tuple[dict | None, bool]: + """The actual full-table bcrypt scan. Only ever called by the single-flight + leader in _lookup_token — never call this directly from a second thread + for the same cache_key. + + Returns ``(snapshot, fresh)``. ``fresh`` is False when the cache was + cleared while this scan was running: the rows behind ``snapshot`` were read + before a revoke/rotation landed, so the result is a description of a world + that no longer exists. It is neither cached nor trusted — see F1 in + _TOKEN_CACHE_EPOCH and the fail-closed handling in _lookup_token. + """ + # Snapshot the generation BEFORE reading any rows. Everything after this + # point is computed against the state as of `epoch`. + epoch = _token_cache_epoch() with db.connect() as conn: rows = conn.execute( "SELECT token_hash, tenant_id, identity_type, created_at FROM api_keys" @@ -117,13 +391,172 @@ def _lookup_token(token: str, db: SquadDB) -> AuthContext | None: (hash_token(token), legacy_hash), ) if not matched: - return None - return AuthContext( - token=token, - identity=_identity_from_row(matched), - tenant_id=matched["tenant_id"], - is_system=False, - ) + fresh = _token_cache_put(cache_key, None, epoch=epoch) + return None, fresh + snapshot = { + "tenant_id": matched["tenant_id"], + "identity_type": matched["identity_type"], + } + fresh = _token_cache_put(cache_key, snapshot, epoch=epoch) + return snapshot, fresh + + +def _lookup_token(token: str, db: SquadDB) -> AuthContext | None: + # P0-A fix (sos-205-47f5f8c2 gate-3): SYSTEM_TOKEN defaults to "" when + # SOS_SYSTEM_TOKEN is unset, and `token == SYSTEM_TOKEN` with an empty + # SYSTEM_TOKEN matched an empty presented token — i.e. NO Authorization + # header at all (`_parse_bearer(None) -> ""`) authenticated as + # system:sos with is_system=True on all 34 `_parse_bearer` routes. + # `.env.example` and docker-compose.yml never set this var, so the + # documented deploy path was the vulnerable one (only a host-local + # `~/.env.secrets` dotenv accident masked it here). Fail closed: the + # system-token branch cannot fire at all when no token is configured, + # and the compare is constant-time (LOW-3 — the repo already uses + # hmac.compare_digest in 8+ other places, incl. sos/kernel/auth.py:179's + # identical `env_val and hmac.compare_digest(...)` shape; this line was + # the one place that still used `==`). + # + # BLOCK-A fix (sos-205-790a2a63 gate-4): the P0-A fix above swapped `==` + # for `hmac.compare_digest(token, SYSTEM_TOKEN)` on two `str` arguments. + # `hmac.compare_digest` RAISES `TypeError` when either `str` argument + # contains a codepoint above 127 ("comparing strings with non-ASCII + # characters is not supported") — `==` never raised. So the constant-time + # fix quietly added an unauthenticated crash path: ANY bearer token + # containing a single non-ASCII byte (umlaut, Cyrillic, a lone 0x80-0xFF + # byte, even a zero-width character) turns into an uncaught 500 here, + # before any DB work, on every one of the 36 `lookup_token` call sites — + # cheaper for an attacker to trigger than a normal miss (no bcrypt scan) + # and impossible to throttle by scan cost. + # + # httpx/starlette's TestClient REFUSES to encode a non-ASCII header + # client-side (UnicodeEncodeError before the request is even sent), so + # this is structurally invisible to any TestClient-based test — the CI + # suite stays green while a raw socket against a real uvicorn gets a 500. + # HTTP header values are ISO-8859-1 on the wire and starlette decodes them + # as latin-1, so this is trivially reachable in production. + # + # Fix: compare BYTES, not `str`. `hmac.compare_digest` has no ASCII + # restriction on `bytes` — encoding both sides to UTF-8 first removes the + # crash path entirely while keeping the comparison constant-time. Same + # treatment applied to `sos/kernel/auth.py:179`'s identical + # `env_val and hmac.compare_digest(env_val, raw_token)` shape (also a + # `str`/`str` compare on a wire-supplied value). The two OTHER + # `compare_digest` sites in this repo (`sos/kernel/auth.py:232`, + # `sos/services/squad/auth.py:88`) compare hex digest strings on BOTH + # sides — `hashlib.*.hexdigest()` output is always ASCII regardless of + # input, so those cannot raise and were left alone. `app.py:2164` + # (`_hmac.compare_digest(presented, expected)`) already encodes both + # sides to bytes via `.encode()`, so it was never affected either. + if SYSTEM_TOKEN and hmac.compare_digest(token.encode("utf-8"), SYSTEM_TOKEN.encode("utf-8")): + return AuthContext(token=token, identity=SYSTEM_IDENTITY, tenant_id=None, is_system=True) + cache_key = _token_cache_key(token, db) + hit, snapshot = _token_cache_get(cache_key) + if hit: + return _context_from_snapshot(token, snapshot) + + # Single-flight (BLOCK-1b): register-or-join under the SAME lock that + # guards the cache pools, so there is no window between the miss check + # above and the Future registration where a second caller could slip in + # and start its own scan. + with _TOKEN_CACHE_LOCK: + future = _TOKEN_CACHE_INFLIGHT.get(cache_key) + if future is None: + future = concurrent.futures.Future() + _TOKEN_CACHE_INFLIGHT[cache_key] = future + is_leader = True + else: + is_leader = False + + if not is_leader: + # A scan for this exact cache_key is already running — wait for it + # instead of running a second one. Propagates the leader's exception + # (e.g. a DB error) to every follower too. + try: + snapshot = future.result(timeout=_TOKEN_CACHE_INFLIGHT_TIMEOUT_S) + except concurrent.futures.TimeoutError: + # LOW-1 fix: give up waiting on a leader that has stalled past + # the worst-case scan time, and evict so this key doesn't stay + # wedged for the NEXT caller too — a fresh lookup becomes a new + # leader. Does not disturb the original leader; it still resolves + # `future` when/if it finishes, just to nobody waiting on it any + # more. Re-raised, not swallowed: this fails closed (the caller + # sees an error, never a synthesized auth result). + _inflight_release(cache_key, future) + raise + return _context_from_snapshot(token, snapshot) + + try: + snapshot, fresh = _scan_and_cache(token, db, cache_key) + except BaseException as exc: + _inflight_release(cache_key, future) + future.set_exception(exc) + raise + if not fresh: + # F1: the cache was cleared while this scan was running, so `snapshot` + # was computed from rows read BEFORE a revoke/rotation landed. It has + # already been refused entry to the cache; it must not be trusted for + # THIS request either, nor handed to the followers waiting on the + # Future — they would each get the same zombie result. + # + # Fail closed rather than re-scanning: the caller sees an auth miss and + # retries, and the retry runs a fresh scan against post-revoke rows. A + # live credential costs one spurious 401 inside the revoke window; a + # revoked one stops working immediately, which is the property the + # whole revoke path exists to provide. + snapshot = None + _inflight_release(cache_key, future) + future.set_result(snapshot) + return _context_from_snapshot(token, snapshot) + + +async def lookup_token(token: str, db: SquadDB) -> AuthContext | None: + """Async, event-loop-safe entry point for token lookup — the ONLY place + that should offload _lookup_token's scan to a worker thread. + + BLOCK-1 fix (sos-205-b5307dd7 re-gate): the a7c2fc44 fix offloaded the + scan only inside require_capability's dependency; 34 other app.py route + handlers called the synchronous _lookup_token directly, so the scan ran + ON the event loop for the entire RBAC/CRM/referrals surface. Measured: + 27,956ms of dead event loop (heartbeat scheduled exactly once) for a + single unpatched replay. Every caller — require_capability included — + now goes through this one function, so there is exactly one offload + point to keep correct, not one-per-caller. + """ + return await anyio.to_thread.run_sync(_lookup_token, token, db) + + +def revoke_api_key(tenant_id: str, db: SquadDB | None = None, *, flush_cache: bool = True) -> int: + """Revoke every api_key row for ``tenant_id`` and (by default) invalidate + the cache. + + BLOCK-2 fix (sos-205-a7c2fc44 adversarial gate): the service had no + revocation path at all — a deleted row could still authenticate for up to + 300s because nothing ever cleared the in-process positive cache. This is + the missing invalidation hook. It clears the ENTIRE _TOKEN_CACHE (both + pools), not a per-entry lookup: raw tokens are never stored, only a + one-way hash, so there is no way to identify which cache entries belong + to the revoked tenant. A whole-cache clear is correct and cheap at this + scale (<=256 entries; the next request just repopulates its own entry). + + P2-G fix (sos-205-790a2a63 gate-4): ``flush_cache`` splits "delete the DB + rows" from "clear the process-wide cache" so a caller doing its own + throttled flush (see app.py's ``/auth/revoke`` route) can guarantee the + DELETE always runs while only the (expensive, DoS-able) whole-cache clear + is ever rate-limited. Defaults to ``True`` so every existing caller (the + CLI, tests) keeps today's behaviour unchanged. + + Returns the number of rows deleted. + """ + database = db or SquadDB() + with database.connect() as conn: + cursor = conn.execute( + "DELETE FROM api_keys WHERE tenant_id = ?", + (tenant_id,), + ) + deleted = cursor.rowcount if cursor.rowcount is not None else 0 + if flush_cache: + _token_cache_clear() + return deleted async def _emit_squad_policy( @@ -182,7 +615,11 @@ async def dependency( reason="missing_authorization", ) raise HTTPException(status_code=401, detail="missing_authorization") - auth = _lookup_token(token, database) + # BLOCK-1 fix (sos-205-a7c2fc44 adversarial gate, generalized in + # sos-205-b5307dd7): the offload lives in lookup_token() now — this + # dependency is just one of its many callers (app.py's 34 direct + # call sites go through the same function), not a special case. + auth = await lookup_token(token, database) if not auth: await _emit_squad_policy( agent="anonymous", @@ -232,22 +669,105 @@ async def dependency( return dependency -def create_api_key(tenant_id: str, identity_type: str = "user", db: SquadDB | None = None) -> tuple[str, str]: +def create_api_key( + tenant_id: str, + identity_type: str = "user", + db: SquadDB | None = None, + rotate: bool = False, +) -> tuple[str, str]: + """Mint a new api_key row for ``tenant_id``. + + WARN-2 fix (sos-205-a7c2fc44 adversarial gate): this used to be + ``INSERT OR REPLACE`` keyed on ``token_hash`` — but bcrypt salts are + random, so a freshly minted token_hash is never equal to an existing row + and ``OR REPLACE`` was dead code. Every historical key stayed live + forever (no revocation path existed either — see revoke_api_key / BLOCK-2) + and the table grew monotonically, which is also the actual root cause of + BLOCK-1's scan cost growing without bound. + + Default mint is now a plain, additive INSERT — existing rows for this + tenant/identity_type are left alone (unchanged default behaviour from the + caller's perspective; the only change is that duplicate token_hash + collisions can no longer silently clobber each other, which they never + did anyway). Pass ``rotate=True`` to explicitly retire this tenant's + existing keys of the same identity_type before minting the replacement — + real revocation, not an accidental hash-collision no-op. + + The durable fix for unbounded table growth is the indexed + token-fingerprint column (follow-up: "squad auth: indexed token + fingerprint column at mint time", tracked on Mumega-com/sos); rotation + only bounds growth for callers that opt in. + """ database = db or SquadDB() token = f"sk-squad-{tenant_id}-{secrets.token_hex(16)}" token_hash = hash_token(token) created_at = now_iso() with database.connect() as conn: + if rotate: + conn.execute( + "DELETE FROM api_keys WHERE tenant_id = ? AND identity_type = ?", + (tenant_id, identity_type), + ) conn.execute( """ - INSERT OR REPLACE INTO api_keys (token_hash, tenant_id, identity_type, created_at) + INSERT INTO api_keys (token_hash, tenant_id, identity_type, created_at) VALUES (?, ?, ?, ?) """, (token_hash, tenant_id, identity_type, created_at), ) + if rotate: + # Old keys for this tenant/identity_type just died; their cache + # entries (if any) can't be targeted individually (see + # _token_cache_clear docstring), so drop the whole cache. + _token_cache_clear() + # A brand-new token may sit in the negative cache from a pre-mint probe; + # clear so it authenticates immediately. + _token_cache_forget(_token_cache_key(token, database)) return token, created_at +def _revoke_via_service( + tenant_id: str, squad_url: str, system_token: str +) -> dict[str, Any] | None: + """POST /auth/revoke on the LIVE running service so its in-process cache + is actually cleared — the CLI's own cache is a separate, always-empty + process and clearing it (what the old code did) clears nothing real. + + Returns the service's parsed response body on a genuine 200, else None + (no token configured, connection refused, timeout, non-200, unparseable + body). The caller must never report a cleared cache on None — nor on a + 200 whose ``cache_flushed`` is not exactly True (BLOCK-C, sos-205-f1a3aee4 + gate-5). + + BLOCK-C: this returned a bare ``status_code == 200`` bool, which became a + false receipt the moment P2-G changed the throttled branch from a 429 to a + 200 with ``cache_flushed: false``. A throttled revoke read as full success + and the CLI printed ``cache_cleared=service`` while the credential kept + authenticating from cache for the rest of the positive TTL. The status code + stopped carrying the property the caller asserts, so the body must be + returned and inspected — same class as BLOCK-2 above (a receipt may only + claim state its emitter can observe). + """ + if not system_token: + return None + try: + resp = requests.post( + f"{squad_url.rstrip('/')}/auth/revoke", + json={"tenant_id": tenant_id}, + headers={"Authorization": f"Bearer {system_token}"}, + timeout=5, + ) + except requests.RequestException: + return None + if resp.status_code != 200: + return None + try: + body = resp.json() + except ValueError: + return None + return body if isinstance(body, dict) else None + + def _cli() -> int: parser = argparse.ArgumentParser(description="Squad Service auth tooling") sub = parser.add_subparsers(dest="command", required=True) @@ -255,15 +775,66 @@ def _cli() -> int: generate = sub.add_parser("generate", help="Generate a tenant API key") generate.add_argument("--tenant", required=True, help="Tenant identifier") generate.add_argument("--identity-type", default="user", choices=["user", "agent", "service"]) + generate.add_argument( + "--rotate", + action="store_true", + help="Retire this tenant's existing keys of the same identity-type before minting", + ) + + # BLOCK-2 fix (sos-205-a7c2fc44 adversarial gate): the service had no + # revoke path at all — this is it. + revoke = sub.add_parser("revoke", help="Revoke all API keys for a tenant") + revoke.add_argument("--tenant", required=True, help="Tenant identifier") args = parser.parse_args() if args.command == "generate": - token, created_at = create_api_key(args.tenant, args.identity_type) + token, created_at = create_api_key( + args.tenant, args.identity_type, rotate=args.rotate + ) print(f"api_key={token}") print(f"tenant_id={args.tenant}") print(f"identity_type={args.identity_type}") print("permissions=tenant-scoped kernel capabilities") print(f"created_at={created_at}") + print(f"rotated={args.rotate}") + return 0 + if args.command == "revoke": + deleted = revoke_api_key(args.tenant) + print(f"tenant_id={args.tenant}") + print(f"revoked_rows={deleted}") + # BLOCK-2 fix (sos-205-b5307dd7 re-gate): revoke_api_key() above only + # clears the CLI's OWN in-process cache, which is always empty — the + # CLI is a separate process from the running uvicorn service and has + # no way to reach the service's _TOKEN_CACHE_POSITIVE/_NEGATIVE + # directly. Printing cache_cleared=true unconditionally (the old + # behaviour) was a false receipt: a revoked-but-cached token kept + # authenticating against the live service for up to the positive TTL + # (measured: 30s). Try the service's own /auth/revoke route first — + # that runs in-process on the server and can actually clear it. Only + # fall back to "we deleted the DB rows but can't prove the cache is + # clear" when the service is unreachable. + squad_url = os.getenv("SQUAD_URL", "http://localhost:8060") + # BLOCK-C fix (sos-205-f1a3aee4 gate-5): a 200 is NOT proof the cache + # was flushed — P2-G's throttled branch answers 200 with + # cache_flushed=false. Assert on the field that carries the property, + # never the status code, and print the service's own retry_after so + # the operator knows exactly when a real flush becomes possible. + service_resp = _revoke_via_service(args.tenant, squad_url, SYSTEM_TOKEN) + if service_resp is not None and service_resp.get("cache_flushed") is True: + print("cache_cleared=service") + elif service_resp is not None: + retry_after = service_resp.get("retry_after") + suffix = f"; retry after {retry_after}s" if retry_after is not None else "" + print( + "cache_cleared=THROTTLED-NOT-FLUSHED — DB rows deleted, but the " + "running service declined the flush, so entries already cached " + f"there may authenticate for up to 30s (positive TTL){suffix}" + ) + else: + print( + "cache_cleared=LOCAL-PROCESS-ONLY — running service still holds " + "cached entries up to 30s TTL; hit POST /auth/revoke or restart" + ) return 0 return 1 diff --git a/sos/services/squad/roles.py b/sos/services/squad/roles.py index f53b67960..3059d8b96 100644 --- a/sos/services/squad/roles.py +++ b/sos/services/squad/roles.py @@ -52,7 +52,7 @@ def create_role( if "UNIQUE" in str(exc): raise RoleDuplicateError(f"Role '{name}' already exists in project '{project_id}'") from exc raise - return self._get_role_row(role_id) + return self._get_role_row(role_id, tenant_id=tenant_id) def list_roles(self, project_id: str, *, tenant_id: str = "default") -> list[dict]: with self.db.connect() as conn: @@ -62,14 +62,44 @@ def list_roles(self, project_id: str, *, tenant_id: str = "default") -> list[dic ).fetchall() return [dict(r) for r in rows] - def get_role(self, role_id: str) -> dict: - return self._get_role_row(role_id) + def get_role(self, role_id: str, *, tenant_id: str | None) -> dict: + return self._get_role_row(role_id, tenant_id=tenant_id) - def _get_role_row(self, role_id: str) -> dict: + def _get_role_row(self, role_id: str, *, tenant_id: str | None) -> dict: + """Fetch a role row by id. + + P0-B fix (sos-205-47f5f8c2 gate-3): `tenant_id=None` means + UNRESTRICTED lookup — reserved for system-tier callers + (AuthContext.tenant_scope is None only when is_system=True). Any + other value scopes the lookup to that tenant, same as + SquadService.get()'s `tenant_id: str | None = DEFAULT_TENANT_ID` + pattern. A role that exists but belongs to a different tenant raises + the SAME RoleNotFoundError as a role that doesn't exist at all — the + route must not let a caller distinguish "not found" from "not + yours". + + P2-F fix (sos-205-790a2a63 gate-4): `tenant_id` no longer defaults to + `None`. A `str | None = None` default made "I forgot to scope this + call" and "I deliberately want every tenant" the SAME call shape — + and two call sites (BLOCK-B's `assign_role`, P2-E's + `get_token_roles`) forgot it IN THE SAME COMMIT that added the + kwarg. `None` is still a legal value — it is the explicit, + documented system-tier spelling above — but every caller must now + STATE it. Omitting the keyword is a `TypeError` at call time (or an + import-time break for any caller that got missed), not a silent + cross-tenant read. Same change applied to every sibling method below + that takes `tenant_id`. + """ with self.db.connect() as conn: - row = conn.execute( - "SELECT * FROM roles WHERE id = ?", (role_id,) - ).fetchone() + if tenant_id is None: + row = conn.execute( + "SELECT * FROM roles WHERE id = ?", (role_id,) + ).fetchone() + else: + row = conn.execute( + "SELECT * FROM roles WHERE id = ? AND tenant_id = ?", + (role_id, tenant_id), + ).fetchone() if not row: raise RoleNotFoundError(f"Role {role_id} not found") return dict(row) @@ -78,8 +108,8 @@ def _get_role_row(self, role_id: str) -> dict: # Permissions # ------------------------------------------------------------------ - def add_permission(self, role_id: str, permission: str) -> dict: - self._get_role_row(role_id) # raises if missing + def add_permission(self, role_id: str, permission: str, *, tenant_id: str | None) -> dict: + self._get_role_row(role_id, tenant_id=tenant_id) # raises if missing or foreign-tenant with self.db.connect() as conn: conn.execute( "INSERT OR IGNORE INTO role_permissions (role_id, permission) VALUES (?, ?)", @@ -87,7 +117,8 @@ def add_permission(self, role_id: str, permission: str) -> dict: ) return {"role_id": role_id, "permission": permission} - def remove_permission(self, role_id: str, permission: str) -> None: + def remove_permission(self, role_id: str, permission: str, *, tenant_id: str | None) -> None: + self._get_role_row(role_id, tenant_id=tenant_id) # raises if missing or foreign-tenant with self.db.connect() as conn: conn.execute( "DELETE FROM role_permissions WHERE role_id = ? AND permission = ?", @@ -120,7 +151,7 @@ def caller_max_rank(self, caller_id: str) -> int: ).fetchone() return row["max_rank"] if row and row["max_rank"] is not None else 0 - def check_can_assign(self, caller_id: str, target_role_id: str) -> None: + def check_can_assign(self, caller_id: str, target_role_id: str, *, tenant_id: str | None) -> None: """Raise RolePrivilegeError if caller cannot assign target_role_id. Rule: caller's max rank must be >= target role's rank. @@ -130,10 +161,17 @@ def check_can_assign(self, caller_id: str, target_role_id: str) -> None: not a backdoor. Without it, seeding the first principal would require an existing principal to assign them (infinite regress). The system bearer is never issued to end-users; it is held only by the service runtime. + + BLOCK-B fix (sos-205-790a2a63 gate-4): this used to call + `self._get_role_row(target_role_id)` with NO `tenant_id`, which + defaulted to the fail-open `None` = unrestricted lookup — the actual + cross-tenant hole (a foreign tenant could look up, and then assign, + another tenant's role_id). `tenant_id` is now required and forwarded + straight through, same scoping as every sibling lookup. """ if caller_id.startswith("system:") or caller_id == "system": return - target_role = self._get_role_row(target_role_id) + target_role = self._get_role_row(target_role_id, tenant_id=tenant_id) target_rank: int = target_role.get("rank", 0) if target_rank == 0: return # unranked role — no restriction @@ -153,14 +191,27 @@ def assign_role( role_id: str, assignee_id: str, *, + tenant_id: str | None, assignee_type: str = "agent", assigned_by: str, caller_id: Optional[str] = None, ) -> dict: - """Assign role_id to assignee_id. If caller_id is provided, rank check is enforced.""" + """Assign role_id to assignee_id. If caller_id is provided, rank check is enforced. + + BLOCK-B fix (sos-205-790a2a63 gate-4): this was the 6th RBAC route on + this surface and the only one the P0-B fix (sos-205-47f5f8c2) missed + — it called `self._get_role_row(role_id)` with no `tenant_id`, which + defaulted to unrestricted, so ANY tenant's valid api key could plant + a role_assignment row into ANOTHER tenant's role (and the target + tenant's own `revoke_assignment`/`add_permission` calls ARE scoped, + so the planted row was also attacker-unremovable by anyone but the + victim tenant or system). `tenant_id` is now required and forwarded + to both the existence check below and `check_can_assign`'s internal + lookup, identical to the five siblings. + """ if caller_id: - self.check_can_assign(caller_id, role_id) - self._get_role_row(role_id) + self.check_can_assign(caller_id, role_id, tenant_id=tenant_id) + self._get_role_row(role_id, tenant_id=tenant_id) # raises if missing or foreign-tenant assigned_at = now_iso() with self.db.connect() as conn: conn.execute( @@ -179,14 +230,16 @@ def assign_role( "assigned_by": assigned_by, } - def revoke_assignment(self, role_id: str, assignee_id: str) -> None: + def revoke_assignment(self, role_id: str, assignee_id: str, *, tenant_id: str | None) -> None: + self._get_role_row(role_id, tenant_id=tenant_id) # raises if missing or foreign-tenant with self.db.connect() as conn: conn.execute( "DELETE FROM role_assignments WHERE role_id = ? AND assignee_id = ?", (role_id, assignee_id), ) - def list_assignments(self, role_id: str) -> list[dict]: + def list_assignments(self, role_id: str, *, tenant_id: str | None) -> list[dict]: + self._get_role_row(role_id, tenant_id=tenant_id) # raises if missing or foreign-tenant with self.db.connect() as conn: rows = conn.execute( "SELECT * FROM role_assignments WHERE role_id = ? ORDER BY assigned_at", @@ -194,21 +247,47 @@ def list_assignments(self, role_id: str) -> list[dict]: ).fetchall() return [dict(r) for r in rows] - def get_agent_roles(self, assignee_id: str) -> list[dict]: - """All roles held by an agent across all projects.""" + def get_agent_roles(self, assignee_id: str, *, tenant_id: str | None) -> list[dict]: + """All roles held by an agent across all projects. + + P0-B fix (sos-205-47f5f8c2 gate-3): `tenant_id=None` (system-tier + only) returns roles across every tenant, matching the pre-fix + behaviour. Any other value filters the join to `r.tenant_id`, so a + tenant-scoped caller can no longer enumerate an agent's roles in a + tenant it doesn't own. + """ with self.db.connect() as conn: - rows = conn.execute( - """ - SELECT r.*, ra.assignee_type, ra.assigned_at, ra.assigned_by - FROM role_assignments ra - JOIN roles r ON r.id = ra.role_id - WHERE ra.assignee_id = ? - ORDER BY r.project_id, r.name - """, - (assignee_id,), - ).fetchall() + if tenant_id is None: + rows = conn.execute( + """ + SELECT r.*, ra.assignee_type, ra.assigned_at, ra.assigned_by + FROM role_assignments ra + JOIN roles r ON r.id = ra.role_id + WHERE ra.assignee_id = ? + ORDER BY r.project_id, r.name + """, + (assignee_id,), + ).fetchall() + else: + rows = conn.execute( + """ + SELECT r.*, ra.assignee_type, ra.assigned_at, ra.assigned_by + FROM role_assignments ra + JOIN roles r ON r.id = ra.role_id + WHERE ra.assignee_id = ? AND r.tenant_id = ? + ORDER BY r.project_id, r.name + """, + (assignee_id, tenant_id), + ).fetchall() return [dict(r) for r in rows] def get_token_roles(self, tenant_id: str) -> list[dict]: - """All roles assigned to the identity matching tenant_id (for /me/roles).""" - return self.get_agent_roles(tenant_id) + """All roles assigned to the identity matching tenant_id (for /me/roles). + + P2-E fix (sos-205-790a2a63 gate-4): this called `get_agent_roles` + (assignee_id) WITHOUT the new `tenant_id` kwarg, which fell through + to the fail-open `None` default and returned every tenant's matching + role rows — /me/roles for tenant B disclosed tenant A's role. Now + forwarded explicitly, scoping the lookup to the caller's own tenant. + """ + return self.get_agent_roles(tenant_id, tenant_id=tenant_id) diff --git a/sovereign/brain.py b/sovereign/brain.py index 79713d628..0966dc27d 100644 --- a/sovereign/brain.py +++ b/sovereign/brain.py @@ -28,6 +28,7 @@ import json import time import logging +import unicodedata import requests from collections import deque from datetime import datetime, timezone @@ -82,6 +83,7 @@ from kernel.config import ( MIRROR_URL, MIRROR_TOKEN, SQUAD_URL, SOS_ENGINE_URL, BRAIN_TENANT_SCOPE, BRAIN_SCOPE_TYPE, BRAIN_TOKEN_BUDGET, + MUPOT_MCP_URL, MUPOT_BRAIN_TOKEN, ) # ── MemoryPort — memory I/O routes through this adapter (#267 K1) ────────── # All four /store and /search call sites in this file are ported: @@ -159,6 +161,59 @@ def _assert_in_scope(project: str) -> None: _AGENT_HOME_CACHE_TS: float = 0.0 _AGENT_HOME_TTL = 300.0 # seconds +# Zero-width / invisible characters `.strip()` does not remove: zero-width +# space, zero-width non-joiner, zero-width joiner, BOM/zero-width no-break +# space. Part of the P2-D fix below. +_ZERO_WIDTH_CHARS = ("​", "‌", "‍", "") + + +def _normalize_agent_subject(agent: object) -> str | None: + """Normalize an untrusted `agent` value into a roster lookup key, or + None if it isn't one. + + P2-D fix (sos-205-47f5f8c2 gate-3): `agent` originates from the LLM + decision JSON (`action.get("agent", ...)`) and used to reach the + capability gate through a bare `str(agent).strip().lower()`. That + defended against nothing: a zero-width space, a Turkish dotless-i + homoglyph, a trailing dot/slash, an embedded space, or a non-str value + (None / 0 / a list / a dict) each turned a KNOWN tenant-bound agent name + into a roster MISS — and `_agent_home_tenant` treats a miss as "no home + tenant = ungated colony agent". Two non-str cases (list, dict) are worse + than a silent miss: `_agent_available`'s `agent not in _AGENT_SESSION` + check on an unhashable value (list/dict) raises TypeError uncaught, + crashing the whole brain cycle before this gate is even reached. + + NFKC + stripped zero-width chars + case-fold closes the mutation class + for values that ARE meant to match a roster entry. It does NOT resolve + genuine Unicode confusables (e.g. dotless-i is a distinct codepoint, not + NFKC-equivalent to 'i') — those still normalize to a string that simply + doesn't match anything in the roster, which is the SAME safe outcome an + honestly-unknown agent name already gets today (unknown → colony/shared, + per `_agent_home_tenant`'s documented contract). This function only + upgrades "reachable but silently wrong" to "handled the same way as any + other unrecognized string" and rejects non-str/empty input outright + instead of coercing it with `str(...)`. + + IMPORTANT: this normalization does NOT make the capability gate the + enforcing layer. `_agent_available` (`_AGENT_SESSION`, an exact-match + whitelist, default-deny) is what actually decides dispatchability — see + motor_execute, which checks it before any gate call. Treating a gate as + the enforcer instead of the roster was the exact vacuous-gate mistake + already made once on this code path (sos-205-a7c2fc44). + """ + if not isinstance(agent, str): + return None + normalized = unicodedata.normalize("NFKC", agent) + for ch in _ZERO_WIDTH_CHARS: + normalized = normalized.replace(ch, "") + # WARN-I/LOW-J fix (sos-205-790a2a63 gate-4): casefold(), not lower() — + # this is the ONE normalization pipeline; see below for where it is now + # ALSO applied to the roster keys it gets compared against (both + # `_AGENT_HOME_CACHE` and `_AGENT_SESSION`), so no widening happens on + # only one side of a comparison. + normalized = normalized.strip().casefold() + return normalized or None + def _agent_home_tenant(agent: str) -> str | None: """Resolve an agent's home tenant (its AgentDef.project) via the squad @@ -180,11 +235,23 @@ def _agent_home_tenant(agent: str) -> str | None: r.raise_for_status() payload = r.json() rows = payload.get("agents", payload) if isinstance(payload, dict) else payload - mapping = { - str(row.get("name", "")).strip().lower(): str(row.get("project", "") or "").strip().lower() - for row in rows - if str(row.get("name", "")).strip() - } + # LOW-J fix (sos-205-790a2a63 gate-4): the roster KEY used to be + # built with a bare `.strip().lower()` while the LOOKUP side + # (`_normalize_agent_subject`, called just below) ran NFKC + + # zero-width-strip + casefold. Normalizing only one side of a + # comparison is worse than normalizing neither: a roster entry + # whose registered name is not already NFKC-normal (fullwidth, + # zero-width-padded, etc.) became UNREACHABLE in this map, + # resolved to `home=None`, and was treated as an ungated colony + # agent — the opposite of default-deny. Route the roster key + # through the SAME `_normalize_agent_subject` function used for + # the lookup key so both sides always agree. + mapping = {} + for row in rows: + name = _normalize_agent_subject(row.get("name", "")) + if not name: + continue + mapping[name] = str(row.get("project", "") or "").strip().casefold() if mapping: _AGENT_HOME_CACHE = mapping _AGENT_HOME_CACHE_TS = now @@ -193,7 +260,12 @@ def _agent_home_tenant(agent: str) -> str | None: logger.error(f"[capability-gate] agent resolver cold-start failed — failing safe to static tenant-bound set: {exc}") else: logger.warning(f"[capability-gate] agent resolver refresh failed — using stale roster: {exc}") - key = str(agent).strip().lower() + # P2-D fix: normalize (see _normalize_agent_subject). A non-str/empty + # subject has no home tenant to report — the caller-side default-deny + # roster check is what actually gates dispatch of such a value; this + # function's contract is "resolve a home tenant or None", not "decide + # dispatchability". + key = _normalize_agent_subject(agent) or "" if _AGENT_HOME_CACHE: home = _AGENT_HOME_CACHE.get(key, "") else: @@ -368,7 +440,7 @@ def prefrontal_think(context: str) -> str: {{ "action": "one-line description of what to do", "goal_id": "which goal this advances (or 'maintenance')", - "agent": "which agent should do it (kasra/athena/sol/dandan/system)", + "agent": "which agent should do it (kasra/system)", "method": "how to do it (create_task/post_content/send_outreach/fix_code/research)", "details": "specific instructions for the executing agent", "expected_progress": 0.1, @@ -549,23 +621,62 @@ def _task_governor_allows() -> bool: return True -# Agent name → tmux session name (empty string = system/no session needed) +# Agent name → tmux session name (empty string = system/no session needed). +# Active roster per Hadi directive 2026-07-27: kasra + system only. Paused +# agents (athena/river/sol/dandan) are intentionally absent — dispatching to +# them produced the "no tmux session" self-investigation loop. _AGENT_SESSION: dict[str, str] = { "kasra": "kasra", - "athena": "athena", - "river": "river", - "sol": "sol", - "dandan": "dandan", "system": "", } +# WARN-I fix (sos-205-790a2a63 gate-4): motor_execute normalizes `agent` via +# `_normalize_agent_subject` (NFKC + zero-width-strip + casefold) BEFORE +# `_agent_available` sees it, but `_AGENT_SESSION`'s keys above are plain +# literals — an asymmetric comparison, same defect class as LOW-J. There are +# two honest fixes for a one-sided comparison: normalize neither side, or +# normalize both. We choose BOTH, deliberately, and document it here rather +# than let it be an accident of whatever mutation the model's JSON happens +# to contain: run the roster's own keys through the SAME +# `_normalize_agent_subject` pipeline the presented subject already goes +# through. Effect: 'KASRA', fullwidth 'ｋａｓｒａ', and a zero-width-padded +# 'kasra​' all resolve to the same roster entry as 'kasra' — a +# DELIBERATE widening, not a silent one. No privilege gain today +# (_AGENT_SESSION only holds 'kasra'/'system', neither tenant-bound) — but +# this is the correct posture for the day a tenant-bound agent joins this +# roster, when the unknown-subject gate (P2-H, not yet closed — see +# `_normalize_agent_subject`'s docstring) is the only thing standing between +# a mutated spelling and dispatch. +_AGENT_SESSION_NORMALIZED: dict[str, str] = { + key: value + for raw_key, value in _AGENT_SESSION.items() + for key in (_normalize_agent_subject(raw_key),) + if key is not None +} + def _agent_available(agent: str) -> bool: - """Return True if the agent has a running tmux session (or needs none).""" + """Return True if the agent is on the active roster and reachable. + + Default-deny: an agent not in the normalized _AGENT_SESSION roster is + NOT dispatchable, regardless of what the model proposes. A failed tmux + probe also counts as unavailable — assuming available on error re-opens + the ghost loop. + + WARN-I fix (sos-205-790a2a63 gate-4): checks `_AGENT_SESSION_NORMALIZED` + (roster keys run through `_normalize_agent_subject`) rather than raw + `_AGENT_SESSION`, and normalizes `agent` itself too — so this stays + correct even if a future caller invokes it directly with an unnormalized + subject, not only through motor_execute's existing normalize-then-check + call order. + """ import subprocess - session = _AGENT_SESSION.get(agent, "") + normalized_agent = _normalize_agent_subject(agent) + if normalized_agent is None or normalized_agent not in _AGENT_SESSION_NORMALIZED: + return False + session = _AGENT_SESSION_NORMALIZED[normalized_agent] if not session: - return True # system / unknown agents — no session requirement + return True # system — no session requirement try: result = subprocess.run( ["tmux", "has-session", "-t", session], @@ -573,7 +684,57 @@ def _agent_available(agent: str) -> bool: ) return result.returncode == 0 except Exception: - return True # assume available if we can't check + return False + + +def _mupot_dispatch_task(squad_id: str, title: str, description: str, priority: str, labels: list) -> dict: + """ + Create a task on mupot's REAL, live board (agent-bound token, the + 'sovereign' identity minted 2026-07-22) instead of the legacy SQUAD_URL + board that mupot's own operator loop never reads. Deliberately supplies + NO assignee -- mupot's own routeUnassignedWork (src/tasks/effort-route.ts) + picks the builder from the live, current roster (kasra/cursor/codex/agy/ + kayhermes), not brain's own free-text guess against a stale hardcoded + hint (the #490 root cause). Returns the same {"success","result","task_id"} + shape the legacy SQUAD_URL callers already expect, so callers don't change. + """ + if not MUPOT_MCP_URL or not MUPOT_BRAIN_TOKEN: + logger.warning("mupot dispatch skipped: MUPOT_MCP_URL/MUPOT_BRAIN_TOKEN not configured") + return {"success": False, "result": "mupot not configured", "task_id": None} + done_when = f"Task '{title[:80]}' is completed, with a receipt reflecting success or failure." + try: + r = requests.post( + MUPOT_MCP_URL, + headers={"Authorization": f"Bearer {MUPOT_BRAIN_TOKEN}", "Content-Type": "application/json"}, + json={ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { + "name": "task_create", + "arguments": { + "squad_id": squad_id, + "title": title, + "body": f"{description}\n\n[brain-generated, priority={priority}, labels={','.join(labels)}]", + "done_when": done_when, + }, + }, + }, + timeout=10, + ) + data = r.json() + content = data.get("result", {}).get("content", [{}]) + text = content[0].get("text", "{}") if content else "{}" + payload = json.loads(text) + if not payload.get("ok"): + logger.error(f"mupot task_create failed: {payload}") + return {"success": False, "result": f"mupot task_create failed: {payload}", "task_id": None} + task = payload.get("result", {}).get("task", {}) + task_id = task.get("id", "?") + return {"success": True, "result": f"mupot task created: {task_id} (squad={squad_id})", "task_id": task_id} + except Exception as e: + logger.error(f"mupot dispatch exception: {e}") + return {"success": False, "result": f"mupot dispatch failed: {e}", "task_id": None} def motor_execute(action: dict) -> dict: @@ -584,19 +745,52 @@ def motor_execute(action: dict) -> dict: """ method = action.get("method", "") details = action.get("details", "") - agent = action.get("agent", "system") action_title = action.get("action", "") + # P2-D fix (sos-205-47f5f8c2 gate-3): `agent` is untrusted LLM-decision + # JSON and flows into every `_capability_block`/`_agent_home_tenant` call + # below, plus the `_agent_available` roster check. Normalize it HERE, + # once, before anything downstream sees it — see + # `_normalize_agent_subject` for the full mutation-class rationale. A + # non-str/empty subject (None, 0, a list, a dict — none of these are a + # legitimate "agent" field) is rejected outright rather than silently + # coerced via `str(agent)`: unhashable values (list/dict) used to reach + # `_agent_available`'s `agent not in _AGENT_SESSION` dict check and raise + # an uncaught TypeError there, crashing the whole cycle. Skipping here + # returns the same calm, non-error shape every other "nothing to do this + # cycle" branch in this function uses. + raw_agent = action.get("agent", "system") + agent = _normalize_agent_subject(raw_agent) + if agent is None: + logger.info(f"Brain proposed a non-string/empty agent subject ({raw_agent!r}) — skipping: {action_title[:60]}") + return { + "success": True, + "skipped": True, + "result": "Decision-layer proposed an invalid agent subject; task intentionally not dispatched. This is expected when the model's JSON is malformed — do not investigate.", + } + blocked_reason = _blocked_stale_cleanup_reason(action) if blocked_reason is not None: logger.warning(f"Blocked brain action: {blocked_reason}: {action_title[:120]}") return {"success": True, "result": f"Skipped stale brain directive: {blocked_reason}"} if method not in _SUPPORTED_BRAIN_METHODS: - return {"success": False, "result": f"Unsupported brain method: {method}"} + # skipped=True + non-error phrasing: a hallucinated method name is a + # decision-layer miss, not a system fault. Error-shaped text here fed + # "investigate unsupported method" proposals in following cycles. + return {"success": True, "skipped": True, "result": f"Method '{method}' is not in the supported set; action intentionally not executed. Pick only from the documented methods — do not investigate."} # Agent availability check — skip if the target agent has no running session if not _agent_available(agent): + # WARN-I fix: check the same normalized roster _agent_available just + # checked, not the raw dict — `agent` here is already normalized + # (see above), so this stays consistent with the actual gate result. + if agent not in _AGENT_SESSION_NORMALIZED: + logger.info(f"Agent '{agent}' not on active roster — skipping task: {action_title[:60]}") + # Deliberate, not an error: paused agents are expected to be absent. + # Phrasing avoids "error"/"no tmux session" so the next brain cycle + # does not propose investigating its own roster policy. + return {"success": True, "result": f"Agent '{agent}' is paused by roster policy; task intentionally not dispatched. This is expected — do not investigate."} logger.info(f"Agent '{agent}' has no active session — skipping task: {action_title[:60]}") return {"success": True, "result": f"Agent '{agent}' unavailable (no tmux session). Task skipped."} @@ -671,6 +865,8 @@ def motor_execute(action: dict) -> dict: return block if squad_id: + if normalize_project(project) == "mumega": + return _mupot_dispatch_task(squad_id, title, details, "high", labels) # Route through Squad Service — project isolation import uuid task_id = f"brain-{uuid.uuid4().hex[:8]}" @@ -732,6 +928,8 @@ def motor_execute(action: dict) -> dict: if (block := _capability_block(outreach_assignee, outreach_project)) is not None: return block if squad_id: + if normalize_project(outreach_project) == "mumega": + return _mupot_dispatch_task(squad_id, f"Outreach: {action.get('action', '')}", details, "medium", outreach_labels) import uuid task_id = f"brain-{uuid.uuid4().hex[:8]}" r = requests.post(f"{SQUAD_URL}/tasks", json={ @@ -764,6 +962,8 @@ def motor_execute(action: dict) -> dict: if (block := _capability_block(code_assignee, project)) is not None: return block if squad_id: + if normalize_project(project) == "mumega": + return _mupot_dispatch_task(squad_id, f"Fix: {action.get('action', '')}", details, "high", code_labels) import uuid task_id = f"brain-{uuid.uuid4().hex[:8]}" r = requests.post(f"{SQUAD_URL}/tasks", json={ @@ -789,9 +989,38 @@ def motor_execute(action: dict) -> dict: return {"success": True, "result": "Code task created for Kasra"} elif method == "research": - # Create research task for River (shared/colony agent — gate for uniformity) - if (block := _capability_block("river", project)) is not None: + # BLOCK-4 fix (sos-205-a7c2fc44 adversarial gate): the mumega + # early-return used to sit BEFORE this gate and return first, so + # for project=="mumega" the colony capability gate + # (_assert_agent_in_tenant) was skipped entirely — the only + # branch among create_task/send_outreach/fix_code/research that + # did. Gate first, unconditionally, before branching on the + # dispatch target, matching every other branch. + # + # Re-gate fix (sos-205-b5307dd7): the gate subject used to be the + # hardcoded literal "river". _agent_home_tenant('river') is + # always None (river is a shared/colony agent, not in the + # tenant-bound roster), so _capability_block("river", project) + # could NEVER return DENY at any position in this function — it + # was structurally in the right place but gating a subject that + # can't fail, i.e. decorative. Gate the real `agent` variable + # instead: the entity this dispatch believes is acting (already + # resolved through the PROJECT_LEADS reroute above, same as + # every other branch's assignee). + if (block := _capability_block(agent, project)) is not None: return block + if normalize_project(project) == "mumega": + # Hardcoding a name as the DISPATCH target (not the gate + # subject, fixed above) was the exact #490 root-cause pattern + # (a stale roster assumption, not a live check) -- defer to + # mupot's own effort-router for WHO does the work. "squad-core" + # is research's fixed mumega squad target: LABEL_SQUAD_MAP has + # no "research" entry, so the generic `squad_id` resolved + # above is always None for this method and is deliberately + # not reused here. + research_squad_id = "squad-core" + return _mupot_dispatch_task(research_squad_id, f"Research: {action.get('action', '')}", details, "medium", ["research", "brain-generated"]) + # Create research task for River (shared/colony agent — gate for uniformity) r = requests.post(f"{MIRROR_URL}/tasks", json={ "title": f"Research: {action.get('action', '')}", "agent": "river", @@ -1041,6 +1270,15 @@ def report_to_discord(action: dict, result: dict): f"{reason_line}" ) + # Escalation-only emission (Hadi directive 2026-07-27): kasra is the + # repair escalation path, not the brain's activity feed. Routine cycles + # (executed housekeeping, dedup/roster/mode-off skips) stay in the journal; + # only failures — the repairable class — page out. The kasra bus inbox is + # bridged to Hadi's Telegram, so every message here is a phone ping. + logger.info(f"brain-cycle {status_word}: {summary[:120]}") + if status_word != "failed": + return + try: from kernel.bus import send as bus_send if not bus_send(to="kasra", text=msg): diff --git a/sovereign/kernel/config.py b/sovereign/kernel/config.py index 993a18840..78c3f9117 100644 --- a/sovereign/kernel/config.py +++ b/sovereign/kernel/config.py @@ -26,6 +26,17 @@ SQUAD_URL = os.getenv("SQUAD_URL", "http://localhost:8060") +# mupot's live MCP endpoint + agent-bound "sovereign" identity token, used by +# brain.py's _mupot_dispatch_task (added a7c2fc44 / sos PR #205). Both were +# referenced via `from kernel.config import MUPOT_MCP_URL, MUPOT_BRAIN_TOKEN` +# without ever being defined here, which made `import brain` raise +# ImportError unconditionally — found while testing the BLOCK-4 fix on +# PR #205 (2026-07-27). Empty-string defaults are intentional: +# _mupot_dispatch_task already warns + no-ops when either is unset, so an +# unconfigured environment degrades the same way it was designed to. +MUPOT_MCP_URL = os.getenv("MUPOT_MCP_URL", "") +MUPOT_BRAIN_TOKEN = os.getenv("MUPOT_BRAIN_TOKEN", "") + SOS_ENGINE_URL = os.getenv("SOS_ENGINE_URL", "http://localhost:6060") REDIS_URL = os.getenv("REDIS_URL", "redis://localhost:6379/0") diff --git a/tests/brain/test_capability_gate.py b/tests/brain/test_capability_gate.py index 32cf4b3ec..ce7819f8d 100644 --- a/tests/brain/test_capability_gate.py +++ b/tests/brain/test_capability_gate.py @@ -246,3 +246,233 @@ def test_motor_execute_blocks_cross_tenant_outreach(monkeypatch): assert res["success"] is False assert "Capability scope violation" in res["result"] assert posts == [] + + +# ── BLOCK-4 regression (sos-205-a7c2fc44 adversarial gate) ──────────────────── +# research's mumega branch used to early-return via _mupot_dispatch_task +# BEFORE _capability_block ran, skipping the colony gate entirely for +# method="research" + project="mumega" — the only branch that did. +# +# Re-gate update (sos-205-b5307dd7): the gate subject used to be the +# hardcoded literal "river", which has no home tenant in ANY roster +# (_agent_home_tenant('river') is always None) — so a test that made "river" +# tenant-bound and then dispatched with agent="kasra" was only exercising +# the OLD hardcoded subject, not the real one. The fix gates the real +# `agent` value from the action, so these tests now drive the gate through +# that same real subject — including a genuinely tenant-bound agent (digid) +# to prove the gate can still deny. + +def test_motor_execute_blocks_research_mumega_when_gate_subject_tenant_bound(monkeypatch): + # digid is tenant-bound to project "digid" (non-None home tenant). + # Dispatch a mumega research directive AS digid: this must be blocked, + # and _mupot_dispatch_task must never be called. + roster = {"agents": _ROSTER["agents"] + [ + {"name": "digid", "project": "digid", "role": "SPECIALIST", "type": "OPENCLAW"}, + ]} + _patch_dispatch(monkeypatch) + _patch_roster(monkeypatch, payload=roster) + called = {"mupot": False} + + def _spy_dispatch(*a, **k): + called["mupot"] = True + return {"success": True, "result": "should never run"} + + monkeypatch.setattr(brain, "_mupot_dispatch_task", _spy_dispatch) + res = brain.motor_execute(_action("research", "digid", goal="goal_mumega", details="look into X")) + assert res["success"] is False + assert "Capability scope violation" in res["result"] + assert called["mupot"] is False # gate must run BEFORE dispatch, not after + + +def test_motor_execute_research_mumega_dispatches_when_gate_allows(monkeypatch): + # kasra is shared/colony (project="" in the roster → no home tenant) → + # gate passes on the real acting agent → mumega research still routes to + # mupot via the guarded "squad-core" target, exactly like before the fix. + _patch_dispatch(monkeypatch) + _patch_roster(monkeypatch) + called = {} + + def _fake_dispatch(squad_id, title, description, priority, labels): + called["squad_id"] = squad_id + return {"success": True, "result": f"dispatched to {squad_id}"} + + monkeypatch.setattr(brain, "_mupot_dispatch_task", _fake_dispatch) + res = brain.motor_execute(_action("research", "kasra", goal="goal_mumega", details="look into Y")) + assert res["success"] is True + assert called["squad_id"] == "squad-core" + + +def test_motor_execute_research_non_mumega_still_gates_and_dispatches_mirror(monkeypatch): + # Non-mumega research path is unchanged by the fix: gate on the real + # acting agent (kasra, shared → no home tenant), then Mirror dispatch, + # no mupot call. + posts = _patch_dispatch(monkeypatch) + monkeypatch.setattr(brain, "_mupot_dispatch_task", lambda *a, **k: pytest.fail("mupot must not be called for non-mumega research")) + res = brain.motor_execute(_action("research", "kasra", goal="goal_gaf", details="look into Z")) + assert res["success"] is True + assert len(posts) == 1 + assert posts[0][0] == f"{brain.MIRROR_URL}/tasks" + + +def test_motor_execute_blocks_research_non_mumega_cross_tenant(monkeypatch): + # New (sos-205-b5307dd7): the real-subject gate must also fire on the + # non-mumega dispatch path, not just mumega — digid dispatched for a + # DIFFERENT tenant's goal must be blocked before any Mirror POST. + # Project deliberately has NO PROJECT_LEADS entry so `agent` is not + # rerouted before the gate runs (gaf/dentalnearyou/etc. would silently + # swap "digid" for their own lead, which is a routing property, not part + # of what this test is checking). + posts = _patch_dispatch(monkeypatch) + roster = {"agents": _ROSTER["agents"] + [ + {"name": "digid", "project": "digid", "role": "SPECIALIST", "type": "OPENCLAW"}, + ]} + _patch_roster(monkeypatch, payload=roster) + monkeypatch.setattr(brain, "_mupot_dispatch_task", lambda *a, **k: pytest.fail("mupot must not be called for this project")) + res = brain.motor_execute(_action("research", "digid", goal="goal_unknown-tenant", details="look into W")) + assert res["success"] is False + assert "Capability scope violation" in res["result"] + assert posts == [] + + +# ── P2-D: agent-subject normalization (sos-205-47f5f8c2 gate-3) ──────────── +# `_agent_home_tenant` used to key its roster lookup with a bare +# `str(agent).strip().lower()`. A zero-width space, a trailing dot/slash, an +# embedded space, or a non-str value (None/list/dict) each turned a +# roster-miss into "no home tenant = ungated colony agent". The fix +# normalizes (NFKC + zero-width strip + casefold) once, at the top of +# motor_execute, and rejects non-str/empty subjects outright instead of +# coercing them with str(...). The roster default-deny in _agent_available +# remains the enforcing layer for actual dispatchability — these tests +# exercise it with it bypassed (_patch_dispatch), matching how gate-3 proved +# the underlying mutation class, and separately confirm motor_execute's own +# entry-point guard for the non-str cases. + + +def test_normalize_agent_subject_strips_zero_width_and_casefolds(): + assert brain._normalize_agent_subject("Digid") == "digid" + assert brain._normalize_agent_subject(" digid ") == "digid" + assert brain._normalize_agent_subject("digid​") == "digid" # zero-width space + assert brain._normalize_agent_subject("digid") == "digid" # BOM / ZWNBSP + + +def test_normalize_agent_subject_rejects_non_str_and_empty(): + assert brain._normalize_agent_subject(None) is None + assert brain._normalize_agent_subject(0) is None + assert brain._normalize_agent_subject(["digid"]) is None + assert brain._normalize_agent_subject({"a": "digid"}) is None + assert brain._normalize_agent_subject("") is None + assert brain._normalize_agent_subject(" ") is None + + +def test_normalize_agent_subject_homoglyph_does_not_crash_and_does_not_match(): + # NFKC does not fold the Turkish dotless-i to 'i' (distinct codepoint, + # not a canonical equivalence) — this is documented, not a bypass: the + # normalized string simply doesn't match the roster, same as any other + # honestly-unknown agent name, and _agent_available's exact-match + # whitelist is what actually decides dispatchability. + assert brain._normalize_agent_subject("dıgıd") == "dıgıd" + + +# ── WARN-I / LOW-J: symmetric roster normalization (sos-205-790a2a63 gate-4) +# The lookup side (`_normalize_agent_subject`) normalized NFKC + zero-width + +# casefold, but the roster SIDES did not: `_AGENT_HOME_CACHE` was built with +# a bare `.strip().lower()`, and `_AGENT_SESSION` (the actual dispatch +# whitelist `_agent_available` enforces) was compared against with plain `in` +# on un-normalized literals. Normalizing only the lookup side is worse than +# normalizing neither: it makes non-normal roster ENTRIES unreachable (LOW-J +# — resolves to home=None, i.e. ungated) while ALSO silently widening the +# exact-match whitelist to accept mutated spellings (WARN-I). Fix: run BOTH +# sides through the SAME `_normalize_agent_subject` pipeline. The widening is +# now deliberate and documented, not an accident. + + +def test_agent_home_tenant_roster_key_reachable_when_registry_name_has_zero_width(monkeypatch): + # LOW-J: a roster row whose `name` is not already NFKC-normal (here, a + # zero-width space embedded in the registered name) used to build an + # UNREACHABLE map key — a lookup for the plain, canonical spelling + # resolved home=None (ungated colony agent), the wrong direction for a + # tenant-bound name. + roster = {"agents": [ + {"name": "so​l", "project": "therealmofpatterns", "role": "SPECIALIST", "type": "OPENCLAW"}, + ]} + _patch_roster(monkeypatch, payload=roster) + assert brain._agent_home_tenant("sol") == "therealmofpatterns" + + +def test_agent_home_tenant_roster_key_fullwidth_reachable(monkeypatch): + roster = {"agents": [ + {"name": "ｓｏｌ", "project": "therealmofpatterns", "role": "SPECIALIST", "type": "OPENCLAW"}, + ]} + _patch_roster(monkeypatch, payload=roster) + assert brain._agent_home_tenant("sol") == "therealmofpatterns" + + +def test_agent_available_accepts_case_and_zero_width_mutations_of_roster_entry(): + # 'system' has an empty tmux-session requirement (session == ""), so + # this exercises the roster-membership check in isolation without a live + # tmux dependency. 'SYSTEM'/fullwidth/zero-width-padded spellings are now + # a DELIBERATE, documented accept (WARN-I) — the same identity as + # 'system', not a different one silently let through. + assert brain._agent_available("system") is True + assert brain._agent_available("SYSTEM") is True + assert brain._agent_available("system​") is True # zero-width space + assert brain._agent_available("ｓｙｓｔｅｍ") is True # fullwidth + + +def test_agent_available_rejects_unknown_agent(): + assert brain._agent_available("nobody") is False + assert brain._agent_available("") is False + + +def test_motor_execute_rejects_non_str_agent_with_calm_skip(monkeypatch): + # Unhashable values (list/dict) used to reach `agent not in + # _AGENT_SESSION` (a dict membership test) and raise an uncaught + # TypeError there, crashing the whole brain cycle. Must now be a calm, + # non-error skip instead. + for bad_agent in (None, ["digid"], {"a": "digid"}, 0): + res = brain.motor_execute(_action("create_task", bad_agent)) + assert res["success"] is True + assert res.get("skipped") is True + assert "invalid agent subject" in res["result"] + + +def test_motor_execute_gate_now_catches_case_and_zero_width_mutations(monkeypatch): + # These normalize to the exact roster key ("digid") under NFKC + strip + + # casefold — pre-fix they missed the roster and slipped through + # ungated; post-fix the gate catches them like the canonical name. + roster = {"agents": _ROSTER["agents"] + [ + {"name": "digid", "project": "digid", "role": "SPECIALIST", "type": "OPENCLAW"}, + ]} + posts = _patch_dispatch(monkeypatch) + _patch_roster(monkeypatch, payload=roster) + + for mutated in ("digid​", "DIGID", " digid ", "digid"): + posts.clear() + res = brain.motor_execute(_action("create_task", mutated, goal="goal_mumega")) + assert res["success"] is False, f"{mutated!r} should be caught by the gate post-normalization" + assert "Capability scope violation" in res["result"] + assert posts == [] + + +def test_motor_execute_gate_evasion_strings_refuse_cleanly_not_crash(monkeypatch): + # These do NOT normalize to a roster hit (punctuation/embedded-space/NUL + # mutations aren't Unicode-equivalence, and normalization deliberately + # doesn't try to collapse them — see _normalize_agent_subject's + # docstring). They fall back to "unknown agent" — the SAME safe, + # documented outcome any honestly-unrecognized name gets + # (test_gate_allows_unknown_agent). The property under test is "does not + # crash and does not silently act as a DIFFERENT, tenant-bound identity" + # — not "gets denied", which is not this fix's job with + # _agent_available bypassed. + roster = {"agents": _ROSTER["agents"] + [ + {"name": "digid", "project": "digid", "role": "SPECIALIST", "type": "OPENCLAW"}, + ]} + posts = _patch_dispatch(monkeypatch) + _patch_roster(monkeypatch, payload=roster) + + for mutated in ("digid.", "digid/", "digid\x00", "di gid"): + posts.clear() + res = brain.motor_execute(_action("create_task", mutated, goal="goal_mumega")) + assert isinstance(res, dict) + assert "success" in res + assert "Capability scope violation" not in res.get("result", "") diff --git a/tests/services/test_squad_auth_revoke_route.py b/tests/services/test_squad_auth_revoke_route.py new file mode 100644 index 000000000..1bd19f39f --- /dev/null +++ b/tests/services/test_squad_auth_revoke_route.py @@ -0,0 +1,505 @@ +"""BLOCK-2 fix regression tests (sos-205-b5307dd7 re-gate). + +The re-gate verdict proved the CLI's `revoke` subcommand printed +`cache_cleared=true` while only ever clearing its OWN (separate-process, +always-empty) cache — the running service kept authenticating a revoked +token for up to the positive-cache TTL. The fix has two halves: + +1. An in-service `POST /auth/revoke` route (app.py) that runs + `revoke_api_key` IN the serving process, so it can actually clear the + cache that matters. +2. An honest CLI (`sos/services/squad/auth.py::_cli`) that tries that route + first via `_revoke_via_service` and only prints a receipt claiming the + cache is clear when it genuinely reached the service — never a bare + `cache_cleared=true` fabricated from the CLI's own empty state. +""" +from __future__ import annotations + +import sys +import time +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient + +from sos.services.squad import app as app_module +from sos.services.squad import auth +from sos.services.squad.service import SquadDB + + +_DDL = """ + CREATE TABLE api_keys ( + token_hash TEXT PRIMARY KEY, + tenant_id TEXT NOT NULL, + identity_type TEXT NOT NULL, + created_at TEXT NOT NULL + ) +""" + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + auth._token_cache_clear() + auth._TOKEN_CACHE_INFLIGHT.clear() + # P2-C/P2-G rider: the revoke rate-limit guard is process-global state, + # same shape as the token cache — reset it around every test in this + # file so test order/timing can never leak a throttle into an unrelated + # test. P2-G (sos-205-790a2a63 gate-4) made this a per-tenant dict; F2 + # (sos-205-769a2651 diverse-correctness gate) put it back to one float, + # because the resource being throttled — the whole-process cache flush — + # was never per-tenant, so a per-tenant key made the limit unbindable. + app_module._LAST_REVOKE_FLUSH_TS = 0.0 + yield + auth._token_cache_clear() + auth._TOKEN_CACHE_INFLIGHT.clear() + app_module._LAST_REVOKE_FLUSH_TS = 0.0 + + +# ── POST /auth/revoke route ────────────────────────────────────────────── + + +def test_auth_revoke_route_requires_system_bearer(monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(app_module, "_SYSTEM_BEARERS", {"sys-tok-test"}) + client = TestClient(app_module.app) + + resp = client.post("/auth/revoke", json={"tenant_id": "acme"}) + assert resp.status_code == 401 # no Authorization header at all + + resp = client.post( + "/auth/revoke", + json={"tenant_id": "acme"}, + headers={"Authorization": "Bearer wrong-token"}, + ) + assert resp.status_code == 403 + + +def test_auth_revoke_route_deletes_rows_and_clears_service_process_cache( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """The whole point of this route: a positive cache entry populated IN + THIS PROCESS (simulating what require_capability/lookup_token would have + done for a live request) must be gone after the route call, and the + revoked token must stop authenticating immediately — no TTL wait.""" + db_path = tmp_path / "revoke_route.db" + database = SquadDB(db_path=db_path) + with database.connect() as conn: + conn.execute(_DDL) + + monkeypatch.setattr(app_module, "_SYSTEM_BEARERS", {"sys-tok-test"}) + # The route builds its own SquadDB(); redirect it at the app-module + # import site to this test's throwaway DB. + monkeypatch.setattr(app_module, "SquadDB", lambda: database) + + token, _created = auth.create_api_key("fired-contractor", "agent", db=database) + assert auth._lookup_token(token, database) is not None # populate positive cache + assert auth._TOKEN_CACHE_POSITIVE # sanity: something is cached + + client = TestClient(app_module.app) + resp = client.post( + "/auth/revoke", + json={"tenant_id": "fired-contractor"}, + headers={"Authorization": "Bearer sys-tok-test"}, + ) + assert resp.status_code == 200 + assert resp.json() == { + "tenant_id": "fired-contractor", + "revoked_rows": 1, + "cache_flushed": True, + } + + # This is the property the old CLI-only fix could never deliver: the + # SAME cache _lookup_token reads from, cleared in the SAME process. + assert auth._TOKEN_CACHE_POSITIVE == {} + assert auth._lookup_token(token, database) is None + + +# ── auth._revoke_via_service (the CLI's honest probe) ──────────────────── + + +class _Resp: + def __init__(self, status_code: int, body: object | None = None): + self.status_code = status_code + self._body = {} if body is None else body + + def json(self): + if isinstance(self._body, Exception): + raise self._body + return self._body + + +def test_revoke_via_service_returns_body_on_200(monkeypatch: pytest.MonkeyPatch): + body = {"tenant_id": "acme", "revoked_rows": 1, "cache_flushed": True} + monkeypatch.setattr(auth.requests, "post", lambda *a, **k: _Resp(200, body)) + assert auth._revoke_via_service("acme", "http://localhost:8060", "sys-tok") == body + + +def test_revoke_via_service_none_on_non_200(monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth.requests, "post", lambda *a, **k: _Resp(403)) + assert auth._revoke_via_service("acme", "http://localhost:8060", "sys-tok") is None + + +def test_revoke_via_service_none_on_connection_error(monkeypatch: pytest.MonkeyPatch): + def _boom(*a, **k): + raise auth.requests.exceptions.ConnectionError("refused") + + monkeypatch.setattr(auth.requests, "post", _boom) + assert auth._revoke_via_service("acme", "http://localhost:8060", "sys-tok") is None + + +def test_revoke_via_service_none_without_system_token(): + # No token configured -> must refuse to even attempt the call, not send + # an unauthenticated revoke request. + assert auth._revoke_via_service("acme", "http://localhost:8060", "") is None + + +def test_revoke_via_service_none_on_unparseable_body(monkeypatch: pytest.MonkeyPatch): + # BLOCK-C: a 200 whose body can't be read carries no cache_flushed + # property, so the probe must not hand the caller something it can + # mistake for proof. + monkeypatch.setattr( + auth.requests, "post", lambda *a, **k: _Resp(200, ValueError("not json")) + ) + assert auth._revoke_via_service("acme", "http://localhost:8060", "sys-tok") is None + + +def test_revoke_via_service_none_on_non_dict_body(monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth.requests, "post", lambda *a, **k: _Resp(200, ["nope"])) + assert auth._revoke_via_service("acme", "http://localhost:8060", "sys-tok") is None + + +# ── CLI receipt honesty ─────────────────────────────────────────────────── + + +def test_cli_revoke_prints_service_receipt_when_reachable( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +): + monkeypatch.setattr(auth, "revoke_api_key", lambda tenant, db=None: 1) + monkeypatch.setattr( + auth, + "_revoke_via_service", + lambda tenant, url, tok: {"revoked_rows": 1, "cache_flushed": True}, + ) + monkeypatch.setattr(sys, "argv", ["auth.py", "revoke", "--tenant", "acme"]) + + rc = auth._cli() + assert rc == 0 + out = capsys.readouterr().out + assert "cache_cleared=service" in out + assert "cache_cleared=true" not in out # the old false receipt must never print + + +def test_cli_revoke_prints_honest_local_receipt_when_service_unreachable( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +): + monkeypatch.setattr(auth, "revoke_api_key", lambda tenant, db=None: 1) + monkeypatch.setattr(auth, "_revoke_via_service", lambda tenant, url, tok: None) + monkeypatch.setattr(sys, "argv", ["auth.py", "revoke", "--tenant", "acme"]) + + rc = auth._cli() + assert rc == 0 + out = capsys.readouterr().out + assert "cache_cleared=LOCAL-PROCESS-ONLY" in out + + +# ── BLOCK-C (sos-205-f1a3aee4 gate-5): a 200 is not proof of a flush ────── +# P2-G changed the throttled branch from 429 to 200 {"cache_flushed": false}. +# The CLI probe still asserted on the status code, so a throttled revoke read +# as full success and printed cache_cleared=service while the credential kept +# authenticating from the service's cache for the rest of the positive TTL. + + +def test_cli_revoke_does_not_claim_cleared_when_flush_throttled( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +): + monkeypatch.setattr(auth, "revoke_api_key", lambda tenant, db=None: 1) + monkeypatch.setattr( + auth, + "_revoke_via_service", + lambda tenant, url, tok: { + "revoked_rows": 1, + "cache_flushed": False, + "retry_after": 3.2, + }, + ) + monkeypatch.setattr(sys, "argv", ["auth.py", "revoke", "--tenant", "acme"]) + + rc = auth._cli() + assert rc == 0 + out = capsys.readouterr().out + assert "cache_cleared=service" not in out # the BLOCK-C false receipt + assert "cache_cleared=THROTTLED-NOT-FLUSHED" in out + assert "3.2" in out # the operator is told when a real flush is possible + + +def test_cli_revoke_does_not_claim_cleared_when_field_absent( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +): + # An older/unknown server that answers 200 without the field states + # nothing about the cache — fail closed on the receipt. + monkeypatch.setattr(auth, "revoke_api_key", lambda tenant, db=None: 1) + monkeypatch.setattr( + auth, "_revoke_via_service", lambda tenant, url, tok: {"revoked_rows": 1} + ) + monkeypatch.setattr(sys, "argv", ["auth.py", "revoke", "--tenant", "acme"]) + + rc = auth._cli() + assert rc == 0 + out = capsys.readouterr().out + assert "cache_cleared=service" not in out + assert "THROTTLED-NOT-FLUSHED" in out + + +def test_cli_revoke_rejects_truthy_non_true_cache_flushed( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +): + # `is True`, not truthiness: a string "false" is truthy in Python and + # must not be read as a flush. + monkeypatch.setattr(auth, "revoke_api_key", lambda tenant, db=None: 1) + monkeypatch.setattr( + auth, + "_revoke_via_service", + lambda tenant, url, tok: {"revoked_rows": 1, "cache_flushed": "false"}, + ) + monkeypatch.setattr(sys, "argv", ["auth.py", "revoke", "--tenant", "acme"]) + + rc = auth._cli() + assert rc == 0 + out = capsys.readouterr().out + assert "cache_cleared=service" not in out + + +def test_cli_revoke_end_to_end_throttled_receipt_matches_real_route( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +): + """The probe reads a REAL throttled response from the real route, not a + hand-written dict — the receipt must stay honest against the actual + server contract, which is what drifted in BLOCK-C. + """ + client, headers = _revoke_rate_limit_fixture(tmp_path, monkeypatch) + + first = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert first.status_code == 200 and first.json()["cache_flushed"] is True + + second = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert second.status_code == 200 and second.json()["cache_flushed"] is False + + monkeypatch.setattr(auth, "revoke_api_key", lambda tenant, db=None: 1) + monkeypatch.setattr( + auth.requests, "post", lambda *a, **k: _Resp(200, second.json()) + ) + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "sys-tok", raising=False) + monkeypatch.setattr(sys, "argv", ["auth.py", "revoke", "--tenant", "acme"]) + + assert auth._cli() == 0 + out = capsys.readouterr().out + assert "cache_cleared=service" not in out + assert "THROTTLED-NOT-FLUSHED" in out + + +# ── P2-C/P2-G: revoke cache-flush throttle (sos-205-47f5f8c2 gate-3, +# hardened sos-205-790a2a63 gate-4) ─────────────────────────────────────── +# revoke_api_key() clears the WHOLE in-process token cache (both pools, all +# tenants) on every call. The gate-3 verdict measured a single ~6ms revoke +# forcing ~7757x cost onto every other live client's next lookup, and +# observed the brain's capability-gate roster fetch (a 5s hardcoded timeout) +# lose that race and degrade to its static fallback during the same window. +# This is a blunt min-interval guard, not the durable fix (per-tenant cache +# invalidation via an indexed fingerprint column — sos#206). +# +# gate-4 P2-G found the ORIGINAL throttle was a single global timestamp +# checked BEFORE the DB delete: a 429 for tenant B's revoke — caused purely +# by tenant A revoking 3s earlier — aborted the whole request and left +# tenant B's key rows fully present. Fixed: the delete always runs; only the +# cache flush is throttled, per tenant; a throttled flush is a 200 with +# `cache_flushed: false`, never a bare 429. + + +def _revoke_rate_limit_fixture(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> tuple[TestClient, dict[str, str]]: + db_path = tmp_path / "revoke_rate.db" + database = SquadDB(db_path=db_path) + with database.connect() as conn: + conn.execute(_DDL) + monkeypatch.setattr(app_module, "_SYSTEM_BEARERS", {"sys-tok-test"}) + monkeypatch.setattr(app_module, "SquadDB", lambda: database) + return TestClient(app_module.app), {"Authorization": "Bearer sys-tok-test"} + + +def test_auth_revoke_route_second_rapid_call_flush_throttled_not_429(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + client, headers = _revoke_rate_limit_fixture(tmp_path, monkeypatch) + + first = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert first.status_code == 200 + assert first.json()["cache_flushed"] is True + + second = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert second.status_code == 200 # P2-G: never a bare 429 + body = second.json() + assert body["cache_flushed"] is False + assert body["retry_after"] > 0 + assert "warning" in body + assert "TTL" in body["warning"] + + +def test_auth_revoke_route_delete_always_runs_even_when_flush_throttled(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + """The DB delete must happen on EVERY call, independent of the flush + throttle — a revoked tenant's key rows must never survive a throttled + call. Mint two keys for the same tenant so the second revoke call still + has a row to delete and its `revoked_rows` count proves the DELETE ran.""" + db_path = tmp_path / "revoke_delete_always.db" + database = SquadDB(db_path=db_path) + with database.connect() as conn: + conn.execute(_DDL) + monkeypatch.setattr(app_module, "_SYSTEM_BEARERS", {"sys-tok-test"}) + monkeypatch.setattr(app_module, "SquadDB", lambda: database) + client = TestClient(app_module.app) + headers = {"Authorization": "Bearer sys-tok-test"} + + auth.create_api_key("acme", "agent", db=database) + first = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert first.status_code == 200 + assert first.json()["revoked_rows"] == 1 + + auth.create_api_key("acme", "agent", db=database) # a fresh row to prove the 2nd delete ran + second = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert second.status_code == 200 + assert second.json()["cache_flushed"] is False # throttled... + assert second.json()["revoked_rows"] == 1 # ...but the delete still ran + + with database.connect() as conn: + remaining = conn.execute( + "SELECT COUNT(*) AS n FROM api_keys WHERE tenant_id = ?", ("acme",) + ).fetchone() + assert remaining["n"] == 0 + + +def test_auth_revoke_route_throttled_flush_never_aborts_another_tenants_delete( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """gate-4 P2-G: a throttled flush for tenant A must never abort or look + like it aborted a DIFFERENT tenant's revoke. + + This test previously asserted that tenant B's flush ran unthrottled right + after tenant A's — i.e. it pinned the PER-TENANT CLOCK rather than the + property the clock was chosen to deliver. F2 (sos-205-769a2651 diverse + correctness gate) showed that mechanism was itself the defect: the flush is + process-wide, so keying its limit by tenant meant varying `tenant_id` gave + unlimited whole-cache flushes (20/20 measured against a 5s interval). + + The durable property is DELETE-ALWAYS, not clock granularity. Tenant B may + now legitimately see `cache_flushed: false` when A just consumed the global + slot — what must never happen is B's rows surviving, or B getting an abort + it could mistake for "nothing happened." + """ + db_path = tmp_path / "revoke_per_tenant.db" + database = SquadDB(db_path=db_path) + with database.connect() as conn: + conn.execute(_DDL) + monkeypatch.setattr(app_module, "_SYSTEM_BEARERS", {"sys-tok-test"}) + monkeypatch.setattr(app_module, "SquadDB", lambda: database) + client = TestClient(app_module.app) + headers = {"Authorization": "Bearer sys-tok-test"} + + auth.create_api_key("tenant-a", "agent", db=database) + auth.create_api_key("tenant-b", "agent", db=database) + + resp_a = client.post("/auth/revoke", json={"tenant_id": "tenant-a"}, headers=headers) + assert resp_a.status_code == 200 + assert resp_a.json()["cache_flushed"] is True + + # tenant-b revokes immediately after. Its flush is expected to be + # throttled now (one global slot, just consumed by tenant-a) — that is + # correct behaviour, not a regression. What matters is that the throttle + # touches ONLY the flush. + resp_b = client.post("/auth/revoke", json={"tenant_id": "tenant-b"}, headers=headers) + assert resp_b.status_code == 200, "a throttled flush must never surface as an abort" + body_b = resp_b.json() + assert body_b["cache_flushed"] is False, ( + "one global flush slot: tenant-a just consumed it, so tenant-b's flush " + "is throttled and must say so honestly" + ) + assert body_b["retry_after"] > 0 + assert "positive-cache TTL" in body_b["warning"] + # The delete is NOT throttled — this is the P2-G property that must hold. + assert body_b["revoked_rows"] == 1 + + with database.connect() as conn: + remaining = conn.execute( + "SELECT COUNT(*) AS n FROM api_keys WHERE tenant_id = ?", ("tenant-b",) + ).fetchone() + assert remaining["n"] == 0 + + +def test_auth_revoke_route_allows_again_after_interval_elapses(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + client, headers = _revoke_rate_limit_fixture(tmp_path, monkeypatch) + monkeypatch.setattr(app_module, "_REVOKE_MIN_INTERVAL_S", 0.05) + + first = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert first.status_code == 200 + + time.sleep(0.1) + + second = client.post("/auth/revoke", json={"tenant_id": "acme"}, headers=headers) + assert second.status_code == 200 + assert second.json()["cache_flushed"] is True + + +def test_auth_revoke_route_rate_limit_does_not_bypass_bearer_check(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + """The throttle guard must not become a way to probe auth: an + unauthenticated/wrong-bearer call still gets 401/403 — + _require_system_bearer runs first.""" + client, _headers = _revoke_rate_limit_fixture(tmp_path, monkeypatch) + + resp = client.post("/auth/revoke", json={"tenant_id": "acme"}) + assert resp.status_code == 401 + + resp = client.post( + "/auth/revoke", json={"tenant_id": "acme"}, headers={"Authorization": "Bearer wrong"} + ) + assert resp.status_code == 403 + + +def test_auth_revoke_flush_throttle_binds_across_varying_tenant_ids( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """F2 (sos-205-769a2651, Cursor Grok 4.5 diverse-correctness gate). + + The throttled resource — _token_cache_clear() — is process-wide. When the + clock was keyed by tenant_id, varying that field made every request a + "first" request: 20/20 whole-cache flushes landed in one instant against a + nominal 5s interval, and `payload.tenant_id` need not even name a real + tenant. That defeated the amplification argument the throttle was added to + answer. + + Locks the invariant that the limit binds regardless of what tenant_id says. + """ + db_path = tmp_path / "revoke_amplify.db" + database = SquadDB(db_path=db_path) + with database.connect() as conn: + conn.execute(_DDL) + monkeypatch.setattr(app_module, "_SYSTEM_BEARERS", {"sys-tok-test"}) + monkeypatch.setattr(app_module, "SquadDB", lambda: database) + client = TestClient(app_module.app) + headers = {"Authorization": "Bearer sys-tok-test"} + + flushes = 0 + real_clear = auth._token_cache_clear + + def _counting_clear() -> None: + nonlocal flushes + flushes += 1 + real_clear() + + monkeypatch.setattr(app_module, "_token_cache_clear", _counting_clear) + + # 20 back-to-back revokes, every one naming a DIFFERENT (and nonexistent) + # tenant — the exact shape that bypassed the per-tenant clock. + for i in range(20): + resp = client.post( + "/auth/revoke", json={"tenant_id": f"ghost-tenant-{i}"}, headers=headers + ) + assert resp.status_code == 200 + + assert flushes == 1, ( + f"expected the global flush slot to admit exactly 1 flush in this " + f"window, got {flushes} — varying tenant_id still amplifies" + ) diff --git a/tests/services/test_squad_auth_token_cache.py b/tests/services/test_squad_auth_token_cache.py new file mode 100644 index 000000000..4520aa9f0 --- /dev/null +++ b/tests/services/test_squad_auth_token_cache.py @@ -0,0 +1,677 @@ +"""Squad auth token-verification cache — 2026-07-27 incident regression tests. + +The incident: _lookup_token bcrypt-checked the presented token against every +api_keys row synchronously on the event loop (~5s of CPU with 17 bcrypt rows). +Timeout-retrying clients turned that into congestion collapse: the service +pegged a core, stopped answering, and restarts replayed the same load. + +These tests lock the fix, mutation-style: each one fails if the specific +mechanism it names is deleted. + +Hardened 2026-07-27 against the sos-205-a7c2fc44 adversarial gate verdict +(/home/mumega/mupot-worktrees/_gate-verdicts/sos-205-a7c2fc44-adversarial.md): +BLOCK-2 (revocation), BLOCK-3 (positive/negative pool isolation), and a +cross-thread lock smoke test for BLOCK-1's thread offload. +""" +from __future__ import annotations + +import concurrent.futures +import sqlite3 +import threading +import time +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient + +from sos.services.squad import app as app_module +from sos.services.squad import auth +from sos.services.squad.service import SquadDB + + +class CountingSquadDB(SquadDB): + """SquadDB that counts connect() calls — a cache hit must not connect.""" + + def __init__(self, db_path: Path): + super().__init__(db_path) + self.connect_count = 0 + + def connect(self) -> sqlite3.Connection: + self.connect_count += 1 + return super().connect() + + +@pytest.fixture() +def db(tmp_path: Path) -> CountingSquadDB: + database = CountingSquadDB(tmp_path / "squads.db") + with database.connect() as conn: + conn.execute( + """ + CREATE TABLE api_keys ( + token_hash TEXT PRIMARY KEY, + tenant_id TEXT NOT NULL, + identity_type TEXT NOT NULL, + created_at TEXT NOT NULL + ) + """ + ) + database.connect_count = 0 + return database + + +@pytest.fixture(autouse=True) +def _fresh_cache(): + auth._token_cache_clear() + auth._TOKEN_CACHE_INFLIGHT.clear() + yield + auth._token_cache_clear() + auth._TOKEN_CACHE_INFLIGHT.clear() + + +def _insert_key(db: SquadDB, token: str, tenant_id: str = "t1", identity_type: str = "agent") -> None: + with db.connect() as conn: + conn.execute( + "INSERT INTO api_keys (token_hash, tenant_id, identity_type, created_at) VALUES (?, ?, ?, ?)", + (auth.hash_token(token), tenant_id, identity_type, "2026-07-27T00:00:00Z"), + ) + + +def test_valid_token_cached_no_second_scan(db: CountingSquadDB): + _insert_key(db, "tok-alpha") + db.connect_count = 0 + + first = auth._lookup_token("tok-alpha", db) + assert first is not None and first.tenant_id == "t1" + scans_for_first = db.connect_count + assert scans_for_first >= 1 + + second = auth._lookup_token("tok-alpha", db) + assert second is not None and second.tenant_id == "t1" + assert db.connect_count == scans_for_first, "cache hit must not touch the DB" + # Identity rebuilt from snapshot matches the row path + assert second.identity.metadata["tenant_id"] == "t1" + assert second.identity.metadata["identity_type"] == "agent" + + +def test_invalid_token_negative_cached(db: CountingSquadDB): + _insert_key(db, "tok-alpha") + db.connect_count = 0 + + assert auth._lookup_token("tok-wrong", db) is None + scans_for_first = db.connect_count + assert auth._lookup_token("tok-wrong", db) is None + assert db.connect_count == scans_for_first, "negative hit must not rescan" + + +def test_expired_entry_is_a_miss(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + _insert_key(db, "tok-alpha") + assert auth._lookup_token("tok-alpha", db) is not None + before = db.connect_count + + key = auth._token_cache_key("tok-alpha", db) + expires_at, snapshot = auth._TOKEN_CACHE_POSITIVE[key] + auth._TOKEN_CACHE_POSITIVE[key] = (expires_at - auth._TOKEN_CACHE_POSITIVE_TTL_S - 1, snapshot) + + assert auth._lookup_token("tok-alpha", db) is not None + assert db.connect_count > before, "expired entry must rescan" + + +def test_cache_never_stores_raw_token(db: CountingSquadDB): + _insert_key(db, "tok-alpha") + auth._lookup_token("tok-alpha", db) + for pool in (auth._TOKEN_CACHE_POSITIVE, auth._TOKEN_CACHE_NEGATIVE): + for cache_key, (_, snapshot) in pool.items(): + assert "tok-alpha" not in cache_key + if snapshot is not None: + assert "tok-alpha" not in str(snapshot) + + +def test_cache_bounded(db: CountingSquadDB): + for i in range(auth._TOKEN_CACHE_POSITIVE_MAX + 20): + auth._token_cache_put(f"pos-key-{i}", {"tenant_id": "t", "identity_type": "agent"}) + assert len(auth._TOKEN_CACHE_POSITIVE) <= auth._TOKEN_CACHE_POSITIVE_MAX + + for i in range(auth._TOKEN_CACHE_NEGATIVE_MAX + 20): + auth._token_cache_put(f"neg-key-{i}", None) + assert len(auth._TOKEN_CACHE_NEGATIVE) <= auth._TOKEN_CACHE_NEGATIVE_MAX + + +def test_create_api_key_clears_negative_entry(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + # A pre-mint probe with the future token leaves a negative entry; the + # mint must clear it so the fresh key authenticates immediately. + real_token_hex = auth.secrets.token_hex + + def fixed_hex(n: int) -> str: + return "f" * (2 * n) + + monkeypatch.setattr(auth.secrets, "token_hex", fixed_hex) + predicted = f"sk-squad-t9-{fixed_hex(16)}" + assert auth._lookup_token(predicted, db) is None # negative-cached + + token, _created = auth.create_api_key("t9", "agent", db=db) + assert token == predicted + ctx = auth._lookup_token(token, db) + assert ctx is not None and ctx.tenant_id == "t9" + + +def test_system_token_bypasses_cache_and_db(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "sys-tok") + db.connect_count = 0 + ctx = auth._lookup_token("sys-tok", db) + assert ctx is not None and ctx.is_system + assert db.connect_count == 0 + assert auth._TOKEN_CACHE_POSITIVE == {} + assert auth._TOKEN_CACHE_NEGATIVE == {} + + +# ── BLOCK-2: revocation ──────────────────────────────────────────────────── + + +def test_revoke_api_key_invalidates_immediately(db: CountingSquadDB): + token, _created = auth.create_api_key("fired-contractor", "agent", db=db) + ctx = auth._lookup_token(token, db) + assert ctx is not None and ctx.tenant_id == "fired-contractor" # cached positive + + deleted = auth.revoke_api_key("fired-contractor", db=db) + assert deleted == 1 + + # No stale-validity window: the very next lookup must fail, not just the + # one after the (dropped) 300s/30s TTL. + assert auth._lookup_token(token, db) is None + + +def test_revoke_api_key_clears_unrelated_cached_entries_too(db: CountingSquadDB): + # Whole-cache clear is the documented, correct trade-off (raw tokens are + # never stored, so per-entry targeting is impossible) — assert it really + # does clear entries for OTHER tenants, not just the revoked one. + victim_token, _ = auth.create_api_key("bystander", "agent", db=db) + auth._lookup_token(victim_token, db) # populate a positive entry + assert auth._TOKEN_CACHE_POSITIVE # sanity: something is cached + + fired_token, _ = auth.create_api_key("fired-contractor", "agent", db=db) + auth._lookup_token(fired_token, db) + auth.revoke_api_key("fired-contractor", db=db) + + assert auth._TOKEN_CACHE_POSITIVE == {} + assert auth._TOKEN_CACHE_NEGATIVE == {} + # The bystander's row is untouched in the DB, just re-scanned once more. + ctx = auth._lookup_token(victim_token, db) + assert ctx is not None and ctx.tenant_id == "bystander" + + +def test_revoke_api_key_only_deletes_named_tenant(db: CountingSquadDB): + _insert_key(db, "tok-keep", tenant_id="keep-me") + _insert_key(db, "tok-gone", tenant_id="fired-contractor") + + deleted = auth.revoke_api_key("fired-contractor", db=db) + assert deleted == 1 + assert auth._lookup_token("tok-gone", db) is None + assert auth._lookup_token("tok-keep", db) is not None + + +# ── BLOCK-3: positive/negative pool isolation ─────────────────────────────── + + +def test_eviction_pool_isolation_negatives_cannot_evict_positives(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + # Real bcrypt scans are ~0.3-1.2s each on this host (see the adversarial + # verdict's own measurements); spraying the real 192-entry negative cap + # would mean 200+ full-table bcrypt scans in one test. Shrink the caps + # for this test only — the eviction-isolation MECHANISM being locked + # (negatives can only evict negatives) does not depend on the cap size. + monkeypatch.setattr(auth, "_TOKEN_CACHE_NEGATIVE_MAX", 5) + + # Mint and cache a real, live positive entry via the actual DB + bcrypt + # path (not a synthetic _token_cache_put) so this exercises the real + # _lookup_token miss/hit flow, matching the adversarial probe's B2/B3. + token, _created = auth.create_api_key("victim-tenant", "agent", db=db) + ctx = auth._lookup_token(token, db) + assert ctx is not None + assert len(auth._TOKEN_CACHE_POSITIVE) == 1 + + # Spray more unique bad tokens than the (shrunk) negative pool cap. + for i in range(auth._TOKEN_CACHE_NEGATIVE_MAX + 10): + auth._lookup_token(f"attacker-spray-{i}", db) + + assert len(auth._TOKEN_CACHE_NEGATIVE) <= auth._TOKEN_CACHE_NEGATIVE_MAX + # The legit positive entry must have survived the negative spray — + # a single shared pool (the pre-fix design) would have evicted it. + assert len(auth._TOKEN_CACHE_POSITIVE) == 1 + db.connect_count = 0 + assert auth._lookup_token(token, db) is not None + assert db.connect_count == 0, "surviving positive entry must still be a cache hit" + + +def test_positive_pool_capped_independently(db: CountingSquadDB): + for i in range(auth._TOKEN_CACHE_POSITIVE_MAX + 10): + auth._token_cache_put(f"pos-{i}", {"tenant_id": f"t{i}", "identity_type": "agent"}) + assert len(auth._TOKEN_CACHE_POSITIVE) <= auth._TOKEN_CACHE_POSITIVE_MAX + # Negative pool is untouched by positive traffic. + assert auth._TOKEN_CACHE_NEGATIVE == {} + + +# ── BLOCK-1: cross-thread lock smoke test ─────────────────────────────────── + + +def test_concurrent_lookups_do_not_corrupt_cache(db: CountingSquadDB): + # _lookup_token now runs off the event loop via anyio.to_thread.run_sync + # in require_capability, which makes _TOKEN_CACHE_POSITIVE/_NEGATIVE + # genuinely shared mutable state across threads. This does not prove the + # lock is sufficient under every interleaving, but it is a real smoke + # test: 50 concurrent lookups (mixed valid/invalid tokens) from a thread + # pool must all resolve correctly and must not raise (e.g. a + # "dictionary changed size during iteration" from the unguarded + # min()-over-dict eviction scan). + _insert_key(db, "tok-shared", tenant_id="concurrent-tenant") + + def _lookup(i: int): + token = "tok-shared" if i % 2 == 0 else f"tok-bad-{i}" + return token, auth._lookup_token(token, db) + + with ThreadPoolExecutor(max_workers=16) as pool: + results = list(pool.map(_lookup, range(50))) + + for token, ctx in results: + if token == "tok-shared": + assert ctx is not None and ctx.tenant_id == "concurrent-tenant" + else: + assert ctx is None + + # Lock must exist and actually be a lock (mutation-check: a + # threading.Lock instance, not e.g. a no-op placeholder). + assert isinstance(auth._TOKEN_CACHE_LOCK, type(threading.Lock())) + + +# ── BLOCK-1b: single-flight (sos-205-b5307dd7 re-gate) ────────────────────── +# The re-gate verdict measured 8 concurrent replays of ONE cold token costing +# 8 full-table scans (vs 1 when replayed sequentially) — the cache-miss check +# and the scan were not atomic w.r.t. each other, so every concurrent caller +# raced to scan before any of them had cached a result. + + +def test_single_flight_one_scan_for_concurrent_replays_of_same_token( + db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch +): + """Mutation-style: fails if _TOKEN_CACHE_INFLIGHT / its lookup in + _lookup_token is removed. The scan is artificially slowed so all 8 + ThreadPoolExecutor workers are reliably in-flight before the leader + finishes — otherwise this could pass by accident on a fast machine even + without the fix (a follower scheduled after the leader already cached + the result would just get a legitimate, unrelated cache hit).""" + _insert_key(db, "tok-hot", tenant_id="t1") + + real_scan = auth._scan_and_cache + + def _slow_scan(token: str, database: SquadDB, cache_key: str): + time.sleep(0.2) + return real_scan(token, database, cache_key) + + monkeypatch.setattr(auth, "_scan_and_cache", _slow_scan) + db.connect_count = 0 + + with ThreadPoolExecutor(max_workers=8) as pool: + results = list(pool.map(lambda _i: auth._lookup_token("tok-hot", db), range(8))) + + assert db.connect_count == 1, ( + f"expected exactly 1 DB scan for 8 concurrent replays of one cold " + f"token, got {db.connect_count}" + ) + for ctx in results: + assert ctx is not None and ctx.tenant_id == "t1" + + +def test_single_flight_propagates_scan_exception_to_followers( + db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch +): + """A DB error during the leader's scan must reach every follower waiting + on the same Future, not hang them or silently return None.""" + + def _boom(token: str, database: SquadDB, cache_key: str): + time.sleep(0.1) + raise RuntimeError("simulated scan failure") + + monkeypatch.setattr(auth, "_scan_and_cache", _boom) + + def _lookup(_i: int): + try: + auth._lookup_token("tok-error", db) + return "no-error" + except RuntimeError as exc: + return str(exc) + + with ThreadPoolExecutor(max_workers=4) as pool: + results = list(pool.map(_lookup, range(4))) + + assert results == ["simulated scan failure"] * 4 + # The in-flight entry must be evicted on failure too, or every future + # lookup of this token would hang forever waiting on a dead Future. + assert auth._token_cache_key("tok-error", db) not in auth._TOKEN_CACHE_INFLIGHT + + +# ── WARN-1: cache keyed by db (sos-205-b5307dd7 re-gate) ──────────────────── + + +def test_cache_key_scoped_by_db(tmp_path: Path): + """The cache used to be keyed on the token alone, so a token that only + exists in DB A would authenticate against DB B via a shared cache entry + without DB B ever being queried. _lookup_token(token, db) and + revoke_api_key(tenant, db=...) both advertise a per-db contract; the + cache must honor it.""" + db_a = CountingSquadDB(tmp_path / "a.db") + db_b = CountingSquadDB(tmp_path / "b.db") + ddl = """ + CREATE TABLE api_keys ( + token_hash TEXT PRIMARY KEY, + tenant_id TEXT NOT NULL, + identity_type TEXT NOT NULL, + created_at TEXT NOT NULL + ) + """ + for database in (db_a, db_b): + with database.connect() as conn: + conn.execute(ddl) + database.connect_count = 0 + + _insert_key(db_a, "tok-only-in-a", tenant_id="tenant-a") + + assert auth._lookup_token("tok-only-in-a", db_a) is not None + + # Same token presented against DB B: must be a genuine miss (DB B gets + # queried), never a hit served from DB A's cache entry. + db_b.connect_count = 0 + assert auth._lookup_token("tok-only-in-a", db_b) is None + assert db_b.connect_count >= 1, "cache must not be shared across distinct SquadDB instances" + + assert auth._token_cache_key("tok-only-in-a", db_a) != auth._token_cache_key("tok-only-in-a", db_b) + + +# ── P0-A: fail-closed SYSTEM_TOKEN (sos-205-47f5f8c2 gate-3) ──────────────── +# `token == SYSTEM_TOKEN` with SYSTEM_TOKEN defaulting to "" (unset env) +# matched an empty presented token — i.e. NO Authorization header at all — +# and granted system:sos, unrestricted-cross-tenant access. Fixed to +# `SYSTEM_TOKEN and hmac.compare_digest(token, SYSTEM_TOKEN)`: the branch +# cannot fire at all while no token is configured. + + +def test_empty_system_token_does_not_match_empty_presented_token(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "") + assert auth._lookup_token("", db) is None + + +def test_empty_system_token_does_not_match_anything(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "") + assert auth._lookup_token("some-random-token", db) is None + assert auth._lookup_token("", db) is None + + +# ── BLOCK-A: non-ASCII bearer must not raise (sos-205-790a2a63 gate-4) ───── +# `hmac.compare_digest` on `str` arguments raises `TypeError` for any +# codepoint above 127 -- `==` (what P0-A replaced) never raised, so the +# constant-time fix quietly added an unauthenticated crash path (500, not a +# bypass -- fails closed, but still a P1). httpx/starlette's TestClient +# REFUSES to encode a non-ASCII header client-side (UnicodeEncodeError +# before the request is even sent), so this is structurally invisible to any +# TestClient-based HTTP test -- CI stays green while a raw socket against a +# real uvicorn (HTTP header bytes are ISO-8859-1 on the wire; starlette +# decodes them as latin-1) gets a 500. Driving `_lookup_token` directly is +# the only way this test suite can exercise that byte space at all. + + +def test_lookup_token_non_ascii_bearer_does_not_raise(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "the-real-system-token") + # café-token: 'é' is U+00E9, codepoint > 127 -- the exact shape gate-4 + # proved crashes `hmac.compare_digest(token, SYSTEM_TOKEN)` pre-fix. + assert auth._lookup_token("café-token", db) is None + # A lone high byte (0xC3) decoded as latin-1 -- how starlette decodes a + # real raw-socket header value gate-4 sent against live uvicorn. + assert auth._lookup_token("\xc3", db) is None + # The non-ASCII codepoint can legally land on EITHER side of the + # compare -- also prove a non-ASCII SYSTEM_TOKEN doesn't raise, and that + # bytes-encoding didn't accidentally break a genuine (if unusual) match. + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "tökén") + assert auth._lookup_token("wrong-token", db) is None + assert auth._lookup_token("tökén", db) is not None + + +_ROLE_DDL = """ + CREATE TABLE roles ( + id TEXT PRIMARY KEY, + project_id TEXT NOT NULL, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + description TEXT, + created_at TEXT NOT NULL, + rank INTEGER NOT NULL DEFAULT 0, + UNIQUE(project_id, name, tenant_id) + ); + CREATE TABLE role_permissions ( + role_id TEXT NOT NULL, + permission TEXT NOT NULL, + PRIMARY KEY (role_id, permission) + ); + CREATE TABLE role_assignments ( + role_id TEXT NOT NULL, + assignee_id TEXT NOT NULL, + assignee_type TEXT NOT NULL DEFAULT 'agent', + assigned_at TEXT NOT NULL, + assigned_by TEXT NOT NULL, + PRIMARY KEY (role_id, assignee_id) + ); +""" + + +@pytest.fixture() +def http_client(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + """A TestClient wired at the throwaway `db` fixture, for the + `_parse_bearer` route surface (the 34 routes P0-A actually affects).""" + with db.connect() as conn: + conn.executescript(_ROLE_DDL) + monkeypatch.setattr(app_module, "SquadDB", lambda: db) + monkeypatch.setattr(app_module._role_svc, "db", db) + return TestClient(app_module.app) + + +def test_p0a_empty_env_no_header_is_401(http_client: TestClient, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "") + resp = http_client.get("/me/roles") + assert resp.status_code == 401 + + +def test_p0a_empty_env_empty_bearer_is_401(http_client: TestClient, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "") + resp = http_client.get("/me/roles", headers={"Authorization": "Bearer "}) + assert resp.status_code == 401 + + +def test_p0a_set_env_correct_token_passes(http_client: TestClient, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "the-real-system-token") + resp = http_client.get("/me/roles", headers={"Authorization": "Bearer the-real-system-token"}) + assert resp.status_code == 200 + + +def test_p0a_set_env_wrong_token_is_401(http_client: TestClient, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "the-real-system-token") + resp = http_client.get("/me/roles", headers={"Authorization": "Bearer nope"}) + assert resp.status_code == 401 + + +# ── LOW-1: bounded follower wait (sos-205-47f5f8c2 gate-3) ────────────────── + + +def test_follower_timeout_evicts_and_raises(db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch): + """A follower waiting on a stalled leader must not hang forever. Timeout + shortened for test speed; asserts both the raise AND that the inflight + entry is evicted so the NEXT caller isn't stuck behind the same dead + wait either.""" + monkeypatch.setattr(auth, "_TOKEN_CACHE_INFLIGHT_TIMEOUT_S", 0.2) + _insert_key(db, "tok-stall", tenant_id="t1") + + release_leader = threading.Event() + real_scan = auth._scan_and_cache + + def _stalling_scan(token: str, database: SquadDB, cache_key: str): + release_leader.wait(timeout=5) + return real_scan(token, database, cache_key) + + monkeypatch.setattr(auth, "_scan_and_cache", _stalling_scan) + + leader_started = threading.Event() + + def _leader() -> None: + leader_started.set() + auth._lookup_token("tok-stall", db) + + leader_thread = threading.Thread(target=_leader) + leader_thread.start() + leader_started.wait(timeout=2) + time.sleep(0.05) # let the leader register its Future before we follow + + with pytest.raises(concurrent.futures.TimeoutError): + auth._lookup_token("tok-stall", db) + + key = auth._token_cache_key("tok-stall", db) + assert key not in auth._TOKEN_CACHE_INFLIGHT, "timed-out follower must evict, not leave the key wedged" + + release_leader.set() + leader_thread.join(timeout=5) + + +# ── F1: revoke must beat an in-flight scan ─────────────────────────────── +# sos-205-769a2651, Cursor Grok 4.5 diverse-correctness gate. +# +# Five prior adversarial gates chased "a revoked credential must stop +# authenticating" through RECEIPTS — is the CLI honest, is the status code +# right, does the DELETE always run. None examined the property under +# CONCURRENCY. _token_cache_clear() emptied both pools but did not coordinate +# with a scan that had already read its rows: the leader finished bcrypt and +# republished a pre-revoke snapshot INTO the freshly-flushed cache. The flush +# was honest and the property was still false. + + +def test_revoke_during_inflight_scan_does_not_republish_stale_positive( + db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch +): + """The gate's stated minimum bar: with a revoke landing between the scan's + row-read and its cache write, no positive entry may survive and + _lookup_token must return None while the DB is empty.""" + _insert_key(db, "tok-victim", tenant_id="t1") + + real_put = auth._token_cache_put + rows_read = threading.Event() + revoke_done = threading.Event() + leader_error: list[BaseException] = [] + + def _slow_put(key, snapshot, *args, **kwargs): + # The scan has finished reading + bcrypt-matching and is about to + # commit. In production this gap is the tail of a multi-second scan. + rows_read.set() + revoke_done.wait(timeout=10) + return real_put(key, snapshot, *args, **kwargs) + + monkeypatch.setattr(auth, "_token_cache_put", _slow_put) + + def _leader() -> None: + try: + auth._lookup_token("tok-victim", db) + except BaseException as exc: # noqa: BLE001 - surfaced as a failure below + leader_error.append(exc) + + leader_thread = threading.Thread(target=_leader) + leader_thread.start() + assert rows_read.wait(timeout=10), "scan never reached its cache write" + + # Operator revokes mid-scan. + deleted = auth.revoke_api_key("t1", db, flush_cache=True) + revoke_done.set() + leader_thread.join(timeout=15) + + assert not leader_error, f"leader crashed, test proves nothing: {leader_error!r}" + assert deleted == 1 + + monkeypatch.setattr(auth, "_token_cache_put", real_put) + key = auth._token_cache_key("tok-victim", db) + assert key not in auth._TOKEN_CACHE_POSITIVE, ( + "the in-flight scan republished a pre-revoke snapshot into the " + "just-flushed cache — the revoked token authenticates from cache" + ) + with db.connect() as conn: + remaining = conn.execute("SELECT COUNT(*) AS n FROM api_keys").fetchone() + assert remaining["n"] == 0 + assert auth._lookup_token("tok-victim", db) is None + + +def test_token_cache_put_refuses_a_write_from_a_superseded_epoch(db: CountingSquadDB): + """Unit-level lock on the mechanism: a put carrying a stale generation is + dropped and reports False, so callers can tell they lost the race.""" + key = auth._token_cache_key("tok-epoch", db) + epoch = auth._token_cache_epoch() + + auth._token_cache_clear() # someone revokes; generation moves on + + stored = auth._token_cache_put(key, {"tenant_id": "t1", "identity_type": "agent"}, epoch=epoch) + assert stored is False + assert key not in auth._TOKEN_CACHE_POSITIVE + + # A put carrying the CURRENT epoch still lands — the check is not a + # blanket refusal. + assert auth._token_cache_put( + key, {"tenant_id": "t1", "identity_type": "agent"}, epoch=auth._token_cache_epoch() + ) is True + assert key in auth._TOKEN_CACHE_POSITIVE + + +def test_lookup_fails_closed_when_cache_cleared_mid_scan( + db: CountingSquadDB, monkeypatch: pytest.MonkeyPatch +): + """The leader's own answer is stale too, not just the cache write — it must + not authenticate the caller off rows read before the revoke.""" + _insert_key(db, "tok-closed", tenant_id="t1") + + real_scan = auth._scan_and_cache + + def _scan_then_revoke(token: str, database: SquadDB, cache_key: str): + snapshot, fresh = real_scan(token, database, cache_key) + return snapshot, fresh + + # Clear the cache from inside the scan, after the rows are read. + real_put = auth._token_cache_put + + def _clear_then_put(key, snapshot, *args, **kwargs): + auth._token_cache_clear() + return real_put(key, snapshot, *args, **kwargs) + + monkeypatch.setattr(auth, "_scan_and_cache", _scan_then_revoke) + monkeypatch.setattr(auth, "_token_cache_put", _clear_then_put) + + assert auth._lookup_token("tok-closed", db) is None, ( + "a scan whose rows predate a cache clear must fail closed, not hand " + "back an AuthContext built from the pre-revoke world" + ) + + +# ── F5: the leader must not evict its successor's Future ───────────────── + + +def test_leader_finish_does_not_evict_a_successor_future(db: CountingSquadDB): + """After a follower times out and evicts a stalled leader, a NEW leader + registers under the same key. The original leader finishing must not pop + the successor's registration — that silently breaks single-flight exactly + under the stall conditions the timeout exists to handle.""" + key = auth._token_cache_key("tok-successor", db) + + stalled_leader_future: concurrent.futures.Future = concurrent.futures.Future() + successor_future: concurrent.futures.Future = concurrent.futures.Future() + + # Successor is the registered in-flight scan for this key. + auth._TOKEN_CACHE_INFLIGHT[key] = successor_future + + # The original (evicted) leader now finishes and releases. + auth._inflight_release(key, stalled_leader_future) + + assert auth._TOKEN_CACHE_INFLIGHT.get(key) is successor_future, ( + "the finishing leader popped its successor's Future" + ) + + # And the rightful owner can still deregister itself. + auth._inflight_release(key, successor_future) + assert key not in auth._TOKEN_CACHE_INFLIGHT diff --git a/tests/services/test_squad_role_tenant_scope.py b/tests/services/test_squad_role_tenant_scope.py new file mode 100644 index 000000000..0a6db8dc8 --- /dev/null +++ b/tests/services/test_squad_role_tenant_scope.py @@ -0,0 +1,370 @@ +"""P0-B fix regression tests (sos-205-47f5f8c2 gate-3). + +Five RBAC routes in app.py used to authenticate a caller and then discard +the AuthContext entirely (`await lookup_token(...) or _raise_401()`), so any +tenant's valid api key could mutate or read ANY other tenant's role: + + - POST /roles/{role_id}/permissions (add_role_permission) + - DELETE /roles/{role_id}/permissions/{permission} (remove_role_permission) + - DELETE /roles/{role_id}/assignments/{assignee_id}(revoke_role_assignment) + - GET /roles/{role_id}/assignments (list_role_assignments) + - GET /agents/{agent_id}/roles (get_agent_roles) + +The fix binds `auth` and scopes every role_id lookup by `auth.tenant_scope`, +mirroring the sibling create_project_role / list_project_roles routes that +were already correct. Driven end-to-end over real HTTP (TestClient), the +same production code path a real client hits — not the service layer in +isolation. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient + +from sos.services.squad import app as app_module +from sos.services.squad import auth +from sos.services.squad.roles import RoleService +from sos.services.squad.service import SquadDB + + +_DDL = """ + CREATE TABLE api_keys ( + token_hash TEXT PRIMARY KEY, + tenant_id TEXT NOT NULL, + identity_type TEXT NOT NULL, + created_at TEXT NOT NULL + ); + CREATE TABLE roles ( + id TEXT PRIMARY KEY, + project_id TEXT NOT NULL, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + description TEXT, + created_at TEXT NOT NULL, + rank INTEGER NOT NULL DEFAULT 0, + UNIQUE(project_id, name, tenant_id) + ); + CREATE TABLE role_permissions ( + role_id TEXT NOT NULL, + permission TEXT NOT NULL, + PRIMARY KEY (role_id, permission) + ); + CREATE TABLE role_assignments ( + role_id TEXT NOT NULL, + assignee_id TEXT NOT NULL, + assignee_type TEXT NOT NULL DEFAULT 'agent', + assigned_at TEXT NOT NULL, + assigned_by TEXT NOT NULL, + PRIMARY KEY (role_id, assignee_id) + ); +""" + + +@pytest.fixture() +def two_tenants(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + """A throwaway DB with one role owned by tenant-A, plus valid api keys + for tenant-A and tenant-B. Wires app.py's per-request SquadDB() AND the + module-level `_role_svc` (created once at import time with the real + default SquadDB — NOT re-created per-request, so it must be repointed + separately) at this throwaway DB.""" + db_path = tmp_path / "roles_tenant_scope.db" + database = SquadDB(db_path=db_path) + with database.connect() as conn: + conn.executescript(_DDL) + + monkeypatch.setattr(app_module, "SquadDB", lambda: database) + monkeypatch.setattr(app_module._role_svc, "db", database) + + auth._token_cache_clear() + auth._TOKEN_CACHE_INFLIGHT.clear() + + token_a, _ = auth.create_api_key("tenant-a", "user", db=database) + token_b, _ = auth.create_api_key("tenant-b", "user", db=database) + + role_svc = RoleService(db=database) + role = role_svc.create_role("proj-a", "admins", tenant_id="tenant-a") + role_svc.assign_role( + role["id"], "kasra-test", + tenant_id="tenant-a", assignee_type="agent", assigned_by="test", + ) + + yield { + "client": TestClient(app_module.app), + "role_id": role["id"], + "token_a": token_a, + "token_b": token_b, + "database": database, + } + + auth._token_cache_clear() + auth._TOKEN_CACHE_INFLIGHT.clear() + + +def _bearer(token: str) -> dict[str, str]: + return {"Authorization": f"Bearer {token}"} + + +# ── add_role_permission ─────────────────────────────────────────────────── + + +def test_add_role_permission_owner_ok(two_tenants): + t = two_tenants + resp = t["client"].post( + f"/roles/{t['role_id']}/permissions", + json={"permission": "squad:read"}, + headers=_bearer(t["token_a"]), + ) + assert resp.status_code == 200 + assert resp.json() == {"role_id": t["role_id"], "permission": "squad:read"} + + +def test_add_role_permission_foreign_tenant_blocked(two_tenants): + t = two_tenants + resp = t["client"].post( + f"/roles/{t['role_id']}/permissions", + json={"permission": "squad:admin"}, + headers=_bearer(t["token_b"]), + ) + assert resp.status_code in (403, 404) + # Must not have actually granted the permission. + with t["database"].connect() as conn: + row = conn.execute( + "SELECT 1 FROM role_permissions WHERE role_id = ? AND permission = ?", + (t["role_id"], "squad:admin"), + ).fetchone() + assert row is None + + +# ── remove_role_permission ──────────────────────────────────────────────── + + +def test_remove_role_permission_foreign_tenant_blocked(two_tenants): + t = two_tenants + with t["database"].connect() as conn: + conn.execute( + "INSERT INTO role_permissions (role_id, permission) VALUES (?, ?)", + (t["role_id"], "squad:write"), + ) + resp = t["client"].delete( + f"/roles/{t['role_id']}/permissions/squad:write", + headers=_bearer(t["token_b"]), + ) + assert resp.status_code in (403, 404) + with t["database"].connect() as conn: + row = conn.execute( + "SELECT 1 FROM role_permissions WHERE role_id = ? AND permission = ?", + (t["role_id"], "squad:write"), + ).fetchone() + assert row is not None # NOT deleted by the foreign caller + + +def test_remove_role_permission_owner_ok(two_tenants): + t = two_tenants + with t["database"].connect() as conn: + conn.execute( + "INSERT INTO role_permissions (role_id, permission) VALUES (?, ?)", + (t["role_id"], "squad:write"), + ) + resp = t["client"].delete( + f"/roles/{t['role_id']}/permissions/squad:write", + headers=_bearer(t["token_a"]), + ) + assert resp.status_code == 200 + assert resp.json() == {"deleted": True} + + +# ── revoke_role_assignment ──────────────────────────────────────────────── + + +def test_revoke_role_assignment_foreign_tenant_blocked(two_tenants): + t = two_tenants + resp = t["client"].delete( + f"/roles/{t['role_id']}/assignments/kasra-test", + headers=_bearer(t["token_b"]), + ) + assert resp.status_code in (403, 404) + with t["database"].connect() as conn: + row = conn.execute( + "SELECT 1 FROM role_assignments WHERE role_id = ? AND assignee_id = ?", + (t["role_id"], "kasra-test"), + ).fetchone() + assert row is not None # NOT revoked by the foreign caller + + +def test_revoke_role_assignment_owner_ok(two_tenants): + t = two_tenants + resp = t["client"].delete( + f"/roles/{t['role_id']}/assignments/kasra-test", + headers=_bearer(t["token_a"]), + ) + assert resp.status_code == 200 + assert resp.json() == {"revoked": True} + + +# ── list_role_assignments ───────────────────────────────────────────────── + + +def test_list_role_assignments_foreign_tenant_blocked(two_tenants): + t = two_tenants + resp = t["client"].get( + f"/roles/{t['role_id']}/assignments", + headers=_bearer(t["token_b"]), + ) + assert resp.status_code in (403, 404) + + +def test_list_role_assignments_owner_ok(two_tenants): + t = two_tenants + resp = t["client"].get( + f"/roles/{t['role_id']}/assignments", + headers=_bearer(t["token_a"]), + ) + assert resp.status_code == 200 + assignees = [a["assignee_id"] for a in resp.json()["assignments"]] + assert "kasra-test" in assignees + + +# ── get_agent_roles ─────────────────────────────────────────────────────── + + +def test_get_agent_roles_foreign_tenant_sees_nothing(two_tenants): + t = two_tenants + resp = t["client"].get( + "/agents/kasra-test/roles", + headers=_bearer(t["token_b"]), + ) + # Not a 404 (the route never raises for this one) — but the foreign + # tenant must not see tenant-A's role in the result. + assert resp.status_code == 200 + assert resp.json()["roles"] == [] + + +def test_get_agent_roles_owner_sees_role(two_tenants): + t = two_tenants + resp = t["client"].get( + "/agents/kasra-test/roles", + headers=_bearer(t["token_a"]), + ) + assert resp.status_code == 200 + role_ids = [r["id"] for r in resp.json()["roles"]] + assert t["role_id"] in role_ids + + +# ── assign_role — BLOCK-B (sos-205-790a2a63 gate-4) ─────────────────────── +# The 6th RBAC route on this surface. The P0-B fix (sos-205-47f5f8c2) scoped +# the five siblings but missed this one: `assign_role` called +# `self._get_role_row(role_id)` with no `tenant_id`, defaulting to the +# fail-open unrestricted lookup, so ANY tenant's valid api key could plant a +# role_assignment row into ANOTHER tenant's role — and, because +# revoke_role_assignment IS scoped, the victim tenant (or system) was the +# only one who could remove it. + + +def test_assign_role_foreign_tenant_blocked(two_tenants): + t = two_tenants + resp = t["client"].post( + f"/roles/{t['role_id']}/assignments", + json={"assignee_id": "attacker-planted", "assigned_by": "tenant-b"}, + headers=_bearer(t["token_b"]), + ) + assert resp.status_code in (403, 404) + with t["database"].connect() as conn: + row = conn.execute( + "SELECT 1 FROM role_assignments WHERE role_id = ? AND assignee_id = ?", + (t["role_id"], "attacker-planted"), + ).fetchone() + assert row is None # NOT planted by the foreign caller + + +def test_assign_role_owner_ok(two_tenants): + t = two_tenants + resp = t["client"].post( + f"/roles/{t['role_id']}/assignments", + json={"assignee_id": "new-teammate", "assigned_by": "tenant-a"}, + headers=_bearer(t["token_a"]), + ) + assert resp.status_code == 200 + assert resp.json()["assignee_id"] == "new-teammate" + with t["database"].connect() as conn: + row = conn.execute( + "SELECT 1 FROM role_assignments WHERE role_id = ? AND assignee_id = ?", + (t["role_id"], "new-teammate"), + ).fetchone() + assert row is not None + + +# ── /me/roles — P2-E (sos-205-790a2a63 gate-4) ──────────────────────────── +# `get_token_roles` called `get_agent_roles(tenant_id)` (positional — +# `tenant_id` filled the `assignee_id` slot) with NO `tenant_id=` kwarg, +# defaulting to unrestricted. A role_assignment row whose `assignee_id` +# equals another tenant's identity — exactly what an attacker could plant +# through the unfixed BLOCK-B hole — surfaced through that OTHER tenant's +# own /me/roles. Simulated here by inserting the row directly, independent +# of whether BLOCK-B itself is fixed, so this test locks P2-E on its own. + + +def test_me_roles_does_not_disclose_cross_tenant_planted_assignment(two_tenants): + t = two_tenants + with t["database"].connect() as conn: + conn.execute( + """ + INSERT INTO role_assignments (role_id, assignee_id, assignee_type, assigned_at, assigned_by) + VALUES (?, ?, 'agent', '2026-01-01T00:00:00Z', 'attacker') + """, + (t["role_id"], "tenant-b"), + ) + resp = t["client"].get("/me/roles", headers=_bearer(t["token_b"])) + assert resp.status_code == 200 + assert resp.json()["roles"] == [] # tenant-a's role must not surface for tenant-b + + +def test_me_roles_owner_sees_own_assignment(two_tenants): + t = two_tenants + with t["database"].connect() as conn: + conn.execute( + """ + INSERT INTO role_assignments (role_id, assignee_id, assignee_type, assigned_at, assigned_by) + VALUES (?, ?, 'agent', '2026-01-01T00:00:00Z', 'test') + """, + (t["role_id"], "tenant-a"), + ) + resp = t["client"].get("/me/roles", headers=_bearer(t["token_a"])) + assert resp.status_code == 200 + role_ids = [r["id"] for r in resp.json()["roles"]] + assert t["role_id"] in role_ids + + +# ── system tier keeps cross-tenant access ───────────────────────────────── + + +def test_system_bearer_bypasses_tenant_scope(two_tenants, monkeypatch: pytest.MonkeyPatch): + t = two_tenants + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "sys-tok-test") + resp = t["client"].get( + f"/roles/{t['role_id']}/assignments", + headers=_bearer("sys-tok-test"), + ) + assert resp.status_code == 200 + assignees = [a["assignee_id"] for a in resp.json()["assignments"]] + assert "kasra-test" in assignees + + resp = t["client"].get( + "/agents/kasra-test/roles", + headers=_bearer("sys-tok-test"), + ) + assert resp.status_code == 200 + role_ids = [r["id"] for r in resp.json()["roles"]] + assert t["role_id"] in role_ids + + +def test_system_bearer_can_assign_across_tenants(two_tenants, monkeypatch: pytest.MonkeyPatch): + t = two_tenants + monkeypatch.setattr(auth, "SYSTEM_TOKEN", "sys-tok-test") + resp = t["client"].post( + f"/roles/{t['role_id']}/assignments", + json={"assignee_id": "system-planted", "assigned_by": "system"}, + headers=_bearer("sys-tok-test"), + ) + assert resp.status_code == 200