feat: add message validation #191

zekth · 2022-04-19T13:02:23Z

First implementation. Basically the use would be:

const Fastify = require('fastify')
const fastifyWebsocket = require('.')
const { ValidateWebSocket } = require('.')
const fastify = Fastify({ logger: true })

fastify.register(fastifyWebsocket, { options: { WebSocket: ValidateWebSocket } })

fastify.get('/', {
    websocket: true,
    schema: {
        body: {
            oneOf:
                [
                    { type: 'null' }, // This is required because of the upgrade
                    {
                        type: 'object',
                        properties: {
                            foo: {
                                type: 'string'
                            }
                        },
                        required: [
                            'foo'
                        ],
                        additionalProperties: false
                    }
                ]

        }
    }
}, (connection, request) => {
    connection.socket.on('message', () => {
        connection.socket.send('ok')
    })
})

fastify.listen(3000, '0.0.0.0', (err) => {
})

This also adds 2 way of handling the validation errors with the strictMode. If is set to true it closes the connection with a 1003 code, otherwise it sends the validator errors in the payload.

Kudos to @climba03003 who helped a lot

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

climba03003 · 2022-04-19T13:08:35Z

index.js

      const connection = WebSocket.createWebSocketStream(socket, opts.connectionOptions)
+      socket.afterDuplex = true
+      socket.validator = request && request.context.schema ? fastify.validatorCompiler({ schema: request.context.schema.body }) : null


It should move to onRoute hook. It compiles on every request which is not optimal.

// onRoute hook const kWebSocketSchema = Symbol('websocket-body-schema') requset.context[kWebSocketSchema] = routeOption.schema && routeOption.schema.body ? fastify.validatorCompiler({ schema: routeOption.schema.body }) : null // here socket.validator = requset.context[kWebSocketSchema]

mcollina

Could you emit an event in case a packet is dropped?

zekth · 2022-04-19T21:54:26Z

Could you emit an event in case a packet is dropped?

In a case of strictMode set to false? Yes sure

airhorns · 2022-04-20T12:57:14Z

@zekth I see why you'd want to do this but I am wondering if this could be done completely in userland or in a separate package. Not super opposed to merging this, but this implementation has to make a few big assumptions:

that a user wants to validate every message
that a developer wants to validate according to the same schema the whole time
that messages to validate are JSON
that a developer wants adopt this mode approach to errors.

I can contrive situations where each one of those things is not true. If any of those are not true, the developer has to go back to square one and write validation code themselves -- dunno how likely they are or how likely the happy path is where this thing helps. But because it hasn't come up a lot, I am a little hesitant to stick it right in the mainline code. It also requires some kinda ugly stuff like setting those properties on the socket.

One other thing we could do would be to add the facilities to make this possible and nice in userland, and then add examples that show how you might validation json yourself to the docs, as well as showing how you might validate msgpack or something like that too!

zekth · 2022-04-20T13:16:14Z

that a user wants to validate every message

I don't get why some people would want to validate only a subset of messages

that a developer wants to validate according to the same schema the whole time

Would you change your route schema on the fly?

that messages to validate are JSON

currently indeed it validates only the JSON, but using the schema definition we can validate any type, this case juste needs to be handled too.

that a developer wants adopt this mode approach to errors.

What would be a better approach?

One other thing we could do would be to add the facilities to make this possible and nice in userland, and then add examples that show how you might validation json yourself to the docs, as well as showing how you might validate msgpack or something like that too!

How would you achieve that? Seems like a bad DX as the user won't have the benefit of using the tools that fastify provides. The idea in here is to have only validated messages to be propagated inside the server message event.

Main idea behind this feature is to be able to build AsyncApi specs like: https://www.asyncapi.com/blog/websocket-part2
We have api contracts for Rest and GraphQl and i don't get why we won't have for websockets.

airhorns · 2022-04-20T14:06:07Z

I don't get why some people would want to validate only a subset of messages

Sometimes other systems are already doing validation and hand you back strings to send, like y.js or graphql-ws

Would you change your route schema on the fly?

No, but for stateful connections like a websocket, you might enter different phases of the connection that only accept certain message types at certain times. Lots of wire protocols have modes where they flip from some structured message format to a binary stream for a while, like Postgres' COPY instruction

currently indeed it validates only the JSON, but using the schema definition we can validate any type, this case juste needs to be handled too.

That's kinda what I am saying -- this is a maximalist approach, do we want that? I am ok if we do, it just increases the maintenance burden here for a potentially narrow set of use cases which require it.

What would be a better approach?

