Repository navigation
fix: fail closed on invalid boolean env settings and support yes/no/on/off - #3801
Open
yunusarda-sudo wants to merge 1 commit into
Open
yunusarda-sudo wants to merge 1 commit into
yunusarda-sudo wants to merge 1 commit into
Conversation
…n/off
When configuring boolean settings via environment variables (such as
GITHUB_READ_ONLY or GITHUB_LOCKDOWN_MODE), Viper binds pflag flags which
use strconv.ParseBool. Any unrecognised value (e.g. 'yes', 'on', 'tru')
causes pflag's Set method to fail silently and Viper defaults to false.
This meant GITHUB_READ_ONLY=yes would silently leave read-only mode off,
exposing write tools without warning.
Introduce parseBoolSetting which accepts common truthy ('1', 't', 'true',
'yes', 'y', 'on', 'enabled') and falsy ('0', 'f', 'false', 'no', 'n',
'off', 'disabled') values, while failing closed with a descriptive error
on unrecognised values. Explicit CLI flags preserve standard precedence.
Fixes github#3747
Co-authored-by: Rafe Walker <rafe.walker@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Boolean-setting validation remains incomplete: whitespace-only input can leave write tools enabled, list-scopes bypasses validation, and the parsing test depends on another test initializing Viper.
3 open issues
What changed in this PR
This PR addresses #3747 by adding explicit boolean environment parsing to the GitHub MCP server’s stdio and HTTP startup paths, aiming to prevent invalid settings from silently disabling protections such as read-only mode.
Changes:
- Accepts common boolean spellings, including
yes/no,on/off, andenabled/disabled, while returning errors for unrecognized values. - Preserves explicit CLI flag precedence and defaults for unset or empty settings.
- Adds unit tests for accepted values, invalid input, defaults, and CLI precedence.
| File | Description |
|---|---|
cmd/github-mcp-server/main.go |
Adds the shared boolean parser and uses it for stdio and HTTP settings. |
cmd/github-mcp-server/main_test.go |
Adds boolean parsing and configuration precedence tests. |
Review effort: Max (auto)
|
|
||
| ttl := viper.GetDuration("repo-access-cache-ttl") | ||
|
|
||
| readOnly, err := parseBoolSetting("read-only", cmd.Flag("read-only")) |
Comment on lines
+440
to
+446
| str := strings.TrimSpace(viper.GetString(key)) | ||
| if str == "" { | ||
| if flag != nil { | ||
| return strconv.ParseBool(flag.DefValue) | ||
| } | ||
| return false, nil | ||
| } |
| } | ||
| } | ||
|
|
||
| func TestParseBoolSetting(t *testing.T) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When configuring boolean settings via environment variables (such as
GITHUB_READ_ONLYorGITHUB_LOCKDOWN_MODE), Viper binds pflag flags which usestrconv.ParseBool. Becausepflag'sboolValue.Setonly accepts"1", "t", "T", "true", "TRUE", "True", "0", "f", "F", "false", "FALSE", "False", any unrecognised value (e.g.yes,on,enabled, or typos liketru) returns a syntax error that Viper silently swallows, falling back tofalse.As reported in #3747, setting
GITHUB_READ_ONLY=yessilently leaves read-only mode off, exposing 46 tools (includingmerge_pull_requestanddelete_file) instead of 27 without warning or error.Changes
parseBoolSetting(key string, flag *pflag.Flag) (bool, error):"1", "t", "true", "yes", "y", "on", "enabled"."0", "f", "false", "no", "n", "off", "disabled".invalid boolean value "tru" for GITHUB_READ_ONLY: expected true, false, 1, 0, yes, no, on, off).stdioandhttpsubcommands (read-only,lockdown-mode,insiders,export-translations,enable-command-logging,scope-challenge,trust-proxy-headers).cmd/github-mcp-server/main_test.gocovering truthy/falsy cases, fail-closed invalid syntax handling, empty/unset defaults, and CLI flag precedence over env vars.Fixes #3747