diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 2be1471b72..ceef75f4b3 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -64,3 +64,4 @@ export { DeleteFolderDto } from './folders/delete-folder.dto'; export { ListFolderQueryDto } from './folders/list-folder-query.dto'; export { ListInsightsWorkflowQueryDto } from './insights/list-workflow-query.dto'; +export { PaginationDto } from './pagination/pagination.dto'; diff --git a/packages/@n8n/api-types/src/dto/insights/list-workflow-query.dto.ts b/packages/@n8n/api-types/src/dto/insights/list-workflow-query.dto.ts index 8905a4d03d..2d3572ad8b 100644 --- a/packages/@n8n/api-types/src/dto/insights/list-workflow-query.dto.ts +++ b/packages/@n8n/api-types/src/dto/insights/list-workflow-query.dto.ts @@ -1,6 +1,8 @@ import { z } from 'zod'; import { Z } from 'zod-class'; +import { paginationSchema } from '../pagination/pagination.dto'; + const VALID_SORT_OPTIONS = [ 'total:asc', 'total:desc', @@ -22,31 +24,11 @@ const VALID_SORT_OPTIONS = [ // Parameter Validators // --------------------- -// Skip parameter validation -const skipValidator = z - .string() - .optional() - .transform((val) => (val ? parseInt(val, 10) : 0)) - .refine((val) => !isNaN(val), { - message: 'Skip must be a valid number', - }); - -// Take parameter validation -const takeValidator = z - .string() - .optional() - .transform((val) => (val ? parseInt(val, 10) : 10)) - .refine((val) => !isNaN(val), { - message: 'Take must be a valid number', - }); - -// SortBy parameter validation const sortByValidator = z .enum(VALID_SORT_OPTIONS, { message: `sortBy must be one of: ${VALID_SORT_OPTIONS.join(', ')}` }) .optional(); export class ListInsightsWorkflowQueryDto extends Z.class({ - skip: skipValidator, - take: takeValidator, + ...paginationSchema, sortBy: sortByValidator, }) {} diff --git a/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts b/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts new file mode 100644 index 0000000000..3a32b00a12 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts @@ -0,0 +1,136 @@ +import { PaginationDto, MAX_ITEMS_PER_PAGE } from '../pagination.dto'; + +describe('PaginationDto', () => { + describe('valid inputs', () => { + test('should validate with both take and skip', () => { + const result = PaginationDto.safeParse({ take: '10', skip: '5' }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toEqual({ take: 10, skip: 5 }); + } + }); + + test('should validate with only take', () => { + const result = PaginationDto.safeParse({ take: '10' }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toEqual({ take: 10, skip: 0 }); + } + }); + + test('should cap take at MAX_ITEMS_PER_PAGE', () => { + const result = PaginationDto.safeParse({ take: `${MAX_ITEMS_PER_PAGE + 10}` }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toEqual({ take: MAX_ITEMS_PER_PAGE, skip: 0 }); + } + }); + + test('should handle zero values', () => { + const result = PaginationDto.safeParse({ take: '0', skip: '0' }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toEqual({ take: 0, skip: 0 }); + } + }); + }); + + describe('invalid inputs', () => { + test('should reject non-integer take', () => { + const result = PaginationDto.safeParse({ take: 'hello' }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('take'); + } + }); + + test('should reject non-integer skip', () => { + const result = PaginationDto.safeParse({ take: '10', skip: 'hello' }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('skip'); + } + }); + + test('should reject non-numeric take', () => { + const result = PaginationDto.safeParse({ take: 'abc' }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('take'); + } + }); + + test('should reject non-numeric skip', () => { + const result = PaginationDto.safeParse({ take: '10', skip: 'abc' }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('skip'); + } + }); + + test('should reject object as take', () => { + const result = PaginationDto.safeParse({ take: {} }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('take'); + } + }); + + test('should reject array as skip', () => { + const result = PaginationDto.safeParse({ take: '10', skip: [] }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('skip'); + } + }); + + test('should reject negative take', () => { + const result = PaginationDto.safeParse({ take: '-5' }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('take'); + } + }); + + test('should reject negative skip', () => { + const result = PaginationDto.safeParse({ take: '10', skip: '-5' }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toContain('skip'); + } + }); + }); + + describe('edge cases', () => { + test('should apply cap on extremely large numbers', () => { + const veryLargeNumber = '9'.repeat(20); + const result = PaginationDto.safeParse({ take: veryLargeNumber }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.take).toBe(MAX_ITEMS_PER_PAGE); + } + }); + + test('should fall back to default on empty string value', () => { + const result = PaginationDto.safeParse({ take: '' }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.take).toBe(10); + } + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts b/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts new file mode 100644 index 0000000000..d4c8ffae33 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts @@ -0,0 +1,34 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export const MAX_ITEMS_PER_PAGE = 50; + +const skipValidator = z + .string() + .optional() + .transform((val) => (val ? parseInt(val, 10) : 0)) + .refine((val) => !isNaN(val) && Number.isInteger(val), { + message: 'Param `skip` must be a valid integer', + }) + .refine((val) => val >= 0, { + message: 'Param `skip` must be a non-negative integer', + }); + +const takeValidator = z + .string() + .optional() + .transform((val) => (val ? parseInt(val, 10) : 10)) + .refine((val) => !isNaN(val) && Number.isInteger(val), { + message: 'Param `take` must be a valid integer', + }) + .refine((val) => val >= 0, { + message: 'Param `take` must be a non-negative integer', + }) + .transform((val) => Math.min(val, MAX_ITEMS_PER_PAGE)); + +export const paginationSchema = { + skip: skipValidator, + take: takeValidator, +}; + +export class PaginationDto extends Z.class(paginationSchema) {} diff --git a/packages/cli/src/middlewares/list-query/index.ts b/packages/cli/src/middlewares/list-query/index.ts index 2e4781337a..81f6215cf9 100644 --- a/packages/cli/src/middlewares/list-query/index.ts +++ b/packages/cli/src/middlewares/list-query/index.ts @@ -13,6 +13,9 @@ export type ListQueryMiddleware = ( next: NextFunction, ) => void; +/** + * @deprecated Please create Zod validators in `@n8n/api-types` instead. + */ export const listQueryMiddleware: ListQueryMiddleware[] = [ filterListQueryMiddleware, selectListQueryMiddleware, diff --git a/packages/cli/src/modules/insights/insights.controller.ts b/packages/cli/src/modules/insights/insights.controller.ts index 7fb59cf27b..a808fad3d4 100644 --- a/packages/cli/src/modules/insights/insights.controller.ts +++ b/packages/cli/src/modules/insights/insights.controller.ts @@ -2,8 +2,6 @@ import { ListInsightsWorkflowQueryDto } from '@n8n/api-types'; import type { InsightsSummary, InsightsByTime, InsightsByWorkflow } from '@n8n/api-types'; import { Get, GlobalScope, Licensed, Query, RestController } from '@/decorators'; -import { paginationListQueryMiddleware } from '@/middlewares/list-query/pagination'; -import { sortByQueryMiddleware } from '@/middlewares/list-query/sort-by'; import { AuthenticatedRequest } from '@/requests'; import { InsightsService } from './insights.service'; @@ -22,7 +20,7 @@ export class InsightsController { }); } - @Get('/by-workflow', { middlewares: [paginationListQueryMiddleware, sortByQueryMiddleware] }) + @Get('/by-workflow') @GlobalScope('insights:list') @Licensed('feat:insights:viewDashboard') async getInsightsByWorkflow( diff --git a/packages/cli/src/workflows/workflow-history.ee/workflow-history.controller.ee.ts b/packages/cli/src/workflows/workflow-history.ee/workflow-history.controller.ee.ts index 144a92d8f6..cbd392caaa 100644 --- a/packages/cli/src/workflows/workflow-history.ee/workflow-history.controller.ee.ts +++ b/packages/cli/src/workflows/workflow-history.ee/workflow-history.controller.ee.ts @@ -1,10 +1,10 @@ +import { PaginationDto } from '@n8n/api-types'; import { Request, Response, NextFunction } from 'express'; -import { RestController, Get, Middleware } from '@/decorators'; +import { RestController, Get, Middleware, Query } from '@/decorators'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { SharedWorkflowNotFoundError } from '@/errors/shared-workflow-not-found.error'; import { WorkflowHistoryVersionNotFoundError } from '@/errors/workflow-history-version-not-found.error'; -import { paginationListQueryMiddleware } from '@/middlewares/list-query/pagination'; import { WorkflowHistoryRequest } from '@/requests'; import { isWorkflowHistoryEnabled, isWorkflowHistoryLicensed } from './workflow-history-helper.ee'; @@ -36,14 +36,14 @@ export class WorkflowHistoryController { next(); } - @Get('/workflow/:workflowId', { middlewares: [paginationListQueryMiddleware] }) - async getList(req: WorkflowHistoryRequest.GetList) { + @Get('/workflow/:workflowId') + async getList(req: WorkflowHistoryRequest.GetList, _res: Response, @Query query: PaginationDto) { try { return await this.historyService.getList( req.user, req.params.workflowId, - req.query.take ?? DEFAULT_TAKE, - req.query.skip ?? 0, + query.take ?? DEFAULT_TAKE, + query.skip ?? 0, ); } catch (e) { if (e instanceof SharedWorkflowNotFoundError) { diff --git a/packages/cli/test/integration/insights/insights.api.test.ts b/packages/cli/test/integration/insights/insights.api.test.ts index b351edca4a..17db449f28 100644 --- a/packages/cli/test/integration/insights/insights.api.test.ts +++ b/packages/cli/test/integration/insights/insights.api.test.ts @@ -72,9 +72,9 @@ describe('GET /insights/by-workflow', () => { take: 'not_a_number', }, ])( - 'Call should return internal server error with invalid pagination query parameters', + 'Call should return bad request with invalid pagination query parameters', async (queryParams) => { - await agents.owner.get('/insights/by-workflow').query(queryParams).expect(500); + await agents.owner.get('/insights/by-workflow').query(queryParams).expect(400); }, );