fix: close the token bypass below a route, review fixes for 4b

The routes matched any path below them, and a web asset name there
passed the token check: GET /backup/a.js returned the backup under
Secured. Every route is now an exact match, and a web asset sits in
the root only.

- e2e: those paths answer 401 or 404; the restore test now fails when
  the chunks skip the token check (checked with the check removed).
- WEB_CORS lets the socket take Origin: null instead of skipping the
  Origin check.
- ADR 0006, GLOSSARY and README: the Origin check in either mode, /ws
  as a public route, the Host header behind a reverse proxy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

GLOSSARY.md

@@ -137,7 +137,7 @@ Whether the device asks for login: *None* (everything open) or *Secured* (a PIN
 _Avoid_: Secure Config Only (removed)
 
 **Public route**
-What anyone may reach under Secured without a token: the web UI's files, `/loginContext`, `/login`, `/upnp.xml` and preflight requests. Static serving answers the web UI's files alone, so `shades.cfg` isn't one.
+What anyone may reach under Secured without a token: the web UI's files, `/loginContext`, `/login`, `/upnp.xml` and preflight requests. `/ws` counts as one at the HTTP level, because the socket checks the token itself in the handshake. Static serving answers the web UI's files alone, so `shades.cfg` isn't one.
 
 **Token**
 What logging in returns. It stands for the PIN or password, on one client address, and every request under Secured carries it.

README.md

@@ -100,6 +100,8 @@ Since 4.0.0 everything is on port 80. Ports 8081 (HTTP API) and 8080 (events soc
 A script that called port 8081 only changes the port, if it used one of the three commands. Reads such as `/controller`, `/shades` and `/discovery` are gone: use the socket, or MQTT. The Home Assistant integration needs version 3.0.0 or newer.
 
 When the device is Secured (a PIN or password), every route above except the web UI and the login routes needs the token, in an `apikey` header; the socket takes it as `?apikey=<token>`. Without it the reply is 401. "Secure Config Only" is gone: a device set to it is Secured from 4.0.0, so scripts that sent commands without logging in now need the token.
+
+In either mode the socket refuses an upgrade whose `Origin` header doesn't name the host it was sent to, so another site's page can't open it. Behind a reverse proxy, pass the `Host` header through unchanged.
   
 ## Sources for this Project
 I spent some time reading about a myriad of topics but in the end the primary source for this project comes from https://pushstack.wordpress.com/somfy-rts-protocol/.  The work done on pushstack regarding the protocol timing made this feasible without burning a bunch of time measuring pulses.  

docs/SECURITY_PLAN.md

@@ -43,7 +43,7 @@
 
 ### CORS
 - The wildcard `Access-Control-Allow-*` default headers go. The UI is served from the same origin, and HA and scripts aren't browsers, so CORS doesn't affect them.
-- A build flag (`WEB_CORS` in `platformio.ini`) turns the headers back on for UI development from `file://`, which `baseUrl` in `index.js` supports. It also skips the Origin check, since such a page sends `Origin: null`.
+- A build flag (`WEB_CORS` in `platformio.ini`) turns the headers back on for UI development from `file://`, which `baseUrl` in `index.js` supports. It also lets the socket take `Origin: null`, which such a page sends.
 
 ### UI
 - The token is kept in `sessionStorage`: it survives a reload, is dropped when the tab closes, and is cleared when the socket upgrade is refused.

docs/adr/0006-two-security-modes-eleven-http-routes.md

@@ -18,7 +18,7 @@ With [ADR 0005](0005-one-websocket-protocol.md) moving everything else to the we
 
 Static serving covers the web assets only, so `shades.cfg` and the other config files are never served. The script commands keep today's full field names, since people write those requests by hand. `/discovery` goes: Home Assistant's setup reads the device info from `/loginContext`, and mDNS still advertises `serverId`.
 
-There are two security modes. **None** leaves everything open. **Secured** (PIN or password) needs the token on the socket upgrade (`?apikey=`) and on every route except the static files, `/loginContext`, `/login` and `/upnp.xml`. Devices set to "Secure Config Only" become Secured when they upgrade. The rest of the security work (the token key, the login throttle, CORS, the Origin check) is in [SECURITY_PLAN.md](../SECURITY_PLAN.md).
+There are two security modes. **None** leaves everything open, except that a socket upgrade from another site's page is refused in either mode (the Origin check). **Secured** (PIN or password) needs the token on the socket upgrade (`?apikey=`) and on every route except the static files, `/loginContext`, `/login` and `/upnp.xml`. Devices set to "Secure Config Only" become Secured when they upgrade. The rest of the security work (the token key, the login throttle, CORS, the Origin check) is in [SECURITY_PLAN.md](../SECURITY_PLAN.md).
 
 ## Considered Options
 

platformio.ini

@@ -43,8 +43,8 @@ build_flags =
 	; interleave.  The core already wraps log_printf for ESP Insights.
 	-Wl,--wrap=log_printfv
 	; For work on the web UI from a file:// page (baseUrl in index.js): sends
-	; the CORS headers and lets the socket skip its Origin check.  Never in a
+	; the CORS headers and lets the socket take Origin: null, which such a page
-	; release (docs/SECURITY_PLAN.md, CORS).
+	; sends.  Never in a release (docs/SECURITY_PLAN.md, CORS).
 	; -DWEB_CORS
 monitor_speed = 115200
 monitor_filters = 

src/HttpAccess.h

@@ -7,10 +7,11 @@
 // (docs/adr/0006-two-security-modes-eleven-http-routes.md).  Paths are the
 // request's path, without the query.
 
-// The files the web UI loads.  Static serving answers these alone, so the
+// The files the web UI loads, which all sit in the root.  Static serving
-// rolling codes in shades.cfg leave the device only through /backup.
+// answers these alone, so the rolling codes in shades.cfg leave the device
+// only through /backup.  A path below a route, such as /backup/a.js, isn't one.
 inline bool isWebAsset(const char *path) {
-  if(!path || path[0] != '/' || strstr(path, "..")) return false;
+  if(!path || path[0] != '/' || strchr(path + 1, '/') || strstr(path, "..")) return false;
   if(strcmp(path, "/") == 0 || strcmp(path, "/appversion") == 0) return true;
   static const char *const types[] = {".html", ".js", ".css", ".png", ".svg", ".ico"};
   const size_t len = strlen(path);

src/entryPoints/HttpRoutes.cpp

@@ -52,8 +52,10 @@ static bool uploadAllowed(AsyncWebServerRequest *request, size_t index) {
   if(index == 0) request->setAttribute("uploadAllowed", webServer.isAuthenticated(request));
   return request->getAttribute("uploadAllowed", false);
 }
+// Every route is an exact match.  The library's default also takes any path
+// below the route, and /backup/a.js then passed the token check as a web asset.
 void registerHttpRoutes(AsyncWebServer &server) {
-  server.on("/loginContext", HTTP_GET, [](AsyncWebServerRequest *request) {
+  server.on(AsyncURIMatcher::exact("/loginContext"), HTTP_GET, [](AsyncWebServerRequest *request) {
     AsyncJsonResponse *response = new AsyncJsonResponse();
     JsonObject root = response->getRoot().to<JsonObject>();
     root["type"] = static_cast<uint8_t>(settings.Security.type);
@@ -65,31 +67,31 @@ void registerHttpRoutes(AsyncWebServer &server) {
     request->send(response);
   });
   // Commands for scripts, as a JSON body or query parameters.
-  server.addHandler(new AsyncCallbackJsonWebHandler("/shadeCommand",
+  server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/shadeCommand"),
     [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleShadeCommand(request, json); }));
-  server.on("/shadeCommand", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleShadeCommand(request, v); });
+  server.on(AsyncURIMatcher::exact("/shadeCommand"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleShadeCommand(request, v); });
 
-  server.addHandler(new AsyncCallbackJsonWebHandler("/groupCommand",
+  server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/groupCommand"),
     [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleGroupCommand(request, json); }));
-  server.on("/groupCommand", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleGroupCommand(request, v); });
+  server.on(AsyncURIMatcher::exact("/groupCommand"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleGroupCommand(request, v); });
 
-  server.addHandler(new AsyncCallbackJsonWebHandler("/tiltCommand",
+  server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/tiltCommand"),
     [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleTiltCommand(request, json); }));
-  server.on("/tiltCommand", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleTiltCommand(request, v); });
+  server.on(AsyncURIMatcher::exact("/tiltCommand"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleTiltCommand(request, v); });
 
-  server.on("/upnp.xml", HTTP_GET, [](AsyncWebServerRequest *request) {
+  server.on(AsyncURIMatcher::exact("/upnp.xml"), HTTP_GET, [](AsyncWebServerRequest *request) {
     AsyncResponseStream *response = request->beginResponseStream("text/xml");
     SSDP.schema(*response);
     request->send(response);
   });
 
-  server.addHandler(new AsyncCallbackJsonWebHandler("/login",
+  server.addHandler(new AsyncCallbackJsonWebHandler(AsyncURIMatcher::exact("/login"),
     [](AsyncWebServerRequest *request, JsonVariant &json) { webServer.handleLogin(request, json); }));
-  server.on("/login", HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleLogin(request, v); });
+  server.on(AsyncURIMatcher::exact("/login"), HTTP_GET, [](AsyncWebServerRequest *request) { JsonVariant v; webServer.handleLogin(request, v); });
 
-  server.on("/backup", HTTP_GET, [](AsyncWebServerRequest *request) { webServer.handleBackup(request); });
+  server.on(AsyncURIMatcher::exact("/backup"), HTTP_GET, [](AsyncWebServerRequest *request) { webServer.handleBackup(request); });
 
-  server.on("/restore", HTTP_POST,
+  server.on(AsyncURIMatcher::exact("/restore"), HTTP_POST,
     [](AsyncWebServerRequest *request) {
       if(webServer.uploadSuccess) {
         restore_options_t opts;
@@ -148,7 +150,7 @@ void registerHttpRoutes(AsyncWebServer &server) {
     });
 
   // updateFirmware - file upload
-  server.on("/updateFirmware", HTTP_POST,
+  server.on(AsyncURIMatcher::exact("/updateFirmware"), HTTP_POST,
     [](AsyncWebServerRequest *request) {
       if(Update.hasError())
         request->send(500, _encoding_json, "{\"status\":\"ERROR\",\"desc\":\"Error updating firmware: \"}");
@@ -190,7 +192,7 @@ void registerHttpRoutes(AsyncWebServer &server) {
     });
 
   // updateApplication - file upload
-  server.on("/updateApplication", HTTP_POST,
+  server.on(AsyncURIMatcher::exact("/updateApplication"), HTTP_POST,
     [](AsyncWebServerRequest *request) {
       if(Update.hasError())
         request->send(500, _encoding_json, "{\"status\":\"ERROR\",\"desc\":\"Error updating application: \"}");

src/outputs/ProtoSocket.cpp

@@ -263,12 +263,14 @@ static void queueDeviceOp(AsyncWebSocketClient *client, const proto_message_t &m
 }
 // A browser can't set headers on a socket, so the token comes in the query.
 static bool authorized(AsyncWebServerRequest *request) {
-#ifndef WEB_CORS
-  // A UI opened from a file:// page sends Origin: null, so WEB_CORS skips it.
   const AsyncWebHeader *origin = request->getHeader("Origin");
   const AsyncWebHeader *host = request->getHeader("Host");
-  if(!originMatchesHost(origin ? origin->value().c_str() : nullptr, host ? host->value().c_str() : nullptr)) return false;
+  const char *from = origin ? origin->value().c_str() : nullptr;
+#ifdef WEB_CORS
+  // The UI opened from a file:// page sends Origin: null.
+  if(from && strcmp(from, "null") == 0) from = nullptr;
 #endif
+  if(!originMatchesHost(from, host ? host->value().c_str() : nullptr)) return false;
   const AsyncWebParameter *key = request->getParam("apikey");
   return webServer.tokenAccepted(request, key ? key->value().c_str() : nullptr);
 }

test/e2e/test_routes.py

@@ -4,13 +4,14 @@ mode (docs/adr/0006-two-security-modes-eleven-http-routes.md).
 Like test_auth, it tests the mode it finds: pass --password or --pin for a
 Secured device.
 """
+import json
 import time
 import urllib.error
 import urllib.request
 
 import pytest
 
-from proto_client import ProtoClient, heap
+from proto_client import ProtoClient
 
 # Every route that needs the token under Secured, sent without a body or
 # parameters, so a route that let one through would still change nothing.
@@ -28,23 +29,25 @@ SHADE_FILES = ["/shades.cfg", "/shades.tmp"]
 
 
 def http(host, path, method="GET", body=None, headers=None):
-    """The status and headers of a request, refusals included."""
+    """The status, headers and body of a request, refusals included."""
     req = urllib.request.Request(f"http://{host}{path}", data=body, method=method, headers=headers or {})
     try:
         with urllib.request.urlopen(req, timeout=20) as r:
-            r.read()
+            return r.status, r.headers, r.read()
-            return r.status, r.headers
     except urllib.error.HTTPError as e:
-        e.read()
+        return e.code, e.headers, e.read()
-        return e.code, e.headers
 
 
-def upload(host, path, headers=None):
+def upload(host, path, headers=None, with_file=True):
-    """Posts a small file that isn't a backup or an image."""
+    """Posts a small file that isn't a backup or an image, or a form without a file."""
     boundary = "e2e-upload"
-    body = (
+    if with_file:
-        f"--{boundary}\r\nContent-Disposition: form-data; name=\"file\"; filename=\"e2e.bin\"\r\n"
+        part = (
-        "Content-Type: application/octet-stream\r\n\r\n").encode() + b"\0" * 256 + f"\r\n--{boundary}--\r\n".encode()
+            f"--{boundary}\r\nContent-Disposition: form-data; name=\"file\"; filename=\"e2e.bin\"\r\n"
+            "Content-Type: application/octet-stream\r\n\r\n").encode() + b"\0" * 256
+    else:
+        part = f"--{boundary}\r\nContent-Disposition: form-data; name=\"e2e\"\r\n\r\n1".encode()
+    body = part + f"\r\n--{boundary}--\r\n".encode()
     return http(host, path, "POST", body, {"Content-Type": f"multipart/form-data; boundary={boundary}", **(headers or {})})
 
 
@@ -53,7 +56,7 @@ def test_secured_refuses_a_route_without_a_token(host, secured, method, path):
     if not secured:
         pytest.skip("the device's security is None")
     body = b"{}" if method == "PUT" else None
-    status, _ = http(host, path, method, body, {"Content-Type": "application/json"} if body else None)
+    status, _, _ = http(host, path, method, body, {"Content-Type": "application/json"} if body else None)
     assert status == 401
 
 
@@ -65,49 +68,80 @@ def test_secured_refuses_a_route_with_a_wrong_token(host, secured, method, path)
     headers = {"apikey": "0" * 64}
     if body:
         headers["Content-Type"] = "application/json"
-    status, _ = http(host, path, method, body, headers)
+    status, _, _ = http(host, path, method, body, headers)
     assert status == 401
 
 
+@pytest.mark.parametrize("method,path", TOKEN_ROUTES + [
+    ("POST", "/restore"), ("POST", "/updateFirmware"), ("POST", "/updateApplication")])
+def test_secured_refuses_a_web_asset_name_below_a_route(host, secured, method, path):
+    """/backup/a.js once reached /backup and passed the token check as a web asset.
+
+    The uploads are sent without a body: one that got through would reboot
+    the device.
+    """
+    if not secured:
+        pytest.skip("the device's security is None")
+    body = b"{}" if method == "PUT" else None
+    status, _, _ = http(host, path + "/a.js", method, body, {"Content-Type": "application/json"} if body else None)
+    assert status in (401, 404)
+
+
 def test_secured_accepts_the_token_on_a_route(host, secured, token):
     if not secured:
         pytest.skip("the device's security is None")
-    status, _ = http(host, "/backup", headers={"apikey": token})
+    status, _, _ = http(host, "/backup", headers={"apikey": token})
     assert status == 200
 
 
 @pytest.mark.parametrize("path", ["/restore", "/updateFirmware"])
-def test_secured_drops_an_upload_without_a_token(host, secured, token, path):
+def test_secured_refuses_an_upload_without_a_token(host, secured, path):
-    """The token check runs once the body is in, so the uploads check it on their first chunk.
+    """/updateApplication isn't sent: one that got through would write over the file system."""
+    if not secured:
+        pytest.skip("the device's security is None")
+    status, _, _ = upload(host, path)
+    assert status == 401
 
-    An /updateFirmware that got through would stop the radio and reboot the
+
-    device once the connection closed.  /updateApplication isn't sent: one
+def restore_without_a_file(host, token):
-    that got through would write over the file system.
+    status, _, body = upload(host, "/restore", {"apikey": token}, with_file=False)
+    return status, json.loads(body or b"{}").get("desc", "")
+
+
+def test_secured_drops_the_chunks_of_an_upload_without_a_token(host, secured, token):
+    """The token check runs once the body is in, so /restore's chunks check the token themselves.
+
+    A chunk that got through would mark the upload complete.  /restore with
+    the token and no file would then find the refused file and call it
+    invalid, instead of saying no upload completed.  Neither restores anything.
     """
     if not secured:
         pytest.skip("the device's security is None")
-    before = heap(host, token)
+    status, desc = restore_without_a_file(host, token)
-    status, _ = upload(host, path)
+    if status != 400 or "didn't complete" not in desc:
+        pytest.skip("an earlier upload is still marked complete: reboot the device")
+    status, _, _ = upload(host, "/restore")
     assert status == 401
-    after = heap(host, token)
+    status, desc = restore_without_a_file(host, token)
-    assert after["uptime"] >= before["uptime"], "the device rebooted"
+    assert status == 400
+    assert "didn't complete" in desc, desc
 
 
 @pytest.mark.parametrize("path", PUBLIC_ROUTES)
 def test_public_routes_need_no_token(host, path):
-    status, _ = http(host, path)
+    status, _, _ = http(host, path)
     assert status == 200
 
 
 @pytest.mark.parametrize("path", SHADE_FILES)
 def test_the_shade_files_are_not_served(host, path):
     """Their rolling codes leave the device only through /backup."""
-    status, _ = http(host, path)
+    status, _, _ = http(host, path)
     assert status in (401, 404)
 
 
 def test_replies_carry_no_cors_headers(host):
-    _, headers = http(host, "/loginContext", headers={"Origin": "http://example.com"})
+    _, headers, _ = http(host, "/loginContext", headers={"Origin": "http://example.com"})
     assert headers.get("Access-Control-Allow-Origin") is None
 
 

test/test_httpaccess/test_main.cpp

@@ -34,6 +34,11 @@ static void test_a_path_leaving_the_root_is_not_a_web_asset() {
   TEST_ASSERT_FALSE(isWebAsset("/../shades.cfg.js"));
   TEST_ASSERT_FALSE(isWebAsset("/x/../index.js"));
 }
+static void test_a_file_below_the_root_is_not_a_web_asset() {
+  TEST_ASSERT_FALSE(isWebAsset("/backup/a.js"));
+  TEST_ASSERT_FALSE(isWebAsset("/js/index.js"));
+  TEST_ASSERT_FALSE(isWebAsset("//index.js"));
+}
 static void test_a_missing_or_relative_path_is_not_a_web_asset() {
   TEST_ASSERT_FALSE(isWebAsset(nullptr));
   TEST_ASSERT_FALSE(isWebAsset(""));
@@ -71,6 +76,12 @@ static void test_a_route_differing_in_case_or_length_needs_the_token() {
   TEST_ASSERT_FALSE(isPublicRoute("/logins", false));
   TEST_ASSERT_FALSE(isPublicRoute("/ws/", false));
 }
+static void test_a_web_asset_name_below_a_route_needs_the_token() {
+  TEST_ASSERT_FALSE(isPublicRoute("/backup/a.js", false));
+  TEST_ASSERT_FALSE(isPublicRoute("/shadeCommand/a.png", false));
+  TEST_ASSERT_FALSE(isPublicRoute("/updateFirmware/x.js", false));
+  TEST_ASSERT_FALSE(isPublicRoute("/login/a.js", false));
+}
 static void test_a_preflight_is_public() {
   TEST_ASSERT_TRUE(isPublicRoute("/backup", true));
 }
@@ -118,6 +129,7 @@ int main(int, char **) {
   RUN_TEST(test_a_compressed_file_is_not_asked_for_by_name);
   RUN_TEST(test_a_type_alone_is_not_a_web_asset);
   RUN_TEST(test_a_path_leaving_the_root_is_not_a_web_asset);
+  RUN_TEST(test_a_file_below_the_root_is_not_a_web_asset);
   RUN_TEST(test_a_missing_or_relative_path_is_not_a_web_asset);
   RUN_TEST(test_logging_in_and_ssdp_are_public);
   RUN_TEST(test_the_socket_checks_its_own_token);
@@ -125,6 +137,7 @@ int main(int, char **) {
   RUN_TEST(test_the_other_routes_need_the_token);
   RUN_TEST(test_an_unknown_path_needs_the_token);
   RUN_TEST(test_a_route_differing_in_case_or_length_needs_the_token);
+  RUN_TEST(test_a_web_asset_name_below_a_route_needs_the_token);
   RUN_TEST(test_a_preflight_is_public);
   RUN_TEST(test_the_page_the_device_served_matches);
   RUN_TEST(test_https_behind_a_proxy_matches);