From 4ce5c1e8354d45c89a54a289ee1992bd44d5593f Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Tue, 4 Aug 2026 16:52:55 +0200 Subject: [PATCH 1/2] feat(agent-bff): validate relation filters against the foreign collection --- .../src/data/data-routes-middleware.ts | 53 ++- .../test/data/data-routes-middleware.test.ts | 303 +++++++++++++++++- 2 files changed, 350 insertions(+), 6 deletions(-) diff --git a/packages/agent-bff/src/data/data-routes-middleware.ts b/packages/agent-bff/src/data/data-routes-middleware.ts index b06c131aef..d7ea323813 100644 --- a/packages/agent-bff/src/data/data-routes-middleware.ts +++ b/packages/agent-bff/src/data/data-routes-middleware.ts @@ -81,11 +81,12 @@ function assertCollectionStillAllowed(readModel: ReadModel, collection: string): function resolveCapabilities( deps: RequestHandlerDeps, + collection: string, ): Promise<{ capabilities: CapabilitiesResult; readModel: ReadModel }> { return callAgent( () => deps.store.getCapabilities( - deps.collection, + collection, createAgentCapabilitiesFetcher({ agentUrl: deps.agentUrl, token: deps.token, @@ -111,7 +112,7 @@ async function handleList(ctx: Context, body: ListRequestBody, deps: ListHandler let { primaryKeys } = deps; if (hasCapabilityConstrainedInput(validationInput)) { - const { capabilities, readModel } = await resolveCapabilities(deps); + const { capabilities, readModel } = await resolveCapabilities(deps, deps.collection); assertCollectionStillAllowed(readModel, deps.collection); assertValidAgainstCapabilities(validationInput, capabilities); primaryKeys = readModel.getPrimaryKeys(deps.collection); @@ -129,7 +130,7 @@ async function handleCount(ctx: Context, body: CountRequestBody, deps: RequestHa // Count carries only a filter (no sort/projection), so that is all there is to validate. if (body.filter !== undefined) { - const { capabilities, readModel } = await resolveCapabilities(deps); + const { capabilities, readModel } = await resolveCapabilities(deps, deps.collection); assertCollectionStillAllowed(readModel, deps.collection); assertValidAgainstCapabilities({ filter: body.filter }, capabilities); } @@ -150,6 +151,31 @@ interface RelationHandlerDeps extends RequestHandlerDeps { type RelationListHandlerDeps = RelationHandlerDeps & { primaryKeys: PrimaryKeyField[] }; +// The route resolved parent, relation and foreign collection against an earlier generation; a schema +// refresh during the capabilities fetch can invalidate any of the three, so all three are re-checked +// against the generation the capabilities belong to. The foreign name must come out unchanged: the +// capabilities are cached under it, the agent query projects under it and the response is stamped +// with it, so a re-targeted relation would validate one collection and serve another. +async function resolveRelationCapabilities( + deps: RelationHandlerDeps, +): Promise<{ capabilities: CapabilitiesResult; primaryKeys: PrimaryKeyField[] }> { + const { capabilities, readModel } = await resolveCapabilities(deps, deps.foreignCollection); + + assertCollectionStillAllowed(readModel, deps.collection); + + const foreignCollection = resolveForeignCollection( + readModel.getRelationTarget(deps.collection, deps.relation), + ); + + if (foreignCollection !== deps.foreignCollection) { + throw unknownRelation(`Unknown relation: ${deps.collection}.${deps.relation}`); + } + + assertCollectionStillAllowed(readModel, foreignCollection); + + return { capabilities, primaryKeys: readModel.getPrimaryKeys(foreignCollection) }; +} + async function handleRelationList( ctx: Context, body: RelationListRequestBody, @@ -160,6 +186,20 @@ async function handleRelationList( // browse is never checked. Plain foreign fields (no `:`) are unaffected. assertNoRelationFieldPaths(collectListFieldPaths(body)); + const validationInput = { + filter: body.filter, + sortFields: body.sort?.map(clause => clause.field), + projectionFields: body.projection, + }; + + let { primaryKeys } = deps; + + if (hasCapabilityConstrainedInput(validationInput)) { + const resolved = await resolveRelationCapabilities(deps); + assertValidAgainstCapabilities(validationInput, resolved.capabilities); + primaryKeys = resolved.primaryKeys; + } + const query = buildListAgentQuery(deps.foreignCollection, deps.timezone, body); const records = await callAgent( () => deps.client.listRelation(deps.collection, body.parentId, deps.relation, query), @@ -167,7 +207,7 @@ async function handleRelationList( ); ctx.status = 200; - ctx.body = mapListResponse(deps.foreignCollection, records, deps.primaryKeys); + ctx.body = mapListResponse(deps.foreignCollection, records, primaryKeys); } async function handleRelationCount( @@ -177,6 +217,11 @@ async function handleRelationCount( ) { assertNoRelationFieldPaths(collectCountFieldPaths(body)); + if (body.filter !== undefined) { + const { capabilities } = await resolveRelationCapabilities(deps); + assertValidAgainstCapabilities({ filter: body.filter }, capabilities); + } + const query = buildCountAgentQuery(deps.timezone, body); const raw = await callAgent( () => deps.client.countRelationRaw(deps.collection, body.parentId, deps.relation, query), diff --git a/packages/agent-bff/test/data/data-routes-middleware.test.ts b/packages/agent-bff/test/data/data-routes-middleware.test.ts index 1cbe66232d..0c8830c785 100644 --- a/packages/agent-bff/test/data/data-routes-middleware.test.ts +++ b/packages/agent-bff/test/data/data-routes-middleware.test.ts @@ -764,7 +764,7 @@ describe('data routes middleware', () => { .send({ parentId: '7', projection: ['id', 'title'], - filter: { field: 'title', operator: 'present' }, + filter: { field: 'title', operator: 'Present' }, sort: [{ field: 'title', direction: 'desc' }], }); @@ -774,7 +774,7 @@ describe('data routes middleware', () => { 'posts', expect.objectContaining({ 'fields[posts]': 'id,title', - filters: JSON.stringify({ field: 'title', operator: 'present' }), + filters: JSON.stringify({ field: 'title', operator: 'Present' }), sort: '-title', }), ); @@ -991,4 +991,303 @@ describe('data routes middleware', () => { expect(countRelationRaw).not.toHaveBeenCalled(); }); }); + + describe('relation capabilities validation', () => { + // The parent is deliberately the permissive one: a lookup against `users` would accept every + // field below, so any test here that passes while the implementation reads the parent's + // capabilities would be proving nothing. + const perCollectionCapabilities: CapabilitiesStub = async collectionName => { + if (collectionName === 'posts') { + return { fields: [{ name: 'title', type: 'String', operators: ['present'] }] }; + } + + return { + fields: ['id', 'email', 'title', 'ghost'].map(name => ({ + name, + type: 'String', + operators: BROAD_SNAKE_OPERATORS, + })), + }; + }; + + it('should fetch capabilities for the foreign collection, not the parent', async () => { + const listRelation = jest.fn(async () => []); + const getCapabilities = jest.fn(perCollectionCapabilities); + const app = buildApp(storeOf(relationReadModel, getCapabilities), { listRelation }); + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', projection: ['title'] }); + + expect(response.status).toBe(200); + expect(getCapabilities).toHaveBeenCalledWith('posts'); + expect(getCapabilities).not.toHaveBeenCalledWith('users'); + }); + + it('should reject a projection field that only exists on the parent collection', async () => { + const listRelation = jest.fn(async () => []); + const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { listRelation }); + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', projection: ['email'] }); + + expect(response.status).toBe(422); + expect(response.body.error).toMatchObject({ + type: 'unknown_field', + status: 422, + details: { field: 'email' }, + }); + expect(listRelation).not.toHaveBeenCalled(); + }); + + it('should reject a relation list sort field absent from the foreign capabilities', async () => { + const listRelation = jest.fn(async () => []); + const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { listRelation }); + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', sort: [{ field: 'ghost', direction: 'asc' }] }); + + expect(response.status).toBe(422); + expect(response.body.error).toMatchObject({ + type: 'unknown_field', + status: 422, + details: { field: 'ghost' }, + }); + expect(listRelation).not.toHaveBeenCalled(); + }); + + it('should reject a relation list filter operator the foreign capabilities do not support', async () => { + const listRelation = jest.fn(async () => []); + const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { listRelation }); + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', filter: { field: 'title', operator: 'Equal' } }); + + expect(response.status).toBe(400); + expect(response.body.error).toMatchObject({ + type: 'invalid_filter_operator', + status: 400, + details: { field: 'title', validOperators: ['Present'] }, + }); + expect(listRelation).not.toHaveBeenCalled(); + }); + + it('should reject a relation count filter operator the foreign capabilities do not support', async () => { + const countRelationRaw = jest.fn(async () => ({ count: 0 })); + const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { + countRelationRaw, + }); + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/count') + .send({ parentId: '7', filter: { field: 'title', operator: 'Equal' } }); + + expect(response.status).toBe(400); + expect(response.body.error).toMatchObject({ + type: 'invalid_filter_operator', + status: 400, + details: { field: 'title', validOperators: ['Present'] }, + }); + expect(countRelationRaw).not.toHaveBeenCalled(); + }); + + it('should read the foreign capabilities before calling the agent', async () => { + const calls: string[] = []; + const listRelation = jest.fn(async () => { + calls.push('agent'); + + return []; + }); + const getCapabilities = jest.fn(async (collectionName: string) => { + calls.push(`capabilities:${collectionName}`); + + return perCollectionCapabilities(collectionName); + }); + const app = buildApp(storeOf(relationReadModel, getCapabilities), { listRelation }); + + await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', projection: ['title'] }); + + expect(calls).toEqual(['capabilities:posts', 'agent']); + }); + + it.each([['list'], ['count']])( + 'should skip the capabilities fetch when a relation %s carries nothing to validate', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + const getCapabilities = jest.fn(async () => { + throw new AgentHttpError(503, {}, 'Service Unavailable'); + }); + const app = buildApp(storeOf(relationReadModel, getCapabilities), client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7' }); + + expect(response.status).toBe(200); + expect(getCapabilities).not.toHaveBeenCalled(); + }, + ); + + it.each([['list'], ['count']])( + 'should return 404 on a relation %s when a refresh during the capabilities read drops the foreign collection', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + // The newer generation keeps users.posts as a relation but no longer exposes posts itself. + const refreshedStore = { + getReadModel: async () => relationReadModel, + getCapabilities: async () => ({ + capabilities: { fields: [{ name: 'title', type: 'String', operators: ['present'] }] }, + readModel: new ReadModel([ + collection('users', [column('id'), relation('posts', 'HasMany', 'posts.id')]), + ]), + }), + } as unknown as ReadModelStore; + const app = buildApp(refreshedStore, client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(response.status).toBe(404); + expect(response.body.error).toMatchObject({ type: 'unknown_collection', status: 404 }); + expect(client.listRelation).not.toHaveBeenCalled(); + expect(client.countRelationRaw).not.toHaveBeenCalled(); + }, + ); + + it.each([['list'], ['count']])( + 'should return 404 on a relation %s when a refresh re-targets the relation to another collection', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + // Same relation name, different target: the capabilities just fetched belong to `posts`, so + // serving `archived_posts` records under them would validate one collection and return another. + const refreshedStore = { + getReadModel: async () => relationReadModel, + getCapabilities: async () => ({ + capabilities: { fields: [{ name: 'title', type: 'String', operators: ['present'] }] }, + readModel: new ReadModel([ + collection('users', [ + column('id'), + relation('posts', 'HasMany', 'archived_posts.id'), + ]), + collection('archived_posts', [column('id'), column('title')]), + ]), + }), + } as unknown as ReadModelStore; + const app = buildApp(refreshedStore, client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(response.status).toBe(404); + expect(response.body.error).toMatchObject({ type: 'unknown_relation', status: 404 }); + expect(client.listRelation).not.toHaveBeenCalled(); + expect(client.countRelationRaw).not.toHaveBeenCalled(); + }, + ); + + it.each([['list'], ['count']])( + 'should map a foreign capabilities fetch failure on a relation %s to agent_unavailable', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + const getCapabilities = jest.fn(async () => { + throw new AgentHttpError(503, {}, 'Service Unavailable'); + }); + const app = buildApp(storeOf(relationReadModel, getCapabilities), client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(response.status).toBe(503); + expect(response.body.error).toEqual( + expect.objectContaining({ type: 'agent_unavailable', status: 503 }), + ); + expect(client.listRelation).not.toHaveBeenCalled(); + expect(client.countRelationRaw).not.toHaveBeenCalled(); + }, + ); + + it.each([['list'], ['count']])( + 'should return 404 on a relation %s when a refresh during the capabilities read drops the parent collection', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + const refreshedStore = { + getReadModel: async () => relationReadModel, + getCapabilities: async () => ({ + capabilities: { fields: [{ name: 'title', type: 'String', operators: ['present'] }] }, + readModel: new ReadModel([collection('posts', [column('id'), column('title')])]), + }), + } as unknown as ReadModelStore; + const app = buildApp(refreshedStore, client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(response.status).toBe(404); + expect(response.body.error).toMatchObject({ type: 'unknown_collection', status: 404 }); + expect(client.listRelation).not.toHaveBeenCalled(); + expect(client.countRelationRaw).not.toHaveBeenCalled(); + }, + ); + + it('should stamp the foreign primary keys from the generation the capabilities belong to', async () => { + const listRelation = jest.fn(async () => [{ id: 'acme|42', title: 'Hello' }]); + // The two generations disagree on the foreign primary key: single `id` before the fetch, the + // composite `(tenant, id)` after. Keeping the pre-fetch keys would unpack a two-value packed id + // against a one-key schema and 500 in `unpackPrimaryKey`. + const preFetch = new ReadModel([ + collection('users', [column('id'), relation('posts', 'HasMany', 'posts.id')]), + collection('posts', [column('id'), column('title')]), + ]); + const refreshedStore = { + getReadModel: async () => preFetch, + getCapabilities: async () => ({ + capabilities: { fields: [{ name: 'title', type: 'String', operators: ['present'] }] }, + readModel: new ReadModel([ + collection('users', [column('id'), relation('posts', 'HasMany', 'posts.id')]), + collection('posts', [ + { ...column('tenant'), isPrimaryKey: true }, + column('id'), + column('title'), + ]), + ]), + }), + } as unknown as ReadModelStore; + const app = buildApp(refreshedStore, { listRelation }); + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', projection: ['title'] }); + + expect(response.status).toBe(200); + expect(response.body.data[0]).toEqual({ + id: 'acme|42', + title: 'Hello', + __forest: { collection: 'posts', primaryKey: { tenant: 'acme', id: '42' } }, + }); + }); + }); }); From f858855b878f38c117fc8cdef89700e64fb686b1 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Tue, 4 Aug 2026 18:05:32 +0200 Subject: [PATCH 2/2] fix: add review feedbacks --- packages/agent-bff/src/data/agent-query.ts | 55 +++--- .../src/data/data-routes-middleware.ts | 57 ++++-- .../src/validation/capabilities-validator.ts | 4 +- .../agent-bff/test/data/agent-query.test.ts | 54 ++++++ .../test/data/data-routes-middleware.test.ts | 179 +++++++++++++++--- 5 files changed, 267 insertions(+), 82 deletions(-) diff --git a/packages/agent-bff/src/data/agent-query.ts b/packages/agent-bff/src/data/agent-query.ts index 5b7892edc5..daf3bfdb06 100644 --- a/packages/agent-bff/src/data/agent-query.ts +++ b/packages/agent-bff/src/data/agent-query.ts @@ -1,4 +1,8 @@ import { invalidRequest } from '../http/bff-local-errors'; +import { MAX_FILTER_DEPTH, isBranch, isLeaf } from '../validation/capabilities-validator'; +import { filterTooDeep } from '../validation/validation-errors'; + +export { MAX_FILTER_DEPTH as MAX_PARSED_FILTER_DEPTH }; export interface BffSortClause { field: string; @@ -31,6 +35,23 @@ function isPlainObject(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } +function assertNoNodeReadableAsBothLeafAndBranch(node: unknown, depth = 0): void { + if (depth > MAX_FILTER_DEPTH) throw filterTooDeep(MAX_FILTER_DEPTH); + if (typeof node !== 'object' || node === null) return; + + const readableAsBranch = isBranch(node); + + if (isLeaf(node) && readableAsBranch) { + throw invalidRequest('A filter node cannot carry both "field" and "conditions"'); + } + + if (readableAsBranch) { + node.conditions.forEach(condition => + assertNoNodeReadableAsBothLeafAndBranch(condition, depth + 1), + ); + } +} + // Validate the untyped request body before it reaches the query builders, so malformed shapes // (e.g. `projection` or `sort` as a string) surface as 400 invalid_request rather than a 500 from // an array method blowing up downstream. @@ -59,8 +80,9 @@ export function parseListRequest(body: unknown): ListRequestBody { if (!valid) throw invalidRequest('sort must be an array of { field, direction? }'); } - if (filter !== undefined && !isPlainObject(filter)) { - throw invalidRequest('filter must be an object'); + if (filter !== undefined) { + if (!isPlainObject(filter)) throw invalidRequest('filter must be an object'); + assertNoNodeReadableAsBothLeafAndBranch(filter); } if (page !== undefined) { @@ -83,37 +105,14 @@ export function parseListRequest(body: unknown): ListRequestBody { export function parseCountRequest(body: unknown): CountRequestBody { if (!isPlainObject(body)) throw invalidRequest('Request body must be an object'); - if (body.filter !== undefined && !isPlainObject(body.filter)) { - throw invalidRequest('filter must be an object'); + if (body.filter !== undefined) { + if (!isPlainObject(body.filter)) throw invalidRequest('filter must be an object'); + assertNoNodeReadableAsBothLeafAndBranch(body.filter); } return body as CountRequestBody; } -interface ConditionTreeBranch { - conditions: unknown[]; -} - -interface ConditionTreeLeaf { - field: string; -} - -function isBranch(node: unknown): node is ConditionTreeBranch { - return ( - typeof node === 'object' && - node !== null && - Array.isArray((node as { conditions?: unknown }).conditions) - ); -} - -function isLeaf(node: unknown): node is ConditionTreeLeaf { - return ( - typeof node === 'object' && - node !== null && - typeof (node as { field?: unknown }).field === 'string' - ); -} - function collectFilterFields(filter: unknown, acc: string[]): void { if (isBranch(filter)) { filter.conditions.forEach(condition => collectFilterFields(condition, acc)); diff --git a/packages/agent-bff/src/data/data-routes-middleware.ts b/packages/agent-bff/src/data/data-routes-middleware.ts index d7ea323813..ddbad88190 100644 --- a/packages/agent-bff/src/data/data-routes-middleware.ts +++ b/packages/agent-bff/src/data/data-routes-middleware.ts @@ -151,29 +151,44 @@ interface RelationHandlerDeps extends RequestHandlerDeps { type RelationListHandlerDeps = RelationHandlerDeps & { primaryKeys: PrimaryKeyField[] }; -// The route resolved parent, relation and foreign collection against an earlier generation; a schema -// refresh during the capabilities fetch can invalidate any of the three, so all three are re-checked -// against the generation the capabilities belong to. The foreign name must come out unchanged: the -// capabilities are cached under it, the agent query projects under it and the response is stamped -// with it, so a re-targeted relation would validate one collection and serve another. -async function resolveRelationCapabilities( - deps: RelationHandlerDeps, -): Promise<{ capabilities: CapabilitiesResult; primaryKeys: PrimaryKeyField[] }> { - const { capabilities, readModel } = await resolveCapabilities(deps, deps.foreignCollection); - +function assertRelationStillExposed(readModel: ReadModel, deps: RelationHandlerDeps): void { assertCollectionStillAllowed(readModel, deps.collection); - const foreignCollection = resolveForeignCollection( + const stillTargets = resolveForeignCollection( readModel.getRelationTarget(deps.collection, deps.relation), ); - if (foreignCollection !== deps.foreignCollection) { + if (stillTargets !== deps.foreignCollection) { throw unknownRelation(`Unknown relation: ${deps.collection}.${deps.relation}`); } - assertCollectionStillAllowed(readModel, foreignCollection); + assertCollectionStillAllowed(readModel, deps.foreignCollection); +} + +async function resolveForeignCapabilitiesAndReassertRelationIsStillExposed( + deps: RelationHandlerDeps, +): Promise<{ capabilities: CapabilitiesResult; readModel: ReadModel }> { + assertRelationStillExposed(await resolveReadModel(deps.store), deps); + + let result: { capabilities: CapabilitiesResult; readModel: ReadModel }; + + try { + result = await resolveCapabilities(deps, deps.foreignCollection); + } catch (error) { + deps.logger('Warn', 'Foreign capabilities lookup failed; re-checking relation exposure', { + collection: deps.collection, + relation: deps.relation, + foreignCollection: deps.foreignCollection, + cause: error instanceof Error ? `${error.name}: ${error.message}` : String(error), + }); + + assertRelationStillExposed(await resolveReadModel(deps.store), deps); + throw error; + } + + assertRelationStillExposed(result.readModel, deps); - return { capabilities, primaryKeys: readModel.getPrimaryKeys(foreignCollection) }; + return result; } async function handleRelationList( @@ -181,9 +196,6 @@ async function handleRelationList( body: RelationListRequestBody, deps: RelationListHandlerDeps, ) { - // The nested-relation guard IS wired here: the agent's list-related asserts browse only on the - // immediate foreign collection, so a nested `:`-path would traverse to a third collection whose - // browse is never checked. Plain foreign fields (no `:`) are unaffected. assertNoRelationFieldPaths(collectListFieldPaths(body)); const validationInput = { @@ -195,9 +207,10 @@ async function handleRelationList( let { primaryKeys } = deps; if (hasCapabilityConstrainedInput(validationInput)) { - const resolved = await resolveRelationCapabilities(deps); - assertValidAgainstCapabilities(validationInput, resolved.capabilities); - primaryKeys = resolved.primaryKeys; + const { capabilities, readModel } = + await resolveForeignCapabilitiesAndReassertRelationIsStillExposed(deps); + assertValidAgainstCapabilities(validationInput, capabilities); + primaryKeys = readModel.getPrimaryKeys(deps.foreignCollection); } const query = buildListAgentQuery(deps.foreignCollection, deps.timezone, body); @@ -218,7 +231,9 @@ async function handleRelationCount( assertNoRelationFieldPaths(collectCountFieldPaths(body)); if (body.filter !== undefined) { - const { capabilities } = await resolveRelationCapabilities(deps); + const { capabilities } = await resolveForeignCapabilitiesAndReassertRelationIsStillExposed( + deps, + ); assertValidAgainstCapabilities({ filter: body.filter }, capabilities); } diff --git a/packages/agent-bff/src/validation/capabilities-validator.ts b/packages/agent-bff/src/validation/capabilities-validator.ts index be142dd53f..6d4d9237d1 100644 --- a/packages/agent-bff/src/validation/capabilities-validator.ts +++ b/packages/agent-bff/src/validation/capabilities-validator.ts @@ -21,7 +21,7 @@ interface FilterLeaf { operator?: string; } -function isBranch(node: unknown): node is { conditions: unknown[] } { +export function isBranch(node: unknown): node is { conditions: unknown[] } { return ( typeof node === 'object' && node !== null && @@ -29,7 +29,7 @@ function isBranch(node: unknown): node is { conditions: unknown[] } { ); } -function isLeaf(node: unknown): node is FilterLeaf { +export function isLeaf(node: unknown): node is FilterLeaf { return ( typeof node === 'object' && node !== null && diff --git a/packages/agent-bff/test/data/agent-query.test.ts b/packages/agent-bff/test/data/agent-query.test.ts index 20cf01e26d..30d1bddeb6 100644 --- a/packages/agent-bff/test/data/agent-query.test.ts +++ b/packages/agent-bff/test/data/agent-query.test.ts @@ -1,4 +1,5 @@ import { + MAX_PARSED_FILTER_DEPTH, buildCountAgentQuery, buildListAgentQuery, collectCountFieldPaths, @@ -132,6 +133,59 @@ describe('parseCountRequest', () => { }); }); +describe('a filter node readable as both a leaf and a branch', () => { + const READABLE_AS_BOTH = { + field: 'publisher:secretRevenue', + operator: 'Equal', + value: 1, + conditions: [], + }; + + it.each([ + ['parseListRequest', parseListRequest], + ['parseCountRequest', parseCountRequest], + ])('should reject it in %s with 400 invalid_request', (_label, parse) => { + expect(() => parse({ filter: READABLE_AS_BOTH })).toThrow( + expect.objectContaining({ type: 'invalid_request', status: 400 }), + ); + }); + + it('should reject it nested inside a legitimate branch', () => { + expect(() => + parseListRequest({ filter: { aggregator: 'And', conditions: [READABLE_AS_BOTH] } }), + ).toThrow(expect.objectContaining({ type: 'invalid_request', status: 400 })); + }); + + it('should be invisible to the field-path collector', () => { + expect(collectCountFieldPaths({ filter: READABLE_AS_BOTH })).toEqual([]); + }); + + it('should still accept a plain leaf and a plain branch', () => { + const leaf = { field: 'title', operator: 'Present' }; + + expect(() => parseCountRequest({ filter: leaf })).not.toThrow(); + expect(() => + parseListRequest({ filter: { aggregator: 'And', conditions: [leaf] } }), + ).not.toThrow(); + }); + + it('should reject a filter nested past the depth cap with 400 rather than blowing the stack', () => { + let filter: unknown = { field: 'title', operator: 'Present' }; + + for (let i = 0; i <= MAX_PARSED_FILTER_DEPTH; i += 1) { + filter = { aggregator: 'And', conditions: [filter] }; + } + + expect(() => parseCountRequest({ filter })).toThrow( + expect.objectContaining({ + type: 'filter_too_deep', + status: 400, + details: { maxDepth: MAX_PARSED_FILTER_DEPTH }, + }), + ); + }); +}); + describe('parseParentId', () => { it('should return a non-empty string unchanged, including a composite packed id', () => { expect(parseParentId('a|b')).toBe('a|b'); diff --git a/packages/agent-bff/test/data/data-routes-middleware.test.ts b/packages/agent-bff/test/data/data-routes-middleware.test.ts index 0c8830c785..cb8f86ce76 100644 --- a/packages/agent-bff/test/data/data-routes-middleware.test.ts +++ b/packages/agent-bff/test/data/data-routes-middleware.test.ts @@ -59,9 +59,11 @@ function buildApp( { agentToken = 'agent-jwt', createClient = () => client as AgentDataClient, + logger = noopLogger, }: { agentToken?: string | null; createClient?: (options: { agentUrl: string; token: string }) => AgentDataClient; + logger?: Logger; } = {}, ) { const app = new Koa(); @@ -77,7 +79,7 @@ function buildApp( createDataRoutesMiddleware({ store, agentUrl: AGENT_URL, - logger: noopLogger, + logger, createClient, }), ); @@ -993,26 +995,24 @@ describe('data routes middleware', () => { }); describe('relation capabilities validation', () => { - // The parent is deliberately the permissive one: a lookup against `users` would accept every - // field below, so any test here that passes while the implementation reads the parent's - // capabilities would be proving nothing. - const perCollectionCapabilities: CapabilitiesStub = async collectionName => { - if (collectionName === 'posts') { - return { fields: [{ name: 'title', type: 'String', operators: ['present'] }] }; - } - - return { - fields: ['id', 'email', 'title', 'ghost'].map(name => ({ - name, - type: 'String', - operators: BROAD_SNAKE_OPERATORS, - })), - }; + const NARROW_FOREIGN_CAPABILITIES = { + fields: [{ name: 'title', type: 'String', operators: ['present'] }], + }; + + const PERMISSIVE_PARENT_CAPABILITIES = { + fields: ['id', 'email', 'title'].map(name => ({ + name, + type: 'String', + operators: BROAD_SNAKE_OPERATORS, + })), }; + const narrowOnForeignPermissiveOnParent: CapabilitiesStub = async collectionName => + collectionName === 'posts' ? NARROW_FOREIGN_CAPABILITIES : PERMISSIVE_PARENT_CAPABILITIES; + it('should fetch capabilities for the foreign collection, not the parent', async () => { const listRelation = jest.fn(async () => []); - const getCapabilities = jest.fn(perCollectionCapabilities); + const getCapabilities = jest.fn(narrowOnForeignPermissiveOnParent); const app = buildApp(storeOf(relationReadModel, getCapabilities), { listRelation }); const response = await request(app.callback()) @@ -1024,9 +1024,32 @@ describe('data routes middleware', () => { expect(getCapabilities).not.toHaveBeenCalledWith('users'); }); + it('should forward a relation count filter the foreign capabilities accept', async () => { + const countRelationRaw = jest.fn(async () => ({ count: 4 })); + const app = buildApp(storeOf(relationReadModel, narrowOnForeignPermissiveOnParent), { + countRelationRaw, + }); + const filter = { field: 'title', operator: 'Present' }; + + const response = await request(app.callback()) + .post('/agent/v1/users/relations/posts/count') + .send({ parentId: '7', filter }); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ count: 4, countStatus: 'available' }); + expect(countRelationRaw).toHaveBeenCalledWith( + 'users', + '7', + 'posts', + expect.objectContaining({ filters: JSON.stringify(filter) }), + ); + }); + it('should reject a projection field that only exists on the parent collection', async () => { const listRelation = jest.fn(async () => []); - const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { listRelation }); + const app = buildApp(storeOf(relationReadModel, narrowOnForeignPermissiveOnParent), { + listRelation, + }); const response = await request(app.callback()) .post('/agent/v1/users/relations/posts/list') @@ -1041,26 +1064,30 @@ describe('data routes middleware', () => { expect(listRelation).not.toHaveBeenCalled(); }); - it('should reject a relation list sort field absent from the foreign capabilities', async () => { + it('should reject a relation list sort field that only exists on the parent collection', async () => { const listRelation = jest.fn(async () => []); - const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { listRelation }); + const app = buildApp(storeOf(relationReadModel, narrowOnForeignPermissiveOnParent), { + listRelation, + }); const response = await request(app.callback()) .post('/agent/v1/users/relations/posts/list') - .send({ parentId: '7', sort: [{ field: 'ghost', direction: 'asc' }] }); + .send({ parentId: '7', sort: [{ field: 'email', direction: 'asc' }] }); expect(response.status).toBe(422); expect(response.body.error).toMatchObject({ type: 'unknown_field', status: 422, - details: { field: 'ghost' }, + details: { field: 'email' }, }); expect(listRelation).not.toHaveBeenCalled(); }); it('should reject a relation list filter operator the foreign capabilities do not support', async () => { const listRelation = jest.fn(async () => []); - const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { listRelation }); + const app = buildApp(storeOf(relationReadModel, narrowOnForeignPermissiveOnParent), { + listRelation, + }); const response = await request(app.callback()) .post('/agent/v1/users/relations/posts/list') @@ -1077,7 +1104,7 @@ describe('data routes middleware', () => { it('should reject a relation count filter operator the foreign capabilities do not support', async () => { const countRelationRaw = jest.fn(async () => ({ count: 0 })); - const app = buildApp(storeOf(relationReadModel, perCollectionCapabilities), { + const app = buildApp(storeOf(relationReadModel, narrowOnForeignPermissiveOnParent), { countRelationRaw, }); @@ -1104,7 +1131,7 @@ describe('data routes middleware', () => { const getCapabilities = jest.fn(async (collectionName: string) => { calls.push(`capabilities:${collectionName}`); - return perCollectionCapabilities(collectionName); + return narrowOnForeignPermissiveOnParent(collectionName); }); const app = buildApp(storeOf(relationReadModel, getCapabilities), { listRelation }); @@ -1136,6 +1163,41 @@ describe('data routes middleware', () => { }, ); + it.each([['list'], ['count']])( + 'should return 404 on a relation %s without fetching capabilities when the relation is already gone', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + const getCapabilities = jest.fn(narrowOnForeignPermissiveOnParent); + const withoutPosts = new ReadModel([collection('users', [column('id')])]); + let served = relationReadModel; + const store = { + getReadModel: async () => { + const current = served; + served = withoutPosts; + + return current; + }, + getCapabilities: async (name: string) => ({ + capabilities: await getCapabilities(name), + readModel: withoutPosts, + }), + } as unknown as ReadModelStore; + const app = buildApp(store, client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(response.status).toBe(404); + expect(getCapabilities).not.toHaveBeenCalled(); + expect(client.listRelation).not.toHaveBeenCalled(); + expect(client.countRelationRaw).not.toHaveBeenCalled(); + }, + ); + it.each([['list'], ['count']])( 'should return 404 on a relation %s when a refresh during the capabilities read drops the foreign collection', async operation => { @@ -1143,7 +1205,6 @@ describe('data routes middleware', () => { listRelation: jest.fn(async () => []), countRelationRaw: jest.fn(async () => ({ count: 0 })), }; - // The newer generation keeps users.posts as a relation but no longer exposes posts itself. const refreshedStore = { getReadModel: async () => relationReadModel, getCapabilities: async () => ({ @@ -1173,8 +1234,6 @@ describe('data routes middleware', () => { listRelation: jest.fn(async () => []), countRelationRaw: jest.fn(async () => ({ count: 0 })), }; - // Same relation name, different target: the capabilities just fetched belong to `posts`, so - // serving `archived_posts` records under them would validate one collection and return another. const refreshedStore = { getReadModel: async () => relationReadModel, getCapabilities: async () => ({ @@ -1201,6 +1260,40 @@ describe('data routes middleware', () => { }, ); + it.each([['list'], ['count']])( + 'should return 404 on a relation %s when the foreign collection disappears during a failed capabilities read', + async operation => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + const refreshedRelationReadModel = new ReadModel([ + collection('users', [column('id'), relation('posts', 'HasMany', 'posts.id')]), + ]); + let readModelReads = 0; + const refreshedStore = { + getReadModel: async () => { + readModelReads += 1; + + return readModelReads < 3 ? relationReadModel : refreshedRelationReadModel; + }, + getCapabilities: async () => { + throw new Error('foreign collection is no longer exposed'); + }, + } as unknown as ReadModelStore; + const app = buildApp(refreshedStore, client); + + const response = await request(app.callback()) + .post(`/agent/v1/users/relations/posts/${operation}`) + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(response.status).toBe(404); + expect(response.body.error).toMatchObject({ type: 'unknown_collection', status: 404 }); + expect(client.listRelation).not.toHaveBeenCalled(); + expect(client.countRelationRaw).not.toHaveBeenCalled(); + }, + ); + it.each([['list'], ['count']])( 'should map a foreign capabilities fetch failure on a relation %s to agent_unavailable', async operation => { @@ -1226,6 +1319,33 @@ describe('data routes middleware', () => { }, ); + it('should log the cause when a foreign capabilities fetch fails on a relation list', async () => { + const client = { + listRelation: jest.fn(async () => []), + countRelationRaw: jest.fn(async () => ({ count: 0 })), + }; + const getCapabilities = jest.fn(async () => { + throw new AgentHttpError(503, {}, 'Service Unavailable'); + }); + const logger = jest.fn(); + const app = buildApp(storeOf(relationReadModel, getCapabilities), client, { logger }); + + await request(app.callback()) + .post('/agent/v1/users/relations/posts/list') + .send({ parentId: '7', filter: { field: 'title', operator: 'Present' } }); + + expect(logger).toHaveBeenCalledWith( + 'Warn', + 'Foreign capabilities lookup failed; re-checking relation exposure', + expect.objectContaining({ + collection: 'users', + relation: 'posts', + foreignCollection: 'posts', + cause: 'BffHttpError: The agent is unavailable', + }), + ); + }); + it.each([['list'], ['count']])( 'should return 404 on a relation %s when a refresh during the capabilities read drops the parent collection', async operation => { @@ -1255,9 +1375,6 @@ describe('data routes middleware', () => { it('should stamp the foreign primary keys from the generation the capabilities belong to', async () => { const listRelation = jest.fn(async () => [{ id: 'acme|42', title: 'Hello' }]); - // The two generations disagree on the foreign primary key: single `id` before the fetch, the - // composite `(tenant, id)` after. Keeping the pre-fetch keys would unpack a two-value packed id - // against a one-key schema and 500 in `unpackPrimaryKey`. const preFetch = new ReadModel([ collection('users', [column('id'), relation('posts', 'HasMany', 'posts.id')]), collection('posts', [column('id'), column('title')]),