fix(insurance): keep the API key out of every field fmt can print

pkg/insurance/client.go

@@ -33,9 +33,10 @@ const maxErrorMessageLen = 200
 // Config identifies the vendor organization and credential. APIKey is sent
 // only as the x-api-key header. Format redacts it for every fmt verb except %p
 // and %T on a non-pointer Config, which fmt handles before consulting Format;
-// never pass a Config value to %p. fmt also cannot call methods on unexported
-// struct fields, so a consumer must hold a *Client, or a Config only in an
-// exported field, and never log a Config stored in an unexported field.
+// never pass a Config value to %p. fmt cannot call methods on unexported
+// struct fields, so a Config held by the caller in an unexported field still
+// prints the key raw: drop the Config once NewClient has returned, and never
+// log one. NewClient does not retain the Config.
 type Config struct {
 	APIKey string `json:"-"`
 	OrgID  string
@@ -67,20 +68,22 @@ func (e *HTTPError) Error() string {
 
 // Client is the real read-only QuoteClient. It issues single-shot GETs and
 // never follows redirects, so the API key cannot be forwarded to another host.
+//
+// The API key is held only inside the setAuth and redact closures, never in a
+// field fmt or reflection can read, so no fmt verb prints it wherever the
+// Client (or a pointer to it) is stored.
 type Client struct {
 	baseURL string
-	cfg     Config
+	orgID   string
 	hc      *http.Client
+	// setAuth adds the credential to one outgoing request.
+	setAuth func(*http.Request)
+	// redact masks the credential in a vendor-supplied message.
+	redact func(string) string
 }
 
 var _ QuoteClient = (*Client)(nil)
 
-// Format keeps the embedded configuration, and with it the key, out of any
-// fmt output of a Client (same %p/%T limit as Config).
-func (c Client) Format(s fmt.State, _ rune) {
-	_, _ = fmt.Fprintf(s, "insurance.Client{OrgID:%q, APIKey:[redacted]}", c.cfg.OrgID)
-}
-
 // NewClient returns a Client for the fixed Archera origin. hc is optional; nil
 // uses the shared SSRF-hardened client with a 30-second timeout.
 func NewClient(cfg Config, hc *http.Client) (*Client, error) {
@@ -99,7 +102,19 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) {
 	}
 	noRedirect := *hc
 	noRedirect.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }
-	return &Client{baseURL: baseURL, cfg: cfg, hc: &noRedirect}, nil
+	key := cfg.APIKey
+	return &Client{
+		baseURL: baseURL,
+		orgID:   cfg.OrgID,
+		hc:      &noRedirect,
+		setAuth: func(req *http.Request) { req.Header.Set("x-api-key", key) },
+		redact: func(msg string) string {
+			if key == "" {
+				return msg
+			}
+			return strings.ReplaceAll(msg, key, "[redacted]")
+		},
+	}, nil
 }
 
 // Comparison reads the documented comparison operation. Empty request slices
@@ -132,7 +147,7 @@ func (c *Client) Comparison(ctx context.Context, req ComparisonRequest) (*Compar
 		return nil, err
 	}
 	defer func() { _ = body.Close() }()
-	return DecodeComparison(&boundedReader{r: body, n: maxResponseBytes}, c.cfg.OrgID, req.PlanID, fetchedAt)
+	return DecodeComparison(&boundedReader{r: body, n: maxResponseBytes}, c.orgID, req.PlanID, fetchedAt)
 }
 
 // Plan reads the documented CommitmentPlan operation.
@@ -149,7 +164,7 @@ func (c *Client) Plan(ctx context.Context, planID string) (*Plan, error) {
 }
 
 func (c *Client) planPath(planID string) string {
-	return "/v1/org/" + c.cfg.OrgID + "/commitment-plans/" + planID
+	return "/v1/org/" + c.orgID + "/commitment-plans/" + planID
 }
 
 var errResponseTooLarge = fmt.Errorf("response exceeds %d bytes", maxResponseBytes)
