mirror of
https://github.com/lukaszraczylo/traefik-llmgateway.git
synced 2026-10-18 05:47:38 +00:00
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.
This commit is contained in:
@@ -119,6 +119,9 @@ func newAuthStore(cfg *Config) (*authStore, error) {
|
|||||||
nowFn: time.Now,
|
nowFn: time.Now,
|
||||||
}
|
}
|
||||||
for name, gc := range cfg.Groups {
|
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 {
|
if err := gc.Limits.validate(); err != nil {
|
||||||
return nil, fmt.Errorf("llmgateway: group %q: %w", name, err)
|
return nil, fmt.Errorf("llmgateway: group %q: %w", name, err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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) {
|
func TestNewAuthStore_NoUsers_OK(t *testing.T) {
|
||||||
cfg := &Config{
|
cfg := &Config{
|
||||||
Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}},
|
Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}},
|
||||||
|
|||||||
@@ -5,10 +5,16 @@
|
|||||||
// upstreams, and a real shared Redis, all orchestrated by
|
// upstreams, and a real shared Redis, all orchestrated by
|
||||||
// integration/docker-compose.yml. It assumes the compose stack is already
|
// integration/docker-compose.yml. It assumes the compose stack is already
|
||||||
// up and healthy — the `integration` Makefile target starts it, waits for
|
// up and healthy — the `integration` Makefile target starts it, waits for
|
||||||
// traefik1 to answer, runs this package, then tears the stack down; run it
|
// traefik1 to answer, runs this package, then tears the stack down. To keep
|
||||||
// directly with `go test -tags integration ./...` from this directory
|
// the stack alive between runs for debugging, use `make integration-keep`
|
||||||
// after `docker compose up -d --build` if you want to keep the stack alive
|
// (or `make integration-up` followed directly by
|
||||||
// between runs (see the `integration-keep` Makefile target).
|
// `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
|
package integration
|
||||||
|
|
||||||
import (
|
import (
|
||||||
@@ -213,8 +219,12 @@ func TestUnifiedChatAllProviders(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
geminiReq := map[string]any{
|
geminiReq := map[string]any{
|
||||||
"model": "gemini/gemini-mock",
|
"model": "gemini/gemini-mock",
|
||||||
"messages": []map[string]any{{"role": "user", "content": "hi"}},
|
// 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)
|
resp3, body3 := doJSON(t, http.MethodPost, traefik1URL+"/v1/chat/completions", aliceKey, geminiReq)
|
||||||
if resp3.StatusCode != http.StatusOK {
|
if resp3.StatusCode != http.StatusOK {
|
||||||
|
|||||||
+21
-5
@@ -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+")")
|
g.proxyUpstream(w, r, upstreamURL, g.targetClient, nil, false, kind+" target (name "+name+")")
|
||||||
}
|
}
|
||||||
|
|
||||||
// validateTargetURLs checks that every configured MCP-server and agent
|
// validateTargetURLs checks every configured MCP-server and agent entry at
|
||||||
// target URL parses (url.Parse) and uses an http or https scheme. A
|
// construction: the map value must be non-nil, the name must pass
|
||||||
// malformed or non-HTTP target is rejected once, here, at construction —
|
// validateConfigName (same character set and reserved-name rules as a
|
||||||
// a constructor error, not a 502 the first time some caller happens to
|
// provider name — an MCP server or agent is routed at "/mcp/{name}/*" or
|
||||||
// address it.
|
// "/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 {
|
func validateTargetURLs(cfg *Config) error {
|
||||||
for name, tc := range cfg.MCPServers {
|
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 {
|
if err := validateTargetURL(tc.URL); err != nil {
|
||||||
return fmt.Errorf("llmgateway: mcpServers %q: %w", name, err)
|
return fmt.Errorf("llmgateway: mcpServers %q: %w", name, err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
for name, ac := range cfg.Agents {
|
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 {
|
if err := validateTargetURL(ac.URL); err != nil {
|
||||||
return fmt.Errorf("llmgateway: agents %q: %w", name, err)
|
return fmt.Errorf("llmgateway: agents %q: %w", name, err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -666,6 +666,36 @@ func TestNewGateway_TargetURLValidation(t *testing.T) {
|
|||||||
agents: map[string]*AgentConfig{"agent1": {URL: "http://agent1.internal:notaport"}},
|
agents: map[string]*AgentConfig{"agent1": {URL: "http://agent1.internal:notaport"}},
|
||||||
wantErr: true,
|
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 {
|
for _, tt := range tests {
|
||||||
t.Run(tt.name, func(t *testing.T) {
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
|||||||
@@ -657,4 +657,67 @@ func TestBuildAdapters(t *testing.T) {
|
|||||||
t.Fatalf("buildAdapters: want error for keyless gemini provider, got nil")
|
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)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+50
-9
@@ -8,6 +8,7 @@ import (
|
|||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"regexp"
|
||||||
"strings"
|
"strings"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -188,19 +189,59 @@ func upstreamJSON(ctx context.Context, client *http.Client, method, url string,
|
|||||||
return resp, nil
|
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
|
// buildAdapters resolves cfg.Providers into a map of providerAdapter keyed
|
||||||
// by provider name. For each provider it resolves APIKey via resolveSecret
|
// by provider name. Every provider name is validated (validateConfigName)
|
||||||
// (propagating that error) and defaults BaseURL by provider type when the
|
// and every ProviderConfig value must be non-nil before anything else runs,
|
||||||
// config omits one, trimming any trailing slash either way. An unknown
|
// so a malformed map key or a nil map value fails construction with a clear
|
||||||
// provider type is a config error. A resolved empty key is a valid
|
// error instead of a nil-pointer panic. For each provider it resolves
|
||||||
// keyless upstream for openai-type providers, but a constructor error for
|
// APIKey via resolveSecret (propagating that error) and defaults BaseURL by
|
||||||
// anthropic-type ones — newAnthropicAdapter enforces that, per ruling (c);
|
// provider type when the config omits one, trimming any trailing slash
|
||||||
// this function just propagates whatever error it returns. A configured
|
// either way. An unknown provider type is a config error. A resolved empty
|
||||||
// gemini provider follows the same constructor-error ruling —
|
// key is a valid keyless upstream for openai-type providers, but a
|
||||||
// newGeminiAdapter enforces it too.
|
// 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) {
|
func buildAdapters(cfg *Config) (map[string]providerAdapter, error) {
|
||||||
adapters := make(map[string]providerAdapter, len(cfg.Providers))
|
adapters := make(map[string]providerAdapter, len(cfg.Providers))
|
||||||
for name, pc := range 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)
|
apiKey, err := resolveSecret(pc.APIKey)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("llmgateway: provider %q: %w", name, err)
|
return nil, fmt.Errorf("llmgateway: provider %q: %w", name, err)
|
||||||
|
|||||||
+4
-3
@@ -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
|
// v0.16.1 panics interpreting a multi-value assignment mixing a
|
||||||
// variable and a literal in one statement on this exact live path (the
|
// 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
|
// same class of bug as sse.go's readSSE dispatch and registry.go's
|
||||||
// resolveAgainst — see those files' comments). Verified empirically
|
// resolveAgainst — see those files' comments). The hasMaxTokens=true
|
||||||
// under real Traefik (Task 15's integration suite, the gemini chat
|
// branch below is exercised by the integration suite's gemini chat
|
||||||
// completion path).
|
// request (integration/integration_test.go, TestUnifiedChatAllProviders,
|
||||||
|
// which sets "max_tokens") under real Traefik + Yaegi.
|
||||||
if v, ok := req["max_completion_tokens"]; ok {
|
if v, ok := req["max_completion_tokens"]; ok {
|
||||||
if n, ok2 := toInt64(v); ok2 {
|
if n, ok2 := toInt64(v); ok2 {
|
||||||
maxTokens = n
|
maxTokens = n
|
||||||
|
|||||||
Reference in New Issue
Block a user