Fix review findings: stopping-flag race, unpinned SDK download, key-leak via redirect/logs
Code review findings from before merging feat/livekit-integration to main: - I1: sourceDestroy set self->stopping outside self->mutex, then notified. The worker's condition-variable predicate reads `stopping` under that same mutex, so the store+notify could land between the worker's predicate check and it entering the wait, dropping the notification and leaving the worker asleep for its full backoff (up to 30s) with the OBS UI thread blocked in worker.join(). Now set under the lock, matching how `generation` is already mutated in applySettings. - I3: the LiveKit SDK archive download in cmake/LiveKitSDK.cmake had no SHA256 pin wired up from the top-level CMakeLists.txt, unlike the obs-deps bootstrap right next to it. Added real SHA256 hashes -- computed by downloading each release archive and running sha256sum -- for every triple the pinned v1.10.1 release can resolve to (Linux x64/arm64, macOS x64/arm64, Windows x64), keyed by version+triple so a future version bump fails loudly (via message(WARNING)) instead of silently going unverified. Verified end-to-end locally: a deliberately wrong hash makes the configure step fail with a HASH mismatch error. Only Linux was also build-tested in this environment; macOS/Windows archives were downloaded and hashed but not build-tested here. - I4: the curl HTTP backend followed up to 3 redirects while the read key travels as a URL query parameter, so a malicious/misconfigured redirect (including an HTTPS->HTTP downgrade, which curl doesn't refuse by default) could leak the key. This client only ever talks to two fixed, first-party endpoints, so redirects are disabled outright (CURLOPT_FOLLOWLOCATION 0), matching the WinHTTP backend's existing default behavior. Left normalizeServerUrl's explicit-http:// pass-through as-is with a comment, per review guidance. - I5: ApiClient::redactedUrl was tested but never called. No current call site logs a request URL, so rather than inventing one, added a one-line comment marking it a deliberate guard rail for future logging. - I7: the LiveKit SDK log bridge (livekitLogToObs) wrote SDK messages straight into the OBS log. LiveKit's signaling URL carries the access token as a query parameter; defensively scrub "access_token=" and "key=" values before they ever reach obs_log. New ApiClient::redactSensitiveParams generalizes redactedUrl's redaction pattern to arbitrary text (not just a bare URL), with 6 new unit tests in test_api_client.cpp. - I2: added a code comment on session.cpp's auto_subscribe=true noting the known, unaddressed bandwidth/CPU cost of pulling every participant's track in multi-camera rooms, and that per-publication unsubscribe is a future optimization. No behavior change (out of scope per review). Verified: cmake configure + build + `ctest --test-dir build --output-on-failure` all pass, 6/6 suites (test_api_client now 127 checks, up from 121). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RL8abRmgFXkVASHkkqiJbE
This commit is contained in:
@@ -410,7 +410,17 @@ void sourceDestroy(void *data)
|
||||
if (!self)
|
||||
return;
|
||||
|
||||
self->stopping.store(true);
|
||||
{
|
||||
// Must be set while holding `mutex`, matching how `generation` is
|
||||
// mutated in applySettings: the worker's wait predicate reads
|
||||
// `stopping` under this same lock, so setting it outside the lock
|
||||
// can race between the worker's predicate check and it entering
|
||||
// the wait, dropping the notify_all() below and leaving the worker
|
||||
// asleep for its full backoff (up to kBackoffMaxMs) while this
|
||||
// (OBS UI) thread blocks in worker.join().
|
||||
std::lock_guard<std::mutex> guard(self->mutex);
|
||||
self->stopping.store(true);
|
||||
}
|
||||
self->wake.notify_all();
|
||||
if (self->worker.joinable())
|
||||
self->worker.join();
|
||||
@@ -563,7 +573,14 @@ void livekitLogToObs(livekit::LogLevel level, const std::string &, const std::st
|
||||
case livekit::LogLevel::Info: obs_level = LOG_INFO; break;
|
||||
default: obs_level = LOG_DEBUG; break;
|
||||
}
|
||||
obs_log(obs_level, "livekit: %s", message.c_str());
|
||||
// LiveKit's signaling connection URL carries the access token as a
|
||||
// query parameter. This is defensive, not a response to a confirmed
|
||||
// leak: if the SDK ever logs that URL (or anything else carrying
|
||||
// "access_token=" or "key="), the token must not land verbatim in an
|
||||
// OBS log file that a director might paste into a support ticket. Scrub
|
||||
// unconditionally before this message ever reaches obs_log.
|
||||
const std::string scrubbed = ApiClient::redactSensitiveParams(message);
|
||||
obs_log(obs_level, "livekit: %s", scrubbed.c_str());
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
||||
Reference in New Issue
Block a user