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

[abdulrafey1]

Closes: Users can call endpoints of plugins they have disabled

What

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. This makes the gate enforce, and covers it with the behavioural tests it never had.

The gate was inert at three independent points, not the one the issue describes. Each silently passed the request on, so the failure was invisible.

#	Where	Why it did nothing
1	sparkth/core/routes/__init__.py:28	register_router's stamping loop iterated prefixed_router.routes, which since FastAPI 0.137 is a single lazy _IncludedRouter branch with no .endpoint. PLUGIN_NAME_ATTRIBUTE was never set on any endpoint — the loop body never ran.
2	sparkth/core/plugins/middleware.py:67	The lookup matched that same _IncludedRouter branch, so get_route_plugin_name returned None. This is the layer #586 describes.
3	sparkth/core/plugins/middleware.py:40	dispatch read request.state.user, which nothing in the codebase writes — that line was the only mention of request.state in sparkth/. Auth is a dependency, so it resolves after all middleware has run; user was always None.

Layers 1 and 2 are the same FastAPI 0.137 change hitting two places: include_router() no longer copies sub-routes into the parent's routes. Both now flatten those branches with iter_route_contexts.

Layer 3 needed a decision rather than a repair, since the middleware cannot be handed a user that has not been resolved yet. It now identifies the caller from the request's bearer token, reusing the helpers get_current_user is built from so the two paths cannot drift.

Note

The snippet in the issue's proposed-fix comment does not work as written, as that comment anticipated: context.original_route.matches(scope) returns Match.NONE, because original_route.path is the unprefixed /completions. Matching goes through context.matches(scope) (whose path_regex carries the include prefix); original_route is only used to read the stamp.

Anonymous requests deliberately pass the gate. 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.

Changes

• fix(plugins): stamp the plugin name on the routes include_router actually mounts, via iter_route_contexts
• fix(plugins): resolve the route's plugin through the flattened route contexts instead of the _IncludedRouter branch
• fix(plugins): identify the caller from the request's bearer token, since no middleware populates request.state.user
• fix(plugins): drop the "/" entry from the default exclude_paths — entries are matched with startswith, so it excluded every path
• refactor(auth): extract decode_token_username and get_user_by_username from get_current_user, so the gate and the dependency share one implementation
• refactor(plugins): move BEARER_SCHEME into core/plugins/constants.py, alongside PLUGIN_NAME_PATTERN
• fix(deps): raise the fastapi floor to >=0.138.0, the first version exposing iter_route_contexts
• test(core): cover the gate's behaviour — blocking, letting through, failing open on anonymous, failing closed on a database error — plus the plugin-name stamp and the auth helpers
• docs(plugins): document that plugin routes are gated, and that unauthenticated endpoints are not

How to Test

1. make mypy && make lint.backend && make lint.format.backend check=1 — all clean.

2. python -m pytest -q — 1680 passed, 3 skipped. No existing test changed behaviour.

3. python -m pytest tests/core/plugins/test_middleware.py tests/core/test_routes.py tests/lib/test_auth.py -q — 26 passed, the new coverage.

4. Confirm the tests fail without the fix: git stash the sparkth/ changes and re-run step 3. test_blocks_a_plugin_the_user_disabled returns 422 instead of 403 — the request sailed past the gate and reached body validation — and test_register_router_stamps_the_plugin_name_on_its_routes asserts [None] == ['stamp-test'].

5. Run the reproduction from the issue against this branch. It prints chat where it printed None:

   from sparkth.main import assemble_app
   from sparkth.core.plugins.middleware import PluginAccessMiddleware
   
   app = assemble_app()
   mw = PluginAccessMiddleware(app)
   scope = {"type": "http", "method": "POST", "path": "/api/v1/chat/completions",
            "root_path": "", "headers": [], "query_string": b""}
   
   class R:
       def __init__(self, app, scope): self.app, self.scope = app, scope
   
   print(mw._get_route_plugin_name(R(app, scope)))   # -> "chat"

