Skip to content

fix: move validateTrustedBots to validation.go and enforce on stdin path #2217

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
lpcox merged 2 commits into from
Mar 20, 2026
Merged

fix: move validateTrustedBots to validation.go and enforce on stdin path #2217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a89889d
Initial plan
Copilot Mar 20, 2026
7b49503
fix: move validateTrustedBots to validation.go and apply to stdin path
Copilot Mar 20, 2026
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
18 changes: 0 additions & 18 deletions internal/config/config_core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"io"
"log"
"os"
"strings"

"github.com/BurntSushi/toml"
"github.com/github/gh-aw-mcpg/internal/logger"
Expand Down Expand Up @@ -283,23 +282,6 @@ func LoadFromFile(path string) (*Config, error) {
return &cfg, nil
}

// validateTrustedBots checks that the trusted_bots list conforms to spec §4.1.3.4:
// when present, it must be a non-empty array of non-empty strings.
func validateTrustedBots(bots []string) error {
if bots == nil {
return nil
}
if len(bots) == 0 {
return fmt.Errorf("trusted_bots must be a non-empty array when present (spec §4.1.3.4)")
}
for i, bot := range bots {
if strings.TrimSpace(bot) == "" {
return fmt.Errorf("trusted_bots[%d] must be a non-empty string", i)
}
}
return nil
}

// logger for config package
var logConfig = log.New(io.Discard, "[CONFIG] ", log.LstdFlags)

Expand Down
5 changes: 4 additions & 1 deletion internal/config/config_stdin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,10 @@ func convertStdinConfig(stdinCfg *StdinConfig) (*Config, error) {
if stdinCfg.Gateway.PayloadDir != "" {
cfg.Gateway.PayloadDir = stdinCfg.Gateway.PayloadDir
}
if len(stdinCfg.Gateway.TrustedBots) > 0 {
if stdinCfg.Gateway.TrustedBots != nil {
if err := validateTrustedBots(stdinCfg.Gateway.TrustedBots); err != nil {
return nil, err
}
cfg.Gateway.TrustedBots = stdinCfg.Gateway.TrustedBots
}
} else {
Expand Down
9 changes: 4 additions & 5 deletions internal/config/config_stdin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,18 +988,17 @@ func TestConvertStdinConfig_TrustedBots(t *testing.T) {
assert.Equal(t, []string{"copilot-swe-agent[bot]", "my-org-bot"}, cfg.Gateway.TrustedBots)
})

t.Run("empty trustedBots not propagated", func(t *testing.T) {
t.Run("empty trustedBots rejected per spec §4.1.3.4", func(t *testing.T) {
stdinCfg := &StdinConfig{
MCPServers: map[string]*StdinServerConfig{},
Gateway: &StdinGatewayConfig{
TrustedBots: []string{},
},
}

cfg, err := convertStdinConfig(stdinCfg)
require.NoError(t, err)
require.NotNil(t, cfg.Gateway)
assert.Nil(t, cfg.Gateway.TrustedBots)
_, err := convertStdinConfig(stdinCfg)
require.Error(t, err)
assert.Contains(t, err.Error(), "trusted_bots must be a non-empty array when present")
})

t.Run("nil trustedBots not propagated", func(t *testing.T) {
Expand Down
35 changes: 35 additions & 0 deletions internal/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1738,3 +1738,38 @@ func TestLoadFromStdin_WithTrustedBots(t *testing.T) {

assert.Equal(t, []string{"github-actions[bot]", "copilot-swe-agent[bot]"}, cfg.Gateway.TrustedBots)
}

// TestLoadFromStdin_WithEmptyTrustedBots verifies JSON stdin parsing rejects trustedBots: [].
// Covers spec §4.1.3.4 (trustedBots MUST be a non-empty array when present).
func TestLoadFromStdin_WithEmptyTrustedBots(t *testing.T) {
stdinJSON := `{
"mcpServers": {
"test": {
"container": "test/container:latest",
"type": "stdio"
}
},
"gateway": {
"port": 8080,
"domain": "localhost",
"apiKey": "test-key",
"trustedBots": []
}
}`

oldStdin := os.Stdin
defer func() { os.Stdin = oldStdin }()

r, w, err := os.Pipe()
require.NoError(t, err)
os.Stdin = r

go func() {
defer w.Close()
_, _ = w.Write([]byte(stdinJSON))
}()

_, err = LoadFromStdin()
require.Error(t, err)
assert.Contains(t, err.Error(), "trustedBots")
}
22 changes: 22 additions & 0 deletions internal/config/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,32 @@ func validateGatewayConfig(gateway *StdinGatewayConfig) error {
}
}

// Validate trustedBots per spec §4.1.3.4: must be non-empty array when present
if err := validateTrustedBots(gateway.TrustedBots); err != nil {
return err
}
Comment on lines +368 to +371
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateGatewayConfig now enforces the trustedBots constraint, but TestValidateGatewayConfig in validation_test.go doesn’t cover the new cases (empty slice, whitespace-only entries). Adding focused unit cases there would prevent regressions even if JSON schema behavior changes and would directly exercise the new validation branch.

Copilot uses AI. Check for mistakes.

logValidation.Print("Gateway config validation passed")
return nil
}

// validateTrustedBots checks that the trusted_bots/trustedBots list conforms to spec §4.1.3.4:
// when present, it must be a non-empty array of non-empty strings.
func validateTrustedBots(bots []string) error {
if bots == nil {
return nil
}
if len(bots) == 0 {
return fmt.Errorf("trusted_bots must be a non-empty array when present (spec §4.1.3.4)")
}
for i, bot := range bots {
if strings.TrimSpace(bot) == "" {
return fmt.Errorf("trusted_bots[%d] must be a non-empty string", i)
Comment on lines +384 to +388
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateTrustedBots returns plain fmt.Errorf messages, while the rest of validateGatewayConfig primarily returns *rules.ValidationError (with consistent Field/JSONPath/Suggestion formatting). This leads to inconsistent error formatting for trustedBots compared to port/timeouts/payloadDir. Consider returning rules.InvalidValue (and include an appropriate JSONPath like "gateway.trustedBots" / "gateway.trustedBots[i]") so error output is consistent across gateway validations (and still usable from the TOML path).

Suggested change
return fmt.Errorf("trusted_bots must be a non-empty array when present (spec §4.1.3.4)")
}
for i, bot := range bots {
if strings.TrimSpace(bot) == "" {
return fmt.Errorf("trusted_bots[%d] must be a non-empty string", i)
return rules.InvalidValue(
"trusted_bots",
"gateway.trustedBots",
"trusted_bots must be a non-empty array when present (spec §4.1.3.4)",
"Remove the trusted_bots setting or provide at least one non-empty trusted bot identifier.",
)
}
for i, bot := range bots {
if strings.TrimSpace(bot) == "" {
return rules.InvalidValue(
"trusted_bots",
fmt.Sprintf("gateway.trustedBots[%d]", i),
"trusted_bots entries must be non-empty strings",
"Remove empty entries from trusted_bots or replace them with valid trusted bot identifiers.",
)

Copilot uses AI. Check for mistakes.
}
}
return nil
}

// validateTOMLStdioContainerization validates that TOML stdio servers use Docker for containerization.
// This enforces MCP Gateway Specification Section 3.2.1: "Stdio-based MCP servers MUST be containerized."
func validateTOMLStdioContainerization(servers map[string]*ServerConfig) error {
Expand Down
Loading