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

Message Validations #190

Open
2 tasks done
zekth opened this issue Apr 18, 2022 · 7 comments
Open
2 tasks done

Message Validations #190

zekth opened this issue Apr 18, 2022 · 7 comments

Comments

@zekth
Copy link
Member

zekth commented Apr 18, 2022

Prerequisites

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

🚀 Feature Proposal

What i'm suggesting is using the route schema available already in fastify route definition and use it to validate messages payload.

  const handleUpgrade = (rawRequest, validate, callback) => {
    wss.handleUpgrade(rawRequest, rawRequest[kWs], rawRequest[kWsHead], (socket) => {
      wss.emit('connection', socket, rawRequest)
      
      const connection = WebSocket.createWebSocketStream(socket, opts.connectionOptions)
      connection.socket = socket
      connection.socket.on('message',(message)=>{
        // we would need something like so
        if(!validate(message)) {
          connection.socket.send('RIP')
        } else {
          connection.socket.emit('parsedMessage', {foo:'bar'}) // we emit a parsed message event
        }
      })
      connection.socket.on('newListener', event => {
        if (event === 'message') {
          connection.resume()
        }
      })

      callback(connection)
    })
  }

Constraint here is we cannot unemit an event on the socket, i propose the parsedMessage event which is used for the validation. message event handler will still listen to all the events.

Problem currently is we cannot have access to the ajv-compiler from the fastify instance. If we can have access to it on within the instance or the onRoute hook we can compile the schema from the route option and create the validation function for the websocket connection.

What do you think?

note: also slight change needed in the onRoute hook

    if (routeOptions.websocket || routeOptions.wsHandler) {
      let wsSchema = routeOptions.schema // otherwise the onUpgrade fails with invalid body
      delete routeOptions.schema

Motivation

Reviving fastify/help#102
It would be great to have message validations based on the route schemas

Example

No response

@mcollina
Copy link
Member

It's a great feature but possibly a bit too opinionated for WebSocket handling. Maybe it could be behind an option?

@climba03003
Copy link
Member

climba03003 commented Apr 19, 2022

It's a great feature but possibly a bit too opinionated for WebSocket handling. Maybe it could be behind an option?

Not really need an option, but we should provide a custom WebSocket class if people need this feature.

import { WebSocket } from 'ws'
class ValidateWebSocket extends WebSocket {
  on (event, handler, options) {
    if(event === 'message') {
      // wrap the event handler here
    }
    super.on(event, handler, options)
  }
}

import FastifyWebSocket, { ValidateWebSocket } from 'fastify-websocket'
fastify.register(FastifyWebSocket, {
  options: { WebSocket: ValidateWebSocket }
})

fastify.get('/', { websocket: true }, function(connection, request) {
  connection.socket.on('message', function(data) {
    // data here is the validated object
  })
})

@zekth
Copy link
Member Author

zekth commented Apr 19, 2022

@climba03003 do we really need to pass the validateWebSocket as an option to override the WebSocket one? If there a specific reason to not do it inside fastify-websocket? Other your approach looks fine to me.

I thought about options like:

fastify.get('/', { websocket: true, websocketValidation: true }, function(connection, request) {
  connection.socket.on('message', function(data) {
    // data here is the validated object
  })
})

We can detect if there is a schema on the route already but we turn the validation on on explicit mention (at least for the current state).

@mcollina do you think it's possible to expose the ajv-compiler within the fastifyInstance object?

@climba03003
Copy link
Member

climba03003 commented Apr 19, 2022

do we really need to pass the validateWebSocket as an option to override the WebSocket one? If there a specific reason to not do it inside fastify-websocket? Other your approach looks fine to me.

Yes, because overriding WebSocket object is a server options. And we need to expose the class if people (advanced user) want a custom class and also get the benefit validation.

I do not see a problem of passing WebSocket object conflict to the route option or plugin option. It can be a shorten option to replace the WebSocket class.

@zekth zekth mentioned this issue Apr 19, 2022
4 tasks
@TheSainEyereg
Copy link

Not really need an option, but we should provide a custom WebSocket class if people need this feature.

That would be great (even though the answer was 2 years ago). I want to use my custom deserializers when receiving messages and serializers on send, but as I can see it's not possible (yet) in @fastify/websocket. Is there any workaround for now?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 30, 2023

@TheSainEyereg

Are you up to provide a PR?

@TheSainEyereg
Copy link

@Uzlopak sorry, I found out that you can specify custom WebSocket instance through opts.options.WebSocket, so my problem is 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

Successfully merging a pull request may close this issue.

5 participants