From 9ea876f2ba8d4a2dd7f202ff5794d8eb69216a41 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 20 Aug 2026 14:32:12 +0200 Subject: [PATCH 01/10] fix(agent): serve only the columns of collections the caller may read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A read was permission-checked on the root collection alone. Every column a projection, a filter or a sort reached through a relation path came back unchecked, so a role with `read` on `cards` and nothing on `holders` could ask for `holder:nationalId` — and get it. The filter is the sharper half: it never returns the column, but one row back or zero rows back answers a `starts_with` guess, which recovers the value character by character. Check the collection each path *ends* on. Collections crossed on the way confer and require nothing: reaching one through a relation is a join, not a read, so `account:organization:name` needs `read` on `organizations` alone. That also keeps a ManyToMany through-collection out of it, which contributes no returned column. What happens on a denial depends on who asked for the field: - named by the caller, through `fields[]` or `Forest-Projection` — refused, with every offending path in one message so a client drops them all and retries once; - never asked for — dropped from the projection. `ProjectionFactory.all` expands every column of every to-one relation when no `fields[]` is sent, so refusing would turn an ordinary listing into a 403. Filters, sorts, extended searches and a chart's group-by or aggregated field are always refused. They have no prunable equivalent — dropping a condition widens the result set, dropping a sort clause silently reorders it, and a grouped-by key is chart output. A leaderboard counting a relation is the one aggregation no path describes: its value traces back to no field, so the rule above sees nothing to check. What it exposes is the cardinality of the related collection, which is what `browse` governs on `/forest//count` and on `/relationships//count` — so assert that, on the foreign collection rather than on the through-collection a ManyToMany aggregates. The check reads the caller's own query only. Scopes and segments are injected by the agent and may legitimately reference a collection the caller cannot read. Covers get, list, csv, list-related, csv-related, count, count-related and the chart routes. Co-Authored-By: Claude Opus 5 --- packages/agent/src/routes/access/chart.ts | 58 ++- .../agent/src/routes/access/count-related.ts | 2 + packages/agent/src/routes/access/count.ts | 2 + .../agent/src/routes/access/csv-related.ts | 11 +- packages/agent/src/routes/access/csv.ts | 11 +- packages/agent/src/routes/access/get.ts | 6 +- .../agent/src/routes/access/list-related.ts | 6 +- packages/agent/src/routes/access/list.ts | 7 +- .../services/authorization/authorization.ts | 122 +++++- packages/agent/src/utils/csv-generator.ts | 10 + packages/agent/src/utils/field-path.ts | 22 + packages/agent/src/utils/query-string.ts | 22 +- .../authorization/authorization.ts | 6 + .../security/related-read-permissions.test.ts | 388 ++++++++++++++++++ .../authorization/authorization.test.ts | 106 +++++ .../agent/test/utils/csv-generator.test.ts | 44 ++ packages/agent/test/utils/field-path.test.ts | 80 ++++ .../agent/test/utils/query-string.test.ts | 30 +- 18 files changed, 913 insertions(+), 20 deletions(-) create mode 100644 packages/agent/src/utils/field-path.ts create mode 100644 packages/agent/test/security/related-read-permissions.test.ts create mode 100644 packages/agent/test/utils/field-path.test.ts diff --git a/packages/agent/src/routes/access/chart.ts b/packages/agent/src/routes/access/chart.ts index ebea91fc42..5ea89ffede 100644 --- a/packages/agent/src/routes/access/chart.ts +++ b/packages/agent/src/routes/access/chart.ts @@ -1,5 +1,6 @@ import type { Caller, + Collection, ConditionTreeBranch, DateOperation, Filter, @@ -29,6 +30,7 @@ import { DateTime } from 'luxon'; import { v1 as uuidv1 } from 'uuid'; import ContextFilterFactory from '../../utils/context-filter-factory'; +import FieldPathUtils from '../../utils/field-path'; import QueryStringParser from '../../utils/query-string'; import CollectionRoute from '../collection-route'; @@ -67,6 +69,8 @@ export default class ChartRoute extends CollectionRoute { chartRequest, }); + await this.services.authorization.assertCanReadQueryFields(context, this.collection); + switch (chartRequest.type) { case ChartType.Value: return this.makeValueChart(context); @@ -120,6 +124,11 @@ export default class ChartRoute extends CollectionRoute { aggregateFieldName: aggregateField, } = context.request.body; + await this.assertCanReadAggregatedFields(context, this.collection, [ + ['group a chart by', groupByField], + ['aggregate a chart on', aggregateField], + ]); + const rows = await this.collection.aggregate( QueryStringParser.parseCaller(context), await this.getFilter(context), @@ -144,6 +153,11 @@ export default class ChartRoute extends CollectionRoute { timeRange, } = context.request.body; + await this.assertCanReadAggregatedFields(context, this.collection, [ + ['group a chart by', groupByDateField], + ['aggregate a chart on', aggregateField], + ]); + const filter = await this.getFilter(context); const filterOnlyWithValues = filter.override({ conditionTree: ConditionTreeFactory.intersect( @@ -233,9 +247,25 @@ export default class ChartRoute extends CollectionRoute { } if (collection && filter && aggregation) { - const rows = await this.dataSource - .getCollection(collection) - .aggregate(QueryStringParser.parseCaller(context), filter, aggregation, Number(body.limit)); + const aggregatedCollection = this.dataSource.getCollection(collection); + + await this.assertCanReadAggregatedFields(context, aggregatedCollection, [ + ['group a leaderboard by', aggregation.groups[0].field], + ['aggregate a leaderboard on', aggregation.field], + ]); + + // A count exposes the cardinality of the relation, which `/relationships//count` puts + // behind `browse`. + if (!aggregation.field) { + await this.services.authorization.assertCanBrowse(context, field.foreignCollection); + } + + const rows = await aggregatedCollection.aggregate( + QueryStringParser.parseCaller(context), + filter, + aggregation, + Number(body.limit), + ); return rows.map(row => ({ key: row.group[aggregation.groups[0].field] as string, @@ -254,6 +284,10 @@ export default class ChartRoute extends CollectionRoute { ); const aggregation = new Aggregation({ operation: aggregator, field: aggregateField }); + await this.assertCanReadAggregatedFields(context, this.collection, [ + ['aggregate a chart on', aggregateField], + ]); + const rows = await this.collection.aggregate( QueryStringParser.parseCaller(context), filter, @@ -263,6 +297,24 @@ export default class ChartRoute extends CollectionRoute { return rows.length ? (rows[0].value as number) : 0; } + private async assertCanReadAggregatedFields( + context: Context, + collection: Collection, + fields: Array<[action: string, path: string]>, + ): Promise { + await this.services.authorization.assertCanReadUsages( + context, + this.collection.name, + fields + .filter(([, path]) => path) + .map(([action, path]) => ({ + action, + path, + collectionName: FieldPathUtils.getLeafCollection(collection, path).name, + })), + ); + } + private async getFilter(context: Context): Promise { const scope = await this.services.authorization.getScope(this.collection, context); diff --git a/packages/agent/src/routes/access/count-related.ts b/packages/agent/src/routes/access/count-related.ts index e52f4413e3..05490589fc 100644 --- a/packages/agent/src/routes/access/count-related.ts +++ b/packages/agent/src/routes/access/count-related.ts @@ -20,6 +20,8 @@ export default class CountRelatedRoute extends RelationRoute { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); if (this.foreignCollection.schema.countable) { + await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection); + const parentId = IdUtils.unpackId(this.collection.schema, context.params.parentId); const scope = await this.services.authorization.getScope(this.foreignCollection, context); const caller = QueryStringParser.parseCaller(context); diff --git a/packages/agent/src/routes/access/count.ts b/packages/agent/src/routes/access/count.ts index b312b440b9..d9a3c3508d 100644 --- a/packages/agent/src/routes/access/count.ts +++ b/packages/agent/src/routes/access/count.ts @@ -16,6 +16,8 @@ export default class CountRoute extends CollectionRoute { await this.services.authorization.assertCanBrowse(context, this.collection.name); if (this.collection.schema.countable) { + await this.services.authorization.assertCanReadQueryFields(context, this.collection); + const scope = await this.services.authorization.getScope(this.collection, context); const caller = QueryStringParser.parseCaller(context); let filter = ContextFilterFactory.build(this.collection, context, scope); diff --git a/packages/agent/src/routes/access/csv-related.ts b/packages/agent/src/routes/access/csv-related.ts index b25699877a..ef5325cfb4 100644 --- a/packages/agent/src/routes/access/csv-related.ts +++ b/packages/agent/src/routes/access/csv-related.ts @@ -23,13 +23,20 @@ export default class CsvRelatedRoute extends RelationRoute { async handleRelatedCsv(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); await this.services.authorization.assertCanExport(context, this.foreignCollection.name); + await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection); - const { header } = context.request.query as Record; + const { header: requestedHeader } = context.request.query as Record; - const projection = QueryStringParser.parseProjectionFromHeaderOrQuery( + const requested = QueryStringParser.parseProjectionFromHeaderOrQuery( this.foreignCollection, context, ); + const projection = await this.services.authorization.redactProjection( + context, + this.foreignCollection, + requested, + ); + const header = CsvGenerator.filterHeader(requestedHeader, requested.projection, projection); const scope = await this.services.authorization.getScope(this.foreignCollection, context); const caller = QueryStringParser.parseCaller(context); const filter = ContextFilterFactory.buildPaginated(this.foreignCollection, context, scope); diff --git a/packages/agent/src/routes/access/csv.ts b/packages/agent/src/routes/access/csv.ts index 47fd5f685c..02eb795465 100644 --- a/packages/agent/src/routes/access/csv.ts +++ b/packages/agent/src/routes/access/csv.ts @@ -17,10 +17,17 @@ export default class CsvRoute extends CollectionRoute { async handleCsv(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.collection.name); await this.services.authorization.assertCanExport(context, this.collection.name); + await this.services.authorization.assertCanReadQueryFields(context, this.collection); - const { header } = context.request.query as Record; + const { header: requestedHeader } = context.request.query as Record; - const projection = QueryStringParser.parseProjectionFromHeaderOrQuery(this.collection, context); + const requested = QueryStringParser.parseProjectionFromHeaderOrQuery(this.collection, context); + const projection = await this.services.authorization.redactProjection( + context, + this.collection, + requested, + ); + const header = CsvGenerator.filterHeader(requestedHeader, requested.projection, projection); const scope = await this.services.authorization.getScope(this.collection, context); const caller = QueryStringParser.parseCaller(context); const filter = ContextFilterFactory.buildPaginated(this.collection, context, scope); diff --git a/packages/agent/src/routes/access/get.ts b/packages/agent/src/routes/access/get.ts index 99a64defd0..fe189577d8 100644 --- a/packages/agent/src/routes/access/get.ts +++ b/packages/agent/src/routes/access/get.ts @@ -24,7 +24,11 @@ export default class GetRoute extends CollectionRoute { ), }); - const projection = QueryStringParser.parseProjectionFromHeaderOrQuery(this.collection, context); + const projection = await this.services.authorization.redactProjection( + context, + this.collection, + QueryStringParser.parseProjectionFromHeaderOrQuery(this.collection, context), + ); const records = await this.collection.list( QueryStringParser.parseCaller(context), diff --git a/packages/agent/src/routes/access/list-related.ts b/packages/agent/src/routes/access/list-related.ts index 9e0c0e0781..f12a17533a 100644 --- a/packages/agent/src/routes/access/list-related.ts +++ b/packages/agent/src/routes/access/list-related.ts @@ -18,6 +18,7 @@ export default class ListRelatedRoute extends RelationRoute { public async handleListRelated(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); + await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection); const parentId = IdUtils.unpackId(this.collection.schema, context.params.parentId); const scope = await this.services.authorization.getScope(this.foreignCollection, context); @@ -27,9 +28,10 @@ export default class ListRelatedRoute extends RelationRoute { scope, ); - const projection = QueryStringParser.parseProjectionFromHeaderOrQuery( - this.foreignCollection, + const projection = await this.services.authorization.redactProjection( context, + this.foreignCollection, + QueryStringParser.parseProjectionFromHeaderOrQuery(this.foreignCollection, context), ); const records = await CollectionUtils.listRelation( diff --git a/packages/agent/src/routes/access/list.ts b/packages/agent/src/routes/access/list.ts index 47aa5d6237..081cd6bb19 100644 --- a/packages/agent/src/routes/access/list.ts +++ b/packages/agent/src/routes/access/list.ts @@ -12,6 +12,7 @@ export default class ListRoute extends CollectionRoute { public async handleList(context: Context) { await this.services.authorization.assertCanBrowse(context, this.collection.name); + await this.services.authorization.assertCanReadQueryFields(context, this.collection); const scope = await this.services.authorization.getScope(this.collection, context); let paginatedFilter = ContextFilterFactory.buildPaginated(this.collection, context, scope); @@ -20,7 +21,11 @@ export default class ListRoute extends CollectionRoute { paginatedFilter, ); - const projection = QueryStringParser.parseProjectionFromHeaderOrQuery(this.collection, context); + const projection = await this.services.authorization.redactProjection( + context, + this.collection, + QueryStringParser.parseProjectionFromHeaderOrQuery(this.collection, context), + ); const records = await this.collection.list( QueryStringParser.parseCaller(context), diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index e7a9ac8a28..8b4851a611 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -1,8 +1,9 @@ +import type { RequestedProjection } from '../../utils/query-string'; import type { Collection, ConditionTree } from '@forestadmin/datasource-toolkit'; import type { ForestAdminClient } from '@forestadmin/forestadmin-client'; import type { Context } from 'koa'; -import { UnprocessableError } from '@forestadmin/datasource-toolkit'; +import { ForbiddenError, Projection, UnprocessableError } from '@forestadmin/datasource-toolkit'; import { ChainedSQLQueryError, CollectionActionEvent, @@ -12,6 +13,10 @@ import { import { HttpCode } from '../../types'; import ConditionTreeParser from '../../utils/condition-tree-parser'; +import FieldPathUtils from '../../utils/field-path'; +import QueryStringParser from '../../utils/query-string'; + +type FieldUsage = { action: string; path: string; collectionName: string }; export default class AuthorizationService { constructor(private readonly forestAdminClient: ForestAdminClient) {} @@ -40,6 +45,121 @@ export default class AuthorizationService { await this.assertCanOnCollection(CollectionActionEvent.Export, context, collectionName); } + public async canRead(context: Context, collectionName: string): Promise { + return this.forestAdminClient.permissionService.canOnCollection({ + userId: context.state.user.id, + event: CollectionActionEvent.Read, + collectionName, + }); + } + + /** + * An unnamed field is redacted rather than refused: `ProjectionFactory.all` expands every column + * of every to-one relation when no `fields[]` is sent, so refusing would turn an ordinary + * listing into a 403. + */ + public async redactProjection( + context: Context, + collection: Collection, + requested: RequestedProjection, + ): Promise { + const { projection, explicit } = requested; + const owners = projection.map(path => FieldPathUtils.getLeafCollection(collection, path).name); + const permissions = await this.getReadPermissions(context, collection.name, owners); + const isReadable = (index: number) => permissions.get(owners[index]); + + if (explicit) { + const denied = projection + .map((field, index) => ({ field, collection: owners[index] })) + .filter((_, index) => !isReadable(index)); + + if (denied.length) { + const fields = denied + .map(({ field, collection: name }) => `'${field}' from the '${name}' collection`) + .join(', '); + + throw new ForbiddenError(`You are not allowed to read ${fields}.`); + } + } + + return new Projection(...projection.filter((_, index) => isReadable(index))); + } + + /** + * Refused rather than redacted: dropping a condition widens the result set and dropping a sort + * clause silently reorders it, while both leak the value they touch anyway — a `starts_with` + * filter answers one guess per request without returning a column of its own. + */ + public async assertCanReadQueryFields(context: Context, collection: Collection): Promise { + const usages: FieldUsage[] = []; + const push = (action: string, path: string) => + usages.push({ + action, + path, + collectionName: FieldPathUtils.getLeafCollection(collection, path).name, + }); + + QueryStringParser.parseConditionTree(collection, context)?.forEachLeaf(leaf => + push('filter on', leaf.field), + ); + + for (const { field } of QueryStringParser.parseSort(collection, context)) { + push('sort on', field); + } + + if ( + QueryStringParser.parseSearch(collection, context) && + QueryStringParser.parseSearchExtended(context) + ) { + for (const [name, field] of Object.entries(collection.schema.fields)) { + if (field.type === 'ManyToOne' || field.type === 'OneToOne') { + usages.push({ + action: 'run an extended search through', + path: name, + collectionName: field.foreignCollection, + }); + } + } + } + + await this.assertCanReadUsages(context, collection.name, usages); + } + + public async assertCanReadUsages( + context: Context, + rootCollectionName: string, + usages: FieldUsage[], + ): Promise { + const permissions = await this.getReadPermissions( + context, + rootCollectionName, + usages.map(usage => usage.collectionName), + ); + const denied = usages.find(usage => !permissions.get(usage.collectionName)); + + if (denied) { + throw new ForbiddenError( + `You cannot ${denied.action} '${denied.path}': you are not allowed to read the ` + + `'${denied.collectionName}' collection.`, + ); + } + } + + /** The root is skipped: its own route already asserts `browse` on a listing, `read` on a get. */ + private async getReadPermissions( + context: Context, + rootCollectionName: string, + collectionNames: string[], + ): Promise> { + const toCheck = [...new Set(collectionNames)].filter(name => name !== rootCollectionName); + const allowed = await Promise.all(toCheck.map(name => this.canRead(context, name))); + + return new Map([ + [rootCollectionName, true], + ...toCheck.map((name, index): [string, boolean] => [name, allowed[index]]), + ]); + } + private async assertCanOnCollection( event: CollectionActionEvent, context: Context, diff --git a/packages/agent/src/utils/csv-generator.ts b/packages/agent/src/utils/csv-generator.ts index 9e0e30f5c9..3adfefb57f 100644 --- a/packages/agent/src/utils/csv-generator.ts +++ b/packages/agent/src/utils/csv-generator.ts @@ -52,6 +52,16 @@ export default class CsvGenerator { } } + /** Labels are positionally aligned with the requested projection; dropping one shifts the rest. */ + static filterHeader(header: string, requested: Projection, kept: Projection): string { + if (!header || kept.length === requested.length) return header; + + return header + .split(',') + .filter((_, index) => kept.includes(requested[index])) + .join(','); + } + private static convert(records: RecordData[], projection: Projection): Promise { return writeToString( records.map(record => diff --git a/packages/agent/src/utils/field-path.ts b/packages/agent/src/utils/field-path.ts new file mode 100644 index 0000000000..c496489e43 --- /dev/null +++ b/packages/agent/src/utils/field-path.ts @@ -0,0 +1,22 @@ +import type { Collection } from '@forestadmin/datasource-toolkit'; + +import { SchemaUtils } from '@forestadmin/datasource-toolkit'; + +export default class FieldPathUtils { + static getLeafCollection(collection: Collection, path: string): Collection { + const index = path.indexOf(':'); + + if (index === -1) return collection; + + const relation = SchemaUtils.getRelation( + collection.schema, + path.substring(0, index), + collection.name, + ); + + return FieldPathUtils.getLeafCollection( + collection.dataSource.getCollection(relation.foreignCollection), + path.substring(index + 1), + ); + } +} diff --git a/packages/agent/src/utils/query-string.ts b/packages/agent/src/utils/query-string.ts index 0c77ad76f4..dcb4f951c6 100644 --- a/packages/agent/src/utils/query-string.ts +++ b/packages/agent/src/utils/query-string.ts @@ -21,6 +21,8 @@ import { getRequestId } from './correlation-id'; const DEFAULT_ITEMS_PER_PAGE = 15; const DEFAULT_PAGE_TO_SKIP = 1; +export type RequestedProjection = { projection: Projection; explicit: boolean }; + export default class QueryStringParser { private static VALID_TIMEZONES = new Set(); @@ -86,11 +88,21 @@ export default class QueryStringParser { } } - static parseProjectionFromHeaderOrQuery(collection: Collection, context: Context): Projection { - return ( - QueryStringParser.parseProjectionFromHeader(collection, context) ?? - QueryStringParser.parseProjection(collection, context) - ); + /** `explicit` is false when the projection is the default `ProjectionFactory.all` expansion. */ + static parseProjectionFromHeaderOrQuery( + collection: Collection, + context: Context, + ): RequestedProjection { + const fromHeader = QueryStringParser.parseProjectionFromHeader(collection, context); + + if (fromHeader) return { projection: fromHeader, explicit: true }; + + const fields = context.request.query[`fields[${collection.name}]`]; + + return { + projection: QueryStringParser.parseProjection(collection, context), + explicit: fields !== '' && fields !== undefined, + }; } static parseSearch(collection: Collection, context: Context): string { diff --git a/packages/agent/test/__factories__/authorization/authorization.ts b/packages/agent/test/__factories__/authorization/authorization.ts index a92742a7b1..321ae86312 100644 --- a/packages/agent/test/__factories__/authorization/authorization.ts +++ b/packages/agent/test/__factories__/authorization/authorization.ts @@ -15,6 +15,12 @@ export class AuthorizationsFactory extends Factory { Authorizations.getScope = jest.fn(); Authorizations.assertCanExecuteChart = jest.fn(); Authorizations.invalidateScopeCache = jest.fn(); + Authorizations.canRead = jest.fn().mockResolvedValue(true); + Authorizations.assertCanReadQueryFields = jest.fn(); + Authorizations.assertCanReadUsages = jest.fn(); + Authorizations.redactProjection = jest + .fn() + .mockImplementation(async (context, collection, requested) => requested.projection); }); } } diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts new file mode 100644 index 0000000000..de7ab8fd38 --- /dev/null +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -0,0 +1,388 @@ +import type { DataSource } from '@forestadmin/datasource-toolkit'; + +import { CollectionActionEvent } from '@forestadmin/forestadmin-client'; +import { createMockContext } from '@shopify/jest-koa-mocks'; + +import Chart from '../../src/routes/access/chart'; +import Count from '../../src/routes/access/count'; +import Csv from '../../src/routes/access/csv'; +import Get from '../../src/routes/access/get'; +import List from '../../src/routes/access/list'; +import AuthorizationService from '../../src/services/authorization/authorization'; +import * as factories from '../__factories__'; + +describe('read permissions on related collections', () => { + const options = factories.forestAdminHttpDriverOptions.build(); + + const buildDataSource = (): DataSource => + factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'cards', + schema: factories.collectionSchema.build({ + searchable: true, + countable: true, + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + panLast4: factories.columnSchema.build({ columnType: 'String' }), + accountId: factories.columnSchema.build({ columnType: 'Uuid' }), + holderId: factories.columnSchema.build({ columnType: 'Uuid' }), + account: factories.manyToOneSchema.build({ + foreignCollection: 'accounts', + foreignKey: 'accountId', + }), + holder: factories.manyToOneSchema.build({ + foreignCollection: 'holders', + foreignKey: 'holderId', + }), + }, + }), + }), + factories.collection.build({ + name: 'accounts', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + iban: factories.columnSchema.build({ columnType: 'String' }), + balance: factories.columnSchema.build({ columnType: 'Number' }), + organizationId: factories.columnSchema.build({ columnType: 'Uuid' }), + organization: factories.manyToOneSchema.build({ + foreignCollection: 'organizations', + foreignKey: 'organizationId', + }), + }, + }), + }), + factories.collection.build({ + name: 'organizations', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + name: factories.columnSchema.build({ columnType: 'String' }), + }, + }), + }), + factories.collection.build({ + name: 'holders', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + fullName: factories.columnSchema.text().build(), + nationalId: factories.columnSchema.text().build(), + cards: factories.oneToManySchema.build({ + foreignCollection: 'cards', + originKey: 'holderId', + originKeyTarget: 'id', + }), + }, + }), + }), + ]); + + const buildServices = (readableCollections: string[] = []) => { + const forestAdminClient = factories.forestAdminClient.build(); + + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockImplementation( + ({ event, collectionName }) => + collectionName === 'cards' || + (event === CollectionActionEvent.Read && readableCollections.includes(collectionName)), + ); + + const services = factories.forestAdminHttpDriverServices.build(); + services.authorization = new AuthorizationService(forestAdminClient); + services.serializer.serialize = jest.fn(); + services.serializer.serializeWithSearchMetadata = jest.fn(); + + return services; + }; + + const buildContext = ( + customProperties: Record, + headers = {}, + requestBody?: unknown, + ) => + createMockContext({ + headers, + requestBody, + state: { user: { id: 35, renderingId: 42, email: 'operator@domain.com' } }, + customProperties: { + ...customProperties, + query: { timezone: 'Europe/Paris', ...(customProperties.query as object) }, + }, + }); + + describe('projection the caller named', () => { + it('should refuse the request and name every offending field at once', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await expect( + new List(services, options, dataSource, 'cards').handleList( + buildContext({}, { 'forest-projection': 'id,holder:nationalId,account:iban' }), + ), + ).rejects.toThrow( + "You are not allowed to read 'holder:nationalId' from the 'holders' collection, " + + "'account:iban' from the 'accounts' collection.", + ); + + expect(list).not.toHaveBeenCalled(); + }); + + it('should refuse a named field on a get-one too', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new Get(services, options, dataSource, 'cards').handleGet( + buildContext( + { params: { id: '2d162303-78bf-599e-b197-93590ac3d315' } }, + { 'forest-projection': 'id,holder:fullName' }, + ), + ), + ).rejects.toThrow("You are not allowed to read 'holder:fullName' from the 'holders'"); + }); + + it('should refuse fields named through the fields[] query params', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new List(services, options, dataSource, 'cards').handleList( + buildContext({ + query: { 'fields[cards]': 'id,holder', 'fields[holder]': 'nationalId' }, + }), + ), + ).rejects.toThrow("You are not allowed to read 'holder:nationalId' from the 'holders'"); + }); + + it('should serve a named field on a collection the caller can read', async () => { + const dataSource = buildDataSource(); + const services = buildServices(['holders']); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new List(services, options, dataSource, 'cards').handleList( + buildContext({}, { 'forest-projection': 'id,holder:nationalId' }), + ); + + expect([...list.mock.calls[0][2]].sort()).toEqual(['holder:id', 'holder:nationalId', 'id']); + }); + + it('should traverse a collection it cannot read to reach a column it can', async () => { + const dataSource = buildDataSource(); + const services = buildServices(['organizations']); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new List(services, options, dataSource, 'cards').handleList( + buildContext({}, { 'forest-projection': 'id,account:organization:name' }), + ); + + // The `account:` primary keys `withPks` re-adds are already on the row as `cards.accountId`. + expect([...list.mock.calls[0][2]].sort()).toEqual([ + 'account:id', + 'account:organization:id', + 'account:organization:name', + 'id', + ]); + }); + }); + + describe('projection the caller never asked for', () => { + it('should redact rather than refuse, so an ordinary listing keeps working', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new List(services, options, dataSource, 'cards').handleList(buildContext({})); + + expect(list.mock.calls[0][2].sort()).toEqual(['accountId', 'holderId', 'id', 'panLast4']); + }); + }); + + describe('filter', () => { + const oracleFilter = JSON.stringify({ + field: 'holder:nationalId', + operator: 'starts_with', + value: '1850', + }); + + it('should refuse a filter reading a collection the caller cannot read', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await expect( + new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { filters: oracleFilter } }), + ), + ).rejects.toThrow( + "You cannot filter on 'holder:nationalId': you are not allowed to read the " + + "'holders' collection.", + ); + + expect(list).not.toHaveBeenCalled(); + }); + + it('should refuse the same filter on the count route', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const aggregate = jest.spyOn(dataSource.getCollection('cards'), 'aggregate'); + + await expect( + new Count(services, options, dataSource, 'cards').handleCount( + buildContext({ query: { filters: oracleFilter } }), + ), + ).rejects.toThrow("you are not allowed to read the 'holders' collection"); + + expect(aggregate).not.toHaveBeenCalled(); + }); + + it('should accept a filter reading a collection the caller can read', async () => { + const dataSource = buildDataSource(); + const services = buildServices(['holders']); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { filters: oracleFilter } }), + ); + + expect(list).toHaveBeenCalled(); + }); + + it('should not check the scope, which the agent injects rather than the caller', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + jest + .spyOn(services.authorization, 'getScope') + .mockResolvedValue( + factories.conditionTreeLeaf.build({ field: 'holder:nationalId', value: '1850' }), + ); + + await new List(services, options, dataSource, 'cards').handleList(buildContext({})); + + expect(list).toHaveBeenCalled(); + }); + }); + + describe('sort', () => { + it('should refuse a sort reading a collection the caller cannot read', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { sort: '-account.balance' } }), + ), + ).rejects.toThrow( + "You cannot sort on 'account:balance': you are not allowed to read the " + + "'accounts' collection.", + ); + }); + }); + + describe('extended search', () => { + it('should refuse an extended search when a to-one relation cannot be read', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { search: 'martin', searchExtended: '1' } }), + ), + ).rejects.toThrow('you are not allowed to read the'); + }); + + it('should accept a plain search, which never leaves the root collection', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { search: 'martin' } }), + ); + + expect(list).toHaveBeenCalled(); + }); + + it('should accept an extended search once every to-one relation is readable', async () => { + const dataSource = buildDataSource(); + const services = buildServices(['holders', 'accounts']); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { search: 'martin', searchExtended: '1' } }), + ); + + expect(list).toHaveBeenCalled(); + }); + }); + + describe('csv export', () => { + it('should refuse an export naming a column of an unreadable collection', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + const context = buildContext({ + query: { + header: 'Id,Holder national id', + 'fields[cards]': 'id,holder', + 'fields[holder]': 'nationalId', + }, + }); + + await expect( + new Csv(services, options, dataSource, 'cards').handleCsv(context), + ).rejects.toThrow("You are not allowed to read 'holder:nationalId' from the 'holders'"); + }); + }); + + describe('chart', () => { + it('should refuse to group a chart by a column of an unreadable collection', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const body = { + type: 'Pie', + aggregator: 'Count', + groupByFieldName: 'holder:fullName', + }; + + (services.chartHandler.getChartWithContextInjected as jest.Mock).mockResolvedValue(body); + + await expect( + new Chart(services, options, dataSource, 'cards').handleChart(buildContext({}, {}, body)), + ).rejects.toThrow( + "You cannot group a chart by 'holder:fullName': you are not allowed to read the " + + "'holders' collection.", + ); + }); + + it('should assert browse on the collection a leaderboard counts, which no field names', async () => { + const dataSource = buildDataSource(); + const forestAdminClient = factories.forestAdminClient.build(); + const services = factories.forestAdminHttpDriverServices.build(); + services.authorization = new AuthorizationService(forestAdminClient); + const body = { + type: 'Leaderboard', + aggregator: 'Count', + relationshipFieldName: 'cards', + labelFieldName: 'fullName', + limit: 5, + }; + + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); + (services.chartHandler.getChartWithContextInjected as jest.Mock).mockResolvedValue(body); + jest.spyOn(dataSource.getCollection('cards'), 'aggregate').mockResolvedValue([]); + + await new Chart(services, options, dataSource, 'holders').handleChart( + buildContext({}, {}, body), + ); + + expect(forestAdminClient.permissionService.canOnCollection).toHaveBeenCalledWith({ + userId: 35, + event: CollectionActionEvent.Browse, + collectionName: 'cards', + }); + }); + }); +}); diff --git a/packages/agent/test/services/authorization/authorization.test.ts b/packages/agent/test/services/authorization/authorization.test.ts index f11182aebd..95de5a7bf5 100644 --- a/packages/agent/test/services/authorization/authorization.test.ts +++ b/packages/agent/test/services/authorization/authorization.test.ts @@ -1,5 +1,6 @@ import type { Context } from 'koa'; +import { Projection } from '@forestadmin/datasource-toolkit'; import { ChainedSQLQueryError, ChartType, @@ -433,4 +434,109 @@ describe('AuthorizationService', () => { expect(forestAdminClient.markScopesAsUpdated).toHaveBeenCalledWith(42); }); }); + + describe('keepReadableProjection', () => { + const buildDataSource = () => + factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'cards', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + holderId: factories.columnSchema.build({ columnType: 'Uuid' }), + holder: factories.manyToOneSchema.build({ + foreignCollection: 'holders', + foreignKey: 'holderId', + }), + }, + }), + }), + factories.collection.build({ + name: 'holders', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + fullName: factories.columnSchema.build(), + nationalId: factories.columnSchema.build(), + }, + }), + }), + ]); + + const context = { + state: { user: { id: 35, renderingId: 42 } }, + } as unknown as Context; + + it('should spend no permission check on the root collection', async () => { + const forestAdminClient = factories.forestAdminClient.build(); + const authorizationService = new AuthorizationService(forestAdminClient); + + const projection = await authorizationService.redactProjection( + context, + buildDataSource().getCollection('cards'), + { projection: new Projection('id', 'holderId'), explicit: true }, + ); + + expect(projection).toEqual(['id', 'holderId']); + expect(forestAdminClient.permissionService.canOnCollection).not.toHaveBeenCalled(); + }); + + it('should check a traversed collection once, whatever the number of paths', async () => { + const forestAdminClient = factories.forestAdminClient.build(); + const authorizationService = new AuthorizationService(forestAdminClient); + + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); + + await authorizationService.redactProjection( + context, + buildDataSource().getCollection('cards'), + { + projection: new Projection('id', 'holder:fullName', 'holder:nationalId'), + explicit: true, + }, + ); + + expect(forestAdminClient.permissionService.canOnCollection).toHaveBeenCalledTimes(1); + expect(forestAdminClient.permissionService.canOnCollection).toHaveBeenCalledWith({ + userId: 35, + event: CollectionActionEvent.Read, + collectionName: 'holders', + }); + }); + + it('should drop every path reaching a denied collection', async () => { + const forestAdminClient = factories.forestAdminClient.build(); + const authorizationService = new AuthorizationService(forestAdminClient); + + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(false); + + const projection = await authorizationService.redactProjection( + context, + buildDataSource().getCollection('cards'), + { + projection: new Projection('id', 'holder:fullName', 'holder:nationalId'), + explicit: false, + }, + ); + + expect(projection).toEqual(['id']); + }); + + it('should refuse instead of redacting when the caller named the fields', async () => { + const forestAdminClient = factories.forestAdminClient.build(); + const authorizationService = new AuthorizationService(forestAdminClient); + + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(false); + + await expect( + authorizationService.redactProjection(context, buildDataSource().getCollection('cards'), { + projection: new Projection('id', 'holder:fullName', 'holder:nationalId'), + explicit: true, + }), + ).rejects.toThrow( + "You are not allowed to read 'holder:fullName' from the 'holders' collection, " + + "'holder:nationalId' from the 'holders' collection.", + ); + }); + }); }); diff --git a/packages/agent/test/utils/csv-generator.test.ts b/packages/agent/test/utils/csv-generator.test.ts index a12471f975..06df6bf64f 100644 --- a/packages/agent/test/utils/csv-generator.test.ts +++ b/packages/agent/test/utils/csv-generator.test.ts @@ -409,4 +409,48 @@ describe('CsvGenerator', () => { }); }); }); + + describe('filterHeader', () => { + it('should drop the labels of the paths that were pruned', () => { + const header = CsvGenerator.filterHeader( + 'Id,Pan,Holder national id', + new Projection('id', 'panLast4', 'holder:nationalId'), + new Projection('id', 'panLast4'), + ); + + expect(header).toEqual('Id,Pan'); + }); + + it('should drop a label from the middle without shifting the others', () => { + const header = CsvGenerator.filterHeader( + 'Id,Holder national id,Pan', + new Projection('id', 'holder:nationalId', 'panLast4'), + new Projection('id', 'panLast4'), + ); + + expect(header).toEqual('Id,Pan'); + }); + + it('should return the header untouched when nothing was pruned', () => { + const projection = new Projection('id', 'panLast4'); + + expect(CsvGenerator.filterHeader('Id,Pan', projection, projection)).toEqual('Id,Pan'); + }); + + it('should return an absent header as is', () => { + expect( + CsvGenerator.filterHeader(undefined, new Projection('id', 'x:y'), new Projection('id')), + ).toBeUndefined(); + }); + + it('should drop every label when every path was pruned', () => { + const header = CsvGenerator.filterHeader( + 'Holder national id', + new Projection('holder:nationalId'), + new Projection(), + ); + + expect(header).toEqual(''); + }); + }); }); diff --git a/packages/agent/test/utils/field-path.test.ts b/packages/agent/test/utils/field-path.test.ts new file mode 100644 index 0000000000..62c5cfd5c8 --- /dev/null +++ b/packages/agent/test/utils/field-path.test.ts @@ -0,0 +1,80 @@ +import FieldPathUtils from '../../src/utils/field-path'; +import * as factories from '../__factories__'; + +describe('FieldPathUtils', () => { + const dataSource = factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'cards', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + panLast4: factories.columnSchema.build({ columnType: 'String' }), + accountId: factories.columnSchema.build({ columnType: 'Uuid' }), + account: factories.manyToOneSchema.build({ + foreignCollection: 'accounts', + foreignKey: 'accountId', + }), + }, + }), + }), + factories.collection.build({ + name: 'accounts', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + iban: factories.columnSchema.build({ columnType: 'String' }), + organizationId: factories.columnSchema.build({ columnType: 'Uuid' }), + organization: factories.manyToOneSchema.build({ + foreignCollection: 'organizations', + foreignKey: 'organizationId', + }), + }, + }), + }), + factories.collection.build({ + name: 'organizations', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + name: factories.columnSchema.build({ columnType: 'String' }), + }, + }), + }), + ]); + + describe('getLeafCollection', () => { + it('should return the collection itself for a plain column', () => { + const owner = FieldPathUtils.getLeafCollection(dataSource.getCollection('cards'), 'panLast4'); + + expect(owner.name).toEqual('cards'); + }); + + it('should return the collection the relation points to for a one hop path', () => { + const owner = FieldPathUtils.getLeafCollection( + dataSource.getCollection('cards'), + 'account:iban', + ); + + expect(owner.name).toEqual('accounts'); + }); + + it('should return the last collection of the path, not the ones crossed on the way', () => { + const owner = FieldPathUtils.getLeafCollection( + dataSource.getCollection('cards'), + 'account:organization:name', + ); + + expect(owner.name).toEqual('organizations'); + }); + + it('should return the root collection for a self referencing relation', () => { + const cards = dataSource.getCollection('cards'); + cards.schema.fields.parent = factories.manyToOneSchema.build({ + foreignCollection: 'cards', + foreignKey: 'id', + }); + + expect(FieldPathUtils.getLeafCollection(cards, 'parent:panLast4').name).toEqual('cards'); + }); + }); +}); diff --git a/packages/agent/test/utils/query-string.test.ts b/packages/agent/test/utils/query-string.test.ts index 4c890bc326..720b5e7b0c 100644 --- a/packages/agent/test/utils/query-string.test.ts +++ b/packages/agent/test/utils/query-string.test.ts @@ -430,7 +430,7 @@ describe('QueryStringParser', () => { context, ); - expect(projection).toEqual(new Projection('name')); + expect(projection).toEqual({ projection: new Projection('name'), explicit: true }); }); test('should fallback to the query string when the header is missing', () => { @@ -443,7 +443,7 @@ describe('QueryStringParser', () => { context, ); - expect(projection).toEqual(new Projection('name')); + expect(projection).toEqual({ projection: new Projection('name'), explicit: true }); }); test('should fallback to the query string when the header is empty', () => { @@ -457,7 +457,31 @@ describe('QueryStringParser', () => { context, ); - expect(projection).toEqual(new Projection('name')); + expect(projection).toEqual({ projection: new Projection('name'), explicit: true }); + }); + + test('should report the default expansion as not explicit when no field is requested', () => { + const context = createMockContext({ customProperties: { query: {} } }); + + const { explicit } = QueryStringParser.parseProjectionFromHeaderOrQuery( + collectionSimple, + context, + ); + + expect(explicit).toBe(false); + }); + + test('should report an empty fields query param as not explicit', () => { + const context = createMockContext({ + customProperties: { query: { 'fields[books]': '' } }, + }); + + const { explicit } = QueryStringParser.parseProjectionFromHeaderOrQuery( + collectionSimple, + context, + ); + + expect(explicit).toBe(false); }); test('should throw on an invalid header instead of falling back to the query string', () => { From a88dbfed93930137a59972190edf01d245926079 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 20 Aug 2026 17:40:48 +0200 Subject: [PATCH 02/10] fix(agent): check the collections a search string names through its own syntax MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relation.column:term` is documented end-user search syntax, and it needs neither `filters` nor `searchExtended`: `FieldsQueryWalker` rewrites the dot to a colon, and the search decorator resolves the result across relations — to-many ones included. So `?search=holder.nationalId:1850` reached a column of a collection the caller had no `read` on, and the guard pushed no usage at all. That is PRD-900's read 3 oracle, still open through a second door. Resolve those paths with the decorator's own resolver instead of a second approximation of it. `lenientGetSchema` moves out of `SearchCollectionDecorator` into a module the decorator now calls, and `getSearchedFieldPaths(collection, search)` is exported from the customizer — a string in, resolved field paths out, so no ANTLR-generated type reaches the public API. Co-Authored-By: Claude Opus 5 --- .../services/authorization/authorization.ts | 28 ++++++++----- .../security/related-read-permissions.test.ts | 39 +++++++++++++++++ .../src/decorators/search/collection.ts | 30 +------------ .../src/decorators/search/field-paths.ts | 42 +++++++++++++++++++ packages/datasource-customizer/src/index.ts | 1 + 5 files changed, 101 insertions(+), 39 deletions(-) create mode 100644 packages/datasource-customizer/src/decorators/search/field-paths.ts diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index 8b4851a611..e793bf17b2 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -3,6 +3,7 @@ import type { Collection, ConditionTree } from '@forestadmin/datasource-toolkit' import type { ForestAdminClient } from '@forestadmin/forestadmin-client'; import type { Context } from 'koa'; +import { getSearchedFieldPaths } from '@forestadmin/datasource-customizer'; import { ForbiddenError, Projection, UnprocessableError } from '@forestadmin/datasource-toolkit'; import { ChainedSQLQueryError, @@ -107,17 +108,22 @@ export default class AuthorizationService { push('sort on', field); } - if ( - QueryStringParser.parseSearch(collection, context) && - QueryStringParser.parseSearchExtended(context) - ) { - for (const [name, field] of Object.entries(collection.schema.fields)) { - if (field.type === 'ManyToOne' || field.type === 'OneToOne') { - usages.push({ - action: 'run an extended search through', - path: name, - collectionName: field.foreignCollection, - }); + const search = QueryStringParser.parseSearch(collection, context); + + if (search) { + // `relation.column:term` is end-user search syntax and works without extended search, so the + // paths it names are resolved by the search decorator's own resolver rather than guessed at. + for (const path of getSearchedFieldPaths(collection, search)) push('search on', path); + + if (QueryStringParser.parseSearchExtended(context)) { + for (const [name, field] of Object.entries(collection.schema.fields)) { + if (field.type === 'ManyToOne' || field.type === 'OneToOne') { + usages.push({ + action: 'run an extended search through', + path: name, + collectionName: field.foreignCollection, + }); + } } } } diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index de7ab8fd38..6a36d4d03b 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -293,6 +293,45 @@ describe('read permissions on related collections', () => { ).rejects.toThrow('you are not allowed to read the'); }); + it('should refuse a search naming a relation column through the dot syntax', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const aggregate = jest.spyOn(dataSource.getCollection('cards'), 'aggregate'); + + await expect( + new Count(services, options, dataSource, 'cards').handleCount( + buildContext({ query: { search: 'holder.nationalId:1850' } }), + ), + ).rejects.toThrow( + "You cannot search on 'holder:nationalId': you are not allowed to read the " + + "'holders' collection.", + ); + + expect(aggregate).not.toHaveBeenCalled(); + }); + + it('should refuse a search reaching a denied collection across a to-many relation', async () => { + const dataSource = buildDataSource(); + const forestAdminClient = factories.forestAdminClient.build(); + const services = factories.forestAdminHttpDriverServices.build(); + services.authorization = new AuthorizationService(forestAdminClient); + services.serializer.serializeWithSearchMetadata = jest.fn(); + + // Searching from `holders`, so `cards` is the denied one here. + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockImplementation( + ({ collectionName }) => collectionName !== 'cards', + ); + + await expect( + new List(services, options, dataSource, 'holders').handleList( + buildContext({ query: { search: 'cards.panLast4:4242' } }), + ), + ).rejects.toThrow( + "You cannot search on 'cards:panLast4': you are not allowed to read the " + + "'cards' collection.", + ); + }); + it('should accept a plain search, which never leaves the root collection', async () => { const dataSource = buildDataSource(); const services = buildServices(); diff --git a/packages/datasource-customizer/src/decorators/search/collection.ts b/packages/datasource-customizer/src/decorators/search/collection.ts index 4e5d8f0df3..8c4a4ea6be 100644 --- a/packages/datasource-customizer/src/decorators/search/collection.ts +++ b/packages/datasource-customizer/src/decorators/search/collection.ts @@ -14,7 +14,7 @@ import type { import { CollectionDecorator, ConditionTreeFactory } from '@forestadmin/datasource-toolkit'; import CollectionSearchContext from './collection-search-context'; -import normalizeName from './normalize-name'; +import { lenientGetSchema } from './field-paths'; import { extractSpecifiedFields, generateConditionTree, parseQuery } from './parse-query'; export default class SearchCollectionDecorator extends CollectionDecorator { @@ -91,7 +91,7 @@ export default class SearchCollectionDecorator extends CollectionDecorator { [ ...defaultFields, ...[...specifiedFields, ...(options?.onlyFields ?? []), ...(options?.includeFields ?? [])] - .map(name => this.lenientGetSchema(name)) + .map(name => lenientGetSchema(this, name)) .filter(Boolean) .map(schema => [schema.field, schema.schema] as [string, ColumnSchema]), ] @@ -136,30 +136,4 @@ export default class SearchCollectionDecorator extends CollectionDecorator { return fields; } - private lenientGetSchema(path: string): { field: string; schema: ColumnSchema } | null { - const [prefix, suffix] = path.split(/:(.*)/); - const fuzzyPrefix = normalizeName(prefix); - - for (const [field, schema] of Object.entries(this.schema.fields)) { - const fuzzyFieldName = normalizeName(field); - - if (fuzzyPrefix === fuzzyFieldName) { - if (!suffix && schema.type === 'Column') { - return { field, schema }; - } - - if ( - suffix && - (schema.type === 'OneToMany' || schema.type === 'ManyToOne' || schema.type === 'OneToOne') - ) { - const related = this.dataSource.getCollection(schema.foreignCollection); - const fuzzy = related.lenientGetSchema(suffix); - - if (fuzzy) return { field: `${field}:${fuzzy.field}`, schema: fuzzy.schema }; - } - } - } - - return null; - } } diff --git a/packages/datasource-customizer/src/decorators/search/field-paths.ts b/packages/datasource-customizer/src/decorators/search/field-paths.ts new file mode 100644 index 0000000000..8c29ccfafa --- /dev/null +++ b/packages/datasource-customizer/src/decorators/search/field-paths.ts @@ -0,0 +1,42 @@ +import type { Collection, ColumnSchema } from '@forestadmin/datasource-toolkit'; + +import normalizeName from './normalize-name'; +import { extractSpecifiedFields, parseQuery } from './parse-query'; + +export function lenientGetSchema( + collection: Collection, + path: string, +): { field: string; schema: ColumnSchema } | null { + const [prefix, suffix] = path.split(/:(.*)/); + const fuzzyPrefix = normalizeName(prefix); + + for (const [field, schema] of Object.entries(collection.schema.fields)) { + if (fuzzyPrefix === normalizeName(field)) { + if (!suffix && schema.type === 'Column') { + return { field, schema }; + } + + if ( + suffix && + (schema.type === 'OneToMany' || schema.type === 'ManyToOne' || schema.type === 'OneToOne') + ) { + const related = collection.dataSource.getCollection(schema.foreignCollection); + const fuzzy = lenientGetSchema(related, suffix); + + if (fuzzy) return { field: `${field}:${fuzzy.field}`, schema: fuzzy.schema }; + } + } + } + + return null; +} + +/** + * The field paths a search string reaches through the `relation.column:term` syntax, resolved the + * same way the search decorator resolves them — fuzzily, and across to-many relations too. + */ +export function getSearchedFieldPaths(collection: Collection, search: string): string[] { + return extractSpecifiedFields(parseQuery(search)) + .map(name => lenientGetSchema(collection, name)?.field) + .filter(Boolean); +} diff --git a/packages/datasource-customizer/src/index.ts b/packages/datasource-customizer/src/index.ts index 3de392f5c5..d0fc11b126 100644 --- a/packages/datasource-customizer/src/index.ts +++ b/packages/datasource-customizer/src/index.ts @@ -13,6 +13,7 @@ export { default as CollectionChartContext } from './decorators/chart/context'; export { ComputedDefinition } from './decorators/computed/types'; export { OperatorDefinition } from './decorators/operators-emulate/types'; export { RelationDefinition } from './decorators/relation/types'; +export { getSearchedFieldPaths } from './decorators/search/field-paths'; export { SearchDefinition } from './decorators/search/types'; export { SegmentDefinition } from './decorators/segment/types'; export * from './decorators/write/write-replace/types'; From d90b26ce98fcb9c2462ba7f0610d94fd1aa4f141 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 20 Aug 2026 17:44:52 +0200 Subject: [PATCH 03/10] test(agent): pin the guards the route factory stubs out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `assertCanReadQueryFields` is stubbed to a no-op and `redactProjection` to an identity passthrough in the route test factory, so the existing suites stay green whether or not a route calls either. The three `*-related` routes are the only sites resolving against `foreignCollection` rather than the route's own collection, and nothing would have caught a swap between the two; the Line, Value and Objective chart shapes went through `assertCanReadAggregatedFields` unasserted as well. Cover all of them against a real `AuthorizationService`, and make the four allow-side cases assert the clause that survived — a filter condition, the injected scope leaf, `search` and `searchExtended` — rather than that the request was not refused. Bare called-ness passes just as well when a permitted query is silently narrowed, which is the failure mode this change chose 403 over. Also scope the `withPks` note to the relation type it holds for: a ManyToOne already carries the re-added key on the row, a OneToOne does not. Co-Authored-By: Claude Opus 5 --- .../services/authorization/authorization.ts | 2 +- .../security/related-read-permissions.test.ts | 122 +++++++++++++++++- 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index e793bf17b2..fc06311d41 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -151,7 +151,7 @@ export default class AuthorizationService { } } - /** The root is skipped: its own route already asserts `browse` on a listing, `read` on a get. */ + /** The root is skipped: `browse` gates a listing, `read` a get, and the signed hash a chart. */ private async getReadPermissions( context: Context, rootCollectionName: string, diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index 6a36d4d03b..ddb104ee98 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -5,9 +5,12 @@ import { createMockContext } from '@shopify/jest-koa-mocks'; import Chart from '../../src/routes/access/chart'; import Count from '../../src/routes/access/count'; +import CountRelated from '../../src/routes/access/count-related'; import Csv from '../../src/routes/access/csv'; +import CsvRelated from '../../src/routes/access/csv-related'; import Get from '../../src/routes/access/get'; import List from '../../src/routes/access/list'; +import ListRelated from '../../src/routes/access/list-related'; import AuthorizationService from '../../src/services/authorization/authorization'; import * as factories from '../__factories__'; @@ -42,7 +45,7 @@ describe('read permissions on related collections', () => { schema: factories.collectionSchema.build({ fields: { id: factories.columnSchema.uuidPrimaryKey().build(), - iban: factories.columnSchema.build({ columnType: 'String' }), + iban: factories.columnSchema.text().build(), balance: factories.columnSchema.build({ columnType: 'Number' }), organizationId: factories.columnSchema.build({ columnType: 'Uuid' }), organization: factories.manyToOneSchema.build({ @@ -176,7 +179,8 @@ describe('read permissions on related collections', () => { buildContext({}, { 'forest-projection': 'id,account:organization:name' }), ); - // The `account:` primary keys `withPks` re-adds are already on the row as `cards.accountId`. + // `withPks` re-adds `account:id`, which a ManyToOne already carries on the row as + // `cards.accountId`. A OneToOne intermediate would expose a key the row does not carry. expect([...list.mock.calls[0][2]].sort()).toEqual([ 'account:id', 'account:organization:id', @@ -245,7 +249,11 @@ describe('read permissions on related collections', () => { buildContext({ query: { filters: oracleFilter } }), ); - expect(list).toHaveBeenCalled(); + expect(list.mock.calls[0][1].conditionTree).toMatchObject({ + field: 'holder:nationalId', + operator: 'StartsWith', + value: '1850', + }); }); it('should not check the scope, which the agent injects rather than the caller', async () => { @@ -261,7 +269,10 @@ describe('read permissions on related collections', () => { await new List(services, options, dataSource, 'cards').handleList(buildContext({})); - expect(list).toHaveBeenCalled(); + expect(list.mock.calls[0][1].conditionTree).toMatchObject({ + field: 'holder:nationalId', + value: '1850', + }); }); }); @@ -341,7 +352,7 @@ describe('read permissions on related collections', () => { buildContext({ query: { search: 'martin' } }), ); - expect(list).toHaveBeenCalled(); + expect(list.mock.calls[0][1]).toMatchObject({ search: 'martin', searchExtended: false }); }); it('should accept an extended search once every to-one relation is readable', async () => { @@ -353,7 +364,7 @@ describe('read permissions on related collections', () => { buildContext({ query: { search: 'martin', searchExtended: '1' } }), ); - expect(list).toHaveBeenCalled(); + expect(list.mock.calls[0][1]).toMatchObject({ search: 'martin', searchExtended: true }); }); }); @@ -376,6 +387,70 @@ describe('read permissions on related collections', () => { }); }); + // The related routes are the only sites resolving against `foreignCollection` rather than the + // route's own collection, so a swap between the two would otherwise go unnoticed. + describe('related routes', () => { + const holderCardsContext = () => + buildContext({ + params: { parentId: '2d162303-78bf-599e-b197-93590ac3d315' }, + query: { + filters: JSON.stringify({ + field: 'account:iban', + operator: 'equal', + value: 'FR76', + }), + }, + }); + + it('should refuse a filter on a denied collection reached from the related list', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new ListRelated(services, options, dataSource, 'holders', 'cards').handleListRelated( + holderCardsContext(), + ), + ).rejects.toThrow( + "You cannot filter on 'account:iban': you are not allowed to read the " + + "'accounts' collection.", + ); + }); + + it('should refuse the same filter on the related export', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new CsvRelated(services, options, dataSource, 'holders', 'cards').handleRelatedCsv( + holderCardsContext(), + ), + ).rejects.toThrow("you are not allowed to read the 'accounts' collection"); + }); + + it('should refuse the same filter on the related count', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + + await expect( + new CountRelated(services, options, dataSource, 'holders', 'cards').handleCountRelated( + holderCardsContext(), + ), + ).rejects.toThrow("you are not allowed to read the 'accounts' collection"); + }); + + it('should redact the related list projection rather than refuse it', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + + await new ListRelated(services, options, dataSource, 'holders', 'cards').handleListRelated( + buildContext({ params: { parentId: '2d162303-78bf-599e-b197-93590ac3d315' } }), + ); + + expect(list.mock.calls[0][2].sort()).toEqual(['accountId', 'holderId', 'id', 'panLast4']); + }); + }); + describe('chart', () => { it('should refuse to group a chart by a column of an unreadable collection', async () => { const dataSource = buildDataSource(); @@ -396,6 +471,41 @@ describe('read permissions on related collections', () => { ); }); + it('should refuse a line chart grouped by a column of an unreadable collection', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const body = { + type: 'Line', + aggregator: 'Count', + groupByFieldName: 'holder:fullName', + timeRange: 'Day', + }; + + (services.chartHandler.getChartWithContextInjected as jest.Mock).mockResolvedValue(body); + + await expect( + new Chart(services, options, dataSource, 'cards').handleChart(buildContext({}, {}, body)), + ).rejects.toThrow( + "You cannot group a chart by 'holder:fullName': you are not allowed to read the " + + "'holders' collection.", + ); + }); + + it('should refuse a value chart aggregating a column of an unreadable collection', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const body = { type: 'Value', aggregator: 'Sum', aggregateFieldName: 'account:balance' }; + + (services.chartHandler.getChartWithContextInjected as jest.Mock).mockResolvedValue(body); + + await expect( + new Chart(services, options, dataSource, 'cards').handleChart(buildContext({}, {}, body)), + ).rejects.toThrow( + "You cannot aggregate a chart on 'account:balance': you are not allowed to read the " + + "'accounts' collection.", + ); + }); + it('should assert browse on the collection a leaderboard counts, which no field names', async () => { const dataSource = buildDataSource(); const forestAdminClient = factories.forestAdminClient.build(); From 78944c537c3d398f1f67133406c22b767b3a68a5 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 20 Aug 2026 18:01:58 +0200 Subject: [PATCH 04/10] fix(agent): ask the stack what a search reaches instead of deriving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard enumerated the collections an extended search could touch from the schema it holds — the top of the decorator stack. The search decorator reads `childCollection`, below the publication and renaming layers, so the two sets disagreed in both directions. A relation hidden by `.removeField` while its target stayed published was absent from the guard's view and still searched, so `searchExtended=1` reached its columns with no check. And a collection using `replaceSearch` was refused on `searchExtended` although its handler never runs the default enumeration at all, which the deliberate exemption for replaced searches was supposed to spare. Move the question, not the decision: `getSearchedFields(search, extended)` walks down the stack from `CollectionDecorator` and the search decorator answers it from `childCollection`, returning `null` when a replacer makes the fields the customer's choice rather than the caller's. Enforcement stays at the route, which is the only place a caller's own query is still separable from the scope and the segment the agent injects into the same filter. Each half is now pinned where it lives: the decorator's answer in the customizer's suite, the agent's use of it — refuse what is named, forward the caller's flag, serve when the stack cannot say — in the security suite. Co-Authored-By: Claude Opus 5 --- .../services/authorization/authorization.ts | 33 ++++++----- .../security/related-read-permissions.test.ts | 58 ++++++++++--------- .../src/decorators/search/collection.ts | 23 +++++++- .../src/decorators/search/field-paths.ts | 15 +++++ .../decorators/search/collections.test.ts | 53 +++++++++++++++++ .../src/decorators/collection-decorator.ts | 12 ++++ packages/datasource-toolkit/src/index.ts | 2 +- 7 files changed, 149 insertions(+), 47 deletions(-) diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index fc06311d41..14739e2cd2 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -1,9 +1,12 @@ import type { RequestedProjection } from '../../utils/query-string'; -import type { Collection, ConditionTree } from '@forestadmin/datasource-toolkit'; +import type { + Collection, + CollectionDecorator, + ConditionTree, +} from '@forestadmin/datasource-toolkit'; import type { ForestAdminClient } from '@forestadmin/forestadmin-client'; import type { Context } from 'koa'; -import { getSearchedFieldPaths } from '@forestadmin/datasource-customizer'; import { ForbiddenError, Projection, UnprocessableError } from '@forestadmin/datasource-toolkit'; import { ChainedSQLQueryError, @@ -111,20 +114,18 @@ export default class AuthorizationService { const search = QueryStringParser.parseSearch(collection, context); if (search) { - // `relation.column:term` is end-user search syntax and works without extended search, so the - // paths it names are resolved by the search decorator's own resolver rather than guessed at. - for (const path of getSearchedFieldPaths(collection, search)) push('search on', path); - - if (QueryStringParser.parseSearchExtended(context)) { - for (const [name, field] of Object.entries(collection.schema.fields)) { - if (field.type === 'ManyToOne' || field.type === 'OneToOne') { - usages.push({ - action: 'run an extended search through', - path: name, - collectionName: field.foreignCollection, - }); - } - } + // Asked of the stack rather than derived from the schema: `relation.column:term` is end-user + // syntax that needs no extended search, and the fields an extended one reaches are read below + // the publication and renaming layers. A `null` answer means the collection cannot say — a + // replaced search, where the customer's handler picks the fields and the caller only supplies + // the text. + const searched = (collection as CollectionDecorator).getSearchedFields?.( + search, + QueryStringParser.parseSearchExtended(context), + ); + + for (const { path, collection: name } of searched ?? []) { + usages.push({ action: 'search on', path, collectionName: name }); } } diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index ddb104ee98..a8bdc5b162 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -1,4 +1,4 @@ -import type { DataSource } from '@forestadmin/datasource-toolkit'; +import type { CollectionDecorator, DataSource } from '@forestadmin/datasource-toolkit'; import { CollectionActionEvent } from '@forestadmin/forestadmin-client'; import { createMockContext } from '@shopify/jest-koa-mocks'; @@ -293,21 +293,14 @@ describe('read permissions on related collections', () => { }); describe('extended search', () => { - it('should refuse an extended search when a to-one relation cannot be read', async () => { + it('should refuse whatever the stack says the search will reach', async () => { const dataSource = buildDataSource(); const services = buildServices(); + const cards = dataSource.getCollection('cards') as CollectionDecorator; + const aggregate = jest.spyOn(cards, 'aggregate'); - await expect( - new List(services, options, dataSource, 'cards').handleList( - buildContext({ query: { search: 'martin', searchExtended: '1' } }), - ), - ).rejects.toThrow('you are not allowed to read the'); - }); - - it('should refuse a search naming a relation column through the dot syntax', async () => { - const dataSource = buildDataSource(); - const services = buildServices(); - const aggregate = jest.spyOn(dataSource.getCollection('cards'), 'aggregate'); + // `relation.column:term` needs no extended search, and only the stack knows it resolves. + cards.getSearchedFields = () => [{ path: 'holder:nationalId', collection: 'holders' }]; await expect( new Count(services, options, dataSource, 'cards').handleCount( @@ -321,26 +314,35 @@ describe('read permissions on related collections', () => { expect(aggregate).not.toHaveBeenCalled(); }); - it('should refuse a search reaching a denied collection across a to-many relation', async () => { + it('should ask the stack with the extended flag the caller sent', async () => { const dataSource = buildDataSource(); - const forestAdminClient = factories.forestAdminClient.build(); - const services = factories.forestAdminHttpDriverServices.build(); - services.authorization = new AuthorizationService(forestAdminClient); - services.serializer.serializeWithSearchMetadata = jest.fn(); + const services = buildServices(); + const cards = dataSource.getCollection('cards') as CollectionDecorator; + const getSearchedFields = jest.fn().mockReturnValue([]); + cards.getSearchedFields = getSearchedFields; + jest.spyOn(cards, 'list').mockResolvedValue([]); - // Searching from `holders`, so `cards` is the denied one here. - (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockImplementation( - ({ collectionName }) => collectionName !== 'cards', + await new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { search: 'martin', searchExtended: '1' } }), ); - await expect( - new List(services, options, dataSource, 'holders').handleList( - buildContext({ query: { search: 'cards.panLast4:4242' } }), - ), - ).rejects.toThrow( - "You cannot search on 'cards:panLast4': you are not allowed to read the " + - "'cards' collection.", + expect(getSearchedFields).toHaveBeenCalledWith('martin', true); + }); + + it('should serve the request when the stack cannot say what a search reaches', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const cards = dataSource.getCollection('cards') as CollectionDecorator; + const list = jest.spyOn(cards, 'list').mockResolvedValue([]); + + // A replaced search: the handler picks the fields, the caller only supplies the text. + cards.getSearchedFields = () => null; + + await new List(services, options, dataSource, 'cards').handleList( + buildContext({ query: { search: 'martin', searchExtended: '1' } }), ); + + expect(list.mock.calls[0][1]).toMatchObject({ search: 'martin', searchExtended: true }); }); it('should accept a plain search, which never leaves the root collection', async () => { diff --git a/packages/datasource-customizer/src/decorators/search/collection.ts b/packages/datasource-customizer/src/decorators/search/collection.ts index 8c4a4ea6be..6f015edb3c 100644 --- a/packages/datasource-customizer/src/decorators/search/collection.ts +++ b/packages/datasource-customizer/src/decorators/search/collection.ts @@ -9,12 +9,13 @@ import type { DataSourceDecorator, PaginatedFilter, PlainConditionTree, + SearchedField, } from '@forestadmin/datasource-toolkit'; import { CollectionDecorator, ConditionTreeFactory } from '@forestadmin/datasource-toolkit'; import CollectionSearchContext from './collection-search-context'; -import { lenientGetSchema } from './field-paths'; +import { getLeafCollectionName, getSearchedFieldPaths, lenientGetSchema } from './field-paths'; import { extractSpecifiedFields, generateConditionTree, parseQuery } from './parse-query'; export default class SearchCollectionDecorator extends CollectionDecorator { @@ -118,6 +119,25 @@ export default class SearchCollectionDecorator extends CollectionDecorator { return conditionTree?.toPlainObject(); } + /** + * Answers against `childCollection`, which is what the search actually reads — a field hidden by + * the publication or renaming layers above is still searched. Returns `null` when a replacer is + * installed: the customer's handler chooses the fields, and the caller only supplies the text. + */ + override getSearchedFields(search: string, extended: boolean): SearchedField[] | null { + if (this.replacer) return null; + + const paths = [ + ...getSearchedFieldPaths(this.childCollection, search), + ...this.getFields(this.childCollection, extended).map(([path]) => path), + ]; + + return paths.map(path => ({ + path, + collection: getLeafCollectionName(this.childCollection, path), + })); + } + private getFields(collection: Collection, extended: boolean): [string, ColumnSchema][] { const fields: [string, ColumnSchema][] = []; @@ -135,5 +155,4 @@ export default class SearchCollectionDecorator extends CollectionDecorator { return fields; } - } diff --git a/packages/datasource-customizer/src/decorators/search/field-paths.ts b/packages/datasource-customizer/src/decorators/search/field-paths.ts index 8c29ccfafa..cdac235365 100644 --- a/packages/datasource-customizer/src/decorators/search/field-paths.ts +++ b/packages/datasource-customizer/src/decorators/search/field-paths.ts @@ -40,3 +40,18 @@ export function getSearchedFieldPaths(collection: Collection, search: string): s .map(name => lenientGetSchema(collection, name)?.field) .filter(Boolean); } + +export function getLeafCollectionName(collection: Collection, path: string): string { + const index = path.indexOf(':'); + + if (index === -1) return collection.name; + + const relation = collection.schema.fields[path.substring(0, index)]; + + if (!relation || relation.type === 'Column') return collection.name; + + return getLeafCollectionName( + collection.dataSource.getCollection(relation.foreignCollection), + path.substring(index + 1), + ); +} diff --git a/packages/datasource-customizer/test/decorators/search/collections.test.ts b/packages/datasource-customizer/test/decorators/search/collections.test.ts index 54e261bca6..b9f5b910ac 100644 --- a/packages/datasource-customizer/test/decorators/search/collections.test.ts +++ b/packages/datasource-customizer/test/decorators/search/collections.test.ts @@ -749,3 +749,56 @@ describe('SearchCollectionDecorator', () => { }); }); }); + +describe('getSearchedFields', () => { + const buildCards = () => + buildCollection( + { + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + panLast4: factories.columnSchema.build({ columnType: 'String' }), + holderId: factories.columnSchema.build({ columnType: 'Uuid' }), + holder: factories.manyToOneSchema.build({ + foreignCollection: 'holders', + foreignKey: 'holderId', + }), + }, + }, + [ + factories.collection.build({ + name: 'holders', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + nationalId: factories.columnSchema.build({ columnType: 'String' }), + }, + }), + }), + ], + ); + + test('it names the relation column the dot syntax reaches, without extended search', () => { + const searched = buildCards().getSearchedFields('holder.nationalId:1850', false); + + expect(searched).toContainEqual({ path: 'holder:nationalId', collection: 'holders' }); + }); + + test('it names every to-one column an extended search reaches', () => { + const searched = buildCards().getSearchedFields('martin', true); + + expect(searched).toContainEqual({ path: 'holder:nationalId', collection: 'holders' }); + }); + + test('it names none of them when the search is not extended', () => { + const searched = buildCards().getSearchedFields('martin', false); + + expect(searched.every(({ collection }) => collection !== 'holders')).toBe(true); + }); + + test('it says it cannot tell when a replacer picks the fields', () => { + const decorator = buildCards(); + decorator.replaceSearch(value => ({ field: 'id', operator: 'Equal', value })); + + expect(decorator.getSearchedFields('martin', true)).toBeNull(); + }); +}); diff --git a/packages/datasource-toolkit/src/decorators/collection-decorator.ts b/packages/datasource-toolkit/src/decorators/collection-decorator.ts index 4747989b5e..40528d7855 100644 --- a/packages/datasource-toolkit/src/decorators/collection-decorator.ts +++ b/packages/datasource-toolkit/src/decorators/collection-decorator.ts @@ -10,6 +10,8 @@ import type Projection from '../interfaces/query/projection'; import type { CompositeId, RecordData } from '../interfaces/record'; import type { CollectionSchema } from '../interfaces/schema'; +export type SearchedField = { path: string; collection: string }; + export default class CollectionDecorator implements Collection { readonly dataSource: DataSource; protected childCollection: Collection; @@ -34,6 +36,16 @@ export default class CollectionDecorator implements Collection { return this.childCollection.name; } + /** + * Which fields a search will actually reach, and where they live — `null` when the collection + * cannot say, which a caller must read as "unknown", never as "none". + */ + getSearchedFields(search: string, extended: boolean): SearchedField[] | null { + return this.childCollection instanceof CollectionDecorator + ? this.childCollection.getSearchedFields(search, extended) + : null; + } + constructor(childCollection: Collection, dataSource: DataSource) { this.childCollection = childCollection; this.dataSource = dataSource; diff --git a/packages/datasource-toolkit/src/index.ts b/packages/datasource-toolkit/src/index.ts index 97048ff3e8..7f1e499b40 100644 --- a/packages/datasource-toolkit/src/index.ts +++ b/packages/datasource-toolkit/src/index.ts @@ -7,7 +7,7 @@ export { MAP_ALLOWED_OPERATORS_FOR_COLUMN_TYPE as allowedOperatorsForColumnType export { default as BaseCollection } from './base-collection'; export { default as BaseDataSource } from './base-datasource'; export { default as DataSourceDecorator } from './decorators/datasource-decorator'; -export { default as CollectionDecorator } from './decorators/collection-decorator'; +export { default as CollectionDecorator, SearchedField } from './decorators/collection-decorator'; // Query Interface export { default as Aggregation } from './interfaces/query/aggregation'; From 46659eb72d962d967a87e3f4285c390db23060f6 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 20 Aug 2026 18:29:54 +0200 Subject: [PATCH 05/10] test(agent): name the suite after the method it pins The describe block named a symbol that does not exist, so a grep for the suite pinning the redact-vs-refuse policy found nothing. Also renames the chart guard's path argument, which is not always the collection whose name is passed as the permission root. Co-Authored-By: Claude Opus 5 --- packages/agent/src/routes/access/chart.ts | 4 ++-- .../agent/test/services/authorization/authorization.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/agent/src/routes/access/chart.ts b/packages/agent/src/routes/access/chart.ts index 5ea89ffede..7b4e114985 100644 --- a/packages/agent/src/routes/access/chart.ts +++ b/packages/agent/src/routes/access/chart.ts @@ -299,7 +299,7 @@ export default class ChartRoute extends CollectionRoute { private async assertCanReadAggregatedFields( context: Context, - collection: Collection, + pathCollection: Collection, fields: Array<[action: string, path: string]>, ): Promise { await this.services.authorization.assertCanReadUsages( @@ -310,7 +310,7 @@ export default class ChartRoute extends CollectionRoute { .map(([action, path]) => ({ action, path, - collectionName: FieldPathUtils.getLeafCollection(collection, path).name, + collectionName: FieldPathUtils.getLeafCollection(pathCollection, path).name, })), ); } diff --git a/packages/agent/test/services/authorization/authorization.test.ts b/packages/agent/test/services/authorization/authorization.test.ts index 95de5a7bf5..8b6dfa0967 100644 --- a/packages/agent/test/services/authorization/authorization.test.ts +++ b/packages/agent/test/services/authorization/authorization.test.ts @@ -435,7 +435,7 @@ describe('AuthorizationService', () => { }); }); - describe('keepReadableProjection', () => { + describe('redactProjection', () => { const buildDataSource = () => factories.dataSource.buildWithCollections([ factories.collection.build({ From bc3925a48e7ea14c2bf24402dda32a9daf6a9b79 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 09:45:00 +0200 Subject: [PATCH 06/10] fix(agent): redact the record an update serializes back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PUT re-listed the row it had just written with a projection covering every to-one relation, so edit on a collection returned columns of collections the caller cannot read — the same disclosure the read routes now refuse, one HTTP verb away. The projection is agent-chosen there, so it is redacted rather than refused: a write must not fail because the row carries a relation the caller may not read. Co-Authored-By: Claude Opus 5 --- .../agent/src/routes/modification/update.ts | 5 +- .../security/related-read-permissions.test.ts | 54 +++++++++++++++++++ .../src/decorators/search/field-paths.ts | 42 +++++++++------ 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/packages/agent/src/routes/modification/update.ts b/packages/agent/src/routes/modification/update.ts index a19f832218..506b078d26 100644 --- a/packages/agent/src/routes/modification/update.ts +++ b/packages/agent/src/routes/modification/update.ts @@ -40,7 +40,10 @@ export default class UpdateRoute extends CollectionRoute { const [updateResult] = await this.collection.list( caller, new Filter({ conditionTree }), - ProjectionFactory.all(this.collection), + await this.services.authorization.redactProjection(context, this.collection, { + projection: ProjectionFactory.all(this.collection), + explicit: false, + }), ); context.response.body = this.services.serializer.serialize(this.collection, updateResult); diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index a8bdc5b162..cae8900ab9 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -11,6 +11,7 @@ import CsvRelated from '../../src/routes/access/csv-related'; import Get from '../../src/routes/access/get'; import List from '../../src/routes/access/list'; import ListRelated from '../../src/routes/access/list-related'; +import Update from '../../src/routes/modification/update'; import AuthorizationService from '../../src/services/authorization/authorization'; import * as factories from '../__factories__'; @@ -389,6 +390,59 @@ describe('read permissions on related collections', () => { }); }); + // `edit` is the entry point here, not `read`: the route re-lists the row it just wrote with a + // projection the caller never supplied, so the same disclosure is one HTTP verb away. + describe('update', () => { + const updateContext = () => + buildContext( + { params: { id: '2d162303-78bf-599e-b197-93590ac3d315' } }, + {}, + { + data: { + id: '2d162303-78bf-599e-b197-93590ac3d315', + attributes: { panLast4: '4242' }, + }, + }, + ); + + it('should redact the record it serializes back rather than refusing the write', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const cards = dataSource.getCollection('cards'); + cards.update = jest.fn(); + const list = jest.spyOn(cards, 'list').mockResolvedValue([{ id: 'a-card' }]); + + await new Update(services, options, dataSource, 'cards').handleUpdate(updateContext()); + + expect([...list.mock.calls[0][2]].sort()).toEqual([ + 'accountId', + 'holderId', + 'id', + 'panLast4', + ]); + }); + + it('should keep the columns of a collection the caller may read', async () => { + const dataSource = buildDataSource(); + const services = buildServices(['holders']); + const cards = dataSource.getCollection('cards'); + cards.update = jest.fn(); + const list = jest.spyOn(cards, 'list').mockResolvedValue([{ id: 'a-card' }]); + + await new Update(services, options, dataSource, 'cards').handleUpdate(updateContext()); + + expect([...list.mock.calls[0][2]].sort()).toEqual([ + 'accountId', + 'holder:fullName', + 'holder:id', + 'holder:nationalId', + 'holderId', + 'id', + 'panLast4', + ]); + }); + }); + // The related routes are the only sites resolving against `foreignCollection` rather than the // route's own collection, so a swap between the two would otherwise go unnoticed. describe('related routes', () => { diff --git a/packages/datasource-customizer/src/decorators/search/field-paths.ts b/packages/datasource-customizer/src/decorators/search/field-paths.ts index cdac235365..3588337079 100644 --- a/packages/datasource-customizer/src/decorators/search/field-paths.ts +++ b/packages/datasource-customizer/src/decorators/search/field-paths.ts @@ -1,30 +1,40 @@ -import type { Collection, ColumnSchema } from '@forestadmin/datasource-toolkit'; +import type { + Collection, + ColumnSchema, + FieldSchema, + RelationSchema, +} from '@forestadmin/datasource-toolkit'; import normalizeName from './normalize-name'; import { extractSpecifiedFields, parseQuery } from './parse-query'; +const SEARCHABLE_THROUGH = ['OneToMany', 'ManyToOne', 'OneToOne']; + +const isSearchableThrough = (schema: FieldSchema): schema is RelationSchema => + SEARCHABLE_THROUGH.includes(schema.type); + export function lenientGetSchema( collection: Collection, path: string, ): { field: string; schema: ColumnSchema } | null { const [prefix, suffix] = path.split(/:(.*)/); const fuzzyPrefix = normalizeName(prefix); + const matches = Object.entries(collection.schema.fields).filter( + ([field]) => fuzzyPrefix === normalizeName(field), + ); + + if (!suffix) { + const column = matches.find(([, schema]) => schema.type === 'Column'); + + return column ? { field: column[0], schema: column[1] as ColumnSchema } : null; + } + + for (const [field, schema] of matches) { + if (isSearchableThrough(schema)) { + const related = collection.dataSource.getCollection(schema.foreignCollection); + const fuzzy = lenientGetSchema(related, suffix); - for (const [field, schema] of Object.entries(collection.schema.fields)) { - if (fuzzyPrefix === normalizeName(field)) { - if (!suffix && schema.type === 'Column') { - return { field, schema }; - } - - if ( - suffix && - (schema.type === 'OneToMany' || schema.type === 'ManyToOne' || schema.type === 'OneToOne') - ) { - const related = collection.dataSource.getCollection(schema.foreignCollection); - const fuzzy = lenientGetSchema(related, suffix); - - if (fuzzy) return { field: `${field}:${fuzzy.field}`, schema: fuzzy.schema }; - } + if (fuzzy) return { field: `${field}:${fuzzy.field}`, schema: fuzzy.schema }; } } From 2e3e7b5f5efbaa73fc545d28062f8d4615eee2e4 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 10:11:56 +0200 Subject: [PATCH 07/10] fix(agent): fail closed when a searched path does not resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unresolvable prefix answered with the collection it was asked about, which the caller pins to readable — so "this path does not resolve" read as "this path is allowed". It now goes through the same resolver as the agent side, which throws. The two permissive search tests granted permissions they never used: the collection factory defines no getSearchedFields, so the guard saw an empty list and passed with no permissions at all. They now stub it, and fail when the grant is removed. Co-Authored-By: Claude Opus 5 --- .../security/related-read-permissions.test.ts | 14 ++++- .../src/decorators/search/field-paths.ts | 15 ++++- .../decorators/search/field-paths.test.ts | 60 +++++++++++++++++++ 3 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 packages/datasource-customizer/test/decorators/search/field-paths.test.ts diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index cae8900ab9..1f11fac907 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -349,7 +349,10 @@ describe('read permissions on related collections', () => { it('should accept a plain search, which never leaves the root collection', async () => { const dataSource = buildDataSource(); const services = buildServices(); - const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + const cards = dataSource.getCollection('cards') as CollectionDecorator; + const list = jest.spyOn(cards, 'list').mockResolvedValue([]); + + cards.getSearchedFields = () => [{ path: 'panLast4', collection: 'cards' }]; await new List(services, options, dataSource, 'cards').handleList( buildContext({ query: { search: 'martin' } }), @@ -361,7 +364,14 @@ describe('read permissions on related collections', () => { it('should accept an extended search once every to-one relation is readable', async () => { const dataSource = buildDataSource(); const services = buildServices(['holders', 'accounts']); - const list = jest.spyOn(dataSource.getCollection('cards'), 'list').mockResolvedValue([]); + const cards = dataSource.getCollection('cards') as CollectionDecorator; + const list = jest.spyOn(cards, 'list').mockResolvedValue([]); + + cards.getSearchedFields = () => [ + { path: 'panLast4', collection: 'cards' }, + { path: 'holder:fullName', collection: 'holders' }, + { path: 'account:iban', collection: 'accounts' }, + ]; await new List(services, options, dataSource, 'cards').handleList( buildContext({ query: { search: 'martin', searchExtended: '1' } }), diff --git a/packages/datasource-customizer/src/decorators/search/field-paths.ts b/packages/datasource-customizer/src/decorators/search/field-paths.ts index 3588337079..92c38c2d7f 100644 --- a/packages/datasource-customizer/src/decorators/search/field-paths.ts +++ b/packages/datasource-customizer/src/decorators/search/field-paths.ts @@ -5,6 +5,8 @@ import type { RelationSchema, } from '@forestadmin/datasource-toolkit'; +import { SchemaUtils } from '@forestadmin/datasource-toolkit'; + import normalizeName from './normalize-name'; import { extractSpecifiedFields, parseQuery } from './parse-query'; @@ -51,14 +53,21 @@ export function getSearchedFieldPaths(collection: Collection, search: string): s .filter(Boolean); } +/** + * The collection a path's last column belongs to — the one a read permission applies to. A prefix + * that names no relation throws rather than falling back to `collection`, which the caller pins to + * readable: an unresolvable path must not read as an allowed one. + */ export function getLeafCollectionName(collection: Collection, path: string): string { const index = path.indexOf(':'); if (index === -1) return collection.name; - const relation = collection.schema.fields[path.substring(0, index)]; - - if (!relation || relation.type === 'Column') return collection.name; + const relation = SchemaUtils.getRelation( + collection.schema, + path.substring(0, index), + collection.name, + ); return getLeafCollectionName( collection.dataSource.getCollection(relation.foreignCollection), diff --git a/packages/datasource-customizer/test/decorators/search/field-paths.test.ts b/packages/datasource-customizer/test/decorators/search/field-paths.test.ts new file mode 100644 index 0000000000..2757fc6b82 --- /dev/null +++ b/packages/datasource-customizer/test/decorators/search/field-paths.test.ts @@ -0,0 +1,60 @@ +import * as factories from '@forestadmin/datasource-toolkit/dist/test/__factories__'; + +import { getLeafCollectionName } from '../../../src/decorators/search/field-paths'; + +describe('getLeafCollectionName', () => { + const buildDataSource = () => + factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'cards', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + panLast4: factories.columnSchema.build({ columnType: 'String' }), + holderId: factories.columnSchema.build({ columnType: 'Uuid' }), + holder: factories.manyToOneSchema.build({ + foreignCollection: 'holders', + foreignKey: 'holderId', + }), + }, + }), + }), + factories.collection.build({ + name: 'holders', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + nationalId: factories.columnSchema.text().build(), + }, + }), + }), + ]); + + it('should return the collection owning the last column of the path', () => { + const cards = buildDataSource().getCollection('cards'); + + expect(getLeafCollectionName(cards, 'holder:nationalId')).toBe('holders'); + }); + + it('should return the collection itself for one of its own columns', () => { + const cards = buildDataSource().getCollection('cards'); + + expect(getLeafCollectionName(cards, 'panLast4')).toBe('cards'); + }); + + // The caller pins the collection it asked about to readable, so answering `cards` here would + // turn "this path does not resolve" into "this path is allowed". + it('should throw rather than fall back to the collection it was asked about', () => { + const cards = buildDataSource().getCollection('cards'); + + expect(() => getLeafCollectionName(cards, 'panLast4:nationalId')).toThrow( + /relation.*panLast4|panLast4.*not/i, + ); + }); + + it('should throw when the prefix names nothing at all', () => { + const cards = buildDataSource().getCollection('cards'); + + expect(() => getLeafCollectionName(cards, 'unknown:nationalId')).toThrow(); + }); +}); From 680ec65791b7b296cea54d5f76de6dbefc5c014f Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 10:36:14 +0200 Subject: [PATCH 08/10] refactor(agent): name the projection flag instead of commenting it `explicit` needed a doc comment to say what it was explicit about. The call site in update.ts now reads as its own explanation, and the comment is gone. Also drops the parts of the search comment that repeat what the two declaration sites document since the resolution moved down the stack. Co-Authored-By: Claude Opus 5 --- .../agent/src/routes/modification/update.ts | 2 +- .../services/authorization/authorization.ts | 11 ++++------- packages/agent/src/utils/query-string.ts | 7 +++---- .../security/related-read-permissions.test.ts | 1 - .../authorization/authorization.test.ts | 8 ++++---- packages/agent/test/utils/query-string.test.ts | 18 +++++++++--------- 6 files changed, 21 insertions(+), 26 deletions(-) diff --git a/packages/agent/src/routes/modification/update.ts b/packages/agent/src/routes/modification/update.ts index 506b078d26..2c86ba0119 100644 --- a/packages/agent/src/routes/modification/update.ts +++ b/packages/agent/src/routes/modification/update.ts @@ -42,7 +42,7 @@ export default class UpdateRoute extends CollectionRoute { new Filter({ conditionTree }), await this.services.authorization.redactProjection(context, this.collection, { projection: ProjectionFactory.all(this.collection), - explicit: false, + namedByCaller: false, }), ); diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index 14739e2cd2..3172a3b7e8 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -67,12 +67,12 @@ export default class AuthorizationService { collection: Collection, requested: RequestedProjection, ): Promise { - const { projection, explicit } = requested; + const { projection, namedByCaller } = requested; const owners = projection.map(path => FieldPathUtils.getLeafCollection(collection, path).name); const permissions = await this.getReadPermissions(context, collection.name, owners); const isReadable = (index: number) => permissions.get(owners[index]); - if (explicit) { + if (namedByCaller) { const denied = projection .map((field, index) => ({ field, collection: owners[index] })) .filter((_, index) => !isReadable(index)); @@ -114,11 +114,8 @@ export default class AuthorizationService { const search = QueryStringParser.parseSearch(collection, context); if (search) { - // Asked of the stack rather than derived from the schema: `relation.column:term` is end-user - // syntax that needs no extended search, and the fields an extended one reaches are read below - // the publication and renaming layers. A `null` answer means the collection cannot say — a - // replaced search, where the customer's handler picks the fields and the caller only supplies - // the text. + // Asked whatever the extended flag: `relation.column:term` is end-user syntax and reaches a + // relation without it. An unknown answer serves the request — the `replaceSearch` exemption. const searched = (collection as CollectionDecorator).getSearchedFields?.( search, QueryStringParser.parseSearchExtended(context), diff --git a/packages/agent/src/utils/query-string.ts b/packages/agent/src/utils/query-string.ts index dcb4f951c6..08a046530e 100644 --- a/packages/agent/src/utils/query-string.ts +++ b/packages/agent/src/utils/query-string.ts @@ -21,7 +21,7 @@ import { getRequestId } from './correlation-id'; const DEFAULT_ITEMS_PER_PAGE = 15; const DEFAULT_PAGE_TO_SKIP = 1; -export type RequestedProjection = { projection: Projection; explicit: boolean }; +export type RequestedProjection = { projection: Projection; namedByCaller: boolean }; export default class QueryStringParser { private static VALID_TIMEZONES = new Set(); @@ -88,20 +88,19 @@ export default class QueryStringParser { } } - /** `explicit` is false when the projection is the default `ProjectionFactory.all` expansion. */ static parseProjectionFromHeaderOrQuery( collection: Collection, context: Context, ): RequestedProjection { const fromHeader = QueryStringParser.parseProjectionFromHeader(collection, context); - if (fromHeader) return { projection: fromHeader, explicit: true }; + if (fromHeader) return { projection: fromHeader, namedByCaller: true }; const fields = context.request.query[`fields[${collection.name}]`]; return { projection: QueryStringParser.parseProjection(collection, context), - explicit: fields !== '' && fields !== undefined, + namedByCaller: fields !== '' && fields !== undefined, }; } diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index 1f11fac907..e09bfe627c 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -300,7 +300,6 @@ describe('read permissions on related collections', () => { const cards = dataSource.getCollection('cards') as CollectionDecorator; const aggregate = jest.spyOn(cards, 'aggregate'); - // `relation.column:term` needs no extended search, and only the stack knows it resolves. cards.getSearchedFields = () => [{ path: 'holder:nationalId', collection: 'holders' }]; await expect( diff --git a/packages/agent/test/services/authorization/authorization.test.ts b/packages/agent/test/services/authorization/authorization.test.ts index 8b6dfa0967..30ebb5a815 100644 --- a/packages/agent/test/services/authorization/authorization.test.ts +++ b/packages/agent/test/services/authorization/authorization.test.ts @@ -474,7 +474,7 @@ describe('AuthorizationService', () => { const projection = await authorizationService.redactProjection( context, buildDataSource().getCollection('cards'), - { projection: new Projection('id', 'holderId'), explicit: true }, + { projection: new Projection('id', 'holderId'), namedByCaller: true }, ); expect(projection).toEqual(['id', 'holderId']); @@ -492,7 +492,7 @@ describe('AuthorizationService', () => { buildDataSource().getCollection('cards'), { projection: new Projection('id', 'holder:fullName', 'holder:nationalId'), - explicit: true, + namedByCaller: true, }, ); @@ -515,7 +515,7 @@ describe('AuthorizationService', () => { buildDataSource().getCollection('cards'), { projection: new Projection('id', 'holder:fullName', 'holder:nationalId'), - explicit: false, + namedByCaller: false, }, ); @@ -531,7 +531,7 @@ describe('AuthorizationService', () => { await expect( authorizationService.redactProjection(context, buildDataSource().getCollection('cards'), { projection: new Projection('id', 'holder:fullName', 'holder:nationalId'), - explicit: true, + namedByCaller: true, }), ).rejects.toThrow( "You are not allowed to read 'holder:fullName' from the 'holders' collection, " + diff --git a/packages/agent/test/utils/query-string.test.ts b/packages/agent/test/utils/query-string.test.ts index 720b5e7b0c..d19022f457 100644 --- a/packages/agent/test/utils/query-string.test.ts +++ b/packages/agent/test/utils/query-string.test.ts @@ -430,7 +430,7 @@ describe('QueryStringParser', () => { context, ); - expect(projection).toEqual({ projection: new Projection('name'), explicit: true }); + expect(projection).toEqual({ projection: new Projection('name'), namedByCaller: true }); }); test('should fallback to the query string when the header is missing', () => { @@ -443,7 +443,7 @@ describe('QueryStringParser', () => { context, ); - expect(projection).toEqual({ projection: new Projection('name'), explicit: true }); + expect(projection).toEqual({ projection: new Projection('name'), namedByCaller: true }); }); test('should fallback to the query string when the header is empty', () => { @@ -457,31 +457,31 @@ describe('QueryStringParser', () => { context, ); - expect(projection).toEqual({ projection: new Projection('name'), explicit: true }); + expect(projection).toEqual({ projection: new Projection('name'), namedByCaller: true }); }); - test('should report the default expansion as not explicit when no field is requested', () => { + test('should report the default expansion as not named by the caller', () => { const context = createMockContext({ customProperties: { query: {} } }); - const { explicit } = QueryStringParser.parseProjectionFromHeaderOrQuery( + const { namedByCaller } = QueryStringParser.parseProjectionFromHeaderOrQuery( collectionSimple, context, ); - expect(explicit).toBe(false); + expect(namedByCaller).toBe(false); }); - test('should report an empty fields query param as not explicit', () => { + test('should report an empty fields query param as not named by the caller', () => { const context = createMockContext({ customProperties: { query: { 'fields[books]': '' } }, }); - const { explicit } = QueryStringParser.parseProjectionFromHeaderOrQuery( + const { namedByCaller } = QueryStringParser.parseProjectionFromHeaderOrQuery( collectionSimple, context, ); - expect(explicit).toBe(false); + expect(namedByCaller).toBe(false); }); test('should throw on an invalid header instead of falling back to the query string', () => { From aeb3c3c2f0eb30fe3a4a250e460be8e36e7a9fec Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 10:48:39 +0200 Subject: [PATCH 09/10] test(agent): state what withPks re-adds, and when it is harmless The comment claimed a ManyToOne makes the re-added key redundant. That holds only when foreignKeyTarget is the primary key, which the schema does not require. Co-Authored-By: Claude Opus 5 --- .../agent/test/security/related-read-permissions.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index e09bfe627c..f3e144d2d7 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -180,8 +180,11 @@ describe('read permissions on related collections', () => { buildContext({}, { 'forest-projection': 'id,account:organization:name' }), ); - // `withPks` re-adds `account:id`, which a ManyToOne already carries on the row as - // `cards.accountId`. A OneToOne intermediate would expose a key the row does not carry. + // `withPks` runs after the check and re-adds a key per surviving relation, so `account:id` + // comes back from a collection the caller cannot read. It carries nothing new only here, + // where the ManyToOne targets the primary key and the row already holds it as + // `cards.accountId`. A OneToOne, or a `foreignKeyTarget` that is not the primary key, + // exposes a key the row does not carry. expect([...list.mock.calls[0][2]].sort()).toEqual([ 'account:id', 'account:organization:id', From 938559242ea7641bb3570a294b988e0ba20a5ffc Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 15:54:33 +0200 Subject: [PATCH 10/10] fix(agent): check only the query components a route applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard read filters, sorts and searches on every route, but ContextFilterFactory.build carries no sort — only buildPaginated adds one. So count, count-related and the chart routes refused a sort naming a denied collection on a request that sort never reached. Each route now names what it consumes. The default stays all three, so a new route is checked fully until it says otherwise. Co-Authored-By: Claude Opus 5 --- packages/agent/src/routes/access/chart.ts | 5 ++- .../agent/src/routes/access/count-related.ts | 5 ++- packages/agent/src/routes/access/count.ts | 5 ++- .../services/authorization/authorization.ts | 32 +++++++++++++++---- .../security/related-read-permissions.test.ts | 16 ++++++++++ 5 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/agent/src/routes/access/chart.ts b/packages/agent/src/routes/access/chart.ts index 7b4e114985..90ca1da19b 100644 --- a/packages/agent/src/routes/access/chart.ts +++ b/packages/agent/src/routes/access/chart.ts @@ -69,7 +69,10 @@ export default class ChartRoute extends CollectionRoute { chartRequest, }); - await this.services.authorization.assertCanReadQueryFields(context, this.collection); + await this.services.authorization.assertCanReadQueryFields(context, this.collection, [ + 'filter', + 'search', + ]); switch (chartRequest.type) { case ChartType.Value: diff --git a/packages/agent/src/routes/access/count-related.ts b/packages/agent/src/routes/access/count-related.ts index 05490589fc..3d0a45d80d 100644 --- a/packages/agent/src/routes/access/count-related.ts +++ b/packages/agent/src/routes/access/count-related.ts @@ -20,7 +20,10 @@ export default class CountRelatedRoute extends RelationRoute { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); if (this.foreignCollection.schema.countable) { - await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection); + await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection, [ + 'filter', + 'search', + ]); const parentId = IdUtils.unpackId(this.collection.schema, context.params.parentId); const scope = await this.services.authorization.getScope(this.foreignCollection, context); diff --git a/packages/agent/src/routes/access/count.ts b/packages/agent/src/routes/access/count.ts index d9a3c3508d..a6802e043e 100644 --- a/packages/agent/src/routes/access/count.ts +++ b/packages/agent/src/routes/access/count.ts @@ -16,7 +16,10 @@ export default class CountRoute extends CollectionRoute { await this.services.authorization.assertCanBrowse(context, this.collection.name); if (this.collection.schema.countable) { - await this.services.authorization.assertCanReadQueryFields(context, this.collection); + await this.services.authorization.assertCanReadQueryFields(context, this.collection, [ + 'filter', + 'search', + ]); const scope = await this.services.authorization.getScope(this.collection, context); const caller = QueryStringParser.parseCaller(context); diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index 3172a3b7e8..fa4ac5a93f 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -22,6 +22,10 @@ import QueryStringParser from '../../utils/query-string'; type FieldUsage = { action: string; path: string; collectionName: string }; +export type QueryComponent = 'filter' | 'sort' | 'search'; + +const ALL_QUERY_COMPONENTS: QueryComponent[] = ['filter', 'sort', 'search']; + export default class AuthorizationService { constructor(private readonly forestAdminClient: ForestAdminClient) {} @@ -93,8 +97,16 @@ export default class AuthorizationService { * Refused rather than redacted: dropping a condition widens the result set and dropping a sort * clause silently reorders it, while both leak the value they touch anyway — a `starts_with` * filter answers one guess per request without returning a column of its own. + * + * `consumes` names the query components the calling route applies to its filter. Checking one it + * drops would refuse a request the denied field cannot reach: `ContextFilterFactory.build` carries + * no sort, which only `buildPaginated` adds. */ - public async assertCanReadQueryFields(context: Context, collection: Collection): Promise { + public async assertCanReadQueryFields( + context: Context, + collection: Collection, + consumes: QueryComponent[] = ALL_QUERY_COMPONENTS, + ): Promise { const usages: FieldUsage[] = []; const push = (action: string, path: string) => usages.push({ @@ -103,15 +115,21 @@ export default class AuthorizationService { collectionName: FieldPathUtils.getLeafCollection(collection, path).name, }); - QueryStringParser.parseConditionTree(collection, context)?.forEachLeaf(leaf => - push('filter on', leaf.field), - ); + if (consumes.includes('filter')) { + QueryStringParser.parseConditionTree(collection, context)?.forEachLeaf(leaf => + push('filter on', leaf.field), + ); + } - for (const { field } of QueryStringParser.parseSort(collection, context)) { - push('sort on', field); + if (consumes.includes('sort')) { + for (const { field } of QueryStringParser.parseSort(collection, context)) { + push('sort on', field); + } } - const search = QueryStringParser.parseSearch(collection, context); + const search = consumes.includes('search') + ? QueryStringParser.parseSearch(collection, context) + : null; if (search) { // Asked whatever the extended flag: `relation.column:term` is end-user syntax and reaches a diff --git a/packages/agent/test/security/related-read-permissions.test.ts b/packages/agent/test/security/related-read-permissions.test.ts index f3e144d2d7..991f83d25c 100644 --- a/packages/agent/test/security/related-read-permissions.test.ts +++ b/packages/agent/test/security/related-read-permissions.test.ts @@ -281,6 +281,22 @@ describe('read permissions on related collections', () => { }); describe('sort', () => { + // A count applies no sort — `ContextFilterFactory.build` carries none, only `buildPaginated` + // does — so refusing one would refuse a request the denied field cannot reach. + it('should ignore a sort on the count route, which never applies one', async () => { + const dataSource = buildDataSource(); + const services = buildServices(); + const aggregate = jest + .spyOn(dataSource.getCollection('cards'), 'aggregate') + .mockResolvedValue([{ value: 3, group: {} }]); + + await new Count(services, options, dataSource, 'cards').handleCount( + buildContext({ query: { sort: '-holder.nationalId' } }), + ); + + expect(aggregate).toHaveBeenCalled(); + }); + it('should refuse a sort reading a collection the caller cannot read', async () => { const dataSource = buildDataSource(); const services = buildServices();