From 02517ce0e0e4589a9f4cd73211d79e8dd95e52ba Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 24 Aug 2026 08:50:19 +0700 Subject: [PATCH 1/2] Return None instead of raising on a cross-drive userdata path (#15820) os.path.commonpath() raises ValueError when the two paths are on different Windows drives, so a request for /userdata/C:%5CWindows%5Ctemp%5Ctest.txt on an install whose user directory is on D: left get_request_user_filepath() through an exception instead of the intended rejection, and aiohttp answered 500 instead of 403/404. A path on another drive is by definition not inside the user directory, so treat the ValueError as "not inside" and return None. --- app/user_manager.py | 11 +- .../app_test/user_manager_cross_drive_test.py | 126 ++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 tests-unit/app_test/user_manager_cross_drive_test.py 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..b5262f947a6 --- /dev/null +++ b/tests-unit/app_test/user_manager_cross_drive_test.py @@ -0,0 +1,126 @@ +"""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; the tests below simulate the drive +mismatch on every platform by patching `os.path.commonpath` to raise the same +ValueError, and additionally run the real thing on Windows when a second drive +is actually available. +""" + +import os +import sys +import tempfile +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 + + +def test_cross_drive_path_is_rejected_not_raised(user_manager, request_): + """A drive mismatch must return None, the same as any other escape.""" + real_commonpath = os.path.commonpath + + def commonpath_raising_on_mismatch(paths): + first, second = paths + if os.path.splitdrive(first)[0].lower() != os.path.splitdrive(second)[0].lower(): + raise ValueError("Paths don't have the same drive") + return real_commonpath(paths) + + with patch("os.path.commonpath", side_effect=commonpath_raising_on_mismatch): + result = user_manager.get_request_user_filepath( + request_, "Z:\\Windows\\temp\\test.txt", create_dir=False + ) + + 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.""" + real_commonpath = os.path.commonpath + + def commonpath_raising_on_mismatch(paths): + first, second = paths + if os.path.splitdrive(first)[0].lower() != os.path.splitdrive(second)[0].lower(): + raise ValueError("Paths don't have the same drive") + return real_commonpath(paths) + + with patch("os.path.commonpath", side_effect=commonpath_raising_on_mismatch): + result = user_manager.get_request_user_filepath( + request_, "Z:%5CWindows%5Ctemp%5Ctest.txt", create_dir=False + ) + + 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 + ) From a7cd5a85fb05b1af33910a1ff77f59e8c2b801c2 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 24 Aug 2026 09:02:06 +0700 Subject: [PATCH 2/2] test: make the cross-drive simulation fire on POSIX too The mock decided when to raise by comparing drive letters, which only ever differ on Windows: on POSIX a leading `Z:` is an ordinary filename character, so the join landed inside the user directory, nothing raised, and the two simulated tests failed on Linux CI instead of covering the guard. Raise from the `(user_root, path)` comparison by call position instead, and assert the mock actually reached it so the test cannot pass without exercising the guard. --- .../app_test/user_manager_cross_drive_test.py | 57 ++++++++++++------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/tests-unit/app_test/user_manager_cross_drive_test.py b/tests-unit/app_test/user_manager_cross_drive_test.py index b5262f947a6..84c65dc6c2d 100644 --- a/tests-unit/app_test/user_manager_cross_drive_test.py +++ b/tests-unit/app_test/user_manager_cross_drive_test.py @@ -15,15 +15,18 @@ '../escape.json' -> None (rejected, as it should be) 'sub/ok.json' -> -The cross-drive case is Windows-only; the tests below simulate the drive -mismatch on every platform by patching `os.path.commonpath` to raise the same -ValueError, and additionally run the real thing on Windows when a second drive -is actually available. +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 @@ -52,39 +55,51 @@ def request_(): return request -def test_cross_drive_path_is_rejected_not_raised(user_manager, request_): - """A drive mismatch must return None, the same as any other escape.""" +@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 commonpath_raising_on_mismatch(paths): - first, second = paths - if os.path.splitdrive(first)[0].lower() != os.path.splitdrive(second)[0].lower(): - raise ValueError("Paths don't have the same drive") - return real_commonpath(paths) - with patch("os.path.commonpath", side_effect=commonpath_raising_on_mismatch): +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.""" - real_commonpath = os.path.commonpath - - def commonpath_raising_on_mismatch(paths): - first, second = paths - if os.path.splitdrive(first)[0].lower() != os.path.splitdrive(second)[0].lower(): - raise ValueError("Paths don't have the same drive") - return real_commonpath(paths) - - with patch("os.path.commonpath", side_effect=commonpath_raising_on_mismatch): + 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