From dd687f02d793342d2ab3114031cae0ac12c3f847 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Sat, 10 Oct 2026 15:13:22 -0400 Subject: [PATCH] [rooms] fix issue where dev/mod cant make private instance of unowned room --- CLAUDE.md | 16 +++++++----- apps/rooms/src/rooms.app.ts | 27 ++++++++++++++------- apps/rooms/src/test/integration/api.test.ts | 9 +++++++ 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5d951b0..3116350 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,12 +123,16 @@ inconsistency here without checking the client first. `UnityAsset`/`UnityAssetHash`, and there is no `Tags`. Three shapes of one save; keep them straight. - The save reads (`rooms`: `…/saves`, `…/saves/no_unity_assets` and `…/saves/:saveId`) are - auth-gated and readable by the room's creator, a CO-OWNER, or anyone whose live `presence` - row puts them in that room. They list unpublished staged saves, so they aren't public; but - a co-owner opens the save history from the room's settings without being in the room - (creator-only showed them the buttons and a list that 403'd), and a visitor resolves which - version an instance is running from this list, so managers-only locks them out of loading - the room. The presence grant expires with the presence row. + auth-gated and readable by the room's creator, a CO-OWNER, anyone whose live `presence` + row puts them in that room, or STAFF (a `developer`/`moderator` token). They list + unpublished staged saves, so they aren't public; but a co-owner opens the save history from + the room's settings without being in the room (creator-only showed them the buttons and a + list that 403'd), and a visitor resolves which version an instance is running from this + list, so managers-only locks them out of loading the room. The presence grant expires with + the presence row. The client reads `…/saves/no_unity_assets` BEFORE it matchmakes a private + instance, when the caller has no presence in the room yet, so presence alone cannot let a + non-owner create one: a developer creating a private instance of a room they hold no role + on was 403'd there until staff were let through. - A room save writes ONLY to the subroom and its save row — never to the room. Everything the body carries describes that one revision: `Description` is the save comment shown in `…/saves`, and `PersistenceVersion`/`InventionUsage` describe the scene just saved (the diff --git a/apps/rooms/src/rooms.app.ts b/apps/rooms/src/rooms.app.ts index 8e9f4d4..d46f26f 100644 --- a/apps/rooms/src/rooms.app.ts +++ b/apps/rooms/src/rooms.app.ts @@ -443,8 +443,14 @@ async function handlePhotonAccessToken(c: Context) { * lasts only as long as the player is actually there (rows carry an absolute expiry and * expired ones don't read back). * - * The presence read only happens for someone who doesn't manage the room, so the owner's - * and a co-owner's path stays one query. + * And so may staff — a token carrying the `developer` / `moderator` role — in any room, + * like the other room-admin surfaces here. They are not in the room yet when this is + * read: the client asks for the save list BEFORE it matchmakes a private instance (to + * resolve which version to load), so there is no presence to grant on, and a developer + * who runs none of a room's roles was refused a private instance of it with a 403. + * + * The presence read only happens for someone who neither manages the room nor is staff, + * so the owner's and a co-owner's path stays one query, and staff's stays none. */ async function canReadSaves( c: Context, @@ -453,6 +459,7 @@ async function canReadSaves( accountId: number ): Promise { if (canManageRoom(room, accountId)) return true + if (await isStaff(c)) return true const instance = (await getPresence(c.env.DB, accountId))?.roomInstance return instance?.roomId === roomId } @@ -3668,7 +3675,7 @@ const app = new Hono() // A subroom's saved-data versions — the room-history / "restore a save" list. Every // save is its own `subroom_save` row (nothing is overwritten), so this is real // history, newest first, paged by skip/take. Auth-gated (401), and readable by the - // room's creator, a co-owner, or anyone whose presence puts them in the room (see + // room's creator, a co-owner, anyone whose presence puts them in the room, or staff (see // `canReadSaves`). .get( '/rooms/:roomId{[0-9]+}/subrooms/:subRoomId{[0-9]+}/saves', @@ -3684,7 +3691,8 @@ const app = new Hono() '', 'The list includes STAGED saves that were never published, so it is not public:', 'the room’s creator or a co-owner may read it, and so may anyone standing IN the room', - '(their live presence says so). Anyone else is a 403. It is what the client reads to resolve', + '(their live presence says so) and anyone whose token carries the `developer` /', + '`moderator` role. Anyone else is a 403. It is what the client reads to resolve', '“load the latest or the published version?” on entering a private instance — a', 'visitor who cannot read it cannot load what the instance is running.', '', @@ -3757,8 +3765,9 @@ const app = new Hono() 'paged wrapper, with no `{ success, error, value }` envelope around it.', '', 'Gated exactly like `…/saves`, and for the same reason: the list includes STAGED', - 'saves that were never published, so it is the room’s creator, a co-owner, or anyone', - 'whose live presence puts them in the room, and anyone else is a 403.', + 'saves that were never published, so it is the room’s creator, a co-owner, anyone', + 'whose live presence puts them in the room, or staff (a `developer` / `moderator`', + 'token), and anyone else is a 403.', '', '`TotalResults` and `TotalCount` carry the same number — the client’s paged DTO and', 'the reference disagree on the name, so both are emitted.', @@ -3826,9 +3835,9 @@ const app = new Hono() 'cannot read another’s save by guessing an id: a save that belongs elsewhere is a', '404, same as an unknown one.', '', - 'Gated like the list it details — the room’s creator, a co-owner, or anyone whose', - 'presence puts them in the room. A save id resolves whether or not it was ever published, so this', - 'reads unpublished work.', + 'Gated like the list it details — the room’s creator, a co-owner, anyone whose', + 'presence puts them in the room, or staff (a `developer` / `moderator` token). A save id', + 'resolves whether or not it was ever published, so this reads unpublished work.', ].join(' '), security: AUTHED, parameters: [ diff --git a/apps/rooms/src/test/integration/api.test.ts b/apps/rooms/src/test/integration/api.test.ts index b69c421..c5f53df 100644 --- a/apps/rooms/src/test/integration/api.test.ts +++ b/apps/rooms/src/test/integration/api.test.ts @@ -5782,6 +5782,15 @@ describe('rooms endpoints', () => { expect((await get(light, '999')).status).toBe(200) await clearPresence(999) expect((await get(light, '999')).status).toBe(403) + + // Staff read it from anywhere: the client asks for this list BEFORE it matchmakes a + // private instance, so a developer with no role on the room and no presence in it + // would otherwise be refused the instance. + const staff = async (roles: string[]) => + SELF.fetch(`${ORIGIN}${light}`, { headers: await bearer('999', roles) }) + expect((await staff(['gameClient', 'developer'])).status).toBe(200) + expect((await staff(['gameClient', 'moderator'])).status).toBe(200) + expect((await staff(['gameClient'])).status).toBe(403) }) it('GET /rooms/:id/subrooms/:sid/saves/:saveId is the detail behind a history row', async () => {