fix(transports): share plugin context with tool, prompt and resource hooks on /mcp - #6140
fix(transports): share plugin context with tool, prompt and resource hooks on /mcp#61400717376 wants to merge 3 commits into
Conversation
5044fe6 to
37eb445
Compare
|
@araujof can you review this so that we can correctly use CPEX to share context safely. i think you mentioned https://contextforge-org.github.io/cpex/docs/extensions/ previously in this #3915 (comment) |
37eb445 to
2715edd
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Reviewed against issue #3879 end-to-end: root cause, fix wiring, and blast radius all check out.
- Root cause confirmed:
call_tool()never readplugin_context_table/plugin_global_contextoffrequest.stateand never forwarded them toinvoke_tool(), soTOOL_PRE_INVOKEhooks on/mcpalways saw empty context while REST paths worked fine. - Fix verified correct, not just accepted as-is: traced the full chain —
HttpAuthMiddlewarewritesrequest.state.plugin_context_table/plugin_global_context→ new_get_plugin_contexts_or_none()reads the same keys back offscope["state"](same technique already proven by_get_request_context_or_default()Path 2 andmiddleware/rbac.py) →call_tool()forwards both intoinvoke_tool()→tool_service.pythreadsplugin_context_tableintolocal_contextsand mergesplugin_global_contextvia_apply_tool_payload_to_global_context(), which enriches rather than overwrites (soserver_idis always corrected regardless of what's forwarded). - Minimal, scoped correctly: only touches
call_tool();invoke_tool_direct()(direct_proxy) and the Rust runtime path are explicitly out of scope,get_prompt/read_resourcehave the same gap but are reasonably deferred to a follow-up. - No schema/migration needed — no persisted state involved.
- No breaking change: checked every shipped plugin implementing
tool_pre_invoke(header_filter,schema_guard,vault,webhook_notification) against every plugin implementinghttp_pre_request(custom_auth_example,simple_token_auth) — zero overlap today, so no existing plugin's behavior changes. Fallback stays(None, None), identical to current behavior. - Security note: this closes a real authz-relevant gap — a custom authorization plugin pairing these two hooks over
/mcpwas silently getting an empty context; if such a plugin fails open on missing state, that was a live access-control risk specific to this transport. Worth double-checking both the populated and empty-context paths on any custom authz plugin before relying on this in production. - Tests: ran the new tests plus the full transport suite locally — all green, no regressions. Coverage is solid (forward path, empty-scope path, no-request-context path).
- Confirmed via
gh pr diffthat the change is scoped to exactly the two files it should touch, no unrelated changes.
Two purely optional, non-blocking suggestions for a future pass: a test covering the isinstance type-guard fallback for malformed scope state, and an e2e test modeled on the issue's own repro for extra confidence. Neither blocks this.
Follow-up to the review on IBM#6140, addressing both optional suggestions. _get_plugin_contexts_or_none() type-guards each slot of scope["state"] independently, but nothing exercised the rejection paths. Add a parametrised test for a non-mapping scope, a missing or non-mapping state, and a wrongly typed value in either slot - the last two also assert that one bad slot does not discard the other, valid one. Add an end-to-end test modelled on the issue's own repro: a real PluginManager loaded with the shared CrossHookContextPlugin fixture runs HTTP_PRE_REQUEST and HTTP_AUTH_CHECK_PERMISSION, the resulting contexts are handed over on the ASGI scope the way HttpAuthMiddleware does them, and call_tool() drives TOOL_PRE_INVOKE through the same dispatch ToolService performs. The fixture plugin raises as soon as a hook cannot see earlier hook state, so reaching the assertions is the guarantee. A negative control pins the pre-fix behaviour: invoking TOOL_PRE_INVOKE with local_contexts=None still fails with "http_timestamp not found in tool hook", so the e2e test cannot silently stop testing anything. PluginManager is a Borg singleton, so both tests reset its shared state around themselves. Signed-off-by: Sergey Muravsky <s.muravsky@gigab2b.ru>
2715edd to
0ee5f77
Compare
|
Hi @0717376, Please check this review. Some of the reviews may be way out of line, if so, then comment about them here and ignore it. Others, please work on them if you think they are legit. SummaryBlocking: 1 | Functionally Impacting: 1 | Suggestions: 3 | Minor: 2 PR adds 🚨 BlockingF-1:
|
| Branch / Line | Covered |
|---|---|
LookupError from request_context |
✅ test_get_plugin_contexts_returns_none_without_request_context |
except Exception (non-LookupError) |
❌ No test |
scope not a dict |
✅ scope_not_a_dict param |
state missing from scope |
✅ state_missing param |
state not a dict |
✅ state_not_a_dict param |
global_context wrong type |
✅ global_context_wrong_type param |
context_table wrong type |
✅ context_table_wrong_type param |
Both slots None |
✅ both_none param |
| Both slots populated and forwarded | ✅ test_call_tool_forwards_plugin_contexts_from_scope |
| Plugin manager receives forwarded contexts | ✅ E2E test |
| Pre-fix failure reproduced | ✅ test_cross_hook_plugin_fails_without_forwarded_contexts |
get_prompt context gap (known) |
❌ No test; no tracking marker |
read_resource context gap (known) |
❌ No test; no tracking marker |
HttpAuthMiddleware records the GlobalContext and PluginContextTable produced by HTTP_PRE_REQUEST hooks on request.state, and every REST handler in main.py forwards them into the service layer so later hooks can read state written by earlier ones. call_tool() in the Streamable HTTP transport did not, so tool_service built a fresh GlobalContext and passed local_contexts=None: anything a plugin stored during HTTP_PRE_REQUEST was dropped by the time TOOL_PRE_INVOKE ran on /mcp. Read both contexts from the ASGI scope (where request.state writes them) and forward them to invoke_tool(). Reading the scope rather than a ContextVar mirrors path 2 of _get_request_context_or_default() and survives the MCP SDK task-group boundaries. Missing or unexpected values degrade to (None, None), preserving current behaviour. Closes IBM#3879 Signed-off-by: Sergey Muravsky <0717376@gmail.com>
Follow-up to the review on IBM#6140, addressing both optional suggestions. _get_plugin_contexts_or_none() type-guards each slot of scope["state"] independently, but nothing exercised the rejection paths. Add a parametrised test for a non-mapping scope, a missing or non-mapping state, and a wrongly typed value in either slot - the last two also assert that one bad slot does not discard the other, valid one. Add an end-to-end test modelled on the issue's own repro: a real PluginManager loaded with the shared CrossHookContextPlugin fixture runs HTTP_PRE_REQUEST and HTTP_AUTH_CHECK_PERMISSION, the resulting contexts are handed over on the ASGI scope the way HttpAuthMiddleware does them, and call_tool() drives TOOL_PRE_INVOKE through the same dispatch ToolService performs. The fixture plugin raises as soon as a hook cannot see earlier hook state, so reaching the assertions is the guarantee. A negative control pins the pre-fix behaviour: invoking TOOL_PRE_INVOKE with local_contexts=None still fails with "http_timestamp not found in tool hook", so the e2e test cannot silently stop testing anything. PluginManager is a Borg singleton, so both tests reset its shared state around themselves. Signed-off-by: Sergey Muravsky <s.muravsky@gigab2b.ru>
…on /mcp get_prompt() and read_resource() on the Streamable HTTP transport had the same gap as call_tool(): the REST handlers in main.py forward plugin_global_context and plugin_context_table into PromptService.get_prompt() and ResourceService.read_resource(), the /mcp handlers did not. Both services already accept the arguments, so PROMPT_PRE_FETCH and RESOURCE_PRE_FETCH hooks reached through /mcp saw an empty context for exactly the reason described in IBM#3879. Reuse _get_plugin_contexts_or_none() in both handlers and forward the result. The direct-proxy branch of read_resource() runs no gateway-side hooks and is unaffected. Document the hand-off in the three handler docstrings. Review follow-ups on the tests: - Cover the broad-except branch of _get_plugin_contexts_or_none(): a non-LookupError while resolving the request context is logged at debug level and degrades to (None, None). - Add forwarding tests for get_prompt() and read_resource(), mirroring the call_tool() one. - Restructure the end-to-end tests around a shared session helper that runs HTTP_PRE_REQUEST and HTTP_AUTH_CHECK_PERMISSION on a real PluginManager, then drive all three /mcp handlers through the same pre-hook dispatch the services perform. The negative control is now parametrised over the three MCP pre-hooks. - Evaluate _malformed_scope_cases() once at collection time and pass client_host/client_port consistently. Signed-off-by: Sergey Muravsky <s.muravsky@gigab2b.ru>
0ee5f77 to
a82ff93
Compare
|
@ja8zyjits thanks — this was a useful pass. Everything is in F-1 — added F-2 — you were right that a tracking marker is the minimum, but it turned out to cost about as much as the fix: S-1 — fixed, S-2 — left as is, deliberately: S-3 — both e2e paths now go through one M-1 — agreed, noted for a future consolidation; not touching it here. M-2 — pre-existing in Docs — one sentence added to each of the three handler docstrings. The Locally: transports + plugins + prompt/resource service suites 1870 passed; doctests, pre-commit and pylint clean. |
📌 Summary
Closes #3879.
Tool calls, prompt fetches and resource reads made over the Streamable HTTP
/mcptransport never receive the plugin contexts produced by
HTTP_PRE_REQUEST, so a plugincannot share state between its own hooks on the one path an MCP client actually uses. Every REST handler already
forwards them;
/mcpis the only remaining gap.🔁 Reproduction Steps
http_pre_request+tool_pre_invokeplugin that stores a value incontext.stateduring the first hook and reads it back in the second(e.g. a per-user authorization plugin that stashes the caller's token).
POST /servers/{server_id}/mcp/.tool_pre_invokeobserves an empty context and the stored value is gone.The same plugin works when the tool is invoked over the REST API.
🐞 Root Cause
HttpAuthMiddlewarerecords the results of the pre-request hooks on the requeststate (
mcpgateway/middleware/http_auth_middleware.py:224-227):ToolService.invoke_tool()accepts both (plugin_context_tableis documented as"Optional plugin context table from previous hooks for cross-hook state sharing"),
and
mcpgateway/main.pyreads them offrequest.stateat every REST call site.call_tool()inmcpgateway/transports/streamablehttp_transport.pydoes not — itcalls
invoke_tool()without either argument, sotool_servicebuilds a freshGlobalContextand passeslocal_contexts=Noneto the hook manager. Anything aplugin wrote during
HTTP_PRE_REQUESTis dropped.💡 Fix Description
call_tool(),get_prompt()andread_resource()now resolve the two contexts andforward them to the matching service method — all three already accept the arguments.
The new
_get_plugin_contexts_or_none()helper reads them from the ASGI scope(
scope["state"], which is whatrequest.statewrites into) rather than from aContextVar. This mirrors path 2 of the existing_get_request_context_or_default()helper and, as its docstring notes, survives the task-group boundaries introduced by
the MCP SDK where
ContextVars may be lost. Missing or unexpected values degrade to(None, None), i.e. exactly today's behaviour.Scope notes:
invoke_tool()path is touched.invoke_tool_direct()(
direct_proxy) does not take plugin contexts, and the Rust runtime is trackedseparately in [FEATURE]: Port MCP plugin context sharing to the Rust MCP runtime #4119.
Relationship to the earlier attempt: #3915 fixed the same gap and was closed in favour
of a CPEX gated-extensions approach. Since then
mainhas continued to standardise onplugin_context_table— it is threaded through ~13 REST call sites inmain.pyandmiddleware/rbac.pynow uses it to share context withHTTP_AUTH_CHECK_PERMISSION—while
mcpgateway/transports/still contains zero references to it. This PR closesthat inconsistency with the mechanism already in use. If maintainers would rather land
this as a CPEX gated extension, I'm glad to redo it that way.
📏 Reviewability
triage🧪 Verification
19 tests cover the change in
tests/unit/mcpgateway/transports/test_streamablehttp_transport.py:forwarding for all three handlers, every type-guard fallback of the helper, an
end-to-end run of the issue's repro against a real
PluginManagerwith the sharedCrossHookContextPluginfixture for each ofTOOL_PRE_INVOKE/PROMPT_PRE_FETCH/RESOURCE_PRE_FETCH, and a negative control per hook that reproduces the pre-fixfailure with
local_contexts=None.pytest tests/unit/mcpgateway/transports/ tests/unit/mcpgateway/plugins/+ prompt/resource service testspytest --doctest-modules mcpgateway/transports/streamablehttp_transport.pypre-commit run --files <both files>pylint mcpgateway/transports/streamablehttp_transport.pymain📐 MCP Compliance (if relevant)
✅ Checklist
black,isort,ruff)