From 84a9a5c2e308bf78a2af352acb2194a3beb7ea18 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 20:12:01 +0200 Subject: [PATCH] fix(insurance): keep the API key out of every field fmt can print A *Client in an unexported struct field printed the key under %s and %q, and a Client or Config held by value leaked under every verb, because fmt never calls Format on unexported fields. Client now keeps the key only inside setAuth and redact closures and the org ID as a plain identifier. Client.Format is removed as redundant; Config.Format stays. Closes #325 Co-Authored-By: Claude Opus 5.5 --- pkg/insurance/client.go | 60 ++++++++++------- pkg/insurance/key_leak_test.go | 120 +++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 25 deletions(-) create mode 100644 pkg/insurance/key_leak_test.go diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index c7af007..3aad910 100644 --- a/pkg/insurance/client.go +++ b/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 { diff --git a/pkg/insurance/key_leak_test.go b/pkg/insurance/key_leak_test.go new file mode 100644 index 0000000..b7bc93b --- /dev/null +++ b/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") +}