From 8ca3610514dc9e16c1c14c82fce529042b728e47 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 06:21:46 +0200 Subject: [PATCH 1/7] fix(security): apply the Image-security defaults instead of storing them (#1296) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four controls in Settings → Image security were written, reloaded and rendered as toggles, 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". `enable_devtools_protection` was the only one of the five ever wired (#317), and its plumbing is the pattern this follows. Reported for enable_canvas_rendering by @leonlivevocalist-svg while instrumenting #1287 — the setting was globally true on their install and zero canvas elements were created. Checking the neighbours found three more of the same, so fixing one and leaving three would have been worse than leaving all four. CREATION-TIME ONLY, deliberately. 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 memory-expensive at scale and is the exact profile under investigation in #1287. New events inherit; existing rows are untouched. A missing or malformed value yields no key, so creation falls through to the column default exactly as before — including the ranges, where an out-of-range quality or fragmentation level is ignored rather than clamped into something the operator did not choose. `false` is carried through rather than dropped as falsy, or "off" would be unreachable. The spread sits after the explicit columns so a value supplied by the request still wins. 12 tests: the mapping, the false case, seven malformed inputs falling through, partial configuration, and that a settings failure cannot block event creation. --- .../integration/imageSecurityDefaults.test.js | 99 +++++++++++++++++++ backend/src/routes/adminEvents/crud.js | 19 +++- backend/src/routes/adminEvents/helpers.js | 80 +++++++++++++++ 3 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/integration/imageSecurityDefaults.test.js diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js new file mode 100644 index 00000000..32b61ded --- /dev/null +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -0,0 +1,99 @@ +/** + * 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; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ getImageSecurityDefaults } = 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'], + ])('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({}); + }); +}); diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index fc2dce8e..d6a0d500 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, 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 @@ -544,6 +545,10 @@ 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. Creation-time only; see + // getImageSecurityDefaults for why existing events are left alone. + const imageSecurityDefaults = await getImageSecurityDefaults(); const effectiveEnableDevtoolsProtection = enableDevtoolsProtectionInput !== undefined ? enableDevtoolsProtectionInput @@ -617,6 +622,18 @@ 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), + // Spread AFTER the explicit columns so a value the request supplied + // still wins; each key is present only when the global setting held + // a usable value, so anything unset falls through to the column + // default exactly as before (#1296). + ...(imageSecurityDefaults.protection_level !== undefined + ? { protection_level: imageSecurityDefaults.protection_level } : {}), + ...(imageSecurityDefaults.image_quality !== undefined + ? { image_quality: imageSecurityDefaults.image_quality } : {}), + ...(imageSecurityDefaults.use_canvas_rendering !== undefined + ? { use_canvas_rendering: formatBoolean(imageSecurityDefaults.use_canvas_rendering) } : {}), + ...(imageSecurityDefaults.fragmentation_level !== undefined + ? { fragmentation_level: imageSecurityDefaults.fragmentation_level } : {}), watermark_downloads: formatBoolean(watermark_downloads !== undefined ? watermark_downloads : false), watermark_text, allow_presigned_download: formatBoolean(allow_presigned_download === true || allow_presigned_download === 'true'), diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index be10a8cc..fd271bee 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -95,6 +95,85 @@ 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']; + +const getImageSecurityDefaults = async () => { + const defaults = {}; + try { + const rows = await db('app_settings') + .whereIn('setting_key', [ + 'default_protection_level', + 'default_image_quality', + 'enable_canvas_rendering', + 'default_fragmentation_level', + ]) + .select('setting_key', 'setting_value'); + + const read = (key) => { + const row = rows.find((r) => r.setting_key === key); + if (!row) return undefined; + let value = row.setting_value; + if (typeof value === 'string') { + try { value = JSON.parse(value); } catch { /* keep raw */ } + } + return 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 = parseInt(read('default_image_quality'), 10); + if (Number.isInteger(quality) && 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 = parseInt(read('default_fragmentation_level'), 10); + if (Number.isInteger(fragmentation) && 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; +}; + // Helper to get branding defaults for new events (Feature 7: Branding Inheritance). // // Note: `branding_logo_position` (header bar — left/center/right) is a @@ -528,6 +607,7 @@ module.exports = { getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, + getImageSecurityDefaults, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, From ab6c33d9eb485cc3ed05187a98a22f51926cc125 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 07:11:17 +0200 Subject: [PATCH 2/7] fix(security): apply image-security defaults on every creation path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the #1296 fix. The defaults were resolved only in the admin POST / handler. POST /api/v1/events builds its own insert and resolved just the devtools setting, so an API-created gallery still fell back to the column defaults — the same split that made #592 a separate bug from #317, about to be repeated. Both paths now share resolveImageSecurityColumns(). An explicitly supplied value now wins over the global default. The create routes never accepted these four fields at all, though PUT /:id has validated them all along, so a client sending protection_level on create had it silently dropped. The previous comment claimed the spread ordering preserved a request value; there was no request value to preserve, and a later spread would have overridden one anyway. Settings validation no longer leans on parseInt, which rescues '72oops', 72.5 and [72] into valid-looking integers. The settings PUT stores whatever JSON it is handed without validating values, so those really can reach the resolver. fragmentation_level is still stored and consumed by no renderer — ProtectedImage hardcodes a 4-grid and secureImageService a 3x3. Noted in the API docs rather than silently implied to work. Refs #1296 --- .../integration/imageSecurityDefaults.test.js | 61 ++++++++++++++++++- backend/src/routes/adminEvents/crud.js | 32 +++++----- backend/src/routes/adminEvents/helpers.js | 55 +++++++++++++++-- .../routes/v1/__tests__/events.create.test.js | 34 ++++++----- backend/src/routes/v1/events.js | 20 ++++++ 5 files changed, 167 insertions(+), 35 deletions(-) diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js index 32b61ded..d64b0e43 100644 --- a/backend/__tests__/integration/imageSecurityDefaults.test.js +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -23,10 +23,12 @@ describe('image-security creation defaults', () => { let db; let cleanup; let getImageSecurityDefaults; + let resolveImageSecurityColumns; beforeAll(async () => { ({ db, cleanup } = await bootCrmDb()); - ({ getImageSecurityDefaults } = require('../../src/routes/adminEvents/helpers')); + ({ getImageSecurityDefaults, resolveImageSecurityColumns } = + require('../../src/routes/adminEvents/helpers')); }, 120000); afterAll(async () => { @@ -82,6 +84,14 @@ describe('image-security creation defaults', () => { ['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({}); @@ -96,4 +106,53 @@ describe('image-security creation defaults', () => { await setSetting('default_image_quality', { nonsense: true }); await expect(getImageSecurityDefaults()).resolves.toEqual({}); }); + + 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/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index d6a0d500..f91640b5 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -31,7 +31,7 @@ const { getFrontendBaseUrl, getAbsoluteFrontendUrl } = require('../../utils/fron const downloadZipService = require('../../services/downloadZipService'); const { resolveEventFeedbackDefaults, applyFeedbackDefaults, KEYBIND_MODES } = require('../../services/feedbackDefaults'); const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, - getImageSecurityDefaults, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers'); + 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 @@ -229,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().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('use_canvas_rendering').optional().isBoolean().toBoolean(), + body('image_quality').optional().isInt({ min: 1, max: 100 }).toInt(), + body('fragmentation_level').optional().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". @@ -546,9 +553,13 @@ module.exports = (router) => { // 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. Creation-time only; see + // 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 imageSecurityDefaults = await getImageSecurityDefaults(); + const imageSecurityColumns = resolveImageSecurityColumns( + req.body, + await getImageSecurityDefaults(), + ); const effectiveEnableDevtoolsProtection = enableDevtoolsProtectionInput !== undefined ? enableDevtoolsProtectionInput @@ -622,18 +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), - // Spread AFTER the explicit columns so a value the request supplied - // still wins; each key is present only when the global setting held - // a usable value, so anything unset falls through to the column - // default exactly as before (#1296). - ...(imageSecurityDefaults.protection_level !== undefined - ? { protection_level: imageSecurityDefaults.protection_level } : {}), - ...(imageSecurityDefaults.image_quality !== undefined - ? { image_quality: imageSecurityDefaults.image_quality } : {}), - ...(imageSecurityDefaults.use_canvas_rendering !== undefined - ? { use_canvas_rendering: formatBoolean(imageSecurityDefaults.use_canvas_rendering) } : {}), - ...(imageSecurityDefaults.fragmentation_level !== undefined - ? { fragmentation_level: imageSecurityDefaults.fragmentation_level } : {}), + // 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'), diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index fd271bee..7f62ecee 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -122,6 +122,19 @@ const getDownloadProtectionDefaults = async () => { */ 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 () => { const defaults = {}; try { @@ -152,8 +165,8 @@ const getImageSecurityDefaults = async () => { // 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 = parseInt(read('default_image_quality'), 10); - if (Number.isInteger(quality) && quality >= 1 && quality <= 100) { + const quality = toInteger(read('default_image_quality')); + if (quality !== undefined && quality >= 1 && quality <= 100) { defaults.image_quality = quality; } @@ -162,8 +175,8 @@ const getImageSecurityDefaults = async () => { defaults.use_canvas_rendering = canvas; } - const fragmentation = parseInt(read('default_fragmentation_level'), 10); - if (Number.isInteger(fragmentation) && fragmentation >= 1 && fragmentation <= 10) { + const fragmentation = toInteger(read('default_fragmentation_level')); + if (fragmentation !== undefined && fragmentation >= 1 && fragmentation <= 10) { defaults.fragmentation_level = fragmentation; } } catch (error) { @@ -174,6 +187,39 @@ const getImageSecurityDefaults = async () => { 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 = {}; + const pick = (key) => (body[key] !== undefined ? body[key] : 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 @@ -608,6 +654,7 @@ module.exports = { readBooleanSetting, getDownloadProtectionDefaults, getImageSecurityDefaults, + resolveImageSecurityColumns, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, 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..54990a6d 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 } = 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().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('use_canvas_rendering').optional().isBoolean().toBoolean(), + body('image_quality').optional().isInt({ min: 1, max: 100 }).toInt(), + body('fragmentation_level').optional().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']) @@ -235,6 +244,15 @@ router.post( } 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 +342,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, From 19c518aaa50f1bd8fb7cef260a55bf3d5f3eb7f3 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 07:27:18 +0200 Subject: [PATCH 3/7] fix(security): close the remaining image-security default gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-two review follow-ups. Settings survive the tab round trip. GET returns setting_value without decoding it and ImageSecurityTab PUTs the whole fetched object back through JSON.stringify, so on SQLite one visit to the tab re-encodes every value it read. A single parse then yields the string "true", the type checks reject it, and the defaults go quietly dead — the exact bug this change exists to fix, returning by a different route. The reader now unwraps until the value stops being a JSON string, bounded. Array overrides rejected. express-validator applies isInt/isIn/isBoolean element-wise, so `image_quality: [72]` passed the chain and arrived still an array — a PG insert error, and `[false]` coerced to true by formatBoolean. Both create routes now use .not().isArray(), and the shared resolver ignores non-scalars for any future caller. Two more creation paths covered. quoteService.convertToEvent builds its own events row, so CRM-converted galleries fell back to column defaults. /:id/duplicate copies fifteen source columns including enable_devtools_protection but missed these four, so duplicating a 'maximum' gallery produced a 'standard' one — a duplicate now inherits the source's values, not the current globals, since copying the gallery is the point. The PUT /:id chain has the same array weakness. Pre-existing and outside this fix; left alone deliberately. Refs #1296 --- .../integration/imageSecurityDefaults.test.js | 33 +++++++++++++++++++ backend/src/routes/adminEvents/crud.js | 17 +++++++--- backend/src/routes/adminEvents/helpers.js | 28 ++++++++++++++-- backend/src/routes/v1/events.js | 8 ++--- backend/src/services/quoteService.js | 7 ++++ 5 files changed, 82 insertions(+), 11 deletions(-) diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js index d64b0e43..1e8bea4a 100644 --- a/backend/__tests__/integration/imageSecurityDefaults.test.js +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -107,6 +107,39 @@ describe('image-security creation defaults', () => { 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('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('resolveImageSecurityColumns', () => { it('omits every column when neither the request nor the settings supply one', () => { expect(resolveImageSecurityColumns({}, {})).toEqual({}); diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index f91640b5..0555d773 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -232,10 +232,10 @@ module.exports = (router) => { // 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().isIn(['basic', 'standard', 'enhanced', 'maximum']), - body('use_canvas_rendering').optional().isBoolean().toBoolean(), - body('image_quality').optional().isInt({ min: 1, max: 100 }).toInt(), - body('fragmentation_level').optional().isInt({ min: 1, max: 10 }).toInt(), + 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". @@ -1419,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, diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index 7f62ecee..e031c604 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -147,12 +147,25 @@ const getImageSecurityDefaults = async () => { ]) .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. + // Unwrap until it stops being a JSON string, bounded so nothing spins. const read = (key) => { const row = rows.find((r) => r.setting_key === key); if (!row) return undefined; let value = row.setting_value; - if (typeof value === 'string') { - try { value = JSON.parse(value); } catch { /* keep raw */ } + for (let i = 0; i < 4 && typeof value === 'string'; i += 1) { + let parsed; + try { parsed = JSON.parse(value); } catch { break; } + if (parsed === value) break; + value = parsed; } return value; }; @@ -203,7 +216,16 @@ const getImageSecurityDefaults = async () => { const resolveImageSecurityColumns = (body = {}, defaults = {}) => { const { formatBoolean } = require('../../utils/dbCompat'); const columns = {}; - const pick = (key) => (body[key] !== undefined ? body[key] : defaults[key]); + // 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; diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 54990a6d..57be3edd 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -184,10 +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().isIn(['basic', 'standard', 'enhanced', 'maximum']), - body('use_canvas_rendering').optional().isBoolean().toBoolean(), - body('image_quality').optional().isInt({ min: 1, max: 100 }).toInt(), - body('fragmentation_level').optional().isInt({ min: 1, max: 10 }).toInt(), + 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']) diff --git a/backend/src/services/quoteService.js b/backend/src/services/quoteService.js index 9cfdd399..e92245e1 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()); 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)) { From 0e560ebb193d8243ed4de059ea150f3f64fa1409 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 07:37:19 +0200 Subject: [PATCH 4/7] fix(security): decode settings at the API boundary and honour the transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-three review follow-ups. getImageSecurityDefaults now accepts a transaction, the way getAppSetting two lines above it already does. quoteService.convertToEvent called it from inside db.transaction() through the global db; sqlite3 runs a single-connection pool, so that read would have waited on the connection its own transaction was holding until the acquire timeout, and the helper's catch would then have swallowed the error and dropped the defaults silently. The double-encoding is fixed where it starts. GET /admin/image-security/settings returned setting_value undecoded, so it shipped "true" to a tab that types the field as boolean — and since the tab PUTs the whole object back through JSON.stringify, every save wrapped another layer around values nobody edited. It decodes now, so a round trip is idempotent. The tab is the only consumer of that endpoint. The reader unwraps to any depth instead of four. The depth on an existing install is however many times someone opened that tab, which is not a number to cap. It terminates because each parse of a string is strictly shorter than its input. Refs #1296 --- .../integration/imageSecurityDefaults.test.js | 9 +++++++++ backend/src/routes/adminEvents/helpers.js | 18 ++++++++++++++---- backend/src/routes/adminImageSecurity.js | 19 ++++++++++++++++--- backend/src/services/quoteService.js | 2 +- 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js index 1e8bea4a..74ca0c11 100644 --- a/backend/__tests__/integration/imageSecurityDefaults.test.js +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -134,6 +134,15 @@ describe('image-security creation defaults', () => { 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({}); diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index e031c604..f6fef319 100644 --- a/backend/src/routes/adminEvents/helpers.js +++ b/backend/src/routes/adminEvents/helpers.js @@ -135,10 +135,16 @@ const toInteger = (value) => { return undefined; }; -const getImageSecurityDefaults = async () => { +const getImageSecurityDefaults = async (trx = null) => { const defaults = {}; try { - const rows = await db('app_settings') + // 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', @@ -156,12 +162,16 @@ const getImageSecurityDefaults = async () => { // `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. - // Unwrap until it stops being a JSON string, bounded so nothing spins. + // 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; let value = row.setting_value; - for (let i = 0; i < 4 && typeof value === 'string'; i += 1) { + while (typeof value === 'string') { let parsed; try { parsed = JSON.parse(value); } catch { break; } if (parsed === value) break; diff --git a/backend/src/routes/adminImageSecurity.js b/backend/src/routes/adminImageSecurity.js index 877a5421..e83df3b3 100644 --- a/backend/src/routes/adminImageSecurity.js +++ b/backend/src/routes/adminImageSecurity.js @@ -32,9 +32,22 @@ 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. + let value = setting.setting_value; + while (typeof value === 'string') { + let parsed; + try { parsed = JSON.parse(value); } catch { break; } + if (parsed === value) break; + value = parsed; + } + config[setting.setting_key] = value; }); res.json(config); diff --git a/backend/src/services/quoteService.js b/backend/src/services/quoteService.js index e92245e1..e6943973 100644 --- a/backend/src/services/quoteService.js +++ b/backend/src/services/quoteService.js @@ -1684,7 +1684,7 @@ async function convertToEvent(quoteId, adminId, options = {}) { // — 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()); + 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}`, From 0deef2584f4a6287bb947bdc545f585ae30aab67 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 07:48:41 +0200 Subject: [PATCH 5/7] fix(security): one settings decoder, and the last creation path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-four review follow-ups. Every reader of app_settings now shares decodeSettingValue. The previous commit taught the GET handler to decode, which on a legacy SQLite install made the tab show devtools protection as disabled while readBooleanSetting — parsing once, getting the string 'false', rejecting it — left new galleries with it enabled. A decoder used by only some readers is worse than none, because the UI and the behaviour disagree. readBooleanSetting, getImageSecurityDefaults, the v1 devtools fallback and the settings GET all use it now. Standalone contract conversion covered. contract/conversions.js takes Path B and inserts its own event row when the contract has no source quote, so signed standalone contracts were the last path still landing on the migration-038 column defaults. Refs #1296 --- .../integration/imageSecurityDefaults.test.js | 28 ++++++++++++- backend/src/routes/adminEvents/helpers.js | 41 +++++++++++++------ backend/src/routes/adminImageSecurity.js | 10 +---- backend/src/routes/v1/events.js | 11 ++--- backend/src/services/contract/conversions.js | 7 ++++ 5 files changed, 71 insertions(+), 26 deletions(-) diff --git a/backend/__tests__/integration/imageSecurityDefaults.test.js b/backend/__tests__/integration/imageSecurityDefaults.test.js index 74ca0c11..b2998705 100644 --- a/backend/__tests__/integration/imageSecurityDefaults.test.js +++ b/backend/__tests__/integration/imageSecurityDefaults.test.js @@ -24,10 +24,11 @@ describe('image-security creation defaults', () => { let cleanup; let getImageSecurityDefaults; let resolveImageSecurityColumns; + let readBooleanSetting; beforeAll(async () => { ({ db, cleanup } = await bootCrmDb()); - ({ getImageSecurityDefaults, resolveImageSecurityColumns } = + ({ getImageSecurityDefaults, resolveImageSecurityColumns, readBooleanSetting } = require('../../src/routes/adminEvents/helpers')); }, 120000); @@ -149,6 +150,31 @@ describe('image-security creation defaults', () => { }); }); + 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({}); diff --git a/backend/src/routes/adminEvents/helpers.js b/backend/src/routes/adminEvents/helpers.js index f6fef319..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 }); @@ -170,14 +193,7 @@ const getImageSecurityDefaults = async (trx = null) => { const read = (key) => { const row = rows.find((r) => r.setting_key === key); if (!row) return undefined; - let value = row.setting_value; - while (typeof value === 'string') { - let parsed; - try { parsed = JSON.parse(value); } catch { break; } - if (parsed === value) break; - value = parsed; - } - return value; + return decodeSettingValue(row.setting_value); }; const level = read('default_protection_level'); @@ -684,6 +700,7 @@ module.exports = { getStoragePath, getEventFieldRequirements, readBooleanSetting, + decodeSettingValue, getDownloadProtectionDefaults, getImageSecurityDefaults, resolveImageSecurityColumns, diff --git a/backend/src/routes/adminImageSecurity.js b/backend/src/routes/adminImageSecurity.js index e83df3b3..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(); @@ -40,14 +41,7 @@ router.get('/settings', adminAuth, requirePermission(['settings.view', 'image_se // 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. - let value = setting.setting_value; - while (typeof value === 'string') { - let parsed; - try { parsed = JSON.parse(value); } catch { break; } - if (parsed === value) break; - value = parsed; - } - config[setting.setting_key] = value; + config[setting.setting_key] = decodeSettingValue(setting.setting_value); }); res.json(config); diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 57be3edd..121c22ea 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -37,7 +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 } = require('../adminEvents/helpers'); +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'); @@ -236,10 +236,11 @@ 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); 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)) { From 8f3436f17d6390a776c4258c53475d4c6038a63e Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 08:33:03 +0200 Subject: [PATCH 6/7] fix(security): reject array values on the event update route too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PUT /:id has the same weakness the create chains just had: express-validator runs isIn/isBoolean/isInt element-wise, so `image_quality: [72]` satisfies every check and stays an array. This handler spreads req.body straight into the update, so the array reached a scalar column — a PG insert error, and `[false]` read as true. Covers all six fields in that block, not only the four this PR is about. enable_devtools_protection and overlay_protection sit in the same list with the identical flaw, and leaving two known holes next to four closed ones would have been the odd choice. Refs #1296 --- backend/src/routes/adminEvents/crud.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index 0555d773..6b18236b 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -1591,13 +1591,18 @@ module.exports = (router) => { body('source_mode').optional().isIn(['managed', 'reference']), body('external_path').optional({ nullable: true }).isString().trim(), body('require_password').optional().isBoolean(), - // Download protection settings - body('protection_level').optional().isIn(['basic', 'standard', 'enhanced', 'maximum']), - body('enable_devtools_protection').optional().isBoolean(), - body('use_canvas_rendering').optional().isBoolean(), - body('overlay_protection').optional().isBoolean(), - body('image_quality').optional().isInt({ min: 1, max: 100 }), - body('fragmentation_level').optional().isInt({ min: 1, max: 10 }), + // Download protection settings. .not().isArray() because + // express-validator runs isIn/isBoolean/isInt element-wise: a + // single-element array like `image_quality: [72]` satisfies every check + // and stays an array, and this handler spreads req.body straight into + // the update — so it reached a scalar column as an array (a PG error, + // and `[false]` read as true). Same guard as the create chain (#1296). + body('protection_level').optional().not().isArray().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('enable_devtools_protection').optional().not().isArray().isBoolean(), + body('use_canvas_rendering').optional().not().isArray().isBoolean(), + body('overlay_protection').optional().not().isArray().isBoolean(), + body('image_quality').optional().not().isArray().isInt({ min: 1, max: 100 }), + body('fragmentation_level').optional().not().isArray().isInt({ min: 1, max: 10 }), body('password').optional().isString().custom((value) => { if (value === undefined || value === null || value === '') { return true; From 933f2d8e0ee0685128f2e8f8bed5169a06b4116c Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 11:47:00 +0200 Subject: [PATCH 7/7] fix(security): reject array values for every field on the event update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the six per-field .not().isArray() guards from the previous commit. Those were too narrow, and arbitrarily so. PUT /:id spreads req.body into `updates` (crud.js:1631) and passes it to .update() (:1990) with only targeted deletes in between — there is no column allow-list. express-validator applies isInt/isIn/isBoolean element-wise to arrays, so a single-element array satisfies its field validator and survives the whole way to the column. That is true of all 44 validated fields, not of the protection block I happened to be looking at; seven of them also run through formatBoolean, where [false] reads as true. So the guard belongs where the body is spread, not on chosen fields. `customer_account_ids` is the only field legitimately an array — it has an isArray() validator and its own element rules — and it is deleted from `updates` before the write, so exempting it costs nothing. Tested across the protection fields and two outside that block, plus the customer_account_ids exemption. With the guard's condition disabled, exactly those six array cases fail and the other 15 in the suite pass. Refs #1296 --- .../routes/adminEvents.smoke.test.js | 38 +++++++++++++++++++ backend/src/routes/adminEvents/crud.js | 38 +++++++++++++------ 2 files changed, 64 insertions(+), 12 deletions(-) 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 6b18236b..60d2642f 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -1591,18 +1591,13 @@ module.exports = (router) => { body('source_mode').optional().isIn(['managed', 'reference']), body('external_path').optional({ nullable: true }).isString().trim(), body('require_password').optional().isBoolean(), - // Download protection settings. .not().isArray() because - // express-validator runs isIn/isBoolean/isInt element-wise: a - // single-element array like `image_quality: [72]` satisfies every check - // and stays an array, and this handler spreads req.body straight into - // the update — so it reached a scalar column as an array (a PG error, - // and `[false]` read as true). Same guard as the create chain (#1296). - body('protection_level').optional().not().isArray().isIn(['basic', 'standard', 'enhanced', 'maximum']), - body('enable_devtools_protection').optional().not().isArray().isBoolean(), - body('use_canvas_rendering').optional().not().isArray().isBoolean(), - body('overlay_protection').optional().not().isArray().isBoolean(), - body('image_quality').optional().not().isArray().isInt({ min: 1, max: 100 }), - body('fragmentation_level').optional().not().isArray().isInt({ min: 1, max: 10 }), + // Download protection settings + body('protection_level').optional().isIn(['basic', 'standard', 'enhanced', 'maximum']), + body('enable_devtools_protection').optional().isBoolean(), + body('use_canvas_rendering').optional().isBoolean(), + body('overlay_protection').optional().isBoolean(), + body('image_quality').optional().isInt({ min: 1, max: 100 }), + body('fragmentation_level').optional().isInt({ min: 1, max: 10 }), body('password').optional().isString().custom((value) => { if (value === undefined || value === null || value === '') { return true; @@ -1663,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