chore(insurance): harden vendor-text sanitizing and key handling

CHANGELOG.md

@@ -93,6 +93,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- **`pkg/insurance`:** hardened vendor-text handling in `Client` errors
+  (#286). The sanitizer now also strips format, line and paragraph separator
+  characters (Unicode Cf, Zl, Zp) from vendor text; other invisible characters
+  (for example U+034F, variation selectors, U+3164) are not stripped and can
+  still split an echoed key, which is why `NewClient` now rejects any API key
+  that is not printable ASCII (0x21-0x7E). `Comparison` and `Plan` decode
+  errors go through the same strip, redact and truncate pipeline as
+  `HTTPError.Message`, and a `Retry-After` beyond the int range is capped at
+  24 h instead of ignored.
 - **`pkg/insurance`:** decode errors no longer echo an uncapped vendor value
   (#336). A rejected enum value is shown only when it is a short identifier
   (`[A-Za-z0-9_.-]`, up to 32 bytes); anything else is reported as

pkg/insurance/client.go

@@ -81,9 +81,19 @@ func NewClient(cfg Config, hc *http.Client) (*Client, error) {
 }
 
 func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) {
-	if strings.TrimSpace(cfg.APIKey.Reveal()) == "" {
+	key := cfg.APIKey.Reveal()
+	if strings.TrimSpace(key) == "" {
 		return nil, errors.New("archera API key is not configured")
 	}
+	// The key travels in an HTTP header and is redacted verbatim from vendor
+	// text, so only printable ASCII is accepted: an invisible or look-alike
+	// character in the key would let a vendor echo defeat the redaction once
+	// cleanMessage strips it. The error names the position, never the key.
+	for i := 0; i < len(key); i++ {
+		if key[i] < 0x21 || key[i] > 0x7e {
+			return nil, fmt.Errorf("archera API key contains a non-printable or non-ASCII character at byte %d", i)
+		}
+	}
 	if !isUUID(cfg.OrgID) {
 		return nil, errors.New("archera org ID must be a UUID")
 	}
@@ -92,18 +102,12 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) {
 	}
 	noRedirect := *hc
 	noRedirect.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }
-	key := cfg.APIKey.Reveal()
 	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]")
-		},
+		redact:  func(msg string) string { return strings.ReplaceAll(msg, key, "[redacted]") },
 	}, nil
 }
 
@@ -137,7 +141,11 @@ 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.orgID, req.PlanID, fetchedAt)
+	cmp, err := DecodeComparison(&boundedReader{r: body, n: maxResponseBytes}, c.orgID, req.PlanID, fetchedAt)
+	if err != nil {
+		return nil, sanitizeDecodeError(err, c.redact)
+	}
+	return cmp, nil
 }
 
 // Plan reads the documented CommitmentPlan operation.
@@ -150,7 +158,11 @@ func (c *Client) Plan(ctx context.Context, planID string) (*Plan, error) {
 		return nil, err
 	}
 	defer func() { _ = body.Close() }()
