[cristim]

Some variable slots in the stored-error allowlist used broad classes
([^;\s]+, \S+, "[^"]*"), which accept vertical tabs, NUL, NBSP, U+2028
and other format characters, and the unknown-provider slot accepted any
text including newlines. Each slot now admits only what its writer
produces: Postgres UUIDs for plan, execution and cloud-account IDs, a
provider ID alphabet for commitment IDs, the closed resource-type grammar
for the EC2 zonal note, a short token for an unknown provider name, and
Go %q output for the operator's quoted reason. Anything else falls back
to the category message, as before. The allowlist comment now states the
rule. Test fixtures that used non-UUID IDs now use the real shape.

Closes #1721

Verification

Local at 87de708 (rebased on c376828): go vet, unit tests for internal/purchase and internal/api, integration tests matching Sanitize/StoredError/Note, and golangci-lint on internal/purchase all pass. The rebase kept main's new Redshift refusal notes. They are fixed regexp.QuoteMeta constants, so they add no variable slot. The new test TestSanitizeStoredError_SlotsRejectWhitespaceAndControls fills every slot with whitespace, line breaks, NUL, NEL, NBSP, U+2028 and a zero-width space and checks none passes verbatim, and checks the real ID shapes still do.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/server-note-slot-alphabets
• File: .coderabbit.yaml
• Commit: bbaf49b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: e89a0077-1de8-44df-a4a6-7db5a4845b75

Commits

Reviewing files that changed from the base of the PR and between a392f7e and bbaf49b.

Files selected for processing: 2

• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVED 87de708 by claude-ui-fix-2

Cold review of #1721 (+67/-15, internal/purchase/errnotes.go and its tests).

Zero-width, bidi and control characters: closed

• Every variable slot that took a broad class ([^;\s]+, \S+, "[^"]*", [^:\s]+) now takes its writer's own alphabet:

  • uuidPattern (lowercase hex 8-4-4-4-12, as Postgres and uuid.String() render) for plan, execution and cloud-account IDs;
  • commitmentIDPattern [A-Za-z0-9._:/-]{1,256} for ARNs and Azure or GCP resource paths;
  • identityKeyPattern for skipped rec keys;
  • the closed resourceTypePattern grammar for the EC2 zonal note;
  • [A-Za-z0-9._+-]{0,32} for an unknown provider name.

  These are ASCII-only classes, so U+200B (ZWSP), U+200C/D, U+202A-202E and U+2066-2069 (bidi controls), U+00A0, U+0085, U+2028 and C0 controls can't enter them.

• The operator's reason (quotedTextPattern) is the one Unicode slot: (?:[^"\\\p{C}\p{Z}]| |\\[ -~])*. \p{C} excludes every control, format (Cf: zero-width and bidi), surrogate and private-use code point, and \p{Z} excludes every separator except an explicit ASCII space. That matches what Go's %q writes, since strconv.Quote escapes non-printable runes as \uXXXX, which the \\[ -~] branch plus plain characters admits. A raw bidi override in a stored row therefore falls back to the category message.

Homoglyphs

The ID, provider and resource-type slots are ASCII-only, so a Cyrillic or Greek look-alike can't impersonate an ID. The free-text reason does allow printable non-ASCII letters, which is correct: it's the admin's own deliberately typed text, quoted and labelled reason=, and it can't impersonate the fixed fields before it.

Length limits

Each ID class is bounded (256, 32 and the fixed UUID length). The reason pattern is unbounded *, but its only writer caps input at riExchangeResolveReasonMax = 300 (api/handler_ri_exchange_resolve.go:32) before %q, so the stored text is bounded at its source.

Legitimate messages still render

The real ID shapes pass verbatim in TestSanitizeStoredError_SlotsRejectWhitespaceAndControls's positive set: an ARN commitment, a UUID plan or execution or account, a full RecIdentityKey, m5.large, oracle-cloud, and a reason with \" and \n escapes. The existing allowlist tests now use real UUID shapes. A non-UUID legacy id only loses the optional execution <id>: prefix, or falls back to the category message, which fails closed. The Redshift refusals stay fixed QuoteMeta constants.

Attacks tried (reading)

• whitespace, \n \r \t \v \f, NEL, NBSP, U+2028, ZWSP and NUL in each slot are all rejected, and the test covers each of these;
• a bidi override or ZWSP inside a UUID or commitment ID can't match the ASCII classes;
• a smuggled " to break out of reason="…" can only appear escaped (\\[ -~]), so the closing quote is the real one.

CI at this SHA was still running when I posted (4 pass, 32 pending). The gate needs it green.

[cristim]

APPROVED bbaf49b by claude-ui-fix-2

Merge-only delta from my approval at 87de708f.

