From 234e964fdc62a2984b30e080fbfddffb101b240a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 14:46:56 +0200 Subject: [PATCH 1/3] fix(insurance): harden vendor-text sanitizing and key handling Strip Cf, Zl and Zp in cleanMessage, run Comparison and Plan decode errors through the same clean, redact, truncate pipeline as HTTPError.Message, reject API keys with surrounding whitespace, cap an out-of-range Retry-After, and drop the unreachable empty-key guard. Pin the pipeline order, repeated keys, split keys and the Retry-After boundaries with tests. Closes #286 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 7 +++ pkg/insurance/client.go | 62 +++++++++++++++---- pkg/insurance/client_test.go | 114 +++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9796de1..0b218c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,13 @@ 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 invisible format, line and paragraph + separator characters (Unicode Cf, Zl, Zp) so a vendor cannot split an echoed + API key or spoof bidi text; `Comparison` and `Plan` decode errors go through + the same strip, redact and truncate pipeline as `HTTPError.Message`; + `NewClient` rejects an API key with leading or trailing whitespace; 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 diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index 3aad910..5668348 100644 --- a/pkg/insurance/client.go +++ b/pkg/insurance/client.go @@ -94,6 +94,11 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) { if strings.TrimSpace(cfg.APIKey) == "" { return nil, errors.New("archera API key is not configured") } + // The key is sent and redacted verbatim, so surrounding whitespace would + // make the header and the redaction disagree. + if cfg.APIKey != strings.TrimSpace(cfg.APIKey) { + return nil, errors.New("archera API key has leading or trailing whitespace") + } if !isUUID(cfg.OrgID) { return nil, errors.New("archera org ID must be a UUID") } @@ -108,12 +113,7 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) { 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 } @@ -147,7 +147,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. @@ -160,7 +164,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 { @@ -234,7 +242,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 @@ -249,15 +260,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 diff --git a/pkg/insurance/client_test.go b/pkg/insurance/client_test.go index a6fa9fb..58740ad 100644 --- a/pkg/insurance/client_test.go +++ b/pkg/insurance/client_test.go @@ -185,6 +185,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}, } @@ -414,3 +418,113 @@ 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_RejectsPaddedKey(t *testing.T) { + for _, k := range []string{testKey + " ", " " + testKey, testKey + "\n"} { + _, err := newClient("http://127.0.0.1:1", Config{APIKey: k, OrgID: testOrg}, nil) + require.Error(t, err, "%q", k) + assert.NotContains(t, err.Error(), "sekret") + } +} + +// 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("zero-width split key as enum value", func(t *testing.T) { + err := reflect(t, testKey[:6]+"\u200b"+testKey[6:]) + assert.NotContains(t, err.Error(), testKey) + assert.NotContains(t, err.Error(), "sekret") + }) + 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) + }) +} From e54515312e9184c67f4df2b217c78f7280fe3ccc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 14:56:10 +0200 Subject: [PATCH 2/3] fix(insurance): adapt key whitespace check to the Secret type Co-Authored-By: Claude Sonnet 5.5 --- pkg/insurance/client.go | 6 +++--- pkg/insurance/client_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index 5da88fe..8dcf9ab 100644 --- a/pkg/insurance/client.go +++ b/pkg/insurance/client.go @@ -81,12 +81,13 @@ 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 is sent and redacted verbatim, so surrounding whitespace would // make the header and the redaction disagree. - if cfg.APIKey != strings.TrimSpace(cfg.APIKey) { + if key != strings.TrimSpace(key) { return nil, errors.New("archera API key has leading or trailing whitespace") } if !isUUID(cfg.OrgID) { @@ -97,7 +98,6 @@ 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, diff --git a/pkg/insurance/client_test.go b/pkg/insurance/client_test.go index 7175a2e..784ed29 100644 --- a/pkg/insurance/client_test.go +++ b/pkg/insurance/client_test.go @@ -546,7 +546,7 @@ func TestClient_ErrorMessageRedactsBeforeTruncating(t *testing.T) { func TestNewClient_RejectsPaddedKey(t *testing.T) { for _, k := range []string{testKey + " ", " " + testKey, testKey + "\n"} { - _, err := newClient("http://127.0.0.1:1", Config{APIKey: k, OrgID: testOrg}, nil) + _, err := newClient("http://127.0.0.1:1", Config{APIKey: NewSecret(k), OrgID: testOrg}, nil) require.Error(t, err, "%q", k) assert.NotContains(t, err.Error(), "sekret") } From 038d0f872b3d37a41d85de3fb23fea3d274e2efe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 15:00:20 +0200 Subject: [PATCH 3/3] fix(insurance): accept only printable ASCII API keys A key with a leading BOM or ZWSP passed the whitespace check, then cleanMessage stripped that character from a vendor echo before redaction ran, so the valid key was printed. Reject any byte outside 0x21-0x7E in newClient, naming the byte position and never the key. Add the end-to-end echoed-header test, drop the decode subtest that never exercised the sanitizer, and narrow the CHANGELOG claim. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 14 ++++++---- pkg/insurance/client.go | 12 +++++--- pkg/insurance/client_test.go | 53 ++++++++++++++++++++++++++++++------ 3 files changed, 60 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2d01b8..2681a70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,12 +94,14 @@ 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 invisible format, line and paragraph - separator characters (Unicode Cf, Zl, Zp) so a vendor cannot split an echoed - API key or spoof bidi text; `Comparison` and `Plan` decode errors go through - the same strip, redact and truncate pipeline as `HTTPError.Message`; - `NewClient` rejects an API key with leading or trailing whitespace; and a - `Retry-After` beyond the int range is capped at 24 h instead of ignored. + (#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 diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index 8dcf9ab..beb73d2 100644 --- a/pkg/insurance/client.go +++ b/pkg/insurance/client.go @@ -85,10 +85,14 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) { if strings.TrimSpace(key) == "" { return nil, errors.New("archera API key is not configured") } - // The key is sent and redacted verbatim, so surrounding whitespace would - // make the header and the redaction disagree. - if key != strings.TrimSpace(key) { - return nil, errors.New("archera API key has leading or trailing whitespace") + // 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") diff --git a/pkg/insurance/client_test.go b/pkg/insurance/client_test.go index 784ed29..13a6639 100644 --- a/pkg/insurance/client_test.go +++ b/pkg/insurance/client_test.go @@ -544,14 +544,54 @@ func TestClient_ErrorMessageRedactsBeforeTruncating(t *testing.T) { assert.Equal(t, strings.Repeat("x", 195)+"yyyyy...", got) } -func TestNewClient_RejectsPaddedKey(t *testing.T) { - for _, k := range []string{testKey + " ", " " + testKey, testKey + "\n"} { +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) - require.Error(t, err, "%q", k) - assert.NotContains(t, err.Error(), "sekret") + 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 @@ -574,11 +614,6 @@ func TestClient_DecodeErrorsAreRedacted(t *testing.T) { assert.NotContains(t, err.Error(), testKey) assert.Contains(t, err.Error(), `unsupported value "[redacted]"`) }) - t.Run("zero-width split key as enum value", func(t *testing.T) { - err := reflect(t, testKey[:6]+"\u200b"+testKey[6:]) - assert.NotContains(t, err.Error(), testKey) - assert.NotContains(t, err.Error(), "sekret") - }) t.Run("plan decode error", func(t *testing.T) { body := strings.Replace(validPlanJSON, `"status": "reviewed"`, `"status": "`+testKey+`"`, 1) require.NotEqual(t, validPlanJSON, body)