From 88402743d53e2f682022d5b0107fc1547ce92aa5 Mon Sep 17 00:00:00 2001 From: kbx81 Date: Wed, 2 Sep 2026 22:27:52 -0500 Subject: [PATCH] [serial_proxy] Enforce session scoping and RAW inertness for the port mode Address review findings: - Only the live subscriber may set the mode, so a mode set by a client that never subscribes cannot persist past its session - With a subscriber attached, the mode alone decides whether the tap is served; tap_needs_port() bypasses it only while the port is unheld, and write_from_tap() is gated the same way, so RAW is inert by code - The explicit UNSUBSCRIBE path keeps the loop alive for a tap that still needs the port, mirroring the disconnect path in loop() - Mode values from the wire are validated; unknown values are refused with INVALID_ARGUMENT instead of stored and acknowledged OK - Add a test variant that defines USE_SERIAL_PROXY_TAP so the tap code paths compile in a real build --- esphome/components/api/api.proto | 1 + .../components/serial_proxy/serial_proxy.cpp | 28 +++++++++++++------ .../components/serial_proxy/serial_proxy.h | 12 ++++++-- .../serial_proxy/test-tap.esp32-idf.yaml | 14 ++++++++++ 4 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 tests/components/serial_proxy/test-tap.esp32-idf.yaml diff --git a/esphome/components/api/api.proto b/esphome/components/api/api.proto index 8e7bdbe45d..6e5d0d6200 100644 --- a/esphome/components/api/api.proto +++ b/esphome/components/api/api.proto @@ -2851,6 +2851,7 @@ enum SerialProxyMode { SERIAL_PROXY_MODE_PROTOCOL = 1; } +// Only the subscribed client may change the mode; others are refused with PORT_IN_USE. message SerialProxySetModeRequest { option (id) = 152; option (source) = SOURCE_CLIENT; diff --git a/esphome/components/serial_proxy/serial_proxy.cpp b/esphome/components/serial_proxy/serial_proxy.cpp index 45347548d7..36e5aee678 100644 --- a/esphome/components/serial_proxy/serial_proxy.cpp +++ b/esphome/components/serial_proxy/serial_proxy.cpp @@ -120,11 +120,10 @@ bool SerialProxy::tap_observing_() const { if (this->tap_ == nullptr) { return false; } - // A tap that needs the port is mid-protocol-work of its own -- the boot-time handshake - // with the device, which runs before any client has connected and so before anyone could - // have chosen a mode. Withholding bytes from it there would strand it, so it is served - // regardless of mode. - if (this->tap_->tap_needs_port()) { + // With no subscriber, a tap doing its own protocol work (the boot-time handshake with + // the device, say) is served regardless of mode -- nobody has chosen one yet. Once a + // subscriber holds the port, the mode alone decides, so RAW stays inert. + if (this->api_connection_ == nullptr && this->tap_->tap_needs_port()) { return true; } // Otherwise the mode decides. RAW must be inert: a client that flips to RAW before @@ -233,13 +232,19 @@ SerialProxyResult SerialProxy::configure(api::APIConnection *api_connection, uin SerialProxyResult SerialProxy::set_mode(api::APIConnection *api_connection, api::enums::SerialProxyMode mode) { #ifdef USE_API - if (this->port_claimed_by_other_(api_connection)) { - ESP_LOGW(TAG, "Ignoring mode request from client without port access [%" PRIu32 "]", this->instance_index_); + // Only the live subscriber may change the mode, so the mode cannot outlive a session + if (this->api_connection_ != api_connection) { + ESP_LOGW(TAG, "Ignoring mode request from client without port subscription [%" PRIu32 "]", this->instance_index_); return SerialProxyResult::SERIAL_PROXY_RESULT_PORT_IN_USE; } #endif + // Values come from a remote client + if (mode != api::enums::SERIAL_PROXY_MODE_RAW && mode != api::enums::SERIAL_PROXY_MODE_PROTOCOL) { + ESP_LOGW(TAG, "Invalid mode: %" PRIu32, static_cast(mode)); + return SerialProxyResult::SERIAL_PROXY_RESULT_INVALID_ARGUMENT; + } ESP_LOGD(TAG, "Serial proxy [%" PRIu32 "] mode set to %s", this->instance_index_, - mode == api::enums::SERIAL_PROXY_MODE_PROTOCOL ? "PROTOCOL" : "RAW"); + mode == api::enums::SERIAL_PROXY_MODE_PROTOCOL ? LOG_STR_LITERAL("PROTOCOL") : LOG_STR_LITERAL("RAW")); const bool leaving_protocol_mode = this->mode_ != api::enums::SERIAL_PROXY_MODE_RAW && mode == api::enums::SERIAL_PROXY_MODE_RAW; this->mode_ = mode; @@ -368,7 +373,14 @@ SerialProxyResult SerialProxy::serial_proxy_request(api::APIConnection *api_conn } this->api_connection_ = nullptr; this->reset_mode_(); +#ifdef USE_SERIAL_PROXY_TAP + // Keep the loop alive for a tap that still needs the port (mirrors loop()) + if (this->tap_ == nullptr || !this->tap_->tap_needs_port()) { + this->disable_loop(); + } +#else this->disable_loop(); +#endif ESP_LOGV(TAG, "API connection unsubscribed from serial proxy [%" PRIu32 "]", this->instance_index_); return SerialProxyResult::SERIAL_PROXY_RESULT_OK; default: diff --git a/esphome/components/serial_proxy/serial_proxy.h b/esphome/components/serial_proxy/serial_proxy.h index 52ece5ad7f..a48e69fba7 100644 --- a/esphome/components/serial_proxy/serial_proxy.h +++ b/esphome/components/serial_proxy/serial_proxy.h @@ -71,7 +71,8 @@ class SerialProxyTap { virtual void on_client_tx(const uint8_t *data, size_t len) = 0; /// True when the port must keep reading even with no subscriber attached, so a tap can - /// do its own protocol work while nobody is listening. + /// do its own protocol work while nobody is listening. Honoured only while no + /// subscriber holds the port; with one attached, the port mode alone decides. virtual bool tap_needs_port() const = 0; /// A client explicitly turned protocol handling off for this port. Distinct from the @@ -165,8 +166,13 @@ class SerialProxy final : public uart::UARTDevice, public Component { void set_tap(SerialProxyTap *tap) { this->tap_ = tap; } /// Write bytes originating from the tap rather than from a client. Bypasses the - /// subscriber ownership check, since the tap is part of the device, not a client of it. - void write_from_tap(const uint8_t *data, size_t len) { this->write_array(data, len); } + /// subscriber ownership check, but only while the tap is being served bytes -- so a + /// port in RAW mode with a subscriber attached stays inert. + void write_from_tap(const uint8_t *data, size_t len) { + if (this->tap_observing_()) { + this->write_array(data, len); + } + } /// Resume reading after a tap's needs change. loop() disables itself when there is /// neither a subscriber nor a tap that wants the port, so a tap starting fresh work diff --git a/tests/components/serial_proxy/test-tap.esp32-idf.yaml b/tests/components/serial_proxy/test-tap.esp32-idf.yaml new file mode 100644 index 0000000000..5522e53c47 --- /dev/null +++ b/tests/components/serial_proxy/test-tap.esp32-idf.yaml @@ -0,0 +1,14 @@ +substitutions: + tx_pin: GPIO4 + rx_pin: GPIO5 + +# Compile the tap code paths; no tap is attached, so this exercises the +# null-tap branches that a normal build never defines. +esphome: + platformio_options: + build_flags: + - "-DUSE_SERIAL_PROXY_TAP" + +packages: + uart: !include ../../test_build_components/common/uart/esp32-idf.yaml + serial_proxy: !include common.yaml