From 247beb4ecfca9e5cd9a33414614841ffe79a8d1d Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 4 Sep 2026 15:22:06 +0300 Subject: [PATCH] Authorize MCP Skills extension requests --- docs/authz.md | 40 +++- pkg/authz/authorizers/cedar/core.go | 71 +++++-- pkg/authz/authorizers/core.go | 5 +- pkg/authz/authorizers/http/porc_test.go | 12 ++ pkg/authz/middleware.go | 72 ++++++- pkg/authz/middleware_test.go | 24 ++- pkg/authz/response_filter.go | 78 ++++++- pkg/authz/skills_direct_proxy_test.go | 259 ++++++++++++++++++++++++ pkg/mcp/parser.go | 2 + pkg/mcp/parser_test.go | 21 ++ 10 files changed, 553 insertions(+), 31 deletions(-) create mode 100644 pkg/authz/skills_direct_proxy_test.go diff --git a/docs/authz.md b/docs/authz.md index 45a2bba0ad..78100c9f7d 100644 --- a/docs/authz.md +++ b/docs/authz.md @@ -118,7 +118,8 @@ Create a configuration file (JSON or YAML) with the following structure: "policies": [ "permit(principal, action == Action::\"call_tool\", resource == Tool::\"weather\");", "permit(principal, action == Action::\"get_prompt\", resource == Prompt::\"greeting\");", - "permit(principal, action == Action::\"read_resource\", resource == Resource::\"data\");" + "permit(principal, action == Action::\"read_resource\", resource == Resource::\"data\");", + "permit(principal, action == Action::\"get_skill\", resource == Skill::\"mcp://example/skill\");" ], "entities_json": "[]" } @@ -135,6 +136,7 @@ cedar: - 'permit(principal, action == Action::"call_tool", resource == Tool::"weather");' - 'permit(principal, action == Action::"get_prompt", resource == Prompt::"greeting");' - 'permit(principal, action == Action::"read_resource", resource == Resource::"data");' + - 'permit(principal, action == Action::"get_skill", resource == Skill::"mcp://example/skill");' entities_json: "[]" ``` @@ -181,10 +183,11 @@ In the context of MCP servers, the following entities are used: - `Action::"call_tool"`: Call a tool - `Action::"get_prompt"`: Get a prompt - `Action::"read_resource"`: Read a resource + - `Action::"get_skill"`: Get a skill - Note: List operations (`tools/list`, `prompts/list`, `resources/list`) are always + Note: List operations (`tools/list`, `prompts/list`, `resources/list`, `skills/list`) are always allowed but the response is filtered based on the corresponding call/get/read policies. - Define policies for the specific operations (call_tool, get_prompt, read_resource) + Define policies for the specific operations (call_tool, get_prompt, read_resource, get_skill) and the list responses will automatically show only the items the user is authorized to access. - **Resource**: The object being accessed. @@ -194,14 +197,35 @@ In the context of MCP servers, the following entities are used: - `Prompt::"greeting"`: The greeting prompt - `Resource::"data"`: A short resource name - `Resource::"file:///etc/passwd"`: An MCP resource URI (exact URI is the Cedar entity ID) + - `Skill::"mcp://example/skill"`: An MCP skill URI (exact URI is the Cedar entity ID) - `FeatureType::"tool"`: The tool feature type (used for list operations) For `read_resource`, the Cedar entity ID is the **exact resource URI** (for example `Resource::"file:///ok"` or `Resource::"mcp://srv/config:admin"`). Do not rewrite characters such as `/`, `:`, or `?` into underscores; policies must name the URI as - the client and server see it. In Cedar source the ID is a double-quoted string - literal, so almost every URI character is ordinary, but `"` and `\` must be escaped - (for example `Resource::"file://C:\\share\\data"`). + the client and server see it. Skill URI values follow these same Cedar string-literal + escaping rules. In Cedar source the ID is a double-quoted string literal, so almost + every URI character is ordinary, but `"` and `\` must be escaped (for example + `Resource::"file://C:\\share\\data"`). + +#### Skills (SEP-2640 direct proxy) + +Direct proxies authorize `skills/get` with `Action::"get_skill"` on an exact +`Skill::""` entity. The URI is passed through verbatim: it is not +canonicalized and no scheme or suffix is validated. A missing, empty, or non-string +`params.uri` is denied before the authorizer or backend is called. Requests with duplicate +immediate `params.uri` members are likewise denied so the proxy and backend cannot +interpret an ambiguous URI differently. + +`skills/list` itself has no separate list policy. It is forwarded and each entry is +shown only when its exact string `uri` is permitted by `get_skill`; all other entries, +including their manifests, are removed. Skill permission is independent of +`read_resource` permission. + +This support is only for the direct proxy. Skill capability negotiation, including +initialize extension maps, is passed through unchanged; the proxy neither fabricates +nor rewrites capabilities. Directory reads and all other SEP-2640 operations are not +supported by this authorization layer. #### Example policies @@ -243,13 +267,15 @@ permit( ##### List operations -List operations (`tools/list`, `prompts/list`, `resources/list`) do not require explicit policies. +List operations (`tools/list`, `prompts/list`, `resources/list`, `skills/list`) do not require explicit policies. They are always allowed but the response is automatically filtered based on the user's permissions for the corresponding operations: - `tools/list` shows only tools the user can call (based on `call_tool` policies) - `prompts/list` shows only prompts the user can get (based on `get_prompt` policies) - `resources/list` shows only resources the user can read (based on `read_resource` policies) +- `skills/list` shows only skill entries the user can get (based on `get_skill` policies); entries + without exactly one non-empty string `uri` fail closed with a generic internal JSON-RPC error. For example, if you have this policy: ```plain diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index e370653628..2ba8bfbbc2 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -1115,6 +1115,31 @@ func (a *Authorizer) authorizeResourceRead( return a.IsAuthorized(principal, action, resource, contextMap, entities) } +// authorizeSkillGet authorizes a skills/get operation using its exact URI. +func (a *Authorizer) authorizeSkillGet( + clientID, skillURI string, + claimsMap map[string]interface{}, + attrsMap map[string]interface{}, + groups []string, +) (bool, error) { + principal := fmt.Sprintf("Client::%s", clientID) + attributes := mergeContexts(map[string]interface{}{ + "name": skillURI, + "uri": skillURI, + "operation": "get", + "feature": "skill", + }, attrsMap) + entities, err := a.entityFactory.CreateEntitiesForRequest( + principal, "Action::get_skill", fmt.Sprintf("Skill::%s", skillURI), + claimsMap, attributes, groups, a.serverName, + ) + if err != nil { + return false, fmt.Errorf("failed to create Cedar entities: %w", err) + } + return a.IsAuthorized(principal, "Action::get_skill", fmt.Sprintf("Skill::%s", skillURI), + mergeContexts(claimsMap, attrsMap), entities) +} + // authorizeFeatureList authorizes a list operation for a feature. // This method is used when a client tries to list available tools, prompts, or resources. // It checks if the client is authorized to list the specified feature type. @@ -1230,22 +1255,42 @@ func (a *Authorizer) AuthorizeWithJWTClaims( addMultiValuedClaimSets(processedClaims, resolvedClaims, a.multiValuedClaims) processedArgs := preprocessArguments(arguments) - // Authorize based on the feature and operation - switch { - case feature == authorizers.MCPFeatureTool && operation == authorizers.MCPOperationCall: - return a.authorizeToolCall(ctx, clientID, resourceID, processedClaims, processedArgs, groups) - - case feature == authorizers.MCPFeaturePrompt && operation == authorizers.MCPOperationGet: - return a.authorizePromptGet(clientID, resourceID, processedClaims, processedArgs, groups) - - case feature == authorizers.MCPFeatureResource && operation == authorizers.MCPOperationRead: - return a.authorizeResourceRead(clientID, resourceID, processedClaims, processedArgs, groups) - - case operation == authorizers.MCPOperationList: + // Authorize based on the feature and operation. + switch operation { + case authorizers.MCPOperationCall: + if feature == authorizers.MCPFeatureTool { + return a.authorizeToolCall(ctx, clientID, resourceID, processedClaims, processedArgs, groups) + } + case authorizers.MCPOperationGet: + return a.authorizeGet(clientID, feature, resourceID, processedClaims, processedArgs, groups) + case authorizers.MCPOperationRead: + if feature == authorizers.MCPFeatureResource { + return a.authorizeResourceRead(clientID, resourceID, processedClaims, processedArgs, groups) + } + case authorizers.MCPOperationList: return a.authorizeFeatureList(clientID, feature, processedClaims, processedArgs, groups) + } + return false, fmt.Errorf("unsupported feature/operation combination: %s/%s", feature, operation) +} +// authorizeGet dispatches get operations to their feature-specific Cedar mappings. +func (a *Authorizer) authorizeGet( + clientID string, + feature authorizers.MCPFeature, + resourceID string, + claimsMap map[string]interface{}, + attrsMap map[string]interface{}, + groups []string, +) (bool, error) { + switch feature { + case authorizers.MCPFeaturePrompt: + return a.authorizePromptGet(clientID, resourceID, claimsMap, attrsMap, groups) + case authorizers.MCPFeatureSkill: + return a.authorizeSkillGet(clientID, resourceID, claimsMap, attrsMap, groups) + case authorizers.MCPFeatureTool, authorizers.MCPFeatureResource: + fallthrough default: - return false, fmt.Errorf("unsupported feature/operation combination: %s/%s", feature, operation) + return false, fmt.Errorf("unsupported get feature: %s", feature) } } diff --git a/pkg/authz/authorizers/core.go b/pkg/authz/authorizers/core.go index 6c54fbfd45..5239d2c0ce 100644 --- a/pkg/authz/authorizers/core.go +++ b/pkg/authz/authorizers/core.go @@ -8,10 +8,11 @@ import ( ) // MCPFeature represents an MCP feature type. -// In the MCP protocol, there are three main features: +// In the MCP protocol, ToolHive authorizes tools, prompts, resources, and skills. // - Tools: Allow models to call functions in external systems // - Prompts: Provide structured templates for interacting with language models // - Resources: Share data that provides context to language models +// - Skills: Package reusable MCP capabilities type MCPFeature string const ( @@ -21,6 +22,8 @@ const ( MCPFeaturePrompt MCPFeature = "prompt" // MCPFeatureResource represents the MCP resource feature. MCPFeatureResource MCPFeature = "resource" + // MCPFeatureSkill represents the MCP skill feature. + MCPFeatureSkill MCPFeature = "skill" ) // MCPOperation represents an operation on an MCP feature. diff --git a/pkg/authz/authorizers/http/porc_test.go b/pkg/authz/authorizers/http/porc_test.go index 621bfb053d..a0e8e1da5f 100644 --- a/pkg/authz/authorizers/http/porc_test.go +++ b/pkg/authz/authorizers/http/porc_test.go @@ -62,6 +62,18 @@ func TestBuildPORC(t *testing.T) { wantOp: "mcp:resource:read", wantRes: "mrn:mcp:test:resource:file://data.json", }, + { + name: "skill get", + feature: authorizers.MCPFeatureSkill, + operation: authorizers.MCPOperationGet, + resourceID: "mcp://example/skill", + claims: map[string]interface{}{ + "sub": "user@example.com", + }, + arguments: nil, + wantOp: "mcp:skill:get", + wantRes: "mrn:mcp:test:skill:mcp://example/skill", + }, { name: "tool list", feature: authorizers.MCPFeatureTool, diff --git a/pkg/authz/middleware.go b/pkg/authz/middleware.go index 50a37a5578..f08e7079c6 100644 --- a/pkg/authz/middleware.go +++ b/pkg/authz/middleware.go @@ -7,8 +7,10 @@ package authz import ( + "bytes" "encoding/json" "fmt" + "io" "log/slog" "net/http" "strings" @@ -52,6 +54,10 @@ var MCPMethodToFeatureOperation = map[string]featureOperation{ "resources/subscribe": {Feature: authorizers.MCPFeatureResource, Operation: authorizers.MCPOperationRead}, "resources/unsubscribe": {Feature: authorizers.MCPFeatureResource, Operation: authorizers.MCPOperationRead}, + // Skill operations - list responses are filtered by get authorization. + "skills/get": {Feature: authorizers.MCPFeatureSkill, Operation: authorizers.MCPOperationGet}, + "skills/list": {Feature: authorizers.MCPFeatureSkill, Operation: authorizers.MCPOperationList}, + // Discovery and capability methods - always allowed "features/list": {Feature: "", Operation: authorizers.MCPOperationList}, // Capability discovery "roots/list": {Feature: "", Operation: ""}, // Root directory discovery @@ -140,6 +146,59 @@ func shouldSkipSubsequentAuthorization(method string) bool { return false } +// invalidSkillGet reports whether a skills/get request lacks exactly one valid URI. +func invalidSkillGet(featureOp featureOperation, resourceID string, params json.RawMessage) bool { + if featureOp.Feature != authorizers.MCPFeatureSkill || featureOp.Operation != authorizers.MCPOperationGet { + return false + } + return resourceID == "" || duplicateSkillURI(params) +} + +// hasDuplicateURI reports whether an immediate JSON object has more than one uri member. +// It uses a token decoder because unmarshalling into a map would silently retain only +// the final duplicate member. +func hasDuplicateURI(raw json.RawMessage) (bool, error) { + dec := json.NewDecoder(bytes.NewReader(raw)) + token, err := dec.Token() + if err != nil { + return false, err + } + if delimiter, ok := token.(json.Delim); !ok || delimiter != '{' { + return false, nil + } + + seen := false + for dec.More() { + key, err := dec.Token() + if err != nil { + return false, err + } + if key == "uri" { + if seen { + return true, nil + } + seen = true + } + var value json.RawMessage + if err := dec.Decode(&value); err != nil { + return false, err + } + } + if _, err := dec.Token(); err != nil { + return false, err + } + var trailing json.RawMessage + if err := dec.Decode(&trailing); err != io.EOF { + return false, err + } + return false, nil +} + +func duplicateSkillURI(raw json.RawMessage) bool { + duplicate, err := hasDuplicateURI(raw) + return err != nil || duplicate +} + // handleUnauthorized handles unauthorized requests. The client always sees the fixed // "Unauthorized" message -- err (an authorizer failure) can carry policy detail that // security.md forbids returning to callers, so it is logged server-side instead. @@ -179,9 +238,10 @@ func rejectInvalidMCPRequest(w http.ResponseWriter) { // This middleware extracts the MCP message from the request, determines the feature, // operation, and resource ID, and authorizes the request using the configured authorizer. // -// For list operations (tools/list, prompts/list, resources/list), the middleware allows +// For list operations (tools/list, prompts/list, resources/list, skills/list), the middleware allows // the request to proceed but intercepts the response to filter out items that the user -// is not authorized to access based on the corresponding call/get/read policies. +// is not authorized to access based on the corresponding call/get/read policy. In +// particular, skills/list entries are filtered individually by get_skill authorization. // // An in-memory annotation cache is maintained per middleware instance. When a // tools/list response passes through, tool annotations are captured. When a @@ -255,6 +315,14 @@ func Middleware(a authorizers.Authorizer, next http.Handler, passThroughTools ma return } + // skills/get identifies its target only by params.uri. An absent, empty, + // or non-string URI must never reach an authorizer, whose policy might + // otherwise accidentally permit an empty identifier. + if invalidSkillGet(featureOp, parsedRequest.ResourceID, parsedRequest.Params) { + handleUnauthorized(w, parsedRequest.ID, nil) + return + } + // Handle list operations differently - allow them through but filter the response if featureOp.Operation == authorizers.MCPOperationList { diff --git a/pkg/authz/middleware_test.go b/pkg/authz/middleware_test.go index a38eb82bad..08c24fd221 100644 --- a/pkg/authz/middleware_test.go +++ b/pkg/authz/middleware_test.go @@ -33,21 +33,31 @@ import ( // stubAuthorizer is a minimal Authorizer for unit tests, avoiding Cedar setup overhead. type stubAuthorizer struct { - allowed bool - err error - lastToolID string - lastCtx context.Context + allowed bool + err error + lastID string + lastFeature authorizers.MCPFeature + lastOperation authorizers.MCPOperation + lastCtx context.Context + calls int + authorize func(authorizers.MCPFeature, authorizers.MCPOperation, string) (bool, error) } func (s *stubAuthorizer) AuthorizeWithJWTClaims( ctx context.Context, - _ authorizers.MCPFeature, - _ authorizers.MCPOperation, + feature authorizers.MCPFeature, + operation authorizers.MCPOperation, resourceID string, _ map[string]interface{}, ) (bool, error) { - s.lastToolID = resourceID + s.lastID = resourceID + s.lastFeature = feature + s.lastOperation = operation s.lastCtx = ctx + s.calls++ + if s.authorize != nil { + return s.authorize(feature, operation, resourceID) + } return s.allowed, s.err } diff --git a/pkg/authz/response_filter.go b/pkg/authz/response_filter.go index 16c439cf6e..be427f1a59 100644 --- a/pkg/authz/response_filter.go +++ b/pkg/authz/response_filter.go @@ -521,12 +521,13 @@ func (rfw *ResponseFilteringWriter) filterSSEEventData(data []byte) (replacement } // requiresResponseFiltering reports whether the method needs response filtering. -// This covers the three MCP list operations and the optimizer's find_tool call, +// This covers the MCP list operations and the optimizer's find_tool call, // whose response embeds a filtered tool list inside a CallToolResult. func requiresResponseFiltering(method string) bool { return method == string(mcp.MethodToolsList) || method == string(mcp.MethodPromptsList) || method == string(mcp.MethodResourcesList) || + method == "skills/list" || method == optimizerdec.FindToolName } @@ -656,6 +657,8 @@ func (rfw *ResponseFilteringWriter) filterListResponse(response *jsonrpc2.Respon return rfw.filterPromptsResponse(response) case string(mcp.MethodResourcesList): return rfw.filterResourcesResponse(response) + case "skills/list": + return rfw.filterSkillsResponse(response) case optimizerdec.FindToolName: return rfw.filterFindToolResponse(response) default: @@ -846,6 +849,79 @@ func (rfw *ResponseFilteringWriter) filterResourcesResponse(response *jsonrpc2.R return filteredResponse, nil } +// filterSkillsResponse filters skills/list entries by get_skill authorization. +// It retains each permitted entry as its original JSON value and preserves all +// result-level fields, including SEP extension fields the proxy does not own. +func (rfw *ResponseFilteringWriter) filterSkillsResponse(response *jsonrpc2.Response) (*jsonrpc2.Response, error) { + var result map[string]json.RawMessage + if err := json.Unmarshal(response.Result, &result); err != nil { + return nil, fmt.Errorf("invalid skills/list result: %w", err) + } + + rawSkills, ok := result["skills"] + if !ok { + return nil, errors.New("skills/list result is missing skills") + } + if !bytes.HasPrefix(bytes.TrimSpace(rawSkills), []byte("[")) { + return nil, errors.New("skills/list skills is not an array") + } + var entries []json.RawMessage + if err := json.Unmarshal(rawSkills, &entries); err != nil { + return nil, fmt.Errorf("invalid skills/list skills: %w", err) + } + + permitted := make([]json.RawMessage, 0, len(entries)) + for _, entry := range entries { + allowed, err := rfw.isSkillEntryAllowed(entry) + if err != nil { + return nil, err + } + if allowed { + permitted = append(permitted, entry) + } + } + + filteredSkills, err := json.Marshal(permitted) + if err != nil { + return nil, fmt.Errorf("encode filtered skills: %w", err) + } + result["skills"] = filteredSkills + filteredResult, err := json.Marshal(result) + if err != nil { + return nil, fmt.Errorf("encode filtered skills/list result: %w", err) + } + return &jsonrpc2.Response{ID: response.ID, Result: json.RawMessage(filteredResult)}, nil +} + +// isSkillEntryAllowed validates one opaque skills/list entry and checks its get_skill policy. +func (rfw *ResponseFilteringWriter) isSkillEntryAllowed(entry json.RawMessage) (bool, error) { + duplicateURI, err := hasDuplicateURI(entry) + if err != nil || duplicateURI { + return false, errors.New("skill entry has a duplicate uri") + } + var skill map[string]json.RawMessage + if err := json.Unmarshal(entry, &skill); err != nil { + return false, fmt.Errorf("invalid skill entry: %w", err) + } + rawURI, ok := skill["uri"] + if !ok { + return false, errors.New("skill entry is missing uri") + } + var uri string + if err := json.Unmarshal(rawURI, &uri); err != nil || uri == "" { + return false, errors.New("skill entry has an invalid uri") + } + + authorized, err := rfw.authorizer.AuthorizeWithJWTClaims( + rfw.request.Context(), authorizers.MCPFeatureSkill, authorizers.MCPOperationGet, uri, nil, + ) + if err != nil { + slog.Warn("authorization check failed for skill, skipping", "uri", uri, "error", err) + return false, nil + } + return authorized, nil +} + // errorResponseBody logs the full filtering error server-side and encodes a // JSON-RPC error response carrying a deliberately generic client-visible // message: err can originate in policy evaluation and name tools or resources, diff --git a/pkg/authz/skills_direct_proxy_test.go b/pkg/authz/skills_direct_proxy_test.go new file mode 100644 index 0000000000..842ae51a41 --- /dev/null +++ b/pkg/authz/skills_direct_proxy_test.go @@ -0,0 +1,259 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package authz + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/exp/jsonrpc2" + + "github.com/stacklok/toolhive/pkg/auth" + "github.com/stacklok/toolhive/pkg/authz/authorizers" + "github.com/stacklok/toolhive/pkg/authz/authorizers/cedar" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" +) + +func skillRequest(t *testing.T, method, params string) *http.Request { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/messages", bytes.NewBufferString( + `{"jsonrpc":"2.0","id":1,"method":"`+method+`","params":`+params+`}`, + )) + req.Header.Set("Content-Type", "application/json") + return req.WithContext(auth.WithIdentity(context.Background(), &auth.Identity{PrincipalInfo: auth.PrincipalInfo{ + Subject: "user", Claims: map[string]interface{}{"sub": "user"}, + }})) +} + +func TestMiddlewareSkillsGet(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + policy string + allowed bool + }{ + { + name: "allowed exact URI reaches handler", + policy: `permit(principal, action == Action::"get_skill", resource == Skill::"mcp://example/allowed");`, + allowed: true, + }, + { + name: "denied URI does not reach handler", + policy: `permit(principal, action == Action::"get_skill", resource == Skill::"mcp://example/other");`, + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{Policies: []string{tt.policy}, EntitiesJSON: `[]`}, "") + require.NoError(t, err) + + handlerCalled := false + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { handlerCalled = true }) + rr := httptest.NewRecorder() + mcpparser.ParsingMiddleware(Middleware(authorizer, next, nil)).ServeHTTP( + rr, skillRequest(t, "skills/get", `{"uri":"mcp://example/allowed"}`), + ) + + assert.Equal(t, tt.allowed, handlerCalled) + if !tt.allowed { + assert.Equal(t, http.StatusForbidden, rr.Code) + } + }) + } +} + +func TestMiddlewareDeniesSkillsGetWithDuplicateURI(t *testing.T) { + t.Parallel() + + for _, params := range []string{ + `{"uri":"mcp://example/allowed","uri":"mcp://example/denied"}`, + `{"uri":"mcp://example/allowed","name":"skill","uri":"mcp://example/allowed"}`, + } { + authorizer := &stubAuthorizer{allowed: true} + handlerCalled := false + rr := httptest.NewRecorder() + mcpparser.ParsingMiddleware(Middleware(authorizer, http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + handlerCalled = true + }), nil)).ServeHTTP(rr, skillRequest(t, "skills/get", params)) + + assert.Equal(t, http.StatusForbidden, rr.Code) + assert.False(t, handlerCalled) + assert.Zero(t, authorizer.calls) + } +} + +func TestMiddlewareDeniesSkillsGetWithoutURI(t *testing.T) { + t.Parallel() + + for _, params := range []string{`{}`, `{"uri":""}`, `{"uri":42}`} { + authorizer := &stubAuthorizer{allowed: true} + handlerCalled := false + rr := httptest.NewRecorder() + mcpparser.ParsingMiddleware(Middleware(authorizer, http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + handlerCalled = true + }), nil)).ServeHTTP(rr, skillRequest(t, "skills/get", params)) + + assert.Equal(t, http.StatusForbidden, rr.Code) + assert.False(t, handlerCalled) + assert.Zero(t, authorizer.calls) + } +} + +func TestMiddlewareSkillsListResponseFilter(t *testing.T) { + t.Parallel() + + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ + Policies: []string{`permit(principal, action == Action::"get_skill", resource == Skill::"mcp://example/allowed");`}, + EntitiesJSON: `[]`, + }, "") + require.NoError(t, err) + + response, err := jsonrpc2.EncodeMessage(&jsonrpc2.Response{ID: jsonrpc2.Int64ID(1), Result: json.RawMessage(`{ + "skills":[ + {"uri":"mcp://example/allowed","name":"allowed","manifest":{"keep":true}}, + {"uri":"mcp://example/denied","name":"denied","manifest":{"secret":true}} + ],"nextCursor":"next","extension":{"preserved":true}}`)}) + require.NoError(t, err) + + next := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, err := w.Write(response) + require.NoError(t, err) + }) + rr := httptest.NewRecorder() + mcpparser.ParsingMiddleware(Middleware(authorizer, next, nil)).ServeHTTP(rr, skillRequest(t, "skills/list", `{}`)) + + message, err := jsonrpc2.DecodeMessage(rr.Body.Bytes()) + require.NoError(t, err) + filtered := message.(*jsonrpc2.Response) + require.Nil(t, filtered.Error) + assert.JSONEq(t, `{ + "skills":[{"uri":"mcp://example/allowed","name":"allowed","manifest":{"keep":true}}], + "nextCursor":"next","extension":{"preserved":true}}`, string(filtered.Result)) +} + +func TestSkillsListResponseFilterFailsClosed(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + body string + }{ + {name: "malformed entry", body: `{"skills":[{"name":"missing-uri"}]}`}, + {name: "duplicate URI", body: `{"skills":[{"uri":"mcp://example/allowed","uri":"mcp://example/denied"}]}`}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + rr.Header().Set("Content-Type", "application/json") + writer := NewResponseFilteringWriter(rr, &stubAuthorizer{allowed: true}, httptest.NewRequest(http.MethodPost, "/messages", nil), "skills/list", nil, nil) + body, err := jsonrpc2.EncodeMessage(&jsonrpc2.Response{ID: jsonrpc2.Int64ID(1), Result: json.RawMessage(tt.body)}) + require.NoError(t, err) + _, err = writer.Write(body) + require.NoError(t, err) + require.NoError(t, writer.FlushAndFilter()) + + assert.Equal(t, http.StatusInternalServerError, rr.Code) + assert.Contains(t, rr.Body.String(), "internal error") + assert.NotContains(t, rr.Body.String(), "missing-uri") + assert.NotContains(t, rr.Body.String(), "mcp://example/") + }) + } +} + +func TestSkillsListResponseFilterSkipsAuthorizerErrors(t *testing.T) { + t.Parallel() + + authorizer := &stubAuthorizer{authorize: func(feature authorizers.MCPFeature, operation authorizers.MCPOperation, id string) (bool, error) { + if id == "mcp://example/error" { + return false, errors.New("authorizer unavailable") + } + return feature == authorizers.MCPFeatureSkill && operation == authorizers.MCPOperationGet && id == "mcp://example/allowed", nil + }} + rr := httptest.NewRecorder() + rr.Header().Set("Content-Type", "application/json") + writer := NewResponseFilteringWriter(rr, authorizer, httptest.NewRequest(http.MethodPost, "/messages", nil), "skills/list", nil, nil) + body, err := jsonrpc2.EncodeMessage(&jsonrpc2.Response{ID: jsonrpc2.Int64ID(1), Result: json.RawMessage(`{"skills":[ + {"uri":"mcp://example/allowed"},{"uri":"mcp://example/error"}]}`)}) + require.NoError(t, err) + _, err = writer.Write(body) + require.NoError(t, err) + require.NoError(t, writer.FlushAndFilter()) + + assert.Contains(t, rr.Body.String(), "mcp://example/allowed") + assert.NotContains(t, rr.Body.String(), "mcp://example/error") + assert.Equal(t, 2, authorizer.calls) + assert.Equal(t, authorizers.MCPFeatureSkill, authorizer.lastFeature) + assert.Equal(t, authorizers.MCPOperationGet, authorizer.lastOperation) +} + +func TestSkillsListResponseFilterSSE(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + authorizer authorizers.Authorizer + result string + contains string + omits string + internal bool + }{ + { + name: "allowed entry", + authorizer: mustSkillAuthorizer(t, + `permit(principal, action == Action::"get_skill", resource == Skill::"mcp://example/allowed");`), + result: `{"skills":[{"uri":"mcp://example/allowed"},{"uri":"mcp://example/denied"}]}`, + contains: `mcp://example/allowed`, + omits: `mcp://example/denied`, + }, + { + name: "denied entries", + authorizer: mustSkillAuthorizer(t, `permit(principal, action == Action::"get_skill", resource == Skill::"mcp://example/other");`), + result: `{"skills":[{"uri":"mcp://example/denied"}]}`, + omits: `mcp://example/denied`, + }, + { + name: "malformed entry fails closed", + authorizer: &stubAuthorizer{allowed: true}, + result: `{"skills":[{"name":"missing-uri"}]}`, + omits: `missing-uri`, + internal: true, + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + payload, err := jsonrpc2.EncodeMessage(&jsonrpc2.Response{ID: jsonrpc2.Int64ID(1), Result: json.RawMessage(tt.result)}) + require.NoError(t, err) + rr := httptest.NewRecorder() + rr.Header().Set("Content-Type", "text/event-stream") + writer := NewResponseFilteringWriter(rr, tt.authorizer, skillRequest(t, "skills/list", `{}`), "skills/list", nil, nil) + _, err = writer.Write(append(append([]byte("data: "), payload...), []byte("\n\n")...)) + require.NoError(t, err) + require.NoError(t, writer.FlushAndFilter()) + + if tt.contains != "" { + assert.Contains(t, rr.Body.String(), tt.contains) + } + assert.NotContains(t, rr.Body.String(), tt.omits) + if tt.internal { + assert.Contains(t, rr.Body.String(), "internal error") + } + }) + } +} + +func mustSkillAuthorizer(t *testing.T, policy string) authorizers.Authorizer { + t.Helper() + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{Policies: []string{policy}, EntitiesJSON: `[]`}, "") + require.NoError(t, err) + return authorizer +} diff --git a/pkg/mcp/parser.go b/pkg/mcp/parser.go index 2dd3d4f378..2a63842330 100644 --- a/pkg/mcp/parser.go +++ b/pkg/mcp/parser.go @@ -346,7 +346,9 @@ var methodHandlers = map[string]methodHandler{ "tools/call": handleNamedResourceMethod, "prompts/get": handleNamedResourceMethod, "resources/read": handleResourceReadMethod, + "skills/get": handleResourceReadMethod, "resources/list": handleListMethod, + "skills/list": handleListMethod, "tools/list": handleListMethod, "prompts/list": handleListMethod, "notifications/message": handleNotificationMethod, diff --git a/pkg/mcp/parser_test.go b/pkg/mcp/parser_test.go index e6c157e303..c197ebec8f 100644 --- a/pkg/mcp/parser_test.go +++ b/pkg/mcp/parser_test.go @@ -400,6 +400,27 @@ func TestExtractResourceAndArguments(t *testing.T) { "capabilities": map[string]interface{}{}, }, }, + { + name: "skills/get with exact URI", + method: "skills/get", + params: `{"uri":"mcp://example/skill?version=1"}`, + expectedResourceID: "mcp://example/skill?version=1", + expectedArguments: nil, + }, + { + name: "skills/get with non-string URI", + method: "skills/get", + params: `{"uri":42}`, + expectedResourceID: "", + expectedArguments: nil, + }, + { + name: "skills/list with cursor", + method: "skills/list", + params: `{"cursor":"next-page"}`, + expectedResourceID: "next-page", + expectedArguments: nil, + }, { name: "resources/read with URI", method: "resources/read",