From b685de7bc36c826ab194ecad6280ce3b4f585e4b Mon Sep 17 00:00:00 2001 From: Andy Lo-A-Foe Date: Thu, 2 Apr 2026 07:07:44 +0200 Subject: [PATCH] fix(config): handle non-addressable SecureString values in collectSensitive When iterating over map values (e.g., TeamsWebhookConfig.Webhooks), Go reflection returns non-addressable values. The collectSensitive function was calling v.Addr() unconditionally, causing a panic: panic: reflect.Value.Addr of unaddressable value This fix checks v.CanAddr() before calling Addr(), and creates an addressable copy when needed. This allows sensitive data filtering to work correctly with channels that use maps containing SecureString values (like teams_webhook). Includes regression tests to verify the fix. Co-Authored-By: Claude Opus 4.5 --- pkg/config/security.go | 42 +++++++++++----- pkg/config/security_test.go | 95 +++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 13 deletions(-) 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) + }) +}