fix(plugins): enforce the per-user plugin access gate

docs/guides/plugins.md

@@ -283,6 +283,32 @@ 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.
+
+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.
+
+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:

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",

sparkth/core/plugins/constants.py

@@ -10,3 +10,7 @@
 # ``/dashboard/<name>``) 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"

sparkth/core/plugins/middleware.py

@@ -2,28 +2,64 @@
 
 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
 from starlette.middleware.base import BaseHTTPMiddleware
 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.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__)
 
 
+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 +73,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 +101,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.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
+        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
 
 

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,17 @@ 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.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 (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:
+            setattr(endpoint, PLUGIN_NAME_ATTRIBUTE, plugin.name)
 
     hooks.PLUGIN_ROUTERS.add_item(plugin, prefixed_router)
 

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,