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

Params validation #315

Open
2 tasks done
Zack-Heisnberg opened this issue Jun 21, 2023 · 7 comments
Open
2 tasks done

Params validation #315

Zack-Heisnberg opened this issue Jun 21, 2023 · 7 comments

Comments

@Zack-Heisnberg
Copy link

Zack-Heisnberg commented Jun 21, 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.0.0

Plugin version

No response

Node.js version

19

Operating system

Linux

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

20.04

Description

the required for param validation doesn't work and allow empty strings

Steps to Reproduce

            fastify.get<{
              Params: {
                model: string;
              },
              Querystring: {
                sort: string;
                asc: number;
              }
            }>('/listAll/:model/', {
              schema: {
                params: {
                  type: 'object',
                  properties: {
                    model: { type: 'string' },
                  },
                  required: ['model'],
                },
                querystring: {
                  type: 'object',
                  properties: {
                    sort: { type: 'string' },
                    asc: { type: 'number' },
                  },
                  required: ['sort'],
                },
              },
            }, async function (request, reply) {
              return 'db endpoint for => ' + request.params.model;
            });

i undestand this can be fixed with

           params: {
                  type: 'object',
                  properties: {
                    model: { type: 'string' },
                    minLength: 1, 
                  },
                  required: ['model'],
                },

but i think the required on a string should do it for params

Current Behavior

CURL /listAll/t?sort=p => this is ok

CURL /listAll/t=> validation works , sort is required

CURL /listAll//?sort=p => get ok 200 , validation doesn't works , and allow the model to be empty

Expected Behavior

CURL /listAll/t?sort=p => this is ok

CURL /listAll/t=> validation works , sort is required

CURL /listAll//?sort=p => validation works , and doesn't allow the model to be empty

@giuliowaitforitdavide
Copy link

giuliowaitforitdavide commented Jun 21, 2023

I don't think this could be considered an issue, it seems like an acceptable behaviour to me: empty string is still a string, different from undefined

@Eomm
Copy link
Member

Eomm commented Jun 21, 2023

I would check here: https://stackoverflow.com/questions/45888524/empty-values-validation-in-json-schema-using-ajv

I would not expect that behaviour with that schema since typeof "" === 'string' is true

In any case, it is an AJV thing and it is not related to fastify:

but i think the required on a string should do it for params

Adding this custom logic could be a harmful as it would be not a standard JSON schema anymore - you can implement it on your app using the onRoute hook to add such logic tho

@Eomm Eomm closed this as not planned Won't fix, can't repro, duplicate, stale Jun 21, 2023
@Zack-Heisnberg
Copy link
Author

I don't think this could be considered an issue, it seems like an acceptable behaviour to me: empty string is still a string, different from undefined

I would check here: https://stackoverflow.com/questions/45888524/empty-values-validation-in-json-schema-using-ajv

I would not expect that behaviour with that schema since typeof "" === 'string' is true

In any case, it is an AJV thing and it is not related to fastify:

but i think the required on a string should do it for params

Adding this custom logic could be a harmful as it would be not a standard JSON schema anymore - you can implement it on your app using the onRoute hook to add such logic tho

i am not speaking about the validation of the string ,

i am speaking about the

     required: ['model'],

for a querystring

                      querystring: {
                        type: 'object',
                        properties: {
                          sort: { type: 'string' },
                          asc: { type: 'number' },
                        },
                        required: ['sort'],
                      },

even without the minlength it's work as the request when we don't set "sort" it's undefined

but for params

           params: {
                          type: 'object',
                          properties: {
                            model: { type: 'string' },
                          },
                          required: ['model'],
                        },

the required is useless as it is , because it's replaced by the route no matter what we define it

and the problem is when we access req.params.model , it's shows as undefined and not an empty string , so why the required is not working

@Eomm
Copy link
Member

Eomm commented Jun 23, 2023

and the problem is when we access req.params.model , it's shows as undefined and not an empty string , so why the required is not working

Can't reproduce

const app = require('fastify')({ logger: true })
app.get('/listAll/:model/', {
  schema: {
    params: {
      type: 'object',
      properties: {
        model: { type: 'string' }
      },
      required: ['model']
    },
    querystring: {
      type: 'object',
      properties: {
        sort: { type: 'string' },
        asc: { type: 'number' }
      },
      required: ['sort']
    }
  }
}, async (request, reply) => {
  return { hello: request.params.model }
  // prints {"hello":""}
})

app.listen({ port: 8080 })

Could you add a Minimal, Reproducible Example?

Without it, we are unable to help you.

@Zack-Heisnberg
Copy link
Author

interesting the issue is with using the fastify/autoload

how to reproduce

1 - install fastify-cli
npm install fastify-cli --global

2 - genereate a new server and run npm install
fastify generate . --esm

3 - change the root.js ( or any route point inside routes ) add the exact same code

                fastify.get('/listAll/:model/', {
                      schema: {
                        params: {
                          type: 'object',
                          properties: {
                            model: { type: 'string' }
                          },
                          required: ['model']
                        },
                        querystring: {
                          type: 'object',
                          properties: {
                            sort: { type: 'string' },
                            asc: { type: 'number' }
                          },
                          required: ['sort']
                        }
                      }
                    }, async (request, reply) => {
                      return { hello: request.params.model }
                      // prints {"hello":""}
                    })

4 - now this endpoint
http://localhost:3000/admin/listAll//?sort=dsa
you see how the :model is empty
you will get
{"hello":""}

and if you change route to

              fastify.get('/listAll/:model',

using this
http://localhost:3000/admin/listAll/?sort=dsa
we get
{"hello":""}

without using the @fastify/autoload this issue doesn't happen :/ instead you get 404 error
the issue happen with nested folders as well

so should i move this issue there ?

@Eomm
Copy link
Member

Eomm commented Jun 27, 2023

It worth investigating on it, I will move the issue 👍🏼

@Eomm Eomm reopened this Jun 27, 2023
@Eomm Eomm transferred this issue from fastify/fastify Jun 27, 2023
@Eomm
Copy link
Member

Eomm commented Jul 19, 2023

@Zack-Heisnberg could you check if fastify/fastify#4903 solved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants