From dc7499cfed3407cf79783d9ce0e9dfe3bda24ecd Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Wed, 14 Oct 2026 09:44:31 -0400 Subject: [PATCH 1/4] Add an opt-in plaintext message trace for conformance tooling Adds SendspinClientConfig::message_trace behind SENDSPIN_ENABLE_MESSAGE_TRACE, a host-only CMake option that is off by default and has no Kconfig entry. The callback receives every protocol message's plaintext on the protocol task: inbound after decryption and reassembly, outbound before encryption, and the cleartext handshake. It mirrors sendspin-server-cpp's ServerConfig::trace, so the conformance harness can observe sendspin-cpp's messages and audio chunks. --- .claude/skills/house-patterns/SKILL.md | 7 +- .github/workflows/ci.yml | 7 +- CLAUDE.md | 4 +- CMakeLists.txt | 10 ++ cmake/sources.cmake | 7 + docs/conventions.md | 11 +- docs/integration-guide.md | 12 ++ docs/internals.md | 2 +- include/sendspin/config.h | 40 +++++ src/connection.cpp | 19 ++ src/connection.h | 18 ++ src/connection_manager.cpp | 6 + src/message_trace.cpp | 119 +++++++++++++ src/message_trace.h | 60 +++++++ src/noise_transport.cpp | 5 + src/noise_transport.h | 17 ++ tests/CMakeLists.txt | 5 + tests/test_message_trace.cpp | 235 +++++++++++++++++++++++++ 18 files changed, 575 insertions(+), 9 deletions(-) create mode 100644 src/message_trace.cpp create mode 100644 src/message_trace.h create mode 100644 tests/test_message_trace.cpp diff --git a/.claude/skills/house-patterns/SKILL.md b/.claude/skills/house-patterns/SKILL.md index 7188430c..dc455111 100644 --- a/.claude/skills/house-patterns/SKILL.md +++ b/.claude/skills/house-patterns/SKILL.md @@ -93,7 +93,12 @@ the descriptions in `docs/internals.md`: the codec gate `SENDSPIN_ENABLE_OPUS` only in `src/decoder.h`, `src/decoder.cpp`, `src/player_role.cpp`, `src/source_role.cpp`, `src/source_task.cpp`, and - the source's Opus encoder list in `cmake/sources.cmake`. Examples must + the source's Opus encoder list in `cmake/sources.cmake`; the host-only + message trace gate `SENDSPIN_ENABLE_MESSAGE_TRACE` only in + `include/sendspin/config.h`, `src/connection.h`, `src/connection.cpp`, + `src/connection_manager.cpp`, `src/noise_transport.h`, + `src/noise_transport.cpp`, and its own source list in `cmake/sources.cmake` + (no Kconfig entry). Examples must build with roles (and Opus) disabled: role and opus usage in `examples/` is guarded like an external consumer would guard it. Verify this by grepping the example for the role's types diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1af441be..31b723c1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,8 +104,9 @@ jobs: - name: Configure CMake # BUILD_EXAMPLES=OFF so the test job configures only sendspin + tests and skips the - # example-only FetchContent deps (e.g. FTXUI). - run: cmake -B build-tests -DENABLE_WERROR=ON -DSENDSPIN_BUILD_TESTS=ON -DENABLE_SANITIZERS=ON -DBUILD_EXAMPLES=OFF . + # example-only FetchContent deps (e.g. FTXUI). The message trace is on here and in the TSan + # job; the build and no-opus jobs build it off, as consumers get it. + run: cmake -B build-tests -DENABLE_WERROR=ON -DSENDSPIN_BUILD_TESTS=ON -DENABLE_SANITIZERS=ON -DSENDSPIN_ENABLE_MESSAGE_TRACE=ON -DBUILD_EXAMPLES=OFF . - name: Build tests run: | @@ -190,7 +191,7 @@ jobs: - name: Configure CMake # ENABLE_TSAN instead of ENABLE_SANITIZERS: TSan cannot be combined with ASan/UBSan, and # the concurrency tests only report a data race when built this way. - run: cmake -B build-tsan -DENABLE_WERROR=ON -DSENDSPIN_BUILD_TESTS=ON -DENABLE_TSAN=ON -DBUILD_EXAMPLES=OFF . + run: cmake -B build-tsan -DENABLE_WERROR=ON -DSENDSPIN_BUILD_TESTS=ON -DENABLE_TSAN=ON -DSENDSPIN_ENABLE_MESSAGE_TRACE=ON -DBUILD_EXAMPLES=OFF . - name: Build tests run: | diff --git a/CLAUDE.md b/CLAUDE.md index ab5ce84f..01898b6d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,6 +30,7 @@ The library provides `SendspinClient` as the main public API. It handles the ful - `RecordStore` (`record_store.h`): pairing records and the Pairing PSK (configured, stored, or generated), seeded from the client config and the persistence provider - `Inbox` / `InboxSlot` / `GenerationSlot` (`inbox.h`): single-mutex mailbox for all main-loop-bound cross-thread state - atomic topic bitmask polled lock-free by `loop()`, plus a fixed event ring for ordered lifecycle events - `RoleTeardown` / `TeardownTracker` (`teardown_tracker.h`): every role's teardown generation and the main-loop half of a role's teardown +- `MessageTracer` (`message_trace.h`): one connection's handle on the opt-in plaintext message trace (`SendspinClientConfig::message_trace`), built only with `SENDSPIN_ENABLE_MESSAGE_TRACE` - `SendspinTimeFilter` (`time_filter.h`): 2D Kalman filter for NTP-style time sync - `SendspinTimeBurst` (`time_burst.h`): burst-based time message coordinator, one per connection - `SendspinDecoder` (`decoder.h`): FLAC/PCM decoder wrapper, plus Opus when built with `SENDSPIN_ENABLE_OPUS` @@ -38,7 +39,7 @@ The library provides `SendspinClient` as the main public API. It handles the ful Roles are added to the client at runtime via `add_player()`, `add_metadata()`, etc. Each role receives a `SendspinClient*` at construction time and uses it to access shared services (time sync, state publishing, message sending). The consumer provides behavior by implementing listener interfaces (`PlayerRoleListener`, `MetadataRoleListener`, etc.) and setting them via `set_listener()`. Required callbacks are pure virtual; optional callbacks have default no-op implementations. The client dispatches messages to roles via null-pointer checks on role pointers. -Roles can be disabled at compile time via `SENDSPIN_ENABLE_*` cmake options (host build) or Kconfig entries (ESP-IDF build). When a role is disabled, its source files are not compiled and its `add_*()` declaration, accessor, and `unique_ptr` member are removed from `client.h`. Role `#ifdef` guards live in exactly two places in the library: `cmake/sources.cmake` (source lists) and `include/sendspin/client.h`, `src/client.cpp` and `src/client_dispatch.cpp` (dispatch points); the codec gate `SENDSPIN_ENABLE_OPUS` (cmake option / Kconfig entry, default on) lives only in `src/decoder.h`, `src/decoder.cpp`, `src/player_role.cpp`, `src/source_role.cpp`, and `src/source_task.cpp`, plus the `SENDSPIN_SOURCE_OPUS_SOURCES` list (`src/source_encoder_opus.cpp`) in `cmake/sources.cmake`. Examples and tests guard their own role and opus usage like any consumer. micro-flac is only linked when the player role is enabled; micro-opus only with `SENDSPIN_ENABLE_OPUS` and the player or source role enabled. +Roles can be disabled at compile time via `SENDSPIN_ENABLE_*` cmake options (host build) or Kconfig entries (ESP-IDF build). When a role is disabled, its source files are not compiled and its `add_*()` declaration, accessor, and `unique_ptr` member are removed from `client.h`. Role `#ifdef` guards live in exactly two places in the library: `cmake/sources.cmake` (source lists) and `include/sendspin/client.h`, `src/client.cpp` and `src/client_dispatch.cpp` (dispatch points); the codec gate `SENDSPIN_ENABLE_OPUS` (cmake option / Kconfig entry, default on) lives only in `src/decoder.h`, `src/decoder.cpp`, `src/player_role.cpp`, `src/source_role.cpp`, and `src/source_task.cpp`, plus the `SENDSPIN_SOURCE_OPUS_SOURCES` list (`src/source_encoder_opus.cpp`) in `cmake/sources.cmake`. The host-only message trace gate `SENDSPIN_ENABLE_MESSAGE_TRACE` (cmake option, default off, no Kconfig entry) lives only in `include/sendspin/config.h`, `src/connection.h`, `src/connection.cpp`, `src/connection_manager.cpp`, `src/noise_transport.h`, `src/noise_transport.cpp`, and the `SENDSPIN_MESSAGE_TRACE_SOURCES` list (`src/message_trace.cpp`). Examples and tests guard their own role and opus usage like any consumer. micro-flac is only linked when the player role is enabled; micro-opus only with `SENDSPIN_ENABLE_OPUS` and the player or source role enabled. The consuming platform (e.g., ESPHome) supplies the listener implementations plus `SendspinNetworkProvider` and the optional `SendspinPersistenceProvider`/`SendspinClientListener` providers; `docs/integration-guide.md` has the full wiring, including a minimal working example. @@ -95,6 +96,7 @@ Core source files in `src/` have no `#ifdef ESP_PLATFORM` guards; all platform d - **Host (CMake)**: `cmake -B build && cmake --build build`. Uses ccache when installed (`SENDSPIN_USE_CCACHE`, default on when standalone). Fetches dependencies via FetchContent: ArduinoJson, noise-c, IXWebSocket, micro-flac for the player role, and micro-opus for the player or source role when `SENDSPIN_ENABLE_OPUS` is on. - **Tests**: `cmake -B build-tests -DSENDSPIN_BUILD_TESTS=ON -DENABLE_SANITIZERS=ON -DBUILD_EXAMPLES=OFF .`, then `cmake --build build-tests --target sendspin_tests` and `ctest --test-dir build-tests --output-on-failure`. - **ThreadSanitizer tests**: `cmake -B build-tsan -DSENDSPIN_BUILD_TESTS=ON -DENABLE_TSAN=ON -DBUILD_EXAMPLES=OFF .`, then `cmake --build build-tsan --target sendspin_tests` and `TSAN_OPTIONS=halt_on_error=1 ctest --test-dir build-tsan --output-on-failure`. `ENABLE_TSAN` applies the thread sanitizer to every target, including the fetched dependencies, and cannot be combined with `ENABLE_SANITIZERS`. +- **Message trace**: `-DSENDSPIN_ENABLE_MESSAGE_TRACE=ON` compiles in `SendspinClientConfig::message_trace` and `tests/test_message_trace.cpp`; CI's test and TSan jobs build it on, the build and no-opus jobs off. - **Stack budgets**: the ESP task stack defaults in `config.h` are static call-graph bounds from `tools/stack_usage/` (recipe and per-task figures in its README); re-derive them, and update the README table with them, after a change that deepens a task's call chain. - **ESP dependencies**: ArduinoJson, noise-c, esp_websocket_client, micro-flac, micro-opus (with `SENDSPIN_ENABLE_OPUS`), esp_http_server, mbedtls, pthread, esp_ringbuf, esp_hw_support - **Host dependencies**: ArduinoJson, noise-c, micro-flac, micro-opus (with `SENDSPIN_ENABLE_OPUS`), IXWebSocket, pthreads diff --git a/CMakeLists.txt b/CMakeLists.txt index ae5dcaf8..d214d1b9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -161,6 +161,10 @@ else() # patent-encumbered; see the player and source role specs. option(SENDSPIN_ENABLE_OPUS "Enable Opus in the player and source roles" ON) + # Plaintext trace of every protocol message (SendspinClientConfig::message_trace), for + # conformance tooling. Host only: the ESP-IDF build has no switch for it. + option(SENDSPIN_ENABLE_MESSAGE_TRACE "Enable the plaintext protocol message trace" OFF) + include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/host.cmake) # Get sources @@ -193,6 +197,9 @@ else() list(APPEND SENDSPIN_ALL_SOURCES ${SENDSPIN_SOURCE_OPUS_SOURCES}) endif() endif() + if(SENDSPIN_ENABLE_MESSAGE_TRACE) + list(APPEND SENDSPIN_ALL_SOURCES ${SENDSPIN_MESSAGE_TRACE_SOURCES}) + endif() # Create the library: core sources, enabled role sources, host networking (via host.cmake) add_library(sendspin STATIC ${SENDSPIN_ALL_SOURCES}) @@ -222,6 +229,9 @@ else() if(SENDSPIN_ENABLE_SOURCE) target_compile_definitions(sendspin PUBLIC SENDSPIN_ENABLE_SOURCE) endif() + if(SENDSPIN_ENABLE_MESSAGE_TRACE) + target_compile_definitions(sendspin PUBLIC SENDSPIN_ENABLE_MESSAGE_TRACE) + endif() # Host build options option(ENABLE_WERROR "Treat warnings as errors" OFF) diff --git a/cmake/sources.cmake b/cmake/sources.cmake index 003af2dc..e0f317f0 100644 --- a/cmake/sources.cmake +++ b/cmake/sources.cmake @@ -107,6 +107,13 @@ function(sendspin_get_sources BASE_DIR) PARENT_SCOPE ) + # The plaintext message trace, appended on host when SENDSPIN_ENABLE_MESSAGE_TRACE is on + set(SENDSPIN_MESSAGE_TRACE_SOURCES + ${BASE_DIR}/src/message_trace.cpp + + PARENT_SCOPE + ) + # ESP-IDF only sources: networking layer deeply coupled to ESP-IDF APIs set(SENDSPIN_ESP_SOURCES ${BASE_DIR}/src/esp/server_connection.cpp diff --git a/docs/conventions.md b/docs/conventions.md index 035e6677..974ee675 100644 --- a/docs/conventions.md +++ b/docs/conventions.md @@ -148,9 +148,14 @@ checklists in `.claude/skills/` apply these standards to a diff. and the codec gate `SENDSPIN_ENABLE_OPUS` only in `src/decoder.h`, `src/decoder.cpp`, `src/player_role.cpp`, `src/source_role.cpp`, `src/source_task.cpp`, and - the source's Opus encoder list in `cmake/sources.cmake`; consumers, - including the examples, guard their own role and codec usage (see Public - API). + the source's Opus encoder list in `cmake/sources.cmake`. The host-only + message trace gate `SENDSPIN_ENABLE_MESSAGE_TRACE` lives only in + `include/sendspin/config.h`, `src/connection.h`, `src/connection.cpp`, + `src/connection_manager.cpp`, `src/noise_transport.h`, + `src/noise_transport.cpp`, and its own source list in + `cmake/sources.cmake` (`src/message_trace.cpp`); it has no Kconfig entry. + Consumers, including the examples, guard their own role and codec usage + (see Public API). - Logging uses the `SS_LOG*` macros; allocation uses the `platform_malloc` family with an explicit `MemoryLocation` choice where it matters. - Code that only builds on one platform still keeps the other platform's build diff --git a/docs/integration-guide.md b/docs/integration-guide.md index 45cb0b76..d352e8dc 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -1185,6 +1185,7 @@ Most listener callbacks fire on the main loop thread (the thread calling `client | `ArtworkRoleListener::on_image_decode()` | Dedicated artwork decode thread | | `ArtworkRoleListener::on_image_display()` | Main loop thread | | `VisualizerRoleListener` data callbacks (`on_loudness()`, `on_beat()`, `on_f_peak()`, `on_spectrum()`, `on_peak()`) | Dedicated visualizer drain thread | +| `SendspinClientConfig::message_trace` (built with `SENDSPIN_ENABLE_MESSAGE_TRACE`) | Protocol task | | All other listener methods | Main loop thread | `PlayerRole::notify_audio_played()` is thread-safe and is designed to be called from an audio output callback thread. @@ -1310,6 +1311,16 @@ Available options (all `ON` by default): When `SENDSPIN_ENABLE_PLAYER` is `OFF`, micro-flac is not fetched, and micro-opus is fetched only for the source role. `SENDSPIN_ENABLE_OPUS=OFF` drops micro-opus alone, for products that cannot ship Opus (see the patent note in the player and source role specs); the player then refuses `OPUS` entries in `audio_formats`, and the source refuses an `OPUS` codec in its config. +### Message trace (host only) + +`SENDSPIN_ENABLE_MESSAGE_TRACE` (`OFF` by default) adds `SendspinClientConfig::message_trace`, a callback that receives every protocol message's plaintext (inbound after decryption, outbound before encryption, plus the cleartext text frames), for conformance tooling and interop debugging. The ESP-IDF build has no switch for it. The callback runs on the protocol task and must not block. The pairing secrets a client sends read `""`; `SendspinTracedMessage` in `config.h` documents the fields. + +```cpp +config.message_trace = [](const SendspinTracedMessage& message) { + // message.plaintext is valid only during this call: copy what you keep +}; +``` + ### ESP-IDF (Kconfig) Role flags are exposed via Kconfig under `Component config → sendspin-cpp`: @@ -1375,6 +1386,7 @@ X25519 keypair and read back via `client.client_id()` after `start()`. | `pairing_psk_locations` | `std::vector` | `{}` | Where the operator can find the pairing token the device shipped with: any of `"device"`, `"leaflet"`, `"operator"`. Advertised as the informational `locations` hint on the `pairing_psk` descriptor in `client/hello`; empty omits the hint, see [The locations hint](#the-locations-hint). | | `static_pairing_code_locations` | `std::vector` | `{}` | Where the operator can find the static pairing code the device shipped with, same values as above. Advertised on the `static_pairing_code` descriptor in `client/hello`; empty omits the hint. | | `json_arena_size` | `size_t` | `2048` | Size in bytes of a fixed internal-RAM scratch buffer that backs every JSON document the protocol task works with: the parse of each incoming protocol message and every message it builds, instead of PSRAM. Costs this many bytes of internal RAM permanently but removes PSRAM traffic from the protocol task on every message. The arena holds one document at a time: a parsed message is released before the reply it triggers (the `client/state` after a `server/activate`, a pairing reply, a re-handshake's second message) is built. The default covers one steady-state protocol message, including the FLAC stream-start header; a document larger than the budget on its own, such as a large track-metadata message, falls back to PSRAM (those arrive only once per song). That holds on a 32-bit target, where ArduinoJson allocates each variant pool as one 1,024-byte block. Every freed block is wiped, so a document that held key material leaves none behind. Set to `0` to send every document to PSRAM. On host there is no PSRAM distinction, so the arena is just a fixed scratch buffer, and since a variant pool alone is 4 KB there, the default fits no document's pool on host. | +| `message_trace` | `SendspinMessageTraceFn` | empty | Only in a build with `SENDSPIN_ENABLE_MESSAGE_TRACE` (host only): called with every protocol message's plaintext, on the protocol task. See [Message trace (host only)](#message-trace-host-only). | --- diff --git a/docs/internals.md b/docs/internals.md index a0962c91..2debc138 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -6,7 +6,7 @@ This document maps how the library's parts work together: which threads exist, h Each role class uses the pimpl pattern. The public header (`include/sendspin/_role.h`) exposes the protocol types, the listener interface, and a thin role class holding `std::unique_ptr impl_`; all private state and thread management live in `src/_role_impl.h` and the role's `.cpp`. `SendspinClient` is a `friend` of each role class so it can dispatch into `impl_`. Internal references below use the `Impl` qualification (e.g. `PlayerRole::Impl::drain_events()`). -Roles are disabled at build time through two cooperating mechanisms. `cmake/sources.cmake` keeps a source list per role, so a disabled role's translation units are never compiled and its dependencies are never required; `#ifdef SENDSPIN_ENABLE_` guards in `include/sendspin/client.h`, `src/client.cpp` and `src/client_dispatch.cpp` (the protocol task's tick and message dispatch) remove the members, accessors, and dispatch branches that must name the role's type. The split exists because CMake cannot gate individual declarations, and `#ifdef` around whole files would still put the codec headers on the include path. On ESP-IDF the codec dependencies are gated by `idf_component.yml`'s Kconfig rules alone, because ESP-IDF collects `REQUIRES` before `sdkconfig` is loaded. Opus is a third gate, inside the player and source roles: `SENDSPIN_ENABLE_OPUS` (a CMake option on host, on ESP-IDF a Kconfig entry that depends on the player or the source role, on by default) decides whether micro-opus is fetched, required and linked. The source's encoder (`src/source_encoder_opus.cpp`) is a source list of its own (`SENDSPIN_SOURCE_OPUS_SOURCES`). The decoder shares its translation unit with FLAC and PCM, so `src/decoder.h` and `src/decoder.cpp` guard its Opus include, state and branches, and `src/player_role.cpp` derives one constexpr from the macro to refuse opus formats; `src/source_role.cpp` likewise derives one to refuse an `OPUS` `SourceRoleConfig`, and `src/source_task.cpp` guards the encoder's construction. No other file under `src/` references the macro. +Roles are disabled at build time through two cooperating mechanisms. `cmake/sources.cmake` keeps a source list per role, so a disabled role's translation units are never compiled and its dependencies are never required; `#ifdef SENDSPIN_ENABLE_` guards in `include/sendspin/client.h`, `src/client.cpp` and `src/client_dispatch.cpp` (the protocol task's tick and message dispatch) remove the members, accessors, and dispatch branches that must name the role's type. The split exists because CMake cannot gate individual declarations, and `#ifdef` around whole files would still put the codec headers on the include path. On ESP-IDF the codec dependencies are gated by `idf_component.yml`'s Kconfig rules alone, because ESP-IDF collects `REQUIRES` before `sdkconfig` is loaded. Opus is a third gate, inside the player and source roles: `SENDSPIN_ENABLE_OPUS` (a CMake option on host, on ESP-IDF a Kconfig entry that depends on the player or the source role, on by default) decides whether micro-opus is fetched, required and linked. The source's encoder (`src/source_encoder_opus.cpp`) is a source list of its own (`SENDSPIN_SOURCE_OPUS_SOURCES`). The decoder shares its translation unit with FLAC and PCM, so `src/decoder.h` and `src/decoder.cpp` guard its Opus include, state and branches, and `src/player_role.cpp` derives one constexpr from the macro to refuse opus formats; `src/source_role.cpp` likewise derives one to refuse an `OPUS` `SourceRoleConfig`, and `src/source_task.cpp` guards the encoder's construction. No other file under `src/` references the macro. The plaintext message trace (`SendspinClientConfig::message_trace`) is a fourth gate, `SENDSPIN_ENABLE_MESSAGE_TRACE`: a host-only CMake option, off by default, with no Kconfig entry. Each connection owns a `MessageTracer` (`src/message_trace.h`) that the protocol task calls when a cleartext text frame is received or sent, when a transport message is decrypted and reassembled, and as each frame is encrypted (`NoiseTransport::encrypt_in_place()`, which every encrypted send goes through). Adding a role therefore means: a `SENDSPIN__SOURCES` list in `cmake/sources.cmake`, the guarded member, accessor, and dispatch branches in `client.h`, `client.cpp` and `client_dispatch.cpp`, and any heavy dependency kept behind the role's private headers. A role-private header such as `src/decoder.h`, which pulls in the codec headers, may only be reached from the role's own sources or a guarded include in `client.cpp` or `client_dispatch.cpp`; public role headers stay codec-free because core files such as `src/protocol_messages.h` include them unconditionally. diff --git a/include/sendspin/config.h b/include/sendspin/config.h index 65656f54..69fc5e4b 100644 --- a/include/sendspin/config.h +++ b/include/sendspin/config.h @@ -22,6 +22,9 @@ #include #include #include +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE +#include +#endif #include #include #include @@ -114,6 +117,38 @@ struct SendspinPsk { } }; +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE +/// @brief Which way a traced message crossed the connection +enum class SendspinTraceDirection : uint8_t { + INBOUND, // Received from the server + OUTBOUND, // Sent to the server +}; + +/// @brief One protocol message as the message trace sees it (SendspinClientConfig::message_trace) +struct SendspinTracedMessage { + /// Unique per connection while the process runs + uint64_t connection_id{0}; + SendspinTraceDirection direction{SendspinTraceDirection::INBOUND}; + /// False for a cleartext text frame (the handshake, or a message sent before the transport is + /// up); true for a transport message, type byte first. Outbound fragments are traced one by + /// one; inbound messages once reassembled. + bool binary{false}; + /// Inbound: the receive stamp; outbound: just before encryption or the cleartext send. On the + /// clock SendspinClient::get_client_time() converts to. + int64_t stamp_us{0}; + /// Valid only during the call. The pairing secrets (`long_term_psk`, `wrapped_psk`, + /// `wrapped_nonce_B`) read "". + const uint8_t* plaintext{nullptr}; + size_t plaintext_len{0}; + /// Inbound player audio chunk: its bytes of encoded audio; 0 for any other message + size_t audio_bytes{0}; +}; + +/// @brief Called on the protocol task for every message on every connection; must not block. +/// Each connection holds its own copy. +using SendspinMessageTraceFn = std::function; +#endif + // The implicit copy and move members touch the deprecated pairing_window_supported; only a // consumer's own use of it should warn. #pragma GCC diagnostic push @@ -321,6 +356,11 @@ struct SendspinClientConfig { /// Set to 0 to send every document to PSRAM. On host there is no PSRAM distinction and the /// arena is a plain scratch buffer. Used by the protocol task only. size_t json_arena_size{DEFAULT_JSON_ARENA_SIZE}; + +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + /// @brief Opt-in plaintext trace of every protocol message; empty for none + SendspinMessageTraceFn message_trace; +#endif }; #pragma GCC diagnostic pop diff --git a/src/connection.cpp b/src/connection.cpp index 02c75ebe..b14f2db6 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -110,6 +110,9 @@ SsErr SendspinConnection::send_app_json(const char* json, size_t len) { return this->send_encrypted_text(json, len); } // Pre-handshake cold path: the text-frame API takes a std::string. +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + this->message_tracer_.trace_outbound_text(std::string_view(json, len)); +#endif return this->send_text_message(std::string(json, len)); } @@ -216,6 +219,9 @@ void SendspinConnection::send_noise_client_init() { } std::string client_init = this->noise_handshake_->build_client_init(); if (!client_init.empty()) { +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + this->message_tracer_.trace_outbound_text(client_init); +#endif this->send_text_message(client_init); } } @@ -226,6 +232,9 @@ void SendspinConnection::handle_noise_handshake_text(std::string_view text) { } auto send_fn = [this](const std::string& msg) -> bool { +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + this->message_tracer_.trace_outbound_text(msg); +#endif auto err = this->send_text_message(msg); return err == SsErr::OK; }; @@ -387,6 +396,11 @@ SS_HOT SendspinConnection::InboundDispatch SendspinConnection::process_inbound_m const bool noise_pending = !noise_active && this->noise_handshake_; if (message.kind == InboundKind::TEXT) { +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + this->message_tracer_.trace_inbound( + false, widen_time_stamp_us(message.receive_time_us, platform_time_us()), message.data, + message.len); +#endif if (noise_pending) { // Feed the handshake driver; it handles server/init and noise/handshake frames. this->handle_noise_handshake_text( @@ -458,6 +472,11 @@ SS_HOT SendspinConnection::InboundDispatch SendspinConnection::process_inbound_m complete.receive_time_us = message.receive_time_us; complete.kind = InboundKind::BINARY; } +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + this->message_tracer_.trace_inbound( + true, widen_time_stamp_us(complete.receive_time_us, platform_time_us()), complete.data, + complete.len); +#endif return classify_complete_noise_message(complete); } diff --git a/src/connection.h b/src/connection.h index c69d3b80..645882bd 100644 --- a/src/connection.h +++ b/src/connection.h @@ -21,6 +21,9 @@ #include "crypto/cpace.h" #include "crypto/keys.h" #include "inbound_ring.h" +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE +#include "message_trace.h" +#endif #include "noise_handshake.h" #include "noise_transport.h" #include "platform/crypto.h" @@ -842,6 +845,15 @@ class SendspinConnection : public std::enable_shared_from_thisjson_arena_ = &arena; } +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + /// @brief Installs the client's message trace for this connection + /// @note Must be called before the connection reaches the protocol task, like set_json_arena(). + void set_message_trace(const SendspinMessageTraceFn& trace) { + this->message_tracer_.configure(trace, this->instance_id); + this->noise_transport_.set_message_tracer(&this->message_tracer_); + } +#endif + protected: // ======================================== // Transport frames @@ -1025,6 +1037,12 @@ class SendspinConnection : public std::enable_shared_from_thisset_inbound_buffer_location(this->client_->config_.inbound_ring_location); conn->set_noise_buffer_location(this->client_->config_.noise_buffer_location); conn->set_json_arena(this->json_arena()); +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + conn->set_message_trace(this->client_->config_.message_trace); +#endif conn->attach_inbound(this->client_->inbound_ring_.get(), this->client_->protocol_task_.get()); ProtocolCommand command; @@ -429,6 +432,9 @@ void ConnectionManager::connect_to(const std::string& url) { client_conn->set_inbound_buffer_location(this->client_->config_.inbound_ring_location); client_conn->set_noise_buffer_location(this->client_->config_.noise_buffer_location); client_conn->set_json_arena(this->json_arena()); +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + client_conn->set_message_trace(this->client_->config_.message_trace); +#endif // Attached before start(): the transport may deliver, or report its upgrade, from its own // thread as soon as it runs. diff --git a/src/message_trace.cpp b/src/message_trace.cpp new file mode 100644 index 00000000..1cc5235c --- /dev/null +++ b/src/message_trace.cpp @@ -0,0 +1,119 @@ +// Copyright 2026 Sendspin Contributors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "message_trace.h" + +#include "crypto/constants.h" +#include "inbound_ring.h" +#include "platform/time.h" +#include "protocol_messages.h" + +#include +#include +#include + +namespace sendspin { + +namespace { + +/// The pairing secrets a client sends (pairing.md "Wrapping"), each as its key and the opening +/// quote of its value, as ArduinoJson serializes them: with no whitespace. +constexpr std::string_view SECRET_FIELDS[] = {"\"long_term_psk\":\"", "\"wrapped_psk\":\"", + "\"wrapped_nonce_B\":\""}; + +constexpr std::string_view REDACTED = ""; + +/// @brief Copies `text` with the value of each secret field replaced by REDACTED, never copying a +/// secret byte +/// @return The redacted copy, or nullopt when `text` holds no secret field. +std::optional redact_secrets(std::string_view text) { + std::string out; + size_t copied = 0; + bool found = false; + while (true) { + size_t field = std::string_view::npos; + size_t field_len = 0; + for (const std::string_view key : SECRET_FIELDS) { + const size_t at = text.find(key, copied); + if (at < field) { + field = at; + field_len = key.size(); + } + } + if (field == std::string_view::npos) { + break; + } + found = true; + const size_t value_start = field + field_len; + out.append(text.substr(copied, value_start - copied)); + out.append(REDACTED); + // A value with no closing quote runs to the end, so nothing after its key is copied. + const size_t value_end = text.find('"', value_start); + copied = (value_end == std::string_view::npos) ? text.size() : value_end; + } + if (!found) { + return std::nullopt; + } + out.append(text.substr(copied)); + return out; +} + +} // namespace + +void MessageTracer::trace_inbound(bool binary, int64_t receive_time_us, const uint8_t* plaintext, + size_t len) const { + if (!this->trace_) { + return; + } + SendspinTracedMessage message; + message.connection_id = this->connection_id_; + message.direction = SendspinTraceDirection::INBOUND; + message.binary = binary; + message.stamp_us = receive_time_us; + message.plaintext = plaintext; + message.plaintext_len = len; + // The player hands its decoder the bytes after the chunk header (PlayerRole::Impl:: + // handle_binary(), whose AUDIO_FRAME_OFFSET is asserted equal to this header size). + if (binary && len >= INBOUND_AUDIO_CHUNK_HEADER_BYTES && + plaintext[0] == SENDSPIN_BINARY_PLAYER_AUDIO) { + message.audio_bytes = len - INBOUND_AUDIO_CHUNK_HEADER_BYTES; + } + this->trace_(message); +} + +void MessageTracer::emit_outbound(bool binary, const uint8_t* plaintext, size_t len) const { + if (!this->trace_) { + return; + } + SendspinTracedMessage message; + message.connection_id = this->connection_id_; + message.direction = SendspinTraceDirection::OUTBOUND; + message.binary = binary; + message.stamp_us = platform_time_us(); + message.plaintext = plaintext; + message.plaintext_len = len; + // Only JSON carries a secret. The messages that do are far below MAX_TRANSPORT_PLAINTEXT, so + // never fragmented. + std::optional redacted; + if (!binary || (len > 0 && plaintext[0] == MSG_TYPE_JSON_BODY)) { + redacted = redact_secrets(std::string_view(reinterpret_cast(plaintext), len)); + } + if (redacted.has_value()) { + message.plaintext = reinterpret_cast(redacted->data()); + message.plaintext_len = redacted->size(); + } + this->trace_(message); +} + +} // namespace sendspin diff --git a/src/message_trace.h b/src/message_trace.h new file mode 100644 index 00000000..40a7aa62 --- /dev/null +++ b/src/message_trace.h @@ -0,0 +1,60 @@ +// Copyright 2026 Sendspin Contributors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// @file message_trace.h +/// @brief Compiled only with SENDSPIN_ENABLE_MESSAGE_TRACE + +#pragma once + +#include "sendspin/config.h" + +#include +#include +#include + +namespace sendspin { + +/// @brief Builds each SendspinTracedMessage for one connection and hands it to the client's +/// trace (SendspinClientConfig::message_trace). Protocol task only. +class MessageTracer { +public: + /// @brief Installs the client's trace for one connection; an empty trace leaves tracing off + void configure(const SendspinMessageTraceFn& trace, uint64_t connection_id) { + this->trace_ = trace; + this->connection_id_ = connection_id; + } + + /// @brief Traces one complete received message; `receive_time_us` is on platform_time_us() + void trace_inbound(bool binary, int64_t receive_time_us, const uint8_t* plaintext, + size_t len) const; + + /// @brief Traces one frame about to be encrypted, type byte first + void trace_outbound(const uint8_t* plaintext, size_t len) const { + this->emit_outbound(true, plaintext, len); + } + + /// @brief Traces one cleartext text frame about to be sent + void trace_outbound_text(std::string_view text) const { + this->emit_outbound(false, reinterpret_cast(text.data()), text.size()); + } + +private: + /// @brief Hands the trace one outbound message, stamped now, with its secrets redacted + void emit_outbound(bool binary, const uint8_t* plaintext, size_t len) const; + + SendspinMessageTraceFn trace_; + uint64_t connection_id_{0}; +}; + +} // namespace sendspin diff --git a/src/noise_transport.cpp b/src/noise_transport.cpp index 68420384..b6108ec3 100644 --- a/src/noise_transport.cpp +++ b/src/noise_transport.cpp @@ -51,6 +51,11 @@ size_t NoiseTransport::encrypt_in_place(uint8_t* buf, size_t buf_capacity, size_ SS_LOGE(TAG, "encrypt_in_place: buffer lacks AEAD tag room"); return 0; } +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + if (this->message_tracer_ != nullptr) { + this->message_tracer_->trace_outbound(buf, plaintext_len); + } +#endif // noise-c advances the send nonce past a failed encrypt too. const size_t ct_len = this->session_->encrypt(buf, plaintext_len, buf_capacity); diff --git a/src/noise_transport.h b/src/noise_transport.h index 27865645..ed8f4dc0 100644 --- a/src/noise_transport.h +++ b/src/noise_transport.h @@ -28,6 +28,9 @@ #include "platform/memory.h" #include "platform/types.h" #include "sendspin/types.h" +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE +#include "message_trace.h" +#endif #include #include @@ -186,6 +189,14 @@ class NoiseTransport { this->buffer_location_ = location; } +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + /// @brief Sets the tracer each frame goes through before its encrypt. Call before any + /// transport traffic. + void set_message_tracer(const MessageTracer* tracer) { + this->message_tracer_ = tracer; + } +#endif + private: /// @brief Encrypts one frame in place under the current session. Call only with a session /// and the send not desynced, which every caller checks first. A failed encrypt sets @@ -275,6 +286,12 @@ class NoiseTransport { /// Protocol task only. std::unique_ptr session_; +#ifdef SENDSPIN_ENABLE_MESSAGE_TRACE + /// From set_message_tracer(); nullptr for none. Written once at setup, then protocol task + /// only. + const MessageTracer* message_tracer_{nullptr}; +#endif + // size_t fields /// Bytes used in reasm_buf_ (including the leading orig_type byte), 0 while the in-flight /// message is being discarded. Protocol task only. diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7a8fd8aa..f68da861 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -67,6 +67,11 @@ add_executable(sendspin_tests # chunk back, and nothing on the public surface reaches it without the sync thread. set_source_files_properties(test_decoder.cpp PROPERTIES COMPILE_OPTIONS -fno-access-control) +# Built only with the trace it tests. +if(SENDSPIN_ENABLE_MESSAGE_TRACE) + target_sources(sendspin_tests PRIVATE test_message_trace.cpp) +endif() + # test_client_lifecycle.cpp checks what a role's stop() left on its item list, which nothing on # the public surface reports, and plays the protocol task on the test thread to install a stand-in # connection in ConnectionManager's admitted slot, to stage an accept or a full command queue at diff --git a/tests/test_message_trace.cpp b/tests/test_message_trace.cpp new file mode 100644 index 00000000..02fc4d56 --- /dev/null +++ b/tests/test_message_trace.cpp @@ -0,0 +1,235 @@ +// Copyright 2026 Sendspin Contributors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// The plaintext message trace (SendspinClientConfig::message_trace). Built only with +// SENDSPIN_ENABLE_MESSAGE_TRACE. + +#include "lifecycle_test_fixtures.h" +#include "message_trace.h" +#include "platform/time.h" +#include "sendspin/client.h" +#include "sendspin/config.h" + +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace sendspin { +namespace { + +constexpr uint16_t TRACE_TEST_PORT = 19110; + +/// Encoded audio bytes in the chunk the fake server sends. +constexpr size_t AUDIO_PAYLOAD_BYTES = 1000; + +/// One traced message, copied out of the callback. +struct TraceRecord { + uint64_t connection_id{0}; + SendspinTraceDirection direction{SendspinTraceDirection::INBOUND}; + bool binary{false}; + int64_t stamp_us{0}; + std::vector plaintext; + size_t audio_bytes{0}; + + /// The JSON `type` of a cleartext message or a transport JSON body, or "binary " for any + /// other transport message. + std::string type() const { + const bool json = !this->binary || (!this->plaintext.empty() && this->plaintext[0] == 0); + if (!json) { + return "binary " + std::to_string(this->plaintext.empty() ? -1 : this->plaintext[0]); + } + const size_t offset = this->binary ? 1 : 0; + JsonDocument doc; + if (deserializeJson(doc, reinterpret_cast(this->plaintext.data()) + offset, + this->plaintext.size() - offset)) { + return "unparsable"; + } + return doc["type"].as(); + } +}; + +/// Collects every traced message. The trace runs on the protocol task; the test thread reads. +class TraceCollector { +public: + SendspinMessageTraceFn callback() { + return [this](const SendspinTracedMessage& message) { + TraceRecord record; + record.connection_id = message.connection_id; + record.direction = message.direction; + record.binary = message.binary; + record.stamp_us = message.stamp_us; + record.plaintext.assign(message.plaintext, message.plaintext + message.plaintext_len); + record.audio_bytes = message.audio_bytes; + std::lock_guard lock(this->mutex_); + this->records_.push_back(std::move(record)); + }; + } + + std::vector records() const { + std::lock_guard lock(this->mutex_); + return this->records_; + } + + bool has(SendspinTraceDirection direction, const std::string& type) const { + for (const TraceRecord& record : this->records()) { + if (record.direction == direction && record.type() == type) { + return true; + } + } + return false; + } + +private: + mutable std::mutex mutex_; + std::vector records_; +}; + +using Dir = SendspinTraceDirection; + +// A server-initiated connection read through the trace alone: the handshake in order, transport +// messages as plaintext, and an audio chunk's encoded audio bytes. +TEST(MessageTrace, RecordsTheHandshakeTheTransportMessagesAndAnAudioChunk) { + TraceCollector collector; + SendspinClientConfig config; + config.name = "Message Trace Test Client"; + config.server_port = TRACE_TEST_PORT; + config.message_trace = collector.callback(); + + const int64_t start_us = platform_time_us(); + PairedClientBundle bundle(std::move(config)); + SendspinClient& client = bundle.client(); + ASSERT_TRUE(bundle.start()); + + FakeEncryptedServerOptions options; + options.first_roles_json = R"([])"; + auto server = connect_paired_server(bundle.peer, TRACE_TEST_PORT, std::move(options)); + pump_until(client, [&] { return collector.has(Dir::INBOUND, "server/activate"); }); + + // Traced before dispatch, so the chunk shows although no player takes it. + ASSERT_TRUE(server->send_audio(platform_time_us(), AUDIO_PAYLOAD_BYTES)); + pump_until(client, [&] { return collector.has(Dir::INBOUND, "binary 4"); }); + // Snapshot first: the protocol task may trace more messages while this runs. + const std::vector records = collector.records(); + const int64_t end_us = platform_time_us(); + + // Each step answers the one before it, so the order is fixed (messaging.md "Communication"). + // Readable transport JSON shows it was traced before the encrypt and after the decrypt. + struct Expected { + Dir direction; + bool binary; + const char* type; + }; + const std::vector expected_prefix = { + {Dir::OUTBOUND, false, "client/init"}, {Dir::INBOUND, false, "server/init"}, + {Dir::INBOUND, false, "noise/handshake"}, {Dir::OUTBOUND, false, "noise/handshake"}, + {Dir::INBOUND, true, "server/hello"}, {Dir::OUTBOUND, true, "client/hello"}, + {Dir::INBOUND, true, "server/activate"}, + }; + ASSERT_GT(records.size(), expected_prefix.size()); + for (size_t i = 0; i < expected_prefix.size(); ++i) { + EXPECT_EQ(records[i].direction, expected_prefix[i].direction) << "entry " << i; + EXPECT_EQ(records[i].binary, expected_prefix[i].binary) << "entry " << i; + EXPECT_EQ(records[i].type(), expected_prefix[i].type) << "entry " << i; + } + + // The audio chunk: type byte, 8-byte timestamp, 4-byte send_ahead, then the audio. + size_t audio_chunks = 0; + for (const TraceRecord& record : records) { + if (record.type() == "binary 4") { + ++audio_chunks; + EXPECT_EQ(record.plaintext.size(), 1 + 8 + 4 + AUDIO_PAYLOAD_BYTES); + EXPECT_EQ(record.audio_bytes, AUDIO_PAYLOAD_BYTES); + } else { + EXPECT_EQ(record.audio_bytes, 0U) << record.type(); + } + } + EXPECT_EQ(audio_chunks, 1U); + + // One connection, and every stamp read from the clock while the test ran. + for (const TraceRecord& record : records) { + EXPECT_NE(record.connection_id, 0U); + EXPECT_EQ(record.connection_id, records.front().connection_id); + EXPECT_GE(record.stamp_us, start_us) << record.type(); + EXPECT_LE(record.stamp_us, end_us) << record.type(); + } + + client.disconnect(SendspinGoodbyeReason::SHUTDOWN); + pump_for(client, 100); +} + +// Each pairing secret reads "" and nothing else changes; a message with no secret, and +// a binary message, reach the trace as the very bytes sent. +TEST(MessageTrace, RedactsThePairingSecretsAndNothingElse) { + struct Row { + const char* name; + bool binary; + std::string sent; + std::string traced; // Empty: the sent bytes, unchanged and not copied. + }; + const std::string json_body(1, '\0'); + const std::string audio("\x04", 1); + const std::vector rows = { + {"cleartext long_term_psk", false, + R"({"type":"client/pair-finalize","payload":{"long_term_psk":"SECRETSECRET"}})", + R"({"type":"client/pair-finalize","payload":{"long_term_psk":""}})"}, + {"transport wrapped_psk", true, + json_body + R"({"type":"client/pair-finalize","payload":{"wrapped_psk":"SECRET"}})", + json_body + R"({"type":"client/pair-finalize","payload":{"wrapped_psk":""}})"}, + {"wrapped_nonce_B beside client_kc", true, + json_body + + R"({"type":"client/pair-confirm","payload":{"client_kc":"TAG","wrapped_nonce_B":"SECRET"}})", + json_body + + R"({"type":"client/pair-confirm","payload":{"client_kc":"TAG","wrapped_nonce_B":""}})"}, + {"unterminated value", true, json_body + R"({"payload":{"wrapped_psk":"SECR)", + json_body + R"({"payload":{"wrapped_psk":")"}, + {"no secret", true, json_body + R"({"type":"client/state","payload":{}})", ""}, + {"binary audio", true, audio + R"("wrapped_psk":"BYTES")", ""}, + }; + + for (const Row& row : rows) { + std::vector traced; + std::vector pointers; + MessageTracer tracer; + tracer.configure( + [&](const SendspinTracedMessage& message) { + traced.emplace_back(reinterpret_cast(message.plaintext), + message.plaintext_len); + pointers.push_back(message.plaintext); + }, + 1); + const auto* sent = reinterpret_cast(row.sent.data()); + if (row.binary) { + tracer.trace_outbound(sent, row.sent.size()); + } else { + tracer.trace_outbound_text(row.sent); + } + + ASSERT_EQ(traced.size(), 1U) << row.name; + if (row.traced.empty()) { + EXPECT_EQ(pointers[0], sent) << row.name; + } else { + EXPECT_EQ(traced[0], row.traced) << row.name; + EXPECT_EQ(traced[0].find("SECR"), std::string::npos) << row.name; + } + } +} + +} // namespace +} // namespace sendspin From b3eb3d2028fdff8ff5fb44a37f0a80f669be4e03 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Wed, 14 Oct 2026 09:52:55 -0400 Subject: [PATCH 2/4] Analyze the message trace in the clang-tidy run clang-tidy analyzes every src/ file against the default build's compile database, where the trace is off, so message_trace.cpp was analyzed without its macro. Generate the database with the trace on. --- script/clang-tidy.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/script/clang-tidy.sh b/script/clang-tidy.sh index 3db5f1ee..86fdd113 100755 --- a/script/clang-tidy.sh +++ b/script/clang-tidy.sh @@ -33,10 +33,10 @@ if [ -z "$CLANG_TIDY" ]; then exit 1 fi -# Ensure compile_commands.json exists +# Ensure compile_commands.json exists. The message trace is on so its gated code is analyzed too. if [ ! -f "${BUILD_DIR}/compile_commands.json" ]; then echo "Generating compile_commands.json..." - cmake -B "$BUILD_DIR" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON "$ROOT_DIR" + cmake -B "$BUILD_DIR" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DSENDSPIN_ENABLE_MESSAGE_TRACE=ON "$ROOT_DIR" fi # Find all source files, excluding build/ and src/esp/ directories From 12a11f67a1b43f6d0d2b41a8ae407b2bbaadc754 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Wed, 14 Oct 2026 09:55:23 -0400 Subject: [PATCH 3/4] Test redaction through the real pairing formatters The secret-bearing rows of the redaction test are now built by the client's own formatters, so a change to how they spell a field fails the test. The conventions threading rule names message_trace as the protocol task's one consumer callback. clang-tidy.sh configures its own build-tidy directory with the trace on every run, so it never reuses a trace-off build or changes the main one. --- docs/conventions.md | 13 ++++---- script/clang-tidy.sh | 13 ++++---- tests/test_message_trace.cpp | 58 +++++++++++++++++++++++------------- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/docs/conventions.md b/docs/conventions.md index 974ee675..7688190e 100644 --- a/docs/conventions.md +++ b/docs/conventions.md @@ -36,12 +36,13 @@ checklists in `.claude/skills/` apply these standards to a diff. provider poll, nothing else (while the client is stopped it only frees the outbound attempts `stop()` left parked): do not add mutex-protected endpoints or atomics that `loop()` polls for work. The protocol task calls no - listener, a role thread calls only the data-path callbacks - `docs/integration-guide.md` names for it (`on_audio_write()` on the sync - task, `on_image_decode()` on the artwork decode thread, the visualizer data - callbacks on its drain thread), and only the main loop calls the persistence - and network providers: everything else a consumer hears it hears from the - main loop's drain. A thread that must wait for the main loop to + listener (its one consumer callback is the host-only + `SendspinClientConfig::message_trace`), a role thread calls only the + data-path callbacks `docs/integration-guide.md` names for it + (`on_audio_write()` on the sync task, `on_image_decode()` on the artwork + decode thread, the visualizer data callbacks on its drain thread), and only + the main loop calls the persistence and network providers: everything else a + consumer hears it hears from the main loop's drain. A thread that must wait for the main loop to have called a listener waits for a grant the drain publishes (the high-performance grant), never for the main loop itself. - The Inbox event ring is for ordered lifecycle events only (stream start and diff --git a/script/clang-tidy.sh b/script/clang-tidy.sh index 86fdd113..9244a973 100755 --- a/script/clang-tidy.sh +++ b/script/clang-tidy.sh @@ -7,7 +7,8 @@ set -e SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" ROOT_DIR="$(dirname "$SCRIPT_DIR")" -BUILD_DIR="${ROOT_DIR}/build" +# Its own build directory, so the trace option below never changes the main build +BUILD_DIR="${ROOT_DIR}/build-tidy" # Find clang-tidy CLANG_TIDY="" @@ -33,11 +34,11 @@ if [ -z "$CLANG_TIDY" ]; then exit 1 fi -# Ensure compile_commands.json exists. The message trace is on so its gated code is analyzed too. -if [ ! -f "${BUILD_DIR}/compile_commands.json" ]; then - echo "Generating compile_commands.json..." - cmake -B "$BUILD_DIR" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DSENDSPIN_ENABLE_MESSAGE_TRACE=ON "$ROOT_DIR" -fi +# Configure on every run (cheap once configured) with the message trace on, so its gated code is +# analyzed too +echo "Generating compile_commands.json..." +cmake -B "$BUILD_DIR" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DSENDSPIN_ENABLE_MESSAGE_TRACE=ON \ + "$ROOT_DIR" > /dev/null # Find all source files, excluding build/ and src/esp/ directories SOURCES=$(find "$ROOT_DIR/src" \ diff --git a/tests/test_message_trace.cpp b/tests/test_message_trace.cpp index 02fc4d56..8ca891ae 100644 --- a/tests/test_message_trace.cpp +++ b/tests/test_message_trace.cpp @@ -15,15 +15,20 @@ // The plaintext message trace (SendspinClientConfig::message_trace). Built only with // SENDSPIN_ENABLE_MESSAGE_TRACE. +#include "crypto/psk_wrap.h" #include "lifecycle_test_fixtures.h" #include "message_trace.h" +#include "platform/base64.h" #include "platform/time.h" +#include "protocol_messages.h" #include "sendspin/client.h" #include "sendspin/config.h" +#include "test_util.h" #include #include +#include #include #include #include @@ -174,33 +179,46 @@ TEST(MessageTrace, RecordsTheHandshakeTheTransportMessagesAndAnAudioChunk) { pump_for(client, 100); } -// Each pairing secret reads "" and nothing else changes; a message with no secret, and -// a binary message, reach the trace as the very bytes sent. +/// Copies `text` with every occurrence of `secret` replaced by "". +std::string with_redacted(std::string text, const std::string& secret) { + for (size_t at = text.find(secret); at != std::string::npos; at = text.find(secret, at)) { + text.replace(at, secret.size(), ""); + } + return text; +} + +// Each pairing secret, as the client's own formatters spell it, reads "" and nothing +// else changes; a message with no secret, and a binary message, reach the trace as the very bytes +// sent. TEST(MessageTrace, RedactsThePairingSecretsAndNothingElse) { + std::array psk{}; + psk.fill(0x11); + std::array wrapped{}; + wrapped.fill(0x22); + std::array client_kc{}; + client_kc.fill(0x33); + const std::string psk_b64 = b64url_encode(psk.data(), psk.size()); + const std::string wrapped_b64 = b64url_encode(wrapped.data(), wrapped.size()); + struct Row { const char* name; bool binary; std::string sent; - std::string traced; // Empty: the sent bytes, unchanged and not copied. + std::string secret; // Empty: the sent bytes reach the trace unchanged and not copied. }; const std::string json_body(1, '\0'); - const std::string audio("\x04", 1); const std::vector rows = { - {"cleartext long_term_psk", false, - R"({"type":"client/pair-finalize","payload":{"long_term_psk":"SECRETSECRET"}})", - R"({"type":"client/pair-finalize","payload":{"long_term_psk":""}})"}, - {"transport wrapped_psk", true, - json_body + R"({"type":"client/pair-finalize","payload":{"wrapped_psk":"SECRET"}})", - json_body + R"({"type":"client/pair-finalize","payload":{"wrapped_psk":""}})"}, + {"cleartext long_term_psk", false, format_client_pair_finalize_message(psk, TestArena()), + psk_b64}, + {"wrapped_psk", true, + json_body + format_client_pair_finalize_wrapped_message(wrapped, TestArena()), + wrapped_b64}, {"wrapped_nonce_B beside client_kc", true, - json_body + - R"({"type":"client/pair-confirm","payload":{"client_kc":"TAG","wrapped_nonce_B":"SECRET"}})", - json_body + - R"({"type":"client/pair-confirm","payload":{"client_kc":"TAG","wrapped_nonce_B":""}})"}, - {"unterminated value", true, json_body + R"({"payload":{"wrapped_psk":"SECR)", - json_body + R"({"payload":{"wrapped_psk":")"}, + json_body + format_client_pair_confirm_message(client_kc, wrapped, TestArena()), + wrapped_b64}, + {"unterminated value", true, json_body + R"({"payload":{"wrapped_psk":"SECRET)", "SECRET"}, {"no secret", true, json_body + R"({"type":"client/state","payload":{}})", ""}, - {"binary audio", true, audio + R"("wrapped_psk":"BYTES")", ""}, + {"binary audio", true, std::string("\x04", 1) + R"("wrapped_psk":"BYTES")", ""}, }; for (const Row& row : rows) { @@ -222,11 +240,11 @@ TEST(MessageTrace, RedactsThePairingSecretsAndNothingElse) { } ASSERT_EQ(traced.size(), 1U) << row.name; - if (row.traced.empty()) { + if (row.secret.empty()) { EXPECT_EQ(pointers[0], sent) << row.name; } else { - EXPECT_EQ(traced[0], row.traced) << row.name; - EXPECT_EQ(traced[0].find("SECR"), std::string::npos) << row.name; + ASSERT_NE(row.sent.find(row.secret), std::string::npos) << row.name; + EXPECT_EQ(traced[0], with_redacted(row.sent, row.secret)) << row.name; } } } From d0339cf0c9f6843c1bb67de41fc85b1fe3dcbf8e Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Wed, 14 Oct 2026 10:05:59 -0400 Subject: [PATCH 4/4] Check the redaction controls' traced bytes and label them The pass-through rows now compare the traced bytes with what was sent, so a wrong plaintext_len fails them, and carry the Control: label the test standards require. --- tests/test_message_trace.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_message_trace.cpp b/tests/test_message_trace.cpp index 8ca891ae..5cfa0bfb 100644 --- a/tests/test_message_trace.cpp +++ b/tests/test_message_trace.cpp @@ -217,8 +217,8 @@ TEST(MessageTrace, RedactsThePairingSecretsAndNothingElse) { json_body + format_client_pair_confirm_message(client_kc, wrapped, TestArena()), wrapped_b64}, {"unterminated value", true, json_body + R"({"payload":{"wrapped_psk":"SECRET)", "SECRET"}, - {"no secret", true, json_body + R"({"type":"client/state","payload":{}})", ""}, - {"binary audio", true, std::string("\x04", 1) + R"("wrapped_psk":"BYTES")", ""}, + {"Control: no secret", true, json_body + R"({"type":"client/state","payload":{}})", ""}, + {"Control: binary audio", true, std::string("\x04", 1) + R"("wrapped_psk":"BYTES")", ""}, }; for (const Row& row : rows) { @@ -242,6 +242,7 @@ TEST(MessageTrace, RedactsThePairingSecretsAndNothingElse) { ASSERT_EQ(traced.size(), 1U) << row.name; if (row.secret.empty()) { EXPECT_EQ(pointers[0], sent) << row.name; + EXPECT_EQ(traced[0], row.sent) << row.name; } else { ASSERT_NE(row.sent.find(row.secret), std::string::npos) << row.name; EXPECT_EQ(traced[0], with_redacted(row.sent, row.secret)) << row.name;