diff --git a/backend/__tests__/utils/photoContentType.test.js b/backend/__tests__/utils/photoContentType.test.js new file mode 100644 index 00000000..406cd196 --- /dev/null +++ b/backend/__tests__/utils/photoContentType.test.js @@ -0,0 +1,47 @@ +/** + * photos.mime_type is client-influenced (chunked uploads stored the declared + * type verbatim; the S3 importer stores whatever mime-types derives). Every + * serving route must go through resolvePhotoContentType so the header is + * always image/* or video/* and never the stored value as given. + */ +const fs = require('fs'); +const path = require('path'); +const { resolvePhotoContentType } = require('../../src/utils/photoContentType'); + +describe('resolvePhotoContentType', () => { + it('never echoes a non-media stored MIME', () => { + expect(resolvePhotoContentType({ filename: 'a.jpg', mime_type: 'text/html' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'text/html' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a.gif', mime_type: 'application/javascript' })).toBe('image/gif'); + }); + + it('never honours the scriptable svg / xml family or header-invalid values', () => { + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'image/svg+xml' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'image/x\r\nX-Injected: 1' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a.mp4', mime_type: 'video/mp4\r\nX: y' })).toBe('video/mp4'); + }); + + it('prefers the mapped extension for images and the stored type for videos', () => { + expect(resolvePhotoContentType({ filename: 'a.png', mime_type: 'image/jpeg' })).toBe('image/png'); + expect(resolvePhotoContentType({ filename: 'a.mov', mime_type: null })).toBe('video/quicktime'); + expect(resolvePhotoContentType({ filename: 'a.bin', media_type: 'video' })).toBe('video/mp4'); + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'image/avif' })).toBe('image/avif'); + expect(resolvePhotoContentType({ filename: 'a.constructor', mime_type: null })).toBe('image/jpeg'); + }); +}); + +describe('serving routes use the resolver', () => { + const routes = ['gallery.js', 'secureImages.js', 'protectedImages.js', 'adminPhotos.js']; + it.each(routes)('%s sets no Content-Type from photo.mime_type directly', (name) => { + const src = fs.readFileSync(path.join(__dirname, '../../src/routes', name), 'utf8'); + expect(src).not.toMatch(/'Content-Type':\s*photo\.mime_type/); + expect(src).not.toMatch(/set\('Content-Type',\s*photo\.mime_type\)/); + expect(src).toMatch(/resolvePhotoContentType\(photo\)/); + }); + + it('chunked-upload init derives the MIME from the allow-listed extension', () => { + const src = fs.readFileSync(path.join(__dirname, '../../src/routes/adminPhotos.js'), 'utf8'); + expect(src).not.toMatch(/const \{ filename, fileSize, mimeType, totalChunks \} = req\.body/); + expect(src).toMatch(/allowedMimeTypes\.includes\(mimeType\)/); + }); +}); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 1a8058c0..ee9064be 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -23,8 +23,10 @@ const { getMaxFileSizeBytes, getMaxVideoSizeBytes, DEFAULT_MAX_FILE_SIZE_MB, - DEFAULT_MAX_VIDEO_SIZE_MB + DEFAULT_MAX_VIDEO_SIZE_MB, + EXTENSION_TO_MIME } = require('../services/uploadSettings'); +const { resolvePhotoContentType } = require('../utils/photoContentType'); const { processUploadedPhotos } = require('../services/photoProcessor'); const chunkedUpload = require('../services/chunkedUploadService'); const watermarkGeneratorService = require('../services/watermarkGeneratorService'); @@ -1165,7 +1167,7 @@ router.get('/:eventId/photos/:photoId/download', adminAuth, requirePermission('p return res.status(404).json({ error: 'Photo file not found' }); } res.set({ - 'Content-Type': photo.mime_type || 'application/octet-stream', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Length': stat.size, 'Content-Disposition': contentDisposition, }); @@ -1182,7 +1184,7 @@ router.get('/:eventId/photos/:photoId/download', adminAuth, requirePermission('p return res.status(404).json({ error: 'Photo file not found' }); } res.set({ - 'Content-Type': photo.mime_type || 'application/octet-stream', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Disposition': contentDisposition, }); res.sendFile(filePath); @@ -1441,64 +1443,9 @@ router.get('/:eventId/photo/:photoId', adminAuth, requirePermission('photos.view const event = await db('events').where('id', eventId).first(); const storageKey = resolvePhotoStorageKey(event, photo); - // Content-Type resolution (#908 + external review). Invariant: the - // header is ALWAYS image/* or video/*. - // - photos.mime_type is never echoed verbatim unless it is a video/ - // type: the chunked-upload path stores the client-sent MIME - // unvalidated, so a stored text/html served inline under the app - // origin would be a same-origin XSS gift. - // - Images ignore the stored value entirely — migration 039 - // backfilled image/jpeg onto every legacy row (PNGs included), so - // the extension is the more trustworthy signal; normalized via the - // shared map (image/jpg → image/jpeg), jpeg fallback when unknown. - // - Videos prefer a stored video/ type, then the extension map - // (.mov → video/quicktime, .webm → video/webm, …), then video/mp4. - // The old ext-derived image/ (image/mp4) is what made the - // admin player's blob unplayable (#908). - const { EXTENSION_TO_MIME } = require('../services/uploadSettings'); - const ext = path.extname(photo.filename).slice(1).toLowerCase(); - // Own-property lookup (review): a client-controlled filename ending in - // .constructor / .__proto__ / .toString would otherwise return an - // inherited Object.prototype member, and the extMime.startsWith below - // would throw — a permanent 500 for that photo instead of the fallback. - const extMime = Object.prototype.hasOwnProperty.call(EXTENSION_TO_MIME, ext) - ? EXTENSION_TO_MIME[ext] - : null; - // Full-token validation, not just a prefix check: the stored value is - // client-controlled, and header-invalid characters (video/mp4\r\nX: y) - // would make setHeader throw — a permanent 500 for that photo. Bare - // 'video/' is equally invalid; both fall back to the extension map. - const storedVideoMime = photo.mime_type && /^video\/[\w.+-]+$/.test(photo.mime_type) - ? photo.mime_type - : null; - // Honor a stored image MIME for any header-safe RASTER type (#908 - // review): the S3 auto-importer accepts arbitrary image/* from - // mime-types and stores it (avif/bmp/tiff/heic/apng/ico/jxl/…), and a - // hand-listed allowlist kept missing formats. Allow image/ but - // NEVER the scriptable svg / *+xml family (image/svg+xml executes - // inline). The strict token + anchors also block header injection - // (image/x\r\nY:). Migration 039's blanket image/jpeg backfill on - // legacy rows is why the mapped extension still wins ahead of this. - const storedImageMime = - photo.mime_type && - /^image\/[\w.+-]+$/.test(photo.mime_type) && - !/^image\/svg|xml/i.test(photo.mime_type) - ? photo.mime_type - : null; - const isVideo = photo.media_type === 'video' || - Boolean(storedVideoMime) || - Boolean(extMime && extMime.startsWith('video/')); - // Never interpolate the raw extension on the image side: it would - // synthesize image/svg+xml (scriptable inline) or header-invalid values - // from client-controlled chunked-upload filenames. Precedence is - // mapped-extension (also corrects the 039 legacy-jpeg backfill on PNGs) - // -> safe stored raster MIME (auto-imported avif/bmp/tiff) -> image/jpeg. - // A stored type outside the allowlist degrades to image/jpeg; browsers - // sniff image bytes in /blob contexts, so a mislabel is harmless - // where an injected type is not. - const contentType = isVideo - ? storedVideoMime || (extMime && extMime.startsWith('video/') ? extMime : null) || 'video/mp4' - : (extMime && extMime.startsWith('image/') ? extMime : null) || storedImageMime || 'image/jpeg'; + // Content-Type resolution (#908 + external review) lives in + // utils/photoContentType so the gallery routes apply the same rule. + const contentType = resolvePhotoContentType(photo); res.setHeader('Content-Type', contentType); res.setHeader('Cache-Control', 'private, max-age=3600'); res.setHeader('Cross-Origin-Resource-Policy', 'cross-origin'); @@ -1666,7 +1613,7 @@ router.get('/:eventId/debug', adminAuth, requirePermission('photos.view'), requi router.post('/:eventId/chunked-upload/init', adminAuth, requirePermission('photos.upload'), requireEventOwnership, async (req, res) => { try { const { eventId } = req.params; - const { filename, fileSize, mimeType, totalChunks } = req.body; + const { filename, fileSize, totalChunks } = req.body; // Validate event exists const event = await db('events').where({ id: eventId }).first(); @@ -1675,8 +1622,23 @@ router.post('/:eventId/chunked-upload/init', adminAuth, requirePermission('photo } // Validate required fields - if (!filename || !fileSize || !mimeType) { - return res.status(400).json({ error: 'Missing required fields: filename, fileSize, mimeType' }); + if (!filename || !fileSize) { + return res.status(400).json({ error: 'Missing required fields: filename, fileSize' }); + } + + // The client-declared mimeType is not trusted. It used to be stored on + // the photo row verbatim and echoed as Content-Type by the gallery + // routes, so a JPEG/HTML polyglot declared as text/html rendered inline + // on the app origin. The MIME is derived from the extension instead, + // and the extension has to be on the admin's allow-list, which is what + // the multipart path enforces through its multer fileFilter. + const ext = path.extname(String(filename)).slice(1).toLowerCase(); + const mimeType = Object.prototype.hasOwnProperty.call(EXTENSION_TO_MIME, ext) + ? EXTENSION_TO_MIME[ext] + : null; + const allowedMimeTypes = await getAllowedMimeTypes(); + if (!mimeType || !allowedMimeTypes.includes(mimeType)) { + return res.status(400).json({ error: 'File type not allowed' }); } // Validate file size against the configured per-file cap. Hardcoding 10GB diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 6504ba2e..1d647dee 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -10,6 +10,7 @@ const { parseBooleanInput } = require('../utils/parsers'); const { getAppSetting } = require('../utils/appSettings'); const archiver = require('archiver'); const path = require('path'); +const { resolvePhotoContentType } = require('../utils/photoContentType'); const router = express.Router(); // #756: a NULL per-event hero_logo_visible means "inherit the global @@ -1613,7 +1614,7 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken, if (req.method === 'HEAD') { const headUseOriginal = await getUseOriginalFilenames(); const headHeaders = { - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Disposition': buildContentDisposition(pickRawDownloadName(photo, headUseOriginal)), 'Accept-Ranges': 'bytes', }; @@ -1711,7 +1712,7 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken, if (rendered) { res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Disposition': contentDisposition, 'Content-Length': rendered.length }); @@ -1769,7 +1770,7 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken, const lastModified = stat.mtime ? new Date(stat.mtime).toUTCString() : null; const headers = { - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Disposition': contentDisposition, 'Accept-Ranges': 'bytes', }; @@ -1852,7 +1853,7 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken, // their bytes on download. Set the header explicitly and stream the // file with res.sendFile-equivalent semantics. res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Disposition': contentDisposition, }); res.sendFile(filePath, (downloadError) => { @@ -2603,7 +2604,7 @@ router.get('/:slug/photo/:photoId', 'Content-Range': `bytes ${start}-${end}/${fileSize}`, 'Accept-Ranges': 'bytes', 'Content-Length': chunksize, - 'Content-Type': photo.mime_type || 'video/mp4', + 'Content-Type': resolvePhotoContentType(photo), 'Cache-Control': 'private, max-age=1800', 'X-Protection-Level': 'basic' }); @@ -2615,7 +2616,7 @@ router.get('/:slug/photo/:photoId', } else { res.writeHead(200, { 'Content-Length': fileSize, - 'Content-Type': photo.mime_type || 'video/mp4', + 'Content-Type': resolvePhotoContentType(photo), 'Accept-Ranges': 'bytes', 'Cache-Control': 'private, max-age=1800', 'X-Protection-Level': 'basic' @@ -2659,7 +2660,7 @@ router.get('/:slug/photo/:photoId', const wmStat = await storage.stat(photo.watermark_path); if (wmStat) { res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Length': wmStat.size, 'Cache-Control': 'private, max-age=1800', 'ETag': etag, @@ -2672,7 +2673,7 @@ router.get('/:slug/photo/:photoId', const watermarkFilePath = path.join(getStoragePath(), photo.watermark_path); if (fs.existsSync(watermarkFilePath)) { res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Cache-Control': 'private, max-age=1800', 'ETag': etag, 'X-Protection-Level': 'basic' @@ -2698,7 +2699,7 @@ router.get('/:slug/photo/:photoId', .catch(err => logger.warn(`Background watermark generation failed for photo ${photo.id}:`, err.message)); res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Cache-Control': 'private, max-age=1800', 'ETag': etag, 'X-Protection-Level': 'basic' @@ -2713,7 +2714,7 @@ router.get('/:slug/photo/:photoId', }); if (useStorageBackend) { res.set('Content-Length', stat.size); - if (photo.mime_type) res.set('Content-Type', photo.mime_type); + res.set('Content-Type', resolvePhotoContentType(photo)); const stream = await storage.get(storageKey); pipeStreamToResponse(stream, res, { context: `photo ${photo.id}` }); } else { diff --git a/backend/src/routes/protectedImages.js b/backend/src/routes/protectedImages.js index 8b078dd5..6f94c9ff 100644 --- a/backend/src/routes/protectedImages.js +++ b/backend/src/routes/protectedImages.js @@ -1,4 +1,5 @@ const express = require('express'); +const { resolvePhotoContentType } = require('../utils/photoContentType'); const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const { verifyGalleryAccess } = require('../middleware/gallery'); @@ -170,7 +171,7 @@ router.get('/:slug/photo/:photoId/view', verifyGalleryAccess, blockHiddenGallery // Set security headers res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Length': finalImage.length, 'Cache-Control': 'private, no-cache, no-store, must-revalidate', 'Pragma': 'no-cache', @@ -351,7 +352,7 @@ router.get('/:slug/photo/:photoId/signed/:token', async (req, res) => { // Set appropriate headers res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Length': imageBuffer.length, 'Cache-Control': 'private, max-age=3600', 'X-Content-Type-Options': 'nosniff' diff --git a/backend/src/routes/secureImages.js b/backend/src/routes/secureImages.js index 43bdb119..c0b229c6 100644 --- a/backend/src/routes/secureImages.js +++ b/backend/src/routes/secureImages.js @@ -1,4 +1,5 @@ const express = require('express'); +const { resolvePhotoContentType } = require('../utils/photoContentType'); const { db } = require('../database/db'); const { verifyGalleryAccess, denySlideshowToken } = require('../middleware/gallery'); const { blockHiddenGallery, bypassesReveal, isGalleryHidden } = require('../utils/revealMode'); @@ -246,7 +247,7 @@ router.get('/:slug/secure/:photoId/:token', // Set content type and security headers res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Length': processedImage.length, 'X-Protection-Level': protectionSettings.protectionLevel, 'X-Remaining-Uses': tokenValidation.remaining @@ -436,7 +437,7 @@ router.get('/:slug/secure-download/:photoId/:token', const downloadName = pickRawDownloadName(photo, useOriginal); res.set({ - 'Content-Type': photo.mime_type || 'image/jpeg', + 'Content-Type': resolvePhotoContentType(photo), 'Content-Disposition': buildContentDisposition(downloadName), 'Content-Length': fileBuffer.length, 'X-Download-Protected': 'true' diff --git a/backend/src/utils/photoContentType.js b/backend/src/utils/photoContentType.js new file mode 100644 index 00000000..357bb39d --- /dev/null +++ b/backend/src/utils/photoContentType.js @@ -0,0 +1,49 @@ +/** + * Content-Type for a served photo row. Invariant: the header is ALWAYS + * image/* or video/*, never the stored value verbatim. + * + * photos.mime_type is client-influenced: the chunked-upload path used to + * store whatever MIME the browser (or a crafted request) declared, and the + * S3 auto-importer stores whatever mime-types derives. Echoing it inline + * under the app origin turned a JPEG/HTML polyglot with mime_type text/html + * into stored HTML injection for every gallery guest. The admin photo route + * (#908 + external review) already resolved this properly; this is that + * logic, shared so every serving route applies the same rule. + * + * - Images ignore the stored value unless it is a header-safe raster type: + * migration 039 backfilled image/jpeg onto every legacy row (PNGs + * included), so the extension is the more trustworthy signal, normalised + * via the shared map, jpeg fallback when unknown. The scriptable svg / + * *+xml family is never honoured. + * - Videos prefer a stored video/ type, then the extension map (.mov -> + * video/quicktime, .webm -> video/webm, ...), then video/mp4. + * - Full-token validation, not a prefix check: header-invalid characters + * (video/mp4\r\nX: y) would make setHeader throw -- a permanent 500 for + * that photo instead of a safe fallback. + */ +const path = require('path'); +const { EXTENSION_TO_MIME } = require('../services/uploadSettings'); + +function resolvePhotoContentType(photo) { + const ext = path.extname(photo?.filename || '').slice(1).toLowerCase(); + // Own-property lookup: a client-controlled filename ending in .constructor + // / .__proto__ would otherwise return an inherited Object.prototype member. + const extMime = Object.prototype.hasOwnProperty.call(EXTENSION_TO_MIME, ext) + ? EXTENSION_TO_MIME[ext] + : null; + const stored = typeof photo?.mime_type === 'string' ? photo.mime_type : ''; + const storedVideoMime = /^video\/[\w.+-]+$/.test(stored) ? stored : null; + const storedImageMime = + /^image\/[\w.+-]+$/.test(stored) && !/^image\/svg|xml/i.test(stored) + ? stored + : null; + const isVideo = photo?.media_type === 'video' || + Boolean(storedVideoMime) || + Boolean(extMime && extMime.startsWith('video/')); + + return isVideo + ? storedVideoMime || (extMime && extMime.startsWith('video/') ? extMime : null) || 'video/mp4' + : (extMime && extMime.startsWith('image/') ? extMime : null) || storedImageMime || 'image/jpeg'; +} + +module.exports = { resolvePhotoContentType };