diff --git a/CHANGELOG.md b/CHANGELOG.md index f3cb091..2681a70 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index c58d077..beb73d2 100644 --- a/pkg/insurance/client.go +++ b/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 diff --git a/pkg/insurance/client_test.go b/pkg/insurance/client_test.go index 1882200..13a6639 100644 --- a/pkg/insurance/client_test.go +++ b/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) + }) +}