@@ -192,7 +207,7 @@ func (c *Client) get(ctx context.Context, path string, q url.Values) (io.ReadClo
 	if err != nil {
 		return nil, time.Time{}, fmt.Errorf("building archera request: %w", err)
 	}
-	req.Header.Set("x-api-key", c.cfg.APIKey)
+	c.setAuth(req)
 	req.Header.Set("Accept", "application/json")
 	// baseURL is the fixed BaseURL constant; only in-package tests override it.
 	resp, err := c.hc.Do(req) //nolint:gosec // G704: see comment above
@@ -202,7 +217,7 @@ func (c *Client) get(ctx context.Context, path string, q url.Values) (io.ReadClo
 	fetchedAt := time.Now().UTC()
 	if resp.StatusCode < 200 || resp.StatusCode > 299 {
 		defer func() { _ = resp.Body.Close() }()
-		return nil, fetchedAt, newHTTPError(resp, c.cfg.APIKey)
+		return nil, fetchedAt, newHTTPError(resp, c.redact)
 	}
 	return resp.Body, fetchedAt, nil
 }
@@ -217,7 +232,7 @@ func sanitizeTransportError(err error) error {
 	return err
 }
 
-func newHTTPError(resp *http.Response, apiKey string) *HTTPError {
+func newHTTPError(resp *http.Response, redact func(string) string) *HTTPError {
 	e := &HTTPError{StatusCode: resp.StatusCode}
 	if s, err := strconv.Atoi(strings.TrimSpace(resp.Header.Get("Retry-After"))); err == nil && s > 0 {
 		// Compare before multiplying: s*time.Second overflows int64 for large s.
@@ -234,24 +249,19 @@ func newHTTPError(resp *http.Response, apiKey string) *HTTPError {
 	}
 	raw, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10))
 	if err == nil && json.Unmarshal(raw, &shape) == nil {
-		e.Message = truncate(cleanMessage(shape.Message, apiKey), maxErrorMessageLen)
+		e.Message = truncate(redact(cleanMessage(shape.Message)), maxErrorMessageLen)
 	}
 	return e
 }
 
-// cleanMessage drops control characters (log forging, ANSI injection) and
-// masks the API key if the vendor echoes it back.
-func cleanMessage(msg, apiKey string) string {
-	msg = strings.Map(func(r rune) rune {
+// cleanMessage drops control characters (log forging, ANSI injection).
+func cleanMessage(msg string) string {
+	return strings.Map(func(r rune) rune {
 		if unicode.IsControl(r) {
 			return -1
 		}
 		return r
 	}, msg)
-	if apiKey != "" {
-		msg = strings.ReplaceAll(msg, apiKey, "[redacted]")
-	}
-	return msg
 }
 
 func truncate(s string, n int) string {

pkg/insurance/key_leak_test.go

@@ -0,0 +1,120 @@
+package insurance
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"fmt"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type unexportedPtrHolder struct {
+	n      int
+	client *Client
+}
+
+type unexportedValHolder struct{ client Client }
+
+type ExportedPtrHolder struct{ Client *Client }
+
+type nestedHolder struct{ inner unexportedPtrHolder }
+
+// Issue #325: fmt does not call Format on unexported fields, so the key must
+// not be stored in any field fmt can reach.
+func TestClientKeyNeverPrintedWherever(t *testing.T) {
+	c, err := newClient("http://127.0.0.1:1", Config{APIKey: testKey, OrgID: testOrg}, nil)
+	require.NoError(t, err)
+
+	holders := map[string]any{
+		"unexported *Client":       unexportedPtrHolder{1, c},
+		"&unexported *Client":      &unexportedPtrHolder{1, c},
+		"unexported Client value":  unexportedValHolder{*c},
+		"&unexported Client value": &unexportedValHolder{*c},
+		"exported *Client":         ExportedPtrHolder{c},
+		"nested":                   nestedHolder{unexportedPtrHolder{1, c}},
+		"[]*Client":                []*Client{c},
+		"map":                      map[string]*Client{"a": c},
+		"[]any":                    []any{c, unexportedValHolder{*c}},
+		"wrapped error":            fmt.Errorf("x: %w", fmt.Errorf("%v", unexportedValHolder{*c})),
+	}
+	forbidden := []string{
+		testKey,
+		fmt.Sprintf("%x", testKey), fmt.Sprintf("%X", testKey),
+		base64.StdEncoding.EncodeToString([]byte(testKey)),
+		fmt.Sprintf("%q", testKey),
+	}
+	verbs := []string{"%v", "%+v", "%#v", "%s", "%q", "%x", "%X", "%d", "%T", "%p"}
+	for name, h := range holders {
+		for _, verb := range append([]string{"Sprint", "Sprintln"}, verbs...) {
+			t.Run(name+"/"+verb, func(t *testing.T) {
+				var out string
+				switch verb {
+				case "Sprint":
+					out = fmt.Sprint(h)
+				case "Sprintln":
+					out = fmt.Sprintln(h)
+				default:
+					out = fmt.Sprintf(verb, h)
+				}
+				for _, bad := range forbidden {
+					assert.NotContains(t, out, bad)
+				}
+			})
+		}
+	}
+	b, err := json.Marshal(ExportedPtrHolder{c})
+	require.NoError(t, err)
+	for _, bad := range forbidden {
+		require.NotContains(t, string(b), bad)
+	}
+}
+
+// The invariant must not depend on fmt behavior: walk every field of a
+// Client, unexported ones included, and require that no string equals or
+// contains the key.
+func TestClientHoldsNoStringContainingKey(t *testing.T) {
+	c, err := newClient("http://127.0.0.1:1", Config{APIKey: testKey, OrgID: testOrg}, nil)
+	require.NoError(t, err)
+	seen := map[uintptr]bool{}
+	var walk func(path string, v reflect.Value)
+	walk = func(path string, v reflect.Value) {
+		switch v.Kind() {
+		case reflect.String:
+			require.NotContains(t, v.String(), testKey, path)
+		case reflect.Pointer, reflect.Interface:
+			if v.IsNil() {
+				return
+			}
+			if v.Kind() == reflect.Pointer {
+				if seen[v.Pointer()] {
+					return
+				}
+				seen[v.Pointer()] = true
+			}
+			walk(path+"*", v.Elem())
+		case reflect.Struct:
+			for i := 0; i < v.NumField(); i++ {
+				walk(path+"."+v.Type().Field(i).Name, v.Field(i))
+			}
+		case reflect.Slice, reflect.Array:
+			if v.Kind() == reflect.Slice && v.Type().Elem().Kind() == reflect.Uint8 {
+				require.NotContains(t, string(v.Bytes()), testKey, path)
+				return
+			}
+			for i := 0; i < v.Len(); i++ {
+				walk(fmt.Sprintf("%s[%d]", path, i), v.Index(i))
+			}
+		case reflect.Map:
+			for _, k := range v.MapKeys() {
+				walk(path+"{key}", k)
+				walk(path+"{val}", v.MapIndex(k))
+			}
+		}
+	}
+	walk("Client", reflect.ValueOf(c))
+	require.True(t, strings.HasPrefix(c.baseURL, "http"), "walk reached the struct")
+}