|
| 1 | +--- |
| 2 | +"@objectstack/plugin-approvals": patch |
| 3 | +--- |
| 4 | + |
| 5 | +fix(approvals): the record lock now holds for predicate (`multi`) updates (#4778) |
| 6 | + |
| 7 | +The ADR-0019 record lock — "while a record has a pending `sys_approval_request`, |
| 8 | +block edits to it" — was enforced only for updates that reach the hook with an |
| 9 | +`input.id`. The engine extracts that id from a **scalar** `where.id` alone; an |
| 10 | +operator object (`{ $in: [...] }`) or any other predicate is a multi-row write |
| 11 | +that routes to `updateMany` and arrives with no id. The hook opened with |
| 12 | +`if (!id) return`, so it read *"no row was resolved"* as *"there is nothing to |
| 13 | +authorize"* when the truth was *"nothing was ever queried"*. |
| 14 | + |
| 15 | +Rewriting the very same edit as `multi: true` therefore walked straight past the |
| 16 | +lock: |
| 17 | + |
| 18 | +```ts |
| 19 | +// rec_1 carries a pending approval, lockRecord is not disabled |
| 20 | +await ql.update('crm_opportunity', { amount: 999 }, { where: { id: 'rec_1' } }); // RECORD_LOCKED |
| 21 | +await ql.update('crm_opportunity', { amount: 999 }, { where: { id: { $in: ['rec_1'] } }, multi: true }); // went through |
| 22 | +await ql.update('crm_opportunity', { amount: 999 }, { where: { name: 'x' }, multi: true }); // went through |
| 23 | +``` |
| 24 | + |
| 25 | +No privilege was needed for that bypass — not an `admin` role, not `isSystem`, |
| 26 | +not `lockRecord: false`, not a whitelisted `approvalStatusField`. Every caller |
| 27 | +shape that can spell a predicate (SDK, ObjectQL, a flow's `update_record`) could |
| 28 | +produce it. It is the same fail-open reasoning fixed for `sys_attachment` |
| 29 | +(#4757) and `sys_comment` (#4630), in the one place where it needed no |
| 30 | +privilege at all. |
| 31 | + |
| 32 | +**The hook now resolves the rows a write touches before deciding.** By-id writes |
| 33 | +are unchanged (the driver writes by primary key, so the rest of `where` must not |
| 34 | +narrow the verdict). A predicate write is decided by intersecting the caller's |
| 35 | +predicate with the records that are actually locked — which is also what keeps |
| 36 | +it cheap: the query is bounded by the object's **pending approvals**, never by |
| 37 | +the update's match set, so a mass update of 50 000 unlocked rows costs one |
| 38 | +bookkeeping probe and is allowed. An unscoped `multi` update over the whole |
| 39 | +table reaches every locked row of the object and is refused while any is held. |
| 40 | + |
| 41 | +**Fail-closed, both ways.** Past 1 000 locked records — the bound the attachment |
| 42 | +and comment guards use — or if the intersection query fails, the write is |
| 43 | +refused rather than allowed: the lock could not prove the write misses a locked |
| 44 | +row. The approvals bookkeeping being unreadable at all stays the one fail-open, |
| 45 | +as before: this hook is global over every object, so a kernel without |
| 46 | +`sys_approval_request` would otherwise refuse every update in the deployment. |
| 47 | +Both the bookkeeping and the match-set resolution are read under a **system** |
| 48 | +context — a guard's own input must never be narrowed by the caller's |
| 49 | +visibility, since a locked row you cannot read is still a row you may not write. |
| 50 | + |
| 51 | +**Every exemption moved with the guard**, which is the other way this class of |
| 52 | +fix goes wrong — a guard extended to more rows that carries only its deny rules |
| 53 | +turns a fail-open into a false-positive. `isSystem`, the `admin` override, the |
| 54 | +`approvalStatusField` status mirror, `lockRecord: false` and the owning run's |
| 55 | +`flowRunId` (#3456 / #3712) all decide a predicate write exactly as they decide |
| 56 | +a by-id write, each pinned by tests on both predicate shapes. Refusals now name |
| 57 | +the record and object that are locked. |
0 commit comments