From 5a1d5a1734db91185e332777e4556b56d7c3ec99 Mon Sep 17 00:00:00 2001 From: Developer Date: Tue, 29 Sep 2026 15:08:17 -0500 Subject: [PATCH 1/3] fix: a player's own login no longer opens the GM's routes and admin socket events Every token is signed with the same secret, and the checks only asked whether a token was valid and not temporary, which a player's login token is. It passed authenticate (delete buildings, GM notes, approve accounts, reset passwords) and every admin socket check (socket admin at identify, grant editor rights, set any bank balance, NPC users, purge dice history). The checks now say what they mean: authenticate admits the GM or a granted editor; authenticatePlayer adds a player's own login for their own sheet and portrait; optionalAuthenticate treats players as anonymous; admin socket events require the GM's own login. Three tests signed a GM token without the role the real login always carries, and now sign it the real way. Tests walk a player token against every route behind the GM check, and hold GM login, granted editors, player sheet and portrait, and chat. Verified end to end on a real server in secure mode. --- CHANGELOG.md | 10 + README.md | 3 +- backend/__tests__/gm_route_auth.test.js | 313 ++++++++++++++++++ .../__tests__/shop_catalogue_sockets.test.js | 2 +- backend/__tests__/sockets.deathsave.test.js | 2 +- .../__tests__/sockets.identify.secure.test.js | 2 +- backend/middleware/auth.js | 83 +++-- backend/routes/sheets.js | 6 +- backend/sockets/index.js | 25 +- 9 files changed, 407 insertions(+), 39 deletions(-) create mode 100644 backend/__tests__/gm_route_auth.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index d169885e..2bca77c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] +### Security + +- **Players can no longer use the GM's tools.** A player's own login worked as a key to the GM's + side of the server: with the right request, a signed-in player could delete buildings, edit the + map, read GM notes, approve accounts, reset passwords, set anyone's bank balance, give themselves + editor rights, or post in chat as anyone. Only the GM, and players the GM has granted editing + rights, can now do those things. Nothing a player normally does changes: their sheet, portrait, + chat, shops and bank all work as before, and granting, revoking and giving back editor rights + work as before. + ### Under the hood - **The first piece of the system builder.** An engine that works out a sheet's derived diff --git a/README.md b/README.md index 70e66948..ddfd8246 100644 --- a/README.md +++ b/README.md @@ -334,7 +334,7 @@ CITY_NET/ │ ├── net/ │ │ └── outbound.js # Every request to a host we do not own goes through here. A named destination (exact hostname, never a suffix test), HTTPS, a deadline covering the body as well as the connection, a byte cap, and no redirect following — none of which a caller can opt out of. Two callers, one auditable surface │ ├── middleware/ -│ │ ├── auth.js # JWT verify middleware (admin + elevated users) +│ │ ├── auth.js # Who a token belongs to, in one place. Every token is signed with the same secret, so a valid signature is not enough: `authenticate` admits the GM (role admin) or a granted editor, `authenticatePlayer` also admits a player's own login for their own sheet and portrait, `optionalAuthenticate` treats players as anonymous; `isMainAdmin` is what every admin socket event checks │ │ ├── uploadConstraints.js # What an upload may be and how to say so when it is not. One message shape naming the file, what was wrong and what would have worked — plus a handler for multer's own failures, since an oversized file previously reached Express's HTML error page and the client reported a JSON syntax error to the user │ │ ├── uploadHeaders.js # What a browser may do with a file somebody uploaded. `/uploads` is served with no auth, so a sandbox CSP puts anything opened from it in an opaque origin and nosniff stops it being re-read as HTML — which is what lets the upload allowlists stay as wide as the file pickers │ │ └── rateLimit.js # A sliding per-caller ceiling, for the one open route that spends our outbound requests on an anonymous caller's say-so. Bounded in memory, since the key is whoever is asking; evicts the least recently seen, so it forgives rather than blocks @@ -415,6 +415,7 @@ CITY_NET/ │ │ └── testDb.js # In-memory SQLite factory for isolated test DBs │ ├── admin.test.js # Admin endpoints (auth, settings, undo access); update routes — 409 with a reason rather than a false success, unauthenticated status, boot id on /version; check-update against a stubbed registry — upgrades only, dev tags per channel, and a prerelease not hiding a stable release │ ├── large_deletes.test.js # A map-sized city (40,000 buildings, past SQLite's bound-value limit) deleted, purged and undone; a failed delete rolling back whole; the history capped, oversized entries marked too large to undo, and a failed history write logged rather than crashing +│ ├── gm_route_auth.test.js # Walks a player's real login token against every route behind the GM check (all refused), and holds what must keep working: GM login, granted editors (grant, use, revoke, surrender), a player's own sheet and portrait, chat, and a player unable to become a socket admin, grant rights or set a bank balance │ ├── cpr_stats.test.js # CP:R stat rolls — BODY rollable, MOVE and LUCK not, and every roll button in the template backed by a server-side roll │ ├── shop_checkout_sockets.test.js # The cart's checkout over the socket: totals in each direction, every way a line fails taking the whole checkout down with it, a changed total, overdraft asked once, and the payer from the socket │ ├── nginx_config.test.js # The assumptions the app makes about the proxy every request arrives through, which no other test here touches — body ceiling at least the largest upload limit, X-Forwarded-For present, the socket able to upgrade, and every mounted path actually proxied. Two faults in one release lived exactly in that gap diff --git a/backend/__tests__/gm_route_auth.test.js b/backend/__tests__/gm_route_auth.test.js new file mode 100644 index 00000000..6acdcbb3 --- /dev/null +++ b/backend/__tests__/gm_route_auth.test.js @@ -0,0 +1,313 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import express from 'express'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import fs from 'fs'; +import path from 'path'; +import { createRequire } from 'module'; +import { makeTestDb, get, run } from './helpers/testDb.js'; +import { drain } from './helpers/until.js'; + +/** + * Who may use the GM's doors. + * + * Every token the server issues is signed with one secret: the GM's login (role 'admin'), a + * player's login (role 'player'), and a granted editor's (isTemporary). A valid signature only + * says the server issued it. The checks used to stop there, so a player's own login token got + * past `authenticate` and every "not temporary" test: it deleted buildings, read GM notes, + * approved accounts, and at socket sign-in it made the player a full socket admin (grant editor + * rights, set anyone's bank balance, speak as anyone in chat). + * + * What must keep working, and is held here too: the GM's login, granted editors (grant, use, + * revoke, surrender), players' own sheet and portrait, and chat. + */ + +process.env.JWT_SECRET = 'test-secret'; +process.env.DICE_ANIM_MS = '0'; +const SECRET = 'test-secret'; + +const require_ = createRequire(import.meta.url); +// The same module instances the routes and sockets hold, so a grant made here is seen there. +const auth = require_('../middleware/auth'); +const { authenticate, authenticatePlayer, optionalAuthenticate, elevatedUsers } = auth; +const socketsFactory = require_('../sockets/index.js'); + +/** Exactly what each login signs. */ +const GM = jwt.sign({ id: 1, username: 'gm', role: 'admin', isTemporary: false }, SECRET); // routes/admin.js +const PLAYER = jwt.sign({ username: 'vex', role: 'player', tempPassword: false }, SECRET, { expiresIn: '7d' }); // routes/player.js +const RESET = jwt.sign({ username: 'vex', role: 'player_reset' }, SECRET, { expiresIn: '15m' }); // routes/player.js +const EDITOR = jwt.sign({ username: 'ghost', isTemporary: true }, SECRET, { expiresIn: '12h' }); // sockets grantElevatedAccess +const FORGED = jwt.sign({ id: 1, username: 'gm', role: 'admin', isTemporary: false }, 'not-the-secret'); +const bearer = (t) => ({ Authorization: `Bearer ${t}` }); + +afterEach(() => { elevatedUsers.clear(); vi.restoreAllMocks(); }); + +describe('the checks themselves', () => { + const app = express(); + app.get('/gm', authenticate, (req, res) => res.json({ user: req.user.username })); + app.get('/player', authenticatePlayer, (req, res) => res.json({ user: req.user.username })); + app.get('/public', optionalAuthenticate, (req, res) => res.json({ user: req.user ? req.user.username : null })); + const hit = (url, token) => (token ? request(app).get(url).set(bearer(token)) : request(app).get(url)); + + it('GM routes let in the GM', async () => { + const res = await hit('/gm', GM); + expect(res.status).toBe(200); + expect(res.body.user).toBe('gm'); + }); + + it('GM routes let in a granted editor while the grant stands, and not after', async () => { + elevatedUsers.add('ghost'); + expect((await hit('/gm', EDITOR)).status).toBe(200); + elevatedUsers.delete('ghost'); + expect((await hit('/gm', EDITOR)).status).toBe(401); + }); + + it('GM routes refuse a player, a reset token, a forged token and no token', async () => { + expect((await hit('/gm', PLAYER)).status).toBe(403); + expect((await hit('/gm', RESET)).status).toBe(403); + expect((await hit('/gm', FORGED)).status).toBe(400); + expect((await hit('/gm')).status).toBe(401); + }); + + it("a player's own routes let in the player, the GM and an editor, but not a reset token", async () => { + elevatedUsers.add('ghost'); + expect((await hit('/player', PLAYER)).body.user).toBe('vex'); + expect((await hit('/player', GM)).status).toBe(200); + expect((await hit('/player', EDITOR)).status).toBe(200); + expect((await hit('/player', RESET)).status).toBe(403); + expect((await hit('/player')).status).toBe(401); + }); + + it('public routes treat a player as anyone else, and only know the GM or an editor', async () => { + elevatedUsers.add('ghost'); + expect((await hit('/public', PLAYER)).body.user).toBeNull(); + expect((await hit('/public', FORGED)).body.user).toBeNull(); + expect((await hit('/public')).body.user).toBeNull(); + expect((await hit('/public', GM)).body.user).toBe('gm'); + expect((await hit('/public', EDITOR)).body.user).toBe('ghost'); + }); + + it('names what each token is', () => { + const d = (t) => jwt.verify(t, SECRET); + expect(auth.isMainAdmin(d(GM))).toBe(true); + for (const t of [PLAYER, RESET, EDITOR]) expect(auth.isMainAdmin(d(t))).toBe(false); + expect(auth.isPlayer(d(PLAYER))).toBe(true); + expect(auth.isPlayer(d(RESET))).toBe(false); + expect(auth.isMainAdmin(null)).toBe(false); + }); +}); + +/** Every router, mounted where server.js mounts it. */ +const MOUNTS = [ + ['/api/locations', '../routes/locations.js', 'full'], + ['/api/locations/:id/battle_maps', '../routes/battle_maps.js', 'full'], + ['/api/locations/:id', '../routes/buildingDetails.js', 'full'], + ['/api/battle_maps', '../routes/battle_maps.js', 'full'], + ['/api/maps', '../routes/maps.js', 'full'], + ['/api/roads', '../routes/roads.js', 'full'], + ['/api/overpasses', '../routes/overpasses.js', 'full'], + ['/api/signs', '../routes/signs.js', 'full'], + ['/api/custom_dice', '../routes/custom_dice.js', 'full'], + ['/api/fonts', '../routes/fonts.js', 'io'], + ['/api/player', '../routes/player.js', 'io'], + ['/api', '../routes/admin.js', 'full'], + ['/api/music', '../routes/music.js', 'io'], + ['/api/sheets', '../routes/sheets.js', 'io'], +]; + +const helpers = { emitUpdate: () => {}, recordAction: () => {} }; +const io = { emit: () => {}, to: () => ({ emit: () => {} }) }; + +const mountAll = (db) => { + const app = express(); + app.use(express.json()); + const routes = []; + for (const [prefix, file, kind] of MOUNTS) { + const factory = require_(file); + const router = kind === 'full' ? factory(db, io, helpers) : factory(db, io); + app.use(prefix, router); + for (const layer of router.stack) { + if (!layer.route) continue; + const handles = layer.route.stack.map((s) => s.handle); + if (!handles.includes(authenticate)) continue; + for (const method of Object.keys(layer.route.methods)) { + routes.push({ method, url: `${prefix}${layer.route.path}`.replace(/:\w+/g, '1') }); + } + } + } + return { app, routes }; +}; + +describe('every GM route, walked with a player token', () => { + it('refuses a player everywhere the GM check stands', async () => { + const db = await makeTestDb(); + await run(db, `INSERT INTO locations (name, x, y, z) VALUES ('CITY HALL', 0, 0, 0)`); + const { app, routes } = mountAll(db); + // The walk proves nothing if it found nothing to walk. + expect(routes.length).toBeGreaterThan(60); + + const let_in = []; + for (const { method, url } of routes) { + const res = await request(app)[method](url).set(bearer(PLAYER)).send({}); + if (res.status !== 403) let_in.push(`${method.toUpperCase()} ${url} -> ${res.status}`); + } + expect(let_in).toEqual([]); + // And nothing was changed on the way. + expect((await get(db, 'SELECT COUNT(*) AS n FROM locations')).n).toBe(1); + }); + + it('still lets the GM through the same routes', async () => { + const db = await makeTestDb(); + const { app } = mountAll(db); + const res = await request(app).get('/api/player/admin/players').set(bearer(GM)); + expect(res.status).not.toBe(401); + expect(res.status).not.toBe(403); + }); +}); + +describe("a player's own routes", () => { + const PNG = Buffer.from('89504e470d0a1a0a0000000d4948445200000001000000010806000000' + + '1f15c4890000000d4944415478da6364f8ffbf1e000501020149a2b8f90000000049454e44ae426082', 'hex'); + let db; + let app; + const written = []; + + beforeEach(async () => { + db = await makeTestDb(); + await run(db, `INSERT INTO global_settings (key, value) VALUES ('game_system', 'generic')`); + await run(db, `INSERT INTO character_sheets (username, system, data, is_npc) VALUES ('vex', 'generic', '{"name":"VEX"}', 0)`); + await run(db, `INSERT INTO character_sheets (username, system, data, is_npc) VALUES ('rook', 'generic', '{"name":"ROOK"}', 0)`); + app = express(); + app.use(express.json()); + app.use('/api/sheets', require_('../routes/sheets.js')(db, io)); + }); + + afterEach(() => { + for (const f of written.splice(0)) { try { fs.unlinkSync(f); } catch { /* already gone */ } } + }); + + it('loads their own sheet', async () => { + const res = await request(app).get('/api/sheets/own').set(bearer(PLAYER)); + expect(res.status).toBe(200); + expect(res.body.name).toBe('VEX'); + }); + + it("uploads their portrait to their own sheet, and cannot aim it at someone else's", async () => { + const res = await request(app).post('/api/sheets/portrait?username=rook').set(bearer(PLAYER)) + .attach('portrait', PNG, 'me.png'); + expect(res.status).toBe(200); + written.push(path.join(path.dirname(require_.resolve('../routes/sheets.js')), '..', res.body.portrait_url)); + expect((await get(db, `SELECT portrait_url FROM character_sheets WHERE username = 'vex'`)).portrait_url).toBe(res.body.portrait_url); + expect((await get(db, `SELECT portrait_url FROM character_sheets WHERE username = 'rook'`)).portrait_url).toBeNull(); + }); + + it('are not a way into the GM sheet routes', async () => { + expect((await request(app).get('/api/sheets/user/rook').set(bearer(PLAYER))).status).toBe(403); + }); +}); + +describe('sockets: sign-in, chat and granting editor rights', () => { + let db; + + /** One socket on the real handlers, sharing the auth module's grant list as server.js does. */ + const boot = () => { + const emitted = []; + let connectionCb; + const ioFake = { + on: (event, cb) => { if (event === 'connection') connectionCb = cb; }, + emit: (event, data) => emitted.push({ event, data }), + to: () => ({ emit: (event, data) => emitted.push({ event, data }) }), + }; + socketsFactory(ioFake, db, { elevatedUsers, emitUpdate: vi.fn(), recordAction: vi.fn() }); + const handlers = {}; + const socket = { + id: `auth-${Math.random().toString(36).slice(2)}`, + on: (event, fn) => { handlers[event] = fn; }, + emit: (event, data) => emitted.push({ event, data, direct: true }), + broadcast: { emit: () => {} }, + use: () => {}, join: () => {}, disconnect: vi.fn(), + }; + connectionCb(socket); + return { handlers, emitted }; + }; + const signIn = async (payload) => { + const s = boot(); + s.handlers.identify(payload); + await drain(db); + return s; + }; + const events = (emitted, name) => emitted.filter((e) => e.event === name); + + beforeEach(async () => { + db = await makeTestDb(); + await run(db, `CREATE TABLE IF NOT EXISTS player_banks ( + username TEXT PRIMARY KEY, balance REAL, debt REAL, + first_pay_done INTEGER DEFAULT 0, high_roller_done INTEGER DEFAULT 0)`); + vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + it('a player chats as themselves', async () => { + const { handlers, emitted } = await signIn('vex'); + handlers.sendMessage({ sender: 'vex', text: 'on my way' }); + await drain(db); + expect(events(emitted, 'receiveMessage').at(-1).data).toMatchObject({ sender: 'vex', text: 'on my way' }); + }); + + it('the GM can speak as someone else in chat', async () => { + const { handlers, emitted } = await signIn({ userName: 'gm', isAdmin: true, token: GM }); + handlers.sendMessage({ sender: 'FIXER', text: 'job is on' }); + await drain(db); + expect(events(emitted, 'receiveMessage').at(-1).data.sender).toBe('FIXER'); + }); + + it("a player who claims to be the GM with their own login is not the GM: chat keeps their name", async () => { + const { handlers, emitted } = await signIn({ userName: 'vex', isAdmin: true, token: PLAYER }); + handlers.sendMessage({ sender: 'FIXER', text: 'free money' }); + await drain(db); + expect(events(emitted, 'receiveMessage').at(-1).data.sender).toBe('vex'); + }); + + it('the GM grants editor rights; the editor then passes the GM check; revoking ends it', async () => { + const { handlers, emitted } = await signIn({ userName: 'gm', isAdmin: true, token: GM }); + handlers.grantElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); + const granted = events(emitted, 'accessGranted').at(-1); + expect(granted.data.targetUser).toBe('ghost'); + expect(elevatedUsers.has('ghost')).toBe(true); + + const app = express(); + app.get('/gm', authenticate, (req, res) => res.json({ ok: true })); + expect((await request(app).get('/gm').set(bearer(granted.data.token))).status).toBe(200); + + handlers.revokeElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); + expect(elevatedUsers.has('ghost')).toBe(false); + expect((await request(app).get('/gm').set(bearer(granted.data.token))).status).toBe(401); + }); + + it('an editor can give their rights back', async () => { + elevatedUsers.add('ghost'); + const { handlers } = await signIn('ghost'); + handlers.surrenderAccess({ token: EDITOR }); + expect(elevatedUsers.has('ghost')).toBe(false); + }); + + it('a player cannot grant editor rights with their own login, to themselves or anyone', async () => { + const { handlers, emitted } = await signIn({ userName: 'vex', isAdmin: true, token: PLAYER }); + handlers.grantElevatedAccess({ adminToken: PLAYER, targetUser: 'vex' }); + expect(elevatedUsers.has('vex')).toBe(false); + expect(events(emitted, 'accessGranted')).toEqual([]); + }); + + it("a player cannot set anyone's bank balance with their own login; the GM still can", async () => { + await run(db, `INSERT INTO player_banks (username, balance, debt) VALUES ('rook', 100, 0)`); + const player = await signIn('vex'); + player.handlers.adminUpdateBank({ token: PLAYER, username: 'rook', balance: 999999, debt: 0 }); + await drain(db); + expect((await get(db, `SELECT balance FROM player_banks WHERE username = 'rook'`)).balance).toBe(100); + + const gm = await signIn({ userName: 'gm', isAdmin: true, token: GM }); + gm.handlers.adminUpdateBank({ token: GM, username: 'rook', balance: 250, debt: 0 }); + await drain(db); + expect((await get(db, `SELECT balance FROM player_banks WHERE username = 'rook'`)).balance).toBe(250); + }); +}); diff --git a/backend/__tests__/shop_catalogue_sockets.test.js b/backend/__tests__/shop_catalogue_sockets.test.js index 4a3cd818..a6b2a262 100644 --- a/backend/__tests__/shop_catalogue_sockets.test.js +++ b/backend/__tests__/shop_catalogue_sockets.test.js @@ -75,7 +75,7 @@ const admin = async (name = 'GM') => { booted.handlers['identify']({ userName: name, isAdmin: true, - token: jwt.sign({ username: name, isTemporary: false }, 'test-secret'), + token: jwt.sign({ id: 1, username: name, role: 'admin', isTemporary: false }, 'test-secret'), }); await drain(db); return booted; diff --git a/backend/__tests__/sockets.deathsave.test.js b/backend/__tests__/sockets.deathsave.test.js index b111ab81..0580b42a 100644 --- a/backend/__tests__/sockets.deathsave.test.js +++ b/backend/__tests__/sockets.deathsave.test.js @@ -198,7 +198,7 @@ describe('generateNpcSheet with tier', () => { `INSERT INTO locations (name, x, y, z, shape, owner, hp_current, hp_max) VALUES ('Guy', 0, 0, 0, 'enemy_rhombus', 'SYSTEM', 5, 5)`); const loc = await get(db, `SELECT id FROM locations WHERE name = 'Guy'`); const { handlers, emitted } = boot(db); - handlers['identify']({ userName: 'admin', isAdmin: true, token: jwt.sign({ username: 'admin', isTemporary: false }, 'test-secret') }); + handlers['identify']({ userName: 'admin', isAdmin: true, token: jwt.sign({ id: 1, username: 'admin', role: 'admin', isTemporary: false }, 'test-secret') }); await flush(50); handlers['generateNpcSheet']({ location_id: loc.id, tier: 'elite' }); diff --git a/backend/__tests__/sockets.identify.secure.test.js b/backend/__tests__/sockets.identify.secure.test.js index ba35667a..31656e43 100644 --- a/backend/__tests__/sockets.identify.secure.test.js +++ b/backend/__tests__/sockets.identify.secure.test.js @@ -32,7 +32,7 @@ const socketsFactory = (await import('../sockets/index.js')).default; */ const flush = () => drain(db); -const ADMIN_TOKEN = jwt.sign({ username: 'admin', isTemporary: false }, SECRET); +const ADMIN_TOKEN = jwt.sign({ id: 1, username: 'admin', role: 'admin', isTemporary: false }, SECRET); // as routes/admin.js signs it const TEMP_ADMIN_TOKEN = jwt.sign({ username: 'helper', isTemporary: true }, SECRET); const PLAYER_TOKEN = jwt.sign({ username: 'realplayer', role: 'player' }, SECRET); const HELPER_PLAYER_TOKEN = jwt.sign({ username: 'helper', role: 'player' }, SECRET); diff --git a/backend/middleware/auth.js b/backend/middleware/auth.js index e708e471..6aeb236b 100644 --- a/backend/middleware/auth.js +++ b/backend/middleware/auth.js @@ -3,34 +3,75 @@ const jwt = require('jsonwebtoken'); const SECRET = process.env.JWT_SECRET; const elevatedUsers = new Set(); +// Who a token belongs to, decided in one place. +// +// Three kinds of token are signed with the same secret: the GM's (routes/admin.js, role +// 'admin'), a player's (routes/player.js, role 'player'), and a granted editor's (sockets, +// isTemporary). A valid signature therefore says only that the server issued the token, not +// that its holder is the GM - and the checks used to stop there. A player's own login token +// passed `authenticate` and every "not temporary" test, which opened the GM's routes and admin +// socket events to any signed-in player. These say what each check actually means. + +/** The GM's own login. Only admin login signs role 'admin'. */ +const isMainAdmin = (v) => !!v && v.role === 'admin' && !v.isTemporary; + +/** A player the GM has granted editing rights, while the grant still stands. */ +const isGrantedEditor = (v) => !!v && !!v.isTemporary && elevatedUsers.has(v.username); + +/** The GM or a granted editor: who the GM-facing routes are for. */ +const canEdit = (v) => isMainAdmin(v) || isGrantedEditor(v); + +/** A player's own login (not a password-reset token). */ +const isPlayer = (v) => !!v && v.role === 'player' && !v.isTemporary; + +/** The verified payload of an `Authorization: Bearer` header, or null. */ +const verifyHeader = (header) => { + try { return jwt.verify(String(header).split(' ')[1], SECRET); } catch { return null; } +}; + +/** GM-facing routes: the GM or a granted editor. */ const authenticate = (req, res, next) => { - const token = req.headers['authorization']; - if (!token) return res.status(401).json({ error: 'Access denied' }); - try { - const verified = jwt.verify(token.split(' ')[1], SECRET); - if (verified.isTemporary && !elevatedUsers.has(verified.username)) { - return res.status(401).json({ error: 'Temporary access revoked' }); - } + const header = req.headers['authorization']; + if (!header) return res.status(401).json({ error: 'Access denied' }); + const verified = verifyHeader(header); + if (!verified) return res.status(400).json({ error: 'Invalid token' }); + if (canEdit(verified)) { req.user = verified; - next(); - } catch (err) { - res.status(400).json({ error: 'Invalid token' }); + return next(); } + if (verified.isTemporary) return res.status(401).json({ error: 'Temporary access revoked' }); + return res.status(403).json({ error: 'GM only' }); }; -const optionalAuthenticate = (req, res, next) => { - const token = req.headers['authorization']; - if (!token) { - req.user = null; +/** + * The few routes a player calls about themselves (their own sheet, their portrait): a + * player's login, or anyone `authenticate` accepts. + */ +const authenticatePlayer = (req, res, next) => { + const header = req.headers['authorization']; + if (!header) return res.status(401).json({ error: 'Access denied' }); + const verified = verifyHeader(header); + if (!verified) return res.status(400).json({ error: 'Invalid token' }); + if (canEdit(verified) || isPlayer(verified)) { + req.user = verified; return next(); } - try { - const verified = jwt.verify(token.split(' ')[1], SECRET); - req.user = (verified.isTemporary && !elevatedUsers.has(verified.username)) ? null : verified; - } catch (err) { - req.user = null; - } + if (verified.isTemporary) return res.status(401).json({ error: 'Temporary access revoked' }); + return res.status(403).json({ error: 'Not allowed' }); +}; + +/** + * Public routes that show the GM more: `req.user` is set only for someone `authenticate` + * would accept. Anyone else, players included, is treated as anonymous. + */ +const optionalAuthenticate = (req, res, next) => { + const header = req.headers['authorization']; + const verified = header ? verifyHeader(header) : null; + req.user = canEdit(verified) ? verified : null; next(); }; -module.exports = { authenticate, optionalAuthenticate, elevatedUsers }; +module.exports = { + authenticate, authenticatePlayer, optionalAuthenticate, elevatedUsers, + isMainAdmin, isGrantedEditor, canEdit, isPlayer, +}; diff --git a/backend/routes/sheets.js b/backend/routes/sheets.js index c777970c..835558ec 100644 --- a/backend/routes/sheets.js +++ b/backend/routes/sheets.js @@ -4,7 +4,7 @@ const fs = require('fs'); const path = require('path'); const crypto = require('crypto'); const multer = require('multer'); -const { authenticate, optionalAuthenticate } = require('../middleware/auth'); +const { authenticate, authenticatePlayer, optionalAuthenticate } = require('../middleware/auth'); const { canReadNpcSheets, redactTokenCard } = require('../sheets/npcPrivacy'); const { TEMPLATES, DEFAULT_SYSTEM, isValidSystem, getLinkedFields, applyDerived, cwnEffectiveAc, @@ -104,7 +104,7 @@ module.exports = (db, io) => { }); // Player's own sheet — used by non-admin players to fetch stats (e.g. SR6 initiative roll). - router.get('/own', authenticate, (req, res) => { + router.get('/own', authenticatePlayer, (req, res) => { getGameSystem((err, system) => { if (err) return res.status(500).json({ error: err.message }); db.get( @@ -621,7 +621,7 @@ module.exports = (db, io) => { // Portrait upload — player uploads their own portrait; admin can upload // for any username via ?username= query param. - router.post('/portrait', authenticate, upload.single('portrait'), (req, res) => { + router.post('/portrait', authenticatePlayer, upload.single('portrait'), (req, res) => { if (!req.file) return res.status(400).json({ error: 'portrait file required' }); const ext = path.extname(req.file.originalname).toLowerCase() || '.jpg'; const allowed = ['.jpg', '.jpeg', '.png', '.webp', '.gif']; diff --git a/backend/sockets/index.js b/backend/sockets/index.js index 029ce456..0559ef63 100644 --- a/backend/sockets/index.js +++ b/backend/sockets/index.js @@ -1,4 +1,5 @@ const jwt = require('jsonwebtoken'); +const { isMainAdmin } = require('../middleware/auth'); const { cryptoRng } = require('../utils/random'); const { registerInitiativeHandlers } = require('./initiative'); const sheetTemplates = require('../sheets/templates'); @@ -218,7 +219,9 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { if (info.isAdmin && info.token) { try { const verified = jwt.verify(info.token, SECRET); - if (verified.isTemporary) info.isAdmin = false; + // The GM's own login only. A player's login token verifies too, and used to pass + // here as "not temporary", which made any player a socket admin. + if (!isMainAdmin(verified)) info.isAdmin = false; } catch (err) { console.warn(`User ${info.userName} claimed admin but provided invalid token.`); info.isAdmin = false; @@ -320,7 +323,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('grantElevatedAccess', (data) => { try { const verified = jwt.verify(data.adminToken, SECRET); - if (verified && !verified.isTemporary) { + if (isMainAdmin(verified)) { elevatedUsers.add(data.targetUser); const tempToken = jwt.sign({ username: data.targetUser, isTemporary: true }, SECRET, { expiresIn: '12h' }); console.log(`Admin ${verified.username} granted temporary access to ${data.targetUser}`); @@ -333,7 +336,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('revokeElevatedAccess', (data) => { try { const verified = jwt.verify(data.adminToken, SECRET); - if (verified && !verified.isTemporary) { + if (isMainAdmin(verified)) { elevatedUsers.delete(data.targetUser); console.log(`Admin ${verified.username} revoked temporary access from ${data.targetUser}`); io.emit('accessRevoked', { targetUser: data.targetUser }); @@ -357,7 +360,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('createNPC', (data) => { try { const verified = jwt.verify(data.adminToken, SECRET); - if (verified && !verified.isTemporary) { + if (isMainAdmin(verified)) { db.run('INSERT INTO fake_users (username, isActive) VALUES (?, 1)', [data.npcName], function(err) { if (!err) { activeNPCs.push({ userName: data.npcName, isActive: true }); @@ -371,7 +374,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('toggleNPCStatus', (data) => { try { const verified = jwt.verify(data.adminToken, SECRET); - if (verified && !verified.isTemporary) { + if (isMainAdmin(verified)) { db.run('UPDATE fake_users SET isActive = ? WHERE username = ?', [data.isActive ? 1 : 0, data.npcName], function(err) { if (!err) { const npc = activeNPCs.find(n => n.userName === data.npcName); @@ -385,7 +388,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('deleteNPC', (data) => { try { const verified = jwt.verify(data.adminToken, SECRET); - if (verified && !verified.isTemporary) { + if (isMainAdmin(verified)) { db.run('DELETE FROM fake_users WHERE username = ?', [data.npcName], function(err) { if (!err) { activeNPCs = activeNPCs.filter(n => n.userName !== data.npcName); @@ -779,7 +782,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('purgeDiceHistory', (data) => { if (!data.token) return; jwt.verify(data.token, SECRET, (err, decoded) => { - if (err || decoded.isTemporary) return; + if (err || !isMainAdmin(decoded)) return; db.run('DELETE FROM dice_rolls', (err) => { if (err) console.error('Error purging dice rolls:', err); io.emit('diceRollHistory', []); @@ -2069,7 +2072,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { if (!data || !data.token || !Array.isArray(data.usernames) || data.totalAmount === undefined) return; jwt.verify(data.token, SECRET, (err, decoded) => { if (err) return; - if (decoded.isTemporary || (decoded.role && decoded.role !== 'admin')) return; + if (!isMainAdmin(decoded)) return; const count = data.usernames.length; if (count === 0) return; const amountPerPlayer = Math.ceil((parseFloat(data.totalAmount) / count) * 100) / 100; @@ -2098,7 +2101,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { if (!data || !data.token || !Array.isArray(data.usernames)) return; jwt.verify(data.token, SECRET, (err, decoded) => { if (err) return; - if (decoded.isTemporary || (decoded.role && decoded.role !== 'admin')) return; + if (!isMainAdmin(decoded)) return; getGameSystem((sysErr, system) => { if (sysErr) return; // Which column the table advances on, so the award can carry the level with it. @@ -2130,7 +2133,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { if (!data || !data.token || !Array.isArray(data.usernames)) return; jwt.verify(data.token, SECRET, (err, decoded) => { if (err) return; - if (decoded.isTemporary || (decoded.role && decoded.role !== 'admin')) return; + if (!isMainAdmin(decoded)) return; getGameSystem((sysErr, system) => { if (sysErr) return; awardXpModule.adjustLevel(db, { system, usernames: data.usernames, delta: data.delta }, (reason, results) => { @@ -2147,7 +2150,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('adminUpdateBank', (data) => { if (!data || !data.token || !data.username) return; jwt.verify(data.token, SECRET, (err, decoded) => { - if (err || decoded.isTemporary) return; + if (err || !isMainAdmin(decoded)) return; const balance = parseFloat(data.balance); const debt = parseFloat(data.debt); if (isNaN(balance) || isNaN(debt)) return; From b193d4f15bfb4572553d9e94d325df3b48cf79f2 Mon Sep 17 00:00:00 2001 From: Developer Date: Tue, 29 Sep 2026 15:25:09 -0500 Subject: [PATCH 2/3] fix: editor grants reach only the player promoted; edit approvals need the GM grantElevatedAccess and approveEditing sent the new editor's token with io.emit, so every connected client received a working key. They now send it only to the target's own connections (the client already ignored grants for anyone else, so the promoted player sees no difference). approveEditing, denyEditing and revokeEditing had no check at all: a player could approve their own edit request. They now require the GM or a granted editor, the same people who see those buttons. Tests run several connections on one server; mutation-checked. Verified on a real secure-mode server: grant, use, revoke, surrender, approve and kick all work, a bystander receives nothing, a player cannot approve themselves. --- CHANGELOG.md | 6 + README.md | 2 +- backend/__tests__/gm_route_auth.test.js | 233 +++++++++++++++++------- backend/sockets/index.js | 26 ++- 4 files changed, 192 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bca77c5..7010578d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). chat, shops and bank all work as before, and granting, revoking and giving back editor rights work as before. +- **Editor rights go only to the player receiving them.** Granting editor rights, or approving an + edit request, used to send the new editor's key to every connected player, and any of them + could copy it. It now reaches only the player being promoted. Approving, denying and ending an + edit request were also open to anyone: a player could approve their own request. Those buttons + now work only for the GM and granted editors, as they appear in the admin panel. + ### Under the hood - **The first piece of the system builder.** An engine that works out a sheet's derived diff --git a/README.md b/README.md index ddfd8246..a3e77efd 100644 --- a/README.md +++ b/README.md @@ -415,7 +415,7 @@ CITY_NET/ │ │ └── testDb.js # In-memory SQLite factory for isolated test DBs │ ├── admin.test.js # Admin endpoints (auth, settings, undo access); update routes — 409 with a reason rather than a false success, unauthenticated status, boot id on /version; check-update against a stubbed registry — upgrades only, dev tags per channel, and a prerelease not hiding a stable release │ ├── large_deletes.test.js # A map-sized city (40,000 buildings, past SQLite's bound-value limit) deleted, purged and undone; a failed delete rolling back whole; the history capped, oversized entries marked too large to undo, and a failed history write logged rather than crashing -│ ├── gm_route_auth.test.js # Walks a player's real login token against every route behind the GM check (all refused), and holds what must keep working: GM login, granted editors (grant, use, revoke, surrender), a player's own sheet and portrait, chat, and a player unable to become a socket admin, grant rights or set a bank balance +│ ├── gm_route_auth.test.js # Walks a player's real login token against every route behind the GM check (all refused), and holds what must keep working: GM login, granted editors (grant, use, revoke, surrender), a player's own sheet and portrait, chat, and a player unable to become a socket admin, grant rights, approve their own edit request or set a bank balance; editor grants reach only the player promoted │ ├── cpr_stats.test.js # CP:R stat rolls — BODY rollable, MOVE and LUCK not, and every roll button in the template backed by a server-side roll │ ├── shop_checkout_sockets.test.js # The cart's checkout over the socket: totals in each direction, every way a line fails taking the whole checkout down with it, a changed total, overdraft asked once, and the payer from the socket │ ├── nginx_config.test.js # The assumptions the app makes about the proxy every request arrives through, which no other test here touches — body ceiling at least the largest upload limit, X-Forwarded-For present, the socket able to upgrade, and every mounted path actually proxied. Two faults in one release lived exactly in that gap diff --git a/backend/__tests__/gm_route_auth.test.js b/backend/__tests__/gm_route_auth.test.js index 6acdcbb3..80e7858e 100644 --- a/backend/__tests__/gm_route_auth.test.js +++ b/backend/__tests__/gm_route_auth.test.js @@ -206,37 +206,40 @@ describe("a player's own routes", () => { }); }); -describe('sockets: sign-in, chat and granting editor rights', () => { +describe('sockets: sign-in, chat and editor rights', () => { let db; - /** One socket on the real handlers, sharing the auth module's grant list as server.js does. */ - const boot = () => { - const emitted = []; + /** + * One server on the real handlers, several connections to it, sharing the auth module's + * grant list as server.js does. `sent` records every emit with where it went: 'all' for a + * broadcast, the socket id for io.to(id), 'self' for a reply down one socket. + */ + const server = () => { + const sent = []; let connectionCb; const ioFake = { on: (event, cb) => { if (event === 'connection') connectionCb = cb; }, - emit: (event, data) => emitted.push({ event, data }), - to: () => ({ emit: (event, data) => emitted.push({ event, data }) }), + emit: (event, data) => sent.push({ event, data, to: 'all' }), + to: (id) => ({ emit: (event, data) => sent.push({ event, data, to: id }) }), }; socketsFactory(ioFake, db, { elevatedUsers, emitUpdate: vi.fn(), recordAction: vi.fn() }); - const handlers = {}; - const socket = { - id: `auth-${Math.random().toString(36).slice(2)}`, - on: (event, fn) => { handlers[event] = fn; }, - emit: (event, data) => emitted.push({ event, data, direct: true }), - broadcast: { emit: () => {} }, - use: () => {}, join: () => {}, disconnect: vi.fn(), + const connect = async (identify) => { + const handlers = {}; + const socket = { + id: `auth-${Math.random().toString(36).slice(2)}`, + on: (event, fn) => { handlers[event] = fn; }, + emit: (event, data) => sent.push({ event, data, to: 'self' }), + broadcast: { emit: () => {} }, + use: () => {}, join: () => {}, disconnect: vi.fn(), + }; + connectionCb(socket); + handlers.identify(identify); + await drain(db); + return { id: socket.id, handlers }; }; - connectionCb(socket); - return { handlers, emitted }; + return { sent, connect }; }; - const signIn = async (payload) => { - const s = boot(); - s.handlers.identify(payload); - await drain(db); - return s; - }; - const events = (emitted, name) => emitted.filter((e) => e.event === name); + const events = (sent, name) => sent.filter((e) => e.event === name); beforeEach(async () => { db = await makeTestDb(); @@ -247,65 +250,155 @@ describe('sockets: sign-in, chat and granting editor rights', () => { vi.spyOn(console, 'warn').mockImplementation(() => {}); }); - it('a player chats as themselves', async () => { - const { handlers, emitted } = await signIn('vex'); - handlers.sendMessage({ sender: 'vex', text: 'on my way' }); - await drain(db); - expect(events(emitted, 'receiveMessage').at(-1).data).toMatchObject({ sender: 'vex', text: 'on my way' }); - }); - - it('the GM can speak as someone else in chat', async () => { - const { handlers, emitted } = await signIn({ userName: 'gm', isAdmin: true, token: GM }); - handlers.sendMessage({ sender: 'FIXER', text: 'job is on' }); - await drain(db); - expect(events(emitted, 'receiveMessage').at(-1).data.sender).toBe('FIXER'); - }); - - it("a player who claims to be the GM with their own login is not the GM: chat keeps their name", async () => { - const { handlers, emitted } = await signIn({ userName: 'vex', isAdmin: true, token: PLAYER }); - handlers.sendMessage({ sender: 'FIXER', text: 'free money' }); - await drain(db); - expect(events(emitted, 'receiveMessage').at(-1).data.sender).toBe('vex'); + describe('chat', () => { + it('a player chats as themselves', async () => { + const s = server(); + const vex = await s.connect('vex'); + vex.handlers.sendMessage({ sender: 'vex', text: 'on my way' }); + await drain(db); + expect(events(s.sent, 'receiveMessage').at(-1).data).toMatchObject({ sender: 'vex', text: 'on my way' }); + }); + + it('the GM can speak as someone else', async () => { + const s = server(); + const gm = await s.connect({ userName: 'gm', isAdmin: true, token: GM }); + gm.handlers.sendMessage({ sender: 'FIXER', text: 'job is on' }); + await drain(db); + expect(events(s.sent, 'receiveMessage').at(-1).data.sender).toBe('FIXER'); + }); + + it('a player claiming to be the GM with their own login still chats as themselves', async () => { + const s = server(); + const vex = await s.connect({ userName: 'vex', isAdmin: true, token: PLAYER }); + vex.handlers.sendMessage({ sender: 'FIXER', text: 'free money' }); + await drain(db); + expect(events(s.sent, 'receiveMessage').at(-1).data.sender).toBe('vex'); + }); }); - it('the GM grants editor rights; the editor then passes the GM check; revoking ends it', async () => { - const { handlers, emitted } = await signIn({ userName: 'gm', isAdmin: true, token: GM }); - handlers.grantElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); - const granted = events(emitted, 'accessGranted').at(-1); - expect(granted.data.targetUser).toBe('ghost'); - expect(elevatedUsers.has('ghost')).toBe(true); - - const app = express(); - app.get('/gm', authenticate, (req, res) => res.json({ ok: true })); - expect((await request(app).get('/gm').set(bearer(granted.data.token))).status).toBe(200); - - handlers.revokeElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); - expect(elevatedUsers.has('ghost')).toBe(false); - expect((await request(app).get('/gm').set(bearer(granted.data.token))).status).toBe(401); - }); - - it('an editor can give their rights back', async () => { - elevatedUsers.add('ghost'); - const { handlers } = await signIn('ghost'); - handlers.surrenderAccess({ token: EDITOR }); - expect(elevatedUsers.has('ghost')).toBe(false); + describe('temporary admin, granted by the GM', () => { + it('reaches the player it is for, on every connection they have, and nobody else', async () => { + const s = server(); + const gm = await s.connect({ userName: 'gm', isAdmin: true, token: GM }); + const ghostGame = await s.connect('ghost'); + const ghostSheetTab = await s.connect('ghost'); + const bystander = await s.connect('rook'); + + gm.handlers.grantElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); + // The socket module keeps its list of connections for the life of the process, so + // earlier tests' connections are still in it: assert on this test's own. + const grants = events(s.sent, 'accessGranted'); + const to = grants.map((g) => g.to); + expect(to).toContain(ghostGame.id); + expect(to).toContain(ghostSheetTab.id); + expect(to).not.toContain(bystander.id); + expect(to).not.toContain(gm.id); + expect(to).not.toContain('all'); + expect(grants.every((g) => g.data.targetUser === 'ghost' && g.data.token)).toBe(true); + expect(elevatedUsers.has('ghost')).toBe(true); + }); + + it('works as a key to the GM routes until it is revoked', async () => { + const s = server(); + const gm = await s.connect({ userName: 'gm', isAdmin: true, token: GM }); + await s.connect('ghost'); + gm.handlers.grantElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); + const token = events(s.sent, 'accessGranted').at(-1).data.token; + + const app = express(); + app.get('/gm', authenticate, (req, res) => res.json({ ok: true })); + expect((await request(app).get('/gm').set(bearer(token))).status).toBe(200); + + gm.handlers.revokeElevatedAccess({ adminToken: GM, targetUser: 'ghost' }); + expect(elevatedUsers.has('ghost')).toBe(false); + // Revoking still tells everyone: it carries no token, and the client only acts on its own. + expect(events(s.sent, 'accessRevoked').at(-1)).toMatchObject({ to: 'all', data: { targetUser: 'ghost' } }); + expect((await request(app).get('/gm').set(bearer(token))).status).toBe(401); + }); + + it('can be given back by the editor', async () => { + elevatedUsers.add('ghost'); + const s = server(); + const ghost = await s.connect('ghost'); + ghost.handlers.surrenderAccess({ token: EDITOR }); + expect(elevatedUsers.has('ghost')).toBe(false); + }); + + it('cannot be granted by a player with their own login, to themselves or anyone', async () => { + const s = server(); + const vex = await s.connect({ userName: 'vex', isAdmin: true, token: PLAYER }); + vex.handlers.grantElevatedAccess({ adminToken: PLAYER, targetUser: 'vex' }); + expect(elevatedUsers.has('vex')).toBe(false); + expect(events(s.sent, 'accessGranted')).toEqual([]); + }); }); - it('a player cannot grant editor rights with their own login, to themselves or anyone', async () => { - const { handlers, emitted } = await signIn({ userName: 'vex', isAdmin: true, token: PLAYER }); - handlers.grantElevatedAccess({ adminToken: PLAYER, targetUser: 'vex' }); - expect(elevatedUsers.has('vex')).toBe(false); - expect(events(emitted, 'accessGranted')).toEqual([]); + describe('editing requests (REQUEST EDIT on a building)', () => { + it('the GM approves: the player becomes an editor, and only they get the token', async () => { + const s = server(); + const gm = await s.connect({ userName: 'gm', isAdmin: true, token: GM }); + const vex = await s.connect('vex'); + const rook = await s.connect('rook'); + vex.handlers.requestEditing({ userId: 'vex', userName: 'vex', locationId: 1, locationName: 'BAR' }); + expect(events(s.sent, 'editingRequested')).toHaveLength(1); + + gm.handlers.approveEditing({ userId: 'vex', location: { id: 1 } }); + expect(elevatedUsers.has('vex')).toBe(true); + const grants = events(s.sent, 'accessGranted'); + const to = grants.map((g) => g.to); + expect(to).toContain(vex.id); + expect(to).not.toContain(rook.id); + expect(to).not.toContain(gm.id); + expect(to).not.toContain('all'); + expect(grants.every((g) => g.data.targetUser === 'vex' && g.data.forEditing === true)).toBe(true); + expect(events(s.sent, 'editingApproved')).toHaveLength(1); + }); + + it('a granted editor can still approve, as before', async () => { + elevatedUsers.add('ghost'); + const s = server(); + const ghost = await s.connect('ghost'); + await s.connect('vex'); + ghost.handlers.approveEditing({ userId: 'vex' }); + expect(elevatedUsers.has('vex')).toBe(true); + }); + + it('a player cannot approve their own request', async () => { + const s = server(); + const vex = await s.connect('vex'); + vex.handlers.approveEditing({ userId: 'vex' }); + expect(elevatedUsers.has('vex')).toBe(false); + expect(events(s.sent, 'accessGranted')).toEqual([]); + }); + + it('the GM can deny a request and kick an editor; a player can do neither', async () => { + elevatedUsers.add('ghost'); + const s = server(); + const gm = await s.connect({ userName: 'gm', isAdmin: true, token: GM }); + const vex = await s.connect('vex'); + + vex.handlers.revokeEditing({ userId: 'ghost' }); + vex.handlers.denyEditing({ userId: 'ghost' }); + expect(elevatedUsers.has('ghost')).toBe(true); + expect(events(s.sent, 'editingRevoked')).toEqual([]); + expect(events(s.sent, 'editingDenied')).toEqual([]); + + gm.handlers.denyEditing({ userId: 'vex' }); + gm.handlers.revokeEditing({ userId: 'ghost' }); + expect(events(s.sent, 'editingDenied')).toHaveLength(1); + expect(elevatedUsers.has('ghost')).toBe(false); + }); }); it("a player cannot set anyone's bank balance with their own login; the GM still can", async () => { await run(db, `INSERT INTO player_banks (username, balance, debt) VALUES ('rook', 100, 0)`); - const player = await signIn('vex'); - player.handlers.adminUpdateBank({ token: PLAYER, username: 'rook', balance: 999999, debt: 0 }); + const s = server(); + const vex = await s.connect('vex'); + vex.handlers.adminUpdateBank({ token: PLAYER, username: 'rook', balance: 999999, debt: 0 }); await drain(db); expect((await get(db, `SELECT balance FROM player_banks WHERE username = 'rook'`)).balance).toBe(100); - const gm = await signIn({ userName: 'gm', isAdmin: true, token: GM }); + const gm = await s.connect({ userName: 'gm', isAdmin: true, token: GM }); gm.handlers.adminUpdateBank({ token: GM, username: 'rook', balance: 250, debt: 0 }); await drain(db); expect((await get(db, `SELECT balance FROM player_banks WHERE username = 'rook'`)).balance).toBe(250); diff --git a/backend/sockets/index.js b/backend/sockets/index.js index 0559ef63..27e20f0a 100644 --- a/backend/sockets/index.js +++ b/backend/sockets/index.js @@ -129,6 +129,20 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { return !!info && (info.isAdmin || elevatedUsers.has(info.userName)); }; + /** + * Send to every connection signed in as `username`, and to nobody else. + * + * For what only that person may see. A grant of editor rights carries a working token, and + * it used to go out with io.emit: every connected client received it, and any of them could + * copy it and act as the editor. The client only ever used the one addressed to itself, so + * sending it there alone changes nothing for the person being granted. + */ + const emitToUser = (username, event, data) => { + userSockets.forEach((info, id) => { + if (info && info.userName === username) io.to(id).emit(event, data); + }); + }; + const buildActiveUsers = () => { const userMap = new Map(); userSockets.forEach((info) => { @@ -327,7 +341,7 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { elevatedUsers.add(data.targetUser); const tempToken = jwt.sign({ username: data.targetUser, isTemporary: true }, SECRET, { expiresIn: '12h' }); console.log(`Admin ${verified.username} granted temporary access to ${data.targetUser}`); - io.emit('accessGranted', { targetUser: data.targetUser, token: tempToken }); + emitToUser(data.targetUser, 'accessGranted', { targetUser: data.targetUser, token: tempToken }); broadcastActiveUsers(); } } catch (err) { console.warn('Unauthorized attempt to grant access:', err.message); } @@ -444,16 +458,20 @@ module.exports = (io, db, { elevatedUsers, emitUpdate, recordAction }) => { socket.on('requestEditing', (data) => { io.emit('editingRequested', data); }); + // Approving, denying and ending an edit are the GM's (or a granted editor's) buttons in the + // admin panel. They had no check, so any player could send approveEditing for themselves and + // become an editor; the connection's verified sign-in now decides. socket.on('approveEditing', (data) => { + if (!isAdminSocket(socket) || !data || !data.userId) return; elevatedUsers.add(data.userId); const tempToken = jwt.sign({ username: data.userId, isTemporary: true }, SECRET, { expiresIn: '12h' }); - io.emit('accessGranted', { targetUser: data.userId, token: tempToken, forEditing: true }); + emitToUser(data.userId, 'accessGranted', { targetUser: data.userId, token: tempToken, forEditing: true }); io.emit('editingStarted', data); io.emit('editingApproved', data); }); - socket.on('denyEditing', (data) => { io.emit('editingDenied', data); }); - socket.on('revokeEditing', (data) => { elevatedUsers.delete(data.userId); io.emit('editingStopped'); io.emit('editingRevoked', data); broadcastActiveUsers(); }); + socket.on('denyEditing', (data) => { if (!isAdminSocket(socket)) return; io.emit('editingDenied', data); }); + socket.on('revokeEditing', (data) => { if (!isAdminSocket(socket) || !data) return; elevatedUsers.delete(data.userId); io.emit('editingStopped'); io.emit('editingRevoked', data); broadcastActiveUsers(); }); socket.on('editingFinished', (data) => { if (data?.userId) elevatedUsers.delete(data.userId); io.emit('editingStopped'); }); socket.on('requestRhombusPurge', (data) => { From 6991264877f23f2cca362b68c1a10758e6990abd Mon Sep 17 00:00:00 2001 From: Developer Date: Tue, 29 Sep 2026 15:32:12 -0500 Subject: [PATCH 3/3] ci: run the test suites on every pull request, whatever its base The trigger listed only main and dev, so PRs into a feature branch (feature/system-builder and its sb/* pieces) were never tested until the final merge to main. --- .github/workflows/ci.yml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 23124034..b8dd9cb2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,12 +1,13 @@ name: CI Tests on: - # Runs tests whenever someone opens or updates a Pull Request targeting 'main' or - # 'dev' — the integration branch that publishes development images. + # Runs tests whenever someone opens or updates a Pull Request, whatever branch it targets. + # + # This used to be limited to 'main' and 'dev', so a long-running feature branch that takes + # its pieces as PRs of its own (feature/system-builder, with sb/* pieces merged into it) + # was never tested until the final merge to main. Every PR is a change about to land + # somewhere, so every PR gets the suites. pull_request: - branches: - - main - - dev # Runs tests when code is merged or pushed directly to 'main'. #