Skip to content

Add a WS subprotocol check #3

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add a WS subprotocol check #3

wants to merge 5 commits into from

Conversation

yanfali
Copy link
Collaborator

@yanfali yanfali commented Apr 12, 2022

This seems like an interesting idea, specify a websocket sub protocol

https://websockets.spec.whatwg.org/#ref-for-dom-websocket-websocket%E2%91%A0

I'm not sure how to do this in fastify with a websocket connection, so add the check on the message for now.

Description

yanfali added 5 commits April 11, 2022 21:10
 - just trying this out
 - add a verifyClient handler in options
 - specify initial subprotocol of XAPWS1
 - set sec-websocket-protocol headers on response if they match
 - verify sec-websocket-protocol on request
 - check the sub protocol header before upgrading connection
* @param protocols set of subprotocols from header
* @returns subprotocol accepted or false
*/
handleProtocols: (protocols: Set<string>) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd replace handleProtocols with:

    verifyClient: (info: { origin: string; req: IncomingMessage; secure: boolean }) => {
      return (
        (info.req.headers['sec-websocket-protocol'] &&
          validSubprotocols.find((x) => info.req.headers['sec-websocket-protocol'] === x)) ||
        false
      )
    }

If there is no subprotocol this will not be executed. See: websockets/ws#1552 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the notes they recommended not using this hook. I wasn't sure why

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle protocols only sets the headers as per the spec. It doesn't seem to care about verification

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried verify client in an earlier version of patch but the docs pointed at prevalidatoon for some reaspn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can have prevalidation, so we need to use app.route and add prehandler on it. We cannot use the shorthand route definition like app.get

})

app.register(async (fastify: FastifyInstance) => {
fastify.addHook('preValidation', async (req: FastifyRequest, reply: FastifyReply) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the comment on handleProtocols we can get rid of prevalidation hook.
also you don't need to await the reply.send() just return it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@@ -1,4 +1,4 @@
import fastify from 'fastify'
import fastify, { FastifyInstance } from 'fastify'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import fastify, { FastifyInstance } from 'fastify'
import fastify from 'fastify'
import type { FastifyInstance } from 'fastify'

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 this pull request may close these issues.

2 participants