diff --git a/server/router/api/v1/gateway_marshaler_test.go b/server/router/api/v1/gateway_marshaler_test.go new file mode 100644 index 00000000..2df8a646 --- /dev/null +++ b/server/router/api/v1/gateway_marshaler_test.go @@ -0,0 +1,91 @@ +package v1 + +import ( + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/timestamppb" + + v1pb "github.com/usememos/memos/proto/gen/api/v1" +) + +// TestGatewayMarshalerOmitsUnsetMessageFields pins the REST payload shape the +// generated OpenAPI schema describes. grpc-gateway's stock marshaler writes +// `"motionMedia": null` for a plain image attachment, which no schema declares +// as nullable, and clients that validate against the spec — including strict +// MCP clients, whose tool outputSchema is that same schema — reject the +// response outright. +func TestGatewayMarshalerOmitsUnsetMessageFields(t *testing.T) { + attachment := &v1pb.Attachment{ + Name: "attachments/plainimage1", + CreateTime: timestamppb.New(time.Unix(1700000000, 0).UTC()), + Filename: "sunset.png", + Type: "image/png", + } + + payload := marshalThroughGateway(t, attachment) + + require.NotContains(t, payload, "motionMedia", "an unset message field must be omitted, not emitted as null") + // Scalar defaults stay in the payload: the schema lists filename and type as + // required, so dropping unpopulated scalars would break validation instead. + require.Equal(t, "", payload["externalLink"]) + require.Equal(t, "sunset.png", payload["filename"]) + require.Equal(t, "image/png", payload["type"]) + require.Equal(t, "0", payload["size"]) +} + +func TestGatewayMarshalerKeepsPopulatedMessageFields(t *testing.T) { + attachment := &v1pb.Attachment{ + Name: "attachments/livephoto1", + Filename: "walk.heic", + Type: "image/heic", + MotionMedia: &v1pb.MotionMedia{ + Family: v1pb.MotionMediaFamily_APPLE_LIVE_PHOTO, + Role: v1pb.MotionMediaRole_STILL, + GroupId: "group1", + }, + } + + payload := marshalThroughGateway(t, attachment) + + motionMedia, ok := payload["motionMedia"].(map[string]any) + require.True(t, ok, "a populated message field must still be emitted: %v", payload["motionMedia"]) + require.Equal(t, "APPLE_LIVE_PHOTO", motionMedia["family"]) + require.Equal(t, "group1", motionMedia["groupId"]) +} + +// TestGatewayMarshalerKeepsEmptyCollections guards the other half of the +// default-value behaviour: list fields the schema types as arrays must not +// disappear when empty. +func TestGatewayMarshalerKeepsEmptyCollections(t *testing.T) { + memo := &v1pb.Memo{ + Name: "memos/plainmemo1", + Content: "", + State: v1pb.State_NORMAL, + Visibility: v1pb.Visibility_PRIVATE, + } + + payload := marshalThroughGateway(t, memo) + + require.Equal(t, []any{}, payload["attachments"]) + require.Equal(t, []any{}, payload["relations"]) + require.Equal(t, "", payload["content"]) + require.Equal(t, false, payload["pinned"]) + // location is `optional` in the proto, so it is already omitted rather than + // null; property has no such marker and would otherwise be null here. + require.NotContains(t, payload, "location") + require.NotContains(t, payload, "property") +} + +func marshalThroughGateway(t *testing.T, message any) map[string]any { + t.Helper() + + data, err := newGatewayMarshaler().Marshal(message) + require.NoError(t, err) + + payload := map[string]any{} + require.NoError(t, json.Unmarshal(data, &payload)) + return payload +} diff --git a/server/router/api/v1/v1.go b/server/router/api/v1/v1.go index 4c6888cd..034d6f7d 100644 --- a/server/router/api/v1/v1.go +++ b/server/router/api/v1/v1.go @@ -9,6 +9,7 @@ import ( "github.com/labstack/echo/v5" "github.com/pkg/errors" "golang.org/x/sync/semaphore" + "google.golang.org/protobuf/encoding/protojson" "github.com/usememos/memos/internal/markdown" "github.com/usememos/memos/internal/profile" @@ -65,6 +66,29 @@ func NewAPIV1Service(secret string, profile *profile.Profile, store *store.Store } } +// newGatewayMarshaler mirrors grpc-gateway's default JSON marshaler with one +// change: EmitDefaultValues replaces EmitUnpopulated. Both keep proto3 scalar +// defaults ("" / 0 / false) and empty lists in the payload — the generated +// OpenAPI schema declares several of them required — but EmitUnpopulated also +// writes `null` for every unset message field (e.g. Attachment.motion_media on +// a plain image). No schema marks those fields nullable, so a client that +// validates responses against the spec rejects them; the MCP tools serve the +// same schema as their outputSchema, and strict MCP clients fail every call +// that returns an attachment. EmitDefaultValues omits unset message fields +// instead, which the schema already allows. +func newGatewayMarshaler() *runtime.HTTPBodyMarshaler { + return &runtime.HTTPBodyMarshaler{ + Marshaler: &runtime.JSONPb{ + MarshalOptions: protojson.MarshalOptions{ + EmitDefaultValues: true, + }, + UnmarshalOptions: protojson.UnmarshalOptions{ + DiscardUnknown: true, + }, + }, + } +} + // RegisterGateway registers the gRPC-Gateway and Connect handlers with the given Echo instance. func (s *APIV1Service) RegisterGateway(ctx context.Context, echoServer *echo.Echo) error { // Shared authorizer: one source of truth for authentication and anonymous-access @@ -108,6 +132,7 @@ func (s *APIV1Service) RegisterGateway(ctx context.Context, echoServer *echo.Ech // Create gRPC-Gateway mux with auth middleware. gwMux := runtime.NewServeMux( + runtime.WithMarshalerOption(runtime.MIMEWildcard, newGatewayMarshaler()), runtime.WithMiddlewares(gatewayAuthMiddleware), ) if err := v1pb.RegisterInstanceServiceHandlerServer(ctx, gwMux, s); err != nil { diff --git a/server/router/mcp/README.md b/server/router/mcp/README.md index 97b69520..dec2a0ce 100644 --- a/server/router/mcp/README.md +++ b/server/router/mcp/README.md @@ -198,6 +198,15 @@ not replace API authorization. This is deliberate: it fixes [#6022](https://github.com/usememos/memos/issues/6022), where collection tools returned a bare array that strict MCP clients reject. +Inside that envelope the API's JSON is passed through verbatim, so the gateway's +own encoding is part of the tool contract: whatever it emits is validated against +the output schema resolved from the same OpenAPI spec. grpc-gateway's stock +marshaler emits `null` for unset message fields, which no schema declares as +nullable — `RegisterGateway` therefore installs a marshaler that omits them +(`newGatewayMarshaler` in `server/router/api/v1/v1.go`). That fixes +[#6139](https://github.com/usememos/memos/issues/6139), where `"motionMedia": null` +failed every tool call returning an attachment. + ## Error handling Failures are returned as MCP tool errors (`CallToolResult` with `IsError: true` diff --git a/server/test/startup_test.go b/server/test/startup_test.go index 9a40ac05..373352a2 100644 --- a/server/test/startup_test.go +++ b/server/test/startup_test.go @@ -415,6 +415,48 @@ func TestStartupPrivateInstanceGatewayPolicy(t *testing.T) { "anonymous RSS should be unavailable on a private instance") } +// TestStartupGatewayOmitsNullMessageFields checks the JSON the gateway actually +// puts on the wire against the contract the generated OpenAPI schema publishes. +// grpc-gateway's stock marshaler emits `"motionMedia": null` for a plain image +// attachment; the schema types that field as an object, so clients validating +// responses against it — MCP clients serve that schema as the tool outputSchema +// — reject every memo carrying an attachment. The marshaler override lives in +// RegisterGateway, so only a booted server proves it is wired in. +func TestStartupGatewayOmitsNullMessageFields(t *testing.T) { + ctx := context.Background() + inst := bootInstance(ctx, t, instanceOptions{instanceURL: "http://localhost"}) + + inst.createAdmin(t) + token := inst.signIn(t) + + // A 1x1 GIF: small enough to inline, real enough for the type sniffing the + // attachment service runs on upload. + const onePixelGIF = "R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" + status, body := inst.do(t, http.MethodPost, "/api/v1/attachments", token, map[string]any{ + "filename": "pixel.gif", + "type": "image/gif", + "content": onePixelGIF, + }) + require.Equal(t, http.StatusOK, status, "creating an attachment should succeed: %s", body) + + var created struct { + Name string `json:"name"` + } + require.NoError(t, json.Unmarshal(body, &created)) + require.NotEmpty(t, created.Name, "the created attachment should carry a name: %s", body) + + status, body = inst.do(t, http.MethodGet, "/api/v1/"+created.Name, token, nil) + require.Equal(t, http.StatusOK, status, "reading the attachment should succeed: %s", body) + + fetched := map[string]any{} + require.NoError(t, json.Unmarshal(body, &fetched)) + require.NotContains(t, fetched, "motionMedia", "an unset message field must be omitted, not null: %s", body) + // Scalar defaults still ship, so schema-required fields stay present. + require.Equal(t, "", fetched["externalLink"]) + require.Equal(t, "pixel.gif", fetched["filename"]) + require.Equal(t, "image/gif", fetched["type"]) +} + // TestStartupDemoMode verifies demo mode boots, which exercises the seed path // in store.Migrate that prod-mode startups never touch. func TestStartupDemoMode(t *testing.T) {