-
Notifications
You must be signed in to change notification settings - Fork 1
Add secure-ws library #22
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
base: main
Are you sure you want to change the base?
Conversation
…er instead of JWKS-slim" This reverts commit fe359a5.
| this.server.emit('connection', ws, request); | ||
| }); | ||
| } else { | ||
| // socket.write('HTTP/1.1 404 Not Found\r\n\r\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a log here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private routes: Record<string, AddRouteParams>; | ||
|
|
||
| constructor() { | ||
| this.server = new WebSocketServer({ noServer: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we set noServer to false? Then we are able to set our own upgrade logic in the specified http server?
https://www.reddit.com/r/node/comments/sfgmum/can_someone_kindly_explain_what_noserver_mode/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If noServer is set to false (which it is by default), it spins up an http server on the specified port. In this case we won't have any control over the upgrade logic.
const wss = new WebSocketServer({ port: 8080 });
In our case, we already have an http server provided by express and we want to just use the socket connection after upgrade happens. Hence noServer.
The alternative is to pass the server to the constructor.
import { createServer } from 'https';
import { WebSocketServer } from 'ws';
const server = createServer({
cert: readFileSync('/path/to/cert.pem'),
key: readFileSync('/path/to/key.pem')
});
const wss = new WebSocketServer({ server });
But even in that case, this.server.handleUpgrade will be called automatically on all upgrades. We don't want this as we only want to upgrade if the enpoint matches a registered route [Ref lines 38-41 core/web-socket-provider.ts]
From docs:
One of port, server or noServer must be provided or an error is thrown.
Refs:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the idea is to run the WS server in parallel with regular REST API? I'd imagine having a separate server for WS only might help plan scaling up and rate limiting better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that depends on the number of ws endpoints one wants across services. If we want to have one endpoint for most of the services that would mean a common ws server would need access to all resources for those microservices. This ws server would become a monolith soon. For just a few endpoints, having a common ws makes sense for scalability but, in my opinion, the drawbacks would overcome the benefits as the number of endpoints increase.
The other alternative would be to have ws microservices in parallel with the existing microservices. But that would nearly double the number of microservices we have right now, leading to additional overhead of managing these services.
Another approach that was discussed previously was to have a common ws-service that only manages a single socket connection from the client and relays the request to the respective services. However it would still need ws endpoints on the services that it connects to. We can add rate limiting rules to this service and then later plug this between the client and server with the current architecture.
| @@ -0,0 +1,16 @@ | |||
| export class WSProtocolCodec { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather Base64UrlSafeCodec if it isn't specific to WS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is specific to the websocket as the name of the websocket protocol (Sec-WebSocket-Protocol) needs to be url safe
Add new secure-ws library
Overview
Add secure-ws library to manage websocket connections for Node.js and TypeScript projects.
Motivation
Resolution
Linked to getfundwave/discussions/issues/445
Closes getfundwave/discussions/issues/477
Features
Core WebSocket Provider:
Implements a modular WebSocket provider in
core/web-socket-provider.tswith support for custom protocols and extensibility.Protocol Codec:
Encapsulates WebSocket protocol encoding/decoding logic in
core/ws-protocol-codec.tsto send headers and body in the websocket upgrade handshakeMiddleware Support:
Includes utilities for adapting Express-style middleware to WebSocket handlers in
utils/middleware-adapter.tsandutils/inject-http-request.ts.