From fc1bf534129092ca3638e4a4bc47274cd297fa5f Mon Sep 17 00:00:00 2001 From: paul Date: Wed, 1 Oct 2025 15:55:02 +0200 Subject: [PATCH] fix: harden gallery downloads and per-gallery auth --- .../__tests__/services/photoResolver.test.js | 75 +++++++++ backend/src/routes/gallery.js | 145 +++++++++++++----- backend/src/routes/secureImages.js | 41 +++-- backend/src/services/externalMediaService.js | 46 +++++- frontend/src/pages/admin/CreateEventPage.tsx | 15 +- .../pages/admin/CreateEventPageEnhanced.tsx | 4 +- frontend/src/services/events.service.ts | 2 +- frontend/src/utils/cleanupGalleryAuth.ts | 15 ++ 8 files changed, 282 insertions(+), 61 deletions(-) create mode 100644 backend/__tests__/services/photoResolver.test.js diff --git a/backend/__tests__/services/photoResolver.test.js b/backend/__tests__/services/photoResolver.test.js new file mode 100644 index 0000000..63b48df --- /dev/null +++ b/backend/__tests__/services/photoResolver.test.js @@ -0,0 +1,75 @@ +const path = require('path'); +const mockPath = path; + +jest.mock('../../src/services/externalMediaService', () => ({ + resolveExternalPath: jest.fn((event, relPath) => mockPath.join('/mock/external', event.external_path || '', relPath || '')), +})); + +const { resolveExternalPath } = require('../../src/services/externalMediaService'); +const { resolvePhotoFilePath } = require('../../src/services/photoResolver'); + +describe('resolvePhotoFilePath', () => { + const backendRoot = path.resolve(__dirname, '../../'); + const originalStoragePath = process.env.STORAGE_PATH; + + beforeEach(() => { + process.env.STORAGE_PATH = path.join(backendRoot, 'storage'); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + afterAll(() => { + if (typeof originalStoragePath === 'string') { + process.env.STORAGE_PATH = originalStoragePath; + } else { + delete process.env.STORAGE_PATH; + } + }); + + it('returns absolute path for managed photos with legacy slug paths', () => { + const event = { slug: 'wedding-party', source_mode: 'managed' }; + const photo = { path: 'wedding-party/hero.jpg' }; + + const result = resolvePhotoFilePath(event, photo); + + expect(result).toBe(path.join(backendRoot, 'storage', 'events/active', 'wedding-party', 'hero.jpg')); + }); + + it('normalizes prefixed managed paths without duplicating segments', () => { + const event = { slug: 'wedding-party', source_mode: 'managed' }; + const photo = { path: 'events/active/wedding-party/hero.jpg' }; + + const result = resolvePhotoFilePath(event, photo); + + expect(result).toBe(path.join(backendRoot, 'storage', 'events/active', 'wedding-party', 'hero.jpg')); + }); + + it('delegates external photos to external media resolver', () => { + const event = { slug: 'fashion-show', source_mode: 'reference', external_path: 'picsum-demo' }; + const photo = { source_origin: 'external', external_relpath: 'individual/look-01.jpg' }; + + const result = resolvePhotoFilePath(event, photo); + + expect(resolveExternalPath).toHaveBeenCalledWith(event, 'individual/look-01.jpg'); + expect(result).toBe(path.join('/mock/external', 'picsum-demo', 'individual', 'look-01.jpg')); + }); + + it('deduplicates folder names when event external path already ends with segment', () => { + const event = { slug: 'fashion-show', source_mode: 'reference', external_path: 'picsum-demo/individual' }; + const photo = { source_origin: 'external', external_relpath: 'individual/look-02.jpg' }; + + const result = resolvePhotoFilePath(event, photo); + + expect(resolveExternalPath).toHaveBeenCalledWith(event, 'look-02.jpg'); + expect(result).toBe(path.join('/mock/external', 'picsum-demo/individual', 'look-02.jpg')); + }); + + it('throws when external photo is missing relative path data', () => { + const event = { slug: 'fashion-show', source_mode: 'reference', external_path: 'picsum-demo' }; + const photo = { source_origin: 'external' }; + + expect(() => resolvePhotoFilePath(event, photo)).toThrow('Missing external_relpath for external photo'); + }); +}); diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 3ca4bac..da25787 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -1,5 +1,4 @@ const express = require('express'); -const jwt = require('jsonwebtoken'); const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const archiver = require('archiver'); @@ -8,8 +7,8 @@ const router = express.Router(); const watermarkService = require('../services/watermarkService'); const { verifyGalleryAccess } = require('../middleware/gallery'); const secureImageService = require('../services/secureImageService'); -const secureImageMiddleware = require('../middleware/secureImageMiddleware'); const logger = require('../utils/logger'); +const { resolvePhotoFilePath } = require('../services/photoResolver'); // Get storage path from environment or default const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../storage'); @@ -48,9 +47,22 @@ router.get('/:slug/info', async (req, res) => { const { token } = req.query; const event = await db('events') - .where({ slug: slug }) - .select('event_name', 'event_type', 'event_date', 'expires_at', 'is_active', 'is_archived', 'share_link', - 'allow_downloads', 'disable_right_click', 'watermark_downloads', 'watermark_text', 'require_password', 'color_theme') + .where({ slug }) + .select( + 'event_name', + 'event_type', + 'event_date', + 'expires_at', + 'is_active', + 'is_archived', + 'share_link', + 'allow_downloads', + 'disable_right_click', + 'watermark_downloads', + 'watermark_text', + 'require_password', + 'color_theme' + ) .first(); if (!event) { @@ -101,7 +113,6 @@ router.get('/:slug/photos', verifyGalleryAccess, async (req, res) => { try { // Get filter parameters from query const { filter, guest_id } = req.query; - const feedbackService = require('../services/feedbackService'); // First get all photos let photos = await db('photos') @@ -319,16 +330,17 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, async (req, res) => photo_id: photoId }); - // Photo path should be in storage/events/active directory - // Handle both legacy paths (just slug/filename) and new paths (events/active/slug/filename) - const storagePath = getStoragePath(); let filePath; - if (photo.path.startsWith('events/active/')) { - // New format: path already includes events/active/ prefix - filePath = path.join(storagePath, photo.path); - } else { - // Legacy format: path is just slug/filename - filePath = path.join(storagePath, 'events/active', photo.path); + try { + filePath = resolvePhotoFilePath(req.event, photo); + } catch (resolveError) { + logger.error('Failed to resolve photo path for download', { + slug: req.params.slug, + photoId, + eventId: req.event.id, + error: resolveError.message, + }); + return res.status(404).json({ error: 'Photo file not found' }); } // Get watermark settings @@ -347,9 +359,24 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, async (req, res) => res.send(watermarkedBuffer); } else { // Send original file - res.download(filePath, photo.filename); + res.download(filePath, photo.filename, (downloadError) => { + if (downloadError) { + logger.error('Error streaming gallery download', { + slug: req.params.slug, + photoId, + eventId: req.event.id, + error: downloadError.message, + }); + } + }); } } catch (error) { + logger.error('Unexpected error processing gallery download', { + slug: req.params.slug, + photoId: req.params.photoId, + eventId: req.event?.id, + error: error.message, + }); res.status(500).json({ error: 'Failed to download photo' }); } }); @@ -392,16 +419,17 @@ router.get('/:slug/download-all', verifyGalleryAccess, async (req, res) => { // Add photos to archive for (const photo of photos) { - // Photo path should be in storage/events/active directory - // Handle both legacy paths (just slug/filename) and new paths (events/active/slug/filename) - const storagePath = getStoragePath(); let filePath; - if (photo.path.startsWith('events/active/')) { - // New format: path already includes events/active/ prefix - filePath = path.join(storagePath, photo.path); - } else { - // Legacy format: path is just slug/filename - filePath = path.join(storagePath, 'events/active', photo.path); + try { + filePath = resolvePhotoFilePath(req.event, photo); + } catch (resolveError) { + logger.warn('Skipping photo in bulk download due to unresolved path', { + slug: req.params.slug, + photoId: photo.id, + eventId: req.event.id, + error: resolveError.message, + }); + continue; } // Determine the file name in the archive @@ -416,11 +444,18 @@ router.get('/:slug/download-all', verifyGalleryAccess, async (req, res) => { } if (watermarkSettings && watermarkSettings.enabled) { - // Apply watermark - const watermarkedBuffer = await watermarkService.applyWatermark(filePath, watermarkSettings); - archive.append(watermarkedBuffer, { name: archiveName }); + try { + const watermarkedBuffer = await watermarkService.applyWatermark(filePath, watermarkSettings); + archive.append(watermarkedBuffer, { name: archiveName }); + } catch (watermarkError) { + logger.warn('Failed to watermark photo for bulk download, skipping original to avoid leak', { + slug: req.params.slug, + photoId: photo.id, + eventId: req.event.id, + error: watermarkError.message, + }); + } } else { - // Add original file archive.file(filePath, { name: archiveName }); } } @@ -435,6 +470,11 @@ router.get('/:slug/download-all', verifyGalleryAccess, async (req, res) => { action: 'download_all' }); } catch (error) { + logger.error('Error creating bulk gallery download', { + slug: req.params.slug, + eventId: req.event?.id, + error: error.message, + }); res.status(500).json({ error: 'Failed to create download archive' }); } }); @@ -479,30 +519,47 @@ router.post('/:slug/download-selected', verifyGalleryAccess, async (req, res) => const archive = archiver('zip', { zlib: { level: 5 } }); archive.on('error', (err) => { - console.error('Zip error:', err); - try { res.status(500).end(); } catch (e) {} + logger.error('Zip error generating selected download', { + slug: req.params.slug, + eventId: req.event?.id, + error: err.message, + }); + try { + res.status(500).end(); + } catch (_) { + // ignore double-send errors + } }); archive.pipe(res); - const { resolvePhotoFilePath } = require('../services/photoResolver'); - const fs = require('fs'); // Check watermark settings similar to download-all const watermarkSettings = await watermarkService.getWatermarkSettings(); for (const photo of photos) { try { const filePath = resolvePhotoFilePath(req.event, photo); - if (filePath && fs.existsSync(filePath)) { - const name = photo.filename || `photo-${photo.id}.jpg`; - if (watermarkSettings && watermarkSettings.enabled) { - // Apply watermark like download-all + const name = photo.filename || `photo-${photo.id}.jpg`; + if (watermarkSettings && watermarkSettings.enabled) { + try { const watermarkedBuffer = await watermarkService.applyWatermark(filePath, watermarkSettings); archive.append(watermarkedBuffer, { name }); - } else { - archive.file(filePath, { name }); + } catch (watermarkError) { + logger.warn('Failed to watermark selected photo, skipping original to avoid leak', { + slug: req.params.slug, + photoId: photo.id, + eventId: req.event.id, + error: watermarkError.message, + }); } + } else { + archive.file(filePath, { name }); } - } catch (e) { - // skip missing/inaccessible files + } catch (resolveError) { + logger.warn('Skipping selected photo due to unresolved path', { + slug: req.params.slug, + photoId: photo.id, + eventId: req.event.id, + error: resolveError.message, + }); } } @@ -515,7 +572,11 @@ router.post('/:slug/download-selected', verifyGalleryAccess, async (req, res) => action: 'download_selected' }); } catch (error) { - console.error('Error in download-selected:', error); + logger.error('Error in download-selected:', { + slug: req.params.slug, + eventId: req.event?.id, + error: error.message, + }); res.status(500).json({ error: 'Failed to download selected photos' }); } }); diff --git a/backend/src/routes/secureImages.js b/backend/src/routes/secureImages.js index 7e5ff33..e1cdf67 100644 --- a/backend/src/routes/secureImages.js +++ b/backend/src/routes/secureImages.js @@ -1,17 +1,14 @@ const express = require('express'); -const path = require('path'); const { db } = require('../database/db'); const { verifyGalleryAccess } = require('../middleware/gallery'); const secureImageService = require('../services/secureImageService'); const secureImageMiddleware = require('../middleware/secureImageMiddleware'); const logger = require('../utils/logger'); const { formatBoolean } = require('../utils/dbCompat'); +const { resolvePhotoFilePath } = require('../services/photoResolver'); const router = express.Router(); -// Get storage path from environment or default -const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); - /** * Generate secure token for image access */ @@ -94,11 +91,11 @@ router.get('/:slug/secure/:photoId/:token', const { slug, photoId, token } = req.params; // Move outside try block for error handler access try { - console.log('Secure image route hit:', { - slug: slug, - photoId: photoId, + logger.debug('Secure image route hit', { + slug, + photoId, tokenLength: token?.length, - headers: req.headers.authorization ? 'present' : 'absent' + hasAuthHeader: Boolean(req.headers.authorization), }); const { fragment } = req.query; @@ -142,7 +139,18 @@ router.get('/:slug/secure/:photoId/:token', return res.status(404).json({ error: 'Photo not found' }); } - const filePath = path.join(getStoragePath(), 'events/active', photo.path); + let filePath; + try { + filePath = resolvePhotoFilePath(req.event, photo); + } catch (resolveError) { + logger.error('Failed to resolve photo path for secure token generation', { + slug: req.params.slug, + photoId, + eventId: req.event.id, + error: resolveError.message, + }); + return res.status(404).json({ error: 'Photo file not found' }); + } // Get protection settings for this event const protectionSettings = { @@ -284,7 +292,18 @@ router.get('/:slug/secure-download/:photoId/:token', return res.status(404).json({ error: 'Photo not found' }); } - const filePath = path.join(getStoragePath(), 'events/active', photo.path); + let filePath; + try { + filePath = resolvePhotoFilePath(req.event, photo); + } catch (resolveError) { + logger.error('Failed to resolve photo path for secure download', { + slug: req.params.slug, + photoId, + eventId: req.event.id, + error: resolveError.message, + }); + return res.status(404).json({ error: 'Photo file not found' }); + } // Apply watermark if enabled const watermarkService = require('../services/watermarkService'); @@ -426,4 +445,4 @@ async function getSuspiciousActivityStats() { } } -module.exports = router; \ No newline at end of file +module.exports = router; diff --git a/backend/src/services/externalMediaService.js b/backend/src/services/externalMediaService.js index 31913cd..f696ae7 100644 --- a/backend/src/services/externalMediaService.js +++ b/backend/src/services/externalMediaService.js @@ -1,9 +1,52 @@ const fs = require('fs').promises; +const fsSync = require('fs'); const path = require('path'); const { safePathJoin } = require('../utils/fileSecurityUtils'); +let cachedRoot = null; + +function resolveDefaultRoot() { + const containerDefault = '/external-media'; + try { + if (fsSync.existsSync(containerDefault)) { + return containerDefault; + } + } catch (error) { + // ignore lookup errors, fallback below + } + + const localFallback = path.resolve(__dirname, '../../..', 'storage/external-media'); + try { + if (fsSync.existsSync(localFallback)) { + return localFallback; + } + } catch (error) { + // ignore and return container default + } + + return containerDefault; +} + function getExternalMediaRoot() { - return process.env.EXTERNAL_MEDIA_ROOT || '/external-media'; + if (cachedRoot) { + return cachedRoot; + } + + const configured = process.env.EXTERNAL_MEDIA_ROOT; + if (configured && configured.trim()) { + const resolvedConfigured = path.resolve(configured.trim()); + try { + if (fsSync.existsSync(resolvedConfigured)) { + cachedRoot = resolvedConfigured; + return cachedRoot; + } + } catch (error) { + // ignore lookup errors and fall back to defaults + } + } + + cachedRoot = resolveDefaultRoot(); + return cachedRoot; } function isUnderRoot(p) { @@ -64,4 +107,3 @@ module.exports = { list, resolveExternalPath, }; - diff --git a/frontend/src/pages/admin/CreateEventPage.tsx b/frontend/src/pages/admin/CreateEventPage.tsx index 8161d54..634c9cb 100644 --- a/frontend/src/pages/admin/CreateEventPage.tsx +++ b/frontend/src/pages/admin/CreateEventPage.tsx @@ -241,20 +241,22 @@ export const CreateEventPage: React.FC = () => { const selectedTheme = COLOR_THEMES.find(t => t.value === formData.color_theme); - createMutation.mutate({ + const payload = { event_type: formData.event_type, event_name: formData.event_name, event_date: formData.event_date, host_email: formData.host_email, admin_email: formData.admin_email, require_password: formData.require_password, - password: formData.require_password ? formData.password : '', + password: formData.require_password ? formData.password : undefined, welcome_message: formData.welcome_message || '', color_theme: selectedTheme ? JSON.stringify(selectedTheme.theme) : undefined, expiration_days: formData.expires_in_days, allow_user_uploads: formData.allow_user_uploads, upload_category_id: formData.upload_category_id, - }); + }; + + createMutation.mutate(payload); }; const handleInputChange = (field: keyof FormData) => ( @@ -438,7 +440,12 @@ export const CreateEventPage: React.FC = () => { checked={formData.require_password} onChange={(e) => { const checked = e.target.checked; - setFormData(prev => ({ ...prev, require_password: checked })); + setFormData(prev => ({ + ...prev, + require_password: checked, + password: checked ? prev.password : '', + confirm_password: checked ? prev.confirm_password : '' + })); if (!checked) { setErrors(prev => ({ ...prev, password: '', confirm_password: '' })); } diff --git a/frontend/src/pages/admin/CreateEventPageEnhanced.tsx b/frontend/src/pages/admin/CreateEventPageEnhanced.tsx index feaf931..4472aee 100644 --- a/frontend/src/pages/admin/CreateEventPageEnhanced.tsx +++ b/frontend/src/pages/admin/CreateEventPageEnhanced.tsx @@ -240,7 +240,7 @@ export const CreateEventPageEnhanced: React.FC = () => { host_email: formData.host_email, admin_email: formData.admin_email, require_password: formData.require_password, - password: formData.require_password ? formData.password : '', + password: formData.require_password ? formData.password : undefined, welcome_message: formData.welcome_message || '', color_theme: JSON.stringify(formData.theme_config), expiration_days: formData.expires_in_days, @@ -511,6 +511,8 @@ export const CreateEventPageEnhanced: React.FC = () => { setFormData(prev => ({ ...prev, require_password: checked, + password: checked ? prev.password : '', + confirm_password: checked ? prev.confirm_password : '', })); if (!checked) { setErrors(prev => ({ ...prev, password: undefined, confirm_password: undefined })); diff --git a/frontend/src/services/events.service.ts b/frontend/src/services/events.service.ts index 11fac2c..e77ae4b 100644 --- a/frontend/src/services/events.service.ts +++ b/frontend/src/services/events.service.ts @@ -14,7 +14,7 @@ interface CreateEventData { host_email: string; admin_email: string; require_password?: boolean; - password: string; + password?: string; welcome_message?: string; color_theme?: string; expiration_days: number; diff --git a/frontend/src/utils/cleanupGalleryAuth.ts b/frontend/src/utils/cleanupGalleryAuth.ts index e5a2f4a..ff5751c 100644 --- a/frontend/src/utils/cleanupGalleryAuth.ts +++ b/frontend/src/utils/cleanupGalleryAuth.ts @@ -24,4 +24,19 @@ export const cleanupOldGalleryAuth = () => { sessionStorage.removeItem('gallery_event'); sessionStorage.removeItem('gallery_token'); sessionStorage.removeItem('gallery_active_slug'); + + // Remove slug-specific session storage entries as well + try { + const sessionKeysToRemove: string[] = []; + for (let i = 0; i < sessionStorage.length; i += 1) { + const key = sessionStorage.key(i); + if (key && (key.startsWith('gallery_event_') || key.startsWith('gallery_token_'))) { + sessionKeysToRemove.push(key); + } + } + + sessionKeysToRemove.forEach((key) => sessionStorage.removeItem(key)); + } catch { + // Session storage may be unavailable; ignore cleanup failures + } };