[rooms] fix issue where dev/mod cant make private instance of unowned room

This commit is contained in:
Devin Zuczek
2026-10-10 15:13:22 -04:00
parent 9a3ab3e50c
commit dd687f02d7
3 changed files with 37 additions and 15 deletions
+10 -6
View File
@@ -123,12 +123,16 @@ inconsistency here without checking the client first.
`UnityAsset`/`UnityAssetHash`, and there is no `Tags`. Three shapes of one save; keep `UnityAsset`/`UnityAssetHash`, and there is no `Tags`. Three shapes of one save; keep
them straight. them straight.
- The save reads (`rooms`: `…/saves`, `…/saves/no_unity_assets` and `…/saves/:saveId`) are - 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` auth-gated and readable by the room's creator, a CO-OWNER, anyone whose live `presence`
row puts them in that room. They list unpublished staged saves, so they aren't public; but row puts them in that room, or STAFF (a `developer`/`moderator` token). They list
a co-owner opens the save history from the room's settings without being in the room unpublished staged saves, so they aren't public; but a co-owner opens the save history from
(creator-only showed them the buttons and a list that 403'd), and a visitor resolves which the room's settings without being in the room (creator-only showed them the buttons and a
version an instance is running from this list, so managers-only locks them out of loading list that 403'd), and a visitor resolves which version an instance is running from this
the room. The presence grant expires with the presence row. 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 - 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 the body carries describes that one revision: `Description` is the save comment shown in
`…/saves`, and `PersistenceVersion`/`InventionUsage` describe the scene just saved (the `…/saves`, and `PersistenceVersion`/`InventionUsage` describe the scene just saved (the
+18 -9
View File
@@ -443,8 +443,14 @@ async function handlePhotonAccessToken(c: Context<App>) {
* lasts only as long as the player is actually there (rows carry an absolute expiry and * lasts only as long as the player is actually there (rows carry an absolute expiry and
* expired ones don't read back). * expired ones don't read back).
* *
* The presence read only happens for someone who doesn't manage the room, so the owner's * And so may staff — a token carrying the `developer` / `moderator` role — in any room,
* and a co-owner's path stays one query. * 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( async function canReadSaves(
c: Context<App>, c: Context<App>,
@@ -453,6 +459,7 @@ async function canReadSaves(
accountId: number accountId: number
): Promise<boolean> { ): Promise<boolean> {
if (canManageRoom(room, accountId)) return true if (canManageRoom(room, accountId)) return true
if (await isStaff(c)) return true
const instance = (await getPresence<PresenceView>(c.env.DB, accountId))?.roomInstance const instance = (await getPresence<PresenceView>(c.env.DB, accountId))?.roomInstance
return instance?.roomId === roomId return instance?.roomId === roomId
} }
@@ -3668,7 +3675,7 @@ const app = new Hono<App>()
// A subroom's saved-data versions — the room-history / "restore a save" list. Every // 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 // 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 // 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`). // `canReadSaves`).
.get( .get(
'/rooms/:roomId{[0-9]+}/subrooms/:subRoomId{[0-9]+}/saves', '/rooms/:roomId{[0-9]+}/subrooms/:subRoomId{[0-9]+}/saves',
@@ -3684,7 +3691,8 @@ const app = new Hono<App>()
'', '',
'The list includes STAGED saves that were never published, so it is not public:', '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', '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', '“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.', 'visitor who cannot read it cannot load what the instance is running.',
'', '',
@@ -3757,8 +3765,9 @@ const app = new Hono<App>()
'paged wrapper, with no `{ success, error, value }` envelope around it.', 'paged wrapper, with no `{ success, error, value }` envelope around it.',
'', '',
'Gated exactly like `…/saves`, and for the same reason: the list includes STAGED', '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', 'saves that were never published, so it is the room’s creator, a co-owner, anyone',
'whose live presence puts them in the room, and anyone else is a 403.', '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', '`TotalResults` and `TotalCount` carry the same number — the client’s paged DTO and',
'the reference disagree on the name, so both are emitted.', 'the reference disagree on the name, so both are emitted.',
@@ -3826,9 +3835,9 @@ const app = new Hono<App>()
'cannot read another’s save by guessing an id: a save that belongs elsewhere is a', 'cannot read another’s save by guessing an id: a save that belongs elsewhere is a',
'404, same as an unknown one.', '404, same as an unknown one.',
'', '',
'Gated like the list it details — the room’s creator, a co-owner, or anyone whose', 'Gated like the list it details — the room’s creator, a co-owner, anyone whose',
'presence puts them in the room. A save id resolves whether or not it was ever published, so this', 'presence puts them in the room, or staff (a `developer` / `moderator` token). A save id',
'reads unpublished work.', 'resolves whether or not it was ever published, so this reads unpublished work.',
].join(' '), ].join(' '),
security: AUTHED, security: AUTHED,
parameters: [ parameters: [
@@ -5782,6 +5782,15 @@ describe('rooms endpoints', () => {
expect((await get(light, '999')).status).toBe(200) expect((await get(light, '999')).status).toBe(200)
await clearPresence(999) await clearPresence(999)
expect((await get(light, '999')).status).toBe(403) 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 () => { it('GET /rooms/:id/subrooms/:sid/saves/:saveId is the detail behind a history row', async () => {