-
Notifications
You must be signed in to change notification settings - Fork 97
Online server #4502
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: master
Are you sure you want to change the base?
Online server #4502
Conversation
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.
Pull request overview
This PR introduces matchmaking server integration to enable players to discover and join multiplayer lobbies through an online service. The implementation is described as "minimal" and "incomplete" in the PR description.
Changes:
- Adds matchmaking client implementation with HTTP/HTTPS support (WinHTTP for Windows, cURL for Linux)
- Integrates matchmaking lobby registration and discovery into the network session flow
- Updates build configuration to include required dependencies (libcurl for Linux, winhttp for Windows)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/net_matchmaking.h | Defines matchmaking API with functions for lobby registration, ping, listing, and data structures |
| src/net_matchmaking.c | Implements matchmaking client with platform-specific HTTP handling and JSON parsing |
| src/frontmenu_net_data.cpp | Integrates lobby registration on session creation |
| src/front_network.c | Adds periodic matchmaking lobby fetching (5s interval) and lobby pinging (30s interval) |
| src/bflib_network.h | Adds getter for server port and matchmaking fetch function declaration |
| src/bflib_network.cpp | Implements server port getter and matchmaking lobby population logic |
| linux.mk | Adds net_matchmaking.c to sources and libcurl dependency |
| Makefile | Adds net_matchmaking.o to objects and winhttp library dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wchar_t wpath[128]; | ||
| mbstowcs(wmethod, method, 16); | ||
| mbstowcs(wpath, path, 128); | ||
| HINTERNET request = WinHttpOpenRequest(http_connection, wmethod, wpath, NULL, WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, WINHTTP_FLAG_SECURE); |
Copilot
AI
Jan 25, 2026
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.
The WinHTTP flags should include WINHTTP_FLAG_SECURE_DEFAULTS or specific security settings to ensure proper SSL/TLS certificate validation. Without this, the connection might not properly validate server certificates, potentially allowing man-in-the-middle attacks. Consider adding security flags when opening the request.
| HINTERNET request = WinHttpOpenRequest(http_connection, wmethod, wpath, NULL, WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, WINHTTP_FLAG_SECURE); | |
| DWORD flags = WINHTTP_FLAG_SECURE; | |
| #ifdef WINHTTP_FLAG_SECURE_DEFAULTS | |
| flags |= WINHTTP_FLAG_SECURE_DEFAULTS; | |
| #endif | |
| HINTERNET request = WinHttpOpenRequest(http_connection, wmethod, wpath, NULL, WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, flags); |
| TbBool matchmaking_register_lobby(const char *name, uint16_t port) { | ||
| if (lobby_registered) { | ||
| matchmaking_unregister_lobby(); | ||
| } | ||
| char body[256]; | ||
| snprintf(body, sizeof(body), "{\"name\":\"%s\",\"port\":%u}", name, port); |
Copilot
AI
Jan 25, 2026
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.
No input sanitization for lobby name: The lobby name is directly inserted into a JSON string without escaping special characters like quotes or backslashes. This could cause JSON parsing errors or potentially security issues. The name should be properly escaped before being inserted into the JSON body.
| TbBool matchmaking_register_lobby(const char *name, uint16_t port) { | |
| if (lobby_registered) { | |
| matchmaking_unregister_lobby(); | |
| } | |
| char body[256]; | |
| snprintf(body, sizeof(body), "{\"name\":\"%s\",\"port\":%u}", name, port); | |
| static void json_escape_string(const char *in, char *out, size_t out_size) | |
| { | |
| size_t i = 0; | |
| if (out_size == 0) { | |
| return; | |
| } | |
| /* Reserve space for terminating null */ | |
| while (*in && i + 1 < out_size) { | |
| unsigned char c = (unsigned char)*in++; | |
| if (c == '\"' || c == '\\') { | |
| if (i + 2 >= out_size) { | |
| break; | |
| } | |
| out[i++] = '\\'; | |
| out[i++] = (char)c; | |
| } else if (c == '\n') { | |
| if (i + 2 >= out_size) { | |
| break; | |
| } | |
| out[i++] = '\\'; | |
| out[i++] = 'n'; | |
| } else if (c == '\r') { | |
| if (i + 2 >= out_size) { | |
| break; | |
| } | |
| out[i++] = '\\'; | |
| out[i++] = 'r'; | |
| } else if (c == '\t') { | |
| if (i + 2 >= out_size) { | |
| break; | |
| } | |
| out[i++] = '\\'; | |
| out[i++] = 't'; | |
| } else { | |
| out[i++] = (char)c; | |
| } | |
| } | |
| out[i] = '\0'; | |
| } | |
| TbBool matchmaking_register_lobby(const char *name, uint16_t port) { | |
| if (lobby_registered) { | |
| matchmaking_unregister_lobby(); | |
| } | |
| char body[256]; | |
| char escaped_name[192]; | |
| json_escape_string(name, escaped_name, sizeof(escaped_name)); | |
| snprintf(body, sizeof(body), "{\"name\":\"%s\",\"port\":%u}", escaped_name, port); |
| char url[256]; | ||
| snprintf(url, sizeof(url), "%s%s", MATCHMAKING_SERVER_URL, path); | ||
| curl_easy_reset(curl_handle); | ||
| curl_easy_setopt(curl_handle, CURLOPT_URL, url); |
Copilot
AI
Jan 25, 2026
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.
No SSL certificate verification is configured for cURL. By default, cURL should verify certificates, but it's best practice to explicitly set CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST to ensure secure connections and prevent man-in-the-middle attacks. Consider adding: curl_easy_setopt(curl_handle, CURLOPT_SSL_VERIFYPEER, 1L) and curl_easy_setopt(curl_handle, CURLOPT_SSL_VERIFYHOST, 2L).
| curl_easy_setopt(curl_handle, CURLOPT_URL, url); | |
| curl_easy_setopt(curl_handle, CURLOPT_URL, url); | |
| curl_easy_setopt(curl_handle, CURLOPT_SSL_VERIFYPEER, 1L); | |
| curl_easy_setopt(curl_handle, CURLOPT_SSL_VERIFYHOST, 2L); |
| static CURL *curl_handle = NULL; | ||
| static size_t curl_response_pos = 0; | ||
|
|
||
| static size_t matchmaking_write_callback(void *contents, size_t size, size_t nmemb, void *userp) { | ||
| size_t realsize = size * nmemb; | ||
| if (curl_response_pos + realsize >= MATCHMAKING_RESPONSE_BUFFER_SIZE - 1) { | ||
| realsize = MATCHMAKING_RESPONSE_BUFFER_SIZE - 1 - curl_response_pos; | ||
| } | ||
| memcpy(response_buffer + curl_response_pos, contents, realsize); | ||
| curl_response_pos += realsize; | ||
| response_buffer[curl_response_pos] = '\0'; | ||
| return size * nmemb; | ||
| } | ||
|
|
||
| static int http_request(const char *method, const char *path, const char *body) { | ||
| if (curl_handle == NULL) { | ||
| curl_global_init(CURL_GLOBAL_DEFAULT); | ||
| curl_handle = curl_easy_init(); | ||
| if (curl_handle == NULL) { | ||
| return 0; | ||
| } | ||
| } |
Copilot
AI
Jan 25, 2026
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.
The cURL handle is never cleaned up with curl_easy_cleanup and curl_global_cleanup. Consider adding a cleanup function to free these resources when matchmaking is no longer needed or when the application exits.
| return Lb_OK; | ||
| } | ||
|
|
||
| void LbNetwork_FetchMatchmakingLobbies(void) { |
Copilot
AI
Jan 25, 2026
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.
Calling memset on sessions array clears all existing session entries, which could include locally discovered sessions on the LAN. This means matchmaking lobbies will completely replace local sessions rather than supplementing them. Consider either: (1) only clearing matchmaking-sourced sessions, (2) merging matchmaking results with existing sessions, or (3) documenting this behavior as intentional if matchmaking is meant to be exclusive.
| void LbNetwork_FetchMatchmakingLobbies(void) { | |
| void LbNetwork_FetchMatchmakingLobbies(void) { | |
| // NOTE: This intentionally clears *all* existing sessions (including locally | |
| // discovered LAN sessions). Matchmaking lobbies are meant to replace the | |
| // current session list rather than supplement it. |
| static HINTERNET http_session = NULL; | ||
| static HINTERNET http_connection = NULL; | ||
|
|
||
| static int http_request(const char *method, const char *path, const char *body) { | ||
| if (http_session == NULL) { | ||
| http_session = WinHttpOpen(L"KeeperFX/1.0", WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0); | ||
| if (http_session == NULL) { | ||
| return 0; | ||
| } | ||
| int timeout = MATCHMAKING_HTTP_TIMEOUT_MS; | ||
| WinHttpSetOption(http_session, WINHTTP_OPTION_CONNECT_TIMEOUT, &timeout, sizeof(timeout)); | ||
| WinHttpSetOption(http_session, WINHTTP_OPTION_SEND_TIMEOUT, &timeout, sizeof(timeout)); | ||
| WinHttpSetOption(http_session, WINHTTP_OPTION_RECEIVE_TIMEOUT, &timeout, sizeof(timeout)); | ||
| http_connection = WinHttpConnect(http_session, L"matchmaking.keeperfx.workers.dev", INTERNET_DEFAULT_HTTPS_PORT, 0); | ||
| if (http_connection == NULL) { | ||
| WinHttpCloseHandle(http_session); | ||
| http_session = NULL; | ||
| return 0; | ||
| } | ||
| } |
Copilot
AI
Jan 25, 2026
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.
The HTTP session and connection handles are never cleaned up. Consider adding a cleanup function that calls WinHttpCloseHandle on both http_connection and http_session when the application exits or when matchmaking is no longer needed. This would prevent resource leaks.
| static char current_lobby_id[MATCHMAKING_LOBBY_ID_LEN] = ""; | ||
| static TbBool lobby_registered = 0; | ||
| static char response_buffer[MATCHMAKING_RESPONSE_BUFFER_SIZE]; |
Copilot
AI
Jan 25, 2026
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.
The response_buffer is a static global variable shared across all HTTP requests. This creates a race condition if multiple threads call matchmaking functions simultaneously. Consider using thread-local storage or adding mutex protection around the HTTP requests to ensure thread safety.
Aiming for something minimal here, still incomplete