try / catch in the message handler for super explicitness. error event handlers kinda suck IMO because you need to remember to register them, otherwise the errors just go into the void. Stack based error handling at least interrupts the rest of your code's execution and is loud. This approach doesn't really jive with fastify-websocket doing the validation though. Another approach would be adding an onError callback option, that's a bit more explicit and we could make it required, and another higher level approach would be adding a higher order function like withMessageValidation(yourExistingHandler, options} that wrapped the handler to do validation outside fastify-websocket itself.

zekth · 2022-04-20T15:22:53Z

Lots of wire protocols have modes where they flip from some structured message format to a binary stream for a while, like Postgres' COPY instruction

https://github.com/fastify/fastify-websocket/pull/191/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R218

narrow set of use cases which require it.

I think we are in a chicken vs egg problem in here. Is there a reason not to push forward api contracts and message validation over websocket?

try / catch in the message handler for super explicitness.

With this approach there is no need for this feature in the end, because anybody can use their own validator in the end.

another higher level approach would be adding a higher order function like withMessageValidation(yourExistingHandler, options} that wrapped the handler to do validation outside fastify-websocket itself.

But the logic is in ValidateWebSocket which is not really inside fastify-websocket. That means any developer can extend it or replace it. Having another package just to handle the validation makes no sense to me. We can isolate this part: https://github.com/fastify/fastify-websocket/pull/191/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R62-R64
which in the end in a case of a non ValidatedWebsocket option there will be no change inside the fastify-websocket plugin.

zekth · 2022-04-26T13:07:15Z

Any thoughts @mcollina ?

mcollina

Docs are missing.

I'm ok with adding this feature. It fits with the ethos of Fastify and I would use it in a few places myself.

mcollina · 2022-04-26T21:58:00Z

index.js

@@ -200,7 +205,46 @@ function close (fastify, done) {
  server.close(done)
 }

+class ValidateWebSocket extends WebSocket {


Move this to a separate file in lib

mcollina · 2022-04-26T22:00:17Z

index.js

@@ -119,10 +122,12 @@ function fastifyWebsocket (fastify, opts, next) {
    // we always override the route handler so we can close websocket connections to routes to handlers that don't support websocket connections
    // This is not an arrow function to fetch the encapsulated this
    routeOptions.handler = function (request, reply) {
+      request.context[kWebSocketSchema] = request.context.schema ? fastify.validatorCompiler({ schema: request.context.schema.body }) : null


Do not reuse body, but use a different property.

If we're not using body property the issue is the validator will not be available because the fastify instance won't compute and expose it.

Maybe we need to expose something more from Fastify to do it.

Correct. I'll make a proposal in fastify

mcollina · 2022-04-26T22:02:07Z

index.js

+          return this.close(1003, 'Unsupported payload')
+        } else {
+          return this.send('Unsupported payload')
+        }


Instead of adding a strict flag, make the validation error handling configurable.

zekth added 2 commits April 19, 2022 14:55

feat: add message validation

04b938e

fix

d55d456

zekth requested review from climba03003 and mcollina April 19, 2022 13:03

climba03003 requested changes Apr 19, 2022

View reviewed changes

zekth added 2 commits April 19, 2022 15:16

fix: review

6316ab9

fix

fc525f5

mcollina requested changes Apr 19, 2022

View reviewed changes

zekth added 2 commits April 20, 2022 10:59

fix: add tests

dbba4b8

fix

a559ed0

mcollina requested changes Apr 26, 2022

View reviewed changes

zekth closed this Jan 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add message validation #191

feat: add message validation #191

zekth commented Apr 19, 2022

climba03003 Apr 19, 2022 •

edited

Loading

mcollina left a comment

zekth commented Apr 19, 2022

airhorns commented Apr 20, 2022

zekth commented Apr 20, 2022

airhorns commented Apr 20, 2022

zekth commented Apr 20, 2022

zekth commented Apr 26, 2022

mcollina left a comment

mcollina Apr 26, 2022

mcollina Apr 26, 2022

zekth Apr 27, 2022

mcollina Apr 27, 2022

zekth Apr 29, 2022

mcollina Apr 26, 2022

feat: add message validation #191

feat: add message validation #191

Conversation

zekth commented Apr 19, 2022

Checklist

climba03003 Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

zekth commented Apr 19, 2022

airhorns commented Apr 20, 2022

zekth commented Apr 20, 2022

airhorns commented Apr 20, 2022

zekth commented Apr 20, 2022

zekth commented Apr 26, 2022

mcollina left a comment

Choose a reason for hiding this comment

mcollina Apr 26, 2022

Choose a reason for hiding this comment

mcollina Apr 26, 2022

Choose a reason for hiding this comment

zekth Apr 27, 2022

Choose a reason for hiding this comment

mcollina Apr 27, 2022

Choose a reason for hiding this comment

zekth Apr 29, 2022

Choose a reason for hiding this comment

mcollina Apr 26, 2022

Choose a reason for hiding this comment

climba03003 Apr 19, 2022 •

edited

Loading