diff --git a/pkg/config/security.go b/pkg/config/security.go index c5d3bf507..41c1b2230 100644 --- a/pkg/config/security.go +++ b/pkg/config/security.go @@ -250,6 +250,31 @@ func (sec *Config) collectSensitiveValues() []string { return values } +// secureStringValue extracts the string value from a SecureString reflect.Value. +// Uses typed call when possible for better performance and type safety. +func secureStringValue(v reflect.Value) string { + // Prefer typed call over reflective MethodByName for performance and type safety + if v.CanInterface() { + if ss, ok := v.Interface().(SecureString); ok { + return ss.String() + } + } + // Fallback to reflection for unexported/non-interfaceable values + var ptrVal reflect.Value + if v.CanAddr() { + ptrVal = v.Addr() + } else { + tmp := reflect.New(v.Type()).Elem() + tmp.Set(v) + ptrVal = tmp.Addr() + } + result := ptrVal.MethodByName("String").Call(nil) + if len(result) > 0 { + return result[0].String() + } + return "" +} + // collectSensitive recursively traverses the value and collects SecureString/SecureStrings values. func collectSensitive(v reflect.Value, values *[]string) { for v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface { @@ -276,14 +301,8 @@ func collectSensitive(v reflect.Value, values *[]string) { // SecureString: collect via String() method (defined on *SecureString) if t == reflect.TypeOf(SecureString{}) { - // Create a new pointer to make it addressable for method calls - ptr := reflect.New(t) - ptr.Elem().Set(v) - result := ptr.MethodByName("String").Call(nil) - if len(result) > 0 { - if s := result[0].String(); s != "" { - *values = append(*values, s) - } + if s := secureStringValue(v); s != "" { + *values = append(*values, s) } return } @@ -300,11 +319,8 @@ func collectSensitive(v reflect.Value, values *[]string) { elem = elem.Elem() } if elem.IsValid() && elem.Type() == reflect.TypeOf(SecureString{}) { - result := elem.Addr().MethodByName("String").Call(nil) - if len(result) > 0 { - if s := result[0].String(); s != "" { - *values = append(*values, s) - } + if s := secureStringValue(elem); s != "" { + *values = append(*values, s) } } } diff --git a/pkg/config/security_test.go b/pkg/config/security_test.go index 23daf3231..3d2a0ea32 100644 --- a/pkg/config/security_test.go +++ b/pkg/config/security_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "os" "path/filepath" + "reflect" "testing" "github.com/caarlos0/env/v11" @@ -270,3 +271,97 @@ skills: assert.Equal(t, "abc", envCfg.Tools.Web.Brave.APIKeys[1].raw) }) } + +// TestCollectSensitiveWithMapValues verifies that collectSensitive handles +// non-addressable values from map lookups without panicking. +// This is a regression test for the bug where iterating over maps containing +// SecureString values would panic with "reflect.Value.Addr of unaddressable value". +func TestCollectSensitiveWithMapValues(t *testing.T) { + // Test struct that mimics TeamsWebhookConfig with a map of targets + type WebhookTarget struct { + WebhookURL SecureString `json:"webhook_url" yaml:"webhook_url"` + Title string `json:"title" yaml:"title"` + } + type WebhookConfig struct { + Enabled bool `json:"enabled" yaml:"enabled"` + Webhooks map[string]WebhookTarget `json:"webhooks" yaml:"webhooks"` + } + + t.Run("map with SecureString values does not panic", func(t *testing.T) { + cfg := WebhookConfig{ + Enabled: true, + Webhooks: map[string]WebhookTarget{ + "default": { + WebhookURL: *NewSecureString("https://secret-webhook-url.com/abc123"), + Title: "Default Webhook", + }, + "alerts": { + WebhookURL: *NewSecureString("https://another-secret.com/xyz789"), + Title: "Alerts Webhook", + }, + }, + } + + var values []string + // This should not panic - previously it would panic with: + // "reflect.Value.Addr of unaddressable value" + assert.NotPanics(t, func() { + collectSensitive(reflect.ValueOf(&cfg), &values) + }) + + // Verify the sensitive values were collected + assert.Len(t, values, 2) + assert.Contains(t, values, "https://secret-webhook-url.com/abc123") + assert.Contains(t, values, "https://another-secret.com/xyz789") + }) + + t.Run("nested map with SecureString values", func(t *testing.T) { + type NestedConfig struct { + Outer map[string]map[string]SecureString + } + + cfg := NestedConfig{ + Outer: map[string]map[string]SecureString{ + "level1": { + "secret1": *NewSecureString("nested-secret-1"), + "secret2": *NewSecureString("nested-secret-2"), + }, + }, + } + + var values []string + assert.NotPanics(t, func() { + collectSensitive(reflect.ValueOf(&cfg), &values) + }) + + assert.Len(t, values, 2) + assert.Contains(t, values, "nested-secret-1") + assert.Contains(t, values, "nested-secret-2") + }) + + t.Run("empty map does not panic", func(t *testing.T) { + cfg := WebhookConfig{ + Enabled: true, + Webhooks: map[string]WebhookTarget{}, + } + + var values []string + assert.NotPanics(t, func() { + collectSensitive(reflect.ValueOf(&cfg), &values) + }) + assert.Empty(t, values) + }) + + t.Run("nil map does not panic", func(t *testing.T) { + cfg := WebhookConfig{ + Enabled: true, + Webhooks: nil, + } + + var values []string + assert.NotPanics(t, func() { + collectSensitive(reflect.ValueOf(&cfg), &values) + }) + assert.Empty(t, values) + }) +}