-
Notifications
You must be signed in to change notification settings - Fork 34
Enforce Docker containerization requirement for TOML stdio servers #1029
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.
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
Changes from all commits
3abb6da
7126c59
3adc1df
44a0828
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -712,3 +712,166 @@ func TestLoadFromStdin_ValidationErrors(t *testing.T) { | |
| } | ||
|
|
||
| // Helper function - defined in validation_string_patterns_test.go | ||
|
|
||
| func TestValidateTOMLStdioContainerization(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| servers map[string]*ServerConfig | ||
| shouldErr bool | ||
| errorMsg string | ||
| }{ | ||
| { | ||
| name: "valid Docker command for stdio server", | ||
| servers: map[string]*ServerConfig{ | ||
| "github": { | ||
| Type: "stdio", | ||
| Command: "docker", | ||
| Args: []string{"run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"}, | ||
| }, | ||
| }, | ||
| shouldErr: false, | ||
| }, | ||
| { | ||
| name: "valid Docker command with empty type (defaults to stdio)", | ||
| servers: map[string]*ServerConfig{ | ||
| "github": { | ||
| Type: "", | ||
| Command: "docker", | ||
| Args: []string{"run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"}, | ||
| }, | ||
| }, | ||
| shouldErr: false, | ||
| }, | ||
| { | ||
| name: "valid Docker command with local type (alias for stdio)", | ||
| servers: map[string]*ServerConfig{ | ||
| "github": { | ||
| Type: "local", | ||
| Command: "docker", | ||
| Args: []string{"run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"}, | ||
| }, | ||
| }, | ||
| shouldErr: false, | ||
| }, | ||
|
Comment on lines
+716
to
+755
|
||
| { | ||
| name: "invalid node command for stdio server", | ||
| servers: map[string]*ServerConfig{ | ||
| "filesystem": { | ||
| Type: "stdio", | ||
| Command: "node", | ||
| Args: []string{"/path/to/server.js"}, | ||
| }, | ||
| }, | ||
| shouldErr: true, | ||
| errorMsg: "stdio servers must use containerized execution (command must be 'docker', got 'node')", | ||
| }, | ||
| { | ||
| name: "invalid python command for stdio server", | ||
| servers: map[string]*ServerConfig{ | ||
| "custom": { | ||
| Type: "stdio", | ||
| Command: "python", | ||
| Args: []string{"-m", "mcp_server"}, | ||
| }, | ||
| }, | ||
| shouldErr: true, | ||
| errorMsg: "stdio servers must use containerized execution (command must be 'docker', got 'python')", | ||
| }, | ||
| { | ||
| name: "invalid npx command with empty type (defaults to stdio)", | ||
| servers: map[string]*ServerConfig{ | ||
| "custom": { | ||
| Command: "npx", | ||
| Args: []string{"@modelcontextprotocol/server-everything"}, | ||
| }, | ||
| }, | ||
| shouldErr: true, | ||
| errorMsg: "stdio servers must use containerized execution (command must be 'docker', got 'npx')", | ||
| }, | ||
| { | ||
| name: "http server not affected by validation", | ||
| servers: map[string]*ServerConfig{ | ||
| "httpserver": { | ||
| Type: "http", | ||
| URL: "https://example.com/mcp", | ||
| }, | ||
| }, | ||
| shouldErr: false, | ||
| }, | ||
| { | ||
| name: "mixed valid Docker stdio and http servers", | ||
| servers: map[string]*ServerConfig{ | ||
| "github": { | ||
| Type: "stdio", | ||
| Command: "docker", | ||
| Args: []string{"run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"}, | ||
| }, | ||
| "httpserver": { | ||
| Type: "http", | ||
| URL: "https://example.com/mcp", | ||
| }, | ||
| }, | ||
| shouldErr: false, | ||
| }, | ||
| { | ||
| name: "mixed Docker stdio, http, and invalid node stdio servers", | ||
| servers: map[string]*ServerConfig{ | ||
| "github": { | ||
| Type: "stdio", | ||
| Command: "docker", | ||
| Args: []string{"run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"}, | ||
| }, | ||
| "httpserver": { | ||
| Type: "http", | ||
| URL: "https://example.com/mcp", | ||
| }, | ||
| "filesystem": { | ||
| Type: "stdio", | ||
| Command: "node", | ||
| Args: []string{"/path/to/server.js"}, | ||
| }, | ||
| }, | ||
| shouldErr: true, | ||
| errorMsg: "server 'filesystem': stdio servers must use containerized execution (command must be 'docker', got 'node')", | ||
| }, | ||
| { | ||
| name: "error message includes specification reference", | ||
| servers: map[string]*ServerConfig{ | ||
| "bad": { | ||
| Type: "stdio", | ||
| Command: "bash", | ||
| Args: []string{"script.sh"}, | ||
| }, | ||
| }, | ||
| shouldErr: true, | ||
| errorMsg: "MCP Gateway Specification Section 3.2.1", | ||
| }, | ||
| { | ||
| name: "error message includes specification URL", | ||
| servers: map[string]*ServerConfig{ | ||
| "bad": { | ||
| Type: "stdio", | ||
| Command: "go", | ||
| Args: []string{"run", "main.go"}, | ||
| }, | ||
| }, | ||
| shouldErr: true, | ||
| errorMsg: "https://github.com/github/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#321-containerization-requirement", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := validateTOMLStdioContainerization(tt.servers) | ||
|
|
||
| if tt.shouldErr { | ||
| require.Error(t, err) | ||
| if tt.errorMsg != "" { | ||
| assert.ErrorContains(t, err, tt.errorMsg) | ||
| } | ||
| } else { | ||
| assert.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,8 +30,8 @@ func TestBinaryInvocation_RoutedMode(t *testing.T) { | |
| // Create a temporary config file | ||
| configFile := createTempConfig(t, map[string]interface{}{ | ||
| "testserver": map[string]interface{}{ | ||
| "command": "echo", | ||
| "args": []string{}, | ||
| "command": "docker", | ||
| "args": []string{"run", "--rm", "-i", "alpine:latest", "echo"}, | ||
| }, | ||
|
Comment on lines
31
to
35
|
||
| }) | ||
| defer os.Remove(configFile) | ||
|
|
@@ -130,12 +130,12 @@ func TestBinaryInvocation_UnifiedMode(t *testing.T) { | |
|
|
||
| configFile := createTempConfig(t, map[string]interface{}{ | ||
| "backend1": map[string]interface{}{ | ||
| "command": "echo", | ||
| "args": []string{}, | ||
| "command": "docker", | ||
| "args": []string{"run", "--rm", "-i", "alpine:latest", "echo"}, | ||
| }, | ||
| "backend2": map[string]interface{}{ | ||
| "command": "echo", | ||
| "args": []string{}, | ||
| "command": "docker", | ||
| "args": []string{"run", "--rm", "-i", "alpine:latest", "echo"}, | ||
| }, | ||
| }) | ||
| defer os.Remove(configFile) | ||
|
|
@@ -286,8 +286,8 @@ func TestBinaryInvocation_PipeOutput(t *testing.T) { | |
| // Create a simple config | ||
| configFile := createTempConfig(t, map[string]interface{}{ | ||
| "testserver": map[string]interface{}{ | ||
| "command": "echo", | ||
| "args": []string{}, | ||
| "command": "docker", | ||
| "args": []string{"run", "--rm", "-i", "alpine:latest", "echo"}, | ||
| }, | ||
| }) | ||
| defer os.Remove(configFile) | ||
|
|
@@ -748,8 +748,8 @@ func TestBinaryInvocation_LogFileCreation(t *testing.T) { | |
| // Create a temporary config file | ||
| configFile := createTempConfig(t, map[string]interface{}{ | ||
| "testserver": map[string]interface{}{ | ||
| "command": "echo", | ||
| "args": []string{}, | ||
| "command": "docker", | ||
| "args": []string{"run", "--rm", "-i", "alpine:latest", "echo"}, | ||
| }, | ||
| }) | ||
| defer os.Remove(configFile) | ||
|
|
@@ -896,8 +896,8 @@ func TestBinaryInvocation_LogDirEnvironmentVariable(t *testing.T) { | |
| // Create a temporary config file | ||
| configFile := createTempConfig(t, map[string]interface{}{ | ||
| "testserver": map[string]interface{}{ | ||
| "command": "echo", | ||
| "args": []string{}, | ||
| "command": "docker", | ||
| "args": []string{"run", "--rm", "-i", "alpine:latest", "echo"}, | ||
| }, | ||
| }) | ||
| defer os.Remove(configFile) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateTOMLStdioContainerizationonly enforces the Docker requirement whencfg.Typeis "", "stdio", or "local". However, the launcher treats any non-"http" type as a stdio server, so a TOML config can bypass this check by settingtype = "HTTP"(case mismatch) or any unknown value and using a non-docker command. Consider normalizingType(e.g., strings.ToLower/TrimSpace) and enforcing Docker for all non-HTTP servers (or explicitly rejecting unsupportedtypevalues in TOML).