Skip to content

fix(cliclient): path-escape server and tool names in per-server URLs #1751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Dumbris merged 1 commit into from
Oct 26, 2026
+74 −19
Merged

fix(cliclient): path-escape server and tool names in per-server URLs #1751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 15 additions & 19 deletions internal/cliclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,21 +607,17 @@ func (c *Client) GetServerLogs(ctx context.Context, serverName string, tail int)

// ServerAction performs an action on a server (enable, disable, restart).
func (c *Client) ServerAction(ctx context.Context, serverName, action string) error {
var url string
var reqURL string
method := http.MethodPost

switch action {
case "enable":
url = fmt.Sprintf("%s/api/v1/servers/%s/enable", c.baseURL, serverName)
case "disable":
url = fmt.Sprintf("%s/api/v1/servers/%s/disable", c.baseURL, serverName)
case "restart":
url = fmt.Sprintf("%s/api/v1/servers/%s/restart", c.baseURL, serverName)
case "enable", "disable", "restart":
reqURL = fmt.Sprintf("%s/api/v1/servers/%s/%s", c.baseURL, url.PathEscape(serverName), action)
default:
return fmt.Errorf("unknown action: %s", action)
}

req, err := http.NewRequestWithContext(ctx, method, url, nil)
req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
if err != nil {
return fmt.Errorf("failed to create request: %w", err)
}
Expand Down Expand Up @@ -1209,7 +1205,7 @@ func (c *Client) GetGlobalToolsView(ctx context.Context, query url.Values) ([]ma

// GetServerTools retrieves tools for a specific server from daemon.
func (c *Client) GetServerTools(ctx context.Context, serverName string) ([]map[string]interface{}, error) {
url := fmt.Sprintf("%s/api/v1/servers/%s/tools", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s/tools", c.baseURL, url.PathEscape(serverName))
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1256,7 +1252,7 @@ func (c *Client) GetServerTools(ctx context.Context, serverName string) ([]map[s
// quarantined or toggled before — see Runtime.SetToolEnabled.
func (c *Client) SetToolEnabled(ctx context.Context, serverName, toolName string, enabled bool) error {
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/%s/enabled",
c.baseURL, serverName, toolName)
c.baseURL, url.PathEscape(serverName), url.PathEscape(toolName))
body, err := json.Marshal(map[string]bool{"enabled": enabled})
if err != nil {
return fmt.Errorf("failed to marshal request: %w", err)
Expand Down Expand Up @@ -1304,7 +1300,7 @@ func (c *Client) SetAllToolsEnabled(ctx context.Context, serverName string, enab
if enabled {
path = "enable_all"
}
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/%s", c.baseURL, serverName, path)
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/%s", c.baseURL, url.PathEscape(serverName), path)
req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, nil)
if err != nil {
return 0, fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1367,7 +1363,7 @@ func (c *Client) TriggerOAuthLogin(ctx context.Context, serverName string) error
// the browser could not be opened (headless hosts, SSH sessions).
// Returns *contracts.OAuthFlowError for structured OAuth errors (Spec 020).
func (c *Client) TriggerOAuthLoginWithResult(ctx context.Context, serverName string) (*OAuthLoginResult, error) {
url := fmt.Sprintf("%s/api/v1/servers/%s/login", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s/login", c.baseURL, url.PathEscape(serverName))
req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1454,7 +1450,7 @@ func (c *Client) TriggerOAuthLoginWithResult(ctx context.Context, serverName str

// TriggerOAuthLogout clears OAuth token and disconnects a server.
func (c *Client) TriggerOAuthLogout(ctx context.Context, serverName string) error {
url := fmt.Sprintf("%s/api/v1/servers/%s/logout", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s/logout", c.baseURL, url.PathEscape(serverName))
req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, nil)
if err != nil {
return fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1597,7 +1593,7 @@ func (c *Client) AddServer(ctx context.Context, req *AddServerRequest) (*AddServ
// {"headers": {"X-Set": "v", "X-Old": null}} -- combined
// {"env": {"API_KEY": null}, "enabled": true} -- env delete + enable
func (c *Client) PatchServer(ctx context.Context, serverName string, body []byte) error {
url := fmt.Sprintf("%s/api/v1/servers/%s", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s", c.baseURL, url.PathEscape(serverName))
req, err := http.NewRequestWithContext(ctx, http.MethodPatch, url, bytes.NewReader(body))
if err != nil {
return fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1639,7 +1635,7 @@ func (c *Client) PatchServer(ctx context.Context, serverName string, body []byte

// RemoveServer removes an upstream server via the daemon API.
func (c *Client) RemoveServer(ctx context.Context, serverName string) error {
url := fmt.Sprintf("%s/api/v1/servers/%s", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s", c.baseURL, url.PathEscape(serverName))
req, err := http.NewRequestWithContext(ctx, http.MethodDelete, url, nil)
if err != nil {
return fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1865,7 +1861,7 @@ type ToolApprovalRecord struct {

// GetToolApprovals fetches tool approval records for a server (Spec 032).
func (c *Client) GetToolApprovals(ctx context.Context, serverName string) ([]ToolApprovalRecord, error) {
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/export", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/export", c.baseURL, url.PathEscape(serverName))
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1909,7 +1905,7 @@ func (c *Client) GetToolApprovals(ctx context.Context, serverName string) ([]Too

// GetToolDiff fetches the diff for a changed tool (Spec 032).
func (c *Client) GetToolDiff(ctx context.Context, serverName, toolName string) (*ToolApprovalRecord, error) {
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/%s/diff", c.baseURL, serverName, toolName)
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/%s/diff", c.baseURL, url.PathEscape(serverName), url.PathEscape(toolName))
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
Expand Down Expand Up @@ -1951,7 +1947,7 @@ func (c *Client) GetToolDiff(ctx context.Context, serverName, toolName string) (

// ApproveTools approves specific tools or all tools for a server (Spec 032).
func (c *Client) ApproveTools(ctx context.Context, serverName string, toolNames []string, approveAll bool) (int, error) {
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/approve", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/approve", c.baseURL, url.PathEscape(serverName))

reqBody := struct {
Tools []string `json:"tools,omitempty"`
Expand Down Expand Up @@ -2012,7 +2008,7 @@ func (c *Client) ApproveTools(ctx context.Context, serverName string, toolNames
// (Spec 032). Block atomically approves AND disables the tool so it is never
// left in the approved+enabled state — mirroring the Web UI "Block" action.
func (c *Client) BlockTools(ctx context.Context, serverName string, toolNames []string, blockAll bool) (int, error) {
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/block", c.baseURL, serverName)
url := fmt.Sprintf("%s/api/v1/servers/%s/tools/block", c.baseURL, url.PathEscape(serverName))

reqBody := struct {
Tools []string `json:"tools,omitempty"`
Expand Down
59 changes: 59 additions & 0 deletions internal/cliclient/server_path_escape_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package cliclient

import (
"context"
"net/http"
"net/http/httptest"
"testing"

"go.uber.org/zap"
)

// Official-registry server ids are namespace/name (io.github.owner/repo). The
// daemon decodes a %2F-encoded {id} (MCP-1118), but only if the client sends
// one: an unescaped slash splits the name across path segments and the request
// lands on a different route.
func TestPerServerRequests_EscapeServerAndToolNames(t *testing.T) {
const (
server = "io.github.owner/repo"
srvEnc = "io.github.owner%2Frepo"
tool = "ns/erase"
tlEnc = "ns%2Ferase"
)

var got string
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
got = r.URL.EscapedPath()
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"success":true,"data":{}}`))
}))
defer ts.Close()

c := NewClient(ts.URL, zap.NewNop().Sugar())
ctx := context.Background()

for _, tc := range []struct {
name string
call func()
want string
}{
{"enable", func() { _ = c.ServerAction(ctx, server, "enable") }, "/api/v1/servers/" + srvEnc + "/enable"},
{"restart", func() { _ = c.ServerAction(ctx, server, "restart") }, "/api/v1/servers/" + srvEnc + "/restart"},
{"tools", func() { _, _ = c.GetServerTools(ctx, server) }, "/api/v1/servers/" + srvEnc + "/tools"},
{"tool enabled", func() { _ = c.SetToolEnabled(ctx, server, tool, false) }, "/api/v1/servers/" + srvEnc + "/tools/" + tlEnc + "/enabled"},
{"all tools", func() { _, _ = c.SetAllToolsEnabled(ctx, server, true) }, "/api/v1/servers/" + srvEnc + "/tools/enable_all"},
{"logout", func() { _ = c.TriggerOAuthLogout(ctx, server) }, "/api/v1/servers/" + srvEnc + "/logout"},
{"patch", func() { _ = c.PatchServer(ctx, server, []byte(`{}`)) }, "/api/v1/servers/" + srvEnc},
{"remove", func() { _ = c.RemoveServer(ctx, server) }, "/api/v1/servers/" + srvEnc},
{"approvals", func() { _, _ = c.GetToolApprovals(ctx, server) }, "/api/v1/servers/" + srvEnc + "/tools/export"},
{"diff", func() { _, _ = c.GetToolDiff(ctx, server, tool) }, "/api/v1/servers/" + srvEnc + "/tools/" + tlEnc + "/diff"},
{"approve", func() { _, _ = c.ApproveTools(ctx, server, nil, true) }, "/api/v1/servers/" + srvEnc + "/tools/approve"},
{"block", func() { _, _ = c.BlockTools(ctx, server, nil, true) }, "/api/v1/servers/" + srvEnc + "/tools/block"},
} {
got = ""
tc.call()
if got != tc.want {
t.Errorf("%s: request path = %q, want %q", tc.name, got, tc.want)
}
}
}
Loading