diff --git a/ixwebsocket/IXSocket.cpp b/ixwebsocket/IXSocket.cpp index 32ce21ae..d8442b37 100644 --- a/ixwebsocket/IXSocket.cpp +++ b/ixwebsocket/IXSocket.cpp @@ -222,4 +222,30 @@ namespace ix } } } + + std::pair 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)); + } } diff --git a/ixwebsocket/IXSocket.h b/ixwebsocket/IXSocket.h index 24f1dce2..3454c4a1 100644 --- a/ixwebsocket/IXSocket.h +++ b/ixwebsocket/IXSocket.h @@ -45,6 +45,7 @@ namespace ix const CancellationRequest& isCancellationRequested); bool writeBytes(const std::string& str, const CancellationRequest& isCancellationRequested); + std::pair readLine(const CancellationRequest& isCancellationRequested); int getErrno() const; static bool init(); // Required on Windows to initialize WinSocket diff --git a/ixwebsocket/IXWebSocketServer.cpp b/ixwebsocket/IXWebSocketServer.cpp index 22bcaacf..549b03e0 100644 --- a/ixwebsocket/IXWebSocketServer.cpp +++ b/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 lock(_clientsMutex); diff --git a/ixwebsocket/IXWebSocketServer.h b/ixwebsocket/IXWebSocketServer.h index e503b829..f3be5063 100644 --- a/ixwebsocket/IXWebSocketServer.h +++ b/ixwebsocket/IXWebSocketServer.h @@ -31,6 +31,7 @@ namespace ix void setOnConnectionCallback(const OnConnectionCallback& callback); void start(); void wait(); + void stop(); std::pair listen(); @@ -65,7 +66,6 @@ namespace ix // Methods void run(); - void stop(); void handleConnection(int fd); // Logging diff --git a/ixwebsocket/IXWebSocketTransport.cpp b/ixwebsocket/IXWebSocketTransport.cpp index a253f2c1..ff0d43a9 100644 --- a/ixwebsocket/IXWebSocketTransport.cpp +++ b/ixwebsocket/IXWebSocketTransport.cpp @@ -254,9 +254,20 @@ namespace ix _socket = std::make_shared(); } - 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 - char line[256]; - int i; - for (i = 0; i < 2 || (i < 255 && line[i-2] != '\r' && line[i-1] != '\n'); ++i) + // Read HTTP status line + auto lineResult = _socket->readLine(isCancellationRequested); + auto lineValid = lineResult.first; + 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); - } - } - line[i] = 0; - if (i == 255) - { - return WebSocketInitResult(false, 0, std::string("Got bad status line connecting to ") + _url); + return WebSocketInitResult(false, 0, + std::string("Failed reading HTTP status line from ") + 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(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]; - int i; - for (i = 0; i < 2 || (i < 255 && line[i-2] != '\r' && line[i-1] != '\n'); ++i) - { - 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); - } + auto lineResult = _socket->readLine(isCancellationRequested); + auto lineValid = lineResult.first; + auto line = lineResult.second; // 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 WebSocketInitResult(false, 401, errorMsg); + return sendErrorResponse(400, "Missing Sec-WebSocket-Key value"); } char output[29] = {}; diff --git a/ixwebsocket/IXWebSocketTransport.h b/ixwebsocket/IXWebSocketTransport.h index 86727cb5..a40c0aec 100644 --- a/ixwebsocket/IXWebSocketTransport.h +++ b/ixwebsocket/IXWebSocketTransport.h @@ -165,5 +165,7 @@ namespace ix // Parse HTTP headers std::pair parseHttpHeaders(const CancellationRequest& isCancellationRequested); + + WebSocketInitResult sendErrorResponse(int code, std::string reason); }; } diff --git a/makefile b/makefile index 5be1fed7..03134b1f 100644 --- a/makefile +++ b/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) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 881ed3c9..d7e7401e 100644 --- a/test/CMakeLists.txt +++ b/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 ) diff --git a/test/IXTestWebSocketServer.cpp b/test/IXTestWebSocketServer.cpp new file mode 100644 index 00000000..613e3184 --- /dev/null +++ b/test/IXTestWebSocketServer.cpp @@ -0,0 +1,173 @@ +/* + * IXTestWebSocketServer.cpp + * Author: Benjamin Sergeant + * Copyright (c) 2019 Machine Zone. All rights reserved. + */ + +#include +#include +#include +#include + +#include "IXTest.h" + +#include "catch.hpp" + +using namespace ix; + +namespace ix +{ + bool startServer(ix::WebSocketServer& server) + { + server.setOnConnectionCallback( + [&server](std::shared_ptr 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); + } +} diff --git a/test/run.sh b/test/run.sh index f9ee444c..2b0cd1f2 100644 --- a/test/run.sh +++ b/test/run.sh @@ -4,4 +4,5 @@ mkdir build cd build cmake .. || exit 1 make || exit 1 -./ixwebsocket_unittest + +./ixwebsocket_unittest ${TEST}