Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
Comment |
|
Updated 9:36 AM PT - Jun 29th, 2026
❌ @robobun, your commit ab79e73 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32891That installs a local version of the PR into your bun-32891 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the doc-comment fix — nothing else from me. Deferring to a human reviewer since this threads a new WTF-8 encoding path through core string infrastructure (unsafe simdutf FFI + a hand-rolled C++ WTF-8 decoder), which is more than I'm comfortable auto-approving.
Extended reasoning...
Overview
This PR fixes pub/sub topic aliasing in Bun.serve WebSockets: topic names that differ only in lone surrogates previously collapsed to the same byte key because the JS→bytes path replaced unpaired surrogates with U+FFFD. The fix adds a parallel WTF-8 encoding path — strings::to_wtf8_alloc, ZigString::to_slice_wtf8, WTFStringImplExt::to_wtf8, String::to_wtf8, JSString::to_slice_wtf8, JSValue::to_slice_wtf8 — and switches every topic-name call site (subscribe/unsubscribe/isSubscribed/publish/publishText/publishBinary on both ServerWebSocket and the server, plus the *_without_type_checks fast paths) to it. uws_bindings.cpp gains a hand-rolled topicBytesToString WTF-8→UTF-16 decoder so ws.subscriptions round-trips the original JS strings. 10 files changed plus a thorough new integration test.
Security risks
The bug being fixed is itself security-adjacent (user-controlled topic names with injected lone surrogates could alias onto another subscriber's topic). The fix introduces no new untrusted-input surfaces, but the new encoding primitives must be correct: to_wtf8_alloc wraps a simdutf FFI call inside unsafe { vec::fill_spare(...) } and falls back to a scalar encoder on SURROGATE; the C++ side adds a manual multi-byte decoder. Both only ever see bytes that Bun itself produced via the new encoder (uWS stores topic keys verbatim), so malformed-input handling is defensive rather than exposed — but encoding bugs here would be subtle and broadly reachable.
Level of scrutiny
Medium-high. This is not a config tweak or a localized bugfix — it adds a new encoding family to bun_core::strings / bun_core::string and the JSC bridge layer, with unsafe FFI and a bespoke C++ decoder. Well-formed strings are claimed (and appear) byte-identical to the existing UTF-8 path, so the blast radius for normal inputs is small, but the new primitives become part of the core string toolkit and deserve a human pass on the capacity math (len * 3 + 16), the simdutf-vs-scalar fallback, and the C++ decoder's branch coverage.
Other factors
- My only prior comment (stale "UTF-8 slice" doc string on
topic_dispatch) was addressed in 6eb39c5. - The bug-hunting pass found no issues this round.
- The new test exercises every entry point (server.publish, ws.publish/publishText/publishBinary, subscribe/unsubscribe/isSubscribed, subscriberCount, ws.subscriptions round-trip) against four distinct surrogate-bearing topics plus a valid-pair control, which is solid coverage.
- The single CI failure (
sql-mysql.auth.test.tson one Linux lane) is unrelated to anything this PR touches.
Net: looks correct to me, but the scope and the new unsafe/C++ encoding code put it outside what I'll auto-approve.
|
Final CI status, after build 66795 completed on the current head ( Summary: 281 test lanes passed and 5 failed. Every failure is a tracked repo-wide CI issue, infrastructure, or an agent timeout that the same test passes on a sibling lane in the same build. None executes any code this PR changes, and The failures, by lane:
The Windows failures were all auto-annotated as flaky by CI and retried ( None of these can involve this diff: a History, for completeness: across all five CI runs of this PR the only failure that was ever real was |
6eb39c5 to
a1cf85e
Compare
There was a problem hiding this comment.
No issues found beyond the resolved nit, but this adds new WTF-8 encoding infrastructure across bun_core/jsc (including an unsafe simdutf path) plus a hand-rolled WTF-8 decoder in C++, so it's worth a human pass on the encoding details.
Extended reasoning...
Overview
This PR fixes pub/sub topic aliasing in Bun.serve WebSockets: previously, topic names were keyed via lossy UTF-8 conversion that maps every unpaired surrogate to U+FFFD, so distinct JS strings like "a\uD800b" and "a\uDC00b" collided onto the same uWS topic. The fix introduces a parallel WTF-8 conversion path (to_wtf8_alloc, String::to_wtf8, ZigString::to_slice_wtf8, JSValue::to_slice_wtf8, JSString::to_slice_wtf8) that encodes lone surrogates as their 3-byte WTF-8 sequence, switches all topic-name call sites in ServerWebSocket.rs and server_body.rs to it, and adds a hand-rolled WTF-8 → UTF-16 decoder (topicBytesToString) in uws_bindings.cpp so ws.subscriptions round-trips. Ten files touched plus a thorough new integration test.
Security risks
The bug being fixed is itself security-adjacent (user-controlled topic names could alias onto unrelated topics). The fix doesn't introduce new attack surface that I can see — WTF-8 bytes stay internal to the uWS topic map and never go on the wire. The new unsafe block in to_wtf8_alloc mirrors the existing convert_utf16_to_utf8_append pattern exactly (same simdutf call, same fill_spare contract, same SURROGATE→0-commit fallback), and the len*3+16 capacity bound is correct for WTF-8 (each u16 → ≤3 bytes). The C++ decoder only ever sees bytes produced by our own encoder, so its lax continuation-byte handling isn't exploitable, but it is a fresh hand-rolled multi-byte decoder.
Level of scrutiny
Medium-high. This is not a mechanical change: it adds new public string-encoding API across three crate layers, a new unsafe simdutf block, and a custom C++ decoder, and it changes the keying semantics of a production pub/sub system. The implementation looks correct to me (decode_wtf16_raw + encode_wtf8_rune are pre-existing and the new code just composes them; the C++ side correctly handles the 4-byte → surrogate-pair case), but encoding edge cases and the design choice of WTF-8 keying are things a maintainer should sign off on.
Other factors
- My earlier doc-comment nit was addressed (commit a1cf85e); no outstanding review comments remain.
- CI failures are unrelated infra (MySQL docker harness) per the author's analysis; the new websocket test passes on all lanes that ran.
- Test coverage for the new behavior is comprehensive (subscribe/unsubscribe/isSubscribed/subscriberCount/subscriptions round-trip plus all four publish entry points), and existing well-formed-string tests in the same file continue to exercise the unchanged-bytes guarantee.
|
Addressing the two spots flagged for a closer look, here are the invariants they rely on, so a reviewer does not have to reconstruct them.
Happy to restructure either piece if a different shape is preferred. |
…one surrogates Topic names were converted from JS strings to bytes via a lossy UTF-16 to UTF-8 path that replaces every unpaired surrogate with U+FFFD. Distinct JS strings like "a\uD800b", "a\uDC00b", "a\uDFFFb", and the literal "a\uFFFDb" all produced the same byte key, so subscribing to one made the socket a subscriber of all of them and publishes to any of them were delivered cross-topic. Key topic names as WTF-8 instead: unpaired surrogates are encoded as their 3-byte WTF-8 sequence so distinct JS strings map to distinct byte keys. ws.subscriptions decodes WTF-8 back so the round trip returns the original JS string. Well-formed strings (ASCII, Latin-1, valid surrogate pairs) encode to the same bytes as before.
a1cf85e to
e96486c
Compare
|
Rebased onto main ( Main moved 45 commits, two of which landed on top of this change.
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…Text, not publishBinary publish_text_without_type_checks copied its empty-topic guard from publish_binary_without_type_checks, including the method name in the error string. Every other topic error message in this file already names its own function.
There was a problem hiding this comment.
Both earlier nits are addressed and I didn't find any new issues, but the new unsafe simdutf path in to_wtf8_alloc and the hand-rolled WTF-8→WTF-16 decoder in uws_bindings.cpp warrant a human pass.
Extended reasoning...
Overview
This PR fixes topic-name aliasing in Bun.serve WebSocket pub/sub: distinct JS strings that differ only in lone surrogates previously collapsed to the same uWS topic key because the JS→bytes conversion replaced every unpaired surrogate with U+FFFD. The fix introduces a WTF-8 encoding path (to_wtf8_alloc in bun_core, plus to_wtf8/to_slice_wtf8 wrappers up through JSValue/JSString), switches all ~10 topic-name call sites in ServerWebSocket.rs and server_body.rs to it, and adds a custom C++ WTF-8 decoder (topicBytesToString) so ws.subscriptions round-trips. A thorough new test exercises subscribe/unsubscribe/isSubscribed/subscriberCount/publish across all entry points. The two .mdx changes are autofix.ci whitespace reflow only.
Security risks
The bug being fixed is itself security-relevant (cross-topic message delivery via surrogate injection). The fix introduces new unsafe Rust (vec::fill_spare over a simdutf FFI call with a manually-justified len*3+16 capacity bound) and a hand-written multi-byte decoder in C++. The author's invariant write-up is convincing — the capacity bound holds, the C++ decoder bounds-checks every continuation byte and always advances, and reserveInitialCapacity(n) only covers the 4-byte arm by one (it emits 2 code units but consumes 4 input bytes, so n remains an upper bound). I don't see a memory-safety issue, but this is exactly the kind of code where a second pair of human eyes is the right call.
Level of scrutiny
Medium-high. This is not a mechanical change: it threads a new encoding semantics through five layers of the string stack, adds unsafe FFI interop, and replaces a WTF library call (fromUTF8ReplacingInvalidSequences) with a bespoke decoder. The design choice (WTF-8 keying rather than, say, rejecting ill-formed topic names) is reasonable and preserves backward compatibility for well-formed input, but it's a decision a maintainer should ratify.
Other factors
Both inline nits I raised on earlier revisions (stale "UTF-8 slice" doc comment; wrong method name in the publishText fast-path error) have been fixed in e96486c and ab79e73. The current bug-hunting pass found nothing. CI's only failure (test-net-connect-memleak.js) is an unrelated node-compat flake. The new test is comprehensive and the author has documented the safety invariants in-thread, which should make the human review quick.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Problem
Bun.serveWebSocket pub/sub keys topic names by converting the JS string to UTF-8 via a lossy path that replaces every unpaired surrogate with U+FFFD. Distinct JS strings"a\uD800b","a\uDC00b","a\uDFFFb", and the literal"a\uFFFDb"all produce the same byte key, so they alias onto the same topic:Any application that builds topic names from user-controlled input (room names, user ids) can be made to receive, or have its publishes delivered to, a topic it never named by injecting a lone surrogate.
Root cause
JSValue::to_slice/ZigString::to_sliceroute 16-bit strings throughstrings::to_utf8_alloc, whose scalar fallback (append_wtf8_from_utf16) decodes withdecode_utf16_with_fffd, mapping every unpaired surrogate to U+FFFD. The topic map in uWS then sees identical byte keys.Fix
Add a WTF-8 conversion path (
strings::to_wtf8_alloc,String::to_wtf8,ZigString::to_slice_wtf8,JSValue::to_slice_wtf8,JSString::to_slice_wtf8) that encodes unpaired surrogates as their 3-byte WTF-8 sequence (ED A0 80..ED BF BF) instead of replacing them. Distinct JS strings now produce distinct byte keys.All topic-name call sites switch to the WTF-8 path:
ServerWebSocket:subscribe/unsubscribe/isSubscribed/publish/publishText/publishBinary(and the*_without_type_checksfast paths)publish/subscriberCountws.subscriptionsnow decodes the stored WTF-8 bytes back to WTF-16 so the returned array round-trips the original JS string.Well-formed strings (ASCII, Latin-1, valid surrogate pairs / emoji) encode to exactly the same bytes as before, so existing topic names are unchanged.
Verification
New test in
test/js/bun/websocket/websocket-server.test.tssubscribes to"a\uD800b"and"a😶b"only, then asserts:subscriberCount/isSubscribedreport 0 / false for"a\uDC00b","a\uFFFDb","a\uDFFFb"unsubscribeon those other topics returnsfalseand leaves the real subscription intactws.subscriptionsreturns the exact original stringsserver.publish/ws.publish/ws.publishText/ws.publishBinaryto any of the other topics deliver nothing to the subscriberTest fails on the released binary and passes with this change.