From e18ab0d84270fcc7903e3cc92db14a8ed2532ee7 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:23:04 +0200 Subject: [PATCH 01/33] fix(upload): enforce the configured per-file size limit on admin uploads getMaxFileSizeBytes() (general_max_file_size_mb, default 50MB) was only read by adminSettings.js to display the value. The admin upload routes streamed against a hardcoded ceiling instead, so the dropzone's "max. 50MB pro Datei" was never enforced: - adminPhotos.js POST /:eventId/upload -> 10GB hardcoded - adminPhotos.js POST /:eventId/chunked-upload/init -> 10GB hardcoded - v1/events.js POST /events/:id/photos -> 100MB hardcoded Resolve the cap per request (it is admin-configurable at runtime) and build the multer instance from it, mirroring what gallery.js and adminTransfers.js already do. The 400 names the configured limit and reuses gallery.js's exact error string so the frontend surfaces it identically. getMaxFileSizeBytes() clamps to MAX_ALLOWED_FILE_SIZE_MB, so the 10GB hard ceiling still bounds everything. gallery.js (guest upload) already enforced this correctly and is unchanged -- the report's claim that it did not is stale. Interpretation: general_max_file_size_mb is a single per-file cap with no photo/video split, and gallery.js already applies it blanket to guest video uploads, so admin video uploads now share it too. On a default install that means a 200MB video needs the setting raised first -- which is what the UI has been advertising all along. Refs testplan REPORT.md #1 (Part 7.06). --- .../routes/adminPhotoUploadSizeLimit.test.js | 148 ++++++++++++++++++ backend/src/routes/adminPhotos.js | 40 +++-- .../v1/__tests__/events.category.test.js | 8 + backend/src/routes/v1/events.js | 29 +++- 4 files changed, 211 insertions(+), 14 deletions(-) create mode 100644 backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js diff --git a/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js b/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js new file mode 100644 index 00000000..f6a96bfd --- /dev/null +++ b/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js @@ -0,0 +1,148 @@ +/** + * Per-file upload size limit on the admin photo routes. + * + * `general_max_file_size_mb` (Settings → General, default 50MB) is what the + * dropzone advertises ("max. 50MB per file"), but the admin upload route + * hardcoded multer's cap at 10GB and the chunked-upload init route at 10GB + * too — so the advertised limit was never enforced anywhere server-side and a + * 50.74MB JPEG uploaded cleanly. + * + * Pins: + * - a file over the configured cap is rejected with a 400 naming the limit + * - the chunked-upload init route honours the same cap (it would otherwise + * be a trivial bypass of the multipart route's cap) + * - a file under the cap still gets past the size gate + * - the limit is read per request, so an admin raising it takes effect + */ + +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-upload-size-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'upload-size-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-upload-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 = 'upload-size-test-event'; + +describe('admin upload per-file size limit (general_max_file_size_mb)', () => { + let db; + let cleanup; + let app; + let eventId; + let adminToken; + let uploadSettings; + + const setLimitMb = async (mb) => { + await db('app_settings') + .insert({ + setting_key: 'general_max_file_size_mb', + setting_value: JSON.stringify(mb), + setting_type: 'general', + updated_at: new Date().toISOString(), + }) + .onConflict('setting_key') + .merge({ setting_value: JSON.stringify(mb) }); + uploadSettings.clearMaxFileSizeCache(); + }; + + const postUpload = (bytes, filename = 'shot.jpg') => request(app) + .post(`/api/admin/photos/${eventId}/upload`) + .set('Authorization', `Bearer ${adminToken}`) + .attach('photos', Buffer.alloc(bytes, 0x41), { filename, contentType: 'image/jpeg' }); + + const postChunkedInit = (fileSize) => request(app) + .post(`/api/admin/photos/${eventId}/chunked-upload/init`) + .set('Authorization', `Bearer ${adminToken}`) + .send({ filename: 'clip.mp4', fileSize, mimeType: 'video/mp4', totalChunks: 1 }); + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const inserted = await db('events').insert({ + slug: SLUG, + event_type: 'wedding', + event_name: 'Upload 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: 'upload-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: 'upload-size-admin', + email: 'upload-size-admin@example.com', + password_hash: await bcrypt.hash('UploadSize123', 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: 'upload-size-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' } + ); + + uploadSettings = require('../../src/services/uploadSettings'); + + app = express(); + app.use(express.json()); + app.use('/api/admin/photos', require('../../src/routes/adminPhotos')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('rejects a file over the configured limit with a 400 naming the limit', async () => { + await setLimitMb(1); + const res = await postUpload(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 an over-limit chunked upload at init instead of allowing 10GB', async () => { + await setLimitMb(1); + const res = await postChunkedInit(200 * 1024 * 1024); + expect(res.status).toBe(400); + expect(res.body.error).toBe('File too large. Maximum size is 1 MB per file.'); + }); + + it('lets a file under the limit past the size gate', async () => { + await setLimitMb(1); + // Junk bytes, so it still fails downstream on the content check — that is + // the point: the failure is no longer about size. + const res = await postUpload(64 * 1024); + expect(res.status).toBe(400); + expect(res.body.error).toBe('File content does not match declared type: shot.jpg'); + }); + + it('reads the limit per request, so raising it takes effect immediately', async () => { + await setLimitMb(1); + expect((await postUpload(2 * 1024 * 1024)).status).toBe(400); + + await setLimitMb(10); + const res = await postUpload(2 * 1024 * 1024); + expect(res.status).toBe(400); + expect(res.body.error).toBe('File content does not match declared type: shot.jpg'); + }); +}); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 4c31ef1c..65e1c139 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -17,7 +17,7 @@ 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 } = require('../services/uploadSettings'); +const { getMaxFilesPerUpload, getAllowedMimeTypes, getMaxFileSizeBytes, DEFAULT_MAX_FILE_SIZE_MB } = require('../services/uploadSettings'); const { processUploadedPhotos } = require('../services/photoProcessor'); const chunkedUpload = require('../services/chunkedUploadService'); const watermarkGeneratorService = require('../services/watermarkGeneratorService'); @@ -67,10 +67,16 @@ const { validateFileType, createFileUploadValidator } = require('../utils/fileSe // The allowed types are fetched from the database once per request (before multer // processes files) and attached to req.allowedMimeTypes so that the fileFilter // callback can read them synchronously. -const upload = multer({ +// +// The per-file size cap is resolved per request too (general_max_file_size_mb), +// so the uploader has to be built per request like the transfer routes do. It +// was hardcoded to 10GB here, which meant the advertised "max. 50MB per file" +// in the dropzone was never enforced anywhere server-side. getMaxFileSizeBytes() +// clamps to MAX_ALLOWED_FILE_SIZE_MB (10GB), so that hard ceiling still applies. +const createUpload = (maxFileSizeBytes) => multer({ storage: storage, limits: { - fileSize: 10 * 1024 * 1024 * 1024, // 10GB limit per file to support large videos + fileSize: maxFileSizeBytes, files: 2000, // Hard safety ceiling; actual limit enforced dynamically fieldSize: 10 * 1024 * 1024, // 10MB for non-file fields parts: 10000, @@ -105,7 +111,9 @@ const validateUploadContent = async (req, res, next) => { const allowedTypes = req.allowedMimeTypes || ['image/jpeg', 'image/png', 'image/webp']; const validator = createFileUploadValidator({ allowedTypes, - maxFileSize: 10 * 1024 * 1024 * 1024, // 10GB to support large videos + // 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, validateContent: true }); return validator(req, res, next); @@ -132,21 +140,25 @@ const uploadTimeout = (timeout = 300000) => { // 5 minutes default }; // Upload photos for an event -// Max file count is configurable via general settings +// 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 let maxFilesPerUpload; + let maxFileSizeBytes; try { maxFilesPerUpload = await getMaxFilesPerUpload(); + maxFileSizeBytes = await getMaxFileSizeBytes(); } catch (error) { return errorResponse(res, error, 500, 'Unable to determine upload limits'); } + req.maxFileSizeBytes = maxFileSizeBytes; + const maxFileSizeMb = Math.floor(maxFileSizeBytes / (1024 * 1024)); - upload.array('photos', maxFilesPerUpload)(req, res, (err) => { + createUpload(maxFileSizeBytes).array('photos', maxFilesPerUpload)(req, res, (err) => { if (err) { logger.error('Multer error:', err); if (err instanceof multer.MulterError) { if (err.code === 'LIMIT_FILE_SIZE') { - return res.status(400).json({ error: 'File too large. Maximum size is 10GB per file.' }); + return res.status(400).json({ error: `File too large. Maximum size is ${maxFileSizeMb} MB per file.` }); } if (err.code === 'LIMIT_FILE_COUNT' || err.code === 'LIMIT_UNEXPECTED_FILE') { return res.status(400).json({ error: `Too many files. Maximum ${maxFilesPerUpload} files per upload.` }); @@ -1582,10 +1594,18 @@ router.post('/:eventId/chunked-upload/init', adminAuth, requirePermission('photo return res.status(400).json({ error: 'Missing required fields: filename, fileSize, mimeType' }); } - // Validate file size (max 10GB) - const maxSize = 10 * 1024 * 1024 * 1024; + // Validate file size against the configured per-file cap. Hardcoding 10GB + // here let the chunked path sidestep general_max_file_size_mb entirely. + let maxSize; + try { + maxSize = await getMaxFileSizeBytes(); + } catch { + maxSize = DEFAULT_MAX_FILE_SIZE_MB * 1024 * 1024; + } if (fileSize > maxSize) { - return res.status(400).json({ error: 'File too large. Maximum size is 10GB.' }); + return res.status(400).json({ + error: `File too large. Maximum size is ${Math.floor(maxSize / (1024 * 1024))} MB per file.` + }); } const result = await chunkedUpload.initializeUpload({ diff --git a/backend/src/routes/v1/__tests__/events.category.test.js b/backend/src/routes/v1/__tests__/events.category.test.js index f3655dc7..bd935526 100644 --- a/backend/src/routes/v1/__tests__/events.category.test.js +++ b/backend/src/routes/v1/__tests__/events.category.test.js @@ -93,6 +93,14 @@ jest.mock('multer', () => { return factory; }); +// The upload middleware resolves the per-file size cap from app_settings on +// every request (general_max_file_size_mb). That read would consume one of +// this suite's sequenced db chains and shift every later assertion, so stub it. +jest.mock('../../../services/uploadSettings', () => ({ + getMaxFileSizeBytes: jest.fn().mockResolvedValue(50 * 1024 * 1024), + DEFAULT_MAX_FILE_SIZE_MB: 50, +})); + // Stub sharp so the happy-path test doesn't actually decode an image // (the temp file is a 0-byte placeholder — see the beforeAll below). jest.mock('sharp', () => jest.fn(() => ({ diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index bb07a3ab..7f1d3e63 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -38,6 +38,7 @@ const { formatBoolean } = require('../../utils/dbCompat'); const { parseBooleanInput } = require('../../utils/parsers'); const { isValidEventType } = require('../../services/eventTypeService'); const { replacePhoto } = require('../../services/photoReplacementService'); +const { getMaxFileSizeBytes, DEFAULT_MAX_FILE_SIZE_MB } = require('../../services/uploadSettings'); const downloadZipService = require('../../services/downloadZipService'); const { PhotoFilterBuilder } = require('../../utils/photoFilterBuilder'); const { PhotoExportService } = require('../../services/photoExportService'); @@ -65,14 +66,34 @@ const photoStorage = multer.diskStorage({ cb(null, `v1_${Date.now()}_${crypto.randomBytes(4).toString('hex')}${ext}`); } }); -const photoUpload = multer({ +const buildPhotoUpload = (maxFileSizeBytes) => multer({ storage: photoStorage, - limits: { fileSize: 100 * 1024 * 1024 }, // 100MB per file for v1 + limits: { fileSize: maxFileSizeBytes }, fileFilter: (_req, file, cb) => { if (/^image\//.test(file.mimetype)) cb(null, true); else cb(new Error('Only image uploads are accepted on this endpoint')); } -}); +}).single('photo'); + +// The per-file cap was hardcoded to 100MB here, so general_max_file_size_mb +// (Settings → General) didn't apply to the v1 upload either. Resolve it per +// request — the admin can change it at runtime — and turn multer's generic +// "File too large" into a 400 that names the configured limit. +const photoUpload = async (req, res, next) => { + let maxFileSizeBytes; + try { + maxFileSizeBytes = await getMaxFileSizeBytes(); + } catch { + maxFileSizeBytes = DEFAULT_MAX_FILE_SIZE_MB * 1024 * 1024; + } + buildPhotoUpload(maxFileSizeBytes)(req, res, (err) => { + if (err && err.code === 'LIMIT_FILE_SIZE') { + const limitMb = Math.floor(maxFileSizeBytes / (1024 * 1024)); + return res.status(400).json({ error: `File too large. Maximum size is ${limitMb} MB per file.` }); + } + next(err); + }); +}; // slugify now imported from ../../utils/slug — shared with adminEvents // and events.js so the diacritic fix from #502 lands here too (#525). @@ -616,7 +637,7 @@ router.post( requireApiScope('write'), requirePermission('photos.upload'), requireEventOwnership, - photoUpload.single('photo'), + photoUpload, async (req, res) => { let tempPath = null; try { From 6f7aa59fadc8ffaef8c9e1188c5b9e0a9e110229 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:23:22 +0200 Subject: [PATCH 02/33] fix(feedback): align word-filter severity vocabulary with the admin UI WordFilterManager.tsx sends low/moderate/high/block; the validator only accepted mild/moderate/severe, so 3 of the 4 UI levels 400'd with "Invalid severity level" -- including "block", the strongest advertised tier. Aligning isIn() alone would have made "block" accepted but semantically inert: feedbackModeration.js branches on 'severe'/'moderate', so "block" would fall through to the flag-only branch and behave as the weakest level. Map the UI vocabulary onto the existing outcomes instead, per the legend the UI itself renders: block -> reject, moderate/high -> needs approval, low -> flag only. 'severe' stays an accepted alias in the blocking predicate so any row written through the old validator (the field is optional, so a direct API caller could have stored one) keeps blocking. No data migration needed: the column is a bare varchar(20) default 'moderate' with no CHECK, no enum and no seed rows, and 'mild' already lands in the flag-only branch that 'low' now means. Refs testplan REPORT.md #2 (Part 3, J.11). --- .../adminFeedbackWordFilterSeverity.test.js | 72 +++++++++++++++++++ backend/src/services/feedbackModeration.js | 17 +++-- backend/src/utils/feedbackValidation.js | 2 +- 3 files changed, 83 insertions(+), 8 deletions(-) create mode 100644 backend/__tests__/routes/adminFeedbackWordFilterSeverity.test.js diff --git a/backend/__tests__/routes/adminFeedbackWordFilterSeverity.test.js b/backend/__tests__/routes/adminFeedbackWordFilterSeverity.test.js new file mode 100644 index 00000000..a7329ce7 --- /dev/null +++ b/backend/__tests__/routes/adminFeedbackWordFilterSeverity.test.js @@ -0,0 +1,72 @@ +/** + * Word-filter severity vocabulary. The Settings → Moderation UI offers + * low / moderate / high / block, but the validator only accepted the + * unrelated mild / moderate / severe set, so 3 of the 4 levels — including + * "block", the strongest tier — 400'd on every add. + */ +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-wfsev-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'wfsev-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-wfsev-storage-')); + +const request = require('supertest'); +const express = require('express'); +const cookieParser = require('cookie-parser'); +const { bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken } = require('../integration/helpers/crmDb'); + +describe('word filter severity levels', () => { + let db; let cleanup; let app; let superTok; + + const auth = (req) => req.set('Authorization', `Bearer ${superTok}`); + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + const { adminId: superId } = await seedMinimal(db); + await assignAdminRole(db, superId, 'super_admin'); + superTok = mintAdminToken(superId); + + app = express(); + app.use(express.json()); + app.use(cookieParser()); + app.use('/api/admin/feedback', require('../../src/routes/adminFeedback')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it.each(['low', 'moderate', 'high', 'block'])('accepts severity "%s"', async (severity) => { + const word = `zzsev${severity}`; + const res = await auth(request(app).post('/api/admin/feedback/word-filters')) + .send({ word, severity }); + + expect(res.status).toBe(200); + const row = await db('feedback_word_filters').where({ word }).first(); + expect(row.severity).toBe(severity); + }); + + it('still rejects a severity outside the vocabulary', async () => { + const res = await auth(request(app).post('/api/admin/feedback/word-filters')) + .send({ word: 'zzsevbogus', severity: 'catastrophic' }); + + expect(res.status).toBe(400); + expect(res.body.errors.some((e) => e.path === 'severity')).toBe(true); + }); + + it('blocks a comment matching a "block" filter and only flags a "low" one', async () => { + const moderation = require('../../src/services/feedbackModeration'); + moderation.clearCache(); + + const blocked = await moderation.moderateText('this is zzsevblock speech'); + expect(blocked.approved).toBe(false); + expect(blocked.violations.map((v) => v.word)).toEqual(['zzsevblock']); + + const flagged = await moderation.moderateText('this is zzsevlow speech'); + expect(flagged.approved).toBe(true); + expect(flagged.flagged).toBe(true); + }); +}); diff --git a/backend/src/services/feedbackModeration.js b/backend/src/services/feedbackModeration.js index 73da47ce..df9babb0 100644 --- a/backend/src/services/feedbackModeration.js +++ b/backend/src/services/feedbackModeration.js @@ -66,17 +66,20 @@ class FeedbackModerationService { } } - // Check for severe violations - if (violations.some(v => v.severity === 'severe')) { + // Check for blocking violations. 'severe' is the legacy vocabulary the + // validator used to accept before it was aligned with the UI's + // low/moderate/high/block levels — rows stored under it still apply. + const isBlocking = (v) => v.severity === 'block' || v.severity === 'severe'; + if (violations.some(isBlocking)) { return { approved: false, reason: 'Content contains prohibited words', - violations: violations.filter(v => v.severity === 'severe') + violations: violations.filter(isBlocking) }; } - - // Check for moderate violations - if (violations.some(v => v.severity === 'moderate')) { + + // Check for moderate/high violations + if (violations.some(v => v.severity === 'moderate' || v.severity === 'high')) { return { approved: false, reason: 'Content requires moderation', @@ -84,7 +87,7 @@ class FeedbackModerationService { }; } - // Check for mild violations (may just flag for review) + // Check for low-severity violations (may just flag for review) if (violations.length > 0) { return { approved: true, diff --git a/backend/src/utils/feedbackValidation.js b/backend/src/utils/feedbackValidation.js index bd684ab7..2dc77b69 100644 --- a/backend/src/utils/feedbackValidation.js +++ b/backend/src/utils/feedbackValidation.js @@ -241,7 +241,7 @@ const validateWordFilter = [ .withMessage('Word must be between 2 and 100 characters'), body('severity') .optional() - .isIn(['mild', 'moderate', 'severe']) + .isIn(['low', 'moderate', 'high', 'block']) .withMessage('Invalid severity level') ]; From 5fa04e647e0810c86c3a3b2ecba5f53a3859e8d8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:23:28 +0200 Subject: [PATCH 03/33] fix(categories): validate category name length instead of 500ing photo_categories.name is varchar(100). Neither the input nor the route checked length, so a 267-char name hit a raw Postgres "value too long", came back as a 500, and the form silently stayed open with no toast. Add isLength({ max: 100 }) to POST / and PUT /:id (the update route had the identical gap) so it returns the route family's normal 400 { errors: [...] } shape that the toast helper already renders, and maxLength={100} on the three category-name inputs (create + inline edit in CategoryManager, create in EventCategoryManager). Refs testplan REPORT.md #4 (Part 7.01). --- .../routes/adminCategoriesNameLength.test.js | 76 +++++++++++++++++++ backend/src/routes/adminCategories.js | 9 ++- .../src/components/admin/CategoryManager.tsx | 2 + .../components/admin/EventCategoryManager.tsx | 1 + 4 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/routes/adminCategoriesNameLength.test.js diff --git a/backend/__tests__/routes/adminCategoriesNameLength.test.js b/backend/__tests__/routes/adminCategoriesNameLength.test.js new file mode 100644 index 00000000..6d0a7915 --- /dev/null +++ b/backend/__tests__/routes/adminCategoriesNameLength.test.js @@ -0,0 +1,76 @@ +/** + * photo_categories.name is varchar(100). Without a length check the insert + * hit Postgres' "value too long" and the route's catch turned it into a raw + * 500 with no message the form could surface — a >100-char name must come + * back as a normal 400 validation error instead. + */ +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-catlen-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'catlen-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-catlen-storage-')); + +const request = require('supertest'); +const express = require('express'); +const cookieParser = require('cookie-parser'); +const { bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken } = require('../integration/helpers/crmDb'); + +const TOO_LONG = 'z'.repeat(101); + +describe('category name length validation', () => { + let db; let cleanup; let app; let superTok; + + const auth = (req) => req.set('Authorization', `Bearer ${superTok}`); + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + const { adminId: superId } = await seedMinimal(db); + await assignAdminRole(db, superId, 'super_admin'); + superTok = mintAdminToken(superId); + + app = express(); + app.use(express.json()); + app.use(cookieParser()); + app.use('/api/admin/categories', require('../../src/routes/adminCategories')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('rejects a >100-char name on create with a 400, not a 500', async () => { + const res = await auth(request(app).post('/api/admin/categories')) + .send({ name: TOO_LONG, is_global: true }); + + expect(res.status).toBe(400); + expect(res.body.errors.some((e) => e.path === 'name')).toBe(true); + const rows = await db('photo_categories').where('name', TOO_LONG); + expect(rows).toHaveLength(0); + }); + + it('rejects a >100-char name on update with a 400, not a 500', async () => { + const created = await auth(request(app).post('/api/admin/categories')) + .send({ name: 'zzcatlen-ok', is_global: true }); + expect(created.status).toBe(200); + + const res = await auth(request(app).put(`/api/admin/categories/${created.body.id}`)) + .send({ name: TOO_LONG }); + + expect(res.status).toBe(400); + expect(res.body.errors.some((e) => e.path === 'name')).toBe(true); + const row = await db('photo_categories').where('id', created.body.id).first(); + expect(row.name).toBe('zzcatlen-ok'); + }); + + it('still accepts a name at exactly the 100-char limit', async () => { + const name = 'y'.repeat(100); + const res = await auth(request(app).post('/api/admin/categories')) + .send({ name, is_global: true }); + + expect(res.status).toBe(200); + expect(res.body.name).toBe(name); + }); +}); diff --git a/backend/src/routes/adminCategories.js b/backend/src/routes/adminCategories.js index d5ae2404..e7bddada 100644 --- a/backend/src/routes/adminCategories.js +++ b/backend/src/routes/adminCategories.js @@ -40,7 +40,11 @@ router.get('/event/:eventId', adminAuth, requirePermission('settings.view'), req // Create a new category router.post('/', adminAuth, requirePermission('settings.edit'), [ - body('name').notEmpty().withMessage('Category name is required'), + // photo_categories.name is varchar(100) — without the length check Postgres + // raises "value too long" and the catch below turns it into a raw 500 with + // no usable message for the form. + body('name').notEmpty().withMessage('Category name is required') + .isLength({ max: 100 }).withMessage('Category name must be at most 100 characters'), body('slug').optional(), body('is_global').optional().isBoolean(), body('event_id').optional().isInt(), @@ -127,7 +131,8 @@ router.post('/', adminAuth, requirePermission('settings.edit'), [ // Update a category router.put('/:id', adminAuth, requirePermission('settings.edit'), [ - body('name').notEmpty().withMessage('Category name is required'), + body('name').notEmpty().withMessage('Category name is required') + .isLength({ max: 100 }).withMessage('Category name must be at most 100 characters'), body('hero_photo_id').optional({ nullable: true }).custom((value) => { if (value === null || value === undefined) return true; return Number.isInteger(Number(value)); diff --git a/frontend/src/components/admin/CategoryManager.tsx b/frontend/src/components/admin/CategoryManager.tsx index 786d28ad..712b0829 100644 --- a/frontend/src/components/admin/CategoryManager.tsx +++ b/frontend/src/components/admin/CategoryManager.tsx @@ -138,6 +138,7 @@ export const CategoryManager: React.FC = () => { onChange={(e) => setNewCategoryName(e.target.value)} onKeyPress={(e) => e.key === 'Enter' && handleCreate()} placeholder={t('categories.categoryName')} + maxLength={100} className="flex-1 px-3 py-2 border border-neutral-300 dark:border-neutral-600 rounded-md bg-white dark:bg-neutral-800 text-neutral-900 dark:text-neutral-100 focus:ring-2 focus:ring-primary-500" autoFocus /> @@ -188,6 +189,7 @@ export const CategoryManager: React.FC = () => { if (e.key === 'Enter') handleUpdate(category.id); if (e.key === 'Escape') cancelEdit(); }} + maxLength={100} className="flex-1 px-3 py-1 border border-neutral-300 dark:border-neutral-600 rounded-md bg-white dark:bg-neutral-800 text-neutral-900 dark:text-neutral-100 focus:ring-2 focus:ring-primary-500" autoFocus /> diff --git a/frontend/src/components/admin/EventCategoryManager.tsx b/frontend/src/components/admin/EventCategoryManager.tsx index cfcea573..a49127f1 100644 --- a/frontend/src/components/admin/EventCategoryManager.tsx +++ b/frontend/src/components/admin/EventCategoryManager.tsx @@ -206,6 +206,7 @@ export const EventCategoryManager: React.FC = ({ even onChange={(e) => setNewCategoryName(e.target.value)} onKeyPress={(e) => e.key === 'Enter' && handleCreate()} placeholder={t('categories.categoryName')} + maxLength={100} className="flex-1 px-3 py-1.5 text-sm border border-neutral-300 dark:border-neutral-600 rounded-md bg-white dark:bg-neutral-800 text-neutral-900 dark:text-neutral-100 focus:ring-2 focus:ring-primary-500" autoFocus /> From 3489610cb8d9334198daffd9bf86ae488061f4b8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:23:40 +0200 Subject: [PATCH 04/33] fix(analytics): warn about the CSP allowlist on every tracker provider A self-hosted Umami/Rybbit domain configured in Settings -> Analytics is always blocked by the static script-src allowlist, silently, with only a console error. The amber CSP warning that explains this already existed but was rendered only inside the "custom" provider panel -- not on the two providers where an admin actually types a self-hosted URL. Extract it to a local CspWarning and render it in the Umami and Rybbit panels too. Both translation keys already exist in en.json/de.json. Interpretation: the dynamic-CSP option was investigated and rejected as not reachable for the header that actually governs these documents. In the Docker deployment nginx.conf:58 does `proxy_hide_header Content-Security-Policy`, so helmet's CSP and the res.setHeader CSP at server.js:445 are stripped before they leave the stack -- nginx's static server-level CSP is the only one the browser sees for the SPA documents the tracker is injected into. nginx.conf is COPYied verbatim by the Dockerfile (only index.html goes through envsubst), and the tracker URL lives in the DB rather than the environment, so making it reflect the setting would need start-time templating plus a DB read. The CSP itself therefore still has to be edited by hand; the warning now says so where the admin can see it. Refs testplan REPORT.md #18 (Part 3, B.02). --- .../features/settings/tabs/AnalyticsTab.tsx | 50 +++++++++++++------ 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/frontend/src/features/settings/tabs/AnalyticsTab.tsx b/frontend/src/features/settings/tabs/AnalyticsTab.tsx index e266bba4..6b178977 100644 --- a/frontend/src/features/settings/tabs/AnalyticsTab.tsx +++ b/frontend/src/features/settings/tabs/AnalyticsTab.tsx @@ -15,6 +15,35 @@ interface AnalyticsTabProps { const PROVIDER_OPTIONS: TrackerProvider[] = ['none', 'umami', 'rybbit', 'custom']; +/** + * The shipped CSP `script-src` is a static allowlist that no configured + * tracker domain is ever added to, so a self-hosted Umami/Rybbit instance is + * blocked by the browser with nothing but a console error to show for it. + * Shown for every provider that loads a script from another origin. + */ +const CspWarning: React.FC = () => { + const { t } = useTranslation(); + + return ( +
+
+ +
+

+ {t('settings.analytics.customCspWarning', 'Content-Security-Policy reminder')} +

+

+ {t( + 'settings.analytics.customCspWarningText', + 'PicPeak ships with a strict CSP (`script-src \'self\'`). If your tracker loads from another domain, add that domain to your reverse-proxy or nginx CSP config — otherwise the browser silently blocks the script.', + )} +

+
+
+
+ ); +}; + export const AnalyticsTab: React.FC = ({ analyticsSettings, setAnalyticsSettings, @@ -128,6 +157,8 @@ export const AnalyticsTab: React.FC = ({ )}

+ + )} @@ -191,6 +222,8 @@ export const AnalyticsTab: React.FC = ({ )}

+ + )} @@ -222,22 +255,7 @@ export const AnalyticsTab: React.FC = ({

-
-
- -
-

- {t('settings.analytics.customCspWarning', 'Content-Security-Policy reminder')} -

-

- {t( - 'settings.analytics.customCspWarningText', - 'PicPeak ships with a strict CSP (`script-src \'self\'`). If your tracker loads from another domain, add that domain to your reverse-proxy or nginx CSP config — otherwise the browser silently blocks the script.', - )} -

-
-
-
+ )} From c2428aa23a77c7f4910f39066ebe69c12dac6125 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:28:21 +0200 Subject: [PATCH 05/33] fix(events): render a not-found state instead of hanging on a 404 EventDetailsPage gated on `if (eventLoading || !event)`. The backend returns a clean 404 for a nonexistent id, but once isLoading settled false `event` stayed undefined forever, so /admin/events/999999 sat on the loading spinner permanently with no error state. Destructure isError and split the gate: spinner while loading, then a not-found Card. Reuses the existing `events.notFound` key (already used by EventFeedbackPage for the same entity) and the Card padding="lg" not-found shape from contracts/ContractDetailPage. No new i18n keys. Refs testplan REPORT.md #5 (Part 7.02). --- frontend/src/pages/admin/EventDetailsPage.tsx | 19 ++++- .../__tests__/eventDetailsNotFound.test.tsx | 84 +++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 frontend/src/pages/admin/__tests__/eventDetailsNotFound.test.tsx diff --git a/frontend/src/pages/admin/EventDetailsPage.tsx b/frontend/src/pages/admin/EventDetailsPage.tsx index 5c5b3823..d9c58e40 100644 --- a/frontend/src/pages/admin/EventDetailsPage.tsx +++ b/frontend/src/pages/admin/EventDetailsPage.tsx @@ -5,7 +5,7 @@ import { useTranslation } from 'react-i18next'; import { toast } from 'react-toastify'; import { useLocalizedDate } from '../../hooks/useLocalizedDate'; -import { Loading } from '../../components/common'; +import { Button, Card, Loading } from '../../components/common'; import { PasswordResetModal, PublishGalleryDialog, SendGalleryEmailDialog, DuplicateEventDialog, EventRenameDialog, AdminGuestsList } from '../../components/admin'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { eventsService } from '../../services/events.service'; @@ -101,7 +101,7 @@ export const EventDetailsPage: React.FC = () => { }); // Fetch event details - const { data: event, isLoading: eventLoading, refetch: refetchEvent } = useQuery({ + const { data: event, isLoading: eventLoading, isError: eventError, refetch: refetchEvent } = useQuery({ queryKey: ['admin-event', id], queryFn: () => eventsService.getEvent(parseInt(id!)), enabled: !!id, @@ -324,7 +324,7 @@ export const EventDetailsPage: React.FC = () => { }, }); - if (eventLoading || !event) { + if (eventLoading) { return (
@@ -332,6 +332,19 @@ export const EventDetailsPage: React.FC = () => { ); } + // A 404 (or any settled failure) leaves `event` undefined forever — without + // this branch the spinner above never resolved (QA 7.02). + if (eventError || !event) { + return ( + +

{t('events.notFound', 'Event not found')}

+ +
+ ); + } + const expiresAtDate = safeParseDate(event.expires_at); // Timestamp comparison, not truncated whole days (#909): the old // differenceInDays <= 0 marked events "expired" up to 24h early. diff --git a/frontend/src/pages/admin/__tests__/eventDetailsNotFound.test.tsx b/frontend/src/pages/admin/__tests__/eventDetailsNotFound.test.tsx new file mode 100644 index 00000000..621f7cdb --- /dev/null +++ b/frontend/src/pages/admin/__tests__/eventDetailsNotFound.test.tsx @@ -0,0 +1,84 @@ +/** + * /admin/events/:id hung on the spinner forever for a nonexistent id + * (QA 7.02). The backend returns a clean 404, but the page gated on + * `eventLoading || !event`, so once the query settled `event` stayed + * undefined and the condition never went false. + */ +import React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter, Route, Routes } from 'react-router-dom'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k), + i18n: { language: 'en' }, + }), + }; +}); + +vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), info: vi.fn() } })); + +const getEvent = vi.fn(); +vi.mock('../../../services/events.service', () => ({ + eventsService: { + getEvent: (...args: unknown[]) => getEvent(...args), + updateEvent: vi.fn(), + deleteEvent: vi.fn(), + extendExpiration: vi.fn(), + duplicateEvent: vi.fn(), + resetPassword: vi.fn(), + publishEvent: vi.fn(), + renameEvent: vi.fn(), + }, +})); + +vi.mock('../../../hooks/usePublicSettings', () => ({ + PUBLIC_SETTINGS_QUERY_KEY: ['public-settings'], + usePublicSettings: () => ({ data: {} }), +})); + +vi.mock('../../../contexts/FeatureFlagsContext', () => ({ + useFeatureFlags: () => ({ flags: {}, isLoading: false }), + useFeatureEnabled: () => false, +})); + +vi.mock('../../../contexts/PermissionsContext', () => ({ + usePermissions: () => ({ hasAnyPermission: () => true, hasPermission: () => true, isLoading: false }), +})); + +import { EventDetailsPage } from '../EventDetailsPage'; + +function renderPage() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + } /> + events list
} /> + + + + ); +} + +describe('EventDetailsPage 404 handling (QA 7.02)', () => { + it('renders a not-found state instead of spinning forever when the event 404s', async () => { + getEvent.mockRejectedValue({ response: { status: 404, data: { error: 'Event not found' } } }); + + renderPage(); + + expect(screen.getByText('events.loadingEventDetails')).toBeInTheDocument(); + + await waitFor(() => { + expect(screen.getByText('Event not found')).toBeInTheDocument(); + }); + expect(screen.queryByText('events.loadingEventDetails')).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'events.backToEvents' })).toBeInTheDocument(); + }); +}); From 673f05556d0f33e59a325714e5a2679a3c4356b6 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:28:21 +0200 Subject: [PATCH 06/33] fix(settings): don't crash on a fresh load before permissions resolve On a hard navigation or deep link, usePermissions() starts out empty, which filters every settings nav group down to nothing. allItems is then [], so `allItems.find(...) ?? allItems[0]` yields undefined and `` threw -- sometimes into the error boundary, sometimes racing past it. Reproduced 6+ times across the webhooks/moderation/slideshow/security/events tabs; in-app SPA navigation never hit it. Extend the file's existing early-return to `isLoading || permissionsLoading`. activeTab lives in useState seeded from ?tab= at mount, independent of the gate, so deep links still land on the right tab once permissions arrive. Also null-guard activeItem before the section heading: a role holding zero settings-tab permissions crashes identically even after permissions finish loading, which the loading gate alone does not cover. Refs testplan REPORT.md #11 (Part 3, J.08). --- frontend/src/pages/admin/SettingsPage.tsx | 10 +- .../__tests__/settingsPageMountRace.test.tsx | 110 ++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 frontend/src/pages/admin/__tests__/settingsPageMountRace.test.tsx diff --git a/frontend/src/pages/admin/SettingsPage.tsx b/frontend/src/pages/admin/SettingsPage.tsx index 4337bd04..0450592f 100644 --- a/frontend/src/pages/admin/SettingsPage.tsx +++ b/frontend/src/pages/admin/SettingsPage.tsx @@ -170,7 +170,7 @@ export const SettingsPage: React.FC = () => { const { t } = useTranslation(); const [searchParams, setSearchParams] = useSearchParams(); const { flags, isLoading: flagsLoading } = useFeatureFlags(); - const { hasAnyPermission } = usePermissions(); + const { hasAnyPermission, isLoading: permissionsLoading } = usePermissions(); // Read ?tab=… on mount; default to Features per the redesign. const initialTab: TabType = isValidTab(searchParams.get('tab')) @@ -288,7 +288,11 @@ export const SettingsPage: React.FC = () => { // eslint-disable-next-line react-hooks/exhaustive-deps }, [flagsLoading, activeTab, flags.quotes, flags.bills, flags.contracts, flags.reminderEmails, flags.accounting, flags.whatsapp, flags.slideshow]); - if (isLoading) { + // Wait for the permissions context too: on a fresh/hard mount it starts out + // empty, which filters every nav group down to nothing and left `activeItem` + // undefined below (QA J.08 crash). `activeTab` is held in state, so a + // deep-linked ?tab= still lands on the right tab once permissions arrive. + if (isLoading || permissionsLoading) { return (
@@ -483,7 +487,7 @@ export const SettingsPage: React.FC = () => {
- {showSectionHeading && ( + {showSectionHeading && activeItem && (
{/* Section heading icon stays neutral so the Settings diff --git a/frontend/src/pages/admin/__tests__/settingsPageMountRace.test.tsx b/frontend/src/pages/admin/__tests__/settingsPageMountRace.test.tsx new file mode 100644 index 00000000..934538de --- /dev/null +++ b/frontend/src/pages/admin/__tests__/settingsPageMountRace.test.tsx @@ -0,0 +1,110 @@ +/** + * Settings crashed on a fresh/hard load of any non-default tab (QA J.08). + * + * The nav groups are permission-filtered, so before PermissionsContext has + * resolved every group filters to empty, `allItems[0]` is undefined, and the + * section heading's `` throws. In-app SPA navigation never + * hit it because the context was already warm. + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k) }), + }; +}); + +const flagsState = { flags: {} as Record, isLoading: false }; +vi.mock('../../../contexts/FeatureFlagsContext', () => ({ + useFeatureFlags: () => flagsState, + useFeatureEnabled: () => false, +})); + +const permissionsState = { hasAnyPermission: (_: string[]) => true, isLoading: false }; +vi.mock('../../../contexts/PermissionsContext', () => ({ + usePermissions: () => permissionsState, +})); + +// The settings barrel pulls in every tab; stub it down to the shell's needs. +vi.mock('../../../features/settings', () => { + const Stub = () => null; + return { + useSettingsState: () => ({ isLoading: false }), + FeaturesTab: Stub, + GeneralTab: Stub, + EventsTab: Stub, + StatusTab: Stub, + SecurityTab: Stub, + ImageSecurityTab: Stub, + CategoriesTab: Stub, + AnalyticsTab: Stub, + ModerationTab: Stub, + StylingTab: Stub, + SEOTab: Stub, + ThumbnailsTab: Stub, + DownloadsTab: Stub, + ApiTokensTab: Stub, + WebhooksTab: Stub, + AccountingTab: Stub, + WhatsAppTab: Stub, + SsoTab: Stub, + }; +}); + +vi.mock('../EmailConfigPage', () => ({ EmailConfigPage: () => null })); +vi.mock('../BrandingPage', () => ({ BrandingPage: () => null })); +vi.mock('../EventTypesPage', () => ({ EventTypesPage: () => null })); +vi.mock('../SlideshowSettingsPage', () => ({ SlideshowSettingsPage: () => null })); +vi.mock('../BackupManagement', () => ({ BackupManagement: () => null })); +vi.mock('../CMSPage', () => ({ CMSPage: () => null })); +vi.mock('../settings/SettingsBusinessProfilePage', () => ({ SettingsBusinessProfilePage: () => null })); +vi.mock('../settings/CrmSettingsPage', () => ({ CrmSettingsPage: () => null })); +vi.mock('../settings/ReminderTemplatesPage', () => ({ ReminderTemplatesPage: () => null })); +vi.mock('../contracts/BlockLibraryPage', () => ({ BlockLibraryPage: () => null })); + +import { SettingsPage } from '../SettingsPage'; + +function renderAt(tab: string) { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + + + ); +} + +describe('SettingsPage fresh-mount permission race (QA J.08)', () => { + beforeEach(() => { + permissionsState.hasAnyPermission = () => true; + permissionsState.isLoading = false; + }); + + it('does not crash on a deep-linked tab while permissions are still loading', () => { + permissionsState.isLoading = true; + permissionsState.hasAnyPermission = () => false; + + expect(() => renderAt('webhooks')).not.toThrow(); + expect(screen.getByText('settings.loadingSettings')).toBeInTheDocument(); + }); + + it('still lands on the deep-linked tab once permissions arrive', () => { + renderAt('webhooks'); + + expect(screen.getByRole('heading', { level: 2, name: 'Webhooks' })).toBeInTheDocument(); + }); + + it('does not crash when the role has no settings tab permissions at all', () => { + permissionsState.hasAnyPermission = () => false; + + expect(() => renderAt('webhooks')).not.toThrow(); + expect(screen.getByText('settings.title')).toBeInTheDocument(); + }); +}); From c19e944b995dfb6c54c0cb5da5772700000613d8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:28:31 +0200 Subject: [PATCH 07/33] fix(events): guard create-event submit against re-entrant submissions Correction to the QA root cause: the submit Button has carried `disabled={createMutation.isPending}` since 3424bd22, and it does disable synchronously after the first click (validateForm's setErrors forces a re-render that re-reads the mutation snapshot), so an ordinary double-click could not by itself produce two POSTs. What was actually missing is a re-entrancy guard in handleSubmit, so any submission that never touches the button -- implicit form submission, a programmatic requestSubmit, or two submit events dispatched in one task, which is the likely shape of the QA repro -- still fired two mutate() calls racing the same computed slug, one of which 500'd on events_slug_unique. Add isSubmittingRef (matching the isMountedRef idiom already in this file), cleared in onSettled. Test proves 2 submit events -> 1 POST. Not done: turning the backend's raw 500 on events_slug_unique into a graceful "event already exists" 409. That is an adminEvents.js change and a separate concern from the client-side race. Refs testplan REPORT.md #6 (Part 7.03). --- frontend/src/pages/admin/CreateEventPage.tsx | 16 +- .../createEventDoubleSubmit.test.tsx | 138 ++++++++++++++++++ 2 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 frontend/src/pages/admin/__tests__/createEventDoubleSubmit.test.tsx diff --git a/frontend/src/pages/admin/CreateEventPage.tsx b/frontend/src/pages/admin/CreateEventPage.tsx index 8c39c0c6..b0a46cc3 100644 --- a/frontend/src/pages/admin/CreateEventPage.tsx +++ b/frontend/src/pages/admin/CreateEventPage.tsx @@ -99,6 +99,12 @@ export const CreateEventPage: React.FC = () => { const { t } = useTranslation(); const { format } = useLocalizedDate(); const isMountedRef = useRef(true); + // Re-entrancy guard for the create submit. The Button's + // `disabled={createMutation.isPending}` covers the ordinary double-click, but + // not a submission that never touches the button (implicit form submission, + // a programmatic requestSubmit) — those raced two POSTs onto the same slug, + // one of which 500'd on `events_slug_unique` (QA 7.03). + const isSubmittingRef = useRef(false); const [showThemeCustomizer, setShowThemeCustomizer] = useState(false); // const [showPreview, setShowPreview] = useState(false); @@ -396,6 +402,9 @@ export const CreateEventPage: React.FC = () => { toast.error(errorMessage); } }, + onSettled: () => { + isSubmittingRef.current = false; + }, }); const validateForm = (): boolean => { @@ -461,7 +470,11 @@ export const CreateEventPage: React.FC = () => { const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); - + + if (isSubmittingRef.current) { + return; + } + if (!validateForm()) { return; } @@ -518,6 +531,7 @@ export const CreateEventPage: React.FC = () => { customer_account_ids: formData.customer_accounts.map((c) => c.id), }; + isSubmittingRef.current = true; createMutation.mutate(payload); }; diff --git a/frontend/src/pages/admin/__tests__/createEventDoubleSubmit.test.tsx b/frontend/src/pages/admin/__tests__/createEventDoubleSubmit.test.tsx new file mode 100644 index 00000000..6043fd36 --- /dev/null +++ b/frontend/src/pages/admin/__tests__/createEventDoubleSubmit.test.tsx @@ -0,0 +1,138 @@ +/** + * "Create event" could fire two real POSTs racing the same slug — one 500'd on + * `events_slug_unique` (QA 7.03). + * + * The Button already carried `disabled={createMutation.isPending}`, which + * covers the ordinary double-click. `handleSubmit` itself had no re-entrancy + * guard though, so any submit that does not go through the button (implicit + * form submission, a programmatic `requestSubmit`) still raced a second POST. + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k), + i18n: { language: 'en' }, + }), + }; +}); + +vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), info: vi.fn() } })); + +const createEvent = vi.fn(); +vi.mock('../../../services/events.service', () => ({ + eventsService: { createEvent: (...args: unknown[]) => createEvent(...args) }, +})); + +vi.mock('../../../services/categories.service', () => ({ + categoriesService: { getCategories: vi.fn(async () => []) }, +})); +vi.mock('../../../services/settings.service', () => ({ + settingsService: { getAllSettings: vi.fn(async () => ({})) }, +})); +vi.mock('../../../services/cssTemplates.service', () => ({ + cssTemplatesService: { getEnabledTemplates: vi.fn(async () => []) }, +})); +vi.mock('../../../services/eventTypes.service', () => ({ + eventTypesService: { getEventTypes: vi.fn(async () => []) }, +})); +vi.mock('../../../services/userManagement.service', () => ({ + userManagementService: { getUsers: vi.fn(async () => []) }, +})); + +// Every "is this field required" flag off, so the only thing validateForm +// needs is the event name. +vi.mock('../../../hooks/usePublicSettings', () => ({ + PUBLIC_SETTINGS_QUERY_KEY: ['public-settings'], + usePublicSettings: () => ({ + data: { + event_require_customer_name: false, + event_require_customer_email: false, + event_require_admin_email: false, + event_require_event_date: false, + event_require_expiration: false, + event_default_require_password: false, + }, + }), +})); + +vi.mock('../../../contexts/AdminAuthContext', () => ({ + useAdminAuth: () => ({ user: null }), +})); + +vi.mock('../../../contexts/FeatureFlagsContext', () => ({ + useFeatureFlags: () => ({ flags: {}, isLoading: false }), + useFeatureEnabled: () => false, +})); + +// Heavy children not involved in the submit path. +vi.mock('../../../components/admin', async () => { + const actual = await vi.importActual('../../../components/admin'); + return { + ...actual, + ThemeCustomizerEnhanced: () => null, + GalleryPreview: () => null, + WelcomeMessageEditor: () => null, + FeedbackSettings: () => null, + }; +}); +vi.mock('../../../components/admin/CustomerAccountPicker', () => ({ + CustomerAccountPicker: () => null, +})); + +import { CreateEventPage } from '../CreateEventPage'; + +function renderPage() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + + + ); +} + +describe('CreateEventPage double-submit guard (QA 7.03)', () => { + beforeEach(() => { + createEvent.mockReset(); + // Never settles — keeps the mutation in flight for the whole test. + createEvent.mockImplementation(() => new Promise(() => {})); + }); + + it('fires exactly one POST when the form is submitted twice in a row', async () => { + renderPage(); + + fireEvent.change(screen.getByPlaceholderText('events.eventNamePlaceholder'), { + target: { value: 'ZZTEST double submit' }, + }); + + const form = screen.getByRole('button', { name: 'events.createEvent' }).closest('form')!; + fireEvent.submit(form); + fireEvent.submit(form); + + await waitFor(() => expect(createEvent).toHaveBeenCalled()); + expect(createEvent).toHaveBeenCalledTimes(1); + }); + + it('disables the submit button while the request is in flight', async () => { + renderPage(); + + fireEvent.change(screen.getByPlaceholderText('events.eventNamePlaceholder'), { + target: { value: 'ZZTEST in flight' }, + }); + + const submit = screen.getByRole('button', { name: 'events.createEvent' }) as HTMLButtonElement; + fireEvent.click(submit); + + expect(submit).toBeDisabled(); + await waitFor(() => expect(createEvent).toHaveBeenCalledTimes(1)); + }); +}); From 31ffbc8ae40f3b913649ac11a1f5f5b1a14a130f Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:28:31 +0200 Subject: [PATCH 08/33] fix(users): give the cancel-invitation dialog a distinct confirm label The cancelInvitation dialog type fell through to the generic t('userManagement.cancel'), colliding with ConfirmDialog's own dismiss button -- two buttons both reading "Cancel", where clicking the wrong one does the opposite of what the user intends. Reuse the existing userManagement.cancelInvitation key: "Cancel Invitation" vs "Cancel" (EN), "Einladung abbrechen" vs "Abbrechen" (DE). No new key. Refs testplan REPORT.md #19 (Part 3, I.04). --- frontend/src/pages/admin/UserManagementPage.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/pages/admin/UserManagementPage.tsx b/frontend/src/pages/admin/UserManagementPage.tsx index a1a6e5f3..c05a62f3 100644 --- a/frontend/src/pages/admin/UserManagementPage.tsx +++ b/frontend/src/pages/admin/UserManagementPage.tsx @@ -1023,7 +1023,9 @@ export const UserManagementPage: React.FC = () => { confirmDialog.type === 'deactivate' ? t('userManagement.deactivate') : confirmDialog.type === 'activate' ? t('userManagement.activate', 'Reactivate') : confirmDialog.type === 'delete' ? t('userManagement.delete', 'Delete permanently') - : t('userManagement.cancel') + // Not the generic `cancel` — that collides with ConfirmDialog's own + // dismiss button, giving the dialog two "Cancel" buttons (QA I.04). + : t('userManagement.cancelInvitation') } isLoading={ confirmDialog.type === 'deactivate' ? deactivateUserMutation.isPending From c5c5a6b0c87ad7a3797e7f8ab695c5f64abfacf8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:29:03 +0200 Subject: [PATCH 09/33] fix(webhooks): write delivery timestamps as ISO strings Applies the repo's documented Jest+SQLite guidance (CLAUDE.md) to the webhook delivery path, which was the last one still passing raw Date objects into knex writes. Under jest those store as the literal string "[object Object]", so next_retry_at came back NaN and the retry/backoff test could not assert on it. Production (PG, and SQLite outside jest) was unaffected. Convert the timestamp writes -- and the `next_retry_at <=` due comparison, which has to stay type-consistent with them -- to .toISOString(), matching the existing precedent in downloadJobService.js. Refs testplan REPORT.md #22 (Part 1.2.01). --- .../integration/webhookDelivery.test.js | 12 ++++---- backend/src/services/webhookDeliveryWorker.js | 28 +++++++++---------- backend/src/services/webhookService.js | 8 +++--- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/backend/__tests__/integration/webhookDelivery.test.js b/backend/__tests__/integration/webhookDelivery.test.js index 14ed1ab5..35de5a18 100644 --- a/backend/__tests__/integration/webhookDelivery.test.js +++ b/backend/__tests__/integration/webhookDelivery.test.js @@ -146,8 +146,8 @@ describe('webhook delivery worker (#327)', () => { payload: JSON.stringify({ id: 'd1', type: 'event.published', data: {} }), attempt_count: 4, status: 'pending', - next_retry_at: new Date(), - created_at: new Date(), + next_retry_at: new Date().toISOString(), + created_at: new Date().toISOString(), }); await __test.tick(); @@ -190,8 +190,8 @@ describe('webhook delivery worker (#327)', () => { payload: JSON.stringify({ id: 'd1', type: 'event.published', data: {} }), attempt_count: 0, status: 'pending', - next_retry_at: new Date(), - created_at: new Date(), + next_retry_at: new Date().toISOString(), + created_at: new Date().toISOString(), }); await __test.tick(); @@ -214,8 +214,8 @@ describe('webhook delivery worker (#327)', () => { payload: JSON.stringify({ id: 'd1', type: 'event.published', data: {} }), attempt_count: 0, status: 'pending', - next_retry_at: new Date(), - created_at: new Date(), + next_retry_at: new Date().toISOString(), + created_at: new Date().toISOString(), }); await __test.tick(); diff --git a/backend/src/services/webhookDeliveryWorker.js b/backend/src/services/webhookDeliveryWorker.js index a619e05b..2bb91cb4 100644 --- a/backend/src/services/webhookDeliveryWorker.js +++ b/backend/src/services/webhookDeliveryWorker.js @@ -51,7 +51,7 @@ async function fetchPending(limit) { const excludeIds = Array.from(inFlight); let q = db('webhook_deliveries') .where('status', 'pending') - .where('next_retry_at', '<=', new Date()) + .where('next_retry_at', '<=', new Date().toISOString()) .orderBy('next_retry_at', 'asc') .limit(limit); if (excludeIds.length > 0) { @@ -71,7 +71,7 @@ async function deliverOne(row) { .update({ status: 'failed', last_error: 'webhook subscription no longer exists', - completed_at: new Date(), + completed_at: new Date().toISOString(), attempt_count: row.attempt_count + 1, }); return; @@ -85,7 +85,7 @@ async function deliverOne(row) { .update({ status: 'failed', last_error: 'webhook is disabled', - completed_at: new Date(), + completed_at: new Date().toISOString(), attempt_count: row.attempt_count + 1, }); return; @@ -172,10 +172,10 @@ async function deliverOne(row) { response_body: truncate(stringifyBody(response.data), RESPONSE_TRUNCATE_BYTES), latency_ms: latency, attempt_count: newAttempt, - completed_at: new Date(), + completed_at: new Date().toISOString(), next_retry_at: null, }); - await db('webhooks').where({ id: webhook.id }).update({ last_success_at: new Date() }); + await db('webhooks').where({ id: webhook.id }).update({ last_success_at: new Date().toISOString() }); return; } @@ -194,10 +194,10 @@ async function deliverOne(row) { last_error: errorMsg, latency_ms: latency, attempt_count: newAttempt, - completed_at: new Date(), + completed_at: new Date().toISOString(), next_retry_at: null, }); - await db('webhooks').where({ id: webhook.id }).update({ last_failure_at: new Date() }); + await db('webhooks').where({ id: webhook.id }).update({ last_failure_at: new Date().toISOString() }); return; } @@ -211,9 +211,9 @@ async function deliverOne(row) { last_error: errorMsg, latency_ms: latency, attempt_count: newAttempt, - next_retry_at: new Date(Date.now() + backoff), + next_retry_at: new Date(Date.now() + backoff).toISOString(), }); - await db('webhooks').where({ id: webhook.id }).update({ last_failure_at: new Date() }); + await db('webhooks').where({ id: webhook.id }).update({ last_failure_at: new Date().toISOString() }); } async function markFailedFinal(row, reason) { @@ -223,10 +223,10 @@ async function markFailedFinal(row, reason) { status: 'failed', last_error: reason, attempt_count: row.attempt_count + 1, - completed_at: new Date(), + completed_at: new Date().toISOString(), next_retry_at: null, }); - await db('webhooks').where({ id: row.webhook_id }).update({ last_failure_at: new Date() }); + await db('webhooks').where({ id: row.webhook_id }).update({ last_failure_at: new Date().toISOString() }); } // Schedule the normal retry/backoff for a transient failure that must not @@ -242,7 +242,7 @@ async function scheduleTransientRetry(row, webhook, errorMsg) { status: 'failed', last_error: errorMsg, attempt_count: newAttempt, - completed_at: new Date(), + completed_at: new Date().toISOString(), next_retry_at: null, }); } else { @@ -253,10 +253,10 @@ async function scheduleTransientRetry(row, webhook, errorMsg) { status: 'pending', last_error: errorMsg, attempt_count: newAttempt, - next_retry_at: new Date(Date.now() + backoff), + next_retry_at: new Date(Date.now() + backoff).toISOString(), }); } - await db('webhooks').where({ id: webhook.id }).update({ last_failure_at: new Date() }); + await db('webhooks').where({ id: webhook.id }).update({ last_failure_at: new Date().toISOString() }); } function stringifyBody(data) { diff --git a/backend/src/services/webhookService.js b/backend/src/services/webhookService.js index 9f450d66..80e58c9d 100644 --- a/backend/src/services/webhookService.js +++ b/backend/src/services/webhookService.js @@ -190,8 +190,8 @@ async function fire(eventType, data) { payload: JSON.stringify(envelope), attempt_count: 0, status: 'pending', - next_retry_at: now, - created_at: now, + next_retry_at: now.toISOString(), + created_at: now.toISOString(), }); } @@ -232,8 +232,8 @@ async function enqueueForWebhook(webhookId, eventType, data) { payload: JSON.stringify(envelope), attempt_count: 0, status: 'pending', - next_retry_at: now, - created_at: now, + next_retry_at: now.toISOString(), + created_at: now.toISOString(), }); return { enqueued: true, webhookId: w.id, deliveryId: deliveryUuid }; } catch (err) { From 1d84c738d8e7df46246b0f896fceee36afe20813 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:29:03 +0200 Subject: [PATCH 10/33] test: repair four stale backend suites All four asserted contracts the product has since moved past. No genuine product bugs behind any of them; assertions were tightened, not loosened. adminAuth (3 tests): never mounted errorHandler, so ConflictError/ ValidationError arrived as empty Express defaults. The route also checks username before email, so the "email conflict" fixture was hitting the username branch. Mount the handler, fix the fixture, match the real response shapes. backupService.enhanced (12 tests): three stacked drifts -- the db mock had no .returning(), so every runBackup threw at the insert; ensureDatabaseDumpForBackup now lazily requires ./databaseBackup inside the run, which fails under mock-fs; and the rsync path moved from exec(shell string) to spawnAsync('rsync', args) with an isHostAllowed SSRF preflight. Also updates getBackupStatus to its current shape (frontend aliases, nextScheduledRun null when no schedule is enabled, #871). adminSettings.logo: POST /logo gained requirePermission('settings.edit'); the hand-rolled db mock returns a bare Promise from select(), so the permission lookup threw a TypeError into a 500. Mock the permissions middleware alongside the already-mocked auth. crmMintPaths (2 tests): macOS-only. The expected prefix was realpath'd while the services persist under the raw STORAGE_PATH -- identical on Linux CI (/var vs /private/var only diverges on macOS), which is why it passed there. The comment justifying the realpath referenced process.cwd() behaviour the services no longer have. Refs testplan REPORT.md #22 (Part 1.2.01). --- backend/__tests__/adminSettings.logo.test.js | 5 + .../integration/crmMintPaths.test.js | 11 +- .../services/backupService.enhanced.test.js | 155 ++++++++++++------ .../src/routes/__tests__/adminAuth.test.js | 24 ++- 4 files changed, 139 insertions(+), 56 deletions(-) diff --git a/backend/__tests__/adminSettings.logo.test.js b/backend/__tests__/adminSettings.logo.test.js index 16eb8911..0ff1c17d 100644 --- a/backend/__tests__/adminSettings.logo.test.js +++ b/backend/__tests__/adminSettings.logo.test.js @@ -110,6 +110,11 @@ describe('Admin settings logo upload flow', () => { } })); + jest.doMock('../src/middleware/permissions', () => ({ + requirePermission: () => (req, res, next) => next(), + userHasAnyPermission: jest.fn().mockResolvedValue(true) + })); + jest.doMock('../src/services/publicSiteService', () => ({ clearPublicSiteCache: jest.fn(), getDefaultPublicSitePayload: jest.fn(), diff --git a/backend/__tests__/integration/crmMintPaths.test.js b/backend/__tests__/integration/crmMintPaths.test.js index 74536a9c..92e19d6a 100644 --- a/backend/__tests__/integration/crmMintPaths.test.js +++ b/backend/__tests__/integration/crmMintPaths.test.js @@ -148,10 +148,15 @@ async function seedCustomerSignedContract() { beforeAll(async () => { ({ db, cleanup, tmpDir } = await bootCrmDb()); // Business-doc PDFs (quotes/invoices/contracts) persist under - // `process.cwd()/storage/business-docs/...` — chdir into the temp dir - // so every test artifact lands isolated and gets cleaned up. + // `getStoragePath()/business-docs/...`, and safePath also allows a + // `process.cwd()/storage/business-docs/...` root — chdir into the temp + // dir so every test artifact lands isolated and gets cleaned up. process.chdir(tmpDir); - storageRoot = path.join(fs.realpathSync(tmpDir), 'storage', 'business-docs'); + // Mirror what the services store: the raw STORAGE_PATH bootCrmDb + // exported, NOT a symlink-resolved variant. On macOS os.tmpdir() is + // /var/... while realpath is /private/var/..., so canonicalizing here + // would make every stored path fail the prefix check. + storageRoot = path.join(process.env.STORAGE_PATH, 'business-docs'); // Fail-fast on the pre-existing logActivity-inside-transaction // deadlock: createContract and createStorno call logActivity() from diff --git a/backend/__tests__/services/backupService.enhanced.test.js b/backend/__tests__/services/backupService.enhanced.test.js index e2349727..a300b9d2 100644 --- a/backend/__tests__/services/backupService.enhanced.test.js +++ b/backend/__tests__/services/backupService.enhanced.test.js @@ -11,8 +11,27 @@ jest.mock('../../src/services/emailProcessor'); jest.mock('node-cron'); jest.mock('../../src/services/backupManifest'); jest.mock('../../src/services/storage/s3Storage'); +// runBackup lazily requires this from inside the run — resolve+register it +// here so the require doesn't hit the (mock-fs'd) filesystem mid-backup. +jest.mock('../../src/services/databaseBackup', () => ({ + databaseBackupService: { + backup: jest.fn() + } +})); +// Same deal for the rsync path's lazy requires. +jest.mock('../../src/utils/safeExec', () => ({ + spawnAsync: jest.fn(), + spawnToFile: jest.fn(), + spawnFromFile: jest.fn() +})); +jest.mock('../../src/utils/networkValidation', () => ({ + isHostAllowed: jest.fn().mockResolvedValue(true) +})); const backupService = require('../../src/services/backupService'); +const { databaseBackupService } = require('../../src/services/databaseBackup'); +const { spawnAsync } = require('../../src/utils/safeExec'); +const { isHostAllowed } = require('../../src/utils/networkValidation'); const { db } = require('../../src/database/db'); const logger = require('../../src/utils/logger'); const { queueEmail } = require('../../src/services/emailProcessor'); @@ -20,6 +39,20 @@ const cron = require('node-cron'); const backupManifest = require('../../src/services/backupManifest'); const S3StorageAdapter = require('../../src/services/storage/s3Storage'); +// `runBackup` opens the run row with `db('backup_runs').insert(...).returning('id')`, +// so the insert mock has to be awaitable AND carry a `.returning()`. +const insertResult = (value) => { + const thenable = Promise.resolve(value); + thenable.returning = jest.fn().mockResolvedValue(value); + return thenable; +}; + +// Every runBackup goes through ensureDatabaseDumpForBackup, which stats the +// DB dump on disk and refuses to continue without it — seed it into every +// mock-fs tree. +const DB_DUMP_PATH = '/backup/db-dump.sql'; +const mockStorage = (tree) => mockFs({ [DB_DUMP_PATH]: Buffer.from('database dump'), ...tree }); + describe('Enhanced Backup Service Tests', () => { let mockDb; let mockS3Client; @@ -36,7 +69,7 @@ describe('Enhanced Backup Service Tests', () => { orderBy: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), first: jest.fn(), - insert: jest.fn(), + insert: jest.fn(() => insertResult([1])), update: jest.fn(), delete: jest.fn() }; @@ -75,6 +108,19 @@ describe('Enhanced Backup Service Tests', () => { logger.error = jest.fn(); logger.warn = jest.fn(); logger.debug = jest.fn(); + + // The inline DB dump and its on-disk verification run on every backup and + // throw when no dump is available — give both a passing default so each + // test can focus on the destination path it actually covers. + databaseBackupService.backup.mockResolvedValue({ path: DB_DUMP_PATH, size: 13 }); + isHostAllowed.mockResolvedValue(true); + jest.spyOn(backupService, 'getDatabaseBackupInfo').mockResolvedValue({ + type: 'sqlite', + backupFile: DB_DUMP_PATH, + size: 13, + checksum: 'abc123', + hasChanged: false + }); }); afterEach(() => { @@ -132,7 +178,7 @@ describe('Enhanced Backup Service Tests', () => { describe('S3 Backup Functionality', () => { beforeEach(() => { // Mock file system - mockFs({ + mockStorage({ '/storage/events/active/event1': { 'photo1.jpg': Buffer.from('photo1 content'), 'photo2.jpg': Buffer.from('photo2 content') @@ -165,12 +211,12 @@ describe('Enhanced Backup Service Tests', () => { mockDb.select.mockResolvedValue([]); mockDb.where.mockReturnThis(); mockDb.first.mockResolvedValue(null); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); jest.spyOn(backupService, 'getDatabaseBackupInfo').mockResolvedValue({ type: 'sqlite', - backupFile: null, + backupFile: DB_DUMP_PATH, hasChanged: true }); @@ -202,7 +248,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -240,7 +286,7 @@ describe('Enhanced Backup Service Tests', () => { }); mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -265,7 +311,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); jest.spyOn(backupService, 'getDatabaseBackupInfo').mockResolvedValue({ @@ -277,7 +323,7 @@ describe('Enhanced Backup Service Tests', () => { }); // Mock database backup file - mockFs({ + mockStorage({ '/storage/events/active': {}, '/backup/db-backup.sql': Buffer.from('database backup content') }); @@ -301,7 +347,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -326,12 +372,12 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active/event1': { 'photo1.jpg': Buffer.from('photo1 content') }, @@ -365,7 +411,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([2]); + mockDb.insert.mockReturnValue(insertResult([2])); mockDb.first.mockImplementation(() => Promise.resolve(lastBackup)); mockDb.orderBy.mockReturnThis(); mockDb.where.mockReturnThis(); @@ -373,7 +419,7 @@ describe('Enhanced Backup Service Tests', () => { jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active': {}, '/backup': {} }); @@ -395,7 +441,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -406,7 +452,7 @@ describe('Enhanced Backup Service Tests', () => { }; backupManifest.generateManifest.mockResolvedValue(manifest); - mockFs({ + mockStorage({ '/storage/events/active': {}, '/storage/temp': {} }); @@ -431,12 +477,12 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active/event1': { 'photo1.jpg': Buffer.from('photo1 content') }, @@ -461,28 +507,27 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - - // Mock exec for rsync - const { exec } = require('child_process'); - const mockExec = jest.fn((cmd, callback) => { - callback(null, { stdout: 'Number of files transferred: 1\nTotal file size: 1024 bytes' }); + + // rsync is spawned argv-style (no shell) — assert that shape, not the + // legacy `exec('rsync ...')` string. + spawnAsync.mockResolvedValue({ + stdout: 'Number of files transferred: 1\nTotal file size: 1024 bytes' }); - exec.mockImplementation(mockExec); - - mockFs({ + + mockStorage({ '/storage/events/active': {} }); - + await backupService.runBackup(); - - expect(mockExec).toHaveBeenCalledWith( - expect.stringContaining('rsync'), - expect.any(Function) - ); + + expect(spawnAsync).toHaveBeenCalledWith('rsync', expect.any(Array)); + const [, rsyncArgs] = spawnAsync.mock.calls[0]; + expect(rsyncArgs).toContain('-avz'); + expect(rsyncArgs[rsyncArgs.length - 1]).toBe('backup@backup.example.com:/remote/backup'); }); }); @@ -497,7 +542,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -514,7 +559,7 @@ describe('Enhanced Backup Service Tests', () => { return originalCreateReadStream(path); }); - mockFs({ + mockStorage({ '/storage/events/active': { 'error.jpg': Buffer.from('content'), 'good.jpg': Buffer.from('content') @@ -546,7 +591,7 @@ describe('Enhanced Backup Service Tests', () => { ]; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.where.mockReturnThis(); jest.spyOn(backupService, 'getBackupConfig') @@ -555,7 +600,13 @@ describe('Enhanced Backup Service Tests', () => { // Force an error jest.spyOn(backupService, 'getFilesToBackup').mockRejectedValue(new Error('Storage error')); - + + // The DB-dump verification runs first and would throw its own error — + // give it a tree so 'Storage error' is what actually surfaces. + mockStorage({ + '/storage/events/active': {} + }); + // Mock admin users query db.mockImplementation((table) => { if (table === 'admin_users') { @@ -588,7 +639,7 @@ describe('Enhanced Backup Service Tests', () => { jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active': {}, '/backup': {} }); @@ -675,20 +726,32 @@ describe('Enhanced Backup Service Tests', () => { ]; mockDb.limit.mockResolvedValue(recentRuns); - + // getBackupStatus also reads the backup config to compute the next run; + // an unscheduled/disabled backup legitimately yields null (#871). + mockDb.select.mockResolvedValue([ + { setting_key: 'backup_enabled', setting_value: 'true' }, + { setting_key: 'backup_schedule', setting_value: '"daily"' } + ]); + backupManifest.validateManifest.mockImplementation(() => true); - + const status = await backupService.getBackupStatus(); - + + // Runs are returned with a `created_at` alias for the frontend. + const run = { ...recentRuns[0], created_at: recentRuns[0].started_at }; + expect(status).toEqual({ isRunning: false, isHealthy: true, - lastRun: expect.objectContaining({ - ...recentRuns[0], - manifestValid: true - }), - recentRuns: recentRuns, - nextScheduledRun: expect.any(String) + lastRun: { ...run, manifestValid: true }, + lastBackup: { ...run, manifestValid: true }, + lastSuccessfulBackup: run, + zombieRuns: [], + recentRuns: [run], + recentBackups: [run], + totalBackups: 1, + nextScheduledRun: expect.any(String), + nextBackup: expect.any(String) }); }); diff --git a/backend/src/routes/__tests__/adminAuth.test.js b/backend/src/routes/__tests__/adminAuth.test.js index 934fd464..8ce68347 100644 --- a/backend/src/routes/__tests__/adminAuth.test.js +++ b/backend/src/routes/__tests__/adminAuth.test.js @@ -36,11 +36,13 @@ jest.mock('../../middleware/auth', () => ({ const { db, logActivity } = require('../../database/db'); const adminAuthRouter = require('../adminAuth'); +const { errorHandler } = require('../../middleware/errorHandler'); describe('adminAuth profile updates', () => { const app = express(); app.use(express.json()); app.use('/auth/admin', adminAuthRouter); + app.use(errorHandler); beforeEach(() => { jest.clearAllMocks(); @@ -55,8 +57,8 @@ describe('adminAuth profile updates', () => { }; db.__setImplementations( - buildChain({ firstResult: null }), // email check buildChain({ firstResult: null }), // username check + buildChain({ firstResult: null }), // email check buildChain({ updateResult: 1 }), // update buildChain({ firstResult: updatedUser }), // fetch updated user ); @@ -66,18 +68,22 @@ describe('adminAuth profile updates', () => { .send({ username: updatedUser.username, email: updatedUser.email }) .expect(200); - expect(response.body).toEqual({ user: updatedUser }); + expect(response.body).toEqual({ + message: 'Admin profile updated successfully', + user: updatedUser + }); expect(logActivity).toHaveBeenCalledWith( 'admin_profile_updated', - { admin_id: 1, updated_fields: ['username', 'email'] }, + { username: updatedUser.username, email: updatedUser.email }, null, - { type: 'admin', id: 1, name: updatedUser.username } + { type: 'admin', id: 1, name: 'admin' } ); }); it('rejects email conflicts', async () => { db.__setImplementations( - buildChain({ firstResult: { id: 2 } }) + buildChain({ firstResult: null }), // username check + buildChain({ firstResult: { id: 2 } }), // email check ); const response = await request(app) @@ -85,7 +91,11 @@ describe('adminAuth profile updates', () => { .send({ username: 'newadmin', email: 'taken@example.com' }) .expect(409); - expect(response.body).toEqual({ error: 'Email is already in use by another admin' }); + expect(response.body).toEqual({ + error: 'Email address is already in use', + code: 'CONFLICT', + field: 'email' + }); }); it('validates input', async () => { @@ -94,6 +104,6 @@ describe('adminAuth profile updates', () => { .send({ username: '', email: 'not-an-email' }) .expect(400); - expect(response.body.errors).toBeDefined(); + expect(response.body.details).toBeDefined(); }); }); From 3790156fc9d613b2f1769f7ffd1181fb4db694b1 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:29:33 +0200 Subject: [PATCH 11/33] fix(accounting): let "bill to a customer" work with the portal off CustomerAccountPicker returns null when customerPortal is off. That is right for its original use -- the event form assigns portal logins that bypass the gallery password -- but the Accounting flows reuse it as-is, so their required "Client" field rendered a bare label with no input and the submit button could never enable, with no explanation. Accounting-on + CRM-off is a valid, UI-supported flag combination. Took option (a): the bill-to-customer path does not depend on the portal. POST /admin/expenses/:id/invoice is gated by requireExpenses + accounting.manage only, and /admin/customers{,/search} are permission-gated rather than flag-gated -- POST /admin/customers exists precisely to create passive, portal-less customers "to attach a quote / invoice / gallery to". The un-gated CustomerPicker used by the quote/bill/contract editors is the precedent. (The comment claiming search 410s with the flag off was stale.) Add portalAssignment (default true) so the gate and the portal-specific label/help text apply only in event-assignment mode; the accounting call sites render their own label. Event-form behaviour is unchanged. Also fixes AccountingInboxPage's TriageModal, which has the identical label-only failure on the rebill disposition from the same root cause -- outside the reported surface, but leaving it would half-fix the bug. Refs testplan REPORT.md #7 (Part 8, S10). --- .../admin/CustomerAccountPicker.tsx | 42 +++++++++---- .../customerAccountPickerPortalGate.test.tsx | 62 +++++++++++++++++++ .../admin/accounting/AccountingInboxPage.tsx | 6 +- .../admin/accounting/ExpensesLedgerPage.tsx | 6 +- 4 files changed, 103 insertions(+), 13 deletions(-) create mode 100644 frontend/src/components/admin/__tests__/customerAccountPickerPortalGate.test.tsx diff --git a/frontend/src/components/admin/CustomerAccountPicker.tsx b/frontend/src/components/admin/CustomerAccountPicker.tsx index 9d2f710a..eda64a8e 100644 --- a/frontend/src/components/admin/CustomerAccountPicker.tsx +++ b/frontend/src/components/admin/CustomerAccountPicker.tsx @@ -24,6 +24,21 @@ interface Props { value: SelectedCustomer[]; onChange: (next: SelectedCustomer[]) => void; disabled?: boolean; + /** + * Event-form mode (default): this picker IS part of the customer-portal + * feature — it assigns portal logins to a gallery, so it hides itself + * when `customerPortal` is off and explains the password bypass. + * + * Pass false where the picker only needs to identify an existing + * customer record (Accounting → "bill this to a client"). Those + * surfaces have their own gates (`accounting` / `expenses` / + * `incomingInvoices`) and their data path never touches the portal: + * /admin/customers{,/search} are permission-gated, not flag-gated, and + * POST /admin/customers explicitly creates passive, portal-less + * customers "to attach a quote / invoice / gallery to". Callers in this + * mode render their own field label. + */ + portalAssignment?: boolean; } const labelFor = (c: { email: string; displayName?: string | null; companyName?: string | null }) => { @@ -31,7 +46,7 @@ const labelFor = (c: { email: string; displayName?: string | null; companyName?: return display ? `${display} · ${c.email}` : c.email; }; -export const CustomerAccountPicker: React.FC = ({ value, onChange, disabled }) => { +export const CustomerAccountPicker: React.FC = ({ value, onChange, disabled, portalAssignment = true }) => { const { t } = useTranslation(); // Rules of Hooks: the feature-flag gate (early-return) is moved to // the very end of this hook list (see end of function). The previous @@ -111,19 +126,24 @@ export const CustomerAccountPicker: React.FC = ({ value, onChange, disabl ); // Feature-flag gate (deliberately placed AFTER all hooks — see the - // long comment at the top of this component for why). When the - // customerPortal flag is off the backend returns 410 on - // /admin/customers/search anyway, but hiding the UI here keeps the - // event form clean and removes the dangling "Customer accounts" - // label that would otherwise appear above an empty placeholder. - if (!customerPortalEnabled) return null; + // long comment at the top of this component for why). Only applies to + // the event-assignment mode: hiding the UI there keeps the event form + // clean and removes the dangling "Customer accounts" label that would + // otherwise appear above an empty placeholder. Non-portal call sites + // must NOT be gated — their required customer field would render as a + // lone label with no input at all (QA S10). + if (portalAssignment && !customerPortalEnabled) return null; return (
- -

{helpText}

+ {portalAssignment && ( + <> + +

{helpText}

+ + )} {/* Selected chips */} {value.length > 0 && ( diff --git a/frontend/src/components/admin/__tests__/customerAccountPickerPortalGate.test.tsx b/frontend/src/components/admin/__tests__/customerAccountPickerPortalGate.test.tsx new file mode 100644 index 00000000..e78efcc2 --- /dev/null +++ b/frontend/src/components/admin/__tests__/customerAccountPickerPortalGate.test.tsx @@ -0,0 +1,62 @@ +/** + * The Accounting "bill this to a client" modals reuse CustomerAccountPicker, + * which used to hide itself whenever `customerPortal` was off — the default. + * The required field then rendered as a lone label with no input and the + * submit button could never enable (QA S10). + * + * Accounting/customerPortal is a supported flag combination: /admin/customers + * and /admin/customers/search are permission-gated, not flag-gated, and + * POST /admin/customers creates passive (portal-less) customers on purpose. + */ +import React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k) }), + }; +}); + +let portalEnabled = false; +vi.mock('../../../contexts/FeatureFlagsContext', () => ({ + useFeatureEnabled: () => portalEnabled, +})); + +vi.mock('../../../services/customerAdmin.service', () => ({ + customerAdminService: { search: vi.fn().mockResolvedValue([]) }, +})); + +import { CustomerAccountPicker } from '../CustomerAccountPicker'; + +const SEARCH_PLACEHOLDER = 'Search by email, name, or company'; +const PORTAL_LABEL = 'Customer accounts'; + +describe('CustomerAccountPicker portal gate (QA S10)', () => { + it('renders a usable search input with customerPortal off when portalAssignment=false', () => { + portalEnabled = false; + render( {}} />); + + expect(screen.getByPlaceholderText(SEARCH_PLACEHOLDER)).toBeInTheDocument(); + // The caller renders its own field label ("Client *"), so the portal + // label + gallery-password help text stay out of the way. + expect(screen.queryByText(PORTAL_LABEL)).not.toBeInTheDocument(); + }); + + it('still hides itself entirely on the event form when customerPortal is off', () => { + portalEnabled = false; + const { container } = render( {}} />); + + expect(container).toBeEmptyDOMElement(); + }); + + it('keeps the portal label + help text on the event form when customerPortal is on', () => { + portalEnabled = true; + render( {}} />); + + expect(screen.getByText(PORTAL_LABEL)).toBeInTheDocument(); + expect(screen.getByPlaceholderText(SEARCH_PLACEHOLDER)).toBeInTheDocument(); + }); +}); diff --git a/frontend/src/pages/admin/accounting/AccountingInboxPage.tsx b/frontend/src/pages/admin/accounting/AccountingInboxPage.tsx index 3b8f1b8c..0db463c2 100644 --- a/frontend/src/pages/admin/accounting/AccountingInboxPage.tsx +++ b/frontend/src/pages/admin/accounting/AccountingInboxPage.tsx @@ -295,7 +295,11 @@ const TriageModal: React.FC<{ doc: InboundDocument; categories: ExpenseCategory[ {BOOKING_DISPOSITIONS.includes(disposition) && (
- setCustomer(next.slice(-1))} /> + {/* portalAssignment={false} — same reason as the expenses + ledger: this is an `incomingInvoices` flow, not a + customer-portal one, and the rebill disposition's + required field would otherwise render label-only. */} + setCustomer(next.slice(-1))} /> {disposition === 'durchlaufend' &&

{t('accounting.inbox.field.passthroughCustomerHint', 'Optional — attach a client to re-bill this passthrough; leave empty to only book it to the event.')}

}
{/* Markup is a re-bill concept only. A pass-through is invoiced diff --git a/frontend/src/pages/admin/accounting/ExpensesLedgerPage.tsx b/frontend/src/pages/admin/accounting/ExpensesLedgerPage.tsx index 67e6021d..660811f1 100644 --- a/frontend/src/pages/admin/accounting/ExpensesLedgerPage.tsx +++ b/frontend/src/pages/admin/accounting/ExpensesLedgerPage.tsx @@ -209,7 +209,11 @@ const InvoiceExpenseModal: React.FC<{ expense: Expense; onClose: () => void; onD

{t('accounting.ledger.invoiceHint', 'This creates a billable line on the client’s next scheduled invoice and locks the expense from further edits.')}

- setCustomer(next.slice(-1))} /> + {/* portalAssignment={false}: re-billing an expense is an Accounting + flow gated by `expenses`, not by the customer portal — without + this the required field renders a bare label and the submit + button can never enable (QA S10). */} + setCustomer(next.slice(-1))} />
shape where the "none" option carries value="0". Fixed at all three call sites that share the branch -- PATCH /photos/:photoId, POST /photos/bulk-update, and the upload route, where the dangling 0 was written at creation time and the scope-validation guard (`if (parsedCategoryId && ...)`) skipped on the falsy 0 and let it in unvalidated. Only the PATCH one was behind the failing test; leaving the other two would have left the bad state creatable. The suite's 3 failures were all masked by a fixture gap, not this bug: it stubs middleware/auth but not middleware/permissions, so requirePermission's admin_users JOIN roles query hit tables the fixture never creates and every request 500'd before reaching a handler. Stub it, bring the photos fixture up to the 7 migrations it had drifted behind, and correct a stale 200 that became 202 when uploads went async in 851744c3. Known adjacent gap, not fixed (wider than this bug): PATCH and bulk-update accept any positive category_id with no existence or scope check, unlike the upload route which validates event_id = X OR is_global per #500/#525 -- so a photo can be PATCHed into another event's category. Refs testplan REPORT.md #22 (Part 1.2.01). --- .../integration/adminPhotos.reference.test.js | 48 ++++++++++++++++++- backend/src/routes/adminPhotos.js | 19 ++++++-- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/backend/__tests__/integration/adminPhotos.reference.test.js b/backend/__tests__/integration/adminPhotos.reference.test.js index 76248264..67424970 100644 --- a/backend/__tests__/integration/adminPhotos.reference.test.js +++ b/backend/__tests__/integration/adminPhotos.reference.test.js @@ -40,6 +40,15 @@ describe('Admin photos in reference mode', () => { } })); + // The routes gained requirePermission() after this fixture was written. + // It resolves the caller's role through admin_users/roles, which this + // minimal schema does not create, so every request died in the RBAC + // lookup before reaching the handler. RBAC is not what this suite is + // about — stub it out the same way adminAuth already is. + jest.doMock('../../src/middleware/permissions', () => ({ + requirePermission: () => (_req, _res, next) => next() + })); + jest.doMock('../../src/services/imageProcessor', () => ({ generateThumbnail: jest.fn().mockResolvedValue('thumbnails/mock-thumb.jpg'), ensureThumbnail: jest.fn() @@ -98,8 +107,21 @@ describe('Admin photos in reference mode', () => { table.string('type').notNullable(); table.integer('size_bytes'); table.integer('category_id'); - table.string('source_origin'); + // Mirrors migration 041: the upload route never writes this column, it + // relies on the NOT NULL DEFAULT 'managed' to mark managed originals. + table.string('source_origin').notNullable().defaultTo('managed'); table.string('external_relpath'); + // Columns the upload insert writes (migrations 048, 062, 071, 085, 193) + // and the PATCH handler writes (migration 178). Without them the insert + // and the update both fail on "no such column". + table.string('original_filename', 512); + table.string('source_filename', 255); + table.datetime('captured_at').nullable(); + table.string('media_type').defaultTo('image'); + table.string('mime_type'); + table.string('processing_status', 16).notNullable().defaultTo('complete'); + table.string('upload_id', 64).nullable(); + table.boolean('auto_categorized'); table.datetime('uploaded_at').defaultTo(db.fn.now()); table.float('average_rating').defaultTo(0); table.integer('like_count').defaultTo(0); @@ -153,7 +175,10 @@ describe('Admin photos in reference mode', () => { .field('category_id', String(categoryId)) .attach('photos', Buffer.from('fake image data'), 'photo.jpg'); - expect(uploadResponse.status).toBe(200); + // 202 Accepted since the upload route went async (851744c3): the files are + // stored and a pending row is inserted, thumbnails/EXIF follow in the + // background worker. This assertion still said 200 from before that. + expect(uploadResponse.status).toBe(202); expect(uploadResponse.body).toHaveProperty('photos'); expect(Array.isArray(uploadResponse.body.photos)).toBe(true); @@ -203,5 +228,24 @@ describe('Admin photos in reference mode', () => { const updated = await db('photos').where({ id: photo.id }).first(); expect(updated.category_id).toBeNull(); + + // A real id still round-trips — the '0' guard must not swallow it. + await request(app) + .patch(`/api/admin/events/1/photos/${photo.id}`) + .send({ category_id: String(categoryId) }) + .expect(200); + expect((await db('photos').where({ id: photo.id }).first()).category_id).toBe(categoryId); + + // Numeric 0 and unparseable input clear the category too, rather than + // writing a category id that can never exist. + for (const value of [0, 'not-a-category']) { + await request(app) + .patch(`/api/admin/events/1/photos/${photo.id}`) + .send({ category_id: value }) + .expect(200); + expect((await db('photos').where({ id: photo.id }).first()).category_id).toBeNull(); + + await db('photos').where({ id: photo.id }).update({ category_id: categoryId }); + } }); }); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 65e1c139..62a3e1f6 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -243,8 +243,11 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r } // Parse category_id to number if provided (handle string values like 'individual', 'collage') + // Same 0-is-not-a-category rule as the PATCH route below: '0' is truthy, so + // it parsed to 0 and the scope-validation guard (`if (parsedCategoryId && ...)`) + // then skipped on the falsy 0 and let it into the insert unvalidated. const rawParsed = category_id ? parseInt(category_id, 10) : NaN; - const parsedCategoryId = !isNaN(rawParsed) ? rawParsed : null; + const parsedCategoryId = rawParsed > 0 ? rawParsed : null; // Determine photo type and category name let photoType = 'individual'; // default @@ -846,9 +849,16 @@ router.patch('/:eventId/photos/:photoId', adminAuth, requirePermission('photos.e // Explicitly clear category updateData.category_id = null; } else { - // Handle numeric category IDs from photo_categories table + // Handle numeric category IDs from photo_categories table. + // 0 and negatives mean "no category", not category zero: photo_categories.id + // is an increments() column so it starts at 1, and a elements on those tabs: Tailwind preflight sets color:inherit on form controls, so they picked up the near-white body colour on a white background. Same root cause, not previously reported. Plus one line of defence-in-depth on the admin shell (AdminLayout): an explicit text colour there stops the whole admin panel inheriting the themed body colour. Components with their own class, including text-theme, still win. Interpretation -- the robust fix was evaluated and rejected. Scoping the theme tokens to gallery contexts is not feasible: the leak is deliberate product behaviour (GlobalThemeProvider applies branding on every non-gallery page), 40 files read var(--color-*) with only 9 under components/gallery, and it would break the customer portal, the public token pages, AdminLoginPage and the Branding live preview. It also cannot be done at container level without moving `body { color: ... }` and the whole .text-theme/.bg-surface/.card-themed utility family, which are global by construction. Known remaining instances, not converted: SystemHealthPage, CrmOverviewSection and HoursSection use text-theme explicitly on admin surfaces, so they keep the themed colour and stay affected. Outside the reported surfaces. Refs testplan REPORT.md #14 (Part 8, S3/S4/S13). --- frontend/src/components/admin/AdminLayout.tsx | 8 ++- .../src/components/common/CMSContentBlock.tsx | 13 +++- .../__tests__/brandingThemeTextLeak.test.ts | 59 +++++++++++++++++++ .../pages/admin/settings/CrmSettingsPage.tsx | 34 +++++------ .../admin/settings/ReminderTemplatesPage.tsx | 18 +++--- .../settings/SettingsBusinessProfilePage.tsx | 30 +++++----- frontend/src/pages/public/LegalPage.tsx | 11 +++- 7 files changed, 129 insertions(+), 44 deletions(-) create mode 100644 frontend/src/pages/admin/__tests__/brandingThemeTextLeak.test.ts diff --git a/frontend/src/components/admin/AdminLayout.tsx b/frontend/src/components/admin/AdminLayout.tsx index f78ea4aa..6d7f65d8 100644 --- a/frontend/src/components/admin/AdminLayout.tsx +++ b/frontend/src/components/admin/AdminLayout.tsx @@ -72,7 +72,13 @@ interface AdminLayoutInnerProps { const AdminLayoutInner: React.FC = ({ sidebarOpen, setSidebarOpen, sidebarCollapsed, setSidebarCollapsed, mustChangePassword }) => { return ( -
+ // Explicit text colour on the admin shell: the branding theme sets + // --color-text on app-wide (GlobalThemeProvider applies it on every + // non-gallery page, by design), so any admin component that forgot its own + // colour class inherited it through `body { color: var(--color-text) }` and + // rendered near-invisible on a dark-toned theme. Components with an + // explicit class or `text-theme` still win over this. +
{/* Mandatory Password Change Modal */} {mustChangePassword && } diff --git a/frontend/src/components/common/CMSContentBlock.tsx b/frontend/src/components/common/CMSContentBlock.tsx index 1fae596b..56ade091 100644 --- a/frontend/src/components/common/CMSContentBlock.tsx +++ b/frontend/src/components/common/CMSContentBlock.tsx @@ -83,7 +83,18 @@ export const CMSContentBlock: React.FC = ({ slug, fallback
- + {/* + * The card surface has to follow the theme too: `.card` hardcodes + * bg-white, so a dark-toned branding theme paired with the themed + * text below rendered near-white text on a white card (QA S3/S4). + */} + {/* * Heading + body now read from theme tokens so dark themes * (and force-dark mode) render correctly without dark: variants diff --git a/frontend/src/pages/admin/__tests__/brandingThemeTextLeak.test.ts b/frontend/src/pages/admin/__tests__/brandingThemeTextLeak.test.ts new file mode 100644 index 00000000..b022f196 --- /dev/null +++ b/frontend/src/pages/admin/__tests__/brandingThemeTextLeak.test.ts @@ -0,0 +1,59 @@ +/** + * ThemeContext.applyTheme() writes the branding theme's `--color-text` as an + * inline style on , so `body { color: var(--color-text) }` applies + * everywhere — including the light admin chrome and the light-chromed public + * legal pages. Any heading that ships without an explicit text-color class + * therefore renders near-white on white as soon as the install picks a + * dark-toned branding theme (QA S3 / S4 / S13). + * + * Source-inspection guard: every heading on the surfaces that were fixed must + * declare its own colour rather than inheriting the themed body colour. + */ +import fs from 'fs'; +import path from 'path'; +import { describe, it, expect } from 'vitest'; + +const SRC = path.resolve(__dirname, '../../..'); + +const read = (rel: string) => fs.readFileSync(path.join(SRC, rel), 'utf8'); + +// Headings must set a colour explicitly. `text-theme` / `text-muted-theme` are +// deliberately NOT accepted — they resolve to the same leaking variables. +const EXPLICIT_COLOR = /\btext-(neutral|white|amber|blue|red|green|primary|accent)\b|\btext-(neutral|amber|blue|red|green|primary)-\d/; + +const HEADING_TAG = /<(h[1-4])(\s[^>]*?)?>/gs; + +const HEADING_FILES = [ + 'pages/admin/settings/SettingsBusinessProfilePage.tsx', + 'pages/admin/settings/CrmSettingsPage.tsx', + 'pages/admin/settings/ReminderTemplatesPage.tsx', + 'pages/public/LegalPage.tsx', +]; + +describe('branding-theme text colour leak (QA S3 / S4 / S13)', () => { + it.each(HEADING_FILES)('every heading in %s declares an explicit text colour', (rel) => { + const source = read(rel); + const offenders: string[] = []; + + for (const match of source.matchAll(HEADING_TAG)) { + const attrs = match[2] || ''; + const className = /className="([^"]*)"/.exec(attrs)?.[1] ?? ''; + if (!EXPLICIT_COLOR.test(className)) offenders.push(match[0]); + } + + expect(offenders).toEqual([]); + }); + + it('gives the LegalPage CMS body an explicit colour instead of the themed body colour', () => { + const source = read('pages/public/LegalPage.tsx'); + expect(source).toMatch(/className="prose prose-neutral max-w-none text-neutral-\d00"/); + }); + + it('keeps the CMS 404 card surface on the same theme tokens as its text', () => { + // CMSContentBlock intentionally renders themed text (var(--color-text)); + // the card surface has to follow, because `.card` hardcodes bg-white. + const source = read('components/common/CMSContentBlock.tsx'); + expect(source).toContain("backgroundColor: 'var(--color-surface)'"); + expect(source).toContain("color: 'var(--color-text)'"); + }); +}); diff --git a/frontend/src/pages/admin/settings/CrmSettingsPage.tsx b/frontend/src/pages/admin/settings/CrmSettingsPage.tsx index 98d73b1c..c455d8d0 100644 --- a/frontend/src/pages/admin/settings/CrmSettingsPage.tsx +++ b/frontend/src/pages/admin/settings/CrmSettingsPage.tsx @@ -136,7 +136,7 @@ export const CrmSettingsPage: React.FC = () => { const setVal = (k: string, v: any) => setValues((s) => ({ ...s, [k]: v })); const checkbox = (k: string, label: string) => ( -