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

[cristim]

Closes #286

pkg/insurance only; no exported API change.

• cleanMessage also strips Cf, Zl and Zp (zero-width, bidi override, line/paragraph separators); a test per category.
• Comparison and Plan decode errors now go through the same clean, redact, truncate pipeline as HTTPError.Message (sanitizeVendorText), keeping the original in the Unwrap chain (errors.Is(err, errMissing) still works). Decode errors are built in quote.go, which cannot reach the client's redactor.
• newClient rejects an API key with leading or trailing whitespace (the header and the redaction used different strings).
• Retry-After past the int range is capped at 24 h (Atoi ErrRange) instead of ignored; 86399, 86401 and the overflow case are pinned. > vs >= at the cap is an equivalent mutant.
• Deleted the unreachable empty-key guard in the redact closure.
• Tests pin pipeline order: split key (exact equality), key repeated, key straddling byte 200, clean-before-truncate budget.
• Not changed: the %p rule documented at client.go:33-39 (go#338 owns it).

Verification (local, httptest fixtures, no live vendor call): 13 of 14 mutants killed, each by a named test (truncate-first, redact-before-strip, clean-after-truncate, each of Cf/Zl/Zp removed, Replace once, both decode bypasses, trim check removed, ErrRange handling removed, cap off-by-one, Unwrap dropped); the survivor is the equivalent >= cap. golangci-lint run ./... and go test ./... in pkg pass (GOTOOLCHAIN=go1.26.9). After #337, most decode errors withhold vendor values, so the key-as-enum-value test uses a short identifier-shaped key, which is still echoed and is redacted here.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 18 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 0a73a980-716b-4be9-a2f1-685f8142ff69

---

📥 Commits

Reviewing files that changed from the base of the PR and between c01a9f0 and 038d0f8.

---

📒 Files selected for processing (3)

• CHANGELOG.md
• pkg/insurance/client.go
• pkg/insurance/client_test.go

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Gate review (Opus 5.5) at 234e964: non-blocking findings from my own probes (none leaks the plain key; strings.Contains(out, key) was false for every probe below).

1. Low, CHANGELOG overclaim. "strips ... (Cf, Zl, Zp) so a vendor cannot split an echoed API key": default-ignorable characters outside Cf still split a key invisibly through sanitizeVendorText: U+034F CGJ (Mn), U+FE0F / U+E0100 variation selectors (Mn), U+3164 Hangul filler (Lo), combining marks, NBSP. E.g. "m=sekret͏-key-value" survives unredacted. This matches the owner's Cf/Zl/Zp ruling and needs a hostile vendor that already holds the key, so I'm not blocking on it. Follow-up: also strip unicode.Variation_Selector and unicode.Other_Default_Ignorable_Code_Point (and Mn?), or reword the CHANGELOG.
2. Low. newClient (client.go:97-101) accepts a key that itself contains a Cf/Cc rune ("abc​defghij", "abc\x01def"). cleanMessage strips that rune from the echoed key, redact matches the raw key and misses, so k=abcdefghij is printed. Real keys are ASCII and such a key would not authenticate. Follow-up: reject cleanMessage(key) != key.
3. Nit, test honesty. TestClient_DecodeErrorsAreRedacted/zero-width_split_key_as_enum_value also passes on base 0715a9f: describeVendorValue withholds the non-identifier value, so the subtest never reaches the sanitizer. Worth saying so in the test comment.
4. Info. If a key is made of marker text (]zz, d], [redacted]), ReplaceAll output can contain the key, e.g. key d] with msg d]zz gives [redacted]zz. Not reachable with real keys.

Heads-up for #338: the textual merge with #338 is clean, but it does not compile. cfg.APIKey != strings.TrimSpace(cfg.APIKey) fails on the Secret type (client.go:89 after merge), and TestNewClient_RejectsPaddedKey builds Config{APIKey: string}. GitHub shows no conflict, so only CI catches this.

[cristim]

Gate verdict at e545153: CHANGES REQUESTED (1 blocking regression).

Blocking: this PR makes HTTPError.Message leak a key that carries an invisible Cf character. newClient (client.go:84-93) accepts a key with a leading BOM or ZWSP, because TrimSpace does not trim Cf. That is a common artifact when a key is read from a file or pasted. The header sends the key verbatim. If the vendor echoes it, the new cleanMessage strips the U+FEFF first, so redact (which looks for the raw key) no longer matches, and the bare, valid credential is printed. I reproduced it end-to-end: an httptest vendor returns 401 {"message":"invalid api key <x-api-key header>"}, with Config{APIKey: NewSecret("sekret-key-value")}:

• base c01a9f0 (main): archera API returned HTTP 401: invalid api key [redacted]
• head e545153: archera API returned HTTP 401: invalid api key sekret-key-value

This is the same header/redaction disagreement the whitespace reject was added to prevent. Also accepted: a key made only of Cf ("​​") passes the blank check. Suggested fix (fail loud, same shape as the whitespace rule): in newClient, reject cleanMessage(key) != key with an error that does not echo the key. Add tests for a ""+testKey / "​"+testKey / Cf-only key, plus the echo case above through Plan.

Otherwise clean at this head:

• Fail-before reproduced on 0715a9f: 5 new tests fail.
• 20 mutants on 234e964, 18 killed by named tests; >->>= and >->> cap-1 are equivalent. At e545153, the 6 key and pipeline mutants are all killed.
• Retry-After: -5, 0, +5, padded, hex, float, 1e3, +/-1e20, MaxInt64+1, HTTP-date and full-width digits are all sane, with no panics.
• sanitizedError keeps Is/As, and double wrapping is harmless.
• Reveal() is called once (client.go:84), and the fmt (all verbs) and json probes on Config, *Config, Client and nested holders show no key.
• pkg tests exit 0, golangci-lint reports 0 issues, the GOWORK=off build is OK, and the merge diff vs 234e964 contains only the Secret adaptation.

[cristim]

Gate verdict at 038d0f8: APPROVED (Opus 5.5 independent review).

• Blocker resolved: my end-to-end repro (NewSecret("sekret-key-value"), vendor echoes x-api-key in a 401) is now rejected at construction: archera API key contains a non-printable or non-ASCII character at byte 0. The key is not echoed. newClient is the only constructor (Client fields are unexported), and a zero Config/Secret still gets the blank-key error.
• Mutants at this head: 12/12 killed by named tests. ASCII check removed, 0x20/0x7f bounds, error echoing the key, first-byte-only and skip-last-byte loops all fail TestNewClient_KeyMustBePrintableASCII. Redact-before-clean, truncate-before-redact, drop Cf, unsanitized decode msg and the 2 Retry-After mutants (drop ErrRange, s != 0) fail their named tests.
• Earlier evidence still holds: fail-before on 0715a9f, the Retry-After edge inputs, Is/As through sanitizedError, and no key in fmt or json output.
• Local at this exact head (git archive, GOTOOLCHAIN=go1.26.9, macOS): go test ./... in pkg exit 0; golangci-lint 0 issues; GOWORK=off build OK.
• CI all green; mergeStateStatus CLEAN; labels mirror chore(insurance): P3 follow-ups from the #285 review #286. The remaining non-blocking notes (marker-text keys) are not reachable with printable-ASCII real keys.