diff --git a/server/router/api/v1/v1.go b/server/router/api/v1/v1.go index 378b41e8..7bf98d87 100644 --- a/server/router/api/v1/v1.go +++ b/server/router/api/v1/v1.go @@ -17,7 +17,10 @@ import ( "github.com/usememos/memos/store" ) -const maxAPIRequestBytes = 256 << 20 +// MaxAPIRequestBytes caps the size of a request body accepted by the API. The +// in-process MCP endpoint forwards every tool call through these same routes, so +// it derives its own limit from this constant to keep the two gates in lockstep. +const MaxAPIRequestBytes = 256 << 20 type APIV1Service struct { v1pb.UnimplementedInstanceServiceServer @@ -123,7 +126,7 @@ func (s *APIV1Service) RegisterGateway(ctx context.Context, echoServer *echo.Ech gwGroup := echoServer.Group("") // Register SSE endpoint with same CORS as rest of /api/v1. RegisterSSERoutes(gwGroup, s.SSEHub, s.Store, s.Secret) - handler := echo.WrapHandler(http.MaxBytesHandler(gwMux, maxAPIRequestBytes)) + handler := echo.WrapHandler(http.MaxBytesHandler(gwMux, MaxAPIRequestBytes)) gwGroup.Any("/api/v1/*", handler) gwGroup.Any("/file/*", handler) @@ -138,10 +141,10 @@ func (s *APIV1Service) RegisterGateway(ctx context.Context, echoServer *echo.Ech ) connectMux := http.NewServeMux() connectHandler := NewConnectServiceHandler(s) - connectHandler.RegisterConnectHandlers(connectMux, connectInterceptors, connect.WithReadMaxBytes(maxAPIRequestBytes)) + connectHandler.RegisterConnectHandlers(connectMux, connectInterceptors, connect.WithReadMaxBytes(MaxAPIRequestBytes)) connectGroup := echoServer.Group("") - connectGroup.Any("/memos.api.v1.*", echo.WrapHandler(http.MaxBytesHandler(connectMux, maxAPIRequestBytes))) + connectGroup.Any("/memos.api.v1.*", echo.WrapHandler(http.MaxBytesHandler(connectMux, MaxAPIRequestBytes))) return nil } diff --git a/server/router/mcp/README.md b/server/router/mcp/README.md index 43f2f080..97b69520 100644 --- a/server/router/mcp/README.md +++ b/server/router/mcp/README.md @@ -49,17 +49,18 @@ fast on any inconsistency: `RegisterRoutes` binds `echoServer.Any("/mcp", ...)`. Each request: 1. `isAllowedMCPOrigin` (`origin.go`) rejects disallowed cross-origin browser requests with `403`. -2. The SDK streamable handler dispatches the MCP message. -3. On a `tools/call` request, `newMCPToolHandler` (`service.go`) decodes the JSON +2. The request body is capped at 256 MiB before the SDK reads it. +3. The SDK streamable handler dispatches the MCP message. +4. On a `tools/call` request, `newMCPToolHandler` (`service.go`) decodes the JSON arguments into a map. -4. `validateToolArguments` (`validation.go`) checks them against the tool's +5. `validateToolArguments` (`validation.go`) checks them against the tool's input schema. -5. The caller's `Authorization` header is read from the request (`request.Extra.Header` on the SDK's `*sdkmcp.CallToolRequest`). -6. `apiAdapter.execute` (`adapter.go`) builds the API request +6. The caller's `Authorization` header is read from the request (`request.Extra.Header` on the SDK's `*sdkmcp.CallToolRequest`). +7. `apiAdapter.execute` (`adapter.go`) builds the API request (`buildAPIRequest`: path-parameter substitution, query encoding, JSON body), forwards the bearer token, and runs it against the Echo server through an `httptest.ResponseRecorder`. -7. The recorder body is decoded; a non-2xx status becomes a tool error +8. The recorder body is decoded; a non-2xx status becomes a tool error (`newToolErrorResult`), otherwise the value is wrapped by `newStructuredToolResult`. @@ -86,6 +87,10 @@ use `$ref`. `openapi.go` resolves these into local definitions: parameter stays in `required`. - A request body becomes a single `body` property; a required body adds `body` to `required`. Body `$defs` are lifted to the schema's top-level `$defs`. +- Per-operation overrides relax resource-level requirements for create and + partial-update bodies and remove fields already supplied by a path binding + from `body: "*"` schemas. Memo updates may omit `updateMask` so the REST + gateway can infer it from the fields present in the request body. - The schema sets `"additionalProperties": false`. The output schema is the operation's 200 `application/json` schema. When a 200 @@ -100,6 +105,7 @@ response has no JSON body, the fallback is: - **Endpoint:** `POST /mcp` (the SDK may also use `GET`/`DELETE` on the same path for the Streamable HTTP transport). - **Transport:** Streamable HTTP, **stateless**, JSON responses. +- **Request size:** request bodies are limited to 256 MiB before SDK dispatch. - **Auth:** the caller's `Authorization: Bearer ` header is forwarded to the in-process API request. Mutating tools therefore require a valid token (personal access token or access token); public reads may work without one, @@ -172,15 +178,16 @@ by `_`. So `MemoService_ListMemos → memo_list_memos`. | DELETE | false | true | true | | other (POST, PATCH, …) | false | false | false | -A per-operation override (`idempotentOperationIDs`) then corrects cases the -method heuristic gets wrong: `MemoService_SetMemoAttachments` and -`MemoService_SetMemoRelations` are PATCH but declaratively replace the full set -on a memo, so they report `IdempotentHint: true`. +Per-operation overrides then correct cases the method heuristic gets wrong. +`MemoService_SetMemoAttachments` and `MemoService_SetMemoRelations` are PATCH +but declaratively replace the full set on a memo, so they report both +`IdempotentHint: true` and `DestructiveHint: true`. `MemoService_UpdateMemo` +also reports `DestructiveHint: true` because it can overwrite existing fields. `OpenWorldHint` is `false` for all tools. Annotations are client hints; they do not replace API authorization. -**Result shape.** Every result carries object-shaped `structuredContent` +**Result shape.** Every successful result carries object-shaped `structuredContent` (`normalizeStructuredContent` in `result.go`): - a JSON object is returned unchanged; @@ -194,8 +201,9 @@ where collection tools returned a bare array that strict MCP clients reject. ## Error handling Failures are returned as MCP tool errors (`CallToolResult` with `IsError: true` -and an `error.message` in `structuredContent`), not JSON-RPC protocol errors — -the handler returns `(result, nil)`: +and a text content block), not JSON-RPC protocol errors — the handler returns +`(result, nil)`. Error results omit `structuredContent` so strict clients do not +validate an error payload against the tool's success-only output schema: | Failure | Result | | --- | --- | diff --git a/server/router/mcp/adapter.go b/server/router/mcp/adapter.go index 9437179e..daa0ff84 100644 --- a/server/router/mcp/adapter.go +++ b/server/router/mcp/adapter.go @@ -92,46 +92,111 @@ func buildAPIRequest(ctx context.Context, operation *openAPIOperation, arguments } func substitutePathParameters(operation *openAPIOperation, arguments map[string]any) (string, error) { - path := operation.Path for _, parameter := range operation.Parameters { if parameter.In != "path" { continue } - value, ok := arguments[parameter.Name] if !ok || value == nil || valueToString(value) == "" { return "", errors.Errorf(`missing required path parameter "%s"`, parameter.Name) } - id := trimResourceNamePrefix(operation.Path, parameter.Name, valueToString(value)) - path = strings.ReplaceAll(path, "{"+parameter.Name+"}", url.PathEscape(id)) + } + + // Resolve placeholders in the order they appear in the path so a nested + // resource name (e.g. "memos/abc123/reactions/reaction456") can be matched + // against its already-resolved parent segments. Each placeholder is resolved + // exactly once from the argument map and cached in resolved, so a value that + // itself contains a "{" can never be re-expanded into a longer prefix. + path := operation.Path + resolved := map[string]string{} + for _, name := range pathPlaceholderNames(operation.Path) { + value, ok := arguments[name] + if !ok { + continue + } + id := trimResourceNamePrefix(operation.Path, name, valueToString(value), resolved) + resolved[name] = id + path = strings.ReplaceAll(path, "{"+name+"}", url.PathEscape(id)) } return path, nil } +// pathPlaceholderNames returns the "{name}" placeholder names in the order they +// appear in path. +func pathPlaceholderNames(path string) []string { + var names []string + for { + start := strings.Index(path, "{") + if start < 0 { + break + } + endOffset := strings.Index(path[start:], "}") + if endOffset < 0 { + break + } + end := start + endOffset + names = append(names, path[start+1:end]) + path = path[end+1:] + } + return names +} + // trimResourceNamePrefix accepts canonical resource names for path parameters. -// The API returns names like "memos/abc123", but the REST paths take the bare -// ID ("/api/v1/memos/{memo}"), so clients that round-trip a returned name -// would otherwise request "/api/v1/memos/memos/abc123" and get a 404. When the -// placeholder directly follows its collection segment and the value carries -// that collection prefix, strip the prefix; bare IDs pass through unchanged. -func trimResourceNamePrefix(path, parameterName, value string) string { +// It uses the already-resolved parent segments so nested names such as +// "memos/abc123/reactions/reaction456" are accepted only when their parent +// segments match the other arguments. Bare IDs pass through unchanged. +func trimResourceNamePrefix(path, parameterName, value string, resolved map[string]string) string { placeholder := "/{" + parameterName + "}" index := strings.Index(path, placeholder) if index < 0 { return value } - head := path[:index] - collection := head[strings.LastIndex(head, "/")+1:] - if collection == "" { + + prefix, ok := resolvedResourceNamePrefix(path[:index], resolved) + if !ok || prefix == "" { return value } - id, ok := strings.CutPrefix(value, collection+"/") + + id, ok := strings.CutPrefix(value, prefix+"/") if !ok || id == "" || strings.Contains(id, "/") { return value } return id } +// resolvedResourceNamePrefix rebuilds the collection prefix preceding a +// placeholder by substituting each earlier placeholder with its already-resolved +// bare id. It reads resolved ids from the map instead of recursing, and rejects +// any id that is not a single bare segment, so every iteration removes one "{" +// and the loop always terminates. +func resolvedResourceNamePrefix(prefix string, resolved map[string]string) (string, bool) { + const apiPrefix = "/api/v1/" + prefix, ok := strings.CutPrefix(prefix, apiPrefix) + if !ok { + return "", false + } + + for { + start := strings.Index(prefix, "{") + if start < 0 { + break + } + endOffset := strings.Index(prefix[start:], "}") + if endOffset < 0 { + return "", false + } + end := start + endOffset + parameterName := prefix[start+1 : end] + id, ok := resolved[parameterName] + if !ok || id == "" || strings.ContainsAny(id, "/{}") { + return "", false + } + prefix = prefix[:start] + id + prefix[end+1:] + } + + return strings.Trim(prefix, "/"), true +} + func valueToString(value any) string { switch typed := value.(type) { case string: diff --git a/server/router/mcp/adapter_test.go b/server/router/mcp/adapter_test.go index 64b13674..b6189721 100644 --- a/server/router/mcp/adapter_test.go +++ b/server/router/mcp/adapter_test.go @@ -147,6 +147,110 @@ func TestBuildAPIRequestAcceptsResourceNamesForPathParameters(t *testing.T) { } } +func TestBuildAPIRequestAcceptsHierarchicalResourceNamesForPathParameters(t *testing.T) { + tests := []struct { + name string + path string + parameters []string + arguments map[string]any + wantPath string + }{ + { + name: "canonical reaction name", + path: "/api/v1/memos/{memo}/reactions/{reaction}", + parameters: []string{"memo", "reaction"}, + arguments: map[string]any{ + "memo": "memos/abc123", + "reaction": "memos/abc123/reactions/reaction456", + }, + wantPath: "/api/v1/memos/abc123/reactions/reaction456", + }, + { + name: "canonical nested name with bare parent id", + path: "/api/v1/memos/{memo}/reactions/{reaction}", + parameters: []string{"memo", "reaction"}, + arguments: map[string]any{ + "memo": "abc123", + "reaction": "memos/abc123/reactions/reaction456", + }, + wantPath: "/api/v1/memos/abc123/reactions/reaction456", + }, + { + name: "parameter declaration order does not matter", + path: "/api/v1/users/{user}/shortcuts/{shortcut}", + parameters: []string{"shortcut", "user"}, + arguments: map[string]any{ + "user": "users/user123", + "shortcut": "users/user123/shortcuts/shortcut456", + }, + wantPath: "/api/v1/users/user123/shortcuts/shortcut456", + }, + { + name: "canonical nested name on action route", + path: "/api/v1/users/{user}/webhooks/{webhook}:getSigningSecret", + parameters: []string{"user", "webhook"}, + arguments: map[string]any{ + "user": "users/user123", + "webhook": "users/user123/webhooks/webhook456", + }, + wantPath: "/api/v1/users/user123/webhooks/webhook456:getSigningSecret", + }, + { + name: "mismatched parent is left untouched", + path: "/api/v1/memos/{memo}/reactions/{reaction}", + parameters: []string{"memo", "reaction"}, + arguments: map[string]any{ + "memo": "memos/abc123", + "reaction": "memos/other/reactions/reaction456", + }, + wantPath: "/api/v1/memos/abc123/reactions/memos%2Fother%2Freactions%2Freaction456", + }, + { + name: "extra nested segment is left untouched", + path: "/api/v1/memos/{memo}/reactions/{reaction}", + parameters: []string{"memo", "reaction"}, + arguments: map[string]any{ + "memo": "memos/abc123", + "reaction": "memos/abc123/reactions/reaction456/extra", + }, + wantPath: "/api/v1/memos/abc123/reactions/memos%2Fabc123%2Freactions%2Freaction456%2Fextra", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + parameters := make([]openAPIParameter, 0, len(test.parameters)) + for _, name := range test.parameters { + parameters = append(parameters, openAPIParameter{Name: name, In: "path", Required: true, Schema: jsonSchema{"type": "string"}}) + } + operation := &openAPIOperation{ + Method: "GET", + Path: test.path, + Parameters: parameters, + } + req, err := buildAPIRequest(context.Background(), operation, test.arguments, "") + require.NoError(t, err) + require.Equal(t, test.wantPath, req.URL.EscapedPath()) + }) + } +} + +func TestBuildAPIRequestAcceptsCanonicalReactionNameForCuratedOperation(t *testing.T) { + spec, err := loadMCPServiceOpenAPISpec() + require.NoError(t, err) + registry, err := buildOperationRegistry(spec) + require.NoError(t, err) + operation := registry["MemoService_DeleteMemoReaction"] + require.NotNil(t, operation) + + req, err := buildAPIRequest(context.Background(), operation, map[string]any{ + "memo": "memos/abc123", + "reaction": "memos/abc123/reactions/reaction456", + }, "") + require.NoError(t, err) + require.Equal(t, "/api/v1/memos/abc123/reactions/reaction456", req.URL.EscapedPath()) +} + func TestBuildAPIRequestRequiresPathParameters(t *testing.T) { operation := &openAPIOperation{ Method: "GET", diff --git a/server/router/mcp/catalog.go b/server/router/mcp/catalog.go index bb2c8807..ed768bc9 100644 --- a/server/router/mcp/catalog.go +++ b/server/router/mcp/catalog.go @@ -1,6 +1,7 @@ package mcp import ( + "maps" "regexp" "strings" @@ -42,8 +43,81 @@ type registeredOperation struct { InputSchema jsonSchema } +type requestBodySchemaOverride struct { + // required replaces the base schema's required list when non-empty. + required []string + // clearRequired explicitly removes the base schema's required list. + clearRequired bool + omittedProperties []string + // minProperties, when > 0, requires the body to carry at least that many + // properties. It replaces a cleared required list for partial updates so an + // empty body is rejected up front instead of failing later at the API. + minProperties int +} + +// requestBodySchemaOverrides adjusts resource schemas to match how each HTTP +// binding consumes its request body. Resource-level required fields are too +// strict for partial updates, while body: "*" schemas include fields already +// supplied by the path. +var requestBodySchemaOverrides = map[string]requestBodySchemaOverride{ + "MemoService_CreateMemo": { + required: []string{"content"}, + }, + "MemoService_UpdateMemo": { + clearRequired: true, + omittedProperties: []string{"name"}, + minProperties: 1, + }, + "MemoService_CreateMemoComment": { + required: []string{"content"}, + }, + "MemoService_SetMemoAttachments": { + required: []string{"attachments"}, + omittedProperties: []string{"name"}, + }, + "MemoService_SetMemoRelations": { + required: []string{"relations"}, + omittedProperties: []string{"name"}, + }, + "MemoService_UpsertMemoReaction": { + required: []string{"reaction"}, + omittedProperties: []string{"name"}, + }, +} + var wordBoundary = regexp.MustCompile(`([a-z0-9])([A-Z])`) +// validateOperationOverrides fails fast when a per-operation override table +// references an operation that is not in the registry (e.g. after a proto RPC +// rename). Without this, a stale key would silently miss and the renamed +// operation would lose its schema/annotation override with no error. +func validateOperationOverrides(registry map[string]*openAPIOperation) error { + tables := []struct { + name string + ids []string + }{ + {"requestBodySchemaOverrides", mapKeys(requestBodySchemaOverrides)}, + {"idempotentOperationIDs", mapKeys(idempotentOperationIDs)}, + {"destructiveOperationIDs", mapKeys(destructiveOperationIDs)}, + } + for _, table := range tables { + for _, operationID := range table.ids { + if _, ok := registry[operationID]; !ok { + return errors.Errorf("%s references unknown operation %q", table.name, operationID) + } + } + } + return nil +} + +func mapKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for key := range m { + keys = append(keys, key) + } + return keys +} + func buildCuratedTools(registry map[string]*openAPIOperation) ([]*sdkmcp.Tool, map[string]*registeredOperation, error) { tools := make([]*sdkmcp.Tool, 0, len(curatedOperationIDs)) operations := map[string]*registeredOperation{} @@ -61,6 +135,10 @@ func buildCuratedTools(registry map[string]*openAPIOperation) ([]*sdkmcp.Tool, m tools = append(tools, tool) operations[tool.Name] = registered } + + if err := validateOperationOverrides(registry); err != nil { + return nil, nil, err + } return tools, operations, nil } @@ -160,7 +238,30 @@ func requestBodySchema(operation *openAPIOperation) jsonSchema { if operation.RequestBodySchema == nil { return jsonSchema{"type": "object"} } - return cloneSchema(operation.RequestBodySchema) + schema := cloneSchema(operation.RequestBodySchema) + override, ok := requestBodySchemaOverrides[operation.OperationID] + if !ok { + return schema + } + + if override.clearRequired { + delete(schema, "required") + } else if len(override.required) > 0 { + schema["required"] = override.required + } + + if len(override.omittedProperties) > 0 { + properties := maps.Clone(schemaProperties(schema["properties"])) + for _, name := range override.omittedProperties { + delete(properties, name) + } + schema["properties"] = properties + } + + if override.minProperties > 0 { + schema["minProperties"] = override.minProperties + } + return schema } func outputSchemaForOperation(operation *openAPIOperation) jsonSchema { @@ -197,6 +298,14 @@ var idempotentOperationIDs = map[string]bool{ "MemoService_SetMemoRelations": true, } +// destructiveOperationIDs lists mutating operations that can overwrite or +// remove existing user data despite not using DELETE. +var destructiveOperationIDs = map[string]bool{ + "MemoService_UpdateMemo": true, + "MemoService_SetMemoAttachments": true, + "MemoService_SetMemoRelations": true, +} + // annotationsForOperation derives the method-based annotations and then applies // per-operation overrides that the HTTP method alone cannot express. func annotationsForOperation(operation *openAPIOperation, title string) *sdkmcp.ToolAnnotations { @@ -204,6 +313,10 @@ func annotationsForOperation(operation *openAPIOperation, title string) *sdkmcp. if idempotentOperationIDs[operation.OperationID] { annotations.IdempotentHint = true } + if destructiveOperationIDs[operation.OperationID] { + destructive := true + annotations.DestructiveHint = &destructive + } return annotations } diff --git a/server/router/mcp/catalog_test.go b/server/router/mcp/catalog_test.go index f4afb12a..1f31ffec 100644 --- a/server/router/mcp/catalog_test.go +++ b/server/router/mcp/catalog_test.go @@ -96,14 +96,139 @@ func TestBuildToolFromOperationIncludesRequestBodySchema(t *testing.T) { err = validateToolArguments(input, map[string]any{ "body": map[string]any{ - "state": "NORMAL", - "content": "hello", - "visibility": "PRIVATE", + "content": "hello", }, }) require.NoError(t, err) } +func TestBuildToolFromOperationTailorsRequestBodySchemas(t *testing.T) { + spec, err := loadOpenAPISpec("../../../proto/gen/openapi.yaml") + require.NoError(t, err) + registry, err := buildOperationRegistry(spec) + require.NoError(t, err) + + tests := []struct { + name string + operationID string + arguments map[string]any + omittedProperties []string + }{ + { + name: "partial memo update", + operationID: "MemoService_UpdateMemo", + arguments: map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{"content": "updated"}, + }, + omittedProperties: []string{"name"}, + }, + { + name: "comment defaults state and visibility", + operationID: "MemoService_CreateMemoComment", + arguments: map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{"content": "comment"}, + }, + }, + { + name: "set attachments gets name from path", + operationID: "MemoService_SetMemoAttachments", + arguments: map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{"attachments": []any{}}, + }, + omittedProperties: []string{"name"}, + }, + { + name: "set relations gets name from path", + operationID: "MemoService_SetMemoRelations", + arguments: map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{"relations": []any{}}, + }, + omittedProperties: []string{"name"}, + }, + { + name: "upsert reaction gets name from path", + operationID: "MemoService_UpsertMemoReaction", + arguments: map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{ + "reaction": map[string]any{ + "contentId": "memos/abc123", + "reactionType": "👍", + }, + }, + }, + omittedProperties: []string{"name"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + tool, _ := buildToolFromOperation(registry[test.operationID]) + input, ok := tool.InputSchema.(jsonSchema) + require.True(t, ok) + require.NoError(t, validateToolArguments(input, test.arguments)) + + properties := schemaProperties(input["properties"]) + body := schemaProperties(properties["body"]) + bodyProperties := schemaProperties(body["properties"]) + for _, property := range test.omittedProperties { + require.NotContains(t, bodyProperties, property) + } + }) + } +} + +func TestBuildToolFromOperationRejectsEmptyMemoUpdateBody(t *testing.T) { + spec, err := loadOpenAPISpec("../../../proto/gen/openapi.yaml") + require.NoError(t, err) + registry, err := buildOperationRegistry(spec) + require.NoError(t, err) + + tool, _ := buildToolFromOperation(registry["MemoService_UpdateMemo"]) + input, ok := tool.InputSchema.(jsonSchema) + require.True(t, ok) + + // An empty body carries no fields to update; reject it at the schema instead of + // letting the gateway infer an empty field mask and fail late. + require.Error(t, validateToolArguments(input, map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{}, + })) + require.NoError(t, validateToolArguments(input, map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{"content": "updated"}, + })) +} + +func TestRequestBodySchemaOverridePreservesRequiredByDefault(t *testing.T) { + const operationID = "TestService_UpdateResource" + requestBodySchemaOverrides[operationID] = requestBodySchemaOverride{ + omittedProperties: []string{"name"}, + } + t.Cleanup(func() { + delete(requestBodySchemaOverrides, operationID) + }) + + schema := requestBodySchema(&openAPIOperation{ + OperationID: operationID, + RequestBodySchema: jsonSchema{ + "type": "object", + "required": []string{"content"}, + "properties": map[string]any{ + "name": jsonSchema{"type": "string"}, + "content": jsonSchema{"type": "string"}, + }, + }, + }) + + require.Equal(t, []string{"content"}, schema["required"]) + require.NotContains(t, schemaProperties(schema["properties"]), "name") +} + func TestBuildToolFromOperationExposesCreateAttachment(t *testing.T) { spec, err := loadOpenAPISpec("../../../proto/gen/openapi.yaml") require.NoError(t, err) @@ -183,10 +308,23 @@ func TestBuildToolFromOperationMarksSetOperationsIdempotent(t *testing.T) { // override restores the declarative "set" semantics. require.True(t, tool.Annotations.IdempotentHint, operationID) require.False(t, tool.Annotations.ReadOnlyHint, operationID) - require.False(t, *tool.Annotations.DestructiveHint, operationID) + require.True(t, *tool.Annotations.DestructiveHint, operationID) } } +func TestBuildToolFromOperationMarksUpdateMemoDestructive(t *testing.T) { + spec, err := loadOpenAPISpec("../../../proto/gen/openapi.yaml") + require.NoError(t, err) + registry, err := buildOperationRegistry(spec) + require.NoError(t, err) + + tool, operation := buildToolFromOperation(registry["MemoService_UpdateMemo"]) + require.Equal(t, "PATCH", operation.Method) + require.False(t, tool.Annotations.ReadOnlyHint) + require.True(t, *tool.Annotations.DestructiveHint) + require.False(t, tool.Annotations.IdempotentHint) +} + func TestBuildCuratedToolsHasUniqueNames(t *testing.T) { spec, err := loadOpenAPISpec("../../../proto/gen/openapi.yaml") require.NoError(t, err) @@ -221,6 +359,19 @@ func TestBuildCuratedToolsRejectsMissingOperation(t *testing.T) { require.ErrorContains(t, err, "not found") } +func TestValidateOperationOverridesRejectsStaleKey(t *testing.T) { + spec, err := loadOpenAPISpec("../../../proto/gen/openapi.yaml") + require.NoError(t, err) + registry, err := buildOperationRegistry(spec) + require.NoError(t, err) + require.NoError(t, validateOperationOverrides(registry)) + + // A renamed/removed operation must be reported instead of silently losing its + // override. + delete(registry, "MemoService_UpdateMemo") + require.ErrorContains(t, validateOperationOverrides(registry), "MemoService_UpdateMemo") +} + func TestBuildCuratedToolsRejectsDuplicateToolNames(t *testing.T) { registry := make(map[string]*openAPIOperation, len(curatedOperationIDs)) for _, operationID := range curatedOperationIDs { diff --git a/server/router/mcp/service.go b/server/router/mcp/service.go index b7c4a192..4d736476 100644 --- a/server/router/mcp/service.go +++ b/server/router/mcp/service.go @@ -6,14 +6,20 @@ import ( "net/http" "github.com/labstack/echo/v5" + "github.com/labstack/echo/v5/middleware" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/pkg/errors" "gopkg.in/yaml.v3" "github.com/usememos/memos/internal/profile" memosproto "github.com/usememos/memos/proto" + apiv1 "github.com/usememos/memos/server/router/api/v1" ) +// maxMCPRequestBytes caps the /mcp request body. It tracks the API limit because +// every tool call is forwarded in-process through the API routes. +const maxMCPRequestBytes int64 = apiv1.MaxAPIRequestBytes + // MCPService serves the OpenAPI-driven MCP endpoint. type MCPService struct { profile *profile.Profile @@ -52,7 +58,7 @@ func NewMCPService(profile *profile.Profile, echoServer *echo.Echo) (*MCPService server.AddTool(tool, newMCPToolHandler(adapter, operation)) } - handler := sdkmcp.NewStreamableHTTPHandler(func(*http.Request) *sdkmcp.Server { + streamableHandler := sdkmcp.NewStreamableHTTPHandler(func(*http.Request) *sdkmcp.Server { return server }, &sdkmcp.StreamableHTTPOptions{ Stateless: true, @@ -65,11 +71,10 @@ func NewMCPService(profile *profile.Profile, echoServer *echo.Echo) (*MCPService // CSRF / DNS-rebinding protection instead. DisableLocalhostProtection: true, }) - return &MCPService{ profile: profile, operationsByTool: operationsByTool, - handler: handler, + handler: streamableHandler, }, nil } @@ -113,5 +118,5 @@ func (s *MCPService) RegisterRoutes(echoServer *echo.Echo) { } s.handler.ServeHTTP(c.Response(), request) return nil - }) + }, middleware.BodyLimit(maxMCPRequestBytes)) } diff --git a/server/router/mcp/service_test.go b/server/router/mcp/service_test.go index 54586f8c..53a8a37e 100644 --- a/server/router/mcp/service_test.go +++ b/server/router/mcp/service_test.go @@ -178,6 +178,143 @@ func TestMCPToolCallReturnsObjectStructuredContent(t *testing.T) { }, result["structuredContent"]) } +func TestMCPToolCallAllowsGatewayToInferMemoUpdateMask(t *testing.T) { + echoServer := echo.New() + routeHits := 0 + echoServer.PATCH("/api/v1/memos/:memo", func(c *echo.Context) error { + routeHits++ + require.Equal(t, "abc123", c.Param("memo")) + require.Empty(t, c.QueryParam("updateMask")) + + body := map[string]any{} + require.NoError(t, json.NewDecoder(c.Request().Body).Decode(&body)) + require.Equal(t, map[string]any{"content": "updated"}, body) + return c.JSON(http.StatusOK, map[string]any{ + "name": "memos/abc123", + "content": "updated", + }) + }) + + service, err := NewMCPService(&profile.Profile{Version: "test-version"}, echoServer) + require.NoError(t, err) + service.RegisterRoutes(echoServer) + + initializeMCP(t, echoServer) + response := postMCP(t, echoServer, map[string]any{ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": map[string]any{ + "name": "memo_update_memo", + "arguments": map[string]any{ + "memo": "memos/abc123", + "body": map[string]any{"content": "updated"}, + }, + }, + }) + + result, ok := response["result"].(map[string]any) + require.True(t, ok) + require.NotEqual(t, true, result["isError"]) + require.Equal(t, map[string]any{ + "name": "memos/abc123", + "content": "updated", + }, result["structuredContent"]) + require.Equal(t, 1, routeHits) +} + +func TestMCPToolCallBindsMemoFromPathForBodyStarOperations(t *testing.T) { + register := func(e *echo.Echo, method, path string, handler func(*echo.Context) error) { + switch method { + case http.MethodPatch: + e.PATCH(path, handler) + case http.MethodPost: + e.POST(path, handler) + default: + t.Fatalf("unsupported method %q", method) + } + } + + tests := []struct { + name string + method string + path string + toolName string + body map[string]any + response map[string]any + }{ + { + name: "set attachments", + method: http.MethodPatch, + path: "/api/v1/memos/:memo/attachments", + toolName: "memo_set_memo_attachments", + body: map[string]any{"attachments": []any{}}, + response: map[string]any{}, + }, + { + name: "set relations", + method: http.MethodPatch, + path: "/api/v1/memos/:memo/relations", + toolName: "memo_set_memo_relations", + body: map[string]any{"relations": []any{}}, + response: map[string]any{}, + }, + { + name: "upsert reaction", + method: http.MethodPost, + path: "/api/v1/memos/:memo/reactions", + toolName: "memo_upsert_memo_reaction", + body: map[string]any{ + "reaction": map[string]any{ + "contentId": "memos/abc123", + "reactionType": "👍", + }, + }, + response: map[string]any{"reactionType": "👍"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + echoServer := echo.New() + routeHits := 0 + register(echoServer, test.method, test.path, func(c *echo.Context) error { + routeHits++ + // The memo must be bound from the path, and the omitted "name" + // property must not reappear in the forwarded body. + require.Equal(t, "abc123", c.Param("memo")) + body := map[string]any{} + require.NoError(t, json.NewDecoder(c.Request().Body).Decode(&body)) + require.NotContains(t, body, "name") + return c.JSON(http.StatusOK, test.response) + }) + + service, err := NewMCPService(&profile.Profile{Version: "test-version"}, echoServer) + require.NoError(t, err) + service.RegisterRoutes(echoServer) + + initializeMCP(t, echoServer) + response := postMCP(t, echoServer, map[string]any{ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": map[string]any{ + "name": test.toolName, + "arguments": map[string]any{ + "memo": "memos/abc123", + "body": test.body, + }, + }, + }) + + result, ok := response["result"].(map[string]any) + require.True(t, ok) + require.NotEqual(t, true, result["isError"], result) + require.Equal(t, 1, routeHits) + }) + } +} + func TestMCPToolCallRejectsInvalidArguments(t *testing.T) { echoServer := echo.New() routeHits := 0