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()