From 11f9f584ded5f777a61dc2e1e637d478a61ac377 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 2 Aug 2026 21:18:46 +0200 Subject: [PATCH] fix(security): scope dashboard stats/analytics/activity to the caller's events (stable) (#964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): scope dashboard endpoints to the caller's events (GHSA-c2jj, gqx7, jhcf) /dashboard/stats, /analytics and /activity are gated only by analytics.view, which the editor role holds — but the events LIST restricts editors to their own rows (adminEvents/crud.js: roleName === 'editor' -> created_by = admin.id). So an editor saw instance-wide totals, and via /analytics topGalleries other admins' gallery NAMES and SLUGS (the public gallery URL component), for events invisible to them everywhere else. - stats: all 10 aggregates scoped (events by id, photos/access_logs by event_id). - analytics: all 8 series/aggregates scoped, including topGalleries. The external tracker device breakdown reports instance-wide data with no event filter, so a scoped caller falls through to the access_logs heuristic instead, which IS scoped. - activity: feed scoped. activity_logs.event_id is nullable and the join is a leftJoin, so system-level rows (logins, settings changes) are deliberately excluded for a scoped caller — those are precisely the cross-admin actions the advisory is about. Scoping keys on 'editor' to mirror the events list exactly, so the admin role's dashboard is unchanged. filterOwnedEventIds uses the broader '!== super_admin' rule; the two conventions disagree in this codebase and matching the list is the no-regression choice. * fix(security): codex round 2 — fix activity misattribution, scope via subquery (GHSA-jhcf, c2jj, gqx7) - expenseService passed adminId as logActivity's THIRD positional parameter, which is eventId — so admin ids were being written into activity_logs.event_id. The /activity scoping filter trusts that column, and admin/event id sequences overlap, so a foreign admin's expense metadata could surface under an editor's event. All 11 calls now pass null for eventId and the admin as the actor, which is what they meant. - Dashboard scoping now uses a SUBQUERY instead of pluck()+whereIn. An editor owning more events than the driver's bind-parameter limit (~999 SQLite, 65535 Postgres) would have turned all three endpoints into 500s once each id became a placeholder; below the limit it still re-sent the full list for each of the ~10 aggregates per request. Note: two billInboundNow() calls also end in ', adminId)' but have an unrelated signature — verified untouched. * fix(security): codex round 3 — correct legacy accounting activity rows (GHSA-jhcf) expenseService called logActivity(type, metadata, adminId), but logActivity's third positional parameter is eventId. Every expense / incoming-invoice entry therefore stored the ACTING ADMIN'S ID in activity_logs.event_id. Round 2 scoped the activity feed with `WHERE activity_logs.event_id IN (SELECT id FROM events WHERE created_by = me)`, which does nothing about the rows already on disk. Admin ids and event ids are small integers from the same range, so on any upgraded instance an editor who owns the event whose id happens to equal another admin's id is served that admin's accounting activity, verbatim metadata included — GHSA-jhcf, still live. Migration 168 re-attributes those rows (event_id holds exactly the actor id that was lost) and then clears event_id so the scope predicate can no longer match them. All ten activity types are emitted by expenseService and nothing else, so no row with a genuine event_id is touched. Also: the round-2 rewrite passed `{ type: 'admin', id: adminId }` unconditionally, which stored actor_type='admin' with a null id for the automated mailbox intake (emailIntakeService calls recordInboundDocument with no adminId). adminActor() restores 'system' attribution for those. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me (cherry picked from commit 459e9e42434defd0dc7b87246e4d894dd47dcc56) --------- Co-authored-by: Paul Nothaft --- .../168_fix_expense_activity_event_id.test.js | 83 ++++++++ .../__tests__/routes/dashboardScope.test.js | 196 ++++++++++++++++++ .../core/168_fix_expense_activity_event_id.js | 64 ++++++ backend/src/routes/adminDashboard.js | 95 +++++++-- backend/src/services/expenseService.js | 31 ++- 5 files changed, 435 insertions(+), 34 deletions(-) create mode 100644 backend/__tests__/migrations/168_fix_expense_activity_event_id.test.js create mode 100644 backend/__tests__/routes/dashboardScope.test.js create mode 100644 backend/migrations/core/168_fix_expense_activity_event_id.js diff --git a/backend/__tests__/migrations/168_fix_expense_activity_event_id.test.js b/backend/__tests__/migrations/168_fix_expense_activity_event_id.test.js new file mode 100644 index 00000000..49f00183 --- /dev/null +++ b/backend/__tests__/migrations/168_fix_expense_activity_event_id.test.js @@ -0,0 +1,83 @@ +/** + * GHSA-jhcf round 3: scoping the activity feed does nothing about the rows + * already on disk. expenseService used to pass adminId into logActivity's + * `eventId` slot, so upgraded instances carry accounting rows whose event_id + * is an ADMIN id — and the scope predicate happily matches those against a + * same-numbered event the caller owns. + */ + +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-mig168-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'mig168-test-secret'; + +const { bootCrmDb } = require('../integration/helpers/crmDb'); +const migration = require('../../migrations/core/168_fix_expense_activity_event_id'); + +describe('migration 168 — legacy accounting activity rows (GHSA-jhcf)', () => { + let db; let cleanup; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('re-attributes the admin id and clears event_id, leaving real rows alone', async () => { + await db('activity_logs').insert([ + // Legacy shape: event_id is really admin #7, no actor recorded. + { + activity_type: 'expense_created', + actor_type: 'system', + actor_id: null, + event_id: 7, + metadata: JSON.stringify({ expenseId: 1 }), + created_at: new Date().toISOString(), + }, + { + activity_type: 'incoming_invoice_captured', + actor_type: 'system', + actor_id: null, + event_id: 9, + metadata: JSON.stringify({ inboundDocumentId: 2 }), + created_at: new Date().toISOString(), + }, + // A genuine event-scoped row from another subsystem must survive intact. + { + activity_type: 'photo_uploaded', + actor_type: 'admin', + actor_id: 3, + event_id: 7, + metadata: JSON.stringify({}), + created_at: new Date().toISOString(), + }, + ]); + + await migration.up(db); + + const expense = await db('activity_logs').where({ activity_type: 'expense_created' }).first(); + expect(expense.event_id == null).toBe(true); + expect(Number(expense.actor_id)).toBe(7); + expect(expense.actor_type).toBe('admin'); + + const captured = await db('activity_logs').where({ activity_type: 'incoming_invoice_captured' }).first(); + expect(captured.event_id == null).toBe(true); + expect(Number(captured.actor_id)).toBe(9); + + const photo = await db('activity_logs').where({ activity_type: 'photo_uploaded' }).first(); + expect(Number(photo.event_id)).toBe(7); + expect(Number(photo.actor_id)).toBe(3); + }); + + it('is idempotent on re-run', async () => { + await expect(migration.up(db)).resolves.toBeUndefined(); + const expense = await db('activity_logs').where({ activity_type: 'expense_created' }).first(); + expect(Number(expense.actor_id)).toBe(7); + expect(expense.event_id == null).toBe(true); + }); +}); diff --git a/backend/__tests__/routes/dashboardScope.test.js b/backend/__tests__/routes/dashboardScope.test.js new file mode 100644 index 00000000..ece77993 --- /dev/null +++ b/backend/__tests__/routes/dashboardScope.test.js @@ -0,0 +1,196 @@ +/** + * Dashboard endpoints must not leak other admins' data to event-scoped + * editors — GHSA-c2jj (/stats), GHSA-gqx7 (/analytics), GHSA-jhcf (/activity). + * + * All three are gated only by `analytics.view`, which the `editor` role holds. + * But the events LIST restricts editors to their own rows + * (adminEvents/crud.js: roleName === 'editor' → created_by = admin.id), so an + * editor saw instance-wide totals — and, via /analytics topGalleries, other + * admins' gallery names and SLUGS (the public gallery URL component) — for + * events invisible to them everywhere else. + * + * Scoping deliberately keys on `editor` to mirror the events list exactly, so + * the `admin` role's dashboard is unchanged. + */ + +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-dashscope-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'dashscope-test-secret'; + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('dashboard scoping (GHSA-c2jj / gqx7 / jhcf)', () => { + let db; let cleanup; let app; + let editorToken; let superToken; + let ownEventId; let foreignEventId; + + 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'); + const id = r[0]?.id ?? r[0]; + const token = jwt.sign( + { id, username, type: 'admin', role: roleName, loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' }, + ); + return { id, token }; + }; + + const mkEvent = async (slug, createdBy) => { + const r = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: `${slug}-name`, + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_token: `tok-${slug}`, + share_link: `/gallery/${slug}/tok-${slug}`, + 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 editor = await mkAdmin('scoped-editor', 'editor'); + const sup = await mkAdmin('root-admin', 'super_admin'); + editorToken = editor.token; + superToken = sup.token; + + ownEventId = await mkEvent('own-gallery', editor.id); + foreignEventId = await mkEvent('foreign-gallery', sup.id); + + // One photo + one view per event so the aggregates are non-zero. + for (const [eventId, name] of [[ownEventId, 'own'], [foreignEventId, 'foreign']]) { + await db('photos').insert({ + event_id: eventId, + filename: `${name}.jpg`, + path: `events/active/${name}.jpg`, + type: 'individual', + size_bytes: 1000, + uploaded_at: new Date().toISOString(), + }); + await db('access_logs').insert({ + event_id: eventId, + action: 'view', + ip_address: `10.0.0.${eventId}`, + user_agent: 'Mozilla/5.0', + timestamp: new Date().toISOString(), + }); + await db('activity_logs').insert({ + activity_type: 'photo_viewed', + actor_type: 'admin', + actor_name: `${name}-actor`, + event_id: eventId, + created_at: new Date().toISOString(), + }); + } + + app = express(); + app.use(express.json()); + app.use('/api/admin/dashboard', require('../../src/routes/adminDashboard')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('/stats counts only the editor\'s own events and photos', async () => { + const res = await request(app) + .get('/api/admin/dashboard/stats') + .set('Authorization', `Bearer ${editorToken}`); + + expect(res.status).toBe(200); + expect(Number(res.body.totalEvents)).toBe(1); + expect(Number(res.body.totalPhotos)).toBe(1); + expect(Number(res.body.storageUsed)).toBe(1000); + }); + + it('/analytics does not expose a foreign gallery name or slug', async () => { + const res = await request(app) + .get('/api/admin/dashboard/analytics?days=7') + .set('Authorization', `Bearer ${editorToken}`); + + expect(res.status).toBe(200); + const body = JSON.stringify(res.body); + expect(body).not.toContain('foreign-gallery'); + expect(body).not.toContain('foreign-gallery-name'); + expect(res.body.topGalleries.map((g) => g.slug)).toEqual(['own-gallery']); + }); + + it('/activity does not surface a foreign event\'s entries', async () => { + const res = await request(app) + .get('/api/admin/dashboard/activity') + .set('Authorization', `Bearer ${editorToken}`); + + expect(res.status).toBe(200); + const actors = res.body.map((a) => a.actorName); + expect(actors).toContain('own-actor'); + expect(actors).not.toContain('foreign-actor'); + }); + + it('leaves super_admin unscoped across all three', async () => { + const stats = await request(app) + .get('/api/admin/dashboard/stats') + .set('Authorization', `Bearer ${superToken}`); + expect(Number(stats.body.totalEvents)).toBe(2); + + const analytics = await request(app) + .get('/api/admin/dashboard/analytics?days=7') + .set('Authorization', `Bearer ${superToken}`); + expect(analytics.body.topGalleries.map((g) => g.slug).sort()) + .toEqual(['foreign-gallery', 'own-gallery']); + + const activity = await request(app) + .get('/api/admin/dashboard/activity') + .set('Authorization', `Bearer ${superToken}`); + expect(activity.body.map((a) => a.actorName)).toContain('foreign-actor'); + }); +}); + +/** + * Codex round 2: the /activity filter trusts `activity_logs.event_id`, but + * expenseService was passing `adminId` into logActivity's third positional + * parameter — which is `eventId`. Admin and event id sequences overlap, so a + * foreign admin's expense metadata could surface under an editor's event. + * Those writers now pass the actor instead, leaving event_id NULL. + */ +describe('activity writers do not put admin ids in event_id (GHSA-jhcf)', () => { + it('expenseService passes the actor, not adminId, as the event id', () => { + const fs2 = require('fs'); + const src = fs2.readFileSync( + require('path').join(__dirname, '../../src/services/expenseService.js'), 'utf8', + ); + // No logActivity call may end with a bare `, adminId)` — that slot is eventId. + const offenders = src.split('\n').filter( + (l) => l.includes('logActivity(') && /,\s*adminId\s*\)/.test(l), + ); + expect(offenders).toEqual([]); + // And the actor form must actually be in use. + expect(src).toContain("{ type: 'admin', id: adminId }"); + }); +}); diff --git a/backend/migrations/core/168_fix_expense_activity_event_id.js b/backend/migrations/core/168_fix_expense_activity_event_id.js new file mode 100644 index 00000000..098a79a9 --- /dev/null +++ b/backend/migrations/core/168_fix_expense_activity_event_id.js @@ -0,0 +1,64 @@ +/** + * GHSA-jhcf — data correction for legacy accounting activity rows. + * + * expenseService called `logActivity(type, metadata, adminId)`, but the third + * positional parameter of logActivity is `eventId`, not the actor. Every + * expense / incoming-invoice entry therefore stored the ACTING ADMIN'S ID in + * `activity_logs.event_id` (and no actor at all). + * + * That is not merely cosmetic. The dashboard activity feed now scopes rows via + * `WHERE activity_logs.event_id IN (SELECT id FROM events WHERE created_by = me)`. + * Admin ids and event ids are both small integers drawn from the same range, so + * on any upgraded instance an editor who happens to own the event whose id + * equals another admin's id is served that admin's accounting activity — + * verbatim metadata included. Scoping new writes correctly does nothing for the + * rows already on disk, so they are corrected here. + * + * The stored value is exactly the actor id we lost, so this re-attributes + * rather than discards: event_id → actor_id (when no actor was recorded), then + * event_id is cleared so the scope predicate can no longer match it. + * + * All ten activity types below are emitted by expenseService and nothing else, + * so no row with a genuine event_id is touched. + */ + +const AFFECTED_TYPES = [ + 'incoming_invoice_captured', + 'incoming_invoice_updated', + 'incoming_invoice_categorized', + 'incoming_invoice_rebilled', + 'incoming_invoices_rebilled_bundle', + 'incoming_invoice_supplier_payment', + 'expense_created', + 'expense_updated', + 'expense_invoiced', + 'expense_paid', +]; + +exports.up = async function up(knex) { + if (!(await knex.schema.hasTable('activity_logs'))) return; + if (!(await knex.schema.hasColumn('activity_logs', 'event_id'))) return; + + const hasActorId = await knex.schema.hasColumn('activity_logs', 'actor_id'); + const hasActorType = await knex.schema.hasColumn('activity_logs', 'actor_type'); + + if (hasActorId) { + const patch = { actor_id: knex.ref('event_id') }; + if (hasActorType) patch.actor_type = 'admin'; + await knex('activity_logs') + .whereIn('activity_type', AFFECTED_TYPES) + .whereNotNull('event_id') + .whereNull('actor_id') + .update(patch); + } + + await knex('activity_logs') + .whereIn('activity_type', AFFECTED_TYPES) + .whereNotNull('event_id') + .update({ event_id: null }); +}; + +// Irreversible by design: this is a data correction, and the pre-migration +// state is a cross-admin disclosure. Re-planting admin ids in event_id would +// reopen GHSA-jhcf. +exports.down = async function down() {}; diff --git a/backend/src/routes/adminDashboard.js b/backend/src/routes/adminDashboard.js index 3e2070e1..9c04840e 100644 --- a/backend/src/routes/adminDashboard.js +++ b/backend/src/routes/adminDashboard.js @@ -24,11 +24,47 @@ function normaliseDateKey(value) { return String(value).slice(0, 10); } +/** + * Event ids the caller's dashboard may aggregate over, or `null` when the + * caller is unrestricted (GHSA-c2jj / gqx7 / jhcf). + * + * These endpoints are gated only by `analytics.view`, which the `editor` role + * holds — yet the events *list* restricts editors to their own rows + * (adminEvents/crud.js: `roleName === 'editor'` → `created_by = admin.id`). + * The dashboard therefore reported instance-wide totals, and the analytics + * endpoint returned other admins' gallery names and slugs, to a role that + * cannot see those events anywhere else. + * + * Scoped on `editor` specifically to mirror the events list exactly, so the + * `admin` role's dashboard is unchanged. (`filterOwnedEventIds` uses the + * broader `!== super_admin` rule; the two conventions disagree in this + * codebase and matching the list is the no-regression choice.) + * + * @returns {Promise} ids to restrict to, or null for no limit + */ +function isScopedAdmin(admin) { + return admin?.roleName === 'editor'; +} + +/** + * Restrict `query` to the caller's own events. + * + * Uses a SUBQUERY rather than materialising the id list. An editor owning more + * events than the driver's bind-parameter limit (~999 on SQLite, 65535 on + * Postgres) would otherwise blow past it once every id became a placeholder, + * turning all three dashboard endpoints into 500s — and even well below that + * limit the whole list was re-sent for each of the ~10 aggregates per request. + */ +function applyEventScope(query, admin, column) { + if (!isScopedAdmin(admin)) return query; + return query.whereIn(column, db('events').select('id').where('created_by', admin.id)); +} + // Get dashboard statistics router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, res) => { try { // Get active events count - const activeEvents = await db('events') + const activeEvents = await applyEventScope(db('events'), req.admin, 'id') .where('is_active', formatBoolean(true)) .where('is_archived', formatBoolean(false)) .count('id as count') @@ -39,7 +75,7 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, sevenDaysFromNow.setDate(sevenDaysFromNow.getDate() + 7); const now = new Date(); - const expiringEvents = await db('events') + const expiringEvents = await applyEventScope(db('events'), req.admin, 'id') .where('is_active', formatBoolean(true)) .where('is_archived', formatBoolean(false)) .where('expires_at', '<=', sevenDaysFromNow.toISOString()) @@ -48,12 +84,12 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, .first(); // Get total photos count - const totalPhotos = await db('photos') + const totalPhotos = await applyEventScope(db('photos'), req.admin, 'event_id') .count('id as count') .first(); // Get storage usage (sum of all photo sizes) - const storageUsed = await db('photos') + const storageUsed = await applyEventScope(db('photos'), req.admin, 'event_id') .sum('size_bytes as total') .first(); @@ -61,21 +97,21 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, const thirtyDaysAgo = new Date(); thirtyDaysAgo.setDate(thirtyDaysAgo.getDate() - 30); - const totalViews = await db('access_logs') + const totalViews = await applyEventScope(db('access_logs'), req.admin, 'event_id') .where('action', 'view') .where('timestamp', '>=', thirtyDaysAgo.toISOString()) .count('id as count') .first(); // Get total downloads (last 30 days) - include both single and bulk downloads - const totalDownloads = await db('access_logs') + const totalDownloads = await applyEventScope(db('access_logs'), req.admin, 'event_id') .whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected']) .where('timestamp', '>=', thirtyDaysAgo.toISOString()) .count('id as count') .first(); // Get archived events count - const archivedEvents = await db('events') + const archivedEvents = await applyEventScope(db('events'), req.admin, 'id') .where('is_archived', formatBoolean(true)) .count('id as count') .first(); @@ -83,7 +119,7 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, // Get total events count (all events regardless of status) — used by the // events list page to render accurate "All (N)" / Total Events counters // when the table is server-paginated (#346). - const totalEvents = await db('events') + const totalEvents = await applyEventScope(db('events'), req.admin, 'id') .count('id as count') .first(); @@ -91,14 +127,14 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, const sixtyDaysAgo = new Date(); sixtyDaysAgo.setDate(sixtyDaysAgo.getDate() - 60); - const previousViews = await db('access_logs') + const previousViews = await applyEventScope(db('access_logs'), req.admin, 'event_id') .where('action', 'view') .where('timestamp', '>=', sixtyDaysAgo.toISOString()) .where('timestamp', '<', thirtyDaysAgo.toISOString()) .count('id as count') .first(); - const previousDownloads = await db('access_logs') + const previousDownloads = await applyEventScope(db('access_logs'), req.admin, 'event_id') .whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected']) .where('timestamp', '>=', sixtyDaysAgo.toISOString()) .where('timestamp', '<', thirtyDaysAgo.toISOString()) @@ -136,9 +172,19 @@ router.get('/activity', adminAuth, requirePermission('analytics.view'), async (r try { const { limit } = getPagination(req, { limit: 10 }); - const activities = await db('activity_logs') - .select('activity_logs.*', 'events.event_name') - .leftJoin('events', 'activity_logs.event_id', 'events.id') + // Scope the feed to the caller's own events (GHSA-jhcf) — it otherwise + // returned every admin's actions, including actor names and verbatim + // metadata. `activity_logs.event_id` is NULLABLE: system-level entries + // (logins, settings changes) carry no event, and those are deliberately + // EXCLUDED for a scoped caller rather than shown, since they are exactly + // the cross-admin actions this advisory is about. + const activities = await applyEventScope( + db('activity_logs') + .select('activity_logs.*', 'events.event_name') + .leftJoin('events', 'activity_logs.event_id', 'events.id'), + req.admin, + 'activity_logs.event_id' + ) .orderBy('activity_logs.created_at', 'desc') .limit(limit); @@ -244,7 +290,7 @@ router.get('/health', adminAuth, requirePermission('settings.view'), async (req, router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (req, res) => { try { const days = sanitizeDays(req.query.days || 7); - + // Generate date range const dates = []; for (let i = days - 1; i >= 0; i--) { @@ -262,21 +308,21 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async ( const startDateStr = startDate.toISOString(); // Get views per day - const viewsData = await db('access_logs') + const viewsData = await applyEventScope(db('access_logs'), req.admin, 'event_id') .select(db.raw('DATE(timestamp) as date'), db.raw('COUNT(*) as count')) .where('action', 'view') .where('timestamp', '>=', startDateStr) .groupByRaw('DATE(timestamp)'); // Get downloads per day - include both single and bulk downloads - const downloadsData = await db('access_logs') + const downloadsData = await applyEventScope(db('access_logs'), req.admin, 'event_id') .select(db.raw('DATE(timestamp) as date'), db.raw('COUNT(*) as count')) .whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected']) .where('timestamp', '>=', startDateStr) .groupByRaw('DATE(timestamp)'); // Get unique visitors per day - const visitorsData = await db('access_logs') + const visitorsData = await applyEventScope(db('access_logs'), req.admin, 'event_id') .select(db.raw('DATE(timestamp) as date'), db.raw('COUNT(DISTINCT ip_address) as count')) .where('timestamp', '>=', startDateStr) .groupByRaw('DATE(timestamp)'); @@ -303,7 +349,7 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async ( }); // Get top galleries by views with additional metrics - const topGalleries = await db('access_logs') + const topGalleries = await applyEventScope(db('access_logs'), req.admin, 'access_logs.event_id') .select('events.id', 'events.event_name', 'events.slug') .select(db.raw('COUNT(CASE WHEN action = \'view\' THEN 1 END) as views')) .select(db.raw('COUNT(DISTINCT CASE WHEN action = \'view\' THEN ip_address END) as uniqueVisitors')) @@ -324,7 +370,10 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async ( let devices = { desktop: 0, mobile: 0, tablet: 0 }; let devicesSource = 'access_logs'; - const adapter = await resolveAdapter(); + // The external tracker reports instance-wide device data with no way to + // filter it by event, so a scoped caller must not receive it (GHSA-gqx7). + // They fall through to the access_logs heuristic, which IS scoped. + const adapter = isScopedAdmin(req.admin) ? null : await resolveAdapter(); if (adapter) { try { const trackerDevices = await adapter.fetchDeviceBreakdown({ @@ -346,7 +395,7 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async ( // Local heuristic on access_logs user_agent. Coarse — `LIKE` doesn't // cover every UA shape (some Android browsers, embedded webviews, etc.) // — and counts come back as strings on Postgres, hence Number() below. - const deviceData = await db('access_logs') + const deviceData = await applyEventScope(db('access_logs'), req.admin, 'event_id') .select( db.raw(` CASE @@ -370,19 +419,19 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async ( } // Calculate totals for the period (matching /stats logic) - const totalViews = await db('access_logs') + const totalViews = await applyEventScope(db('access_logs'), req.admin, 'event_id') .where('action', 'view') .where('timestamp', '>=', startDateStr) .count('id as count') .first(); - const totalDownloadsCount = await db('access_logs') + const totalDownloadsCount = await applyEventScope(db('access_logs'), req.admin, 'event_id') .whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected']) .where('timestamp', '>=', startDateStr) .count('id as count') .first(); - const totalUniqueVisitors = await db('access_logs') + const totalUniqueVisitors = await applyEventScope(db('access_logs'), req.admin, 'event_id') .where('timestamp', '>=', startDateStr) .countDistinct('ip_address as count') .first(); diff --git a/backend/src/services/expenseService.js b/backend/src/services/expenseService.js index 8943805a..fbb3524c 100644 --- a/backend/src/services/expenseService.js +++ b/backend/src/services/expenseService.js @@ -22,6 +22,15 @@ const { AppError } = require('../utils/errors'); const logger = require('../utils/logger'); const invoiceService = require('./invoiceService'); +/** + * Actor for logActivity. `adminId` is legitimately absent on automated paths — + * emailIntakeService calls recordInboundDocument() with none — and an + * unconditional `{ type: 'admin' }` would store actor_type='admin' with a null + * id, mislabelling mailbox captures as somebody's deliberate action. Returning + * null restores logActivity's 'system' attribution for those. + */ +const adminActor = (adminId) => (adminId ? { type: 'admin', id: adminId } : null); + const DISPOSITIONS = ['rebill', 'durchlaufend', 'eigener_aufwand', 'duplikat', 'abgelehnt']; const TAX_TREATMENTS = ['domestic', 'reverse_charge_service', 'foreign_vat_non_reclaimable', 'import_goods']; const MARKUP_TYPES = ['none', 'percent', 'flat']; @@ -193,7 +202,7 @@ async function recordInboundDocument({ source, filePath, originalFilename, mimeT }; const inserted = await db('inbound_documents').insert(row).returning('id'); const id = typeof inserted[0] === 'object' ? inserted[0].id : inserted[0]; - await logActivity('incoming_invoice_captured', { inboundDocumentId: id, source: row.source, duplicate: !!duplicateOfId }, adminId); + await logActivity('incoming_invoice_captured', { inboundDocumentId: id, source: row.source, duplicate: !!duplicateOfId }, null, adminActor(adminId)); return getInbound(id); } @@ -242,7 +251,7 @@ async function updateInbound(id, payload, adminId) { if (payload[camel] !== undefined) patch[snake] = payload[camel] === '' ? null : payload[camel]; } await db('inbound_documents').where({ id }).update(patch); - await logActivity('incoming_invoice_updated', { inboundDocumentId: id }, adminId); + await logActivity('incoming_invoice_updated', { inboundDocumentId: id }, null, adminActor(adminId)); return getInbound(id); } @@ -458,8 +467,8 @@ async function categorizeInbound(id, payload, adminId) { }); // Audit logging AFTER commit — logActivity writes via the global db and would // deadlock if run inside the transaction above on a SQLite-backed install. - await logActivity('incoming_invoice_categorized', { inboundDocumentId: id, disposition }, adminId); - if (billedInvoiceId) await logActivity('incoming_invoice_rebilled', { inboundDocumentId: id, invoiceId: billedInvoiceId }, adminId); + await logActivity('incoming_invoice_categorized', { inboundDocumentId: id, disposition }, null, adminActor(adminId)); + if (billedInvoiceId) await logActivity('incoming_invoice_rebilled', { inboundDocumentId: id, invoiceId: billedInvoiceId }, null, adminActor(adminId)); return getInbound(id); } @@ -494,7 +503,7 @@ async function rebillInbound(id, payload, adminId, trx0) { const invoiceId = trx0 ? await run(trx0) : await db.transaction(run); // Log after commit (global-db write — see billInboundNow). When a caller // supplied trx0, that outer transaction owns the audit log instead. - if (!trx0) await logActivity('incoming_invoice_rebilled', { inboundDocumentId: id, invoiceId }, adminId); + if (!trx0) await logActivity('incoming_invoice_rebilled', { inboundDocumentId: id, invoiceId }, null, adminActor(adminId)); return { document: await getInbound(id), invoiceId }; } @@ -607,7 +616,7 @@ async function billPendingRebills(customerId, adminId) { return { invoiceId, count: pending.length }; }); // Audit log after commit (global-db write — see billInboundNow). - await logActivity('incoming_invoices_rebilled_bundle', { customerId: customer.id, invoiceId: result.invoiceId, count: result.count }, adminId); + await logActivity('incoming_invoices_rebilled_bundle', { customerId: customer.id, invoiceId: result.invoiceId, count: result.count }, null, adminActor(adminId)); return result; } @@ -624,7 +633,7 @@ async function markInboundSupplierPayment(id, { paid, paidAt, paymentMethod, pay supplier_payment_ref: paid ? (paymentReference || null) : null, updated_at: new Date(), }); - await logActivity('incoming_invoice_supplier_payment', { inboundDocumentId: id, paid: !!paid }, adminId); + await logActivity('incoming_invoice_supplier_payment', { inboundDocumentId: id, paid: !!paid }, null, adminActor(adminId)); return getInbound(id); } @@ -714,7 +723,7 @@ async function createExpense(payload, adminId, { receiptPath } = {}) { }); const inserted = await db('expenses').insert(row).returning('id'); const id = typeof inserted[0] === 'object' ? inserted[0].id : inserted[0]; - await logActivity('expense_created', { expenseId: id, kind: row.kind }, adminId); + await logActivity('expense_created', { expenseId: id, kind: row.kind }, null, adminActor(adminId)); return getExpense(id); } @@ -747,7 +756,7 @@ async function updateExpense(id, payload, adminId, { receiptPath } = {}) { } if (receiptPath) patch.receipt_path = receiptPath; await db('expenses').where({ id }).update(patch); - await logActivity('expense_updated', { expenseId: id }, adminId); + await logActivity('expense_updated', { expenseId: id }, null, adminActor(adminId)); return getExpense(id); } @@ -787,7 +796,7 @@ async function rebillExpense(id, payload, adminId, trx0) { status: 'invoiced', updated_at: new Date(), }); - await logActivity('expense_invoiced', { expenseId: id, invoiceId }, adminId); + await logActivity('expense_invoiced', { expenseId: id, invoiceId }, null, adminActor(adminId)); return invoiceId; }; const invoiceId = trx0 ? await run(trx0) : await db.transaction(run); @@ -807,7 +816,7 @@ async function markExpensePaid(id, { paid, paidAt, paymentMethod, paymentReferen payment_reference: paid ? (paymentReference || null) : null, updated_at: new Date(), }); - await logActivity('expense_paid', { expenseId: id, paid: !!paid }, adminId); + await logActivity('expense_paid', { expenseId: id, paid: !!paid }, null, adminActor(adminId)); return getExpense(id); }