diff --git a/docs/api/README.md b/docs/api/README.md index 7bc223a..fd896fe 100644 --- a/docs/api/README.md +++ b/docs/api/README.md @@ -95,6 +95,12 @@ Same logical operation, opposite id semantics. Also, MCP `list` omits `userVerification`, `agentForward`, and `isDemo` that REST returns. Two clients build different mental models of "an endpoint." +Related, now resolved in both backends: Node's MCP tool schema used to +silently strip `userVerification`/`agentForward` from create/update `data`, so +endpoints were not fully editable via MCP. That was a bug, not the contract — +create/update accept and validate both fields (see +[`mcp-tools.md`](./mcp-tools.md)). + ### D. Near-identical WS message names differ by one character / tense `terminal:close` (client→server, _do close_) vs. `terminal:closed` diff --git a/docs/api/mcp-tools.md b/docs/api/mcp-tools.md index 1d36ef5..e18f82c 100644 --- a/docs/api/mcp-tools.md +++ b/docs/api/mcp-tools.md @@ -27,9 +27,19 @@ Manage SSH endpoints. Input: ```ts { action: "list" | "read" | "create" | "update" | "delete"; id?: string; // required for read/update/delete - data?: { label?; host?; port?; username?; description?: string|null } } + data?: { label?; host?; port?; username?; + userVerification?: "required" | "preferred" | "discouraged"; + agentForward?: boolean; + description?: string|null /* ≤1000 chars; null clears */ } } ``` +Every REST-editable field is editable here too. `create` defaults: +`userVerification: "required"`, `agentForward: true`, `port: 22`. Invalid +values (unknown `userVerification`, wrong types, over-long `description`) +are rejected without writing. _(Node's tool schema used to silently strip +`userVerification`/`agentForward` — that was a bug, not the contract; both +backends now accept them.)_ + Per-action success payload (JSON in the text block): - `list` → `{ endpoints: [{ id, label, host, port, username, description }] }` diff --git a/internal/agent/session.go b/internal/agent/session.go index 89a7927..747622c 100644 --- a/internal/agent/session.go +++ b/internal/agent/session.go @@ -104,38 +104,50 @@ func (s *Session) CreateEndpoint(ctx context.Context, ep store.Endpoint) error { return s.deps.Endpoints.Create(ctx, ep) } -// UpdateEndpoint read-merges a patch into an account-scoped endpoint and writes -// it back. Returns false when no endpoint matched. Unknown keys are ignored. -func (s *Session) UpdateEndpoint(ctx context.Context, id string, patch map[string]any) (bool, error) { +// EndpointPatch is a typed partial endpoint update; nil fields keep the +// stored value. Description carries an explicit set flag so a JSON null can +// clear it. +type EndpointPatch struct { + Label *string + Host *string + Port *int64 + Username *string + UserVerification *string + AgentForward *bool + Description *string // applied only when DescriptionSet; nil clears + DescriptionSet bool +} + +// UpdateEndpoint read-merges a typed patch into an account-scoped endpoint and +// writes it back. Returns false when no endpoint matched. Wire-shape decoding +// and field validation (userVerification enum, description cap) happen at the +// caller (internal/mcp). +func (s *Session) UpdateEndpoint(ctx context.Context, id string, patch EndpointPatch) (bool, error) { existing, err := s.deps.Endpoints.GetForAccount(ctx, id, s.accountID) if err != nil || existing == nil { return false, err } merged := *existing - if v, ok := patch["label"].(string); ok { - merged.Label = v + if patch.Label != nil { + merged.Label = *patch.Label } - if v, ok := patch["host"].(string); ok { - merged.Host = v + if patch.Host != nil { + merged.Host = *patch.Host } - if v, ok := patch["username"].(string); ok { - merged.Username = v + if patch.Port != nil { + merged.Port = *patch.Port } - if v, ok := patch["userVerification"].(string); ok { - merged.UserVerification = v + if patch.Username != nil { + merged.Username = *patch.Username } - if v, ok := patch["port"].(float64); ok { // JSON numbers decode as float64 - merged.Port = int64(v) + if patch.UserVerification != nil { + merged.UserVerification = *patch.UserVerification } - if v, ok := patch["agentForward"].(bool); ok { - merged.AgentForward = v + if patch.AgentForward != nil { + merged.AgentForward = *patch.AgentForward } - if v, ok := patch["description"]; ok { - if s2, isStr := v.(string); isStr { - merged.Description = &s2 - } else if v == nil { - merged.Description = nil - } + if patch.DescriptionSet { + merged.Description = patch.Description } return s.deps.Endpoints.Update(ctx, merged) } diff --git a/internal/agent/session_endpoint_test.go b/internal/agent/session_endpoint_test.go index 9a11750..73113c3 100644 --- a/internal/agent/session_endpoint_test.go +++ b/internal/agent/session_endpoint_test.go @@ -33,7 +33,8 @@ func TestSessionEndpointMutations(t *testing.T) { } // Update (partial patch merges). - ok, err := sess.UpdateEndpoint(ctx, "ep1", map[string]any{"label": "Renamed", "port": float64(2222), "agentForward": true}) + label, port, fwd := "Renamed", int64(2222), true + ok, err := sess.UpdateEndpoint(ctx, "ep1", EndpointPatch{Label: &label, Port: &port, AgentForward: &fwd}) if err != nil || !ok { t.Fatalf("update: ok=%v err=%v", ok, err) } @@ -42,8 +43,25 @@ func TestSessionEndpointMutations(t *testing.T) { t.Fatalf("update merge wrong: %+v", ep) } + // Full editability: userVerification and description are patchable too. + uv, desc := "discouraged", "jump host" + if ok, err := sess.UpdateEndpoint(ctx, "ep1", EndpointPatch{UserVerification: &uv, Description: &desc, DescriptionSet: true}); err != nil || !ok { + t.Fatalf("uv/desc update: ok=%v err=%v", ok, err) + } + ep, _ = sess.GetEndpoint(ctx, "ep1") + if ep.UserVerification != "discouraged" || ep.Description == nil || *ep.Description != "jump host" { + t.Fatalf("uv/desc merge wrong: %+v", ep) + } + // DescriptionSet with nil clears; unset leaves it alone. + if ok, err := sess.UpdateEndpoint(ctx, "ep1", EndpointPatch{DescriptionSet: true}); err != nil || !ok { + t.Fatalf("desc clear: ok=%v err=%v", ok, err) + } + if ep, _ = sess.GetEndpoint(ctx, "ep1"); ep.Description != nil { + t.Fatalf("description should be cleared: %+v", ep) + } + // Update a missing endpoint -> false. - if ok, _ := sess.UpdateEndpoint(ctx, "nope", map[string]any{"label": "x"}); ok { + if ok, _ := sess.UpdateEndpoint(ctx, "nope", EndpointPatch{Label: &label}); ok { t.Error("update of missing endpoint should return false") } // Cross-account isolation: another account can't touch ep1. diff --git a/internal/httpserver/mcp_test.go b/internal/httpserver/mcp_test.go index 1e5dd7e..137f8a8 100644 --- a/internal/httpserver/mcp_test.go +++ b/internal/httpserver/mcp_test.go @@ -175,3 +175,79 @@ func TestMCPToolGoldens(t *testing.T) { map[string]any{"sessionId": sid, "keys": []string{"text:echo hi", "enter"}}) assertToolGolden(t, "mcp-close-session", "shellwatch_close_session", sess, map[string]any{"sessionId": sid}) } + +// Full endpoint editability via MCP: create + update accept every +// REST-editable field, with the same validation as REST. Not golden-pinned — +// these flows postdate the golden freeze (both backends changed together). +func TestMCPEndpointFullEdit(t *testing.T) { + ts := mcpServer(t) + sess := mcpConnect(t, ts) + + text, isErr := callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{ + "action": "create", "id": "edit-me", + "data": map[string]any{ + "label": "Edit Me", "host": "10.0.0.1", "username": "ops", + "userVerification": "preferred", "agentForward": false, "description": "staging box", + }, + }) + if isErr { + t.Fatalf("create: %s", text) + } + + text, isErr = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{ + "action": "update", "id": "edit-me", + "data": map[string]any{"userVerification": "discouraged", "agentForward": true, "description": nil}, + }) + if isErr { + t.Fatalf("update: %s", text) + } + + text, isErr = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{"action": "read", "id": "edit-me"}) + if isErr { + t.Fatalf("read: %s", text) + } + var ep struct { + UserVerification string `json:"userVerification"` + AgentForward bool `json:"agentForward"` + Description *string `json:"description"` + } + if err := json.Unmarshal([]byte(text), &ep); err != nil { + t.Fatal(err) + } + if ep.UserVerification != "discouraged" || !ep.AgentForward || ep.Description != nil { + t.Fatalf("update not applied: %+v", ep) + } + + // Invalid enum is rejected — whether by the tool handler (isError) or by + // SDK-side schema validation (protocol error), the write must not land. + res, err := sess.CallTool(context.Background(), &mcpsdk.CallToolParams{ + Name: "shellwatch_manage_endpoints", + Arguments: map[string]any{ + "action": "update", "id": "edit-me", + "data": map[string]any{"userVerification": "none"}, + }, + }) + if err == nil && !res.IsError { + t.Error("invalid userVerification was accepted") + } + text, _ = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{"action": "read", "id": "edit-me"}) + _ = json.Unmarshal([]byte(text), &ep) + if ep.UserVerification != "discouraged" { + t.Errorf("invalid update mutated the row: %+v", ep) + } + + // Create defaults match REST/Node: userVerification required, agentForward true. + if text, isErr := callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{ + "action": "create", "id": "defaults", + "data": map[string]any{"label": "D", "host": "10.0.0.2", "username": "ops"}, + }); isErr { + t.Fatalf("create defaults: %s", text) + } + text, _ = callTool(t, sess, "shellwatch_manage_endpoints", map[string]any{"action": "read", "id": "defaults"}) + if err := json.Unmarshal([]byte(text), &ep); err != nil { + t.Fatal(err) + } + if ep.UserVerification != "required" || !ep.AgentForward { + t.Errorf("create defaults wrong: %+v", ep) + } +} diff --git a/internal/mcp/tools_endpoints.go b/internal/mcp/tools_endpoints.go index efd00bb..d987e41 100644 --- a/internal/mcp/tools_endpoints.go +++ b/internal/mcp/tools_endpoints.go @@ -2,11 +2,15 @@ // Endpoint + key management tools (port of src/mcp/tools/endpoints.ts, keys.ts). // manage_endpoints list omits userVerification/agentForward/isDemo (contract // item C); read returns the full row. create requires a caller-supplied id -// (item C — opposite of REST). +// (item C — opposite of REST). create/update accept userVerification and +// agentForward — full endpoint editability via MCP is the contract +// (docs/api/mcp-tools.md); Node's zod schema stripped them until the same +// change landed there. package mcp import ( "context" + "strings" mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" @@ -17,6 +21,90 @@ import ( const demoReadOnlyErr = "Demo endpoints are read-only" +// Same limits the REST handlers enforce (rest/endpoints.go); duplicated until +// a shared endpoint service exists. +const endpointDescriptionMaxLen = 1000 + +var userVerificationValues = []string{"required", "preferred", "discouraged"} + +func isUserVerification(v string) bool { + for _, u := range userVerificationValues { + if u == v { + return true + } + } + return false +} + +// endpointPatchFromWire converts the tool's raw data object into a typed +// patch, validating field types, the userVerification enum, and the +// description cap (the zod-equivalent layer; Node validates in the tool +// schema). Returns a non-empty message when a field is invalid. +func endpointPatchFromWire(data map[string]any) (agent.EndpointPatch, string) { + var p agent.EndpointPatch + strField := func(key string, dst **string) string { + v, ok := data[key] + if !ok { + return "" + } + s, isStr := v.(string) + if !isStr { + return "data." + key + " must be a string" + } + *dst = &s + return "" + } + if msg := strField("label", &p.Label); msg != "" { + return p, msg + } + if msg := strField("host", &p.Host); msg != "" { + return p, msg + } + if msg := strField("username", &p.Username); msg != "" { + return p, msg + } + if v, ok := data["port"]; ok { + n, isNum := v.(float64) // JSON numbers decode as float64 + if !isNum { + return p, "data.port must be a number" + } + port := int64(n) + p.Port = &port + } + if v, ok := data["userVerification"]; ok { + s, isStr := v.(string) + if !isStr || !isUserVerification(s) { + return p, "data.userVerification must be one of: " + strings.Join(userVerificationValues, ", ") + } + p.UserVerification = &s + } + if v, ok := data["agentForward"]; ok { + b, isBool := v.(bool) + if !isBool { + return p, "data.agentForward must be a boolean" + } + p.AgentForward = &b + } + if v, present := data["description"]; present { + p.DescriptionSet = true + if v != nil { + s, isStr := v.(string) + if !isStr || len(s) > endpointDescriptionMaxLen { + return p, "data.description must be a string up to 1000 characters (pass null to clear)" + } + p.Description = &s + } + } + return p, "" +} + +func strOrEmpty(p *string) string { + if p == nil { + return "" + } + return *p +} + func registerEndpointTools(srv *mcpsdk.Server, as *agent.Session) { srv.AddTool(&mcpsdk.Tool{ Name: "shellwatch_manage_endpoints", @@ -24,7 +112,28 @@ func registerEndpointTools(srv *mcpsdk.Server, as *agent.Session) { InputSchema: objSchema(map[string]any{ "action": map[string]any{"type": "string", "enum": []string{"list", "read", "create", "update", "delete"}}, "id": map[string]any{"type": "string"}, - "data": map[string]any{"type": "object"}, + "data": map[string]any{ + "type": "object", + "description": "Endpoint fields (for create and update)", + "properties": map[string]any{ + "label": map[string]any{"type": "string"}, + "host": map[string]any{"type": "string"}, + "port": map[string]any{"type": "number"}, + "username": map[string]any{"type": "string"}, + "userVerification": map[string]any{ + "type": "string", "enum": userVerificationValues, + "description": "WebAuthn user-verification policy for passkey signing (create default: required)", + }, + "agentForward": map[string]any{ + "type": "boolean", + "description": "Offer SSH agent forwarding to the remote host (create default: true)", + }, + "description": map[string]any{ + "type": []string{"string", "null"}, + "description": "Free-form context (max 1000 chars) shown to agents on connect. Pass null to clear.", + }, + }, + }, }, "action"), }, func(ctx context.Context, req *mcpsdk.CallToolRequest) (*mcpsdk.CallToolResult, error) { var args struct { @@ -65,23 +174,28 @@ func registerEndpointTools(srv *mcpsdk.Server, as *agent.Session) { if demo.IsID(args.ID) { return errResult(demoReadOnlyErr), nil } - label, _ := args.Data["label"].(string) - host, _ := args.Data["host"].(string) - username, _ := args.Data["username"].(string) - if args.ID == "" || label == "" || host == "" || username == "" { + patch, msg := endpointPatchFromWire(args.Data) + if msg != "" { + return errResult(msg), nil + } + if args.ID == "" || strOrEmpty(patch.Label) == "" || strOrEmpty(patch.Host) == "" || strOrEmpty(patch.Username) == "" { return errResult("id, data.label, data.host, data.username are required"), nil } - port := int64(22) - if p, ok := args.Data["port"].(float64); ok { - port = int64(p) + // Defaults match the Node repo (endpoint-repo.ts create): + // userVerification "required", agentForward true, port 22. + ep := store.Endpoint{ + ID: args.ID, Label: *patch.Label, Host: *patch.Host, Port: 22, + Username: *patch.Username, UserVerification: "required", AgentForward: true, + Description: patch.Description, } - var desc *string - if d, ok := args.Data["description"].(string); ok { - desc = &d + if patch.Port != nil { + ep.Port = *patch.Port } - ep := store.Endpoint{ - ID: args.ID, Label: label, Host: host, Port: port, Username: username, - UserVerification: "required", Description: desc, + if patch.UserVerification != nil { + ep.UserVerification = *patch.UserVerification + } + if patch.AgentForward != nil { + ep.AgentForward = *patch.AgentForward } if err := as.CreateEndpoint(ctx, ep); err != nil { return errResult(err.Error()), nil @@ -94,7 +208,11 @@ func registerEndpointTools(srv *mcpsdk.Server, as *agent.Session) { if args.ID == "" || args.Data == nil { return errResult("id and data are required"), nil } - ok, err := as.UpdateEndpoint(ctx, args.ID, args.Data) + patch, msg := endpointPatchFromWire(args.Data) + if msg != "" { + return errResult(msg), nil + } + ok, err := as.UpdateEndpoint(ctx, args.ID, patch) if err != nil { return errResult(err.Error()), nil } diff --git a/internal/mcp/tools_endpoints_test.go b/internal/mcp/tools_endpoints_test.go new file mode 100644 index 0000000..83d1f9e --- /dev/null +++ b/internal/mcp/tools_endpoints_test.go @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: LicenseRef-FSL-1.1-Apache-2.0 +package mcp + +import ( + "strings" + "testing" +) + +func TestEndpointPatchFromWire(t *testing.T) { + p, msg := endpointPatchFromWire(map[string]any{ + "label": "L", "host": "h", "username": "u", "port": float64(2222), + "userVerification": "preferred", "agentForward": false, "description": "d", + }) + if msg != "" { + t.Fatalf("valid patch rejected: %s", msg) + } + if *p.Label != "L" || *p.Host != "h" || *p.Username != "u" || *p.Port != 2222 { + t.Fatalf("basic fields wrong: %+v", p) + } + if *p.UserVerification != "preferred" || *p.AgentForward != false { + t.Fatalf("uv/forward wrong: %+v", p) + } + if !p.DescriptionSet || p.Description == nil || *p.Description != "d" { + t.Fatalf("description wrong: %+v", p) + } + + // Absent fields stay nil; a JSON null description means "clear". + p, msg = endpointPatchFromWire(map[string]any{"description": nil}) + if msg != "" || !p.DescriptionSet || p.Description != nil { + t.Fatalf("null description: msg=%q patch=%+v", msg, p) + } + if p.Label != nil || p.UserVerification != nil || p.AgentForward != nil { + t.Fatalf("absent fields must be nil: %+v", p) + } + + for _, tc := range []struct { + name string + data map[string]any + want string + }{ + {"uv unknown value", map[string]any{"userVerification": "none"}, + "data.userVerification must be one of: required, preferred, discouraged"}, + {"uv wrong type", map[string]any{"userVerification": true}, + "data.userVerification must be one of: required, preferred, discouraged"}, + {"agentForward string", map[string]any{"agentForward": "yes"}, + "data.agentForward must be a boolean"}, + {"port string", map[string]any{"port": "22"}, + "data.port must be a number"}, + {"label number", map[string]any{"label": float64(5)}, + "data.label must be a string"}, + {"description too long", map[string]any{"description": strings.Repeat("x", 1001)}, + "data.description must be a string up to 1000 characters (pass null to clear)"}, + {"description wrong type", map[string]any{"description": float64(1)}, + "data.description must be a string up to 1000 characters (pass null to clear)"}, + } { + if _, msg := endpointPatchFromWire(tc.data); msg != tc.want { + t.Errorf("%s: got %q want %q", tc.name, msg, tc.want) + } + } +} diff --git a/src/mcp/server.test.ts b/src/mcp/server.test.ts index 2ff6492..f69f90b 100644 --- a/src/mcp/server.test.ts +++ b/src/mcp/server.test.ts @@ -112,6 +112,60 @@ describe("MCP Server Tools", () => { expect(parsed.endpoints[0].id).toBe("dev-box"); expect(parsed.endpoints[0].privateKeyPath).toBeUndefined(); }); + + // Full editability: userVerification/agentForward are settable on create + // and update (they used to be stripped by the tool schema). + it("creates and updates userVerification/agentForward", async () => { + const client = await setupClient(mockManager); + const create = await client.callTool({ + name: "shellwatch_manage_endpoints", + arguments: { + action: "create", + id: "edit-me", + data: { + label: "Edit Me", + host: "10.0.0.1", + username: "ops", + userVerification: "preferred", + agentForward: false, + }, + }, + }); + expect(create.isError).toBeFalsy(); + + const update = await client.callTool({ + name: "shellwatch_manage_endpoints", + arguments: { + action: "update", + id: "edit-me", + data: { userVerification: "discouraged", agentForward: true }, + }, + }); + expect(update.isError).toBeFalsy(); + + const read = await client.callTool({ + name: "shellwatch_manage_endpoints", + arguments: { action: "read", id: "edit-me" }, + }); + const ep = JSON.parse((read.content as { type: string; text: string }[])[0].text); + expect(ep.userVerification).toBe("discouraged"); + expect(ep.agentForward).toBe(true); + + // Unknown enum values are rejected (schema validation), not written. + const bad = await client + .callTool({ + name: "shellwatch_manage_endpoints", + arguments: { action: "update", id: "edit-me", data: { userVerification: "none" } }, + }) + .catch(() => ({ isError: true })); + expect(bad.isError).toBe(true); + const reread = await client.callTool({ + name: "shellwatch_manage_endpoints", + arguments: { action: "read", id: "edit-me" }, + }); + const ep2 = JSON.parse((reread.content as { type: string; text: string }[])[0].text); + expect(ep2.userVerification).toBe("discouraged"); + }); }); // Demo-endpoint visibility/mutation behavior via MCP. Locks in the contract diff --git a/src/mcp/tools/endpoints.ts b/src/mcp/tools/endpoints.ts index 103e5dc..5c6671b 100644 --- a/src/mcp/tools/endpoints.ts +++ b/src/mcp/tools/endpoints.ts @@ -34,6 +34,16 @@ export function registerEndpointTools(mcpServer: McpServer, deps: EndpointToolDe host: z.string().optional(), port: z.number().optional(), username: z.string().optional(), + userVerification: z + .enum(["required", "preferred", "discouraged"]) + .optional() + .describe( + "WebAuthn user-verification policy for passkey signing (create default: required)", + ), + agentForward: z + .boolean() + .optional() + .describe("Offer SSH agent forwarding to the remote host (create default: true)"), description: z .string() .max(ENDPOINT_DESCRIPTION_MAX_LENGTH) @@ -101,6 +111,8 @@ export function registerEndpointTools(mcpServer: McpServer, deps: EndpointToolDe host: data.host, port: data.port ?? 22, username: data.username, + userVerification: data.userVerification, + agentForward: data.agentForward, description: data.description ?? null, }); return { content: [{ type: "text", text: JSON.stringify({ status: "created", id }) }] };