From 3fe7d6bba203d75f2491aadae3342c2892bc7dcb Mon Sep 17 00:00:00 2001 From: Zac Date: Sat, 30 May 2026 12:52:29 +0000 Subject: [PATCH] fix(mam-api): close cross-project authz gaps in assets/bins/jobs/upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the v2 auth landing found four places where the per-project RBAC helpers weren't applied to destination/source projects, letting a scoped editor write into projects they don't have access to: - assets PATCH /:id: bin_id moved with no check, so an editor in project A could stuff their asset into a bin in project B. Now validates the bin's project_id matches the asset's own project (assets don't change project). - assets POST /:id/copy: body's projectId/binId never checked, so any reachable asset could be cloned into an arbitrary project. Now asserts edit on the destination project and validates binId belongs there. - bins POST /:id/assets: requireBinEdit checks edit on the bin's project but not on the source asset's project, so an asset from project B could be pulled into A's bin tree (and surfaced in A's views). Now the asset must belong to the bin's own project. - jobs POST /conform: project_id from body never gated, so any logged-in user could enqueue conform jobs against any project. Now asserts edit. - upload POST /init, POST /simple: projectId/binId from body never gated, same class of bug. Now asserts edit on projectId and validates binId. - upload GET /: returned every in-progress upload globally, leaking filenames across projects. Now scoped via accessibleProjectIds. These are the same pattern as the holes 2615143 closed on recorders/ sequences/imports/comments — these routes existed before the RBAC commit landed and were never marked TODO(authz), so the broad sweep missed them. Co-Authored-By: Claude Opus 4.7 --- services/mam-api/src/routes/assets.js | 35 +++++++++++++++++-- services/mam-api/src/routes/bins.js | 11 +++--- services/mam-api/src/routes/jobs.js | 5 +++ services/mam-api/src/routes/upload.js | 50 ++++++++++++++++++++++----- 4 files changed, 87 insertions(+), 14 deletions(-) diff --git a/services/mam-api/src/routes/assets.js b/services/mam-api/src/routes/assets.js index 73a5f71..038df5c 100644 --- a/services/mam-api/src/routes/assets.js +++ b/services/mam-api/src/routes/assets.js @@ -320,6 +320,18 @@ router.patch('/:id', requireAssetEdit, async (req, res, next) => { try { const { id } = req.params; const { display_name, tags, notes, bin_id } = req.body; + + // bin_id must reference a bin in the asset's OWN project — otherwise an + // editor in project A could stuff their asset into project B's bin tree. + // Null/empty clears the bin, which is always allowed. + if (bin_id) { + const bin = await pool.query('SELECT project_id FROM bins WHERE id = $1', [bin_id]); + if (bin.rows.length === 0) return res.status(400).json({ error: 'bin_id not found' }); + if (bin.rows[0].project_id !== req.assetProjectId) { + return res.status(400).json({ error: 'bin_id belongs to a different project' }); + } + } + const updates = [], params = []; let paramCount = 1; if (display_name !== undefined) { updates.push(`display_name = $${paramCount++}`); params.push(display_name); } @@ -345,6 +357,25 @@ router.post('/:id/copy', requireAssetEdit, async (req, res, next) => { const r = await pool.query('SELECT * FROM assets WHERE id = $1', [id]); if (r.rows.length === 0) return res.status(404).json({ error: 'Asset not found' }); const src = r.rows[0]; + + // Destination project defaults to source's. If the caller overrides it, + // assert edit on the target — without this, an editor in project A could + // clone any asset they can see into project B with no grant on B. + const destProjectId = projectId || src.project_id; + if (projectId && projectId !== src.project_id) { + await assertProjectAccess(req.user, destProjectId, 'edit'); + } + // Destination bin (if any) must belong to the destination project — same + // class of bug as the PATCH bin_id hole. + const destBinId = binId === undefined ? src.bin_id : (binId || null); + if (destBinId) { + const bin = await pool.query('SELECT project_id FROM bins WHERE id = $1', [destBinId]); + if (bin.rows.length === 0) return res.status(400).json({ error: 'binId not found' }); + if (bin.rows[0].project_id !== destProjectId) { + return res.status(400).json({ error: 'binId belongs to a different project than the destination' }); + } + } + const newId = uuidv4(); // Bug #60: null out proxy_s3_key and thumbnail_s3_key on the copy to avoid // sharing S3 objects with the source. Set status to 'processing' so the copy @@ -359,8 +390,8 @@ router.post('/:id/copy', requireAssetEdit, async (req, res, next) => { $1,$2,$3,$4,$5,$6,$7,$8,NULL,NULL,$9,$10,$11,$12,$13,$14,$15,$16,NOW(),NOW() ) RETURNING *`, [ - newId, projectId || src.project_id, - binId === undefined ? src.bin_id : (binId || null), + newId, destProjectId, + destBinId, src.filename, src.display_name, 'processing', src.media_type, src.original_s3_key, src.codec, src.resolution, src.fps, src.duration_ms, src.start_tc, diff --git a/services/mam-api/src/routes/bins.js b/services/mam-api/src/routes/bins.js index 53359ec..646d1eb 100644 --- a/services/mam-api/src/routes/bins.js +++ b/services/mam-api/src/routes/bins.js @@ -171,10 +171,13 @@ router.post('/:id/assets', requireBinEdit, async (req, res, next) => { return res.status(400).json({ error: 'asset_id is required' }); } - // Verify bin exists - const binCheck = await pool.query('SELECT id FROM bins WHERE id = $1', [id]); - if (binCheck.rows.length === 0) { - return res.status(404).json({ error: 'Bin not found' }); + // Asset must live in the bin's own project. Without this, an editor in + // project A (where the bin lives) could pull an asset from project B (no + // grant) into A's bin tree, exposing it in A's views. + const a = await pool.query('SELECT project_id FROM assets WHERE id = $1', [asset_id]); + if (a.rows.length === 0) return res.status(404).json({ error: 'Asset not found' }); + if (a.rows[0].project_id !== req.binProjectId) { + return res.status(400).json({ error: 'asset belongs to a different project than the bin' }); } // Update asset's bin_id diff --git a/services/mam-api/src/routes/jobs.js b/services/mam-api/src/routes/jobs.js index 1079b19..9efa751 100644 --- a/services/mam-api/src/routes/jobs.js +++ b/services/mam-api/src/routes/jobs.js @@ -1,6 +1,7 @@ import express from 'express'; import pool from '../db/pool.js'; import { Queue } from 'bullmq'; +import { assertProjectAccess } from '../auth/authz.js'; const router = express.Router(); // Note: jobs use BullMQ id format ":" (e.g. "conform:42"), @@ -324,6 +325,10 @@ router.post('/conform', async (req, res, next) => { }); } + // Conform writes back into a project — require edit on that project. Without + // this, any logged-in user could enqueue conform jobs targeting any project. + await assertProjectAccess(req.user, project_id, 'edit'); + const bullJob = await conformQueue.add('conform-task', { edl, projectId: project_id, diff --git a/services/mam-api/src/routes/upload.js b/services/mam-api/src/routes/upload.js index fcb89ef..c1ab9f3 100644 --- a/services/mam-api/src/routes/upload.js +++ b/services/mam-api/src/routes/upload.js @@ -14,6 +14,7 @@ import { AbortMultipartUploadCommand, } from '@aws-sdk/client-s3'; import { getAmppConfig, ensureFolderPath } from '../ampp/client.js'; +import { assertProjectAccess, accessibleProjectIds } from '../auth/authz.js'; const router = express.Router(); @@ -138,16 +139,24 @@ function mediaTypeFromMime(mime = '') { return 'document'; } -// GET /api/v1/upload - List in-progress uploads (#68) +// GET /api/v1/upload - List in-progress uploads (#68). Scoped to projects the +// caller can see — admins are unfiltered; a scoped viewer/editor only sees +// uploads for projects they have access to (no enumeration of other projects' +// in-flight filenames). router.get('/', async (req, res, next) => { try { - const result = await pool.query( - `SELECT id, filename, display_name, project_id, bin_id, status, created_at, updated_at - FROM assets - WHERE status = 'ingesting' - ORDER BY created_at DESC - LIMIT 50` - ); + const access = await accessibleProjectIds(req.user); + let query = `SELECT id, filename, display_name, project_id, bin_id, status, created_at, updated_at + FROM assets + WHERE status = 'ingesting'`; + const params = []; + if (!access.all) { + if (access.ids.size === 0) return res.json([]); + query += ` AND project_id = ANY($1::uuid[])`; + params.push([...access.ids]); + } + query += ` ORDER BY created_at DESC LIMIT 50`; + const result = await pool.query(query, params); res.json(result.rows); } catch (err) { next(err); } }); @@ -163,6 +172,17 @@ router.post('/init', async (req, res, next) => { }); } + // Uploading creates an asset under a project — require edit on that project. + // Without this, any logged-in user could write into any project. + await assertProjectAccess(req.user, projectId, 'edit'); + if (binId) { + const bin = await pool.query('SELECT project_id FROM bins WHERE id = $1', [binId]); + if (bin.rows.length === 0) return res.status(400).json({ error: 'binId not found' }); + if (bin.rows[0].project_id !== projectId) { + return res.status(400).json({ error: 'binId belongs to a different project' }); + } + } + const assetId = uuidv4(); const s3Key = `originals/${assetId}/${filename}`; const tagsArray = tags ? (Array.isArray(tags) ? tags : [tags]) : []; @@ -326,6 +346,20 @@ router.post('/simple', upload.single('file'), async (req, res, next) => { }); } + // Same authz gate as /init. + await assertProjectAccess(req.user, projectId, 'edit'); + if (binId) { + const bin = await pool.query('SELECT project_id FROM bins WHERE id = $1', [binId]); + if (bin.rows.length === 0) { + unlinkPart(tmpPath); + return res.status(400).json({ error: 'binId not found' }); + } + if (bin.rows[0].project_id !== projectId) { + unlinkPart(tmpPath); + return res.status(400).json({ error: 'binId belongs to a different project' }); + } + } + const assetId = uuidv4(); const s3Key = `originals/${assetId}/${filename}`; const mimeType = contentType || req.file.mimetype;