fix(security): enforce event ownership on the v1 API surface (GHSA-9697) (#957)
* fix(security): enforce event ownership on the v1 API surface (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. Every ownership helper keys on roleName, so the v1 surface could not tell a super_admin from a demoted viewer. Now joins roles and emits the same req.admin shape adminAuth does, including the roles-table-missing upgrade fallback. - No v1 route applied any ownership predicate: GET /events listed every event on the instance, and GET /events/:id/share-link returned ANY event's share_token — the gallery access credential, same class as GHSA-rh8r. List is now scoped via a new scopeEventsQuery helper; the three :id routes (detail, photo upload, share-link) use the existing requireEventOwnership. Not a breaking change: tokens are minted by super_admins, who bypass ownership. It closes the case where a token's owner is later demoted — userManagementService never touches api_tokens, so the token outlived the demotion with full read of every gallery's share token. events.category.test.js stubbed apiTokenAuth without roleName; giving the stub super_admin keeps requireEventOwnership from issuing a DB query and desyncing that suite's sequenced dbMock. * fix(security): codex round 2 — intersect v1 token scopes with role permissions (GHSA-9697) Ownership scoping alone left half the documented control missing. 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. A token minted while its owner was super_admin therefore kept write access after the owner was demoted to viewer — userManagementService never touches api_tokens, so the token outlives the demotion, and ownership scoping does not help because the demoted owner still owns their events. Adds requirePermission to all six v1 routes (events.create on create, events.view on the reads, photos.upload on upload). It keys on req.admin.id, which apiTokenAuth already populates. The two existing v1 suites mock the database, so a real permission lookup 500s — they now mock the permissions middleware as pass-through, matching how they already mock apiTokenAuth. Those suites cover route logic; the intersection is pinned by the new v1TokenPermissions suite. * fix(security): codex round 3 — fail closed on the roles-join fallback (GHSA-9697) The round-2 fix loaded the token owner's role so the v1 ownership checks could tell a super_admin from a demoted viewer, and mirrored adminAuth's roles-table-missing fallback. That fallback assigns role_name = 'super_admin', and the catch around it was unconditional — so ANY failure of the joined query (connection reset, deadlock, statement timeout) elevated the token owner to super_admin as long as the simpler fallback query then succeeded. A restricted owner could ride that into listing, reading and share-tokening every event on the instance, which is the exact hole GHSA-9697 closes. The fallback is now reached only for an error that genuinely names a missing roles table/column (PG 42P01/42703 or the SQLite/MySQL wording); anything else propagates to the 500 handler. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
co-authored by
Paul Nothaft
parent
1b4e5fee3e
commit
e2ce95ee48
@@ -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(),
|
||||
|
||||
@@ -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'] };
|
||||
|
||||
@@ -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');
|
||||
@@ -116,6 +125,7 @@ router.post(
|
||||
'/events',
|
||||
apiTokenAuth,
|
||||
requireApiScope('admin'),
|
||||
requirePermission('events.create'),
|
||||
[
|
||||
body('event_name').isString().trim().notEmpty(),
|
||||
// Validate against the live event_types catalog (admins can rename/delete
|
||||
@@ -427,6 +437,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()
|
||||
@@ -437,14 +448,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 } });
|
||||
@@ -484,7 +500,7 @@ router.get(
|
||||
* name: { type: string }
|
||||
* emoji: { type: string }
|
||||
*/
|
||||
router.get('/event-types', apiTokenAuth, requireApiScope('read'), async (req, res) => {
|
||||
router.get('/event-types', apiTokenAuth, requireApiScope('read'), requirePermission('events.view'), async (req, res) => {
|
||||
try {
|
||||
const types = await db('event_types')
|
||||
.where('is_active', formatBoolean(true))
|
||||
@@ -517,7 +533,7 @@ router.get('/event-types', apiTokenAuth, requireApiScope('read'), async (req, re
|
||||
* 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' });
|
||||
@@ -583,6 +599,8 @@ router.post(
|
||||
'/events/:id/photos',
|
||||
apiTokenAuth,
|
||||
requireApiScope('write'),
|
||||
requirePermission('photos.upload'),
|
||||
requireEventOwnership,
|
||||
photoUpload.single('photo'),
|
||||
async (req, res) => {
|
||||
let tempPath = null;
|
||||
@@ -735,7 +753,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' });
|
||||
|
||||
Reference in New Issue
Block a user