Merge 40a25de926 into e6be419a30

app/user_manager.py

@@ -82,7 +82,7 @@ class UserManager():
         path = user_root
 
         # prevent leaving /{type}
-        if os.path.commonpath((root_dir, user_root)) != root_dir:
+        if not folder_paths.is_path_within_directory(user_root, root_dir):
             return None
 
         if file is not None:
@@ -92,7 +92,7 @@ class UserManager():
 
             # prevent leaving /{type}/{user}
             path = os.path.abspath(os.path.join(user_root, file))
-            if os.path.commonpath((user_root, path)) != user_root:
+            if not folder_paths.is_path_within_directory(path, user_root):
                 return None
 
         parent = os.path.split(path)[0]

comfy/sd1_clip.py

@@ -425,7 +425,10 @@ def load_embed(embedding_name, embedding_directory, embedding_size, embed_key=No
         try:
             if os.path.commonpath((embed_dir, embed_path)) != embed_dir:
                 continue
-        except:
+        except ValueError:
+            # On Windows, commonpath raises ValueError when paths are
+            # on different drives.  The embedding is clearly not inside
+            # this directory, so skip it.
             continue
         if not os.path.isfile(embed_path):
             extensions = ['.safetensors', '.pt', '.bin']

folder_paths.py

@@ -11,6 +11,30 @@ from comfy.cli_args import args
 
 supported_pt_extensions: set[str] = {'.ckpt', '.pt', '.pt2', '.bin', '.pth', '.safetensors', '.pkl', '.sft'}
 
+
+def is_path_within_directory(path: str, directory: str) -> bool:
+    """Check whether 'path' is located inside 'directory'.
+
+    This is a safe replacement for the pattern::
+
+        os.path.commonpath((directory, path)) == directory
+
+    which raises ``ValueError`` on Windows when the two paths reside on
+    different drives (e.g. ``C:\\`` vs ``D:\\``).  When the drives differ the
+    path is clearly **not** inside the directory, so we return ``False``
+    instead of letting the exception propagate.
+
+    Both arguments are normalised with ``os.path.abspath`` before comparison.
+    """
+    try:
+        return os.path.commonpath((os.path.abspath(path), os.path.abspath(directory))) == os.path.abspath(directory)
+    except ValueError:
+        # On Windows, commonpath raises ValueError when paths are on
+        # different drives.  Different drives means the path is not
+        # within the directory.
+        return False
+
+
 folder_names_and_paths: dict[str, tuple[list[str], set[str]]] = {}
 
 # --base-directory - Resets all default paths configured in folder_paths with a new base path
@@ -455,11 +479,10 @@ def get_save_image_path(filename_prefix: str, output_dir: str, image_width=0, im
 
     full_output_folder = os.path.join(output_dir, subfolder)
 
-    if os.path.commonpath((output_dir, os.path.abspath(full_output_folder))) != output_dir:
+    if not is_path_within_directory(full_output_folder, output_dir):
         err = "**** ERROR: Saving image outside the output folder is not allowed." + \
               "\n full_output_folder: " + os.path.abspath(full_output_folder) + \
-              "\n         output_dir: " + output_dir + \
-              "\n         commonpath: " + os.path.commonpath((output_dir, os.path.abspath(full_output_folder)))
+              "\n         output_dir: " + output_dir
         logging.error(err)
         raise Exception(err)
 

server.py

@@ -398,7 +398,7 @@ class PromptServer():
                 full_output_folder = os.path.join(upload_dir, os.path.normpath(subfolder))
                 filepath = os.path.abspath(os.path.join(full_output_folder, filename))
 
-                if os.path.commonpath((upload_dir, filepath)) != upload_dir:
+                if not folder_paths.is_path_within_directory(filepath, upload_dir):
                     return web.Response(status=400)
 
                 if not os.path.exists(full_output_folder):
@@ -476,7 +476,7 @@ class PromptServer():
 
                 if original_ref.get("subfolder", "") != "":
                     full_output_dir = os.path.join(output_dir, original_ref["subfolder"])
-                    if os.path.commonpath((os.path.abspath(full_output_dir), output_dir)) != output_dir:
+                    if not folder_paths.is_path_within_directory(full_output_dir, output_dir):
                         return web.Response(status=403)
                     output_dir = full_output_dir
 
@@ -533,7 +533,7 @@ class PromptServer():
 
                     if "subfolder" in request.rel_url.query:
                         full_output_dir = os.path.join(output_dir, request.rel_url.query["subfolder"])
-                        if os.path.commonpath((os.path.abspath(full_output_dir), output_dir)) != output_dir:
+                        if not folder_paths.is_path_within_directory(full_output_dir, output_dir):
                             return web.Response(status=403)
                         output_dir = full_output_dir
 

tests-unit/comfy_test/folder_path_test.py

@@ -160,3 +160,35 @@ def test_base_path_change_clears_old(set_base_dir):
 
     for name in ["controlnet", "diffusion_models", "text_encoders"]:
         assert len(folder_paths.get_folder_paths(name)) == 2
+
+
+class TestIsPathWithinDirectory:
+    """Tests for folder_paths.is_path_within_directory."""
+
+    def test_path_inside_directory(self, temp_dir):
+        inner = os.path.join(temp_dir, "sub", "file.txt")
+        assert folder_paths.is_path_within_directory(inner, temp_dir) is True
+
+    def test_path_is_directory_itself(self, temp_dir):
+        assert folder_paths.is_path_within_directory(temp_dir, temp_dir) is True
+
+    def test_path_outside_directory(self, temp_dir):
+        outside = os.path.join(temp_dir, "..", "other")
+        assert folder_paths.is_path_within_directory(outside, temp_dir) is False
+
+    def test_sibling_directory(self, temp_dir):
+        sibling = os.path.join(os.path.dirname(temp_dir), "sibling")
+        assert folder_paths.is_path_within_directory(sibling, temp_dir) is False
+
+    def test_parent_not_within_child(self, temp_dir):
+        parent = os.path.dirname(temp_dir)
+        assert folder_paths.is_path_within_directory(parent, temp_dir) is False
+
+    def test_different_drives_returns_false(self):
+        """Simulate the Windows cross-drive scenario (issue #1488).
+
+        On non-Windows systems os.path.commonpath won't raise ValueError
+        for these paths, so we mock it to verify the except-branch.
+        """
+        with patch("folder_paths.os.path.commonpath", side_effect=ValueError("Paths don't have the same drive")):
+            assert folder_paths.is_path_within_directory("D:\\data\\img.png", "C:\\ComfyUI\\output") is False