diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua index 00707a194..6b7f9c7b8 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua @@ -832,7 +832,10 @@ function BookOrbitCatalog:showServerError(err) end function BookOrbitCatalog:errorText(err) - if isAuthError(err) then + local transfer_error = CatalogUtil.transferErrorText(err) + if transfer_error then + return transfer_error + elseif isAuthError(err) then return _("BookOrbit login failed. Check your KOReader credentials.") elseif type(err) == "number" then return T(_("BookOrbit server error (%1)."), err) diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_util.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_util.lua index 4698b8894..f71aab999 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_util.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_util.lua @@ -386,6 +386,15 @@ function CatalogUtil.safeFilenameBase(detail) return base end +function CatalogUtil.transferErrorText(err) + if err == "unsafe_destination" then + return _("The download destination is outside the selected folder.") + elseif err == "temp_dir_failed" then + return _("Could not create a temporary download folder in the selected folder.") + end + return nil +end + function CatalogUtil.coverLabel(book) local lines = {} local title = book and book.title or _("Untitled") diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_download_transfer.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_download_transfer.lua index 7b58bc60d..365394e34 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_download_transfer.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_download_transfer.lua @@ -31,11 +31,14 @@ Transfer.TEMP_DIR_NAME = TEMP_DIR_NAME Transfer.POLL_INTERVAL = POLL_INTERVAL local function normalize(path) - return tostring(path or ""):gsub("\\", "/"):gsub("/+", "/"):gsub("/$", "") + local normalized = tostring(path or ""):gsub("\\", "/"):gsub("/+", "/") + if normalized == "/" then return normalized end + return normalized:gsub("/$", "") end function Transfer.tempDir(root) - return normalize(root) .. "/" .. TEMP_DIR_NAME + local normalized_root = normalize(root) + return (normalized_root ~= "/" and normalized_root or "") .. "/" .. TEMP_DIR_NAME end function Transfer.ensureTempDir(root) @@ -52,7 +55,11 @@ function Transfer.isInsideRoot(root, path) local normalized_root = normalize(root) local normalized_path = normalize(path) if normalized_root == "" or normalized_path == "" then return false end - if normalized_path:sub(1, #normalized_root + 1) ~= normalized_root .. "/" then return false end + if normalized_root == "/" then + if normalized_path == "/" or normalized_path:sub(1, 1) ~= "/" then return false end + elseif normalized_path:sub(1, #normalized_root + 1) ~= normalized_root .. "/" then + return false + end for segment in normalized_path:gmatch("[^/]+") do if segment == ".." then return false end end diff --git a/koreader-plugin/bookorbit.koplugin/main.lua b/koreader-plugin/bookorbit.koplugin/main.lua index e63a5dc45..ffccf4f6c 100644 --- a/koreader-plugin/bookorbit.koplugin/main.lua +++ b/koreader-plugin/bookorbit.koplugin/main.lua @@ -48,7 +48,7 @@ local BookOrbitProgressSync = require("bookorbit_progress_sync") local BookOrbitSweep = require("bookorbit_sweep") local BookOrbitUpdater = require("bookorbit_updater") -local PLUGIN_VERSION = "1.5.0" +local PLUGIN_VERSION = "1.5.1" local SYNC_STRATEGY = { PROMPT = 1, diff --git a/koreader-plugin/spec/bookorbit_catalog_transfer_error_test.lua b/koreader-plugin/spec/bookorbit_catalog_transfer_error_test.lua new file mode 100644 index 000000000..a5660f5b6 --- /dev/null +++ b/koreader-plugin/spec/bookorbit_catalog_transfer_error_test.lua @@ -0,0 +1,42 @@ +local function identity(value) + return value +end + +package.loaded["gettext"] = identity +package.loaded["document/documentregistry"] = { + hasProvider = function() + return true + end, +} +package.loaded["util"] = { + fixUtf8 = identity, +} +package.loaded["ffi/util"] = { + template = function(value) + return value + end, +} + +package.path = "koreader-plugin/bookorbit.koplugin/?.lua;" .. package.path + +local CatalogUtil = require("bookorbit_catalog_util") + +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 + +assertEqual( + CatalogUtil.transferErrorText("unsafe_destination"), + "The download destination is outside the selected folder.", + "unsafe destinations are reported as local path failures" +) +assertEqual( + CatalogUtil.transferErrorText("temp_dir_failed"), + "Could not create a temporary download folder in the selected folder.", + "temporary directory failures are reported as local filesystem failures" +) +assertEqual(CatalogUtil.transferErrorText("network_error"), nil, "network errors keep the existing connection message") + +print("bookorbit_catalog_transfer_error_test.lua: ok") diff --git a/koreader-plugin/spec/bookorbit_download_transfer_test.lua b/koreader-plugin/spec/bookorbit_download_transfer_test.lua index 235b2d2ea..1ba1e9788 100644 --- a/koreader-plugin/spec/bookorbit_download_transfer_test.lua +++ b/koreader-plugin/spec/bookorbit_download_transfer_test.lua @@ -10,7 +10,10 @@ package.loaded["ui/uimanager"] = { end, } -local directories = { ["/downloads/.bookorbit-tmp"] = true } +local directories = { + ["/.bookorbit-tmp"] = true, + ["/downloads/.bookorbit-tmp"] = true, +} local files = {} local now = 1000 package.loaded["libs/libkoreader-lfs"] = { @@ -66,10 +69,16 @@ end -- Destination containment is checked before anything is written and again -- before the publishing rename. assertEqual(Transfer.isInsideRoot("/downloads", "/downloads/Books/a.epub"), true, "a path under the root is authorized") +assertEqual(Transfer.isInsideRoot("/downloads", "/downloads/The Shards.epub"), true, + "a file directly under the download root is authorized") assertEqual(Transfer.isInsideRoot("/downloads", "/elsewhere/a.epub"), false, "a path outside the root is rejected") assertEqual(Transfer.isInsideRoot("/downloads", "/downloads/../etc/passwd"), false, "traversal is rejected") assertEqual(Transfer.isInsideRoot("/downloads", "/downloads"), false, "the root itself is not a destination") assertEqual(Transfer.tempDir("/downloads"), "/downloads/.bookorbit-tmp", "temporaries live under the destination root") +assertEqual(Transfer.isInsideRoot("/", "/The Shards.epub"), true, "the filesystem root supports a direct book destination") +assertEqual(Transfer.isInsideRoot("/", "/Standalone/The Shards.epub"), true, "the filesystem root supports nested destinations") +assertEqual(Transfer.isInsideRoot("/", "/../etc/passwd"), false, "the filesystem root still rejects traversal") +assertEqual(Transfer.tempDir("/"), "/.bookorbit-tmp", "the filesystem root gets one temporary directory separator") -- Leftovers from a killed run are removed, and the sweep stays bounded. files["/downloads/.bookorbit-tmp/bo_1_1.part"] = { size = 10, modified = 1 } @@ -141,6 +150,25 @@ runScheduled() assertEqual(#scheduled, 0, "a poll that fires after completion does not reschedule itself") assertEqual(#observed, 1, "no progress is reported once the transfer returned") +-- A user-selected filesystem root used to normalize to an empty string and +-- reject the destination before perform could start the HTTP request. +local root_request_started = false +local root_completed = Transfer.run{ + root = "/", + destination = "/The Shards.epub", + generation = 5, + expected_bytes = 2089926, + is_current = function() return true end, + perform = function(download_opts) + root_request_started = true + files[download_opts.temp_path] = { size = 2089926, modified = now } + return { temp_path = download_opts.temp_path, bytes = 2089926 } + end, +} +assertEqual(root_request_started, true, "the reported book reaches the HTTP transfer callback") +assertEqual(root_completed, true, "the reported book publishes under a filesystem-root download folder") +assertEqual(files["/The Shards.epub"] ~= nil, true, "the reported book is published at the requested destination") + -- A cancelled generation discards a late child result instead of publishing it. renames = {} removed = {}