Skip to content

Commit 69dfd73

Browse files
committed
fix: Plug shutdown UAF, reset reconnect state, lock down OAuth/HTTP
- plugin-main: tear down EgressLinkDock / WsPortalDock before deleting apiClient so OBS-owned dock destructors do not run against freed WsPortalClient state - WsClient: drop CURLOPT_CONNECTTIMEOUT to 3s so OBS shutdown waits at most ~3s per worker; document the QPointer/joinConnectThread invariant on the worker -> UI invokeMethod; pin pingTimestamp to successful sends and gate pong RTT on a non-zero timestamp; log a partial fragment-buffer discard on close; include the port in the Origin header when it is not the scheme default; document the order invariant of cleanup() (notifier disconnect/deleteLater before curl_easy_cleanup); document sendRaw()'s UI-thread invariant; align the fragment-buffer cast with qsizetype - CurlHttpClient: pin allowed protocols to http,https on the easy handle and on redirected requests; reject CR/LF/NUL in header keys or values and fail the request via the existing nullptr path; classify headerCallback overflow as ResponseTooLarge to match writeCallback; FIXME the RequestContext raw-new pattern for a separate RAII PR - HttpRequestInvoker: in the 401 retry refresh-failure branch, remove self from the queue under the mutex before unlink/emit/delete to match the non-401 path and avoid re-entrant deadlock - SRCLinkApiClient: use OS ephemeral port for the OAuth callback listener; switch the cached-token freshness check to the static QDateTime::currentSecsSinceEpoch() form; FIXME the terminate() fire-and-forget DELETE that is canceled by plugin teardown - SRCLinkWebSocketClient / WsPortalClient: reset reconnect counter, timer, and pending flag on a successful connection so flap-then- recover restarts from the short backoff - LocalHttpServer: hard-fail listen on Windows when setsockopt(SO_EXCLUSIVEADDRUSE) is rejected to keep the OAuth callback off shared ports - IngressLinkSource: guard the reload_stages button callback with QPointer<IngressLinkSource> and a non-null source check before re-opening source properties; defer onSettingsUpdate's else-branch saveSettings() to the UI thread, matching the if-branch pattern - CMakeLists: replace the mbedtls fail-fast text with an explicit unsupported-host message; reject SRC_LINK_LINUX_CA_PATH values containing quote or backslash before they reach the C macro - build-aux/.run-format.zsh: enforce the cmake-format 0.6.13 upper bound locally to mirror the CI pin - CLAUDE.md: drop the deleted Frameworks.cmake.in entry and replace the old request-invoker reference with the src/net/ subtree
1 parent f94ac43 commit 69dfd73

13 files changed

Lines changed: 127 additions & 27 deletions

CLAUDE.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ src-link/
2424
├── CMakePresets.json # CMake presets (windows-x64, macos, linux-x86_64, linux-aarch64, CI variants)
2525
├── buildspec.json # Build specification (name, version, dependencies, UUIDs)
2626
├── build.ps1 # Local Windows build script (RelWithDebInfo)
27-
├── Frameworks.cmake.in # macOS framework configuration
2827
├── .clang-format # C++ code style (clang-format ≥ 16)
2928
├── .cmake-format.json # CMake code style
3029
├── .github/
@@ -41,7 +40,15 @@ src-link/
4140
│ ├── plugin-main.cpp # Plugin entry point (module load/unload)
4241
│ ├── api-client.hpp / .cpp # SRCLinkApiClient: central orchestrator, OAuth, REST API, WebSocket
4342
│ ├── api-websocket.hpp / .cpp # SRCLinkWebSocketClient: control API WebSocket client
44-
│ ├── request-invoker.hpp / .cpp # HTTP request sequencing (OAuth2 via in-tree src/net/ stack)
43+
│ ├── net/
44+
│ │ ├── http-request-invoker.hpp / .cpp # OAuth2-aware HTTP request invoker
45+
│ │ ├── http-response.hpp # HTTP response struct
46+
│ │ ├── http-error.hpp / .cpp # HTTP error mapping helpers
47+
│ │ ├── curl-http-client.hpp / .cpp # libcurl-based HTTP client
48+
│ │ ├── ws-client.hpp / .cpp # libcurl-based WebSocket client
49+
│ │ ├── oauth2-client.hpp / .cpp # OAuth2 authorization-code flow client
50+
│ │ ├── local-http-server.hpp / .cpp # OAuth2 redirect-uri loopback server
51+
│ │ └── linux-ca-locations.hpp / .cpp # Linux CA bundle path discovery
4552
│ ├── settings.hpp / .cpp # SRCLinkSettingsStore: persistent settings (OBS profile INI)
4653
│ ├── schema.hpp # JSON data schemas (Account, Party, Stage, Uplink, Downlink, etc.)
4754
│ ├── utils.hpp / .cpp # Utility functions, encoder helpers, network helpers

