fix(captive-portal): route by Host header and serve the portal only on the AP

.changes/captive-portal-host-routing.md

@@ -0,0 +1,16 @@
+---
+kind: fixed
+---
+Route captive portal requests by Host header, and serve the portal only on its own network
+
+## Release Note
+More reliable captive portal detection
+
+Phones and computers joining the hub's setup network now reliably open the sign-in screen, whatever address they check. The setup portal and its DNS server are now only reachable from the setup network, not from your home network.
+
+- Any request to another website over the setup network is sent to the portal, not just a fixed list of check addresses
+- Unknown pages on the hub show "Not Found" instead of jumping back to the start page
+- The captive portal API also answers at /captiveportal/api
+
+## Notices
+- note: **The setup portal no longer answers on your home network.** Opening the hub's home network address in a browser now fails. Join the hub's setup network to reach the portal.

components/captive_portal/include/captiveportal/Manager.h

@@ -22,6 +22,10 @@ namespace OpenShock::CaptivePortal {
   /// @brief The fixed IPv4 address the captive portal AP serves from (e.g. "4.3.2.1").
   const char* ApIPv4String();
 
+  /// @brief The lwIP name of the captive portal AP's network interface (e.g. "ap1"), for binding sockets to it.
+  /// Empty until the portal has started once.
+  const char* ApInterfaceName();
+
   bool SendMessageTXT(uint8_t socketId, std::string_view data);
   bool SendMessageBIN(uint8_t socketId, std::span<const uint8_t> data);
 

components/captive_portal/src/captiveportal/ApiHandlers.cpp

@@ -387,19 +387,19 @@ static esp_err_t apiOtaCheck(httpd_req_t* req)
 
 void CaptivePortal::RegisterApiHandlers(httpd_handle_t server, CaptivePortalInstance* instance)
 {
-  registerHandler(server, "/api/board", HTTP_GET, apiBoard, instance);
-  registerHandler(server, "/api/portal/close", HTTP_POST, apiPortalClose, instance);
-  registerHandler(server, "/api/wifi/scan", HTTP_POST, apiWifiScan, instance);
-  registerHandler(server, "/api/wifi/networks", HTTP_DELETE, apiWifiNetworksDelete, instance);
-  registerHandler(server, "/api/wifi/networks", HTTP_POST, apiWifiNetworksAdd, instance);
-  registerHandler(server, "/api/wifi/connect", HTTP_POST, apiWifiConnect, instance);
-  registerHandler(server, "/api/wifi/disconnect", HTTP_POST, apiWifiDisconnect, instance);
-  registerHandler(server, "/api/account/link", HTTP_POST, apiAccountLink, instance);
-  registerHandler(server, "/api/account", HTTP_DELETE, apiAccountDelete, instance);
-  registerHandler(server, "/api/config/rf/pin", HTTP_PUT, apiConfigRfPin, instance);
+  registerHandler<apiBoard>(server, "/api/board", HTTP_GET, instance);
+  registerHandler<apiPortalClose>(server, "/api/portal/close", HTTP_POST, instance);
+  registerHandler<apiWifiScan>(server, "/api/wifi/scan", HTTP_POST, instance);
+  registerHandler<apiWifiNetworksDelete>(server, "/api/wifi/networks", HTTP_DELETE, instance);
+  registerHandler<apiWifiNetworksAdd>(server, "/api/wifi/networks", HTTP_POST, instance);
+  registerHandler<apiWifiConnect>(server, "/api/wifi/connect", HTTP_POST, instance);
+  registerHandler<apiWifiDisconnect>(server, "/api/wifi/disconnect", HTTP_POST, instance);
+  registerHandler<apiAccountLink>(server, "/api/account/link", HTTP_POST, instance);
+  registerHandler<apiAccountDelete>(server, "/api/account", HTTP_DELETE, instance);
+  registerHandler<apiConfigRfPin>(server, "/api/config/rf/pin", HTTP_PUT, instance);
   // No /api/config/estop/*: the portal is an open, unauthenticated AP, so the E-Stop is configured over serial only.
   // No /api/ota/domain: the portal is an open, unauthenticated AP, and OTA trusts the hashes served by that domain, so
   // changing it would let anyone in range install firmware. It can only be changed over serial (jsonconfig).
-  registerHandler(server, "/api/ota/settings", HTTP_PUT, apiOtaSettings, instance);
-  registerHandler(server, "/api/ota/check", HTTP_POST, apiOtaCheck, instance);
+  registerHandler<apiOtaSettings>(server, "/api/ota/settings", HTTP_PUT, instance);
+  registerHandler<apiOtaCheck>(server, "/api/ota/check", HTTP_POST, instance);
 }

components/captive_portal/src/captiveportal/CaptivePortalInstance.cpp

@@ -11,6 +11,9 @@ const char* const TAG = "CaptivePortalInstance";
 
 #include <lwip/sockets.h>
 
+#include <cstdint>
+#include <string_view>
+
 const uint16_t HTTP_PORT = 80;
 // httpd needs 3 lwIP sockets besides its client sockets; the DNS server, the gateway connection and an outbound HTTP request need one each
 static_assert(CONFIG_LWIP_MAX_SOCKETS >= CONFIG_OPENSHOCK_PORTAL_HTTPD_MAX_SOCKETS + 3 + 3, "httpd client sockets would leave no lwIP sockets for the DNS server and outbound connections");
@@ -23,8 +26,8 @@ void CaptivePortal::CaptivePortalInstance::registerHandlers()
   // API endpoints (registered before the static wildcard so they take priority).
   RegisterApiHandlers(m_server, this);
 
-  // OS captive-detection probes + RFC 8908 endpoint + 404 redirect (rfc8908 component).
-  // Registered before the static wildcard so the specific probe paths take priority.
+  // RFC 8908 endpoint (rfc8908 component); probes fall through to the static handler's redirect.
+  // Registered before the static wildcard so the API paths take priority.
   RFC8908::RegisterProbeHandlers(m_server, CaptivePortal::ApIPv4String());
 
   m_webSocket.registerHandler(m_server);
@@ -46,6 +49,18 @@ static esp_err_t onSessionOpen(httpd_handle_t /*server*/, int sockfd)
 
 bool CaptivePortal::CaptivePortalInstance::startHttpServer()
 {
+  // Bound to the AP interface (SO_BINDTODEVICE), so lwIP only hands it connections that arrive on the AP. Checking
+  // the local address is not enough: lwIP accepts a packet for any of the hub's addresses on any interface, so a LAN
+  // client that routes the AP address via the hub's STA address would get through. The portal's API is the local
+  // protocol, which has no compatibility rules: nothing on the LAN may come to depend on it.
+  ifreq apInterface         = {};  // zeroed, so the copied name is NUL-terminated
+  std::string_view apIfName = CaptivePortal::ApInterfaceName();
+  if (apIfName.empty() || apIfName.size() >= sizeof(apInterface.ifr_name)) {
+    OS_LOGE(TAG, "No usable AP interface name (\"%.*s\"), not starting the HTTP server", static_cast<int>(apIfName.size()), apIfName.data());
+    return false;
+  }
+  apIfName.copy(apInterface.ifr_name, apIfName.size());
+
   httpd_config_t config   = HTTPD_DEFAULT_CONFIG();
   config.server_port      = HTTP_PORT;
   config.max_uri_handlers = 40;
@@ -57,6 +72,7 @@ bool CaptivePortal::CaptivePortalInstance::startHttpServer()
   config.send_wait_timeout = CONFIG_OPENSHOCK_PORTAL_HTTPD_TIMEOUT_S;
   config.uri_match_fn      = httpd_uri_match_wildcard;
   config.open_fn           = onSessionOpen;
+  config.if_name           = &apInterface;  // only read during httpd_start
   // Same core as the WiFi/lwIP tasks, away from the E-Stop and RF tasks on core 1
   config.core_id = 0;
 
@@ -78,8 +94,9 @@ CaptivePortal::CaptivePortalInstance::CaptivePortalInstance()
     return;
   }
 
-  // Start the wildcard DNS responder (all A queries -> the portal AP IP).
-  m_dnsStarted = m_dnsServer.start(CaptivePortal::ApIPv4String());
+  // Start the wildcard DNS responder (all A queries -> the portal AP IP), on the AP interface only: the LAN must not
+  // get its answers.
+  m_dnsStarted = m_dnsServer.start(CaptivePortal::ApIPv4String(), CaptivePortal::ApIPv4String(), CaptivePortal::ApInterfaceName());
   if (!m_dnsStarted) {
     OS_LOGE(TAG, "Failed to start DNS server");
   }

components/captive_portal/src/captiveportal/HttpUtils.cpp

@@ -21,7 +21,7 @@ esp_err_t CaptivePortal::sendResp(httpd_req_t* req, const char* status, const ch
   return httpd_resp_send(req, body.data(), body.size());
 }
 
-void CaptivePortal::registerHandler(httpd_handle_t server, const char* uri, httpd_method_t method, esp_err_t (*handler)(httpd_req_t*), void* userCtx)
+void CaptivePortal::detail::registerHandler(httpd_handle_t server, const char* uri, httpd_method_t method, esp_err_t (*handler)(httpd_req_t*), void* userCtx)
 {
   httpd_uri_t def = {};
   def.uri         = uri;

components/captive_portal/src/captiveportal/HttpUtils.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include "rfc8908/RFC8908Handler.h"
+
 #include <esp_http_server.h>
 
 #include <expected>
@@ -26,8 +28,29 @@ namespace OpenShock::CaptivePortal {
 
   esp_err_t sendResp(httpd_req_t* req, const char* status, const char* type, std::string_view body);
 
-  /// @brief Registers a route, logging a failure. With wildcard matching, routes registered earlier take priority.
-  void registerHandler(httpd_handle_t server, const char* uri, httpd_method_t method, esp_err_t (*handler)(httpd_req_t*), void* userCtx);
+  namespace detail {
+    void registerHandler(httpd_handle_t server, const char* uri, httpd_method_t method, esp_err_t (*handler)(httpd_req_t*), void* userCtx);
+
+    // httpd has no hook that runs before every request, so each route is wrapped in this. A request addressed to
+    // another host (a connectivity probe, a page on a hijacked or rebound name) never reaches a portal handler; it
+    // gets the captive redirect instead.
+    template<esp_err_t (*Handler)(httpd_req_t*)>
+    esp_err_t ownHostOnly(httpd_req_t* req)
+    {
+      if (RFC8908::IsForeignHost(req)) {
+        return RFC8908::EmitRedirect(req);
+      }
+      return Handler(req);
+    }
+  }  // namespace detail
+
+  /// @brief Registers a route that only answers requests addressed to us (see detail::ownHostOnly), logging a failure.
+  /// With wildcard matching, routes registered earlier take priority.
+  template<esp_err_t (*Handler)(httpd_req_t*)>
+  void registerHandler(httpd_handle_t server, const char* uri, httpd_method_t method, void* userCtx)
+  {
+    detail::registerHandler(server, uri, method, &detail::ownHostOnly<Handler>, userCtx);
+  }
 
   // Read and URL-decode a query-string parameter (key matched case-insensitively, as httpd does). The query is
   // parsed in place in the request URI: no copy, and no limit on the value's length.

components/captive_portal/src/captiveportal/Manager.cpp

@@ -17,6 +17,7 @@ const char* const TAG = "CaptivePortal";
 #include "Temporal.h"
 #include "wifi/WiFiConfig.h"
 
+#include <esp_event.h>
 #include <esp_mac.h>
 #include <esp_netif.h>
 #include <esp_timer.h>
@@ -74,6 +75,12 @@ static std::atomic<bool> s_instanceLive = false;
 // Instances taken down whose destruction waits for other tasks to drop their references; destroyed by the manager task.
 static RetiredList<CaptivePortalInstance> s_retiredInstances;
 static esp_netif_t* s_apNetif = nullptr;
+// Given once WIFI_EVENT_AP_START has added s_apNetif to lwIP, so its interface name can be read.
+static SemaphoreHandle_t s_apStarted = nullptr;
+// lwIP's name for s_apNetif, which the portal's servers bind to. Re-read on every start: lwIP only knows the netif
+// while the AP runs, and gives it a new number each time the AP starts again.
+static char s_apIfName[8]                   = {};
+static constexpr TickType_t kApStartTimeout = pdMS_TO_TICKS(2000);
 
 static_assert(IPv4Utils::IsSlash24Host(CONFIG_OPENSHOCK_PORTAL_AP_IP), "CONFIG_OPENSHOCK_PORTAL_AP_IP must be a dotted-decimal IPv4 address with a host octet of 1-254");
 
@@ -139,6 +146,13 @@ static bool DestroyInstance()
   return false;
 }
 
+// Registered after esp_wifi's default WIFI_EVENT_AP_START handler, so it runs once that one has added the AP netif
+// to lwIP: only from then on does lwIP hand out the netif's real interface name.
+static void captiveportal_onapstart(void*, esp_event_base_t, int32_t, void*)
+{
+  xSemaphoreGive(s_apStarted);
+}
+
 static bool captiveportal_start()
 {
   if (GetInstance() != nullptr) {
@@ -178,6 +192,17 @@ static bool captiveportal_start()
     esp_netif_dhcps_start(s_apNetif);
   }
 
+  // esp_netif_create_default_wifi_ap() registered esp_wifi's default handlers, so this one runs after them.
+  static bool apStartHandlerRegistered = false;
+  if (!apStartHandlerRegistered) {
+    esp_err_t evtErr = esp_event_handler_register(WIFI_EVENT, WIFI_EVENT_AP_START, captiveportal_onapstart, nullptr);
+    if (evtErr != ESP_OK) {
+      OS_LOGE(TAG, "Failed to register the AP start handler: %s", esp_err_to_name(evtErr));
+      return false;
+    }
+    apStartHandlerRegistered = true;
+  }
+
   // AP SSID = prefix + this device's STA MAC (matches the old Arduino naming).
   uint8_t mac[6];
   esp_read_mac(mac, ESP_MAC_WIFI_STA);
@@ -190,6 +215,9 @@ static bool captiveportal_start()
   apConfig.ap.max_connection  = CONFIG_OPENSHOCK_PORTAL_MAX_CLIENTS;
   apConfig.ap.beacon_interval = 100;
 
+  s_apIfName[0] = '\0';
+  xSemaphoreTake(s_apStarted, 0);  // drop a start left over from an earlier run
+
   esp_err_t err = esp_wifi_set_mode(WIFI_MODE_APSTA);
   if (err != ESP_OK) {
     OS_LOGE(TAG, "Failed to enable AP mode: %s", esp_err_to_name(err));
@@ -203,6 +231,22 @@ static bool captiveportal_start()
     return false;
   }
 
+  // The servers bind to the AP's lwIP interface, which only exists once the AP has started. Reading its name earlier
+  // returns another interface's (the STA's), and would put the portal on the LAN.
+  if (!esp_netif_is_netif_up(s_apNetif) && xSemaphoreTake(s_apStarted, kApStartTimeout) != pdTRUE) {
+    OS_LOGE(TAG, "AP did not start");
+    esp_wifi_set_mode(WIFI_MODE_STA);
+    return false;
+  }
+
+  err = esp_netif_get_netif_impl_name(s_apNetif, s_apIfName);
+  if (err != ESP_OK) {
+    s_apIfName[0] = '\0';
+    OS_LOGE(TAG, "Failed to get the AP interface name: %s", esp_err_to_name(err));
+    esp_wifi_set_mode(WIFI_MODE_STA);
+    return false;  // the servers could not be bound to the AP, and must not listen on the LAN
+  }
+
   if (!CreateInstance()) {
     esp_wifi_set_mode(WIFI_MODE_STA);
     return false;
@@ -367,8 +411,9 @@ bool CaptivePortal::Init()
   }
 
   s_forceCloseDone = xSemaphoreCreateBinary();
-  if (s_forceCloseDone == nullptr) {
-    OS_LOGE(TAG, "Failed to create captive portal force-close semaphore");
+  s_apStarted      = xSemaphoreCreateBinary();
+  if (s_forceCloseDone == nullptr || s_apStarted == nullptr) {
+    OS_LOGE(TAG, "Failed to create captive portal semaphores");
     delete initialState;
     return false;
   }
@@ -455,6 +500,11 @@ const char* CaptivePortal::ApIPv4String()
   return CONFIG_OPENSHOCK_PORTAL_AP_IP;
 }
 
+const char* CaptivePortal::ApInterfaceName()
+{
+  return s_apIfName;
+}
+
 bool CaptivePortal::SendMessageTXT(uint8_t socketId, std::string_view data)
 {
   auto instance = GetInstance();

components/captive_portal/src/captiveportal/StaticFileServer.cpp

@@ -158,7 +158,7 @@ static esp_err_t sendNotModified(httpd_req_t* req, const Representation& rep)
 static esp_err_t sendNotAcceptable(httpd_req_t* req)
 {
   httpd_resp_set_hdr(req, "Vary", "Accept-Encoding");
-  return CaptivePortal::sendResp(req, CaptivePortal::S406, HTTP::ContentType::TextPlain, "Only available with Content-Encoding: gzip\n");
+  return CaptivePortal::sendResp(req, CaptivePortal::S406, HTTP::ContentType::TextPlain, "Only available with Content-Encoding: gzip");
 }
 
 // httpd derives Content-Length from the buffer it is handed and has no bodiless send, so the HEAD response (the GET's
@@ -211,15 +211,20 @@ void CaptivePortal::StaticFileServer::registerHandler(httpd_handle_t server)
 {
   // Decided once: the assets are mounted in the constructor and stay that way until destruction
   if (m_www.isMounted()) {
-    CaptivePortal::registerHandler(server, "/*", HTTP_GET, &StaticFileServer::staticFileHandler, this);
-    CaptivePortal::registerHandler(server, "/*", HTTP_HEAD, &StaticFileServer::staticFileHandler, this);
+    CaptivePortal::registerHandler<&StaticFileServer::staticFileHandler>(server, "/*", HTTP_GET, this);
+    CaptivePortal::registerHandler<&StaticFileServer::staticFileHandler>(server, "/*", HTTP_HEAD, this);
   } else {
-    CaptivePortal::registerHandler(server, "/*", HTTP_GET, &StaticFileServer::missingAssetsHandler, nullptr);
+    CaptivePortal::registerHandler<&StaticFileServer::missingAssetsHandler>(server, "/*", HTTP_GET, nullptr);
   }
 }
 
 esp_err_t CaptivePortal::StaticFileServer::missingAssetsHandler(httpd_req_t* req)
 {
+  // A probe still has to fail the connectivity check, or the OS never opens the portal to show this page
+  if (RFC8908::IsProbeRequest(req)) {
+    return RFC8908::EmitRedirect(req);
+  }
+
   // No usable assets on the device; serve the help page for any request.
   return sendResp(
     req,
@@ -245,7 +250,7 @@ esp_err_t CaptivePortal::StaticFileServer::staticFileHandler(httpd_req_t* req)
   // A ".." segment is never a valid asset path. Browsers normalize these away before
   // sending, so this only comes from hand-built requests: refuse it rather than answer like a captive probe.
   if (uri.find("..") != std::string_view::npos) {
-    return sendResp(req, S400, HTTP::ContentType::TextPlain, "Bad Request\n");
+    return sendResp(req, S400, HTTP::ContentType::TextPlain, "Bad Request");
   }
 
   if (uri == "/") {
@@ -255,8 +260,7 @@ esp_err_t CaptivePortal::StaticFileServer::staticFileHandler(httpd_req_t* req)
   // Stored without the leading slash. Every body in the image is gzipped.
   auto asset = self->m_www.find(uri.substr(1));
   if (!asset) {
-    // Unknown path -> captive redirect (the index fallback of a static server).
-    return RFC8908::EmitRedirect(req);
+    return RFC8908::HandleMiss(req);
   }
   if (!clientAcceptsGzip(req)) {
     return sendNotAcceptable(req);

components/captive_portal/src/captiveportal/WebSocketEndpoint.cpp

@@ -7,12 +7,19 @@ const char* const TAG = "CaptivePortalWs";
 #include "GatewayConnectionManager.h"
 #include "Logging.h"
 #include "message_handlers/WebSocket.h"
+#include "rfc8908/RFC8908Handler.h"
 #include "serialization/WSLocal.h"
 #include "wifi/WiFiManager.h"
 #include "wifi/WiFiScanManager.h"
 
 #include <utility>
 
+// Without it httpd_uri_t has no ws_pre_handshake_cb. sdkconfig.defaults enables it, but an existing sdkconfig wins over
+// the defaults, so a build directory configured before the option was added still has it off.
+#if !CONFIG_HTTPD_WS_PRE_HANDSHAKE_CB_SUPPORT
+#error "CONFIG_HTTPD_WS_PRE_HANDSHAKE_CB_SUPPORT is off: this build directory's sdkconfig predates it. Delete build/<board>/sdkconfig and rebuild."
+#endif
+
 using namespace OpenShock;
 
 void CaptivePortal::WebSocketEndpoint::registerHandler(httpd_handle_t server)
@@ -27,6 +34,7 @@ void CaptivePortal::WebSocketEndpoint::registerHandler(httpd_handle_t server)
   ws.is_websocket             = true;
   ws.handle_ws_control_frames = false;
   ws.supported_subprotocol    = "flatbuffers";
+  ws.ws_pre_handshake_cb      = &WebSocketEndpoint::wsPreHandshake;
   ws.ws_post_handshake_cb     = &WebSocketEndpoint::wsPostHandshake;
   if (httpd_register_uri_handler(m_server, &ws) != ESP_OK) {
     OS_LOGE(TAG, "Failed to register WebSocket handler");
@@ -138,6 +146,18 @@ void CaptivePortal::WebSocketEndpoint::wsSessCtxFree(void* ctx)
 
 // httpd does not call the URI handler for the handshake (esp_http_server's httpd_uri.c), only this callback, right
 // after it has switched the socket to WebSocket: allocate an id, arm the disconnect hook and greet the client.
+// The /ws route is registered directly (it needs the websocket fields), so it does its own Host check, here before
+// httpd answers the handshake. Failing closes the connection after the redirect is sent.
+esp_err_t CaptivePortal::WebSocketEndpoint::wsPreHandshake(httpd_req_t* req)
+{
+  if (RFC8908::IsForeignHost(req)) {
+    OS_LOGW(TAG, "Rejecting WebSocket for another host on fd %d", httpd_req_to_sockfd(req));
+    RFC8908::EmitRedirect(req);
+    return ESP_FAIL;
+  }
+  return ESP_OK;
+}
+
 esp_err_t CaptivePortal::WebSocketEndpoint::wsPostHandshake(httpd_req_t* req)
 {
   auto* self = static_cast<WebSocketEndpoint*>(req->user_ctx);
@@ -162,6 +182,11 @@ esp_err_t CaptivePortal::WebSocketEndpoint::wsHandler(httpd_req_t* req)
   auto* self = static_cast<WebSocketEndpoint*>(req->user_ctx);
   int fd     = httpd_req_to_sockfd(req);
 
+  // A plain GET /ws without the upgrade headers reaches here as an ordinary request: treat it like any other miss
+  if (httpd_ws_get_fd_info(req->handle, fd) != HTTPD_WS_CLIENT_WEBSOCKET) {
+    return RFC8908::HandleMiss(req);
+  }
+
   // Data frame: two-call recv (length first, then payload).
   httpd_ws_frame_t frame = {};
   esp_err_t ret          = httpd_ws_recv_frame(req, &frame, 0);

components/captive_portal/src/captiveportal/WebSocketEndpoint.h

@@ -50,6 +50,7 @@ namespace OpenShock::CaptivePortal {
       std::vector<uint8_t> reasm;      // reassembly buffer for continuation frames
     };
 
+    static esp_err_t wsPreHandshake(httpd_req_t* req);
     static esp_err_t wsPostHandshake(httpd_req_t* req);
     static esp_err_t wsHandler(httpd_req_t* req);