Start using FastifyRequest objects for the upgrade request so hooks fire and decorators can be used

[airhorns]

🚀 Feature Proposal

fastify-websocket currently passes the raw http.IncomingMessage into websocket route handlers, not a FastifyRequest object. I think instead passing a FastifyRequest to handlers would improve a number of things.

Motivation

First off, it is unfortunate to not have access to FastifyRequest's basic helpers. The higher level object is more useful than the raw IncomingMessage and has some handy doodads for getting say route params or the logger to work with. Second, fastify promotes and has a really healthy ecosystem of things that decorate FastifyRequest that websocket handlers also can't access. Things like a session from fastify-secure-session, utilities from fastify-url-data, or user-land decorators to get a database client or what have you aren't available in route handlers.

Finally, it's unfortunate that hooks don't run for websocket handlers. We'd have to discuss semantically how this might work, but, if we could come up with sane semantics for this, it'd be really handy. I want to use hooks to implement authorization for example -- deny requests based on the presence of a session value or whatever. This same logic needs to apply to work done over normal HTTP requests as well as work done over websocket connections, but currently I have to duplicate it in a hook and in a verifyClient option. One place (especially one place that had access to decorators) would be simpler.

I also think there's an argument to be made that websocket handlers being different like this violates the principle of least surprise.

How

This'd definitely be a breaking change, so knowing that, we'd either introduce a new API or just do a major version bump. With that, I think it'd be possible to actually implement this by using ws ability to bind to an existing http.Server, as is detailed here: https://www.npmjs.com/package/ws#external-https-server

I think for an UPGRADE request we'd probably not want to provide a FastifyReply object, or we'd want to change the API to use FastifyReply to actually start the websocket connection. I think there's really only one thing you want to do with an upgrade request, which is delegate it to the websocket server, so I think having to do that manually isn't really worth it, and that it's a special enough scenario that having a different handler signature is actually a good thing anyways. So I'd suggest having websocket handlers look pretty similar to how the do right now.

Hooks wise, the semantics that make sense to me are firing everything up until the reply is sending. So, onRequest, preParsing, preValidation, preHandler, preSerialization, and onError could all be relied on, but onSend and onResponse would never be called and would be documented as such. I am not really familiar with Fastify's internals enough to say how hard it would be to actually do this weird half-chain of hooks, but were that easy enough, this makes sense to me.

Context

I don't really know why fastify-websocket works the way it does right now, but if I had to guess, I'd say it's because its really simple to implement. That's not a bad reason. I am finding that I have to do more work in userland because of this though, and I think implementing this inside the library instead of in bespoke and crappy ways outside it each time would save devs lots of time.

Thoughts?

[airhorns]

A couple other things worth noting:

• the decorators that I want to use (fastify-secure-session mostly) aren't exposed as utility functions that I could invoke outside the normal hook cycle. So, in a websocket handler, I'm not really able to emulate what the decorators would do own their own using the http.IncomingMesage. It'd be at least possible to use the session if fastify-secure-session exported a utility that let you just pass in a raw Cookie: header and get out a session, but outside of websockets, there isn't really a need to do that, so it's nice and simple and doesn't do that. So, right now, the only way to get the session is via FastifyRequest.
• I don't think you can get a FastifyRequest from an http.IncomingMessage in userland. All the plugins that add stuff to requests expect to be able to do so during hook execution, so it seems like it'd be really hard to just do wrappedRequest = new FastifyRequest(...) or something like that in a websocket handler. So as far as i can tell, there isn't actually a workaround for this issue right now

[mcollina]

Thanks for this really nice writeup.

This module is implemented the way it is because it was extremely simple to implement. In order to get the hook system to comply, we might have to implement this in Fastify itself. Note that the current hook system is designed for a request/response pattern, so something that works for websockets might be something slightly different,

If you are keen to work on this problem, I'll be very happy to review.

[DRoet]

I love the structure of this plugin, but having an easy way to share session state between sockets/requests is the only thing I'm missing. Access to FastifyRequest would be amazing.

[AyoubElk]

In order to get the hook system to comply, we might have to implement this in Fastify itself. Note that the current hook system is designed for a request/response pattern, so something that works for websockets might be something slightly different

I'd like to work on this feature but I'm still a little uncertain of how it would be best accomplished.

