From e6b6eba1380f6f16cbf71b8397cd32276d4ee442 Mon Sep 17 00:00:00 2001 From: Steppenstreuner Date: Sat, 29 Aug 2026 16:38:25 +0200 Subject: [PATCH] fix(import): close the append path check and the relocate overwrite --- static/app.js | 16 ++++-- tests/test_import_flow.py | 110 ++++++++++++++++++++++++++++++++++++ wordarr/api/imports.py | 21 ++++--- wordarr/importer/mover.py | 37 +++++++++--- wordarr/importer/scanner.py | 13 +++++ wordarr/metadata/audible.py | 3 +- 6 files changed, 181 insertions(+), 19 deletions(-) diff --git a/static/app.js b/static/app.js index ca9b010..ffd14ed 100644 --- a/static/app.js +++ b/static/app.js @@ -42,10 +42,11 @@ function toast(msg, isError = false) { setTimeout(() => (t.hidden = true), 4000); } +// quotes included: the result also lands inside attributes (value="${esc(...)}") +const ESC_CHARS = { "&": "&", "<": "<", ">": ">", '"': """, "'": "'" }; + function esc(s) { - const d = document.createElement("div"); - d.textContent = s ?? ""; - return d.innerHTML; + return String(s ?? "").replace(/[&<>"']/g, (c) => ESC_CHARS[c]); } // last used library per media type @@ -1040,7 +1041,8 @@ $("#import-merge").addEventListener("click", () => { const name = commonPrefix(chosen.map((it) => it.name)) || parentName || chosen[0].name; const merged = { - path: chosen[0].path.slice(0, chosen[0].path.length - chosen[0].name.length - 1) || chosen[0].path, + path: chosen[0].dir || chosen[0].path, + dir: chosen[0].dir, name, rel_dir: chosen[0].rel_dir, media_type: chosen[0].media_type, @@ -1398,6 +1400,12 @@ async function collapseSharedRequests(chosen) { // deepest folder that holds all of the group's entries function sharedParent(group) { + // base64 paths carry no readable separators, so only the scanner's own + // parent dir says anything about them + if (group.some((it) => it.path.startsWith("b64:"))) { + const dirs = new Set(group.map((it) => it.dir)); + return dirs.size === 1 ? group[0].dir : group[0].path; + } const parts = group.map((it) => it.path.split("/")); const first = parts[0]; let i = 0; diff --git a/tests/test_import_flow.py b/tests/test_import_flow.py index fba07e8..eaaad6d 100644 --- a/tests/test_import_flow.py +++ b/tests/test_import_flow.py @@ -983,3 +983,113 @@ def test_a_single_folder_with_many_files_is_still_one_audiobook(client): (d / f"{i:02d}.mp3").write_bytes(b"") items = client.get("/api/import/scan").json()["items"] assert len(items) == 1 and items[0]["is_dir"] and len(items[0]["files"]) == 3 + + +def test_append_rejects_files_outside_download_dir(client): + """The append branch used to skip the download-dir check entirely, so it + would happily pull in - and clean up - folders anywhere on disk.""" + root = client.tmp_path / "library" / "k" + lib_id = client.post("/api/libraries", json={ + "name": "K", "media_type": "audiobook", "root_path": str(root), + "folder_template": "{Title}", "file_template": "{Title}", + }).json()["id"] + req_id = client.post("/api/requests", + json={"library_id": lib_id, "title": "Toteninsel"}).json()["id"] + + part_a = client.downloads / "A" + part_a.mkdir() + for i in (1, 2): + (part_a / f"{i:02d}.mp3").write_bytes(b"") + client.post("/api/import", json={"items": [{ + "path": str(part_a), "is_dir": True, + "files": [str(f) for f in sorted(part_a.iterdir())], "request_id": req_id, + }]}) + + outside = client.tmp_path / "elsewhere" + outside.mkdir() + stranger = outside / "private.mp3" + stranger.write_bytes(b"not yours") + + res = client.post("/api/import", json={"items": [{ + "path": str(outside), "is_dir": True, + "files": [str(stranger)], "request_id": req_id, "append": True, + }]}).json()["results"][0] + assert not res["ok"] + assert res["error"] == "path outside download dir" + assert stranger.exists() and outside.is_dir() + + +def test_import_survives_a_broken_base64_path(client): + """A malformed b64: payload is one bad item, not a 500 for the whole batch.""" + lib_id = client.post("/api/libraries", json={ + "name": "K", "media_type": "ebook", "root_path": str(client.tmp_path / "k"), + }).json()["id"] + req_id = client.post("/api/requests", + json={"library_id": lib_id, "title": "X"}).json()["id"] + resp = client.post("/api/import", json={"items": [{ + "path": "b64:!!!not base64!!!", "is_dir": False, + "files": [], "request_id": req_id, + }]}) + assert resp.status_code == 200 + assert not resp.json()["results"][0]["ok"] + + +def test_relocate_does_not_overwrite_within_the_same_folder(client): + """Renaming in place: a target name may already be held by another file of + the same set. Sorting decides who moves first, so the slot can still be + occupied when the move happens - it must not be overwritten.""" + root = client.tmp_path / "library" / "k" + lib_id = client.post("/api/libraries", json={ + "name": "K", "media_type": "audiobook", "root_path": str(root), + "folder_template": "{Title}", "file_template": "{Title}", + }).json()["id"] + req_id = client.post("/api/requests", + json={"library_id": lib_id, "title": "Insel"}).json()["id"] + src = client.downloads / "insel" + src.mkdir() + for i in (1, 2): + (src / f"{i:02d}.mp3").write_bytes(f"track {i}".encode()) + client.post("/api/import", json={"items": [{ + "path": str(src), "is_dir": True, + "files": [str(f) for f in sorted(src.iterdir())], "request_id": req_id, + }]}) + dest = root / "Insel" + + # "Aaa" sorts first, so it claims "Part 01" while part 1 still sits there + (dest / "Insel - Part 02.mp3").rename(dest / "Aaa.mp3") + res = client.post(f"/api/requests/{req_id}/relocate").json() + assert res["ok"], res + + assert sorted(f.name for f in dest.iterdir()) == [ + "Insel - Part 01.mp3", "Insel - Part 02.mp3", + ] + blobs = [f.read_bytes() for f in dest.iterdir()] + assert all(any(f"track {i}".encode() in b for b in blobs) for i in (1, 2)) + + +def test_single_file_folder_import_removes_the_empty_source(client): + lib_id = client.post("/api/libraries", json={ + "name": "K", "media_type": "audiobook", "root_path": str(client.tmp_path / "k"), + }).json()["id"] + req_id = client.post("/api/requests", + json={"library_id": lib_id, "title": "Solo"}).json()["id"] + folder = client.downloads / "solo release" + folder.mkdir() + (folder / "book.m4b").write_bytes(b"") + res = client.post("/api/import", json={"items": [{ + "path": str(folder), "is_dir": True, + "files": [str(folder / "book.m4b")], "request_id": req_id, + }]}).json()["results"][0] + assert res["ok"], res + assert not folder.exists() + + +def test_scan_does_not_loop_on_a_symlink_cycle(client): + from wordarr.importer import scanner + + d = client.downloads / "series" + d.mkdir() + (d / "book.m4b").write_bytes(b"") + (d / "back").symlink_to(client.downloads, target_is_directory=True) + items = scanner.scan(client.downloads) + assert [i["name"] for i in items] == ["book"] diff --git a/wordarr/api/imports.py b/wordarr/api/imports.py index 325322f..36962be 100644 --- a/wordarr/api/imports.py +++ b/wordarr/api/imports.py @@ -59,14 +59,26 @@ def do_import(data: ImportIn, session: Session = Depends(get_session)): download_root = Path(config.DOWNLOAD_DIR).resolve() for item in data.items: # the client hands back whatever the scan produced, base64 included - item.path = decode_path(item.path) - item.files = [decode_path(f) for f in item.files] + shown = item.path + try: + item.path = decode_path(item.path) + item.files = [decode_path(f) for f in item.files] + except Exception as exc: + results.append({"path": shown, "ok": False, + "error": f"unlesbarer Pfad: {exc}"}) + continue shown = readable(item.path) req = session.get(BookRequest, item.request_id) if not req: results.append({"path": shown, "ok": False, "error": f"Anfrage #{item.request_id} existiert nicht mehr"}) continue + # every branch below moves these files, so check them before branching. + # merged items carry files from several folders, so check them all + paths = [Path(p).resolve() for p in [item.path, *item.files]] + if any(download_root not in p.parents and p != download_root for p in paths): + results.append({"path": shown, "ok": False, "error": "path outside download dir"}) + continue path_gone = not req.imported_path or not Path(req.imported_path).exists() if req.status == "imported" and path_gone: # the library copy is gone; treat this as a fresh import @@ -89,11 +101,6 @@ def do_import(data: ImportIn, session: Session = Depends(get_session)): " — mehrere Einträge auf dieselbe Anfrage? Dann vorher zusammenfassen." )}) continue - # merged items carry files from several folders, so check them all - paths = [Path(p).resolve() for p in [item.path, *item.files]] - if any(download_root not in p.parents and p != download_root for p in paths): - results.append({"path": shown, "ok": False, "error": "path outside download dir"}) - continue try: dest = mover.import_item(item.path, item.files, item.is_dir, req, req.library) except Exception as exc: diff --git a/wordarr/importer/mover.py b/wordarr/importer/mover.py index 00a114d..2da46dd 100644 --- a/wordarr/importer/mover.py +++ b/wordarr/importer/mover.py @@ -30,7 +30,6 @@ def import_item(item_path: str, files: list[str], is_dir: bool, request, library Returns the destination path (folder for multi-file audiobooks, else the file).""" root = Path(library.root_path) folder = root / render_template(library.folder_template, request) - folder.mkdir(parents=True, exist_ok=True) base = render_template(library.file_template, request) paths = [Path(f) for f in files] @@ -49,6 +48,7 @@ def import_item(item_path: str, files: list[str], is_dir: bool, request, library + (f" (und {len(taken) - 1} weitere)" if len(taken) > 1 else "") + " — trägt ein anderer Titel nach diesem Namensschema denselben Namen?" ) + folder.mkdir(parents=True, exist_ok=True) for i, (src, dest) in enumerate(zip(paths, targets), 1): shutil.move(str(src), dest) if request.media_type == "audiobook": @@ -60,9 +60,13 @@ def import_item(item_path: str, files: list[str], is_dir: bool, request, library dest = folder / sanitize(f"{base}{src.suffix.lower()}") if dest.exists(): raise FileExistsError(f"Destination already exists: {dest}") + folder.mkdir(parents=True, exist_ok=True) shutil.move(str(src), dest) if request.media_type == "audiobook": tagger.tag_audio(dest, request) + if is_dir: + # a folder holding a single audiobook file is emptied by the move too + _cleanup_sources(Path(item_path), paths) return str(dest) @@ -134,16 +138,35 @@ def relocate(request, library) -> str: folder.mkdir(parents=True, exist_ok=True) width = max(2, len(str(len(audio)))) - for i, f in enumerate(audio, 1): - name = sanitize( + targets = [ + folder / sanitize( f"{base} - Part {i:0{width}d}{f.suffix.lower()}" if len(audio) > 1 else f"{base}{f.suffix.lower()}" ) - dest = folder / name - if dest.resolve() != f.resolve(): - shutil.move(str(f), dest) + for i, f in enumerate(audio, 1) + ] + # renaming in place may aim at a name that is already taken: fine when the + # occupant is one of the files being moved, never when it is a stranger + sources = {f.resolve() for f in audio} + taken = [d for d in targets if d.exists() and d.resolve() not in sources] + if taken or len(set(targets)) != len(targets): + raise FileExistsError( + f"Destination already exists: {taken[0] if taken else targets[0]}" + " — trägt ein anderer Titel nach diesem Namensschema denselben Namen?" + ) + current = list(audio) + for i, dest in enumerate(targets): + if current[i].resolve() != dest.resolve(): + if dest.exists(): + # a file further down the list still sits in this slot + j = next(k for k, c in enumerate(current) if c.resolve() == dest.resolve()) + stashed = dest.with_name(dest.name + ".wordarr-tmp") + shutil.move(str(current[j]), stashed) + current[j] = stashed + shutil.move(str(current[i]), dest) + current[i] = dest if request.media_type == "audiobook": - tagger.tag_audio(dest, request, track=i, total=len(audio)) + tagger.tag_audio(dest, request, track=i + 1, total=len(audio)) for extra in extras: target = folder / extra.name if target.resolve() != extra.resolve() and not target.exists(): diff --git a/wordarr/importer/scanner.py b/wordarr/importer/scanner.py index b9839fa..8f60024 100644 --- a/wordarr/importer/scanner.py +++ b/wordarr/importer/scanner.py @@ -113,13 +113,24 @@ def scan(root: Path, split_dirs: bool = False) -> list[dict]: except ValueError: return "" + seen: set[Path] = set() + def walk(d: Path): + # symlinked folders may point back up; without this a loop never ends + try: + real = d.resolve() + except OSError: + return + if real in seen: + return + seen.add(real) audio = audio_files(d) if not split_dirs: discs = disc_audio(d) if d != root else None if discs: items.append({ "path": encode_path(str(d)), + "dir": encode_path(str(d.parent)), "name": readable(d.name), "rel_dir": readable(rel_dir(d.parent)), "media_type": "audiobook", @@ -133,6 +144,7 @@ def scan(root: Path, split_dirs: bool = False) -> list[dict]: if len(audio) > 1 and d != root: items.append({ "path": encode_path(str(d)), + "dir": encode_path(str(d.parent)), "name": readable(d.name), "rel_dir": readable(rel_dir(d.parent)), "media_type": "audiobook", @@ -151,6 +163,7 @@ def scan(root: Path, split_dirs: bool = False) -> list[dict]: if mt: items.append({ "path": encode_path(str(entry)), + "dir": encode_path(str(d)), "name": readable(entry.stem), "rel_dir": readable(rel_dir(d)), "media_type": mt, diff --git a/wordarr/metadata/audible.py b/wordarr/metadata/audible.py index f3184fb..75a242a 100644 --- a/wordarr/metadata/audible.py +++ b/wordarr/metadata/audible.py @@ -37,7 +37,8 @@ async def _query(client: httpx.AsyncClient, tld: str, query: str, num_results: i resp = await client.get(f"https://api.audible.{tld}/1.0/catalog/products", params=params) resp.raise_for_status() return resp.json().get("products", []) - except httpx.HTTPError: + except (httpx.HTTPError, ValueError): + # a 200 that is not JSON (error page, captcha) must not kill the search return []