Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response schema compiler generated validation code does not check for null values on optional object fields #669

Closed
2 tasks done
rhighs opened this issue Nov 28, 2023 · 9 comments
Labels
bug Confirmed bug

Comments

@rhighs
Copy link
Contributor

rhighs commented Nov 28, 2023

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.17.0

Plugin version

No response

Node.js version

18.14.2

Operating system

macOS

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

13.2

Description

Defining a response schema with some non-required fields whose type is object, will carry @fastify/fast-json-stringify-compiler to generate static validation code that does not check for the possibility of such fields being null. In other words, it will only check for those fields being strictly equal to undefined. While it might seem reasonable to only check for undefined, the error caused by this scenario is very inconvenient and hard to track down as the generated code does not seem to lie on any specific file, instead it is allocated on some runtime memory.

I know this is kind of confusing so i'll provide a screenshot showing the portion of generated code I'm referring to, along with the schema that was associated with it.

image
and the schema that led to it:

const schema = {
  response: {
    200: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          tracks: {
            type: 'array',
            items: {
              type: 'object',
              properties: {
                ...
                genres: {
                  type: 'array',
                  items: {
                    type: 'object',
                    properties: {
                      id: { type: 'string' },
                      name: { type: 'string' },
                      parent: {
                        type: 'object', 
                        properties: { 
                          id: { type: 'string' },
                          name: { type: 'string' }
                        }
                      }
                    }
                  }
                  ...
                }
              }
            },
            additionalProperties: true
          }
        }
      },
      additionalProperties: true
    }
  }
}

If, for some reason, the "parent" field inside "genres" is presen with a value of null the validation code will enter the if branch calling whatever other generated function to validate its fields. Since accessing keys on null is illegal, the code will simply crash leading to this issue.

Steps to Reproduce

Just run this

const Fastify = require('fastify')
const fastify = Fastify()

const HOST = 'localhost'
const PORT = 3000

fastify.get('/', {
  handler: (req, res) => res.code(200).send({ someObject: null }),
  schema: {
    response: {
      200: {
        type: 'object',
        properties: {
          someObject: {
            type: 'object',
            properties: { id: { type: 'string' }, name: { type: 'string' } }
          }
        }
      }
    }
  }
})

fastify.listen({ host: HOST, port: PORT }).then(_ => console.log(`listening at ${HOST}:${PORT}`))

Then

$ curl -X GET localhost:3000

Expected Behavior

With the steps provided above you should get this response:

{"statusCode":500,"error":"Internal Server Error","message":"Cannot read properties of null (reading 'id')"}
@mcollina mcollina added the bug Confirmed bug label Dec 26, 2023
@mcollina
Copy link
Member

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina
Copy link
Member

I suspect this is a bug in fast-json-stringify.

@metcoder95
Copy link
Member

I'd say that we should transfer it, it seems scoped to fast-json-stringify using a normal JSON.stringify solves the problem.

Most likely fast-json-stringify might require to handle null as is.

@mcollina mcollina transferred this issue from fastify/fastify Dec 27, 2023
@rhighs
Copy link
Contributor Author

rhighs commented Dec 27, 2023

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

Sure, I can do it :)

@ivan-tymoshenko
Copy link
Member

@rhighs If you want to cover the case when object can be null I think you should use nullable keyword or type: ['object', 'null'].

@rhighs
Copy link
Contributor Author

rhighs commented Dec 27, 2023

@rhighs If you want to cover the case when object can be null I think you should use nullable keyword or type: ['object', 'null'].

Ok, sounds good. I'll have a look at the code first!

@ivan-tymoshenko
Copy link
Member

I mean use the nullable keyword in your schema.

@rhighs
Copy link
Contributor Author

rhighs commented Dec 27, 2023

I mean use the nullable keyword in your schema.

I saw the docs already at the time being, what I'm trying to say with this issue is that it's really hard to see what's happening when you stumble upon this bug. Having some sort of message would be great instead of exploding on the stack like above.

Unfortunately there are cases where you can't say for sure if a type is nullable, so you might want some sort of error when that happens.

@rhighs
Copy link
Contributor Author

rhighs commented Jan 6, 2024

Closing as completed by #670

@rhighs rhighs closed this as completed Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

5 participants
@mcollina @metcoder95 @ivan-tymoshenko @rhighs and others