Skip to content

Add OIDC fail-fast validation to TOML config path #3538

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 3 commits into from
Apr 10, 2026
+80 −0
Merged

Add OIDC fail-fast validation to TOML config path #3538

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25268f5
Initial plan
Copilot Apr 10, 2026
1068ff2
Add OIDC fail-fast auth validation to TOML config loading path
Copilot Apr 10, 2026
14664cd
Reject auth on non-HTTP servers in TOML config path
Copilot Apr 10, 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
12 changes: 12 additions & 0 deletions internal/config/config_core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,18 @@ func LoadFromFile(path string) (*Config, error) {
return nil, err
}

// Validate auth configs (e.g. fail-fast for missing OIDC env vars).
// This ensures parity with the JSON stdin path which calls validateAuthConfig
// via convertStdinServerConfig → validateServerConfigWithCustomSchemas.
for name, serverCfg := range cfg.Servers {
if serverCfg.Auth != nil {
jsonPath := fmt.Sprintf("servers.%s", name)
if err := validateAuthConfig(serverCfg.Auth, name, jsonPath); err != nil {
return nil, err
}
}
Comment on lines +348 to +361
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

LoadFromFile calls validateAuthConfig whenever serverCfg.Auth is non-nil, but it doesn’t first enforce that the server is an HTTP backend. As written, a TOML config can attach auth to stdio/local servers and still pass validation (or fail with an unrelated OIDC env-var error), which is inconsistent with validateStandardServerConfig (stdin path) that rejects auth on non-HTTP servers. Consider checking serverCfg.Type == "http" (and possibly that serverCfg.URL is non-empty) before calling validateAuthConfig, and returning a clear unsupported-field error when auth is set on non-HTTP servers.

Copilot uses AI. Check for mistakes.
}

// Initialize gateway if not present
if cfg.Gateway == nil {
cfg.Gateway = &GatewayConfig{}
Expand Down
45 changes: 45 additions & 0 deletions internal/config/config_core_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,51 @@ func TestGetAPIKey_ReturnsKey(t *testing.T) {
assert.Equal(t, "super-secret-key", cfg.GetAPIKey())
}

// TestLoadFromFile_OIDCAuthMissingEnvVar verifies that LoadFromFile returns an error
// when a server uses github-oidc auth but ACTIONS_ID_TOKEN_REQUEST_URL is not set.
// This ensures parity with the JSON stdin config path (Spec §9 Fail-Fast Startup).
func TestLoadFromFile_OIDCAuthMissingEnvVar(t *testing.T) {
t.Setenv("ACTIONS_ID_TOKEN_REQUEST_URL", "")

path := writeTempTOML(t, `
[servers.secure]
type = "http"
url = "https://example.com/mcp"

[servers.secure.auth]
type = "github-oidc"
audience = "https://example.com"
`)
cfg, err := LoadFromFile(path)
require.Error(t, err)
assert.Nil(t, cfg)
assert.Contains(t, err.Error(), "ACTIONS_ID_TOKEN_REQUEST_URL")
}

// TestLoadFromFile_OIDCAuthWithEnvVarSet verifies that LoadFromFile succeeds
// when a server uses github-oidc auth and ACTIONS_ID_TOKEN_REQUEST_URL is set.
func TestLoadFromFile_OIDCAuthWithEnvVarSet(t *testing.T) {
t.Setenv("ACTIONS_ID_TOKEN_REQUEST_URL", "https://token.actions.example.com")

path := writeTempTOML(t, `
[servers.secure]
type = "http"
url = "https://example.com/mcp"

[servers.secure.auth]
type = "github-oidc"
audience = "https://example.com"
`)
cfg, err := LoadFromFile(path)
require.NoError(t, err)
require.NotNil(t, cfg)
server := cfg.Servers["secure"]
require.NotNil(t, server)
require.NotNil(t, server.Auth)
assert.Equal(t, "github-oidc", server.Auth.Type)
assert.Equal(t, "https://example.com", server.Auth.Audience)
}

Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

The new TOML auth validation is covered for missing/present ACTIONS_ID_TOKEN_REQUEST_URL, but there’s no test ensuring TOML rejects an auth block on non-HTTP servers (stdio/local). Adding a regression test for that case would help keep TOML behavior aligned with the stdin validation rules.

Suggested change
// TestLoadFromFile_AuthOnNonHTTPServerRejected verifies that TOML configs reject
// auth blocks on non-HTTP servers so TOML validation stays aligned with stdin/local rules.
func TestLoadFromFile_AuthOnNonHTTPServerRejected(t *testing.T) {
path := writeTempTOML(t, `
[servers.local]
command = "docker"
args = ["run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"]
[servers.local.auth]
type = "github-oidc"
audience = "https://example.com"
`)
cfg, err := LoadFromFile(path)
require.Error(t, err)
assert.Nil(t, cfg)
assert.Contains(t, err.Error(), "auth")
assert.Contains(t, err.Error(), "http")
}

Copilot uses AI. Check for mistakes.
// TestLoadFromFile_NegativePayloadSizeThresholdRejected verifies that TOML configs with
// a negative payload_size_threshold are rejected per spec §4.1.3.3.
func TestLoadFromFile_NegativePayloadSizeThresholdRejected(t *testing.T) {
Expand Down
Loading