diff --git a/pkg/modules/mcp/apply.go b/pkg/modules/mcp/apply.go index 63f2eb3ef..b48a4463f 100644 --- a/pkg/modules/mcp/apply.go +++ b/pkg/modules/mcp/apply.go @@ -28,6 +28,7 @@ import ( "fmt" "reflect" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/web/handler" ) @@ -80,8 +81,10 @@ func applyArgs(model handler.CObject, spec *opSpec, args map[string]json.RawMess } // popReadAllParams extracts (and removes) the reserved search/page/per_page -// arguments so applyArgs only sees model-bound keys. They map onto -// handler.DoReadAll's positional parameters. +// arguments so applyArgs only sees model-bound keys, and normalises them the +// way the REST layer does before calling handler.DoReadAll. The normalisation +// is not optional: page < 1 makes the models skip the LIMIT clause entirely, +// so an omitted per_page would dump every row the caller can see. func popReadAllParams(args map[string]json.RawMessage) (search string, page, perPage int, err error) { pop := func(name string, dst any) error { raw, ok := args[name] @@ -100,6 +103,22 @@ func popReadAllParams(args map[string]json.RawMessage) (search string, page, per if err = pop(argPage, &page); err != nil { return } - err = pop(argPerPage, &perPage) - return + if err = pop(argPerPage, &perPage); err != nil { + return + } + + if page < 0 { + return "", 0, 0, fmt.Errorf("invalid value for %q: must not be negative", argPage) + } + if page == 0 { + page = 1 + } + if perPage < 0 { + return "", 0, 0, fmt.Errorf("invalid value for %q: must not be negative", argPerPage) + } + maxPerPage := config.ServiceMaxItemsPerPage.GetInt() + if perPage == 0 || perPage > maxPerPage { + perPage = maxPerPage + } + return search, page, perPage, nil } diff --git a/pkg/modules/mcp/apply_test.go b/pkg/modules/mcp/apply_test.go index e6b8e7c85..5abfc0b08 100644 --- a/pkg/modules/mcp/apply_test.go +++ b/pkg/modules/mcp/apply_test.go @@ -20,6 +20,7 @@ import ( "encoding/json" "testing" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/models" "github.com/stretchr/testify/assert" @@ -105,6 +106,7 @@ func TestValidate_NonObjectArgumentsRejected(t *testing.T) { } func TestPopReadAllParams(t *testing.T) { + config.InitDefaultConfig() args := map[string]json.RawMessage{ argSearch: json.RawMessage(`"foo"`), argPage: json.RawMessage(`2`), diff --git a/pkg/modules/mcp/dispatcher.go b/pkg/modules/mcp/dispatcher.go index 5a8794feb..901181c2f 100644 --- a/pkg/modules/mcp/dispatcher.go +++ b/pkg/modules/mcp/dispatcher.go @@ -21,7 +21,11 @@ import ( "encoding/json" "errors" "fmt" + "reflect" + "strings" + "code.vikunja.io/api/pkg/models" + "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/web" "code.vikunja.io/api/pkg/web/handler" ) @@ -83,6 +87,12 @@ func Dispatch(ctx context.Context, toolName string, rawArgs json.RawMessage) (an return nil, fmt.Errorf("%w: %s", ErrToolNotFound, toolName) } + // tools/list and find_action already hide gated resources; do_action + // would otherwise reach them by name. + if !ref.resource.enabled() { + return nil, fmt.Errorf("%w: %s", ErrToolNotFound, toolName) + } + // Fail closed: do_action must not reach a tool the token was never // registered for. if !tokenAuthorizes(TokenFromContext(ctx), ref.resource.Name, ref.op) { @@ -113,6 +123,14 @@ func Dispatch(ctx context.Context, toolName string, rawArgs json.RawMessage) (an return nil, fmt.Errorf("mcp: invalid arguments for %s: %w", toolName, err) } + // The REST layer runs this via echo's CustomValidator before the handler; + // without it MCP writes bypass every `valid:` tag rule on the model. + if ref.op == OpCreate || ref.op == OpUpdate { + if err := models.ValidateStructFields(model, suppliedFieldNames(model, spec, args)); err != nil { + return nil, validationFailure(toolName, err) + } + } + switch ref.op { case OpCreate: if err := crud.doCreate(ctx, model, u); err != nil { @@ -127,11 +145,11 @@ func Dispatch(ctx context.Context, toolName string, rawArgs json.RawMessage) (an return model, nil case OpReadAll: - result, _, _, err := crud.doReadAll(ctx, model, u, search, page, perPage) + result, resultCount, totalItems, err := crud.doReadAll(ctx, model, u, search, page, perPage) if err != nil { return nil, err } - return result, nil + return newReadAllResult(result, resultCount, totalItems, page, perPage), nil case OpUpdate: if err := crud.doUpdate(ctx, model, u); err != nil { @@ -148,3 +166,61 @@ func Dispatch(ctx context.Context, toolName string, rawArgs json.RawMessage) (an return nil, fmt.Errorf("mcp: unsupported op %d for tool %s", ref.op, toolName) } + +// validationFailure renders a `valid:` tag failure for a tool result, which is +// plain text — ValidationHTTPError keeps the offending field names out of its +// message. +func validationFailure(toolName string, err error) error { + var invalid models.ValidationHTTPError + if errors.As(err, &invalid) && len(invalid.InvalidFields) > 0 { + return fmt.Errorf("mcp: invalid arguments for %s: %s", toolName, strings.Join(invalid.InvalidFields, "; ")) + } + return fmt.Errorf("mcp: invalid arguments for %s: %w", toolName, err) +} + +// suppliedFieldNames returns the names govalidator may report for the +// arguments the caller actually sent: the JSON property name plus the Go +// field name, which govalidator falls back to for `json:"-"` fields. +func suppliedFieldNames(model handler.CObject, spec *opSpec, args map[string]json.RawMessage) map[string]bool { + modelType := reflect.TypeOf(model).Elem() + names := make(map[string]bool, len(args)*2) + for name := range args { + names[name] = true + if idx, ok := spec.fields[name]; ok { + names[modelType.Field(idx).Name] = true + } + } + return names +} + +// readAllResult is the read_all envelope. A bare array left clients no way to +// tell a truncated page from the last one, and no way to page on from it. +type readAllResult struct { + Items any `json:"items"` + ResultCount int `json:"result_count"` + TotalItems int64 `json:"total_items"` + Page int `json:"page"` + PerPage int `json:"per_page"` +} + +func newReadAllResult(items any, resultCount int, totalItems int64, page, perPage int) *readAllResult { + // read_all hands out user rows directly, skipping the per-parent + // serialisation the REST layer relies on to hide addresses. + if users, ok := items.([]*user.User); ok { + for _, u := range users { + if u != nil { + u.Email = "" + } + } + } + if v := reflect.ValueOf(items); !v.IsValid() || (v.Kind() == reflect.Slice && v.IsNil()) { + items = []any{} + } + return &readAllResult{ + Items: items, + ResultCount: resultCount, + TotalItems: totalItems, + Page: page, + PerPage: perPage, + } +} diff --git a/pkg/modules/mcp/dispatcher_test.go b/pkg/modules/mcp/dispatcher_test.go index 37432676e..b930aa502 100644 --- a/pkg/modules/mcp/dispatcher_test.go +++ b/pkg/modules/mcp/dispatcher_test.go @@ -22,6 +22,7 @@ import ( "errors" "testing" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/web" @@ -205,6 +206,7 @@ func TestDispatchCallsReadOne(t *testing.T) { func TestDispatchCallsReadAll(t *testing.T) { resetRegistry(t) installStubCRUD(t) + config.InitDefaultConfig() tracker := &stubTracker{} require.NoError(t, Register(Resource{ Name: "stubs", @@ -212,13 +214,121 @@ func TestDispatchCallsReadAll(t *testing.T) { Ops: OpReadAll, })) - out, err := Dispatch(newAuthedCtx(t), "stubs_read_all", json.RawMessage(`{"search":"foo","page":2,"per_page":50}`)) + out, err := Dispatch(newAuthedCtx(t), "stubs_read_all", json.RawMessage(`{"search":"foo","page":2,"per_page":25}`)) require.NoError(t, err) require.NotNil(t, tracker.last) assert.Equal(t, "ReadAll", tracker.last.called) // The stub's ReadAll echoes the search/page/per_page so we can confirm // the dispatcher threaded the wrapper's pagination fields through. - assert.Equal(t, []string{"foo"}, out) + env := requireReadAllResult(t, out) + assert.Equal(t, []string{"foo"}, env.Items) + assert.Equal(t, 2, env.Page) + assert.Equal(t, 25, env.PerPage) + assert.Equal(t, 2, env.ResultCount) + assert.Equal(t, int64(25), env.TotalItems) +} + +func requireReadAllResult(t *testing.T, out any) *readAllResult { + t.Helper() + env, ok := out.(*readAllResult) + require.Truef(t, ok, "read_all must return an envelope, got %T", out) + return env +} + +// dispatchReadAll registers a stub resource and lists it with the given raw +// arguments. +func dispatchReadAll(t *testing.T, rawArgs string) (any, error) { + t.Helper() + resetRegistry(t) + installStubCRUD(t) + config.InitDefaultConfig() + tracker := &stubTracker{} + require.NoError(t, Register(Resource{ + Name: "stubs", + Model: tracker.empty, + Ops: OpReadAll, + })) + return Dispatch(newAuthedCtx(t), "stubs_read_all", json.RawMessage(rawArgs)) +} + +func TestDispatchReadAllPaginationDefaults(t *testing.T) { + // Omitted page/per_page must land on page 1 with the server maximum — + // passing them through as zero makes the models drop the LIMIT clause. + out, err := dispatchReadAll(t, `{}`) + require.NoError(t, err) + env := requireReadAllResult(t, out) + assert.Equal(t, 1, env.Page) + assert.Equal(t, config.ServiceMaxItemsPerPage.GetInt(), env.PerPage) +} + +func TestDispatchReadAllPerPageClampedToMax(t *testing.T) { + out, err := dispatchReadAll(t, `{"per_page":1000000}`) + require.NoError(t, err) + env := requireReadAllResult(t, out) + assert.Equal(t, config.ServiceMaxItemsPerPage.GetInt(), env.PerPage) +} + +func TestDispatchReadAllRejectsNegativePagination(t *testing.T) { + _, err := dispatchReadAll(t, `{"page":-1}`) + require.Error(t, err) + assert.Contains(t, err.Error(), `invalid value for "page"`) + + _, err = dispatchReadAll(t, `{"per_page":-1}`) + require.Error(t, err) + assert.Contains(t, err.Error(), `invalid value for "per_page"`) +} + +func TestDispatchGatedResourceIsNotFound(t *testing.T) { + // A disabled resource is hidden from tools/list and find_action; + // do_action must not be able to name it either. + resetRegistry(t) + installStubCRUD(t) + tracker := &stubTracker{} + require.NoError(t, Register(Resource{ + Name: "stubs", + Model: tracker.empty, + Ops: OpReadOne, + Gate: func() bool { return false }, + })) + + _, err := Dispatch(newAuthedCtx(t), "stubs_read_one", json.RawMessage(`{"id":1}`)) + require.Error(t, err) + require.ErrorIs(t, err, ErrToolNotFound) + assert.Empty(t, tracker.last.called, "a gated resource must never reach its model") +} + +// validatedStub carries a `valid:` tag so the dispatcher's tag validation has +// something to reject. The embedded stub supplies the CRUD methods; schema +// derivation skips anonymous fields, so only "amount" becomes an argument. +type validatedStub struct { + Amount int64 `json:"amount" valid:"range(0|10)"` + stubCObject +} + +func TestDispatchValidatesTagRules(t *testing.T) { + // `valid:` tags are enforced by echo's validator in REST; MCP has to run + // them itself or writes bypass them entirely. + resetRegistry(t) + installStubCRUD(t) + var last *validatedStub + require.NoError(t, Register(Resource{ + Name: "stubs", + Model: func() handler.CObject { + last = &validatedStub{} + return last + }, + Ops: OpCreate, + })) + + _, err := Dispatch(newAuthedCtx(t), "stubs_create", json.RawMessage(`{"amount":50}`)) + require.Error(t, err) + assert.Contains(t, err.Error(), "amount") + require.NotNil(t, last) + assert.Empty(t, last.called, "validation must run before the model is touched") + + _, err = Dispatch(newAuthedCtx(t), "stubs_create", json.RawMessage(`{"amount":5}`)) + require.NoError(t, err) + assert.Equal(t, "Create", last.called) } func TestDispatchCallsUpdate(t *testing.T) { diff --git a/pkg/modules/mcp/schema.go b/pkg/modules/mcp/schema.go index bbaaa159b..4079232bf 100644 --- a/pkg/modules/mcp/schema.go +++ b/pkg/modules/mcp/schema.go @@ -166,8 +166,8 @@ func buildOpSpec(modelType reflect.Type, op Op, r *Resource) (*opSpec, error) { if op == OpReadAll { addQueryOnlyArgs(modelType, props, fields, excluded) props[argSearch] = &jsonschema.Schema{Type: "string", Description: "Filter results by a case-insensitive substring match on the resource's primary text field."} - props[argPage] = &jsonschema.Schema{Type: "integer", Description: "1-based page number; 0 or omitted uses the server default (first page)."} - props[argPerPage] = &jsonschema.Schema{Type: "integer", Description: "Page size; 0 or omitted uses the server default."} + props[argPage] = &jsonschema.Schema{Type: "integer", Description: "1-based page number; 0 or omitted means the first page. Negative values are rejected."} + props[argPerPage] = &jsonschema.Schema{Type: "integer", Description: "Page size; 0 or omitted uses the server maximum, and larger values are clamped to it. The response reports the page size actually applied."} } sort.Strings(required) diff --git a/pkg/webtests/mcp_catalog_test.go b/pkg/webtests/mcp_catalog_test.go index 567a76eca..dae0bfb56 100644 --- a/pkg/webtests/mcp_catalog_test.go +++ b/pkg/webtests/mcp_catalog_test.go @@ -100,7 +100,7 @@ func TestMCP_Catalog_DoActionLabelRoundTrip(t *testing.T) { }) require.NotContains(t, result, "isError") var labels []map[string]any - require.NoError(t, json.Unmarshal([]byte(toolResultText(t, result)), &labels)) + readAllItems(t, result, &labels) ids := map[float64]bool{} for _, l := range labels { ids[l["id"].(float64)] = true @@ -123,7 +123,7 @@ func TestMCP_Catalog_DoActionListsProjectViews(t *testing.T) { require.NotContains(t, result, "isError", "do_action projects_views_read_all errored: %v", result) var views []map[string]any - require.NoError(t, json.Unmarshal([]byte(toolResultText(t, result)), &views)) + readAllItems(t, result, &views) require.NotEmpty(t, views) for _, v := range views { assert.EqualValues(t, 1, v["project_id"]) diff --git a/pkg/webtests/mcp_labels_test.go b/pkg/webtests/mcp_labels_test.go index 775dc3e6f..3f86bd8ec 100644 --- a/pkg/webtests/mcp_labels_test.go +++ b/pkg/webtests/mcp_labels_test.go @@ -62,9 +62,8 @@ func TestMCP_Labels_ReadAll(t *testing.T) { result := c.callTool("labels_read_all", map[string]any{}) require.NotContains(t, result, "isError") - text := toolResultText(t, result) var labels []map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &labels)) + readAllItems(t, result, &labels) require.NotEmpty(t, labels, "expected at least one label") } diff --git a/pkg/webtests/mcp_projects_test.go b/pkg/webtests/mcp_projects_test.go index 92682417a..3b237a006 100644 --- a/pkg/webtests/mcp_projects_test.go +++ b/pkg/webtests/mcp_projects_test.go @@ -125,6 +125,19 @@ func toolResultText(t *testing.T, result map[string]any) string { return text } +// readAllItems unmarshals the items array out of a read_all envelope into +// dest. read_all returns {items, result_count, total_items, page, per_page}, +// not a bare array. +func readAllItems(t *testing.T, result map[string]any, dest any) { + t.Helper() + text := toolResultText(t, result) + var env struct { + Items json.RawMessage `json:"items"` + } + require.NoError(t, json.Unmarshal([]byte(text), &env), "text was: %s", text) + require.NoError(t, json.Unmarshal(env.Items, dest), "items were: %s", env.Items) +} + func TestMCP_Projects_ToolsListAll(t *testing.T) { // Token 11 has every project scope plus the scopes added in Task 7 // (tasks, labels, teams, tasks_comments, tasks_assignees). The total @@ -227,9 +240,8 @@ func TestMCP_Projects_ReadAll(t *testing.T) { result := c.callTool("projects_read_all", map[string]any{}) require.NotContains(t, result, "isError", "read_all errored: %v", result) - text := toolResultText(t, result) var projects []map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &projects), "text was: %s", text) + readAllItems(t, result, &projects) require.NotEmpty(t, projects, "expected at least one project") // User 1 owns Test1 (project id 1); confirm it's in the response. @@ -250,9 +262,8 @@ func TestMCP_Projects_ReadAllSearch(t *testing.T) { }) require.NotContains(t, result, "isError") - text := toolResultText(t, result) var projects []map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &projects)) + readAllItems(t, result, &projects) // At minimum the matching project Test1 should appear. require.NotEmpty(t, projects) for _, p := range projects { diff --git a/pkg/webtests/mcp_task_assignees_test.go b/pkg/webtests/mcp_task_assignees_test.go index 499a18a73..6daab04f6 100644 --- a/pkg/webtests/mcp_task_assignees_test.go +++ b/pkg/webtests/mcp_task_assignees_test.go @@ -60,10 +60,12 @@ func TestMCP_TaskAssignees_ReadAllAccess(t *testing.T) { // Either the model bug surfaces as IsError (current state) or the // upstream fix succeeds; both are acceptable for this MCP test. if isErr, _ := result["isError"].(bool); !isErr { - text := toolResultText(t, result) var assignees []map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &assignees)) + readAllItems(t, result, &assignees) require.NotEmpty(t, assignees, "expected at least one assignee on task 30") + for _, a := range assignees { + assert.Empty(t, a["email"], "read_all must not leak assignee email addresses: %v", a) + } } } diff --git a/pkg/webtests/mcp_task_comments_test.go b/pkg/webtests/mcp_task_comments_test.go index b7460abf9..4dfca7a9a 100644 --- a/pkg/webtests/mcp_task_comments_test.go +++ b/pkg/webtests/mcp_task_comments_test.go @@ -83,9 +83,8 @@ func TestMCP_TaskComments_ReadAll(t *testing.T) { result := c.callTool("tasks_comments_read_all", map[string]any{"task_id": 1}) require.NotContains(t, result, "isError") - text := toolResultText(t, result) var comments []map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &comments)) + readAllItems(t, result, &comments) // Fixture task 1 has at least one comment. require.NotEmpty(t, comments) } @@ -114,4 +113,25 @@ func TestMCP_TaskComments_DisabledByConfig(t *testing.T) { assert.Falsef(t, strings.HasPrefix(name, "tasks_comments_"), "tasks_comments_* tool must be absent when comments are disabled: %s", name) } + + // do_action names tools directly, bypassing tools/list, so the gate has + // to be re-checked in the dispatcher. + result := c.callTool("do_action", map[string]any{ + "action": "tasks_comments_create", + "arguments": map[string]any{"task_id": 1, "comment": "must not be created"}, + }) + require.Equal(t, true, result["isError"], "do_action must not reach a disabled resource: %v", result) + assert.Contains(t, toolResultText(t, result), "mcp: tool not found: tasks_comments_create") +} + +func TestMCP_TaskComments_CreateRejectsEmptyComment(t *testing.T) { + // TaskComment.Comment is valid:"required"; the REST layer rejects an + // empty one before the handler and MCP must do the same. + c := newMCPClient(t, mcpFullProjectsToken) + result := c.callTool("tasks_comments_create", map[string]any{ + "task_id": 1, + "comment": "", + }) + require.Equal(t, true, result["isError"], "expected isError: %v", result) + assert.Contains(t, toolResultText(t, result), "comment") } diff --git a/pkg/webtests/mcp_tasks_test.go b/pkg/webtests/mcp_tasks_test.go index bc715bce3..383b206bb 100644 --- a/pkg/webtests/mcp_tasks_test.go +++ b/pkg/webtests/mcp_tasks_test.go @@ -20,6 +20,8 @@ import ( "encoding/json" "testing" + "code.vikunja.io/api/pkg/config" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -52,7 +54,7 @@ func TestMCP_Tasks_ReadAllWithFilter(t *testing.T) { require.NotContains(t, result, "isError", "read_all errored: %v", result) var tasks []map[string]any - require.NoError(t, json.Unmarshal([]byte(toolResultText(t, result)), &tasks)) + readAllItems(t, result, &tasks) require.NotEmpty(t, tasks, "fixtures contain done tasks") for _, task := range tasks { assert.Equal(t, true, task["done"], "filter must only return done tasks: %v", task["id"]) @@ -61,13 +63,50 @@ func TestMCP_Tasks_ReadAllWithFilter(t *testing.T) { // Scoped to a single project via the optional project_id argument. result = c.callTool("tasks_read_all", map[string]any{"project_id": 1}) require.NotContains(t, result, "isError") - require.NoError(t, json.Unmarshal([]byte(toolResultText(t, result)), &tasks)) + readAllItems(t, result, &tasks) require.NotEmpty(t, tasks) for _, task := range tasks { assert.InDelta(t, float64(1), task["project_id"], 0.0001, "task %v outside project 1", task["id"]) } } +func TestMCP_Tasks_ReadAllPagination(t *testing.T) { + c := newMCPClient(t, mcpFullProjectsToken) + result := c.callTool("tasks_read_all", map[string]any{"per_page": 1000000}) + require.NotContains(t, result, "isError", "read_all errored: %v", result) + + var env struct { + Items []map[string]any `json:"items"` + ResultCount int `json:"result_count"` + TotalItems int64 `json:"total_items"` + Page int `json:"page"` + PerPage int `json:"per_page"` + } + text := toolResultText(t, result) + require.NoError(t, json.Unmarshal([]byte(text), &env), "text was: %s", text) + assert.Equal(t, 1, env.Page, "an omitted page must default to the first one") + assert.Equal(t, config.ServiceMaxItemsPerPage.GetInt(), env.PerPage, "per_page must be clamped to the server maximum") + assert.LessOrEqual(t, len(env.Items), env.PerPage, "more items than the page size") + assert.Equal(t, len(env.Items), env.ResultCount) + assert.Positive(t, env.TotalItems) + + result = c.callTool("tasks_read_all", map[string]any{"page": -1}) + assert.Equal(t, true, result["isError"], "a negative page must be rejected: %v", result) +} + +func TestMCP_Tasks_CreateRejectsInvalidTagValue(t *testing.T) { + // repeat_after carries valid:"range(0|...)"; without the dispatcher + // running the model's tag rules a negative value would reach the DB. + c := newMCPClient(t, mcpFullProjectsToken) + result := c.callTool("tasks_create", map[string]any{ + "title": "task with a negative repeat", + "project_id": 1, + "repeat_after": -5, + }) + require.Equal(t, true, result["isError"], "expected isError: %v", result) + assert.Contains(t, toolResultText(t, result), "repeat_after") +} + func TestMCP_Tasks_Create(t *testing.T) { c := newMCPClient(t, mcpFullProjectsToken) result := c.callTool("tasks_create", map[string]any{ diff --git a/pkg/webtests/mcp_teams_test.go b/pkg/webtests/mcp_teams_test.go index b8b65de80..1073c297a 100644 --- a/pkg/webtests/mcp_teams_test.go +++ b/pkg/webtests/mcp_teams_test.go @@ -62,9 +62,8 @@ func TestMCP_Teams_ReadAll(t *testing.T) { result := c.callTool("teams_read_all", map[string]any{}) require.NotContains(t, result, "isError") - text := toolResultText(t, result) var teams []map[string]any - require.NoError(t, json.Unmarshal([]byte(text), &teams)) + readAllItems(t, result, &teams) // User 1 created several testteam* teams (fixtures). require.NotEmpty(t, teams) }