diff --git a/packages/agent/src/routes/modification/action/action-authorization.ts b/packages/agent/src/routes/modification/action/action-authorization.ts index 9bdbb220ef..1e55d62ddc 100644 --- a/packages/agent/src/routes/modification/action/action-authorization.ts +++ b/packages/agent/src/routes/modification/action/action-authorization.ts @@ -27,7 +27,11 @@ export default class ActionAuthorizationService { filterForCaller, filterForAllCaller, caller, - }: CanPerformCustomActionParams): Promise { + resolveSelectAllRecordIds, + }: CanPerformCustomActionParams & { + // Set on "select all": resolves the selection to the ids stored in the approval request. + resolveSelectAllRecordIds?: () => Promise>; + }): Promise { const canTrigger = await this.canTriggerCustomAction( caller, customActionName, @@ -54,7 +58,10 @@ export default class ActionAuthorizationService { filterForAllCaller, ); - throw new CustomActionRequiresApprovalError(roleIdsAllowedToApprove); + throw new CustomActionRequiresApprovalError( + roleIdsAllowedToApprove, + await resolveSelectAllRecordIds?.(), + ); } } diff --git a/packages/agent/src/routes/modification/action/action.ts b/packages/agent/src/routes/modification/action/action.ts index 2ffcd018a0..407b20a069 100644 --- a/packages/agent/src/routes/modification/action/action.ts +++ b/packages/agent/src/routes/modification/action/action.ts @@ -21,6 +21,7 @@ import { } from '@forestadmin/datasource-toolkit'; import ActionAuthorizationService from './action-authorization'; +import ApprovalSelectionTooLargeError from './errors/approval-selection-too-large-error'; import { MAX_SNAPSHOT_RECORDS, buildRecorder, @@ -101,9 +102,15 @@ export default class ActionRoute extends CollectionRoute { requesterId: requestBody.data.attributes.requester_id, }); } else { - await this.actionAuthorizationService.assertCanTriggerCustomAction( - canPerformCustomActionParams, - ); + await this.actionAuthorizationService.assertCanTriggerCustomAction({ + ...canPerformCustomActionParams, + // Global actions target no specific records (mirrors auditedRecordIds). + resolveSelectAllRecordIds: + requestBody?.data?.attributes?.all_records && + this.collection.schema.actions[this.actionName].scope !== 'Global' + ? () => this.resolveApprovalRecordIds(context, caller, filterForCaller) + : undefined, + }); } const rawData = requestBody.data.attributes.values; @@ -294,6 +301,28 @@ export default class ActionRoute extends CollectionRoute { return []; } + // Fetching cap+1 distinguishes "over the cap" from "exactly the cap". + private async resolveApprovalRecordIds( + context: Context, + caller: Caller, + filterForCaller: Filter, + ): Promise> { + const max = this.options.maxRecordsForApproval; + const paginatedFilter = await this.services.segmentQueryHandler.handleLiveQuerySegmentFilter( + context, + new PaginatedFilter({ ...filterForCaller, page: new Page(0, max + 1) }), + ); + const records = await this.collection.list( + caller, + paginatedFilter, + new Projection(...SchemaUtils.getPrimaryKeys(this.collection.schema)), + ); + + if (records.length > max) throw new ApprovalSelectionTooLargeError(max); + + return IdUtils.packIds(this.collection.schema, records); + } + private async handleHook(context: Context): Promise { const body = context.request.body as SmartActionHookRequestBody; const { id: userId } = context.state.user as UserInfo; diff --git a/packages/agent/src/routes/modification/action/errors/approval-selection-too-large-error.ts b/packages/agent/src/routes/modification/action/errors/approval-selection-too-large-error.ts new file mode 100644 index 0000000000..768392d71d --- /dev/null +++ b/packages/agent/src/routes/modification/action/errors/approval-selection-too-large-error.ts @@ -0,0 +1,10 @@ +import { UnprocessableError } from '@forestadmin/datasource-toolkit'; + +export default class ApprovalSelectionTooLargeError extends UnprocessableError { + constructor(max: number) { + super( + `This action requires approval and cannot be triggered on more than ${max} records at once. ` + + `Please refine your selection.`, + ); + } +} diff --git a/packages/agent/src/routes/modification/action/errors/custom-action-requires-approval-error.ts b/packages/agent/src/routes/modification/action/errors/custom-action-requires-approval-error.ts index 3a00e1a033..5bfc810ee4 100644 --- a/packages/agent/src/routes/modification/action/errors/custom-action-requires-approval-error.ts +++ b/packages/agent/src/routes/modification/action/errors/custom-action-requires-approval-error.ts @@ -1,9 +1,10 @@ import { ForbiddenError } from '@forestadmin/datasource-toolkit'; export default class CustomActionRequiresApprovalError extends ForbiddenError { - constructor(roleIdsAllowedToApprove: number[]) { + constructor(roleIdsAllowedToApprove: number[], recordIds?: Array) { super('This action requires to be approved.', { roleIdsAllowedToApprove, + ...(recordIds ? { recordIds } : {}), }); } } diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index 375d094cee..b49b9be255 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -74,6 +74,12 @@ export type AgentOptions = { */ ignoreMissingSchemaElementErrors?: boolean; useUnsafeActionEndpoint?: boolean; + /** + * Max number of records a "select all" approval-required action may target. + * Must not exceed the Forest server's own cap (500). + * @default 500 + */ + maxRecordsForApproval?: number; /** * Base URL of the workflow executor to proxy requests to. * When set, the agent forwards `/_internal/executor/*` to the executor verbatim, diff --git a/packages/agent/src/utils/options-validator.ts b/packages/agent/src/utils/options-validator.ts index 254f8bcb96..7176e6c653 100644 --- a/packages/agent/src/utils/options-validator.ts +++ b/packages/agent/src/utils/options-validator.ts @@ -9,6 +9,8 @@ import { createSqlAuditStore } from '../audit-trail'; const DEFAULT_MINIMUM_CACHE_DURATION = 60; // One year cache duration when using events const DEFAULT_CACHE_DURATION_WITH_EVENTS = 31560000; +// Cross-service contract: the Forest server rejects approval requests above this many record ids. +export const DEFAULT_MAX_RECORDS_FOR_APPROVAL = 500; export default class OptionsValidator { private static loggerPrefix = { @@ -42,6 +44,10 @@ export default class OptionsValidator { copyOptions.instantCacheRefresh = copyOptions.instantCacheRefresh ?? true; copyOptions.workflowExecutorUrl = copyOptions.workflowExecutorUrl ?? null; copyOptions.auditTrail = copyOptions.auditTrail ?? null; + // Number.isFinite so NaN (e.g. Number() on an unset env var) also gets the default. + copyOptions.maxRecordsForApproval = Number.isFinite(copyOptions.maxRecordsForApproval) + ? copyOptions.maxRecordsForApproval + : DEFAULT_MAX_RECORDS_FOR_APPROVAL; copyOptions.maxBodySize = copyOptions.maxBodySize || '50mb'; copyOptions.bodyParserOptions = copyOptions.bodyParserOptions || { jsonLimit: '50mb', diff --git a/packages/agent/test/__factories__/forest-admin-http-driver-options.ts b/packages/agent/test/__factories__/forest-admin-http-driver-options.ts index 15191aa311..fc12942c57 100644 --- a/packages/agent/test/__factories__/forest-admin-http-driver-options.ts +++ b/packages/agent/test/__factories__/forest-admin-http-driver-options.ts @@ -29,6 +29,7 @@ export default Factory.define(() => ({ }, ignoreMissingSchemaElementErrors: false, useUnsafeActionEndpoint: false, + maxRecordsForApproval: 500, workflowExecutorUrl: null, auditTrail: null, })); diff --git a/packages/agent/test/routes/modification/action/action.test.ts b/packages/agent/test/routes/modification/action/action.test.ts index bdababfcda..4c6af27766 100644 --- a/packages/agent/test/routes/modification/action/action.test.ts +++ b/packages/agent/test/routes/modification/action/action.test.ts @@ -713,6 +713,192 @@ describe('ActionRoute', () => { }); }); + describe('when an approval-required action is triggered on a select-all selection', () => { + beforeEach(() => { + dataSource = factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'books', + schema: { + actions: { MyBulkAction: { scope: 'Bulk' } }, + fields: { id: factories.columnSchema.uuidPrimaryKey().build() }, + }, + getForm: jest.fn().mockResolvedValue([]), + execute: jest.fn(), + }), + ]); + ( + options.forestAdminClient.permissionService + .doesTriggerCustomActionRequiresApproval as jest.Mock + ).mockResolvedValue(true); + ( + options.forestAdminClient.permissionService + .getRoleIdsAllowedToApproveWithoutConditions as jest.Mock + ).mockResolvedValue([7]); + ( + options.forestAdminClient.permissionService.getConditionalApproveConditions as jest.Mock + ).mockResolvedValue(new Map()); + }); + + const selectAllContext = () => + createMockContext({ + ...baseContext, + requestBody: { + data: { + attributes: { + ...baseContext.requestBody.data.attributes, + ids: [], + all_records: true, + all_records_ids_excluded: [], + }, + }, + }, + }); + + test('resolves the selection to concrete ids and returns them in the approval error', async () => { + (dataSource.getCollection('books').list as jest.Mock).mockResolvedValue([ + { id: '123e4567-e89b-12d3-a456-426614174000' }, + { id: '123e4567-e89b-12d3-a456-426614174001' }, + ]); + route = new ActionRoute(services, options, dataSource, 'books', 'MyBulkAction'); + + // @ts-expect-error: test private method + await expect(route.handleExecute(selectAllContext())).rejects.toMatchObject({ + name: 'CustomActionRequiresApprovalError', + data: { + roleIdsAllowedToApprove: [7], + recordIds: [ + '123e4567-e89b-12d3-a456-426614174000', + '123e4567-e89b-12d3-a456-426614174001', + ], + }, + }); + }); + + test('rejects with ApprovalSelectionTooLargeError above the configured cap', async () => { + const cappedOptions = factories.forestAdminHttpDriverOptions.build({ + maxRecordsForApproval: 2, + }); + ( + cappedOptions.forestAdminClient.permissionService.canTriggerCustomAction as jest.Mock + ).mockResolvedValue(true); + ( + cappedOptions.forestAdminClient.permissionService + .doesTriggerCustomActionRequiresApproval as jest.Mock + ).mockResolvedValue(true); + ( + cappedOptions.forestAdminClient.permissionService + .getRoleIdsAllowedToApproveWithoutConditions as jest.Mock + ).mockResolvedValue([7]); + ( + cappedOptions.forestAdminClient.permissionService + .getConditionalApproveConditions as jest.Mock + ).mockResolvedValue(new Map()); + // cap+1 rows fetched => over the cap of 2 + (dataSource.getCollection('books').list as jest.Mock).mockResolvedValue([ + { id: '123e4567-e89b-12d3-a456-426614174000' }, + { id: '123e4567-e89b-12d3-a456-426614174001' }, + { id: '123e4567-e89b-12d3-a456-426614174002' }, + ]); + route = new ActionRoute(services, cappedOptions, dataSource, 'books', 'MyBulkAction'); + + // @ts-expect-error: test private method + await expect(route.handleExecute(selectAllContext())).rejects.toMatchObject({ + name: 'ApprovalSelectionTooLargeError', + message: expect.stringContaining('more than 2 records'), + }); + }); + + test('accepts a selection of exactly the configured cap', async () => { + const cappedOptions = factories.forestAdminHttpDriverOptions.build({ + maxRecordsForApproval: 2, + }); + ( + cappedOptions.forestAdminClient.permissionService.canTriggerCustomAction as jest.Mock + ).mockResolvedValue(true); + ( + cappedOptions.forestAdminClient.permissionService + .doesTriggerCustomActionRequiresApproval as jest.Mock + ).mockResolvedValue(true); + ( + cappedOptions.forestAdminClient.permissionService + .getRoleIdsAllowedToApproveWithoutConditions as jest.Mock + ).mockResolvedValue([7]); + ( + cappedOptions.forestAdminClient.permissionService + .getConditionalApproveConditions as jest.Mock + ).mockResolvedValue(new Map()); + (dataSource.getCollection('books').list as jest.Mock).mockResolvedValue([ + { id: '123e4567-e89b-12d3-a456-426614174000' }, + { id: '123e4567-e89b-12d3-a456-426614174001' }, + ]); + route = new ActionRoute(services, cappedOptions, dataSource, 'books', 'MyBulkAction'); + + // @ts-expect-error: test private method + await expect(route.handleExecute(selectAllContext())).rejects.toMatchObject({ + name: 'CustomActionRequiresApprovalError', + data: { + recordIds: [ + '123e4567-e89b-12d3-a456-426614174000', + '123e4567-e89b-12d3-a456-426614174001', + ], + }, + }); + // cap+1 is requested so "exactly the cap" and "over the cap" are distinguishable + expect(dataSource.getCollection('books').list).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ page: expect.objectContaining({ skip: 0, limit: 3 }) }), + expect.anything(), + ); + }); + + test('does not resolve ids on a global action (it targets no specific records)', async () => { + dataSource = factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'books', + schema: { + actions: { MyGlobalAction: { scope: 'Global' } }, + fields: { id: factories.columnSchema.uuidPrimaryKey().build() }, + }, + getForm: jest.fn().mockResolvedValue([]), + execute: jest.fn(), + }), + ]); + route = new ActionRoute(services, options, dataSource, 'books', 'MyGlobalAction'); + + // @ts-expect-error: test private method + const error = await route.handleExecute(selectAllContext()).catch(e => e); + + expect(error).toMatchObject({ name: 'CustomActionRequiresApprovalError' }); + expect(error.data.recordIds).toBeUndefined(); + // No 422 above the cap, no id snapshot in the error. + expect(dataSource.getCollection('books').list).not.toHaveBeenCalled(); + }); + + test('does not resolve ids nor cap an explicit selection requiring approval', async () => { + route = new ActionRoute(services, options, dataSource, 'books', 'MyBulkAction'); + + const context = createMockContext({ + ...baseContext, + requestBody: { + data: { + attributes: { + ...baseContext.requestBody.data.attributes, + ids: ['123e4567-e89b-12d3-a456-426614174000'], + all_records: false, + }, + }, + }, + }); + + // @ts-expect-error: test private method + await expect(route.handleExecute(context)).rejects.toMatchObject({ + name: 'CustomActionRequiresApprovalError', + }); + // No select-all resolution => the id-listing query is never issued. + expect(dataSource.getCollection('books').list).not.toHaveBeenCalled(); + }); + }); + describe('with a global action used from list-view, detail-view & summary', () => { beforeEach(() => { dataSource = factories.dataSource.buildWithCollections([ diff --git a/packages/agent/test/utils/http-driver-options.test.ts b/packages/agent/test/utils/http-driver-options.test.ts index bc1a4ecc29..4ea7eac3b6 100644 --- a/packages/agent/test/utils/http-driver-options.test.ts +++ b/packages/agent/test/utils/http-driver-options.test.ts @@ -21,6 +21,32 @@ describe('OptionsValidator', () => { expect(options).toHaveProperty('skipSchemaUpdate', false); }); + describe('maxRecordsForApproval', () => { + test('defaults to 500 when not configured', () => { + const options = OptionsValidator.withDefaults(mandatoryOptions); + + expect(options).toHaveProperty('maxRecordsForApproval', 500); + }); + + test('defaults to 500 when NaN is passed (e.g. Number() on an unset env var)', () => { + const options = OptionsValidator.withDefaults({ + ...mandatoryOptions, + maxRecordsForApproval: Number(process.env.SOME_UNSET_ENV_VAR), + }); + + expect(options).toHaveProperty('maxRecordsForApproval', 500); + }); + + test('keeps a configured value', () => { + const options = OptionsValidator.withDefaults({ + ...mandatoryOptions, + maxRecordsForApproval: 100, + }); + + expect(options).toHaveProperty('maxRecordsForApproval', 100); + }); + }); + test('logger should be callable', () => { jest.spyOn(console, 'error').mockReturnValue(); diff --git a/packages/mcp-server/src/utils/error-parser.ts b/packages/mcp-server/src/utils/error-parser.ts index 78f0a65916..b54d8b3863 100644 --- a/packages/mcp-server/src/utils/error-parser.ts +++ b/packages/mcp-server/src/utils/error-parser.ts @@ -19,7 +19,7 @@ function jsonApiDetail(body: unknown, text?: string): string | null { // Turn an agent RPC error into a human-readable message: the JSON:API detail with its HTTP status // when one can be extracted, else the raw message (which already carries the status). -export default function parseAgentError(error: unknown): string | null { +export default function parseAgentError(error: unknown, depth = 0): string | null { if (error instanceof AgentHttpError) { const detail = jsonApiDetail(error.body, error.responseText); @@ -27,7 +27,16 @@ export default function parseAgentError(error: unknown): string | null { } if (error && typeof error === 'object' && 'message' in error) { - return (error as { message: string }).message || null; + const { message } = error as { message: string }; + // Wrapper errors (e.g. ApprovalRequestCreationError) carry the actionable detail in `cause`. + const causeDetail = + 'cause' in error && depth < 3 + ? parseAgentError((error as { cause: unknown }).cause, depth + 1) + : null; + + if (message && causeDetail) return `${message} ${causeDetail}`; + + return message || causeDetail; } return null; diff --git a/packages/mcp-server/test/utils/error-parser.test.ts b/packages/mcp-server/test/utils/error-parser.test.ts index bc9b1a2400..c9734fd30a 100644 --- a/packages/mcp-server/test/utils/error-parser.test.ts +++ b/packages/mcp-server/test/utils/error-parser.test.ts @@ -47,6 +47,36 @@ describe('parseAgentError', () => { expect(parseAgentError({ unknownProperty: 'some value' })).toBeNull(); }); + it('appends the cause detail of a wrapper error (e.g. ApprovalRequestCreationError)', () => { + const error = new Error( + 'The action requires an approval, but the approval request could not be created.', + ) as Error & { cause: unknown }; + error.cause = new AgentHttpError(422, { + errors: [ + { detail: 'Approval requests are limited to 500 records; please narrow your selection' }, + ], + }); + + expect(parseAgentError(error)).toBe( + 'The action requires an approval, but the approval request could not be created. ' + + 'Approval requests are limited to 500 records; please narrow your selection (HTTP 422)', + ); + }); + + it('ignores a cause without extractable detail', () => { + const error = new Error('Wrapper message') as Error & { cause: unknown }; + error.cause = { foo: 'bar' }; + + expect(parseAgentError(error)).toBe('Wrapper message'); + }); + + it('does not recurse forever on a cyclic cause chain', () => { + const error = new Error('Cyclic') as Error & { cause: unknown }; + error.cause = error; + + expect(parseAgentError(error)).toBe('Cyclic Cyclic Cyclic Cyclic'); + }); + it('returns null for null/undefined', () => { expect(parseAgentError(null)).toBeNull(); expect(parseAgentError(undefined)).toBeNull();