From ece8d1eb136e9e156a8907a26eb5b08f6614bddf Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 13 Jul 2026 22:48:35 -0600 Subject: [PATCH 01/12] Record-scoped allowRead: unified row-level read access control (#1422 gap 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- resources/DESIGN.md | 31 ++--- resources/Resource.ts | 23 ++++ resources/Table.ts | 97 +++++++++----- resources/search.ts | 13 +- unitTests/resources/vectorIndex.test.js | 166 ++++++++++++++++-------- 5 files changed, 226 insertions(+), 104 deletions(-) diff --git a/resources/DESIGN.md b/resources/DESIGN.md index b5a2380ae..40a2165c6 100644 --- a/resources/DESIGN.md +++ b/resources/DESIGN.md @@ -75,21 +75,22 @@ One giant `makeTable()` factory that returns a `TableResource extends Resource` ## "Where is X" cheat sheet -| Question | Where | -| --------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| How is a CRUD request authorized? | `Table.ts → #section: authz-hooks`; defaults in `Resource.ts` (`allowRead` etc.) | -| Where does versioning / conflict resolution happen? | `Table.ts → _writeUpdate` (`#section: write-path-internals`) | -| How does `search()` choose an index? | `Table.ts → search` (`#section: search-query`) | -| How are subscriptions replayed? | `Table.ts → subscribe` (`#section: pub-sub`) | -| How is the response body shaped (select clause)? | `Table.ts → transformEntryForSelect` (`#section: search-query`) | -| Where is record-level TTL evaluated? | `Table.ts → setTTLExpiration` (`#section: lifecycle-admin`); `Updatable.getExpiresAt` (`#section: setup-and-factory`) | -| How are residencies enforced (replication)? | `Table.ts → #section: lifecycle-admin` (residency block: `getResidencyRecord`, `setResidency`, `setResidencyById`, `getResidency`) | -| How is the RecordObject prototype applied? | `RecordEncoder.ts` (see `../DESIGN.md`) | -| Where is the per-request transaction stored? | `transaction.ts` + `contextStorage` (AsyncLocalStorage) | -| How does a query opt out of a read snapshot? | Pass `snapshot: false` on the search request (e.g. `get_analytics`). `Table.ts → search` calls `txn.useReadTxn(snapshot === false)`; on RocksDB `DatabaseTransaction.getReadTxn` then builds the read txn with `{ disableSnapshot: true }` so a long scan reads latest without pinning a snapshot. No-op on LMDB (`LMDBTransaction.useReadTxn`). | -| How does a URL path map to a Resource? | `Resources.ts → getMatch` (exact/prefix fast path) then `matchParamRoute` (parameterised routes); see "Path routing" below | -| How does HNSW keep the graph connected on delete? | `indexes/HierarchicalNavigableSmallWorld.ts → index()` delete path: zero-degree orphans reindexed via `needsReindexing`; severed multi-node islands detected and reconnected by `repairSeveredNeighbors` (#1712) | -| How is a filter applied _during_ a vector search? | Predicate-aware traversal (#1241): `search.ts → executeConditions` composes companion AND conditions + a request `vectorFilter` + a static `allowReadRecord` RBAC hook into one `(primaryKey) => boolean` (`composeRecordFilter`) and passes it to `HierarchicalNavigableSmallWorld.search(cond, ctx, filter)`. The filter gates result admission at layer 0 only (routing ignores it, ACORN-style); a visit budget (`filterExpansion`) bounds the under-filled/selective case. Very selective _condition_ filters are instead diverted to the exact brute-force path by the query planner's `estimateCountAsSort` ordering. | +| Question | Where | +| --------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| How is a CRUD request authorized? | `Table.ts → #section: authz-hooks`; defaults in `Resource.ts` (`allowRead` etc.) | +| Where does versioning / conflict resolution happen? | `Table.ts → _writeUpdate` (`#section: write-path-internals`) | +| How does `search()` choose an index? | `Table.ts → search` (`#section: search-query`) | +| How are subscriptions replayed? | `Table.ts → subscribe` (`#section: pub-sub`) | +| How is the response body shaped (select clause)? | `Table.ts → transformEntryForSelect` (`#section: search-query`) | +| Where is record-level TTL evaluated? | `Table.ts → setTTLExpiration` (`#section: lifecycle-admin`); `Updatable.getExpiresAt` (`#section: setup-and-factory`) | +| How are residencies enforced (replication)? | `Table.ts → #section: lifecycle-admin` (residency block: `getResidencyRecord`, `setResidency`, `setResidencyById`, `getResidency`) | +| How is the RecordObject prototype applied? | `RecordEncoder.ts` (see `../DESIGN.md`) | +| Where is the per-request transaction stored? | `transaction.ts` + `contextStorage` (AsyncLocalStorage) | +| How does a query opt out of a read snapshot? | Pass `snapshot: false` on the search request (e.g. `get_analytics`). `Table.ts → search` calls `txn.useReadTxn(snapshot === false)`; on RocksDB `DatabaseTransaction.getReadTxn` then builds the read txn with `{ disableSnapshot: true }` so a long scan reads latest without pinning a snapshot. No-op on LMDB (`LMDBTransaction.useReadTxn`). | +| How does a URL path map to a Resource? | `Resources.ts → getMatch` (exact/prefix fast path) then `matchParamRoute` (parameterised routes); see "Path routing" below | +| How does HNSW keep the graph connected on delete? | `indexes/HierarchicalNavigableSmallWorld.ts → index()` delete path: zero-degree orphans reindexed via `needsReindexing`; severed multi-node islands detected and reconnected by `repairSeveredNeighbors` (#1712) | +| How is a filter applied _during_ a vector search? | Predicate-aware traversal (#1241): `search.ts → executeConditions` composes companion AND conditions + a request `vectorFilter` + a record-scoped `allowRead` override into one `(primaryKey) => boolean` (`composeRecordFilter`) and passes it to `HierarchicalNavigableSmallWorld.search(cond, ctx, filter)`. The filter gates result admission at layer 0 only (routing ignores it, ACORN-style); a visit budget (`filterExpansion`) bounds the under-filled/selective case. Very selective _condition_ filters are instead diverted to the exact brute-force path by the query planner's `estimateCountAsSort` ordering. | +| How is row-level read access control enforced? | Unified `allowRead` (#1422 gap 2 / #1241): an application-OVERRIDDEN `allowRead` (detected via the `isDefaultAllowRead` marker on the framework defaults) is record-scoped — evaluated once per record with `this` = the (frozen) record during query execution, fail-closed on throw, dispatched via the resolved method (never `record.allowRead` lookup — data shadowing). The authorize wrapper (`Resource.ts → authorizeActionOnResource`) defers collection reads on tables (`supportsRowLevelAllowRead`) to this per-record path; single-record `get` keeps the entry check (record loaded, proxied reads work). Records also expose a non-enumerable `allowRead` delegate on the per-table `structPrototype`. | --- diff --git a/resources/Resource.ts b/resources/Resource.ts index 75a2a1216..f01e58946 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -491,6 +491,12 @@ export class Resource implements ResourceInterface< _assignPackageExport('Resource', Resource); +// Mark the built-in allowRead so the authorization flow can tell a framework default from an +// application override. An overridden allowRead on a table is evaluated per RECORD during query +// execution (#1422 gap 2 / #1241) — with `this` being each record — instead of once at collection +// entry where `this` has no record to inspect. Table.ts marks its table-level default the same way. +(Resource.prototype.allowRead as any).isDefaultAllowRead = true; + export function snakeCase(camelCase: string) { return ( camelCase[0].toLowerCase() + @@ -733,6 +739,23 @@ function transactional( } if (checkPermission) { if (loadAsInstance !== false) { + // Record-scoped allowRead on a table collection read (#1422 gap 2): an overridden + // allowRead is evaluated per record during query execution (`this` = each record), not + // once here where `this` is a collection resource with no record loaded — an entry + // verdict would be meaningless (spurious 403s or, worse, granting the whole scan). + // Leave query.checkPermission set; Table.search() converts it into per-record + // enforcement. Only tables opt in (supportsRowLevelAllowRead) — plain Resource + // subclasses have no row-level machinery and keep the entry check. + if ( + options.type === 'read' && + (query.id == null || query.isCollection) && + (resource.constructor as any)?.supportsRowLevelAllowRead && + !(resource.allowRead as any)?.isDefaultAllowRead + ) { + return when(data, (data) => { + return runAction(data); + }); + } // do permission checks, with allow methods let allowed; try { diff --git a/resources/Table.ts b/resources/Table.ts index 1cf56ca1f..b23131655 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -2640,17 +2640,25 @@ export function makeTable(options) { const txn = txnForContext(context); if (!target) throw new Error('No query provided'); if (target.parseError) throw target.parseError; // if there was a parse error, we can throw it now + // An application-overridden allowRead is a RECORD-scoped check (#1422 gap 2 / #1241): it is + // evaluated once per record during query execution with `this` = the record, instead of once + // at entry where `this` is a collection resource with nothing loaded. The framework defaults + // (marked isDefaultAllowRead) are this-free table/RBAC checks and keep the entry evaluation. + const recordScopedAllowRead = + target.checkPermission && !(this.allowRead as any)?.isDefaultAllowRead ? this.allowRead : undefined; if (target.checkPermission) { - // requesting authorization verification - let allowed; - try { - allowed = this.allowRead((context as any).user, target, context); - } catch { - // allow* threw — fail closed rather than letting the request proceed - throw new AccessViolation((context as any).user); - } - if (!allowed) { - throw new AccessViolation((context as any).user); + if (!recordScopedAllowRead) { + // requesting authorization verification + let allowed; + try { + allowed = this.allowRead((context as any).user, target, context); + } catch { + // allow* threw — fail closed rather than letting the request proceed + throw new AccessViolation((context as any).user); + } + if (!allowed) { + throw new AccessViolation((context as any).user); + } } } if (context) context.lastModified = UNCACHEABLE_TIMESTAMP; @@ -2854,16 +2862,30 @@ export function makeTable(options) { // scans), the read transaction reads against the latest committed data without pinning a // consistent snapshot, so the scan doesn't hold a snapshot that blocks compaction. const readTxn = txn.useReadTxn(target.snapshot === false); - // Record-level read guard (#1241): a resource may define a static allowReadRecord(user, record) - // that participates in query execution (pushed into HNSW traversal for vector sorts, applied as a - // post-filter otherwise). Resolve off the actual (possibly subclassed) constructor so overrides win. - // SCOPE: this is a QUERY-result filter, not a general read-authorization boundary — direct - // single-record get(id) does not consult it (use allowRead/instance hooks for that). It must be - // synchronous, side-effect free, and fast; it can run once per candidate record during traversal. - const allowReadRecord = (this.constructor as any).allowReadRecord; + // Record-level allowRead guard (#1241/#1422): an application-overridden allowRead runs once + // per record with `this` = the (frozen) record, participating in query execution — pushed + // into HNSW traversal for vector sorts, applied as a post-filter otherwise. It must be + // synchronous, side-effect free, and fast; a throw denies that record (fail closed, #1422 + // gap 1 parity). Dispatched via the method resolved from the resource — never via a + // `record.allowRead` property lookup, so a record attribute named allowRead can't shadow it. + let recordGuard: ((record: any) => boolean) | undefined; + if (recordScopedAllowRead) { + const user = (context as any)?.user; + recordGuard = (record: any) => { + let allowed; + try { + allowed = recordScopedAllowRead.call(record, user, target, context); + } catch { + return false; // fail closed on a throwing check + } + if (allowed?.then) + throw new ClientError('allowRead must be synchronous when evaluated per record in a query'); + return Boolean(allowed); + }; + } const recordAccess = - typeof allowReadRecord === 'function' || typeof target.vectorFilter === 'function' - ? { allowReadRecord, user: (context as any)?.user, vectorFilter: target.vectorFilter } + recordGuard || typeof target.vectorFilter === 'function' + ? { recordGuard, vectorFilter: target.vectorFilter } : undefined; const entries = executeConditions( conditions, @@ -2877,16 +2899,13 @@ export function makeTable(options) { recordAccess ); const ensure_loaded = (target as any).ensureLoaded !== false; - // Authoritative RBAC enforcement (#1241): the guards inside executeConditions evaluate the LOCAL - // record, but on a caching table transformEntryForSelect may then revalidate an expired/invalidated - // row from source and return a DIFFERENT record. An authorization check must hold on the record - // actually returned, so allowReadRecord is re-checked there, after materialization (the earlier - // evaluation stays as a prune that also bounds HNSW traversal). vectorFilter and condition filters - // intentionally keep the local-record semantics all query filters have on caching tables. - const recordGuard = - typeof allowReadRecord === 'function' - ? (record: any) => allowReadRecord((context as any)?.user, record) - : undefined; + // Authoritative enforcement point (#1241): the guards inside executeConditions evaluate the + // LOCAL record, but on a caching table transformEntryForSelect may then revalidate an + // expired/invalidated row from source and return a DIFFERENT record. An authorization check + // must hold on the record actually returned, so the record-scoped allowRead is re-checked + // there, after materialization (the earlier evaluation stays as a prune that also bounds HNSW + // traversal). vectorFilter and condition filters intentionally keep the local-record + // semantics all query filters have on caching tables. const transformToRecord = TableResource.transformEntryForSelect( select, context, @@ -4503,6 +4522,26 @@ export function makeTable(options) { } ); + // Mark the table-level allowRead as a framework default (it is a this-free table/RBAC check): + // only an APPLICATION override is record-scoped and evaluated per record during query execution + // (#1422 gap 2 / #1241). supportsRowLevelAllowRead tells the authorization wrapper in + // Resource.ts that this class has the per-record machinery to defer collection reads to. + (TableResource.prototype.allowRead as any).isDefaultAllowRead = true; + (TableResource as any).supportsRowLevelAllowRead = true; + // Records can be asked directly (`record.allowRead(user, target, context)`), delegating to the + // table's allowRead with `this` = the record. Non-enumerable so it never serializes; defined on + // the per-table record prototype alongside computed properties. Engine-level enforcement does + // NOT route through this (it resolves the active resource class's allowRead, which honors + // endpoint subclass overrides and can't be shadowed by a record attribute of the same name). + if (primaryStore?.encoder?.structPrototype) { + Object.defineProperty(primaryStore.encoder.structPrototype, 'allowRead', { + value: function (user: User, target: RequestTarget, context: Context) { + return TableResource.prototype.allowRead.call(this, user, target, context); + }, + configurable: true, + writable: true, + }); + } TableResource.updatedAttributes(); // on creation, update accessors as well if (expirationMs) TableResource.setTTLExpiration(expirationMs / 1000); if (expiresAtProperty) runRecordExpirationEviction(); diff --git a/resources/search.ts b/resources/search.ts index 5a062c9a8..cbf2ef78e 100644 --- a/resources/search.ts +++ b/resources/search.ts @@ -56,7 +56,7 @@ export function executeConditions( recordAccess? ) { const firstSearch = conditions[0]; - // Record-level guards (a caller-supplied vectorFilter + RBAC allowReadRecord) apply to every record + // Record-level guards (a caller-supplied vectorFilter + a record-scoped allowRead) apply to every record // the query returns, independent of which condition leads (#1241). `recordAccess` is supplied only on // the top-level executeConditions call (Table.search) and deliberately NOT threaded into the recursive // calls below, so the guards run exactly once — on the final result set — rather than redundantly at @@ -161,9 +161,10 @@ export function executeConditions( } /** - * Build the record-level guards that apply to a query independent of its conditions (#1241): + * Build the record-level guards that apply to a query independent of its conditions (#1241/#1422): * - `vectorFilter`: a caller-supplied `(record) => boolean` predicate (JS-API only). - * - `allowReadRecord`: a resource's static record-level RBAC check `(user, record) => boolean`. + * - `recordGuard`: the record-scoped allowRead check, pre-bound (fail-closed) by Table.search — + * invokes the resource class's overridden allowRead with `this` = each record. * Both arrive already resolved on `recordAccess` (assembled once in Table.search). Returns an array of * `(record) => boolean` predicates, or undefined when neither is defined (the common case — zero * overhead). Predicates must be synchronous and side-effect free; records passed in are frozen. @@ -173,11 +174,7 @@ function buildRecordGuards(recordAccess): ((record: any) => boolean)[] | undefin const guards: ((record: any) => boolean)[] = []; const vectorFilter = recordAccess.vectorFilter; if (typeof vectorFilter === 'function') guards.push((record) => vectorFilter(record)); - const allowReadRecord = recordAccess.allowReadRecord; - if (typeof allowReadRecord === 'function') { - const user = recordAccess.user; - guards.push((record) => allowReadRecord(user, record)); - } + if (recordAccess.recordGuard) guards.push(recordAccess.recordGuard); return guards.length > 0 ? guards : undefined; } diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 527e84285..f2060fe31 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1481,34 +1481,77 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => ); }); - it('allowReadRecord restricts a vector search to records the user may see', async () => { - T.allowReadRecord = (user, record) => record.ownerId === user.id; - try { - const results = await fromAsync( - T.search( - { - sort: { attribute: 'vector', target: [0, 0], distance: 'euclidean' }, - select: ['id', 'ownerId'], - limit: 5, - }, - { user: { id: 1 } } - ) - ); - assert( - results.every((r) => r.ownerId === 1), - 'a restricted user only sees their own records' - ); - // ownerId === 1 for ids 1,4,7,10,13,… — the k nearest VISIBLE, not k-minus-redacted. - assert.deepStrictEqual( - results.map((r) => r.id), - [1, 4, 7, 10, 13] - ); - } finally { - delete T.allowReadRecord; + it('record-scoped allowRead override restricts a vector search to records the user may see', async () => { + // Unified model (#1422/#1241): overriding allowRead makes it a record-scoped check — evaluated + // once per record with `this` = the record during query execution, including HNSW traversal. + class Restricted extends T { + allowRead(user) { + return this.ownerId === user.id; + } } + const results = await fromAsync( + Restricted.search( + { + sort: { attribute: 'vector', target: [0, 0], distance: 'euclidean' }, + select: ['id', 'ownerId'], + limit: 5, + checkPermission: true, + }, + { user: { id: 1 } } + ) + ); + assert( + results.every((r) => r.ownerId === 1), + 'a restricted user only sees their own records' + ); + // ownerId === 1 for ids 1,4,7,10,13,… — the k nearest VISIBLE, not k-minus-redacted. + assert.deepStrictEqual( + results.map((r) => r.id), + [1, 4, 7, 10, 13] + ); }); - it('re-checks allowReadRecord on the source-revalidated record, not the stale cached copy', async () => { + it('non-overridden allowRead keeps the entry check (no per-record deferral)', async () => { + // The framework default is a this-free table/RBAC check marked isDefaultAllowRead; with + // checkPermission and a non-super user it must still deny at entry, not defer to per-record. + await assert.rejects( + async () => + fromAsync( + T.search( + { + sort: { attribute: 'vector', target: [0, 0], distance: 'euclidean' }, + limit: 5, + checkPermission: true, + }, + { user: { id: 1, role: { permission: {} } } } + ) + ), + (err) => /unauthorized/i.test(err.message) || err.statusCode === 403 + ); + }); + + it('a throwing record-scoped allowRead fails closed (denies that record)', async () => { + class Throwy extends T { + allowRead(user) { + if (this.ownerId === 0) throw new Error('boom'); // ownerId 0 rows must be DENIED, not leaked + return this.ownerId === user.id; + } + } + const results = await fromAsync( + Throwy.search( + { + sort: { attribute: 'vector', target: [0, 0], distance: 'euclidean' }, + select: ['id', 'ownerId'], + limit: 5, + checkPermission: true, + }, + { user: { id: 0 } } + ) + ); + assert.strictEqual(results.length, 0, 'every candidate either threw (denied) or belonged to another user'); + }); + + it('re-checks a record-scoped allowRead on the source-revalidated record, not the stale cached copy', async () => { // A caching table: the query filters evaluate the LOCAL (possibly stale) copy, but the // returned record may be revalidated from source. The authorization verdict must hold on // the record actually returned (transformEntryForSelect recordGuard), or ownership changes @@ -1524,7 +1567,11 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => return sourceRecords.get(id); }, }); - C.allowReadRecord = (user, record) => record.ownerId === user.id; + class RestrictedC extends C { + allowRead(user) { + return this.ownerId === user.id; + } + } try { sourceRecords.set(1, { kind: 'doc', ownerId: 1 }); C.setTTLExpiration(0.01); // 10ms — expiry retains the stale value (unlike invalidate), which is the vulnerable path @@ -1535,40 +1582,55 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => sourceRecords.set(1, { kind: 'doc', ownerId: 2 }); await new Promise((resolve) => setTimeout(resolve, 20)); const results = await fromAsync( - C.search({ conditions: [{ attribute: 'kind', value: 'doc' }] }, { user: { id: 1 } }) + RestrictedC.search( + { conditions: [{ attribute: 'kind', value: 'doc' }], checkPermission: true }, + { user: { id: 1 } } + ) ); assert.strictEqual(results.length, 0, 'a record now owned by another user must not be returned'); } finally { - delete C.allowReadRecord; C.dropTable(); } }); - it('enforces allowReadRecord on OR queries (no RBAC bypass)', async () => { - T.allowReadRecord = (user, record) => record.ownerId === user.id; - try { - // active=true OR active=false spans every record; the record guard must still filter the union. - const results = await fromAsync( - T.search( - { - conditions: [ - { attribute: 'active', comparator: 'equals', value: true }, - { attribute: 'active', comparator: 'equals', value: false }, - ], - operator: 'or', - select: ['id', 'ownerId'], - }, - { user: { id: 2 } } - ) - ); - assert(results.length > 0, 'expected some visible records'); - assert( - results.every((r) => r.ownerId === 2), - 'an OR union must still be filtered by allowReadRecord' - ); - } finally { - delete T.allowReadRecord; + it('enforces a record-scoped allowRead on OR queries (no RBAC bypass)', async () => { + class Restricted extends T { + allowRead(user) { + return this.ownerId === user.id; + } } + // active=true OR active=false spans every record; the record guard must still filter the union. + const results = await fromAsync( + Restricted.search( + { + conditions: [ + { attribute: 'active', comparator: 'equals', value: true }, + { attribute: 'active', comparator: 'equals', value: false }, + ], + operator: 'or', + select: ['id', 'ownerId'], + checkPermission: true, + }, + { user: { id: 2 } } + ) + ); + assert(results.length > 0, 'expected some visible records'); + assert( + results.every((r) => r.ownerId === 2), + 'an OR union must still be filtered by the record-scoped allowRead' + ); + }); + + it('records expose allowRead directly (RecordObject delegate, non-enumerable)', async () => { + const results = await fromAsync( + T.search({ conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], limit: 1 }, {}) + ); + const record = results[0]; + assert.strictEqual(typeof record.allowRead, 'function', 'records carry an allowRead delegate'); + // Delegates to the table default (this-free RBAC check): super user allowed, plain user not. + assert(record.allowRead({ role: { permission: { super_user: true } } })); + assert(!record.allowRead({ role: { permission: {} } })); + assert(!JSON.stringify(record).includes('allowRead'), 'delegate must not serialize'); }); it('honors a companion AND condition during the vector query', async () => { From 1491421af905074356f155e3113f824203c2cb09 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 13 Jul 2026 23:01:43 -0600 Subject: [PATCH 02/12] review: exclude subscribe/connect from per-record deferral (would grant 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 --- resources/Resource.ts | 9 +++++++-- unitTests/resources/vectorIndex.test.js | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/resources/Resource.ts b/resources/Resource.ts index f01e58946..a68e89721 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -744,10 +744,15 @@ function transactional( // once here where `this` is a collection resource with no record loaded — an entry // verdict would be meaningless (spurious 403s or, worse, granting the whole scan). // Leave query.checkPermission set; Table.search() converts it into per-record - // enforcement. Only tables opt in (supportsRowLevelAllowRead) — plain Resource - // subclasses have no row-level machinery and keep the entry check. + // enforcement (get/query on a collection route into search, which consumes it). Only + // tables opt in (supportsRowLevelAllowRead) — plain Resource subclasses have no + // row-level machinery and keep the entry check. Subscriptions are excluded: their + // delivery path never reaches search(), so deferring would grant them UNCHECKED — + // they keep the entry evaluation (which fails closed for a record-scoped override) + // until per-event delivery checks exist (#1419). if ( options.type === 'read' && + !isSubscribeAction && (query.id == null || query.isCollection) && (resource.constructor as any)?.supportsRowLevelAllowRead && !(resource.allowRead as any)?.isDefaultAllowRead diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index f2060fe31..d8b151cd3 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1621,6 +1621,26 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => ); }); + it('subscribe with a record-scoped allowRead is denied at entry, never granted unchecked', async () => { + // Subscription delivery never reaches search(), so the per-record deferral must NOT apply to + // subscribe — otherwise a collection subscription would be granted with no check at all. + // Until per-event delivery checks exist (#1419), the entry evaluation runs the override with + // `this` = collection resource (fields undefined) and fails closed. + class Restricted extends T { + allowRead(user) { + return this.ownerId === user.id; + } + } + await assert.rejects( + async () => + Restricted.subscribe( + { checkPermission: true, omitCurrent: true }, + { user: { id: 1, username: 'u1', role: { permission: {} } } } + ), + (err) => err.name === 'AccessViolation' || /unauthorized/i.test(err.message) || err.statusCode === 403 + ); + }); + it('records expose allowRead directly (RecordObject delegate, non-enumerable)', async () => { const results = await fromAsync( T.search({ conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], limit: 1 }, {}) From d486ec0a5cd244af463654c82793341961e429a0 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 13 Jul 2026 23:11:14 -0600 Subject: [PATCH 03/12] review: compose attribute_permissions narrowing with overrides; tighten get deferral gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- resources/Resource.ts | 9 +++-- resources/Table.ts | 13 ++++++- unitTests/resources/vectorIndex.test.js | 48 +++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/resources/Resource.ts b/resources/Resource.ts index a68e89721..21a4e1d6b 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -749,11 +749,16 @@ function transactional( // row-level machinery and keep the entry check. Subscriptions are excluded: their // delivery path never reaches search(), so deferring would grant them UNCHECKED — // they keep the entry evaluation (which fails closed for a record-scoped override) - // until per-event delivery checks exist (#1419). + // until per-event delivery checks exist (#1419). The collection predicate must be at + // least as strict as the routing it relies on: instance get() only reaches search() + // via isSearchTarget (isCollection), so `get` defers on isCollection alone — a + // hypothetical {id: null, isCollection: false} get keeps the entry check instead of + // skipping both checks. search/query invoke instance search() directly (which + // consumes checkPermission), so id == null suffices there. if ( options.type === 'read' && !isSubscribeAction && - (query.id == null || query.isCollection) && + (query.isCollection || (query.id == null && options.method !== 'get')) && (resource.constructor as any)?.supportsRowLevelAllowRead && !(resource.allowRead as any)?.isDefaultAllowRead ) { diff --git a/resources/Table.ts b/resources/Table.ts index b23131655..2da572de3 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -2647,7 +2647,18 @@ export function makeTable(options) { const recordScopedAllowRead = target.checkPermission && !(this.allowRead as any)?.isDefaultAllowRead ? this.allowRead : undefined; if (target.checkPermission) { - if (!recordScopedAllowRead) { + if (recordScopedAllowRead) { + // Compose with role-level column RBAC: the DEFAULT table allowRead is also the + // enforcement point for attribute_permissions (it narrows target.select). Run it here + // for that side effect — before `select` is captured below — with its verdict + // superseded by the per-record override. A per-record super.allowRead call could NOT + // restore this: the select-driven output transform is built before iteration starts. + try { + TableResource.prototype.allowRead.call(this, (context as any).user, target, context); + } catch { + // narrowing is best-effort under an override; access is governed per record + } + } else { // requesting authorization verification let allowed; try { diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index d8b151cd3..ffdd0e7f0 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1621,6 +1621,54 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => ); }); + it('composes attribute_permissions column narrowing with a record-scoped allowRead override', async () => { + // Role-level column RBAC (attribute_permissions) is enforced by the DEFAULT table allowRead + // narrowing target.select at entry. An overridden (record-scoped) allowRead must not void it: + // search() runs the default for its narrowing side effect before deferring row access to the + // per-record override. + class Restricted extends T { + allowRead(user) { + return this.ownerId === user.id; + } + } + const user = { + id: 1, + role: { + permission: { + test: { + tables: { + HNSWFilter: { + read: true, + attribute_permissions: [ + { attribute_name: 'id', read: true }, + { attribute_name: 'ownerId', read: true }, + { attribute_name: 'group', read: false }, // column denied + ], + }, + }, + }, + }, + }, + }; + const results = await fromAsync( + Restricted.search( + { + sort: { attribute: 'vector', target: [0, 0], distance: 'euclidean' }, + select: ['id', 'group', 'ownerId'], + limit: 3, + checkPermission: true, + }, + { user } + ) + ); + assert(results.length > 0, 'row-filtered results expected'); + for (const r of results) { + assert.strictEqual(r.ownerId, 1, 'row-level override still applies'); + assert.notStrictEqual(r.id, undefined, 'permitted column returned'); + assert.strictEqual(r.group, undefined, 'column denied by attribute_permissions must be stripped'); + } + }); + it('subscribe with a record-scoped allowRead is denied at entry, never granted unchecked', async () => { // Subscription delivery never reaches search(), so the per-record deferral must NOT apply to // subscribe — otherwise a collection subscription would be granted with no check at all. From c0ef526cee65ee3e8c3bef01f711a2634382d073 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 13 Jul 2026 23:23:39 -0600 Subject: [PATCH 04/12] =?UTF-8?q?fix:=20record-scoping=20is=20sync-only=20?= =?UTF-8?q?=E2=80=94=20async=20allowRead=20overrides=20keep=20the=20awaite?= =?UTF-8?q?d=20entry=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- resources/Resource.ts | 6 +++- resources/Table.ts | 24 +++++++++++++-- unitTests/resources/vectorIndex.test.js | 41 +++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/resources/Resource.ts b/resources/Resource.ts index 21a4e1d6b..57e7aea0c 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -755,12 +755,16 @@ function transactional( // hypothetical {id: null, isCollection: false} get keeps the entry check instead of // skipping both checks. search/query invoke instance search() directly (which // consumes checkPermission), so id == null suffices there. + // Record-scoping is SYNC-only: an async allowRead override keeps this entry check, + // which awaits its verdict and fails closed on rejection (#1422 gap 1) — the sync + // per-record traversal path cannot await it. if ( options.type === 'read' && !isSubscribeAction && (query.isCollection || (query.id == null && options.method !== 'get')) && (resource.constructor as any)?.supportsRowLevelAllowRead && - !(resource.allowRead as any)?.isDefaultAllowRead + !(resource.allowRead as any)?.isDefaultAllowRead && + (resource.allowRead as any)?.constructor?.name !== 'AsyncFunction' ) { return when(data, (data) => { return runAction(data); diff --git a/resources/Table.ts b/resources/Table.ts index 2da572de3..c96785888 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -2644,8 +2644,14 @@ export function makeTable(options) { // evaluated once per record during query execution with `this` = the record, instead of once // at entry where `this` is a collection resource with nothing loaded. The framework defaults // (marked isDefaultAllowRead) are this-free table/RBAC checks and keep the entry evaluation. + // Record-scoping is SYNC-only: async-declared overrides keep entry-check semantics (the + // authorize wrapper awaits them and fails closed) rather than entering the sync traversal. const recordScopedAllowRead = - target.checkPermission && !(this.allowRead as any)?.isDefaultAllowRead ? this.allowRead : undefined; + target.checkPermission && + !(this.allowRead as any)?.isDefaultAllowRead && + (this.allowRead as any)?.constructor?.name !== 'AsyncFunction' + ? this.allowRead + : undefined; if (target.checkPermission) { if (recordScopedAllowRead) { // Compose with role-level column RBAC: the DEFAULT table allowRead is also the @@ -2882,6 +2888,7 @@ export function makeTable(options) { let recordGuard: ((record: any) => boolean) | undefined; if (recordScopedAllowRead) { const user = (context as any)?.user; + let warnedAsync = false; recordGuard = (record: any) => { let allowed; try { @@ -2889,8 +2896,19 @@ export function makeTable(options) { } catch { return false; // fail closed on a throwing check } - if (allowed?.then) - throw new ClientError('allowRead must be synchronous when evaluated per record in a query'); + if (typeof allowed?.then === 'function') { + // A sync-declared override that returns a thenable can't be awaited mid-traversal: + // deny the record (fail closed, #1422 gap 1 semantics) rather than fail open on + // promise truthiness or abort the whole query. Declared-async overrides never get + // here — they keep the awaited entry check. + if (!warnedAsync) { + warnedAsync = true; + logger.warn?.( + `allowRead on ${tableName} returned a promise during per-record evaluation; records are denied (record-scoped allowRead must be synchronous)` + ); + } + return false; + } return Boolean(allowed); }; } diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index ffdd0e7f0..5152eeafc 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1621,6 +1621,47 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => ); }); + it('async allowRead override keeps entry-check semantics (awaited, fail-closed), not record scoping', async () => { + // Record-scoping is sync-only: a declared-async override is excluded from the per-record + // deferral, so the authorize wrapper awaits its verdict at entry and fails closed on + // rejection (#1422 gap 1 contract, locked by integrationTests allowread-fail-closed). + class AsyncRejects extends T { + async allowRead() { + throw new Error('async allowRead intentionally rejects'); + } + } + await assert.rejects( + async () => { + // the authorize wrapper awaits the async verdict, so search() returns a promise here + const iterable = await AsyncRejects.search( + { conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], checkPermission: true }, + { user: { id: 1, role: { permission: {} } } } + ); + return fromAsync(iterable); + }, + (err) => err.name === 'AccessViolation' || err.statusCode === 403 + ); + }); + + it('sync allowRead override returning a promise denies records (fail closed), not fail-open truthiness', async () => { + class SneakyAsync extends T { + allowRead(user) { + return Promise.resolve(this.ownerId === user.id); // sync-declared, returns a thenable + } + } + const results = await fromAsync( + SneakyAsync.search( + { + conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], + select: ['id'], + checkPermission: true, + }, + { user: { id: 1 } } + ) + ); + assert.strictEqual(results.length, 0, 'a thenable verdict must deny (fail closed), never grant on truthiness'); + }); + it('composes attribute_permissions column narrowing with a record-scoped allowRead override', async () => { // Role-level column RBAC (attribute_permissions) is enforced by the DEFAULT table allowRead // narrowing target.select at entry. An overridden (record-scoped) allowRead must not void it: From fa19aaa413d69ee78bbe4a26414abc2e4d985a70 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 13 Jul 2026 23:53:04 -0600 Subject: [PATCH 05/12] review: defer per-record enforcement for id-seeded search/query (prefix scans are multi-record) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- resources/Resource.ts | 16 +++++++------ unitTests/resources/vectorIndex.test.js | 32 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/resources/Resource.ts b/resources/Resource.ts index 57e7aea0c..b9e6987d8 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -749,19 +749,21 @@ function transactional( // row-level machinery and keep the entry check. Subscriptions are excluded: their // delivery path never reaches search(), so deferring would grant them UNCHECKED — // they keep the entry evaluation (which fails closed for a record-scoped override) - // until per-event delivery checks exist (#1419). The collection predicate must be at - // least as strict as the routing it relies on: instance get() only reaches search() - // via isSearchTarget (isCollection), so `get` defers on isCollection alone — a - // hypothetical {id: null, isCollection: false} get keeps the entry check instead of - // skipping both checks. search/query invoke instance search() directly (which - // consumes checkPermission), so id == null suffices there. + // until per-event delivery checks exist (#1419). Method semantics decide the deferral: + // `get` with a non-null id is a true single-record read (entry check runs with the + // record loaded, so record-scoped field reads resolve) and only defers when + // isCollection — mirroring the isSearchTarget routing it relies on. ALL search/query + // calls defer: a present id there is a starts_with/prefix SEED (a multi-record scan, + // see Table.search), so an entry verdict would gate — or worse, grant — the whole + // scan; both methods invoke instance search() directly, which consumes + // checkPermission and arms the per-record guard. // Record-scoping is SYNC-only: an async allowRead override keeps this entry check, // which awaits its verdict and fails closed on rejection (#1422 gap 1) — the sync // per-record traversal path cannot await it. if ( options.type === 'read' && !isSubscribeAction && - (query.isCollection || (query.id == null && options.method !== 'get')) && + (options.method !== 'get' || query.isCollection) && (resource.constructor as any)?.supportsRowLevelAllowRead && !(resource.allowRead as any)?.isDefaultAllowRead && (resource.allowRead as any)?.constructor?.name !== 'AsyncFunction' diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 5152eeafc..8310d19ec 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1621,6 +1621,38 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => ); }); + it('id-prefix search with a record-scoped override still filters per record (no entry-verdict gating)', async () => { + // A present `id` on search/query is a starts_with/prefix SEED (multi-record scan), not a + // single record — so it must defer to per-record enforcement like any collection scan. An + // entry verdict here would evaluate the override against a bare resource (fields undefined), + // spuriously denying the scan or, for a permissive-default override, leaking every row. + const P = table({ + table: 'PrefixAuth', + database: 'test', + attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'ownerId' }], + }); + class Restricted extends P { + allowRead(user) { + return this.ownerId === user.id; + } + } + try { + await P.put('doc-1', { ownerId: 1 }); + await P.put('doc-2', { ownerId: 2 }); + await P.put('doc-3', { ownerId: 1 }); + const results = await fromAsync( + Restricted.search({ id: 'doc-', checkPermission: true, select: ['id', 'ownerId'] }, { user: { id: 1 } }) + ); + assert.deepStrictEqual( + results.map((r) => r.id).sort(), + ['doc-1', 'doc-3'], + 'prefix scan returns only records the user may see' + ); + } finally { + P.dropTable(); + } + }); + it('async allowRead override keeps entry-check semantics (awaited, fail-closed), not record scoping', async () => { // Record-scoping is sync-only: a declared-async override is excluded from the per-record // deferral, so the authorize wrapper awaits its verdict at entry and fails closed on From b495c2bbf84275babf76b865717c22fb189b7a17 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 14 Jul 2026 05:37:23 -0600 Subject: [PATCH 06/12] Record-scoped allowRead on subscription delivery (#1419, revives #1524) + GraphQL checkPermission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../subscription-row-allowread/config.yaml | 6 + .../subscription-row-allowread/resources.js | 36 +++ .../subscription-row-allowread/schema.graphql | 11 + .../subscription-row-allowread.test.ts | 259 ++++++++++++++++++ resources/Resource.ts | 44 ++- resources/Table.ts | 57 +++- server/graphqlQuerying.ts | 6 +- unitTests/resources/vectorIndex.test.js | 59 +++- 8 files changed, 452 insertions(+), 26 deletions(-) create mode 100644 integrationTests/security/fixtures/subscription-row-allowread/config.yaml create mode 100644 integrationTests/security/fixtures/subscription-row-allowread/resources.js create mode 100644 integrationTests/security/fixtures/subscription-row-allowread/schema.graphql create mode 100644 integrationTests/security/subscription-row-allowread.test.ts diff --git a/integrationTests/security/fixtures/subscription-row-allowread/config.yaml b/integrationTests/security/fixtures/subscription-row-allowread/config.yaml new file mode 100644 index 000000000..67b257646 --- /dev/null +++ b/integrationTests/security/fixtures/subscription-row-allowread/config.yaml @@ -0,0 +1,6 @@ +# Regression fixture for #1419 — per-row allowRead must filter subscription delivery. +graphqlSchema: + files: '*.graphql' +jsResource: + files: resources.js +rest: true diff --git a/integrationTests/security/fixtures/subscription-row-allowread/resources.js b/integrationTests/security/fixtures/subscription-row-allowread/resources.js new file mode 100644 index 000000000..95b1b5f43 --- /dev/null +++ b/integrationTests/security/fixtures/subscription-row-allowread/resources.js @@ -0,0 +1,36 @@ +// Regression fixture for #1419 — per-row allowRead must filter subscription delivery. +// +// allowRead is designed so that: +// - When `this` is a loaded record (a specific row), allow only if the row's owner +// matches the requesting user. +// - When `this` is the collection (no specific record loaded, this.owner undefined/null), +// allow the subscription to open. This is the permissive side that lets a non-owner open +// a whole-table subscription — the scenario where the pre-#1419 delivery loop leaked every +// row's events because it never re-evaluated allowRead per record. +// +// Super users always pass so that seed writes and setup ops succeed. + +function isSuper(user) { + return !!user?.role?.permission?.super_user; +} + +export class Vault extends tables.Vault { + allowRead(user, _target, _context) { + if (isSuper(user)) return true; + const owner = this?.owner; + // Collection subscribe / no loaded record: allow the connection to open. + if (owner === undefined || owner === null) return true; + // Per-row: only the owner can read this specific record. + return owner === user?.username; + } + + allowUpdate(user, _record, _context) { + return isSuper(user); + } + allowCreate(user, _record, _context) { + return isSuper(user); + } + allowDelete(user, _target, _context) { + return isSuper(user); + } +} diff --git a/integrationTests/security/fixtures/subscription-row-allowread/schema.graphql b/integrationTests/security/fixtures/subscription-row-allowread/schema.graphql new file mode 100644 index 000000000..03da5b3e9 --- /dev/null +++ b/integrationTests/security/fixtures/subscription-row-allowread/schema.graphql @@ -0,0 +1,11 @@ +# Regression fixture for #1419 — per-row allowRead must filter subscription delivery. +# +# Vault rows are owned by a specific user (`owner` field). The allowRead override in +# resources.js permits a read only when the record's owner matches the requesting user, +# while still allowing the collection-level subscription to open. The test verifies that a +# whole-table SSE subscription no longer leaks other owners' row events. +type Vault @table @export { + id: ID @primaryKey + owner: String + secret: String +} diff --git a/integrationTests/security/subscription-row-allowread.test.ts b/integrationTests/security/subscription-row-allowread.test.ts new file mode 100644 index 000000000..cccd691c1 --- /dev/null +++ b/integrationTests/security/subscription-row-allowread.test.ts @@ -0,0 +1,259 @@ +/** + * #1419 — Row-level `allowRead` must filter subscription delivery. + * + * Before the fix, `Table.subscribe` evaluated `allowRead` once at connect time with `this` + * bound to the collection (no loaded record), so a row-level override always passed and every + * row's change events were delivered to any collection subscriber. The fix re-evaluates + * `allowRead` per record-bearing event, binding a resource instance to that row's record. + * + * Fixture: a `Vault` table whose `allowRead` allows the collection subscription to open but + * permits per-row reads only for the row's owner (see fixtures/subscription-row-allowread). + * Alice and Bob are non-admin users owning different rows. + * + * CONTROL — Alice's REST GET of Bob's row is denied (proves allowRead is real and the + * AUTHENTICATION_AUTHORIZELOCAL loopback escape is not in play — real bearer auth). + * PROBE — Alice opens a whole-table `/Vault/` SSE subscription. Writes land on Alice's + * rows, Bob's rows, and a neutral row. Alice's stream must receive ONLY her own + * rows' events; Bob's and the neutral row's events must be filtered out. + * + * Reproduction: + * npm run test:integration -- "integrationTests/security/subscription-row-allowread.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { ok } from 'node:assert'; +import { resolve } from 'node:path'; +import { setTimeout as sleep } from 'node:timers/promises'; +import http from 'node:http'; +import https from 'node:https'; +import { URL } from 'node:url'; + +import request from 'supertest'; + +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +// @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine +import { createApiClient, createHeaders } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/subscription-row-allowread'); +const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; + +const ALICE = { username: 'sub_allowread_alice', password: 'Alice-pw-1419!' }; +const BOB = { username: 'sub_allowread_bob', password: 'Bobby-pw-1419!' }; +const ROLE = 'sub_allowread_role'; + +const ALICE_ROWS = ['row-a1', 'row-a2', 'row-a3']; +const BOB_ROWS = ['row-b1', 'row-b2', 'row-b3']; +const NEUTRAL_ROW = 'row-neutral'; + +// ---------------------------------------------------------------- SSE helpers -- +interface SseEvent { + data?: string; + id?: string; +} +interface SseStream { + events: SseEvent[]; + status: number; + destroy: () => void; +} + +function openSse(urlStr: string, headers: Record): Promise { + const url = new URL(urlStr); + const lib = url.protocol === 'https:' ? https : http; + const events: SseEvent[] = []; + let buffer = ''; + const parseFrame = (frame: string) => { + if (!frame.trim()) return; + const ev: SseEvent = {}; + for (const line of frame.split('\n')) { + const idx = line.indexOf(':'); + if (idx < 0) continue; + const field = line.slice(0, idx); + const val = line.slice(idx + 1).replace(/^ /, ''); + if (field === 'data') ev.data = (ev.data ? ev.data + '\n' : '') + val; + else if (field === 'id') ev.id = val; + } + events.push(ev); + }; + return new Promise((resolvePromise, reject) => { + const req = lib.request( + url, + { method: 'GET', headers: { ...headers, Accept: 'text/event-stream' }, rejectUnauthorized: false } as any, + (res) => { + const stream: SseStream = { + events, + status: res.statusCode ?? 0, + destroy: () => { + res.destroy(); + req.destroy(); + }, + }; + res.setEncoding('utf8'); + res.on('data', (chunk: string) => { + buffer += chunk; + let sep: number; + while ((sep = buffer.indexOf('\n\n')) >= 0) { + parseFrame(buffer.slice(0, sep)); + buffer = buffer.slice(sep + 2); + } + }); + res.on('error', () => {}); + resolvePromise(stream); + } + ); + req.on('error', reject); + req.end(); + }); +} + +function streamMentions(stream: SseStream, text: string): boolean { + return stream.events.some((e) => (e.data ?? '').includes(text)); +} + +async function waitFor(predicate: () => boolean, timeoutMs = 8000, intervalMs = 50): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (predicate()) return true; + await sleep(intervalMs); + } + return predicate(); +} + +suite('#1419 row-level allowRead filters subscription delivery', { skip: skipSuite }, (ctx: ContextWithHarper) => { + let client: ReturnType; + let restURL = ''; + let aliceBearer = ''; + + const openStreams = new Set(); + + const adminPut = (id: string, body: object) => request(restURL).put(`/Vault/${id}`).set(client.headers).send(body); + + before(async () => { + await setupHarperWithFixture(ctx, FIXTURE_PATH, { config: {}, env: {} }); + client = createApiClient(ctx.harper); + restURL = ctx.harper.httpURL; + + // Poll until the Vault route is ready. + const deadline = Date.now() + 30_000; + while (Date.now() < deadline) { + try { + const probe = await client.reqRest('/Vault/').timeout(3000); + if (probe.status !== 404) break; + } catch { + /* not ready */ + } + await sleep(250); + } + + // Non-super role with full table-level READ/WRITE on Vault → the allowRead override is the + // only remaining read gate. + await client + .req() + .send({ + operation: 'add_role', + role: ROLE, + permission: { + super_user: false, + data: { + tables: { + Vault: { read: true, insert: true, update: true, delete: true, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + + for (const u of [ALICE, BOB]) { + await client + .req() + .send({ operation: 'add_user', role: ROLE, username: u.username, password: u.password, active: true }) + .expect(200); + } + + // Seed rows via the ops API (super, bypasses allowCreate). + const records = [ + ...ALICE_ROWS.map((id) => ({ id, owner: ALICE.username, secret: `alice-secret-${id}` })), + ...BOB_ROWS.map((id) => ({ id, owner: BOB.username, secret: `bob-secret-${id}` })), + { id: NEUTRAL_ROW, owner: 'nobody', secret: 'neutral-secret' }, + ]; + await client.req().send({ operation: 'insert', schema: 'data', table: 'Vault', records }).expect(200); + + // Obtain Alice's bearer token so the SSE Authorization header uses real auth (not the + // header-less AUTHORIZELOCAL loopback escape). + const tokenResp = await client + .req() + .send({ operation: 'create_authentication_tokens', username: ALICE.username, password: ALICE.password }); + aliceBearer = + tokenResp.status === 200 && tokenResp.body?.operation_token + ? `Bearer ${tokenResp.body.operation_token}` + : createHeaders(ALICE.username, ALICE.password).Authorization; + }); + + after(async () => { + for (const s of openStreams) { + try { + s.destroy(); + } catch { + /* ignore */ + } + } + openStreams.clear(); + await teardownHarper(ctx); + }); + + test('CONTROL: row-level allowRead is enforced on REST (Alice denied Bob row, allowed own)', async () => { + const ownGet = await request(restURL).get(`/Vault/${ALICE_ROWS[0]}`).set({ Authorization: aliceBearer }); + ok(ownGet.status === 200, `Alice must read her own row, got ${ownGet.status}: ${ownGet.text}`); + + const bobGet = await request(restURL).get(`/Vault/${BOB_ROWS[0]}`).set({ Authorization: aliceBearer }); + ok([401, 403, 404].includes(bobGet.status), `Alice GET Bob row should be denied, got ${bobGet.status}`); + + // Wrong password must not auto-authorize (would indicate an AUTHORIZELOCAL escape). + const badGet = await request(restURL) + .get(`/Vault/${ALICE_ROWS[0]}`) + .set(createHeaders(ALICE.username, 'wrong-pw-1419')); + ok([401, 403].includes(badGet.status), `wrong password was accepted (status ${badGet.status})`); + }); + + test('PROBE: whole-table SSE delivers only Alice rows; Bob/neutral events are filtered', async () => { + const stream = await openSse(`${restURL}/Vault/`, { Authorization: aliceBearer }); + openStreams.add(stream); + + ok(stream.status >= 200 && stream.status < 300, `Alice's collection SSE should open, got ${stream.status}`); + + await sleep(400); // let the subscription attach + + for (const id of ALICE_ROWS) { + await adminPut(id, { id, owner: ALICE.username, secret: `alice-secret-${id}`, updated: true }).expect(204); + } + for (const id of BOB_ROWS) { + await adminPut(id, { id, owner: BOB.username, secret: `bob-secret-${id}`, updated: true }).expect(204); + } + await adminPut(NEUTRAL_ROW, { id: NEUTRAL_ROW, owner: 'nobody', secret: 'neutral-secret', updated: true }).expect( + 204 + ); + + // Positive control: at least one of Alice's events must arrive. + const gotAlice = await waitFor(() => ALICE_ROWS.some((id) => streamMentions(stream, id)), 8000); + + // Give denied rows equal opportunity to leak. + await sleep(1500); + + const bobLeak = + BOB_ROWS.some((id) => streamMentions(stream, id)) || + streamMentions(stream, BOB.username) || + streamMentions(stream, 'bob-secret'); + const neutralLeak = + streamMentions(stream, NEUTRAL_ROW) || + streamMentions(stream, 'neutral-secret') || + streamMentions(stream, 'nobody'); + + ok( + gotAlice, + 'POSITIVE CONTROL FAILED: Alice did not receive updates to her own rows on the collection subscription' + ); + ok(!bobLeak, "ROW-LEVEL LEAK (#1419): Bob's row events arrived on Alice's collection subscription stream"); + ok( + !neutralLeak, + "ROW-LEVEL LEAK (#1419): the neutral row's events arrived on Alice's collection subscription stream" + ); + }); +}); diff --git a/resources/Resource.ts b/resources/Resource.ts index b9e6987d8..e3d9b59e1 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -749,21 +749,22 @@ function transactional( // row-level machinery and keep the entry check. Subscriptions are excluded: their // delivery path never reaches search(), so deferring would grant them UNCHECKED — // they keep the entry evaluation (which fails closed for a record-scoped override) - // until per-event delivery checks exist (#1419). Method semantics decide the deferral: - // `get` with a non-null id is a true single-record read (entry check runs with the - // record loaded, so record-scoped field reads resolve) and only defers when - // isCollection — mirroring the isSearchTarget routing it relies on. ALL search/query - // calls defer: a present id there is a starts_with/prefix SEED (a multi-record scan, - // see Table.search), so an entry verdict would gate — or worse, grant — the whole - // scan; both methods invoke instance search() directly, which consumes - // checkPermission and arms the per-record guard. + // Method semantics decide the deferral: `get` with a non-null id is a true + // single-record read (entry check runs with the record loaded, so record-scoped field + // reads resolve) and only defers when isCollection — mirroring the isSearchTarget + // routing it relies on. ALL search/query calls defer: a present id there is a + // starts_with/prefix SEED (a multi-record scan, see Table.search), so an entry verdict + // would gate — or worse, grant — the whole scan; both methods invoke instance search() + // directly, which consumes checkPermission and arms the per-record guard. + // subscribe/connect defer too: Table.subscribe filters each delivered event through + // the record-scoped check (#1419), so grant-time evaluation of a record-scoped + // override (against a bare collection resource) would only produce spurious verdicts. // Record-scoping is SYNC-only: an async allowRead override keeps this entry check, // which awaits its verdict and fails closed on rejection (#1422 gap 1) — the sync - // per-record traversal path cannot await it. + // per-record/per-event paths cannot await it. if ( options.type === 'read' && - !isSubscribeAction && - (options.method !== 'get' || query.isCollection) && + (isSubscribeAction || options.method !== 'get' || query.isCollection) && (resource.constructor as any)?.supportsRowLevelAllowRead && !(resource.allowRead as any)?.isDefaultAllowRead && (resource.allowRead as any)?.constructor?.name !== 'AsyncFunction' @@ -841,14 +842,29 @@ function registerLiveSubscriptionForContext(subscription: any, resource: any, qu // and getCurrentUser() (which reads the resource's context) — evaluate against current state, // not the stale user captured at subscribe time. if (context) (context as any).user = fresh; - // Re-run the same table/RBAC-level allowRead the subscription was granted with, against the - // fresh user. No per-record evaluation — this matches how access was originally granted. + // Re-run the table/RBAC-level allowRead against the fresh user. For a record-scoped + // override (#1419), the override itself is enforced per delivered event — evaluating it + // here against a bare collection resource would spuriously kill the subscription — so + // re-auth verifies the FRAMEWORK default (the table-level grant the subscription rests + // on) by walking the prototype chain to the method marked isDefaultAllowRead. const reTarget: any = new RequestTarget(); reTarget.id = capturedId; reTarget.isCollection = capturedIsCollection; reTarget.select = capturedSelect; reTarget.checkPermission = fresh.role?.permission; - return !!(await resource.allowRead(fresh, reTarget, context)); + let allowReadFn: any = resource.allowRead; + if (!allowReadFn?.isDefaultAllowRead && (resource.constructor as any)?.supportsRowLevelAllowRead) { + let proto = Object.getPrototypeOf(resource); + while (proto) { + const fn = Object.getOwnPropertyDescriptor(proto, 'allowRead')?.value; + if (fn?.isDefaultAllowRead) { + allowReadFn = fn; + break; + } + proto = Object.getPrototypeOf(proto); + } + } + return !!(await allowReadFn.call(resource, fresh, reTarget, context)); }, }); } diff --git a/resources/Table.ts b/resources/Table.ts index c96785888..edcae45e2 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -3410,6 +3410,50 @@ export function makeTable(options) { let reloadResnapshotRunning = false; let reloadResnapshotPending = false; const thisId = requestTargetToId(request) ?? null; // treat undefined and null as the root + // Record-scoped allowRead on subscription delivery (#1419, reviving #1524 on the unified + // model): a sync application-overridden allowRead is re-evaluated per record-bearing event + // with `this` = the event's record, the delivery-path analog of the per-record query guard. + // Gated to (1) an overridden, sync allowRead — the default is record-independent and already + // enforced at connect, so the common case stays zero-overhead — and (2) checkPermission on + // the request: the authorize wrapper defers a checked subscribe here (leaving the flag set) + // precisely so delivery enforces it, while internal subscribers (replication, system + // watchers) never request permission checks and keep full delivery. The user may still be + // null on a checked request; the override decides (fail-closed for field comparisons). + // Fails closed: a throwing or thenable-returning override drops the event rather than leaks it. + // + // Known limitation (as in #1524): only put/invalidate events carry the authoritative full + // record. delete (tombstone, null value), message payloads, and rawEvents partial values may + // mis-decide an override keyed on row fields; closing the primary leak (record updates) is + // the goal here — authorizing non-record-bearing event types against the full row is deferred. + const subContext = this.getContext() as any; + const subUser = subContext?.user; + const subAllowRead = this.allowRead; + const filterRowReads = + (request as any)?.checkPermission && + !(subAllowRead as any)?.isDefaultAllowRead && + (subAllowRead as any)?.constructor?.name !== 'AsyncFunction'; + let warnedAsyncEvent = false; + const allowsEvent = filterRowReads + ? (event: any): boolean => { + if (event.type === 'end_txn' || event.type === 'reload') return true; // control markers, no record + let decision; + try { + decision = subAllowRead.call(event.value ?? null, subUser, request, subContext); + } catch { + return false; // fail closed: a throwing override drops the event + } + if (decision != null && typeof decision.then === 'function') { + if (!warnedAsyncEvent) { + warnedAsyncEvent = true; + logger.warn?.( + `allowRead on ${tableName} returned a promise during subscription event evaluation; events are dropped (record-scoped allowRead must be synchronous)` + ); + } + return false; + } + return Boolean(decision); + } + : null; const subscription = addSubscription( TableResource, thisId, @@ -3451,8 +3495,11 @@ export function makeTable(options) { type, beginTxn, }; + // Queued events are filtered when the queue drains through send() below; events sent + // directly (queue already drained) are filtered here. Each event is filtered once. if (pendingRealTimeQueue) pendingRealTimeQueue.push(event); else { + if (allowsEvent && !allowsEvent(event)) return; if (databaseName !== 'system') { recordAction(auditRecord.size ?? 1, 'db-message', tableName, null); } @@ -3538,13 +3585,17 @@ export function makeTable(options) { const id = auditRecord.recordId; if (thisId == null || isDescendantId(thisId, id)) { const value = auditRecord.getValue(primaryStore, getFullRecord, auditRecord.localTime); - history.push({ + const historyEntry = { id, localTime: auditRecord.localTime, value, version: auditRecord.version, type: auditRecord.type, - }); + }; + // Filter denied rows BEFORE they consume a previousCount slot, so an authorized + // subscriber still receives up to `count` readable events (#1419). + if (allowsEvent && !allowsEvent(historyEntry)) continue; + history.push(historyEntry); if (--count <= 0) break; } } catch (error) { @@ -3686,6 +3737,8 @@ export function makeTable(options) { subscription.send(error); }); function send(event: any) { + // Covers the pendingRealTimeQueue drain and the reload re-snapshot (#495) delivery. + if (allowsEvent && !allowsEvent(event)) return; if (databaseName !== 'system') { recordAction(event.size ?? 1, 'db-message', tableName, null); } diff --git a/server/graphqlQuerying.ts b/server/graphqlQuerying.ts index 183661016..8d2283f1f 100644 --- a/server/graphqlQuerying.ts +++ b/server/graphqlQuerying.ts @@ -315,11 +315,13 @@ async function processFieldNode( const query = { select: buildSelectQuery(fieldNode.selectionSet, fragments), conditions: buildConditionsQuery(fieldNode.arguments, resolvedVariables), + // Request authorization directly on the query target (checkPermission) rather than via the + // context-level `authorize` alias — same wrapper handling, and it keeps GraphQL consistent + // with the REST query path, including row-level (record-scoped allowRead) enforcement. + checkPermission: true, }; const results = []; - // @ts-expect-error: `authorize` is a custom property on the request object. - request.authorize = true; for await (const result of resource.search(query, request)) { results.push(result); } diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 8310d19ec..b26e3a5fe 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1742,21 +1742,64 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => } }); - it('subscribe with a record-scoped allowRead is denied at entry, never granted unchecked', async () => { - // Subscription delivery never reaches search(), so the per-record deferral must NOT apply to - // subscribe — otherwise a collection subscription would be granted with no check at all. - // Until per-event delivery checks exist (#1419), the entry evaluation runs the override with - // `this` = collection resource (fields undefined) and fails closed. + it('subscription delivery filters events through a record-scoped allowRead (#1419)', async () => { + // A sync record-scoped override defers grant-time evaluation and instead filters each + // delivered event with `this` = the event's record — the subscriber receives only the row + // changes they may read, instead of being denied outright (or, pre-#1419, receiving all). class Restricted extends T { allowRead(user) { - return this.ownerId === user.id; + return user != null && this?.ownerId === user.id; + } + } + // subscribe's arg normalization only recognizes a context carrying transaction/getContext + const subscription = await Restricted.subscribe( + { checkPermission: true, omitCurrent: true }, + { + user: { id: 1, username: 'u1', role: { permission: {} } }, + getContext() { + return this; + }, + } + ); + const received = []; + const collector = (async () => { + for await (const event of subscription) { + if (event.type === 'end_txn') continue; + received.push(event); + } + })(); + try { + // ownerId = i % 3 → id 100 (100%3=1) visible to user 1; id 101 (101%3=2) is not. + await T.put(100, { group: 'blue', ownerId: 1, active: true, vector: [100, 0] }); + await T.put(101, { group: 'red', ownerId: 2, active: true, vector: [101, 0] }); + await new Promise((resolve) => setTimeout(resolve, 150)); + assert( + received.some((e) => e.id === 100), + 'subscriber receives events for records they may read' + ); + assert(!received.some((e) => e.id === 101), 'events for records the user may NOT read are filtered out'); + } finally { + subscription.return?.(); + await Promise.race([collector, new Promise((resolve) => setTimeout(resolve, 100))]); + } + }); + + it('subscribe with an ASYNC record-scoped allowRead stays denied at entry (cannot filter per event)', async () => { + class AsyncRestricted extends T { + async allowRead(user) { + return user != null && this?.ownerId === user.id; } } await assert.rejects( async () => - Restricted.subscribe( + AsyncRestricted.subscribe( { checkPermission: true, omitCurrent: true }, - { user: { id: 1, username: 'u1', role: { permission: {} } } } + { + user: { id: 1, username: 'u1', role: { permission: {} } }, + getContext() { + return this; + }, + } ), (err) => err.name === 'AccessViolation' || /unauthorized/i.test(err.message) || err.statusCode === 403 ); From 76282d85484e2f43c7789cecbae9278b8ea713b9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 14 Jul 2026 05:59:35 -0600 Subject: [PATCH 07/12] =?UTF-8?q?fix:=20subscribe=20keeps=20its=20entry=20?= =?UTF-8?q?check=20(connection=20grant)=20=E2=80=94=20don't=20defer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- resources/Resource.ts | 14 +++++++++----- resources/Table.ts | 19 ++++++++++--------- unitTests/resources/vectorIndex.test.js | 22 +++++++++++++--------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/resources/Resource.ts b/resources/Resource.ts index e3d9b59e1..b1fc94a14 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -756,15 +756,19 @@ function transactional( // starts_with/prefix SEED (a multi-record scan, see Table.search), so an entry verdict // would gate — or worse, grant — the whole scan; both methods invoke instance search() // directly, which consumes checkPermission and arms the per-record guard. - // subscribe/connect defer too: Table.subscribe filters each delivered event through - // the record-scoped check (#1419), so grant-time evaluation of a record-scoped - // override (against a bare collection resource) would only produce spurious verdicts. + // subscribe/connect are NOT deferred: the entry check is the connection grant, and an + // allowRead override there may be connection-level, not record-level — e.g. an MQTT + // topic ACL (@harperdb/acl-connect) decides on context.topic, not record fields, and + // must run at subscribe time to reject the topic. A record-level override instead + // returns permissive at collection scope (no record loaded) to open the subscription, + // and Table.subscribe then filters each delivered event per record (#1419). // Record-scoping is SYNC-only: an async allowRead override keeps this entry check, // which awaits its verdict and fails closed on rejection (#1422 gap 1) — the sync - // per-record/per-event paths cannot await it. + // per-record traversal path cannot await it. if ( options.type === 'read' && - (isSubscribeAction || options.method !== 'get' || query.isCollection) && + !isSubscribeAction && + (options.method !== 'get' || query.isCollection) && (resource.constructor as any)?.supportsRowLevelAllowRead && !(resource.allowRead as any)?.isDefaultAllowRead && (resource.allowRead as any)?.constructor?.name !== 'AsyncFunction' diff --git a/resources/Table.ts b/resources/Table.ts index edcae45e2..385f448d5 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -3411,14 +3411,15 @@ export function makeTable(options) { let reloadResnapshotPending = false; const thisId = requestTargetToId(request) ?? null; // treat undefined and null as the root // Record-scoped allowRead on subscription delivery (#1419, reviving #1524 on the unified - // model): a sync application-overridden allowRead is re-evaluated per record-bearing event - // with `this` = the event's record, the delivery-path analog of the per-record query guard. - // Gated to (1) an overridden, sync allowRead — the default is record-independent and already - // enforced at connect, so the common case stays zero-overhead — and (2) checkPermission on - // the request: the authorize wrapper defers a checked subscribe here (leaving the flag set) - // precisely so delivery enforces it, while internal subscribers (replication, system - // watchers) never request permission checks and keep full delivery. The user may still be - // null on a checked request; the override decides (fail-closed for field comparisons). + // model): the subscribe entry check already granted the connection (topic ACL / collection + // scope); here we ADDITIONALLY re-evaluate a sync application-overridden allowRead per + // record-bearing event with `this` = the event's record, the delivery-path analog of the + // per-record query guard. Gated to (1) an overridden, sync allowRead — the default is + // record-independent and already enforced at connect, so the common case stays zero-overhead + // — and (2) a user principal on the context: the connect-time authorization consumed the + // request's checkPermission flag before we get here, so context.user is the persistent + // signal that this is an authorization-checked (external) subscription; internal subscribers + // (replication, system watchers) have no user and keep full delivery. // Fails closed: a throwing or thenable-returning override drops the event rather than leaks it. // // Known limitation (as in #1524): only put/invalidate events carry the authoritative full @@ -3429,7 +3430,7 @@ export function makeTable(options) { const subUser = subContext?.user; const subAllowRead = this.allowRead; const filterRowReads = - (request as any)?.checkPermission && + subUser != null && !(subAllowRead as any)?.isDefaultAllowRead && (subAllowRead as any)?.constructor?.name !== 'AsyncFunction'; let warnedAsyncEvent = false; diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index b26e3a5fe..561694d66 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1743,12 +1743,14 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => }); it('subscription delivery filters events through a record-scoped allowRead (#1419)', async () => { - // A sync record-scoped override defers grant-time evaluation and instead filters each - // delivered event with `this` = the event's record — the subscriber receives only the row - // changes they may read, instead of being denied outright (or, pre-#1419, receiving all). + // The subscribe entry check grants the connection (collection scope → permissive here, the + // documented contract that lets a whole-table subscription open); delivery then filters each + // event per record, so the subscriber receives only the row changes they may read (instead of + // every row, pre-#1419). class Restricted extends T { allowRead(user) { - return user != null && this?.ownerId === user.id; + if (this?.ownerId == null) return true; // collection scope: open the subscription + return this.ownerId === user?.id; // per-record on delivery } } // subscribe's arg normalization only recognizes a context carrying transaction/getContext @@ -1784,15 +1786,17 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => } }); - it('subscribe with an ASYNC record-scoped allowRead stays denied at entry (cannot filter per event)', async () => { - class AsyncRestricted extends T { - async allowRead(user) { - return user != null && this?.ownerId === user.id; + it('subscribe entry check still guards the connection grant (protects connection-level allowRead)', async () => { + // subscribe is NOT deferred: the entry check runs the override as the connection grant, so a + // connection-level override (e.g. an MQTT topic ACL) that denies is honored at subscribe time. + class DeniesConnection extends T { + allowRead() { + return false; // deny at the connection/collection grant } } await assert.rejects( async () => - AsyncRestricted.subscribe( + DeniesConnection.subscribe( { checkPermission: true, omitCurrent: true }, { user: { id: 1, username: 'u1', role: { permission: {} } }, From 1e8f9caa6cb14d19568b9258c3eec264f36d8320 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 14 Jul 2026 06:04:37 -0600 Subject: [PATCH 08/12] =?UTF-8?q?test:=20cover=20re-auth=20default-fallbac?= =?UTF-8?q?k=20for=20a=20record-scoped-override=20subscription=20(#1414=20?= =?UTF-8?q?=C3=97=20#1419)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../subscription-row-allowread.test.ts | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/integrationTests/security/subscription-row-allowread.test.ts b/integrationTests/security/subscription-row-allowread.test.ts index cccd691c1..3ebd372a4 100644 --- a/integrationTests/security/subscription-row-allowread.test.ts +++ b/integrationTests/security/subscription-row-allowread.test.ts @@ -38,7 +38,10 @@ const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === ' const ALICE = { username: 'sub_allowread_alice', password: 'Alice-pw-1419!' }; const BOB = { username: 'sub_allowread_bob', password: 'Bobby-pw-1419!' }; +const CAROL = { username: 'sub_allowread_carol', password: 'Carol-pw-1419!' }; // revocation subject (#1414 × override) const ROLE = 'sub_allowread_role'; +const ROLE_CAROL = 'sub_allowread_role_carol'; // dedicated so alter_role can strip read without touching ALICE/BOB +const CAROL_ROW = 'row-c1'; const ALICE_ROWS = ['row-a1', 'row-a2', 'row-a3']; const BOB_ROWS = ['row-b1', 'row-b2', 'row-b3']; @@ -161,17 +164,38 @@ suite('#1419 row-level allowRead filters subscription delivery', { skip: skipSui }) .expect(200); + await client + .req() + .send({ + operation: 'add_role', + role: ROLE_CAROL, + permission: { + super_user: false, + data: { + tables: { + Vault: { read: true, insert: true, update: true, delete: true, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + for (const u of [ALICE, BOB]) { await client .req() .send({ operation: 'add_user', role: ROLE, username: u.username, password: u.password, active: true }) .expect(200); } + await client + .req() + .send({ operation: 'add_user', role: ROLE_CAROL, username: CAROL.username, password: CAROL.password, active: true }) + .expect(200); // Seed rows via the ops API (super, bypasses allowCreate). const records = [ ...ALICE_ROWS.map((id) => ({ id, owner: ALICE.username, secret: `alice-secret-${id}` })), ...BOB_ROWS.map((id) => ({ id, owner: BOB.username, secret: `bob-secret-${id}` })), + { id: CAROL_ROW, owner: CAROL.username, secret: 'carol-secret' }, { id: NEUTRAL_ROW, owner: 'nobody', secret: 'neutral-secret' }, ]; await client.req().send({ operation: 'insert', schema: 'data', table: 'Vault', records }).expect(200); @@ -256,4 +280,53 @@ suite('#1419 row-level allowRead filters subscription delivery', { skip: skipSui "ROW-LEVEL LEAK (#1419): the neutral row's events arrived on Alice's collection subscription stream" ); }); + + test('REVOCATION (#1414 × override): alter_role removing read terminates a record-scoped subscription', async () => { + // Exercises the re-auth default-fallback: the override is collection-permissive (so it opens the + // subscription and would ALWAYS pass re-auth at collection scope), so re-auth must instead run + // the framework-default table RBAC against the fresh user. alter_role (not drop_user) keeps the + // user present — findAndValidateUser still returns a role — so termination can only come from the + // default table grant now denying read, which is exactly the fallback branch under test. + const carolToken = await client + .req() + .send({ operation: 'create_authentication_tokens', username: CAROL.username, password: CAROL.password }); + const carolBearer = `Bearer ${carolToken.body.operation_token}`; + + const stream = await openSse(`${restURL}/Vault/`, { Authorization: carolBearer }); + openStreams.add(stream); + ok(stream.status >= 200 && stream.status < 300, `Carol's collection SSE should open, got ${stream.status}`); + await sleep(400); + + // Baseline: Carol receives updates to her own row. + await adminPut(CAROL_ROW, { id: CAROL_ROW, owner: CAROL.username, secret: 'carol-secret', v: 1 }).expect(204); + ok(await waitFor(() => streamMentions(stream, CAROL_ROW), 8000), 'Carol should receive her own row event'); + + // Strip read from Carol's role in place; the user still exists, so only the default table grant + // (via the re-auth fallback) can terminate the stream. + await client + .req() + .send({ + operation: 'alter_role', + id: ROLE_CAROL, + permission: { + super_user: false, + data: { + tables: { + Vault: { read: false, insert: false, update: false, delete: false, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + await sleep(2000); // let the user-change broadcast / re-auth land under CI load + + const eventsAfterRevoke = stream.events.length; + await adminPut(CAROL_ROW, { id: CAROL_ROW, owner: CAROL.username, secret: 'carol-secret', v: 2 }).expect(204); + await sleep(1500); + + ok( + stream.events.length === eventsAfterRevoke, + `role lost read but subscription kept receiving ${stream.events.length - eventsAfterRevoke} event(s) — re-auth default-fallback did not terminate the record-scoped subscription` + ); + }); }); From dd4add0f0276a77d4f098f90a4ea3a8cf5448a9d Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 14 Jul 2026 06:35:19 -0600 Subject: [PATCH 09/12] review: re-auth mirrors the subscribe entry check (re-runs the override), not the default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../subscription-row-allowread/resources.js | 21 ++++++++------- .../subscription-row-allowread.test.ts | 18 ++++++++----- resources/Resource.ts | 26 ++++++------------- 3 files changed, 32 insertions(+), 33 deletions(-) diff --git a/integrationTests/security/fixtures/subscription-row-allowread/resources.js b/integrationTests/security/fixtures/subscription-row-allowread/resources.js index 95b1b5f43..8b561d8b0 100644 --- a/integrationTests/security/fixtures/subscription-row-allowread/resources.js +++ b/integrationTests/security/fixtures/subscription-row-allowread/resources.js @@ -1,12 +1,12 @@ // Regression fixture for #1419 — per-row allowRead must filter subscription delivery. // -// allowRead is designed so that: -// - When `this` is a loaded record (a specific row), allow only if the row's owner -// matches the requesting user. -// - When `this` is the collection (no specific record loaded, this.owner undefined/null), -// allow the subscription to open. This is the permissive side that lets a non-owner open -// a whole-table subscription — the scenario where the pre-#1419 delivery loop leaked every -// row's events because it never re-evaluated allowRead per record. +// The recommended composition pattern for a record-scoped override: +// - Gate on the base table/RBAC grant via `super.allowRead(...)` first, so a user who loses the +// role's table-read is denied — at request entry, and (for a live subscription) when the #1414 +// re-auth recheck re-runs this same override against the fresh user. +// - At collection scope (no record loaded — a whole-table subscribe, or the re-auth recheck), +// return the base grant so the connection opens; per-row filtering happens during delivery. +// - Per record (a loaded row), additionally require the requesting user to own the row. // // Super users always pass so that seed writes and setup ops succeed. @@ -15,10 +15,13 @@ function isSuper(user) { } export class Vault extends tables.Vault { - allowRead(user, _target, _context) { + allowRead(user, target, context) { if (isSuper(user)) return true; + // Base table/RBAC grant — composes with the override so revoking the role's read terminates + // (at entry and via #1414 re-auth), rather than the override standing in for RBAC. + if (!super.allowRead(user, target, context)) return false; const owner = this?.owner; - // Collection subscribe / no loaded record: allow the connection to open. + // Collection subscribe / no loaded record: RBAC passed, allow the connection to open. if (owner === undefined || owner === null) return true; // Per-row: only the owner can read this specific record. return owner === user?.username; diff --git a/integrationTests/security/subscription-row-allowread.test.ts b/integrationTests/security/subscription-row-allowread.test.ts index 3ebd372a4..bab3e5a9b 100644 --- a/integrationTests/security/subscription-row-allowread.test.ts +++ b/integrationTests/security/subscription-row-allowread.test.ts @@ -188,7 +188,13 @@ suite('#1419 row-level allowRead filters subscription delivery', { skip: skipSui } await client .req() - .send({ operation: 'add_user', role: ROLE_CAROL, username: CAROL.username, password: CAROL.password, active: true }) + .send({ + operation: 'add_user', + role: ROLE_CAROL, + username: CAROL.username, + password: CAROL.password, + active: true, + }) .expect(200); // Seed rows via the ops API (super, bypasses allowCreate). @@ -282,11 +288,11 @@ suite('#1419 row-level allowRead filters subscription delivery', { skip: skipSui }); test('REVOCATION (#1414 × override): alter_role removing read terminates a record-scoped subscription', async () => { - // Exercises the re-auth default-fallback: the override is collection-permissive (so it opens the - // subscription and would ALWAYS pass re-auth at collection scope), so re-auth must instead run - // the framework-default table RBAC against the fresh user. alter_role (not drop_user) keeps the - // user present — findAndValidateUser still returns a role — so termination can only come from the - // default table grant now denying read, which is exactly the fallback branch under test. + // The #1414 re-auth recheck re-runs the SAME override that granted the connection, against the + // fresh user. Because this override composes the base RBAC grant via `super.allowRead`, revoking + // the role's table-read makes the override deny at collection scope → the subscription is torn + // down. alter_role (not drop_user) keeps the user present — findAndValidateUser still returns a + // role — so termination can only come from the override re-evaluating the (now-denied) RBAC grant. const carolToken = await client .req() .send({ operation: 'create_authentication_tokens', username: CAROL.username, password: CAROL.password }); diff --git a/resources/Resource.ts b/resources/Resource.ts index b1fc94a14..16b7f8fb6 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -846,29 +846,19 @@ function registerLiveSubscriptionForContext(subscription: any, resource: any, qu // and getCurrentUser() (which reads the resource's context) — evaluate against current state, // not the stale user captured at subscribe time. if (context) (context as any).user = fresh; - // Re-run the table/RBAC-level allowRead against the fresh user. For a record-scoped - // override (#1419), the override itself is enforced per delivered event — evaluating it - // here against a bare collection resource would spuriously kill the subscription — so - // re-auth verifies the FRAMEWORK default (the table-level grant the subscription rests - // on) by walking the prototype chain to the method marked isDefaultAllowRead. + // Re-run the SAME allowRead the subscribe entry check ran, against the fresh user — this + // mirrors the connection grant exactly. For a record-scoped override (#1419), that override + // gated the connection at collection scope (typically composing the table/RBAC grant via + // `super.allowRead`), and its per-record decisions are enforced separately during delivery; + // re-running it here re-verifies whatever it gated the connection on — a connection-level + // override (e.g. an MQTT topic ACL) re-checks its topic grant, and a record-scoped override + // that composes `super` re-checks the RBAC baseline. No per-record evaluation here. const reTarget: any = new RequestTarget(); reTarget.id = capturedId; reTarget.isCollection = capturedIsCollection; reTarget.select = capturedSelect; reTarget.checkPermission = fresh.role?.permission; - let allowReadFn: any = resource.allowRead; - if (!allowReadFn?.isDefaultAllowRead && (resource.constructor as any)?.supportsRowLevelAllowRead) { - let proto = Object.getPrototypeOf(resource); - while (proto) { - const fn = Object.getOwnPropertyDescriptor(proto, 'allowRead')?.value; - if (fn?.isDefaultAllowRead) { - allowReadFn = fn; - break; - } - proto = Object.getPrototypeOf(proto); - } - } - return !!(await allowReadFn.call(resource, fresh, reTarget, context)); + return !!(await resource.allowRead(fresh, reTarget, context)); }, }); } From f6846117382cb42ed97ae19d9411b57ee59e31c9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 14 Jul 2026 11:57:39 -0600 Subject: [PATCH 10/12] review(cb1kenobi): fix QUERY-verb bypass, unfrozen delivery record, stale/anon subscription gate; clarify async MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- resources/Resource.ts | 92 ++++++++++++++++--------- resources/Table.ts | 26 +++---- unitTests/resources/vectorIndex.test.js | 72 ++++++++++++++++--- 3 files changed, 138 insertions(+), 52 deletions(-) diff --git a/resources/Resource.ts b/resources/Resource.ts index 16b7f8fb6..9dcb3aadf 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -18,6 +18,7 @@ import { transaction, contextStorage } from './transaction.ts'; import { parseQuery } from './search.ts'; import { RequestTarget } from './RequestTarget.ts'; import { when, promiseNormalize } from '../utility/when.ts'; +import { logger } from '../utility/logging/logger.ts'; import { registerLiveSubscription } from '../server/liveSubscriptionAuth.ts'; import type { JsonSchemaFragment } from './jsonSchemaTypes.ts'; @@ -283,6 +284,20 @@ export class Resource implements ResourceInterface< static query = transactional( function (resource: any, query: RequestTarget, _request: Context, data: any) { + // On the table path, search(target) uses `data` (the request body carrying the conditions) as + // its sole target, while checkPermission was set on the URL `query`. Thread it across so + // Table.search sees the flag and enforces row-level allowRead — otherwise a QUERY on a table + // with an overridden allowRead returns the full unfiltered set (the deferral skips the entry + // check on the promise that search consumes checkPermission, which it never sees). + if ( + resource.constructor.loadAsInstance !== false && + data && + typeof data === 'object' && + (query as any)?.checkPermission != null && + (data as any).checkPermission == null + ) { + (data as any).checkPermission = (query as any).checkPermission; + } return resource.search ? resource.constructor.loadAsInstance === false ? resource.search(query, data) @@ -491,6 +506,20 @@ export class Resource implements ResourceInterface< _assignPackageExport('Resource', Resource); +// Warn once per table when an ASYNC allowRead override is used on a row-level-capable table: it is +// evaluated only at the collection-scope entry check and gets NO per-record/per-event filtering, so +// the collection-permissive "open, filter per record" pattern would fail open. Row-level filtering +// requires a synchronous override. +const asyncAllowReadWarned = new Set(); +function warnAsyncAllowReadOnce(tableName: string | undefined): void { + const key = tableName ?? ''; + if (asyncAllowReadWarned.has(key)) return; + asyncAllowReadWarned.add(key); + logger.warn?.( + `allowRead override on table "${key}" is async: it is evaluated only at request entry (collection scope) with NO per-record or per-event filtering. Row-level (record-scoped) authorization requires a synchronous allowRead — an async collection-permissive override would return unfiltered results.` + ); +} + // Mark the built-in allowRead so the authorization flow can tell a framework default from an // application override. An overridden allowRead on a table is evaluated per RECORD during query // execution (#1422 gap 2 / #1241) — with `this` being each record — instead of once at collection @@ -739,40 +768,39 @@ function transactional( } if (checkPermission) { if (loadAsInstance !== false) { - // Record-scoped allowRead on a table collection read (#1422 gap 2): an overridden - // allowRead is evaluated per record during query execution (`this` = each record), not - // once here where `this` is a collection resource with no record loaded — an entry - // verdict would be meaningless (spurious 403s or, worse, granting the whole scan). - // Leave query.checkPermission set; Table.search() converts it into per-record - // enforcement (get/query on a collection route into search, which consumes it). Only - // tables opt in (supportsRowLevelAllowRead) — plain Resource subclasses have no - // row-level machinery and keep the entry check. Subscriptions are excluded: their - // delivery path never reaches search(), so deferring would grant them UNCHECKED — - // they keep the entry evaluation (which fails closed for a record-scoped override) - // Method semantics decide the deferral: `get` with a non-null id is a true - // single-record read (entry check runs with the record loaded, so record-scoped field - // reads resolve) and only defers when isCollection — mirroring the isSearchTarget - // routing it relies on. ALL search/query calls defer: a present id there is a - // starts_with/prefix SEED (a multi-record scan, see Table.search), so an entry verdict - // would gate — or worse, grant — the whole scan; both methods invoke instance search() - // directly, which consumes checkPermission and arms the per-record guard. - // subscribe/connect are NOT deferred: the entry check is the connection grant, and an - // allowRead override there may be connection-level, not record-level — e.g. an MQTT - // topic ACL (@harperdb/acl-connect) decides on context.topic, not record fields, and - // must run at subscribe time to reject the topic. A record-level override instead - // returns permissive at collection scope (no record loaded) to open the subscription, - // and Table.subscribe then filters each delivered event per record (#1419). - // Record-scoping is SYNC-only: an async allowRead override keeps this entry check, - // which awaits its verdict and fails closed on rejection (#1422 gap 1) — the sync - // per-record traversal path cannot await it. - if ( + // Does this table carry an APPLICATION-overridden allowRead (record-scoped, #1422 gap 2)? + // The framework defaults are marked isDefaultAllowRead; anything else is an app override. + // Row-level authorization (per-record query traversal, per-event delivery) is SYNC-only. + // A SYNC application override participates in it; an ASYNC override cannot (traversal + // and delivery can't await), so it is evaluated ONLY at the collection-scope entry + // check — a table/connection-level decision, the same as before this feature. It must + // therefore make a complete decision at collection scope; the collection-permissive + // "open, filter per record" pattern requires a synchronous override (warned below). + const overridden = options.type === 'read' && - !isSubscribeAction && - (options.method !== 'get' || query.isCollection) && (resource.constructor as any)?.supportsRowLevelAllowRead && - !(resource.allowRead as any)?.isDefaultAllowRead && - (resource.allowRead as any)?.constructor?.name !== 'AsyncFunction' - ) { + !(resource.allowRead as any)?.isDefaultAllowRead; + const rowLevelOverride = overridden && (resource.allowRead as any)?.constructor?.name !== 'AsyncFunction'; + if (rowLevelOverride) { + // Durable signal that this read was authorization-checked, for the subscription + // delivery filter (the entry check below clears query.checkPermission before + // Table.subscribe runs, and an anonymous-but-checked subscription has no user to key on). + (query as any).rowLevelAuthChecked = true; + } else if (overridden) { + warnAsyncAllowReadOnce((resource.constructor as any)?.name); + } + // Deferral (per-record enforcement instead of a meaningless collection-scope entry + // verdict): `get` with a non-null id is a true single-record read (entry check runs with + // the record loaded) and only defers when isCollection — mirroring the isSearchTarget + // routing. ALL search/query calls defer: a present id there is a starts_with/prefix SEED + // (multi-record scan), so an entry verdict would gate — or grant — the whole scan; both + // invoke instance search() directly, which consumes checkPermission and arms the guard. + // subscribe/connect are NOT deferred: the entry check is the connection grant, and an + // override there may be connection-level (e.g. an MQTT topic ACL that decides on + // context.topic, not record fields, and must run at subscribe time); a record-level + // override returns permissive at collection scope to open, and Table.subscribe then + // filters each delivered event per record (#1419). + if (rowLevelOverride && !isSubscribeAction && (options.method !== 'get' || query.isCollection)) { return when(data, (data) => { return runAction(data); }); diff --git a/resources/Table.ts b/resources/Table.ts index 385f448d5..feec7eed0 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -3414,12 +3414,12 @@ export function makeTable(options) { // model): the subscribe entry check already granted the connection (topic ACL / collection // scope); here we ADDITIONALLY re-evaluate a sync application-overridden allowRead per // record-bearing event with `this` = the event's record, the delivery-path analog of the - // per-record query guard. Gated to (1) an overridden, sync allowRead — the default is - // record-independent and already enforced at connect, so the common case stays zero-overhead - // — and (2) a user principal on the context: the connect-time authorization consumed the - // request's checkPermission flag before we get here, so context.user is the persistent - // signal that this is an authorization-checked (external) subscription; internal subscribers - // (replication, system watchers) have no user and keep full delivery. + // per-record query guard. Gated to (1) an overridden allowRead (the default is + // record-independent and already enforced at connect, so the common case stays zero-overhead) + // and (2) an authorization-checked subscription — the entry check clears checkPermission + // before we get here and an anonymous-but-checked subscription has no user to key on, so the + // wrapper stamps a durable `rowLevelAuthChecked` flag; internal subscribers (replication, + // system watchers) never set it and keep full delivery. // Fails closed: a throwing or thenable-returning override drops the event rather than leaks it. // // Known limitation (as in #1524): only put/invalidate events carry the authoritative full @@ -3427,19 +3427,21 @@ export function makeTable(options) { // mis-decide an override keyed on row fields; closing the primary leak (record updates) is // the goal here — authorizing non-record-bearing event types against the full row is deferred. const subContext = this.getContext() as any; - const subUser = subContext?.user; const subAllowRead = this.allowRead; - const filterRowReads = - subUser != null && - !(subAllowRead as any)?.isDefaultAllowRead && - (subAllowRead as any)?.constructor?.name !== 'AsyncFunction'; + const filterRowReads = (request as any)?.rowLevelAuthChecked && !(subAllowRead as any)?.isDefaultAllowRead; let warnedAsyncEvent = false; const allowsEvent = filterRowReads ? (event: any): boolean => { if (event.type === 'end_txn' || event.type === 'reload') return true; // control markers, no record + // Freeze the record before the override sees it, mirroring the query path — event.value + // is the shared primaryStore object, so an override that writes to `this` would otherwise + // mutate the cache for every reader on the node. + if (event.value) freezeRecord(event.value); let decision; try { - decision = subAllowRead.call(event.value ?? null, subUser, request, subContext); + // Evaluate against the LIVE context user, not a subscribe-time snapshot: #1414 re-auth + // updates subContext.user in place, so a role/claims change must be reflected per event. + decision = subAllowRead.call(event.value ?? null, subContext.user, request, subContext); } catch { return false; // fail closed: a throwing override drops the event } diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 561694d66..02f368dfa 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1653,19 +1653,18 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => } }); - it('async allowRead override keeps entry-check semantics (awaited, fail-closed), not record scoping', async () => { - // Record-scoping is sync-only: a declared-async override is excluded from the per-record - // deferral, so the authorize wrapper awaits its verdict at entry and fails closed on - // rejection (#1422 gap 1 contract, locked by integrationTests allowread-fail-closed). - class AsyncRejects extends T { + it('async allowRead override is evaluated at entry only (no per-record filtering), fail-closed on deny', async () => { + // Async overrides can't participate in sync per-record filtering — they run ONLY the awaited + // entry check (a table/connection-level decision, preserving the #1422 async fail-closed + // contract). A denying async override therefore denies the whole read. + class AsyncDenies extends T { async allowRead() { - throw new Error('async allowRead intentionally rejects'); + return false; // collection-scope deny } } await assert.rejects( async () => { - // the authorize wrapper awaits the async verdict, so search() returns a promise here - const iterable = await AsyncRejects.search( + const iterable = await AsyncDenies.search( { conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], checkPermission: true }, { user: { id: 1, role: { permission: {} } } } ); @@ -1809,6 +1808,63 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => ); }); + it('QUERY verb enforces record-scoped allowRead (checkPermission reaches the search target)', async () => { + // The QUERY verb calls resource.search(data, query) — search's target is `data` (the body), + // while checkPermission was set on the URL `query`. The query action threads it across so + // Table.search arms the per-record guard; without it a QUERY returned the full unfiltered set. + class Restricted extends T { + allowRead(user) { + if (this?.ownerId == null) return true; // collection scope: defer to per-record + return this.ownerId === user?.id; + } + } + const results = await fromAsync( + await Restricted.query( + { checkPermission: true, isCollection: true }, + { conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], select: ['id', 'ownerId'] }, + { user: { id: 1 } } + ) + ); + assert(results.length > 0, 'expected some visible rows'); + assert( + results.every((r) => r.ownerId === 1), + 'QUERY results must be row-filtered by the record-scoped allowRead' + ); + }); + + it('subscription delivery freezes the record before the override sees it', async () => { + let sawFrozen; + class FreezeProbe extends T { + allowRead(user) { + if (this?.ownerId == null) return true; + sawFrozen = Object.isFrozen(this); // capture whether delivery froze the record + return this.ownerId === user?.id; + } + } + const subscription = await FreezeProbe.subscribe( + { checkPermission: true, omitCurrent: true }, + { + user: { id: 1, username: 'u1', role: { permission: {} } }, + getContext() { + return this; + }, + } + ); + const collector = (async () => { + for await (const _ of subscription) { + /* drain */ + } + })(); + try { + await T.put(130, { group: 'blue', ownerId: 1, active: true, vector: [130, 0] }); + await new Promise((resolve) => setTimeout(resolve, 150)); + assert.strictEqual(sawFrozen, true, 'the record handed to allowRead during delivery must be frozen'); + } finally { + subscription.return?.(); + await Promise.race([collector, new Promise((resolve) => setTimeout(resolve, 100))]); + } + }); + it('records expose allowRead directly (RecordObject delegate, non-enumerable)', async () => { const results = await fromAsync( T.search({ conditions: [{ attribute: 'group', comparator: 'equals', value: 'blue' }], limit: 1 }, {}) From 338969abe71fc0fda6566968309001401bd22b0d Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 14 Jul 2026 12:25:25 -0600 Subject: [PATCH 11/12] test: subscription delivery re-evaluates against the live user (#1414 in-place re-auth) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- unitTests/resources/vectorIndex.test.js | 43 +++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index 02f368dfa..34f58eab1 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -1785,6 +1785,49 @@ describeUnlessLmdbFilter('HNSW filtered search via Table.search (#1241)', () => } }); + it('subscription delivery re-evaluates against the LIVE user (reflects #1414 in-place re-auth)', async () => { + // The override keys on a MUTABLE claim (tier), not a stable record field, so a re-auth that + // swaps context.user mid-stream must change the verdict. A snapshot regression would keep + // delivering with the pre-downgrade user and this test would catch it. + const context = { + user: { id: 1, username: 'u1', tier: 'gold', role: { permission: {} } }, + getContext() { + return this; + }, + }; + class TierGated extends T { + allowRead(user) { + if (this?.ownerId == null) return true; // collection scope: open + return user?.tier === 'gold'; // gate on a mutable claim + } + } + const subscription = await TierGated.subscribe({ checkPermission: true, omitCurrent: true }, context); + const received = []; + const collector = (async () => { + for await (const event of subscription) { + if (event.type !== 'end_txn') received.push(event); + } + })(); + try { + await T.put(140, { group: 'blue', ownerId: 1, active: true, vector: [140, 0] }); + await new Promise((resolve) => setTimeout(resolve, 150)); + assert( + received.some((e) => e.id === 140), + 'gold-tier user receives the event' + ); + // #1414 re-auth replaces context.user in place with the refreshed principal — here downgraded. + context.user = { id: 1, username: 'u1', tier: 'silver', role: { permission: {} } }; + const before = received.length; + await T.put(141, { group: 'blue', ownerId: 1, active: true, vector: [141, 0] }); + await new Promise((resolve) => setTimeout(resolve, 150)); + assert.strictEqual(received.length, before, 'after the live downgrade, further events are dropped'); + assert(!received.some((e) => e.id === 141), 'the post-downgrade event is not delivered'); + } finally { + subscription.return?.(); + await Promise.race([collector, new Promise((resolve) => setTimeout(resolve, 100))]); + } + }); + it('subscribe entry check still guards the connection grant (protects connection-level allowRead)', async () => { // subscribe is NOT deferred: the entry check runs the override as the connection grant, so a // connection-level override (e.g. an MQTT topic ACL) that denies is honored at subscribe time. From 5444791b69a5340eddb269ccfc0f43f1bc746fa0 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 15 Jul 2026 15:03:51 -0600 Subject: [PATCH 12/12] review(heskew): fix QUERY checkPermission bypass, freeze source-revalidation guard view, swallow rejected-thenable rejections, bound previousCount backfill scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../reject-thenable-allowread/config.yaml | 7 + .../reject-thenable-allowread/resources.js | 43 +++ .../reject-thenable-allowread/schema.graphql | 8 + ...uery-row-allowread-checkpermission.test.ts | 132 ++++++++ ...ed-thenable-allowread-no-unhandled.test.ts | 283 ++++++++++++++++++ resources/Resource.ts | 15 +- resources/Table.ts | 75 ++++- .../resources/frozen-record-view.test.js | 80 +++++ ...subscriptionPreviousCountScanBound.test.js | 101 +++++++ 9 files changed, 733 insertions(+), 11 deletions(-) create mode 100644 integrationTests/security/fixtures/reject-thenable-allowread/config.yaml create mode 100644 integrationTests/security/fixtures/reject-thenable-allowread/resources.js create mode 100644 integrationTests/security/fixtures/reject-thenable-allowread/schema.graphql create mode 100644 integrationTests/security/query-row-allowread-checkpermission.test.ts create mode 100644 integrationTests/security/rejected-thenable-allowread-no-unhandled.test.ts create mode 100644 unitTests/resources/frozen-record-view.test.js create mode 100644 unitTests/resources/subscriptionPreviousCountScanBound.test.js diff --git a/integrationTests/security/fixtures/reject-thenable-allowread/config.yaml b/integrationTests/security/fixtures/reject-thenable-allowread/config.yaml new file mode 100644 index 000000000..0a4eb2e94 --- /dev/null +++ b/integrationTests/security/fixtures/reject-thenable-allowread/config.yaml @@ -0,0 +1,7 @@ +# Regression fixture for #1786 — a sync-declared allowRead that returns a rejected thenable +# during per-record evaluation must not leak an unhandledRejection. +graphqlSchema: + files: '*.graphql' +jsResource: + files: resources.js +rest: true diff --git a/integrationTests/security/fixtures/reject-thenable-allowread/resources.js b/integrationTests/security/fixtures/reject-thenable-allowread/resources.js new file mode 100644 index 000000000..5e2a74762 --- /dev/null +++ b/integrationTests/security/fixtures/reject-thenable-allowread/resources.js @@ -0,0 +1,43 @@ +// Regression fixture for #1786 — a sync-declared (no `async` keyword) allowRead that returns a +// REJECTED thenable during per-record evaluation. Before the fix, neither the query-traversal +// guard nor the subscription delivery filter attached a handler to that rejection before +// discarding it, so each denied candidate/event reached Harper's global unhandledRejection +// logger. The fix attaches a no-op handler while keeping the fail-closed decision and the +// existing one-time diagnostic warning. + +function isSuper(user) { + return !!user?.role?.permission?.super_user; +} + +// Owner marking a record whose per-record allowRead evaluation always rejects. Kept distinct +// from real owners so a normal, allowed record can also be present — an SSE subscription whose +// EVERY record is denied never writes a byte, so headers wouldn't flush and the test client +// would hang waiting for a response that never comes. This isn't a Harper bug to work around in +// the test; it just means the fixture needs at least one deliverable record. +const REJECTING_OWNER = 'REJECT_SENTINEL'; + +export class Rejecting extends tables.Rejecting { + allowRead(user, target, context) { + if (isSuper(user)) return true; + if (!super.allowRead(user, target, context)) return false; + const owner = this?.owner; + // Collection scope (subscribe entry check) / no loaded record: allow the connection to open. + if (owner === undefined || owner === null) return true; + if (owner === REJECTING_OWNER) { + // Per-record (query traversal, subscription event delivery): sync-declared but returns a + // rejected thenable — must fail closed WITHOUT leaking an unhandledRejection. + return Promise.reject(new Error('allowRead intentionally rejects during per-record evaluation (#1786)')); + } + return owner === user?.username; + } + + allowUpdate(user) { + return isSuper(user); + } + allowCreate(user) { + return isSuper(user); + } + allowDelete(user) { + return isSuper(user); + } +} diff --git a/integrationTests/security/fixtures/reject-thenable-allowread/schema.graphql b/integrationTests/security/fixtures/reject-thenable-allowread/schema.graphql new file mode 100644 index 000000000..a4ccb121e --- /dev/null +++ b/integrationTests/security/fixtures/reject-thenable-allowread/schema.graphql @@ -0,0 +1,8 @@ +# Regression fixture for #1786 — a sync-declared allowRead that returns a rejected thenable +# during per-record evaluation must not leak an unhandledRejection (query traversal guard and +# subscription delivery filter share this same override). +type Rejecting @table @export { + id: ID @primaryKey + owner: String + secret: String +} diff --git a/integrationTests/security/query-row-allowread-checkpermission.test.ts b/integrationTests/security/query-row-allowread-checkpermission.test.ts new file mode 100644 index 000000000..f0db4b597 --- /dev/null +++ b/integrationTests/security/query-row-allowread-checkpermission.test.ts @@ -0,0 +1,132 @@ +/** + * #1786 review (heskew) — QUERY's body must not be able to disable the row-level `allowRead` + * guard. + * + * `Resource.query` threads `checkPermission` from the framework-controlled URL `query` onto the + * client-controlled request `data` (the QUERY body) so `Table.search` sees the flag and enforces + * the record-scoped `allowRead` override (#1422 gap 2). Before the fix, that thread-through only + * filled `data.checkPermission` when it was nullish — a client sending `checkPermission: false` + * in the QUERY body survived untouched and disabled the row-level guard entirely, returning the + * full unfiltered set. + * + * Fixture: reuses the `Vault` table from `subscription-row-allowread` (owner-scoped allowRead, + * composes the base RBAC grant via `super.allowRead`). + * + * Reproduction: + * npm run test:integration -- "integrationTests/security/query-row-allowread-checkpermission.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { deepStrictEqual, ok } from 'node:assert'; +import { resolve } from 'node:path'; + +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +// @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine +import { createApiClient, createHeaders } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/subscription-row-allowread'); +const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; + +const ALICE = { username: 'query_allowread_alice', password: 'Alice-pw-1786!' }; +const BOB = { username: 'query_allowread_bob', password: 'Bobby-pw-1786!' }; +const ROLE = 'query_allowread_role'; + +const ALICE_ROWS = ['qrow-a1', 'qrow-a2']; +const BOB_ROWS = ['qrow-b1', 'qrow-b2']; + +/** + * Issue an HTTP QUERY request via fetch — supertest/superagent has no API for non-standard + * verbs, while undici's fetch passes custom method tokens through. + */ +async function queryVault(restURL: string, headers: Record, body: any): Promise { + const resp = await fetch(`${restURL}/Vault/`, { + method: 'QUERY', + headers: { ...headers, 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); + const text = await resp.text(); + ok(resp.status === 200, `QUERY /Vault/ returned ${resp.status}: ${text}`); + const data = JSON.parse(text); + return Array.isArray(data) ? data : []; +} + +suite( + '#1786 QUERY body checkPermission cannot bypass row-level allowRead', + { skip: skipSuite }, + (ctx: ContextWithHarper) => { + let client: ReturnType; + let restURL = ''; + let aliceHeaders: Record; + + before(async () => { + await setupHarperWithFixture(ctx, FIXTURE_PATH, { config: {}, env: {} }); + client = createApiClient(ctx.harper); + restURL = ctx.harper.httpURL; + + const deadline = Date.now() + 30_000; + while (Date.now() < deadline) { + try { + const probe = await client.reqRest('/Vault/').timeout(3000); + if (probe.status !== 404) break; + } catch { + /* not ready */ + } + await new Promise((r) => setTimeout(r, 250)); + } + + await client + .req() + .send({ + operation: 'add_role', + role: ROLE, + permission: { + super_user: false, + data: { + tables: { + Vault: { read: true, insert: true, update: true, delete: true, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + + for (const u of [ALICE, BOB]) { + await client + .req() + .send({ operation: 'add_user', role: ROLE, username: u.username, password: u.password, active: true }) + .expect(200); + } + + const records = [ + ...ALICE_ROWS.map((id) => ({ id, owner: ALICE.username, secret: `alice-secret-${id}` })), + ...BOB_ROWS.map((id) => ({ id, owner: BOB.username, secret: `bob-secret-${id}` })), + ]; + await client.req().send({ operation: 'insert', schema: 'data', table: 'Vault', records }).expect(200); + + aliceHeaders = createHeaders(ALICE.username, ALICE.password); + }); + + after(async () => { + await teardownHarper(ctx); + }); + + test('CONTROL: a plain QUERY only returns the requesting user’s own rows', async () => { + const results = await queryVault(restURL, aliceHeaders, {}); + const ids = results.map((r: any) => r.id).sort(); + deepStrictEqual(ids, [...ALICE_ROWS].sort(), 'row-level allowRead should scope a QUERY to the caller’s own rows'); + }); + + test('BYPASS ATTEMPT: checkPermission: false in the QUERY body must not disable the guard', async () => { + const results = await queryVault(restURL, aliceHeaders, { checkPermission: false }); + const ids = results.map((r: any) => r.id).sort(); + ok( + !BOB_ROWS.some((id) => ids.includes(id)), + `QUERY-VERB BYPASS (#1786): client-supplied checkPermission:false leaked Bob's rows: ${JSON.stringify(ids)}` + ); + deepStrictEqual( + ids, + [...ALICE_ROWS].sort(), + 'a client-supplied checkPermission:false must not widen the QUERY result beyond the caller’s own rows' + ); + }); + } +); diff --git a/integrationTests/security/rejected-thenable-allowread-no-unhandled.test.ts b/integrationTests/security/rejected-thenable-allowread-no-unhandled.test.ts new file mode 100644 index 000000000..08b781536 --- /dev/null +++ b/integrationTests/security/rejected-thenable-allowread-no-unhandled.test.ts @@ -0,0 +1,283 @@ +/** + * #1786 review (heskew) — a rejected thenable returned by a sync-declared `allowRead` must not + * leak an `unhandledRejection`. + * + * The record-scoped `allowRead` guard (#1422 gap 2) denies a record when a SYNC-declared + * override returns a thenable (it can't be awaited mid-traversal, so it fails closed and warns + * once). Before the fix, that rejected thenable was discarded without a handler attached, so + * every denied candidate reached Harper's global `unhandledRejection` logger — once per + * candidate for the query-traversal guard, and once per dropped event for the parallel + * subscription `allowsEvent` delivery filter. + * + * Fixture: `Rejecting` — a sync `allowRead` that grants the collection-scope entry check (no + * loaded record) and a normal owner-match boolean per record, EXCEPT for a record owned by the + * `REJECT_SENTINEL` marker, which always returns `Promise.reject(...)`. The marker keeps the + * fixture from denying every record (a subscription with nothing ever deliverable never writes a + * byte, so its response headers never flush — a client artifact, not the thing under test) while + * still exercising the rejected-thenable path on both the query guard and the subscription filter. + * + * Reproduction: + * npm run test:integration -- "integrationTests/security/rejected-thenable-allowread-no-unhandled.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { ok, deepStrictEqual } from 'node:assert'; +import { resolve, join } from 'node:path'; +import { readFile } from 'node:fs/promises'; +import { setTimeout as sleep } from 'node:timers/promises'; +import http from 'node:http'; +import https from 'node:https'; +import { URL } from 'node:url'; + +import request from 'supertest'; + +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +// @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine +import { createApiClient, createHeaders } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/reject-thenable-allowread'); +const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; + +const USER = { username: 'reject_thenable_user', password: 'Reject-pw-1786!' }; +const ROLE = 'reject_thenable_role'; +const OWN_ROW = 'reject-own'; // owned by USER — a normal, allowed record (proves the guard isn't fail-open) +const TRIGGER_ROW = 'reject-trigger'; // owner is the REJECT_SENTINEL marker — always rejects + +const UNHANDLED_REJECTION_LOG = /unhandledRejection/i; +const QUERY_DIAGNOSTIC_LOG = /allowRead on Rejecting returned a promise during per-record evaluation/; +const EVENT_DIAGNOSTIC_LOG = /allowRead on Rejecting returned a promise during subscription event evaluation/; + +async function readLog(logDir: string): Promise { + try { + return await readFile(join(logDir, 'hdb.log'), 'utf8'); + } catch { + return ''; + } +} + +async function waitForLogMatch(logDir: string, pattern: RegExp, timeoutMs = 10_000): Promise { + const deadline = Date.now() + timeoutMs; + let contents = ''; + while (Date.now() < deadline) { + contents = await readLog(logDir); + if (pattern.test(contents)) return contents; + await sleep(150); + } + return contents; +} + +async function queryRejecting(restURL: string, headers: Record, body: any): Promise { + const resp = await fetch(`${restURL}/Rejecting/`, { + method: 'QUERY', + headers: { ...headers, 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); + const text = await resp.text(); + ok(resp.status === 200, `QUERY /Rejecting/ returned ${resp.status}: ${text}`); + const data = JSON.parse(text); + return Array.isArray(data) ? data : []; +} + +interface SseStream { + events: string[]; + status: number; + destroy: () => void; +} + +function openSse(urlStr: string, headers: Record): Promise { + const url = new URL(urlStr); + const lib = url.protocol === 'https:' ? https : http; + const events: string[] = []; + let buffer = ''; + return new Promise((resolvePromise, reject) => { + const req = lib.request( + url, + { method: 'GET', headers: { ...headers, Accept: 'text/event-stream' }, rejectUnauthorized: false } as any, + (res) => { + const stream: SseStream = { + events, + status: res.statusCode ?? 0, + destroy: () => { + res.destroy(); + req.destroy(); + }, + }; + res.setEncoding('utf8'); + res.on('data', (chunk: string) => { + buffer += chunk; + let sep: number; + while ((sep = buffer.indexOf('\n\n')) >= 0) { + events.push(buffer.slice(0, sep)); + buffer = buffer.slice(sep + 2); + } + }); + res.on('error', () => {}); + resolvePromise(stream); + } + ); + req.on('error', reject); + req.end(); + }); +} + +async function waitFor(predicate: () => boolean, timeoutMs = 8000, intervalMs = 50): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (predicate()) return true; + await sleep(intervalMs); + } + return predicate(); +} + +suite( + '#1786 rejected thenable from allowRead is handled without leaking unhandledRejection', + { skip: skipSuite }, + (ctx: ContextWithHarper) => { + let client: ReturnType; + let restURL = ''; + let logDir = ''; + let userHeaders: Record; + const openStreams = new Set(); + + before(async () => { + await setupHarperWithFixture(ctx, FIXTURE_PATH, { config: {}, env: {} }); + client = createApiClient(ctx.harper); + restURL = ctx.harper.httpURL; + logDir = ctx.harper.logDir ?? join(ctx.harper.dataRootDir, 'log'); + + const deadline = Date.now() + 30_000; + while (Date.now() < deadline) { + try { + const probe = await client.reqRest('/Rejecting/').timeout(3000); + if (probe.status !== 404) break; + } catch { + /* not ready */ + } + await sleep(250); + } + + await client + .req() + .send({ + operation: 'add_role', + role: ROLE, + permission: { + super_user: false, + data: { + tables: { + Rejecting: { read: true, insert: true, update: true, delete: true, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + + await client + .req() + .send({ operation: 'add_user', role: ROLE, username: USER.username, password: USER.password, active: true }) + .expect(200); + + await client + .req() + .send({ + operation: 'insert', + schema: 'data', + table: 'Rejecting', + records: [ + { id: OWN_ROW, owner: USER.username, secret: 'owner-secret' }, + { id: TRIGGER_ROW, owner: 'REJECT_SENTINEL', secret: 'trigger-secret' }, + ], + }) + .expect(200); + + // SSE uses a bearer token (not the Basic-auth header) — same as subscription-row-allowread's + // setup — for real per-connection auth rather than the header-less AUTHORIZELOCAL loopback. + const tokenResp = await client + .req() + .send({ operation: 'create_authentication_tokens', username: USER.username, password: USER.password }); + const bearer = + tokenResp.status === 200 && tokenResp.body?.operation_token + ? `Bearer ${tokenResp.body.operation_token}` + : createHeaders(USER.username, USER.password).Authorization; + userHeaders = { ...createHeaders(USER.username, USER.password), Authorization: bearer }; + }); + + after(async () => { + for (const s of openStreams) { + try { + s.destroy(); + } catch { + /* ignore */ + } + } + openStreams.clear(); + await teardownHarper(ctx); + }); + + test('QUERY: a rejected thenable denies its record — others still return — without an unhandledRejection', async () => { + const results = await queryRejecting(restURL, userHeaders, {}); + const ids = results.map((r: any) => r.id).sort(); + deepStrictEqual( + ids, + [OWN_ROW], + 'the rejected-thenable record must fail closed (deny) while the normal owned record still returns' + ); + + const withDiagnostic = await waitForLogMatch(logDir, QUERY_DIAGNOSTIC_LOG, 8000); + ok( + QUERY_DIAGNOSTIC_LOG.test(withDiagnostic), + 'expected the one-time "returned a promise during per-record evaluation" diagnostic to be logged' + ); + + // Give any unobserved rejection time to surface before asserting its absence. + await sleep(1000); + const contents = await readLog(logDir); + ok( + !UNHANDLED_REJECTION_LOG.test(contents), + `UNHANDLED REJECTION LEAK (#1786): the query-traversal guard's rejected thenable reached the global logger:\n${contents.slice(-2000)}` + ); + }); + + test('SUBSCRIBE: a rejected thenable drops its event — others still deliver — without an unhandledRejection', async () => { + const stream = await openSse(`${restURL}/Rejecting/`, { Authorization: userHeaders.Authorization }); + openStreams.add(stream); + ok( + stream.status >= 200 && stream.status < 300, + `collection subscribe should open (entry grants), got ${stream.status}` + ); + + // Positive control: the normal owned record delivers (proves the connection is alive and + // events flow at all, so a later absence of the trigger row is a real filter, not a hang). + await request(restURL) + .put(`/Rejecting/${OWN_ROW}`) + .set(client.headers) + .send({ id: OWN_ROW, owner: USER.username, secret: 'owner-secret-updated' }) + .expect(204); + ok( + await waitFor(() => stream.events.some((frame) => frame.includes('owner-secret-updated'))), + 'POSITIVE CONTROL FAILED: the normal owned record was not delivered on the subscription' + ); + + await request(restURL) + .put(`/Rejecting/${TRIGGER_ROW}`) + .set(client.headers) + .send({ id: TRIGGER_ROW, owner: 'REJECT_SENTINEL', secret: 'trigger-secret-updated' }) + .expect(204); + await sleep(1500); // give the (denied) event delivery a chance to arrive if it leaked + + const leaked = stream.events.some((frame) => frame.includes('trigger-secret-updated')); + ok(!leaked, "ROW-LEVEL LEAK: the rejected-thenable-denied event's record was delivered to the subscriber"); + + const withDiagnostic = await waitForLogMatch(logDir, EVENT_DIAGNOSTIC_LOG, 8000); + ok( + EVENT_DIAGNOSTIC_LOG.test(withDiagnostic), + 'expected the one-time "returned a promise during subscription event evaluation" diagnostic to be logged' + ); + + const contents = await readLog(logDir); + ok( + !UNHANDLED_REJECTION_LOG.test(contents), + `UNHANDLED REJECTION LEAK (#1786): the subscription allowsEvent filter's rejected thenable reached the global logger:\n${contents.slice(-2000)}` + ); + }); + } +); diff --git a/resources/Resource.ts b/resources/Resource.ts index 9dcb3aadf..654c5e724 100644 --- a/resources/Resource.ts +++ b/resources/Resource.ts @@ -289,14 +289,13 @@ export class Resource implements ResourceInterface< // Table.search sees the flag and enforces row-level allowRead — otherwise a QUERY on a table // with an overridden allowRead returns the full unfiltered set (the deferral skips the entry // check on the promise that search consumes checkPermission, which it never sees). - if ( - resource.constructor.loadAsInstance !== false && - data && - typeof data === 'object' && - (query as any)?.checkPermission != null && - (data as any).checkPermission == null - ) { - (data as any).checkPermission = (query as any).checkPermission; + // checkPermission is framework-owned: `data` is the client-controlled QUERY body, so it must + // always be overwritten here (not just filled when nullish) — otherwise a client could send + // `checkPermission: false` in the body to disable Table.search's row-level allowRead guard. + if (resource.constructor.loadAsInstance !== false && data && typeof data === 'object') { + const checkPermission = (query as any)?.checkPermission; + if (checkPermission == null) delete (data as any).checkPermission; + else (data as any).checkPermission = checkPermission; } return resource.search ? resource.constructor.loadAsInstance === false diff --git a/resources/Table.ts b/resources/Table.ts index feec7eed0..65476c89e 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -152,6 +152,43 @@ export function freezeRecord(value: any): void { if (value !== null && typeof value === 'object' && !ArrayBuffer.isView(value) && !(value instanceof ArrayBuffer)) Object.freeze(value); } +// Returns a read-only VIEW of `record` for evaluation (e.g. a record-scoped allowRead guard) +// without mutating the original. Locally-loaded records are already frozen by loadLocalRecord, so +// this is a no-op there; the source-revalidation path hands back the SAME object its deferred +// commit still writes to (createdAt/updatedAt) and persists, so that object can't be frozen +// directly. A write-blocking Proxy is used instead of a shallow copy: a copy would eagerly invoke +// every getter (breaking a caching table's lazy structPrototype decode) and silently drop +// non-enumerable properties like Array.length. Only ordinary objects/arrays are wrapped — Date, +// Map, Set, RegExp, etc. carry internal slots that throw "incompatible receiver" when their +// methods run against anything but the exact original instance (a Proxy included), so those are +// returned unwrapped rather than risk breaking an override that calls a method on one; a cached +// record CAN legitimately be one of these (getFromSource only requires `typeof === 'object'`). +export function frozenRecordView(record: any): any { + if (record === null || typeof record !== 'object' || ArrayBuffer.isView(record) || record instanceof ArrayBuffer) + return record; + if (Object.isFrozen(record)) return record; + const tag = Object.prototype.toString.call(record); + if (tag !== '[object Object]' && tag !== '[object Array]') return record; + return new Proxy(record, { + get(target, prop) { + // receiver = target (not this proxy) so a lazy-decode getter runs with `this` bound to + // the real record, matching its behavior when read directly. + return Reflect.get(target, prop, target); + }, + set() { + return false; + }, + defineProperty() { + return false; + }, + deleteProperty() { + return false; + }, + setPrototypeOf() { + return false; + }, + }); +} export const INVALIDATED = 1; export const EVICTED = 8; // note that 2 is reserved for timestamps const TEST_WRITE_KEY_BUFFER = Buffer.allocUnsafeSlow(8192); @@ -163,6 +200,12 @@ const REPLAY_YIELD_INTERVAL = 100; // yield to the event loop every N records du // buffer the entire backward chain per record, synchronously, on every worker — pinning the JS heap // until the worker OOMs (issue #1114). Beyond this depth we fall back to a bounded reconciliation. const MAX_OUT_OF_ORDER_AUDIT_DEPTH = 1000; +// Cap on audit records inspected while backfilling a subscription's `previousCount` history, +// independent of `count` (the number of AUTHORIZED events collected). `count` only decrements on +// records that pass the row-level allowRead filter, so an all-deny override would otherwise force +// a full walk of the retained audit log (#1786) — this bounds that walk regardless of how many +// records the filter accepts. +const MAX_PREVIOUS_COUNT_SCAN = 10_000; const FULL_PERMISSIONS = { read: true, insert: true, @@ -2900,7 +2943,11 @@ export function makeTable(options) { // A sync-declared override that returns a thenable can't be awaited mid-traversal: // deny the record (fail closed, #1422 gap 1 semantics) rather than fail open on // promise truthiness or abort the whole query. Declared-async overrides never get - // here — they keep the awaited entry check. + // here — they keep the awaited entry check. We're not observing this rejection, so + // attach a no-op handler — otherwise a rejected thenable reaches the global + // unhandledRejection logger once per denied candidate. Use `.then(undefined, ...)`, + // not `.catch` — a thenable is only guaranteed to have `.then`. + allowed.then(undefined, () => {}); if (!warnedAsync) { warnedAsync = true; logger.warn?.( @@ -3240,8 +3287,10 @@ export function makeTable(options) { if (record == null) return canSkip ? SKIP : record; // Record-level RBAC (#1241): enforced here because `record` is now the final, materialized // record — a caching table's source revalidation (above) may have replaced the local copy the - // query filters evaluated. Fail closed on the record actually being returned. - if (recordGuard && !recordGuard(record)) return canSkip ? SKIP : undefined; + // query filters evaluated. Fail closed on the record actually being returned. The guard sees a + // frozen view (frozenRecordView) so an allowRead override that writes through `this` can't + // mutate a record the source-revalidation path's deferred commit still needs to write and encode. + if (recordGuard && !recordGuard(frozenRecordView(record))) return canSkip ? SKIP : undefined; if (select && !(select[0] === '*' && select.length === 1)) { let promises: Promise[]; const selectAttribute = (attribute, callback) => { @@ -3446,6 +3495,10 @@ export function makeTable(options) { return false; // fail closed: a throwing override drops the event } if (decision != null && typeof decision.then === 'function') { + // Not observing this rejection — attach a no-op handler so it doesn't reach the + // global unhandledRejection logger once per dropped event. Use `.then(undefined, + // ...)`, not `.catch` — a thenable is only guaranteed to have `.then`. + decision.then(undefined, () => {}); if (!warnedAsyncEvent) { warnedAsyncEvent = true; logger.warn?.( @@ -3577,16 +3630,32 @@ export function makeTable(options) { } } else if (count) { const history = []; + let inspected = 0; // we are collecting the history in reverse order to get the right count, then reversing to send for (const auditRecord of auditStore.getRange({ start: 'z', end: false, reverse: true })) { if (++recordsSinceYield >= REPLAY_YIELD_INTERVAL) { recordsSinceYield = 0; await rest(); + // Subscription.end() nulls `subscriptions` on close; stop backfilling a dead + // subscriber rather than continuing to walk the retained audit log. + if (!subscription.subscriptions) break; } try { if (auditRecord.tableId !== tableId) continue; const id = auditRecord.recordId; if (thisId == null || isDescendantId(thisId, id)) { + // Bound entries INSPECTED for THIS scope, independent of `count` (entries + // AUTHORIZED) — an all-deny allowRead override must not force a full walk of + // the retained audit log (#1786), even though it returns fewer than `count` + // authorized events. Counted only once a record is known to be in scope (right + // table, right id) so unrelated cross-table/cross-scope audit traffic in a busy + // shared log can't spuriously cut a backfill short. + if (++inspected > MAX_PREVIOUS_COUNT_SCAN) { + logger.warn?.( + `previousCount backfill on ${tableName} stopped after inspecting ${MAX_PREVIOUS_COUNT_SCAN} in-scope audit records without collecting ${request.previousCount} authorized event(s); returning ${history.length} instead` + ); + break; + } const value = auditRecord.getValue(primaryStore, getFullRecord, auditRecord.localTime); const historyEntry = { id, diff --git a/unitTests/resources/frozen-record-view.test.js b/unitTests/resources/frozen-record-view.test.js new file mode 100644 index 000000000..7c5a04390 --- /dev/null +++ b/unitTests/resources/frozen-record-view.test.js @@ -0,0 +1,80 @@ +// #1786 review (heskew): the record-scoped allowRead guard must see a read-only view of a +// record on the caching-table source-revalidation path, WITHOUT freezing the actual object — +// that object is the SAME one the deferred commit still mutates (createdAt/updatedAt) and +// persists, so freezing it directly would break that write. +require('../testUtils'); +const assert = require('node:assert'); +const { frozenRecordView } = require('#src/resources/Table'); + +describe('frozenRecordView (#1786 source-revalidation guard view)', () => { + it('returns an already-frozen record unchanged (the common, already-safe local-read path)', () => { + const record = Object.freeze({ id: 1, secret: 'x' }); + assert.strictEqual(frozenRecordView(record), record); + }); + + it('blocks writes on an unfrozen record without freezing the original', () => { + const record = { id: 1, secret: 'x' }; + const view = frozenRecordView(record); + assert.notStrictEqual(view, record, 'must be a distinct view, not the original object'); + assert.equal(Object.isFrozen(record), false, 'the original object must remain mutable'); + + assert.throws(() => { + 'use strict'; + view.secret = 'mutated'; + }, TypeError); + assert.equal(record.secret, 'x', 'a write through the view must not reach the original'); + + // The commit path this guards must still be able to mutate the real object afterward. + record.updatedAt = 12345; + assert.equal(record.updatedAt, 12345); + }); + + it('reads pass through to the original, including getters bound to the real record', () => { + const record = { + id: 1, + get computed() { + // A lazy-decode getter (e.g. a caching table's structPrototype) must see `this` as the + // REAL record, not the view, or it can't reach its own backing state. + return this === record ? 'real' : 'wrong-this'; + }, + }; + const view = frozenRecordView(record); + assert.equal(view.id, 1); + assert.equal(view.computed, 'real'); + }); + + it('leaves an Array root wrapped and read-only without eagerly copying (preserves .length)', () => { + const record = [1, 2, 3]; + const view = frozenRecordView(record); + assert.equal(view.length, 3); + assert.equal(view[0], 1); + assert.throws(() => { + 'use strict'; + view.push(4); + }); + assert.deepEqual(record, [1, 2, 3], 'the original array must be untouched'); + }); + + it('does not freeze/wrap Date/Map/Set roots — internal-slot types can legitimately be a cached record', () => { + const date = new Date(0); + const map = new Map([['a', 1]]); + const set = new Set([1, 2]); + assert.strictEqual(frozenRecordView(date), date); + assert.strictEqual(frozenRecordView(map), map); + assert.strictEqual(frozenRecordView(set), set); + // Confirms these are returned genuinely unwrapped and still fully usable — a Proxy or copy + // would throw "incompatible receiver" on these method calls. + assert.doesNotThrow(() => date.getTime()); + assert.doesNotThrow(() => map.get('a')); + assert.doesNotThrow(() => set.has(1)); + }); + + it('is a harmless no-op on null / undefined / primitives / binary roots', () => { + assert.doesNotThrow(() => frozenRecordView(null)); + assert.doesNotThrow(() => frozenRecordView(undefined)); + assert.equal(frozenRecordView(42), 42); + assert.equal(frozenRecordView('a string'), 'a string'); + const buf = Buffer.from([1, 2, 3]); + assert.strictEqual(frozenRecordView(buf), buf); + }); +}); diff --git a/unitTests/resources/subscriptionPreviousCountScanBound.test.js b/unitTests/resources/subscriptionPreviousCountScanBound.test.js new file mode 100644 index 000000000..7e7fe62b5 --- /dev/null +++ b/unitTests/resources/subscriptionPreviousCountScanBound.test.js @@ -0,0 +1,101 @@ +require('../testUtils'); +const assert = require('assert'); +const { setTimeout: delay } = require('timers/promises'); +const { setupTestDBPath } = require('../testUtils'); +const { table } = require('#src/resources/databases'); +const { setMainIsWorker } = require('#js/server/threads/manageThreads'); +const { logger } = require('#src/utility/logging/logger'); +require('#src/server/serverHelpers/serverUtilities'); + +// #1786 review (heskew): a subscription's `previousCount` history backfill filters denied rows +// BEFORE decrementing `count` (the AUTHORIZED-event budget), so an all-deny row-level allowRead +// override removes count's work bound entirely — the scan would otherwise walk the full retained +// audit log. Table.ts now also bounds entries INSPECTED, independent of `count`, and logs once +// when that independent cap is what stopped the scan. +// +// The count branch uses `start: 'z'` for reverse audit log iteration, which is an lmdb-specific +// encoding ('z' compares above numeric keys) — same pre-existing rocksdb incompatibility noted in +// subscriptionReplay.test.js. +const isLMDB = process.env.HARPER_STORAGE_ENGINE === 'lmdb'; + +describe('Subscription previousCount backfill scan bound (#1786)', () => { + if (!isLMDB) return; + + let ScanBoundTable; + let warnings; + let originalWarn; + + before(async function () { + this.timeout(0); + setupTestDBPath(); + setMainIsWorker(true); + ScanBoundTable = table({ + table: 'SubPrevCountScanBound', + database: 'test', + attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'name' }], + audit: true, + }); + // A record-scoped, sync, ALWAYS-DENYING override. Table.prototype.subscribe only gates its + // row-level filter on `request.rowLevelAuthChecked` and this not being the framework default + // — it doesn't require going through the full Resource.subscribe entry-check wrapper, so + // setting rowLevelAuthChecked directly on the request (below) is sufficient to arm it here. + ScanBoundTable.prototype.allowRead = function () { + return false; + }; + }); + + beforeEach(function () { + warnings = []; + originalWarn = logger.warn; + logger.warn = (...args) => warnings.push(args); + }); + + afterEach(function () { + logger.warn = originalWarn; + }); + + it('an all-deny override stops the backfill at the inspected-entry cap, not a full log walk', async function () { + this.timeout(0); + // #1786's MAX_PREVIOUS_COUNT_SCAN cap in Table.ts — kept in sync here rather than exported, + // matching this file's other magic numbers being test-local constants. + const MAX_PREVIOUS_COUNT_SCAN = 10_000; + const N = MAX_PREVIOUS_COUNT_SCAN + 250; // comfortably past the cap + for (let i = 0; i < N; i++) { + await ScanBoundTable.put(i, { name: 'v' + i }); + } + + const start = Date.now(); + const subscription = await ScanBoundTable.subscribe({ + previousCount: 5, + isCollection: true, + rowLevelAuthChecked: true, + }); + const events = []; + subscription.on('data', (e) => events.push(e)); + // Every candidate is denied, so nothing will ever arrive — wait on the diagnostic instead of + // a data event. + const deadline = Date.now() + 30_000; + while (warnings.length === 0 && Date.now() < deadline) { + await delay(20); + } + const elapsedMs = Date.now() - start; + subscription.return?.(); + + assert.equal(events.length, 0, `an all-deny override must not deliver any history events, got ${events.length}`); + assert.ok( + warnings.length > 0, + 'expected the "stopped after inspecting" diagnostic to fire — the scan either walked the whole log or never bounded' + ); + const [message] = warnings[0]; + assert.match(message, /stopped after inspecting 10000 in-scope audit records/); + assert.match(message, /returning 0 instead/); + // Loose bound: proves the scan didn't walk all N (250 more than the cap) — not a tight perf + // assertion, just a sanity check that inspecting ~10k in-memory audit entries and bailing is + // fast, rather than the many additional seconds a full walk of the (still-growing) log would + // take on a slower CI runner. + assert.ok( + elapsedMs < 10_000, + `backfill took ${elapsedMs}ms — expected it to stop well before scanning all ${N} records` + ); + }); +});