Merge pull request #72 from jpirnay/fix-kosyncreview

fix: Address upstream review comments

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<unsigned char>(normalized[end]))) {

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

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<unsigned char>(p[0]) == 0xEF && static_cast<unsigned char>(p[1]) == 0xBB &&
+      static_cast<unsigned char>(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,20 +392,9 @@ 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 SERVER_ERROR;
-    }
-    const char* p = activeBuf->data;
-    while (*p == ' ' || *p == '\t' || *p == '\r' || *p == '\n') {
-      p++;
-    }
-    if (*p != '{') {
-      return SERVER_ERROR;
+    // Guard against a reverse proxy or captive portal returning HTTP 200 + HTML.
+    if (!activeBuf->data || *skipBomAndWhitespace(activeBuf->data) != '{') {
+      return INVALID_RESPONSE;
     }
     return OK;
   }
@@ -576,7 +580,16 @@ 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.
+    if (activeBuf->data) {
+      const char c = *skipBomAndWhitespace(activeBuf->data);
+      if (c != '\0' && c != '{') {
+        return INVALID_RESPONSE;
+      }
+    }
+    return OK;
+  }
   if (httpCode == 401) return AUTH_FAILED;
   return SERVER_ERROR;
 }
@@ -596,8 +609,24 @@ const char* KOReaderSyncClient::lastFailureDetail() {
   }
   // Network/TLS case: esp_http_client_perform() failed before getting a status code.
   if (lastHttpCode == 0 && lastEspError != 0) {
+    // 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: 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);
+    }
+    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.
@@ -632,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";
   }

lib/KOReaderSync/KOReaderSyncClient.h

@@ -40,7 +40,8 @@ class KOReaderSyncClient {
     NOT_FOUND,
     USER_EXISTS,
     REGISTRATION_DISABLED,
-    REDIRECT_ERROR
+    REDIRECT_ERROR,
+    INVALID_RESPONSE
   };
 
   /**

lib/KOReaderSync/ProgressMapper.cpp

@@ -58,17 +58,13 @@ KOReaderPosition ProgressMapper::toKOReader(const std::shared_ptr<Epub>& 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 {
+  // 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 <body>, 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
   const int tocIndex = epub->getTocIndexForSpineIndex(pos.spineIndex);

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);

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), "", "", "");