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

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
 

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({

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
 

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
 

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

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"

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

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.

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,
     }

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