From 933f2d8e0ee0685128f2e8f8bed5169a06b4116c Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 11:47:00 +0200 Subject: [PATCH] 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