From cbf31ca7c6eb46a54ef33728ed7e21b83c64df07 Mon Sep 17 00:00:00 2001 From: Josh Kelley Date: Mon, 12 Sep 2022 16:37:12 -0400 Subject: [PATCH 1/2] Fix: Initial messages could be lost The use of await when handling the "initially" WebSocket message may cause incoming messages to be lost. Event handlers like `ws.on('message', ...)` may not get installed until after the asynchronous method completes, so any WebSocket events during that time period aren't handled. Fixes #20 --- hapi-plugin-websocket.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/hapi-plugin-websocket.js b/hapi-plugin-websocket.js index dc4a399..6b45a5f 100644 --- a/hapi-plugin-websocket.js +++ b/hapi-plugin-websocket.js @@ -217,7 +217,7 @@ const register = async (server, pluginOptions) => { /* optionally inject an empty initial message */ if (routeOptions.initially) { /* inject incoming WebSocket message as a simulated HTTP request */ - const response = await server.inject({ + server.inject({ /* simulate the hard-coded POST request */ method: "POST", @@ -233,19 +233,19 @@ const register = async (server, pluginOptions) => { plugins: { websocket: { mode: "websocket", ctx, wss, ws, wsf, req, peers, initially: true } } - }) - - /* any HTTP redirection, client error or server error response - leads to an immediate WebSocket connection drop */ - if (response.statusCode >= 300) { - const annotation = `(HAPI handler responded with HTTP status ${response.statusCode})` - if (response.statusCode < 400) - ws.close(1002, `Protocol Error ${annotation}`) - else if (response.statusCode < 500) - ws.close(1008, `Policy Violation ${annotation}`) - else - ws.close(1011, `Server Error ${annotation}`) - } + }).then(response => { + /* any HTTP redirection, client error or server error response + leads to an immediate WebSocket connection drop */ + if (response.statusCode >= 300) { + const annotation = `(HAPI handler responded with HTTP status ${response.statusCode})` + if (response.statusCode < 400) + ws.close(1002, `Protocol Error ${annotation}`) + else if (response.statusCode < 500) + ws.close(1008, `Policy Violation ${annotation}`) + else + ws.close(1011, `Server Error ${annotation}`) + } + }); } /* hook into WebSocket message retrieval */ From 2a34635d9d13c9bdcc76d91b607473ad1ef52820 Mon Sep 17 00:00:00 2001 From: Josh Kelley Date: Sun, 7 May 2023 16:51:22 -0400 Subject: [PATCH 2/2] Fix security issue WebSocket messages should not be processed until `initially` has finished and has a chance to optionally close the connection completely. Disconnects and protocol-level errors are processed regardless - these occur at a lower level and should be preserved for consistency with connects. --- hapi-plugin-websocket.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/hapi-plugin-websocket.js b/hapi-plugin-websocket.js index 6b45a5f..a181664 100644 --- a/hapi-plugin-websocket.js +++ b/hapi-plugin-websocket.js @@ -215,9 +215,10 @@ const register = async (server, pluginOptions) => { delete headers["accept-encoding"] /* optionally inject an empty initial message */ + let initially; if (routeOptions.initially) { /* inject incoming WebSocket message as a simulated HTTP request */ - server.inject({ + initially = server.inject({ /* simulate the hard-coded POST request */ method: "POST", @@ -236,7 +237,9 @@ const register = async (server, pluginOptions) => { }).then(response => { /* any HTTP redirection, client error or server error response leads to an immediate WebSocket connection drop */ - if (response.statusCode >= 300) { + if (response.statusCode < 300) { + return true; + } else { const annotation = `(HAPI handler responded with HTTP status ${response.statusCode})` if (response.statusCode < 400) ws.close(1002, `Protocol Error ${annotation}`) @@ -244,6 +247,7 @@ const register = async (server, pluginOptions) => { ws.close(1008, `Policy Violation ${annotation}`) else ws.close(1011, `Server Error ${annotation}`) + return false; } }); } @@ -252,6 +256,10 @@ const register = async (server, pluginOptions) => { if (routeOptions.frame === true) { /* framed WebSocket communication (correlated request/reply) */ wsf.on("message", async (ev) => { + if (initially && !(await initially)) { + return; + } + /* allow application to hook into raw WebSocket frame processing */ routeOptions.frameMessage.call(ctx, { ctx, wss, ws, wsf, req, peers }, ev.frame) @@ -294,6 +302,10 @@ const register = async (server, pluginOptions) => { else { /* plain WebSocket communication (uncorrelated request/response) */ ws.on("message", async (message) => { + if (initially && !(await initially)) { + return; + } + /* inject incoming WebSocket message as a simulated HTTP request */ const response = await server.inject({ /* simulate the hard-coded POST request */ @@ -322,6 +334,7 @@ const register = async (server, pluginOptions) => { /* hook into WebSocket disconnection */ ws.on("close", () => { /* allow application to hook into WebSocket disconnection */ + /* note that this is done even if the `initially` handler closes the connection */ routeOptions.disconnect.call(ctx, { ctx, wss, ws, wsf, req, peers }) /* stop tracking the peer */ @@ -330,6 +343,7 @@ const register = async (server, pluginOptions) => { }) /* allow application to hook into WebSocket error processing */ + /* note that this is done even if the `initially` handler closes the connection */ ws.on("error", (error) => { routeOptions.error.call(ctx, { ctx, wss, ws, wsf, req, peers }, error) })