From 326a757cdbf1fb08ba98d9b35026ecec137f7703 Mon Sep 17 00:00:00 2001 From: Rajat Ahuja Date: Mon, 22 Jun 2026 16:30:00 -0500 Subject: [PATCH] Fix scoped JWTs (#679) * Peer- and session-scoped JWTs were effectively workspace-scoped: auth() walked the route's declared scope and fell through to a workspace match, so a {w: ws-a, p: alice} token could act on any peer in ws-a. * feat: peer keys can read sessions they belong to; require workspace on scoped keys * fix: authorize JWTs by narrowest scope and gate member reads Follow-up hardening on the narrowest-claim auth fix: - Scope get_peer_config member-read to the caller's own peer; a session member could previously read a co-member's per-session config. - Enforce session membership on POST /peers/{id}/chat: the session_id arrives in the body (invisible to require_auth), so a peer key could read any session's injected message history. Check is_peer_in_session in the handler before the dialectic runs. - Consolidate the workspace-match check in auth() to a single hoisted guard so no branch can silently re-open cross-workspace access. - Normalize empty-string scope claims to None in verify_jwt so a blank workspace can't satisfy the peer/session token-shape invariant. - Extract scope_requires_workspace(), shared by verify_jwt and the keys API so the creation-time guard and verification invariant can't drift. route requires auth) and CLAUDE.md auth-scoping guidance. - docs: describe narrow-scope key semantics in the platform reference. --------- Co-authored-by: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> --- CHANGELOG.md | 11 + CLAUDE.md | 5 + docs/v3/documentation/reference/platform.mdx | 8 +- src/crud/session.py | 32 +++ src/routers/keys.py | 12 + src/routers/messages.py | 54 +++- src/routers/peers.py | 19 +- src/routers/sessions.py | 51 +++- src/routers/webhooks.py | 8 +- src/security.py | 123 ++++++-- tests/conftest.py | 2 + tests/routes/test_auth_route_policy.py | 108 +++++++ tests/routes/test_messages.py | 92 ++++++ tests/routes/test_peers.py | 35 +++ tests/routes/test_scoped_api.py | 8 +- tests/test_security.py | 287 +++++++++++++++++++ 16 files changed, 805 insertions(+), 50 deletions(-) create mode 100644 tests/routes/test_auth_route_policy.py create mode 100644 tests/test_security.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3809fa3b..d31b9d72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +### Changed + +- Peer-scoped JWTs now get read-only access to the sessions their peer is an active member of (session context, summaries, peers, their own per-session config, search, and message reads). Session-scoped JWTs remain confined to their session and cannot reach peer routes. + +### Fixed + +- Peer- and session-scoped JWTs were effectively workspace-scoped: authorization walked the route's declared scope and fell through to a workspace match, so a `{w, p: alice}` token could act on any peer in the workspace. JWTs are now authorized by their narrowest claim and never widen to workspace access. +- The keys API now rejects creating a peer- or session-scoped key without a workspace. Such keys were minted successfully but failed verification on every request. + ## [3.0.10] - 2026-06-15 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 87a95c3e..c9862b59 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,6 +118,11 @@ cd sdks/typescript && bun run tsc --noEmit - **Never hold a DB session during external calls** (LLM, embedding, HTTP). If a function needs both a DB session and an external call result, compute the external result first and pass it as a parameter. This avoids tying up DB connections during slow network I/O. Use `tracked_db` for short-lived, DB-only operations; pass a shared session when multiple DB-only calls can reuse one connection. - **Never write through a read-only session** (`tracked_db(..., read_only=True)`, `get_read_db`, `ReadSessionLocal`). These run in AUTOCOMMIT mode with no transaction: writes are NOT blocked by the database — they silently commit immediately, and `begin_nested()` savepoints break. There is no runtime guard; this is enforced by convention only. Use `read_only=True` strictly for SELECT-only windows; anything that mutates (including get-or-create paths) must use a regular write session. +#### Auth scoping + +- **`allow_member_read=True` (in `require_auth(...)`) is read-only — NEVER set it on a route that mutates state.** It lets a peer-scoped key reach a session route when its peer is an active member of the session, so on a mutating route it would hand any session member write access (message injection, config mutation, deletion). HTTP method is not a reliable read/write signal here (some read routes use POST for a richer body), so this is enforced by an explicit allowlist in `tests/routes/test_auth_route_policy.py` — adding the flag to a new route fails that test until you consciously add the route to `EXPECTED_MEMBER_READ_ROUTES`, and you must never add a mutating method there. +- **When a member-read route is keyed by another sub-resource** (e.g. `peers/{peer_id}/config`), the handler must additionally confirm a peer-scoped caller only reads its OWN resource (`jwt_params.p == peer_id`, else raise `AuthenticationException`). Membership grants session access, not access to a co-member's data. See `get_peer_config` in `src/routers/sessions.py`. + ### Runtime Architecture Honcho runs as two cooperating processes that share a Postgres database and Redis cache: diff --git a/docs/v3/documentation/reference/platform.mdx b/docs/v3/documentation/reference/platform.mdx index 75fc9bd5..abf3c646 100644 --- a/docs/v3/documentation/reference/platform.mdx +++ b/docs/v3/documentation/reference/platform.mdx @@ -60,7 +60,13 @@ The **Performance** page provides comprehensive monitoring with usage metrics, h ## 3. Manage API Keys -The [API Keys](https://app.honcho.dev/api-keys) page allows you to create and manage authentication tokens for different environments. You can create admin-level keys with full instance access or scope keys to specific `Workspaces`, `Peers`, or `Sessions`. +The [API Keys](https://app.honcho.dev/api-keys) page allows you to create and manage authentication tokens for different environments. You can create admin-level keys with full instance access or scope keys to a specific `Workspace`, `Peer`, or `Session`. + +Scoped keys are authorized by their narrowest claim and never widen to the whole workspace: + +- A **peer-scoped** key acts on its own peer, plus **read-only** access to the sessions its peer is an active member of (context, summaries, peers, its own per-session config, search, and message reads). It cannot write to those sessions or act on other peers. +- A **session-scoped** key is confined to its own session and cannot reach peer routes. +- Peer- and session-scoped keys **must carry their parent workspace** — creating one without a workspace is rejected. API Key Management Dashboard diff --git a/src/crud/session.py b/src/crud/session.py index 712188bc..40cdacac 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -834,6 +834,38 @@ async def get_peers_from_session( ) +async def is_peer_in_session( + db: AsyncSession, + workspace_name: str, + session_name: str, + peer_name: str, +) -> bool: + """Return whether a peer is an active member of a session. + + Active membership means a `SessionPeer` row exists with `left_at IS NULL`. + Used by the auth layer to grant a peer-scoped key read access to the + sessions that peer belongs to. + + Args: + db: Database session + workspace_name: Name of the workspace + session_name: Name of the session + peer_name: Name of the peer + + Returns: + True if the peer is currently a member of the session. + """ + result = await db.scalar( + select(models.SessionPeer.peer_name) + .where(models.SessionPeer.workspace_name == workspace_name) + .where(models.SessionPeer.session_name == session_name) + .where(models.SessionPeer.peer_name == peer_name) + .where(models.SessionPeer.left_at.is_(None)) + .limit(1) + ) + return result is not None + + async def get_session_peer_configuration( workspace_name: str, session_name: str, diff --git a/src/routers/keys.py b/src/routers/keys.py index f4a50375..0051db90 100644 --- a/src/routers/keys.py +++ b/src/routers/keys.py @@ -9,6 +9,7 @@ from src.security import ( JWTParams, create_jwt, require_auth, + scope_requires_workspace, ) from src.utils.formatting import format_datetime_utc @@ -42,6 +43,17 @@ async def create_key( "At least one of workspace_id, peer_id, or session_id must be provided" ) + # A peer- or session-scoped key must carry its parent workspace, otherwise + # verify_jwt rejects it on every request (the workspace is required to rule + # out cross-workspace use). Shares the predicate with verify_jwt so the + # creation-time guard and the verification-time invariant cannot drift. + if scope_requires_workspace( + peer=peer_id, session=session_id, workspace=workspace_id + ): + raise ValidationException( + "workspace_id is required when scoping a key to a peer or session" + ) + key_str = create_jwt( JWTParams( exp=format_datetime_utc(expires_at) if expires_at else None, diff --git a/src/routers/messages.py b/src/routers/messages.py index 5e10c472..9ace70cc 100644 --- a/src/routers/messages.py +++ b/src/routers/messages.py @@ -32,9 +32,19 @@ logger = logging.getLogger(__name__) router = APIRouter( prefix="/workspaces/{workspace_id}/sessions/{session_id}/messages", tags=["messages"], - dependencies=[ - Depends(require_auth(workspace_name="workspace_id", session_name="session_id")) - ], +) + +# Read routes additionally allow a peer-scoped key whose peer is a member of the +# session; write routes stay session-scoped only. Applied per-route rather than +# on the router so the two policies can differ. +require_session_read = require_auth( + workspace_name="workspace_id", + session_name="session_id", + allow_member_read=True, +) +require_session_write = require_auth( + workspace_name="workspace_id", + session_name="session_id", ) @@ -82,9 +92,18 @@ async def parse_upload_form( ) -@router.post("", response_model=list[schemas.Message], status_code=201) @router.post( - "/", response_model=list[schemas.Message], status_code=201, include_in_schema=False + "", + response_model=list[schemas.Message], + status_code=201, + dependencies=[Depends(require_session_write)], +) +@router.post( + "/", + response_model=list[schemas.Message], + status_code=201, + include_in_schema=False, + dependencies=[Depends(require_session_write)], ) # backwards compatibility with pre-2.6.0 faulty route endpoint async def create_messages_for_session( background_tasks: BackgroundTasks, @@ -154,7 +173,12 @@ async def create_messages_for_session( raise -@router.post("/upload", response_model=list[schemas.Message], status_code=201) +@router.post( + "/upload", + response_model=list[schemas.Message], + status_code=201, + dependencies=[Depends(require_session_write)], +) async def create_messages_with_file( background_tasks: BackgroundTasks, workspace_id: str = Path(...), @@ -266,7 +290,11 @@ async def create_messages_with_file( return created_messages -@router.post("/list", response_model=Page[schemas.Message]) +@router.post( + "/list", + response_model=Page[schemas.Message], + dependencies=[Depends(require_session_read)], +) async def get_messages( workspace_id: str = Path(...), session_id: str = Path(...), @@ -299,7 +327,11 @@ async def get_messages( raise ResourceNotFoundException("Session not found") from e -@router.get("/{message_id}", response_model=schemas.Message) +@router.get( + "/{message_id}", + response_model=schemas.Message, + dependencies=[Depends(require_session_read)], +) async def get_message( workspace_id: str = Path(...), session_id: str = Path(...), @@ -316,7 +348,11 @@ async def get_message( return honcho_message -@router.put("/{message_id}", response_model=schemas.Message) +@router.put( + "/{message_id}", + response_model=schemas.Message, + dependencies=[Depends(require_session_write)], +) async def update_message( workspace_id: str = Path(...), session_id: str = Path(...), diff --git a/src/routers/peers.py b/src/routers/peers.py index e1aa45dd..90cdc1a5 100644 --- a/src/routers/peers.py +++ b/src/routers/peers.py @@ -14,6 +14,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from src import crud, schemas from src.config import settings +from src.crud.session import is_peer_in_session from src.dependencies import db, read_db, tracked_db from src.dialectic.chat import agentic_chat, agentic_chat_stream from src.embedding_client import embedding_client @@ -167,19 +168,31 @@ async def get_sessions_for_peer( }, }, }, - dependencies=[ - Depends(require_auth(workspace_name="workspace_id", peer_name="peer_id")) - ], ) async def chat( workspace_id: str = Path(...), peer_id: str = Path(...), options: schemas.DialecticOptions = Body(...), + jwt_params: JWTParams = Depends( + require_auth(workspace_name="workspace_id", peer_name="peer_id") + ), ): """ Query a Peer's representation using natural language. Performs agentic search and reasoning to comprehensively answer the query based on all latent knowledge gathered about the peer from their messages and conclusions. """ + # The session id arrives in the body, so require_auth can't gate on it. A + # peer-scoped key may only scope a chat to a session its peer belongs to; + # without this check it could read any session's messages (the dialectic + # injects session history) by naming it here. Workspace/admin tokens + # (jwt_params.p is None) are unaffected. + if jwt_params.p is not None and options.session_id: + async with tracked_db("peers.chat.is_peer_in_session", read_only=True) as s_db: + if not await is_peer_in_session( + s_db, workspace_id, options.session_id, jwt_params.p + ): + raise AuthenticationException("JWT not permissioned for this resource") + # Get or create the peer to ensure it exists async with tracked_db("peers.chat.get_or_create_peer") as peer_db: peers_result = await crud.get_or_create_peers( diff --git a/src/routers/sessions.py b/src/routers/sessions.py index eb8794d7..89ce665c 100644 --- a/src/routers/sessions.py +++ b/src/routers/sessions.py @@ -536,17 +536,28 @@ async def remove_peers_from_session( @router.get( "/{session_id}/peers/{peer_id}/config", response_model=schemas.SessionPeerConfig, - dependencies=[ - Depends(require_auth(workspace_name="workspace_id", session_name="session_id")) - ], ) async def get_peer_config( workspace_id: str = Path(...), session_id: str = Path(...), peer_id: str = Path(...), + jwt_params: JWTParams = Depends( + require_auth( + workspace_name="workspace_id", + session_name="session_id", + allow_member_read=True, + ) + ), db: AsyncSession = read_db, ): - """Get the configuration for a Peer in a Session.""" + """Get the configuration for a Peer in a Session. + + Member-read lets a peer-scoped key reach this route, but a peer may only + read its own per-session config — not a co-member's. Workspace/admin and + session-scoped tokens (which already span the whole session) are unaffected. + """ + if jwt_params.p is not None and jwt_params.p != peer_id: + raise AuthenticationException("JWT not permissioned for this resource") return await crud.get_peer_config( db, workspace_name=workspace_id, @@ -593,7 +604,13 @@ async def set_peer_config( "/{session_id}/peers", response_model=Page[schemas.Peer], dependencies=[ - Depends(require_auth(workspace_name="workspace_id", session_name="session_id")) + Depends( + require_auth( + workspace_name="workspace_id", + session_name="session_id", + allow_member_read=True, + ) + ) ], ) async def get_session_peers( @@ -616,7 +633,13 @@ async def get_session_peers( "/{session_id}/context", response_model=schemas.SessionContext, dependencies=[ - Depends(require_auth(workspace_name="workspace_id", session_name="session_id")) + Depends( + require_auth( + workspace_name="workspace_id", + session_name="session_id", + allow_member_read=True, + ) + ) ], ) async def get_session_context( @@ -808,7 +831,13 @@ async def get_session_context( "/{session_id}/summaries", response_model=schemas.SessionSummaries, dependencies=[ - Depends(require_auth(workspace_name="workspace_id", session_name="session_id")) + Depends( + require_auth( + workspace_name="workspace_id", + session_name="session_id", + allow_member_read=True, + ) + ) ], ) async def get_session_summaries( @@ -849,7 +878,13 @@ async def get_session_summaries( "/{session_id}/search", response_model=list[schemas.Message], dependencies=[ - Depends(require_auth(workspace_name="workspace_id", session_name="session_id")) + Depends( + require_auth( + workspace_name="workspace_id", + session_name="session_id", + allow_member_read=True, + ) + ) ], ) async def search_session( diff --git a/src/routers/webhooks.py b/src/routers/webhooks.py index f66164e9..91fc63f9 100644 --- a/src/routers/webhooks.py +++ b/src/routers/webhooks.py @@ -31,7 +31,7 @@ async def get_or_create_webhook_endpoint( webhook: schemas.WebhookEndpointCreate = Body( ..., description="Webhook endpoint parameters" ), - jwt_params: JWTParams = Depends(require_auth()), + jwt_params: JWTParams = Depends(require_auth(workspace_name="workspace_id")), db: AsyncSession = db, ) -> schemas.WebhookEndpoint: """ @@ -55,7 +55,7 @@ async def get_or_create_webhook_endpoint( @router.get("", response_model=Page[schemas.WebhookEndpoint]) async def list_webhook_endpoints( workspace_id: str = Path(..., description="Workspace ID"), - jwt_params: JWTParams = Depends(require_auth()), + jwt_params: JWTParams = Depends(require_auth(workspace_name="workspace_id")), db: AsyncSession = db, ) -> Page[schemas.WebhookEndpoint]: """ @@ -72,7 +72,7 @@ async def list_webhook_endpoints( async def delete_webhook_endpoint( workspace_id: str = Path(..., description="Workspace ID"), endpoint_id: str = Path(..., description="Webhook endpoint ID"), - jwt_params: JWTParams = Depends(require_auth()), + jwt_params: JWTParams = Depends(require_auth(workspace_name="workspace_id")), db: AsyncSession = db, ) -> None: """ @@ -88,7 +88,7 @@ async def delete_webhook_endpoint( @router.get("/test") async def test_emit( workspace_id: str = Path(..., description="Workspace ID"), - jwt_params: JWTParams = Depends(require_auth()), + jwt_params: JWTParams = Depends(require_auth(workspace_name="workspace_id")), ) -> None: """ Test publishing a webhook event. diff --git a/src/security.py b/src/security.py index 6988919e..3a60d8c1 100644 --- a/src/security.py +++ b/src/security.py @@ -80,6 +80,28 @@ def create_jwt(params: JWTParams) -> str: ) +def scope_requires_workspace( + *, peer: str | None, session: str | None, workspace: str | None +) -> bool: + """Return whether a peer- or session-scoped claim lacks its parent workspace. + + A peer or session scope is meaningless without a workspace: the route-level + check cannot rule out cross-workspace use (a ``{p: "alice"}`` token would + match ``alice`` in any workspace). Truthiness-based so empty-string claims + count as absent. Shared by `verify_jwt` (the token-shape invariant) and the + keys API (the creation-time guard) so the two rules cannot drift apart. + + Args: + peer: The peer claim, if any. + session: The session claim, if any. + workspace: The workspace claim, if any. + + Returns: + True when a peer/session scope is present but the workspace is not. + """ + return bool(peer or session) and not workspace + + def verify_jwt(token: str) -> JWTParams: """Verify a JWT and return the decoded parameters.""" @@ -101,12 +123,23 @@ def verify_jwt(token: str) -> JWTParams: raise AuthenticationException("JWT expired") if "ad" in decoded: params.ad = decoded["ad"] + # Normalize empty-string scope claims to None so a blank `w`/`p`/`s` + # cannot masquerade as a present claim in the checks below. if "w" in decoded: - params.w = decoded["w"] + params.w = decoded["w"] or None if "p" in decoded: - params.p = decoded["p"] + params.p = decoded["p"] or None if "s" in decoded: - params.s = decoded["s"] + params.s = decoded["s"] or None + # Token-shape invariant: a peer- or session-scoped token MUST also + # carry its parent workspace, otherwise the route-level check cannot + # rule out cross-workspace use. + if scope_requires_workspace( + peer=params.p, session=params.s, workspace=params.w + ): + raise AuthenticationException( + "Invalid JWT scope: peer/session token missing workspace" + ) return params except jwt.PyJWTError: raise AuthenticationException("Invalid JWT") from None @@ -117,9 +150,14 @@ def require_auth( workspace_name: str | None = None, peer_name: str | None = None, session_name: str | None = None, + allow_member_read: bool = False, ): """ Generate a dependency that requires authentication for the given parameters. + + Set `allow_member_read=True` on read-only session routes to additionally + grant access to peer-scoped keys whose peer is an active member of the + session. Never set it on routes that mutate state. """ async def auth_dependency( @@ -150,8 +188,14 @@ def require_auth( workspace_name=workspace_name_param, peer_name=peer_name_param, session_name=session_name_param, + allow_member_read=allow_member_read, ) + # Tag the closure so route-policy tests can introspect which routes opt into + # member read without re-deriving it from HTTP method (an unreliable + # read/write signal here — some read routes use POST for a richer body). + auth_dependency.honcho_allow_member_read = allow_member_read # pyright: ignore[reportFunctionMemberAccess] + return auth_dependency @@ -161,6 +205,7 @@ async def auth( workspace_name: str | None = None, peer_name: str | None = None, session_name: str | None = None, + allow_member_read: bool = False, ) -> JWTParams: """Authenticate the given JWT and return the decoded parameters.""" if not settings.AUTH.USE_AUTH: @@ -171,30 +216,66 @@ async def auth( jwt_params = verify_jwt(credentials.credentials) - # based on api operation, verify api key based on that key's permissions + # Authorize by the token's narrowest scope, not by the route's. A + # narrower-than-workspace token must NOT fall back to workspace access: + # `{w: ws, p: alice}` may only act on `alice`, never on a sibling peer. if jwt_params.ad: return jwt_params if admin: raise AuthenticationException("Resource requires admin privileges") - # For session level access - if session_name and jwt_params.s == session_name: - if workspace_name and jwt_params.w != workspace_name: - raise AuthenticationException("JWT not permissioned for this resource") + if not any([session_name, peer_name, workspace_name]): + # Self-authorizing routes decode the token here and compare the claims + # against body/path data inside the handler. This is needed for routes + # whose resource identifier is not available to require_auth(). return jwt_params - # For peer level access - if peer_name and jwt_params.p == peer_name: - if workspace_name and jwt_params.w != workspace_name: - raise AuthenticationException("JWT not permissioned for this resource") - return jwt_params - - # For workspace level access - can access all peers/sessions under this workspace - if workspace_name and jwt_params.w == workspace_name: - return jwt_params - - if any([session_name, peer_name, workspace_name]): + # Every scoped, non-admin path requires the token's workspace to match the + # route's. Check it once here so no individual branch below can forget it + # and silently re-open cross-workspace access (the bug this module fixes). + if workspace_name and jwt_params.w != workspace_name: raise AuthenticationException("JWT not permissioned for this resource") - # Route did not specify any parameters, so it should parse parameters itself - return jwt_params + if jwt_params.s is not None: + # Session-scoped token: confined to its own session. It gets no + # cross-scope access to peer routes. + if not session_name or jwt_params.s != session_name: + raise AuthenticationException("JWT not permissioned for this resource") + return jwt_params + + if jwt_params.p is not None: + # Peer-scoped token: its own peer routes... + if peer_name and jwt_params.p == peer_name: + return jwt_params + # ...plus read-only access to the sessions the peer is a member of. + # Gated on `allow_member_read` so only read routes opt in; writes stay + # denied. Requires the route's workspace so the membership lookup is + # scoped (every session route declares workspace_name); the workspace + # match itself was already verified above. + if allow_member_read and session_name and workspace_name: + # Lazy imports avoid an import cycle with the crud/db layers and + # keep this DB round-trip off the common (same-scope) auth path. + from src.crud.session import is_peer_in_session + from src.dependencies import tracked_db + + # Membership is read on a separate committed-only (read_only) + # connection, so a peer added to the session in a not-yet-committed + # transaction reads as a non-member: writes must commit before a + # member-scoped read. Fails closed. + async with tracked_db( + "auth.is_peer_in_session", read_only=True + ) as member_db: + is_member = await is_peer_in_session( + member_db, workspace_name, session_name, jwt_params.p + ) + if is_member: + return jwt_params + raise AuthenticationException("JWT not permissioned for this resource") + + if jwt_params.w is not None: + # Workspace tokens reach any route inside their workspace (the workspace + # match was verified above). Routes without a declared workspace (e.g. + # POST /v3/workspaces) self-authorize by reading jwt_params.w themselves. + return jwt_params + + raise AuthenticationException("JWT not permissioned for this resource") diff --git a/tests/conftest.py b/tests/conftest.py index 4aeaf56c..301d91d2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -78,6 +78,8 @@ _RUNTIME_MOCK_TEST_BLOCKLIST_PREFIXES = ( # LLM transport tests mock providers directly and don't need database/runtime setup. "tests/utils/test_length_finish_reason.py", "tests/utils/test_clients.py", + # Pure JWT scope tests — operate on src.security directly, no DB needed. + "tests/test_security.py", "tests/test_generate_jwt_script.py", ) diff --git a/tests/routes/test_auth_route_policy.py b/tests/routes/test_auth_route_policy.py new file mode 100644 index 00000000..b31f2cd3 --- /dev/null +++ b/tests/routes/test_auth_route_policy.py @@ -0,0 +1,108 @@ +"""Route-policy regression tests for auth scoping. + +Two invariants this guards: + +1. `allow_member_read=True` grants peer-scoped keys read access to sessions + their peer belongs to. It must appear ONLY on intended read routes — never on + a mutating route, where it would hand session members write access. HTTP + method is not a reliable read/write signal in this codebase (some read + endpoints use POST for a richer request body), so we assert against an + explicit allowlist instead of deriving from the method. + +2. The messages router dropped its router-level auth dependency in favor of + per-route dependencies. Every route on it must still carry auth, or a future + route added without an explicit dependency would serve unauthenticated. +""" + +from fastapi.routing import APIRoute + +from src.main import app + +# (method, path) pairs intentionally granting member peers read access. Adding a +# route here is a deliberate security decision: it must be read-only. Never add +# a mutating route. See CLAUDE.md "Auth scoping" for the rule. +EXPECTED_MEMBER_READ_ROUTES = { + ("POST", "/v3/workspaces/{workspace_id}/sessions/{session_id}/messages/list"), + ( + "GET", + "/v3/workspaces/{workspace_id}/sessions/{session_id}/messages/{message_id}", + ), + ("GET", "/v3/workspaces/{workspace_id}/sessions/{session_id}/context"), + ("GET", "/v3/workspaces/{workspace_id}/sessions/{session_id}/summaries"), + ("GET", "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers"), + ( + "GET", + "/v3/workspaces/{workspace_id}/sessions/{session_id}/peers/{peer_id}/config", + ), + ("POST", "/v3/workspaces/{workspace_id}/sessions/{session_id}/search"), +} + +# Unambiguously mutating methods. POST is intentionally excluded: this codebase +# uses POST for some read endpoints (`/messages/list`, `/search`) to take a +# richer request body, so POST is not a write signal. The allowlist test above +# is the real guard against a write route opting into member read; this test +# additionally catches the clear-cut PUT/PATCH/DELETE mistakes. +MUTATING_METHODS = {"PUT", "PATCH", "DELETE"} + + +def _auth_dependency_calls(route: APIRoute): + """Yield the callables of every honcho auth dependency attached to a route. + + `require_auth(...)` closures are tagged with `honcho_allow_member_read`, so a + dependency is a honcho auth dependency iff its callable has that attribute. + Walks the dependant tree to cover both `dependencies=[Depends(...)]` and + parameter-level `Depends(...)`. + """ + stack = list(route.dependant.dependencies) + while stack: + dep = stack.pop() + if hasattr(dep.call, "honcho_allow_member_read"): + yield dep.call + stack.extend(dep.dependencies) + + +def _method_path_pairs(route: APIRoute): + for method in route.methods or set(): + if method in ("HEAD", "OPTIONS"): + continue + yield (method, route.path) + + +def test_member_read_allowlist_matches_routes(): + """Exactly the allowlisted routes opt into member read — no more, no less.""" + actual: set[tuple[str, str]] = set() + for route in app.routes: + if not isinstance(route, APIRoute): + continue + if any( + getattr(call, "honcho_allow_member_read", False) + for call in _auth_dependency_calls(route) + ): + actual.update(_method_path_pairs(route)) + + assert actual == EXPECTED_MEMBER_READ_ROUTES + + +def test_member_read_never_on_mutating_route(): + """A member-read route must never use a mutating HTTP method.""" + for method, path in EXPECTED_MEMBER_READ_ROUTES: + assert method not in MUTATING_METHODS, ( + f"{method} {path} grants member-read on a mutating method — " + "member peers would gain write access" + ) + + +def test_every_message_route_requires_auth(): + """The messages router has no router-level auth dependency; assert each route + carries its own so a newly added route cannot be silently unauthenticated.""" + prefix = "/v3/workspaces/{workspace_id}/sessions/{session_id}/messages" + message_routes = [ + route + for route in app.routes + if isinstance(route, APIRoute) and route.path.startswith(prefix) + ] + assert message_routes, "expected to find message routes mounted under the prefix" + for route in message_routes: + assert any( + _auth_dependency_calls(route) + ), f"{route.methods} {route.path} has no auth dependency" diff --git a/tests/routes/test_messages.py b/tests/routes/test_messages.py index 2d3c77e5..3539a735 100644 --- a/tests/routes/test_messages.py +++ b/tests/routes/test_messages.py @@ -7,7 +7,9 @@ from nanoid import generate as generate_nanoid from sqlalchemy.ext.asyncio import AsyncSession from src import models +from src.config import settings from src.models import Peer, Workspace +from src.security import JWTParams, create_jwt @pytest.mark.asyncio @@ -278,6 +280,96 @@ async def test_get_messages( assert data["items"][0]["metadata"] == {} +@pytest.mark.asyncio +async def test_member_peer_key_reads_session_but_cannot_write( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + monkeypatch: pytest.MonkeyPatch, +): + """A peer-scoped key may read sessions its peer belongs to (membership-based + cross-scope read), but not write to them. Non-member peer keys and session + keys on peer routes are denied. Exercises the real session_peers lookup.""" + test_workspace, alice = sample_data + session_name = str(generate_nanoid()) + base = f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}" + + # Setup with auth disabled: create the session with alice as an active + # member, then commit so the independent tracked_db session in auth() (which + # only sees committed rows) can resolve membership. + client.post(f"{base}/peers", json={alice.name: {}}) + await db_session.commit() + + # Enforce auth for the assertions below. + monkeypatch.setattr(settings.AUTH, "USE_AUTH", True) + monkeypatch.setattr(settings.AUTH, "JWT_SECRET", "test-secret") + + # Member peer key: reads allowed. + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=alice.name))}" + ) + assert client.post(f"{base}/messages/list", json={}).status_code == 200 + assert client.get(f"{base}/context").status_code == 200 + + # Member peer key: writes denied (write routes don't opt into member read). + assert ( + client.post( + f"{base}/messages", + json={"messages": [{"content": "nope", "peer_id": alice.name}]}, + ).status_code + == 401 + ) + + # Non-member peer key: even reads denied. + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p='not-a-member'))}" + ) + assert client.post(f"{base}/messages/list", json={}).status_code == 401 + + # Session key: no cross-scope access to peer routes. + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, s=session_name))}" + ) + assert ( + client.get( + f"/v3/workspaces/{test_workspace.name}/peers/{alice.name}/card" + ).status_code + == 401 + ) + + +@pytest.mark.asyncio +async def test_member_peer_key_reads_only_own_session_peer_config( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + monkeypatch: pytest.MonkeyPatch, +): + """A member peer key may read its OWN per-session config but not a + co-member's. The route opts into member read, so without the in-handler + self-check alice could read bob's config.""" + test_workspace, alice = sample_data + bob_name = str(generate_nanoid()) + session_name = str(generate_nanoid()) + base = f"/v3/workspaces/{test_workspace.name}/sessions/{session_name}" + + # Create the session with alice and bob as active members; commit so the + # independent read-only tracked_db in auth() can resolve membership. + client.post(f"{base}/peers", json={alice.name: {}, bob_name: {}}) + await db_session.commit() + + monkeypatch.setattr(settings.AUTH, "USE_AUTH", True) + monkeypatch.setattr(settings.AUTH, "JWT_SECRET", "test-secret") + + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=alice.name))}" + ) + # Own config: allowed. + assert client.get(f"{base}/peers/{alice.name}/config").status_code == 200 + # Co-member's config: denied even though alice is a session member. + assert client.get(f"{base}/peers/{bob_name}/config").status_code == 401 + + @pytest.mark.asyncio async def test_get_messages_with_reverse( client: TestClient, db_session: AsyncSession, sample_data: tuple[Workspace, Peer] diff --git a/tests/routes/test_peers.py b/tests/routes/test_peers.py index dd00eff9..847c24e0 100644 --- a/tests/routes/test_peers.py +++ b/tests/routes/test_peers.py @@ -7,7 +7,9 @@ from nanoid import generate as generate_nanoid from sqlalchemy.ext.asyncio import AsyncSession from src import crud, models +from src.config import settings from src.models import Peer, Workspace +from src.security import JWTParams, create_jwt def test_get_or_create_peer(client: TestClient, sample_data: tuple[Workspace, Peer]): @@ -625,6 +627,39 @@ def test_chat( assert "content" in data +@pytest.mark.asyncio +async def test_chat_peer_key_denied_for_non_member_session( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + monkeypatch: pytest.MonkeyPatch, +): + """A peer-scoped key cannot chat scoped to a session its peer is not a member + of — the session id is in the body, so the handler checks membership. The + guard fires before the dialectic runs, so no LLM call is made.""" + test_workspace, alice = sample_data + session_id = str(generate_nanoid()) + + # Session exists but alice is NOT a member of it. + client.post( + f"/v3/workspaces/{test_workspace.name}/sessions", + json={"id": session_id}, + ) + await db_session.commit() + + monkeypatch.setattr(settings.AUTH, "USE_AUTH", True) + monkeypatch.setattr(settings.AUTH, "JWT_SECRET", "test-secret") + client.headers["Authorization"] = ( + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=alice.name))}" + ) + + response = client.post( + f"/v3/workspaces/{test_workspace.name}/peers/{alice.name}/chat", + json={"query": "what do you know?", "stream": False, "session_id": session_id}, + ) + assert response.status_code == 401 + + def test_chat_with_optional_params( client: TestClient, sample_data: tuple[Workspace, Peer], diff --git a/tests/routes/test_scoped_api.py b/tests/routes/test_scoped_api.py index a4d4677a..7b504a7a 100644 --- a/tests/routes/test_scoped_api.py +++ b/tests/routes/test_scoped_api.py @@ -141,7 +141,7 @@ def test_get_peer_by_name_with_auth( # Test with peer-scoped JWT if auth_client.auth_type == "empty": auth_client.headers["Authorization"] = ( - f"Bearer {create_jwt(JWTParams(p=test_peer.name))}" + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=test_peer.name))}" ) # Get specific peer using get_or_create endpoint @@ -218,7 +218,7 @@ def test_create_session_with_auth( # Test with peer-scoped JWT if auth_client.auth_type == "empty": auth_client.headers["Authorization"] = ( - f"Bearer {create_jwt(JWTParams(p=test_peer.name))}" + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=test_peer.name))}" ) session_name2 = str(generate_nanoid()) @@ -262,7 +262,7 @@ def test_get_session_by_name_with_auth( if auth_client.auth_type == "empty": # Test with session-scoped JWT auth_client.headers["Authorization"] = ( - f"Bearer {create_jwt(JWTParams(s=session_name))}" + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, s=session_name))}" ) response = auth_client.post( @@ -282,7 +282,7 @@ def test_get_session_by_name_with_auth( # Test with peer-scoped JWT auth_client.headers["Authorization"] = ( - f"Bearer {create_jwt(JWTParams(p=test_peer.name))}" + f"Bearer {create_jwt(JWTParams(w=test_workspace.name, p=test_peer.name))}" ) assert auth_client.post( diff --git a/tests/test_security.py b/tests/test_security.py new file mode 100644 index 00000000..1785be8b --- /dev/null +++ b/tests/test_security.py @@ -0,0 +1,287 @@ +"""Auth scope tests — DEV-1736 regression coverage. + +Prior to this fix `auth()` walked the route's declared scope first and fell +through to a workspace check, so a `{w, p}` token authorized any peer in `w`. +The contract now is: authorize by the token's narrowest claim, never widen. +""" + +from contextlib import asynccontextmanager + +import jwt as pyjwt +import pytest +from fastapi.security import HTTPAuthorizationCredentials + +from src.config import settings +from src.exceptions import AuthenticationException, ValidationException +from src.security import JWTParams, auth, create_jwt, verify_jwt + + +@pytest.fixture(autouse=True) +def _enable_auth(monkeypatch: pytest.MonkeyPatch): # pyright: ignore[reportUnusedFunction] + monkeypatch.setattr(settings.AUTH, "USE_AUTH", True) + monkeypatch.setattr(settings.AUTH, "JWT_SECRET", "test-secret") + + +def _bearer(token: str) -> HTTPAuthorizationCredentials: + return HTTPAuthorizationCredentials(scheme="Bearer", credentials=token) + + +class TestVerifyJWTShape: + def test_peer_token_without_workspace_rejected(self): + token = pyjwt.encode({"p": "alice"}, b"test-secret", algorithm="HS256") + with pytest.raises(AuthenticationException): + verify_jwt(token) + + def test_session_token_without_workspace_rejected(self): + token = pyjwt.encode({"s": "sess-1"}, b"test-secret", algorithm="HS256") + with pytest.raises(AuthenticationException): + verify_jwt(token) + + def test_workspace_only_token_ok(self): + token = create_jwt(JWTParams(w="ws-a")) + params = verify_jwt(token) + assert params.w == "ws-a" + + def test_workspace_peer_token_ok(self): + token = create_jwt(JWTParams(w="ws-a", p="alice")) + params = verify_jwt(token) + assert params.w == "ws-a" + assert params.p == "alice" + + +class TestAuthPeerScope: + """`{w: ws-a, p: alice}` may only act on alice in ws-a.""" + + @pytest.mark.asyncio + async def test_matches_own_peer(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + params = await auth(credentials=creds, workspace_name="ws-a", peer_name="alice") + assert params.p == "alice" + + @pytest.mark.asyncio + async def test_denies_sibling_peer_same_workspace(self): + """The original bug: peer-scoped token fell through to workspace auth.""" + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-a", peer_name="bob") + + @pytest.mark.asyncio + async def test_denies_workspace_route_with_no_peer(self): + """Peer-scoped token cannot use workspace-listing routes.""" + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-a") + + @pytest.mark.asyncio + async def test_self_authorizing_route_receives_claims(self): + """Body-scoped routes use require_auth() and compare claims in-handler.""" + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + params = await auth(credentials=creds) + assert params.w == "ws-a" + assert params.p == "alice" + + @pytest.mark.asyncio + async def test_denies_cross_workspace(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-b", peer_name="alice") + + +class TestAuthSessionScope: + @pytest.mark.asyncio + async def test_matches_own_session(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a", s="sess-1"))) + params = await auth( + credentials=creds, workspace_name="ws-a", session_name="sess-1" + ) + assert params.s == "sess-1" + + @pytest.mark.asyncio + async def test_denies_sibling_session_same_workspace(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a", s="sess-1"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-a", session_name="sess-2") + + @pytest.mark.asyncio + async def test_denies_workspace_route_with_no_session(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a", s="sess-1"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-a") + + @pytest.mark.asyncio + async def test_self_authorizing_route_receives_claims(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a", s="sess-1"))) + params = await auth(credentials=creds) + assert params.w == "ws-a" + assert params.s == "sess-1" + + +class TestAuthWorkspaceScope: + @pytest.mark.asyncio + async def test_matches_workspace(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a"))) + params = await auth(credentials=creds, workspace_name="ws-a") + assert params.w == "ws-a" + + @pytest.mark.asyncio + async def test_workspace_token_reaches_peer_route(self): + """Workspace tokens still authorize narrower routes inside the workspace.""" + creds = _bearer(create_jwt(JWTParams(w="ws-a"))) + params = await auth(credentials=creds, workspace_name="ws-a", peer_name="alice") + assert params.w == "ws-a" + + @pytest.mark.asyncio + async def test_denies_cross_workspace(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-b") + + @pytest.mark.asyncio + async def test_passes_self_authorizing_route(self): + """Routes with no declared scope (e.g. POST /v3/workspaces) self-authorize + on the token's `w`. The auth dependency must let workspace tokens through.""" + creds = _bearer(create_jwt(JWTParams(w="ws-a"))) + params = await auth(credentials=creds) + assert params.w == "ws-a" + + +@asynccontextmanager +async def _fake_tracked_db(*_args: object, **_kwargs: object): + """Stand-in for tracked_db; the membership query itself is monkeypatched.""" + yield None + + +def _patch_membership(monkeypatch: pytest.MonkeyPatch, *, is_member: bool): + async def _is_peer_in_session(*_args: object, **_kwargs: object) -> bool: + return is_member + + # Names are resolved via lazy imports inside auth(), so patch the source + # modules rather than the security namespace. + monkeypatch.setattr("src.dependencies.tracked_db", _fake_tracked_db) + monkeypatch.setattr("src.crud.session.is_peer_in_session", _is_peer_in_session) + + +class TestAuthMemberRead: + """Peer-scoped key gets read-only access to sessions it is a member of.""" + + @pytest.mark.asyncio + async def test_member_peer_allowed_on_read_route( + self, monkeypatch: pytest.MonkeyPatch + ): + _patch_membership(monkeypatch, is_member=True) + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + params = await auth( + credentials=creds, + workspace_name="ws-a", + session_name="sess-1", + allow_member_read=True, + ) + assert params.p == "alice" + + @pytest.mark.asyncio + async def test_non_member_peer_denied_on_read_route( + self, monkeypatch: pytest.MonkeyPatch + ): + _patch_membership(monkeypatch, is_member=False) + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + with pytest.raises(AuthenticationException): + await auth( + credentials=creds, + workspace_name="ws-a", + session_name="sess-1", + allow_member_read=True, + ) + + @pytest.mark.asyncio + async def test_member_peer_denied_on_write_route( + self, monkeypatch: pytest.MonkeyPatch + ): + """Write routes never set allow_member_read, so membership is irrelevant.""" + _patch_membership(monkeypatch, is_member=True) + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + with pytest.raises(AuthenticationException): + await auth( + credentials=creds, + workspace_name="ws-a", + session_name="sess-1", + allow_member_read=False, + ) + + @pytest.mark.asyncio + async def test_member_peer_denied_cross_workspace( + self, monkeypatch: pytest.MonkeyPatch + ): + _patch_membership(monkeypatch, is_member=True) + creds = _bearer(create_jwt(JWTParams(w="ws-a", p="alice"))) + with pytest.raises(AuthenticationException): + await auth( + credentials=creds, + workspace_name="ws-b", + session_name="sess-1", + allow_member_read=True, + ) + + @pytest.mark.asyncio + async def test_session_token_has_no_cross_scope_to_peer_routes(self): + """A session key never reaches peer routes, even with allow_member_read.""" + creds = _bearer(create_jwt(JWTParams(w="ws-a", s="sess-1"))) + with pytest.raises(AuthenticationException): + await auth( + credentials=creds, + workspace_name="ws-a", + peer_name="alice", + allow_member_read=True, + ) + + +class TestCreateKeyValidation: + @pytest.mark.asyncio + async def test_peer_key_without_workspace_rejected(self): + from src.routers.keys import create_key + + with pytest.raises(ValidationException): + await create_key(workspace_id=None, peer_id="alice", session_id=None) + + @pytest.mark.asyncio + async def test_session_key_without_workspace_rejected(self): + from src.routers.keys import create_key + + with pytest.raises(ValidationException): + await create_key(workspace_id=None, peer_id=None, session_id="sess-1") + + @pytest.mark.asyncio + async def test_peer_key_with_workspace_ok(self): + from src.routers.keys import create_key + + result = await create_key(workspace_id="ws-a", peer_id="alice", session_id=None) + assert "key" in result + + +class TestAuthAdminAndUnscoped: + @pytest.mark.asyncio + async def test_admin_passes_any_route(self): + creds = _bearer(create_jwt(JWTParams(ad=True))) + params = await auth(credentials=creds, workspace_name="ws-a", peer_name="alice") + assert params.ad is True + + @pytest.mark.asyncio + async def test_non_admin_token_denied_on_admin_route(self): + creds = _bearer(create_jwt(JWTParams(w="ws-a"))) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, admin=True) + + @pytest.mark.asyncio + async def test_unscoped_token_on_self_authorizing_route(self): + """A token with no scope claims and a route with no declared scope is the + escape hatch for routes that introspect jwt_params themselves.""" + creds = _bearer(create_jwt(JWTParams())) + params = await auth(credentials=creds) + assert params.w is None + assert params.p is None + assert params.s is None + + @pytest.mark.asyncio + async def test_unscoped_token_denied_on_scoped_route(self): + creds = _bearer(create_jwt(JWTParams())) + with pytest.raises(AuthenticationException): + await auth(credentials=creds, workspace_name="ws-a")