Add secure prompts and resources - #266
max-peroch wants to merge 1 commit into
Conversation
kubinio123
left a comment
There was a problem hiding this comment.
I think 1 is a good idea.
1. Prompts and resources should reuse the context parameter
ServerTool already solves this with one type and a contravariant context:
case class ServerTool[I, O, F[_], -C <: ServerContext[F]](..., logic: (I, C, Seq[Header]) => F[ToolResult[O]])That is why SecuredMcpServer.tools is one line (McpServer.scala:265) and why addTool needs no overload at all.
This PR goes the other way for prompts and resources: three new case classes, twelve add* overloads across the two secured server classes, six @targetName annotations to work around varargs erasure, five dispatch methods on McpServerDef (two of them carrying an @unused context, the first @unused in the repo), and five pure-delegation overrides on SecuredStreamingMcpServer.
Suggestion:
case class ServerPrompt[F[_], -C <: ServerContext[F]](
definition: Prompt,
logic: (Map[String, String], C, Seq[Header]) => F[GetPromptResult]
)Same shape for ServerResource and ServerResourceTemplate. securedServerLogic then returns ServerPrompt[F, SecuredServerContext[F, P]], McpServerDef.prompts becomes List[ServerPrompt[F, C]], and SecuredMcpServer.prompts = server.prompts ++ securedPrompts type-checks by contravariance. The overloads, the @targetNames, the *Definitions and invoke* methods, the secured lookup maps and both @unused annotations all go away. That is roughly 170 of the 200 added non-test lines. It breaks ServerPrompt[F] and ServerResource[F] at source level, which is fine pre-1.0.
2. An unsecured handler silently shadows a secured one with the same name or URI
McpServer.scala:282-285 and :298-306 check the unsecured registry first. Tools do the opposite: McpHandler.scala:30 builds toMap over server.tools ++ securedTools, so there the secured entry wins.
Concrete case. Someone hardens an existing public resource and forgets to drop the old registration:
McpServer[F]()
.addResource(resource("test://profile").handle(() => Right(publicData)))
.serverSecurityLogicPure(...)(...)
.addResource(resource("test://profile").handleSecured[User](u => Right(privEvery resources/read runs the unsecured handler. The principal-aware logic g warns at build time or at startup. The failure mode is "quieter and less
restrictive than intended", which is the wrong direction for a security featu
resources/list also returns test://profile twice, because promptDefinitions concatenate without deduplicating (:273, :287, :290). The MCP spec
treats prompt name and resource uri as identifiers, so the duplicate is as own.
Either reject collisions at registration, or let the secured entry win and deFix 1 collapses this into a single list with a single rule.
3. Public accessors under-report on a secured server
McpServer.scala:258-260 returns only the unsecured entries:
def prompts: List[ServerPrompt[F]] = server.prompts
def resources: List[ServerResource[F]] = server.resources
def resourceTemplates: List[ServerResourceTemplate[F]] = server.resourceTemplateswhile tools at :265 returns the merged list. These are public members of a public trait, so anyone reading securedServer.resources gets a partial answer, and the complete views are private[server]. This is the same trap that forced McpHandler to switch to the new *Definitions accessors. Fix 1 makes the accessors complete. Otherwise narrow them to
private[server].
Give the serverSecurityLogic principal to prompt, resource, and template logic.
Closes #263