From 6053ef4756932de7efe08d42617ef1cb89473559 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Thu, 23 Apr 2026 17:08:08 +0200 Subject: [PATCH] Review comments --- USER_GUIDE.md | 6 +- src/OpdsServerStore.cpp | 99 +++++++++++++++---- src/OpdsServerStore.h | 15 ++- .../settings/OpdsServerListActivity.cpp | 5 +- .../settings/OpdsSettingsActivity.cpp | 51 +++++++--- .../settings/OpdsSettingsActivity.h | 1 + src/network/CrossPointWebServer.cpp | 65 ++++++++++-- src/network/HttpDownloader.cpp | 4 +- src/network/html/SettingsPage.html | 7 +- 9 files changed, 200 insertions(+), 53 deletions(-) diff --git a/USER_GUIDE.md b/USER_GUIDE.md index 5c661391..146de364 100644 --- a/USER_GUIDE.md +++ b/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: diff --git a/src/OpdsServerStore.cpp b/src/OpdsServerStore.cpp index b2151682..781223a9 100644 --- a/src/OpdsServerStore.cpp +++ b/src/OpdsServerStore.cpp @@ -4,16 +4,49 @@ #include #include +#include #include #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 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 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(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 { diff --git a/src/OpdsServerStore.h b/src/OpdsServerStore.h index 87571f65..70c11eb1 100644 --- a/src/OpdsServerStore.h +++ b/src/OpdsServerStore.h @@ -1,4 +1,5 @@ #pragma once +#include #include #include @@ -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 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 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 addServer(const OpdsServer& server); bool updateServer(size_t index, const OpdsServer& server); bool removeServer(size_t index); diff --git a/src/activities/settings/OpdsServerListActivity.cpp b/src/activities/settings/OpdsServerListActivity.cpp index 52987d25..d6964107 100644 --- a/src/activities/settings/OpdsServerListActivity.cpp +++ b/src/activities/settings/OpdsServerListActivity.cpp @@ -3,6 +3,8 @@ #include #include +#include + #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) { diff --git a/src/activities/settings/OpdsSettingsActivity.cpp b/src/activities/settings/OpdsSettingsActivity.cpp index ec07c1f9..c3907cea 100644 --- a/src/activities/settings/OpdsSettingsActivity.cpp +++ b/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(OPDS_STORE.getCount()) - 1; + serverIndex = static_cast(*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(renderer, mappedInput, tr(STR_SERVER_NAME), - editServer.name, 63, InputType::Text), - handler); + startActivityForResult( + std::make_unique(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(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(renderer, mappedInput, tr(STR_OPDS_SERVER_URL), - prefillUrl, 127, InputType::Url), - handler); + startActivityForResult( + std::make_unique(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(renderer, mappedInput, tr(STR_USERNAME), - editServer.username, 63, InputType::Text), - handler); + startActivityForResult( + std::make_unique(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(renderer, mappedInput, tr(STR_PASSWORD), - editServer.password, 63, InputType::Password), - handler); + startActivityForResult( + std::make_unique(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(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)); } diff --git a/src/activities/settings/OpdsSettingsActivity.h b/src/activities/settings/OpdsSettingsActivity.h index 4ea2d61b..2e818edc 100644 --- a/src/activities/settings/OpdsSettingsActivity.h +++ b/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(); diff --git a/src/network/CrossPointWebServer.cpp b/src/network/CrossPointWebServer.cpp index 0def450b..784aedb1 100644 --- a/src/network/CrossPointWebServer.cpp +++ b/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() || doc["password"].is(); 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 idx = doc["index"].as(); if (idx < 0 || idx >= static_cast(OPDS_STORE.getCount())) { @@ -1445,16 +1471,32 @@ void CrossPointWebServer::handlePostOpdsServer() { const auto* existing = OPDS_STORE.getServer(static_cast(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(idx), opdsServer); + if (!OPDS_STORE.updateServer(static_cast(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(idx)); + if (!OPDS_STORE.removeServer(static_cast(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"); } diff --git a/src/network/HttpDownloader.cpp b/src/network/HttpDownloader.cpp index dfcd3a9d..ab6806a9 100644 --- a/src/network/HttpDownloader.cpp +++ b/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); diff --git a/src/network/html/SettingsPage.html b/src/network/html/SettingsPage.html index e9c87cf4..bbee4416 100644 --- a/src/network/html/SettingsPage.html +++ b/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 = '

OPDS Servers

'; + const canAddOpdsServer = opdsServers.length < MAX_OPDS_SERVERS; if (opdsServers.length === 0) { html += '

No OPDS servers configured

'; @@ -550,7 +552,9 @@ } html += '
' + - '' + + (canAddOpdsServer + ? '' + : 'Maximum of 8 servers reached') + '
'; 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;