fix(gateway): truncateCloseReason drops partial UTF-8 code point instead of emitting mojibake#100047
Conversation
|
Closing — will re-open manually. |
|
Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 8:21 PM ET / 00:21 UTC. Summary PR surface: Source +7, Tests +58. Total +65 across 2 files. Reproducibility: yes. A focused read-only Node check reproduces current-main behavior as 121 bytes with U+FFFD for the reported input and shows the PR logic returning 118 bytes without replacement characters. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Land one helper-level UTF-8 boundary fix with its regression tests, then let the linked canonical issue close through the PR. Do we have a high-confidence way to reproduce the issue? Yes. A focused read-only Node check reproduces current-main behavior as 121 bytes with U+FFFD for the reported input and shows the PR logic returning 118 bytes without replacement characters. Is this the best way to solve the issue? Yes. The existing close-reason helper is the narrowest maintainable fix point because the dynamic gateway handshake/auth close paths already funnel through it; patching individual callers would duplicate the byte-boundary invariant. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4ec7842be0af. Label changesLabel justifications:
Evidence reviewedPR surface: Source +7, Tests +58. Total +65 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (4 earlier review cycles)
|
|
Added real behavior proof. Running the fix branch logic against the reported input: BEFORE fix - byte length: 121 (exceeds RFC 6455 123-byte cap), contains mojibake: true Screenshot attached showing terminal output. Unit tests also pass: 28/28. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
…ead of emitting mojibake Buffer.subarray at a raw byte offset can cut inside a multi-byte UTF-8 sequence. Node decodes the dangling continuation bytes as U+FFFD (3 bytes each), so the re-encoded result can exceed the intended maxBytes cap — violating the RFC 6455 close-reason size contract and surfacing garbled text to clients. Back up from the cut point to the nearest UTF-8 sequence start before slicing (UTF-8 continuation bytes match 10xxxxxx; skip them). Closes openclaw#99976
d02eab9 to
9aab516
Compare
|
Land-ready verification completed on exact PR head
No known proof gaps. |
|
Merged via squash.
|
…ead of emitting mojibake (openclaw#100047) * fix(gateway): truncateCloseReason drops partial UTF-8 code point instead of emitting mojibake Buffer.subarray at a raw byte offset can cut inside a multi-byte UTF-8 sequence. Node decodes the dangling continuation bytes as U+FFFD (3 bytes each), so the re-encoded result can exceed the intended maxBytes cap — violating the RFC 6455 close-reason size contract and surfacing garbled text to clients. Back up from the cut point to the nearest UTF-8 sequence start before slicing (UTF-8 continuation bytes match 10xxxxxx; skip them). Closes openclaw#99976 * fix(lint): remove unnecessary non-null assertion on Buffer index * ci: retrigger checks
…ead of emitting mojibake (openclaw#100047) * fix(gateway): truncateCloseReason drops partial UTF-8 code point instead of emitting mojibake Buffer.subarray at a raw byte offset can cut inside a multi-byte UTF-8 sequence. Node decodes the dangling continuation bytes as U+FFFD (3 bytes each), so the re-encoded result can exceed the intended maxBytes cap — violating the RFC 6455 close-reason size contract and surfacing garbled text to clients. Back up from the cut point to the nearest UTF-8 sequence start before slicing (UTF-8 continuation bytes match 10xxxxxx; skip them). Closes openclaw#99976 * fix(lint): remove unnecessary non-null assertion on Buffer index * ci: retrigger checks (cherry picked from commit 161c458)
2 participants
What Problem This Solves
truncateCloseReasontruncated WebSocket close reasons at a raw byte boundary usingBuffer.subarray(0, maxBytes). WhenmaxBytesfell inside a multi-byte UTF-8 sequence, Node decoded the dangling continuation bytes asU+FFFDreplacement characters each 3 bytes wide. This meant:maxBytes, breaking the RFC 6455 123-byte close-reason limit.Why This Change Was Made
The fix backs up from
maxBytesto the nearest UTF-8 sequence start before slicing. UTF-8 continuation bytes match the bit pattern10xxxxxx(byte & 0xC0 === 0x80); we skip them to land on a sequence-start byte, then slice cleanly.User Impact
WebSocket close reasons are now guaranteed to stay within
maxByteswhen re-encoded and never contain replacement characters. The RFC 6455 123-byte limit is reliably enforced for callers using the 120-byte default.Evidence
Repro (from issue):
Tests added in
src/gateway/server/close-reason.test.ts(new file, 7 cases):maxBytesmaxBytesrespectedCloses #99976