diff --git a/app/user_manager.py b/app/user_manager.py index 55e7e81e3d1..f459843f9e1 100644 --- a/app/user_manager.py +++ b/app/user_manager.py @@ -92,7 +92,16 @@ def get_request_user_filepath(self, request, file, type="userdata", create_dir=T # prevent leaving /{type}/{user} path = os.path.abspath(os.path.join(user_root, file)) - if os.path.commonpath((user_root, path)) != user_root: + # commonpath() raises ValueError when the two paths are on + # different Windows drives, so a request for an absolute path on + # another drive (e.g. /userdata/C:%5C... while the user directory + # is on D:) escaped as an unhandled 500 instead of being rejected. + # A path on another drive is, by definition, not inside user_root. + try: + inside_user_root = os.path.commonpath((user_root, path)) == user_root + except ValueError: + inside_user_root = False + if not inside_user_root: return None parent = os.path.split(path)[0] diff --git a/tests-unit/app_test/user_manager_cross_drive_test.py b/tests-unit/app_test/user_manager_cross_drive_test.py new file mode 100644 index 00000000000..84c65dc6c2d --- /dev/null +++ b/tests-unit/app_test/user_manager_cross_drive_test.py @@ -0,0 +1,141 @@ +"""Regression tests for #15820 — a userdata path on another Windows drive. + +`get_request_user_filepath()` joins the requested file onto the user root and +then compares them with `os.path.commonpath()`, which raises + + ValueError: Paths don't have the same drive + +when the two paths sit on different Windows drives. Requesting +`/userdata/C:%5CWindows%5Ctemp%5Ctest.txt` from an install whose user +directory is on `D:` therefore produced an unhandled 500 rather than the +intended rejection. Measured on the pre-fix build, user directory on `D:`: + + 'C:\\\\Windows\\\\temp\\\\test.txt' -> ValueError: Paths don't have the same drive + 'C:%5CWindows%5Ctemp%5Ctest.txt' -> ValueError: Paths don't have the same drive + '../escape.json' -> None (rejected, as it should be) + 'sub/ok.json' -> + +The cross-drive case is Windows-only. To keep the guard covered on POSIX CI as +well, the tests below drive the same ValueError out of `os.path.commonpath()` +by call position rather than by comparing drive letters: on POSIX a leading +`Z:` is an ordinary filename character, so the join lands inside the user +directory and a drive-letter comparison would never fire. One test also does it +for real on Windows when a second drive exists. +""" + +import os +import sys +import tempfile +from contextlib import contextmanager +from unittest.mock import MagicMock, patch + +import pytest + +import folder_paths +from app.user_manager import UserManager + + +@pytest.fixture +def user_manager(): + with tempfile.TemporaryDirectory() as temp_dir: + original = folder_paths.get_user_directory() + folder_paths.set_user_directory(temp_dir) + with patch("app.user_manager.args") as mock_args: + mock_args.multi_user = False + manager = UserManager() + manager.users = {"default": "default"} + yield manager + folder_paths.set_user_directory(original) + + +@pytest.fixture +def request_(): + request = MagicMock() + request.headers = {} + return request + + +@contextmanager +def commonpath_raising_on_the_user_root_check(): + """Raise a drive mismatch from the `(user_root, path)` comparison only. + + Deciding when to raise by comparing drive letters would fire on Windows + only: on POSIX a leading `Z:` is an ordinary filename character, so the + join lands inside the user directory and nothing raises. Keying on the call + instead keeps the guard covered on every platform. + + The earlier `(root_dir, user_root)` comparison in the same function keeps + working: both of those paths are derived from the user directory, so a real + drive mismatch cannot occur there. + """ + real_commonpath = os.path.commonpath + calls = [] + + def fake_commonpath(paths): + calls.append(paths) + if len(calls) == 1: + return real_commonpath(paths) + raise ValueError("Paths don't have the same drive") + + with patch("os.path.commonpath", side_effect=fake_commonpath): + yield calls + + +def test_cross_drive_path_is_rejected_not_raised(user_manager, request_): + """A drive mismatch must return None, the same as any other escape.""" + with commonpath_raising_on_the_user_root_check() as calls: + result = user_manager.get_request_user_filepath( + request_, "Z:\\Windows\\temp\\test.txt", create_dir=False + ) + + assert len(calls) == 2, "the containment check under test was never reached" + assert result is None + + +def test_url_encoded_cross_drive_path_is_rejected(user_manager, request_): + """The reported request shape: the drive letter arrives percent-encoded.""" + with commonpath_raising_on_the_user_root_check() as calls: + result = user_manager.get_request_user_filepath( + request_, "Z:%5CWindows%5Ctemp%5Ctest.txt", create_dir=False + ) + + assert len(calls) == 2, "the containment check under test was never reached" + assert result is None + + +def test_ordinary_paths_are_unaffected(user_manager, request_): + """The guard must not change the two outcomes that already worked.""" + inside = user_manager.get_request_user_filepath( + request_, "sub/workflow.json", create_dir=False + ) + assert inside is not None + assert inside.endswith(os.path.join("sub", "workflow.json")) + + assert ( + user_manager.get_request_user_filepath(request_, "../escape.json", create_dir=False) + is None + ) + assert user_manager.get_request_user_filepath(request_, None, create_dir=False) is not None + + +@pytest.mark.skipif(sys.platform != "win32", reason="drive letters are Windows-only") +def test_real_cross_drive_path_on_windows(user_manager, request_): + """Same case again without patching, when a second real drive exists.""" + user_drive = os.path.splitdrive(folder_paths.get_user_directory())[0].upper() + other = next( + ( + f"{letter}:" + for letter in "CDEFGH" + if f"{letter}:" != user_drive and os.path.exists(f"{letter}:\\") + ), + None, + ) + if other is None: + pytest.skip("no second drive available on this machine") + + assert ( + user_manager.get_request_user_filepath( + request_, f"{other}\\Windows\\temp\\test.txt", create_dir=False + ) + is None + )