fix(security): reject array values for every field on the event update
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
This commit is contained in:
@@ -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',
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user