From f71645753fb8429356354b1354a58c439580d9bc Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sat, 2 May 2026 10:22:01 -0400 Subject: [PATCH] feat(config): resolve MCP url through shell expansion m.URL now runs through the same resolver as command, args, env, and headers so $VAR / $(cmd) work in http and sse endpoints. The empty-url guard runs after resolution so ${X:-} still fails cleanly, and failing expansions surface via the existing StateError path. --- internal/agent/tools/mcp/init.go | 16 +++- internal/agent/tools/mcp/init_test.go | 96 ++++++++++++++++++++++++ internal/config/config.go | 22 ++++++ internal/config/mcp_resolved_url_test.go | 88 ++++++++++++++++++++++ 4 files changed, 218 insertions(+), 4 deletions(-) create mode 100644 internal/config/mcp_resolved_url_test.go diff --git a/internal/agent/tools/mcp/init.go b/internal/agent/tools/mcp/init.go index 7dd90c37206a018ae173e5e0d977cb7c4e46b4c7..7284bb063789baa6ec18f488583f781d9ddd5cc5 100644 --- a/internal/agent/tools/mcp/init.go +++ b/internal/agent/tools/mcp/init.go @@ -461,7 +461,11 @@ func createTransport(ctx context.Context, m config.MCPConfig, resolver config.Va Command: cmd, }, nil case config.MCPHttp: - if strings.TrimSpace(m.URL) == "" { + url, err := m.ResolvedURL(resolver) + if err != nil { + return nil, err + } + if strings.TrimSpace(url) == "" { return nil, fmt.Errorf("mcp http config requires a non-empty 'url' field") } headers, err := m.ResolvedHeaders(resolver) @@ -474,11 +478,15 @@ func createTransport(ctx context.Context, m config.MCPConfig, resolver config.Va }, } return &mcp.StreamableClientTransport{ - Endpoint: m.URL, + Endpoint: url, HTTPClient: client, }, nil case config.MCPSSE: - if strings.TrimSpace(m.URL) == "" { + url, err := m.ResolvedURL(resolver) + if err != nil { + return nil, err + } + if strings.TrimSpace(url) == "" { return nil, fmt.Errorf("mcp sse config requires a non-empty 'url' field") } headers, err := m.ResolvedHeaders(resolver) @@ -491,7 +499,7 @@ func createTransport(ctx context.Context, m config.MCPConfig, resolver config.Va }, } return &mcp.SSEClientTransport{ - Endpoint: m.URL, + Endpoint: url, HTTPClient: client, }, nil default: diff --git a/internal/agent/tools/mcp/init_test.go b/internal/agent/tools/mcp/init_test.go index 94958593750852d30ff96734ada23671252e508e..a2d04484c2c7dd6d8a8f1cef7a25ce1ac9079fe1 100644 --- a/internal/agent/tools/mcp/init_test.go +++ b/internal/agent/tools/mcp/init_test.go @@ -4,6 +4,8 @@ import ( "context" "testing" + "github.com/charmbracelet/crush/internal/config" + "github.com/charmbracelet/crush/internal/env" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/require" "go.uber.org/goleak" @@ -36,3 +38,97 @@ func TestMCPSession_CancelOnClose(t *testing.T) { // After Close, the context must be cancelled. require.ErrorIs(t, ctx.Err(), context.Canceled) } + +// TestCreateTransport_URLResolution pins that m.URL goes through the +// same resolver seam as command, args, env, and headers. Covers both +// the HTTP and SSE branches, success and failure, so a regression in +// ResolvedURL wiring is caught at the transport layer rather than only +// at the config layer. +func TestCreateTransport_URLResolution(t *testing.T) { + t.Parallel() + + shell := config.NewShellVariableResolver(env.NewFromMap(map[string]string{ + "MCP_HOST": "mcp.example.com", + })) + + t.Run("http success expands $VAR", func(t *testing.T) { + t.Parallel() + m := config.MCPConfig{ + Type: config.MCPHttp, + URL: "https://$MCP_HOST/api", + } + tr, err := createTransport(t.Context(), m, shell) + require.NoError(t, err) + require.NotNil(t, tr) + sct, ok := tr.(*mcp.StreamableClientTransport) + require.True(t, ok, "expected StreamableClientTransport, got %T", tr) + require.Equal(t, "https://mcp.example.com/api", sct.Endpoint) + }) + + t.Run("sse success expands $(cmd)", func(t *testing.T) { + t.Parallel() + m := config.MCPConfig{ + Type: config.MCPSSE, + URL: "https://$(echo mcp.example.com)/events", + } + tr, err := createTransport(t.Context(), m, shell) + require.NoError(t, err) + sse, ok := tr.(*mcp.SSEClientTransport) + require.True(t, ok, "expected SSEClientTransport, got %T", tr) + require.Equal(t, "https://mcp.example.com/events", sse.Endpoint) + }) + + t.Run("http unset var surfaces error, no transport created", func(t *testing.T) { + t.Parallel() + m := config.MCPConfig{ + Type: config.MCPHttp, + URL: "https://$MCP_MISSING_HOST/api", + } + tr, err := createTransport(t.Context(), m, shell) + require.Error(t, err) + require.Nil(t, tr) + require.Contains(t, err.Error(), "url:") + require.Contains(t, err.Error(), "$MCP_MISSING_HOST") + }) + + t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) { + t.Parallel() + m := config.MCPConfig{ + Type: config.MCPSSE, + URL: "https://$(false)/events", + } + tr, err := createTransport(t.Context(), m, shell) + require.Error(t, err) + require.Nil(t, tr) + require.Contains(t, err.Error(), "url:") + require.Contains(t, err.Error(), "$(false)") + }) + + t.Run("http empty-after-resolve still fails the non-empty guard", func(t *testing.T) { + t.Parallel() + // ${MCP_EMPTY:-} resolves to the empty string (no error), + // then the existing TrimSpace guard in createTransport must + // reject it so we never spawn a transport against "". + m := config.MCPConfig{ + Type: config.MCPHttp, + URL: "${MCP_EMPTY:-}", + } + tr, err := createTransport(t.Context(), m, shell) + require.Error(t, err) + require.Nil(t, tr) + require.Contains(t, err.Error(), "non-empty 'url'") + }) + + t.Run("identity resolver round-trips template verbatim", func(t *testing.T) { + t.Parallel() + // Client mode forwards the template to the server; no local + // expansion, no error on unset vars. + tmpl := "https://$MCP_MISSING_HOST/api" + m := config.MCPConfig{Type: config.MCPHttp, URL: tmpl} + tr, err := createTransport(t.Context(), m, config.IdentityResolver()) + require.NoError(t, err) + sct, ok := tr.(*mcp.StreamableClientTransport) + require.True(t, ok) + require.Equal(t, tmpl, sct.Endpoint) + }) +} diff --git a/internal/config/config.go b/internal/config/config.go index 40db4aa68890b68f8199ce89b03280ea61257f99..78aafbfdbf48e39b791824fd56bfb32c2b8c6439 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -339,6 +339,28 @@ func (m MCPConfig) ResolvedArgs(r VariableResolver) ([]string, error) { return out, nil } +// ResolvedURL returns m.URL expanded through the given resolver. The +// receiver is not mutated. Errors from the resolver are already +// sanitized by ResolveValue and are wrapped with %w for errors.Is/As. +// +// URLs run through the same shell-expansion pipeline as the other +// fields, so a literal '$' (e.g. OData query strings containing +// $filter/$select) must be escaped as '\$' or '${DOLLAR:-$}' to avoid +// being interpreted as a variable reference. Same constraint already +// applies to command, args, env, and headers. +// +// See ResolvedEnv for guidance on picking a resolver. +func (m MCPConfig) ResolvedURL(r VariableResolver) (string, error) { + if m.URL == "" { + return "", nil + } + v, err := r.ResolveValue(m.URL) + if err != nil { + return "", fmt.Errorf("url: %w", err) + } + return v, nil +} + // ResolvedHeaders returns m.Headers with every value expanded through // the given resolver. A fresh map is allocated; m.Headers is never // mutated. On the first resolution failure it returns nil and an error diff --git a/internal/config/mcp_resolved_url_test.go b/internal/config/mcp_resolved_url_test.go new file mode 100644 index 0000000000000000000000000000000000000000..97c0d9df4725164a0ca05933da5df35239fdb691 --- /dev/null +++ b/internal/config/mcp_resolved_url_test.go @@ -0,0 +1,88 @@ +package config + +import ( + "errors" + "testing" + + "github.com/charmbracelet/crush/internal/env" + "github.com/stretchr/testify/require" +) + +func TestMCPConfig_ResolvedURL(t *testing.T) { + t.Parallel() + + t.Run("empty url short-circuits without calling resolver", func(t *testing.T) { + t.Parallel() + m := MCPConfig{Type: MCPHttp} + got, err := m.ResolvedURL(stubResolver{err: errors.New("should not be called")}) + require.NoError(t, err) + require.Empty(t, got) + }) + + t.Run("literal url passes through unchanged", func(t *testing.T) { + t.Parallel() + m := MCPConfig{Type: MCPHttp, URL: "https://mcp.example.com/api"} + got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil))) + require.NoError(t, err) + require.Equal(t, "https://mcp.example.com/api", got) + }) + + t.Run("expands $VAR with shell resolver", func(t *testing.T) { + t.Parallel() + m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_HOST/api"} + r := NewShellVariableResolver(env.NewFromMap(map[string]string{"MCP_HOST": "mcp.example.com"})) + got, err := m.ResolvedURL(r) + require.NoError(t, err) + require.Equal(t, "https://mcp.example.com/api", got) + }) + + t.Run("expands $(cmd) with shell resolver", func(t *testing.T) { + t.Parallel() + m := MCPConfig{Type: MCPSSE, URL: "https://$(echo mcp.example.com)/events"} + got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil))) + require.NoError(t, err) + require.Equal(t, "https://mcp.example.com/events", got) + }) + + t.Run("unset var is an error wrapping the template", func(t *testing.T) { + t.Parallel() + m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"} + _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil))) + require.Error(t, err) + require.Contains(t, err.Error(), "url:") + require.Contains(t, err.Error(), "$MCP_MISSING_HOST") + require.Contains(t, err.Error(), "unbound") + }) + + t.Run("failing command substitution is an error", func(t *testing.T) { + t.Parallel() + m := MCPConfig{Type: MCPHttp, URL: "https://$(false)/api"} + _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil))) + require.Error(t, err) + require.Contains(t, err.Error(), "url:") + require.Contains(t, err.Error(), "$(false)") + }) + + t.Run("identity resolver round-trips template verbatim", func(t *testing.T) { + t.Parallel() + // In client mode expansion happens server-side; the client must + // forward the template without touching it and without erroring + // on unset vars. + tmpl := "https://$MCP_HOST/$(vault read -f url)" + m := MCPConfig{Type: MCPHttp, URL: tmpl} + got, err := m.ResolvedURL(IdentityResolver()) + require.NoError(t, err) + require.Equal(t, tmpl, got) + }) +} + +// stubResolver returns ("", err) for every call. Paired with a non-nil +// err the empty-URL test asserts ResolvedURL short-circuits before +// reaching ResolveValue: if it didn't, the test would fail with err. +type stubResolver struct { + err error +} + +func (s stubResolver) ResolveValue(v string) (string, error) { + return "", s.err +}