mirror of
https://github.com/comfyanonymous/ComfyUI.git
synced 2026-06-26 09:49:26 +08:00
Fix ValueError in /view and /upload/mask when subfolder is on a different drive
os.path.commonpath raises ValueError when comparing paths that don't share a drive (e.g. on Windows when output_dir is on C: and the resolved subfolder ends up on D:), so a malicious or malformed `subfolder` query/field crashed these handlers with an unhandled exception instead of returning 403. Extract the check into is_path_within_directory(), which treats a different-drive ValueError as "not within" and returns False, restoring the intended 403 response. Fixes #1488
This commit is contained in:
parent
38f750d80e
commit
88a5a1862e
18
server.py
18
server.py
@ -200,6 +200,20 @@ def create_block_external_middleware():
|
||||
return block_external_middleware
|
||||
|
||||
|
||||
def is_path_within_directory(path, directory):
|
||||
"""Returns True if `path` resolves to a location inside `directory`.
|
||||
|
||||
os.path.commonpath raises ValueError when the two paths don't share a
|
||||
drive (e.g. on Windows when one is on C: and the other on D:). That case
|
||||
simply means `path` cannot be inside `directory`, so it is treated as such
|
||||
rather than letting the exception bubble up as an unhandled error.
|
||||
"""
|
||||
try:
|
||||
return os.path.commonpath((os.path.abspath(path), directory)) == directory
|
||||
except ValueError:
|
||||
return False
|
||||
|
||||
|
||||
class PromptServer():
|
||||
def __init__(self, loop):
|
||||
PromptServer.instance = self
|
||||
@ -477,7 +491,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 is_path_within_directory(full_output_dir, output_dir):
|
||||
return web.Response(status=403)
|
||||
output_dir = full_output_dir
|
||||
|
||||
@ -534,7 +548,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 is_path_within_directory(full_output_dir, output_dir):
|
||||
return web.Response(status=403)
|
||||
output_dir = full_output_dir
|
||||
|
||||
|
||||
42
tests-unit/server_test/test_is_path_within_directory.py
Normal file
42
tests-unit/server_test/test_is_path_within_directory.py
Normal file
@ -0,0 +1,42 @@
|
||||
"""Tests for server.is_path_within_directory"""
|
||||
|
||||
import os
|
||||
from unittest.mock import patch
|
||||
|
||||
# Importing `nodes` (a transitive import of `server`) prepends the `comfy/`
|
||||
# directory to sys.path, which shadows the top-level `utils` package with
|
||||
# `comfy/utils.py`. Importing `utils` first caches the real package so later
|
||||
# `server` imports (e.g. app.frontend_management) resolve it correctly.
|
||||
import utils # noqa: F401
|
||||
from server import is_path_within_directory
|
||||
|
||||
|
||||
def test_subpath_is_within_directory(tmp_path):
|
||||
directory = str(tmp_path)
|
||||
sub = os.path.join(directory, "subfolder")
|
||||
assert is_path_within_directory(sub, directory) is True
|
||||
|
||||
|
||||
def test_directory_itself_is_within_directory(tmp_path):
|
||||
directory = str(tmp_path)
|
||||
assert is_path_within_directory(directory, directory) is True
|
||||
|
||||
|
||||
def test_sibling_path_is_not_within_directory(tmp_path):
|
||||
directory = os.path.join(str(tmp_path), "output")
|
||||
sibling = os.path.join(str(tmp_path), "other")
|
||||
assert is_path_within_directory(sibling, directory) is False
|
||||
|
||||
|
||||
def test_parent_path_is_not_within_directory(tmp_path):
|
||||
directory = os.path.join(str(tmp_path), "output")
|
||||
parent = str(tmp_path)
|
||||
assert is_path_within_directory(parent, directory) is False
|
||||
|
||||
|
||||
def test_different_drive_returns_false_instead_of_raising():
|
||||
# On Windows, os.path.commonpath raises ValueError when the paths don't
|
||||
# share a drive (e.g. comparing "C:\\output" with "D:\\secret"). That
|
||||
# should be treated as "not within", not bubble up as an exception.
|
||||
with patch("os.path.commonpath", side_effect=ValueError("Paths don't have the same drive")):
|
||||
assert is_path_within_directory("D:\\secret", "C:\\output") is False
|
||||
Loading…
Reference in New Issue
Block a user