Record-scoped allowRead: unified row-level read access control (#1422 gap 2) by kriszyp · Pull Request #1786 · HarperFast/harper

kriszyp · 2026-07-14T05:11:45Z

Replaces the short-lived static allowReadRecord hook (#1768, unreleased) with a unified model, and closes #1422 (allowRead has two critical security gaps) gap 2 — the row-level enforcement that was deliberately dropped from #1489 (fail closed when allow* hooks throw) under the earlier "grant hooks only" stance, now reversed: allowRead (and friends) are the key mechanism for row-level access control. Closes #1419 (Subscription delivery bypasses row-level allowRead).

The model

One allowRead(user, target, context) definition serves both scopes:

Framework defaults (Resource base + the table RBAC check) are this-free and marked isDefaultAllowRead — they keep today's single entry evaluation. Zero new overhead for non-overriding tables.
An application override on a table is record-scoped: evaluated once per record with this = the (frozen) record during query execution — pushed into HNSW traversal for vector sorts, applied as a post-filter otherwise, re-checked on the materialized record after caching-source revalidation. super.allowRead(...) composes the RBAC baseline into the same function.

The authorize wrapper defers table collection reads (supportsRowLevelAllowRead + overridden) to the per-record path; get/query on collections route into search() which consumes checkPermission and arms the guard. Single-record get(id) keeps the entry check (record loaded; TableResource proxies attribute reads to the record, so the same override reads correctly there — this closes #1422's "single-record 403s but collection leaks every row" asymmetry).

Fail-closed properties

A throwing per-record check denies that record (allowRead has two critical security gaps: (1) throw → fail-open, (2) not enforced on collection scans #1422 gap 1 parity).
A Promise-returning check is a hard ClientError (must be synchronous), never fail-open truthiness.
Dispatch is via the resolved class method — never a record.allowRead property lookup — so a record attribute named allowRead can't shadow the check.
Subscription delivery is enforced per event (Subscription delivery bypasses row-level allowRead — all rows leak to any collection subscriber #1419, revives the closed fix: enforce row-level allowRead during subscription delivery (#1419) #1524): a sync record-scoped allowRead filters each record-bearing event (this = the event's record) across the live listener, queue drain, reload re-snapshot, and previousCount history replay (denied rows don't consume count slots). subscribe/connect defer at entry for sync overrides — delivery is the enforcement; async overrides stay entry-checked (awaited, fail-closed). The Live subscriptions (SSE/MQTT/WS) continue delivering events after drop_user / role revocation — stale-auth leak #1414 re-auth recheck verifies the framework-default (table-level) grant for record-scoped subscriptions instead of spuriously killing them. Known limitation: delete tombstones / message payloads / rawEvents don't carry the full record; row-authorizing those event types is deferred.
Role-level column RBAC (attribute_permissions) still applies under an override: search() runs the default allowRead for its target.select-narrowing side effect before deferring row access.

Records also expose a non-enumerable allowRead delegate on the per-table structPrototype for direct app-code use (delegates to the table default; engine enforcement resolves the active subclass instead).

Where to look

resources/Resource.ts — the deferral gate. Its predicate set (read-only, non-subscribe, collection-shaped, table, overridden) is the security-critical decision; the get arm requires isCollection to mirror the isSearchTarget routing it relies on.
resources/Table.ts search() — override detection + guard construction; the composition call for column narrowing.
Behavioral change to note: an existing table subclass that overrides allowRead and relied on a collection-entry verdict now gets per-record filtering instead (200 + filtered rows rather than a blanket 403). This is the intended semantics of the new model (approved design decision), but it's a semantic shift on a security surface — release-notes callout included in the docs PR.

Known limitations (deliberate)

~~GraphQL~~ GraphQL is covered: it now requests authorization via query.checkPermission (replacing the context-level authorize alias, for consistency with REST), and since GraphQL invokes the static search wrapper, row-level enforcement applies to its collection queries. Verified by the graphql integration suite.
Per-record super.allowRead calls can't affect select narrowing (transform is built before iteration); narrowing is composed at entry instead.

Review

Cross-model review attempted: both outside-model legs failed this round (Gemini/agy empty twice; Codex CLI invocation failed — its output was a same-model trace). The Opus domain adjudication pass ran in full and its two significant findings (column-narrowing composition, deferral-gate asymmetry) are fixed in the last commit. Human review should weigh the deferral-gate predicate carefully given the reduced outside-model coverage.

Docs: companion update in HarperFast/documentation#580 (reworked from allowReadRecord to this model).

Generated by KrAIs (Claude Opus 4.8).

…gap 2) Replace the short-lived static allowReadRecord (#1241/#1768, unreleased) with a unified model: one allowRead(user, target, context) definition serves both scopes. The framework defaults (Resource base + table RBAC check) are this-free and marked isDefaultAllowRead; they keep the single entry evaluation. An application-OVERRIDDEN allowRead on a table is record-scoped: evaluated once per record with `this` = the (frozen) record during query execution — pushed into HNSW traversal for vector sorts, applied as a post-filter otherwise, and re-checked on the materialized record after caching-source revalidation. This closes #1422 gap 2 (single-record GET 403'd but collection GET leaked every row): the authorize wrapper now defers collection reads on tables (supportsRowLevelAllowRead) with an overridden allowRead to the per-record path instead of evaluating the override against a collection resource with no record loaded. Single-record get() keeps the entry check (record loaded; TableResource proxies attribute reads to the record, so record-scoped overrides read correctly there too). Per-record calls are fail-closed on throw (#1422 gap 1 parity), must be synchronous (a returned Promise is a clear error, not fail-open), and dispatch via the resolved class method — never a record.allowRead property lookup, so a record attribute named allowRead can't shadow the check. Records also expose a non-enumerable allowRead delegate on the per-table structPrototype for direct app-code use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…nt unchecked) Subscription delivery never reaches search(), so deferring the entry check for a record-scoped allowRead would grant a collection subscription with no check at all. Subscriptions keep the entry evaluation (fail-closed for record-scoped overrides) until per-event delivery checks exist (#1419). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…en get deferral gate - A record-scoped allowRead override no longer voids role-level column RBAC: search() runs the DEFAULT table allowRead for its target.select narrowing side effect before deferring row access to the per-record override (a per-record super call could not restore it — the select transform is built before iteration). - The wrapper deferral gate for `get` now requires isCollection, mirroring the isSearchTarget routing it relies on, so a hypothetical {id: null, isCollection: false} get keeps the entry check instead of skipping both checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

claude · 2026-07-14T05:22:03Z

Reviewed 5444791 (fixes for the 4 threads heskew left open: QUERY-body checkPermission:false bypass in Resource.ts, unfrozen source-revalidation guard record in Table.ts now via frozenRecordView, rejected-thenable leaking to unhandledRejection in both the query guard and subscription filter, and unbounded previousCount backfill scan). All four fixes are correct, each has a targeted regression test, and all 15 review threads on this PR are resolved. No blockers found.

… awaited entry check A declared-async allowRead override is excluded from the per-record deferral (the sync traversal cannot await it); the authorize wrapper awaits its verdict at entry and fails closed on rejection, preserving the #1422 gap-1 contract (allowread-fail-closed integration suite). A sync-declared override that returns a thenable denies that record (fail closed) with a one-time warning, instead of aborting the query or failing open on promise truthiness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ix scans are multi-record) A present id on search/query is a starts_with/prefix SEED — a multi-record scan — so the deferral gate keying on id == null wrongly kept the entry check there: a record-scoped override evaluated against a bare resource would spuriously deny the scan or, for a permissive-default override, leak every prefix-matched row (#1422 gap 2 reintroduced). Only `get` has single-record id semantics; its arm still requires isCollection to defer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… + GraphQL checkPermission - Subscription delivery filters each record-bearing event through a sync record-scoped allowRead (`this` = event record), covering the live listener, queue drain, reload re-snapshot (#495), and previousCount history replay (denied rows no longer consume count slots). Gated on checkPermission (internal subscribers/replication keep full delivery) and fail-closed on throw or thenable. subscribe/connect now DEFER at entry for sync overrides — delivery is the enforcement — while async overrides remain entry-checked (awaited, fail-closed). - #1414 re-auth recheck runs the framework-default (table-level) allowRead for record-scoped subscriptions, so periodic re-auth verifies the RBAC grant without spuriously killing the subscription. - GraphQL requests authorization via query.checkPermission instead of the context-level `authorize` alias — consistent with REST, and row-level enforcement applies to GraphQL collection queries. - Revives the #1524 integration fixture/test (SSE whole-table subscription delivers only the requesting user's rows) plus unit coverage for granted+filtered and async-denied subscribe. Known limitation (as in #1524): delete tombstones, message payloads, and rawEvents don't carry the full record; row-authorizing those event types is deferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Deferring subscribe/connect for an overridden allowRead bypassed CONNECTION-level overrides that decide on non-record state: an MQTT topic ACL (@harperdb/acl-connect, and the mqtt.test %u/topic ACL suite) overrides allowRead to authorize the topic at subscribe time, which the deferral skipped — granting SUBACKs that must be rejected. subscribe now always runs the entry check (the connection grant, which honors topic ACLs and the collection-permissive contract for record-level overrides), and Table.subscribe ADDITIONALLY filters each delivered event per record (#1419). Delivery filtering is gated on a user principal (checkPermission is consumed by the entry check before subscribe runs) plus a sync override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…scription (#1414 × #1419) Adds an alter_role revocation case to the #1419 suite: a collection-permissive override opens a whole-table subscription, then the role loses read in place (user still present, so termination can only come from the re-auth default table-grant fallback). Verified to fail when the prototype-walk fallback is disabled (the override alone would keep the revoked subscription alive). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…de), not the default The prior default-fallback (walk past the override to the framework default) missed CONNECTION-LEVEL revocation: an MQTT topic ACL override gates the connection at subscribe entry, and re-auth must re-run that same override to catch a revoked topic grant — walking to the default only re-checked table RBAC. Now recheck calls resource.allowRead(fresh, ...) directly, mirroring the entry check exactly. Record-scoped overrides compose the base RBAC grant via super.allowRead (updated the #1419 fixture + docs to show this), so revoking the role's read still terminates a record-scoped subscription — verified by the alter_role revocation test, which now exercises the mirrored path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…tale/anon subscription gate; clarify async Four findings from Barber AI: - HIGH: the QUERY verb calls search(data, query) so search's target is the body, not the checkPermission-bearing URL target — row-level allowRead was never armed → full unfiltered result set. The query action now threads checkPermission onto the body. Regression test added. - HIGH (adjusted): async allowRead can't do sync per-record/per-event filtering. Rather than hard-reject it (which would regress the shipped #1422 async fail-closed contract — ThrowsAsync), async overrides run ONLY the awaited entry check (table/connection scope, as before this feature) and get NO row filtering, with a one-time warning so the collection-permissive-async fail-open isn't silent. - MEDIUM: subscription delivery evaluated the override against a subscribe-time user snapshot (defeating #1414 re-auth for overrides keyed on mutable claims) and skipped filtering entirely for an anonymous-but-authorized subscription. Now evaluates the LIVE subContext.user, and the gate keys on a durable rowLevelAuthChecked flag stamped by the wrapper (covers anonymous; still excludes internal subscribers). - MEDIUM: delivery passed the shared unfrozen primaryStore record to the override (a write to `this` would mutate the cache). Now freezes it first, mirroring the query path. Regression test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…in-place re-auth) cb1kenobi nit — the existing fixtures key on a stable field, so a regression back to the subscribe-time user snapshot would pass green. Adds a delivery test whose override gates on a mutable claim (tier), downgrades context.user mid-stream, and asserts subsequent events are dropped. Verified it fails if the filter reads a captured snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

heskew

AI-assisted review by Codex (GPT-5).

…idation guard view, swallow rejected-thenable rejections, bound previousCount backfill scan Addresses 4 unresolved review threads on #1786 (record-scoped allowRead, #1422 gap 2): - Resource.ts query(): checkPermission is now framework-owned in the QUERY body — always overwritten (or deleted when unset) rather than only filled when nullish, closing a bypass where a client-supplied `checkPermission: false` disabled Table.search's row-level guard. - Table.ts: the record-scoped allowRead guard on the caching-table source-revalidation path now evaluates a read-only Proxy view (frozenRecordView) of the record instead of the live object, so an override that writes through `this` can't corrupt data the deferred commit still needs to mutate and persist. A shallow copy was rejected (see commit body / PR) in favor of a write-blocking Proxy: a copy would eagerly invoke lazy-decode getters and mishandle Array/Date/ Map/Set roots. - Table.ts: a rejected thenable from a sync-declared allowRead (both the query-traversal guard and the subscription allowsEvent filter) now gets a `.then(undefined, ...)` handler attached before being treated as denied, so it no longer reaches the global unhandledRejection logger. - Table.ts: a subscription's previousCount history backfill now bounds audit records INSPECTED (in-scope for the table/id) independently of `count` (events AUTHORIZED), so an all-deny row-level policy can't force a walk of the full retained audit log; also stops early if the subscription closes mid-scan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…letes, per-element allow* on array puts Write-side corollary to #1786 (record-scoped allowRead), closing the gaps where allowUpdate/allowCreate/allowDelete were enforced once per request on multi-record writes: - Query-shaped delete with an application-overridden allowDelete defers its entry check (gated on the framework delete's enforcesCheckPermission marker, so a subclass overriding delete() keeps the entry check) and is enforced once per matching record with FILTER semantics: denied/throwing rows are skipped; limit/offset count allowed rows. `this` is a per-row resource instance, so schema properties and super.allowDelete composition both work, and the target-supplied permission operand is preserved for the RBAC baseline. - Array puts (static collection put and direct instance put) are authorized per element — create-vs-update chosen by that element's existence — with FAIL semantics: one denial throws AccessViolation, aborting the whole transaction. The deferred check runs in the static put action against the RESOLVED body, so a promise body can't bypass it and enforcement is framework-owned for every resource class. - The write paths are async, so unlike the record-scoped allowRead, async overrides participate in per-record evaluation (awaited, fail-closed). - transactional() argument parsing now carries the collection-ness it determined onto the fallback-constructed query, so programmatic `put(records, context)` resolves a collection resource like the equivalent REST target. Operations-API/SQL writes deliberately keep operation-level RBAC only (they never arm checkPermission; raw tables have no override surface). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

kriszyp and others added 3 commits July 13, 2026 22:48

kriszyp requested review from heskew and ldt1996 July 14, 2026 05:12

kriszyp mentioned this pull request Jul 14, 2026

allowRead has two critical security gaps: (1) throw → fail-open, (2) not enforced on collection scans #1422

Closed

This comment was marked as resolved.

Sign in to view

claude Bot reviewed Jul 14, 2026

View reviewed changes