diff --git a/backend/__tests__/middleware/apiTokenRoleFallback.test.js b/backend/__tests__/middleware/apiTokenRoleFallback.test.js new file mode 100644 index 00000000..8a7fc910 --- /dev/null +++ b/backend/__tests__/middleware/apiTokenRoleFallback.test.js @@ -0,0 +1,32 @@ +/** + * The roles-join fallback in apiTokenAuth grants `super_admin` (upgrade-path + * parity with adminAuth). It must therefore fire ONLY when the roles schema is + * genuinely absent — a catch-all turns any transient database failure into a + * privilege escalation that reopens GHSA-9697 for a demoted token owner. + */ + +const { isMissingRolesSchema } = require('../../src/middleware/apiTokenAuth'); + +describe('apiTokenAuth roles-schema fallback predicate (GHSA-9697)', () => { + it('accepts a genuinely missing roles table on both engines', () => { + expect(isMissingRolesSchema(new Error('SQLITE_ERROR: no such table: roles'))).toBe(true); + expect(isMissingRolesSchema( + Object.assign(new Error('relation "roles" does not exist'), { code: '42P01' }), + )).toBe(true); + expect(isMissingRolesSchema( + Object.assign(new Error('column roles.name does not exist'), { code: '42703' }), + )).toBe(true); + }); + + it('rejects transient failures that must not elevate the caller', () => { + expect(isMissingRolesSchema(new Error('Connection terminated unexpectedly'))).toBe(false); + expect(isMissingRolesSchema(new Error('deadlock detected'))).toBe(false); + expect(isMissingRolesSchema(new Error('Knex: Timeout acquiring a connection'))).toBe(false); + expect(isMissingRolesSchema(new Error('canceling statement due to statement timeout'))).toBe(false); + expect(isMissingRolesSchema(undefined)).toBe(false); + }); + + it('rejects a missing-table error for an unrelated table', () => { + expect(isMissingRolesSchema(new Error('SQLITE_ERROR: no such table: api_tokens'))).toBe(false); + }); +}); diff --git a/backend/__tests__/routes/v1EventOwnership.test.js b/backend/__tests__/routes/v1EventOwnership.test.js new file mode 100644 index 00000000..e54932e3 --- /dev/null +++ b/backend/__tests__/routes/v1EventOwnership.test.js @@ -0,0 +1,155 @@ +/** + * v1 API tokens must respect event ownership (GHSA-9697). + * + * migration 081 documents the intent — "the token's effective permissions are + * the intersection of the user's role permissions and the token's own scope + * flags" — but it was never implemented: + * + * - apiTokenAuth selected only id/username/email/role_id, so + * req.admin.roleName was undefined and every ownership helper (which all + * key on roleName) could not distinguish a super_admin from a viewer. + * - No v1 route applied requirePermission or a created_by predicate, so any + * valid token listed every event and — worst — GET /events/:id/share-link + * returned ANY event's share_token, which is the gallery access credential. + * + * Scenario pinned here: a token owned by a restricted (non-super_admin) admin + * must see only its owner's events, and must not obtain a foreign share_token. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-v1own-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'v1own-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-v1own-storage-')); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); +const { generateApiToken } = require('../../src/middleware/apiTokenAuth'); + +describe('v1 event ownership (GHSA-9697)', () => { + let db; let cleanup; let app; + let editorToken; let superToken; + let ownEventId; let foreignEventId; + const FOREIGN_SHARE_TOKEN = 'f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0'; + + const mkAdmin = async (username, roleName) => { + const role = await db('roles').where({ name: roleName }).first(); + const r = await db('admin_users').insert({ + username, + email: `${username}@example.com`, + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + const mkToken = async (adminId, scopes = 'admin') => { + const { plaintext, hashed } = generateApiToken(); + await db('api_tokens').insert({ + name: `tok-${adminId}`, + hashed_token: hashed, + scopes, + created_by: adminId, + created_at: new Date().toISOString(), + }); + return plaintext; + }; + + const mkEvent = async (slug, createdBy, shareToken) => { + const r = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: slug, + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_token: shareToken, + share_link: `/gallery/${slug}/${shareToken}`, + created_by: createdBy, + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const editorId = await mkAdmin('restricted-editor', 'editor'); + const superId = await mkAdmin('root-admin', 'super_admin'); + editorToken = await mkToken(editorId); + superToken = await mkToken(superId); + + ownEventId = await mkEvent('own-event', editorId, 'a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1'); + foreignEventId = await mkEvent('foreign-event', superId, FOREIGN_SHARE_TOKEN); + + app = express(); + app.use(express.json()); + app.use('/api/v1', require('../../src/routes/v1/events')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('lists only the token owner\'s events', async () => { + const res = await request(app) + .get('/api/v1/events') + .set('Authorization', `Bearer ${editorToken}`); + + expect(res.status).toBe(200); + const slugs = res.body.events.map((e) => e.slug); + expect(slugs).toContain('own-event'); + expect(slugs).not.toContain('foreign-event'); + }); + + it('refuses to read a foreign event', async () => { + const res = await request(app) + .get(`/api/v1/events/${foreignEventId}`) + .set('Authorization', `Bearer ${editorToken}`); + + expect([403, 404]).toContain(res.status); + }); + + it('does NOT hand out a foreign event\'s share_token', async () => { + const res = await request(app) + .get(`/api/v1/events/${foreignEventId}/share-link`) + .set('Authorization', `Bearer ${editorToken}`); + + expect([403, 404]).toContain(res.status); + expect(JSON.stringify(res.body)).not.toContain(FOREIGN_SHARE_TOKEN); + }); + + it('still allows the owner to read their own event and share link', async () => { + const detail = await request(app) + .get(`/api/v1/events/${ownEventId}`) + .set('Authorization', `Bearer ${editorToken}`); + expect(detail.status).toBe(200); + + const share = await request(app) + .get(`/api/v1/events/${ownEventId}/share-link`) + .set('Authorization', `Bearer ${editorToken}`); + expect(share.status).toBe(200); + expect(share.body.share_token).toBe('a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1'); + }); + + it('leaves super_admin tokens unrestricted', async () => { + const res = await request(app) + .get(`/api/v1/events/${foreignEventId}/share-link`) + .set('Authorization', `Bearer ${superToken}`); + expect(res.status).toBe(200); + expect(res.body.share_token).toBe(FOREIGN_SHARE_TOKEN); + }); +}); diff --git a/backend/__tests__/routes/v1TokenPermissions.test.js b/backend/__tests__/routes/v1TokenPermissions.test.js new file mode 100644 index 00000000..92188788 --- /dev/null +++ b/backend/__tests__/routes/v1TokenPermissions.test.js @@ -0,0 +1,108 @@ +/** + * v1 token scopes must intersect the owner's CURRENT role permissions + * (GHSA-9697, codex round 2). + * + * Migration 081 documents effective permissions as the intersection of the + * owner's role permissions and the token's scope flags. requireApiScope only + * ever checked the scope half, so a token minted while its owner was + * super_admin kept full write access after the owner was demoted to viewer — + * userManagementService never touches api_tokens, so the token outlives the + * demotion. Ownership scoping alone does not close this: the demoted owner + * still *owns* their events. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-v1perm-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'v1perm-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-v1perm-storage-')); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); +const { generateApiToken } = require('../../src/middleware/apiTokenAuth'); + +describe('v1 token scopes intersect role permissions (GHSA-9697)', () => { + let db; let cleanup; let app; let viewerToken; let viewerEventId; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const role = await db('roles').where({ name: 'viewer' }).first(); + const r = await db('admin_users').insert({ + username: 'demoted-owner', + email: 'demoted@example.com', + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + const ownerId = r[0]?.id ?? r[0]; + + // A token still carrying the broad 'admin' scope from before demotion. + const { plaintext, hashed } = generateApiToken(); + await db('api_tokens').insert({ + name: 'stale-token', + hashed_token: hashed, + scopes: 'admin', + created_by: ownerId, + created_at: new Date().toISOString(), + }); + viewerToken = plaintext; + + const ev = await db('events').insert({ + slug: 'viewer-ev', + event_type: 'wedding', + event_name: 'Viewer Event', + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_token: 'vtok', + share_link: '/gallery/viewer-ev/vtok', + created_by: ownerId, + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + viewerEventId = ev[0]?.id ?? ev[0]; + + app = express(); + app.use(express.json()); + app.use('/api/v1', require('../../src/routes/v1/events')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('denies event creation to a demoted viewer despite an admin-scope token', async () => { + const res = await request(app) + .post('/api/v1/events') + .set('Authorization', `Bearer ${viewerToken}`) + .send({ event_name: 'Nope', event_type: 'wedding' }); + expect(res.status).toBe(403); + }); + + it('denies photo upload to a demoted viewer on their OWN event', async () => { + const res = await request(app) + .post(`/api/v1/events/${viewerEventId}/photos`) + .set('Authorization', `Bearer ${viewerToken}`) + .attach('photo', Buffer.from('x'), 'a.jpg'); + expect(res.status).toBe(403); + }); + + it('still allows the viewer to READ their own event', async () => { + const res = await request(app) + .get(`/api/v1/events/${viewerEventId}`) + .set('Authorization', `Bearer ${viewerToken}`); + expect(res.status).toBe(200); + }); +}); diff --git a/backend/src/middleware/apiTokenAuth.js b/backend/src/middleware/apiTokenAuth.js index c3d826af..da5ec767 100644 --- a/backend/src/middleware/apiTokenAuth.js +++ b/backend/src/middleware/apiTokenAuth.js @@ -1,5 +1,6 @@ const crypto = require('crypto'); const { db } = require('../database/db'); +const { formatBoolean } = require('../utils/dbCompat'); const logger = require('../utils/logger'); const TOKEN_PREFIX = 'pp_live_'; @@ -31,6 +32,24 @@ function parseScopes(raw) { .filter((s) => VALID_SCOPES.includes(s)); } +/** + * Does this error mean the `roles` table/column genuinely isn't there yet + * (mid-upgrade), as opposed to the database being briefly unhappy? + * + * The distinction matters because the fallback below grants super_admin: a + * catch-all would turn any transient failure — connection reset, deadlock, + * statement timeout — into a privilege escalation that hands a demoted viewer + * exactly the access GHSA-9697 closes. + */ +function isMissingRolesSchema(err) { + const message = String(err?.message || ''); + if (!/roles/i.test(message)) return false; + // PG: 42P01 undefined_table / 42703 undefined_column. SQLite carries no + // codes, so match its wording too. + return err?.code === '42P01' || err?.code === '42703' + || /no such table|no such column|does not exist|unknown column/i.test(message); +} + /** * Middleware: authenticate via API token. Maps the token to its owner * admin user, attaches { req.admin, req.apiToken }, then defers to the @@ -63,10 +82,36 @@ async function apiTokenAuth(req, res, next) { return res.status(401).json({ error: 'Token expired', code: 'TOKEN_EXPIRED' }); } - const admin = await db('admin_users') - .where({ id: row.created_by, is_active: true }) - .select('id', 'username', 'email', 'role_id') - .first(); + // Load the owner WITH their role name (GHSA-9697). Without it, + // req.admin.roleName was undefined — and every ownership check keys on + // roleName — so the v1 surface could not tell a super_admin from a + // demoted viewer. Mirrors adminAuth's shape, including the + // roles-table-missing fallback used during upgrades. + let admin; + try { + admin = await db('admin_users') + .leftJoin('roles', 'roles.id', 'admin_users.role_id') + .where({ 'admin_users.id': row.created_by, 'admin_users.is_active': formatBoolean(true) }) + .select( + 'admin_users.id', + 'admin_users.username', + 'admin_users.email', + 'roles.id as role_id', + 'roles.name as role_name' + ) + .first(); + } catch (joinError) { + // Fail CLOSED on anything that isn't a genuinely missing roles schema: + // the fallback fabricates super_admin, so a transient query failure must + // not become a free privilege upgrade. Rethrow → outer catch → 500. + if (!isMissingRolesSchema(joinError)) throw joinError; + logger.debug('Roles table not available in apiTokenAuth', { error: joinError.message }); + admin = await db('admin_users') + .where({ id: row.created_by, is_active: formatBoolean(true) }) + .select('id', 'username', 'email', 'role_id') + .first(); + if (admin) admin.role_name = 'super_admin'; // upgrade-path parity with adminAuth + } if (!admin) { return res.status(401).json({ error: 'Token owner unavailable', code: 'OWNER_INACTIVE' }); } @@ -75,7 +120,15 @@ async function apiTokenAuth(req, res, next) { db('api_tokens').where({ id: row.id }).update({ last_used_at: new Date() }) .catch((err) => logger.debug('api_tokens last_used update failed', { err: err.message })); - req.admin = admin; + // Same shape adminAuth produces, so requirePermission / ownership helpers + // behave identically whether the caller used a session or an API token. + req.admin = { + id: admin.id, + username: admin.username, + email: admin.email, + roleId: admin.role_id, + roleName: admin.role_name + }; req.apiToken = { id: row.id, name: row.name, @@ -118,6 +171,7 @@ module.exports = { generateApiToken, hashToken, parseScopes, + isMissingRolesSchema, TOKEN_PREFIX, VALID_SCOPES }; diff --git a/backend/src/middleware/ownership.js b/backend/src/middleware/ownership.js index 322c0aad..713d2b19 100644 --- a/backend/src/middleware/ownership.js +++ b/backend/src/middleware/ownership.js @@ -32,6 +32,20 @@ function requireEventOwnership(req, res, next) { }); } +/** + * Apply the ownership predicate to a knex query over `events`, for list + * endpoints that can't use requireEventOwnership (no :id to check). + * super_admin is unrestricted; everyone else sees ownerless (legacy/system) + * events plus their own — the same rule requireEventOwnership enforces + * per-row. + */ +function scopeEventsQuery(query, admin, column = 'created_by') { + if (admin?.roleName === 'super_admin') { + return query; + } + return query.where((q) => q.whereNull(column).orWhere(column, admin.id)); +} + /** * Return the subset of `eventIds` the admin may act on, mirroring * requireEventOwnership for bulk routes that can't use it (they take an @@ -64,4 +78,4 @@ async function filterOwnedEventIds(admin, eventIds) { return { allowed, denied }; } -module.exports = { requireEventOwnership, filterOwnedEventIds }; +module.exports = { requireEventOwnership, filterOwnedEventIds, scopeEventsQuery }; diff --git a/backend/src/routes/v1/__tests__/events.category.test.js b/backend/src/routes/v1/__tests__/events.category.test.js index 872ba5fd..f3655dc7 100644 --- a/backend/src/routes/v1/__tests__/events.category.test.js +++ b/backend/src/routes/v1/__tests__/events.category.test.js @@ -43,10 +43,24 @@ jest.mock('../../../database/db', () => { }; }); +// RBAC is enforced on these routes since GHSA-9697 (requirePermission), but +// this suite mocks the database, so a real permission lookup would 500. These +// tests cover route logic, not authorization — the intersection of token +// scopes and role permissions is pinned in __tests__/routes/v1EventOwnership. +jest.mock('../../../middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + userHasAnyPermission: async () => true, + userHasAllPermissions: async () => true, +})); + jest.mock('../../../middleware/apiTokenAuth', () => ({ apiTokenAuth: (req, _res, next) => { req.apiToken = { id: 1, admin_id: 1, scopes: ['write'] }; - req.admin = { id: 1, username: 'token-admin' }; + // roleName matters since GHSA-9697: requireEventOwnership now guards this + // route. super_admin short-circuits it without issuing a DB query, which + // keeps this suite's sequenced dbMock chains aligned — this suite is about + // category scoping, not ownership (see v1EventOwnership.test.js for that). + req.admin = { id: 1, username: 'token-admin', roleName: 'super_admin' }; next(); }, requireApiScope: () => (_req, _res, next) => next(), diff --git a/backend/src/routes/v1/__tests__/events.create.test.js b/backend/src/routes/v1/__tests__/events.create.test.js index a47b1daf..156317c8 100644 --- a/backend/src/routes/v1/__tests__/events.create.test.js +++ b/backend/src/routes/v1/__tests__/events.create.test.js @@ -51,6 +51,16 @@ jest.mock('../../../database/db', () => { }; }); +// RBAC is enforced on these routes since GHSA-9697 (requirePermission), but +// this suite mocks the database, so a real permission lookup would 500. These +// tests cover route logic, not authorization — the intersection of token +// scopes and role permissions is pinned in __tests__/routes/v1EventOwnership. +jest.mock('../../../middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next(), + userHasAnyPermission: async () => true, + userHasAllPermissions: async () => true, +})); + jest.mock('../../../middleware/apiTokenAuth', () => ({ apiTokenAuth: (req, _res, next) => { req.apiToken = { id: 1, admin_id: 1, scopes: ['admin'] }; diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 22a19b25..6887d8c9 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -20,6 +20,15 @@ const sharp = require('sharp'); const { body, query, validationResult } = require('express-validator'); const { db, logActivity } = require('../../database/db'); const { apiTokenAuth, requireApiScope } = require('../../middleware/apiTokenAuth'); +const { requireEventOwnership, scopeEventsQuery } = require('../../middleware/ownership'); +// GHSA-9697: migration 081 defines a token's effective permissions as the +// INTERSECTION of the owner's role permissions and the token's scope flags. +// requireApiScope only ever checked the scope half — so a token minted while +// its owner was super_admin kept full write access after the owner was demoted +// to viewer (userManagementService never touches api_tokens). These +// requirePermission gates supply the missing half; they key on req.admin.id, +// which apiTokenAuth populates. +const { requirePermission } = require('../../middleware/permissions'); const { buildShareLinkVariants } = require('../../services/shareLinkService'); const { generateThumbnail } = require('../../services/imageProcessor'); const logger = require('../../utils/logger'); @@ -115,6 +124,7 @@ router.post( '/events', apiTokenAuth, requireApiScope('admin'), + requirePermission('events.create'), [ body('event_name').isString().trim().notEmpty(), body('event_type').isIn(['wedding', 'birthday', 'corporate', 'other', 'family']), @@ -419,6 +429,7 @@ router.get( '/events', apiTokenAuth, requireApiScope('read'), + requirePermission('events.view'), [ query('page').optional().isInt({ min: 1 }).toInt(), query('limit').optional().isInt({ min: 1, max: 100 }).toInt() @@ -429,14 +440,19 @@ router.get( const limit = req.query.limit || 25; const offset = (page - 1) * limit; + // Scope to events the token owner may see (GHSA-9697). Previously this + // listed every event on the instance regardless of who owned the token. const [events, totalRow] = await Promise.all([ - db('events') - .select('id', 'slug', 'event_name', 'event_type', 'event_date', 'expires_at', - 'is_active', 'is_archived', 'is_draft', 'created_at') + scopeEventsQuery( + db('events') + .select('id', 'slug', 'event_name', 'event_type', 'event_date', 'expires_at', + 'is_active', 'is_archived', 'is_draft', 'created_at'), + req.admin + ) .orderBy('created_at', 'desc') .limit(limit) .offset(offset), - db('events').count('id as count').first() + scopeEventsQuery(db('events').count('id as count'), req.admin).first() ]); const total = parseInt(totalRow?.count || 0, 10); res.json({ events, pagination: { page, limit, total } }); @@ -467,7 +483,7 @@ router.get( * 200: { description: Event details } * 404: { description: Not found } */ -router.get('/events/:id', apiTokenAuth, requireApiScope('read'), async (req, res) => { +router.get('/events/:id', apiTokenAuth, requireApiScope('read'), requirePermission('events.view'), requireEventOwnership, async (req, res) => { try { const event = await db('events').where({ id: req.params.id }).first(); if (!event) return res.status(404).json({ error: 'Event not found' }); @@ -533,6 +549,8 @@ router.post( '/events/:id/photos', apiTokenAuth, requireApiScope('write'), + requirePermission('photos.upload'), + requireEventOwnership, photoUpload.single('photo'), async (req, res) => { let tempPath = null; @@ -685,7 +703,7 @@ router.post( * share_url: { type: string, format: uri } * 404: { description: Not found } */ -router.get('/events/:id/share-link', apiTokenAuth, requireApiScope('read'), async (req, res) => { +router.get('/events/:id/share-link', apiTokenAuth, requireApiScope('read'), requirePermission('events.view'), requireEventOwnership, async (req, res) => { try { const event = await db('events').where({ id: req.params.id }).first(); if (!event) return res.status(404).json({ error: 'Event not found' });