fix(events): drop non-canonical keys from the event update before any check runs (#1346)
PUT /admin/events/:id spreads the body into the UPDATE. SQLite resolves
quoted identifiers case-insensitively, so `{ "Event_Name": ... }` lands
on event_name there — while every check in the handler (validators, the
field-level permission guards, the deny-set) keys on the exact lowercase
name. The deny-set already case-folded for its own columns; every other
column was reachable through a spelling variant.
Every events column and every input-only key the handler accepts is
lowercase snake_case, so a key with any uppercase in it is not something
a legitimate client sends. Such keys are now removed before anything
looks at the body. Postgres was unaffected (quoted identifiers are
case-sensitive there; a variant produced a 500 instead).
Surfaced by the Codex review of the folder-watcher change, where a
photos.upload guard on external_watch could be walked around this way.
Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
co-authored by
Paul Nothaft
parent
8cc7d7d14a
commit
810801a9ab
@@ -137,11 +137,17 @@ describe('authorization / ownership gaps', () => {
|
|||||||
// Case-variant keys — SQLite matches columns case-insensitively.
|
// Case-variant keys — SQLite matches columns case-insensitively.
|
||||||
Password_Hash: 'case-hijack-hash',
|
Password_Hash: 'case-hijack-hash',
|
||||||
Created_By: 88888,
|
Created_By: 88888,
|
||||||
|
// A case variant of an ORDINARY column must not reach the UPDATE
|
||||||
|
// either: field-level guards in the handler key on the exact name,
|
||||||
|
// and on SQLite the variant would still land on the real column.
|
||||||
|
Event_Name: 'case-variant-name',
|
||||||
|
Welcome_Message: 'case-variant-welcome',
|
||||||
});
|
});
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(200);
|
||||||
|
|
||||||
const row = await db('events').where({ id: eventId }).first();
|
const row = await db('events').where({ id: eventId }).first();
|
||||||
expect(row.event_name).toBe('After'); // legit field applied
|
expect(row.event_name).toBe('After'); // legit field applied; Event_Name variant dropped
|
||||||
|
expect(row.welcome_message).toBeFalsy(); // case variant of an ordinary column dropped
|
||||||
expect(row.created_by).toBe(superId); // ownership untouched (+ case-variant)
|
expect(row.created_by).toBe(superId); // ownership untouched (+ case-variant)
|
||||||
expect(row.slug).toBe('authz-mass-assign'); // routing identity untouched
|
expect(row.slug).toBe('authz-mass-assign'); // routing identity untouched
|
||||||
expect(row.share_token).toBe(seedShareToken); // secret untouched
|
expect(row.share_token).toBe(seedShareToken); // secret untouched
|
||||||
|
|||||||
@@ -1730,10 +1730,18 @@ module.exports = (router) => {
|
|||||||
// Legacy mirrors — rejected explicitly below in favour of customer_*.
|
// Legacy mirrors — rejected explicitly below in favour of customer_*.
|
||||||
'host_name', 'host_email',
|
'host_name', 'host_email',
|
||||||
];
|
];
|
||||||
// Case-insensitive match: SQLite treats quoted identifiers
|
// Only canonical keys reach the UPDATE. SQLite resolves quoted
|
||||||
// case-insensitively, so a `{ "Password_Hash": ... }` key would
|
// identifiers case-insensitively, so `{ "Event_Name": ... }` lands on
|
||||||
// otherwise survive a case-sensitive delete and still hit the real
|
// event_name there while every check in this handler — validators,
|
||||||
// column (codex review).
|
// the permission guards on individual fields, the deny-set below — is
|
||||||
|
// keyed on the exact lowercase name. Every events column and every
|
||||||
|
// input-only key this handler accepts is lowercase snake_case, so a key
|
||||||
|
// with any uppercase in it is not something a legitimate client sends;
|
||||||
|
// it is dropped before anything looks at it. The deny-set keeps its own
|
||||||
|
// case-folding as belt and braces (GHSA-3rqx).
|
||||||
|
for (const key of Object.keys(updates)) {
|
||||||
|
if (key !== key.toLowerCase()) delete updates[key];
|
||||||
|
}
|
||||||
const denied = new Set(IMMUTABLE_EVENT_COLUMNS.map((c) => c.toLowerCase()));
|
const denied = new Set(IMMUTABLE_EVENT_COLUMNS.map((c) => c.toLowerCase()));
|
||||||
for (const key of Object.keys(updates)) {
|
for (const key of Object.keys(updates)) {
|
||||||
if (denied.has(key.toLowerCase())) delete updates[key];
|
if (denied.has(key.toLowerCase())) delete updates[key];
|
||||||
|
|||||||
Reference in New Issue
Block a user