Plugin hooks for Prompts/Resources MCP paths - #73
Conversation
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com> # Conflicts: # crates/contextforge-gateway-rs-lib/src/gateway/mcp_service/prompts.rs # crates/contextforge-gateway-rs-lib/src/gateway/mcp_service/resources.rs
There was a problem hiding this comment.
@madhu-mohan-jaishankar, I’m requesting changes based on full-stack testing of commit 0305789 with a real MCP client → gateway → CPEX plugin → backend path.
The blocking issues are:
- Prompt post-hooks can remove sensitive text, but the gateway silently restores and returns the original text.
- The same fail-open behavior affects
resources/readcontent. - Subscription URI rewrites are recomputed during unsubscribe instead of reusing the URI selected during subscribe, so a changed or stateful rewrite can leave the backend subscription and gateway tracking entry active.
| let Some(text) = message.content.iter().find_map(|part| match part { | ||
| ContentPart::Text { text } => Some(text), | ||
| _ => None, | ||
| }) else { |
There was a problem hiding this comment.
A post-hook can remove every text part, but this continue retains the backend’s original text. In a full MCP client → gateway → CPEX plugin → backend test, the plugin cleared review of secret, yet the client still received that original value. Please treat missing text as an invalid plugin result or implement explicit redaction semantics; it must not fall back to the sensitive original.
| } | ||
|
|
||
| for (resource, original_contents) in resources.iter().zip(original.contents.iter_mut()) { | ||
| let Some(text) = &resource.content else { continue }; |
There was a problem hiding this comment.
A plugin can set Resource.content to None, but this branch leaves the original backend text untouched. The end-to-end test removed quarterly numbers, yet the client still received it. Please fail closed or implement explicit deletion semantics instead of restoring the original content.
| // untracked, so a rewritten subscribe and unsubscribe pair resolve to the same tracking key. | ||
| let resource_uri = match &mcp_service.plugin_runtime { | ||
| Some(plugin_runtime) => { | ||
| plugin_runtime.before_unsubscribe(&resource_uri, &service_name).await?.apply_to(resource_uri) |
There was a problem hiding this comment.
The rewritten URI selected during subscribe is not stored against the client’s original URI. An end-to-end plugin rewrote subscribe to rewritten://resource-1 and unsubscribe to rewritten://resource-2; the backend therefore never unsubscribed resource-1 and the gateway retained its tracking entry. Please persist the original-to-upstream mapping at subscribe time and consume that mapping during unsubscribe.
There was a problem hiding this comment.
Thanks for the docs work here, the payload-shape table and the mutation-limits section made this much easier to reason about. Two findings, on top of the review already open:
1. Subscribe URI rewrite misroutes notifications (resources.rs:216-225, backend_client.rs:158-171)
The subscription is tracked under the rewritten URI, and that key is re-namespaced on the way back out, so a client that subscribes to ns/X ends up receiving updates for ns/Y. The original-to-upstream mapping already suggested for the unsubscribe path would cover this as well, provided it is also consumed when forwarding notifications.
2. Empty-identifier listing marker breaks the resource_pre_fetch contract (cmf.rs:201, cmf.rs:255)
Listings signal themselves with an empty name/URI. plugins/resource_filter/resource_filter.py:88-94 in mcp-context-forge denies any schemeless URI, so it would deny every resources/list and resources/templates/list once ported. prompts/list is the weaker case of the two, since there is no part-type fallback to discriminate on. Would an explicit marker be workable instead of the empty-string sentinel?
…mapping Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
|
@lucarlig @gandhipratik203 — thanks both. Fixes are in 1 & 2 (Luca) — Fail-open when a plugin removes text (
|
Pull Request
🔗 Related Issue
Closes #5258
📝 Summary
The Rust dataplane's CPEX integration only covered
tools/call. A plugin such assecrets-detection therefore protected tool traffic while prompts, resources, and
completions passed through unscanned.
This PR widens the supported hook surface from two hook names to six — adding the
prompt and resource families — and wires them into every MCP method the gateway
routes.
prompts/getresources/readresources/subscriberesources/unsubscribecompletion/completeprompts/listresources/listresources/templates/listOrdering. Routed paths run hooks after backend routing, so plugins see the
backend-local identifier and the backend name separately and never need to know the
gateway's namespace scheme. Fan-out paths run the pre hook once before fan-out (a
denial costs zero backend traffic) and the post hook once on the merged, namespaced
result (so plugins see exactly what the client will receive, and cannot break the
routing contract by mutating pre-merge).
Scope note.
resources/readandresources/listare not in the issue's pathlist. See Notes below for why they are included.
Existing tool hook behavior is unchanged.
📏 Reviewability
triage— #5258 is currently labeledtriage; needs triage removed before merge🏷️ Type of Change
No dependencies added or changed.
🧪 Verification
This repo has no
make lint/make test/make coveragetargets — its Makefilecovers Docker only. The commands below are the ones CI runs (
.github/workflows/ci.yml),executed locally on
rustc 1.97.1.cargo fmt --all --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo test --locked --workspacecargo build --locked --workspacecargo bench --locked --workspace --no-runcargo shear --check-test-targets --deny-warnings --lockedcargo deny check advisories bans licensesTest count: 76 → 127 (+51).
contextforge-gateway-rs-cpexunittests/gateway_prompt_plugins.rsprompts/get,prompts/listtests/gateway_resource_plugins.rsresources/read,resources/list,resources/templates/list, subscribe/unsubscribetests/gateway_completion_plugins.rscompletion/complete, both reference kindsEvery path is covered for: hook ordering, pre-deny (asserting the backend was never
called), post-deny (asserting the backend did run), mutation where supported,
structural-rejection where not, and pre→post correlation-id linkage.
Three isolation tests back the "tool behavior unchanged" criterion by asserting each
hook family runs only for its own paths — e.g. a resource-only plugin config records
zero invocations across a tool call and a prompt listing.
✅ Checklist
cargo fmt --all(the template'smake black isort pre-commitis Python tooling; not applicable here)docs/book/src/plugins-and-policy.mdrewritten;docs/book/src/mcp-method-reference.mdupdated per AGENTS.md's "update the matching book page in the same change"📓 Notes
Why
resources/readandresources/listare includedresources/readreturns actual resource contents: config files, database rows,documents. Shipping resource hooks that cover template listings but not
resource reads would leave partial coverage that looks complete — an operator enabling
a redaction plugin would reasonably assume resource reads were scanned.
It is also the cleanest CMF mapping in the change:
TextResourceContents { uri, mime_type, text }→Resource { uri, mime_type, content }, essentially field-for-field.resources/listis included for symmetry with the template listing at near-zero cost;adding it let both listings share one pre/post implementation, so it removed
duplication rather than adding it.