From dc44311e49676853adbc2c9a52ab2752dfbae019 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 18 Jul 2026 14:25:56 -0700 Subject: [PATCH] feat(webconfig): validate request IDs and improve error handling Add validation for request IDs in the web configuration server to ensure they conform to the expected format. Enhance error responses for invalid or unknown request IDs, improving the robustness of request handling. Update the web UI to reflect these changes, ensuring that clients can properly handle errors related to request ID mismatches. --- scripts/webconfig_mock_server.py | 20 +- src/helpers/WebConfigKeys.h | 15 ++ src/helpers/esp32/WebConfigServer.cpp | 194 +++++++++++++----- src/helpers/esp32/WebConfigServer.h | 42 ++-- .../test_webconfig_keys.cpp | 17 ++ webui/index.html | 7 +- 6 files changed, 218 insertions(+), 77 deletions(-) diff --git a/scripts/webconfig_mock_server.py b/scripts/webconfig_mock_server.py index aaa60ee54..f286ab721 100644 --- a/scripts/webconfig_mock_server.py +++ b/scripts/webconfig_mock_server.py @@ -31,6 +31,7 @@ import secrets import threading import time from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from urllib.parse import parse_qs, urlsplit HERE = os.path.dirname(os.path.abspath(__file__)) INDEX_HTML = os.path.join(HERE, "..", "webui", "index.html") @@ -267,6 +268,10 @@ def is_secret_key(key): return key == "wifi.pwd" or bool(re.match(r"^mqtt[1-6]\.(password|token)$", key)) +def valid_reqid(reqid): + return isinstance(reqid, str) and bool(re.fullmatch(r"[0-9A-Fa-f]{16}", reqid)) + + # --------------------------------------------------------------------------- # HTTP handler # --------------------------------------------------------------------------- @@ -383,10 +388,17 @@ class Handler(BaseHTTPRequestHandler): except ValueError: return self._json(400, {"error": "bad json"}) reqid = body.get("reqid", "") + if not valid_reqid(reqid): + return self._json(400, {"error": "bad reqid"}) reboot = bool(body.get("reboot", False)) setmap = body.get("set", {}) or {} with ST.lock: + if ST.batch.get("state") != "idle" and ST.batch.get("reqid") == reqid: + return self._json(202, { + "state": ST.batch["state"], "count": len(ST.batch.get("results", [])), + "reqid": reqid, + }) if ST.batch.get("state") == "pending": return self._json(409, {"error": "busy", "reqid": ST.batch.get("reqid", "")}) # drop unchanged secrets (sentinel), like the firmware does @@ -407,10 +419,16 @@ class Handler(BaseHTTPRequestHandler): return self._json(202, {"state": "pending", "count": len(entries), "reqid": reqid}) def _config_result(self): + query = parse_qs(urlsplit(self.path).query) + reqid = query.get("reqid", [""])[0] + if not valid_reqid(reqid): + return self._json(400, {"error": "bad reqid"}) with ST.lock: b = ST.batch if b.get("state") == "idle": - return self._json(200, {"state": "idle"}) + return self._json(200, {"state": "idle", "reqid": reqid}) + if b.get("reqid") != reqid: + return self._json(404, {"error": "unknown request"}) if b["state"] == "pending" and time.time() < b["done_at"]: return self._json(200, {"state": "pending", "reqid": b["reqid"]}) b["state"] = "done" # stays readable until next POST diff --git a/src/helpers/WebConfigKeys.h b/src/helpers/WebConfigKeys.h index f382309b3..0b1f088cb 100644 --- a/src/helpers/WebConfigKeys.h +++ b/src/helpers/WebConfigKeys.h @@ -60,3 +60,18 @@ static inline bool wcIsSecretKey(const char* key) { && (strcmp(&key[6], "password") == 0 || strcmp(&key[6], "token") == 0)) return true; return false; } + +// Browser-generated request IDs are exactly eight random bytes encoded as +// hexadecimal. Keeping the grammar deliberately small makes the ID safe to +// echo in JSON/logs and prevents an empty or truncated ID from weakening the +// save/result correlation contract. +static inline bool wcIsValidReqId(const char* reqid) { + if (reqid == NULL || strlen(reqid) != 16) return false; + for (size_t i = 0; i < 16; i++) { + char c = reqid[i]; + if (!((c >= '0' && c <= '9') || + (c >= 'a' && c <= 'f') || + (c >= 'A' && c <= 'F'))) return false; + } + return true; +} diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index e8e7f9ba9..592877a0c 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -47,6 +47,12 @@ struct WCLock { }; WebConfigServer* WebConfigServer::_active = NULL; +AsyncWebServer* WebConfigServer::_host = NULL; + +// Protects the permanent route host's active-session pointer and handler +// references across the loop and async_tcp cores. The critical sections only +// copy a pointer/update a counter; handlers themselves never run under it. +static portMUX_TYPE s_wc_route_mux = portMUX_INITIALIZER_UNLOCKED; WebConfigServer::WebConfigServer(NodePrefs* prefs, MQTTPrefs* obs, Callbacks* callbacks, const uint8_t* pub_key, const char* fw_ver, @@ -54,11 +60,10 @@ WebConfigServer::WebConfigServer(NodePrefs* prefs, MQTTPrefs* obs, Callbacks* ca : _prefs(prefs), _obs(obs), _cb(callbacks), _pub_key(pub_key), _fw_ver(fw_ver), _role(role), _board_name(board_name) { _mux = xSemaphoreCreateMutex(); - _active = this; } WebConfigServer::~WebConfigServer() { - if (_active == this) _active = NULL; + detachRoutes(); if (_mux) vSemaphoreDelete(_mux); } @@ -120,8 +125,8 @@ bool WebConfigServer::startSetupMode(char reply[]) { _dns = new DNSServer(); _dns->start(53, "*", ip); // captive portal: every name resolves to us - createServer(); _mode = MODE_SETUP; + createServer(); _was_setup_ap = true; _last_activity = millis(); WiFi.scanNetworks(true); // pre-populate the SSID picker @@ -139,8 +144,8 @@ bool WebConfigServer::startLanMode(char reply[]) { strcpy(reply, "Err: WiFi not connected"); return false; } - createServer(); _mode = MODE_LAN; + createServer(); _last_activity = millis(); int pos = sprintf(reply, "WebConfig started: http://%s/ (admin password login)", @@ -152,7 +157,13 @@ bool WebConfigServer::startLanMode(char reply[]) { } void WebConfigServer::createServer() { - _server = new AsyncWebServer(80); + if (_host == NULL) { + _host = new AsyncWebServer(80); + _server = _host; + registerRoutes(); + } else { + _server = _host; + } // iOS caches plain-HTTP GETs aggressively (keyed by URL, surviving even a // device reflash behind the same IP), which poisons /api/config/result and // friends with stale responses from earlier sessions. Forbid caching on @@ -167,38 +178,43 @@ void WebConfigServer::createServer() { DefaultHeaders::Instance().addHeader("Cache-Control", "no-store"); s_default_headers_registered = true; } - registerRoutes(); + attachRoutes(); _server->begin(); } void WebConfigServer::requestStop() { if (_mode == MODE_OFF && !_stopping) return; + // Detach first. Any request whose headers/body finish parsing after this point + // is dispatched by the permanent host as 503 and never sees this session. + detachRoutes(); if (_server) _server->end(); if (_dns) _dns->stop(); _mode = MODE_OFF; _stopping = true; - // Deleting an AsyncWebServer with a live request is a known crash source (the - // route lambdas capture `this`). end() above stops accepting new connections; - // tick() then frees the server once in-flight requests have drained (see - // _inflight), bounded by a settle window and a hard cap. - uint32_t now = millis(); - _delete_at = now + STOP_SETTLE_MS; - if (_delete_at == 0) _delete_at = 1; - _delete_deadline = now + STOP_MAX_WAIT_MS; - if (_delete_deadline == 0) _delete_deadline = 1; + uint32_t warn_at = millis() + STOP_WARN_MS; + _stop_warn_at = warn_at ? warn_at : 1; + _stop_warned = false; } void WebConfigServer::finalizeTeardown() { - // Design note: we free the server on stop rather than keeping one persistent - // instance for the firmware lifetime. A persistent server would avoid this - // delete entirely (no UAF window, no start/stop heap churn), but the route - // lambdas capture `this`, so keeping them alive means keeping the whole - // ~10 KB WebConfigServer (the _batch[] array dominates) resident forever on - // any node that ever opened the portal — on top of AsyncTCP's ~16 KB worker - // task, which is never reclaimed either way. That permanent heap cost was - // ruled out (fleet includes tight non-PSRAM boards); instead we free on stop - // and gate the delete on _inflight so a live request is never freed under it. - delete _server; + // `_host` and its routes are process-lifetime. AsyncWebServerRequest retains + // its server pointer until disconnect, so deleting the host here cannot be + // made safe using route-level request counts. Only the per-run session is + // reclaimed by MyMesh after this method reports completion. + // + // Design note (reverses the earlier free-on-stop approach): stop used to + // `delete _server` here once an in-flight request count hit zero, gated by a + // settle window and a hard cap. That could never be race-free - the hard cap + // freed the server even with a request still in flight, and because the + // request holds the server pointer until it disconnects, no request count can + // close that window. The disconnect UAF that motivated this file lived + // exactly there. We now keep the AsyncWebServer resident for the firmware + // lifetime (a small, bounded permanent cost: the server object + route table; + // the ~10 KB session with its _batch[] array is still reclaimed, and the + // listener socket is released by _server->end() on stop) and route requests + // through the currently attached session, so nothing a live request points at + // is ever freed. + _server = NULL; delete _dns; _dns = NULL; @@ -214,9 +230,8 @@ void WebConfigServer::finalizeTeardown() { _was_setup_ap = false; } _stopping = false; - _delete_at = 0; - _delete_deadline = 0; - _inflight = 0; + _stop_warn_at = 0; + _stop_warned = false; _reboot_at = 0; _batch_state = BATCH_IDLE; _batch_next = 0; @@ -228,13 +243,13 @@ void WebConfigServer::finalizeTeardown() { void WebConfigServer::tick(uint32_t now) { if (_stopping) { - if (_delete_at) { - // Free once the settle window has passed AND nothing is in flight, or - // unconditionally at the hard cap. The settle window lets any request - // already queued when we closed the listener get counted first. - bool settled = (int32_t)(now - _delete_at) >= 0; - bool capped = (int32_t)(now - _delete_deadline) >= 0; - if ((settled && _inflight <= 0) || capped) finalizeTeardown(); + uint32_t refs = handlerRefCount(); + if (refs == 0) { + finalizeTeardown(); + } else if (!_stop_warned && _stop_warn_at && (int32_t)(now - _stop_warn_at) >= 0) { + _stop_warned = true; + Serial.printf("WC: stop waiting for %lu handler(s); retaining session safely\n", + (unsigned long)refs); } return; } @@ -329,37 +344,67 @@ static void wcLogReq(AsyncWebServerRequest* r) { Serial.printf("WC: http %s %s\n", r->methodToString(), r->url().c_str()); } -// Called at the top of every route handler (async_tcp task). Counts the request -// as in flight until its connection closes, so requestStop() can wait for the -// last response to finish before freeing the server instead of guessing with a -// fixed delay. Responses send `Connection: close`, so onDisconnect fires as soon -// as the reply is delivered — no lingering keep-alive connections to over-count. -void WebConfigServer::beginRequest(AsyncWebServerRequest* req) { +void WebConfigServer::attachRoutes() { + portENTER_CRITICAL(&s_wc_route_mux); + _active = this; + portEXIT_CRITICAL(&s_wc_route_mux); +} + +void WebConfigServer::detachRoutes() { + portENTER_CRITICAL(&s_wc_route_mux); + if (_active == this) _active = NULL; + portEXIT_CRITICAL(&s_wc_route_mux); +} + +uint32_t WebConfigServer::handlerRefCount() const { + portENTER_CRITICAL(&s_wc_route_mux); + uint32_t refs = _handler_refs; + portEXIT_CRITICAL(&s_wc_route_mux); + return refs; +} + +void WebConfigServer::dispatchRequest(AsyncWebServerRequest* req, RequestHandler handler) { wcLogReq(req); - _inflight++; - req->onDisconnect([this]() { if (_inflight > 0) _inflight--; }); + WebConfigServer* target = NULL; + portENTER_CRITICAL(&s_wc_route_mux); + if (_active != NULL) { + target = _active; + target->_handler_refs++; + } + portEXIT_CRITICAL(&s_wc_route_mux); + + if (target == NULL) { + req->send(503, "application/json", "{\"error\":\"webconfig stopped\"}"); + return; + } + + (target->*handler)(req); + + portENTER_CRITICAL(&s_wc_route_mux); + if (target->_handler_refs > 0) target->_handler_refs--; + portEXIT_CRITICAL(&s_wc_route_mux); } void WebConfigServer::registerRoutes() { - _server->on("/", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleRoot(r); }); - _server->on("/api/status", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleStatus(r); }); - _server->on("/api/presets", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handlePresets(r); }); - _server->on("/api/login", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleLogin(r); }, + _server->on("/", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleRoot); }); + _server->on("/api/status", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleStatus); }); + _server->on("/api/presets", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handlePresets); }); + _server->on("/api/login", HTTP_POST, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleLogin); }, NULL, collectBody); - _server->on("/api/logout", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleLogout(r); }); + _server->on("/api/logout", HTTP_POST, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleLogout); }); // NB: plain-string routes match sub-paths too ("/api/config" matches // "/api/config/result") and handlers run in registration order, so the more // specific route MUST be registered first or it never fires. This was why // save confirmations were lost: result polls were answered with config JSON. - _server->on("/api/config/result", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleConfigResult(r); }); - _server->on("/api/config", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleConfigGet(r); }); - _server->on("/api/config", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleConfigPost(r); }, + _server->on("/api/config/result", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleConfigResult); }); + _server->on("/api/config", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleConfigGet); }); + _server->on("/api/config", HTTP_POST, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleConfigPost); }, NULL, collectBody); - _server->on("/api/stats", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleStats(r); }); - _server->on("/api/scan", HTTP_GET, [this](AsyncWebServerRequest* r) { beginRequest(r); handleScan(r); }); - _server->on("/api/reboot", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handleReboot(r); }); - _server->on("/api/portal/exit", HTTP_POST, [this](AsyncWebServerRequest* r) { beginRequest(r); handlePortalExit(r); }); - _server->onNotFound([this](AsyncWebServerRequest* r) { beginRequest(r); handleNotFound(r); }); + _server->on("/api/stats", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleStats); }); + _server->on("/api/scan", HTTP_GET, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleScan); }); + _server->on("/api/reboot", HTTP_POST, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleReboot); }); + _server->on("/api/portal/exit", HTTP_POST, [](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handlePortalExit); }); + _server->onNotFound([](AsyncWebServerRequest* r) { dispatchRequest(r, &WebConfigServer::handleNotFound); }); } // Accumulate a small JSON body into request->_tempObject (freed automatically @@ -577,11 +622,28 @@ void WebConfigServer::handleConfigPost(AsyncWebServerRequest* req) { } bool reboot_after = doc["reboot"] | false; const char* reqid = doc["reqid"] | ""; + if (!wcIsValidReqId(reqid)) { + req->send(400, "application/json", "{\"error\":\"bad reqid\"}"); + return; + } JsonObject set = doc["set"]; WCLock lock(_mux); // A DONE batch stays readable until the next POST claims the slot, so a // client that lost the result response can re-poll instead of failing. + // Repeating a POST with the same request ID is also idempotent: acknowledge + // the batch already occupying the slot instead of applying its commands a + // second time after an ambiguous network failure. + if (_batch_state != BATCH_IDLE && strcmp(reqid, _batch_reqid) == 0) { + StaticJsonDocument<96> ack; + ack["state"] = _batch_state == BATCH_DONE ? "done" : "pending"; + ack["count"] = _batch_count; + ack["reqid"] = (const char*)_batch_reqid; + String out; + serializeJson(ack, out); + req->send(202, "application/json", out); + return; + } if (_batch_state == BATCH_PENDING) { // Echo the in-flight batch's reqid so the caller can tell its own retry // (same reqid — landed, keep polling) from another client's save. @@ -659,6 +721,15 @@ void WebConfigServer::handleConfigPost(AsyncWebServerRequest* req) { void WebConfigServer::handleConfigResult(AsyncWebServerRequest* req) { if (_mode == MODE_OFF) { Serial.println("WC: result read -> 503 (mode off)"); req->send(503); return; } if (!checkAuth(req)) { Serial.println("WC: result read -> 401"); req->send(401, "application/json", "{\"error\":\"auth\"}"); return; } + if (!req->hasParam("reqid")) { + req->send(400, "application/json", "{\"error\":\"bad reqid\"}"); + return; + } + String requested_reqid = req->getParam("reqid")->value(); + if (!wcIsValidReqId(requested_reqid.c_str())) { + req->send(400, "application/json", "{\"error\":\"bad reqid\"}"); + return; + } // Entry print BEFORE the lock (racy state read is fine for diag): if this // fires but no branch print follows, the handler is blocked on _mux. @@ -666,7 +737,16 @@ void WebConfigServer::handleConfigResult(AsyncWebServerRequest* req) { WCLock lock(_mux); if (_batch_state == BATCH_IDLE) { Serial.println("WC: result read -> idle"); - req->send(200, "application/json", "{\"state\":\"idle\"}"); + StaticJsonDocument<64> idle; + idle["state"] = "idle"; + idle["reqid"] = requested_reqid; + String out; + serializeJson(idle, out); + req->send(200, "application/json", out); + return; + } + if (strcmp(requested_reqid.c_str(), _batch_reqid) != 0) { + req->send(404, "application/json", "{\"error\":\"unknown request\"}"); return; } if (_batch_state == BATCH_PENDING) { diff --git a/src/helpers/esp32/WebConfigServer.h b/src/helpers/esp32/WebConfigServer.h index cc0d09eb9..da261a116 100644 --- a/src/helpers/esp32/WebConfigServer.h +++ b/src/helpers/esp32/WebConfigServer.h @@ -14,6 +14,10 @@ // are marshaled into a single-slot command batch that tick() - called from // MyMesh::loop() on the Arduino loop task - drains through the existing CLI // `set` handlers. Prefs-struct reads and batch state are guarded by _mux. +// The HTTP server/routes live for the firmware lifetime; routes acquire the +// currently attached WebConfigServer session only for the synchronous handler +// call. This lets a stopped session be reclaimed without deleting an +// AsyncWebServer that may still be referenced by a partially received request. // // No persisted state: everything here is RAM-only, so prefs-file layouts // (fleet-critical) are untouched. @@ -54,7 +58,7 @@ public: virtual void onConfigBatchEnd() {} // Fill buf with the stats JSON snapshot. Called from tick() (loop task). virtual void buildStatsJson(char* buf, size_t buf_size) = 0; - // Teardown finished (server + DNS freed, WiFi mode restored). + // Teardown finished (session + DNS freed, WiFi mode restored). virtual void onWebConfigStopped() {} }; @@ -75,7 +79,7 @@ public: bool startSetupMode(char reply[]); // open SoftAP + DNS captive portal bool startLanMode(char reply[]); // bind to existing STA connection - void requestStop(); // stop listening now, free after grace period + void requestStop(); // stop listening and detach this session void tick(uint32_t now); // call every loop iteration Mode mode() const { return _mode; } @@ -85,13 +89,10 @@ public: private: static const int MAX_BATCH = 24; static const size_t MAX_BODY = 4096; - // Teardown timing. requestStop() closes the listener immediately; the server - // object is freed once (a) the settle window has elapsed so any request - // already queued on the async_tcp task has been counted, and (b) no request - // is still in flight — but never later than the hard cap, so a stalled client - // can't pin the server (and its heap) open forever. - static const uint32_t STOP_SETTLE_MS = 2000; - static const uint32_t STOP_MAX_WAIT_MS = 10000; + // A detached session normally drains immediately because handlers are short. + // If one does not, keep the session alive (safe) and emit a diagnostic rather + // than freeing memory still referenced by the async task. + static const uint32_t STOP_WARN_MS = 10000; enum BatchState : uint8_t { BATCH_IDLE = 0, BATCH_PENDING, BATCH_DONE }; struct BatchEntry { char key[24]; // allowlisted `set` key (echoed back to the UI) @@ -115,8 +116,12 @@ private: bool _was_setup_ap = false; char _ap_ssid[33] = {0}; - // Most-recently-created instance, for the display's getSetupInfo() poll. + // Currently attached session, also used by the display's setup-info poll. static WebConfigServer* _active; + // Process-lifetime listener and route table. Requests retain a pointer to the + // AsyncWebServer internally until disconnect, so this object is deliberately + // never deleted during normal firmware operation. + static AsyncWebServer* _host; // Command batch: filled by async_tcp under _mux, drained by tick(). volatile BatchState _batch_state = BATCH_IDLE; @@ -149,19 +154,22 @@ private: volatile uint32_t _last_activity = 0; uint32_t _reboot_at = 0; // 0 = none scheduled - uint32_t _delete_at = 0; // earliest teardown time (settle window ends) - uint32_t _delete_deadline = 0; // hard cap: free even if requests are still in flight - // Requests currently being processed/streamed. Mutated only on the async_tcp - // task (increment in beginRequest, decrement in the per-request onDisconnect), - // read on the loop task in tick(); a 32-bit read is atomic so no lock needed. - volatile int _inflight = 0; + uint32_t _stop_warn_at = 0; + bool _stop_warned = false; + // Number of synchronous route handlers currently using this session. Access + // is serialized by the file-local route spinlock in WebConfigServer.cpp. + uint32_t _handler_refs = 0; volatile uint32_t _stats_wanted_until = 0; uint32_t _stats_built_at = 0; char _stats_json[1024] = {0}; void createServer(); void registerRoutes(); - void beginRequest(AsyncWebServerRequest* req); // log + in-flight tracking + typedef void (WebConfigServer::*RequestHandler)(AsyncWebServerRequest*); + static void dispatchRequest(AsyncWebServerRequest* req, RequestHandler handler); + void attachRoutes(); + void detachRoutes(); + uint32_t handlerRefCount() const; void drainBatch(uint32_t now); void finalizeTeardown(); bool checkAuth(AsyncWebServerRequest* req); diff --git a/test/test_webconfig_keys/test_webconfig_keys.cpp b/test/test_webconfig_keys/test_webconfig_keys.cpp index 6158b9525..098463fcf 100644 --- a/test/test_webconfig_keys/test_webconfig_keys.cpp +++ b/test/test_webconfig_keys/test_webconfig_keys.cpp @@ -98,6 +98,23 @@ TEST(WebConfigKeys, EverySecretKeyIsAlsoAllowed) { } } +// ---- request correlation ------------------------------------------------- + +TEST(WebConfigKeys, AcceptsExactHexRequestIds) { + EXPECT_TRUE(wcIsValidReqId("0123456789abcdef")); + EXPECT_TRUE(wcIsValidReqId("ABCDEF0123456789")); +} + +TEST(WebConfigKeys, RejectsMissingMalformedOrWrongLengthRequestIds) { + EXPECT_FALSE(wcIsValidReqId(NULL)); + EXPECT_FALSE(wcIsValidReqId("")); + EXPECT_FALSE(wcIsValidReqId("0123456789abcde")); + EXPECT_FALSE(wcIsValidReqId("0123456789abcdef0")); + EXPECT_FALSE(wcIsValidReqId("0123456789abcdeg")); + EXPECT_FALSE(wcIsValidReqId("01234567-9abcdef")); + EXPECT_FALSE(wcIsValidReqId("01234567 9abcdef")); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/webui/index.html b/webui/index.html index 16e8fd4cc..6334192f0 100644 --- a/webui/index.html +++ b/webui/index.html @@ -722,12 +722,12 @@ function pollResult(cb,errs,idles,acked,reqid){ errs=errs||0;idles=idles||0; function fail(){$("#save-btn").disabled=false; if(cb)cb(null,acked);else toast(acked?"Saved, but confirmation was lost — reload to verify":"Save failed — check connection and retry")} - api("/api/config/result",{timeout:4000}).then(function(r){ + api("/api/config/result?reqid="+encodeURIComponent(reqid),{timeout:4000}).then(function(r){ // The device keeps the last batch's result readable until a new POST claims // the slot. If the reqid doesn't match ours, this result belongs to another // save (a concurrent client, or a stale DONE from before our POST) — never // consume it as our own success. - if(reqid&&r.reqid&&r.reqid!==reqid){fail();return} + if(r.reqid!==reqid){fail();return} if(r.state==="pending"){setTimeout(function(){pollResult(cb,0,idles,true,reqid)},400);return} if(r.state!=="done"){ // idle: the POST likely never arrived — but give it a moment in case @@ -755,6 +755,9 @@ function pollResult(cb,errs,idles,acked,reqid){ loadConfigSoft(); }).catch(function(e){ if(e.message==="auth"){fail();return} + // A syntactically invalid or unknown ID is a definitive rejection, not a + // transient transport failure. Retrying cannot make it become our batch. + if(e.status===400||e.status===404){fail();return} // Transient network error: the save itself can knock the phone off the // AP for a moment (flash writes stall WiFi). Keep polling — the device // holds the result until we manage to read it.