6. End-to-end, against a running app: disable the chat plugin for a user, then POST /api/v1/chat/completions with that user's token. Expect 403 naming the plugin, and a WARNING in the logs. Re-enable it and repeat — the request reaches the handler.

Notes

• No migration and no new env var. One dependency floor raised, see below.
• Behaviour change, intended: endpoints of a disabled plugin now return 403 where they previously served the request. That is the bug being fixed, but anyone relying on the gate being inert will see requests start failing.
• A request to a plugin path with the wrong HTTP method matches Match.PARTIAL, not Match.FULL, so it reaches the router and returns 405 rather than 403. No plugin functionality is reachable that way; called out because it is visible.
• Dependency floor raised: fastapi[standard]>=0.121.2 → >=0.138.0. iter_route_contexts does not exist below 0.138, so the previously-permitted range would ImportError at startup. Measured per version: include_router stopped copying sub-routes in 0.137 (where the gate actually broke, with no iter_route_contexts to fix it), and 0.138–0.140 behave identically for everything this code relies on. The Since FastAPI 0.140 comments, including the pre-existing one in tests/core/test_assemble_app.py, are corrected to 0.137.
• Gated requests cost one extra database round trip (resolve the caller, then check access) in a single session. The route scan in _get_route_plugin_name runs on every non-excluded request as it always has — unchanged by this PR; only the token/DB cost is new, and that falls solely on plugin requests.
• The gate covers HTTP routes only — a plugin's MCP tools are not gated at all. /ai/mcp is a mount, so no plugin name resolves, and the MCP surface has no authenticated caller to check a preference against. Disabling a plugin does not stop its tools being called over MCP. Documented in docs/guides/plugins.md and tracked in Plugin tools remain callable over MCP after the plugin is disabled #591; deliberately not fixed here, since the correct seam is a call-time FastMCP middleware rather than anything in this gate.
• Worth a follow-up, out of scope here: this gate would fit the codebase better as a route dependency (dependencies=[...], as CLAUDE.local.md prescribes for gatekeeping) than as middleware. It would run after routing, so it would need neither its own token decoding nor any coupling to FastAPI's routing internals — which is what broke here, in two places at once. That is an architecture change, and this security fix should be reviewed on its own.

This description was written with the assistance of an LLM (Claude).

[abdulrafey1]

@claude-review

[claude]

Claude finished @abdulrafey1's task in 3m 11s —— View job

---

Claude Code Review

• Read changed files & understand scope
• Security & authorization review
• Correctness & edge cases
• Performance analysis
• Test coverage
• Post review feedback

This is a well-executed, well-documented security fix. The three-layer diagnosis is accurate, the refactor that lets the gate and get_current_user share decode_token_username/get_user_by_username is the right call, and the behavioural test suite is genuinely thorough (every route-owning plugin, core routes, anonymous, invalid-token). Docstrings and the plugins guide are updated alongside the code. Nice work.

A few things worth addressing before merge.

---

🔴 High — the fastapi version floor no longer matches the code

pyproject.toml:17 still pins fastapi[standard]>=0.121.2, but this PR introduces a hard dependency on iter_route_contexts (imported in both sparkth/core/routes/__init__.py:4 and sparkth/core/plugins/middleware.py:5), which — as the PR body itself explains — is the FastAPI 0.140 helper for the new lazy _IncludedRouter branches. iter_route_contexts does not exist in 0.121–0.139, so any environment that resolves a FastAPI in that (currently-allowed) range will ImportError at module import, taking down the whole app rather than just the gate.

uv.lock happens to pin 0.140.0, so CI is green, but the declared floor is the contract for fresh resolves and downstream consumers. Bump it to match the code:

"fastapi[standard]>=0.140.0",

Fix this →

---

🟡 Medium — the fail-closed (and fail-open-on-missing-user) branches are untested

See the inline comment on middleware.py:133-135. The DB-error path returns False (blocks → 403), which is the single most security-relevant behaviour in the gate, and the user is None → return True fail-open path sits right beside it. Neither is exercised. Given the repo's TDD rule and the "verify error paths are covered" review requirement, both deserve a test.

