diff --git a/backend/__tests__/routes/authzPermissionGaps.test.js b/backend/__tests__/routes/authzPermissionGaps.test.js new file mode 100644 index 00000000..b72a2548 --- /dev/null +++ b/backend/__tests__/routes/authzPermissionGaps.test.js @@ -0,0 +1,211 @@ +/** + * Authorization / ownership gaps (GHSA permission cluster): + * - jm7j: API-token list must scope to the caller (non-super sees only own) + * - gprq: API-token revoke must be owner-or-super_admin + * - 3rqx: event update must not mass-assign identity/secret columns + * - j2f4: category hero must belong to that category + */ +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-authz-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'authz-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-authz-storage-')); + +const request = require('supertest'); +const express = require('express'); +const cookieParser = require('cookie-parser'); +const bcrypt = require('bcrypt'); +const { + bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken, +} = require('../integration/helpers/crmDb'); + +describe('authorization / ownership gaps', () => { + let db; let cleanup; let app; + let superId; let superTok; let adminId; let adminTok; + + const grantPermissionToRole = async (roleName, permName) => { + const role = await db('roles').where({ name: roleName }).first(); + const perm = await db('permissions').where({ name: permName }).first(); + const exists = await db('role_permissions') + .where({ role_id: role.id, permission_id: perm.id }).first(); + if (!exists) { + await db('role_permissions').insert({ role_id: role.id, permission_id: perm.id }); + } + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ adminId: superId } = await seedMinimal(db)); + await assignAdminRole(db, superId, 'super_admin'); + superTok = mintAdminToken(superId); + + const pass = await bcrypt.hash('x', 4); + const ins = await db('admin_users').insert({ + username: 'plain-admin', email: 'plain@example.com', + password_hash: pass, must_change_password: false, created_at: new Date(), + }).returning('id'); + adminId = ins[0]?.id ?? ins[0]; + await assignAdminRole(db, adminId, 'admin'); + // Grant settings.edit to the admin role BEFORE any request populates the + // 60s permission cache, so the revoke test exercises the ownership check + // (404) rather than the missing-permission gate (403). This models a + // custom role that carries settings.edit — the scenario GHSA-gprq needs. + await grantPermissionToRole('admin', 'settings.edit'); + adminTok = mintAdminToken(adminId); + + app = express(); + app.use(express.json()); + app.use(cookieParser()); + app.use('/api/admin/api-tokens', require('../../src/routes/adminApiTokens')); + app.use('/api/admin/events', require('../../src/routes/adminEvents')); + app.use('/api/admin/categories', require('../../src/routes/adminCategories')); + // eslint-disable-next-line no-unused-vars + app.use((err, req, res, next) => { + res.status(err.statusCode || err.status || 500).json({ error: err.message, code: err.code }); + }); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + const auth = (req, tok) => req.set('Authorization', `Bearer ${tok}`); + + describe('API tokens (jm7j / gprq)', () => { + let superTokenId; + + beforeAll(async () => { + const res = await auth(request(app).post('/api/admin/api-tokens'), superTok) + .send({ name: 'super-token', scopes: ['read'] }); + expect(res.status).toBe(201); + superTokenId = res.body.id; + }); + + it('non-super admin does not see another admin\'s tokens in the list', async () => { + const res = await auth(request(app).get('/api/admin/api-tokens'), adminTok); + expect(res.status).toBe(200); + expect(res.body.find((t) => t.id === superTokenId)).toBeUndefined(); + }); + + it('super_admin sees all tokens', async () => { + const res = await auth(request(app).get('/api/admin/api-tokens'), superTok); + expect(res.status).toBe(200); + expect(res.body.find((t) => t.id === superTokenId)).toBeDefined(); + }); + + it('a non-owner (with settings.edit) cannot revoke another admin\'s token', async () => { + const res = await auth(request(app).delete(`/api/admin/api-tokens/${superTokenId}`), adminTok); + expect(res.status).toBe(404); + const row = await db('api_tokens').where({ id: superTokenId }).first(); + expect(row.revoked_at).toBeFalsy(); + }); + + it('the owner can revoke their own token', async () => { + const res = await auth(request(app).delete(`/api/admin/api-tokens/${superTokenId}`), superTok); + expect(res.status).toBe(200); + }); + }); + + describe('event update mass-assignment (3rqx)', () => { + it('ignores identity/secret columns in the request body', async () => { + const seedShareToken = 'orig-share-token'; + const ins = await db('events').insert({ + slug: 'authz-mass-assign', event_type: 'wedding', event_name: 'Before', + event_date: '2026-08-01', host_email: 'h@example.com', admin_email: 'a@example.com', + password_hash: 'orig-hash', share_link: '/gallery/authz/share', share_token: seedShareToken, expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, created_by: superId, + created_at: new Date().toISOString(), + }).returning('id'); + const eventId = ins[0]?.id ?? ins[0]; + + const res = await auth(request(app).put(`/api/admin/events/${eventId}`), superTok).send({ + event_name: 'After', + created_by: 99999, + slug: 'hijacked-slug', + share_token: 'hijacked-token', + password_hash: 'hijacked-hash', + is_archived: 1, + archive_path: '/hijacked/archive/path', + hero_logo_path: '/etc/passwd', + is_draft: 1, + project_id: 99999, + // Case-variant keys — SQLite matches columns case-insensitively. + Password_Hash: 'case-hijack-hash', + Created_By: 88888, + }); + expect(res.status).toBe(200); + + const row = await db('events').where({ id: eventId }).first(); + expect(row.event_name).toBe('After'); // legit field applied + expect(row.created_by).toBe(superId); // ownership untouched (+ case-variant) + expect(row.slug).toBe('authz-mass-assign'); // routing identity untouched + expect(row.share_token).toBe(seedShareToken); // secret untouched + expect(row.password_hash).toBe('orig-hash'); // secret untouched (+ case-variant) + expect(row.is_archived).toBeFalsy(); // archive lifecycle untouched + expect(row.archive_path).toBeFalsy(); // forged archive path rejected + expect(row.hero_logo_path).toBeFalsy(); // fs.unlink primitive blocked + expect(row.is_draft).toBeFalsy(); // publish workflow not bypassed + expect(row.project_id).toBeFalsy(); // server-managed relationship untouched + }); + + it('returns 200 (no-op) when the body contains only protected fields', async () => { + const ins = await db('events').insert({ + slug: 'authz-empty-update', event_type: 'wedding', event_name: 'Keep', + event_date: '2026-08-01', host_email: 'h@example.com', admin_email: 'a@example.com', + password_hash: 'x', share_link: '/gallery/authz-empty/share', share_token: 'authz-empty-share', + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, created_by: superId, + created_at: new Date().toISOString(), + }).returning('id'); + const id = ins[0]?.id ?? ins[0]; + // Body reduces to {} after the denylist — must not 500 (Knex rejects + // .update({})). + const res = await auth(request(app).put(`/api/admin/events/${id}`), superTok) + .send({ created_by: 1, slug: 'x', is_archived: 1 }); + expect(res.status).toBe(200); + const row = await db('events').where({ id }).first(); + expect(row.event_name).toBe('Keep'); + }); + }); + + describe('category hero cross-category (j2f4)', () => { + it('rejects a hero photo that is not in the category', async () => { + const evIns = await db('events').insert({ + slug: 'authz-cat', event_type: 'wedding', event_name: 'Cat Event', + event_date: '2026-08-01', host_email: 'h@example.com', admin_email: 'a@example.com', + password_hash: 'x', share_link: '/gallery/authz-cat/share', share_token: 'authz-cat-share', expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, created_by: superId, + created_at: new Date().toISOString(), + }).returning('id'); + const evId = evIns[0]?.id ?? evIns[0]; + + const mkCat = async (name) => { + const c = await db('photo_categories').insert({ + event_id: evId, name, slug: name.toLowerCase(), created_at: new Date().toISOString(), + }).returning('id'); + return c[0]?.id ?? c[0]; + }; + const cat1 = await mkCat('Cat1'); + const cat2 = await mkCat('Cat2'); + + const pIns = await db('photos').insert({ + event_id: evId, filename: 'p.jpg', path: 'authz-cat/p.jpg', type: 'individual', + category_id: cat1, uploaded_at: new Date().toISOString(), + }).returning('id'); + const photoInCat1 = pIns[0]?.id ?? pIns[0]; + + // Pointing cat2's hero at a photo that lives in cat1 must be refused. + const bad = await auth(request(app).put(`/api/admin/categories/${cat2}/hero`), superTok) + .send({ hero_photo_id: photoInCat1 }); + expect(bad.status).toBe(404); + + // The photo's own category accepts it. + const ok = await auth(request(app).put(`/api/admin/categories/${cat1}/hero`), superTok) + .send({ hero_photo_id: photoInCat1 }); + expect(ok.status).toBe(200); + }); + }); +}); diff --git a/backend/src/routes/adminApiTokens.js b/backend/src/routes/adminApiTokens.js index 95bcc81d..3b625609 100644 --- a/backend/src/routes/adminApiTokens.js +++ b/backend/src/routes/adminApiTokens.js @@ -20,7 +20,10 @@ const router = express.Router(); // the plaintext, never recoverable after creation. router.get('/', adminAuth, requirePermission('settings.view'), async (req, res) => { try { - const tokens = await db('api_tokens') + // Scope to the caller's own tokens unless super_admin — the previous + // query returned every admin's token metadata (name/preview/scopes/ + // owner) to any settings.view holder (GHSA-jm7j). + const tokensQuery = db('api_tokens') .leftJoin('admin_users', 'admin_users.id', 'api_tokens.created_by') .select( 'api_tokens.id', @@ -34,6 +37,10 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res) 'admin_users.username as owner_username' ) .orderBy('api_tokens.created_at', 'desc'); + if (req.admin.roleName !== 'super_admin') { + tokensQuery.where('api_tokens.created_by', req.admin.id); + } + const tokens = await tokensQuery; // toIso: last_used_at / revoked_at were written as raw Dates before // this fix — SQLite installs hold epoch numbers in existing rows. res.json(tokens.map((t) => ({ @@ -110,6 +117,12 @@ router.delete('/:id', adminAuth, requirePermission('settings.edit'), async (req, const { id } = req.params; const row = await db('api_tokens').where({ id }).first(); if (!row) return res.status(404).json({ error: 'Token not found' }); + // Only the token's owner (or a super_admin) may revoke it — otherwise + // any settings.edit holder could revoke another admin's tokens + // (GHSA-gprq). 404 rather than 403 so a non-owner can't probe token ids. + if (req.admin.roleName !== 'super_admin' && row.created_by !== req.admin.id) { + return res.status(404).json({ error: 'Token not found' }); + } if (row.revoked_at) return res.status(400).json({ error: 'Token already revoked' }); await db('api_tokens').where({ id }).update({ revoked_at: new Date().toISOString() }); diff --git a/backend/src/routes/adminCategories.js b/backend/src/routes/adminCategories.js index 809cbd43..fd6a1ffc 100644 --- a/backend/src/routes/adminCategories.js +++ b/backend/src/routes/adminCategories.js @@ -152,8 +152,18 @@ router.put('/:id', adminAuth, requirePermission('settings.edit'), [ .trim() }; - // Update hero_photo_id if provided (including null to clear it) + // Update hero_photo_id if provided (including null to clear it). A + // non-null hero must belong to this category (GHSA-j2f4) — the general + // update path previously wrote it with no membership check at all. if (Object.prototype.hasOwnProperty.call(req.body, 'hero_photo_id')) { + if (hero_photo_id) { + const heroPhoto = await db('photos') + .where({ id: hero_photo_id, category_id: id }) + .first(); + if (!heroPhoto) { + return res.status(404).json({ error: 'Photo not found in this category' }); + } + } updateData.hero_photo_id = hero_photo_id || null; } @@ -203,11 +213,16 @@ router.put('/:id/hero', adminAuth, requirePermission('settings.edit'), [ return res.status(404).json({ error: 'Category not found' }); } - // If hero_photo_id is provided, verify it belongs to a photo in this category + // If hero_photo_id is provided, verify the photo actually belongs to + // THIS category — checking existence alone let an admin point a + // category's hero at a photo from a different category or event + // (GHSA-j2f4). if (hero_photo_id) { - const photo = await db('photos').where('id', hero_photo_id).first(); + const photo = await db('photos') + .where({ id: hero_photo_id, category_id: id }) + .first(); if (!photo) { - return res.status(404).json({ error: 'Photo not found' }); + return res.status(404).json({ error: 'Photo not found in this category' }); } } diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index c1cfcfa2..78ac036c 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -1278,6 +1278,52 @@ module.exports = (router) => { const { id } = req.params; const updates = { ...req.body }; + + // Strip identity/provenance/secret columns from the mass-assigned + // body (GHSA-3rqx). The handler spreads req.body straight into the + // events UPDATE, so without this an events.edit holder could rewrite + // ownership (created_by), routing identity (slug/share_link), the + // share/client tokens, or the password hashes directly. Plaintext + // `password`/`client_password` inputs are NOT stripped — those are the + // supported way to change credentials and get hashed below; the + // tokens are regenerated internally where needed. + // The handler spreads req.body straight into the events UPDATE, so any + // column an events.edit holder names is writable unless blocked here. + // This is a COMPLETE deny-set of every server-managed / permission-gated + // events column (enumerated from the schema); everything else is a + // legitimate edit-form field and passes through, including input-only + // keys (password/client_password) the handler transforms below. New + // server-managed columns MUST be added here. (codex review — GHSA-3rqx.) + const IMMUTABLE_EVENT_COLUMNS = [ + // Identity / provenance + 'id', 'created_by', 'created_at', 'updated_at', 'slug', + // Routing + share/client tokens (generated at create / internally) + 'share_link', 'share_token', 'client_share_token', 'show_share_token', + // Secrets (set via the plaintext password/client_password inputs) + 'password_hash', 'client_password_hash', + // Server-consumed file paths — e.g. DELETE /:id/logo fs.unlink()s + // hero_logo_path, so a forged value is an arbitrary-delete primitive. + 'hero_logo_path', 'hero_logo_url', 'archive_path', 'download_zip_path', + // Server-managed timestamps + 'download_zip_generated_at', 'archived_at', 'revealed_at', 'event_reminder_sent_at', + // Lifecycle — governed by dedicated permission-gated routes + // (events.archive/restore, publish, activate/deactivate), not events.edit. + 'is_archived', 'is_draft', 'is_active', + // Relationships — managed by projectService.assignEvent + its + // customer-consistency checks, and events.edit ≠ quotes/contracts perms. + 'project_id', 'quote_id', + // Legacy mirrors — rejected explicitly below in favour of customer_*. + 'host_name', 'host_email', + ]; + // Case-insensitive match: SQLite treats quoted identifiers + // case-insensitively, so a `{ "Password_Hash": ... }` key would + // otherwise survive a case-sensitive delete and still hit the real + // column (codex review). + const denied = new Set(IMMUTABLE_EVENT_COLUMNS.map((c) => c.toLowerCase())); + for (const key of Object.keys(updates)) { + if (denied.has(key.toLowerCase())) delete updates[key]; + } + const customerColumnsAvailable = await hasCustomerContactColumns(); if (Object.prototype.hasOwnProperty.call(updates, 'host_name') || Object.prototype.hasOwnProperty.call(updates, 'host_email')) { @@ -1549,10 +1595,15 @@ module.exports = (router) => { } } - // Update event - await db('events') - .where('id', id) - .update(updates); + // Update event. Skip the write when the denylist (or masked secrets) + // left nothing to change — Knex rejects .update({}) with an error, + // which would surface as a 500 for an otherwise-valid no-op request + // (e.g. a body of only protected fields). (codex review.) + if (Object.keys(updates).length > 0) { + await db('events') + .where('id', id) + .update(updates); + } // Customer-account assignments (#354). Same skip semantics as POST: // ignore when the customer portal flag is off so stale tabs don't diff --git a/backend/src/routes/adminProjects.js b/backend/src/routes/adminProjects.js index c7e47f0b..3c335e65 100644 --- a/backend/src/routes/adminProjects.js +++ b/backend/src/routes/adminProjects.js @@ -15,8 +15,31 @@ const { requirePermission, userHasAnyPermission } = require('../middleware/permi const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers'); const projectService = require('../services/projectService'); const { db } = require('../database/db'); +const { ForbiddenError } = require('../utils/errors'); const router = express.Router(); + +// A deal that spans both quotes and contracts cascades a project link across +// BOTH tables (projectService.linkDealToProject). So attaching one document +// must also require manage permission on the OTHER domain the cascade will +// touch — otherwise quotes.manage alone could re-point a linked contract, and +// vice versa (GHSA-v4vw / codex review). No-op when the deal touches only the +// one domain, or on older instances without the deal_uuid column. +async function assertCascadePermitted(req, docTable, docId, otherTable, otherPerm) { + let doc; + try { + doc = await db(docTable).where({ id: docId }).first('deal_uuid'); + } catch { return; } + if (!doc || !doc.deal_uuid) return; + let linked; + try { + linked = await db(otherTable).where({ deal_uuid: doc.deal_uuid }).first('id'); + } catch { return; } + if (!linked) return; + if (!(await userHasAnyPermission(req.admin.id, [otherPerm]))) { + throw new ForbiddenError(`This deal also links a ${otherTable.replace(/s$/, '')}; the ${otherPerm} permission is required`); + } +} router.use(adminAuth); // Projects is feature-flagged like bills/quotes — when off, the whole cockpit @@ -105,23 +128,30 @@ router.post('/:id/events', ); // Attach a quote to the project (quotes carry no event_id — migration 121). +// Requires quotes.manage in addition to events.edit — attaching a quote +// mutates a separately-permissioned document domain (GHSA-v4vw). router.post('/:id/quotes', - requirePermission('events.edit'), + requirePermission(['events.edit', 'quotes.manage'], { requireAll: true }), [param('id').isInt({ min: 1 }), body('quoteId').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); - const result = await projectService.assignQuote(parseInt(req.params.id, 10), parseInt(req.body.quoteId, 10)); + const quoteId = parseInt(req.body.quoteId, 10); + await assertCascadePermitted(req, 'quotes', quoteId, 'contracts', 'contracts.manage'); + const result = await projectService.assignQuote(parseInt(req.params.id, 10), quoteId); return successResponse(res, result, 200, 'Quote attached to project'); }), ); -// Attach a contract to the project. +// Attach a contract to the project. Requires contracts.manage in addition +// to events.edit (GHSA-v4vw). router.post('/:id/contracts', - requirePermission('events.edit'), + requirePermission(['events.edit', 'contracts.manage'], { requireAll: true }), [param('id').isInt({ min: 1 }), body('contractId').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); - const result = await projectService.assignContract(parseInt(req.params.id, 10), parseInt(req.body.contractId, 10)); + const contractId = parseInt(req.body.contractId, 10); + await assertCascadePermitted(req, 'contracts', contractId, 'quotes', 'quotes.manage'); + const result = await projectService.assignContract(parseInt(req.params.id, 10), contractId); return successResponse(res, result, 200, 'Contract attached to project'); }), );