From ae4d6bab96fe54333cd414ef79800d04b455ebed Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 20 Aug 2026 13:57:56 +0100 Subject: [PATCH] fix: config validation hardening and integration polish - providers.go: validate provider names against ^[a-zA-Z0-9._-]+$ and reject reserved names (v1, mcp, a2a) that would shadow gateway routes; reject nil ProviderConfig map values at construction instead of panicking. - mcp_a2a.go: apply the same name-pattern and reserved-name validation to mcpServers/agents keys, and reject nil TargetConfig/AgentConfig map values at construction. - auth.go: reject nil GroupConfig map values at construction. - provider_openai_test.go, mcp_a2a_test.go, auth_test.go: table-driven constructor-error coverage for the above. - integration/integration_test.go: point the package doc at `make integration-up`/`integration-keep` instead of a bare `docker compose up -d --build`, which skips the dynamic.yml.tmpl render step and can run against a stale or absent dynamic.yml; add max_tokens to the gemini chat request so translate_gemini.go's hasMaxTokens=true branch is actually exercised under real Yaegi. - translate_gemini.go: soften the max_tokens comment's verification claim to match what the integration suite actually now exercises. --- auth.go | 3 ++ auth_test.go | 15 ++++++++ integration/integration_test.go | 22 ++++++++---- mcp_a2a.go | 26 +++++++++++--- mcp_a2a_test.go | 30 ++++++++++++++++ provider_openai_test.go | 63 +++++++++++++++++++++++++++++++++ providers.go | 59 +++++++++++++++++++++++++----- translate_gemini.go | 7 ++-- 8 files changed, 202 insertions(+), 23 deletions(-) diff --git a/auth.go b/auth.go index 0da9867..8e371ee 100644 --- a/auth.go +++ b/auth.go @@ -119,6 +119,9 @@ func newAuthStore(cfg *Config) (*authStore, error) { nowFn: time.Now, } for name, gc := range cfg.Groups { + if gc == nil { + return nil, fmt.Errorf("llmgateway: group %q: config must not be nil", name) + } if err := gc.Limits.validate(); err != nil { return nil, fmt.Errorf("llmgateway: group %q: %w", name, err) } diff --git a/auth_test.go b/auth_test.go index 7af7512..5186e47 100644 --- a/auth_test.go +++ b/auth_test.go @@ -160,6 +160,21 @@ func TestNewAuthStore_EmptyAPIKeyAfterResolution_ReturnsError(t *testing.T) { } } +// TestNewAuthStore_NilGroupConfig_ReturnsError covers a Groups map entry +// whose value is a nil *GroupConfig (a caller can construct this in Go, and +// Traefik's own YAML-to-struct decoding can leave a map value nil for an +// empty mapping entry) — newAuthStore must reject it as a constructor error +// rather than panic dereferencing gc.Limits on a nil gc. +func TestNewAuthStore_NilGroupConfig_ReturnsError(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": nil}, + } + if _, err := newAuthStore(cfg); err == nil { + t.Fatal("want error for a nil GroupConfig value, got nil") + } +} + func TestNewAuthStore_NoUsers_OK(t *testing.T) { cfg := &Config{ Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, diff --git a/integration/integration_test.go b/integration/integration_test.go index ba223e6..7867fd9 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -5,10 +5,16 @@ // upstreams, and a real shared Redis, all orchestrated by // integration/docker-compose.yml. It assumes the compose stack is already // up and healthy — the `integration` Makefile target starts it, waits for -// traefik1 to answer, runs this package, then tears the stack down; run it -// directly with `go test -tags integration ./...` from this directory -// after `docker compose up -d --build` if you want to keep the stack alive -// between runs (see the `integration-keep` Makefile target). +// traefik1 to answer, runs this package, then tears the stack down. To keep +// the stack alive between runs for debugging, use `make integration-keep` +// (or `make integration-up` followed directly by +// `go test -tags integration ./...` from this directory), not a bare +// `docker compose up -d --build`: integration-up first renders +// integration/traefik/dynamic.yml from dynamic.yml.tmpl (substituting +// INTEGRATION_REAL_BASEURL), and that generated dynamic.yml is +// git-ignored — running docker compose directly reuses whatever stale or +// absent dynamic.yml happens to be on disk instead of a freshly rendered +// one. package integration import ( @@ -213,8 +219,12 @@ func TestUnifiedChatAllProviders(t *testing.T) { } geminiReq := map[string]any{ - "model": "gemini/gemini-mock", - "messages": []map[string]any{{"role": "user", "content": "hi"}}, + "model": "gemini/gemini-mock", + // max_tokens exercises translate_gemini.go's hasMaxTokens=true + // branch (see its comment on the yaegi multi-value-assignment + // workaround) under real Traefik + Yaegi, not just go test. + "max_tokens": 64, + "messages": []map[string]any{{"role": "user", "content": "hi"}}, } resp3, body3 := doJSON(t, http.MethodPost, traefik1URL+"/v1/chat/completions", aliceKey, geminiReq) if resp3.StatusCode != http.StatusOK { diff --git a/mcp_a2a.go b/mcp_a2a.go index df39941..55c910e 100644 --- a/mcp_a2a.go +++ b/mcp_a2a.go @@ -183,18 +183,34 @@ func (g *Gateway) handleTargetProxy(w http.ResponseWriter, r *http.Request, u *u g.proxyUpstream(w, r, upstreamURL, g.targetClient, nil, false, kind+" target (name "+name+")") } -// validateTargetURLs checks that every configured MCP-server and agent -// target URL parses (url.Parse) and uses an http or https scheme. A -// malformed or non-HTTP target is rejected once, here, at construction — -// a constructor error, not a 502 the first time some caller happens to -// address it. +// validateTargetURLs checks every configured MCP-server and agent entry at +// construction: the map value must be non-nil, the name must pass +// validateConfigName (same character set and reserved-name rules as a +// provider name — an MCP server or agent is routed at "/mcp/{name}/*" or +// "/a2a/{name}/*", the same shadowing risk applies), and the target URL +// must parse (url.Parse) and use an http or https scheme. A nil value, +// invalid name, or malformed/non-HTTP target is rejected once, here, at +// construction — a constructor error, not a nil-pointer panic or a 502 the +// first time some caller happens to address it. func validateTargetURLs(cfg *Config) error { for name, tc := range cfg.MCPServers { + if tc == nil { + return fmt.Errorf("llmgateway: mcpServers %q: config must not be nil", name) + } + if err := validateConfigName("mcpServers", name); err != nil { + return err + } if err := validateTargetURL(tc.URL); err != nil { return fmt.Errorf("llmgateway: mcpServers %q: %w", name, err) } } for name, ac := range cfg.Agents { + if ac == nil { + return fmt.Errorf("llmgateway: agents %q: config must not be nil", name) + } + if err := validateConfigName("agents", name); err != nil { + return err + } if err := validateTargetURL(ac.URL); err != nil { return fmt.Errorf("llmgateway: agents %q: %w", name, err) } diff --git a/mcp_a2a_test.go b/mcp_a2a_test.go index 44524e7..a6be3f3 100644 --- a/mcp_a2a_test.go +++ b/mcp_a2a_test.go @@ -666,6 +666,36 @@ func TestNewGateway_TargetURLValidation(t *testing.T) { agents: map[string]*AgentConfig{"agent1": {URL: "http://agent1.internal:notaport"}}, wantErr: true, }, + { + name: "nil mcpServers config value is a constructor error, not a panic", + mcpServers: map[string]*TargetConfig{"alpha": nil}, + wantErr: true, + }, + { + name: "nil agents config value is a constructor error, not a panic", + agents: map[string]*AgentConfig{"agent1": nil}, + wantErr: true, + }, + { + name: "mcpServers name reserved (v1) is a constructor error", + mcpServers: map[string]*TargetConfig{"v1": {URL: "http://mcp-alpha.internal:8080"}}, + wantErr: true, + }, + { + name: "mcpServers name reserved (mcp) is a constructor error", + mcpServers: map[string]*TargetConfig{"mcp": {URL: "http://mcp-alpha.internal:8080"}}, + wantErr: true, + }, + { + name: "agents name reserved (a2a) is a constructor error", + agents: map[string]*AgentConfig{"a2a": {URL: "https://agent1.internal"}}, + wantErr: true, + }, + { + name: "mcpServers name with invalid character is a constructor error", + mcpServers: map[string]*TargetConfig{"has a space": {URL: "http://mcp-alpha.internal:8080"}}, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/provider_openai_test.go b/provider_openai_test.go index ec13f8b..41acebc 100644 --- a/provider_openai_test.go +++ b/provider_openai_test.go @@ -657,4 +657,67 @@ func TestBuildAdapters(t *testing.T) { t.Fatalf("buildAdapters: want error for keyless gemini provider, got nil") } }) + + t.Run("bad: nil provider config value is a constructor error, not a panic", func(t *testing.T) { + cfg := &Config{Providers: map[string]*ProviderConfig{"p1": nil}} + if _, err := buildAdapters(cfg); err == nil { + t.Fatalf("buildAdapters: want error for a nil ProviderConfig value, got nil") + } + }) +} + +// TestValidateConfigName covers validateConfigName's character-set and +// reserved-name rules, shared by buildAdapters (provider names) and +// validateTargetURLs (mcpServers/agents names, mcp_a2a_test.go) — a +// provider, MCP server, or agent named "v1", "mcp", or "a2a" would shadow +// one of the gateway's own fixed top-level routes. +func TestValidateConfigName(t *testing.T) { + tests := []struct { + kind string + name string + wantErr bool + }{ + {"provider", "openai", false}, + {"provider", "my-provider_1.local", false}, + {"provider", "openai grok", true}, // space + {"provider", "openai/grok", true}, // slash: would break passthroughRoute's own path splitting + {"provider", "", true}, // empty never matches configNamePattern + {"provider", "v1", true}, // reserved: unified API namespace + {"provider", "mcp", true}, // reserved: MCP target-proxy prefix + {"provider", "a2a", true}, // reserved: A2A target-proxy prefix + {"mcpServers", "v1", true}, // same reserved set applies to every kind + {"agents", "a2a", true}, + } + for _, tt := range tests { + t.Run(tt.kind+"/"+tt.name, func(t *testing.T) { + err := validateConfigName(tt.kind, tt.name) + if (err != nil) != tt.wantErr { + t.Errorf("validateConfigName(%q, %q) error = %v, wantErr %v", tt.kind, tt.name, err, tt.wantErr) + } + }) + } +} + +// TestBuildAdapters_RejectsReservedOrInvalidProviderName covers buildAdapters' +// own call to validateConfigName, end to end through the map key rather than +// validateConfigName in isolation. +func TestBuildAdapters_RejectsReservedOrInvalidProviderName(t *testing.T) { + tests := []struct { + name string + }{ + {"v1"}, + {"mcp"}, + {"a2a"}, + {"has a space"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &Config{Providers: map[string]*ProviderConfig{ + tt.name: {Type: "openai", APIKey: "sk-test"}, + }} + if _, err := buildAdapters(cfg); err == nil { + t.Fatalf("buildAdapters: want error for provider name %q, got nil", tt.name) + } + }) + } } diff --git a/providers.go b/providers.go index 8535798..14e4d1c 100644 --- a/providers.go +++ b/providers.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net/http" + "regexp" "strings" ) @@ -188,19 +189,59 @@ func upstreamJSON(ctx context.Context, client *http.Client, method, url string, return resp, nil } +// configNamePattern is the character set allowed in a provider, mcpServers, +// or agents config key: it is embedded verbatim as a path segment in the +// gateway's own routes (passthroughRoute's "/{provider}/*", +// targetRoute's "/mcp/{name}/*" and "/a2a/{name}/*"), so it is restricted to +// characters safe there rather than the full range a Go map key allows. +var configNamePattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) + +// reservedConfigNames are the provider, mcpServers, and agents names +// rejected at construction because they would shadow one of the gateway's +// own fixed top-level routes: "v1" is the unified API namespace +// (/v1/chat/completions, /v1/models, /v1/embeddings, /v1/mcp/servers, +// /v1/agents), "mcp" and "a2a" are the MCP/A2A target-proxy prefixes +// (/mcp/{name}/..., /a2a/{name}/...). ServeHTTP checks those fixed routes +// ahead of passthrough/target routing (llmgateway.go), so a provider, MCP +// server, or agent configured under one of these names would never be +// reachable — reject it here instead of silently shadowing it. +var reservedConfigNames = map[string]bool{"v1": true, "mcp": true, "a2a": true} + +// validateConfigName reports an error unless name matches configNamePattern +// and is not one of reservedConfigNames. kind labels the config section +// (e.g. "provider", "mcpServers", "agents") in the error message. +func validateConfigName(kind, name string) error { + if !configNamePattern.MatchString(name) { + return fmt.Errorf("llmgateway: %s name %q is invalid: must match %s", kind, name, configNamePattern.String()) + } + if reservedConfigNames[name] { + return fmt.Errorf("llmgateway: %s name %q is reserved and would shadow a gateway route", kind, name) + } + return nil +} + // buildAdapters resolves cfg.Providers into a map of providerAdapter keyed -// by provider name. For each provider it resolves APIKey via resolveSecret -// (propagating that error) and defaults BaseURL by provider type when the -// config omits one, trimming any trailing slash either way. An unknown -// provider type is a config error. A resolved empty key is a valid -// keyless upstream for openai-type providers, but a constructor error for -// anthropic-type ones — newAnthropicAdapter enforces that, per ruling (c); -// this function just propagates whatever error it returns. A configured -// gemini provider follows the same constructor-error ruling — -// newGeminiAdapter enforces it too. +// by provider name. Every provider name is validated (validateConfigName) +// and every ProviderConfig value must be non-nil before anything else runs, +// so a malformed map key or a nil map value fails construction with a clear +// error instead of a nil-pointer panic. For each provider it resolves +// APIKey via resolveSecret (propagating that error) and defaults BaseURL by +// provider type when the config omits one, trimming any trailing slash +// either way. An unknown provider type is a config error. A resolved empty +// key is a valid keyless upstream for openai-type providers, but a +// constructor error for anthropic-type ones — newAnthropicAdapter enforces +// that, per ruling (c); this function just propagates whatever error it +// returns. A configured gemini provider follows the same constructor-error +// ruling — newGeminiAdapter enforces it too. func buildAdapters(cfg *Config) (map[string]providerAdapter, error) { adapters := make(map[string]providerAdapter, len(cfg.Providers)) for name, pc := range cfg.Providers { + if pc == nil { + return nil, fmt.Errorf("llmgateway: provider %q: config must not be nil", name) + } + if err := validateConfigName("provider", name); err != nil { + return nil, err + } apiKey, err := resolveSecret(pc.APIKey) if err != nil { return nil, fmt.Errorf("llmgateway: provider %q: %w", name, err) diff --git a/translate_gemini.go b/translate_gemini.go index f2bf435..fc5dd1e 100644 --- a/translate_gemini.go +++ b/translate_gemini.go @@ -368,9 +368,10 @@ func geminiRequestFromOpenAI(req map[string]any) (map[string]any, error) { // v0.16.1 panics interpreting a multi-value assignment mixing a // variable and a literal in one statement on this exact live path (the // same class of bug as sse.go's readSSE dispatch and registry.go's - // resolveAgainst — see those files' comments). Verified empirically - // under real Traefik (Task 15's integration suite, the gemini chat - // completion path). + // resolveAgainst — see those files' comments). The hasMaxTokens=true + // branch below is exercised by the integration suite's gemini chat + // request (integration/integration_test.go, TestUnifiedChatAllProviders, + // which sets "max_tokens") under real Traefik + Yaegi. if v, ok := req["max_completion_tokens"]; ok { if n, ok2 := toInt64(v); ok2 { maxTokens = n