[NP] route validation does not properly parse multipart/form-data payloads

[pgayvallet]

Related to #56734

While trying to migrate the /api/saved_objects/_resolve_import_errors route (in src/legacy/server/saved_objects/routes), I encountered what seems to be a regression in multipart-data handling in new platform.

The route accepts multipart form-data payload, that contains both a file and some additional data (retries):

({
  path: '/api/saved_objects/_resolve_import_errors',
  method: 'POST',
  config: {
    pre: [prereqs.getSavedObjectsClient],
    payload: {
      maxBytes: server.config().get('savedObjects.maxImportPayloadBytes'),
      output: 'stream',
      allow: 'multipart/form-data',
    },
    validate: {
      payload: Joi.object({
        file: Joi.object().required(),
        retries: Joi.array()
          .items(
            Joi.object({
              type: Joi.string().required(),
              id: Joi.string().required(),
              overwrite: Joi.boolean().default(false),
              replaceReferences: Joi.array()
                .items(
                  Joi.object({
                    type: Joi.string().required(),
                    from: Joi.string().required(),
                    to: Joi.string().required(),
                  })
                )
                .default([]),
            })
          )
          .required(),
      }).default(),
    },
  },

In legacy, the additional retries data is properly parsed and converted to the expected array type

When doing the equivalent in new platform:

router.post(
    {
      path: '/api/saved_objects/_resolve_import_errors',
      options: {
        body: {
          maxBytes: maxImportPayload,
          output: 'stream',
          accepts: 'multipart/form-data',
        },
      },
      validate: {
        body: schema.object({
          file: schema.stream(),
          retries: schema.arrayOf(
            schema.object({
              type: schema.string(),
              id: schema.string(),
              overwrite: schema.boolean({ defaultValue: false }),
              replaceReferences: schema.arrayOf(
                schema.object({
                  type: schema.string(),
                  from: schema.string(),
                  to: schema.string(),
                }),
                { defaultValue: [] }
              ),
            })
          ),
        }),
      },
    },

The retries property received a raw string causing the validation to fails. In particular

kibana/test/api_integration/apis/saved_objects/resolve_import_errors.js

Lines 32 to 44 in 8e9a8a8

	it('should return 200 and import nothing when empty parameters are passed in', async () => {
	await supertest
	.post('/api/saved_objects/_resolve_import_errors')
	.field('retries', '[]')
	.attach('file', join(__dirname, '../../fixtures/import.ndjson'))
	.expect(200)
	.then(resp => {
	expect(resp.body).to.eql({
	success: true,
	successCount: 0,
	});
	});
	});

fails with following error: [request body.retries]: expected value of type [array] but got [string]'

I'm not sure if the regression is caused by a configuration difference in our hapi route config (the root server is the same in LP and NP, so it cannot be a root configuration mismatch), or if the difference if in the use of schema instead or raw joi. Maybe joi was parsing strings when expecting another type and we are not?

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[pgayvallet]

TIL, when using multipart/form-data, all data are sent either a streams for files, or raw strings for other fields (which make sense regarding the format of the actual payload)

JOI automatically tries to convert strings to proper types by default, which explains why this works in legacy route handlers:

node_modules/joi/lib/types/array/index.js

if (typeof value === 'string' && options.convert) {
    internals.safeParse(value, result);
 }

However in NP we are using config-schema instead, and we are ignoring the convert joi option in our custom types coerce overrides, and are returning an error if the raw input is not of the expected type. This works with json payloads, as they are pre-parsed by hapi, but that doesn't work with multipart/form-data

kibana/packages/kbn-config-schema/src/internals/index.ts

Lines 369 to 382 in bbe700d

	{
	name: 'array',
	base: Joi.array(),
	coerce(value: any, state: State, options: ValidationOptions) {
	// If value isn't defined, let Joi handle default value if it's defined.
	if (value !== undefined && !Array.isArray(value)) {
	return this.createError('array.base', { value }, state, options);
	}
	return value;
	},
	rules: [anyCustomRule],
	},

Should the condition on all our coerce block be changed to no longer ignore the convert option?

if (value !== undefined && !options.convert && !Array.isArray(value)) {

The following test does pass with the suggested modification, and fails otherwise (so does the integration test in the initial description)

test.only('uses convert', () => {
  const type = schema.arrayOf(schema.string());
  expect(type.validate('["hello"]', {}, 'foo-namespace')).toMatchInlineSnapshot(`
    Array [
      "hello",
    ]
  `);
});

However, as convert defaults to true, this could be a tricky change, as that causes a real change in the whole kbn-schema default behavior.

One option would be to use convert false for route validation, and only use true for multipart form datas (even if that means some changes in the request validation, as I don't think the validator is aware of the route options ATM). However this option is not currently supported by kbn-config-schema, so these are some heavy changes (annd introduce a 4rth parameter to Type.validate...)

kibana/packages/kbn-config-schema/src/types/type.ts

Lines 71 to 82 in 94b2b83

	public validate(value: any, context: Record<string, any> = {}, namespace?: string): V {
	const { value: validatedValue, error } = internals.validate(value, this.internalSchema, {
	context,
	presence: 'required',
	});
	if (error) {
	throw new ValidationError(error as any, namespace);
	}
	return validatedValue;
	}

One other possible workaround (taking the description example) would be to type retries as string, and manually validate and convert it in the handler body, but that seems very hacky, and is poor integration / developer experience.

Overall, I think the less impacting solution is still to change our custom coerce methods to uses the convert option (which means converting everything, as convert will always be true atm in kbn-config-schema)

WDYT?

[joshdover]

Overall, I think the less impacting solution is still to change our custom coerce methods to uses the convert option (which means converting everything, as convert will always be true atm in kbn-config-schema)

One issue that has come up is that the NP router does not parse query param values. So if you have a request like /my-route?a=[1,2] and you set your query config to schema.object({ a: schema.arrayOf(schema.number()) }) it will always fail with an error because the value of query.a is a string, not an array.

Would this change also fix that situation?

[jbudz]

Do we have version this needs to block on? 7.7?

[pgayvallet]

One issue that has come up is that the NP router does not parse query param values. So if you have a request like /my-route?a=[1,2] and you set your query config to schema.object({ a: schema.arrayOf(schema.number()) }) it will always fail with an error because the value of query.a is a string, not an array.

Would need to manually test to be sure, but it would probably also fix that yes. As long as the stringified value is parsable as expected type, it would work.