diff --git a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_download.lua b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_download.lua index 32baa0bb1..ca91dbda2 100644 --- a/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_download.lua +++ b/koreader-plugin/bookorbit.koplugin/bookorbit_catalog_download.lua @@ -28,12 +28,16 @@ local safeFilenameBase = CatalogUtil.safeFilenameBase local CatalogDownload = {} -local function sanitizeDevicePath(device_path) +local function sanitizeDevicePath(device_path, download_dir) local normalized = tostring(device_path or ""):gsub("\\", "/"):gsub("^/+", "") local segments = {} for segment in normalized:gmatch("[^/]+") do if segment == ".." then return nil end - if segment ~= "." then table.insert(segments, segment) end + if segment ~= "." then + local safe_segment = util.getSafeFilename(segment, download_dir) + if not safe_segment or safe_segment == "" or safe_segment == "." or safe_segment == ".." then return nil end + table.insert(segments, safe_segment) + end end if #segments == 0 then return nil end return table.concat(segments, "/") @@ -44,7 +48,7 @@ local function joinDownloadPath(download_dir, relative_path) end local function resolveRelativeDownloadPath(download_dir, filename, filetype, device_path, filename_override) - local relative = sanitizeDevicePath(device_path) + local relative = sanitizeDevicePath(device_path, download_dir) if relative and not filename_override then return relative end local parent = relative and relative:match("^(.*)/[^/]+$") diff --git a/koreader-plugin/bookorbit.koplugin/main.lua b/koreader-plugin/bookorbit.koplugin/main.lua index d8e429edc..932ab840f 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.1" +local PLUGIN_VERSION = "1.5.2" local SYNC_STRATEGY = { PROMPT = 1, diff --git a/koreader-plugin/spec/bookorbit_catalog_bulk_download_test.lua b/koreader-plugin/spec/bookorbit_catalog_bulk_download_test.lua index 79ab3664f..423eddd96 100644 --- a/koreader-plugin/spec/bookorbit_catalog_bulk_download_test.lua +++ b/koreader-plugin/spec/bookorbit_catalog_bulk_download_test.lua @@ -92,7 +92,7 @@ package.loaded["util"] = { table.insert(made_paths, path) end, getSafeFilename = function(filename) - return filename + return filename:gsub("[\\/:*?\"<>|]", "_") end, removeFile = function(path) files[path] = nil @@ -188,9 +188,14 @@ G_reader_settings = { assertEqual(Catalog:getLocalDownloadPath("fallback", "epub", "Series/Book.epub"), "/downloads/Series/Book.epub", "valid device path") assertEqual(made_paths[1], "/downloads/Series", "valid device parent created") assertEqual(Catalog:getLocalDownloadPath("fallback", "epub", "./Series//Book.epub"), "/downloads/Series/Book.epub", "dot segments normalized") +assertEqual(Catalog:getLocalDownloadPath("fallback", "epub", "Standalone/Book: One.epub"), "/downloads/Standalone/Book_ One.epub", "device filename is filesystem safe") +assertEqual(made_paths[3], "/downloads/Standalone", "sanitized device filename keeps its parent") +assertEqual(Catalog:getLocalDownloadPath("fallback", "epub", "Series: One/Book? One.epub"), "/downloads/Series_ One/Book_ One.epub", "every device path segment is filesystem safe") +assertEqual(made_paths[4], "/downloads/Series_ One", "sanitized device parent is created") +made_paths = {} assertEqual(Catalog:getLocalDownloadPath("fallback", "epub", "../outside.epub"), "/downloads/fallback.epub", "parent traversal falls back") assertEqual(Catalog:getLocalDownloadPath("fallback", "epub", "Series\\..\\..\\outside.epub"), "/downloads/fallback.epub", "mixed separator traversal falls back") -assertEqual(#made_paths, 2, "traversal paths do not create directories") +assertEqual(#made_paths, 0, "traversal paths do not create directories") Catalog.settings = {} Catalog:initBulkDownloadState() @@ -608,12 +613,12 @@ local collision_ctx = { pending_matches = {}, completed = {}, } -local function collisionBook(id, title) +local function collisionBook(id, title, device_path) return { id = id, title = title, formats = { "epub" }, - files = { { id = 100 + id, format = "epub", devicePath = "Shared/Book.epub" } }, + files = { { id = 100 + id, format = "epub", devicePath = device_path } }, } end Catalog.bulkShowStatus = function() end @@ -621,21 +626,21 @@ Catalog.bulkDownloadFile = function(_, _, _, _, local_path) table.insert(transfers, local_path) return true end -Catalog:bulkProcessBook(collision_ctx, collisionBook(1, "First")) +Catalog:bulkProcessBook(collision_ctx, collisionBook(1, "First", "Shared/Book: One.epub")) collision_ctx.index = 2 -Catalog:bulkProcessBook(collision_ctx, collisionBook(2, "Second")) +Catalog:bulkProcessBook(collision_ctx, collisionBook(2, "Second", "Shared/Book? One.epub")) collision_ctx.index = 3 -Catalog:bulkProcessBook(collision_ctx, collisionBook(3, "Third")) +Catalog:bulkProcessBook(collision_ctx, collisionBook(3, "Third", "Shared/Book* One.epub")) -assertEqual(transfers[1], "/downloads/Shared/Book.epub", "first collision path remains unchanged") -assertEqual(transfers[2], "/downloads/Shared/Book [2].epub", "second collision path gains book identity") -assertEqual(transfers[3], "/downloads/Shared/Book [3].epub", "third collision path gains book identity") +assertEqual(transfers[1], "/downloads/Shared/Book_ One.epub", "first sanitized collision path remains unchanged") +assertEqual(transfers[2], "/downloads/Shared/Book_ One [2].epub", "second sanitized collision path gains book identity") +assertEqual(transfers[3], "/downloads/Shared/Book_ One [3].epub", "third sanitized collision path gains book identity") assertEqual(collision_ctx.counts.downloaded, 3, "all colliding books download") assertEqual(collision_ctx.counts.path_conflicts, 2, "collision count recorded") assertEqual(collision_ctx.path_conflicts[1].conflicting_book_id, 1, "collision owner recorded") -assertEqual(collision_ctx.path_conflicts[1].resolved_path, "/downloads/Shared/Book [2].epub", "resolved collision path recorded") -assertEqual(collision_ctx.destination_paths["/downloads/shared/book.epub"].book_id, 1, "original path ownership retained") -assertEqual(collision_ctx.completed["2"], "/downloads/Shared/Book [2].epub", "checkpoint records the renamed destination") +assertEqual(collision_ctx.path_conflicts[1].resolved_path, "/downloads/Shared/Book_ One [2].epub", "resolved collision path recorded") +assertEqual(collision_ctx.destination_paths["/downloads/shared/book_ one.epub"].book_id, 1, "original path ownership retained") +assertEqual(collision_ctx.completed["2"], "/downloads/Shared/Book_ One [2].epub", "checkpoint records the renamed destination") Catalog.bulkReleaseStandby = function() end Catalog:bulkFinish(collision_ctx) diff --git a/koreader-plugin/spec/bookorbit_catalog_download_test.lua b/koreader-plugin/spec/bookorbit_catalog_download_test.lua index 76c40b6c1..6abc0c1b0 100644 --- a/koreader-plugin/spec/bookorbit_catalog_download_test.lua +++ b/koreader-plugin/spec/bookorbit_catalog_download_test.lua @@ -66,8 +66,11 @@ package.loaded["util"] = { makePath = function(path) table.insert(made_paths, path) end, - getSafeFilename = function(filename) - return filename:gsub("[\\/:*?\"<>|]", "_") + getSafeFilename = function(filename, path) + if path and path:match("^/mnt/") then + return filename:gsub("[\\/:*?\"<>|]", "_") + end + return filename end, trim = function(value) return tostring(value or ""):match("^%s*(.-)%s*$") @@ -131,6 +134,27 @@ local resolved = Catalog:getLocalDownloadPath(detail.fallbackFilename, file.form assertEqual(resolved, "/mnt/onboard/library/Series/Southern Reach/1.00 - Annihilation.epub", "configured destination") assertEqual(made_paths[1], "/mnt/onboard/library/Series/Southern Reach", "configured parent created") +made_paths = {} +resolved = Catalog:getLocalDownloadPath( + "Bret Easton Ellis - The Shards: A Novel", + "epub", + "Standalone/Bret Easton Ellis - The Shards: A Novel.epub" +) +assertEqual(resolved, "/mnt/onboard/library/Standalone/Bret Easton Ellis - The Shards_ A Novel.epub", "device filename is filesystem safe") +assertEqual(made_paths[1], "/mnt/onboard/library/Standalone", "sanitized filename keeps configured parent") + +made_paths = {} +resolved = Catalog:getLocalDownloadPath("Fallback", "epub", "Series: One/Book? One.epub") +assertEqual(resolved, "/mnt/onboard/library/Series_ One/Book_ One.epub", "every device path segment is filesystem safe") +assertEqual(made_paths[1], "/mnt/onboard/library/Series_ One", "sanitized device parent is created") + +download_dir = "/library" +made_paths = {} +resolved = Catalog:getLocalDownloadPath("Fallback", "epub", "Standalone/Book: One.epub") +assertEqual(resolved, "/library/Standalone/Book: One.epub", "permissive filesystem keeps supported characters") +assertEqual(made_paths[1], "/library/Standalone", "permissive filesystem keeps configured parent") +download_dir = "/mnt/onboard/library" + made_paths = {} resolved = Catalog:getLocalDownloadPath("Preferred/Name", "EPUB", file.devicePath, true) assertEqual(resolved, "/mnt/onboard/library/Series/Southern Reach/Preferred_Name.epub", "filename override preserves configured folder")