paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(security): scope dashboard stats/analytics/activity to the caller's events (GHSA-c2jj, gqx7, jhcf) (#958)

Browse Source 
* 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

---------

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-02 21:17:08 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 0d4c30884e
commit da855cfef9
5 changed files with 435 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/adminDashboard.js
+72 -23
View File
@@ -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<number[]|null>} 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();
backend/src/services/expenseService.js
+20 -11
View File
@@ -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);
}