@mcollina, It'd be of great help if you can give me any indication on what you think would be a viable approach to get the hook system to comply.

[mcollina]

@AyoubElk this feature should be designed from scratch. I do not have an API in mind.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[airhorns]

I haven't made any progress on this but it is worth mentioning one bit of prior art from the Ruby world which is rack.hijack. I feel this pattern would be a good way to make this work in the Fastify world. Read more about rack hijack here: https://old.blog.phusion.nl/2013/01/23/the-new-rack-socket-hijacking-api/

[AyoubElk]

I'll be working on this feature and I wanted to get some feedback on how I'm thinking of implementing it before I get started:

• Implement this feature inside fastify itself, this will allow access to the needed internals. Alternatively, we can discuss how we might expose some of the fastify instance internals (hooks system, etc..) to allow more flexibility and advanced features for other plugins as well if that's of interest.
• Try to reuse the fastify instance router, this will cause a conflict between the handler and wsHandler in case both are provided, as far as I can tell we can avoid that by either:
  • Suffixing the websocket route url on route registration with some value that will make it unique (i.e. .socket). In the websocket incoming connection handler we can append the suffix before calling router.lookup.
  • Having a separate router instance that will handle websocket routes (similarly to how fourOhFour has its own router)
  • Updating find-my-way in order to support websocket routes having the same method/path as a normal route
• Regarding hooks, we can run all of them up except preSerialization and onResponse. This might be easily done by adding a condition in reply.send() which simply exits the function in case we're handling a websocket request

[airhorns]

Implement this feature inside fastify itself, this will allow access to the needed internals. Alternatively, we can discuss how we might expose some of the fastify instance internals (hooks system, etc..) to allow more flexibility and advanced features for other plugins as well if that's of interest.

That makes sense to me!

What do you think about creating a reply.hijack function that tells fastify that the userland code is going to be responsible for sending the request and to stop running the rest of the hooks?

With that in place, I think websocket handlers could just be totally normal handlers in the same router, because you can use this: https://www.npmjs.com/package/ws#multiple-servers-sharing-a-single-https-server to upgrade one request at a time. You'd do await reply.hijack(); wss.handleUpgrade(reply.request.raw) or something like that. I think you'd also have to listen to the inner http.Server's upgrade event and dispatch those requests through the normal fastify dispatch cycle to trigger that route handler running in the first place.

[adminy]

To be honest it's still unfamiliar to the end user on how the api is implemented.

Websockets have urls, have everything to make it more like the a http method.

fastify.ws('/:urlParams', (request, ws) => {
  // reply can be the socket object,
  // the method itself is called upon the connection object
  // ws.send({msg: 'hello from server'})
})

Nobody needs to know that websockets is actually a get request upgrade.

[airhorns]

@adminy can you be more specific about what you'd like to see? I don't really understand.

Nobody needs to know that websockets is actually a get request upgrade.

I kind of disagree. That's how they work, there's no other way to initiate them, you can't around this. Let's say you want to build authentication for websocket connections to your server. If your server already serves normal authenticated HTTP requests, you already have some way to look at an HTTP request and figure out who is making it and if you trust them, lets say through cookies and fastify-secure-session. A really easy way to authenticate websocket requests would be to use the exact same mechanism, and look at the cookies for the incoming upgrade request. You can only do that because there's a request, and if you are given the request. Forcing users of this library to re-build authentication or any of their middleware/plugin stack in a websocket-specific way just because the transport is different seems unnecessary and wasteful.

[adminy]

@adminy can you be more specific about what you'd like to see? I don't really understand.

An alternative api where you can say fastify.ws(url, cb(request, socket))

Your security example is a middleware problem. I was only talking about the request api.

I can write a polyfill myself like so:

fastify.ws = (url, cb) => {
  fastify.get(url, { websocket: true }, (connection, req, params) => { 
    req.params = params
    cb(req, connection.socket)
  })
}

[airhorns]

I see what you mean, it does remove some noise for creating websocket handlers, but I think the mission of this library should be to make more possible as opposed to just making the common case easier. I think some folks might want access to the connection object instead of just the socket, and I think the PRs linked above remove the need to work with the params as a separate object, and would pass in a FastifyRequest with the params already set up fine.

[adminy]

Cool, let me know when this PR makes it into the release, otherwise I'll have an unexpexted upgrade which breaks the working version.