fastify-websocket icon indicating copy to clipboard operation
fastify-websocket copied to clipboard

Message Validations

Open zekth opened this issue 3 years ago • 7 comments
trafficstars

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] 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 https://github.com/fastify/help/issues/102 It would be great to have message validations based on the route schemas

Example

No response

zekth avatar Apr 18 '22 19:04 zekth

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

mcollina avatar Apr 18 '22 22:04 mcollina

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
  })
})

climba03003 avatar Apr 19 '22 02:04 climba03003

@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?

zekth avatar Apr 19 '22 07:04 zekth

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.

climba03003 avatar Apr 19 '22 08:04 climba03003

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?

TheSainEyereg avatar Nov 30 '23 10:11 TheSainEyereg

@TheSainEyereg

Are you up to provide a PR?

Uzlopak avatar Nov 30 '23 10:11 Uzlopak

@Uzlopak sorry, I found out that you can specify custom WebSocket instance through opts.options.WebSocket, so my problem is solved

TheSainEyereg avatar Nov 30 '23 22:11 TheSainEyereg