fix(rest-api): coerces strings into numbers in external-router when type="number" (#10213)

@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<string, unknown>, 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<string, unknown>, 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)
     }
   }
 }

@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<string, unknown>) => {
+  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' })
+  })
+})