From 19c518aaa50f1bd8fb7cef260a55bf3d5f3eb7f3 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 07:27:18 +0200 Subject: [PATCH] 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)) {