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 <noreply@anthropic.com>
This commit is contained in:
Andy Lo-A-Foe 2026-04-02 07:07:44 +02:00
parent 15a3560533
commit b685de7bc3
2 changed files with 124 additions and 13 deletions

View file

@ -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)
}
}
}

View file

@ -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)
})
}