From 3444b1605b66ef4f89c365e93875ba4f3d870ed3 Mon Sep 17 00:00:00 2001 From: Alex Shnitman Date: Sun, 16 Aug 2026 08:51:42 +0200 Subject: [PATCH] feat: allow a subscription's download folder to be changed (closes #1052) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The folder was already persisted on the subscription and already applied to every download it queued, but it was missing from the small tuple of fields the update route accepts, so it could be set when the subscription was created and never afterwards. That is the same gap the subscription name had in #1044. Add it to the accepted fields and validate it on the way in, following the validate_* helpers already in this module. The check is deliberately narrow — it rejects absolute paths and any '..' component, values that could never be valid — because the authoritative resolution stays where it already lives, in DownloadQueue at download time, along with the CUSTOM_DIRS / CREATE_CUSTOM_DIRS rules and the directory creation. Doing it this way reports a bad edit while the user is looking at the field instead of failing every check from then on, without a second copy of the path logic drifting out of step with the first. subscriptions.py cannot import ytdl.py in any case: ytdl imports _entry_id from it. An empty folder stays valid and means the base download directory. A change applies to future downloads only; files already downloaded are not moved. This covers the API side of the request. The subscriptions table does not show the folder at all today, so exposing it in the UI is a separate change. Co-Authored-By: Claude Opus 5 --- app/main.py | 2 +- app/subscriptions.py | 38 ++++++++++++++++++ app/tests/test_api.py | 27 +++++++++++++ app/tests/test_subscriptions.py | 69 +++++++++++++++++++++++++++++++++ 4 files changed, 135 insertions(+), 1 deletion(-) diff --git a/app/main.py b/app/main.py index 2f7a628..d995feb 100644 --- a/app/main.py +++ b/app/main.py @@ -967,7 +967,7 @@ async def subscriptions_update(request): k: v for k, v in post.items() if k != 'id' - and k in ('enabled', 'check_interval_minutes', 'name', 'title_regex', 'skip_subscriber_only') + and k in ('enabled', 'check_interval_minutes', 'name', 'folder', 'title_regex', 'skip_subscriber_only') } if not changes: raise web.HTTPBadRequest(reason='no valid fields to update') diff --git a/app/subscriptions.py b/app/subscriptions.py index 3c3803c..45cc293 100644 --- a/app/subscriptions.py +++ b/app/subscriptions.py @@ -311,6 +311,33 @@ def validate_subscription_name(value: Any) -> str: return name +def validate_subscription_folder(value: Any) -> str: + """Return a stored subscription folder, or raise ValueError if unusable. + + The folder is relative to the configured download directory, and the + authoritative check still happens at download time in ``DownloadQueue`` — + that is where ``CUSTOM_DIRS``, ``CREATE_CUSTOM_DIRS`` and the + resolves-inside-the-base-directory rule live, and where the directory is + created. This rejects only values that could never be valid, so an edit is + refused while the user is looking at it rather than silently failing every + check from then on. An empty folder is valid and means the base directory. + """ + if value is None: + return "" + if not isinstance(value, str): + raise ValueError("folder must be a string") + folder = value.strip() + if not folder: + return "" + if os.path.isabs(folder): + raise ValueError("folder must be relative to the download directory") + # Check both separators: the value is stored as typed, and a Windows-style + # path would otherwise carry an unexamined '..' past this point. + if any(part == ".." for part in folder.replace("\\", "/").split("/")): + raise ValueError('folder must not contain ".."') + return folder + + def _coerce_bool(value: Any) -> bool: """Accept JSON booleans and common string forms used by API clients.""" if isinstance(value, bool): @@ -705,6 +732,13 @@ class SubscriptionManager: except ValueError as exc: return {"status": "error", "msg": str(exc)} + validated_folder: Optional[str] = None + if "folder" in changes: + try: + validated_folder = validate_subscription_folder(changes["folder"]) + except ValueError as exc: + return {"status": "error", "msg": str(exc)} + validated_tr: Optional[str] = None if "title_regex" in changes: try: @@ -755,6 +789,10 @@ class SubscriptionManager: sub.check_interval_minutes = validated_interval if validated_name is not None: sub.name = validated_name + if validated_folder is not None: + # Applies to future downloads only; files already downloaded + # stay where they are. + sub.folder = validated_folder if validated_tr is not None: sub.title_regex = validated_tr if skip_so_set: diff --git a/app/tests/test_api.py b/app/tests/test_api.py index 629209f..9779769 100644 --- a/app/tests/test_api.py +++ b/app/tests/test_api.py @@ -380,6 +380,33 @@ async def test_subscriptions_update_invalid_interval_returns_error_not_500(mock_ assert body["status"] == "error" +@pytest.mark.asyncio +async def test_subscriptions_update_accepts_folder(monkeypatch, mock_dqueue): + """Issue #1052: folder was absent from the route's accepted fields, so a + folder-only update was rejected outright as having nothing to update.""" + submgr = MagicMock() + submgr.update_subscription = AsyncMock(return_value={"status": "ok"}) + monkeypatch.setattr(main, "submgr", submgr) + + req = _json_request({"id": "abc", "folder": "channels/jane"}) + resp = await main.subscriptions_update(req) + + assert resp.status == 200 + submgr.update_subscription.assert_awaited_once_with("abc", {"folder": "channels/jane"}) + + +@pytest.mark.asyncio +async def test_subscriptions_update_still_drops_unknown_fields(monkeypatch, mock_dqueue): + submgr = MagicMock() + submgr.update_subscription = AsyncMock(return_value={"status": "ok"}) + monkeypatch.setattr(main, "submgr", submgr) + + req = _json_request({"id": "abc", "seen_ids": ["x"], "url": "https://evil.example"}) + with pytest.raises(web.HTTPBadRequest): + await main.subscriptions_update(req) + submgr.update_subscription.assert_not_awaited() + + def test_is_within_state_dir_blocks_state_subtree(): state_dir = main._STATE_DIR_REAL assert main._is_within_state_dir(state_dir) diff --git a/app/tests/test_subscriptions.py b/app/tests/test_subscriptions.py index 7870cf1..d5d5ac5 100644 --- a/app/tests/test_subscriptions.py +++ b/app/tests/test_subscriptions.py @@ -891,6 +891,75 @@ class SubscriptionPersistenceTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(upd["status"], "ok") self.assertEqual(mgr.list_all()[0].name, "x" * 200) + async def test_update_subscription_changes_folder(self): + """Issue #1052: the folder was settable at creation and then frozen, + because it was never added to the fields the update route accepts.""" + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + self.assertEqual(mgr.list_all()[0].folder, "") + + upd = await mgr.update_subscription(sub_id, {"folder": " channels/jane "}) + self.assertEqual(upd["status"], "ok") + self.assertEqual(upd["subscription"]["folder"], "channels/jane") + self.assertEqual(mgr.list_all()[0].folder, "channels/jane") + + async def test_update_subscription_folder_survives_reload(self): + with tempfile.TemporaryDirectory() as tmp: + cfg = _Config(tmp) + mgr = SubscriptionManager(cfg, _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + await mgr.update_subscription(sub_id, {"folder": "archive"}) + + reloaded = SubscriptionManager(cfg, _Queue(), _Notifier()) + self.assertEqual(reloaded.get(sub_id).folder, "archive") + + async def test_update_subscription_clears_folder(self): + # An empty folder is valid and means the base download directory. + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + await mgr.update_subscription(sub_id, {"folder": "archive"}) + + upd = await mgr.update_subscription(sub_id, {"folder": " "}) + self.assertEqual(upd["status"], "ok") + self.assertEqual(mgr.list_all()[0].folder, "") + + async def test_update_subscription_rejects_unusable_folder(self): + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + await mgr.update_subscription(sub_id, {"folder": "keep"}) + + bad_values = ( + "/etc", + "/absolute/path", + "../escape", + "nested/../../escape", + "windows\\..\\escape", + 42, + ["a"], + ) + for bad in bad_values: + upd = await mgr.update_subscription(sub_id, {"folder": bad}) + self.assertEqual(upd["status"], "error", f"expected {bad!r} to be rejected") + self.assertEqual(mgr.list_all()[0].folder, "keep") + + async def test_update_subscription_folder_leaves_other_fields_alone(self): + with tempfile.TemporaryDirectory() as tmp: + mgr = SubscriptionManager(_Config(tmp), _Queue(), _Notifier()) + sub_id = await self._add_one_subscription(mgr) + before = mgr.get(sub_id) + name, interval, enabled = before.name, before.check_interval_minutes, before.enabled + + await mgr.update_subscription(sub_id, {"folder": "only/this"}) + + after = mgr.get(sub_id) + self.assertEqual(after.folder, "only/this") + self.assertEqual(after.name, name) + self.assertEqual(after.check_interval_minutes, interval) + self.assertEqual(after.enabled, enabled) + async def test_update_subscription_skip_subscriber_only(self): with tempfile.TemporaryDirectory() as tmp: queue = _Queue()