Restore operation-scoped read authorization and add explicit row filters - #1915
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the transactional logic in resources/Resource.ts to ensure that table subscriptions still enforce connection grants and row-level delivery markers when opting out of instance loading (loadAsInstance = false). It also adds comprehensive unit tests to verify this behavior. The review feedback suggests replacing a fixed setTimeout in the new tests with a condition-wait helper to prevent potential test flakiness in slower environments.
|
Reviewed; no blockers found. |
702f3e1 to
8fdcf3e
Compare
Devin-Holland
left a comment
There was a problem hiding this comment.
Re-approving at 1e8e4f64, and re-stating scope deliberately — the approval you were carrying from me was given to a different PR. At the sha I approved on 07-23 (85b0eee1) this was 2 files, +140/-1 (resources/Resource.ts +7/-1 and one test). It is now 24 files, +1117/-1020, and range-diff shows zero commit correspondence — both of my approved commits were dropped and replaced. So that approval was stale in a way worth naming rather than silently carrying forward.
Verified this round, by reading the code at head:
-
The read gate fails closed on both paths. Search (
resources/Table.ts:2823-2831): a synchronously throwingallowReadis caught and converted toAccessViolation; a thenable one getsPromise.resolve(allowed).then(onFulfilled, () => ({ allowed: false })), so a rejection both fails closed and cannot surface as an unhandled rejection. Subscribe (:3645-3654):awaitinsidetry/catch, so rejection and throw land identically, andif (!allowed) throw new AccessViolation. -
The gate really does precede the work. On the async path the real
searchis only initialized inside the authorized branch, and insubscribethe check sits ahead of theauditStoreassertion andaddSubscription. Denied collection reads don't start a scan or audit setup. -
No bypass around the gated iterable. The async gate replaces
iterateon anExtendedIterable, so I checked the consumption surface in@harperfast/extended-iterable:[Symbol.asyncIterator],[Symbol.iterator],forEach,map, andasArrayall delegate tothis.iterate. Sync consumption of an async-gated result throws (Can not synchronously iterate while allowRead is asynchronous) rather than returning a truncated success — the right direction. -
Deleting
rejected-thenable-allowread-no-unhandled.test.tsis defensible, and I went looking for the property rather than assuming. The per-row thenableallowReadpath is exactly what this PR rolls back, and the guarantee survives in code:evaluateFilter(:3683-3695) attachesdecision.then(undefined, () => {})before throwingClientError, and bothallowReadpaths in (1) handle rejection.unitTests/resources/IterableEventQueue.test.js:77is a reasonable successor.One note on shape, not a request: the deleted test asserted a process-level absence of unhandled rejections; the replacement asserts terminal-error delivery at the unit level. On an authorization path an unhandled rejection can take a worker down, so the net is slightly narrower than it was. Worth knowing, given how cheap that integration test was to keep running.
-
The
rowFilter/tombstone asymmetry is deliberate and documented. ArowFilter-only subscriber never receives deletes: deletes are classified by a falsyvalue(:3737), sohasAuthoritativeRowis false andallowsEventreturnsBoolean(eventFilter)— i.e. false. That's the security-correct direction (delivering a tombstone for a row the subscriber was never allowed to see would leak its existence), andresources/DESIGN.md:97states the contract: rowFilter covers full-row events,eventFilterhandles tombstones. My only suggestion is that this deserves to reach the public docs, because a subscriber maintaining a materialized view withrowFilteralone will silently diverge on deletes and the failure is invisible.
What I did not cover, so this approval is scoped to the above: roughly two-thirds of the diff — the query-planning and HNSW predicate composition in Table.ts, the 216 changed lines in Resource.ts, the replay/snapshot and reload-resnapshot mechanics, and the REST QUERY checkPermission-stripping path described in DESIGN.md:99. I also could not execute the suites you cite: unitTests/mocha.init.js needs a built dist/, and building in my checkout fails on unrelated dependency drift (structon, busboy, @harperfast/rocksdb-js API mismatch) — an artifact of my environment, not of this PR. So I'm relying on your reported 1,269/3,799/73/9 runs for test evidence.
Net: I found no defect in the authorization semantics I examined, and the fail-closed behaviour is right in every path I traced. Approving on that basis, with the coverage boundary stated so it isn't mistaken for a full sign-off on a 1,100-line auth rewrite.
🤖 Reviewed by Claude Opus 5 via Claude Code
#1842's operationScopedWriteAuth test asserted a denying allowRead on the still-armed caller target yielded an empty set. Under operation-scoped read authorization the armed target is a one-shot admission gate, so the deny throws AccessViolation instead.
Summary
allowReadcontract instead of automatically invoking application overrides per row or subscription eventrowFilter(record, context)and subscriptioneventFilter(event, context)predicates for applications that intentionally need row-level narrowingloadAsInstance = falsesubscription gap with one collection-scopedallowReadcheck in the built-in table handler; custom false-mode handlers still own authorization unless they delegateQUERYbodyThis is a targeted semantic rollback of #1786 rather than a full code revert. Predicate-aware HNSW support remains, but application row filtering is now explicit and happens only after the operation override admits the request.
The narrow v5.1 backport remains #1914.
Compatibility model
The legacy
allowRead,allowUpdate,allowCreate, andallowDeletehooks remain one-time operation gates and are deprecated in favor of operation overrides (get,put,delete, and so on), which have the complete target and access to request context.loadAsInstance = falsetable handlers use one request/collection verdictrowFilter/eventFilterafter admission when row-level behavior is actually requiredThis maximizes compatibility with Harper 5.1 and HarperDB 4.7. Central Manager currently contains custom false-mode handlers with their own authorization; Host Manager does not use Resource/loadAsInstance handlers.
Security details
allowReadhooks fail closedcheckPermissionfields are removed recursively before relationship authorizationselectpreviousCountcounts accepted events and has an independent inspected-entry boundValidation
git diff --checkReview
Thorough cross-model review plus Harper auth-domain adjudication found no remaining code blockers. The review caught and drove fixes for Promise-backed/nested QUERY permission controls, full-target reauthorization, replay-after-close, audit-before-auth ordering, async gate timing and snapshot retention, transformed iterable authorization, GraphQL async-search consumption, terminal predicate failures, and single-record history counting.
Cross-model coverage: author Codex; independent Claude CLI ✓ / Gemini ✓ / final Harper-domain artifact review ✓.
Generated with GPT-5.6 Codex.