diff --git a/@xen-orchestra/rest-api/src/router/external-router.mts b/@xen-orchestra/rest-api/src/router/external-router.mts index 6c221686f8..7d7b6d4ae2 100644 --- a/@xen-orchestra/rest-api/src/router/external-router.mts +++ b/@xen-orchestra/rest-api/src/router/external-router.mts @@ -61,8 +61,8 @@ export function createExternalRouter(swaggerOpenApiSpec: OpenAPIV3.Document): { if (route.security === undefined) route.security = '*' await expressAuthentication(req as AuthenticatedRequest, route.security, route.scope === 'acl' ? ['acl'] : []) - // Coerces query boolean from string to boolean - coerceBooleanQueryParams(req.query, route.query) + // Coerces query strings to the types declared by the route + coerceQueryParams(req.query, route.query) // Validate inputs, throws if invalid paramsSchema?.parse(req.params) @@ -337,11 +337,23 @@ function extractParametersFromZod(schema: z.ZodType, location: 'path' | 'query') return [] } -function coerceBooleanQueryParams(query: Record, def: RouteDefinition['query']): void { +// Express query params are always strings, coerce them to the types declared by the route +// Exported for testing +export function coerceQueryParams(query: Record, def: RouteDefinition['query']): void { if (!def) return for (const [key, field] of Object.entries(def)) { - if (field.type === 'boolean' && typeof query[key] === 'string') { - query[key] = query[key] === 'true' + const value = query[key] + if (typeof value !== 'string') continue + + if (field.type === 'boolean') { + if (value.toLowerCase() === 'true') { + query[key] = true + } else if (value.toLowerCase() === 'false') { + query[key] = false + } + } else if (field.type === 'number') { + // a non-numeric value becomes NaN, which is rejected by the zod validation + query[key] = value.trim() === '' ? NaN : Number(value) } } } diff --git a/@xen-orchestra/rest-api/src/router/external-router.test.mts b/@xen-orchestra/rest-api/src/router/external-router.test.mts new file mode 100644 index 0000000000..7714d4ad68 --- /dev/null +++ b/@xen-orchestra/rest-api/src/router/external-router.test.mts @@ -0,0 +1,49 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' + +// this module depends on the IoC container, which cannot be initialized on its own +// because of circular imports between the services: load the generated routes first, +// like `index.mts` does +import '../open-api/routes/routes.js' + +import { coerceQueryParams } from './external-router.mjs' +import type { RouteDefinition } from './types.mjs' + +const def: RouteDefinition['query'] = { + bool: { type: 'boolean', optional: true }, + num: { type: 'number', optional: true }, + str: { type: 'string', optional: true }, +} + +const coerce = (query: Record) => { + coerceQueryParams(query, def) + return query +} + +describe('coerceQueryParams', () => { + it('coerces booleans', () => { + assert.deepEqual(coerce({ bool: 'true' }), { bool: true }) + assert.deepEqual(coerce({ bool: 'false' }), { bool: false }) + assert.deepEqual(coerce({ bool: 'TRUE' }), { bool: true }) + assert.deepEqual(coerce({ bool: 'FALSE' }), { bool: false }) + }) + it('keeps non-boolean values as strings', () => { + assert.deepEqual(coerce({ bool: 'abc' }), { bool: 'abc' }) + }) + + it('coerces numbers', () => { + assert.deepEqual(coerce({ num: '42' }), { num: 42 }) + assert.deepEqual(coerce({ num: '-4.2' }), { num: -4.2 }) + }) + + it('coerces a non-numeric value to NaN, which fails the validation', () => { + assert.ok(Number.isNaN(coerce({ num: 'abc' }).num)) + }) + it('coerces empty strings to NaN, which fails the validation', () => { + assert.ok(Number.isNaN(coerce({ num: '' }).num)) + }) + + it('leaves strings and undeclared params untouched', () => { + assert.deepEqual(coerce({ str: '42', other: 'true' }), { str: '42', other: 'true' }) + }) +})