Minor wording nit in the same area: the docstring on _user_may_use_plugin and the PR body describe a DB error as "failing open", but the code correctly fails closed (return False). Worth aligning the language so intent is unambiguous.

---

🟢 Low / notes (non-blocking)

• System-wide disable is not enforced for anonymous endpoints. Because the gate fails open on anonymous requests (correct for per-user preferences), a plugin that is disabled system-wide still has its unauthenticated endpoints (e.g. Slack's oauth_callback) reachable over HTTP — check_system_enabled is only consulted once a user is resolved. This is a reasonable trade-off and matches the documented intent, but "disabled system-wide stays reachable over HTTP" for anonymous routes is the exact shape of the original bug, so it's worth an explicit line in the docs/PR notes.
• Coupling to iter_route_contexts. You already flag the route-dependency refactor as the proper long-term fix; agreed. Until then, this is the second time a FastAPI routing-internals change has broken this code, so the follow-up is worth prioritising rather than deferring indefinitely.
• _get_route_plugin_name runs a full route scan on every non-excluded request (core routes included), same as before — not a regression, just noting the PR's "non-plugin requests are unaffected" is true only of the new token/DB cost, not the pre-existing scan.

---

Assessment: Correct fix, strong tests, good docs. The only thing I'd consider blocking is the fastapi floor bump — it's a latent import-time crash for a currently-permitted dependency version. The test-coverage gap on the error branches is a should-fix given the project's TDD stance. Everything else is optional.
· fix/586-plugin-access-middleware-gate

[claude]

Medium — this fail-closed branch is the most security-critical path in the gate and has no test. On a DB error the gate returns False, i.e. it blocks the request (403). The new suite covers blocking-by-preference, blocking-system-wide, and letting-through, but not this "the lookup itself failed, so deny" behaviour, nor the sibling fail-open branch above where the token names a user that no longer exists (user is None -> return True). Given the repo's TDD rule and that error paths are the ones most likely to silently regress, both branches are worth a test — e.g. patch session_scope/get_user_by_username to raise OperationalError and assert 403, and seed a token whose sub matches no user and assert the request isn't 403 from the gate.

Note the docstring/PR wording: a DB error is documented as "failing open" but the code correctly fails closed here — worth aligning the wording so the intent is unambiguous.

[abdulrafey1]

Both branches now have a test, and both were verified to fail when the branch they cover is inverted (flip return False → return True here and test_blocks_when_the_access_lookup_fails fails; flip the user is None branch and test_lets_through_a_token_naming_a_user_that_does_not_exist fails). The DB-error test patches get_user_by_username to raise OperationalError and asserts 403.

On the wording: the docstring already says the DB-error path blocks, not that it fails open —

A database failure blocks — the gate cannot confirm access, so it must not grant it.

The PR body's "failing open" refers to the anonymous path in the test-coverage bullet, not to database errors. Neither describes a DB error as failing open, so there's nothing to align here.

[abdulrafey1]

Addressed — with a correction on the version range

🔴 fastapi floor — real problem, wrong numbers

The finding holds: >=0.121.2 permits versions where iter_route_contexts does not exist, so the app ImportErrors at startup. Bumped — but to >=0.138.0, not >=0.140.0, because the suggested range does not match what the packages actually contain. Measured against real installs of each version:

fastapi	include_router() yields	iter_route_contexts
≤ 0.136.0	APIRoute (has .endpoint) — the old code worked	absent
0.137.0	_IncludedRouter — the gate breaks here	absent — breakage with no way to fix it
0.138.0	_IncludedRouter	present
0.139.0	_IncludedRouter	present
0.140.0	_IncludedRouter	present

0.137.0 -> ImportError: cannot import name 'iter_route_contexts' from 'fastapi.routing'
0.138.0 -> has iter_route_contexts

>=0.140.0 would have worked, but it over-constrains downstream consumers by two minor versions for no reason. 0.138.0 is both necessary and sufficient: the semantics this PR depends on — flattening the branch, context.matches() resolving against the prefixed path, original_route.endpoint carrying the stamp — were exercised on 0.138.0, 0.139.0 and 0.140.0 and all three resolve chat identically.

Two consequences worth noting beyond the pin:

• The gate broke in 0.137, not 0.140. The Since FastAPI 0.140 comments — including the pre-existing one in tests/core/test_assemble_app.py this PR copied its framing from — were wrong, and are corrected to 0.137.
• 0.137 is a genuine dead zone: the lazy branches are there, iter_route_contexts is not. Nothing to do about it other than exclude it, which the new floor does.

🟡 Untested error branches — fixed

test_blocks_when_the_access_lookup_fails (patches get_user_by_username to raise OperationalError, asserts 403) and test_lets_through_a_token_naming_a_user_that_does_not_exist. Both were verified honest by inverting the branch each covers and confirming it fails.

The wording nit is answered in the inline thread: the docstring says the DB path blocks, and the PR body's "failing open" refers to the anonymous path.

🟢 System-wide disable on anonymous endpoints — documented

Correct, and worth stating outright, so docs/guides/plugins.md now says it: 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. "Disabled" is a per-caller answer, not a kill switch for the plugin's HTTP surface.

🟢 Route scan — agreed, the note was imprecise

The full scan on every non-excluded request is pre-existing and unchanged; only the token/DB cost is new. Fair point that "non-plugin requests are unaffected" reads as broader than it is.

🟢 Route-dependency refactor

Agreed, and the argument is stronger than "eventually": this is now the second FastAPI routing-internals change to break this code, and 0.137 shows the failure can arrive in a release with no escape hatch at all. It stays out of this PR so the security fix is reviewable on its own.

This comment was written with the assistance of an LLM (Claude).

[hamza-56]

This section spells out the gate's holes but leaves out the biggest one: a plugin's MCP tools are not gated at all. register_plugin_tools registers every plugin's tools on the shared FastMCP server, and the /ai/mcp mount is a Mount with no endpoint, so _get_route_plugin_name returns None and the gate always passes. An admin who disables a plugin system-wide, expecting it off, still has all of its tools callable over MCP. Worth stating here so nobody reads "disabled" as covering the plugin's whole surface.

[abdulrafey1]

Confirmed and documented in aca30f6. /ai/mcp is not excluded and _get_route_plugin_name returns None for it, so the gate passes every MCP request through — and register_plugin_tools registers every tool unconditionally.

Worth splitting the gap in two, because the halves are not equally fixable: the MCP surface has no authenticated caller at all (FastMCP takes no auth provider, the mount has no dependency, handlers take no user id), so per-user enforcement has nothing to check against. System-wide enforcement needs no identity and is doable now — which is the half your admin example lands on.

The new paragraph says both, and points at the tool itself as the place to enforce rather than the plugin switch.

Fix tracked in #591 with a proposed approach: a call-time FastMCP middleware beside ToolCallAuditMiddleware, not registration-time filtering — tools register once at startup while enabled flips at runtime. Keeping it out of this PR so the HTTP fix stays reviewable on its own.

One thing that turned up while checking: check_user_plugin_access, get_user_enabled_plugins and get_user_disabled_plugins in middleware.py have no callers anywhere. They look like the tool-side gate that was never wired up. Noted in #591 as either its implementation or a deletion.

[hamza-56]

A few small things, nothing blocking.

[hamza-56]

Plain startswith matches past the segment boundary, so /api/v1/auth would also exclude a hypothetical /api/v1/authors. Matching path == p or path.startswith(p + "/") keeps exclusions scoped to their own subtree.

[hamza-56]

This re-flattens and scans the whole route table on every request, and Starlette then repeats the same matching to dispatch. Routes are static after startup, so building the flattened list (or a path to plugin-name cache) once in __init__ would be cheap. Not blocking, just runs on every single request.

[hamza-56]

Heads up that an authenticated plugin request now costs 3 sequential queries here (user, plugin, user_plugin), and then get_current_user decodes the same token and fetches the same user again. Could collapse into one joined query and/or stash the user on request.state for the dependency to reuse. Fine to punt, just worth knowing.