diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js new file mode 100644 index 00000000..b2998705 --- /dev/null +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -0,0 +1,226 @@ +/** + * Image-security settings applied as creation defaults (#1296). + * + * Four controls in Settings → Image security were written, reloaded and + * rendered as toggles, and read by nothing: + * + * default_protection_level, default_image_quality, + * enable_canvas_rendering, default_fragmentation_level + * + * Each maps onto an `events` column migration 038 already created, and each + * is labelled "… by default". `enable_devtools_protection` was the only one + * of the five ever wired. + * + * The load-bearing constraint is that this is CREATION-time only. Applying + * these to existing events would silently change live galleries on upgrade — + * an install with enable_canvas_rendering already on would flip every grid to + * canvas rendering, which is the memory profile under investigation in #1287. + */ + +const { bootCrmDb } = require('./helpers/crmDb'); + +describe('image-security creation defaults', () => { + let db; + let cleanup; + let getImageSecurityDefaults; + let resolveImageSecurityColumns; + let readBooleanSetting; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ getImageSecurityDefaults, resolveImageSecurityColumns, readBooleanSetting } = + require('../../src/routes/adminEvents/helpers')); + }, 120000); + + afterAll(async () => { + if (cleanup) await cleanup(); + }); + + const setSetting = async (key, value) => { + await db('app_settings') + .insert({ setting_key: key, setting_value: JSON.stringify(value), setting_type: 'security' }) + .onConflict('setting_key') + .merge(); + }; + + beforeEach(async () => { + await db('app_settings').whereIn('setting_key', [ + 'default_protection_level', 'default_image_quality', + 'enable_canvas_rendering', 'default_fragmentation_level', + ]).del(); + }); + + it('returns nothing when no settings are configured', async () => { + // Every key absent must fall through to the column defaults, which is + // exactly the behaviour before this existed. + expect(await getImageSecurityDefaults()).toEqual({}); + }); + + it('maps each setting onto its events column', async () => { + await setSetting('default_protection_level', 'enhanced'); + await setSetting('default_image_quality', 72); + await setSetting('enable_canvas_rendering', true); + await setSetting('default_fragmentation_level', 5); + + expect(await getImageSecurityDefaults()).toEqual({ + protection_level: 'enhanced', + image_quality: 72, + use_canvas_rendering: true, + fragmentation_level: 5, + }); + }); + + it('carries a false canvas setting through, rather than dropping it', async () => { + // `false` is a real choice — dropping it as falsy would leave the column + // default in place and make "off" unreachable. + await setSetting('enable_canvas_rendering', false); + expect(await getImageSecurityDefaults()).toEqual({ use_canvas_rendering: false }); + }); + + it.each([ + ['an unknown protection level', 'default_protection_level', 'paranoid'], + ['a non-enum protection level', 'default_protection_level', 42], + ['image quality above 100', 'default_image_quality', 250], + ['image quality of zero', 'default_image_quality', 0], + ['a non-numeric quality', 'default_image_quality', 'high'], + ['fragmentation above the range', 'default_fragmentation_level', 99], + ['a non-boolean canvas value', 'enable_canvas_rendering', 'yes'], + // parseInt would have rescued each of these into a valid-looking + // integer. The settings PUT stores values without validating them, so + // they can genuinely be in the table. + ['a numeric prefix with trailing junk', 'default_image_quality', '72oops'], + ['a fractional quality', 'default_image_quality', 72.5], + ['a single-element array', 'default_image_quality', [72]], + ['a fractional fragmentation level', 'default_fragmentation_level', 3.7], + ['a fragmentation level with trailing junk', 'default_fragmentation_level', '3x'], + ])('ignores %s and falls through to the column default', async (_label, key, value) => { + await setSetting(key, value); + expect(await getImageSecurityDefaults()).toEqual({}); + }); + + it('applies only the keys that are configured', async () => { + await setSetting('default_protection_level', 'maximum'); + expect(await getImageSecurityDefaults()).toEqual({ protection_level: 'maximum' }); + }); + + it('never throws, so a settings failure cannot block event creation', async () => { + await setSetting('default_image_quality', { nonsense: true }); + await expect(getImageSecurityDefaults()).resolves.toEqual({}); + }); + + describe('double-encoded settings (the settings tab round trip)', () => { + // GET returns setting_value undecoded and the tab PUTs the whole object + // back through JSON.stringify, so on SQLite one visit to the tab turns + // every value it read into a doubly-encoded string. A single parse left + // a string behind, the type checks rejected it, and the defaults went + // silently dead again. + const setRaw = async (key, raw) => { + await db('app_settings') + .insert({ setting_key: key, setting_value: raw, setting_type: 'security' }) + .onConflict('setting_key').merge(); + }; + + it('reads a double-encoded boolean', async () => { + await setRaw('enable_canvas_rendering', JSON.stringify(JSON.stringify(true))); + expect(await getImageSecurityDefaults()).toEqual({ use_canvas_rendering: true }); + }); + + it('reads a double-encoded protection level', async () => { + await setRaw('default_protection_level', JSON.stringify(JSON.stringify('enhanced'))); + expect(await getImageSecurityDefaults()).toEqual({ protection_level: 'enhanced' }); + }); + + it('reads a double-encoded integer', async () => { + await setRaw('default_image_quality', JSON.stringify(JSON.stringify(72))); + expect(await getImageSecurityDefaults()).toEqual({ image_quality: 72 }); + }); + + it('reads a value buried under many saves, not just one', async () => { + // Each visit to the settings tab used to add a layer, so the depth is + // however many times someone opened it — not a number to cap. + let raw = JSON.stringify('maximum'); + for (let i = 0; i < 8; i += 1) raw = JSON.stringify(raw); + await setRaw('default_protection_level', raw); + expect(await getImageSecurityDefaults()).toEqual({ protection_level: 'maximum' }); + }); + + it('still rejects a malformed value however many times it was encoded', async () => { + await setRaw('default_image_quality', JSON.stringify(JSON.stringify('72oops'))); + expect(await getImageSecurityDefaults()).toEqual({}); + }); + }); + + describe('readBooleanSetting shares the same decoder', () => { + // Every reader of app_settings has to agree, or the settings tab shows + // protection disabled while newly created galleries turn it on. + const setRaw2 = async (key, raw) => { + await db('app_settings') + .insert({ setting_key: key, setting_value: raw, setting_type: 'security' }) + .onConflict('setting_key').merge(); + }; + + it('reads a double-encoded false as false, not as absent', async () => { + await setRaw2('enable_devtools_protection', JSON.stringify(JSON.stringify(false))); + expect(await readBooleanSetting('enable_devtools_protection')).toBe(false); + }); + + it('still reads a singly-encoded value', async () => { + await setRaw2('enable_devtools_protection', JSON.stringify(true)); + expect(await readBooleanSetting('enable_devtools_protection')).toBe(true); + }); + + it('returns undefined for a non-boolean, so the caller keeps its default', async () => { + await setRaw2('enable_devtools_protection', JSON.stringify('sometimes')); + expect(await readBooleanSetting('enable_devtools_protection')).toBeUndefined(); + }); + }); + + describe('resolveImageSecurityColumns', () => { + it('omits every column when neither the request nor the settings supply one', () => { + expect(resolveImageSecurityColumns({}, {})).toEqual({}); + }); + + it('uses the global default when the request says nothing', () => { + expect(resolveImageSecurityColumns({}, { protection_level: 'maximum' })) + .toEqual({ protection_level: 'maximum' }); + }); + + it('lets an explicit request value win over the global default', () => { + expect(resolveImageSecurityColumns( + { protection_level: 'basic' }, + { protection_level: 'maximum' }, + )).toEqual({ protection_level: 'basic' }); + }); + + it('keeps an explicit false canvas value instead of reading it as absent', () => { + const columns = resolveImageSecurityColumns( + { use_canvas_rendering: false }, + { use_canvas_rendering: true }, + ); + expect(columns.use_canvas_rendering).toBeFalsy(); + }); + + it('keeps a zero-ish explicit value rather than falling through', () => { + // 0 is out of range for the column, but the guard is `!== undefined`, + // not truthiness — the validator is what rejects out-of-range input. + expect(resolveImageSecurityColumns({ image_quality: 0 }, { image_quality: 85 })) + .toEqual({ image_quality: 0 }); + }); + + it('resolves each column independently', () => { + expect(resolveImageSecurityColumns( + { image_quality: 60 }, + { protection_level: 'enhanced', fragmentation_level: 4 }, + )).toEqual({ + protection_level: 'enhanced', + image_quality: 60, + fragmentation_level: 4, + }); + }); + + it('tolerates a missing body, which is what an empty API request looks like', () => { + expect(resolveImageSecurityColumns(undefined, { image_quality: 90 })) + .toEqual({ image_quality: 90 }); + }); + }); +}); diff --git a/backend/__tests__/routes/adminEvents.smoke.test.js b/backend/__tests__/routes/adminEvents.smoke.test.js index 713dcda1..4f857406 100644 --- a/backend/__tests__/routes/adminEvents.smoke.test.js +++ b/backend/__tests__/routes/adminEvents.smoke.test.js @@ -214,6 +214,44 @@ describe('admin events CRUD endpoints (smoke)', () => { expect(row.welcome_message).toBe('Hello guests'); }); + // #1296 — express-validator runs isInt/isIn/isBoolean element-wise on + // arrays, so a single-element array satisfies its field validator and + // survives into `updates`, which is spread into .update() with no column + // allow-list. That put an array into a scalar column (a PG insert error), + // and formatBoolean([false]) read as true. Guarded for every field, not + // just the ones that prompted it. + it.each([ + ['image_quality', [72]], + ['protection_level', ['basic']], + ['use_canvas_rendering', [false]], + ['fragmentation_level', [3]], + // Not a protection field: the guard is not scoped to that block. + ['event_name', ['Arrayed']], + ['allow_downloads', [false]], + ])('400s on an array value for %s', async (field, value) => { + const id = await insertEvent(db, adminId, { event_name: 'Unchanged' }); + const res = await auth(request(app).put(`/api/admin/events/${id}`)) + .send({ [field]: value }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(field); + // And nothing was written. + const row = await db('events').where({ id }).first(); + expect(row.event_name).toBe('Unchanged'); + }); + + it('still accepts customer_account_ids, the one field that is an array', async () => { + const id = await insertEvent(db, adminId, { event_name: 'Keep' }); + const res = await auth(request(app).put(`/api/admin/events/${id}`)).send({ + event_name: 'Renamed', + customer_account_ids: [], + }); + + expect(res.status).toBe(200); + const row = await db('events').where({ id }).first(); + expect(row.event_name).toBe('Renamed'); + }); + it('404s when updating a missing event', async () => { const res = await auth(request(app).put('/api/admin/events/999999')).send({ event_name: 'Ghost', diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index fc2dce8e..60d2642f 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -30,7 +30,8 @@ const { clampIntOrUndefined } = require('../../utils/numericHelpers'); const { getFrontendBaseUrl, getAbsoluteFrontendUrl } = require('../../utils/frontendUrl'); const downloadZipService = require('../../services/downloadZipService'); const { resolveEventFeedbackDefaults, applyFeedbackDefaults, KEYBIND_MODES } = require('../../services/feedbackDefaults'); -const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers'); +const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, + getImageSecurityDefaults, resolveImageSecurityColumns, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers'); /** * `events.slug` is UNIQUE, and both routes that mint one do a read-then-insert @@ -228,6 +229,13 @@ module.exports = (router) => { body('allow_downloads').optional().isBoolean(), body('disable_right_click').optional().isBoolean(), body('enable_devtools_protection').optional().isBoolean(), + // Image security. PUT /:id has validated these all along; create + // accepted none of them, so a value sent here used to be dropped on the + // floor and the column default applied instead (#1296). + body('protection_level').optional().not().isArray().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('use_canvas_rendering').optional().not().isArray().isBoolean().toBoolean(), + body('image_quality').optional().not().isArray().isInt({ min: 1, max: 100 }).toInt(), + body('fragmentation_level').optional().not().isArray().isInt({ min: 1, max: 10 }).toInt(), body('watermark_downloads').optional().isBoolean(), body('watermark_text').optional().trim(), // #328 follow-up: per-event opt-in for presigned-URL "Download All". @@ -544,6 +552,14 @@ module.exports = (router) => { // the request explicitly overrides it (#317 — admin disabled it globally // but new events still got it ON because the column default is true). const protectionDefaults = await getDownloadProtectionDefaults(); + // #1296 — the other four Image-security settings, which were written, + // rendered as controls, and read by nothing. Same inheritance rule as + // the devtools setting below. Creation-time only; see + // getImageSecurityDefaults for why existing events are left alone. + const imageSecurityColumns = resolveImageSecurityColumns( + req.body, + await getImageSecurityDefaults(), + ); const effectiveEnableDevtoolsProtection = enableDevtoolsProtectionInput !== undefined ? enableDevtoolsProtectionInput @@ -617,6 +633,9 @@ module.exports = (router) => { allow_downloads: formatBoolean(allow_downloads !== undefined ? allow_downloads : true), disable_right_click: formatBoolean(disable_right_click !== undefined ? disable_right_click : false), enable_devtools_protection: formatBoolean(effectiveEnableDevtoolsProtection), + // Request value, else the global default, else the column default — + // a key absent here is one the database fills in (#1296). + ...imageSecurityColumns, watermark_downloads: formatBoolean(watermark_downloads !== undefined ? watermark_downloads : false), watermark_text, allow_presigned_download: formatBoolean(allow_presigned_download === true || allow_presigned_download === 'true'), @@ -1400,6 +1419,15 @@ module.exports = (router) => { allow_downloads: source.allow_downloads, disable_right_click: source.disable_right_click, enable_devtools_protection: source.enable_devtools_protection, + // A duplicate inherits the source's protection settings, NOT the + // current global defaults — copying the gallery is the whole point. + // These four sat next to enable_devtools_protection and were simply + // missed, so duplicating a 'maximum' event produced a 'standard' one + // (#1296). + protection_level: source.protection_level, + image_quality: source.image_quality, + use_canvas_rendering: source.use_canvas_rendering, + fragmentation_level: source.fragmentation_level, watermark_downloads: source.watermark_downloads, watermark_text: source.watermark_text, allow_presigned_download: source.allow_presigned_download, @@ -1630,6 +1658,25 @@ module.exports = (router) => { const { id } = req.params; const updates = { ...req.body }; + // express-validator applies isInt/isIn/isBoolean element-wise to + // arrays, so `image_quality: [72]` satisfies its validator and stays + // an array. This handler spreads req.body into .update() with no + // column allow-list, so such a value reaches a scalar column: a PG + // insert error, and `[false]` coerced to true by formatBoolean. + // + // Guarded here rather than per field because it applies to all 44 + // validated fields, not to a chosen few. `customer_account_ids` is the + // only field that is legitimately an array, and it is deleted from + // `updates` below before the write (#1296). + const ARRAY_VALUED_FIELDS = new Set(['customer_account_ids']); + const arrayValued = Object.keys(updates) + .filter((key) => Array.isArray(updates[key]) && !ARRAY_VALUED_FIELDS.has(key)); + if (arrayValued.length > 0) { + return res.status(400).json({ + error: `Array values are not accepted for: ${arrayValued.join(', ')}`, + }); + } + // Strip identity/provenance/secret columns from the mass-assigned // body (GHSA-3rqx). The handler spreads req.body straight into the // events UPDATE, so without this an events.edit holder could rewrite diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index be10a8cc..82e1af16 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -73,14 +73,37 @@ const getEventFieldRequirements = async () => { // Helper to read app_settings booleans by key, used to inherit per-setting // defaults onto new events. Returns `undefined` for missing/non-boolean rows // so callers can fall back to a legacy default. +/** + * Decode an app_settings value into the JS value it represents. + * + * setting_value is JSON text on SQLite and may already be decoded by the + * driver on a PG json column, so one parse does not normalise both. On top + * of that, the Image Security tab used to PUT back values it had read + * undecoded, wrapping another layer of quoting around each one on every + * save — the GET handler decodes now, but installs carry however many + * layers they accumulated before that. + * + * Every reader of app_settings has to agree about this, or the admin UI + * shows one thing while event creation does another. + * + * Terminates: each parse of a string is strictly shorter than its input. + */ +const decodeSettingValue = (raw) => { + let value = raw; + while (typeof value === 'string') { + let parsed; + try { parsed = JSON.parse(value); } catch { break; } + if (parsed === value) break; + value = parsed; + } + return value; +}; + const readBooleanSetting = async (key) => { try { const setting = await db('app_settings').where('setting_key', key).first(); if (!setting) return undefined; - let value = setting.setting_value; - if (typeof value === 'string') { - try { value = JSON.parse(value); } catch { /* keep raw */ } - } + const value = decodeSettingValue(setting.setting_value); return typeof value === 'boolean' ? value : undefined; } catch (error) { logger.error('Failed to read app setting', { key, error: error.message }); @@ -95,6 +118,156 @@ const getDownloadProtectionDefaults = async () => { return { enable_devtools_protection: await readBooleanSetting('enable_devtools_protection') }; }; +/** + * The rest of Settings → Image security, as creation defaults (#1296). + * + * Four settings in that panel were written, reloaded and rendered as + * controls, and read by nothing: + * + * default_protection_level → events.protection_level + * default_image_quality → events.image_quality + * enable_canvas_rendering → events.use_canvas_rendering + * default_fragmentation_level → events.fragmentation_level + * + * Each maps onto a column migration 038 already created, and each is + * labelled "… by default", so applying them at creation is what the panel + * has always claimed to do. `enable_devtools_protection` above is the only + * one of the five that was ever wired. + * + * Creation-time only, deliberately. Applying them to EXISTING events would + * silently change live galleries on upgrade — an install with + * enable_canvas_rendering already on would switch every grid to canvas + * rendering, which is memory-expensive at scale and is the profile under + * investigation in #1287. New events only; existing rows untouched. + * + * Any value that is missing or malformed comes back undefined so the caller + * falls through to the column default, exactly as before this existed. + */ +const PROTECTION_LEVELS = ['basic', 'standard', 'enhanced', 'maximum']; + +// parseInt would rescue malformed settings instead of rejecting them: +// parseInt('72oops') is 72, parseInt(72.5) is 72, parseInt([72]) is 72. +// That matters because the settings PUT stores whatever JSON it is handed +// without validating the value (adminImageSecurity.js writes +// JSON.stringify(value) for any allow-listed key), so those shapes really +// can be sitting in app_settings. Accept only a genuine integer, or a +// string that is exactly one. +const toInteger = (value) => { + if (typeof value === 'number') return Number.isInteger(value) ? value : undefined; + if (typeof value === 'string' && /^[+-]?\d+$/.test(value.trim())) return Number(value.trim()); + return undefined; +}; + +const getImageSecurityDefaults = async (trx = null) => { + const defaults = {}; + try { + // Accepts a transaction the way getAppSetting does. It matters on + // sqlite3, whose pool holds a single connection: a caller already inside + // db.transaction() that read through the global `db` would block on the + // connection its own transaction holds until the acquire timeout, and + // the catch below would then quietly swallow it and drop the defaults. + const query = trx || db; + const rows = await query('app_settings') + .whereIn('setting_key', [ + 'default_protection_level', + 'default_image_quality', + 'enable_canvas_rendering', + 'default_fragmentation_level', + ]) + .select('setting_key', 'setting_value'); + + // app_settings holds JSON text on SQLite, while a PG json column comes + // back already decoded — so one parse is not enough to normalise both. + // Worse, GET /api/admin/image-security/settings returns setting_value + // without decoding it and the settings tab PUTs the whole fetched object + // straight back through JSON.stringify, so opening the tab and saving + // re-encodes every value it read as text. After one such round trip + // `true` is stored as "\"true\"" and a single parse yields the string + // 'true', which the type checks below reject — the settings would go + // quietly dead again, which is the bug this whole change exists to fix. + // The GET handler now decodes, so this stops accumulating — but installs + // that already stacked N layers have to keep working, and N is however + // many times someone opened that tab. So unwrap until it stops being a + // JSON string rather than to a fixed depth; this terminates because each + // parse of a string is strictly shorter than its input. + const read = (key) => { + const row = rows.find((r) => r.setting_key === key); + if (!row) return undefined; + return decodeSettingValue(row.setting_value); + }; + + const level = read('default_protection_level'); + if (typeof level === 'string' && PROTECTION_LEVELS.includes(level)) { + defaults.protection_level = level; + } + + // The column is an integer percentage; anything outside 1..100 is a + // misconfiguration and falls through rather than being clamped into + // something the operator did not choose. + const quality = toInteger(read('default_image_quality')); + if (quality !== undefined && quality >= 1 && quality <= 100) { + defaults.image_quality = quality; + } + + const canvas = read('enable_canvas_rendering'); + if (typeof canvas === 'boolean') { + defaults.use_canvas_rendering = canvas; + } + + const fragmentation = toInteger(read('default_fragmentation_level')); + if (fragmentation !== undefined && fragmentation >= 1 && fragmentation <= 10) { + defaults.fragmentation_level = fragmentation; + } + } catch (error) { + // A settings read must never block event creation; the column defaults + // are a correct fallback. + logger.error('Failed to read image-security defaults', { error: error.message }); + } + return defaults; +}; + +/** + * Build the image-security columns for a NEW event: an explicit request + * value wins, then the global default, then the column default (the key is + * omitted entirely so the database supplies it). + * + * Shared by the admin create route and POST /api/v1/events so the configured + * security level cannot depend on which entry point created the gallery — + * the same split that made #592 (devtools) a separate bug from #317. + * + * `body` values are already validated by the route's express-validator + * chain; `defaults` come from getImageSecurityDefaults(), which validates + * them itself. + */ +const resolveImageSecurityColumns = (body = {}, defaults = {}) => { + const { formatBoolean } = require('../../utils/dbCompat'); + const columns = {}; + // express-validator runs isInt/isIn/isBoolean element-wise on arrays, so a + // single-element array like `image_quality: [72]` passes the route's chain + // and arrives here still an array. The routes reject those with + // .not().isArray(); this guard means any future caller cannot write one + // into a scalar column (a PG insert error, or `[false]` coerced to true). + const scalar = (v) => (v !== null && typeof v === 'object' ? undefined : v); + const pick = (key) => { + const fromBody = scalar(body[key]); + return fromBody !== undefined ? fromBody : defaults[key]; + }; + + const level = pick('protection_level'); + if (level !== undefined) columns.protection_level = level; + + const quality = pick('image_quality'); + if (quality !== undefined) columns.image_quality = quality; + + const canvas = pick('use_canvas_rendering'); + if (canvas !== undefined) columns.use_canvas_rendering = formatBoolean(canvas); + + const fragmentation = pick('fragmentation_level'); + if (fragmentation !== undefined) columns.fragmentation_level = fragmentation; + + return columns; +}; + // Helper to get branding defaults for new events (Feature 7: Branding Inheritance). // // Note: `branding_logo_position` (header bar — left/center/right) is a @@ -527,7 +700,10 @@ module.exports = { getStoragePath, getEventFieldRequirements, readBooleanSetting, + decodeSettingValue, getDownloadProtectionDefaults, + getImageSecurityDefaults, + resolveImageSecurityColumns, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, diff --git a/backend/src/routes/adminImageSecurity.js b/backend/src/routes/adminImageSecurity.js index 877a5421..92a51a09 100644 --- a/backend/src/routes/adminImageSecurity.js +++ b/backend/src/routes/adminImageSecurity.js @@ -4,6 +4,7 @@ const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const secureImageMiddleware = require('../middleware/secureImageMiddleware'); const logger = require('../utils/logger'); +const { decodeSettingValue } = require('./adminEvents/helpers'); const router = express.Router(); @@ -32,9 +33,15 @@ router.get('/settings', adminAuth, requirePermission(['settings.view', 'image_se const config = {}; settings.forEach(setting => { - // PostgreSQL JSON columns are already parsed by the driver - // Just use the value directly - no need to JSON.parse - config[setting.setting_key] = setting.setting_value; + // setting_value is JSON text on SQLite, and already decoded by the + // driver on a PG json column — so returning it raw shipped strings + // like "true" to a tab that types the field as boolean. Worse, the + // tab PUTs this whole object straight back through JSON.stringify, + // so every save wrapped another layer of quoting around values nobody + // edited, until consumers could no longer read them (#1296). Decode + // here so a round trip is idempotent. This terminates: each parse of + // a string is strictly shorter than its input. + config[setting.setting_key] = decodeSettingValue(setting.setting_value); }); res.json(config); diff --git a/backend/src/routes/v1/__tests__/events.create.test.js b/backend/src/routes/v1/__tests__/events.create.test.js index 1e973a86..92b1ad8e 100644 --- a/backend/src/routes/v1/__tests__/events.create.test.js +++ b/backend/src/routes/v1/__tests__/events.create.test.js @@ -126,6 +126,7 @@ const BASE_BODY = { const baseSettingsChains = () => [ buildChain({ firstResult: null }), // feedback default buildChain({ firstResult: null }), // devtools default + buildChain({ selectResult: [] }), // image-security whereIn → empty rows (#1296) buildChain({ selectResult: [] }), // branding whereIn → empty rows ]; @@ -169,19 +170,21 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { it('creates event_feedback_settings row when feedback_enabled=true is sent', async () => { // feedback_enabled provided → feedback probe SKIPPED. Sequence: // 1. devtools probe - // 2. branding probe (whereIn → select) - // 3. slug probe - // 4. events insert - // 5. feedback sub-toggle defaults probe (whereIn → select, #1044) - // 6. event_feedback_settings insert + // 2. image-security probe (whereIn → select, #1296) + // 3. branding probe (whereIn → select) + // 4. slug probe + // 5. events insert + // 6. feedback sub-toggle defaults probe (whereIn → select, #1044) + // 7. event_feedback_settings insert const devtoolsChain = buildChain({ firstResult: null }); + const imageSecurityChain = buildChain({ selectResult: [] }); const brandingChain = buildChain({ selectResult: [] }); const slugChain = buildChain({ firstResult: null }); const insertChain = buildChain({ returningResult: [{ id: 50 }] }); const feedbackDefaultsChain = buildChain({ selectResult: [] }); const feedbackInsertChain = buildChain(); db.__setImplementations( - devtoolsChain, brandingChain, slugChain, insertChain, + devtoolsChain, imageSecurityChain, brandingChain, slugChain, insertChain, feedbackDefaultsChain, feedbackInsertChain, ); @@ -190,7 +193,7 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { .send({ ...BASE_BODY, feedback_enabled: true }) .expect(201); - expect(db).toHaveBeenNthCalledWith(6, 'event_feedback_settings'); + expect(db).toHaveBeenNthCalledWith(7, 'event_feedback_settings'); const feedbackRow = feedbackInsertChain.insert.mock.calls[0][0]; expect(feedbackRow).toMatchObject({ event_id: 50 }); @@ -216,20 +219,21 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { it('honours the event_default_feedback_enabled global when body omits feedback_enabled', async () => { // Feedback probe returns serialized "true" → fallback kicks in and // the feedback insert runs. Sequence: feedback probe, devtools probe, - // branding probe, slug, insert, sub-toggle defaults probe (#1044), - // feedback insert (7 calls total). + // image-security probe (#1296), branding probe, slug, insert, sub-toggle + // defaults probe (#1044), feedback insert (8 calls total). const feedbackProbe = buildChain({ firstResult: { setting_key: 'event_default_feedback_enabled', setting_value: 'true' }, }); const devtoolsChain = buildChain({ firstResult: null }); + const imageSecurityChain = buildChain({ selectResult: [] }); const brandingChain = buildChain({ selectResult: [] }); const slugChain = buildChain({ firstResult: null }); const insertChain = buildChain({ returningResult: [{ id: 51 }] }); const feedbackDefaultsChain = buildChain({ selectResult: [] }); const feedbackInsertChain = buildChain(); db.__setImplementations( - feedbackProbe, devtoolsChain, brandingChain, slugChain, insertChain, - feedbackDefaultsChain, feedbackInsertChain, + feedbackProbe, devtoolsChain, imageSecurityChain, brandingChain, slugChain, + insertChain, feedbackDefaultsChain, feedbackInsertChain, ); await request(buildApp()) @@ -237,7 +241,7 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { .send(BASE_BODY) .expect(201); - expect(db).toHaveBeenNthCalledWith(7, 'event_feedback_settings'); + expect(db).toHaveBeenNthCalledWith(8, 'event_feedback_settings'); expect(feedbackInsertChain.insert).toHaveBeenCalledTimes(1); }); @@ -251,9 +255,9 @@ describe('v1 POST /events — issue #550 (color_theme + feedback row)', () => { .send(BASE_BODY) .expect(201); - // 5 db() calls: feedback + devtools + branding probes, slug, insert. - // event_feedback_settings is never touched. - expect(db).toHaveBeenCalledTimes(5); + // 6 db() calls: feedback + devtools + image-security + branding probes, + // slug, insert. event_feedback_settings is never touched. + expect(db).toHaveBeenCalledTimes(6); expect(db).not.toHaveBeenCalledWith('event_feedback_settings'); }); diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index fd4cb2a9..121c22ea 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -37,6 +37,7 @@ const logger = require('../../utils/logger'); const { slugify } = require('../../utils/slug'); const { formatBoolean } = require('../../utils/dbCompat'); const { parseBooleanInput } = require('../../utils/parsers'); +const { getImageSecurityDefaults, resolveImageSecurityColumns, decodeSettingValue } = require('../adminEvents/helpers'); const { isValidEventType } = require('../../services/eventTypeService'); const { replacePhoto } = require('../../services/photoReplacementService'); const { getMaxFileSizeBytes, DEFAULT_MAX_FILE_SIZE_MB } = require('../../services/uploadSettings'); @@ -134,6 +135,10 @@ const photoUpload = async (req, res, next) => { * color_theme: { type: string, nullable: true, description: "Preset name (e.g. 'default') or JSON-encoded ThemeConfig. Persisted as-is on the event row." } * feedback_enabled: { type: boolean, nullable: true, description: "Enable guest feedback for this gallery. When omitted, falls back to the global event_default_feedback_enabled setting." } * enable_devtools_protection: { type: boolean, nullable: true, description: "Block right-click / devtools shortcuts in the gallery. When omitted, falls back to the global enable_devtools_protection setting." } + * protection_level: { type: string, nullable: true, enum: [basic, standard, enhanced, maximum], description: "Image protection level. When omitted, falls back to the global default_protection_level setting." } + * use_canvas_rendering: { type: boolean, nullable: true, description: "Render gallery images to a canvas instead of an img tag. When omitted, falls back to the global enable_canvas_rendering setting." } + * image_quality: { type: integer, minimum: 1, maximum: 100, nullable: true, description: "Served image quality percentage. When omitted, falls back to the global default_image_quality setting." } + * fragmentation_level: { type: integer, minimum: 1, maximum: 10, nullable: true, description: "Stored for future use; no renderer consumes it yet. When omitted, falls back to the global default_fragmentation_level setting." } * hero_logo_visible: { type: boolean, nullable: true, description: "Show event logo in the hero block. When omitted, falls back to the global branding_logo_display_hero setting." } * hero_logo_size: { type: string, nullable: true, enum: [small, medium, large, xlarge], description: "Hero logo size. When omitted, falls back to the global branding_logo_size setting." } * hero_logo_position: { type: string, nullable: true, enum: [top, center, bottom], description: "Hero logo position. Defaults to 'top' (not settings-backed — see migration 084)." } @@ -179,6 +184,10 @@ router.post( body('color_theme').optional({ nullable: true }).isString().trim(), body('feedback_enabled').optional().isBoolean(), body('enable_devtools_protection').optional().isBoolean(), + body('protection_level').optional().not().isArray().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('use_canvas_rendering').optional().not().isArray().isBoolean().toBoolean(), + body('image_quality').optional().not().isArray().isInt({ min: 1, max: 100 }).toInt(), + body('fragmentation_level').optional().not().isArray().isInt({ min: 1, max: 10 }).toInt(), body('hero_logo_visible').optional().isBoolean(), body('hero_logo_size').optional().isIn(['small', 'medium', 'large', 'xlarge']), body('hero_logo_position').optional().isIn(['top', 'center', 'bottom']) @@ -227,14 +236,24 @@ router.post( if (devtoolsInput === undefined) { const setting = await db('app_settings').where('setting_key', 'enable_devtools_protection').first(); if (setting) { - try { - const parsed = JSON.parse(setting.setting_value); - if (typeof parsed === 'boolean') devtoolsFallback = parsed; - } catch { /* keep true */ } + // Shared decoder: a legacy row can carry several layers of JSON + // quoting, and a single parse would leave the string 'false' here, + // reject it, and quietly enable protection the operator disabled. + const parsed = decodeSettingValue(setting.setting_value); + if (typeof parsed === 'boolean') devtoolsFallback = parsed; } } const enable_devtools_protection = parseBooleanInput(devtoolsInput, devtoolsFallback); + // #1296 — same shape again, for the four Image Security settings that + // were stored and applied nowhere. Shared with the admin create route + // so a gallery's security level does not depend on which endpoint made + // it; #592 above is the bug this would otherwise repeat. + const imageSecurityColumns = resolveImageSecurityColumns( + req.body, + await getImageSecurityDefaults(), + ); + // Same shape as the feedback / devtools fallbacks: honour the global // event_default_require_password toggle (#317). Without this an admin // who disabled "require password by default" globally still got @@ -324,6 +343,8 @@ router.post( // Issue #592 — write the resolved devtools setting (input value // or global fallback) so the column default doesn't shadow it. enable_devtools_protection: formatBoolean(enable_devtools_protection), + // Request value, else the global default, else the column default. + ...imageSecurityColumns, // Branding inheritance — resolved value from body or app_settings. hero_logo_visible: formatBoolean(hero_logo_visible), hero_logo_size, diff --git a/backend/src/services/contract/conversions.js b/backend/src/services/contract/conversions.js index 2b3548cb..0450c992 100644 --- a/backend/src/services/contract/conversions.js +++ b/backend/src/services/contract/conversions.js @@ -229,6 +229,8 @@ async function convertToEvent(contractId, adminId) { || (await resolveDefaultEventType()); const eventCols = await db('events').columnInfo(); + const { getImageSecurityDefaults, resolveImageSecurityColumns } = require('../../routes/adminEvents/helpers'); + const imageSecurityColumns = resolveImageSecurityColumns({}, await getImageSecurityDefaults()); const candidate = { slug: `contract-${contract.contract_number.toLowerCase()}-${crypto.randomBytes(3).toString('hex')}`, // Prefer the contract's event_name snapshot (set on the contract @@ -255,6 +257,11 @@ async function convertToEvent(contractId, adminId) { quote_id: null, created_at: new Date(), updated_at: new Date(), + // #1296 — a signed standalone contract converts straight to a gallery + // here, without going through quoteService, so the global Image Security + // defaults have to be applied on this path too. Not inside a transaction, + // so the global db read is fine. + ...imageSecurityColumns, }; const eventRow = {}; for (const [k, v] of Object.entries(candidate)) { diff --git a/backend/src/services/quoteService.js b/backend/src/services/quoteService.js index 9cfdd399..e6943973 100644 --- a/backend/src/services/quoteService.js +++ b/backend/src/services/quoteService.js @@ -1683,6 +1683,8 @@ async function convertToEvent(quoteId, adminId, options = {}) { // ask the DB which columns exist and only keep the matching pairs // — bullet-proof against schema drift in either direction. const eventCols = await trx('events').columnInfo(); + const { getImageSecurityDefaults, resolveImageSecurityColumns } = require('../routes/adminEvents/helpers'); + const imageSecurityColumns = resolveImageSecurityColumns({}, await getImageSecurityDefaults(trx)); const candidate = { slug: `quote-${quote.quote_number.toLowerCase()}-${crypto.randomBytes(3).toString('hex')}`, event_name: quote.event_name || `Event ${quote.quote_number}`, @@ -1705,6 +1707,11 @@ async function convertToEvent(quoteId, adminId, options = {}) { quote_id: quote.id, created_at: new Date(), updated_at: new Date(), + // #1296 — a converted quote produces a real gallery, so the global + // Image Security defaults have to reach it too. Required lazily: this + // is a service reaching into a route helper, and the lazy form keeps + // the module graph acyclic the way the storage require below does. + ...imageSecurityColumns, }; const eventRow = {}; for (const [k, v] of Object.entries(candidate)) {