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

@@ -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
 

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]
 

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]
 

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 == []