fix(lobby): share protocol version with worker#3746
Conversation
There was a problem hiding this comment.
Code Review
This pull request consolidates the wire-protocol version constants into the lobby-broker Rust crate, exporting them to server-core and exposing them to the Cloudflare Worker shell via WASM. This eliminates the hand-written TypeScript protocol stub and introduces a build-time check script to prevent client-server protocol drift. Feedback is provided regarding unsafe NaN handling in the protocol version check which could bypass handshake validation, and fragile regex patterns in the version check script that are sensitive to minor formatting changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const protocolVersion = Number(frame.data?.protocol_version ?? 0); | ||
| if ( | ||
| protocolVersion < MIN_SUPPORTED_PROTOCOL || | ||
| protocolVersion > PROTOCOL_VERSION | ||
| protocolVersion < minSupportedProtocol || | ||
| protocolVersion > serverProtocolVersion | ||
| ) { |
There was a problem hiding this comment.
[HIGH] Unsafe NaN handling in protocol version check. Evidence: lobby-worker/src/hello-gate.ts:25-28.
Why it matters: If the client sends a non-numeric or malformed protocol_version, Number(...) returns NaN, which makes all comparison checks evaluate to false and incorrectly accepts the handshake.
Suggested fix: Explicitly check for NaN or default to 0 when parsing fails to ensure malformed versions are rejected.
const protocolVersion = Number(frame.data?.protocol_version);
if (
Number.isNaN(protocolVersion) ||
protocolVersion < minSupportedProtocol ||
protocolVersion > serverProtocolVersion
)| const rustVersion = extractVersion( | ||
| rustSource, | ||
| /pub const PROTOCOL_VERSION: u32 = (\d+);/, | ||
| "crates/lobby-broker/src/protocol.rs", | ||
| ); | ||
| const clientVersion = extractVersion( | ||
| clientSource, | ||
| /export const PROTOCOL_VERSION = (\d+);/, | ||
| "client/src/adapter/ws-adapter.ts", | ||
| ); |
There was a problem hiding this comment.
[MED] Fragile regex patterns for version extraction. Evidence: scripts/check-protocol-version.mjs:26.
Why it matters: The current regex patterns are sensitive to minor formatting changes (such as spacing around colons or equals signs) and could cause unexpected build failures.
Suggested fix: Use more robust regex patterns that allow flexible whitespace.
| const rustVersion = extractVersion( | |
| rustSource, | |
| /pub const PROTOCOL_VERSION: u32 = (\d+);/, | |
| "crates/lobby-broker/src/protocol.rs", | |
| ); | |
| const clientVersion = extractVersion( | |
| clientSource, | |
| /export const PROTOCOL_VERSION = (\d+);/, | |
| "client/src/adapter/ws-adapter.ts", | |
| ); | |
| const rustVersion = extractVersion( | |
| rustSource, | |
| /pub\s+const\s+PROTOCOL_VERSION\s*:\s*u32\s*=\s*(\d+)\s*;/, | |
| "crates/lobby-broker/src/protocol.rs", | |
| ); | |
| const clientVersion = extractVersion( | |
| clientSource, | |
| /export\s+const\s+PROTOCOL_VERSION\s*=\s*(\d+)\s*;/, | |
| "client/src/adapter/ws-adapter.ts", | |
| ); |
040c85c to
ba1ee93
Compare
No description provided.