diff --git a/cmd/github-mcp-server/helpers.go b/cmd/github-mcp-server/helpers.go index c5f498813d..34d408de59 100644 --- a/cmd/github-mcp-server/helpers.go +++ b/cmd/github-mcp-server/helpers.go @@ -1,6 +1,14 @@ package main -import "strings" +import ( + "fmt" + "os" + "strconv" + "strings" + + "github.com/spf13/pflag" + "github.com/spf13/viper" +) // formatToolsetName converts a toolset ID to a human-readable name. // Used by both generate_docs.go and list_scopes.go for consistent formatting. @@ -27,3 +35,40 @@ func formatToolsetName(name string) string { return strings.Join(parts, " ") } } + +// boolSettings lists the boolean configuration keys shared by the server commands. +var boolSettings = []string{ + "read-only", + "lockdown-mode", + "insiders", + "export-translations", + "enable-command-logging", +} + +// validateBoolSettings rejects boolean settings whose raw value is not a valid +// boolean. viper.GetBool silently treats unparsable values such as "yes" as +// false, which would leave e.g. read-only mode off without any warning. Flags +// are already parsed strictly by pflag, so in practice this catches invalid +// values coming from environment variables. viper also treats an empty env var +// as unset, so a present-but-empty variable is rejected too, unless the flag +// was set explicitly and overrides it. +func validateBoolSettings(flags *pflag.FlagSet, keys ...string) error { + for _, key := range keys { + if flag := flags.Lookup(key); flag != nil && flag.Changed { + continue + } + envName := "GITHUB_" + strings.ToUpper(strings.ReplaceAll(key, "-", "_")) + value, ok := viper.Get(key).(string) + if !ok { + envValue, present := os.LookupEnv(envName) + if !present || envValue != "" { + continue + } + value = envValue + } + if _, err := strconv.ParseBool(value); err != nil { + return fmt.Errorf("invalid value %q for %s: must be a boolean (1, t, T, true, TRUE, True, 0, f, F, false, FALSE, False)", value, envName) + } + } + return nil +} diff --git a/cmd/github-mcp-server/list_scopes.go b/cmd/github-mcp-server/list_scopes.go index 25e6e87b5c..003ad0f361 100644 --- a/cmd/github-mcp-server/list_scopes.go +++ b/cmd/github-mcp-server/list_scopes.go @@ -12,6 +12,7 @@ import ( "github.com/github/github-mcp-server/v2/pkg/inventory" "github.com/github/github-mcp-server/v2/pkg/translations" "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/spf13/viper" ) @@ -61,8 +62,8 @@ Examples: # Just show unique scopes needed github-mcp-server list-scopes --output=summary`, - RunE: func(_ *cobra.Command, _ []string) error { - return runListScopes() + RunE: func(cmd *cobra.Command, _ []string) error { + return runListScopes(cmd.Flags()) }, } @@ -81,7 +82,11 @@ func formatScopeDisplay(scope string) string { return scope } -func runListScopes() error { +func runListScopes(flags *pflag.FlagSet) error { + if err := validateBoolSettings(flags, "read-only"); err != nil { + return err + } + // Get toolsets configuration (same logic as stdio command) var enabledToolsets []string if viper.IsSet("toolsets") { diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 04ba68e226..9022f041b4 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -40,6 +40,10 @@ var ( Short: "Start stdio server", Long: `Start a server that communicates via standard input/output streams using JSON-RPC messages.`, RunE: func(cmd *cobra.Command, _ []string) error { + if err := validateBoolSettings(cmd.Flags(), boolSettings...); err != nil { + return err + } + info, _ := debug.ReadBuildInfo() serverVersion := resolveServerVersion(version, commit, info) if serverVersion == developmentServerVersion { @@ -172,7 +176,11 @@ var ( Use: "http", Short: "Start HTTP server", Long: `Start an HTTP server that listens for MCP requests over HTTP.`, - RunE: func(_ *cobra.Command, _ []string) error { + RunE: func(cmd *cobra.Command, _ []string) error { + if err := validateBoolSettings(cmd.Flags(), append(boolSettings, "scope-challenge", "trust-proxy-headers")...); err != nil { + return err + } + // Parse toolsets (same approach as stdio — see comment there) var enabledToolsets []string if viper.IsSet("toolsets") { diff --git a/cmd/github-mcp-server/main_test.go b/cmd/github-mcp-server/main_test.go index 48d1367acf..78f3a2c661 100644 --- a/cmd/github-mcp-server/main_test.go +++ b/cmd/github-mcp-server/main_test.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "path/filepath" "strings" @@ -10,6 +11,7 @@ import ( "github.com/github/github-mcp-server/v2/pkg/scopes" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/spf13/pflag" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -57,6 +59,76 @@ func TestAuthorizationServerConfigurationIsHTTPOnly(t *testing.T) { assert.Equal(t, "https://oauth-proxy.example.com", viper.GetString("authorization-server")) } +func TestValidateBoolSettingsFromEnv(t *testing.T) { + // Clear every checked variable so inherited values can't affect the + // assertions; t.Setenv restores the original values at cleanup. + for _, key := range boolSettings { + envName := "GITHUB_" + strings.ToUpper(strings.ReplaceAll(key, "-", "_")) + t.Setenv(envName, "") + require.NoError(t, os.Unsetenv(envName)) + } + + tests := []struct { + value string + want bool + wantErr bool + }{ + {value: "1", want: true}, + {value: "true", want: true}, + {value: "TRUE", want: true}, + {value: "t", want: true}, + {value: "T", want: true}, + {value: "True", want: true}, + {value: "0", want: false}, + {value: "false", want: false}, + {value: "F", want: false}, + {value: "FALSE", want: false}, + {value: "False", want: false}, + {value: "", wantErr: true}, + {value: "yes", wantErr: true}, + {value: "on", wantErr: true}, + {value: "enabled", wantErr: true}, + {value: "tru", wantErr: true}, + } + + for _, envName := range []string{"GITHUB_READ_ONLY", "GITHUB_LOCKDOWN_MODE"} { + key := strings.ReplaceAll(strings.ToLower(strings.TrimPrefix(envName, "GITHUB_")), "_", "-") + for _, tc := range tests { + t.Run(envName+"="+tc.value, func(t *testing.T) { + t.Setenv(envName, tc.value) + initConfig() + + err := validateBoolSettings(pflag.NewFlagSet("test", pflag.ContinueOnError), boolSettings...) + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), envName) + assert.Contains(t, err.Error(), fmt.Sprintf("%q", tc.value)) + assert.Contains(t, err.Error(), "1, t, T, true, TRUE, True, 0, f, F, false, FALSE, False") + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, viper.GetBool(key)) + }) + } + } + + t.Run("explicit flag overrides empty env", func(t *testing.T) { + t.Setenv("GITHUB_READ_ONLY", "") + initConfig() + + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.Bool("read-only", false, "") + require.NoError(t, flags.Parse([]string{"--read-only=true"})) + require.NoError(t, validateBoolSettings(flags, boolSettings...)) + }) + + t.Run("unset", func(t *testing.T) { + initConfig() + require.NoError(t, validateBoolSettings(pflag.NewFlagSet("test", pflag.ContinueOnError), boolSettings...)) + assert.False(t, viper.GetBool("read-only")) + }) +} + func TestWriteToolDocScopes(t *testing.T) { tool := inventory.ServerTool{ Tool: mcp.Tool{Name: "delete", Annotations: &mcp.ToolAnnotations{Title: "Delete"}}, diff --git a/docs/server-configuration.md b/docs/server-configuration.md index 25c641153e..a5ae055cd8 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -17,6 +17,8 @@ We currently support the following ways in which the GitHub MCP Server can be co | Scope Filtering | Always enabled | Always enabled | | Server Name/Title | Not available | `GITHUB_MCP_SERVER_NAME` / `GITHUB_MCP_SERVER_TITLE` env vars or `github-mcp-server-config.json` | +> **Boolean env vars:** `GITHUB_READ_ONLY`, `GITHUB_LOCKDOWN_MODE`, `GITHUB_INSIDERS` and the other boolean settings accept the same values as their flags (`1`, `t`, `T`, `true`, `TRUE`, `True`, `0`, `f`, `F`, `false`, `FALSE`, `False`). Any other value, such as `yes` or `on`, stops the server at startup with an error. This includes a variable that is set but empty (for example `GITHUB_READ_ONLY=` or `docker run -e GITHUB_READ_ONLY=`), which used to be treated as unset. + > **Default behavior:** If you don't specify any configuration, the server uses the **default toolsets**: `context`, `issues`, `pull_requests`, `repos`, `users`. ---