refactor: Add ENABLE_OBS_WEBSOCKET option for vendor request integration

hanatyan128 · hanatyan128 · commit ed982fac4466 · 2026-05-05T01:58:33.000+09:00
Submodule presence is checked only when the option is ON, with FATAL_ERROR
on absence. Includes accumulated input validation, lifetime annotation, and
Security documentation refinements.
diff --git a/.gitignore b/.gitignore
@@ -30,6 +30,8 @@
 !screenshot2.jpg
 !build.ps1
 !/lib
+/lib/*
+!/lib/obs-websocket
 !.gitmodules
 !README_ja.md
 !API.md
diff --git a/API.md b/API.md
diff --git a/API_ja.md b/API_ja.md
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -11,6 +11,15 @@ include(compilerconfig)
 include(defaults)
 include(helpers)
 
+option(ENABLE_OBS_WEBSOCKET "Enable obs-websocket vendor request integration" ON)
+
+if(ENABLE_OBS_WEBSOCKET AND NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/lib/obs-websocket/lib/obs-websocket-api.h")
+  message(
+    FATAL_ERROR
+      "ENABLE_OBS_WEBSOCKET is ON but lib/obs-websocket submodule is missing. Run: git submodule update --init --recursive (or pass -DENABLE_OBS_WEBSOCKET=OFF to disable obs-websocket vendor request integration)"
+  )
+endif()
+
 add_library(${CMAKE_PROJECT_NAME} MODULE)
 
 find_package(libobs REQUIRED)
@@ -34,19 +43,18 @@ if(ENABLE_QT)
                AUTORCC ON)
 endif()
 
-if(NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/lib/obs-websocket/lib/obs-websocket-api.h")
-  message(FATAL_ERROR "lib/obs-websocket submodule not found. Run: git submodule update --init --recursive")
+if(ENABLE_OBS_WEBSOCKET)
+  target_include_directories(${CMAKE_PROJECT_NAME} SYSTEM PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/lib/obs-websocket/lib)
+  target_compile_definitions(${CMAKE_PROJECT_NAME} PRIVATE ENABLE_OBS_WEBSOCKET)
+  target_sources(${CMAKE_PROJECT_NAME} PRIVATE src/plugin-websocket.cpp)
 endif()
 
-target_include_directories(${CMAKE_PROJECT_NAME} SYSTEM PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/lib/obs-websocket/lib)
-
 target_sources(
   ${CMAKE_PROJECT_NAME}
   PRIVATE src/plugin-main.cpp
           src/plugin-streaming.cpp
           src/plugin-stream-recording.cpp
           src/plugin-replay-buffer.cpp
-          src/plugin-websocket.cpp
           src/plugin-ui.cpp
           src/utils.cpp
           src/audio/audio-capture.cpp
diff --git a/README.md b/README.md
@@ -136,3 +136,17 @@ git submodule update --init --recursive
 
 If you cloned with `--recurse-submodules`, this step is unnecessary.
 
+The submodule is pinned to a specific release tag of obs-websocket. Do **not** run
+`git submodule update --remote`, which would move the submodule to the upstream default branch
+and silently introduce unintended changes. To bump the pinned version, fetch the submodule's
+upstream, check out the desired release tag, then stage and commit the new pointer:
+
+```bash
+cd lib/obs-websocket
+git fetch --tags origin
+git checkout <new-tag>
+cd ../..
+git add lib/obs-websocket
+git commit -m "Bump obs-websocket submodule to <new-tag>"
+```
+
diff --git a/README_ja.md b/README_ja.md
@@ -137,3 +137,17 @@ git submodule update --init --recursive
 
 `--recurse-submodules` 付きでクローンした場合は、この手順は不要です。
 
+このサブモジュールは obs-websocket の特定のリリースタグに固定されています。`git submodule update --remote`
+は実行しないでください。アップストリームのデフォルトブランチへサブモジュールを移動させ、意図しない変更が
+混入します。固定バージョンを更新する場合は、サブモジュールのアップストリームを fetch し、目的のリリースタグを
+チェックアウトしたうえで、新しいポインタをステージしてコミットしてください。
+
+```bash
+cd lib/obs-websocket
+git fetch --tags origin
+git checkout <new-tag>
+cd ../..
+git add lib/obs-websocket
+git commit -m "Bump obs-websocket submodule to <new-tag>"
+```
+
diff --git a/build-aux/.run-format.zsh b/build-aux/.run-format.zsh
@@ -70,7 +70,7 @@ invoke_formatter() {
 
       local -a source_files=(**/(CMakeLists.txt|*.cmake)(.N))
       source_files=(${source_files:#(build_*)/*})
-      # Exclude vendored submodules under lib/ from cmake-format (nested paths included)
+      # Exclude vendored submodules: drop every file at any depth under lib/
       source_files=(${source_files:#lib/**/*})
 
       local -a format_args=()
diff --git a/build.ps1 b/build.ps1
@@ -2,21 +2,31 @@ Param (
     [switch]$installer
 )
 
-$BuildSpec = Get-Content -Path ./buildspec.json -Raw | ConvertFrom-Json
-$ProductName = $BuildSpec.name
-$ProductVersion = $BuildSpec.version
+try {
+    Push-Location $PSScriptRoot
 
-$OutputName = "${ProductName}-${ProductVersion}-windows-x64"
+    $BuildSpec = Get-Content -Path ./buildspec.json -Raw | ConvertFrom-Json
+    $ProductName = $BuildSpec.name
+    $ProductVersion = $BuildSpec.version
 
-if (-not (Test-Path "lib/obs-websocket/lib/obs-websocket-api.h")) {
-    git submodule update --init --recursive lib/obs-websocket
-    if (-not $?) { exit 1 }
-}
+    $OutputName = "${ProductName}-${ProductVersion}-windows-x64"
 
-cmake --fresh -S . -B build_x64 -Wdev -Wdeprecated -DCMAKE_SYSTEM_VERSION="10.0.18363.657" -G "Visual Studio 17 2022" -A x64
-cmake --build build_x64 --config RelWithDebInfo --target ALL_BUILD --
-cmake --install build_x64 --prefix release/Package --config RelWithDebInfo
+    if (-not (Test-Path "lib/obs-websocket/lib/obs-websocket-api.h")) {
+        git submodule update --init --recursive lib/obs-websocket
+        if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+    }
 
-if ($installer) {
-    iscc build_x64/installer-Windows.generated.iss /O"release" /F"${OutputName}-Installer-signed"
+    cmake --fresh -S . -B build_x64 -Wdev -Wdeprecated -DCMAKE_SYSTEM_VERSION="10.0.18363.657" -G "Visual Studio 17 2022" -A x64
+    if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+    cmake --build build_x64 --config RelWithDebInfo --target ALL_BUILD --
+    if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+    cmake --install build_x64 --prefix release/Package --config RelWithDebInfo
+    if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+
+    if ($installer) {
+        iscc build_x64/installer-Windows.generated.iss /O"release" /F"${OutputName}-Installer-signed"
+        if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+    }
+} finally {
+    Pop-Location
 }
diff --git a/cmake/linux/defaults.cmake b/cmake/linux/defaults.cmake
@@ -34,9 +34,9 @@ set(CPACK_SOURCE_IGNORE_FILES
     \\.git/
     \\.github/
     \\.gitignore
-    \\.gitmodules
     build_.*
     cmake/\\.CMakeBuildNumber
+    lib/
     release/)
 
 set(CPACK_VERBATIM_VARIABLES YES)
diff --git a/src/plugin-main.cpp b/src/plugin-main.cpp
@@ -34,7 +34,9 @@ with this program. If not, see <https://www.gnu.org/licenses/>
 #include "video/filter-video-capture.hpp"
 #include "plugin-support.h"
 #include "plugin-main.hpp"
+#ifdef ENABLE_OBS_WEBSOCKET
 #include "plugin-websocket.hpp"
+#endif
 #include "utils.hpp"
 
 #define SETTINGS_JSON_NAME "recently.json"
@@ -146,9 +148,12 @@ BranchOutputFilter::BranchOutputFilter(obs_data_t *settings, obs_source_t *sourc
     // FIXME: libobs has no proc_handler_remove(). If it gains one, pair
     // unregistration with ~BranchOutputFilter().
     proc_handler_t *ph = obs_source_get_proc_handler(filterSource);
-    auto replayBufferProcDecl =
+    // static: proc_handler_add() does not document whether it copies the decl string,
+    // so the buffer must outlive every registered handler.
+    static const std::string replayBufferProcDecl =
         std::string("void ") + PROC_OVERRIDE_REPLAY_BUFFER_FILENAME_FORMAT + "(in string format)";
-    auto recordingProcDecl = std::string("void ") + PROC_OVERRIDE_RECORDING_FILENAME_FORMAT + "(in string format)";
+    static const std::string recordingProcDecl =
+        std::string("void ") + PROC_OVERRIDE_RECORDING_FILENAME_FORMAT + "(in string format)";
     proc_handler_add(ph, replayBufferProcDecl.c_str(), onOverrideReplayBufferFilenameFormat, this);
     proc_handler_add(ph, recordingProcDecl.c_str(), onOverrideRecordingFilenameFormat, this);
 
@@ -1929,26 +1934,27 @@ void obs_module_post_load()
     proc_handler_t *ph = obs_get_proc_handler();
     proc_handler_add(ph, "void osi_branch_output_get_filter_list(out string json)", onGetFilterList, nullptr);
 
+#ifdef ENABLE_OBS_WEBSOCKET
     registerWebSocketVendorRequests();
+#endif
 }
 
 void obs_module_unload()
 {
-    // Publish nullptr before unregistering vendor requests so that any
-    // vendor callback already in flight observes a null dock and bails out
-    // of the UI dispatch path. Doing this first shrinks (but does not
-    // close) the worker race window.
+    // Stop accepting new vendor dispatches before tearing down the dock so
+    // a worker cannot start a fresh UI invocation against a destroyed dock.
     //
-    // FIXME: narrow worker race remains. Proper fixes are an in-flight
-    // counter (wait for zero before remove_dock) or QPointer + deleteLater
-    // + processEvents drain. Current code accepts the race because the
-    // worst observable effect is a silent empty result.
+    // FIXME: narrow worker race remains for a callback that already loaded
+    // a non-null dock pointer. Proper fixes are an in-flight counter or
+    // QPointer + deleteLater + processEvents drain.
+#ifdef ENABLE_OBS_WEBSOCKET
+    unregisterWebSocketVendorRequests();
+#endif
+
     if (statusDock.exchange(nullptr) != nullptr) {
         obs_frontend_remove_dock("BranchOutputStatusDock");
     }
 
-    unregisterWebSocketVendorRequests();
-
     pthread_mutex_destroy(&pluginMutex);
 
     obs_log(LOG_INFO, "Plugin unloaded");
diff --git a/src/plugin-websocket.cpp b/src/plugin-websocket.cpp
@@ -24,25 +24,45 @@ with this program. If not, see <https://www.gnu.org/licenses/>
 #include <atomic>
 #include <cstring>
 #include <string>
+#include <type_traits>
 
 #include "plugin-main.hpp"
 #include "plugin-support.h"
 #include "plugin-websocket.hpp"
 
 #define VENDOR_NAME "osi_branch_output"
 
+// Vendor request type names. Shared between register, unregister, and any
+// future dispatch sites so a rename cannot silently desynchronize them.
+// Distinct from PROC_OVERRIDE_* constants, which name proc handler entries.
+static constexpr char WS_REQUEST_GET_FILTER_LIST[] = "get_filter_list";
+static constexpr char WS_REQUEST_OVERRIDE_RECORDING_FILENAME_FORMAT[] = "override_recording_filename_format";
+static constexpr char WS_REQUEST_OVERRIDE_REPLAY_BUFFER_FILENAME_FORMAT[] = "override_replay_buffer_filename_format";
+
 // DoS guard: recording filename templates are typically <100 chars.
 // Reject pathologically long inputs before they reach libobs's path expander.
 static constexpr size_t MAX_FORMAT_LENGTH = 1024;
 
 // Reject formats that would let an authenticated obs-websocket client write
 // outside the configured recording directory.
+// Assumes os_generate_formatted_filename() expansion tokens (%CCYY, %hh, etc.)
+// do not introduce '/', '\\', or ".." sequences.
 static bool isPathTraversalFormat(const char *format, size_t length)
 {
     if (length == 0) {
         return false;
     }
 
+    // Leading ASCII control chars (< 0x20) or space can be silently stripped by
+    // downstream path handling, letting " /etc/x" bypass the absolute-path check
+    // below; rejecting the full class also narrows log injection surface.
+    {
+        unsigned char c = static_cast<unsigned char>(format[0]);
+        if (c < 0x20 || c == ' ') {
+            return true;
+        }
+    }
+
     // Absolute path on POSIX or UNC/rooted path on Windows.
     if (format[0] == '/' || format[0] == '\\') {
         return true;
@@ -78,9 +98,12 @@ static bool isPathTraversalFormat(const char *format, size_t length)
 // Validate canonical 8-4-4-4-12 lowercase-hex UUID with hyphens at fixed positions.
 // Matches the form produced by obs_source_get_uuid(), which is what
 // obs_get_source_by_uuid() compares against byte-for-byte.
-// Caller must guarantee the buffer is at least 36 bytes long.
+// Returns false for null. For non-null pointers, the buffer must be at least 36 bytes long.
 static bool isValidUuidString(const char *uuid)
 {
+    if (!uuid) {
+        return false;
+    }
     constexpr size_t UUID_LENGTH = 36;
     for (size_t i = 0; i < UUID_LENGTH; i++) {
         char c = uuid[i];
@@ -101,6 +124,14 @@ static bool isValidUuidString(const char *uuid)
 // these from a non-main thread; the early-return in registerWebSocketVendorRequests()
 // reads `vendor` without any external lock.
 static std::atomic<obs_websocket_vendor> vendor{nullptr};
+static_assert(
+    std::is_pointer_v<obs_websocket_vendor>,
+    "obs_websocket_vendor must remain a pointer typedef for nullptr / atomic semantics to be sound"
+);
+static_assert(
+    decltype(vendor)::is_always_lock_free,
+    "obs_websocket_vendor (void*) must be lock-free for register/unregister synchronization"
+);
 
 static void onVendorGetFilterList(obs_data_t *, obs_data_t *response, void *)
 {
@@ -154,25 +185,17 @@ static void overrideFilenameFormat(obs_data_t *request, obs_data_t *response, co
     constexpr size_t UUID_LENGTH = 36;
     if (strnlen(filterUuid, UUID_LENGTH + 1) != UUID_LENGTH) {
         obs_data_set_bool(response, "success", false);
-        obs_data_set_string(response, "error", "filter_uuid must be a UUID string");
+        obs_data_set_string(response, "error", "filter_uuid must be a lowercase canonical UUID string");
         return;
     }
 
-    // obs_get_source_by_uuid() does a case-sensitive compare against
-    // the canonical lowercase form, so normalize before validating.
-    std::string normalizedUuid(filterUuid, UUID_LENGTH);
-    for (char &c : normalizedUuid) {
-        if (c >= 'A' && c <= 'Z') {
-            c = static_cast<char>(c - 'A' + 'a');
-        }
-    }
-    if (!isValidUuidString(normalizedUuid.c_str())) {
+    if (!isValidUuidString(filterUuid)) {
         obs_data_set_bool(response, "success", false);
-        obs_data_set_string(response, "error", "filter_uuid must be a UUID string");
+        obs_data_set_string(response, "error", "filter_uuid must be a lowercase canonical UUID string");
         return;
     }
 
-    OBSSourceAutoRelease source = obs_get_source_by_uuid(normalizedUuid.c_str());
+    OBSSourceAutoRelease source = obs_get_source_by_uuid(filterUuid);
     const char *sourceId = source ? obs_source_get_id(source) : nullptr;
     if (!sourceId || strcmp(sourceId, FILTER_ID) != 0 || obs_source_get_type(source) != OBS_SOURCE_TYPE_FILTER ||
         obs_source_removed(source)) {
@@ -208,6 +231,7 @@ static void overrideFilenameFormat(obs_data_t *request, obs_data_t *response, co
 
     obs_data_set_bool(response, "success", ok);
     if (!ok) {
+        obs_log(LOG_WARNING, "Vendor request: proc handler '%s' unavailable on filter %s", procName, filterUuid);
         obs_data_set_string(response, "error", "Internal error: filter proc handler unavailable");
     }
 }
@@ -238,19 +262,19 @@ void registerWebSocketVendorRequests()
         return;
     }
 
-    if (!obs_websocket_vendor_register_request(v, "get_filter_list", onVendorGetFilterList, nullptr)) {
+    if (!obs_websocket_vendor_register_request(v, WS_REQUEST_GET_FILTER_LIST, onVendorGetFilterList, nullptr)) {
         obs_log(LOG_ERROR, "Failed to register vendor request 'get_filter_list'; clients will get UnknownRequestType");
     }
     if (!obs_websocket_vendor_register_request(
-            v, "override_recording_filename_format", onVendorOverrideRecordingFilenameFormat, nullptr
+            v, WS_REQUEST_OVERRIDE_RECORDING_FILENAME_FORMAT, onVendorOverrideRecordingFilenameFormat, nullptr
         )) {
         obs_log(
             LOG_ERROR,
             "Failed to register vendor request 'override_recording_filename_format'; clients will get UnknownRequestType"
         );
     }
     if (!obs_websocket_vendor_register_request(
-            v, "override_replay_buffer_filename_format", onVendorOverrideReplayBufferFilenameFormat, nullptr
+            v, WS_REQUEST_OVERRIDE_REPLAY_BUFFER_FILENAME_FORMAT, onVendorOverrideReplayBufferFilenameFormat, nullptr
         )) {
         obs_log(
             LOG_ERROR,
@@ -276,13 +300,13 @@ void unregisterWebSocketVendorRequests()
     // FIXME: obs_websocket_vendor_unregister_request() does not block until
     // in-flight callbacks return. The same worker race as the statusDock
     // FIXME in obs_module_unload() applies to vendor callbacks too.
-    if (!obs_websocket_vendor_unregister_request(v, "get_filter_list")) {
+    if (!obs_websocket_vendor_unregister_request(v, WS_REQUEST_GET_FILTER_LIST)) {
         obs_log(LOG_DEBUG, "Failed to unregister vendor request: get_filter_list");
     }
-    if (!obs_websocket_vendor_unregister_request(v, "override_recording_filename_format")) {
+    if (!obs_websocket_vendor_unregister_request(v, WS_REQUEST_OVERRIDE_RECORDING_FILENAME_FORMAT)) {
         obs_log(LOG_DEBUG, "Failed to unregister vendor request: override_recording_filename_format");
     }
-    if (!obs_websocket_vendor_unregister_request(v, "override_replay_buffer_filename_format")) {
+    if (!obs_websocket_vendor_unregister_request(v, WS_REQUEST_OVERRIDE_REPLAY_BUFFER_FILENAME_FORMAT)) {
         obs_log(LOG_DEBUG, "Failed to unregister vendor request: override_replay_buffer_filename_format");
     }
     vendor.store(nullptr, std::memory_order_release);
