fix(server): honour enabled on resource templates - #2694
Open
sainikhiljuluri wants to merge 2 commits into
Open
Conversation
registerResource returns a handle with enable(), disable() and enabled for every primitive, but nothing ever read the flag for resource templates. A disabled template stayed in resources/list and resources/templates/list, was still served by resources/read, and still answered completion/complete, while disable() fired resources/list_changed as if the list had changed. Static resources registered through the same call were already guarded, so the two behaved differently three lines apart in the same handler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mcpserver:handle:enable-disable requirement is written generically but was only exercised with a RegisteredTool, which is why the resource-template gap went unnoticed. Adds a second body for a template, and corrects the changeset to say every registration handle rather than registerResource. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 543361c The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
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
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.
Fixes #2693
Problem
enabledis written for resource templates but never read, sodisable()is a no-op. A static resource registered through the sameregisterResourcecall is guarded correctly:disable()also firesnotifications/resources/list_changed, so the SDK reports a list change that did not happen.Fix
Four
enabledchecks inpackages/server/src/server/mcp.ts, each matching the guard its sibling primitive already has:resources/listtemplate loopresources/templates/listresources/readtemplate matchhandleResourceCompletionif (!prompt.enabled)in the prompt pathKept as inline checks rather than a shared helper, since the eight existing
enabledchecks are all inline and a helper used only by templates would be the odd one out.One judgement call worth flagging: in
resources/readthe guard throws insideif (variables), so a disabled template shadows a later enabled template matching the same URI rather than falling through. That matches what static resources already do today — a disabled static resource errors instead of falling through to a template that would match — and fail-closed seems right for an API whose purpose is withdrawing access. Happy to switch to skip-and-continue if you prefer. A test pins the current choice.Tests
Six unit tests in
packages/server/test/server/resourceTemplateEnabled.test.ts, one per guard plus the shadowing case and anenable()restore.Reverting only
mcp.tsfails five of the six, each on the bug itself:The sixth (
enable()restores) passes either way by design — it guards against over-correcting.I also added a resource-template body to
mcpserver:handle:enable-disableintest/e2e/scenarios/dynamic.test.ts. The requirement is written generically but was only exercised with aRegisteredTool, which is why this gap survived. All eight cells fail without the fix.packages/server43 files / 481 tests,pnpm -r --no-bail testgreen across every package, typecheck and lint clean.test/e2e'sprotocol:timeout:max-totalfails ~2 runs in 3 on unpatchedmain— pre-existing flake, unrelated.Notes
enableddefaults totrueand onlyupdate()can clear it, so only code that explicitly calleddisable()is affected.v1.xhas the same omission. Happy to open a backport if useful — not included here.packages/server-legacyhas no template registry, so nothing to do there.Per CONTRIBUTING this is a straightforward bug fix with tests, so I filed the report and the fix together rather than waiting. Glad to close either if you would rather discuss first.
AI assistance was used to investigate and write this change.