A remembered path is trusted only under a folder configured now (#901)

dcc.py

@@ -2609,9 +2609,25 @@ def handle_download_request(irc_sock, user, requested_file, target_chan):
             # path found here goes through the same containment check below
             # as one the scan produces.
             remembered_key = (str(wanted_list), str(requested_file).lower().strip())
-            remembered = (_remembered_path(remembered_key)
-                          or _in_a_recent_folder(str(wanted_list), requested_file))
-            if remembered:
+
+            # UNDER A ROOT THAT IS CONFIGURED NOW (#901). The memories re-check
+            # that the file is still on disk, but a folder the operator has
+            # just removed from the library is still on disk too - so a path
+            # remembered under it passed, and is_safe_path() below then refused
+            # it: "invalid path" for a file the new configuration serves. #889
+            # drops the memories on !rehash, but the dashboard's Folders and
+            # Lists pages change the roots WITHOUT one (the folder set is read
+            # from its file on every call). Checked here, against the same roots the
+            # request is about to be judged by, it cannot outlive a root however
+            # the roots change; a hint outside them falls through to the next
+            # memory, and then to the scan.
+            def _under_a_current_root(path):
+                return bool(path) and any(is_safe_path(root, path) for root in search_roots)
+
+            remembered = _remembered_path(remembered_key)
+            if not _under_a_current_root(remembered):
+                remembered = _in_a_recent_folder(str(wanted_list), requested_file)
+            if _under_a_current_root(remembered):
                 full_path = remembered
 
         if not is_master_zip and not os.path.exists(platform_compat.long_path(full_path)):

docs/UPDATES-PUBLIC.md

@@ -4,7 +4,7 @@
 
 - **Added (mIRC console): the @DCCore window's button now lights up like a channel's.** It turns the message colour (red, by default) when there is new activity - a request, a send, a search - and the highlight colour on a failed transfer, so failures stand out. The five-minute status line and joins, parts and bans do not light it, as they would not in a channel. Needs mIRC 7 or later; update `dccore.mrc` and reload it.
 - **Fixed: pasting a batch of requests could mute you and then ban you for an hour, and the mute notice wrongly said your queue had been cleared.** Asking for a file counted toward the flood protection exactly like a search, so pasting about a dozen rows from the list - one album, the ordinary way these lists are used - could get the eleventh line muted and the twelfth banned for an hour, on any client or connection that sends a paste quickly. Asking for files is no longer rate-limited at all: the limit is your queue, as it always should have been (100 files each by default), and past it you are told once rather than once per line. Searches and the other commands are unchanged. The mute notice was also wrong in a way that caused the ban - what it drops is the bot's pending replies to you, never your queued files - so it now says that other commands are ignored for the next 30 seconds and that your queued files are safe.
-- **Fixed: pasting several lines from the list at once could be answered "Busy looking up other files - try again in a moment".** Requesting a file meant searching the library for it every single time - even for a file just sent, and even for the file next to it in the same album - and only two of those searches could run at once, so most of a pasted batch was refused outright (and "try again in a moment" is the one thing that risks tripping the flood protection). The bot now remembers where it found a file and which folders it has been finding them in, so a batch from one album costs one search instead of nine, and a request that arrives while the library is busy waits its turn instead of being turned away. What it remembers is dropped on `!rehash`, so changing your library folders takes effect for requests straight away rather than a few minutes later.
+- **Fixed: pasting several lines from the list at once could be answered "Busy looking up other files - try again in a moment".** Requesting a file meant searching the library for it every single time - even for a file just sent, and even for the file next to it in the same album - and only two of those searches could run at once, so most of a pasted batch was refused outright (and "try again in a moment" is the one thing that risks tripping the flood protection). The bot now remembers where it found a file and which folders it has been finding them in, so a batch from one album costs one search instead of nine, and a request that arrives while the library is busy waits its turn instead of being turned away. What it remembers only counts for folders that are still in your library, so changing your library folders - on the dashboard's Folders page or by a `!rehash` - takes effect for requests straight away rather than a few minutes later.
 - **A download that fails because your client never answered now says so, to the person downloading.** They used to be told only "transfer did not complete", which names nothing they can act on - while the two causes are both on their side: a DCC prompt nobody accepted in time, or a client set to ignore that kind of file (a user who could take a `.jpg` but never a `.nfo` is the usual shape). The notice now says *your client never accepted it*, and what to try.
 
 ## v1.13.0 — The Audit Release

docs/UPDATES.md

@@ -111,6 +111,18 @@ path* for a name the new configuration serves perfectly well. Nothing about the
 self-heals it, and without the call the wrong answer lasts `LOOKUP_HIT_TTL_SECONDS`. A rebuild (`!update`)
 needs no such call - it changes the lists, not where the files are.
 
+**And the call was not enough on its own (#901).** The dashboard's Folders and Lists pages change the library
+*without* a rehash - they save the file and return, and `library.folders()` reads it on every call - so a folder
+removed there still left *invalid path* for five minutes. A remembered path is now trusted only under a root
+configured at the moment of the request (`is_safe_path()` against that request's own `search_roots`, the check
+it is judged by afterwards anyway); outside them it falls through to the folder memory and then the scan. That
+covers the dashboard, a hand-edited `lists.json`, and a scan still in flight across a rehash that records its
+hit after the forget. The rehash call stays - it frees the memory. `test_a_path_from_a_root_that_is_gone_is_...`
+used to assert the wrong answer as the documented cost of not forgetting; it now asserts the right one with no
+forget at all. `tests/test_a_folder_removed_on_the_dashboard_is_not_served_from_memory.py` (3) changes
+`library.folders` the way the page's save does: the name memory and the folder memory both fall through to the
+new root, and with the folders unchanged the memory still saves the scan.
+
 `tests/test_a_batch_of_requests_is_not_refused.py` (10): one scan
 for a whole pasted batch, a repeat costs nothing, a sibling costs nothing, a file that has since gone is not
 served from memory, the TTL and both caps, the slot is waited for rather than bounced, "busy" still arrives

tests/test_a_folder_removed_on_the_dashboard_is_not_served_from_memory.py

@@ -0,0 +1,110 @@
+"""A folder removed on the dashboard does not leave "invalid path" behind (#901).
+
+#886's lookup memories remember where a name was found and which folders
+lookups landed in; #889 drops them on !rehash. But the dashboard's Folders
+and Lists pages change the library WITHOUT a rehash - they save the file and
+return, and library.folders() reads it on every call - so a path remembered
+under the folder just removed was still on disk, passed the memory's own
+existence check, and was then refused by is_safe_path() against the new
+roots: "Error: Invalid path." for a file the new configuration serves,
+for up to LOOKUP_HIT_TTL_SECONDS.
+
+A remembered path is now trusted only under a root that is configured at
+the moment of the request. Driven through the real request path, changing
+library.folders the way the page's save does - no rehash, no forget.
+"""
+
+import os
+import shutil
+import sys
+import unittest
+from unittest import mock
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+if REPO_ROOT not in sys.path:
+    sys.path.insert(0, REPO_ROOT)
+
+import dcc  # noqa: E402
+import defaults as config  # noqa: E402
+import library  # noqa: E402
+
+from tests.test_an_unknown_filename_does_not_scan_the_library_unbounded import LookupBase  # noqa: E402
+from tests.test_path_security import InlineThread  # noqa: E402
+
+
+class TheDashboardMovesTheLibrary(LookupBase):
+
+    def setUp(self):
+        super().setUp()
+        # What an operator moving to another disk has: the same album,
+        # complete, under a new root - and the old one still on disk.
+        self.new_root = os.path.join(self.tree.root, "new-disk")
+        shutil.copytree(self.tree.music, self.new_root)
+
+    def a_track(self, index=0):
+        return os.path.basename(self.tree.tracks[index])
+
+    def only_the_new_root(self):
+        """What the Folders page's save leaves behind: the new folder set,
+        read live by the next request. Nothing calls forget_library_lookups()."""
+        return mock.patch.object(library, "folders",
+                                 lambda name=None: [library.Folder("Music", self.new_root)])
+
+    def sent_paths(self):
+        return [args[2] for name, args in InlineThread.dispatched if name == "start_dcc_send"]
+
+    def the_first_send_is_done(self):
+        """The first file went out and finished, before the operator moved the
+        library - its transfer and its per-user send lock released, as a
+        completed send releases them. Without this the next request queues
+        rather than sends, and the path read would be the FIRST one's, from
+        before the move."""
+        config.active_transfers[:] = []
+        config.dcc_queue.clear()
+        config.user_processing_lock.clear()
+        InlineThread.dispatched[:] = []
+
+    def test_a_remembered_file_is_served_from_the_new_root(self):
+        name = self.a_track()
+        self.ask(name)
+        self.assertEqual(self.errors(), [], "the first request should have been served")
+        self.the_first_send_is_done()
+
+        with self.only_the_new_root():
+            self.ask(name)
+
+        self.assertEqual(self.errors(), [], "a path remembered under the removed folder was used")
+        self.assertEqual(len(self.sent_paths()), 1, InlineThread.dispatched)
+        self.assertTrue(self.sent_paths()[0].startswith(self.new_root),
+                        "sent from %s, not from the new root" % self.sent_paths()[0])
+
+    def test_a_remembered_folder_outside_the_new_roots_is_not_used_either(self):
+        """The folder memory, not the name memory: a sibling of a file found
+        before the change, asked for after it."""
+        self.ask(self.a_track(0))
+        dcc._lookup_hits.clear()          # only the folder memory is left
+        self.the_first_send_is_done()
+
+        with self.only_the_new_root():
+            self.ask(self.a_track(1))
+
+        self.assertEqual(self.errors(), [])
+        self.assertEqual(len(self.sent_paths()), 1, InlineThread.dispatched)
+        self.assertTrue(self.sent_paths()[0].startswith(self.new_root), self.sent_paths()[0])
+
+    def test_with_the_folders_unchanged_the_memory_still_saves_the_scan(self):
+        """The control: #886's point is kept. A path under a root that is
+        still configured is used as it was, with no second scan."""
+        name = self.a_track()
+        self.ask(name)
+        scans = len(self.walks)
+        self.assertEqual(scans, 1, "the first request should have scanned")
+
+        self.ask(name)
+
+        self.assertEqual(len(self.walks), scans, "a remembered path under a live root was not used")
+        self.assertEqual(self.errors(), [])
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/test_a_rehash_forgets_where_files_were.py

@@ -53,16 +53,22 @@ def point_the_library_at(self, root):
         """What the reload does to search_roots, without reloading."""
         config.FILE_DIRECTORY = root
 
-    def test_a_path_from_a_root_that_is_gone_is_refused_rather_than_served(self):
+    def test_a_path_from_a_root_that_is_gone_is_not_used_even_without_a_forget(self):
+        """#901. This used to assert the bug - "invalid_path" with the
+        memories kept - because #889's rehash call was the only thing that
+        dropped them, and the dashboard's Folders and Lists pages change the
+        roots without a rehash. A remembered path is now trusted only under a
+        root that is configured at the moment of the request, so no forget is
+        needed for the right answer; the rehash still calls it to free them."""
         name = self.a_track()
         self.ask(name)
         self.assertEqual(self.errors(), [], "the first request should have been served")
 
         self.point_the_library_at(self.moved_root)
         self.ask(name)
 
-        self.assertEqual(self.errors(), ["invalid_path"],
-                         "the remembered path survived the root it came from")
+        self.assertEqual(self.errors(), [],
+                         "a path remembered under a removed root was used")
 
     def test_forgetting_them_is_what_serves_it_from_the_new_root(self):
         name = self.a_track()