• Parents: 87de708f (the approved head) and main a392f7e6 (test(purchase): pin newAccountExecution as the per-account key writer #1823, the scanner pin fix).
• PR side: git diff a392f7e6 bbaf49b4de7eb076a011c49ac0ba950053149a37 is byte-identical to the approved merge-base..87de708f diff, index lines ignored.
• Main side: git diff 87de708f bbaf49b4de7eb076a011c49ac0ba950053149a37 is only main's own changes, hunk offsets ignored.
• Remerge: git show --remerge-diff is empty, so there are no conflicts and no hand edits.

CI at this SHA was still running when I posted (1 pass, 35 pending), and the gate needs it green.

[cristim]

APPROVED bbaf49b by claude-darling

Cold review; I did not write this change. I reviewed 87de708. bbaf49b is a merge of main (a392f7e) that leaves the PR's own diff identical. From main, the only render-path change is a Redshift retry gate in handler_purchases.go (+4) that returns a 409 and writes no stored error. Two low findings, neither blocking.

Source to render

• Writers: the notes are written into exec.Error and ri_exchange_history.error:

  • execution.go:923,1130,1133;
  • plan_step_attach.go:239,286;
  • manager.go:673, which formats the provider name with %q;
  • the admin's reason, via %q, at handler_ri_exchange_resolve.go:257. It is 10 to 300 runes, with Cc characters rejected at :207.

  Provider error text can land in the same fields.

• Filter: an allowlist applied only when the text is read: SanitizeStoredError (errsummary.go:294) calls renderServerNotes (errnotes.go:169). A note passes only if every "; " item fully matches a closed pattern; anything else becomes a fixed category message. Nothing is truncated.

• Frontend: escapes on display (utils.ts:240 escapeHtml, used by history.ts:1440 and riexchange.ts:2201).

Attacks

• Fall back to the category message:

  • zero-width characters U+200B to U+200D, U+2060 and U+FEFF;
  • bidi controls U+202A to U+202E, U+2066 to U+2069 and U+061C, including "refund approved" with U+202E;
  • control and odd characters: LF, TAB, ESC, DEL, NUL, U+0085, U+009B, U+00A0, U+2028, U+3000, a tag character, private use, and unassigned code points.

• Pass unchanged: %q-escaped U+202E and U+200B are stored as escape text and are inert.

• Homoglyphs are blocked:

  • a Cyrillic-e version of an allowlisted note;
  • a capitalised version, or one with a suffix or a zero-width character;
  • an allowlisted note followed by "; refund approved".

  All of these fall back, because matching is exact and anchored.

• Bad identifiers fall back: commitment IDs with 0xFF, Cyrillic, <b> or 257 characters; an upper-case UUID; a provider name containing U+202E.

• Allowed by design: Cyrillic, zalgo, and NFD or NFC text inside an admin-typed reason. The reason is printable text the admin entered.

• Real messages: all the allowlisted notes, Azure reservation-order paths, GCP selfLinks, Savings Plan ARNs and the m5.large zonal note still render. Real AWS, Azure and GCP error strings (Spanish text included) fall back to the category message, as intended.

Findings

1. Low (errnotes.go:97, quotedTextPattern): a raw invalid byte passes the reason slot. Go's regexp reads it as U+FFFD, which the class doesn't exclude. So reason="a\xffb" passes unchanged. The %q writer never produces this, and JSON output turns it into U+FFFD, so the effect is cosmetic. Fix: exclude \x{FFFD}; input validation already rejects it.
2. Low, docs (errnotes.go:15-17). The comment says no slot matches whitespace, but the reason slot accepts an ASCII space on purpose.
3. Info, existing before this PR. Skipped notes for "Red Hat Enterprise Linux" or "Windows with SQL Server" fall back to the category message, because the Cost Explorer platform value contains spaces. The old pattern failed on it too.

Probes at 87de708 (each reverted, tree clean afterwards)

• Killed:
  • the reason slot set back to the old class;
  • the Cf (bidi and zero-width) exclusion dropped;
  • case-insensitive matching;
  • the end anchor removed;
  • the identity, UUID, provider and EC2 slots each widened back.
• Survived (each worth one assertion):
  • removing the {1,256} commitment-ID length limit;
  • widening the reason escape from \\[ -~] to \\., which would let a backslash plus a raw U+202E through.

Runs

• Locally at 87de708: go test ./internal/purchase/ ./internal/api/ and the scoped -tags integration -run 'Sanitize|StoredError|Note' both pass.
• CI at bbaf49b is green, 40 of 40 checks: Unit Tests, Integration Tests (purchase) and Integration Tests (api), run https://github.com/LeanerCloud/cloud-commitments-platform/actions/runs/42300783082.