From 8dd1967fde38f9cbebc1347cd6e95046586ff5eb Mon Sep 17 00:00:00 2001 From: "Dr.Lt.Data" <128333288+ltdrdata@users.noreply.github.com> Date: Sat, 14 Nov 2026 14:46:35 +0900 Subject: [PATCH] v4: Share only a file inside the output directory to Matrix (#3515) The share route joined the file name and subfolder of the selected output, both taken from the request, onto the output or temp directory and uploaded that file to the Matrix homeserver named in the same request. A name such as "../../file" left the directory, so a request could have any file that ComfyUI can read uploaded. The route runs the upload only where matrix-nio is installed, which Manager does not install. - Refuse, with 400 and the error shape the share dialog shows, a path that is not inside the directory, in the glob and the legacy copy - Compare the normalized path without resolving symlinks, as ComfyUI's /view does, and open that same path, so an image that the dialog previews through a symlink in the directory can still be shared - Refuse a path on another Windows drive, for which commonpath raises ValueError - Add tests that run the real route of both copies against a recording Matrix client - Add a CHANGELOG entry The same change for main is #3427. The other upload that #3427 first changed was removed from v4 in #3500. --- CHANGELOG.md | 6 + comfyui_manager/glob/share_3rdparty.py | 11 ++ comfyui_manager/legacy/share_3rdparty.py | 11 ++ tests/test_share_asset_path.py | 185 +++++++++++++++++++++++ 4 files changed, 213 insertions(+) create mode 100644 tests/test_share_asset_path.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e5a8b6b1..1620dfca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,12 @@ and this project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/ than `.safetensors` must be in the default channel's list unless the security level is `normal-` or lower on a loopback listener, or `weak` in `personal_cloud` mode (#3428). +- Share to the Matrix server only a file inside ComfyUI's output or temp + directory. The file name and subfolder of the output to share come from the + request and were not checked, so a request could have any file that ComfyUI + can read uploaded to the Matrix homeserver named in the same request. Only + installations with `matrix-nio`, which Manager does not install, were + affected (#3427). ### Fixed diff --git a/comfyui_manager/glob/share_3rdparty.py b/comfyui_manager/glob/share_3rdparty.py index af8c8eb3..80d7847d 100644 --- a/comfyui_manager/glob/share_3rdparty.py +++ b/comfyui_manager/glob/share_3rdparty.py @@ -150,6 +150,17 @@ async def share_art(request): else: asset_filepath = os.path.join(output_dir, asset_filename) + # The filename and the subfolder come from the request: share only a file in that directory. + # Like ComfyUI's /view, this follows a symlink that the user placed in the directory. + output_dir = os.path.abspath(output_dir) + asset_filepath = os.path.abspath(asset_filepath) + try: + is_inside = os.path.commonpath((output_dir, asset_filepath)) == output_dir + except ValueError: # on Windows, a path on another drive + is_inside = False + if not is_inside: + return web.json_response({"error": "The selected output is not in the output directory."}, content_type='application/json', status=400) + # get the mime type of the asset assetFileType = mimetypes.guess_type(asset_filepath)[0] diff --git a/comfyui_manager/legacy/share_3rdparty.py b/comfyui_manager/legacy/share_3rdparty.py index af8c8eb3..80d7847d 100644 --- a/comfyui_manager/legacy/share_3rdparty.py +++ b/comfyui_manager/legacy/share_3rdparty.py @@ -150,6 +150,17 @@ async def share_art(request): else: asset_filepath = os.path.join(output_dir, asset_filename) + # The filename and the subfolder come from the request: share only a file in that directory. + # Like ComfyUI's /view, this follows a symlink that the user placed in the directory. + output_dir = os.path.abspath(output_dir) + asset_filepath = os.path.abspath(asset_filepath) + try: + is_inside = os.path.commonpath((output_dir, asset_filepath)) == output_dir + except ValueError: # on Windows, a path on another drive + is_inside = False + if not is_inside: + return web.json_response({"error": "The selected output is not in the output directory."}, content_type='application/json', status=400) + # get the mime type of the asset assetFileType = mimetypes.guess_type(asset_filepath)[0] diff --git a/tests/test_share_asset_path.py b/tests/test_share_asset_path.py new file mode 100644 index 00000000..5638a522 --- /dev/null +++ b/tests/test_share_asset_path.py @@ -0,0 +1,185 @@ +"""The share route uploads only a file inside the output or temp directory (#3427). + +Each case runs the real `share_art` of the glob and the legacy package against a recording +Matrix client. The filename and the subfolder of the selected output come from the request. +""" +import asyncio +import json +import mimetypes +import ntpath +import os +from types import SimpleNamespace + +import pytest +from aiohttp import web + +from legacy_html_testutil import PACKAGE, load_functions + +SECRET = b"not an output" + + +class LoginResponse: + access_token = "token" + + +class UploadResponse: + content_uri = "mxc://test/asset" + + +def load_share_art(package, output_dir, temp_dir, os_module: object = os): + """Returns `share_art` and the list that receives (homeserver, uploaded bytes).""" + uploads = [] + + class AsyncClient: + def __init__(self, homeserver, username): + self.homeserver = homeserver + + async def login(self, password): + return LoginResponse() + + async def upload(self, data, content_type=None, filename=None): + uploads.append((self.homeserver, data.read())) + return UploadResponse(), None + + async def room_send(self, **kwargs): + pass + + async def close(self): + pass + + namespace = load_functions(PACKAGE / package / "share_3rdparty.py", {"share_art"}, { + "PromptServer": SimpleNamespace(instance=SimpleNamespace(routes=web.RouteTableDef())), + "folder_paths": SimpleNamespace(get_output_directory=lambda: str(output_dir), + get_temp_directory=lambda: str(temp_dir)), + "os": os_module, "web": web, "json": json, "mimetypes": mimetypes, + "matrix_nio_is_available": True, "set_matrix_auth": lambda matrix_auth: None, + "AsyncClient": AsyncClient, "LoginResponse": LoginResponse, "UploadResponse": UploadResponse, + }) + return namespace["share_art"], uploads + + +def share(share_art, output): + body = { + "matrix_auth": {"homeserver": "matrix.example", "username": "user", "password": "password"}, + "share_destinations": ["matrix"], "credits": "", "title": "", "description": "", + "prompt": {"workflow": {}}, "potential_outputs": [output], "selected_output_index": 0, + } + + async def read_json(): + return body + + return asyncio.run(share_art(SimpleNamespace(json=read_json))) + + +def image(filename, subfolder="", type="output"): + return {"type": "image", "image": {"filename": filename, "subfolder": subfolder, "type": type}} + + +@pytest.fixture(params=["glob", "legacy"]) +def package(request): + return request.param + + +@pytest.fixture +def dirs(tmp_path): + output_dir = tmp_path / "output" + temp_dir = tmp_path / "temp" + (output_dir / "sub").mkdir(parents=True) + temp_dir.mkdir() + (output_dir / "a.png").write_bytes(b"output image") + (output_dir / "sub" / "b.png").write_bytes(b"image in a subfolder") + (temp_dir / "c.png").write_bytes(b"preview image") + (tmp_path / "secret.txt").write_bytes(SECRET) + (tmp_path / "output-other").mkdir() + (tmp_path / "output-other" / "d.png").write_bytes(SECRET) + return output_dir, temp_dir + + +@pytest.mark.parametrize("output, content", [ + (image("a.png"), b"output image"), + (image("b.png", "sub"), b"image in a subfolder"), + (image("c.png", type="temp"), b"preview image"), + ({"type": "output", "output": {"filename": "b.png", "subfolder": "sub"}}, b"image in a subfolder"), +]) +def test_a_file_in_the_directory_is_shared(package, dirs, output, content): + share_art, uploads = load_share_art(package, *dirs) + + response = share(share_art, output) + + assert response.status == 200 + assert uploads[0] == ("https://matrix.example", content) + + +@pytest.mark.parametrize("output", [ + image("../secret.txt"), + image("secret.txt", ".."), + image("secret.txt", "sub/../.."), + image("d.png", "../output-other"), # a sibling whose name starts with the directory's name + image("a.png", "../output", type="temp"), # the output directory is not inside the temp directory + {"type": "output", "output": {"filename": "../secret.txt", "subfolder": ""}}, +]) +def test_a_path_that_leaves_the_directory_is_refused(package, dirs, output): + share_art, uploads = load_share_art(package, *dirs) + + response = share(share_art, output) + + assert response.status == 400 + assert "output directory" in json.loads(response.body)["error"] + assert uploads == [] + + +def test_an_absolute_path_is_refused(package, dirs, tmp_path): + share_art, uploads = load_share_art(package, *dirs) + + response = share(share_art, image(str(tmp_path / "secret.txt"), "sub")) + + assert response.status == 400 + assert uploads == [] + + +def test_a_symlink_that_the_user_placed_in_the_directory_is_followed(package, dirs, tmp_path): + """ComfyUI's /view shows such a file, so the image that the dialog previews can be shared.""" + output_dir, _ = dirs + elsewhere = tmp_path / "another-disk" + elsewhere.mkdir() + (elsewhere / "e.png").write_bytes(b"image on another disk") + try: + (output_dir / "link").symlink_to(elsewhere, target_is_directory=True) + except OSError: + pytest.skip("cannot create a symlink here") + share_art, uploads = load_share_art(package, *dirs) + + response = share(share_art, image("e.png", "link")) + + assert response.status == 200 + assert uploads[0][1] == b"image on another disk" + + +def test_the_path_that_was_checked_is_the_path_that_is_read(package, dirs, tmp_path): + """`link/..` is the output directory once normalized, and the parent of the link's target on disk.""" + output_dir, _ = dirs + elsewhere = tmp_path / "another-disk" + elsewhere.mkdir() + (tmp_path / "a.png").write_bytes(SECRET) + try: + (output_dir / "link").symlink_to(elsewhere, target_is_directory=True) + except OSError: + pytest.skip("cannot create a symlink here") + share_art, uploads = load_share_art(package, *dirs) + + response = share(share_art, image("a.png", "link/..")) + + assert response.status == 200 + assert uploads[0][1] == b"output image" + + +def test_a_path_on_another_windows_drive_is_refused(package): + """`os.path.commonpath` raises ValueError for paths on different drives.""" + windows = SimpleNamespace(path=ntpath) + share_art, uploads = load_share_art(package, r"C:\ComfyUI\output", r"C:\ComfyUI\temp", os_module=windows) + + for output in (image(r"D:\secret.txt"), image("secret.txt", "D:\\"), image(r"\\server\share\secret.txt")): + response = share(share_art, output) + + assert response.status == 400 + assert uploads == []