feat: implement core virtual classroom engine - classroom durable object#47
Merged
A1L13N merged 8 commits intoalphaonelabs:mainfrom Apr 20, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Virtual Classroom proof‑of‑concept: a new standalone frontend page implementing a lobby→classroom UI and a Cloudflare Durable Object backend ( Changes
Sequence Diagram(s)sequenceDiagram
participant BrowserA as Client A (Browser)
participant ClassroomDO as ClassroomDO (Durable Object)
participant BrowserB as Client B (Browser)
BrowserA->>ClassroomDO: WebSocket upgrade (/api/classroom/:roomId?participant_id&display_name&token?)
ClassroomDO->>ClassroomDO: create session_id, restore/persist attachment, send user_info
ClassroomDO->>BrowserA: room_state, user_info, participant_joined
BrowserB->>ClassroomDO: WebSocket upgrade
ClassroomDO->>ClassroomDO: create session for B
ClassroomDO->>BrowserA: participant_joined (B)
ClassroomDO->>BrowserB: room_state, participant_joined
BrowserA->>ClassroomDO: position_update (normalized x,y,direction)
ClassroomDO->>ClassroomDO: validate/clamp, update attachments
ClassroomDO->>BrowserB: position_update (A)
BrowserA->>ClassroomDO: update_seat / leave_seat
ClassroomDO->>ClassroomDO: validate seat, enforce exclusivity, persist
ClassroomDO->>BrowserA: seat_updated / seat_left
ClassroomDO->>BrowserB: seat_occupied + room_state
BrowserA->>ClassroomDO: chat_message (text)
ClassroomDO->>ClassroomDO: trim/validate
ClassroomDO->>BrowserA: chat_message (broadcast)
ClassroomDO->>BrowserB: chat_message (broadcast)
BrowserA->>ClassroomDO: WebSocket close
ClassroomDO->>ClassroomDO: remove session, check remaining sessions
ClassroomDO->>BrowserB: participant_left (if no remaining sessions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Author
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/classroom_poc.html`:
- Around line 101-107: The participants controls lack accessible names and an
aria-expanded state: update the buttons (ids btnParticipants and btnLeave and
the participantCount span) to include explicit accessible labels (e.g.,
aria-label or visually hidden text), set button type="button" on both, add and
maintain aria-expanded on btnParticipants and toggle it whenever the sidebar
open/close logic runs, and ensure the close button has an accessible name (e.g.,
aria-label="Close participants panel") so screen readers announce purpose rather
than only an icon or count; also adjust any JS that updates participantCount to
not overwrite accessibility attributes.
- Around line 323-327: The current btnJoin click handler sets myParticipantId by
slugifying the display name (myDisplayName -> myParticipantId), which allows
collisions and hijacking; instead obtain a stable, unguessable participant id
from a trusted source (an authenticated user id or a server-issued opaque id)
and use that for myParticipantId. Replace the client-side slug logic in the
btnJoin handler (and any uses of myParticipantId derived from lobbyDisplayName)
with a call to your auth/session layer or a short fetch to the server to
retrieve/issue a participant_id (or generate a cryptographically secure UUID
client-side only as a fallback), then assign that value to myParticipantId
before joining and remove any reliance on display-name-derived slugs. Ensure
topRoomId and other UI code still use myDisplayName for presentation only.
- Around line 178-184: The chat input and icon-only submit button lack
accessible names; add a visible-for-screen-readers label tied to the input and
an accessible name for the button. Specifically, create a <label
for="chatInput"> (or a visually-hidden span with that for attribute) for the
chatInput element to provide a proper label, and give the submit button an
accessible name by adding either aria-label="Send message" to the button or a
visually-hidden <span class="sr-only">Send message</span> inside the button
(keep the icon for sighted users). Ensure ids/classes used are chatForm,
chatInput and the button element so the change is easy to find.
- Around line 345-397: disconnectWS currently forces reconnectAttempts =
MAX_RECONNECT which disables future reconnects even after a manual re-join, and
btnLeave doesn't clear seat state; change disconnectWS to accept a boolean flag
(e.g., disconnectWS(intentional)) and only set reconnectAttempts = MAX_RECONNECT
when intentional is true, otherwise leave reconnect logic intact; update
btnLeave to call disconnectWS(true) and also reset seat state variables
(isSeated, currentSeat) and any seat UI/flags so a subsequent join starts with a
fresh unseated state; leave other callers of disconnectWS() unchanged or call
disconnectWS(false) so scheduleReconnect and reconnectAttempts behavior remains
correct.
In `@src/worker.py`:
- Around line 1349-1647: Several bare "except Exception: pass" blocks silently
swallow failures and must be changed to log or report the error; update the
restore loop that calls ws.deserializeAttachment(), the server.send() block in
on_fetch, the ws.close() in on_webSocketClose, the top-level parse in
on_webSocketMessage, the exception handling in _session_for_ws, _broadcast, and
_persist_attachment to call capture_exception(error) when available (async
context) or at minimum print/structured log that includes the operation name and
the session_id (or participant_id) where applicable; keep behavior otherwise
(don’t re-raise), but ensure each except captures the exception object and logs
it with identifying symbols like session_id, participant_id, and the function
name (e.g., on_fetch, on_webSocketMessage, on_webSocketClose, _session_for_ws,
_broadcast, _persist_attachment) to aid debugging.
- Around line 1397-1417: When opening a new session for a participant_id, don't
always seed defaults; check self.sessions for an existing session with the same
participant_id (e.g. iterate self.sessions.values() or use next(...) to find a
matching entry), and if found, copy its position, direction, seat_id, is_moving
and display_name into the new session's attachment and into the
self.sessions[session_id] record before calling server.serializeAttachment;
otherwise fall back to the current defaults. Ensure you still set "session_id"
and "participant_id" in the serialized attachment and preserve the existing
fields' shapes (position as {"x","y"}) so room-state and hibernation snapshots
reflect the most recent participant state.
- Around line 1382-1417: The handler currently trusts
participant_id/display_name from the WebSocket query and accepts the socket
immediately; instead, validate the caller's auth token server-side (in your
on_fetch or a pre-accept step) and derive participant identity before
creating/accepting the WebSocket. Concretely: lookup and verify the auth token,
reject the request if unauthenticated, then use the verified identity values
(not qs values) when building attachment, session_id and when populating
self.sessions; only call WebSocketPair.new().object_values() and
self.ctx.acceptWebSocket(server) after successful authentication so
anonymous/impersonated connections are refused.
- Around line 1485-1516: The update_seat branch currently accepts any seat_id
from the client; reject invalid IDs before doing conflict checks. Add a
validation step at the start of the "update_seat" handling that verifies seat_id
is in the canonical set/range of allowed seats (e.g. compare against
self.valid_seat_ids or compute from self.room/seat_count) and return (or send an
error message like type "invalid_seat") if it’s not valid; keep this check
before iterating self.sessions for conflicts. Do not change the broadcast logic
elsewhere (leave _persist_attachment, _broadcast and _broadcast_room_state calls
intact) — only block/ignore invalid seat_id values server-side.
- Around line 46-50: Add a minimal DurableObject stub in the test shim and
expose it on the test workers module: create a class named _DurableObject
(docstring like "Minimal stub for workers.DurableObject.") and then assign
DurableObject = _DurableObject on the existing _WorkersModule class (which
already sets Response = _Response) so that importing DurableObject in the worker
module succeeds during pytest collection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69d75bbd-e702-470a-940d-cb7201474882
📒 Files selected for processing (3)
public/classroom_poc.htmlsrc/worker.pywrangler.toml
✅ Actions performedReviews paused. |
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
src/worker.py (1)
1368-1380:⚠️ Potential issue | 🔴 Critical🔒 Critical:
authenticated_useris computed but never enforced — socket still trusts query params.This one's subtle but important. At Line 1369 you compute
authenticated_userfrom the JWT token, which is great, but then at Lines 1371–1372 the code still derivesparticipant_id/display_namestraight from the untrusted query string, and at Line 1380 the socket is accepted unconditionally. The computedauthenticated_useris never read anywhere.Practical impact:
- Any client can join any room with a forged
participant_id, hijacking another learner's seat (since seat ownership keys offparticipant_idat Lines 1503–1505 and 1517–1520).- The multi-tab dedup at
_broadcast_room_statecan be weaponized to erase another participant's presence by connecting with their id.Because this is an open-source educational platform where learners may share classrooms with strangers, I'd treat this as a must-fix before merging. Either enforce auth strictly, or explicitly whitelist an "anonymous demo" mode (while still generating a server-side opaque id so one anon can't impersonate another).
🛡️ Proposed direction
- token_param = (qs.get("token") or [None])[0] - authenticated_user = verify_token(token_param or "", self.env.JWT_SECRET) if token_param else None - - participant_id = (qs.get("participant_id") or ["anon"])[0] - display_name = (qs.get("display_name") or [participant_id])[0] - - # Sanitise inputs - participant_id = participant_id[:64] - display_name = display_name[:64] + token_param = (qs.get("token") or [None])[0] + authenticated_user = ( + verify_token(token_param or "", self.env.JWT_SECRET) + if token_param else None + ) + + if authenticated_user: + # Trusted identity derived server-side. + participant_id = str(authenticated_user["id"]) + display_name = (authenticated_user.get("username") + or participant_id)[:64] + else: + # Unauthenticated demo path — mint an opaque server-side id so the + # client cannot impersonate anyone else. Display name is still + # client-provided but clearly labeled "Guest". + participant_id = f"anon-{uuid.uuid4()}" + raw_name = (qs.get("display_name") or ["Guest"])[0] + display_name = f"Guest-{raw_name[:32]}"As per coding guidelines,
**/*.py: "Check for security vulnerabilities such as SQL injection, XSS, and insecure use of eval()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1368 - 1380, The code computes authenticated_user via verify_token(self.env.JWT_SECRET) but never enforces it — stop trusting qs values directly: when authenticated_user is present, derive participant_id and display_name from authenticated_user (e.g., authenticated_user.id / name) and reject or close the socket if the token is invalid when auth-required; if anonymous mode is allowed, generate a server-side opaque participant_id (not using the qs "participant_id") and map it to the socket before calling self.ctx.acceptWebSocket; update the WebSocket pairing block around WebSocketPair.new().object_values() and any logic that uses participant_id/display_name so all downstream code (seat ownership, _broadcast_room_state) only uses server-derived IDs and never raw qs values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/classroom_poc.html`:
- Around line 72-76: The buttons btnJoin and btnLeave currently lack an explicit
type attribute (defaulting to type="submit" inside forms) which can cause
accidental form submissions if this markup is later wrapped in a form; update
the <button> elements identified by id="btnJoin" and id="btnLeave" to include
type="button" to prevent unintended submit behavior while preserving their
current click behavior.
- Around line 453-455: The ws.onmessage handler currently swallows all errors
silently (ws.onmessage -> try { handleMessage(JSON.parse(ev.data)); } catch(_)
{}); update the catch so it logs the parsing/handler error and the raw frame for
debugging (e.g. include the caught error and ev.data in a console.warn or
process logger) while preserving existing behavior; target the ws.onmessage
assignment and handleMessage invocation to add the warning message.
- Around line 710-716: The call to btnParticipants.setAttribute(...) is missing
a trailing semicolon which can cause linter errors or break if lines are later
collapsed; add a semicolon after the setAttribute(...) statement (the one that
sets 'aria-expanded' on btnParticipants) so the subsequent
participantsSidebar.classList.toggle(...) and renderParticipantsList() calls
remain safely separated; update the statement that uses
btnParticipants.setAttribute to end with ';'.
- Around line 731-742: Global keyboard handlers (keydown/keyup) are hijacking
typing in the chat: modify the window listeners to ignore events when the
focused element is the chat input (or any input/textarea/contenteditable), e.g.
check document.activeElement or e.target and return early if
activeElement.tagName is INPUT or TEXTAREA or has contentEditable="true" or
activeElement.id === 'chatInput'; only then proceed to update keysPressed, call
e.preventDefault(), and run handleSeatInteraction; apply the same guard for both
Arrow key handling in the keydown/keyup handlers and the 'e' shortcut that calls
handleSeatInteraction so typing "e" in the chat no longer triggers seat actions.
- Around line 465-472: When scheduleReconnect() bails because reconnectAttempts
>= MAX_RECONNECT, provide clear UI feedback and a recovery action: set the
connection indicator to the “disconnected” state (same visual as
WebSocket.CLOSED), display a short message or toast telling the learner the
session lost connection and suggest actions, and enable or surface a retry/leave
control (reuse btnLeave or add a retry button that calls connectWS()). Update
scheduleReconnect() to run these UI updates right before returning and clear any
reconnectTimer; reference scheduleReconnect, reconnectAttempts, MAX_RECONNECT,
btnLeave, connectWS, reconnectTimer, BASE_DELAY when making the change.
In `@src/worker.py`:
- Around line 1630-1640: Remove the stray inline comment that was appended to
the "try:" line in ClassroomDO._persist_attachment; specifically, move or delete
the "# sessions: session_id -> {ws, participant_id, display_name, position,
direction, is_moving, seat_id}" comment so the try: line reads cleanly and keep
the dict-shape doc either at its original doc location (around the earlier
comment at Line 1329) or as a nearby top-of-function comment; ensure
ws.serializeAttachment, session_id and info usage remain unchanged.
- Around line 1503-1515: The loop is creating an unused key variable other_sid
and triggers lint warnings; change the iteration over self.sessions from items()
to values() so you iterate directly over other_info (e.g., for other_info in
self.sessions.values():) and remove other_sid usage in the seat-conflict check
that compares other_info["seat_id"] and other_info["participant_id"] against
seat_id and info["participant_id"]; keep the existing ws.send(...) and
capture_exception(...) calls (retain symbols self.sessions, other_info, seat_id,
info, ws.send, capture_exception) so behavior is unchanged while eliminating the
unused-loop-var warnings.
- Around line 1494-1501: The code in the "update_seat" branch validates seat_id
with a regex and hardcodes the upper bound 15; change it to compute the max seat
number from the shared classroom layout constants (e.g. DESK_ROWS and DESK_COLS
or a single MAX_SEATS constant used by the frontend) instead of the literal 15,
keep the regex and seat_num extraction (seat_id, re.fullmatch, seat_num) the
same, and when validation fails send a lightweight response like
{"type":"invalid_seat"} back to the client rather than silently returning so the
UI can recover.
- Line 1433: The Response call is using the wrong parameter name; change the
Response(...) invocation that currently passes web_socket=client to use the
camelCase parameter webSocket=client so the runtime honors the WebSocket upgrade
(locate the Response(...) call where Response(None, status=101,
web_socket=client) is used and replace the parameter name to webSocket).
- Around line 1569-1570: on_webSocketError currently forwards to
on_webSocketClose causing an unnecessary call; change on_webSocketError to only
perform error logging and related diagnostics (e.g., log/exception with ws and
error) and stop calling await self.on_webSocketClose(ws, 1011, "WebSocket
error", False); keep cleanup solely in on_webSocketClose (which already pops
sessions and guards against double-broadcast via
_session_for_ws/self.sessions.pop), so remove the cross-call and ensure
on_webSocketError records the error context instead.
In `@tests/conftest.py`:
- Around line 124-148: Stub _WebSocketPair.new() returns a pair whose
object_values() yields two fresh MagicMocks but doesn't expose them for test
inspection; modify _WebSocketPair.new to create named mocks (e.g., client and
server), set pair.object_values.return_value = (client, server) and also attach
those mocks to the returned pair (e.g., pair.client and pair.server or
pair.client_ws and pair.server_ws) so tests can assert calls on the server
WebSocket after WebSocketPair.new().object_values() is used in worker.py.
---
Duplicate comments:
In `@src/worker.py`:
- Around line 1368-1380: The code computes authenticated_user via
verify_token(self.env.JWT_SECRET) but never enforces it — stop trusting qs
values directly: when authenticated_user is present, derive participant_id and
display_name from authenticated_user (e.g., authenticated_user.id / name) and
reject or close the socket if the token is invalid when auth-required; if
anonymous mode is allowed, generate a server-side opaque participant_id (not
using the qs "participant_id") and map it to the socket before calling
self.ctx.acceptWebSocket; update the WebSocket pairing block around
WebSocketPair.new().object_values() and any logic that uses
participant_id/display_name so all downstream code (seat ownership,
_broadcast_room_state) only uses server-derived IDs and never raw qs values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81cd620b-c891-4e00-90c1-bbd5b0f2f1e7
📒 Files selected for processing (3)
public/classroom_poc.htmlsrc/worker.pytests/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker.py`:
- Around line 1551-1564: The leave_seat handler currently processes and
broadcasts even when info["seat_id"] is empty; add an early guard at the start
of the msg_type == "leave_seat" branch that returns (or skips processing) if
info.get("seat_id") is falsy/"" so you don't iterate sessions, call
_persist_attachment, _broadcast, or _broadcast_room_state for a no-op leave;
keep the rest of the logic (clearing session["seat_id"], calling
_persist_attachment for affected s_id, and emitting the seat_left payload via
_broadcast) unchanged when a real seat_id exists.
- Around line 1629-1647: The broadcast shows stale movement because
_broadcast_room_state picks the first session per participant while
position_update only mutates the sending session; modify position_update to
mirror update_seat/leave_seat behavior by iterating over self.sessions items and
applying the new position, direction and is_moving to all sessions with the same
participant_id so all sibling sessions stay in sync, then keep the existing
dedupe in _broadcast_room_state; update the position_update function and
reference self.sessions, participant_id, position, direction, and is_moving when
implementing the propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5db57d42-ad86-4f70-a5a7-3ec3a8307bff
📒 Files selected for processing (3)
public/classroom_poc.htmlsrc/worker.pytests/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/worker.py (1)
1372-1387:⚠️ Potential issue | 🔴 CriticalReject unauthenticated sockets instead of falling back to query identity.
If
tokenis missing or invalid, this accepts caller-controlledparticipant_id/display_name, reopening impersonation and seat-hijack paths. Keep the?token=transport, but require a verified token for non-demo deployments; if anonymous POC access must remain, gate it behind an explicit environment flag.🛡️ Proposed fix
authenticated_user = verify_token(token_param or "", self.env.JWT_SECRET) if token_param else None - if authenticated_user: - # Derive identity from the verified token, not from untrusted query params. - participant_id = authenticated_user["id"] - display_name = authenticated_user.get("username") or participant_id - else: - # Allow anonymous POC joins when no token is provided. - if not participant_param: - return Response( - json.dumps({"error": "Authentication required"}), - status=401, - headers={"Content-Type": "application/json"}, - ) - participant_id = participant_param - display_name = display_name_param or participant_id + if not authenticated_user: + return Response( + json.dumps({"error": "Authentication required"}), + status=401, + headers={"Content-Type": "application/json"}, + ) + + # Derive identity from the verified token, not from untrusted query params. + participant_id = authenticated_user["id"] + display_name = authenticated_user.get("username") or participant_idBased on learnings, browsers should pass the WebSocket JWT as
?token=andClassroomDO.on_fetchshould verify it withverify_token(token_param, self.env.JWT_SECRET). As per coding guidelines,**/*.py: “Check for security vulnerabilities”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1372 - 1387, The current logic in ClassroomDO.on_fetch (the verify_token(token_param, self.env.JWT_SECRET) usage) falls back to trusting query params when token is missing/invalid; update it to reject unauthenticated sockets by default: if verify_token returns falsy, return a 401 JSON response unless an explicit environment flag (e.g., self.env.ALLOW_ANON_POC or ALLOW_ANON_DEMO) is set to true; when the flag is true, allow anonymous joins but still never accept participant_id/display_name from untrusted token-absent requests without that flag. Ensure participant_id and display_name are always taken from authenticated_user when verify_token succeeds (use authenticated_user["id"] and authenticated_user.get("username")), and remove the current unconditional fallback to participant_param/display_name_param.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker.py`:
- Around line 1368-1372: The request URL may contain a raw JWT in the token
query param (token_param) inside ClassroomDO.on_fetch (where verify_token is
called), so before passing the request or request.url to any error
logging/capture (e.g., capture_exception or Sentry), strip or redact the token
query parameter (replace value with "<redacted>" or remove the param) and use
that sanitized URL for logging; update the code paths that call
capture_exception/request logging in ClassroomDO.on_fetch and the other similar
spots in this file (the other request-url logging occurrences) to ensure no raw
?token= value is ever sent to logs/Sentry.
- Around line 1401-1446: The bug is that participant_joined is broadcast even
when an existing session for the same participant_id is reused; update the join
logic in the handler that creates self.sessions[session_id] (the block that
computes existing, initial_position/direction/seat_id and builds attachment,
server.serializeAttachment, self.sessions[...] and then sends user_info and
broadcasts) so that the participant_joined broadcast is emitted only when
existing is None (i.e., this is the participant's first active session). Keep
all other behavior (serializeAttachment, storing the session, sending user_info,
_broadcast_room_state) unchanged; conditionally call _broadcast(...) for the
"participant_joined" message only when existing is falsy to avoid duplicate join
notifications.
---
Duplicate comments:
In `@src/worker.py`:
- Around line 1372-1387: The current logic in ClassroomDO.on_fetch (the
verify_token(token_param, self.env.JWT_SECRET) usage) falls back to trusting
query params when token is missing/invalid; update it to reject unauthenticated
sockets by default: if verify_token returns falsy, return a 401 JSON response
unless an explicit environment flag (e.g., self.env.ALLOW_ANON_POC or
ALLOW_ANON_DEMO) is set to true; when the flag is true, allow anonymous joins
but still never accept participant_id/display_name from untrusted token-absent
requests without that flag. Ensure participant_id and display_name are always
taken from authenticated_user when verify_token succeeds (use
authenticated_user["id"] and authenticated_user.get("username")), and remove the
current unconditional fallback to participant_param/display_name_param.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e2295bd-c604-45c5-be01-ac70faa31110
📒 Files selected for processing (1)
src/worker.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/worker.py (2)
1377-1396:⚠️ Potential issue | 🔴 CriticalDo not downgrade missing/invalid WebSocket auth to caller-supplied identity.
Right now, any request without a valid token can still join by providing
participant_id, and an invalid token also falls through to that anonymous path. That reopens impersonation/seat-hijack risk for the production/api/classroom/<room_id>route. Keep the POC mode behind an explicit environment flag, or reject unlessverify_token(token_param, self.env.JWT_SECRET)succeeds.🛡️ Suggested fix
- authenticated_user = verify_token(token_param or "", self.env.JWT_SECRET) if token_param else None + authenticated_user = verify_token(token_param or "", self.env.JWT_SECRET) if token_param else None + allow_anonymous_poc = ( + str(getattr(self.env, "ALLOW_ANON_CLASSROOM_POC", "")).lower() + in {"1", "true", "yes"} + ) if authenticated_user: # Derive identity from the verified token, not from untrusted query params. participant_id = authenticated_user["id"] display_name = authenticated_user.get("username") or participant_id else: - # Allow anonymous POC joins when no token is provided. - if not participant_param: + # Allow anonymous POC joins only when explicitly enabled. + if token_param or not allow_anonymous_poc or not participant_param: return Response( json.dumps({"error": "Authentication required"}), status=401, headers={"Content-Type": "application/json"}, )Based on learnings, WebSocket JWT authentication in this codebase should use a
?token=query parameter verified viaverify_token(token_param, self.env.JWT_SECRET). As per coding guidelines,**/*.py: “Check for security vulnerabilities”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1377 - 1396, The code currently falls back to caller-supplied participant_id/display_name when token is missing or invalid, allowing impersonation; change the logic so that only a successfully-verified token (via verify_token(token_param, self.env.JWT_SECRET)) establishes identity, and do not accept participant_param/display_name_param unless an explicit environment flag (e.g., self.env.ALLOW_ANON_WS or similar) is set to enable anonymous POC joins; implement: check token_param first, call verify_token and if it returns a valid authenticated_user use its id/username to set participant_id/display_name, otherwise if no valid token and self.env.ALLOW_ANON_WS is truthy allow the existing anonymous path, else return a 401 JSON error — update uses of verify_token, participant_param, display_name_param, participant_id, display_name, and self.env.JWT_SECRET accordingly.
1461-1523:⚠️ Potential issue | 🟠 MajorValidate scalar fields before using or broadcasting them.
A malformed WebSocket payload can still break the handler or bypass size limits:
direction: []raises during set membership, non-stringtextraises on.strip(),"false"becomesTrueforisMoving, andtimestampis broadcast unbounded. Add a raw message cap and strict scalar checks before processing.🧰 Suggested hardening
async def on_webSocketMessage(self, ws, message): try: - data = json.loads(message) if isinstance(message, str) else json.loads(message.decode("utf-8")) + raw_message = message if isinstance(message, str) else message.decode("utf-8") + if len(raw_message) > 4096: + return + data = json.loads(raw_message) except Exception as exc: await capture_exception(exc, None, self.env, "classroom_on_webSocketMessage.parse") return + if not isinstance(data, dict): + return @@ - direction = data.get("direction", info["direction"]) - if direction not in {"up", "down", "left", "right"}: + direction = data.get("direction", info["direction"]) + if not isinstance(direction, str) or direction not in {"up", "down", "left", "right"}: direction = info["direction"] + is_moving = data.get("isMoving", False) + if not isinstance(is_moving, bool): + is_moving = False info["position"] = position info["direction"] = direction - info["is_moving"] = bool(data.get("isMoving", False)) + info["is_moving"] = is_moving @@ elif msg_type == "chat_message": - text = (data.get("text") or "").strip()[:500] + raw_text = data.get("text", "") + if not isinstance(raw_text, str): + return + text = raw_text.strip()[:500] if not text: return + raw_timestamp = data.get("timestamp", "") + timestamp = raw_timestamp[:64] if isinstance(raw_timestamp, str) else "" self._broadcast(json.dumps({ "type": "chat_message", "participant_id": info["participant_id"], "display_name": info["display_name"], "text": text, - "timestamp": data.get("timestamp", ""), + "timestamp": timestamp, }))You can verify the Python failure modes with this read-only snippet:
#!/bin/bash python - <<'PY' samples = [ {"direction": []}, {"text": {"nested": "not a string"}}, {"isMoving": "false"}, ] try: _ = samples[0]["direction"] not in {"up", "down", "left", "right"} except Exception as exc: print("direction membership raises:", type(exc).__name__) try: _ = (samples[1].get("text") or "").strip() except Exception as exc: print("non-string text .strip raises:", type(exc).__name__) print("bool('false') evaluates to:", bool(samples[2]["isMoving"])) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1461 - 1523, The handler on_webSocketMessage accepts unvalidated scalar fields which can raise exceptions or bypass limits; add strict type checks and caps: before json.loads enforce a raw message byte/char size cap and reject oversized messages; in the position_update branch keep _valid_norm_position but validate and coerce direction (in on_webSocketMessage use the allowed set {"up","down","left","right"} and default to info["direction"] if not a str or not in the set); treat isMoving only as True when the value is a boolean True or the literal string "true" (case-insensitive) otherwise False; in chat_message ensure text is a str before calling strip, coerce via text = str(text) only after verifying it is a scalar (reject dict/list), then enforce the 500-char slice; validate timestamp is a scalar string and cap its length before broadcasting; update references in the code paths for on_webSocketMessage, _valid_norm_position, the position_update branch, and the chat_message branch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/worker.py`:
- Around line 1377-1396: The code currently falls back to caller-supplied
participant_id/display_name when token is missing or invalid, allowing
impersonation; change the logic so that only a successfully-verified token (via
verify_token(token_param, self.env.JWT_SECRET)) establishes identity, and do not
accept participant_param/display_name_param unless an explicit environment flag
(e.g., self.env.ALLOW_ANON_WS or similar) is set to enable anonymous POC joins;
implement: check token_param first, call verify_token and if it returns a valid
authenticated_user use its id/username to set participant_id/display_name,
otherwise if no valid token and self.env.ALLOW_ANON_WS is truthy allow the
existing anonymous path, else return a 401 JSON error — update uses of
verify_token, participant_param, display_name_param, participant_id,
display_name, and self.env.JWT_SECRET accordingly.
- Around line 1461-1523: The handler on_webSocketMessage accepts unvalidated
scalar fields which can raise exceptions or bypass limits; add strict type
checks and caps: before json.loads enforce a raw message byte/char size cap and
reject oversized messages; in the position_update branch keep
_valid_norm_position but validate and coerce direction (in on_webSocketMessage
use the allowed set {"up","down","left","right"} and default to
info["direction"] if not a str or not in the set); treat isMoving only as True
when the value is a boolean True or the literal string "true" (case-insensitive)
otherwise False; in chat_message ensure text is a str before calling strip,
coerce via text = str(text) only after verifying it is a scalar (reject
dict/list), then enforce the 500-char slice; validate timestamp is a scalar
string and cap its length before broadcasting; update references in the code
paths for on_webSocketMessage, _valid_norm_position, the position_update branch,
and the chat_message branch accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19a00953-6eb9-4524-9442-783cf1662a37
📒 Files selected for processing (1)
src/worker.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR establishes the primary architectural foundation for the Alpha One Labs Virtual Classroom. It implements the ClassroomDO(Durable Object), the central engine responsible for orchestrating the entire room lifecycle and managing realtime participant state at the edge. By establishing this core engine, we now have a stable, centralized state layer that accessory modules (like chat and presence UIs) can safely hook into.
Do watch the Demo Video and have a look at the Proof of Concept:
https://drive.google.com/file/d/1PHGTn8toy3p5WHjbkOJPIhmTJhOASlLJ/view
What's Implemented:
High-Efficiency Real Time Sync: Implemented a Durable Object backend with WebSocket Hibernation support for optimal performance and memory usage.
Advanced State Orchestration:
Movement Relay: Smooth X/Y coordinate broadcasting across all clients.
Interactive Seat Management: Added logic for users to claim/leave seats with conflict detection (prevents two users from sitting in the same spot).
Messaging: Real Time text message broadcasting.
Robust Multi-Session Support: Engineered the system to handle multiple tabs per user without duplicating their presence in the participant list.
Virtual Classroom Engine - Core Durable Object Implementation
This PR implements an edge-resident core engine for the Alpha One Labs Virtual Classroom using a Cloudflare Durable Object (ClassroomDO), plus a browser proof-of-concept UI and supporting configuration/test shims. It provides a centralized, low-latency room state layer for realtime presence, movement, chat, and seat management with multi-session support and WebSocket hibernation awareness.
Purpose
Key changes
Impact / Review focus
Demo
Estimated review effort