CMakeLists.txt

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,8 @@ function(_src_link_setup_curl)
155155
if(NOT TARGET mbedtls
156156
OR NOT TARGET mbedx509
157157
OR NOT TARGET mbedcrypto)
158-
message(FATAL_ERROR "mbedtls FetchContent targets missing — _src_link_setup_mbedtls() must run first")
158+
message(FATAL_ERROR "SRC-Link does not support this host platform. Only Linux, macOS, and Windows are supported. \
159+
mbedtls FetchContent targets are missing because _src_link_setup_mbedtls() runs only on Linux.")
159160
endif()
160161
# FIXME: passing target names as strings relies on CMake ≥ 3.22 (CMP0125 NEW) so regular variables shadow
161162
# find_library cache lookups and prevent system mbedtls fallback. Migrate to find_package(MbedTLS CONFIG) once
@@ -236,6 +237,9 @@ if(OS_LINUX)
236237
set(SRC_LINK_LINUX_CA_PATH
237238
"/etc/ssl/certs"
238239
CACHE STRING "CA bundle path for Linux TLS (directory → CURLOPT_CAPATH, file → CURLOPT_CAINFO)")
240+
if(SRC_LINK_LINUX_CA_PATH MATCHES "[\"\\\\]")
241+
message(FATAL_ERROR "SRC_LINK_LINUX_CA_PATH must not contain '\"' or '\\\\' characters: ${SRC_LINK_LINUX_CA_PATH}")
242+
endif()
239243
target_compile_definitions(${CMAKE_PROJECT_NAME} PRIVATE SRC_LINK_LINUX_CA_PATH="${SRC_LINK_LINUX_CA_PATH}")
240244
endif()
241245

build-aux/.run-format.zsh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ invoke_formatter() {
6363
log_error "cmake-format is not version 0.6.13 or above (found ${cmake_format_version})."
6464
exit 2
6565
fi
66+
67+
if is-at-least 0.6.14 ${cmake_format_version}; then
68+
log_error "cmake-format must be exactly 0.6.13 (CI pinned). Got ${cmake_format_version}."
69+
exit 2
70+
fi
6671
} else {
6772
log_error "No viable cmake-format version found (required 0.6.13)"
6873
exit 2

src/api-client.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ with this program. If not, see <https://www.gnu.org/licenses/>
2323
#include <climits>
2424

2525
#include <QMessageBox>
26-
#include <QRandomGenerator>
2726
#include <QDesktopServices>
2827
#include <QString>
2928
#include <QJsonDocument>
@@ -152,7 +151,7 @@ SRCLinkApiClient::SRCLinkApiClient(QObject *parent)
152151
oauth2Client->setRefreshTokenUrl(TOKEN_URL);
153152
oauth2Client->setClientId(CLIENT_ID);
154153
oauth2Client->setClientSecret(CLIENT_SECRET);
155-
oauth2Client->setLocalPort(QRandomGenerator::system()->bounded(8000, 9000));
154+
oauth2Client->setLocalPort(0);
156155
oauth2Client->setScope(QString::fromLatin1(SCOPE));
157156
oauth2Client->setStore(settings);
158157

@@ -220,7 +219,7 @@ SRCLinkApiClient::SRCLinkApiClient(QObject *parent)
220219
tokenRefreshTimer->setSingleShot(true);
221220
connect(tokenRefreshTimer, &QTimer::timeout, this, [this]() { refresh(); });
222221

223-
if (oauth2Client->expires() - 60 <= QDateTime().currentSecsSinceEpoch()) {
222+
if (oauth2Client->expires() - 60 <= QDateTime::currentSecsSinceEpoch()) {
224223
// Refresh token now
225224
refresh();
226225
} else {
@@ -330,6 +329,10 @@ void SRCLinkApiClient::terminate()
330329
API_LOG("Terminating API client.");
331330
terminating = true;
332331
uplink = QJsonObject();
332+
// FIXME: deleteUplink(true) here is fire-and-forget — the in-flight DELETE is
333+
// canceled by ~CurlHttpClient on plugin teardown, leaving the server uplink
334+
// entry until its TTL. Replace with a bounded QEventLoop pump (or a sync
335+
// curl_easy_perform call) in a separate PR.
333336
deleteUplink(true);
334337
}
335338

src/api-websocket.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ SRCLinkWebSocketClient::~SRCLinkWebSocketClient()
9191

9292
void SRCLinkWebSocketClient::onConnected()
9393
{
94+
reconnectCount = 0;
95+
reconnectTimer->stop();
96+
reconnectPending = false;
9497
API_LOG("WebSocket connected");
9598
emit connected();
9699
}

src/net/curl-http-client.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@ with this program. If not, see <https://www.gnu.org/licenses/>
2222
#include "linux-ca-locations.hpp"
2323
#include "../plugin-support.h"
2424

25+
namespace {
26+
bool containsCrlfOrNul(const QByteArray &data)
27+
{
28+
return data.contains('\r') || data.contains('\n') || data.contains('\0');
29+
}
30+
} // namespace
31+
2532
CurlHttpClient::CurlHttpClient(QObject *parent) : QObject(parent), multi(curl_multi_init()), pollTimer(new QTimer(this))
2633
{
2734
obs_log(LOG_DEBUG, "CurlHttpClient created");
@@ -65,6 +72,17 @@ CURL *CurlHttpClient::createEasyHandle(
6572

6673
struct curl_slist *headerList = nullptr;
6774
for (auto it = headers.constBegin(); it != headers.constEnd(); ++it) {
75+
if (containsCrlfOrNul(it.key()) || containsCrlfOrNul(it.value())) {
76+
obs_log(
77+
LOG_ERROR, "CurlHttpClient: Refusing to send request with CR/LF/NUL in header (key=%s)",
78+
it.key().constData()
79+
);
80+
if (headerList) {
81+
curl_slist_free_all(headerList);
82+
}
83+
curl_easy_cleanup(easy);
84+
return nullptr;
85+
}
6886
QByteArray headerLine = it.key() + ": " + it.value();
6987
struct curl_slist *next = curl_slist_append(headerList, headerLine.constData());
7088
if (!next) {
@@ -81,6 +99,8 @@ CURL *CurlHttpClient::createEasyHandle(
8199
curl_easy_setopt(easy, CURLOPT_SSL_VERIFYPEER, 1L);
82100
curl_easy_setopt(easy, CURLOPT_SSL_VERIFYHOST, 2L);
83101
curl_easy_setopt(easy, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_TLSv1_3);
102+
curl_easy_setopt(easy, CURLOPT_PROTOCOLS_STR, "http,https");
103+
curl_easy_setopt(easy, CURLOPT_REDIR_PROTOCOLS_STR, "http,https");
84104
// Do not follow redirects: libcurl would replay the caller-supplied Authorization
85105
// header on the redirect target, leaking the OAuth2 Bearer token to a different host.
86106
// SRC-Link API endpoints are not expected to return 30x; any future endpoint that
@@ -338,6 +358,7 @@ size_t CurlHttpClient::headerCallback(char *ptr, size_t size, size_t nmemb, void
338358
{
339359
auto *ctx = static_cast<RequestContext *>(userdata);
340360
if (nmemb && size > SIZE_MAX / nmemb) {
361+
ctx->responseTooLarge = true;
341362
return 0;
342363
}
343364
size_t totalSize = size * nmemb;

src/net/curl-http-client.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@ class CurlHttpClient : public QObject {
9090
void cancelAllSilently();
9191

9292
private:
93+
// FIXME: Wrap RequestContext in std::unique_ptr and release ownership at
94+
// startRequest() to remove the duplicated `delete ctx` cleanup paths in the
95+
// 5 verb methods. Defer to a separate refactor PR.
9396
struct RequestContext {
9497
CURL *easy = nullptr;
9598
QByteArray responseData;

src/net/http-request-invoker.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,10 @@ void HttpRequestInvoker::handleResponse(
197197
} else {
198198
// Refresh failed: clear the stored tokens so the user is forced
199199
// back through the OAuth2 authorization flow on the next request.
200-
self->sequencer->oauth2Client->unlink();
201200
QMutexLocker locker(&self->sequencer->mutex);
202201
self->sequencer->requestQueue.removeOne(self);
203202
locker.unlock();
203+
self->sequencer->oauth2Client->unlink();
204204
emit self->finished(HttpError::AuthenticationRequired, data);
205205
self->deleteLater();
206206
}

src/net/local-http-server.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,14 @@ bool LocalHttpServer::listen(int port)
9494
if (setsockopt(
9595
serverSocket, SOL_SOCKET, SO_EXCLUSIVEADDRUSE, reinterpret_cast<const char *>(&optval), sizeof(optval)
9696
) != 0) {
97-
obs_log(LOG_WARNING, "LocalHttpServer: setsockopt(SO_EXCLUSIVEADDRUSE) failed with error %d", WSAGetLastError());
97+
obs_log(
98+
LOG_ERROR,
99+
"LocalHttpServer: setsockopt(SO_EXCLUSIVEADDRUSE) failed (WSAGetLastError=%d) — refusing to listen for OAuth2 callback on shared port",
100+
WSAGetLastError()
101+
);
102+
closeSocket(serverSocket);
103+
serverSocket = INVALID_SOCKET_HANDLE;
104+
return false;
98105
}
99106
#else
100107
// Allow address reuse for quick restart after close

src/net/ws-client.cpp

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -96,14 +96,23 @@ void WsClient::open(const QUrl &url)
9696
}
9797
// Some servers require an Origin header for the upgrade handshake.
9898
if (!requestHeaders.contains("Origin")) {
99-
QString scheme;
100-
if (url.scheme().compare("wss", Qt::CaseInsensitive) == 0) {
101-
scheme = "https";
99+
const QString urlScheme = url.scheme().toLower();
100+
QString originScheme;
101+
if (urlScheme == "wss") {
102+
originScheme = "https";
102103
} else {
103-
scheme = "http";
104+
originScheme = "http";
104105
}
105-
QByteArray origin = QString("%1://%2").arg(scheme, url.host()).toUtf8();
106-
slist = curl_slist_append(slist, QByteArray("Origin: " + origin).constData());
106+
// RFC 6454 §6.1: omit the port when it matches the scheme default.
107+
const int port = url.port();
108+
QString origin;
109+
if ((port == -1) || (port == 80 && (urlScheme == "ws" || urlScheme == "http")) ||
110+
(port == 443 && (urlScheme == "wss" || urlScheme == "https"))) {
111+
origin = QString("%1://%2").arg(originScheme, url.host());
112+
} else {
113+
origin = QString("%1://%2:%3").arg(originScheme, url.host()).arg(port);
114+
}
115+
slist = curl_slist_append(slist, QByteArray("Origin: " + origin.toUtf8()).constData());
107116
}
108117
headerList = slist;
109118

@@ -129,7 +138,7 @@ void WsClient::performConnect()
129138
curl_easy_setopt(easy, CURLOPT_SSL_VERIFYPEER, 1L);
130139
curl_easy_setopt(easy, CURLOPT_SSL_VERIFYHOST, 2L);
131140
curl_easy_setopt(easy, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_TLSv1_3);
132-
curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT, 5L);
141+
curl_easy_setopt(easy, CURLOPT_CONNECTTIMEOUT_MS, 3000L);
133142
curl_easy_setopt(easy, CURLOPT_NOSIGNAL, 1L);
134143
// TCP keepalive for dead peer detection (NAT timeout, cable unplug)
135144
curl_easy_setopt(easy, CURLOPT_TCP_KEEPALIVE, 1L);
@@ -170,6 +179,9 @@ void WsClient::performConnect()
170179
QPointer<WsClient> guard(this);
171180
connectThread = std::thread([guard, easyLocal, gen]() {
172181
CURLcode res = curl_easy_perform(easyLocal);
182+
// QPointer contract: guard.data() is the receiver, but its dereference happens
183+
// lazily inside the queued lambda on the UI thread, after dtor's joinConnectThread()
184+
// ensures the worker has finished.
173185
QMetaObject::invokeMethod(
174186
guard.data(),
175187
[guard, res, gen]() {
@@ -275,6 +287,8 @@ void WsClient::cleanup()
275287
// Detach via deleteLater() so a cleanup() invoked synchronously from inside pollRecv()
276288
// (the recvNotifier's own slot) does not destroy the QObject whose signal stack is active.
277289
recvNotifier->setEnabled(false);
290+
// Order: notifier disconnect/deleteLater must precede curl_easy_cleanup so the
291+
// socket fd is not torn out from under an active slot.
278292
recvNotifier->disconnect();
279293
recvNotifier->deleteLater();
280294
recvNotifier = nullptr;
@@ -349,6 +363,7 @@ qint64 WsClient::sendRaw(const char *data, size_t len, unsigned int flags, bool
349363

350364
size_t totalSent = 0;
351365
int selectRetries = 0;
366+
// UI thread only — no `easy` reset can race the loop iteration.
352367
while (totalSent < len) {
353368
size_t sent = 0;
354369
CURLcode res = curl_ws_send(easy, data + totalSent, len - totalSent, &sent, 0, flags);
@@ -401,12 +416,13 @@ void WsClient::ping()
401416
return;
402417
}
403418

404-
pingTimestamp = QDateTime::currentMSecsSinceEpoch();
405419
size_t sent = 0;
406420
CURLcode res = curl_ws_send(easy, "", 0, &sent, 0, CURLWS_PING);
407421
// CURLE_AGAIN here just means the socket buffer is briefly full; treat it as best-effort.
408422
// Any other error indicates the connection is gone — handle the same way as a recv failure.
409-
if (res != CURLE_OK && res != CURLE_AGAIN) {
423+
if (res == CURLE_OK) {
424+
pingTimestamp = QDateTime::currentMSecsSinceEpoch();
425+
} else if (res != CURLE_AGAIN) {
410426
obs_log(LOG_DEBUG, "WsClient ping send failed: %s", curl_easy_strerror(res));
411427
// Mark disconnected synchronously so subsequent ping ticks short-circuit via isValid().
412428
connected = false;
@@ -463,13 +479,24 @@ void WsClient::pollRecv()
463479
obs_log(LOG_DEBUG, "WsClient close echo failed: %s", curl_easy_strerror(echoRes));
464480
}
465481
connected = false;
482+
if (!fragmentBuffer.isEmpty()) {
483+
obs_log(
484+
LOG_INFO, "Discarding partial fragmented message (%lld bytes, type=%d) on close",
485+
static_cast<long long>(fragmentBuffer.size()), static_cast<int>(fragmentType)
486+
);
487+
}
466488
cleanup();
467489
emit closed();
468490
return;
469491
}
470492

471493
if (meta->flags & CURLWS_PONG) {
472-
qint64 elapsed = QDateTime::currentMSecsSinceEpoch() - pingTimestamp;
494+
if (pingTimestamp == 0) {
495+
// Unsolicited PONG (RFC 6455 §5.5.3) — skip RTT report.
496+
continue;
497+
}
498+
const qint64 elapsed = QDateTime::currentMSecsSinceEpoch() - pingTimestamp;
499+
pingTimestamp = 0; // reset after consuming
473500
emit pongReceived(static_cast<quint64>(elapsed >= 0 ? elapsed : 0));
474501
continue;
475502
}

0 commit comments

Comments
 (0)