Review comments

USER_GUIDE.md

@@ -207,9 +207,9 @@ CrossPoint supports saving multiple OPDS servers and switching between them when
 1. Open **Settings -> System -> OPDS Servers**.
 2. Select **Add Server** to create a new entry, or select an existing server to edit it.
 3. Configure these fields:
-  - **Server Name**: Optional display name (for example, "Home Calibre" or "Public Catalog").
-  - **OPDS Server URL**: Full catalog root URL (for Calibre Content Server, usually ends with `/opds`).
-  - **Username / Password**: Optional credentials for authenticated servers.
+   - **Server Name**: Optional display name (for example, "Home Calibre" or "Public Catalog").
+   - **OPDS Server URL**: Full catalog root URL (for Calibre Content Server, usually ends with `/opds`).
+   - **Username / Password**: Optional credentials for authenticated servers.
 4. Use **Delete Server** inside a server entry to remove it.
 
 Behavior notes:

src/OpdsServerStore.cpp

@@ -4,16 +4,49 @@
 #include <JsonSettingsIO.h>
 #include <Logging.h>
 
+#include <cctype>
 #include <cstring>
 
 #include "CrossPointSettings.h"
+#include "util/UrlUtils.h"
 
 OpdsServerStore OpdsServerStore::instance;
 
 namespace {
 constexpr char OPDS_FILE_JSON[] = "/.crosspoint/opds.json";
+
+bool containsWhitespace(const std::string& value) {
+  for (const unsigned char ch : value) {
+    if (std::isspace(ch)) {
+      return true;
+    }
+  }
+  return false;
+}
 }  // namespace
 
