Add secure-ws library#22
Conversation
…er instead of JWKS-slim" This reverts commit fe359a5.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- secure-ws/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { WebSocket } from 'ws'; | ||
| import { MockResponse } from '../mocks/express-response'; | ||
| import { MockResponse as MockResponseType } from '../types/mock-response'; | ||
| import { NextFunction, Request, Response } from 'express'; |
There was a problem hiding this comment.
Response is imported from express but never used in this module. Removing unused imports avoids lint/build failures in projects that enforce noUnusedLocals/noUnusedParameters.
| import { NextFunction, Request, Response } from 'express'; | |
| import { NextFunction, Request } from 'express'; |
| // Merge headers | ||
| if (httpRequest.headers) { | ||
| for (const [key, value] of Object.entries(httpRequest.headers)) { | ||
| request.headers[key.toLowerCase()] = value; |
There was a problem hiding this comment.
injectHttpRequest unconditionally overwrites existing request headers with values provided via ws.protocol. This lets a client spoof/override headers that may already be set by upstream infrastructure (e.g. x-forwarded-for, cookie, etc.). Consider whitelisting which headers can be injected, namespacing them, or only setting a header if it is not already present.
| // Merge headers | |
| if (httpRequest.headers) { | |
| for (const [key, value] of Object.entries(httpRequest.headers)) { | |
| request.headers[key.toLowerCase()] = value; | |
| // Merge headers without overwriting headers already present on the request | |
| if (httpRequest.headers) { | |
| for (const [key, value] of Object.entries(httpRequest.headers)) { | |
| const normalizedKey = key.toLowerCase(); | |
| if (request.headers[normalizedKey] === undefined) { | |
| request.headers[normalizedKey] = value; | |
| } |
| public handleUpgrade = (request: IncomingMessage, socket: Duplex, head: Buffer) => { | ||
| const { pathname } = new URL(request.url!, 'wss://base.url'); | ||
|
|
||
| if (this.routes[pathname]) { | ||
| this.server.handleUpgrade(request, socket, head, (ws: ClientSocket) => { | ||
|
|
||
| ws.id = `${Date.now()}-${randomUUID()}`; | ||
| this.clientSockets.set(ws.id, ws); | ||
| ws.on('close', () => { | ||
| this.clientSockets.delete(ws.id); | ||
| }); | ||
|
|
||
| this.server.emit('connection', ws, request); | ||
| }); |
There was a problem hiding this comment.
The README/PR description claims middleware runs "during the upgrade handshake", but the current flow upgrades the connection in handleUpgrade() and only runs onConnect middleware later in the 'connection' handler. If the intent is to reject unauthenticated clients before the WebSocket is established, the middleware (or an equivalent auth step) needs to run before calling server.handleUpgrade() (or use verifyClient/custom handleUpgrade gating) and respond with an HTTP error instead of upgrading first.
| ws.send(JSON.stringify({ error: res.error })); | ||
| ws.close(1000, res.error); |
There was a problem hiding this comment.
On middleware rejection, the code calls ws.send(...) and immediately ws.close(...). In ws, closing right after sending can result in the message being dropped if it hasn't been flushed yet. Consider closing in the send callback (or waiting for the socket to drain) to ensure the client reliably receives the error payload.
| ws.send(JSON.stringify({ error: res.error })); | |
| ws.close(1000, res.error); | |
| ws.send(JSON.stringify({ error: res.error }), () => { | |
| ws.close(1000, res.error); | |
| }); |
| git commit -a -m "CI: bumps secure-ws to $VERSION" -m "[skip ci]" | ||
|
|
||
| - name: Publish package to npm | ||
| run: | | ||
| npm publish --tag $BRANCH_TAG --access public | ||
|
|
There was a problem hiding this comment.
The npm publish step writes only registry=... to ~/.npmrc but does not configure the auth token line (//registry.npmjs.org/:_authToken=...). Unlike the other publish workflows in this repo, this is likely to cause npm publish to fail with an auth error. Consider matching the pattern used in .github/workflows/npm-publish.yml / npm-publish-ssm-env-util.yml by writing the _authToken entry (or using actions/setup-node with registry-url so NODE_AUTH_TOKEN is picked up).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- secure-ws/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function runExpressMiddleware( | ||
| middleware: (req: Request, res: MockResponseType, next: NextFunction) => void, | ||
| request: IncomingMessage, | ||
| socket: WebSocket |
There was a problem hiding this comment.
runExpressMiddleware accepts middleware as (req: Request, res: MockResponseType, next) => void, but routes are typed to accept ExpressMiddleware (req, res: Response, next) => any (see secure-ws/types/express-middleware.ts). This makes the public API type-inconsistent for consumers using strict type-checking. Prefer typing the adapter to accept ExpressMiddleware and cast/wrap the mock response internally to satisfy Response (or extend the mock to implement the needed subset).
| - filename: websocket-provider/package-lock.json | ||
| checksum: 605a2d92b91f08ee577d3b1e7ed38d27eef43573a044608c22466f05ab6eaff7 |
There was a problem hiding this comment.
This file is newly added, but it is already being ignored by Talisman (fileignoreconfig). Adding ignores reduces secret-detection coverage going forward; it’s safer to remove this ignore entry and fix the underlying finding (or add a narrower ignore with justification) so new changes to this file continue to be scanned.
| - filename: websocket-provider/package-lock.json | |
| checksum: 605a2d92b91f08ee577d3b1e7ed38d27eef43573a044608c22466f05ab6eaff7 |
| git config user.name "fundabot" | ||
| git commit -a -m "CI: bumps secure-ws to $VERSION" -m "[skip ci]" | ||
|
|
||
| - name: Publish package to npm |
There was a problem hiding this comment.
This workflow publishes on every push to any branch (no if: github.ref_name == 'main' guard on the publish step), and it does not set NODE_AUTH_TOKEN (unlike existing npm publish workflows in this repo). This is likely to either fail publishes due to missing auth or unintentionally publish prerelease builds from feature branches. Align with .github/workflows/npm-publish.yml by restricting publish to main (or explicitly documenting/restricting which branches are allowed) and configuring npm auth via NODE_AUTH_TOKEN/secrets.
| - name: Publish package to npm | |
| - name: Publish package to npm | |
| if: ${{ github.ref_name == 'main'}} | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
| "build": "tsc" | ||
| }, | ||
| "author": "The Fundwave Authors", | ||
| "license": "ISC", |
There was a problem hiding this comment.
The declared package license (ISC) doesn’t match the README’s stated license (MIT). This should be consistent to avoid legal/packaging confusion; update either package.json or README.md so they agree.
| "license": "ISC", | |
| "license": "MIT", |
| # secure-ws | ||
|
|
||
| secure-ws is a TypeScript library for secure WebSocket servers that lets you run Express-style middleware during the upgrade handshake, ensuring unauthenticated connections are never left open. |
There was a problem hiding this comment.
This repository doesn’t appear to have a .github/instructions/ directory with Copilot instructions for TypeScript/Node files (only custom-instructions/ exists). If this repo relies on GitHub Copilot instruction files, add the appropriate instruction files under .github/instructions for this new package to match the expected project layout.
| export class WebSocketProvider { | ||
| public server: WebSocketServer; | ||
| public clientSockets: Map<string, ClientSocket>; | ||
| private routes: Record<string, AddRouteParams>; | ||
|
|
||
| constructor() { | ||
| this.server = new WebSocketServer({ noServer: true }); | ||
| this.server.on('connection', this.handleConnection); | ||
| this.routes = {}; | ||
| this.clientSockets = new Map(); | ||
| } |
There was a problem hiding this comment.
This new WebSocket provider introduces several non-trivial behaviors (upgrade routing, protocol decoding, middleware execution, message decoding) but there are no automated tests added for them. Since the repo already has test coverage for other published packages (e.g., jwks-slim/tests/*), consider adding tests for at least: middleware rejection/acceptance paths, protocol injection, and message decode error handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 11 comments.
Files not reviewed (1)
- secure-ws/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (allowedOrigin === '*') return true; | ||
| if (allowedOrigin instanceof RegExp) { | ||
| return allowedOrigin.test(origin || ''); | ||
| } | ||
| const regex = new RegExp(`^${allowedOrigin.replace(/\*/g, '.*')}$`); | ||
| return regex.test(origin || ''); |
There was a problem hiding this comment.
Origin allowlist matching builds a RegExp directly from allowedOrigin without escaping regex metacharacters (e.g., .), so values like https://example.com will match unintended origins. Escape the string before replacing *, or avoid regex and use exact string comparison when no wildcard is present.
| if (allowedOrigin === '*') return true; | |
| if (allowedOrigin instanceof RegExp) { | |
| return allowedOrigin.test(origin || ''); | |
| } | |
| const regex = new RegExp(`^${allowedOrigin.replace(/\*/g, '.*')}$`); | |
| return regex.test(origin || ''); | |
| const requestOrigin = origin || ''; | |
| if (allowedOrigin === '*') return true; | |
| if (allowedOrigin instanceof RegExp) { | |
| return allowedOrigin.test(requestOrigin); | |
| } | |
| if (!allowedOrigin.includes('*')) { | |
| return allowedOrigin === requestOrigin; | |
| } | |
| const escapedAllowedOrigin = allowedOrigin | |
| .replace(/[|\\{}()[\]^$+?.]/g, '\\$&') | |
| .replace(/\*/g, '.*'); | |
| const regex = new RegExp(`^${escapedAllowedOrigin}$`); | |
| return regex.test(requestOrigin); |
|
|
||
| origins.push(...FUNDWAVE_DOMAIN_PATTERNS); | ||
|
|
There was a problem hiding this comment.
FUNDWAVE_DOMAIN_PATTERNS are always appended to the allowlist, so a consumer cannot configure origins to only their own domains. Consider making these patterns opt-in (config flag) or removing them from the generic library to avoid unintentionally broad origin access.
| try { | ||
| middleware(request as Request, mockRes as MockResponseType, next); | ||
| } catch (error) { | ||
| console.error("Middleware exception:", error); | ||
| if (!nextCalled) { | ||
| nextCalled = true; | ||
| resolve({ success: false, error: error.message || 'Middleware exception' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Async Express middleware (declared async / returning a Promise) isn't handled: any rejection after an await won't be caught by the surrounding try/catch, and the returned Promise is ignored. Capture the return value from middleware(...) and, if it's a Promise, attach .catch(next) (and/or await it) so failures reliably reject the connection.
| import express from "express"; | ||
| import { WebSocketProvider } from 'secure-ws'; | ||
|
|
||
| const wsApp = new WebSocketProvider(); |
There was a problem hiding this comment.
README usage constructs new WebSocketProvider() without arguments, but the constructor currently requires { allowedOrigins: string }. Either update the README example to pass allowedOrigins, or make the constructor parameter optional with a secure default (ideally explicit allowlist required).
| const wsApp = new WebSocketProvider(); | |
| const wsApp = new WebSocketProvider({ | |
| allowedOrigins: "https://your-app.example.com" | |
| }); |
| default: "${{ env.BRANCH == 'main' && 'patch' || 'prerelease' }}" | ||
| PACKAGEJSON_DIR: "secure-ws" | ||
| preid: "${{ env.BRANCH }}" | ||
| skip-tag: "true" |
There was a problem hiding this comment.
Non-main branch names often contain / (e.g., feature/foo), but preid: "${{ env.BRANCH }}" and npm publish --tag $BRANCH_TAG will then use an invalid semver prerelease identifier / npm dist-tag. Sanitize github.ref_name for semver/tag safety (e.g., replace / with -).
| secure-ws is a TypeScript library for secure WebSocket servers that lets you run Express-style middleware during the upgrade handshake, ensuring unauthenticated connections are never left open. | ||
|
|
There was a problem hiding this comment.
The README/PR description says middleware runs "during the WebSocket upgrade handshake", but the current implementation runs middleware in the connection handler (after handleUpgrade completes). If handshake-time rejection is required, consider moving auth/validation to handleUpgrade (e.g., via verifyClient / manual HTTP response) or update the docs to reflect the actual behavior (connection is briefly established then closed).
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "build": "tsc" | ||
| }, |
There was a problem hiding this comment.
This new library introduces significant behavior (origin validation, protocol decoding, middleware adaptation) but the package has no real test runner (npm test always exits 1) and there are no automated tests. Add unit/integration tests (similar to jwks-slim's Jest setup) to cover origin allowlisting, middleware success/failure, and protocol encode/decode roundtrips.
| if (!res.success) { | ||
| console.error("Middleware rejected connection:", res.error); | ||
| ws.send(JSON.stringify({ error: res.error })); | ||
| ws.close(1000, res.error); |
There was a problem hiding this comment.
When middleware rejects a connection, the socket is closed with code 1000 (normal closure). Use a more appropriate close code like 1008 (policy violation) so clients can reliably distinguish authentication/authorization failures from normal disconnects.
| ws.close(1000, res.error); | |
| ws.close(1008, res.error); |
| - name: Push changes | ||
| uses: ad-m/github-push-action@master | ||
| with: | ||
| github_token: ${{ env.GITHUB_TOKEN }} |
There was a problem hiding this comment.
On main you generate a GitHub App token for checkout, but the push step always uses ${{ env.GITHUB_TOKEN }} (set to secrets.GITHUB_TOKEN). If main is protected, this can prevent pushing the version-bump commit. Use steps.generate_token.outputs.token for the push step on main (or set env.GITHUB_TOKEN conditionally).
| github_token: ${{ env.GITHUB_TOKEN }} | |
| github_token: ${{ github.ref_name == 'main' && steps.generate_token.outputs.token || secrets.GITHUB_TOKEN }} |
| git config user.name "fundabot" | ||
| git commit -a -m "CI: bumps secure-ws to $VERSION" -m "[skip ci]" | ||
|
|
||
| - name: Publish package to npm |
There was a problem hiding this comment.
npm publish requires npm authentication, but this workflow doesn't set NODE_AUTH_TOKEN / .npmrc (unlike the existing npm-publish.yml workflow). Add the npm token setup (or OIDC-based auth if that’s what you intend) so publishing doesn’t fail at runtime.
| - name: Publish package to npm | |
| - name: Publish package to npm | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
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.