From 8372f99386cc5b41e94c347ee025ec9eb9110bed Mon Sep 17 00:00:00 2001 From: Mancolt Date: Tue, 22 Sep 2026 11:25:37 -0400 Subject: [PATCH] target path: dot components become _, and the finished path must lie under the media root pathvalidate.sanitize_filename leaves . and .. alone, so a series, title or part of .. from Audible/MAM walked one level up: inside the media root with the default templates, outside it with a bare-token template. Every rendered template component (disc_folder included, now split on / like the others) goes through myx_utilities.pathComponent, which turns an all-dots name into _. myx_utilities.assertUnderRoot then checks the finished path against the media root (realpath + commonpath, so a root of / or a symlinked root is fine and a folder inside the library that links elsewhere is not) and raises rather than link outside it. In metadata: log mode the CSV paths column is checked the same way against the run's Config/paths media roots. Output for every ordinary path is unchanged. Tests for .. as series/title/part under default and bare-token templates, disc_folder, a media root of /, and a log row aimed outside the library. Reviewed (correctness and security): findings applied, re-review clean. Co-authored-by: Cursor --- CHANGELOG.md | 8 ++++++ docs/FORK.md | 1 + myx_classes.py | 15 ++++++++--- myx_utilities.py | 20 ++++++++++++++ tests/test_names.py | 66 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44587f7..353f055 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ first release is 3.0.0 because exit codes and the cookie store changed in ways a ## Unreleased +### Fixed +- A series, title or part rendered as `..` (or `.`) by a target_path template was left alone by the filename + sanitiser and walked one level up: inside the media root with the default templates, outside it with a + template of bare tokens such as `{series}/{part}/{title}`. Such a component now becomes `_` (in `disc_folder` + too), and the finished target path is checked against the media root before any file is linked or copied. In + `metadata: log` mode the log's `paths` column, written by an earlier run or edited by hand, is checked the same + way against the run's `Config/paths` media roots. + ### Fixed - Two releases that both use a bitrate folder (`Title/64k/`, `Title/MP3/128kbps/`) were grouped as one book named `64k` and hardlinked to the first match. Grouping now walks past bitrate folders the same diff --git a/docs/FORK.md b/docs/FORK.md index 1753bf5..f7f2da5 100644 --- a/docs/FORK.md +++ b/docs/FORK.md @@ -33,6 +33,7 @@ Scripts built on upstream booktree keep working: | 6b | Multi-disc releases (`cd1/`, `Disc 01/`) are grouped per disc folder, so each disc is matched on its own, the runtime evidence is one disc long (the wrong edition can win), and `book` logs as `cd1..cdN`. Two releases that both use `cd1/` were also merged into one book. Fixed: grouping walks past disc parents to the release folder | #26 | | 8 | Everything around a run lived in per-host wrapper scripts (ntfy summary, Audiobookshelf scan, inode de-dupe, an ASIN fixer that rewrote `fix.csv`). Added as config, off by default: `notify`, `abs`, `dedupe_roots`, `--pin RELEASE=ASIN` with `--remember` to keep the correction in the hints file (see "Correcting a match" in CONFIG.md) | — | | 9 | A tagged ASIN Audible has no product for (skeleton answer) or that names another book ends the Audible search: the per-ASIN endpoint ignores title/author and no plain search followed. Fixed in 3.0.2: fall back to the title/author search through the same gates, as pinned ASINs already did (see CONFIG.md, release-name parsing) | — | +| 10 | Audible/MAM metadata is remote input, yet a series/title/part of `..` passed `sanitize_filename` unchanged and became a path component; with a template of bare tokens it could leave the media root. Fixed in 3.0.4: `..`/`.` components become `_` and the target path is verified to lie under the media root | — | | 7 | mousehole cookie integration. Fixed: `mousehole_state_file` / `MOUSEHOLE_STATE_FILE`, reads mousehole's v2 (`cookie`) and legacy (`currentCookie`) state files (PR #24 read only the legacy key) | #24 | ## Regression replay diff --git a/myx_classes.py b/myx_classes.py index e39548f..f3fae2f 100644 --- a/myx_classes.py +++ b/myx_classes.py @@ -413,19 +413,22 @@ def getConfigTargetPath(self, cfg, book): x = template.format (**tokens) #use in_series format for p in x.split ("/"): - sPath=os.path.join (sPath, p.strip()) + sPath=os.path.join (sPath, myx_utilities.pathComponent(p)) else: y = no_series.format (**tokens) #use no_series format for p in y.split ("/"): - sPath=os.path.join (sPath, p.strip()) + sPath=os.path.join (sPath, myx_utilities.pathComponent(p)) #add disc for multidisc if len(disc): z = disc_folder.format (**tokens) - sPath=os.path.join(sPath, z.strip()) + for p in z.split ("/"): + sPath=os.path.join(sPath, myx_utilities.pathComponent(p)) - return os.path.join(media_path, sPath) + #every component above went through pathComponent, so this cannot trip on metadata alone; it is the + #last line of defence (a folder already in the library that is a symlink to somewhere outside it). + return myx_utilities.assertUnderRoot(os.path.join(media_path, sPath), [media_path]) def getTargetPaths(self, book, cfg): return self.getConfigTargetPath(cfg, book) @@ -984,7 +987,11 @@ def createHardLinks(self, cfg): #UPDATED 8/30 to allow users to customize target_path formats p = "" if ((metadata == "log") and self.isMatched): + #the log's paths column is what an earlier run computed, possibly edited by the operator; it must + #still lie under one of this run's media roots before anything is linked or copied there p = self.paths + if len(p): + p = myx_utilities.assertUnderRoot(p, [e["media_path"] for e in (cfg.get("Config/paths") or []) if isinstance(e, dict) and e.get("media_path")] or [f.mediaPath]) if (len(p) == 0): p = f.getConfigTargetPath(cfg, self.metadataBook) diff --git a/myx_utilities.py b/myx_utilities.py index f6b0bed..bbd0283 100644 --- a/myx_utilities.py +++ b/myx_utilities.py @@ -259,6 +259,26 @@ def removeGA (author:str): cleanAuthor = author.replace("GraphicAudio","").replace("[","").replace("]","") return cleanAuthor.strip() +def pathComponent(p): + """One folder name rendered from a target_path template. pathvalidate's sanitize_filename leaves "." and ".." + alone, so a series or title of ".." from Audible/MAM (or a stray template) would walk out of the folder it + belongs in. Such a name becomes "_"; a name that is only dots is treated the same.""" + p = p.strip() + if p and p.strip(".") == "": + return "_" + return p + +def assertUnderRoot(target, roots): + """Raise ValueError unless `target` (which need not exist yet) resolves to a path inside one of `roots`. + Both sides are resolved through symlinks, so a media root that is itself a symlink is fine and a folder + already inside the library that links elsewhere is not. A root of "/" contains everything.""" + real = os.path.realpath(target) + for root in roots: + r = os.path.realpath(root) + if os.path.commonpath([r, real]) == r: + return target + raise ValueError(f"target path {target!r} is outside the media root(s) {list(roots)!r}") + def cleanseSeries(series): #remove colons cleanSeries = series diff --git a/tests/test_names.py b/tests/test_names.py index d84f561..29d0ceb 100644 --- a/tests/test_names.py +++ b/tests/test_names.py @@ -1,5 +1,7 @@ import os import unittest +import unittest.mock +import myx_utilities import myx_names as N from tests.support import FakeConfig @@ -413,6 +415,43 @@ def test_series_without_a_part_uses_the_no_part_template(self): self.assertEqual(self.target("3"), "/lib/Lee Child/Jack Reacher/Jack Reacher #3 - Three More Novellas") self.assertEqual(self.target(5), "/lib/Lee Child/Jack Reacher/Jack Reacher #5 - Three More Novellas") + def test_dot_dot_components_cannot_leave_the_media_root(self): + # pathvalidate leaves "." and ".." alone; a series/title/part of ".." from Audible or MAM used to walk up a + # level (inside the root with the default templates, outside with a template of bare tokens) + import myx_classes + def target(series, title, part="", tmpl=None): + book = myx_classes.Book(asin="B000000001", title=title) + book.authors = [myx_classes.Contributor("Lee Child")] + book.series = [myx_classes.Series(series, part)] + over = {"Config/target_path/in_series_no_part": tmpl, "Config/target_path/in_series": tmpl} if tmpl else {} + cfg = FakeConfig("/tmp", **over) + return myx_classes.BookFile("x.m4b", "/dl/x.m4b", "/dl", "/lib").getConfigTargetPath(cfg, book) + self.assertEqual(target("..", "T"), "/lib/Lee Child/_/.. - T") # ".. - T" is a name, not a traversal + self.assertEqual(target("S", ".", "3"), "/lib/Lee Child/S/S #3 - .") # a dot inside a name is fine + self.assertEqual(target("..", "..", tmpl="{author}/{series}/{title}"), "/lib/Lee Child/_/_") + self.assertEqual(target("..", "..", "..", tmpl="{series}/{part}/{title}"), "/lib/_/_/_") + self.assertEqual(target("Jack Reacher", "Killing Floor", "1", tmpl="{series}/{part}/{title}"), + "/lib/Jack Reacher/1/Killing Floor") + # a media root of "/" (or a symlink to it) and a sibling folder that merely shares a prefix are handled + book = myx_classes.Book(asin="B000000001", title="Killing Floor") + book.authors = [myx_classes.Contributor("Lee Child")] + cfg = FakeConfig("/tmp", **{"Config/target_path/no_series": "{author}/{title}"}) + self.assertEqual(myx_classes.BookFile("x.m4b", "/dl/x.m4b", "/dl", "/").getConfigTargetPath(cfg, book), + "/Lee Child/Killing Floor") + self.assertEqual(myx_classes.BookFile("x.m4b", "/dl/x.m4b", "/dl", "/lib/").getConfigTargetPath(cfg, book), + "/lib/Lee Child/Killing Floor") + # disc_folder is split on "/" like the other templates (a bare-token disc template used to be one component) + noSeries = myx_classes.Book(asin="B000000001", title="..") + noSeries.authors = [myx_classes.Contributor("Lee Child")] + self.assertEqual(myx_classes.BookFile("Title/cd1/x.m4b", "/dl/Title/cd1/x.m4b", "/dl", "/lib").getConfigTargetPath( + FakeConfig("/tmp", **{"Config/target_path/no_series": "{author}/{title}", + "Config/target_path/disc_folder": "{disc}/{title}/.."}), + noSeries), "/lib/Lee Child/_/cd1/_/_") + # the last line of defence: a component that still escapes raises instead of returning a path outside the root + with unittest.mock.patch.object(myx_utilities, "pathComponent", lambda p: p.strip()): + with self.assertRaises(ValueError): + target("..", "..", "..", tmpl="{series}/{part}/{title}") + def test_series_part_none_or_int_does_not_crash_getSeriesPart(self): import myx_classes self.assertEqual(myx_classes.Series("Jack Reacher", None).getSeriesPart(), "Jack Reacher") @@ -427,3 +466,30 @@ def test_template_is_configurable_and_the_old_layout_can_be_kept(self): "/lib/Lee Child/Jack Reacher/Jack Reacher # - Three More Novellas") self.assertEqual(self.target("", **{"Config/target_path/in_series_no_part": ""}), # blank = default "/lib/Lee Child/Jack Reacher/Jack Reacher - Three More Novellas") + + +class LogModePathContainmentTest(unittest.TestCase): + def test_log_paths_outside_every_media_root_are_refused(self): + # metadata=log links to the CSV's paths column as written by an earlier run (or edited by hand); a value + # that resolves outside this run's media roots is refused before anything is created there + import myx_classes, tempfile + with tempfile.TemporaryDirectory() as td: + lib, seed = os.path.join(td, "lib"), os.path.join(td, "seed") + os.makedirs(lib); os.makedirs(seed) + src = os.path.join(seed, "a.m4b"); open(src, "w").close() + book = myx_classes.MAMBook("Title") + bf = myx_classes.BookFile("a.m4b", src, seed, lib) + book.files = [bf] + book.isMatched = True + book.metadata = "audible" + book.bestAudibleMatch = myx_classes.Book(asin="B000000001", title="Title") + book.bestAudibleMatch.authors = [myx_classes.Contributor("Lee Child")] + cfg = FakeConfig(td, **{"Config/metadata": "log", "Config/flags/dry_run": 0, "Config/flags/hardlink": 1, + "Config/paths": [{"files": "*.m4b", "source_path": seed, "media_path": lib}]}) + book.paths = os.path.join(lib, "..", "seed", "elsewhere") + with self.assertRaises(ValueError): + book.createHardLinks(cfg) + self.assertFalse(os.path.exists(os.path.join(seed, "elsewhere"))) + book.paths = os.path.join(lib, "Lee Child", "Title") # a normal log path is linked as before + book.createHardLinks(cfg) + self.assertTrue(os.path.exists(os.path.join(lib, "Lee Child", "Title", "a.m4b")))