Skip to content

Add secure prompts and resources - #266

Open
max-peroch wants to merge 1 commit into
softwaremill:masterfrom
max-peroch:secured-prompts-and-resources
Open

max-peroch wants to merge 1 commit into
softwaremill:masterfrom
max-peroch:secured-prompts-and-resources

Conversation

@max-peroch

Copy link
Copy Markdown
Contributor

Give the serverSecurityLogic principal to prompt, resource, and template logic.

Closes #263

@kubinio123 kubinio123 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(priv

Every 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.resourceTemplates

while 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].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security logic result is unavailable to prompts and resources forcing a second header read

2 participants