diff --git a/backend/__tests__/routes/adminPhotoCategoryScope.test.js b/backend/__tests__/routes/adminPhotoCategoryScope.test.js new file mode 100644 index 00000000..853b8c35 --- /dev/null +++ b/backend/__tests__/routes/adminPhotoCategoryScope.test.js @@ -0,0 +1,188 @@ +/** + * Category scope on the admin photo update routes. + * + * The upload route validates a numeric category_id against + * (event_id = this event OR is_global) and 400s an out-of-scope id + * (#500 / #525), but PATCH /:eventId/photos/:photoId and + * POST /:eventId/photos/bulk-update accepted ANY positive id — so a photo + * could be filed under another event's category, where no grid filter + * (neither the category filters nor the whereNull "uncategorized" one) + * would ever show it again. + * + * Pins: + * - an id belonging to a different event is rejected with the same 400 + * shape the upload route uses, on both routes + * - a global category and this event's own category are both accepted + */ + +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-cat-scope-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'cat-scope-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-cat-scope-storage-')); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('admin photo category scope (PATCH / bulk-update)', () => { + let db; + let cleanup; + let app; + let eventId; + let otherEventId; + let photoId; + let adminToken; + let ownCategoryId; + let globalCategoryId; + let foreignCategoryId; + + const unwrap = (rows) => { + const row = rows[0]; + return typeof row === 'object' && row !== null ? row.id : row; + }; + + const seedEvent = async (slug) => { + const inserted = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: `Cat Scope ${slug}`, + event_date: '2026-09-01', + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: `/gallery/${slug}/share`, + share_token: `${slug}-share`, + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + return unwrap(inserted); + }; + + const patchCategory = (categoryId) => request(app) + .patch(`/api/admin/photos/${eventId}/photos/${photoId}`) + .set('Authorization', `Bearer ${adminToken}`) + .send({ category_id: categoryId }); + + const bulkUpdateCategory = (categoryId) => request(app) + .post(`/api/admin/photos/${eventId}/photos/bulk-update`) + .set('Authorization', `Bearer ${adminToken}`) + .send({ photoIds: [photoId], updates: { category_id: categoryId } }); + + const storedCategoryId = async () => { + const row = await db('photos').where({ id: photoId }).first(); + return row.category_id; + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + eventId = await seedEvent('cat-scope-event'); + otherEventId = await seedEvent('cat-scope-other-event'); + + // is_global defaults to TRUE on this table, so the event-scoped rows have + // to say so explicitly — otherwise every category is in scope everywhere. + ownCategoryId = unwrap(await db('photo_categories').insert({ + event_id: eventId, name: 'Ceremony', slug: 'cs-ceremony', is_global: false, created_at: new Date().toISOString(), + }).returning('id')); + globalCategoryId = unwrap(await db('photo_categories').insert({ + event_id: null, name: 'Portraits', slug: 'cs-portraits', is_global: true, created_at: new Date().toISOString(), + }).returning('id')); + foreignCategoryId = unwrap(await db('photo_categories').insert({ + event_id: otherEventId, name: 'Reception', slug: 'cs-reception', is_global: false, created_at: new Date().toISOString(), + }).returning('id')); + + photoId = unwrap(await db('photos').insert({ + event_id: eventId, + filename: 'shot.jpg', + path: 'cat-scope-event/shot.jpg', + type: 'individual', + uploaded_at: new Date().toISOString(), + }).returning('id')); + + const superRole = await db('roles').where({ name: 'super_admin' }).first(); + const rootId = unwrap(await db('admin_users').insert({ + username: 'cat-scope-admin', + email: 'cat-scope-admin@example.com', + password_hash: await bcrypt.hash('CatScope123', 4), + role_id: superRole.id, + is_active: 1, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }).returning('id')); + adminToken = jwt.sign( + { id: rootId, username: 'cat-scope-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' } + ); + + app = express(); + app.use(express.json()); + app.use('/api/admin/photos', require('../../src/routes/adminPhotos')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + beforeEach(async () => { + await db('photos').where({ id: photoId }).update({ category_id: null }); + }); + + it('PATCH rejects a category belonging to another event', async () => { + const res = await patchCategory(foreignCategoryId); + expect(res.status).toBe(400); + expect(res.body.error).toBe(`Unknown or out-of-scope category_id ${foreignCategoryId}`); + expect(await storedCategoryId()).toBeNull(); + }); + + it('PATCH rejects a category id that does not exist at all', async () => { + const res = await patchCategory(999999); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Unknown or out-of-scope category_id 999999'); + expect(await storedCategoryId()).toBeNull(); + }); + + it('PATCH accepts this event\'s own category and a global one', async () => { + expect((await patchCategory(ownCategoryId)).status).toBe(200); + expect(await storedCategoryId()).toBe(ownCategoryId); + + expect((await patchCategory(globalCategoryId)).status).toBe(200); + expect(await storedCategoryId()).toBe(globalCategoryId); + }); + + it('bulk-update rejects a category belonging to another event', async () => { + const res = await bulkUpdateCategory(foreignCategoryId); + expect(res.status).toBe(400); + expect(res.body.error).toBe(`Unknown or out-of-scope category_id ${foreignCategoryId}`); + expect(await storedCategoryId()).toBeNull(); + }); + + it('bulk-update accepts this event\'s own category and a global one', async () => { + expect((await bulkUpdateCategory(ownCategoryId)).status).toBe(200); + expect(await storedCategoryId()).toBe(ownCategoryId); + + expect((await bulkUpdateCategory(globalCategoryId)).status).toBe(200); + expect(await storedCategoryId()).toBe(globalCategoryId); + }); + + it('still clears the category for 0 / individual without a scope lookup', async () => { + await db('photos').where({ id: photoId }).update({ category_id: ownCategoryId }); + expect((await patchCategory('0')).status).toBe(200); + expect(await storedCategoryId()).toBeNull(); + + await db('photos').where({ id: photoId }).update({ category_id: ownCategoryId }); + expect((await bulkUpdateCategory('individual')).status).toBe(200); + expect(await storedCategoryId()).toBeNull(); + }); +}); diff --git a/backend/__tests__/routes/adminPhotoUploadVideoSizeLimit.test.js b/backend/__tests__/routes/adminPhotoUploadVideoSizeLimit.test.js new file mode 100644 index 00000000..e9bd14cc --- /dev/null +++ b/backend/__tests__/routes/adminPhotoUploadVideoSizeLimit.test.js @@ -0,0 +1,202 @@ +/** + * Separate per-file cap for videos (general_max_video_size_mb) and temp-file + * cleanup on rejected uploads. + * + * `general_max_file_size_mb` was a single cap for photos AND videos, so with + * the 50MB default an admin could not upload a normal clip through the Photos + * tab without raising a limit that also governs photos. Videos now have their + * own cap; multer's (type-blind) limit is the larger of the two and the + * per-kind decision happens after multer, where the MIME type is known. + * + * Pins: + * - a video between the photo cap and the video cap gets past the size gate + * - a photo is still held to the photo cap even though multer streamed + * against the (larger) video cap + * - a video over the video cap is rejected naming the video cap + * - both caps are read per request + * - a rejected upload leaves nothing behind in the temp directory + */ + +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-video-size-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'video-size-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-video-size-storage-')); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +const SLUG = 'video-size-test-event'; +// Resolved lazily: bootCrmDb() repoints STORAGE_PATH at its own tmp dir, so a +// path captured at module load is not the one the route uploads into. +const tempRoot = () => path.join(process.env.STORAGE_PATH, 'temp'); + +describe('admin upload per-file video size limit (general_max_video_size_mb)', () => { + let db; + let cleanup; + let app; + let eventId; + let adminToken; + let uploadSettings; + + const setSetting = async (key, value) => { + await db('app_settings') + .insert({ + setting_key: key, + setting_value: JSON.stringify(value), + setting_type: 'general', + updated_at: new Date().toISOString(), + }) + .onConflict('setting_key') + .merge({ setting_value: JSON.stringify(value) }); + }; + + const setCaps = async ({ photoMb, videoMb }) => { + await setSetting('general_max_file_size_mb', photoMb); + await setSetting('general_max_video_size_mb', videoMb); + uploadSettings.clearMaxFileSizeCache(); + uploadSettings.clearMaxVideoSizeCache(); + }; + + const postUpload = (bytes, filename, contentType) => request(app) + .post(`/api/admin/photos/${eventId}/upload`) + .set('Authorization', `Bearer ${adminToken}`) + .attach('photos', Buffer.alloc(bytes, 0x41), { filename, contentType }); + + const postVideo = (bytes) => postUpload(bytes, 'clip.mp4', 'video/mp4'); + const postPhoto = (bytes) => postUpload(bytes, 'shot.jpg', 'image/jpeg'); + + // res.on('finish') cleanup is async, so give it a moment to land. + const tempEntriesAfterSettle = async () => { + let entries = []; + for (let i = 0; i < 40; i++) { + entries = fs.existsSync(tempRoot()) ? fs.readdirSync(tempRoot()) : []; + if (entries.length === 0) return entries; + await new Promise((resolve) => setTimeout(resolve, 25)); + } + return entries; + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const inserted = await db('events').insert({ + slug: SLUG, + event_type: 'wedding', + event_name: 'Video Size Test', + event_date: '2026-09-01', + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: `/gallery/${SLUG}/share`, + share_token: 'video-size-share', + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + eventId = inserted[0]?.id ?? inserted[0]; + + const superRole = await db('roles').where({ name: 'super_admin' }).first(); + const [rootId] = await db('admin_users').insert({ + username: 'video-size-admin', + email: 'video-size-admin@example.com', + password_hash: await bcrypt.hash('VideoSize123', 4), + role_id: superRole.id, + is_active: 1, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }).returning('id').then((r) => [r[0]?.id || r[0]]); + adminToken = jwt.sign( + { id: rootId, username: 'video-size-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' } + ); + + uploadSettings = require('../../src/services/uploadSettings'); + // mp4 has to be an allowed type for the video to reach the size gate. + await setSetting('general_allowed_file_types', 'jpg,jpeg,png,webp,mp4'); + uploadSettings.clearAllowedTypesCache(); + + app = express(); + app.use(express.json()); + app.use('/api/admin/photos', require('../../src/routes/adminPhotos')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('lets a video past the size gate that the photo cap would have rejected', async () => { + await setCaps({ photoMb: 1, videoMb: 10 }); + const res = await postVideo(2 * 1024 * 1024); + // Junk bytes, so it still fails on the content check — that is the point: + // the failure is no longer about size. + expect(res.status).toBe(400); + expect(res.body.error).toBe('File content does not match declared type: clip.mp4'); + }); + + it('still holds a photo to the photo cap even though multer streamed against the video cap', async () => { + await setCaps({ photoMb: 1, videoMb: 10 }); + const res = await postPhoto(2 * 1024 * 1024); + expect(res.status).toBe(400); + expect(res.body.error).toBe('File too large. Maximum size is 1 MB per file.'); + }); + + it('rejects a video over the video cap, naming the video cap', async () => { + // Video cap deliberately BELOW the photo cap, so multer's limit is the + // photo cap and only the MIME-aware gate can reject this. + await setCaps({ photoMb: 10, videoMb: 2 }); + const res = await postVideo(3 * 1024 * 1024); + expect(res.status).toBe(400); + expect(res.body.error).toBe('File too large. Maximum size is 2 MB per file.'); + }); + + it('reads the video cap per request, so raising it takes effect immediately', async () => { + await setCaps({ photoMb: 1, videoMb: 1 }); + expect((await postVideo(2 * 1024 * 1024)).body.error) + .toBe('File too large. Maximum size is 1 MB per file.'); + + await setCaps({ photoMb: 1, videoMb: 10 }); + const res = await postVideo(2 * 1024 * 1024); + expect(res.body.error).toBe('File content does not match declared type: clip.mp4'); + }); + + it('defaults the video cap to 500MB when the setting is absent', async () => { + await db('app_settings').where({ setting_key: 'general_max_video_size_mb' }).del(); + await setSetting('general_max_file_size_mb', 1); + uploadSettings.clearMaxFileSizeCache(); + uploadSettings.clearMaxVideoSizeCache(); + + expect(await uploadSettings.getMaxVideoSizeMb()).toBe(500); + const res = await postVideo(2 * 1024 * 1024); + expect(res.body.error).toBe('File content does not match declared type: clip.mp4'); + }); + + it('leaves no temp files behind when an upload is rejected', async () => { + await setCaps({ photoMb: 1, videoMb: 10 }); + + // Rejected on size (the MIME-aware gate)… + expect((await postPhoto(2 * 1024 * 1024)).status).toBe(400); + expect(await tempEntriesAfterSettle()).toEqual([]); + + // …and rejected on content, with two files in the batch so the shared + // per-request temp directory is exercised. + const res = await request(app) + .post(`/api/admin/photos/${eventId}/upload`) + .set('Authorization', `Bearer ${adminToken}`) + .attach('photos', Buffer.alloc(1024, 0x41), { filename: 'a.jpg', contentType: 'image/jpeg' }) + .attach('photos', Buffer.alloc(1024, 0x41), { filename: 'b.jpg', contentType: 'image/jpeg' }); + expect(res.status).toBe(400); + expect(await tempEntriesAfterSettle()).toEqual([]); + }); +}); diff --git a/backend/__tests__/services/uploadSettingsMaxFileSize.test.js b/backend/__tests__/services/uploadSettingsMaxFileSize.test.js index 671c3978..1e08edbe 100644 --- a/backend/__tests__/services/uploadSettingsMaxFileSize.test.js +++ b/backend/__tests__/services/uploadSettingsMaxFileSize.test.js @@ -54,6 +54,34 @@ test('clamps a nonsense value to the default and caps absurd values at the ceili expect(await svc.getMaxFileSizeMb()).toBe(svc.MAX_ALLOWED_FILE_SIZE_MB); // ceiling }); +test('videos read their own key and default to 500MB, independent of the photo cap', async () => { + await setLimit(50); + expect(await svc.getMaxVideoSizeMb()).toBe(500); + expect(await svc.getMaxVideoSizeBytes()).toBe(500 * 1024 * 1024); + + await db('app_settings') + .insert({ setting_key: 'general_max_video_size_mb', setting_value: JSON.stringify(2000), setting_type: 'general', updated_at: new Date() }) + .onConflict('setting_key').merge({ setting_value: JSON.stringify(2000) }); + svc.clearMaxVideoSizeCache(); + + expect(await svc.getMaxVideoSizeMb()).toBe(2000); + expect(await svc.getMaxFileSizeMb()).toBe(50); // photo cap untouched +}); + +test('clamps the video cap: nonsense falls back to 500, absurd hits the ceiling', async () => { + const setVideoLimit = async (mb) => { + await db('app_settings') + .insert({ setting_key: 'general_max_video_size_mb', setting_value: JSON.stringify(mb), setting_type: 'general', updated_at: new Date() }) + .onConflict('setting_key').merge({ setting_value: JSON.stringify(mb) }); + svc.clearMaxVideoSizeCache(); + }; + + await setVideoLimit(0); + expect(await svc.getMaxVideoSizeMb()).toBe(500); + await setVideoLimit(99_999_999); + expect(await svc.getMaxVideoSizeMb()).toBe(svc.MAX_ALLOWED_FILE_SIZE_MB); +}); + test('caches for the TTL — a mid-window DB change is not seen until the cache is cleared', async () => { await setLimit(200); expect(await svc.getMaxFileSizeMb()).toBe(200); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index e27451b4..226f75b8 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -17,7 +17,14 @@ const { COLOR_LABELS, dominantColorLabel, SHARED_COLOR_LABEL_IDENTITY } = requir const feedbackService = require('../services/feedbackService'); const photoAdminMarksService = require('../services/photoAdminMarksService'); const { validateUploadedFiles } = require('../middleware/uploadValidation'); -const { getMaxFilesPerUpload, getAllowedMimeTypes, getMaxFileSizeBytes, DEFAULT_MAX_FILE_SIZE_MB } = require('../services/uploadSettings'); +const { + getMaxFilesPerUpload, + getAllowedMimeTypes, + getMaxFileSizeBytes, + getMaxVideoSizeBytes, + DEFAULT_MAX_FILE_SIZE_MB, + DEFAULT_MAX_VIDEO_SIZE_MB +} = require('../services/uploadSettings'); const { processUploadedPhotos } = require('../services/photoProcessor'); const chunkedUpload = require('../services/chunkedUploadService'); const watermarkGeneratorService = require('../services/watermarkGeneratorService'); @@ -32,24 +39,46 @@ const router = express.Router(); // Get storage path from environment or default const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); +// Resolve a numeric category id within the scope of one event: it must belong +// to that event or be a global category (#500 / #525 — the same contract the +// public v1 upload route enforces). Returns undefined for an out-of-scope id, +// which every caller turns into a 400 rather than silently filing the photo +// under another event's category. +const findScopedCategory = (eventId, categoryId) => db('photo_categories') + .where({ id: categoryId }) + .andWhere(function () { + this.where({ event_id: eventId }).orWhere('is_global', true); + }) + .first(); + +const outOfScopeCategoryError = (categoryId) => ({ + error: `Unknown or out-of-scope category_id ${categoryId}` +}); + // Configure multer for file uploads // IMPORTANT: Using synchronous functions to prevent file corruption const storage = multer.diskStorage({ destination: (req, file, cb) => { logger.info('Multer destination called for file:', file.originalname); - + // We'll validate the event exists in the route handler - // For now, just create a temp destination - const tempPath = path.join(getStoragePath(), 'temp', `upload_${Date.now()}_${Math.random().toString(36).substring(7)}`); - - // Create directory synchronously - require('fs').mkdirSync(tempPath, { recursive: true }); - logger.info('Temp destination path:', tempPath); - - // Store temp path for cleanup - req.tempUploadPath = tempPath; - - cb(null, tempPath); + // For now, just create a temp destination. + // One directory per REQUEST, not per file: this callback runs for every + // file and used to overwrite req.tempUploadPath each time, so cleanup + // only ever removed the last file's directory and a multi-file upload + // left the rest behind. Temp filenames are already collision-proof. + if (!req.tempUploadPath) { + const tempPath = path.join(getStoragePath(), 'temp', `upload_${Date.now()}_${Math.random().toString(36).substring(7)}`); + + // Create directory synchronously + require('fs').mkdirSync(tempPath, { recursive: true }); + logger.info('Temp destination path:', tempPath); + + // Store temp path for cleanup + req.tempUploadPath = tempPath; + } + + cb(null, req.tempUploadPath); }, filename: (req, file, cb) => { logger.info('Multer filename called for file:', file.originalname); @@ -108,16 +137,52 @@ const resolveAllowedTypes = async (req, res, next) => { // Dynamic content validator middleware that reads allowed types from req const validateUploadContent = async (req, res, next) => { const allowedTypes = req.allowedMimeTypes || ['image/jpeg', 'image/png', 'image/webp']; + const photoCapBytes = req.maxFileSizeBytes || DEFAULT_MAX_FILE_SIZE_MB * 1024 * 1024; + const videoCapBytes = req.maxVideoSizeBytes || DEFAULT_MAX_VIDEO_SIZE_MB * 1024 * 1024; + const capFor = (file) => (isVideoMimeType(file.mimetype) ? videoCapBytes : photoCapBytes); + + // Photos and videos have separate caps (general_max_file_size_mb / + // general_max_video_size_mb), but multer's limit is global — it streamed + // against the larger of the two because it can't branch on MIME type. So + // the per-kind decision has to happen here, where the type is known, + // otherwise a 50MB photo cap would be silently raised to the video cap. + const oversized = (req.files || []).find((file) => file.size > capFor(file)); + if (oversized) { + const capMb = Math.floor(capFor(oversized) / (1024 * 1024)); + return res.status(400).json({ error: `File too large. Maximum size is ${capMb} MB per file.` }); + } + const validator = createFileUploadValidator({ allowedTypes, - // Same per-request cap multer streamed against, so the two layers can't - // disagree; this one names the offending file in the 400. - maxFileSize: req.maxFileSizeBytes || DEFAULT_MAX_FILE_SIZE_MB * 1024 * 1024, + // Same per-request caps as above, so the two layers can't disagree. + maxFileSize: photoCapBytes, + maxVideoFileSize: videoCapBytes, validateContent: true }); return validator(req, res, next); }; +// Remove the multer temp directory on every exit path — success, validation +// 4xx, multer error, server 5xx or a client disconnect. Registered BEFORE +// multer runs (the closure reads req.tempUploadPath lazily) because a +// rejected upload never reaches the final handler, where this used to live: +// every rejection leaked its temp directory and the file inside it. +const registerTempUploadCleanup = (req, res, next) => { + let cleanupDone = false; + const cleanupTempDir = async () => { + if (cleanupDone || !req.tempUploadPath) return; + cleanupDone = true; + try { + await fs.rm(req.tempUploadPath, { recursive: true, force: true }); + } catch (e) { + logger.error('Failed to clean up temp upload directory:', e); + } + }; + res.on('finish', cleanupTempDir); + res.on('close', cleanupTempDir); + next(); +}; + // Request timeout middleware for uploads const uploadTimeout = (timeout = 300000) => { // 5 minutes default return (req, res, next) => { @@ -140,19 +205,25 @@ const uploadTimeout = (timeout = 300000) => { // 5 minutes default // Upload photos for an event // Max file count and max file size are configurable via general settings -router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), requireEventOwnership, uploadTimeout(600000), resolveAllowedTypes, async (req, res, next) => { // 10 minute timeout +router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), requireEventOwnership, uploadTimeout(600000), resolveAllowedTypes, registerTempUploadCleanup, async (req, res, next) => { // 10 minute timeout let maxFilesPerUpload; let maxFileSizeBytes; + let maxVideoSizeBytes; try { maxFilesPerUpload = await getMaxFilesPerUpload(); maxFileSizeBytes = await getMaxFileSizeBytes(); + maxVideoSizeBytes = await getMaxVideoSizeBytes(); } catch (error) { return errorResponse(res, error, 500, 'Unable to determine upload limits'); } req.maxFileSizeBytes = maxFileSizeBytes; - const maxFileSizeMb = Math.floor(maxFileSizeBytes / (1024 * 1024)); + req.maxVideoSizeBytes = maxVideoSizeBytes; + // multer's limit is global, so it has to be the larger of the two caps; + // validateUploadContent then holds each file to the cap for its own kind. + const multerLimitBytes = Math.max(maxFileSizeBytes, maxVideoSizeBytes); + const maxFileSizeMb = Math.floor(multerLimitBytes / (1024 * 1024)); - createUpload(maxFileSizeBytes).array('photos', maxFilesPerUpload)(req, res, (err) => { + createUpload(multerLimitBytes).array('photos', maxFilesPerUpload)(req, res, (err) => { if (err) { logger.error('Multer error:', err); if (err instanceof multer.MulterError) { @@ -169,24 +240,9 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r next(); }); }, validateUploadContent, validateUploadedFiles, async (req, res) => { - // Single cleanup site for the multer temp directory — runs on every - // exit path (success, validation 4xx, server 5xx, multer error). The - // previous code had three inline cleanup blocks for individual early - // returns and missed the success path entirely, leaving an empty - // per-request directory behind on every successful upload (#357 review). - let tempCleanupDone = false; - const cleanupTempDir = async () => { - if (tempCleanupDone || !req.tempUploadPath) return; - tempCleanupDone = true; - try { - await fs.rm(req.tempUploadPath, { recursive: true, force: true }); - } catch (e) { - logger.error('Failed to clean up temp upload directory:', e); - } - }; - res.on('finish', cleanupTempDir); - res.on('close', cleanupTempDir); - + // Temp-directory cleanup is registered by registerTempUploadCleanup above, + // before multer runs, so it also covers the exit paths that never reach + // this handler (multer errors and validation 4xx). try { const { eventId } = req.params; const { category_id, replace_by_name, match_mode } = req.body; @@ -259,16 +315,9 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r // belong to a different event. The v1 route rejects out-of-scope ids // with 400; mirror that here so admin and v1 stay consistent. if (parsedCategoryId && !isNaN(parsedCategoryId)) { - const category = await db('photo_categories') - .where({ id: parsedCategoryId }) - .andWhere(function () { - this.where({ event_id: event.id }).orWhere('is_global', true); - }) - .first(); + const category = await findScopedCategory(event.id, parsedCategoryId); if (!category) { - return res.status(400).json({ - error: `Unknown or out-of-scope category_id ${parsedCategoryId}` - }); + return res.status(400).json(outOfScopeCategoryError(parsedCategoryId)); } categoryName = category.slug || category.name.toLowerCase().replace(/\s+/g, '_'); // Use category slug for type determination @@ -858,6 +907,13 @@ router.patch('/:eventId/photos/:photoId', adminAuth, requirePermission('photos.e // NaN (unparseable input) already fell through to null and still does. const numericCategoryId = parseInt(category_id, 10); if (numericCategoryId > 0) { + // Same scope check the upload route runs: without it any positive id + // was accepted, so a photo could be moved into another event's + // category (the grid then never shows it under any filter). + const category = await findScopedCategory(parseInt(eventId, 10), numericCategoryId); + if (!category) { + return res.status(400).json(outOfScopeCategoryError(numericCategoryId)); + } updateData.category_id = numericCategoryId; } else { updateData.category_id = null; @@ -1043,6 +1099,11 @@ router.post('/:eventId/photos/bulk-update', adminAuth, requirePermission('photos // (0/negative mean "no category" — see the PATCH route above) const numericCategoryId = parseInt(updates.category_id, 10); if (numericCategoryId > 0) { + // Scope check, as on the PATCH and upload routes above. + const category = await findScopedCategory(parseInt(eventId, 10), numericCategoryId); + if (!category) { + return res.status(400).json(outOfScopeCategoryError(numericCategoryId)); + } updateData.category_id = numericCategoryId; } else { updateData.category_id = null; @@ -1303,6 +1364,10 @@ router.get('/:eventId/photos', adminAuth, requirePermission('photos.view'), requ // Always expose a thumbnail URL; backend will generate on demand if missing thumbnail_url: `/admin/photos/${eventId}/thumbnail/${photo.id}`, type: photo.type, + // Guest visibility (#172). This explicit mapper never included it, + // so the admin grid's "Hidden" badge could never render and a photo + // hidden from clients looked identical to a visible one (QA warning). + visibility: photo.visibility === 'hidden' ? 'hidden' : 'visible', category_id: photo.category_id || photo.type, category_name: photo.pc_name || (photo.type === 'individual' ? 'Individual Photos' : 'Collages'), category_slug: photo.pc_slug || photo.type, diff --git a/backend/src/routes/adminSettings.js b/backend/src/routes/adminSettings.js index 65b3c606..64cd07fe 100644 --- a/backend/src/routes/adminSettings.js +++ b/backend/src/routes/adminSettings.js @@ -28,7 +28,7 @@ const { errorResponse } = require('../utils/routeHelpers'); const { measureLocalStorageUsage } = require('../services/localStorageUsage'); const logger = require('../utils/logger'); const router = express.Router(); -const { clearMaxFilesPerUploadCache, MAX_ALLOWED_FILES_PER_UPLOAD, clearMaxFileSizeCache, MAX_ALLOWED_FILE_SIZE_MB } = require('../services/uploadSettings'); +const { clearMaxFilesPerUploadCache, MAX_ALLOWED_FILES_PER_UPLOAD, clearMaxFileSizeCache, clearMaxVideoSizeCache, MAX_ALLOWED_FILE_SIZE_MB } = require('../services/uploadSettings'); const watermarkService = require('../services/watermarkService'); const watermarkGeneratorService = require('../services/watermarkGeneratorService'); @@ -1459,6 +1459,23 @@ router.put('/general', adminAuth, requirePermission('settings.edit'), async (req settings.general_max_file_size_mb = normalizedValue; } + // Per-file size limit for videos (MB). Same bounds and same reasoning as + // the photo cap above — videos just get their own value so a 50MB photo + // limit doesn't also block every clip. + if (Object.prototype.hasOwnProperty.call(settings, 'general_max_video_size_mb')) { + uploadLimitTouched = true; + const rawValue = Number(settings.general_max_video_size_mb); + const normalizedValue = Number.isFinite(rawValue) ? Math.floor(rawValue) : NaN; + + if (!Number.isInteger(normalizedValue) || normalizedValue < 1 || normalizedValue > MAX_ALLOWED_FILE_SIZE_MB) { + return res.status(400).json({ + error: `general_max_video_size_mb must be an integer between 1 and ${MAX_ALLOWED_FILE_SIZE_MB}` + }); + } + + settings.general_max_video_size_mb = normalizedValue; + } + if (publicSiteKeysTouched) { if (Object.prototype.hasOwnProperty.call(settings, 'general_public_site_custom_css')) { settings.general_public_site_custom_css = sanitizeCss(settings.general_public_site_custom_css || ''); @@ -1516,6 +1533,7 @@ router.put('/general', adminAuth, requirePermission('settings.edit'), async (req if (uploadLimitTouched) { clearMaxFilesPerUploadCache(); clearMaxFileSizeCache(); + clearMaxVideoSizeCache(); } if (Object.prototype.hasOwnProperty.call(settings, 'general_short_gallery_urls')) { clearShareLinkSettingsCache(); diff --git a/backend/src/services/uploadSettings.js b/backend/src/services/uploadSettings.js index 13062298..aca9cb14 100644 --- a/backend/src/services/uploadSettings.js +++ b/backend/src/services/uploadSettings.js @@ -11,12 +11,21 @@ const CACHE_TTL_MS = 60_000; const DEFAULT_MAX_FILE_SIZE_MB = 50; const MAX_ALLOWED_FILE_SIZE_MB = 10 * 1024; // 10 GB — matches the admin path's cap +// Separate per-file cap for videos (general_max_video_size_mb). A single cap +// for both meant a 50 MB photo limit also blocked every normal clip, so an +// admin had to raise the photo limit to upload a video. 500 MB is roughly a +// few minutes of phone footage; the same 10 GB hard ceiling applies. +const DEFAULT_MAX_VIDEO_SIZE_MB = 500; + let cachedValue = DEFAULT_MAX_FILES_PER_UPLOAD; let cacheExpiresAt = 0; let cachedFileSizeMb = DEFAULT_MAX_FILE_SIZE_MB; let fileSizeCacheExpiresAt = 0; +let cachedVideoSizeMb = DEFAULT_MAX_VIDEO_SIZE_MB; +let videoSizeCacheExpiresAt = 0; + // Map of file extension to MIME type(s) const EXTENSION_TO_MIME = { 'jpg': 'image/jpeg', @@ -118,12 +127,12 @@ const clearMaxFilesPerUploadCache = () => { cacheExpiresAt = 0; }; -const normalizeFileSizeMb = (value) => { +const normalizeFileSizeMb = (value, fallbackMb = DEFAULT_MAX_FILE_SIZE_MB) => { if (typeof value !== 'number' || !Number.isFinite(value) || value <= 0) { - return DEFAULT_MAX_FILE_SIZE_MB; + return fallbackMb; } const intValue = Math.floor(value); - if (intValue < 1) return DEFAULT_MAX_FILE_SIZE_MB; + if (intValue < 1) return fallbackMb; if (intValue > MAX_ALLOWED_FILE_SIZE_MB) return MAX_ALLOWED_FILE_SIZE_MB; return intValue; }; @@ -164,6 +173,43 @@ const clearMaxFileSizeCache = () => { fileSizeCacheExpiresAt = 0; }; +/** + * Per-file upload size limit for videos in MB (general_max_video_size_mb). + * Same read/cache/clamp contract as getMaxFileSizeMb(); falls back to the + * video default (not the photo one) when the setting is absent. + */ +const getMaxVideoSizeMb = async () => { + if (Date.now() < videoSizeCacheExpiresAt) { + return cachedVideoSizeMb; + } + + try { + const setting = await db('app_settings') + .where({ setting_key: 'general_max_video_size_mb' }) + .first(); + + const parsedValue = normalizeFileSizeMb(parseSettingValue(setting), DEFAULT_MAX_VIDEO_SIZE_MB); + cachedVideoSizeMb = parsedValue; + videoSizeCacheExpiresAt = Date.now() + CACHE_TTL_MS; + return parsedValue; + } catch (error) { + logger.error('Failed to read max video size setting:', error.message); + cachedVideoSizeMb = DEFAULT_MAX_VIDEO_SIZE_MB; + videoSizeCacheExpiresAt = Date.now() + CACHE_TTL_MS; + return DEFAULT_MAX_VIDEO_SIZE_MB; + } +}; + +/** Per-file video size limit in bytes — convenience for multer `limits.fileSize`. */ +const getMaxVideoSizeBytes = async () => { + const mb = await getMaxVideoSizeMb(); + return mb * 1024 * 1024; +}; + +const clearMaxVideoSizeCache = () => { + videoSizeCacheExpiresAt = 0; +}; + /** * Convert a comma-separated list of file extensions into an array of MIME types. * Unknown extensions are silently ignored. @@ -232,6 +278,9 @@ module.exports = { getMaxFileSizeMb, getMaxFileSizeBytes, clearMaxFileSizeCache, + getMaxVideoSizeMb, + getMaxVideoSizeBytes, + clearMaxVideoSizeCache, getAllowedMimeTypes, clearAllowedTypesCache, extensionsToMimeTypes, @@ -239,6 +288,7 @@ module.exports = { DEFAULT_MAX_FILES_PER_UPLOAD, MAX_ALLOWED_FILES_PER_UPLOAD, DEFAULT_MAX_FILE_SIZE_MB, + DEFAULT_MAX_VIDEO_SIZE_MB, MAX_ALLOWED_FILE_SIZE_MB, DEFAULT_ALLOWED_FILE_TYPES }; diff --git a/backend/src/utils/fileSecurityUtils.js b/backend/src/utils/fileSecurityUtils.js index a36f1e47..e80f0976 100644 --- a/backend/src/utils/fileSecurityUtils.js +++ b/backend/src/utils/fileSecurityUtils.js @@ -223,50 +223,72 @@ function createFileUploadValidator(options = {}) { const { allowedTypes = ['image/jpeg', 'image/png', 'image/webp'], maxFileSize = 50 * 1024 * 1024, // 50MB default + // Videos carry their own per-file cap (general_max_video_size_mb). + // Defaults to the photo cap so callers that don't split the two behave + // exactly as before. + maxVideoFileSize = maxFileSize, validateContent = true } = options; - + + const isVideoType = (mimetype) => typeof mimetype === 'string' && mimetype.startsWith('video/'); + return async (req, res, next) => { + // Every exit below rejects the whole request, so nothing downstream will + // ever read what multer already wrote to disk. Drop those files here or + // they leak: the routes register their temp-dir cleanup for the success + // path, which a rejection never reaches. + const discardUploadedFiles = async () => { + await Promise.all((req.files || []).map(async (file) => { + if (!file.path) return; + try { + await fs.unlink(file.path); + } catch (err) { + if (err.code !== 'ENOENT') { + logger.error('Error removing rejected upload:', err); + } + } + })); + }; + try { if (!req.files || req.files.length === 0) { return next(); } - + for (const file of req.files) { // Validate file type if (!validateFileType(file.originalname, file.mimetype, allowedTypes)) { - return res.status(400).json({ - error: `Invalid file type: ${file.originalname}. Allowed types: ${allowedTypes.join(', ')}` + await discardUploadedFiles(); + return res.status(400).json({ + error: `Invalid file type: ${file.originalname}. Allowed types: ${allowedTypes.join(', ')}` }); } - - // Validate file size - if (file.size > maxFileSize) { - return res.status(400).json({ - error: `File too large: ${file.originalname}. Maximum size: ${maxFileSize / 1024 / 1024}MB` + + // Validate file size against the cap for this kind of file + const sizeLimit = isVideoType(file.mimetype) ? maxVideoFileSize : maxFileSize; + if (file.size > sizeLimit) { + await discardUploadedFiles(); + return res.status(400).json({ + error: `File too large: ${file.originalname}. Maximum size: ${sizeLimit / 1024 / 1024}MB` }); } - + // Validate file content if enabled if (validateContent && file.path) { const isValidContent = await validateFileContent(file.path, file.mimetype); if (!isValidContent) { - // Remove the file if content doesn't match - try { - await fs.unlink(file.path); - } catch (err) { - logger.error('Error removing invalid file:', err); - } - return res.status(400).json({ - error: `File content does not match declared type: ${file.originalname}` + await discardUploadedFiles(); + return res.status(400).json({ + error: `File content does not match declared type: ${file.originalname}` }); } } } - + next(); } catch (error) { logger.error('File validation error:', error); + await discardUploadedFiles(); res.status(500).json({ error: 'File validation failed' }); } }; diff --git a/frontend/src/components/admin/PhotoUpload.tsx b/frontend/src/components/admin/PhotoUpload.tsx index b3a7d0a1..8814ff45 100644 --- a/frontend/src/components/admin/PhotoUpload.tsx +++ b/frontend/src/components/admin/PhotoUpload.tsx @@ -127,6 +127,17 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl ? Number(settings?.general_max_file_size_mb) : 50; + // Videos have their own per-file cap; the photo cap would otherwise block + // every normal clip. Backend enforces the same two values per request. + const maxVideoSizeMb = Number.isFinite(Number(settings?.general_max_video_size_mb)) + ? Number(settings?.general_max_video_size_mb) + : 500; + + const videoUploadsAllowed = allowedMimeTypes.some((type) => type.startsWith('video/')); + + const sizeLimitMbFor = (file: File) => + (file.type.startsWith('video/') ? maxVideoSizeMb : maxFileSizeMb); + const remainingSlots = Math.max(maxFilesPerUpload - selectedFiles.length, 0); const [isDragOver, setIsDragOver] = useState(false); @@ -135,7 +146,17 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl // the dashed-border zone looked draggable but silently fell through to // the browser's default "open the file in a new tab" behaviour. const addFiles = (incoming: File[]) => { - const imageFiles = incoming.filter((file) => allowedMimeTypes.includes(file.type)); + const imageFiles = incoming.filter((file) => { + if (!allowedMimeTypes.includes(file.type)) return false; + // Pre-flight size check, mirroring the guest uploader: without it the + // admin streams the whole oversized file before the backend 400s it. + const limitMb = sizeLimitMbFor(file); + if (file.size > limitMb * 1024 * 1024) { + toast.error(t('upload.fileTooLarge', { name: file.name, limit: limitMb })); + return false; + } + return true; + }); if (imageFiles.length === 0) return; const totalFiles = selectedFiles.length + imageFiles.length; @@ -522,6 +543,11 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl

{t('upload.fileRequirements', { formats: formatsLabel, limit: maxFilesPerUpload, sizeLimit: maxFileSizeMb })}

+ {videoUploadsAllowed && ( +

+ {t('upload.videoSizeLimit', 'Videos: max {{sizeLimit}}MB per file', { sizeLimit: maxVideoSizeMb })} +

+ )}

{ + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, opts?: any) => (opts && opts.name ? `${key}:${opts.name}:${opts.limit}` : key), + }), + }; +}); + +const toastError = vi.fn(); +vi.mock('react-toastify', () => ({ + toast: { warning: vi.fn(), info: vi.fn(), error: (...a: any[]) => toastError(...a), success: vi.fn() }, +})); + +const postMock = vi.fn(); +vi.mock('../../../config/api', () => ({ api: { post: (...a: any[]) => postMock(...a), get: vi.fn() } })); + +vi.mock('../../../hooks/useUploadProgress', () => ({ + useUploadProgress: () => ({ + snapshots: {}, + error: null, + aggregate: { total: 0, pending: 0, processing: 0, complete: 0, failed: 0, failedPhotos: [], isComplete: false, isReady: true }, + }), +})); + +vi.mock('../../../services/categories.service', () => ({ + categoriesService: { getEventCategories: vi.fn().mockResolvedValue([]) }, +})); +vi.mock('../../../services/settings.service', () => ({ + settingsService: { + getAllSettings: vi.fn().mockResolvedValue({ + general_allowed_file_types: 'jpg,jpeg,png,webp,mp4', + general_max_file_size_mb: 1, + general_max_video_size_mb: 5, + }), + }, +})); + +const renderWithClient = (ui: ReactElement) => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render({ui}); +}; + +const file = (name: string, type: string, mb: number) => + new File([new Uint8Array(Math.round(mb * 1024 * 1024))], name, { type }); + +const select = async (container: HTMLElement, user: ReturnType, f: File) => { + const input = container.querySelector('input[type="file"]') as HTMLInputElement; + await user.upload(input, f); +}; + +describe('PhotoUpload pre-flight size guard', () => { + beforeEach(() => { + toastError.mockReset(); + postMock.mockReset(); + }); + + it('rejects a photo over the photo cap before it is selected', async () => { + const user = userEvent.setup(); + const { container } = renderWithClient(); + // Wait for the settings query so the caps are not still at their defaults. + await waitFor(() => expect(screen.getByText('upload.videoSizeLimit')).toBeInTheDocument()); + + await select(container, user, file('huge.png', 'image/png', 2)); + + expect(toastError).toHaveBeenCalledWith('upload.fileTooLarge:huge.png:1'); + expect(screen.queryByText('huge.png')).not.toBeInTheDocument(); + expect(postMock).not.toHaveBeenCalled(); + }); + + it('accepts a video that exceeds the photo cap but fits the video cap', async () => { + const user = userEvent.setup(); + const { container } = renderWithClient(); + await waitFor(() => expect(screen.getByText('upload.videoSizeLimit')).toBeInTheDocument()); + + await select(container, user, file('clip.mp4', 'video/mp4', 2)); + + expect(toastError).not.toHaveBeenCalled(); + expect(screen.getByText('clip.mp4')).toBeInTheDocument(); + }); + + it('rejects a video over the video cap', async () => { + const user = userEvent.setup(); + const { container } = renderWithClient(); + await waitFor(() => expect(screen.getByText('upload.videoSizeLimit')).toBeInTheDocument()); + + await select(container, user, file('long.mp4', 'video/mp4', 6)); + + expect(toastError).toHaveBeenCalledWith('upload.fileTooLarge:long.mp4:5'); + expect(screen.queryByText('long.mp4')).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/features/settings/__tests__/GeneralTab.siteUrl.test.tsx b/frontend/src/features/settings/__tests__/GeneralTab.siteUrl.test.tsx index 1bf87246..5ed7870c 100644 --- a/frontend/src/features/settings/__tests__/GeneralTab.siteUrl.test.tsx +++ b/frontend/src/features/settings/__tests__/GeneralTab.siteUrl.test.tsx @@ -23,6 +23,7 @@ const base: GeneralSettings = { site_url_stored: '', default_expiration_days: 30, max_file_size_mb: 50, + max_video_size_mb: 500, max_files_per_upload: 500, allowed_file_types: 'jpg,png', max_upload_batch_size_mb: 95, diff --git a/frontend/src/features/settings/hooks/useSettingsState.ts b/frontend/src/features/settings/hooks/useSettingsState.ts index 3d45d4e9..7717c55e 100644 --- a/frontend/src/features/settings/hooks/useSettingsState.ts +++ b/frontend/src/features/settings/hooks/useSettingsState.ts @@ -24,6 +24,9 @@ export interface GeneralSettings { site_url_stored: string; default_expiration_days: number; max_file_size_mb: number; + /** Videos get their own per-file cap — the photo cap would otherwise + * block every normal clip. */ + max_video_size_mb: number; max_files_per_upload: number; allowed_file_types: string; // #509 — re-added after the main-into-beta merge dropped it. @@ -137,6 +140,7 @@ export function useSettingsState() { site_url_stored: '', default_expiration_days: 30, max_file_size_mb: 50, + max_video_size_mb: 500, max_files_per_upload: 500, allowed_file_types: 'jpg,jpeg,png,gif,webp', max_upload_batch_size_mb: 95, @@ -235,6 +239,7 @@ export function useSettingsState() { site_url_stored: settings.general_site_url || '', default_expiration_days: toNumber(settings.general_default_expiration_days, 30), max_file_size_mb: toNumber(settings.general_max_file_size_mb, 50), + max_video_size_mb: toNumber(settings.general_max_video_size_mb, 500), max_files_per_upload: Math.min( MAX_FILES_PER_UPLOAD_LIMIT, Math.max(1, toNumber(settings.general_max_files_per_upload, 500)) diff --git a/frontend/src/features/settings/tabs/GeneralTab.tsx b/frontend/src/features/settings/tabs/GeneralTab.tsx index 7e5ddb5c..5c5e7141 100644 --- a/frontend/src/features/settings/tabs/GeneralTab.tsx +++ b/frontend/src/features/settings/tabs/GeneralTab.tsx @@ -171,6 +171,20 @@ export const GeneralTab: React.FC = ({ max="500" /> +

+ + setGeneralSettings(prev => ({ ...prev, max_video_size_mb: parseInt(e.target.value) || 500 }))} + min="1" + /> +

+ {t('settings.general.maxVideoSizeHelp', 'Separate per-file limit for video uploads, so photos can keep a smaller limit.')} +

+