diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a98309bf..d764bfd1 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "fmt" "os" "path/filepath" "testing" @@ -1060,3 +1061,42 @@ func TestServerConfig_SkipQuarantine_JSONSerialization(t *testing.T) { func boolPtr(b bool) *bool { return &b } + +// --- T011: DataDir secret-ref expansion in LoadFromFile --- + +// TestLoadConfig_ExpandsDataDir verifies that ${env:...} refs in data_dir are resolved +// before MkdirAll / Validate() run, so the database opens at the resolved path (US3). +func TestLoadConfig_ExpandsDataDir(t *testing.T) { + resolvedDir := t.TempDir() + t.Setenv("TEST_MCPPROXY_EXPAND_DATA_DIR", resolvedDir) + + cfgFile := filepath.Join(t.TempDir(), "config.json") + cfgData := `{"data_dir": "${env:TEST_MCPPROXY_EXPAND_DATA_DIR}"}` + require.NoError(t, os.WriteFile(cfgFile, []byte(cfgData), 0600)) + + cfg, err := LoadFromFile(cfgFile) + require.NoError(t, err) + assert.Equal(t, resolvedDir, cfg.DataDir) +} + +// TestLoadConfig_DataDirExpandFailure verifies that when the env var in data_dir is +// missing, LoadFromFile warns and retains the original unresolved reference rather +// than returning an error (US3 robustness requirement). +func TestLoadConfig_DataDirExpandFailure(t *testing.T) { + // Use a unique name that is almost certainly not set in any environment. + const missingVar = "TEST_MCPPROXY_MISSING_DATA_DIR_XYZ_9876" + os.Unsetenv(missingVar) //nolint:errcheck + + tmpBase := t.TempDir() + cfgFile := filepath.Join(t.TempDir(), "config.json") + // DataDir contains an unresolvable ref; the literal path lives inside tmpBase + // so any directory MkdirAll creates is cleaned up automatically. + cfgData := fmt.Sprintf(`{"data_dir": "%s/${env:%s}"}`, tmpBase, missingVar) + require.NoError(t, os.WriteFile(cfgFile, []byte(cfgData), 0600)) + + // LoadFromFile must succeed even when expansion fails — warn + retain original. + cfg, err := LoadFromFile(cfgFile) + require.NoError(t, err) + assert.Contains(t, cfg.DataDir, fmt.Sprintf("${env:%s}", missingVar), + "original unresolved ref should be retained when expansion fails") +} diff --git a/internal/config/loader.go b/internal/config/loader.go index 1ff5e836..6d6a93cb 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -1,6 +1,7 @@ package config import ( + "context" "crypto/rand" "encoding/hex" "encoding/json" @@ -10,6 +11,7 @@ import ( "strings" "time" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" "github.com/spf13/viper" ) @@ -38,6 +40,9 @@ func LoadFromFile(configPath string) (*Config, error) { cfg.DataDir = filepath.Join(homeDir, DefaultDataDir) } + // Expand secret/env refs in DataDir before creating it + expandDataDir(cfg) + // Create data directory if it doesn't exist if err := os.MkdirAll(cfg.DataDir, 0700); err != nil { return nil, fmt.Errorf("failed to create data directory %s: %w", cfg.DataDir, err) @@ -123,6 +128,9 @@ func Load() (*Config, error) { cfg.DataDir = filepath.Join(homeDir, DefaultDataDir) } + // Expand secret/env refs in DataDir before creating it + expandDataDir(cfg) + // Create data directory if it doesn't exist if err := os.MkdirAll(cfg.DataDir, 0700); err != nil { return nil, fmt.Errorf("failed to create data directory %s: %w", cfg.DataDir, err) @@ -460,6 +468,21 @@ func SetRegistriesInitCallback(callback func(*Config)) { registriesInitCallback = callback } +// expandDataDir expands secret/env refs in cfg.DataDir in place. +// Failures are logged to stderr and the original value is kept. +func expandDataDir(cfg *Config) { + if cfg.DataDir == "" { + return + } + resolver := secret.NewResolver() + resolved, err := resolver.ExpandSecretRefs(context.Background(), cfg.DataDir) + if err != nil { + fmt.Fprintf(os.Stderr, "WARN: Failed to resolve secret ref in data_dir, using original value: reference=%s err=%v\n", cfg.DataDir, err) + return + } + cfg.DataDir = resolved +} + // applyTLSEnvOverrides applies environment variable overrides for TLS configuration func applyTLSEnvOverrides(cfg *Config) { // Ensure TLS config is initialized diff --git a/internal/config/merge.go b/internal/config/merge.go index 2532ce6c..0eb974ad 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -163,14 +163,14 @@ func MergeServerConfig(base, patch *ServerConfig, opts MergeOptions) (*ServerCon return nil, nil, fmt.Errorf("%w: both base and patch are nil", ErrInvalidConfig) } // Return a copy of patch - merged := copyServerConfig(patch) + merged := CopyServerConfig(patch) merged.Updated = time.Now() return merged, NewConfigDiff(), nil } if patch == nil { // If no patch, return a copy of base - merged := copyServerConfig(base) + merged := CopyServerConfig(base) return merged, nil, nil } @@ -189,7 +189,7 @@ func MergeServerConfig(base, patch *ServerConfig, opts MergeOptions) (*ServerCon } // Start with a copy of base - merged := copyServerConfig(base) + merged := CopyServerConfig(base) // Track changes if requested var diff *ConfigDiff @@ -522,7 +522,7 @@ func MergeOAuthConfig(base, patch *OAuthConfig, removeIfNil bool) *OAuthConfig { // Helper functions to copy configs (avoiding pointer aliasing) -func copyServerConfig(src *ServerConfig) *ServerConfig { +func CopyServerConfig(src *ServerConfig) *ServerConfig { if src == nil { return nil } diff --git a/internal/secret/resolver.go b/internal/secret/resolver.go index 4010b6ea..7b73150b 100644 --- a/internal/secret/resolver.go +++ b/internal/secret/resolver.go @@ -179,6 +179,120 @@ func (r *Resolver) expandValue(ctx context.Context, v reflect.Value) error { return nil } +// SecretExpansionError records a failure to resolve a single secret reference during struct expansion. +type SecretExpansionError struct { + FieldPath string // e.g. "WorkingDir", "Isolation.WorkingDir", "Args[0]", "Env[MY_VAR]" + Reference string // the original unresolved reference pattern, e.g. "${env:HOME}" + Err error +} + +// ExpandStructSecretsCollectErrors expands secret references in all string fields of v. +// Unlike ExpandStructSecrets, it does not fail fast: it collects all expansion errors and +// continues processing remaining fields. On error, the field retains its original value. +// v must be a non-nil pointer to a struct. +func (r *Resolver) ExpandStructSecretsCollectErrors(ctx context.Context, v interface{}) []SecretExpansionError { + var errs []SecretExpansionError + r.expandValueCollectErrors(ctx, reflect.ValueOf(v), "", &errs) + return errs +} + +// expandValueCollectErrors mirrors expandValue but tracks field paths and collects errors +// instead of returning on the first failure. On resolution error the field is left unchanged. +func (r *Resolver) expandValueCollectErrors(ctx context.Context, v reflect.Value, path string, errs *[]SecretExpansionError) { + if !v.IsValid() { + return + } + + // Handle pointers + if v.Kind() == reflect.Ptr { + if v.IsNil() { + return + } + r.expandValueCollectErrors(ctx, v.Elem(), path, errs) + return + } + + switch v.Kind() { + case reflect.String: + if v.CanSet() { + original := v.String() + if IsSecretRef(original) { + expanded, err := r.ExpandSecretRefs(ctx, original) + if err != nil { + *errs = append(*errs, SecretExpansionError{ + FieldPath: path, + Reference: original, + Err: err, + }) + // retain original value on failure — do not call SetString + } else { + v.SetString(expanded) + } + } + } + + case reflect.Struct: + t := v.Type() + for i := 0; i < v.NumField(); i++ { + field := v.Field(i) + if !field.CanInterface() { + continue + } + fieldType := t.Field(i) + if !fieldType.IsExported() { + continue + } + fieldName := fieldType.Name + newPath := fieldName + if path != "" { + newPath = path + "." + fieldName + } + r.expandValueCollectErrors(ctx, field, newPath, errs) + } + + case reflect.Slice, reflect.Array: + for i := 0; i < v.Len(); i++ { + newPath := fmt.Sprintf("%s[%d]", path, i) + r.expandValueCollectErrors(ctx, v.Index(i), newPath, errs) + } + + case reflect.Map: + for _, key := range v.MapKeys() { + keyStr := fmt.Sprintf("%v", key.Interface()) + newPath := fmt.Sprintf("%s[%s]", path, keyStr) + mapValue := v.MapIndex(key) + if mapValue.Kind() == reflect.String && IsSecretRef(mapValue.String()) { + original := mapValue.String() + expanded, err := r.ExpandSecretRefs(ctx, original) + if err != nil { + *errs = append(*errs, SecretExpansionError{ + FieldPath: newPath, + Reference: original, + Err: err, + }) + } else { + v.SetMapIndex(key, reflect.ValueOf(expanded)) + } + } else if mapValue.Kind() == reflect.Interface { + actualValue := mapValue.Elem() + if actualValue.Kind() == reflect.String && IsSecretRef(actualValue.String()) { + original := actualValue.String() + expanded, err := r.ExpandSecretRefs(ctx, original) + if err != nil { + *errs = append(*errs, SecretExpansionError{ + FieldPath: newPath, + Reference: original, + Err: err, + }) + } else { + v.SetMapIndex(key, reflect.ValueOf(expanded)) + } + } + } + } + } +} + // ExtractConfigSecrets extracts all secret and environment references from a config structure func (r *Resolver) ExtractConfigSecrets(ctx context.Context, v interface{}) (*ConfigSecretsResponse, error) { allRefs := []Ref{} diff --git a/internal/secret/resolver_test.go b/internal/secret/resolver_test.go index 863cf12f..5c1834f3 100644 --- a/internal/secret/resolver_test.go +++ b/internal/secret/resolver_test.go @@ -2,6 +2,7 @@ package secret import ( "context" + "errors" "testing" "github.com/stretchr/testify/assert" @@ -237,3 +238,229 @@ func TestNewResolver(t *testing.T) { assert.Contains(t, resolver.providers, "env") assert.Contains(t, resolver.providers, "keyring") } + +// --- Tests for ExpandStructSecretsCollectErrors --- + +func TestExpandStructSecretsCollectErrors_HappyPath(t *testing.T) { + type simpleConfig struct { + WorkingDir string + URL string + } + s := &simpleConfig{ + WorkingDir: "${mock:work-dir}", + URL: "https://plain.example.com", + } + + mockProvider := &MockProvider{} + r := &Resolver{providers: make(map[string]Provider)} + r.RegisterProvider("mock", mockProvider) + + ctx := context.Background() + mockProvider.On("CanResolve", "mock").Return(true) + mockProvider.On("IsAvailable").Return(true) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "work-dir", Original: "${mock:work-dir}"}).Return("/home/user/project", nil) + + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Empty(t, errs) + assert.Equal(t, "/home/user/project", s.WorkingDir) + assert.Equal(t, "https://plain.example.com", s.URL) // plain values untouched + mockProvider.AssertExpectations(t) +} + +func TestExpandStructSecretsCollectErrors_PartialFailure(t *testing.T) { + type twoFieldConfig struct { + URL string + WorkingDir string + } + s := &twoFieldConfig{ + URL: "${mock:my-url}", + WorkingDir: "${mock:missing-dir}", + } + + mockProvider := &MockProvider{} + r := &Resolver{providers: make(map[string]Provider)} + r.RegisterProvider("mock", mockProvider) + + ctx := context.Background() + mockProvider.On("CanResolve", "mock").Return(true) + mockProvider.On("IsAvailable").Return(true) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "my-url", Original: "${mock:my-url}"}).Return("https://resolved.example.com", nil) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "missing-dir", Original: "${mock:missing-dir}"}).Return("", errors.New("secret not found")) + + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Len(t, errs, 1) + assert.Equal(t, "WorkingDir", errs[0].FieldPath) + assert.Equal(t, "${mock:missing-dir}", errs[0].Reference) + assert.Error(t, errs[0].Err) + // Successful field is resolved; failed field retains original value + assert.Equal(t, "https://resolved.example.com", s.URL) + assert.Equal(t, "${mock:missing-dir}", s.WorkingDir) + mockProvider.AssertExpectations(t) +} + +func TestExpandStructSecretsCollectErrors_NilPointer(t *testing.T) { + type nested struct { + WorkingDir string + } + type configWithOptional struct { + WorkingDir string + Isolation *nested + } + s := &configWithOptional{WorkingDir: "no-refs", Isolation: nil} + + r := &Resolver{providers: make(map[string]Provider)} + ctx := context.Background() + + // Should not panic on nil nested pointer + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Empty(t, errs) + assert.Equal(t, "no-refs", s.WorkingDir) +} + +func TestExpandStructSecretsCollectErrors_NestedStruct(t *testing.T) { + type isolationConfig struct { + WorkingDir string + } + type serverConfig struct { + Isolation *isolationConfig + } + + mockProvider := &MockProvider{} + r := &Resolver{providers: make(map[string]Provider)} + r.RegisterProvider("mock", mockProvider) + ctx := context.Background() + + t.Run("success expands nested field", func(t *testing.T) { + s := &serverConfig{Isolation: &isolationConfig{WorkingDir: "${mock:iso-dir}"}} + mockProvider.On("CanResolve", "mock").Return(true) + mockProvider.On("IsAvailable").Return(true) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "iso-dir", Original: "${mock:iso-dir}"}).Return("/isolation/dir", nil) + + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Empty(t, errs) + assert.Equal(t, "/isolation/dir", s.Isolation.WorkingDir) + mockProvider.AssertExpectations(t) + }) + + t.Run("failure reports nested field path", func(t *testing.T) { + mockFail := &MockProvider{} + rFail := &Resolver{providers: make(map[string]Provider)} + rFail.RegisterProvider("mock", mockFail) + + s := &serverConfig{Isolation: &isolationConfig{WorkingDir: "${mock:missing}"}} + mockFail.On("CanResolve", "mock").Return(true) + mockFail.On("IsAvailable").Return(true) + mockFail.On("Resolve", ctx, Ref{Type: "mock", Name: "missing", Original: "${mock:missing}"}).Return("", errors.New("not found")) + + errs := rFail.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Len(t, errs, 1) + assert.Equal(t, "Isolation.WorkingDir", errs[0].FieldPath) + assert.Equal(t, "${mock:missing}", errs[0].Reference) + // Original value retained on failure + assert.Equal(t, "${mock:missing}", s.Isolation.WorkingDir) + mockFail.AssertExpectations(t) + }) +} + +func TestExpandStructSecretsCollectErrors_SliceField(t *testing.T) { + type configWithArgs struct { + Args []string + } + s := &configWithArgs{Args: []string{"${mock:arg0}", "${mock:arg1}"}} + + mockProvider := &MockProvider{} + r := &Resolver{providers: make(map[string]Provider)} + r.RegisterProvider("mock", mockProvider) + + ctx := context.Background() + mockProvider.On("CanResolve", "mock").Return(true) + mockProvider.On("IsAvailable").Return(true) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "arg0", Original: "${mock:arg0}"}).Return("resolved-arg0", nil) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "arg1", Original: "${mock:arg1}"}).Return("resolved-arg1", nil) + + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Empty(t, errs) + assert.Equal(t, []string{"resolved-arg0", "resolved-arg1"}, s.Args) + mockProvider.AssertExpectations(t) + + // Verify failure reports correct path "Args[0]" + mockFail := &MockProvider{} + rFail := &Resolver{providers: make(map[string]Provider)} + rFail.RegisterProvider("mock", mockFail) + + sFail := &configWithArgs{Args: []string{"${mock:missing}"}} + mockFail.On("CanResolve", "mock").Return(true) + mockFail.On("IsAvailable").Return(true) + mockFail.On("Resolve", ctx, Ref{Type: "mock", Name: "missing", Original: "${mock:missing}"}).Return("", errors.New("not found")) + + errsFail := rFail.ExpandStructSecretsCollectErrors(ctx, sFail) + assert.Len(t, errsFail, 1) + assert.Equal(t, "Args[0]", errsFail[0].FieldPath) + mockFail.AssertExpectations(t) +} + +func TestExpandStructSecretsCollectErrors_MapField(t *testing.T) { + type configWithEnv struct { + Env map[string]string + } + s := &configWithEnv{Env: map[string]string{"MY_VAR": "${mock:my-secret}"}} + + mockProvider := &MockProvider{} + r := &Resolver{providers: make(map[string]Provider)} + r.RegisterProvider("mock", mockProvider) + + ctx := context.Background() + mockProvider.On("CanResolve", "mock").Return(true) + mockProvider.On("IsAvailable").Return(true) + mockProvider.On("Resolve", ctx, Ref{Type: "mock", Name: "my-secret", Original: "${mock:my-secret}"}).Return("resolved-secret", nil) + + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Empty(t, errs) + assert.Equal(t, "resolved-secret", s.Env["MY_VAR"]) + mockProvider.AssertExpectations(t) + + // Verify failure reports correct path "Env[MY_VAR]" + mockFail := &MockProvider{} + rFail := &Resolver{providers: make(map[string]Provider)} + rFail.RegisterProvider("mock", mockFail) + + sFail := &configWithEnv{Env: map[string]string{"MY_VAR": "${mock:missing}"}} + mockFail.On("CanResolve", "mock").Return(true) + mockFail.On("IsAvailable").Return(true) + mockFail.On("Resolve", ctx, Ref{Type: "mock", Name: "missing", Original: "${mock:missing}"}).Return("", errors.New("not found")) + + errsFail := rFail.ExpandStructSecretsCollectErrors(ctx, sFail) + assert.Len(t, errsFail, 1) + assert.Equal(t, "Env[MY_VAR]", errsFail[0].FieldPath) + mockFail.AssertExpectations(t) +} + +func TestExpandStructSecretsCollectErrors_NoRefs(t *testing.T) { + type simpleConfig struct { + WorkingDir string + URL string + Args []string + } + s := &simpleConfig{ + WorkingDir: "/absolute/path", + URL: "https://plain.example.com", + Args: []string{"--flag", "value"}, + } + + r := &Resolver{providers: make(map[string]Provider)} + ctx := context.Background() + + errs := r.ExpandStructSecretsCollectErrors(ctx, s) + + assert.Empty(t, errs) + assert.Equal(t, "/absolute/path", s.WorkingDir) + assert.Equal(t, "https://plain.example.com", s.URL) + assert.Equal(t, []string{"--flag", "value"}, s.Args) +} diff --git a/internal/upstream/core/client.go b/internal/upstream/core/client.go index dfa6576f..874597b0 100644 --- a/internal/upstream/core/client.go +++ b/internal/upstream/core/client.go @@ -102,88 +102,27 @@ func NewClient(id string, serverConfig *config.ServerConfig, logger *zap.Logger, // NewClientWithOptions creates a new core MCP client with additional options func NewClientWithOptions(id string, serverConfig *config.ServerConfig, logger *zap.Logger, logConfig *config.LogConfig, globalConfig *config.Config, storage *storage.BoltDB, cliDebugMode bool, secretResolver *secret.Resolver) (*Client, error) { - // Resolve secrets in server config before using it - resolvedServerConfig := *serverConfig // Create a copy + // Deep-copy the config so expansion never mutates the caller's value (FR-004). + // ExpandStructSecretsCollectErrors resolves ${env:...} and ${keyring:...} refs in + // every string field of ServerConfig and its nested structs (IsolationConfig, OAuthConfig) + // without an explicit per-field allowlist (FR-001 / US2). + resolvedServerConfig := config.CopyServerConfig(serverConfig) if secretResolver != nil { - // Create a context for secret resolution ctx := context.Background() - - // Resolve secrets in environment variables - if len(resolvedServerConfig.Env) > 0 { - resolvedEnv := make(map[string]string) - for k, v := range resolvedServerConfig.Env { - resolvedValue, err := secretResolver.ExpandSecretRefs(ctx, v) - if err != nil { - logger.Error("CRITICAL: Failed to resolve secret in environment variable - server will use UNRESOLVED placeholder", - zap.String("server", serverConfig.Name), - zap.String("env_var", k), - zap.String("reference", v), - zap.Error(err), - zap.String("help", "Use Web UI (http://localhost:8080/ui/) or API to add the secret to keyring")) - resolvedValue = v // Use original value on error - THIS IS THE PROBLEM! - } else if resolvedValue != v { - logger.Debug("Secret resolved successfully", - zap.String("server", serverConfig.Name), - zap.String("env_var", k), - zap.String("reference", v)) - } - resolvedEnv[k] = resolvedValue - } - resolvedServerConfig.Env = resolvedEnv - } - - // Resolve secrets in arguments - if len(resolvedServerConfig.Args) > 0 { - resolvedArgs := make([]string, len(resolvedServerConfig.Args)) - for i, arg := range resolvedServerConfig.Args { - resolvedValue, err := secretResolver.ExpandSecretRefs(ctx, arg) - if err != nil { - logger.Error("CRITICAL: Failed to resolve secret in argument - server will use UNRESOLVED placeholder", - zap.String("server", serverConfig.Name), - zap.Int("arg_index", i), - zap.String("reference", arg), - zap.Error(err), - zap.String("help", "Use Web UI (http://localhost:8080/ui/) or API to add the secret to keyring")) - resolvedValue = arg // Use original value on error - THIS IS THE PROBLEM! - } else if resolvedValue != arg { - logger.Debug("Secret resolved successfully", - zap.String("server", serverConfig.Name), - zap.Int("arg_index", i), - zap.String("reference", arg)) - } - resolvedArgs[i] = resolvedValue - } - resolvedServerConfig.Args = resolvedArgs - } - - // Resolve secrets in headers - if len(resolvedServerConfig.Headers) > 0 { - resolvedHeaders := make(map[string]string) - for k, v := range resolvedServerConfig.Headers { - resolvedValue, err := secretResolver.ExpandSecretRefs(ctx, v) - if err != nil { - logger.Error("CRITICAL: Failed to resolve secret in header - server will use UNRESOLVED placeholder", - zap.String("server", serverConfig.Name), - zap.String("header", k), - zap.String("reference", v), - zap.Error(err), - zap.String("help", "Use Web UI (http://localhost:8080/ui/) or API to add the secret to keyring")) - resolvedValue = v - } else if resolvedValue != v { - logger.Debug("Secret resolved successfully", - zap.String("server", serverConfig.Name), - zap.String("header", k), - zap.String("reference", v)) - } - resolvedHeaders[k] = resolvedValue - } - resolvedServerConfig.Headers = resolvedHeaders + errs := secretResolver.ExpandStructSecretsCollectErrors(ctx, resolvedServerConfig) + for _, e := range errs { + logger.Error("CRITICAL: Failed to resolve secret reference - field will use UNRESOLVED placeholder", + zap.String("server", serverConfig.Name), + zap.String("field", e.FieldPath), + zap.String("reference", e.Reference), + zap.Error(e.Err), + zap.String("help", "Use Web UI (http://localhost:8080/ui/) or API to add the secret to keyring")) } } c := &Client{ id: id, - config: &resolvedServerConfig, // Use resolved config + config: resolvedServerConfig, globalConfig: globalConfig, storage: storage, secretResolver: secretResolver, // Store resolver for future use diff --git a/internal/upstream/core/client_secret_test.go b/internal/upstream/core/client_secret_test.go index 4660ed23..e0296640 100644 --- a/internal/upstream/core/client_secret_test.go +++ b/internal/upstream/core/client_secret_test.go @@ -2,6 +2,8 @@ package core import ( "context" + "fmt" + "reflect" "testing" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" @@ -213,3 +215,269 @@ func TestClient_HeaderSecretResolution(t *testing.T) { assert.Equal(t, "static-value", client.config.Headers["X-Custom"]) }) } + +// --- T007/T008: NewClientWithOptions struct-wide secret expansion --- + +// makeResolverWithMock creates a Resolver with a "mock" provider for testing. +func makeResolverWithMock(mockP *MockSecretProvider) *secret.Resolver { + r := secret.NewResolver() + r.RegisterProvider("mock", mockP) + return r +} + +// makeTempDB creates a throwaway BoltDB for a test and schedules cleanup. +func makeTempDB(t *testing.T, logger *zap.Logger) *storage.BoltDB { + t.Helper() + db, err := storage.NewBoltDB(t.TempDir()+"/test.db", logger.Sugar()) + if err == nil { + t.Cleanup(func() { db.Close() }) + } + return db +} + +func TestNewClientWithOptions_ExpandsWorkingDir(t *testing.T) { + logger := zap.NewNop() + db := makeTempDB(t, logger) + ctx := context.Background() + + mockP := &MockSecretProvider{} + resolver := makeResolverWithMock(mockP) + + serverConfig := &config.ServerConfig{ + Name: "test-server", + Protocol: "stdio", + Command: "echo", + WorkingDir: "${mock:work-dir}", + } + + mockP.On("CanResolve", "mock").Return(true) + mockP.On("IsAvailable").Return(true) + mockP.On("Resolve", ctx, secret.Ref{ + Type: "mock", + Name: "work-dir", + Original: "${mock:work-dir}", + }).Return("/resolved/work-dir", nil) + + client, err := NewClientWithOptions("test-expand-wd", serverConfig, logger, &config.LogConfig{}, &config.Config{}, db, false, resolver) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.Equal(t, "/resolved/work-dir", client.config.WorkingDir) + mockP.AssertExpectations(t) +} + +func TestNewClientWithOptions_ExpandsIsolationWorkingDir(t *testing.T) { + logger := zap.NewNop() + db := makeTempDB(t, logger) + ctx := context.Background() + + mockP := &MockSecretProvider{} + resolver := makeResolverWithMock(mockP) + + serverConfig := &config.ServerConfig{ + Name: "test-server", + Protocol: "stdio", + Command: "echo", + Isolation: &config.IsolationConfig{ + WorkingDir: "${mock:iso-dir}", + }, + } + + mockP.On("CanResolve", "mock").Return(true) + mockP.On("IsAvailable").Return(true) + mockP.On("Resolve", ctx, secret.Ref{ + Type: "mock", + Name: "iso-dir", + Original: "${mock:iso-dir}", + }).Return("/resolved/iso-dir", nil) + + client, err := NewClientWithOptions("test-expand-iso-wd", serverConfig, logger, &config.LogConfig{}, &config.Config{}, db, false, resolver) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.NotNil(t, client.config.Isolation) + assert.Equal(t, "/resolved/iso-dir", client.config.Isolation.WorkingDir) + mockP.AssertExpectations(t) +} + +func TestNewClientWithOptions_ExpandsURL(t *testing.T) { + logger := zap.NewNop() + db := makeTempDB(t, logger) + ctx := context.Background() + + mockP := &MockSecretProvider{} + resolver := makeResolverWithMock(mockP) + + serverConfig := &config.ServerConfig{ + Name: "test-server", + Protocol: "http", + URL: "https://${mock:api-host}/mcp", + } + + mockP.On("CanResolve", "mock").Return(true) + mockP.On("IsAvailable").Return(true) + mockP.On("Resolve", ctx, secret.Ref{ + Type: "mock", + Name: "api-host", + Original: "${mock:api-host}", + }).Return("api.example.com", nil) + + client, err := NewClientWithOptions("test-expand-url", serverConfig, logger, &config.LogConfig{}, &config.Config{}, db, false, resolver) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.Equal(t, "https://api.example.com/mcp", client.config.URL) + mockP.AssertExpectations(t) +} + +func TestNewClientWithOptions_PreservesExistingEnvArgsHeaders(t *testing.T) { + logger := zap.NewNop() + db := makeTempDB(t, logger) + ctx := context.Background() + + mockP := &MockSecretProvider{} + resolver := makeResolverWithMock(mockP) + + serverConfig := &config.ServerConfig{ + Name: "test-server", + Protocol: "stdio", + Command: "echo", + Env: map[string]string{"MY_VAR": "${mock:env-val}"}, + Args: []string{"${mock:arg-val}"}, + Headers: map[string]string{"X-Key": "${mock:header-val}"}, + } + + mockP.On("CanResolve", "mock").Return(true) + mockP.On("IsAvailable").Return(true) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "env-val", Original: "${mock:env-val}"}).Return("resolved-env", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "arg-val", Original: "${mock:arg-val}"}).Return("resolved-arg", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "header-val", Original: "${mock:header-val}"}).Return("resolved-header", nil) + + client, err := NewClientWithOptions("test-preserve-existing", serverConfig, logger, &config.LogConfig{}, &config.Config{}, db, false, resolver) + + assert.NoError(t, err) + assert.NotNil(t, client) + // FR-008: existing field expansion must not regress + assert.Equal(t, "resolved-env", client.config.Env["MY_VAR"]) + assert.Equal(t, "resolved-arg", client.config.Args[0]) + assert.Equal(t, "resolved-header", client.config.Headers["X-Key"]) + mockP.AssertExpectations(t) +} + +func TestNewClientWithOptions_DoesNotMutateOriginal(t *testing.T) { + logger := zap.NewNop() + db := makeTempDB(t, logger) + ctx := context.Background() + + mockP := &MockSecretProvider{} + resolver := makeResolverWithMock(mockP) + + serverConfig := &config.ServerConfig{ + Name: "test-server", + Protocol: "stdio", + Command: "echo", + WorkingDir: "${mock:work-dir}", + URL: "${mock:url}", + Env: map[string]string{"MY_VAR": "${mock:env-val}"}, + } + + mockP.On("CanResolve", "mock").Return(true) + mockP.On("IsAvailable").Return(true) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "work-dir", Original: "${mock:work-dir}"}).Return("/resolved/dir", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "url", Original: "${mock:url}"}).Return("https://resolved.example.com", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "env-val", Original: "${mock:env-val}"}).Return("resolved-env", nil) + + _, err := NewClientWithOptions("test-no-mutate", serverConfig, logger, &config.LogConfig{}, &config.Config{}, db, false, resolver) + + assert.NoError(t, err) + // FR-004: original ServerConfig must not be mutated after call + assert.Equal(t, "${mock:work-dir}", serverConfig.WorkingDir) + assert.Equal(t, "${mock:url}", serverConfig.URL) + assert.Equal(t, "${mock:env-val}", serverConfig.Env["MY_VAR"]) +} + +// TestNewClientWithOptions_ReflectionRegressionTest (T008) walks all string fields of the +// resolved client config via reflection and asserts that none still match IsSecretRef(). +// This catches any future ServerConfig string field that isn't covered by expansion (SC-004). +func TestNewClientWithOptions_ReflectionRegressionTest(t *testing.T) { + logger := zap.NewNop() + db := makeTempDB(t, logger) + ctx := context.Background() + + mockP := &MockSecretProvider{} + resolver := makeResolverWithMock(mockP) + + serverConfig := &config.ServerConfig{ + Name: "test-server", + Protocol: "stdio", + Command: "echo", + WorkingDir: "${mock:work-dir}", + URL: "${mock:url}", + Env: map[string]string{"MY_VAR": "${mock:env-val}"}, + Args: []string{"${mock:arg-val}"}, + Headers: map[string]string{"X-Key": "${mock:header-val}"}, + Isolation: &config.IsolationConfig{ + WorkingDir: "${mock:iso-work-dir}", + Image: "${mock:iso-image}", + }, + } + + mockP.On("CanResolve", "mock").Return(true) + mockP.On("IsAvailable").Return(true) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "work-dir", Original: "${mock:work-dir}"}).Return("/resolved/work-dir", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "url", Original: "${mock:url}"}).Return("https://resolved.example.com/mcp", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "env-val", Original: "${mock:env-val}"}).Return("resolved-env", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "arg-val", Original: "${mock:arg-val}"}).Return("resolved-arg", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "header-val", Original: "${mock:header-val}"}).Return("resolved-header", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "iso-work-dir", Original: "${mock:iso-work-dir}"}).Return("/resolved/iso-work-dir", nil) + mockP.On("Resolve", ctx, secret.Ref{Type: "mock", Name: "iso-image", Original: "${mock:iso-image}"}).Return("myimage:latest", nil) + + client, err := NewClientWithOptions("test-reflection", serverConfig, logger, &config.LogConfig{}, &config.Config{}, db, false, resolver) + + assert.NoError(t, err) + assert.NotNil(t, client) + + // Walk all string fields of the resolved config and collect any remaining refs. + var unresolvedRefs []string + collectSecretRefs(reflect.ValueOf(client.config), "", &unresolvedRefs) + assert.Empty(t, unresolvedRefs, "all secret refs should be resolved; still unresolved: %v", unresolvedRefs) +} + +// collectSecretRefs recursively walks v and appends "path=value" for any string +// matching secret.IsSecretRef to found. Used by the reflection regression test. +func collectSecretRefs(v reflect.Value, path string, found *[]string) { + if !v.IsValid() { + return + } + if v.Kind() == reflect.Ptr { + if v.IsNil() { + return + } + collectSecretRefs(v.Elem(), path, found) + return + } + switch v.Kind() { + case reflect.String: + if secret.IsSecretRef(v.String()) { + *found = append(*found, fmt.Sprintf("%s=%s", path, v.String())) + } + case reflect.Struct: + t := v.Type() + for i := 0; i < v.NumField(); i++ { + fieldName := t.Field(i).Name + newPath := fieldName + if path != "" { + newPath = path + "." + fieldName + } + collectSecretRefs(v.Field(i), newPath, found) + } + case reflect.Slice, reflect.Array: + for i := 0; i < v.Len(); i++ { + collectSecretRefs(v.Index(i), fmt.Sprintf("%s[%d]", path, i), found) + } + case reflect.Map: + for _, key := range v.MapKeys() { + collectSecretRefs(v.MapIndex(key), fmt.Sprintf("%s[%v]", path, key.Interface()), found) + } + } +} diff --git a/specs/034-expand-secret-refs/checklists/requirements.md b/specs/034-expand-secret-refs/checklists/requirements.md new file mode 100644 index 00000000..a48c3c83 --- /dev/null +++ b/specs/034-expand-secret-refs/checklists/requirements.md @@ -0,0 +1,37 @@ +# Specification Quality Checklist: Expand Secret/Env Refs in All Config String Fields + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-03-10 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- All items pass validation. +- The Assumptions section references existing codebase capabilities (reflection-based expansion, deep-copy function) at a conceptual level without naming specific functions, types, or files — this is acceptable for bridging spec to plan. +- No [NEEDS CLARIFICATION] markers were needed. The issue #333 provided a clear problem statement and the design space is well-constrained. +- Clarification session 2026-03-10: 3 questions asked and answered. FR-003 updated with correct log levels (ERROR/DEBUG), security constraint added (no resolved values in logs), empty string resolution behavior documented. diff --git a/specs/034-expand-secret-refs/plan.md b/specs/034-expand-secret-refs/plan.md new file mode 100644 index 00000000..0f329408 --- /dev/null +++ b/specs/034-expand-secret-refs/plan.md @@ -0,0 +1,239 @@ +# Implementation Plan: Expand Secret/Env Refs in All Config String Fields + +**Branch**: `034-expand-secret-refs` | **Date**: 2026-03-10 | **Spec**: [spec.md](./spec.md) +**Issue**: [#333](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/333) + +## Summary + +Expand `${env:...}` and `${keyring:...}` references in all string fields of `ServerConfig` (not just the current 3 fields) and in `Config.DataDir`. Replace the manual field-by-field expansion in `NewClientWithOptions` with a new reflection-based `ExpandStructSecretsCollectErrors` method that collects errors instead of failing fast. This automatically covers all current and future string fields without code changes. + +## Technical Context + +**Language/Version**: Go 1.24 (toolchain go1.24.10) +**Primary Dependencies**: `go.uber.org/zap` (logging), `internal/secret` (resolver, parser), `internal/config` (ServerConfig, IsolationConfig, OAuthConfig, merge), `context` (stdlib), `reflect` (stdlib) +**Storage**: N/A — no new storage; existing BBolt database unaffected +**Testing**: `go test -race ./...`, `testify/assert`, `testify/mock` (MockProvider already defined in `resolver_test.go`) +**Target Platform**: macOS/Linux/Windows (cross-platform, no platform-specific code in scope) +**Performance Goals**: Expansion runs once per server at startup (not hot path). Reflection overhead is negligible (<1ms per server). +**Constraints**: Must preserve identical error/log behavior for existing fields (`env`, `args`, `headers`). Must not mutate original `ServerConfig`. Empty string is a valid resolved value. +**Scale/Scope**: 7 files modified, 1 new file created. ~78 lines replaced with ~12 lines in `client.go`. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +| Principle | Status | Notes | +|-----------|--------|-------| +| I. Performance at Scale | ✅ Pass | Expansion is startup-time only, not on the tool routing hot path | +| II. Actor-Based Concurrency | ✅ Pass | No new goroutines or shared state introduced | +| III. Configuration-Driven Architecture | ✅ Pass | Directly enhances config-driven architecture by making more fields configurable | +| IV. Security by Default | ✅ Pass | FR-003: resolved values never logged. Existing security surface unchanged. | +| V. Test-Driven Development | ✅ Pass | Tests written before implementation (see Phase 1 task order) | +| VI. Documentation Hygiene | ✅ Pass | CLAUDE.md `Active Technologies` updated by agent context script | + +**Complexity Tracking**: No constitution violations. No new abstractions. Replacing complex manual code with a simpler, more general approach. + +## Project Structure + +### Documentation (this feature) + +```text +specs/034-expand-secret-refs/ +├── spec.md ✅ complete +├── research.md ✅ complete (this phase) +├── plan.md ✅ this file +├── checklists/ +│ └── requirements.md ✅ complete +└── tasks.md (Phase 2 — /speckit.tasks) +``` + +### Source Code (affected files) + +```text +internal/secret/ +├── resolver.go # Add ExpandStructSecretsCollectErrors + SecretExpansionError +└── resolver_test.go # Add tests for new method + +internal/config/ +├── merge.go # Export CopyServerConfig (rename + 3 internal call sites) +├── loader.go # Expand DataDir before Validate() (2 call sites) +└── config_test.go # Add DataDir expansion test + +internal/upstream/core/ +├── client.go # Replace lines 107-182 with struct expansion +└── client_secret_test.go # New: reflection-based regression test (NEW FILE) +``` + +## Phase 0: Research + +**Status: Complete** — see `research.md` for all decisions. + +Key findings: +- `ExpandStructSecrets` fails fast, no path tracking → new `ExpandStructSecretsCollectErrors` needed +- `extractSecretRefs` already has full path tracking → mirror that pattern +- `copyServerConfig` (merge.go:525) handles full deep copy → export it +- `DataDir` expansion belongs in `loader.go` (2 call sites), not in `Validate()` +- Zero existing tests for `ExpandStructSecrets` → TDD from scratch + +## Phase 1: Implementation Plan + +### Step 1 — Add `ExpandStructSecretsCollectErrors` (TDD: test first) + +**File**: `internal/secret/resolver.go` and `resolver_test.go` + +**Tests first** (`resolver_test.go`): +``` +TestExpandStructSecretsCollectErrors_HappyPath + - struct with ${env:...} refs in string field → all resolved, empty error slice +TestExpandStructSecretsCollectErrors_PartialFailure + - struct with 2 fields, 1 resolves, 1 fails → errors contain failed field path, successful field is resolved, failed field retains original +TestExpandStructSecretsCollectErrors_NilPointer + - struct with nil *IsolationConfig → no panic, no errors +TestExpandStructSecretsCollectErrors_NestedStruct + - struct with nested struct pointer containing refs → all expanded with path "Isolation.WorkingDir" +TestExpandStructSecretsCollectErrors_SliceField + - struct with []string args containing refs → expanded with paths "Args[0]", "Args[1]" +TestExpandStructSecretsCollectErrors_MapField + - struct with map[string]string env containing refs → expanded with paths "Env[KEY]" +TestExpandStructSecretsCollectErrors_NoRefs + - struct with plain values → empty error slice, values unchanged +``` + +**Implementation** (`resolver.go`): +```go +type SecretExpansionError struct { + FieldPath string + Reference string + Err error +} + +func (r *Resolver) ExpandStructSecretsCollectErrors(ctx context.Context, v interface{}) []SecretExpansionError +// Internal: expandValueCollectErrors(ctx, reflect.Value, path string, errs *[]SecretExpansionError) +// Mirrors expandValue but: tracks path, appends to errs instead of returning, retains original on failure +``` + +The path format matches `extractSecretRefs`: `"WorkingDir"`, `"Isolation.WorkingDir"`, `"Args[0]"`, `"Env[MY_VAR]"`. + +--- + +### Step 2 — Export `CopyServerConfig` (TDD: verify existing tests still pass) + +**File**: `internal/config/merge.go` + +Rename `copyServerConfig` → `CopyServerConfig` at line 525. Update 3 call sites in the same file. No behavior change. + +Verify: `go test ./internal/config/... -race` (existing merge tests must pass). + +--- + +### Step 3 — Replace manual expansion in `NewClientWithOptions` (TDD: test first) + +**File**: `internal/upstream/core/client_secret_test.go` (new) and `client.go` + +**Tests first** (`client_secret_test.go`): +``` +TestNewClientWithOptions_ExpandsWorkingDir + - ServerConfig{WorkingDir: "${env:TEST_DIR}"} → resolved path used +TestNewClientWithOptions_ExpandsIsolationWorkingDir + - ServerConfig{Isolation: &IsolationConfig{WorkingDir: "${env:TEST_DIR}"}} → resolved +TestNewClientWithOptions_ExpandsURL + - ServerConfig{URL: "https://${env:API_HOST}/mcp"} → resolved +TestNewClientWithOptions_PreservesExistingEnvArgsHeaders + - ServerConfig{Env: {"K": "${env:V}"}, Args: ["${env:A}"], Headers: {"H": "${env:V}"}} → all resolved (FR-008) +TestNewClientWithOptions_DoesNotMutateOriginal + - ServerConfig with refs → original ServerConfig unchanged after NewClientWithOptions returns (FR-004) +TestNewClientWithOptions_ReflectionRegressionTest + - Walk resolved config via reflection; assert no field matches IsSecretRef() (SC-004) +TestNewClientWithOptions_PartialFailureLogsError + - One field fails to resolve → error logged, other fields resolved, server still creates +``` + +**Implementation** (`client.go`, lines 105-182): +```go +// Replace entire block with: +resolvedServerConfig := config.CopyServerConfig(serverConfig) +if secretResolver != nil { + ctx := context.Background() + errs := secretResolver.ExpandStructSecretsCollectErrors(ctx, resolvedServerConfig) + for _, e := range errs { + logger.Error("CRITICAL: Failed to resolve secret reference - field will use UNRESOLVED placeholder", + zap.String("server", serverConfig.Name), + zap.String("field", e.FieldPath), + zap.String("reference", e.Reference), + zap.Error(e.Err), + zap.String("help", "Use Web UI (http://localhost:8080/ui/) or API to add the secret to keyring")) + } +} +``` + +--- + +### Step 4 — Expand `DataDir` in loader (TDD: test first) + +**File**: `internal/config/config_test.go` and `loader.go` + +**Test first** (`config_test.go`): +``` +TestLoadConfig_ExpandsDataDir + - Config file with "data_dir": "${env:TEST_HOME}/.mcpproxy" → DataDir resolved before Validate() +TestLoadConfig_DataDirExpandFailure + - Config with "data_dir": "${env:MISSING_VAR}/.test" and dir not existing → error logged, Validate fails with directory-not-found (not resolver error) +``` + +**Implementation** (`loader.go`, before each `cfg.Validate()` call — lines 50 and 143): +```go +// Expand secret refs in DataDir before validation +if cfg.DataDir != "" { + resolver := secret.NewResolver() + if resolved, err := resolver.ExpandSecretRefs(context.Background(), cfg.DataDir); err != nil { + logger.Warn("Failed to resolve secret ref in data_dir, using original value", + zap.String("reference", cfg.DataDir), zap.Error(err)) + } else { + cfg.DataDir = resolved + } +} +``` + +Note: `DataDir` uses `ExpandSecretRefs` (single-string variant), not the struct variant. Consistent with FR-003 error/DEBUG semantics; uses WARN here since startup is aborting anyway if the dir doesn't exist. + +--- + +### Step 5 — Update agent context + +```bash +.specify/scripts/bash/update-agent-context.sh claude +``` + +Add to CLAUDE.md `Active Technologies`: +``` +- Go 1.24 (toolchain go1.24.10) + reflect (stdlib), internal/secret.ExpandStructSecretsCollectErrors (034-expand-secret-refs) +``` + +--- + +### Step 6 — Full verification + +```bash +go test ./internal/secret/... -race -v # Step 1 tests +go test ./internal/config/... -race -v # Steps 2, 4 tests +go test ./internal/upstream/core/... -race -v # Step 3 tests +go test ./internal/... -race # Full regression check +./scripts/test-api-e2e.sh # E2E sanity +``` + +Manual smoke test: add server with `"working_dir": "${env:HOME}/test"` to config, verify it starts with resolved path. + +## Post-Design Constitution Re-Check + +| Principle | Re-check | Notes | +|-----------|----------|-------| +| V. TDD | ✅ Pass | Tests written in each step before implementation code | +| IV. Security | ✅ Pass | `ExpandStructSecretsCollectErrors` logs `e.Reference` (the pattern), never the resolved value | +| VI. Documentation | ✅ Pass | Agent context script run in Step 5 | + +## Artifacts Generated + +- `specs/034-expand-secret-refs/research.md` ✅ +- `specs/034-expand-secret-refs/plan.md` ✅ (this file) +- No new API contracts (no REST endpoints changed) +- No new data model (no new storage entities) diff --git a/specs/034-expand-secret-refs/research.md b/specs/034-expand-secret-refs/research.md new file mode 100644 index 00000000..945359c2 --- /dev/null +++ b/specs/034-expand-secret-refs/research.md @@ -0,0 +1,77 @@ +# Research: Expand Secret/Env Refs in All Config String Fields + +**Branch**: `034-expand-secret-refs` | **Date**: 2026-03-10 + +## Decision 1: Implement `ExpandStructSecretsCollectErrors` vs. calling `ExpandStructSecrets` per-field + +**Decision**: Add a new method `ExpandStructSecretsCollectErrors` that collects errors instead of failing fast, with full field path tracking. + +**Rationale**: +- `ExpandStructSecrets` (resolver.go:107) fails fast on first error — incompatible with the existing "log and continue" semantics of `NewClientWithOptions` +- The existing `extractSecretRefs` (resolver.go:234) already has full path tracking (`"Isolation.WorkingDir"`, `"Args[0]"`, `"Env[MY_VAR]"`) — the collect-errors variant mirrors this pattern +- `ExpandStructSecrets` doesn't track paths; the new variant adds path tracking for richer error logs + +**Alternatives considered**: +- Wrapping `ExpandStructSecrets` with a recover: rejected — paths still not tracked, awkward error handling +- Keeping manual field expansion but adding missing fields: rejected — doesn't satisfy FR-007 (future-proof) and re-introduces the same class of bug on the next PR + +## Decision 2: Use `CopyServerConfig` (exported) for deep copy before expansion + +**Decision**: Export `copyServerConfig` → `CopyServerConfig` in `internal/config/merge.go` and call it from `NewClientWithOptions`. + +**Rationale**: +- The current code does `resolvedServerConfig := *serverConfig` (shallow copy). Pointer fields `Isolation *IsolationConfig` and `OAuth *OAuthConfig` alias the original. +- `copyServerConfig` (merge.go:525) correctly deep-copies all pointer fields, maps, and slices. +- `ExpandStructSecretsCollectErrors` must be called with a pointer to the copied struct so reflection `CanSet()` works on struct fields. + +**Alternatives considered**: +- `encoding/json` marshal+unmarshal for deep copy: rejected — loses zero values, slower, requires json tags on all fields, drops `*bool` nil vs. false distinction +- `encoding/gob`: rejected — same issues, requires exported fields only (already have them but adds a dependency for a simple copy) + +## Decision 3: Expand `DataDir` in `loader.go`, not in `Validate()` + +**Decision**: Expand `DataDir` in `internal/config/loader.go` immediately before each call to `cfg.Validate()` (lines 50 and 143). + +**Rationale**: +- `Validate()` doesn't have a resolver parameter and is called from multiple places including hot-reload in `runtime.go:1139` — modifying its signature would require widespread changes +- The loader is the canonical "first load" entry point; the two call sites at loader.go:50 and 143 cover both initial load and reload-from-file +- `DataDir` validation at config.go:945 checks directory existence — expansion must precede this check +- `secret.NewResolver()` requires no parameters and can be instantiated locally in the loader + +**Alternatives considered**: +- Add `*secret.Resolver` parameter to `Validate()`: rejected — 6+ call sites need updating, including hot-reload, CLI config validation, test code +- Expand at point-of-use (when BBolt opens at `cli/client.go:66`): rejected — too late, `DataDir` is used for multiple things (logs, search index) before the BBolt client is created + +## Decision 4: Where to call `ExpandStructSecretsCollectErrors` in `NewClientWithOptions` + +**Decision**: Call on the `CopyServerConfig` result (a pointer), replacing all 3 manual expansion blocks (lines 107-182). + +**Rationale**: +- `ExpandStructSecretsCollectErrors` must receive a pointer to the struct for reflection `CanSet()` to work on struct fields +- Calling it on the copy (not the original) satisfies FR-004 +- Replaces ~78 lines with ~12 lines while covering all current and future string fields +- FR-008 (preserve existing behavior for env/args/headers) is automatically satisfied — the reflect walker handles maps and slices the same way as the manual approach + +## Decision 5: Test strategy + +**Decision**: Two new test files + tests in the existing resolver_test.go: +1. `internal/secret/resolver_test.go` — add `TestResolver_ExpandStructSecretsCollectErrors` table-driven tests (MockProvider already defined in same file) +2. `internal/upstream/core/client_secret_test.go` (new file) — test `NewClientWithOptions` with secret refs in all fields including `WorkingDir`; use reflection to assert no fields contain unresolved `${...}` patterns after creation + +**Rationale**: No existing tests for `ExpandStructSecrets` at all (confirmed by audit). The reflection-based assertion in client_secret_test.go serves as SC-004 (regression prevention for future fields). + +## Key File Map + +| File | Change | Why | +|------|--------|-----| +| `internal/secret/resolver.go` | Add `ExpandStructSecretsCollectErrors` + `SecretExpansionError` type | Core of FR-007 | +| `internal/secret/resolver_test.go` | Add tests for new method | Constitution V (TDD) | +| `internal/config/merge.go` | Export `CopyServerConfig` (rename + 3 internal call sites) | FR-004 deep copy | +| `internal/upstream/core/client.go` | Replace lines 107-182, use `CopyServerConfig` + `ExpandStructSecretsCollectErrors` | FR-001 | +| `internal/upstream/core/client_secret_test.go` | New — regression test with reflection assertion | SC-002, SC-004 | +| `internal/config/loader.go` | Expand `DataDir` before each `Validate()` call (2 sites) | FR-002 | +| `internal/config/config_test.go` | Add `DataDir` expansion test | Constitution V | + +## No NEEDS CLARIFICATION items remaining + +All technical unknowns resolved. No blockers to Phase 1. diff --git a/specs/034-expand-secret-refs/spec.md b/specs/034-expand-secret-refs/spec.md new file mode 100644 index 00000000..8767c6a8 --- /dev/null +++ b/specs/034-expand-secret-refs/spec.md @@ -0,0 +1,155 @@ +# Feature Specification: Expand Secret/Env Refs in All Config String Fields + +**Feature Branch**: `034-expand-secret-refs` +**Created**: 2026-03-10 +**Status**: Draft +**Input**: User description: "Expand secret/env refs in working_dir, data_dir, and all config string fields" +**Issue**: [#333](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/333) + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Environment variable references in working directory (Priority: P1) + +As a developer, I want to use `${env:HOME}` or `${env:IH_HOME}` in my server's `working_dir` configuration so that my config files are portable across machines without hardcoding absolute paths. + +**Why this priority**: This is the primary reported bug. Users configuring `"working_dir": "${env:IH_HOME}"` get the literal string instead of the resolved path, causing server startup failures when the literal directory doesn't exist. + +**Independent Test**: Can be tested by configuring a server with `"working_dir": "${env:HOME}/test-project"` and verifying the stdio child process launches with the resolved directory as its working directory. + +**Acceptance Scenarios**: + +1. **Given** a server config with `"working_dir": "${env:HOME}/project"`, **When** the server starts, **Then** the stdio child process runs in the resolved directory (e.g., `/Users/alice/project`). +2. **Given** a server config with `"working_dir": "${env:UNDEFINED_VAR}/project"`, **When** the server starts, **Then** an error is logged and the unresolved value is used as a fallback (matching existing error behavior for env/args/headers). +3. **Given** a server config with `"working_dir": "/absolute/path"` (no refs), **When** the server starts, **Then** behavior is unchanged — the literal path is used. + +--- + +### User Story 2 - Secret references in all server config string fields (Priority: P1) + +As a platform operator, I want `${env:...}` and `${keyring:...}` references to work in any string field of my server configuration — not just `env`, `args`, and `headers` — so I don't have to guess which fields support variable expansion. + +**Why this priority**: This is the systematic fix that prevents future recurrence. Currently only 3 of many string fields are expanded. Fields like `URL`, `Command`, `Isolation.WorkingDir`, and `Isolation.ExtraArgs` silently ignore secret references. + +**Independent Test**: Can be tested by setting `${env:TEST_VAR}` in each string field of a server config and verifying all resolve after client creation. + +**Acceptance Scenarios**: + +1. **Given** a server config with `${env:...}` refs in `URL`, `Command`, `WorkingDir`, `Isolation.WorkingDir`, and `Isolation.ExtraArgs`, **When** the client is created, **Then** all string fields are resolved. +2. **Given** a server config with `${keyring:my-secret}` in the `URL` field, **When** the client is created, **Then** the keyring value is substituted. +3. **Given** a new string field is added to the server config struct in the future, **When** a user sets a `${env:...}` ref in that field, **Then** it is automatically resolved without any code changes to the expansion logic. + +--- + +### User Story 3 - Environment variable references in data directory (Priority: P2) + +As an operator, I want to use `${env:...}` references in the top-level `data_dir` configuration so that I can configure storage paths portably across environments. + +**Why this priority**: `data_dir` controls where the database, search index, and logs are stored. While less commonly customized than per-server fields, portable paths are still valuable for multi-environment deployments. + +**Independent Test**: Can be tested by setting `"data_dir": "${env:HOME}/.mcpproxy"` in the config and verifying the resolved path is used for database operations. + +**Acceptance Scenarios**: + +1. **Given** a config with `"data_dir": "${env:HOME}/.mcpproxy"`, **When** the application starts, **Then** the database opens at the resolved path. +2. **Given** a config with `"data_dir": "${env:MISSING_VAR}/.mcpproxy"`, **When** the application starts, **Then** an error is logged and the unresolved value is used as a fallback. + +--- + +### Edge Cases + +- What happens when a `${env:...}` ref is in a field like `Name` or `Protocol` that doesn't normally contain refs? The expansion is a no-op since those values won't match the `${type:name}` pattern. +- What happens when an env var is set but contains an empty string? Empty string is a successful resolution — the ref is replaced with `""` and no error is logged. Downstream validation (e.g. directory existence check) surfaces any resulting issue. +- What happens when `Isolation` config is nil? The expansion handles nil pointers gracefully without panicking. +- What happens when `OAuth` config contains `${keyring:...}` refs (e.g., in `ClientSecret`)? These are expanded like any other string field. +- What happens when the same secret reference appears in multiple fields and one fails to resolve? Each field is handled independently — successful resolutions are not rolled back due to failures in other fields. +- What happens when expansion is applied to the original config vs. a copy? The original config held by the caller is never mutated. A deep copy is used before expansion. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: System MUST resolve `${env:...}` and `${keyring:...}` references in all string fields of the server configuration, including but not limited to: `WorkingDir`, `URL`, `Command`, and all fields within nested isolation and OAuth configuration structs. +- **FR-002**: System MUST resolve `${env:...}` and `${keyring:...}` references in the top-level data directory configuration field before it is consumed by downstream components. +- **FR-003**: On expansion failure, system MUST log at ERROR level (including field path, reference pattern, and remediation guidance) and fall back to the unresolved value. On successful resolution where the value changed, system MUST log at DEBUG level (including field path and reference pattern). Resolved values MUST NOT appear in log output at any log level. +- **FR-004**: System MUST NOT mutate the original server configuration passed to the client constructor. All expansion must operate on a deep copy. +- **FR-005**: System MUST NOT modify string fields that don't contain secret reference patterns (`${type:name}`). Fields containing plain values like server names or protocol identifiers are left untouched. +- **FR-006**: System MUST handle nil nested configuration structures (isolation, OAuth) gracefully without errors. +- **FR-007**: System MUST automatically cover any new string fields added to the server configuration in the future without requiring code changes to the expansion logic. +- **FR-008**: The expansion behavior for currently-supported fields (`env`, `args`, `headers`) MUST be preserved identically — same error handling, same log output, same fallback to unresolved value on failure. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A server configured with `"working_dir": "${env:HOME}/project"` starts successfully with the resolved path as the working directory. +- **SC-002**: 100% of string fields in the server configuration (including nested structs) containing secret references are resolved after client creation — verified by an automated test. +- **SC-003**: All existing tests pass without modification (zero regressions). +- **SC-004**: Adding a new string field to the server configuration causes the regression test to automatically verify it is expanded — no manual test updates needed. +- **SC-005**: Expansion errors in one field do not prevent resolution of other fields. + +## Clarifications + +### Session 2026-03-10 + +- Q: Should resolved secret values appear in log output? → A: Never log resolved values — log only the reference pattern (e.g. `${keyring:my-token}`) to confirm resolution occurred, never the resolved value itself. +- Q: If an env var is set but empty (`""`), is that a successful resolution or a failure? → A: Treat as success — consistent with current behavior. An empty string is a valid resolved value. Downstream validation (e.g. directory existence check) surfaces the problem at the appropriate point. +- Q: What log level should expansion use? → A: Match existing behavior — ERROR for failure (with unresolved fallback), DEBUG for success (ref resolved, value changed). + +## Assumptions + +- The existing reflection-based struct expansion function is the correct foundation for FR-007. It needs a variant that collects errors instead of failing on the first error. +- The existing deep-copy function for server configurations correctly handles all pointer fields and is the right mechanism for FR-004. +- Fields like `Name`, `Protocol`, `Enabled`, `Created`, `Updated` will never contain `${type:name}` patterns in practice, so expanding them is a safe no-op. + +## Scope Boundaries + +### In Scope + +- Expanding refs in all server config string fields (including nested structs) +- Expanding refs in the top-level data directory config +- Replacing manual field-by-field expansion with automatic approach +- Regression test preventing future fields from being missed + +### Out of Scope + +- Adding new config fields like `Volumes` or `HostPath` to isolation configuration +- Changing the behavior of the existing struct expansion function for other callers +- Expanding refs in non-string fields (booleans, integers, etc.) +- UI changes + +## Commit Message Conventions *(mandatory)* + +When committing changes for this feature, follow these guidelines: + +### Issue References +- Use: `Related #333` - Links the commit to the issue without auto-closing +- Do NOT use: `Fixes #333`, `Closes #333`, `Resolves #333` - These auto-close issues on merge + +**Rationale**: Issues should only be closed manually after verification and testing in production, not automatically on merge. + +### Co-Authorship +- Do NOT include: `Co-Authored-By: Claude ` +- Do NOT include: "Generated with Claude Code" + +**Rationale**: Commit authorship should reflect the human contributors, not the AI tools used. + +### Example Commit Message +``` +feat: expand secret refs in all ServerConfig string fields + +Related #333 + +Replace manual field-by-field expansion with reflection-based +ExpandStructSecretsCollectErrors to automatically cover all current +and future string fields. + +## Changes +- Add ExpandStructSecretsCollectErrors to secret.Resolver +- Export CopyServerConfig for deep-copy before expansion +- Replace ~78 lines of manual expansion with ~12 lines of struct expansion +- Add reflection-based regression test + +## Testing +- All existing tests pass (zero regressions) +- New tests for collect-errors method and regression prevention +``` diff --git a/specs/034-expand-secret-refs/tasks.md b/specs/034-expand-secret-refs/tasks.md new file mode 100644 index 00000000..39445393 --- /dev/null +++ b/specs/034-expand-secret-refs/tasks.md @@ -0,0 +1,157 @@ +# Tasks: Expand Secret/Env Refs in All Config String Fields + +**Input**: Design documents from `/specs/034-expand-secret-refs/` +**Branch**: `034-expand-secret-refs` +**Issue**: [#333](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/333) + +**Tests**: TDD is required by the feature constitution (Principle V). Test tasks are included and MUST precede implementation tasks within each phase. + +**Organization**: Tasks are grouped by user story to enable independent implementation and testing of each story. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies) +- **[Story]**: Which user story this task belongs to (US1, US2, US3) + +--- + +## Phase 1: Setup (Baseline) + +**Purpose**: Establish a green baseline before any changes. All tests must pass before implementation begins. + +- [x] T001 Run baseline tests and confirm green: `go test ./internal/secret/... ./internal/upstream/core/... ./internal/config/... -race -v` + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Add `ExpandStructSecretsCollectErrors` and export `CopyServerConfig` — both are required by US1 and US2 before any server-config expansion can be implemented. + +**⚠️ CRITICAL**: No user story work can begin until this phase is complete. + +> **TDD**: Write tests first (T002), confirm they FAIL, then implement (T003–T004). + +- [x] T002 Write failing tests for `ExpandStructSecretsCollectErrors` in `internal/secret/resolver_test.go` (7 cases: HappyPath, PartialFailure, NilPointer, NestedStruct, SliceField, MapField, NoRefs) — tests must FAIL before T003 +- [x] T003 Add `SecretExpansionError` struct type to `internal/secret/resolver.go` +- [x] T004 Implement `ExpandStructSecretsCollectErrors` and internal `expandValueCollectErrors` helper in `internal/secret/resolver.go` (mirrors `expandValue` with path tracking + collect-errors semantics) +- [x] T005 Export `copyServerConfig` → `CopyServerConfig` in `internal/config/merge.go` and update 3 internal call sites in the same file +- [x] T006 Verify foundational phase: `go test ./internal/secret/... ./internal/config/... -race` — all T002 tests must now pass + +**Checkpoint**: Foundation ready — `ExpandStructSecretsCollectErrors` and `CopyServerConfig` available for user story implementation. + +--- + +## Phase 3: User Stories 1 & 2 — ServerConfig Expansion (Priority: P1) 🎯 MVP + +**Goal (US1)**: `${env:...}` and `${keyring:...}` refs in `ServerConfig.WorkingDir` resolve to the actual path when the server starts. + +**Goal (US2)**: ALL string fields in `ServerConfig` (including nested `IsolationConfig` and `OAuthConfig`) are automatically expanded — current and future fields alike, without manual allowlisting. + +**Independent Test**: Configure a server with `"working_dir": "${env:HOME}/test"`, start it, confirm the stdio child process runs in the resolved directory. Also confirm `URL`, `Command`, `Isolation.WorkingDir`, and `Isolation.ExtraArgs` resolve when set to `${env:...}` refs. + +> **TDD**: Write tests first (T007–T008), confirm they FAIL, then implement (T009). + +- [x] T007 [US1] [US2] Write failing tests for `NewClientWithOptions` expansion in `internal/upstream/core/client_secret_test.go` (new file): ExpandsWorkingDir, ExpandsIsolationWorkingDir, ExpandsURL, PreservesExistingEnvArgsHeaders, DoesNotMutateOriginal — tests must FAIL before T009 +- [x] T008 [P] [US2] Write reflection regression test `TestNewClientWithOptions_ReflectionRegressionTest` in `internal/upstream/core/client_secret_test.go`: walks all string fields of resolved config via reflection and asserts none match `IsSecretRef()` (SC-004) — must FAIL before T009 +- [x] T009 [US1] [US2] Replace manual expansion block (lines 105–182) in `internal/upstream/core/client.go` with `config.CopyServerConfig(serverConfig)` + `secretResolver.ExpandStructSecretsCollectErrors(ctx, resolvedServerConfig)` + error logging loop +- [x] T010 [US1] [US2] Verify: `go test ./internal/upstream/core/... -race` — all T007 and T008 tests must now pass + +**Checkpoint**: US1 and US2 complete. WorkingDir and all other `ServerConfig` string fields expand refs. Existing `Env`/`Args`/`Headers` behavior preserved (FR-008). Original config not mutated (FR-004). + +--- + +## Phase 4: User Story 3 — DataDir Expansion (Priority: P2) + +**Goal**: `${env:...}` refs in the top-level `data_dir` config field resolve before directory validation runs. + +**Independent Test**: Set `"data_dir": "${env:HOME}/.mcpproxy-test"` in config, start the proxy, confirm the database opens at the resolved path (not the literal `${env:HOME}/.mcpproxy-test`). + +> **TDD**: Write tests first (T011), confirm they FAIL, then implement (T012). + +- [x] T011 [US3] Write failing tests for DataDir expansion in `internal/config/config_test.go`: TestLoadConfig_ExpandsDataDir (env var resolves before Validate), TestLoadConfig_DataDirExpandFailure (missing var → warn + Validate fails on dir not found) — tests must FAIL before T012 +- [x] T012 [US3] Implement DataDir expansion in `internal/config/loader.go` at both `cfg.Validate()` call sites (lines ~50 and ~143): `secret.NewResolver().ExpandSecretRefs(ctx, cfg.DataDir)` with WARN on failure +- [x] T013 [US3] Verify: `go test ./internal/config/... -race` — all T011 tests must now pass + +**Checkpoint**: US3 complete. `data_dir` refs expand before validation. All three user stories are independently functional. + +--- + +## Phase 5: Polish & Cross-Cutting Concerns + +- [x] T014 Run `.specify/scripts/bash/update-agent-context.sh claude` to add `ExpandStructSecretsCollectErrors` to CLAUDE.md Active Technologies section +- [x] T015 [P] Full regression: `go test ./internal/... -race` — zero test failures +- [x] T016 [P] E2E sanity: `./scripts/test-api-e2e.sh` — passes clean +- [x] T017 Manual smoke test: add server with `"working_dir": "${env:HOME}/test"` to config and verify it starts with resolved path + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Setup (Phase 1)**: No dependencies — run immediately +- **Foundational (Phase 2)**: Depends on Phase 1 green baseline — **BLOCKS US1, US2, US3** +- **US1+US2 (Phase 3)**: Depends on Phase 2 (`ExpandStructSecretsCollectErrors` + `CopyServerConfig` available) +- **US3 (Phase 4)**: Depends on Phase 2 (`secret.NewResolver()` already available — no new dep on Phase 3, can run in parallel with Phase 3 if desired) +- **Polish (Phase 5)**: Depends on Phases 3 and 4 both complete + +### User Story Dependencies + +- **US1+US2 (Phase 3)**: After Phase 2 — no dependency on US3 +- **US3 (Phase 4)**: After Phase 2 — no dependency on US1/US2 (different files: `loader.go`, `config_test.go`) + +### Within Each Phase + +- Tests (T002, T007, T008, T011) MUST be written and confirmed FAILING before their implementation tasks +- T003 (type declaration) before T004 (method implementation) +- T005 (export CopyServerConfig) before T009 (call it from client.go) + +### Parallel Opportunities + +- T007 and T008 can be written in parallel (both in `client_secret_test.go` — same file, so sequential is safer) +- Phase 3 and Phase 4 can proceed in parallel after Phase 2 (different files: `client.go` vs `loader.go`) +- T015 and T016 (regression + E2E) can run in parallel + +--- + +## Parallel Example: Phases 3 and 4 (after Phase 2 complete) + +```bash +# Once foundational phase is done, these can proceed in parallel: + +# Agent A: Phase 3 (US1+US2) +# Write tests → internal/upstream/core/client_secret_test.go +# Implement → internal/upstream/core/client.go + +# Agent B: Phase 4 (US3) — independent, different package +# Write tests → internal/config/config_test.go +# Implement → internal/config/loader.go +``` + +--- + +## Implementation Strategy + +### MVP First (US1 only — the reported bug fix) + +1. Complete Phase 1: Baseline +2. Complete Phase 2: Foundational (required) +3. Complete Phase 3 T007 + T009 only (skip T008 regression test for MVP speed) +4. **STOP and VALIDATE**: `go test ./internal/upstream/core/... -race` +5. Confirm `working_dir` refs resolve in manual smoke test + +### Full Delivery (all 3 user stories) + +1. Phase 1 → Phase 2 → Phase 3 → Phase 4 → Phase 5 +2. Each phase independently verifiable before proceeding +3. Total: 17 tasks, ~4–6 implementation files touched + +--- + +## Notes + +- [P] tasks operate on different files with no inter-dependencies +- Each user story phase is independently verifiable via its checkpoint +- TDD order is strictly: WRITE TEST → CONFIRM FAIL → IMPLEMENT → CONFIRM PASS +- Commit after each phase checkpoint (T006, T010, T013, T017) +- `ExpandStructSecretsCollectErrors` must never log resolved values — only reference patterns (FR-003, IV. Security)