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'. # diff --git a/CHANGELOG.md b/CHANGELOG.md index d169885e..7010578d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ 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. + +- **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 70e66948..a3e77efd 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, 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 new file mode 100644 index 00000000..80e7858e --- /dev/null +++ b/backend/__tests__/gm_route_auth.test.js @@ -0,0 +1,406 @@ +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 editor rights', () => { + let db; + + /** + * 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) => 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 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 }; + }; + return { sent, connect }; + }; + const events = (sent, name) => sent.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(() => {}); + }); + + 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'); + }); + }); + + 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([]); + }); + }); + + 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 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 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/__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..27e20f0a 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'); @@ -128,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) => { @@ -218,7 +233,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,11 +337,11 @@ 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}`); - 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); } @@ -333,7 +350,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 +374,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 +388,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 +402,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); @@ -441,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) => { @@ -779,7 +800,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 +2090,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 +2119,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 +2151,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 +2168,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;