+namespace OpdsServerValidation {
+
+std::optional<std::string> normalizeUrl(const std::string& url) {
+  if (url.empty() || containsWhitespace(url)) {
+    return std::nullopt;
+  }
+
+  std::string normalized = url;
+  if (normalized.find("://") == std::string::npos) {
+    normalized = "https://" + normalized;
+  }
+
+  const bool hasHttpScheme = normalized.rfind("http://", 0) == 0 || normalized.rfind("https://", 0) == 0;
+  if (!hasHttpScheme || UrlUtils::extractHostname(normalized).empty()) {
+    return std::nullopt;
+  }
+
+  return normalized;
+}
+
+}  // namespace OpdsServerValidation
+
 bool OpdsServerStore::saveToFile() const {
   Storage.mkdir("/.crosspoint");
   return JsonSettingsIO::saveOpds(*this, OPDS_FILE_JSON);
@@ -22,16 +55,25 @@ bool OpdsServerStore::saveToFile() const {
 bool OpdsServerStore::loadFromFile() {
   if (Storage.exists(OPDS_FILE_JSON)) {
     String json = Storage.readFile(OPDS_FILE_JSON);
-    if (!json.isEmpty()) {
-      // resave flag is set when passwords were stored in plaintext and need re-obfuscation
-      bool resave = false;
-      bool result = JsonSettingsIO::loadOpds(*this, json.c_str(), &resave);
-      if (result && resave) {
-        LOG_DBG("OPS", "Resaving JSON with obfuscated passwords");
-        saveToFile();
-      }
-      return result;
+    if (json.isEmpty()) {
+      LOG_ERR("OPS", "Failed to parse %s", OPDS_FILE_JSON);
+      return false;
     }
+
+    // resave flag is set when passwords were stored in plaintext and need re-obfuscation
+    bool resave = false;
+    bool result = JsonSettingsIO::loadOpds(*this, json.c_str(), &resave);
+    if (!result) {
+      LOG_ERR("OPS", "Failed to parse %s", OPDS_FILE_JSON);
+      return false;
+    }
+    if (resave) {
+      LOG_DBG("OPS", "Resaving JSON with obfuscated passwords");
+      if (!saveToFile()) {
+        LOG_ERR("OPS", "Failed to resave %s after password migration", OPDS_FILE_JSON);
+      }
+    }
+    return true;
   }
 
   // No opds.json found — attempt one-time migration from the legacy single-server
@@ -71,15 +113,23 @@ bool OpdsServerStore::migrateFromSettings() {
   return false;
 }
 
-bool OpdsServerStore::addServer(const OpdsServer& server) {
+std::optional<size_t> OpdsServerStore::addServer(const OpdsServer& server) {
   if (servers.size() >= MAX_SERVERS) {
     LOG_DBG("OPS", "Cannot add more servers, limit of %zu reached", MAX_SERVERS);
-    return false;
+    return std::nullopt;
   }
 
+  const auto originalServers = servers;
   servers.push_back(server);
-  LOG_DBG("OPS", "Added server: %s", server.name.c_str());
-  return saveToFile();
+  if (!saveToFile()) {
+    servers = originalServers;
+    LOG_ERR("OPS", "Failed to persist added server, rolled back in-memory state");
+    return std::nullopt;
+  }
+
+  const size_t insertedIndex = servers.size() - 1;
+  LOG_DBG("OPS", "Added server at index %zu: %s", insertedIndex, server.name.c_str());
+  return insertedIndex;
 }
 
 bool OpdsServerStore::updateServer(size_t index, const OpdsServer& server) {
@@ -87,9 +137,16 @@ bool OpdsServerStore::updateServer(size_t index, const OpdsServer& server) {
     return false;
   }
 
+  const auto originalServers = servers;
   servers[index] = server;
-  LOG_DBG("OPS", "Updated server: %s", server.name.c_str());
-  return saveToFile();
+  if (!saveToFile()) {
+    servers = originalServers;
+    LOG_ERR("OPS", "Failed to persist updated server at index %zu, rolled back in-memory state", index);
+    return false;
+  }
+
+  LOG_DBG("OPS", "Updated server at index %zu: %s", index, server.name.c_str());
+  return true;
 }
 
 bool OpdsServerStore::removeServer(size_t index) {
@@ -97,9 +154,17 @@ bool OpdsServerStore::removeServer(size_t index) {
     return false;
   }
 
-  LOG_DBG("OPS", "Removed server: %s", servers[index].name.c_str());
+  const auto originalServers = servers;
+  const std::string removedName = servers[index].name;
   servers.erase(servers.begin() + static_cast<ptrdiff_t>(index));
-  return saveToFile();
+  if (!saveToFile()) {
+    servers = originalServers;
+    LOG_ERR("OPS", "Failed to persist removed server at index %zu, rolled back in-memory state", index);
+    return false;
+  }
+
+  LOG_DBG("OPS", "Removed server at index %zu: %s", index, removedName.c_str());
+  return true;
 }
 
 const OpdsServer* OpdsServerStore::getServer(size_t index) const {

src/OpdsServerStore.h

@@ -1,4 +1,5 @@
 #pragma once
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -15,6 +16,10 @@ bool saveOpds(const OpdsServerStore& store, const char* path);
 bool loadOpds(OpdsServerStore& store, const char* json, bool* needsResave);
 }  // namespace JsonSettingsIO
 
+namespace OpdsServerValidation {
+std::optional<std::string> normalizeUrl(const std::string& url);
+}
+
 /**
  * Singleton class for storing OPDS server configurations on the SD card.
  * Passwords are XOR-obfuscated with the device's unique hardware MAC address
@@ -25,14 +30,18 @@ class OpdsServerStore {
   static OpdsServerStore instance;
   std::vector<OpdsServer> servers;
 
-  static constexpr size_t MAX_SERVERS = 8;
-
   OpdsServerStore() = default;
 
   friend bool JsonSettingsIO::saveOpds(const OpdsServerStore&, const char*);
   friend bool JsonSettingsIO::loadOpds(OpdsServerStore&, const char*, bool*);
 
  public:
+  static constexpr size_t MAX_SERVERS = 8;
+  static constexpr size_t MAX_NAME_LENGTH = 63;
+  static constexpr size_t MAX_URL_LENGTH = 127;
+  static constexpr size_t MAX_USERNAME_LENGTH = 63;
+  static constexpr size_t MAX_PASSWORD_LENGTH = 63;
+
   OpdsServerStore(const OpdsServerStore&) = delete;
   OpdsServerStore& operator=(const OpdsServerStore&) = delete;
 
@@ -41,7 +50,7 @@ class OpdsServerStore {
   bool saveToFile() const;
   bool loadFromFile();
 
-  bool addServer(const OpdsServer& server);
+  std::optional<size_t> addServer(const OpdsServer& server);
   bool updateServer(size_t index, const OpdsServer& server);
   bool removeServer(size_t index);
 

src/activities/settings/OpdsServerListActivity.cpp

@@ -3,6 +3,8 @@
 #include <GfxRenderer.h>
 #include <I18n.h>
 
+#include <algorithm>
+
 #include "MappedInputManager.h"
 #include "OpdsServerStore.h"
 #include "OpdsSettingsActivity.h"
@@ -78,7 +80,8 @@ void OpdsServerListActivity::handleSelection() {
   auto resultHandler = [this](const ActivityResult&) {
     // Reload server list when returning from editor
     OPDS_STORE.loadFromFile();
-    selectedIndex = 0;
+    const int itemCount = getItemCount();
+    selectedIndex = itemCount > 0 ? std::min(selectedIndex, itemCount - 1) : 0;
   };
 
   if (selectedIndex < serverCount) {

src/activities/settings/OpdsSettingsActivity.cpp

@@ -16,6 +16,7 @@ namespace {
 // Editable fields: Name, URL, Username, Password.
 // Existing servers also show a Delete option (BASE_ITEMS + 1).
 constexpr int BASE_ITEMS = 4;
+constexpr char INVALID_OPDS_URL_MESSAGE[] = "Enter a valid OPDS URL";
 }  // namespace
 
 int OpdsSettingsActivity::getMenuItemCount() const {
@@ -28,6 +29,7 @@ void OpdsSettingsActivity::onEnter() {
   selectedIndex = 0;
   isNewServer = (serverIndex < 0);
   showSaveError = false;
+  popupMessage.clear();
 
   if (!isNewServer) {
     // Edit flow: copy the selected server into local editable state.
@@ -75,12 +77,13 @@ bool OpdsSettingsActivity::saveServer() {
 
   if (isNewServer) {
     // Create flow: first save inserts a new server record into the multi-server store.
-    success = OPDS_STORE.addServer(editServer);
+    const auto insertedIndex = OPDS_STORE.addServer(editServer);
+    success = insertedIndex.has_value();
     if (success) {
       // After the first successful save, promote to an existing server so
       // subsequent field edits update in-place rather than creating duplicates.
       isNewServer = false;
-      serverIndex = static_cast<int>(OPDS_STORE.getCount()) - 1;
+      serverIndex = static_cast<int>(*insertedIndex);
     } else {
       LOG_ERR("OPS", "Failed to add OPDS server");
     }
@@ -93,6 +96,9 @@ bool OpdsSettingsActivity::saveServer() {
   }
 
   showSaveError = !success;
+  if (success) {
+    popupMessage.clear();
+  }
   if (showSaveError) {
     requestUpdate();
   }
@@ -113,23 +119,32 @@ void OpdsSettingsActivity::handleSelection() {
         requestUpdate();
       }
     };
-    startActivityForResult(std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_SERVER_NAME),
-                                                                   editServer.name, 63, InputType::Text),
-                           handler);
+    startActivityForResult(
+        std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_SERVER_NAME), editServer.name,
+                                                OpdsServerStore::MAX_NAME_LENGTH, InputType::Text),
+        handler);
   } else if (selectedIndex == 1) {
     // Server URL
     const std::string prefillUrl = editServer.url.empty() ? "https://" : editServer.url;
     auto handler = [this](const ActivityResult& result) {
       if (!result.isCancelled) {
         const auto& kb = std::get<KeyboardResult>(result.data);
-        editServer.url = (kb.text == "https://" || kb.text == "http://") ? "" : kb.text;
+        const auto normalizedUrl = OpdsServerValidation::normalizeUrl(kb.text);
+        if (!normalizedUrl) {
+          popupMessage = INVALID_OPDS_URL_MESSAGE;
+          requestUpdate();
+          return;
+        }
+        popupMessage.clear();
+        editServer.url = *normalizedUrl;
         saveServer();
         requestUpdate();
       }
     };
-    startActivityForResult(std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_OPDS_SERVER_URL),
-                                                                   prefillUrl, 127, InputType::Url),
-                           handler);
+    startActivityForResult(
+        std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_OPDS_SERVER_URL), prefillUrl,
+                                                OpdsServerStore::MAX_URL_LENGTH, InputType::Url),
+        handler);
   } else if (selectedIndex == 2) {
     // Username
     auto handler = [this](const ActivityResult& result) {
@@ -140,9 +155,10 @@ void OpdsSettingsActivity::handleSelection() {
         requestUpdate();
       }
     };
-    startActivityForResult(std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_USERNAME),
-                                                                   editServer.username, 63, InputType::Text),
-                           handler);
+    startActivityForResult(
+        std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_USERNAME), editServer.username,
+                                                OpdsServerStore::MAX_USERNAME_LENGTH, InputType::Text),
+        handler);
   } else if (selectedIndex == 3) {
     // Password
     auto handler = [this](const ActivityResult& result) {
@@ -153,9 +169,10 @@ void OpdsSettingsActivity::handleSelection() {
         requestUpdate();
       }
     };
-    startActivityForResult(std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_PASSWORD),
-                                                                   editServer.password, 63, InputType::Password),
-                           handler);
+    startActivityForResult(
+        std::make_unique<KeyboardEntryActivity>(renderer, mappedInput, tr(STR_PASSWORD), editServer.password,
+                                                OpdsServerStore::MAX_PASSWORD_LENGTH, InputType::Password),
+        handler);
   } else if (selectedIndex == 4 && !isNewServer) {
     // Delete flow is only available for existing servers.
     if (!OPDS_STORE.removeServer(static_cast<size_t>(serverIndex))) {
@@ -214,7 +231,9 @@ void OpdsSettingsActivity::render(RenderLock&&) {
   const auto labels = mappedInput.mapLabels(tr(STR_BACK), tr(STR_SELECT), tr(STR_DIR_UP), tr(STR_DIR_DOWN));
   GUI.drawButtonHints(renderer, labels.btn1, labels.btn2, labels.btn3, labels.btn4);
 
-  if (showSaveError) {
+  if (!popupMessage.empty()) {
+    GUI.drawPopup(renderer, popupMessage.c_str());
+  } else if (showSaveError) {
     GUI.drawPopup(renderer, tr(STR_ERROR_GENERAL_FAILURE));
   }
 

src/activities/settings/OpdsSettingsActivity.h

@@ -30,6 +30,7 @@ class OpdsSettingsActivity final : public Activity {
   OpdsServer editServer;
   bool isNewServer = false;
   bool showSaveError = false;
+  std::string popupMessage;
 
   int getMenuItemCount() const;
   void handleSelection();

src/network/CrossPointWebServer.cpp

@@ -1388,6 +1388,7 @@ void CrossPointWebServer::handleGetOpdsServers() const {
   char output[512];
   constexpr size_t outputSize = sizeof(output);
   JsonDocument doc;
+  bool seenFirst = false;
 
   for (size_t i = 0; i < servers.size(); i++) {
     doc.clear();
@@ -1401,7 +1402,10 @@ void CrossPointWebServer::handleGetOpdsServers() const {
     const size_t written = serializeJson(doc, output, outputSize);
     if (written >= outputSize) continue;
 
-    if (i > 0) server->sendContent(",");
+    if (seenFirst) {
+      server->sendContent(",");
+    }
+    seenFirst = true;
     server->sendContent(output);
   }
 
@@ -1424,16 +1428,38 @@ void CrossPointWebServer::handlePostOpdsServer() {
     return;
   }
 
-  OpdsServer opdsServer;
-  opdsServer.name = doc["name"] | std::string("");
-  opdsServer.url = doc["url"] | std::string("");
-  opdsServer.username = doc["username"] | std::string("");
+  const std::string name = doc["name"] | std::string("");
+  const std::string rawUrl = doc["url"] | std::string("");
+  const std::string username = doc["username"] | std::string("");
 
   // The password field is optional in the JSON payload. When absent (vs. present but empty),
   // we preserve the existing password — the web UI omits it when the user hasn't changed it.
   bool hasPasswordField = doc["password"].is<const char*>() || doc["password"].is<std::string>();
   std::string password = doc["password"] | std::string("");
 
+  const auto normalizedUrl = OpdsServerValidation::normalizeUrl(rawUrl);
+  if (!normalizedUrl) {
+    server->send(400, "text/plain", "Invalid URL");
+    return;
+  }
+  if (name.size() > OpdsServerStore::MAX_NAME_LENGTH) {
+    server->send(400, "text/plain", "Server name too long");
+    return;
+  }
+  if (normalizedUrl->size() > OpdsServerStore::MAX_URL_LENGTH) {
+    server->send(400, "text/plain", "URL too long");
+    return;
+  }
+  if (username.size() > OpdsServerStore::MAX_USERNAME_LENGTH) {
+    server->send(400, "text/plain", "Username too long");
+    return;
+  }
+
+  OpdsServer opdsServer;
+  opdsServer.name = name;
+  opdsServer.url = *normalizedUrl;
+  opdsServer.username = username;
+
   if (doc["index"].is<int>()) {
     int idx = doc["index"].as<int>();
     if (idx < 0 || idx >= static_cast<int>(OPDS_STORE.getCount())) {
@@ -1445,16 +1471,32 @@ void CrossPointWebServer::handlePostOpdsServer() {
       const auto* existing = OPDS_STORE.getServer(static_cast<size_t>(idx));
       if (existing) password = existing->password;
     }
+    if (password.size() > OpdsServerStore::MAX_PASSWORD_LENGTH) {
+      server->send(400, "text/plain", "Password too long");
+      return;
+    }
     opdsServer.password = password;
-    OPDS_STORE.updateServer(static_cast<size_t>(idx), opdsServer);
+    if (!OPDS_STORE.updateServer(static_cast<size_t>(idx), opdsServer)) {
+      server->send(500, "text/plain", "Failed to save server");
+      return;
+    }
     LOG_DBG("WEB", "Updated OPDS server at index %d", idx);
   } else {
-    opdsServer.password = password;
-    if (!OPDS_STORE.addServer(opdsServer)) {
+    if (OPDS_STORE.getCount() >= OpdsServerStore::MAX_SERVERS) {
       server->send(400, "text/plain", "Cannot add server (limit reached)");
       return;
     }
-    LOG_DBG("WEB", "Added new OPDS server: %s", opdsServer.name.c_str());
+    if (password.size() > OpdsServerStore::MAX_PASSWORD_LENGTH) {
+      server->send(400, "text/plain", "Password too long");
+      return;
+    }
+    opdsServer.password = password;
+    const auto insertedIndex = OPDS_STORE.addServer(opdsServer);
+    if (!insertedIndex) {
+      server->send(500, "text/plain", "Failed to save server");
+      return;
+    }
+    LOG_DBG("WEB", "Added new OPDS server at index %zu: %s", *insertedIndex, opdsServer.name.c_str());
   }
 
   server->send(200, "text/plain", "OK");
@@ -1486,7 +1528,10 @@ void CrossPointWebServer::handleDeleteOpdsServer() {
     return;
   }
 
-  OPDS_STORE.removeServer(static_cast<size_t>(idx));
+  if (!OPDS_STORE.removeServer(static_cast<size_t>(idx))) {
+    server->send(500, "text/plain", "Failed to delete server");
+    return;
+  }
   LOG_DBG("WEB", "Deleted OPDS server at index %d", idx);
   server->send(200, "text/plain", "OK");
 }

src/network/HttpDownloader.cpp

@@ -70,7 +70,7 @@ bool HttpDownloader::fetchUrl(const std::string& url, Stream& outContent, const
   http.setTimeout(30000);
   http.addHeader("User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION);
 
-  if (!username.empty() && !password.empty()) {
+  if (!username.empty() || !password.empty()) {
     std::string credentials = username + ":" + password;
     String encoded = base64::encode(credentials.c_str());
     http.addHeader("Authorization", "Basic " + encoded);
@@ -122,7 +122,7 @@ HttpDownloader::DownloadError HttpDownloader::downloadToFile(const std::string&
   http.setTimeout(65535);  // max uint16_t (~65s) — HTTPClient::setTimeout takes uint16_t ms
   http.addHeader("User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION);
 
-  if (!username.empty() && !password.empty()) {
+  if (!username.empty() || !password.empty()) {
     std::string credentials = username + ":" + password;
     String encoded = base64::encode(credentials.c_str());
     http.addHeader("Authorization", "Basic " + encoded);

src/network/html/SettingsPage.html

@@ -509,6 +509,7 @@
   // /api/opds REST endpoints. Password fields are never pre-filled for security;
   // the "(unchanged)" placeholder indicates an existing password is preserved on save.
   let opdsServers = [];
+  const MAX_OPDS_SERVERS = 8;
 
   function renderOpdsServer(srv, idx) {
     const isNew = idx === -1;
@@ -540,6 +541,7 @@
   function renderOpdsSection() {
     const container = document.getElementById('opds-container');
     let html = '<div class="card"><h2>OPDS Servers</h2>';
+    const canAddOpdsServer = opdsServers.length < MAX_OPDS_SERVERS;
 
     if (opdsServers.length === 0) {
       html += '<p style="color:var(--label-color);text-align:center;">No OPDS servers configured</p>';
@@ -550,7 +552,9 @@
     }
 
     html += '<div style="margin-top:12px;text-align:center;">' +
-      '<button class="btn-small btn-add" onclick="addOpdsServer()">+ Add Server</button>' +
+      (canAddOpdsServer
+        ? '<button class="btn-small btn-add" onclick="addOpdsServer()">+ Add Server</button>'
+        : '<span style="color:var(--label-color);">Maximum of 8 servers reached</span>') +
     '</div></div>';
     container.innerHTML = html;
   }
@@ -567,6 +571,7 @@
   }
 
   function addOpdsServer() {
+    if (opdsServers.length >= MAX_OPDS_SERVERS) return;
     const container = document.getElementById('opds-container');
     const card = container.querySelector('.card');
     const addBtn = card.querySelector('.btn-add').parentElement;