Skip to content

fix(lobby): share protocol version with worker#3746

Merged
matthewevans merged 1 commit into
mainfrom
ship/fixlobby-share-protocol-version-with-worker
Jun 18, 2026
Merged

fix(lobby): share protocol version with worker#3746
matthewevans merged 1 commit into
mainfrom
ship/fixlobby-share-protocol-version-with-worker

Conversation

@matthewevans

Copy link
Copy Markdown
Member

No description provided.

@matthewevans matthewevans enabled auto-merge June 18, 2026 20:44

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 25 to 29
const protocolVersion = Number(frame.data?.protocol_version ?? 0);
if (
protocolVersion < MIN_SUPPORTED_PROTOCOL ||
protocolVersion > PROTOCOL_VERSION
protocolVersion < minSupportedProtocol ||
protocolVersion > serverProtocolVersion
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[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
      )

Comment on lines +24 to +33
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",
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[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.

Suggested change
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",
);

@matthewevans matthewevans force-pushed the ship/fixlobby-share-protocol-version-with-worker branch from 040c85c to ba1ee93 Compare June 18, 2026 20:57
@matthewevans matthewevans added this pull request to the merge queue Jun 18, 2026
Merged via the queue into main with commit 3bbf2a5 Jun 18, 2026
10 checks passed
@matthewevans matthewevans deleted the ship/fixlobby-share-protocol-version-with-worker branch June 18, 2026 21:48
@matthewevans matthewevans added the bug Bug fix label Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant