fix(upload): scope category ids, stop temp-file leaks, split the video cap

Four related fixes on the admin upload/photo path.

B5 -- PATCH /photos/:photoId and POST /photos/bulk-update took any
parseInt(...) > 0 straight into the update with no existence or scope check,
so a photo could be moved into another event's category. The upload route
already validated `event_id = X OR is_global` per #500/#525; extracted that
query as findScopedCategory() and used it on all three routes so the 400 body
is byte-identical. 0/negative/'individual'/'collage'/null still clear without
a lookup, so the clear path costs no extra query.

B9 -- three distinct temp-file leaks, not one. The validator's size branch
never unlinked; the cleanup lived in the final handler, unreachable on any
400; and multer's `destination` callback runs per file and overwrote
req.tempUploadPath, so even the success path only ever removed the last
file's directory. Now: discardUploadedFiles() runs on every 4xx and the 500
(ENOENT tolerated, and files are only dropped when the whole request is being
rejected, so the passing path is untouched); cleanup registered before multer
so it also covers multer's own LIMIT_FILE_SIZE return; one directory per
request.

B8 -- the admin uploader filtered on MIME only, so an oversized file was
uploaded in full before the server's 400. Mirrors UserPhotoUpload's existing
per-file toast-and-drop.

C4 -- general_max_file_size_mb was a single cap for photos and videos, so the
50MB default meant admins could not upload ordinary video without also
raising the photo limit. Adds general_max_video_size_mb (default 500MB,
clamped by the same 10GB MAX_ALLOWED_FILE_SIZE_MB ceiling, read per request,
60s cache), editable in Settings -> General.

Photo uploads are protected from regressing by keeping multer's type-blind
limit at max(photoCap, videoCap) and moving the per-kind decision into
validateUploadContent, where file.mimetype exists. It 400s with the existing
message shape, so an oversized photo is still rejected with the identical
body it produced when multer did the rejecting.

Known gap: chunked-upload/init still applies the photo cap to video. Making
it video-aware would change an existing assertion that pins a 200MB video
init being rejected under a 1MB general cap. No component calls that path
today and the direction is strict rather than a bypass, so it is left as-is.
Guest video uploads still share the single cap in gallery.js.

Refs testplan REPORT.md B5, B8, B9, C4.
This commit is contained in:
Paul Nothaft
2026-09-02 09:43:10 +02:00
parent 57dd084763
commit 7c9baff751
12 changed files with 804 additions and 70 deletions
+111 -46
View File
@@ -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,
+19 -1
View File
@@ -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();
+53 -3
View File
@@ -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
};
+41 -19
View File
@@ -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' });
}
};