From a9a271c08d160d055501fc444793baa2108ef0ee Mon Sep 17 00:00:00 2001 From: Abdul Rafey Date: Tue, 11 Aug 2026 13:33:45 +0500 Subject: [PATCH 1/6] refactor(auth): extract token-reading helpers from get_current_user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_current_user is a FastAPI dependency, so it only resolves once routing has picked a handler. Code that must identify a caller earlier had no way to reuse it and would have to decode tokens and query users of its own, leaving two implementations of "who is this request from" free to drift apart — with the copy outside the dependency being the one that guards access. Split the two steps it composes into decode_token_username and get_user_by_username so any caller can reuse them, and rebuild the dependency on top. Behaviour is unchanged: both 401 responses keep their distinct detail messages. Co-Authored-By: Claude Opus 5 (1M context) --- sparkth/lib/auth.py | 62 ++++++++++++++++++++++++++++++++---------- tests/lib/test_auth.py | 57 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 15 deletions(-) diff --git a/sparkth/lib/auth.py b/sparkth/lib/auth.py index bc383bfa..59c253f6 100644 --- a/sparkth/lib/auth.py +++ b/sparkth/lib/auth.py @@ -1,8 +1,16 @@ -"""Authentication dependency for resolving the current user from a bearer token. +"""Bearer-token authentication: the token-reading helpers and the current-user dependency. The single canonical home for ``get_current_user``: every caller (routes, the permission gate, plugins, and the test harness) imports it from here. It is deliberately not re-exported from ``sparkth.api.v1.auth`` — one object keeps FastAPI dependency overrides working. + +``get_current_user`` is a FastAPI dependency, so it is only available to code that runs +*inside* a route. Code that must identify the caller earlier — ``PluginAccessMiddleware`` +runs before routing, so no dependency has resolved yet — composes the same two helpers this +module builds the dependency from, :func:`decode_token_username` and +:func:`get_user_by_username`, rather than decoding tokens or querying users of its own. One +implementation of "who is this request from" keeps the security gate from drifting away from +the dependency as tokens or user lookup change. """ import jwt @@ -14,34 +22,58 @@ from sparkth.core import security from sparkth.core.models.user import User from sparkth.lib.db import get_async_session +from sparkth.lib.log import get_logger + +logger = get_logger(__name__) security_scheme = HTTPBearer() +def decode_token_username(token: str) -> str | None: + """Return the username a bearer token identifies, or ``None`` if it identifies nobody. + + ``None`` covers every way a token can fail to name a user — malformed, signed with the + wrong key, expired, or carrying no ``sub`` claim — because callers treat them alike: + an unreadable token is an unauthenticated request. Callers decide what that means; + this helper never raises. + + Args: + token: The raw JWT from the ``Authorization`` header, without the ``Bearer`` prefix. + """ + try: + payload = security.decode_access_token(token) + except jwt.InvalidTokenError as e: + # Debug, not warning: expired tokens are routine (every session eventually reaches + # this path), so logging louder would bury real failures in noise. + logger.debug(f"Rejected an unreadable access token: {e}") + return None + + username = payload.get("sub") + if not isinstance(username, str) or not username: + return None + return username + + +async def get_user_by_username(username: str, session: AsyncSession) -> User | None: + """Return the user with this username, or ``None`` when no user has it.""" + result = await session.exec(select(User).where(User.username == username)) + return result.one_or_none() + + async def get_current_user( credentials: HTTPAuthorizationCredentials = Depends(security_scheme), session: AsyncSession = Depends(get_async_session), ) -> User: - token = credentials.credentials - - try: - payload = security.decode_access_token(token) - username = payload.get("sub") - if not isinstance(username, str) or username is None: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Could not validate credentials", - headers={"WWW-Authenticate": "Bearer"}, - ) - except jwt.InvalidTokenError: + """Resolve the authenticated user, rejecting the request when the token names no one.""" + username = decode_token_username(credentials.credentials) + if username is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", headers={"WWW-Authenticate": "Bearer"}, ) - result = await session.exec(select(User).where(User.username == username)) - user = result.one_or_none() + user = await get_user_by_username(username, session) if user is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, diff --git a/tests/lib/test_auth.py b/tests/lib/test_auth.py index d12be06d..a5d99d03 100644 --- a/tests/lib/test_auth.py +++ b/tests/lib/test_auth.py @@ -1,5 +1,24 @@ +from datetime import timedelta + +from sqlmodel.ext.asyncio.session import AsyncSession + import sparkth.api.v1.auth as api_auth import sparkth.lib.auth as lib_auth +from sparkth.core.models.user import User +from sparkth.core.security import create_access_token + + +async def _seed_user(session: AsyncSession, username: str) -> User: + user = User( + name="Auth Test", + username=username, + email=f"{username}@example.com", + hashed_password="not-a-real-hash", + ) + session.add(user) + await session.commit() + await session.refresh(user) + return user def test_get_current_user_lives_in_lib_auth() -> None: @@ -12,3 +31,41 @@ def test_get_current_user_not_reexported_from_api_auth() -> None: # so they all share one object. sparkth.api.v1.auth must NOT re-export it — a compat shim would # split the canonical location and let a dependency_overrides key silently miss. assert not hasattr(api_auth, "get_current_user") + + +class TestDecodeTokenUsername: + """Reading the subject out of a bearer token. + + Shared by get_current_user and PluginAccessMiddleware so both read tokens the same way. + """ + + def test_returns_the_token_subject(self) -> None: + token = create_access_token({"sub": "tokenuser"}) + + assert lib_auth.decode_token_username(token) == "tokenuser" + + def test_returns_none_for_a_malformed_token(self) -> None: + assert lib_auth.decode_token_username("not-a-real-token") is None + + def test_returns_none_for_an_expired_token(self) -> None: + token = create_access_token({"sub": "tokenuser"}, expires_delta=timedelta(minutes=-1)) + + assert lib_auth.decode_token_username(token) is None + + def test_returns_none_when_the_token_carries_no_subject(self) -> None: + assert lib_auth.decode_token_username(create_access_token({})) is None + + +class TestGetUserByUsername: + """Looking a user up by username.""" + + async def test_returns_the_matching_user(self, session: AsyncSession) -> None: + await _seed_user(session, "lookupuser") + + found = await lib_auth.get_user_by_username("lookupuser", session) + + assert found is not None + assert found.username == "lookupuser" + + async def test_returns_none_when_no_user_matches(self, session: AsyncSession) -> None: + assert await lib_auth.get_user_by_username("nobody-here", session) is None From 26be0c22d07f535e21002815ce0c528d33c8ff9e Mon Sep 17 00:00:00 2001 From: Abdul Rafey Date: Tue, 11 Aug 2026 13:34:01 +0500 Subject: [PATCH 2/6] fix(plugins): enforce the per-user plugin access gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PluginAccessMiddleware never blocked anything: a user could call the endpoints of a plugin they had disabled, and a plugin disabled system-wide stayed reachable over HTTP. The gate was inert at three independent points, each one silently passing the request on. Since FastAPI 0.140 include_router() no longer copies sub-routes into the parent's routes; it appends a single lazy _IncludedRouter branch with no .endpoint. That broke both halves of the plugin-name stamp: register_router's loop found nothing to stamp, and the middleware's lookup matched the branch object rather than the route. Both now flatten the branches with iter_route_contexts, matching on the context (whose path carries the include prefix) and reading the stamp off original_route. The third point never worked at all: dispatch read request.state.user, which nothing in the codebase writes. Authentication is a dependency, so it resolves after all middleware has run. The gate now identifies the caller from the request's bearer token using the helpers get_current_user is built from. Requests with no readable token still pass — plugin routers carry unauthenticated endpoints such as the Slack OAuth callback, and a per-user preference is meaningless without a user. Also drop the "/" entry from the default exclude_paths: entries are matched with startswith, so it excluded every path and left a middleware built without explicit paths enforcing nothing. Production passes its own list, so no behaviour changes there. Closes #586 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guides/plugins.md | 14 ++ sparkth/core/plugins/middleware.py | 82 +++++++++-- sparkth/core/routes/__init__.py | 15 +- tests/core/plugins/test_middleware.py | 203 ++++++++++++++++++++++++++ tests/core/test_routes.py | 53 +++++++ 5 files changed, 349 insertions(+), 18 deletions(-) create mode 100644 tests/core/plugins/test_middleware.py create mode 100644 tests/core/test_routes.py diff --git a/docs/guides/plugins.md b/docs/guides/plugins.md index 05d56203..fb2e2485 100644 --- a/docs/guides/plugins.md +++ b/docs/guides/plugins.md @@ -283,6 +283,20 @@ Analytics event schemas register through `register_event_schema(self, MyEvent)`: the plugin instance's declared name. The router above is reachable at `http://localhost:7727/api/v1/my-app/`. +### Who can reach plugin routes? + +Every route you register is gated by `PluginAccessMiddleware`. A request from a caller who +has turned your plugin off in their settings — or a request to a plugin disabled +system-wide — is answered with `403` before it reaches your handler, so a handler never has +to check plugin access itself. + +The gate identifies the caller from the request's bearer token. A request carrying no +readable token passes straight through it: a per-user preference is meaningless without a +user, so endpoints that are called by someone other than a logged-in user — an OAuth +callback, an inbound webhook — keep working. Those endpoints are reachable regardless of +anyone's plugin settings, so authenticate them the way you would any other public endpoint +(a signed state parameter, a request signature) rather than relying on the gate. + ## Frontend Metadata The backend is the single source of truth for what the frontend shows about a plugin. Declare it through three per-concern hooks from `sparkth.lib.frontend.hooks`, registered in `__init__` like every other hook. Human-facing names are passed positionally: diff --git a/sparkth/core/plugins/middleware.py b/sparkth/core/plugins/middleware.py index 4ad97794..114fd92d 100644 --- a/sparkth/core/plugins/middleware.py +++ b/sparkth/core/plugins/middleware.py @@ -2,6 +2,7 @@ from fastapi import Request, Response, status from fastapi.responses import JSONResponse +from fastapi.routing import iter_route_contexts from sqlalchemy.exc import DatabaseError, OperationalError from sqlmodel import Session, select from sqlmodel.ext.asyncio.session import AsyncSession @@ -10,20 +11,56 @@ from sparkth.core.models.plugin import Plugin, UserPlugin from sparkth.core.routes import get_route_plugin_name -from sparkth.lib.db import get_async_session +from sparkth.lib.auth import decode_token_username, get_user_by_username +from sparkth.lib.db import session_scope from sparkth.lib.log import get_logger logger = get_logger(__name__) +BEARER_SCHEME = "bearer" + + +def _bearer_token_username(request: Request) -> str | None: + """Username carried by the request's bearer token, or None when it carries no readable one. + + Reads the header directly rather than through the ``HTTPBearer`` security scheme, which + is a FastAPI dependency and so only resolves once routing has picked a handler — after + this middleware has already run. + """ + header = request.headers.get("authorization") + if header is None: + return None + + scheme, _, token = header.partition(" ") + if scheme.lower() != BEARER_SCHEME or not token: + return None + + return decode_token_username(token) + class PluginAccessMiddleware(BaseHTTPMiddleware): + """Reject requests to plugins the caller has turned off, before they reach the route. + + Runs ahead of routing, so it resolves both facts it needs itself: which plugin owns the + requested URL (from the name ``register_router`` stamps on plugin endpoints) and who is + asking (from the request's bearer token, via the helpers in ``sparkth.lib.auth`` that + ``get_current_user`` is built from). + + Anonymous requests fail open. Plugin routers carry unauthenticated endpoints — Slack's + OAuth callback is called by Slack itself, with no token — and a per-user preference is + meaningless without a user; endpoints that do require a caller are still rejected by + their own auth dependency. + """ + def __init__(self, app: Any, exclude_paths: list[str] | None = None) -> None: super().__init__(app) + # Entries are matched with startswith, so every one of them must be a real path + # prefix: a bare "/" would exclude every path there is and leave the gate + # enforcing nothing. self.exclude_paths = exclude_paths or [ "/docs", "/redoc", "/openapi.json", - "/", "/api/v1/auth", ] @@ -37,15 +74,15 @@ async def dispatch(self, request: Request, call_next: Callable[[Request], Awaita response = await call_next(request) return response - user = getattr(request.state, "user", None) - if not user: + username = _bearer_token_username(request) + if username is None: response = await call_next(request) return response - has_access = await self._check_plugin_access(user.id, plugin_name) + has_access = await self._user_may_use_plugin(username, plugin_name) if not has_access: logger.warning( - f"User {user.id} attempted to access disabled plugin '{plugin_name}' at path {request.url.path}" + f"User '{username}' attempted to access disabled plugin '{plugin_name}' at path {request.url.path}" ) return JSONResponse( status_code=status.HTTP_403_FORBIDDEN, @@ -65,19 +102,36 @@ def _is_excluded_path(self, path: str) -> bool: return False def _get_route_plugin_name(self, request: Request) -> str | None: - for route in request.app.routes: - match, _ = route.matches(request.scope) + """Name of the plugin owning the route this request targets, or None for a core route. + + Since FastAPI 0.140 include_router() no longer copies the sub-routes into + app.routes: it appends a single lazy _IncludedRouter branch, which has no .endpoint + carrying the plugin name. iter_route_contexts flattens those branches into contexts + that match on the *prefixed* path — the underlying route's own .path is unprefixed + and would never match the request — while original_route is the real route whose + endpoint holds the stamp. + """ + for context in iter_route_contexts(request.app.routes): + match, _ = context.matches(request.scope) if match == Match.FULL: - return get_route_plugin_name(route) + return get_route_plugin_name(context.original_route) return None - async def _check_plugin_access(self, user_id: int, plugin_name: str) -> bool: + async def _user_may_use_plugin(self, username: str, plugin_name: str) -> bool: + """Whether the user this token names still has the plugin enabled. + + A token naming a user that no longer exists passes: there is no preference to + enforce, and the route's own auth dependency rejects the request anyway. A database + failure blocks — the gate cannot confirm access, so it must not grant it. + """ try: - async for session in get_async_session(): - return await _check_plugin_access_async(user_id, plugin_name, session, check_system_enabled=True) - return False # Fallback if session generator doesn't yield + async with session_scope() as session: + user = await get_user_by_username(username, session) + if user is None or user.id is None: + return True + return await _check_plugin_access_async(user.id, plugin_name, session, check_system_enabled=True) except (DatabaseError, OperationalError) as e: - logger.error(f"Database error checking plugin access for user {user_id} and plugin '{plugin_name}': {e}") + logger.error(f"Database error checking plugin access for user '{username}' and plugin '{plugin_name}': {e}") return False diff --git a/sparkth/core/routes/__init__.py b/sparkth/core/routes/__init__.py index ea6bceaa..a7fe018e 100644 --- a/sparkth/core/routes/__init__.py +++ b/sparkth/core/routes/__init__.py @@ -1,6 +1,7 @@ from typing import cast from fastapi import APIRouter +from fastapi.routing import iter_route_contexts from starlette.routing import BaseRoute from sparkth.lib.plugins import SparkthPlugin @@ -24,10 +25,16 @@ def register_router(plugin: SparkthPlugin, router: APIRouter) -> None: tags=[f"plugin:{plugin.name}", plugin.name], ) - # Associate each route to the plugin - for route in prefixed_router.routes: - if hasattr(route, "endpoint"): - setattr(route.endpoint, PLUGIN_NAME_ATTRIBUTE, plugin.name) + # Associate each route to the plugin. + # + # Since FastAPI 0.140 include_router() no longer copies the sub-routes into + # prefixed_router.routes: it appends a single lazy _IncludedRouter branch, which has no + # .endpoint to stamp. iter_route_contexts flattens those branches back into the + # underlying routes, so the attribute lands on the endpoints the app actually serves. + for context in iter_route_contexts(prefixed_router.routes): + endpoint = getattr(context.original_route, "endpoint", None) + if endpoint is not None: + setattr(endpoint, PLUGIN_NAME_ATTRIBUTE, plugin.name) hooks.PLUGIN_ROUTERS.add_item(plugin, prefixed_router) diff --git a/tests/core/plugins/test_middleware.py b/tests/core/plugins/test_middleware.py new file mode 100644 index 00000000..c2030c90 --- /dev/null +++ b/tests/core/plugins/test_middleware.py @@ -0,0 +1,203 @@ +"""Behavioural tests for the per-user plugin access gate (PluginAccessMiddleware). + +The gate answers one question before a request reaches its route: does the caller still +have this plugin turned on? Each test drives a real request through the assembled app so +the whole chain is exercised — resolving which plugin owns the URL, resolving the caller +from the bearer token, and the access lookup itself. +""" + +from typing import cast + +from fastapi import FastAPI, Request +from httpx import AsyncClient +from sqlmodel.ext.asyncio.session import AsyncSession + +from sparkth.core.models.plugin import Plugin, UserPlugin +from sparkth.core.models.user import User +from sparkth.core.plugins.middleware import PluginAccessMiddleware +from sparkth.core.security import create_access_token +from sparkth.main import assemble_app + +CHAT_COMPLETIONS_PATH = "/api/v1/chat/completions" +SLACK_CALLBACK_PATH = "/api/v1/slack/oauth/callback" + + +def _request(app: FastAPI, method: str, path: str) -> Request: + return Request( + { + "type": "http", + "method": method, + "path": path, + "root_path": "", + "headers": [], + "query_string": b"", + "app": app, + } + ) + + +def _auth_headers(username: str) -> dict[str, str]: + return {"Authorization": f"Bearer {create_access_token({'sub': username})}"} + + +async def _seed_user(session: AsyncSession, username: str) -> User: + user = User( + name="Gate Test", + username=username, + email=f"{username}@example.com", + hashed_password="not-a-real-hash", + ) + session.add(user) + await session.commit() + await session.refresh(user) + return user + + +async def _seed_plugin(session: AsyncSession, name: str, enabled: bool) -> Plugin: + plugin = Plugin(name=name, enabled=enabled) + session.add(plugin) + await session.commit() + await session.refresh(plugin) + return plugin + + +async def _seed_user_plugin(session: AsyncSession, user: User, plugin: Plugin, enabled: bool) -> None: + session.add(UserPlugin(user_id=cast(int, user.id), plugin_id=cast(int, plugin.id), enabled=enabled)) + await session.commit() + + +class TestRoutePluginResolution: + """Which plugin owns the requested URL.""" + + def test_resolves_the_plugin_that_owns_a_plugin_route(self) -> None: + app = assemble_app() + middleware = PluginAccessMiddleware(app) + + assert middleware._get_route_plugin_name(_request(app, "POST", CHAT_COMPLETIONS_PATH)) == "chat" + + def test_returns_none_for_a_core_route(self) -> None: + app = assemble_app() + middleware = PluginAccessMiddleware(app) + + assert middleware._get_route_plugin_name(_request(app, "GET", "/api/v1/user/me")) is None + + def test_returns_none_for_an_unknown_path(self) -> None: + app = assemble_app() + middleware = PluginAccessMiddleware(app) + + assert middleware._get_route_plugin_name(_request(app, "GET", "/api/v1/nothing-here")) is None + + +class TestExcludedPaths: + """Which paths the gate declines to police.""" + + def test_does_not_exclude_plugin_paths_by_default(self) -> None: + # exclude_paths is matched with startswith, so a "/" entry would exclude every path + # there is and leave the gate enforcing nothing at all. + middleware = PluginAccessMiddleware(assemble_app()) + + assert middleware._is_excluded_path(CHAT_COMPLETIONS_PATH) is False + + def test_excludes_the_configured_paths(self) -> None: + middleware = PluginAccessMiddleware(assemble_app(), ["/api/v1/auth"]) + + assert middleware._is_excluded_path("/api/v1/auth/login") is True + + +class TestPluginAccessGate: + """What the gate does with a real request.""" + + async def test_blocks_a_plugin_the_user_disabled(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "disabled-chat-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, False) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + + async def test_blocks_a_plugin_disabled_system_wide(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "system-disabled-user") + plugin = await _seed_plugin(session, "chat", False) + await _seed_user_plugin(session, user, plugin, True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + + async def test_lets_through_a_plugin_the_user_enabled(self, client: AsyncClient, session: AsyncSession) -> None: + # The inverse of the block: without it, a gate that rejects everything would pass the + # tests above while breaking every plugin endpoint in the product. + user = await _seed_user(session, "enabled-chat-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code != 403 + + async def test_lets_through_a_plugin_with_no_user_preference( + self, client: AsyncClient, session: AsyncSession + ) -> None: + user = await _seed_user(session, "no-preference-user") + await _seed_plugin(session, "chat", True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code != 403 + + async def test_does_not_gate_core_routes(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "core-route-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, False) + + response = await client.get("/api/v1/user/me", headers=_auth_headers(user.username)) + + assert response.status_code == 200 + + async def test_does_not_gate_anonymous_requests(self, client: AsyncClient, session: AsyncSession) -> None: + # Unauthenticated plugin endpoints exist — Slack's OAuth callback is called by Slack + # itself, with no bearer token. Per-user access is meaningless without a user, so the + # gate must fail open rather than block the callback. + user = await _seed_user(session, "slack-callback-user") + plugin = await _seed_plugin(session, "slack", True) + await _seed_user_plugin(session, user, plugin, False) + + response = await client.get(SLACK_CALLBACK_PATH) + + assert response.status_code == 422 + + async def test_lets_through_a_request_carrying_an_invalid_token( + self, client: AsyncClient, session: AsyncSession + ) -> None: + # An unreadable token identifies nobody, so there is no per-user preference to enforce. + # The route's own auth dependency still rejects the request. + plugin = await _seed_plugin(session, "chat", True) + user = await _seed_user(session, "invalid-token-user") + await _seed_user_plugin(session, user, plugin, False) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers={"Authorization": "Bearer not-a-real-token"}, + ) + + assert response.json()["detail"] == "Could not validate credentials" diff --git a/tests/core/test_routes.py b/tests/core/test_routes.py new file mode 100644 index 00000000..0c02aedc --- /dev/null +++ b/tests/core/test_routes.py @@ -0,0 +1,53 @@ +"""Tests for plugin route registration in ``sparkth.core.routes``.""" + +from typing import Awaitable, Callable + +from fastapi import APIRouter +from fastapi.routing import iter_route_contexts + +from sparkth.core.routes import get_route_plugin_name, register_router +from sparkth.core.routes.hooks import PLUGIN_ROUTERS +from sparkth.lib.plugins import SparkthPlugin + + +async def _ping() -> dict[str, str]: + """Endpoint stood up purely so the router has a route to stamp.""" + return {"status": "ok"} + + +async def _pong() -> dict[str, str]: + """Second endpoint so each test stamps its own function object.""" + return {"status": "ok"} + + +def _plugin_router(endpoint: Callable[[], Awaitable[dict[str, str]]]) -> APIRouter: + router = APIRouter() + router.add_api_route("/ping", endpoint, methods=["GET"]) + return router + + +def _registered_router(plugin: SparkthPlugin) -> APIRouter: + routers = {registered: router for registered, router in PLUGIN_ROUTERS.iter_items()} + return routers[plugin] + + +def test_register_router_stamps_the_plugin_name_on_its_routes() -> None: + # The stamp is what PluginAccessMiddleware reads to decide which plugin owns a URL, so a + # router whose routes carry no plugin name leaves the per-user access gate with nothing + # to enforce. iter_route_contexts flattens the lazy _IncludedRouter branch include_router + # appends (FastAPI 0.140+) back into the underlying routes. + plugin = SparkthPlugin("stamp-test") + register_router(plugin, _plugin_router(_ping)) + + contexts = list(iter_route_contexts(_registered_router(plugin).routes)) + + assert [get_route_plugin_name(context.original_route) for context in contexts] == ["stamp-test"] + + +def test_register_router_prefixes_routes_with_the_plugin_namespace() -> None: + plugin = SparkthPlugin("prefix-test") + register_router(plugin, _plugin_router(_pong)) + + paths = [context.path for context in iter_route_contexts(_registered_router(plugin).routes)] + + assert paths == ["/api/v1/prefix-test/ping"] From be0272c9afc5f5f8aeebf7841785341b15114762 Mon Sep 17 00:00:00 2001 From: Abdul Rafey Date: Tue, 11 Aug 2026 13:39:07 +0500 Subject: [PATCH 3/6] test(core): cover every route-owning plugin in the access gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate resolving one plugin proves nothing about the rest, and a plugin it cannot name is a plugin it cannot police — the same silent failure this branch fixes. Slack and Google Drive nest their route includes deeper than chat, so cover all three. Co-Authored-By: Claude Opus 5 (1M context) --- tests/core/plugins/test_middleware.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/core/plugins/test_middleware.py b/tests/core/plugins/test_middleware.py index c2030c90..7cb868f7 100644 --- a/tests/core/plugins/test_middleware.py +++ b/tests/core/plugins/test_middleware.py @@ -8,6 +8,7 @@ from typing import cast +import pytest from fastapi import FastAPI, Request from httpx import AsyncClient from sqlmodel.ext.asyncio.session import AsyncSession @@ -69,11 +70,21 @@ async def _seed_user_plugin(session: AsyncSession, user: User, plugin: Plugin, e class TestRoutePluginResolution: """Which plugin owns the requested URL.""" - def test_resolves_the_plugin_that_owns_a_plugin_route(self) -> None: + @pytest.mark.parametrize( + "method, path, plugin_name", + [ + ("POST", CHAT_COMPLETIONS_PATH, "chat"), + ("GET", "/api/v1/slack/oauth/status", "slack"), + ("GET", "/api/v1/google-drive/oauth/status", "google-drive"), + ], + ) + def test_resolves_the_plugin_that_owns_a_plugin_route(self, method: str, path: str, plugin_name: str) -> None: + # Every plugin that mounts routes, not just one: these routers nest their includes to + # different depths, and a plugin the gate cannot name is a plugin it cannot police. app = assemble_app() middleware = PluginAccessMiddleware(app) - assert middleware._get_route_plugin_name(_request(app, "POST", CHAT_COMPLETIONS_PATH)) == "chat" + assert middleware._get_route_plugin_name(_request(app, method, path)) == plugin_name def test_returns_none_for_a_core_route(self) -> None: app = assemble_app() From a514111bcadee66158ee90d520c08989ffe7e8c1 Mon Sep 17 00:00:00 2001 From: Abdul Rafey Date: Tue, 11 Aug 2026 13:50:26 +0500 Subject: [PATCH 4/6] refactor(plugins): move BEARER_SCHEME to the plugin constants module Constants belong in constants.py, where PLUGIN_NAME_PATTERN already lives, rather than sitting at the top of the module that happens to use them. Co-Authored-By: Claude Opus 5 (1M context) --- sparkth/core/plugins/constants.py | 4 ++++ sparkth/core/plugins/middleware.py | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/sparkth/core/plugins/constants.py b/sparkth/core/plugins/constants.py index 1ffa3625..10230e36 100644 --- a/sparkth/core/plugins/constants.py +++ b/sparkth/core/plugins/constants.py @@ -10,3 +10,7 @@ # ``/dashboard/``) and is the key joining the backend plugin, its DB row, # and its frontend counterpart. PLUGIN_NAME_PATTERN = re.compile(r"[a-z0-9]+(-[a-z0-9]+)*") + +# The Authorization scheme the plugin access gate reads the caller's token from, compared +# against the header's scheme lowercased (RFC 7235 makes it case-insensitive). +BEARER_SCHEME = "bearer" diff --git a/sparkth/core/plugins/middleware.py b/sparkth/core/plugins/middleware.py index 114fd92d..61c0b756 100644 --- a/sparkth/core/plugins/middleware.py +++ b/sparkth/core/plugins/middleware.py @@ -10,6 +10,7 @@ from starlette.routing import Match from sparkth.core.models.plugin import Plugin, UserPlugin +from sparkth.core.plugins.constants import BEARER_SCHEME from sparkth.core.routes import get_route_plugin_name from sparkth.lib.auth import decode_token_username, get_user_by_username from sparkth.lib.db import session_scope @@ -17,8 +18,6 @@ logger = get_logger(__name__) -BEARER_SCHEME = "bearer" - def _bearer_token_username(request: Request) -> str | None: """Username carried by the request's bearer token, or None when it carries no readable one. From 7d9f7f868322a116fde1f7d2d3d65a981d1a30e6 Mon Sep 17 00:00:00 2001 From: Abdul Rafey Date: Tue, 11 Aug 2026 14:01:55 +0500 Subject: [PATCH 5/6] fix(deps): raise the fastapi floor to the version this code needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The declared floor was >=0.121.2, but the plugin access gate now imports iter_route_contexts, which does not exist before 0.138 — any environment resolving a permitted version below it fails at import, taking the app down rather than just the gate. The lockfile happens to pin 0.140.0, so CI never saw it. Measured against real installs rather than assumed: include_router stopped copying sub-routes in 0.137 (that is where the gate broke, and 0.137 offers no iter_route_contexts to fix it with), and 0.138 is the first version exposing the helper, verified to flatten, match on the prefixed path, and expose original_route.endpoint exactly as 0.140 does. The comments claiming 0.140 are corrected to 0.137 for the same reason. Also cover the two branches of the gate that decide what happens when the lookup cannot answer: a token naming a user that no longer exists passes, and a database failure blocks. Both were untested, and the second is the most security-relevant path in the gate. Co-Authored-By: Claude Opus 5 (1M context) --- docs/guides/plugins.md | 5 ++++ pyproject.toml | 2 +- sparkth/core/plugins/middleware.py | 2 +- sparkth/core/routes/__init__.py | 7 +++-- tests/core/plugins/test_middleware.py | 40 +++++++++++++++++++++++++++ tests/core/test_assemble_app.py | 2 +- tests/core/test_routes.py | 2 +- uv.lock | 2 +- 8 files changed, 54 insertions(+), 8 deletions(-) diff --git a/docs/guides/plugins.md b/docs/guides/plugins.md index fb2e2485..c2a3c1fd 100644 --- a/docs/guides/plugins.md +++ b/docs/guides/plugins.md @@ -297,6 +297,11 @@ callback, an inbound webhook — keep working. Those endpoints are reachable reg anyone's plugin settings, so authenticate them the way you would any other public endpoint (a signed state parameter, a request signature) rather than relying on the gate. +That applies to the system-wide switch too: disabling a plugin turns its endpoints off for +every *identified* caller, but its unauthenticated endpoints stay reachable, because the +gate has no caller to check them against. Treat "disabled" as a per-caller answer, not as a +kill switch for the plugin's HTTP surface. + ## Frontend Metadata The backend is the single source of truth for what the frontend shows about a plugin. Declare it through three per-concern hooks from `sparkth.lib.frontend.hooks`, registered in `__init__` like every other hook. Human-facing names are passed positionally: diff --git a/pyproject.toml b/pyproject.toml index 1a5cd227..769f3703 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,7 +14,7 @@ requires-python = ">=3.14" dependencies = [ "aiohttp>=3.13.2", "alembic>=1.17.2", - "fastapi[standard]>=0.121.2", + "fastapi[standard]>=0.138.0", "fastmcp>=2.13.1", "pydantic>=2.12.4", "sqlmodel>=0.0.27", diff --git a/sparkth/core/plugins/middleware.py b/sparkth/core/plugins/middleware.py index 61c0b756..eac00d1d 100644 --- a/sparkth/core/plugins/middleware.py +++ b/sparkth/core/plugins/middleware.py @@ -103,7 +103,7 @@ def _is_excluded_path(self, path: str) -> bool: def _get_route_plugin_name(self, request: Request) -> str | None: """Name of the plugin owning the route this request targets, or None for a core route. - Since FastAPI 0.140 include_router() no longer copies the sub-routes into + Since FastAPI 0.137 include_router() no longer copies the sub-routes into app.routes: it appends a single lazy _IncludedRouter branch, which has no .endpoint carrying the plugin name. iter_route_contexts flattens those branches into contexts that match on the *prefixed* path — the underlying route's own .path is unprefixed diff --git a/sparkth/core/routes/__init__.py b/sparkth/core/routes/__init__.py index a7fe018e..1b401873 100644 --- a/sparkth/core/routes/__init__.py +++ b/sparkth/core/routes/__init__.py @@ -27,10 +27,11 @@ def register_router(plugin: SparkthPlugin, router: APIRouter) -> None: # Associate each route to the plugin. # - # Since FastAPI 0.140 include_router() no longer copies the sub-routes into + # Since FastAPI 0.137 include_router() no longer copies the sub-routes into # prefixed_router.routes: it appends a single lazy _IncludedRouter branch, which has no - # .endpoint to stamp. iter_route_contexts flattens those branches back into the - # underlying routes, so the attribute lands on the endpoints the app actually serves. + # .endpoint to stamp. iter_route_contexts (added in 0.138, hence that dependency floor) + # flattens those branches back into the underlying routes, so the attribute lands on the + # endpoints the app actually serves. for context in iter_route_contexts(prefixed_router.routes): endpoint = getattr(context.original_route, "endpoint", None) if endpoint is not None: diff --git a/tests/core/plugins/test_middleware.py b/tests/core/plugins/test_middleware.py index 7cb868f7..6cb548dc 100644 --- a/tests/core/plugins/test_middleware.py +++ b/tests/core/plugins/test_middleware.py @@ -11,6 +11,7 @@ import pytest from fastapi import FastAPI, Request from httpx import AsyncClient +from sqlalchemy.exc import OperationalError from sqlmodel.ext.asyncio.session import AsyncSession from sparkth.core.models.plugin import Plugin, UserPlugin @@ -62,6 +63,11 @@ async def _seed_plugin(session: AsyncSession, name: str, enabled: bool) -> Plugi return plugin +async def _raise_operational_error(username: str, session: AsyncSession) -> User | None: + """Stand in for the user lookup when the database is unreachable.""" + raise OperationalError("SELECT 1", {}, Exception("connection lost")) + + async def _seed_user_plugin(session: AsyncSession, user: User, plugin: Plugin, enabled: bool) -> None: session.add(UserPlugin(user_id=cast(int, user.id), plugin_id=cast(int, plugin.id), enabled=enabled)) await session.commit() @@ -196,6 +202,40 @@ async def test_does_not_gate_anonymous_requests(self, client: AsyncClient, sessi assert response.status_code == 422 + async def test_lets_through_a_token_naming_a_user_that_does_not_exist( + self, client: AsyncClient, session: AsyncSession + ) -> None: + # Nobody to hold a preference, so there is nothing to enforce. The route's own auth + # dependency is what rejects the request. + await _seed_plugin(session, "chat", True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers("no-such-user"), + ) + + assert response.status_code != 403 + assert response.json()["detail"] == "User not found" + + async def test_blocks_when_the_access_lookup_fails( + self, client: AsyncClient, session: AsyncSession, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The gate fails closed: a lookup it could not complete is not permission to proceed. + user = await _seed_user(session, "db-error-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, True) + monkeypatch.setattr("sparkth.core.plugins.middleware.get_user_by_username", _raise_operational_error) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + async def test_lets_through_a_request_carrying_an_invalid_token( self, client: AsyncClient, session: AsyncSession ) -> None: diff --git a/tests/core/test_assemble_app.py b/tests/core/test_assemble_app.py index b7d940da..cd192c2a 100644 --- a/tests/core/test_assemble_app.py +++ b/tests/core/test_assemble_app.py @@ -21,7 +21,7 @@ def _route_paths(application: FastAPI) -> set[str]: """Every APIRoute path, prefixes applied. - Since FastAPI 0.140 include_router() no longer copies the sub-routes into + Since FastAPI 0.137 include_router() no longer copies the sub-routes into application.routes: it appends a single lazy _IncludedRouter branch instead. iter_route_contexts flattens those branches back into per-route contexts whose .path carries the include prefix. diff --git a/tests/core/test_routes.py b/tests/core/test_routes.py index 0c02aedc..4d3aec12 100644 --- a/tests/core/test_routes.py +++ b/tests/core/test_routes.py @@ -35,7 +35,7 @@ def test_register_router_stamps_the_plugin_name_on_its_routes() -> None: # The stamp is what PluginAccessMiddleware reads to decide which plugin owns a URL, so a # router whose routes carry no plugin name leaves the per-user access gate with nothing # to enforce. iter_route_contexts flattens the lazy _IncludedRouter branch include_router - # appends (FastAPI 0.140+) back into the underlying routes. + # appends (FastAPI 0.137+) back into the underlying routes. plugin = SparkthPlugin("stamp-test") register_router(plugin, _plugin_router(_ping)) diff --git a/uv.lock b/uv.lock index 8e42b891..ad67cd1f 100644 --- a/uv.lock +++ b/uv.lock @@ -3063,7 +3063,7 @@ requires-dist = [ { name = "authlib", specifier = ">=1.3.0" }, { name = "beautifulsoup4", specifier = ">=4.0" }, { name = "cryptography", specifier = ">=46.0.3" }, - { name = "fastapi", extras = ["standard"], specifier = ">=0.121.2" }, + { name = "fastapi", extras = ["standard"], specifier = ">=0.138.0" }, { name = "fastmcp", specifier = ">=2.13.1" }, { name = "google-api-core", specifier = ">=2.10.0" }, { name = "greenlet", specifier = ">=3.2.4" }, From aca30f6fe48617749c933c06418ec430e3acd44c Mon Sep 17 00:00:00 2001 From: Abdul Rafey Date: Wed, 12 Aug 2026 15:07:29 +0500 Subject: [PATCH 6/6] docs(plugins): state that the access gate does not cover MCP tools The guide described the gate's limits for HTTP callers but left the surface it does not reach at all unmentioned, so "disabled" read as covering the whole plugin. Tracked as #591. Co-Authored-By: Claude Opus 5 (1M context) --- docs/guides/plugins.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/guides/plugins.md b/docs/guides/plugins.md index c2a3c1fd..6b56f151 100644 --- a/docs/guides/plugins.md +++ b/docs/guides/plugins.md @@ -302,6 +302,13 @@ every *identified* caller, but its unauthenticated endpoints stay reachable, bec gate has no caller to check them against. Treat "disabled" as a per-caller answer, not as a kill switch for the plugin's HTTP surface. +The gate covers HTTP routes and nothing else — **a plugin's MCP tools are not gated at +all**. `/ai/mcp` is a mount rather than a plugin route, so no plugin name resolves for a +request to it, and the MCP surface carries no caller identity to check a preference +against. Disabling a plugin, for one user or system-wide, does not stop its tools being +called over MCP. If a tool must not run for someone, enforce that in the tool itself; do +not rely on the plugin being switched off. + ## Frontend Metadata The backend is the single source of truth for what the frontend shows about a plugin. Declare it through three per-concern hooks from `sparkth.lib.frontend.hooks`, registered in `__init__` like every other hook. Human-facing names are passed positionally: