server unittest for validating client request / new timeout cancellation handling (need refactoring)

ixwebsocket/IXSocket.cpp

@@ -222,4 +222,30 @@ namespace ix
             }
         }
     }
+
+    std::pair<bool, std::string> Socket::readLine(const CancellationRequest& isCancellationRequested)
+    {
+        // FIXME: N should be a parameter
+
+        // Read first line
+        const int N = 255;
+        char line[N+1];
+        int i;
+        for (i = 0; i < 2 || (i < N && line[i-2] != '\r' && line[i-1] != '\n'); ++i)
+        {
+            if (!readByte(line+i, isCancellationRequested))
+            {
+                return std::make_pair(false, std::string());
+            } 
+        }
+
+        if (i == N)
+        {
+            return std::make_pair(false, std::string());
+        }
+
+        line[i] = 0;
+
+        return std::make_pair(true, std::string(line));
+    }
 }

ixwebsocket/IXSocket.h

@@ -45,6 +45,7 @@ namespace ix
                       const CancellationRequest& isCancellationRequested);
         bool writeBytes(const std::string& str,
                         const CancellationRequest& isCancellationRequested);
+        std::pair<bool, std::string> readLine(const CancellationRequest& isCancellationRequested);
 
         int getErrno() const;
         static bool init(); // Required on Windows to initialize WinSocket

ixwebsocket/IXWebSocketServer.cpp

@@ -219,19 +219,22 @@ namespace ix
         }
 
         auto status = webSocket->connectToSocket(fd);
-        if (!status.success)
+        if (status.success)
+        {
+            // Process incoming messages and execute callbacks 
+            // until the connection is closed
+            webSocket->run();
+        }
+        else
         {
             std::stringstream ss;
             ss << "WebSocketServer::handleConnection() error: "
+               << status.http_status
+               << " error: "
                << status.errorStr;
             logError(ss.str());
-            return;
         }
 
-        // Process incoming messages and execute callbacks 
-        // until the connection is closed
-        webSocket->run();
-
         // Remove this client from our client set
         {
             std::lock_guard<std::mutex> lock(_clientsMutex);

ixwebsocket/IXWebSocketServer.h

@@ -31,6 +31,7 @@ namespace ix
         void setOnConnectionCallback(const OnConnectionCallback& callback);
         void start();
         void wait();
+        void stop();
 
         std::pair<bool, std::string> listen();
 
@@ -65,7 +66,6 @@ namespace ix
 
         // Methods
         void run();
-        void stop();
         void handleConnection(int fd);
 
         // Logging

ixwebsocket/IXWebSocketTransport.cpp

@@ -254,9 +254,20 @@ namespace ix
             _socket = std::make_shared<Socket>();
         }
 
-        auto isCancellationRequested = [this]() -> bool
+        // FIXME: timeout should be configurable
+        auto start = std::chrono::system_clock::now();
+        auto timeout = std::chrono::seconds(10);
+
+        auto isCancellationRequested = [this, start, timeout]() -> bool
         {
-            return _requestInitCancellation;
+            // Was an explicit cancellation requested ?
+            if (_requestInitCancellation) return true;
+            
+            auto now = std::chrono::system_clock::now();
+            if ((now - start) > timeout) return true;
+
+            // No cancellation request
+            return false;
         };
 
         std::string errMsg;
@@ -300,27 +311,22 @@ namespace ix
             return WebSocketInitResult(false, 0, std::string("Failed sending GET request to ") + url);
         }
 
-        // Read first line
+        // Read HTTP status line
-        char line[256];
+        auto lineResult = _socket->readLine(isCancellationRequested);
-        int i;
+        auto lineValid = lineResult.first;
-        for (i = 0; i < 2 || (i < 255 && line[i-2] != '\r' && line[i-1] != '\n'); ++i)
+        auto line = lineResult.second;
+
+        if (!lineValid)
         {
-            if (!_socket->readByte(line+i, isCancellationRequested))
+            return WebSocketInitResult(false, 0,
-            {
+                                       std::string("Failed reading HTTP status line from ") + url);
-                return WebSocketInitResult(false, 0, std::string("Failed reading HTTP status line from ") + url);
-            } 
-        }
-        line[i] = 0;
-        if (i == 255)
-        {
-            return WebSocketInitResult(false, 0, std::string("Got bad status line connecting to ") + _url);
         }
 
         // Validate status
         int status;
 
         // HTTP/1.0 is too old.
-        if (sscanf(line, "HTTP/1.0 %d", &status) == 1)
+        if (sscanf(line.c_str(), "HTTP/1.0 %d", &status) == 1)
         {
             std::stringstream ss;
             ss << "Server version is HTTP/1.0. Rejecting connection to " << host
@@ -330,7 +336,7 @@ namespace ix
         }
 
         // We want an 101 HTTP status
-        if (sscanf(line, "HTTP/1.1 %d", &status) != 1 || status != 101)
+        if (sscanf(line.c_str(), "HTTP/1.1 %d", &status) != 1 || status != 101)
         {
             std::stringstream ss;
             ss << "Got bad status connecting to " << host
@@ -380,6 +386,28 @@ namespace ix
         return WebSocketInitResult(true, status, "", headers);
     }
 
+    WebSocketInitResult WebSocketTransport::sendErrorResponse(int code, std::string reason)
+    {
+        std::stringstream ss;
+        ss << "HTTP/1.1 ";
+        ss << code;
+        ss << "\r\n";
+        ss << reason;
+        ss << "\r\n";
+
+        auto isCancellationRequested = [this]() -> bool
+        {
+            return _requestInitCancellation;
+        };
+
+        if (!_socket->writeBytes(ss.str(), isCancellationRequested))
+        {
+            return WebSocketInitResult(false, 500, "Failed sending response");
+        }
+
+        return WebSocketInitResult(false, code, reason);
+    }
+
     // Server
     WebSocketInitResult WebSocketTransport::connectToSocket(int fd)
     {
@@ -391,28 +419,28 @@ namespace ix
         _socket.reset();
         _socket = std::make_shared<Socket>(fd);
 
-        auto isCancellationRequested = [this]() -> bool
+        // FIXME: timeout should be configurable
+        auto start = std::chrono::system_clock::now();
+        auto timeout = std::chrono::seconds(3);
+
+        auto isCancellationRequested = [this, start, timeout]() -> bool
         {
-            return _requestInitCancellation;
+            // Was an explicit cancellation requested ?
+            if (_requestInitCancellation) return true;
+            
+            auto now = std::chrono::system_clock::now();
+            if ((now - start) > timeout) return true;
+
+            // No cancellation request
+            return false;
         };
 
         std::string remote = std::string("remote fd ") + std::to_string(fd);
 
         // Read first line
-        char line[256];
+        auto lineResult = _socket->readLine(isCancellationRequested);
-        int i;
+        auto lineValid = lineResult.first;
-        for (i = 0; i < 2 || (i < 255 && line[i-2] != '\r' && line[i-1] != '\n'); ++i)
+        auto line = lineResult.second;
-        {
-            if (!_socket->readByte(line+i, isCancellationRequested))
-            {
-                return WebSocketInitResult(false, 0, std::string("Failed reading HTTP status line from ") + remote);
-            } 
-        }
-        line[i] = 0;
-        if (i == 255)
-        {
-            return WebSocketInitResult(false, 0, std::string("Got bad status line connecting to ") + remote);
-        }
 
         // FIXME: Validate line content (GET /)
 
@@ -422,13 +450,12 @@ namespace ix
 
         if (!headersValid)
         {
-            return WebSocketInitResult(false, 401, "Error parsing HTTP headers");
+            return sendErrorResponse(400, "Error parsing HTTP headers");
         }
 
         if (headers.find("sec-websocket-key") == headers.end())
         {
-            std::string errorMsg("Missing Sec-WebSocket-Key value");
+            return sendErrorResponse(400, "Missing Sec-WebSocket-Key value");
-            return WebSocketInitResult(false, 401, errorMsg);
         }
 
         char output[29] = {};

ixwebsocket/IXWebSocketTransport.h

@@ -165,5 +165,7 @@ namespace ix
 
         // Parse HTTP headers
         std::pair<bool, WebSocketHttpHeaders> parseHttpHeaders(const CancellationRequest& isCancellationRequested);
+
+        WebSocketInitResult sendErrorResponse(int code, std::string reason);
     };
 }

makefile

@@ -20,6 +20,8 @@ build:
 # a builtin C++ server started in the unittest now
 test_server:
 	(cd test && npm i ws && node broadcast-server.js)
+
+# env TEST=Websocket_server make test
 test:
 	(cd test && sh run.sh)
 

test/CMakeLists.txt

@@ -19,6 +19,7 @@ include_directories(
 add_executable(ixwebsocket_unittest 
   test_runner.cpp
   cmd_websocket_chat.cpp
+  IXTestWebSocketServer.cpp
   IXTest.cpp
   msgpack11.cpp
 )

test/IXTestWebSocketServer.cpp

@@ -0,0 +1,173 @@
+/*
+ *  IXTestWebSocketServer.cpp
+ *  Author: Benjamin Sergeant
+ *  Copyright (c) 2019 Machine Zone. All rights reserved.
+ */
+
+#include <iostream>
+#include <ixwebsocket/IXSocket.h>
+#include <ixwebsocket/IXWebSocket.h>
+#include <ixwebsocket/IXWebSocketServer.h>
+
+#include "IXTest.h"
+
+#include "catch.hpp"
+
+using namespace ix;
+
+namespace ix
+{
+    bool startServer(ix::WebSocketServer& server)
+    {
+        server.setOnConnectionCallback(
+            [&server](std::shared_ptr<ix::WebSocket> webSocket)
+            {
+                webSocket->setOnMessageCallback(
+                    [webSocket, &server](ix::WebSocketMessageType messageType,
+                       const std::string& str,
+                       size_t wireSize,
+                       const ix::WebSocketErrorInfo& error,
+                       const ix::WebSocketCloseInfo& closeInfo,
+                       const ix::WebSocketHttpHeaders& headers)
+                    {
+                        if (messageType == ix::WebSocket_MessageType_Open)
+                        {
+                            std::cerr << "New connection" << std::endl;
+                            std::cerr << "Headers:" << std::endl;
+                            for (auto it : headers)
+                            {
+                                std::cerr << it.first << ": " << it.second << std::endl;
+                            }
+                        }
+                        else if (messageType == ix::WebSocket_MessageType_Close)
+                        {
+                            std::cerr << "Closed connection" << std::endl;
+                        }
+                        else if (messageType == ix::WebSocket_MessageType_Message)
+                        {
+                            for (auto&& client : server.getClients())
+                            {
+                                if (client != webSocket)
+                                {
+                                    client->send(str);
+                                }
+                            }
+                        }
+                    }
+                );
+            }
+        );
+
+        auto res = server.listen();
+        if (!res.first)
+        {
+            std::cerr << res.second << std::endl;
+            return false;
+        }
+
+        server.start();
+        return true;
+    }
+}
+
+TEST_CASE("Websocket_server", "[websocket_server]")
+{
+    SECTION("Connect to the server, do not send anything. Should timeout and return 400")
+    {
+        int port = 8091;
+        ix::WebSocketServer server(port);
+        REQUIRE(startServer(server));
+
+        Socket socket;
+        std::string host("localhost");
+        std::string errMsg;
+        auto isCancellationRequested = []() -> bool
+        {
+            return false;
+        };
+        bool success = socket.connect(host, port, errMsg, isCancellationRequested);
+        REQUIRE(success);
+
+        auto lineResult = socket.readLine(isCancellationRequested);
+        auto lineValid = lineResult.first;
+        auto line = lineResult.second;
+
+        int status = -1;
+        REQUIRE(sscanf(line.c_str(), "HTTP/1.1 %d", &status) == 1);
+        REQUIRE(status == 400);
+
+        // FIXME: explicitely set a client timeout larger than the server one (3)
+
+        // Give us 500ms for the server to notice that clients went away
+        ix::msleep(500);
+        server.stop();
+        REQUIRE(server.getClients().size() == 0);
+    }
+
+    SECTION("Connect to the server. Send GET request without header. Should return 400")
+    {
+        int port = 8092;
+        ix::WebSocketServer server(port);
+        REQUIRE(startServer(server));
+
+        Socket socket;
+        std::string host("localhost");
+        std::string errMsg;
+        auto isCancellationRequested = []() -> bool
+        {
+            return false;
+        };
+        bool success = socket.connect(host, port, errMsg, isCancellationRequested);
+        REQUIRE(success);
+
+        std::cout << "writeBytes" << std::endl;
+        socket.writeBytes("GET /\r\n", isCancellationRequested);
+
+        auto lineResult = socket.readLine(isCancellationRequested);
+        auto lineValid = lineResult.first;
+        auto line = lineResult.second;
+
+        int status = -1;
+        REQUIRE(sscanf(line.c_str(), "HTTP/1.1 %d", &status) == 1);
+        REQUIRE(status == 400);
+
+        // FIXME: explicitely set a client timeout larger than the server one (3)
+
+        // Give us 500ms for the server to notice that clients went away
+        ix::msleep(500);
+        server.stop();
+        REQUIRE(server.getClients().size() == 0);
+    }
+
+    SECTION("Connect to the server. Send GET request with correct header")
+    {
+        int port = 8093;
+        ix::WebSocketServer server(port);
+        REQUIRE(startServer(server));
+
+        Socket socket;
+        std::string host("localhost");
+        std::string errMsg;
+        auto isCancellationRequested = []() -> bool
+        {
+            return false;
+        };
+        bool success = socket.connect(host, port, errMsg, isCancellationRequested);
+        REQUIRE(success);
+
+        socket.writeBytes("GET /\r\nSec-WebSocket-Key: foobar\r\n\r\n", isCancellationRequested);
+        auto lineResult = socket.readLine(isCancellationRequested);
+        auto lineValid = lineResult.first;
+        auto line = lineResult.second;
+
+        int status = -1;
+        REQUIRE(sscanf(line.c_str(), "HTTP/1.1 %d", &status) == 1);
+        REQUIRE(status == 101);
+
+        // Give us 500ms for the server to notice that clients went away
+        ix::msleep(500);
+
+        server.stop();
+        REQUIRE(server.getClients().size() == 0);
+    }
+}

test/run.sh

@@ -4,4 +4,5 @@ mkdir build
 cd build
 cmake .. || exit 1
 make || exit 1
-./ixwebsocket_unittest
+
+./ixwebsocket_unittest ${TEST}