From f39e0e8ab51f4a7ba68d2284cbfbe696b3d67036 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:29:36 +0000 Subject: [PATCH] fix(reader): harden KOReader sync and credential refresh Sanitize match-check batches before strict server validation and classify HTTP failures accurately. Rebind the active catalog client whenever authentication settings change so retries use current credentials. Fixes #1150 --- .../bookorbit.koplugin/bookorbit_api.lua | 69 ++++++++- .../bookorbit.koplugin/bookorbit_catalog.lua | 7 +- .../bookorbit_main_menu.lua | 15 +- .../bookorbit.koplugin/bookorbit_sweep.lua | 14 +- koreader-plugin/bookorbit.koplugin/main.lua | 1 + koreader-plugin/spec/bookorbit_api_test.lua | 64 ++++++++ .../bookorbit_catalog_auth_lifecycle_test.lua | 146 ++++++++++++++++++ koreader-plugin/spec/bookorbit_sweep_test.lua | 20 +++ .../spec/helpers/sweep_harness.lua | 5 +- .../koreader/koreader-plugin-source.test.ts | 7 +- 10 files changed, 335 insertions(+), 13 deletions(-) create mode 100644 koreader-plugin/spec/bookorbit_catalog_auth_lifecycle_test.lua diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_api.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_api.lua index 52a28c898..9af691373 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_api.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_api.lua @@ -18,6 +18,10 @@ local TransferPolicy = require("bookorbit_transfer_policy") local TransferProgress = require("bookorbit_transfer_progress") local MAX_BODY_BYTES = 900 * 1024 -- stays under the server's 1 MiB body limit +local MATCH_HASH_LENGTH = 32 +local MATCH_TITLE_MAX_BYTES = 500 +local MATCH_AUTHORS_MAX_BYTES = 1000 +local MAX_SAFE_INTEGER = 9007199254740991 local MAX_DOWNLOAD_REDIRECTS = 5 @@ -25,6 +29,40 @@ local MAX_DOWNLOAD_REDIRECTS = 5 -- validation; every empty table in our payloads is semantically an array. local ENCODE_OPTIONS = { empty_table_as_array = true } +local function validMatchHash(value) + return type(value) == "string" + and #value == MATCH_HASH_LENGTH + and value:match("^[0-9a-fA-F]+$") ~= nil +end + +-- Byte limits are conservative against the server's character limits and +-- keep a worst-case 500-book payload below the HTTP body cap. +local function boundedUtf8(value, max_bytes) + if type(value) ~= "string" then return nil end + value = util.fixUtf8(value, "?") + if #value <= max_bytes then return value end + return util.fixUtf8(value:sub(1, max_bytes), "") +end + +local function nonNegativeInteger(value) + value = tonumber(value) + if not value or value ~= value or value == math.huge + or value < 0 or value > MAX_SAFE_INTEGER or value ~= math.floor(value) then + return nil + end + return value +end + +local function validMatchSource(value) + if value == "current_file" or value == "file" or value == "statistics" then + return value + end +end + +local function optionalBoolean(value) + if type(value) == "boolean" then return value end +end + -- JSON null decodes to the rapidjson.null lightuserdata, which is truthy in -- Lua and would leak into sidecars and truthiness checks; treat it as absent. local function scrubNulls(value) @@ -482,21 +520,38 @@ end -- BookOrbit plugin endpoints (camelCase wire format) function BookOrbitApi:matchCheck(hashes, candidates) - local payload = { hashes = hashes } + local payload = { hashes = {} } if candidates then payload.books = {} - for _, hash in ipairs(hashes) do + end + + local skipped = 0 + for _, hash in ipairs(hashes or {}) do + local valid_hash = validMatchHash(hash) + if valid_hash then + table.insert(payload.hashes, hash) + else + skipped = skipped + 1 + end + if candidates and valid_hash then local cand = candidates[hash] or {} table.insert(payload.books, { hash = hash, - title = cand.title, - authors = cand.authors, - lastOpen = cand.last_open, - source = cand.source, - metadataAmbiguous = cand.metadata_ambiguous, + title = boundedUtf8(cand.title, MATCH_TITLE_MAX_BYTES), + authors = boundedUtf8(cand.authors, MATCH_AUTHORS_MAX_BYTES), + lastOpen = nonNegativeInteger(cand.last_open), + source = validMatchSource(cand.source), + metadataAmbiguous = optionalBoolean(cand.metadata_ambiguous), }) end end + + if skipped > 0 then + logger.warn("BookOrbit: match-check skipped invalid hashes:", skipped) + end + if #payload.hashes == 0 then + return { matches = {} } + end return self:request("POST", "/koreader/plugin/match-check", self:withDevice(payload)) end diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua index 6b7f9c7b8..5e4a52298 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua @@ -256,8 +256,13 @@ local Menu_onFirstPage = Menu.onFirstPage local Menu_onLastPage = Menu.onLastPage local Menu_onClose = Menu.onClose +function BookOrbitCatalog:setApi(api) + self.api = api + self.client = BookOrbitApi.new(api) +end + function BookOrbitCatalog:init() - self.client = BookOrbitApi.new(self.api) + self:setApi(self.api) self.stack = {} self.settings = self.settings or {} CatalogWidgets.setAssetIconFiles({ diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_main_menu.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_main_menu.lua index 6719ca027..9fb2070c8 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_main_menu.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_main_menu.lua @@ -702,6 +702,17 @@ function MainMenu:showDashboardMenu(catalog) UIManager:show(menu_container) end +function MainMenu:refreshActiveCatalogApi(refresh) + if not self.catalog_browser then return end + self.catalog_browser:setApi(self:apiOpts(true)) + if refresh then self.catalog_browser:refreshCurrent() end +end + +function MainMenu:applyServerAddress(server_url) + self.settings.server_url = server_url + self:refreshActiveCatalogApi(false) +end + function MainMenu:setServerAddress() local dialog dialog = InputDialog:new{ @@ -722,7 +733,7 @@ function MainMenu:setServerAddress() is_enter_default = true, callback = function() local normalized = BookOrbitApi.normalizeServerUrl(dialog:getInputText()) - self.settings.server_url = normalized + self:applyServerAddress(normalized) UIManager:close(dialog) if normalized then Notification:notify(T(_("BookOrbit server set to %1"), normalized)) @@ -801,6 +812,7 @@ function MainMenu:doLogin(username, password, menu) if body then self.settings.username = username self.settings.userkey = userkey + self:refreshActiveCatalogApi(true) if menu then menu:updateItems() end UIManager:show(InfoMessage:new{ text = _("Logged in to BookOrbit.") }) UIManager:scheduleIn(1, function() @@ -821,6 +833,7 @@ end function MainMenu:logout(menu) self.settings.userkey = nil + self:refreshActiveCatalogApi(false) if menu then menu:updateItems() end end diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_sweep.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_sweep.lua index 2237def53..8d55fea21 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_sweep.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_sweep.lua @@ -199,7 +199,7 @@ local function finish(ctx, err) end if ctx.plugin and ctx.plugin.recordSyncError then - if err == "auth" or err == "network" then + if err == "auth" or err == "network" or err == "server" then ctx.plugin:recordSyncError("sweep", err) elseif not err and ctx.had_errors then ctx.plugin:recordSyncError("sweep", "partial_failure") @@ -243,6 +243,17 @@ local function finish(ctx, err) return end + if err == "server" then + if ctx.interactive then + UIManager:show(InfoMessage:new{ text = _("BookOrbit sync: server request failed."), timeout = 4 }) + end + logger.warn("BookOrbit: sweep aborted, server request failed") + if ctx.on_finish then + pcall(ctx.on_finish, err) + end + return + end + if ctx.interactive then local text = T(_("BookOrbit sync done: %1 books matched, %2 reading events, %3 highlights."), ctx.counts.books_matched, ctx.counts.page_stats, ctx.counts.annotations) @@ -606,6 +617,7 @@ local function stepMatchNext(ctx) if aborted(ctx) then return end if not body then if isAuthError(err) then return finish(ctx, "auth") end + if type(err) == "number" then return finish(ctx, "server") end return finish(ctx, "network") end diff --git a/koreader-plugin/bookorbit.koplugin/main.lua b/koreader-plugin/bookorbit.koplugin/main.lua index ffccf4f6c..d8e429edc 100644 --- a/koreader-plugin/bookorbit.koplugin/main.lua +++ b/koreader-plugin/bookorbit.koplugin/main.lua @@ -59,6 +59,7 @@ local SYNC_STRATEGY = { local LAST_ERROR_LABELS = { auth = _("login failed"), network = _("server not reachable"), + server = _("server request failed"), invalid_json = _("invalid server response"), unsupported_server = _("server update required"), body_too_large = _("request too large"), diff --git a/koreader-plugin/spec/bookorbit_api_test.lua b/koreader-plugin/spec/bookorbit_api_test.lua index c8336cdbf..5ca41544c 100644 --- a/koreader-plugin/spec/bookorbit_api_test.lua +++ b/koreader-plugin/spec/bookorbit_api_test.lua @@ -6,12 +6,16 @@ local encoded_subprocess_result package.loaded["logger"] = { dbg = function() end, + warn = function() end, } package.loaded["util"] = { trim = function(value) return tostring(value or ""):match("^%s*(.-)%s*$") end, + fixUtf8 = function(value) + return value + end, urlEncode = function(value) return tostring(value) end, @@ -66,8 +70,10 @@ package.loaded["ltn12"] = { } local last_request_url +local request_count = 0 package.loaded["socket.http"] = { request = function(request) + request_count = request_count + 1 request_ran_in_subprocess = in_subprocess last_request_url = request.url if mock_http_body then @@ -78,6 +84,7 @@ package.loaded["socket.http"] = { } local rapidjson_null = {} +local last_encoded_value package.loaded["rapidjson"] = { null = rapidjson_null, encode = function(value) @@ -86,6 +93,7 @@ package.loaded["rapidjson"] = { encoded_subprocess_result = value return "__subprocess_result__" end + last_encoded_value = value return "{}" end, decode = function(raw) @@ -171,6 +179,62 @@ assertEqual(last_request_url, "https://bookorbit.example.com/api/v1/koreader/plugin/catalog/dashboard/sections/up-next-in-series", "the dashboard-section endpoint is addressed by source type") +local match_client = BookOrbitApi.new{ + server_url = "https://bookorbit.example.com/api/v1", + username = "reader", + userkey = "secret", + device_id = "device-id", + device_model = "Test device", + plugin_version = "1.5.1", +} +local hash_a = string.rep("a", 32) +local hash_b = string.rep("B", 32) +local invalid_hash = "not-an-md5" +local oversized_title = string.rep("t", 501) +local oversized_authors = string.rep("a", 1001) + +mock_http_body = "{\"ok\":true}" +mock_http_code = 200 +body, err = match_client:matchCheck({ hash_a, invalid_hash, hash_b }, { + [hash_a] = { + title = oversized_title, + authors = oversized_authors, + last_open = -1, + source = "invalid", + metadata_ambiguous = "yes", + }, + [hash_b] = { + title = 123, + authors = {}, + last_open = "123", + source = "file", + metadata_ambiguous = false, + }, +}) +assertEqual(body.ok, true, "a match-check with one invalid row still reaches the server") +assertEqual(err, nil, "a sanitized match-check succeeds") +assertEqual(#last_encoded_value.hashes, 2, "an invalid hash is removed without dropping valid peers") +assertEqual(last_encoded_value.hashes[1], hash_a, "the first valid hash keeps its position") +assertEqual(last_encoded_value.hashes[2], hash_b, "the later valid hash closes the filtered gap") +assertEqual(#last_encoded_value.books, 2, "candidate metadata stays aligned with filtered hashes") +assertEqual(#last_encoded_value.books[1].title, 500, "an oversized title is bounded to the server contract") +assertEqual(#last_encoded_value.books[1].authors, 1000, "oversized authors are bounded to the server contract") +assertEqual(last_encoded_value.books[1].lastOpen, nil, "a negative timestamp is omitted") +assertEqual(last_encoded_value.books[1].source, nil, "an unknown source is omitted") +assertEqual(last_encoded_value.books[1].metadataAmbiguous, nil, "a non-boolean ambiguity flag is omitted") +assertEqual(last_encoded_value.books[2].title, nil, "non-string titles are omitted") +assertEqual(last_encoded_value.books[2].authors, nil, "non-string authors are omitted") +assertEqual(last_encoded_value.books[2].lastOpen, 123, "a numeric timestamp string is normalized") +assertEqual(last_encoded_value.books[2].source, "file", "a valid source is preserved") +assertEqual(last_encoded_value.books[2].metadataAmbiguous, false, "a boolean ambiguity flag is preserved") + +local requests_before_empty_match = request_count +body, err = match_client:matchCheck({ invalid_hash }, { [invalid_hash] = { title = "Ignored" } }) +assertEqual(type(body), "table", "an all-invalid batch resolves locally") +assertEqual(#body.matches, 0, "an all-invalid batch has no matches") +assertEqual(err, nil, "an all-invalid batch is not reported as a network error") +assertEqual(request_count, requests_before_empty_match, "an empty sanitized batch is not sent to the server") + local wrapped = true local subprocess_calls = 0 local subprocess_result_mode = "normal" diff --git a/koreader-plugin/spec/bookorbit_catalog_auth_lifecycle_test.lua b/koreader-plugin/spec/bookorbit_catalog_auth_lifecycle_test.lua new file mode 100644 index 000000000..8d3e86e0a --- /dev/null +++ b/koreader-plugin/spec/bookorbit_catalog_auth_lifecycle_test.lua @@ -0,0 +1,146 @@ +package.loaded["gettext"] = function(text) + return text +end + +package.loaded["ffi/util"] = { + template = function(text, value) + return text:gsub("%%1", tostring(value)) + end, +} + +local ignored_input = {} +package.loaded["device"] = { + model = "test-device", + setIgnoreInput = function(_, value) + table.insert(ignored_input, value) + end, +} + +local shown = {} +local scheduled = {} +package.loaded["ui/uimanager"] = { + show = function(_, widget) + table.insert(shown, widget) + end, + scheduleIn = function(_, _, callback) + table.insert(scheduled, callback) + end, +} +package.loaded["ui/widget/infomessage"] = { new = function(_, opts) return opts end } +package.loaded["ui/widget/inputdialog"] = {} +package.loaded["ui/widget/multiinputdialog"] = {} +package.loaded["ui/network/manager"] = {} +package.loaded["ui/widget/notification"] = {} +package.loaded["ffi/sha2"] = { md5 = function(value) return "md5:" .. value end } +package.loaded["util"] = { + trim = function(value) + return tostring(value or ""):match("^%s*(.-)%s*$") + end, +} + +local auth_result = { authorized = "OK" } +local created_clients = {} +package.loaded["bookorbit_api"] = { + new = function(opts) + table.insert(created_clients, opts) + return { + auth = function() + if auth_result then return auth_result end + return nil, 401 + end, + } + end, +} +package.loaded["bookorbit_highlight_diagnostics"] = {} +package.loaded["bookorbit_sweep"] = { + syncStatus = function() + return { lastSweepAt = 0, matched = 0, unmatched = 0 } + end, +} + +package.path = "koreader-plugin/bookorbit.koplugin/?.lua;" .. package.path + +local MainMenu = require("bookorbit_main_menu") + +local function assertEqual(actual, expected, label) + if actual ~= expected then + error(string.format("%s: expected %s, got %s", label, tostring(expected), tostring(actual))) + end +end + +local function newPlugin() + local applied_api = {} + local refreshes = 0 + local plugin = { + SYNC_STRATEGY = { PROMPT = 1, SILENT = 2, DISABLE = 3 }, + settings = { + server_url = "https://old.example/api/v1", + username = "old-user", + userkey = "old-key", + }, + apiOpts = function(self, background_requests) + return { + server_url = self.settings.server_url, + username = self.settings.username, + userkey = self.settings.userkey, + background_requests = background_requests == true, + } + end, + catalog_browser = { + setApi = function(_, api) + table.insert(applied_api, api) + end, + refreshCurrent = function() + refreshes = refreshes + 1 + end, + }, + } + MainMenu.install(plugin) + return plugin, applied_api, function() return refreshes end +end + +local plugin, applied_api, refreshCount = newPlugin() +local menu_updates = 0 +local menu = { updateItems = function() menu_updates = menu_updates + 1 end } + +plugin:doLogin("new-user", "new-password", menu) +assertEqual(plugin.settings.username, "new-user", "successful login stores the username") +assertEqual(plugin.settings.userkey, "md5:new-password", "successful login stores the derived key") +assertEqual(#applied_api, 1, "successful login rebinds the active catalog") +assertEqual(applied_api[1].username, "new-user", "catalog receives the new username") +assertEqual(applied_api[1].userkey, "md5:new-password", "catalog receives the new key") +assertEqual(applied_api[1].background_requests, true, "catalog keeps background requests enabled") +assertEqual(refreshCount(), 1, "successful login refreshes the visible catalog") +assertEqual(menu_updates, 1, "successful login refreshes the account menu") +assertEqual(ignored_input[1], true, "login suppresses input while authenticating") +assertEqual(ignored_input[2], false, "login restores input after authenticating") + +plugin:logout(menu) +assertEqual(plugin.settings.userkey, nil, "logout clears the stored key") +assertEqual(#applied_api, 2, "logout rebinds the active catalog") +assertEqual(applied_api[2].userkey, nil, "logout invalidates the catalog client key") +assertEqual(refreshCount(), 1, "logout does not issue an authenticated refresh") +assertEqual(menu_updates, 2, "logout refreshes the account menu") + +plugin:applyServerAddress("https://new.example/api/v1") +assertEqual(plugin.settings.server_url, "https://new.example/api/v1", "server change updates settings") +assertEqual(#applied_api, 3, "server change rebinds the active catalog") +assertEqual(applied_api[3].server_url, "https://new.example/api/v1", "catalog receives the new server") +assertEqual(refreshCount(), 1, "server change does not make an implicit request") + +plugin.catalog_browser = nil +plugin:applyServerAddress("https://closed.example/api/v1") +assertEqual(plugin.settings.server_url, "https://closed.example/api/v1", "server change works without an open catalog") +assertEqual(#applied_api, 3, "closed catalog needs no client rebind") + +plugin, applied_api, refreshCount = newPlugin() +auth_result = nil +plugin:doLogin("rejected-user", "wrong-password") +assertEqual(plugin.settings.username, "old-user", "failed login preserves the previous username") +assertEqual(plugin.settings.userkey, "old-key", "failed login preserves the previous key") +assertEqual(#applied_api, 0, "failed login does not replace the catalog client") +assertEqual(refreshCount(), 0, "failed login does not refresh the catalog") +assertEqual(shown[#shown].text, "Login failed. Create or check your KOReader credentials in BookOrbit web settings.", + "failed login shows the credential error") + +print("bookorbit_catalog_auth_lifecycle_test: ok") diff --git a/koreader-plugin/spec/bookorbit_sweep_test.lua b/koreader-plugin/spec/bookorbit_sweep_test.lua index 9a8a6b92c..fd29cc80a 100644 --- a/koreader-plugin/spec/bookorbit_sweep_test.lua +++ b/koreader-plugin/spec/bookorbit_sweep_test.lua @@ -167,6 +167,26 @@ do "a candidate-less matched book with no stamp is refreshed by the sweep") end +-- An HTTP response proves that the server was reachable. It must not be +-- collapsed into the transport-error path and shown as an offline failure. +do + local harness = SweepHarness.install{ + books = { { md5 = "aaa", id = 1, title = "A", last_open = 100 } }, + match_error = 400, + } + local Sweep = require("bookorbit_sweep") + + local _, result = startSweep(harness, Sweep) + harness.scheduler:drain() + + local finished, err = result() + assertEqual(finished, true, "a rejected sweep reports completion") + assertEqual(err, "server", "an HTTP rejection is classified as a server failure") + assertEqual(harness.shown.text, "BookOrbit sync: server request failed.", + "an HTTP rejection is not presented as an unreachable server") + assertEqual(harness.calls.sweep_complete, 0, "a rejected match phase is not recorded as a completed sweep") +end + -- A library recreation can replace a server file without an intervening -- unmatched response. The rematch must reset the old target's cursor before -- the statistics queue is built, then retain the new server acknowledgment. diff --git a/koreader-plugin/spec/helpers/sweep_harness.lua b/koreader-plugin/spec/helpers/sweep_harness.lua index 3bb7bee2e..237ca5d94 100644 --- a/koreader-plugin/spec/helpers/sweep_harness.lua +++ b/koreader-plugin/spec/helpers/sweep_harness.lua @@ -80,6 +80,7 @@ local function installClient(handle) isConfigured = function() return true end, matchCheck = function(_, hashes) table.insert(handle.calls.match, hashes) + if responses.match_error then return nil, responses.match_error end local matches = {} for _, hash in ipairs(hashes) do if responses.unmatched[hash] then @@ -129,6 +130,7 @@ opts: - library_version: token every server response carries - unmatched: set of hashes match-check refuses to match - matches: map of hashes to custom { bookId, bookFileId } match results +- match_error: HTTP or transport error returned by match-check - page_stats_response: function returning the server response for an upload - sidecar: overrides for the BookOrbitSidecar stub ]] @@ -151,6 +153,7 @@ function SweepHarness.install(opts) library_version = opts.library_version, unmatched = opts.unmatched or {}, matches = opts.matches or {}, + match_error = opts.match_error, page_stats = opts.page_stats_response, }, calls = { @@ -229,7 +232,7 @@ function SweepHarness.install(opts) handle.calls.scheduled = handle.calls.scheduled + 1 handle.scheduler:scheduleIn(delay, callback) end, - show = function() end, + show = function(_, widget) handle.shown = widget end, close = function() end, setDirty = function() end, } diff --git a/server/src/modules/koreader/koreader-plugin-source.test.ts b/server/src/modules/koreader/koreader-plugin-source.test.ts index 2fff5ab84..c8c44b092 100644 --- a/server/src/modules/koreader/koreader-plugin-source.test.ts +++ b/server/src/modules/koreader/koreader-plugin-source.test.ts @@ -342,8 +342,11 @@ describe('KOReader plugin update source wiring', () => { const sweep = await readPluginFile('bookorbit_sweep.lua'); const statsReader = await readPluginFile('bookorbit_stats_reader.lua'); - expect(api).toContain('source = cand.source'); - expect(api).toContain('metadataAmbiguous = cand.metadata_ambiguous'); + expect(api).toContain('title = boundedUtf8(cand.title, MATCH_TITLE_MAX_BYTES)'); + expect(api).toContain('authors = boundedUtf8(cand.authors, MATCH_AUTHORS_MAX_BYTES)'); + expect(api).toContain('lastOpen = nonNegativeInteger(cand.last_open)'); + expect(api).toContain('source = validMatchSource(cand.source)'); + expect(api).toContain('metadataAmbiguous = optionalBoolean(cand.metadata_ambiguous)'); expect(statsReader).toContain('entry.metadata_ambiguous = (entry._variant_count or 0) > 1'); expect(sweep).toContain('source = "statistics"'); expect(sweep).toContain('metadata_ambiguous = entry.metadata_ambiguous');