Error: Schema for validaton is undefined

[c0sx]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.15.0

Plugin version

No response

Node.js version

any

Operating system

Windows

Operating system version (i.e. 20.04, 11.3, 10)

10

Description

4.15 release have a backward compatibility issue (PR): it throws an error if schema for validation is undefined.

For example, we get a schemas and pass them to fastify.

const { headers, body } = getSchemas();

fastify.post('/', {
  schema: {
    headers,
    body
  }
}

Steps to Reproduce

const fastify = require("fastify")({ logger: true });

fastify.get(
  "/",
  {
    schema: {
      headers: undefined,
    },
  },
  () => {
    return "hello";
  }
);

const start = async () => {
  try {
    await fastify.listen({ port: 3000 });
  } catch (err) {
    fastify.log.error(err);
    process.exit(1);
  }
};

start();

Server throws an error Failed building the validation schema for GET: /, due to error headers schema is undefined" when started

Expected Behavior

I think that undefined is acceptable value, which means that validation will be skipped.

[mcollina]

@Eomm I think we should revert #4620, and possibly undo that for v5.

[Eomm]

I added that fix because I had undefined schemas and all was working fine.

I tested what I was going adding into the db and I spot additional fields that was unexpected.. Then a debug session pointed me to a typo!

I'm sure out there there are a lot of typos and developers that think to have set a validation schema.. But they are not!

I think that setting a schema as undefined is a "don't care" action. Do not add the property if you don't want the schema instead.

[climba03003]

Instead of full revert, we can change to a warning for v4 and bring it back to runtime error in v5.

[c0sx]

I added that fix because I had undefined schemas and all was working fine.

I tested what I was going adding into the db and I spot additional fields that was unexpected.. Then a debug session pointed me to a typo!

I'm sure out there there are a lot of typos and developers that think to have set a validation schema.. But they are not!

I think that setting a schema as undefined is a "don't care" action. Do not add the property if you don't want the schema instead.

Yes, but this is a breaking change for me :C

[Eomm]

I like the @climba03003 proposal as mediation 👍

Even though I think an error shows a real security issue tbh

[c0sx]

I like the @climba03003 proposal as mediation 👍

Even though I think an error shows a real security issue tbh

Sorry, I can't agree with you. This change was about type system and contracts, but not about security. You're trying to shift the user's responsibility on library level.

[mcollina]

@c0sx there are two aspect at play here:

1. the unwanted breakage
2. the actual change

In Fastify, we do deprecation on the current supported line and removal in the next major. If we turned it into a warning, all your code would still work.

Overall I'm not convinced of the actual change. More specifically, undefined is not returned when rendering JSON, so its presence should be "neutral". I would still throw for null.

[Eomm]

More specifically, undefined is not returned when rendering JSON, so its presence should be "neutral". I would still throw for null.

The logic is: if de dav has set the property field (body, headers etc..) the value must not be falsy. I don't think the rendering json is a thing here, a human has written body: fastify.getSchema('typo') and the endpoint does not run the validation.

This change was about type system and contracts, but not about security. You're trying to shift the user's responsibility on library level.

Absolutely yes, it is both. Security because an endpoint must validate its input and because the framework must help the dev and understand clearly unwanted behaviour: why a developer should write body: undefined?

I mean, you need to change it to this code to make it optional:

{
  schema: {
    ...(bodySchema? { body: bodySchema }: null )
  }
}

This will not add the body when the bodySchema does not exist

[franher]

Hello all,

I think #4620 caused a breaking change for me too.

We have schema.headers as undefined in several OpenAPI schemas. Is it an unexpected breaking change that has landed in a minor version, v4.15.0?

[climba03003]

Is it an unexpected breaking change that has landed in a minor version, v4.15.0?

Throughout the conversation, we should change the Error to Warning first.
Would you like to work on this?

[franher]

Is it an unexpected breaking change that has landed in a minor version, v4.15.0?

Throughout the conversation, we should change the Error to Warning first. Would you like to work on this?

Sure, @climba03003. I think this is an excellent opportunity for me to become a Fastify contributor.

To ensure I understand the scope of the change, what we want is to emit a warning instead of throwing an error, don't we?

I have seen there is an abstraction for using warning on Fastify, i.e. warning.emit('FSTDEP009'). Should I use this? If you confirm to me the approach, I can work on it today (I'm in the CET timezone, so my day is starting right now :) ).

[Eomm]

Should I use this?

Yes, so it can be tested easily as well

[franher]

Should I use this?

Yes, so it can be tested easily as well

Thank you for the feedback, @Eomm. I'm working on it today! stay tuned :P