fix(mcp): apply the REST layer's guards in Dispatch

- page/per_page default and clamp like the REST handler; page < 1 dropped
  the LIMIT clause entirely
- config-gated resources are unreachable through do_action
- create/update run the model's valid: tag rules
- read_all returns {items, result_count, total_items, page, per_page} and
  strips emails from user rows
This commit is contained in:
kolaente
2026-08-29 00:48:55 +02:00
parent 1adbae211e
commit 08ab38836b
12 changed files with 303 additions and 26 deletions
+23 -4
View File
@@ -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
}
+2
View File
@@ -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`),
+78 -2
View File
@@ -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,
}
}
+112 -2
View File
@@ -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) {
+2 -2
View File
@@ -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)
+2 -2
View File
@@ -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"])
+1 -2
View File
@@ -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")
}
+15 -4
View File
@@ -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 {
+4 -2
View File
@@ -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)
}
}
}
+22 -2
View File
@@ -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")
}
+41 -2
View File
@@ -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{
+1 -2
View File
@@ -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)
}