Repository navigation
Protect collaborative rooms with capability access - #33
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request adds protected room creation and access using room keys, signed cookies, and room-scoped MCP bearers. It gates HTTP, WebSocket, and MCP routes, supports key rotation, updates the web workspace flow, expands E2E coverage, and adds deployment checks. ChangesProtected room access
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/lib/session-manager.ts (1)
828-833: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winConfirm that a new protected session always persists its room-access record.
loadSessionpassesinitialRoomAccesstopersistSessiononly whenrepairedOnLoad || persistedis true. ForcreateProtectedSession,persistedis always null. The record is therefore written only ifrepairCatalogAndNamesreturnstruefor a fresh emptyY.Doc.If that function ever returns
falsefor an empty document, the room-access record is never stored.getRoomAccessthen returnsnull, every authentication path returns 401, and the room becomes permanently unreachable while its Yjs state still exists. Make the write unconditional wheninitialRoomAccessis present rather than depending on the repair result.🐛 Proposed fix
- if (repairedOnLoad || persisted) { + if (repairedOnLoad || persisted || options.initialRoomAccess) { await this.persistSession(state, { recovery: true, initialRoomAccess: options.initialRoomAccess }); }#!/bin/bash # Description: Inspect repairCatalogAndNames to confirm its return value for an empty document. rg -nP -B3 -A40 'function repairCatalogAndNames' apps/server/src/lib/session-manager.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/lib/session-manager.ts` around lines 828 - 833, Update the persistence condition in loadSession so persistSession runs whenever options.initialRoomAccess is present, regardless of repairedOnLoad or persisted. Preserve the existing recovery payload and conditional behavior for sessions without initialRoomAccess.apps/web/src/components/landing-page-client.tsx (1)
67-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssociate the validation message with the input.
When
parseRoomReferencefails, the help text at Line 82 turns red and the submit button at Line 85 becomesdisabled. Neither change is exposed to assistive technology. The input has noaria-invalidand noaria-describedby, so a screen-reader user receives no reason for the disabled button.Add an ID to the help text and reference it from the input.
♿ Proposed fix
<input data-testid="join-session-input" id="session-id-input" + aria-describedby="session-id-help" + aria-invalid={!joinValueIsValid} autoCapitalize="none" autoComplete="off" autoCorrect="off" className="text-input" inputMode="text" onChange={(event) => { setJoinValue(event.target.value); }} placeholder="a7x9k2mn or https://…/s/a7x9k2mn#roomKey=…" spellCheck={false} value={joinValue} /> - <p className={`field-help${joinValueIsValid ? '' : ' error-text'}`}> + <p id="session-id-help" className={`field-help${joinValueIsValid ? '' : ' error-text'}`}>This is a browser and layout change. Run
npx tsx e2e-validate.tsand usepnpm test:e2e-workspace-uxas the production-mode browser gate for focus and layout stability.Based on learnings: "use
pnpm test:e2e-workspace-uxas the production-mode browser gate for theme, flyout, focus, responsive, toolbar, Fit, and layout stability."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/landing-page-client.tsx` around lines 67 - 86, Add a stable unique ID to the validation help <p> and set the join-session input’s aria-describedby to that ID so assistive technology receives the validation message. Also expose the invalid state through aria-invalid based on joinValueIsValid, while preserving the existing validation styling and submit-button behavior.Sources: Coding guidelines, Learnings
e2e/support/mcp.ts (1)
94-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not report HTTP errors as timeouts.
Line 97 throws for a non-ok response inside the same
trythat line 100 catches. Thecatchrewraps every failure asMCP ${name} request failed within 15s. The status text from line 97 survives only incause, which most harness output does not print.This PR introduces many 401 responses from the new room authentication. A failing bearer now reports a timeout, which points a reader at the wrong subsystem.
Move the status check out of the
tryso the HTTP message reaches the caller unchanged.🐛 Proposed fix
+ let response: Response; try { - const response = await postModernMcp(this.endpoint, this.origin, this.room, name, args, this.nextId++); - if (!response.ok) { - throw new Error(`MCP ${name} returned HTTP ${response.status}: ${await response.text()}`); - } - return response.json() as Promise<McpPayload>; + response = await postModernMcp(this.endpoint, this.origin, this.room, name, args, this.nextId++); } catch (error) { throw new Error(`MCP ${name} request failed within ${MCP_FETCH_TIMEOUT_MS / 1_000}s`, { cause: error }); } + if (!response.ok) { + throw new Error(`MCP ${name} returned HTTP ${response.status}: ${await response.text()}`); + } + return response.json() as Promise<McpPayload>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/mcp.ts` around lines 94 - 103, Update the request flow around postModernMcp so the non-ok HTTP status check and its descriptive error occur outside the timeout-catching try block. Keep only transport or response parsing failures wrapped with the timeout message, allowing HTTP errors such as 401 to reach callers unchanged.
🧹 Nitpick comments (12)
apps/server/src/lib/room-access.ts (2)
260-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the rotated record once.
rotateExclusiveconstructs the same record object twice. If the two literals drift, the persisted record and the returned record diverge.♻️ Proposed refactor
const next = await this.createGrant(); - await this.store.setRoomAccess(sessionId, { - ...next.record, - accessVersion: current.accessVersion + 1, - createdAt: current.createdAt, - }); - return { roomKey: next.roomKey, record: { ...next.record, accessVersion: current.accessVersion + 1, createdAt: current.createdAt } }; + const record: RoomAccessRecord = { + ...next.record, + accessVersion: current.accessVersion + 1, + createdAt: current.createdAt, + }; + await this.store.setRoomAccess(sessionId, record); + return { roomKey: next.roomKey, record };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/lib/room-access.ts` around lines 260 - 270, Update rotateExclusive to construct the rotated access record once after createGrant, including the incremented accessVersion and preserved createdAt, then reuse that same record for both setRoomAccess and the returned RoomAccessGrant.
227-240: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRoom cookies accumulate per session and are sent on every request.
browserCookieHeaderssets one cookie per room withPath=/. A user who opens many rooms accumulates one cookie per room. The browser then sends all of them on every request to the server origin, including the WebSocket upgrade. Browsers cap cookies per domain and servers cap request header size, so heavy users can eventually break requests.Consider scoping the cookie to the room paths it is used on, or storing one cookie that holds a bounded map of authorized rooms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/lib/room-access.ts` around lines 227 - 240, Update browserCookieHeaders so room cookies are not sent on every server-origin request: scope each cookie’s Path to the specific room route(s) that consume it, or replace the per-room cookie format with one bounded authorization map while preserving session, accessVersion, expiry, and signature validation.apps/server/src/lib/room-access.test.ts (1)
49-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for rotation revocation.
The suite tests cookie signing, expiry, and bearer validation, but it does not test
rotate. Rotation is a stated requirement of the linked issue: it must invalidate the previous room key and the previously issued cookie. Add a case that rotates the grant, then asserts that the old cookie and the old bearer both fail with 401 and that the new key succeeds.💚 Proposed test
+ it('revokes prior cookies and bearers after rotation', async () => { + const access = new RoomAccessService(store, { cryptoProfile: 'test', cookieSecret: 'test-secret' }); + const grant = await access.createGrant(); + await store.setRoomAccess('abc123de', grant.record); + const staleCookie = (access.browserCookieHeaders('abc123de', 1)['set-cookie'] as string).split(';')[0]!; + + const rotated = await access.rotate('abc123de', 1); + expect(rotated.record.accessVersion).toBe(2); + await expect(access.authenticateBrowserCookie('abc123de', request({ cookie: staleCookie }))).rejects.toMatchObject({ status: 401 }); + await expect(access.authenticateBearer(request({ authorization: `Bearer abc123de.${grant.roomKey}` }))).rejects.toMatchObject({ status: 401 }); + await expect(access.authenticateBearer(request({ authorization: `Bearer abc123de.${rotated.roomKey}` }))).resolves.toEqual({ sessionId: 'abc123de', accessVersion: 2 }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/lib/room-access.test.ts` around lines 49 - 71, Extend the existing RoomAccessService test around createGrant and cookie authentication to cover rotate: issue and retain the original room key, bearer, and signed cookie, rotate the grant, then assert the old cookie and bearer reject with status 401 while authentication using the rotated key succeeds.apps/web/src/lib/room-access-api.test.ts (1)
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe hardcoded server origin couples the test to one environment.
The assertion pins
http://localhost:4000. If the API base URL is read from an environment variable, this test fails whenever that variable differs, for example in CI or in a production-mode run. Derive the expected prefix from the same source the client uses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/room-access-api.test.ts` around lines 30 - 34, Update the request assertion in the room access API test to derive the expected server origin from the same environment-backed configuration used by the client, rather than hardcoding localhost:4000. Keep the existing path, method, credentials, and signal expectations unchanged.apps/web/src/components/landing-page-client.tsx (1)
16-29: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider aborting the in-flight room creation on unmount.
createRoomaccepts anAbortSignal, buthandleCreatepasses none. If the user leaves the landing page after clicking the button, the request still completes androuter.pushat Line 24 runs. The user is then pulled to/s/<id>#roomKey=…from whatever page they moved to.The server performs a memory-hard derivation during
createGrant, so the response is not instantaneous and this window is reachable.♻️ Proposed refactor
+ const abortRef = useRef<AbortController | null>(null); + useEffect(() => () => { abortRef.current?.abort(); }, []); + const handleCreate = async () => { if (createPending) { return; } setCreatePending(true); setCreateFailed(false); + const controller = new AbortController(); + abortRef.current = controller; try { - const room = await createRoom(); + const room = await createRoom(controller.signal); + if (controller.signal.aborted) return; router.push(getRoomSharePath(room.sessionId, room.roomKey)); } catch { + if (controller.signal.aborted) return; setCreateFailed(true); setCreatePending(false); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/landing-page-client.tsx` around lines 16 - 29, Update handleCreate to use an AbortController whose signal is passed to createRoom, and abort it when the landing-page component unmounts. Ensure the aborted request cannot trigger router.push or update creation state after unmount, while preserving existing failure handling for non-abort errors.apps/server/src/index.ts (1)
98-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not report internal failures as 401 on
/accessand/rotate.
sendRoomAccessErrorconverts every non-RoomAccessErrorinto 401 "Room access denied.". The/api/roomshandler (Line 173) and the/mcphandler (Line 341) narrow toRoomAccessErrorbefore calling it, so they keep 5xx fidelity. The/accesscatch (Line 207) and the/rotatecatch (Line 240) do not narrow.A LevelDB failure inside
roomAccess.rotateorstore.getRoomAccesstherefore reaches the browser as an authentication failure. The user sees "wrong key" during an infrastructure outage, and the server emits no log line.Keep the generic 401 body for real authentication failures. Log and return 500 for the rest.
♻️ Proposed refactor
function sendRoomAccessError(response: ServerResponse, error: unknown, headers: Record<string, unknown> = {}): void { - const roomError = error instanceof RoomAccessError ? error : new RoomAccessError(401); + if (!(error instanceof RoomAccessError)) { + console.error('Room access request failed:', error); + sendJson(response, 500, { error: 'Internal server error.' }, { ...headers, 'cache-control': 'no-store' }); + return; + } + const roomError = error; sendJson(response, roomError.status, { error: 'Room access denied.' }, { ...headers, ...roomAccessErrorHeaders(roomError), 'cache-control': 'no-store', }); }Keep
assertValidSessionIdfailures as 401. Convert them explicitly at the call sites:- assertValidSessionId(accessSessionId); + if (!isValidSessionId(accessSessionId)) throw new RoomAccessError(401);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/index.ts` around lines 98 - 105, Update sendRoomAccessError and the /access and /rotate catch paths to preserve 401 responses for genuine authentication failures, including assertValidSessionId failures, while treating unexpected errors from roomAccess.rotate or store.getRoomAccess as internal failures: log them and return a 500 response instead of the generic room-access-denied 401. Keep the existing RoomAccessError handling and response headers intact.apps/server/src/lib/websocket.test.ts (1)
176-177: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd negative WebSocket authentication coverage and a rotation-revocation test.
apps/server/src/lib/websocket.test.tsonly uses the current valid room cookie. Add denial cases for nocookieheader, a cookie belonging to a different session, and a cookie with a staleaccessVersionafter rotation. Also assert that an already-upgraded WebSocket socket is terminated afterroomAccess.rotate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/lib/websocket.test.ts` around lines 176 - 177, Add negative authentication coverage in the WebSocket tests around room creation and roomCookie setup: verify connections are denied without a cookie, with a cookie from another session, and with the pre-rotation accessVersion after roomAccess.rotate. Also verify an already-upgraded WebSocket is terminated when rotate is called, while preserving the existing valid-cookie success path.Source: Coding guidelines
apps/web/src/components/room-gate.test.tsx (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer asserting only the props argument.
The assertion pins the component call arity to two arguments. The second argument is a React internal detail. React 19 removed legacy context and
forwardRefwrapping, and the renderer call signature for function components is not a public contract. A future React patch release can change the argument count and break this test without any product regression.Use
toHaveBeenCalledWithon the props alone throughmock.calls[0]?.[0], or keepobjectContainingand drop the trailingundefined.♻️ Proposed assertion change
- expect(vi.mocked(SessionWorkspace)).toHaveBeenCalledWith( - expect.objectContaining({ initialRoomKey: 'raw-key', sessionId: 'abc123de' }), - undefined, - ); + expect(vi.mocked(SessionWorkspace).mock.calls[0]?.[0]).toEqual( + expect.objectContaining({ initialRoomKey: 'raw-key', sessionId: 'abc123de' }), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/room-gate.test.tsx` around lines 39 - 42, Update the SessionWorkspace assertion in the room-gate test to validate only the first props argument, using mock.calls[0]?.[0] with the existing objectContaining check, or remove the trailing undefined from toHaveBeenCalledWith. Do not assert the component’s internal call arity.e2e/support/room-access.ts (1)
24-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer
Headers.getSetCookie()for cookie extraction.Line 25 reads
set-cookiethroughHeaders.get. When a response carries more than oneSet-Cookieheader, undici returns them joined by a comma. Line 27 then splits on;and returns the first segment, which would be a corrupted concatenation rather than the room cookie.The current server responses set exactly one cookie, so this works today. It breaks silently if a second cookie is ever added to the access or rotate responses.
Headers.getSetCookie()is available in Node 20 and later and returns each header as a separate array entry.♻️ Proposed change
function cookieFrom(response: Response, description: string): string { - const setCookie = response.headers.get('set-cookie'); - assert(setCookie, `${description} did not set a room access cookie.`); - const cookie = setCookie.split(';', 1)[0]; + const setCookies = response.headers.getSetCookie(); + assert(setCookies.length === 1, + `${description} set ${setCookies.length} cookies; expected exactly one room access cookie.`); + const cookie = setCookies[0]!.split(';', 1)[0]; assert(cookie && cookie.includes('='), `${description} returned an invalid room access cookie.`); return cookie; }Run the following script to confirm the Node target supports
getSetCookie:#!/bin/bash # Check the declared Node engine and TypeScript lib for the E2E harness. fd -H -t f 'package.json' -d 2 --exec sh -c 'echo "== $1"; jq -r ".engines // empty, .devDependencies[\"`@types/node`\"] // empty" "$1"' _ {} fd -H -t f 'tsconfig*.json' -d 2 --exec sh -c 'echo "== $1"; grep -n "\"lib\"\|\"target\"" "$1"' _ {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/room-access.ts` around lines 24 - 30, Update cookieFrom to use response.headers.getSetCookie() and select the room access cookie from the returned individual header values instead of reading the combined set-cookie string. Preserve the existing validation and return behavior, while ensuring multiple Set-Cookie headers cannot corrupt extraction.e2e/support/yjs-session.ts (1)
318-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the cookie-bearing WebSocket polyfill into one shared helper.
connectAgentPresenceine2e-workspace-ux-validate.ts(lines 202-206) builds the sameCookieWebSocketsubclass. Export one factory, for examplecookieWebSocket(cookie, origin), from this module and use it in both places. This keeps the authenticated-header contract in one location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/support/yjs-session.ts` around lines 318 - 324, Extract the cookie-bearing WebSocket subclass creation into an exported helper such as cookieWebSocket(cookie, origin) in this module, preserving the existing cookie and optional origin headers. Replace the local WebSocketPolyfill construction here and the duplicate CookieWebSocket implementation in connectAgentPresence with this shared factory.e2e-validate.ts (1)
90-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the new durable observer and the browser with
try/finally.
validate()closes the browser at line 583 and destroysdurableObserverat line 584 only on the success path. Any throw between them leaves both open. The observer now holds a live WebSocket and aY.Doc, so a failed run can hang instead of exiting. Line 396 throws explicitly inside this unguarded body.Wrap the body in
try { … } finally { await browser.close(); durableObserver.destroy(); }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-validate.ts` around lines 90 - 100, Wrap the main validation body in validate(), including the page-loading and subsequent assertions, with a try/finally block. Move browser.close() and durableObserver.destroy() into the finally block so both resources are released on success and every throw, including the explicit failure path; preserve their existing cleanup order.e2e-workspace-ux-validate.ts (1)
1644-1651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the cookie-only precondition explicit.
The second
expectAgentConnectionModalcall asserts that no raw room key remains in memory. That state holds only becauseexpectThemeContractreloads the page. The dependency is implicit, so reordering or changing the theme helper silently breaks this assertion.Add an explicit
await page.reload({ waitUntil: 'domcontentloaded' })(and the workspace wait) before the cookie-only run, or add a short comment that states the reload requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-workspace-ux-validate.ts` around lines 1644 - 1651, Make the cookie-only precondition explicit before the second expectAgentConnectionModal call: reload the page with domcontentloaded, then perform the existing workspace wait, or add a concise comment documenting that expectThemeContract must reload first. Keep the cookie-only assertion after this explicit precondition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/index.ts`:
- Around line 389-400: In the WebSocket authentication flow after
isValidSessionId, add a one-time error listener to the raw socket before calling
roomAccess.authenticateBrowserCookie, then remove that listener when
authentication settles before invoking websocketServer.upgrade. Ensure the
listener also remains effective if authentication rejects and socket.destroy()
runs, without changing the existing validation or upgrade behavior.
In `@apps/server/src/lib/env.ts`:
- Around line 13-16: Update parseBoolean to accept only the exact strings 'true'
and 'false', returning the corresponding boolean and preserving fallback only
for undefined; throw an error for any other value so invalid environment
configuration is rejected consistently with the other parsers in the file.
In `@apps/server/src/lib/mcp-server.ts`:
- Around line 384-388: Add an MCP-aware end-to-end test covering sequential
requests with different bearer tokens, verifying each request uses its current
authorized session rather than a stale closure value. Assert that the returned
roomSessionId differs or that a cross-room operation is denied, and anchor the
test around the createMcpHandler flow and authorizedSessionId handling.
In `@apps/server/src/lib/room-access.ts`:
- Around line 105-111: Update authenticateBearer and the bearer-grant flow to
avoid running the full scryptProfile derivation on every MCP request: cache
successful bearer verification briefly in memory keyed by the verified bearer,
or derive and validate a cheap HMAC token once when the bearer is granted. Keep
the existing high-cost scrypt path for one-time room-key exchange and preserve
bearer expiration and revocation checks.
In `@apps/web/src/app/globals.css`:
- Around line 1040-1050: Update the `.visually-hidden` rule in
apps/web/src/app/globals.css lines 1040-1050 by replacing the deprecated `clip`
declaration with `clip-path: inset(50%)`. In the rule at
apps/web/src/app/globals.css lines 1970-1978, delete the redundant deprecated
`clip` declaration because `clip-path: inset(50%)` is already present.
In `@apps/web/src/components/room-gate.tsx`:
- Around line 98-117: Update the exchangeRoomKey error handling in the room-gate
flow to clear the fragment only when the caught error is a RoomAccessApiError
with an authentication-failure status. Import and use RoomAccessApiError for
this check; preserve the fragment for network errors and 5xx responses while
keeping the existing access-check and gate-state behavior.
In `@apps/web/src/components/session-workspace.tsx`:
- Around line 1219-1225: Update resetRoomKey to catch rotateRoomKey failures,
set room-key-related feedback to the existing error state, and announce the
failure through setRoomKeyAnnouncement so the live region reports it. Ensure the
callback handles both RoomAccessApiError and network rejections without
returning an unhandled rejected promise, while preserving the existing success
updates.
In `@apps/web/src/components/workspace-settings.tsx`:
- Line 312: Update the keyResetMessage status region in the workspace settings
component to render unconditionally with aria-live and its existing styling,
changing only the text content based on keyResetMessage; preserve the current
empty state when no message exists.
In `@docs/arielcharts-spec.md`:
- Line 240: Update the non-flowchart editing description near the sequence,
class, and state diagram scope in the documentation to remove “read-only” and
instead exclude only structural canvas editing, while preserving source editing
support.
In `@docs/context-map.md`:
- Around line 66-73: Renumber the stale-revision ingress item following the new
item 4 to 5, preserving its existing text and numbering sequence.
In `@e2e-collaboration-validate.ts`:
- Around line 354-360: Ensure resources are acquired within the scopes protected
by their existing cleanup blocks: in e2e-collaboration-validate.ts lines
354-360, declare activeSocket before the try, assign it after opening the
socket, and terminate it with optional cleanup in the finally at line 383; in
lines 476-493, begin the try immediately after chromium.launch so createRoom and
exchangeRoomAccess failures still reach the browser.close() finally at line 758.
- Around line 388-406: Remove the duplicated local ModernMcpClient
implementation and import or extend the shared ModernMcpClient from
./e2e/support/mcp so requests retain timeout-aware postModernMcp behavior and
shared members. Preserve only the suite-specific createDiagramWithLatestRevision
stale-revision retry, implemented in a thin subclass or added to the shared
client as appropriate, while keeping the existing constructor and bound-room
behavior.
In `@e2e-validate.ts`:
- Around line 90-100: Move room provisioning into the existing resource-cleanup
scopes: in e2e-validate.ts lines 90-100, wrap the flow in try/finally and always
call browser.close() and durableObserver.destroy(); in e2e-sequence-validate.ts
lines 281-285, move createRoom inside the existing try that closes the context
and browser; in e2e-workspace-ux-validate.ts lines 1629-1635, move createRoom
and exchangeRoomAccess inside the try whose finally calls browser.close().
---
Outside diff comments:
In `@apps/server/src/lib/session-manager.ts`:
- Around line 828-833: Update the persistence condition in loadSession so
persistSession runs whenever options.initialRoomAccess is present, regardless of
repairedOnLoad or persisted. Preserve the existing recovery payload and
conditional behavior for sessions without initialRoomAccess.
In `@apps/web/src/components/landing-page-client.tsx`:
- Around line 67-86: Add a stable unique ID to the validation help <p> and set
the join-session input’s aria-describedby to that ID so assistive technology
receives the validation message. Also expose the invalid state through
aria-invalid based on joinValueIsValid, while preserving the existing validation
styling and submit-button behavior.
In `@e2e/support/mcp.ts`:
- Around line 94-103: Update the request flow around postModernMcp so the non-ok
HTTP status check and its descriptive error occur outside the timeout-catching
try block. Keep only transport or response parsing failures wrapped with the
timeout message, allowing HTTP errors such as 401 to reach callers unchanged.
---
Nitpick comments:
In `@apps/server/src/index.ts`:
- Around line 98-105: Update sendRoomAccessError and the /access and /rotate
catch paths to preserve 401 responses for genuine authentication failures,
including assertValidSessionId failures, while treating unexpected errors from
roomAccess.rotate or store.getRoomAccess as internal failures: log them and
return a 500 response instead of the generic room-access-denied 401. Keep the
existing RoomAccessError handling and response headers intact.
In `@apps/server/src/lib/room-access.test.ts`:
- Around line 49-71: Extend the existing RoomAccessService test around
createGrant and cookie authentication to cover rotate: issue and retain the
original room key, bearer, and signed cookie, rotate the grant, then assert the
old cookie and bearer reject with status 401 while authentication using the
rotated key succeeds.
In `@apps/server/src/lib/room-access.ts`:
- Around line 260-270: Update rotateExclusive to construct the rotated access
record once after createGrant, including the incremented accessVersion and
preserved createdAt, then reuse that same record for both setRoomAccess and the
returned RoomAccessGrant.
- Around line 227-240: Update browserCookieHeaders so room cookies are not sent
on every server-origin request: scope each cookie’s Path to the specific room
route(s) that consume it, or replace the per-room cookie format with one bounded
authorization map while preserving session, accessVersion, expiry, and signature
validation.
In `@apps/server/src/lib/websocket.test.ts`:
- Around line 176-177: Add negative authentication coverage in the WebSocket
tests around room creation and roomCookie setup: verify connections are denied
without a cookie, with a cookie from another session, and with the pre-rotation
accessVersion after roomAccess.rotate. Also verify an already-upgraded WebSocket
is terminated when rotate is called, while preserving the existing valid-cookie
success path.
In `@apps/web/src/components/landing-page-client.tsx`:
- Around line 16-29: Update handleCreate to use an AbortController whose signal
is passed to createRoom, and abort it when the landing-page component unmounts.
Ensure the aborted request cannot trigger router.push or update creation state
after unmount, while preserving existing failure handling for non-abort errors.
In `@apps/web/src/components/room-gate.test.tsx`:
- Around line 39-42: Update the SessionWorkspace assertion in the room-gate test
to validate only the first props argument, using mock.calls[0]?.[0] with the
existing objectContaining check, or remove the trailing undefined from
toHaveBeenCalledWith. Do not assert the component’s internal call arity.
In `@apps/web/src/lib/room-access-api.test.ts`:
- Around line 30-34: Update the request assertion in the room access API test to
derive the expected server origin from the same environment-backed configuration
used by the client, rather than hardcoding localhost:4000. Keep the existing
path, method, credentials, and signal expectations unchanged.
In `@e2e-validate.ts`:
- Around line 90-100: Wrap the main validation body in validate(), including the
page-loading and subsequent assertions, with a try/finally block. Move
browser.close() and durableObserver.destroy() into the finally block so both
resources are released on success and every throw, including the explicit
failure path; preserve their existing cleanup order.
In `@e2e-workspace-ux-validate.ts`:
- Around line 1644-1651: Make the cookie-only precondition explicit before the
second expectAgentConnectionModal call: reload the page with domcontentloaded,
then perform the existing workspace wait, or add a concise comment documenting
that expectThemeContract must reload first. Keep the cookie-only assertion after
this explicit precondition.
In `@e2e/support/room-access.ts`:
- Around line 24-30: Update cookieFrom to use response.headers.getSetCookie()
and select the room access cookie from the returned individual header values
instead of reading the combined set-cookie string. Preserve the existing
validation and return behavior, while ensuring multiple Set-Cookie headers
cannot corrupt extraction.
In `@e2e/support/yjs-session.ts`:
- Around line 318-324: Extract the cookie-bearing WebSocket subclass creation
into an exported helper such as cookieWebSocket(cookie, origin) in this module,
preserving the existing cookie and optional origin headers. Replace the local
WebSocketPolyfill construction here and the duplicate CookieWebSocket
implementation in connectAgentPresence with this shared factory.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 45a06240-9c40-4e32-8876-fedf3e1fbe55
📒 Files selected for processing (53)
.github/workflows/ci.ymlREADME.mdapps/server/.env.exampleapps/server/AGENTS.mdapps/server/src/index.test.tsapps/server/src/index.tsapps/server/src/lib/env.tsapps/server/src/lib/http.tsapps/server/src/lib/mcp-server.tsapps/server/src/lib/mcp.test.tsapps/server/src/lib/mcp.tsapps/server/src/lib/origin.tsapps/server/src/lib/persistence.tsapps/server/src/lib/room-access.test.tsapps/server/src/lib/room-access.tsapps/server/src/lib/session-manager.test.tsapps/server/src/lib/session-manager.tsapps/server/src/lib/types.tsapps/server/src/lib/websocket.test.tsapps/server/src/lib/websocket.tsapps/web/AGENTS.mdapps/web/next.config.test.tsapps/web/next.config.tsapps/web/src/app/globals.cssapps/web/src/app/page.tsxapps/web/src/app/s/[id]/page.tsxapps/web/src/components/diagram-canvas.tsxapps/web/src/components/landing-page-client.tsxapps/web/src/components/room-gate.test.tsxapps/web/src/components/room-gate.tsxapps/web/src/components/session-workspace.tsxapps/web/src/components/workspace-settings.tsxapps/web/src/lib/history-api.test.tsapps/web/src/lib/history-api.tsapps/web/src/lib/renderer-camera-policy.test.tsapps/web/src/lib/renderer-camera-policy.tsapps/web/src/lib/room-access-api.test.tsapps/web/src/lib/room-access-api.tsapps/web/src/lib/session.test.tsdocs/arielcharts-spec.mddocs/context-map.mde2e-collaboration-validate.tse2e-production-smoke.tse2e-sequence-validate.tse2e-validate.tse2e-workspace-ux-validate.tse2e/support/mcp.tse2e/support/owned-services.tse2e/support/room-access.tse2e/support/workspace.tse2e/support/yjs-session.tsfly.tomlpackage.json
This branch was successfully deployed
1 participant
What changed
#roomKeylinks for signed HttpOnly browser cookies before the workspace mountsAuthorization: Bearer <sessionId>.<roomKey>and removes session enumerationArchitecture and security boundary
The room verifier and access version are server-private LevelDB state, separate from Yjs, history, and activity. Raw keys are returned once, remain in the URL fragment or current-page memory only, and are never accepted in query parameters or tool arguments. The MCP request context is bound to the bearer-authorized room; mutations retain the existing fresh-read/revision checks.
This is lightweight capability access, not identity or RBAC. Anyone holding the full share link or MCP bearer is intentionally a full collaborator.
User impact
A bare room URL now shows an access gate without starting protected document, presence, history, or WebSocket work. A complete share link opens the room, clears the fragment after a successful exchange, and enables realtime browser/MCP collaboration. Transient exchange failures preserve the fragment for retry. Settings can copy the private link or agent prompt and rotate the room key.
The canvas also fits only on initial diagram mount or explicit user action, preventing local, remote, or renderer-kind source updates from unexpectedly changing zoom or position.
Validation
Exact head:
1e14d76c99741be2c617328a5595eee12f00da5epnpm lintpnpm typecheckpnpm test— 7 shared, 63 server, 129 web testspnpm buildpnpm test:e2e-collaboration— two browsers + MCP convergence, stale-write rejection, cross-room isolation, fragment handling, and rotation revocationpnpm test:e2e-workspace-ux— 21 checks, including stable local/remote/renderer camera transitions and explicit Fit behaviorDeployment follow-through
The main-only Fly job requires repository secrets
FLY_API_TOKENandROOM_COOKIE_SECRET. Production verification also requires theapi.arielcharts.donovanyohan.comFly certificate/DNS mapping and the Vercel web environment to point at that API origin.Closes #30