Skip to content

plugin-security: array inserts are never check-gated — the write gate's own guard skips any array payload, and the REST import runner reaches engine.insert with one #16877

Description

@os-trump

Filed unassigned and unlabelled for triage. Found by the isolated CONTRACT_REVIEW_TIER review of PR #16805 as finding F3 (not blocking that PR): #16805 (comment)

The review's expectation is quoted, because it is the reason this is a card and not a note: "file rather than 'noted, not filed'; a caller-reachable route around a p1 gate should have a number."

What happens

The security middleware's write check gate guards itself with !Array.isArray(opCtx.data) (packages/plugins/plugin-security/src/security-plugin.ts, around line 2839). An array payload therefore never has the judgement installed and never has a filter matched against it — on origin/main and on PR #16805's head alike. No array insert is check-gated.

That is pre-existing and it is documented in the gate. What makes it a card rather than a documented boundary is that the route is caller-reachable:

  • @objectstack/rest's import-runner.ts batches CREATE rows through createManyData / insertManyData;
  • metadata-protocol's createManyData (around lines 12295-12307) calls engine.insert(records, { context }) with the array;
  • engine.insertMany likewise forwards to insert(array, { __partialRowErrors: true }).

So the same policy that refuses a single-row insert admits the identical row when it arrives as one element of an array. #16608 measured what that gate is protecting: a cross-organization write, which is why that card is priority:p1.

Why it is cheap NOW and was not before

Before PR #16805 the judgement was a filter matched against opCtx.data inside the middleware, where an array payload had no natural post-image at all. After it, the judgement is installed on the operation context and run by ObjectQL.insert on every live row — the seam's evaluate already receives readonly Record<string, unknown>[], and the engine already skips rows the declared-field door culled from a partial batch.

So the insert half of that guard can be lifted without inventing a mechanism: the seam is already shaped for N rows. What still needs deciding is the batch semantics, and that is the real content of this card:

  • does one refused row refuse the whole batch, or is it culled per-row the way __partialRowErrors mode culls a validation failure?
  • if per-row, which envelope does the row's outcome carry, and does a partial batch still report PERMISSION_DENIED per row?
  • does a refusal in either shape still cost nothing (no autonumber consumed, no sys_secret minted) for the surviving rows as well as the refused ones?

⚠️ Scope note. The predicate (multi: true) UPDATE half of the same guard is a separate gap — an update selected by a predicate rather than by id cannot form a post-image in the middleware at all, and the gate logs and skips rather than guessing. That half is not covered by this card and is not made cheaper by #16805's seam.

Conformance

Whatever shape is chosen, the pin belongs on the same footing as #16608's cell (packages/plugins/plugin-security/src/insert-check-post-image.test.ts): both driver families, refusal and non-landing asserted as separate facts off the driver's own table.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions