fix: resolve extract's sampling session from the calling context instead of server.sessions[0] - #168
Open
karaposu wants to merge 1 commit into
Open
Conversation
The extract tool borrows a language model via MCP sampling and chose the
client with server.sessions[0] -- the first-connected session, unrelated
to the one that called the tool. Under stdio (one client per process)
that happens to be correct; under any multi-client transport (the hosted
HTTP deployment) it sends one user's request -- carrying their full
scraped page -- to a different user's client, bills that user, and
returns an answer from the wrong model. It was a latent cross-tenant
data-disclosure path.
Resolution is now deliberate, in a pure leaf module
(sampling_session.js, importing nothing from server.js so it is
testable):
- ctx.sessionId present -> return the session whose .sessionId matches;
if none matches, throw -- never fall back to [0].
- no id, exactly one session -> that one (the stdio case, unchanged).
- no id, several sessions -> throw. Refuse to guess rather than answer
the wrong user. This is the only behaviour change for a running
multi-session server: from "silently wrong" to "safely refuses."
Verified against fastmcp 3.33.0's runtime source (not just its types):
the tool context is a plain object literal, so ctx.sessionId survives
tool_fn's {...ctx} spread; and ctx.sessionId and FastMCPSession.sessionId
read the same private field, so the match finds the caller on HTTP.
requestSampling exists only on the session, and ctx.session is the auth
payload (not the session), which is why the fix resolves from
server.sessions rather than ctx.
Also, resolution moved to the top of execute, before the scrape, with a
clientCapabilities.sampling guard: a caller we can't route to, or that
can't sample, now fails for free instead of after a billed Web Unlocker
call.
Packaging: sampling_session.js added to package.json files[] (an
allowlist -- omission would publish a server.js importing a missing
module). A new test asserts every local import in the entry modules is
allowlisted, so a forgotten entry fails CI instead of shipping a broken
release.
Tests: resolver matrix (routes by id, refuses when ambiguous, never
[0]) + the files[] consistency guard. Suite 23/23.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
extracttool needs an MCP sampling session to run its inference and picks it with:"First connected client" has no relationship to "the client that called the tool." On single-client stdio the two coincide, so nothing is visible today — but on any multi-session deployment (hosted endpoint, shared Docker container) a request from user A is sent to user B's client: B's model receives the full scraped page content A requested (cross-tenant disclosure), B's provider is billed for A's inference, and A gets an answer generated under B's model and settings.
Fix
Resolve the sampling session from the calling context instead of guessing:
ctx.sessionIdagainstserver.sessions(fastmcp'sContextcarries the id of the session that made the call);Resolution happens before the scrape, so an unresolvable session fails fast instead of wasting a paid fetch, and a client that doesn't advertise
samplingcapability gets an explicit error instead of a protocol failure.The resolver lives in a new leaf module
sampling_session.js(added topackage.jsonfiles[]) so it is unit-testable without booting the server.Tests
test/sampling-session.test.js— the resolver matrix: match-by-id, single-session fallback, no-sessions, id-mismatch, multi-session-refusal.test/package-files.test.js— asserts every locally imported module of the entrypoints is listed infiles[], so a future leaf module can't be forgotten from the npm package.Notes
Single-client stdio behaviour is unchanged. Independent of #163 (no shared commits);
package.jsonfiles[]may need a one-line rebase depending on merge order with sibling PRs.