From 5c567b0a72271a00f8185836b6ba2c5c533989db Mon Sep 17 00:00:00 2001 From: jpirnay Date: Mon, 13 Apr 2026 11:15:20 +0200 Subject: [PATCH 1/4] Address upstream review comments --- lib/KOReaderSync/ChapterXPathIndexer.cpp | 11 ++++++++++- lib/KOReaderSync/ChapterXPathIndexerInternal.cpp | 4 ++-- lib/KOReaderSync/KOReaderSyncClient.cpp | 15 ++++++++++++++- lib/KOReaderSync/ProgressMapper.cpp | 16 ++++++---------- 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/lib/KOReaderSync/ChapterXPathIndexer.cpp b/lib/KOReaderSync/ChapterXPathIndexer.cpp index 61e5ce74..98eb8053 100644 --- a/lib/KOReaderSync/ChapterXPathIndexer.cpp +++ b/lib/KOReaderSync/ChapterXPathIndexer.cpp @@ -77,11 +77,20 @@ bool ChapterXPathIndexer::tryExtractParagraphIndexFromXPath(const std::string& x } const std::string pKey = "/p["; - const size_t pos = normalized.find(pKey, secondBody != std::string::npos ? secondBody : 0); + const size_t searchStart = secondBody != std::string::npos ? secondBody : 0; + const size_t pos = normalized.find(pKey, searchStart); if (pos == std::string::npos) { return false; } + // Only accept p[...] that is a direct child of the /body segment — reject + // paths with intermediate ancestor segments (e.g. /body/.../div[4]/p[1]) + // which would collapse structurally different locations to the same index. + const size_t bodyEnd = (secondBody != std::string::npos ? secondBody : 0) + bodyKey.size(); + if (pos != bodyEnd) { + return false; + } + const size_t start = pos + pKey.size(); size_t end = start; while (end < normalized.size() && std::isdigit(static_cast(normalized[end]))) { diff --git a/lib/KOReaderSync/ChapterXPathIndexerInternal.cpp b/lib/KOReaderSync/ChapterXPathIndexerInternal.cpp index d622be64..94dfa83b 100644 --- a/lib/KOReaderSync/ChapterXPathIndexerInternal.cpp +++ b/lib/KOReaderSync/ChapterXPathIndexerInternal.cpp @@ -342,9 +342,9 @@ size_t countTotalTextBytes(const std::string& tmpPath) { XML_SetElementHandler(parser, bcStart, bcEnd); XML_SetCharacterDataHandler(parser, bcChar); XML_SetDefaultHandlerExpand(parser, bcDefault); - runParse(parser, tmpPath); + const bool ok = runParse(parser, tmpPath); XML_ParserFree(parser); - return state.totalTextBytes; + return ok ? state.totalTextBytes : 0; } } // namespace ChapterXPathIndexerInternal diff --git a/lib/KOReaderSync/KOReaderSyncClient.cpp b/lib/KOReaderSync/KOReaderSyncClient.cpp index 2e60ce73..f40af195 100644 --- a/lib/KOReaderSync/KOReaderSyncClient.cpp +++ b/lib/KOReaderSync/KOReaderSyncClient.cpp @@ -576,7 +576,20 @@ KOReaderSyncClient::Error KOReaderSyncClient::updateProgress(const KOReaderProgr if (err != ESP_OK) return NETWORK_ERROR; if (httpCode >= 300 && httpCode < 400) return REDIRECT_ERROR; - if (httpCode == 200 || httpCode == 202) return OK; + if (httpCode == 200 || httpCode == 202) { + // Guard against a reverse proxy or captive portal returning HTTP 200 + HTML + // instead of the real API response, consistent with authenticate()'s check. + if (activeBuf->data) { + const char* p = activeBuf->data; + while (*p == ' ' || *p == '\t' || *p == '\r' || *p == '\n') { + p++; + } + if (*p != '\0' && *p != '{') { + return SERVER_ERROR; + } + } + return OK; + } if (httpCode == 401) return AUTH_FAILED; return SERVER_ERROR; } diff --git a/lib/KOReaderSync/ProgressMapper.cpp b/lib/KOReaderSync/ProgressMapper.cpp index dd70bc50..4d1df0dc 100644 --- a/lib/KOReaderSync/ProgressMapper.cpp +++ b/lib/KOReaderSync/ProgressMapper.cpp @@ -58,16 +58,12 @@ KOReaderPosition ProgressMapper::toKOReader(const std::shared_ptr& epub, c result.percentage = epub->calculateProgress(pos.spineIndex, intraSpineProgress); // Generate XPath for the current position. - // Prefer paragraph index from the section cache LUT (exact element mapping) over - // byte-offset estimation (which can drift in chapters with non-uniform content density). - if (pos.hasParagraphIndex && pos.paragraphIndex > 0) { - result.xpath = "/body/DocFragment[" + std::to_string(pos.spineIndex + 1) + "]/body/p[" + - std::to_string(pos.paragraphIndex) + "]"; - } else { - result.xpath = ChapterXPathIndexer::findXPathForProgress(epub, pos.spineIndex, intraSpineProgress); - if (result.xpath.empty()) { - result.xpath = generateXPath(pos.spineIndex); - } + // Always use the indexer which SAX-parses the actual XHTML to find the correct + // element path — a naive "/body/DocFragment[N]/body/p[M]" would assume paragraphs + // are direct children of , which breaks for wrapped chapters (e.g. div/section). + result.xpath = ChapterXPathIndexer::findXPathForProgress(epub, pos.spineIndex, intraSpineProgress); + if (result.xpath.empty()) { + result.xpath = generateXPath(pos.spineIndex); } // Get chapter info for logging From 03b3e1e5b72ca3b0c8281a356c74a93526f2ee1c Mon Sep 17 00:00:00 2001 From: jpirnay Date: Mon, 13 Apr 2026 11:32:19 +0200 Subject: [PATCH 2/4] Provide a clearer message for TLS 1.3 cases --- lib/KOReaderSync/KOReaderSyncClient.cpp | 13 +++++++++++-- src/activities/reader/KOReaderSyncActivity.cpp | 10 +++++++++- .../settings/KOReaderAuthActivity.cpp | 17 ++++++++++++++++- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/lib/KOReaderSync/KOReaderSyncClient.cpp b/lib/KOReaderSync/KOReaderSyncClient.cpp index f40af195..b537d2e5 100644 --- a/lib/KOReaderSync/KOReaderSyncClient.cpp +++ b/lib/KOReaderSync/KOReaderSyncClient.cpp @@ -609,8 +609,17 @@ const char* KOReaderSyncClient::lastFailureDetail() { } // Network/TLS case: esp_http_client_perform() failed before getting a status code. if (lastHttpCode == 0 && lastEspError != 0) { - snprintf(g_failureDetailBuf, sizeof(g_failureDetailBuf), "%s: %s (heap %u/%u contig)", lastOperation, - esp_err_to_name(lastEspError), lastHeapAtFailure, lastContigHeapAtFailure); + // Detect TLS handshake failures on HTTPS URLs — likely caused by the server + // requiring TLS 1.3 which the ESP32 mbedTLS library does not support. + if (lastEspError == ESP_ERR_HTTP_CONNECT && KOREADER_STORE.getBaseUrl().rfind("https", 0) == 0) { + snprintf( + g_failureDetailBuf, sizeof(g_failureDetailBuf), + "%s: TLS handshake failed. Server may require TLS 1.3 (unsupported). Try a different server or use HTTP.", + lastOperation); + } else { + snprintf(g_failureDetailBuf, sizeof(g_failureDetailBuf), "%s: %s (heap %u/%u contig)", lastOperation, + esp_err_to_name(lastEspError), lastHeapAtFailure, lastContigHeapAtFailure); + } return g_failureDetailBuf; } // Server case: got an HTTP status the client didn't recognize as success. diff --git a/src/activities/reader/KOReaderSyncActivity.cpp b/src/activities/reader/KOReaderSyncActivity.cpp index 754c5d9a..522080c9 100644 --- a/src/activities/reader/KOReaderSyncActivity.cpp +++ b/src/activities/reader/KOReaderSyncActivity.cpp @@ -529,7 +529,15 @@ void KOReaderSyncActivity::render(RenderLock&&) { if (state == SYNC_FAILED) { renderer.drawCenteredText(UI_10_FONT_ID, 280, tr(STR_SYNC_FAILED_MSG), true, EpdFontFamily::BOLD); - renderer.drawCenteredText(UI_10_FONT_ID, 320, statusMessage.c_str()); + + // Word-wrap the detail message so long TLS/network diagnostics aren't clipped. + const int lineHeight = renderer.getLineHeight(UI_10_FONT_ID); + const auto lines = renderer.wrappedText(UI_10_FONT_ID, statusMessage.c_str(), contentRect.width - 20, 4); + int y = 320; + for (const auto& line : lines) { + renderer.drawCenteredText(UI_10_FONT_ID, y, line.c_str()); + y += lineHeight; + } const auto labels = mappedInput.mapLabels(tr(STR_BACK), "", "", ""); GUI.drawButtonHints(renderer, labels.btn1, labels.btn2, labels.btn3, labels.btn4); diff --git a/src/activities/settings/KOReaderAuthActivity.cpp b/src/activities/settings/KOReaderAuthActivity.cpp index e2733a91..6058b944 100644 --- a/src/activities/settings/KOReaderAuthActivity.cpp +++ b/src/activities/settings/KOReaderAuthActivity.cpp @@ -53,6 +53,11 @@ void KOReaderAuthActivity::performAuthentication() { } else { state = FAILED; errorMessage = KOReaderSyncClient::errorString(result); + const char* detail = KOReaderSyncClient::lastFailureDetail(); + if (detail && detail[0]) { + errorMessage += " — "; + errorMessage += detail; + } } } requestUpdate(); @@ -72,6 +77,11 @@ void KOReaderAuthActivity::performRegistration() { } else { state = FAILED; errorMessage = KOReaderSyncClient::errorString(result); + const char* detail = KOReaderSyncClient::lastFailureDetail(); + if (detail && detail[0]) { + errorMessage += " — "; + errorMessage += detail; + } } } requestUpdate(); @@ -120,7 +130,12 @@ void KOReaderAuthActivity::render(RenderLock&&) { } else if (state == FAILED) { const char* failedMsg = (mode == Mode::REGISTER) ? tr(STR_REGISTER_FAILED) : tr(STR_AUTH_FAILED); renderer.drawCenteredText(UI_10_FONT_ID, top, failedMsg, true, EpdFontFamily::BOLD); - renderer.drawCenteredText(UI_10_FONT_ID, top + height + 10, errorMessage.c_str()); + const auto lines = renderer.wrappedText(UI_10_FONT_ID, errorMessage.c_str(), contentRect.width - 20, 4); + int y = top + height + 10; + for (const auto& line : lines) { + renderer.drawCenteredText(UI_10_FONT_ID, y, line.c_str()); + y += height; + } } const auto labels = mappedInput.mapLabels(tr(STR_BACK), "", "", ""); From 398a25c4177cc7c6b469593264c13d06758d222f Mon Sep 17 00:00:00 2001 From: jpirnay Date: Mon, 13 Apr 2026 11:38:52 +0200 Subject: [PATCH 3/4] Review comment --- lib/KOReaderSync/KOReaderSyncClient.cpp | 15 ++++++++++++--- lib/KOReaderSync/KOReaderSyncClient.h | 3 ++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/KOReaderSync/KOReaderSyncClient.cpp b/lib/KOReaderSync/KOReaderSyncClient.cpp index b537d2e5..e0c81f1d 100644 --- a/lib/KOReaderSync/KOReaderSyncClient.cpp +++ b/lib/KOReaderSync/KOReaderSyncClient.cpp @@ -383,14 +383,14 @@ KOReaderSyncClient::Error KOReaderSyncClient::authenticate() { // Skip leading whitespace before checking for '{' so servers that emit // a BOM or indent their JSON don't get incorrectly rejected. if (!activeBuf->data) { - return SERVER_ERROR; + return INVALID_RESPONSE; } const char* p = activeBuf->data; while (*p == ' ' || *p == '\t' || *p == '\r' || *p == '\n') { p++; } if (*p != '{') { - return SERVER_ERROR; + return INVALID_RESPONSE; } return OK; } @@ -585,7 +585,7 @@ KOReaderSyncClient::Error KOReaderSyncClient::updateProgress(const KOReaderProgr p++; } if (*p != '\0' && *p != '{') { - return SERVER_ERROR; + return INVALID_RESPONSE; } } return OK; @@ -622,6 +622,13 @@ const char* KOReaderSyncClient::lastFailureDetail() { } return g_failureDetailBuf; } + // Invalid-response case: HTTP 200/202 but body was not JSON (e.g. captive portal HTML). + // On real success callers never reach lastFailureDetail(), so a 2xx here means INVALID_RESPONSE. + if ((lastHttpCode == 200 || lastHttpCode == 202) && lastEspError == ESP_OK) { + snprintf(g_failureDetailBuf, sizeof(g_failureDetailBuf), + "%s: expected JSON but received HTML (captive portal or proxy?)", lastOperation); + return g_failureDetailBuf; + } // Server case: got an HTTP status the client didn't recognize as success. if (lastHttpCode != 0) { snprintf(g_failureDetailBuf, sizeof(g_failureDetailBuf), "%s: HTTP %d", lastOperation, lastHttpCode); @@ -654,6 +661,8 @@ const char* KOReaderSyncClient::errorString(Error error) { return "Registration is disabled on this server"; case REDIRECT_ERROR: return "Server redirected (check server URL)"; + case INVALID_RESPONSE: + return "Unexpected response (check server URL)"; default: return "Unknown error"; } diff --git a/lib/KOReaderSync/KOReaderSyncClient.h b/lib/KOReaderSync/KOReaderSyncClient.h index eef7f875..8ffa0203 100644 --- a/lib/KOReaderSync/KOReaderSyncClient.h +++ b/lib/KOReaderSync/KOReaderSyncClient.h @@ -40,7 +40,8 @@ class KOReaderSyncClient { NOT_FOUND, USER_EXISTS, REGISTRATION_DISABLED, - REDIRECT_ERROR + REDIRECT_ERROR, + INVALID_RESPONSE }; /** From b907aa8ff77a9ce794ba97e20257c65ac5f9d910 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Mon, 13 Apr 2026 11:48:02 +0200 Subject: [PATCH 4/4] Minor adjustment for BOM strings --- lib/KOReaderSync/KOReaderSyncClient.cpp | 52 ++++++++++++------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/KOReaderSync/KOReaderSyncClient.cpp b/lib/KOReaderSync/KOReaderSyncClient.cpp index e0c81f1d..2a9d9a31 100644 --- a/lib/KOReaderSync/KOReaderSyncClient.cpp +++ b/lib/KOReaderSync/KOReaderSyncClient.cpp @@ -42,6 +42,21 @@ void beginRequest(const char* operation) { KOReaderSyncClient::lastContigHeapAtFailure = heap_caps_get_largest_free_block(MALLOC_CAP_8BIT | MALLOC_CAP_DEFAULT); } +// Skip a leading UTF-8 BOM (EF BB BF) and ASCII whitespace, returning a pointer +// to the first content character. Used by response-body checks that verify the +// payload starts with '{' (JSON) rather than '<' (HTML captive-portal page). +const char* skipBomAndWhitespace(const char* p) { + // UTF-8 BOM + if (static_cast(p[0]) == 0xEF && static_cast(p[1]) == 0xBB && + static_cast(p[2]) == 0xBF) { + p += 3; + } + while (*p == ' ' || *p == '\t' || *p == '\r' || *p == '\n') { + p++; + } + return p; +} + // Device identifier for CrossPoint reader constexpr char DEVICE_NAME[] = "CrossPoint"; constexpr char DEVICE_ID[] = "crosspoint-reader"; @@ -377,19 +392,8 @@ KOReaderSyncClient::Error KOReaderSyncClient::authenticate() { if (err != ESP_OK) return NETWORK_ERROR; if (httpCode >= 300 && httpCode < 400) return REDIRECT_ERROR; if (httpCode == 200) { - // The kosync auth response is always a JSON object. Guard against a reverse - // proxy returning HTTP 200 + HTML (e.g. a login wall that followed all - // redirects and landed on an auth page instead of the API endpoint). - // Skip leading whitespace before checking for '{' so servers that emit - // a BOM or indent their JSON don't get incorrectly rejected. - if (!activeBuf->data) { - return INVALID_RESPONSE; - } - const char* p = activeBuf->data; - while (*p == ' ' || *p == '\t' || *p == '\r' || *p == '\n') { - p++; - } - if (*p != '{') { + // Guard against a reverse proxy or captive portal returning HTTP 200 + HTML. + if (!activeBuf->data || *skipBomAndWhitespace(activeBuf->data) != '{') { return INVALID_RESPONSE; } return OK; @@ -577,14 +581,10 @@ KOReaderSyncClient::Error KOReaderSyncClient::updateProgress(const KOReaderProgr if (err != ESP_OK) return NETWORK_ERROR; if (httpCode >= 300 && httpCode < 400) return REDIRECT_ERROR; if (httpCode == 200 || httpCode == 202) { - // Guard against a reverse proxy or captive portal returning HTTP 200 + HTML - // instead of the real API response, consistent with authenticate()'s check. + // Guard against a reverse proxy or captive portal returning HTTP 200 + HTML. if (activeBuf->data) { - const char* p = activeBuf->data; - while (*p == ' ' || *p == '\t' || *p == '\r' || *p == '\n') { - p++; - } - if (*p != '\0' && *p != '{') { + const char c = *skipBomAndWhitespace(activeBuf->data); + if (c != '\0' && c != '{') { return INVALID_RESPONSE; } } @@ -609,13 +609,13 @@ const char* KOReaderSyncClient::lastFailureDetail() { } // Network/TLS case: esp_http_client_perform() failed before getting a status code. if (lastHttpCode == 0 && lastEspError != 0) { - // Detect TLS handshake failures on HTTPS URLs — likely caused by the server - // requiring TLS 1.3 which the ESP32 mbedTLS library does not support. + // HTTPS connect failures can be TLS version issues (ESP32 only supports up + // to TLS 1.2), but also DNS, cert, or plain network problems. Include the + // error name and heap stats so the user/bug-report has enough to triage. if (lastEspError == ESP_ERR_HTTP_CONNECT && KOREADER_STORE.getBaseUrl().rfind("https", 0) == 0) { - snprintf( - g_failureDetailBuf, sizeof(g_failureDetailBuf), - "%s: TLS handshake failed. Server may require TLS 1.3 (unsupported). Try a different server or use HTTP.", - lastOperation); + snprintf(g_failureDetailBuf, sizeof(g_failureDetailBuf), + "%s: connect failed — check network, DNS, certs, or TLS 1.2 compat (heap %u/%u contig)", lastOperation, + lastHeapAtFailure, lastContigHeapAtFailure); } else { snprintf(g_failureDetailBuf, sizeof(g_failureDetailBuf), "%s: %s (heap %u/%u contig)", lastOperation, esp_err_to_name(lastEspError), lastHeapAtFailure, lastContigHeapAtFailure);