-	return DecodePlan(&boundedReader{r: body, n: maxResponseBytes})
+	plan, err := DecodePlan(&boundedReader{r: body, n: maxResponseBytes})
+	if err != nil {
+		return nil, sanitizeDecodeError(err, c.redact)
+	}
+	return plan, nil
 }
 
 func (c *Client) planPath(planID string) string {
@@ -224,7 +236,10 @@ func sanitizeTransportError(err error) error {
 
 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 {
+	// A value past the int range makes Atoi return ErrRange with the clamped
+	// maximum, which the cap below handles; a negative overflow stays ignored.
+	s, err := strconv.Atoi(strings.TrimSpace(resp.Header.Get("Retry-After")))
+	if (err == nil || errors.Is(err, strconv.ErrRange)) && s > 0 {
 		// Compare before multiplying: s*time.Second overflows int64 for large s.
 		if s > int(maxRetryAfter/time.Second) {
 			e.RetryAfter = maxRetryAfter
@@ -239,15 +254,42 @@ func newHTTPError(resp *http.Response, redact func(string) string) *HTTPError {
 	}
 	raw, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10))
 	if err == nil && json.Unmarshal(raw, &shape) == nil {
-		e.Message = truncate(redact(cleanMessage(shape.Message)), maxErrorMessageLen)
+		e.Message = sanitizeVendorText(shape.Message, redact)
 	}
 	return e
 }
 
-// cleanMessage drops control characters (log forging, ANSI injection).
+// sanitizeVendorText is the one pipeline for vendor-influenced text: strip
+// invisible characters first so a split key is reassembled, then redact the
+// key, then truncate (redacting after the cut would leave a key prefix).
+func sanitizeVendorText(msg string, redact func(string) string) string {
+	return truncate(redact(cleanMessage(msg)), maxErrorMessageLen)
+}
+
+// sanitizedError carries the sanitized text of a decode error while keeping
+// the original in the Unwrap chain, so errors.Is/As (errMissing, read-size
+// and transport errors) still work.
+type sanitizedError struct {
+	msg string
+	err error
+}
+
+func (e *sanitizedError) Error() string { return e.msg }
+func (e *sanitizedError) Unwrap() error { return e.err }
+
+// sanitizeDecodeError runs a decode error through the same pipeline as
+// HTTPError.Message. Decode errors are built in quote.go, which cannot reach
+// the client's redactor.
+func sanitizeDecodeError(err error, redact func(string) string) error {
+	return &sanitizedError{msg: sanitizeVendorText(err.Error(), redact), err: err}
+}
+
+// cleanMessage drops control characters (log forging, ANSI injection) and the
+// invisible format, line and paragraph separators (Cf, Zl, Zp) that spoof
+// bidi text, forge line breaks or split an echoed key.
 func cleanMessage(msg string) string {
 	return strings.Map(func(r rune) rune {
-		if unicode.IsControl(r) {
+		if unicode.IsControl(r) || unicode.In(r, unicode.Cf, unicode.Zl, unicode.Zp) {
 			return -1
 		}
 		return r

pkg/insurance/client_test.go

@@ -191,6 +191,10 @@ func TestClient_ErrorMapping(t *testing.T) {
 		{"retry-after wraps negative if multiplied", 503, "9223372037", `{}`, "", 24 * time.Hour},
 		{"retry-after wraps tiny if multiplied", 503, "18446744074", `{}`, "", 24 * time.Hour},
 		{"retry-after exactly the cap", 503, "86400", `{}`, "", 24 * time.Hour},
+		{"retry-after one under the cap", 503, "86399", `{}`, "", 86399 * time.Second},
+		{"retry-after one over the cap", 503, "86401", `{}`, "", 24 * time.Hour},
+		{"retry-after past int64 is capped", 503, "99999999999999999999", `{}`, "", 24 * time.Hour},
+		{"retry-after negative past int64 ignored", 503, "-99999999999999999999", `{}`, "", 0},
 		{"retry-after negative ignored", 503, "-5", `{}`, "", 0},
 		{"retry-after date ignored", 503, "Wed, 21 Oct 2026 07:28:00 GMT", `{}`, "", 0},
 	}
@@ -484,3 +488,148 @@ func TestBoundedReader_SurfacesProbeError(t *testing.T) {
 	_, err = io.ReadAll(&boundedReader{r: probeErrReader{io.EOF}, n: 0})
 	assert.NoError(t, err)
 }
+
+// errorMessageFor serves one 500 response with the given vendor message and
+// returns the sanitized HTTPError.Message.
+func errorMessageFor(t *testing.T, message string) string {
+	t.Helper()
+	b, err := json.Marshal(map[string]string{"message": message})
+	require.NoError(t, err)
+	c := testClient(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+		_, _ = w.Write(b)
+	}))
+	_, err = c.Plan(context.Background(), testPlan)
+	var he *HTTPError
+	require.ErrorAs(t, err, &he)
+	return he.Message
+}
+
+func TestClient_ErrorMessageStripsInvisibleFormatting(t *testing.T) {
+	// One input per stripped category: Cf (several), Zl, Zp.
+	for name, r := range map[string]string{
+		"Cf zero-width space":    "\u200b",
+		"Cf zero-width joiner":   "\u200d",
+		"Cf BOM":                 "\ufeff",
+		"Cf RTL override":        "\u202e",
+		"Cf isolate":             "\u2066",
+		"Zl line separator":      "\u2028",
+		"Zp paragraph separator": "\u2029",
+	} {
+		t.Run(name, func(t *testing.T) {
+			assert.Equal(t, "abcd", errorMessageFor(t, "ab"+r+"cd"))
+		})
+	}
+}
+
+func TestClient_ErrorMessageRedactsSplitAndRepeatedKeys(t *testing.T) {
+	split := testKey[:6] + "\u200b" + testKey[6:10] + "\x01" + testKey[10:]
+	// Exact equality: a redact-before-strip pipeline would leave the
+	// reassembled key.
+	assert.Equal(t, "k=[redacted]", errorMessageFor(t, "k="+split))
+	assert.Equal(t, "[redacted] [redacted] [redacted]",
+		errorMessageFor(t, testKey+" "+testKey+" "+testKey))
+}
+
+func TestClient_ErrorMessageRedactsBeforeTruncating(t *testing.T) {
+	// The redacted key ends exactly at byte 200, so it must not be cut.
+	got := errorMessageFor(t, strings.Repeat("x", 190)+testKey)
+	assert.Equal(t, strings.Repeat("x", 190)+"[redacted]", got)
+	// Truncating first would leave the key prefix before the cut.
+	got = errorMessageFor(t, strings.Repeat("x", 195)+testKey)
+	assert.Equal(t, strings.Repeat("x", 195)+"[reda...", got)
+	assert.NotContains(t, got, "sekret")
+	// Cleaning must precede truncation: stripped characters do not use budget.
+	got = errorMessageFor(t, strings.Repeat("x", 195)+strings.Repeat("\x01", 10)+"yyyyyyyyyy")
+	assert.Equal(t, strings.Repeat("x", 195)+"yyyyy...", got)
+}
+
+func TestNewClient_KeyMustBePrintableASCII(t *testing.T) {
+	rejected := map[string]string{
+		"trailing space":            testKey + " ",
+		"leading space":             " " + testKey,
+		"trailing newline":          testKey + "\n",
+		"tab":                       "sekret\tkey",
+		"NUL":                       "sekret\x00key",
+		"DEL":                       "sekret\x7fkey",
+		"BOM":                       "\ufeff" + testKey,
+		"ZWSP":                      "\u200b" + testKey,
+		"NBSP":                      testKey + "\u00a0",
+		"ideographic space":         "\u3000" + testKey,
+		"combining grapheme joiner": "sekret\u034fkey",
+		"variation selector":        "sekret\ufe0fkey",
+		"hangul filler":             "sekret\u3164key",
+		"invalid UTF-8":             "sekret\xffkey",
+		"non-ASCII letter":          "sekr\u00e9t",
+		"all invisible":             "\u200b\u200d\ufeff",
+	}
+	for name, k := range rejected {
+		t.Run("rejects "+name, func(t *testing.T) {
+			_, err := newClient("http://127.0.0.1:1", Config{APIKey: NewSecret(k), OrgID: testOrg}, nil)
+			require.Error(t, err)
+			assert.NotContains(t, err.Error(), "sekret")
+		})
+	}
+	_, err := newClient("http://127.0.0.1:1", Config{APIKey: NewSecret("ab\u200bc"), OrgID: testOrg}, nil)
+	assert.EqualError(t, err, "archera API key contains a non-printable or non-ASCII character at byte 2")
+	for _, k := range []string{"aZ09-_.~+/=", "!", "~"} {
+		_, err := newClient("http://127.0.0.1:1", Config{APIKey: NewSecret(k), OrgID: testOrg}, nil)
+		assert.NoError(t, err, k)
+	}
+}
+
+// A vendor that echoes the x-api-key header back in a 401 message must see it
+// redacted, end to end with a valid key.
+func TestClient_EchoedHeaderKeyIsRedacted(t *testing.T) {
+	c := testClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusUnauthorized)
+		b, _ := json.Marshal(map[string]string{"message": "invalid api key " + r.Header.Get("x-api-key")})
+		_, _ = w.Write(b)
+	}))
+	_, err := c.Plan(context.Background(), testPlan)
+	var he *HTTPError
+	require.ErrorAs(t, err, &he)
+	assert.Equal(t, "invalid api key [redacted]", he.Message)
+}
+
+// Decode errors are built in quote.go, which cannot reach the key; the client
+// layer must sanitize them. After #336 the decode path withholds most vendor
+// values, but a short identifier is still echoed, so a vendor that reflects the
+// key back as an enum value must be redacted.
+func TestClient_DecodeErrorsAreRedacted(t *testing.T) {
+	reflect := func(t *testing.T, value string) error {
+		t.Helper()
+		body := mutate(t, func(m map[string]any) {
+			obj(first(first(m["data"])["candidates"])["offer"])["provider"] = value
+		})
+		c := testClient(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			_, _ = w.Write([]byte(body))
+		}))
+		_, err := c.Comparison(context.Background(), ComparisonRequest{PlanID: testPlan})
+		require.Error(t, err)
+		return err
+	}
+	t.Run("key as enum value", func(t *testing.T) {
+		err := reflect(t, testKey)
+		assert.NotContains(t, err.Error(), testKey)
+		assert.Contains(t, err.Error(), `unsupported value "[redacted]"`)
+	})
+	t.Run("plan decode error", func(t *testing.T) {
+		body := strings.Replace(validPlanJSON, `"status": "reviewed"`, `"status": "`+testKey+`"`, 1)
+		require.NotEqual(t, validPlanJSON, body)
+		c := testClient(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			_, _ = w.Write([]byte(body))
+		}))
+		_, err := c.Plan(context.Background(), testPlan)
+		require.Error(t, err)
+		assert.NotContains(t, err.Error(), testKey)
+		assert.Contains(t, err.Error(), "[redacted]")
+	})
+	t.Run("sentinel survives sanitizing", func(t *testing.T) {
+		c := testClient(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			_, _ = w.Write([]byte(`{}`))
+		}))
+		_, err := c.Plan(context.Background(), testPlan)
+		require.ErrorIs(t, err, errMissing)
+	})
+}