feature: add management api for Comfystream mgmt api #35

ad-astra-video · 2025-03-18T01:43:25Z

Adds API routes to manage custom nodes and models in comfystream.

Includes a hot reload route /settings/reload to close all connections and reload the Pipeline. The hot reload required copying the import_all_nodes_in_workspace function from hiddenswitch/comfyui to remove the check if nodes are already imported.

The managment API routes are included in the comfystream aiohttp server. Calling the routes can cause delays in frame processing because I think some of the underlying calls are not async.

Routes are as follows:

/settings/nodes/{list/install/delete}
/settings/models/{list/add/delete}
/settings/reload
/settings/turn/server/set/account

ad-astra-video · 2025-03-18T01:43:56Z

@hjpotter92 moved the PR here. I included update you suggested to have the routes under /settings/...

server/api/models/models.py

+            logger.info(f"model path: {model_path}")
+
+        # check path is in workspace_dir, raises value error if not
+        model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))


To address the issue, we will:

Normalize the model_path using Path.resolve() to eliminate any .. segments or symbolic links.

Explicitly check that the normalized path starts with the expected root directory (workspace_dir/models) to ensure it does not escape the intended directory structure.

Apply these changes in both the add_model and delete_model functions, as they both construct paths using user-provided data.

server/api/models/models.py

+
+        # check path is in workspace_dir, raises value error if not
+        model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+        os.makedirs(model_path.parent, exist_ok=True)


To fix the issue, we will sanitize and validate the user-provided path (model['path']) before constructing the model_path. Specifically:

Use os.path.normpath to normalize the path and remove any .. segments.

Ensure that the normalized path does not escape the intended directory structure by checking that it starts with the workspace_dir after normalization.

Apply these changes in the add_model and delete_model functions in server/api/models/models.py.

server/api/models/models.py

+        # start downloading the model in background without blocking
+        asyncio.create_task(download_model(model['url'], model_path))
+    except Exception as e:
+        os.remove(model_path)+".downloading"


To fix the issue, we need to ensure that the model_path is validated and sanitized before being used in the os.remove operation. Specifically:

Use os.path.normpath or Path.resolve() to normalize the path and remove any .. segments.

Ensure that the normalized path is within the workspace_dir using the relative_to check.

Add a fallback mechanism to handle cases where the path validation fails, ensuring no unintended file operations occur.

The fix will involve:

Adding a validation step before the os.remove operation on line 86.

Ensuring that the .downloading suffix is appended to the validated path in a safe manner.

server/api/models/models.py

+    try:
+        model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path']))
+        #check path is in workspace_dir, raises value error if not
+        model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))


To fix the issue, we need to ensure that the model_path is normalized before performing the relative_to check. This can be achieved by using Path.resolve() to normalize the path and remove any .. segments or symbolic links. Additionally, we should ensure that the normalized path starts with the intended base directory (workspace_dir/models) to prevent path traversal attacks.

Steps to fix:

Normalize the model_path using Path.resolve() before performing the relative_to check.

Ensure that the normalized path starts with the intended base directory (workspace_dir/models).

Update the delete_model function in server/api/models/models.py to include these changes.

server/api/models/models.py

+        #check path is in workspace_dir, raises value error if not
+        model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+
+        os.remove(model_path)


To fix the issue, we will enhance the validation of model_path to ensure it is safe and contained within the workspace_dir. Specifically:

Use os.path.realpath to resolve the absolute path of model_path and compare it with the absolute path of the workspace_dir to prevent symbolic link attacks.

Perform the validation immediately before the os.remove operation to minimize the risk of race conditions.

Add a fallback exception to handle any unexpected edge cases.

server/api/nodes/nodes.py

+    try:
+        #delete the folder and all its contents.  ignore_errors allows readonly files to be deleted
+        logger.info(f"deleting node {node['name']}")
+        shutil.rmtree(node_path, ignore_errors=True)


To fix the issue, we need to validate the constructed node_path to ensure it is confined within the custom_nodes_path directory. This can be achieved by normalizing the path using os.path.normpath or Path.resolve() and verifying that the resulting path starts with custom_nodes_path. If the validation fails, an exception should be raised to prevent unsafe operations.

Changes will be made in the delete_node and toggle_node functions in server/api/nodes/nodes.py to validate the node_path before performing any operations.

server/api/nodes/nodes.py

+    is_disabled = False
+    #confirm if enabled node exists
+    logger.info(f"toggling node { node['name'] }")
+    if not node_path.exists():


To fix the issue, we need to validate and sanitize the node["name"] value before using it to construct file paths. The best approach is to:

Normalize the constructed path using os.path.normpath or Path.resolve() to eliminate any .. segments.

Ensure that the resulting path is contained within the custom_nodes_path directory by checking that it starts with or is a subpath of custom_nodes_path.

Optionally, use a stricter validation mechanism (e.g., werkzeug.utils.secure_filename) to ensure that node["name"] is a valid and safe directory name.

The changes will be applied to the delete_node and toggle_node functions in server/api/nodes/nodes.py.

server/api/nodes/nodes.py

+        #try with .disabled
+        node_path = custom_nodes_path / str(node['name']+".disabled")
+        logger.info(f"checking if node { node['name'] } is disabled")
+        if not node_path.exists():


To fix the issue, we need to validate and sanitize the node["name"] value before using it in path operations. The best approach is to normalize the constructed path and ensure it remains within the custom_nodes_path directory. This can be achieved using os.path.normpath or Path.resolve() to handle any .. segments in the path and then verifying that the resulting path starts with custom_nodes_path.

Steps to implement the fix:

Normalize the constructed path using Path.resolve() to eliminate any .. segments.

Check that the normalized path is a subpath of custom_nodes_path.

Raise an exception if the validation fails.

server/api/nodes/nodes.py

+            #rename the folder to remove .disabled
+            logger.info(f"enabling node {node['name']}")
+            new_name = node_path.with_name(node["name"])
+            shutil.move(str(node_path), str(new_name))


To fix the issue, we need to validate and sanitize the node["name"] value before using it in path expressions. The best approach is to:

Normalize the constructed path using os.path.normpath or Path.resolve() to eliminate any .. segments.

Ensure that the resulting path is within the intended custom_nodes_path directory by checking that it starts with the custom_nodes_path prefix.

Optionally, use a stricter validation mechanism, such as werkzeug.utils.secure_filename, to ensure the node["name"] value does not contain special characters or unexpected patterns.

The changes will be applied to the delete_node and toggle_node functions in server/api/nodes/nodes.py.

server/api/nodes/nodes.py

+            #rename the folder to add .disabled
+            logger.info(f"disbling node {node['name']}")
+            new_name = node_path.with_name(node["name"]+".disabled")
+            shutil.move(str(node_path), str(new_name))


To fix the issue, we need to validate and sanitize the user-provided node["name"] to ensure it does not contain malicious characters or sequences that could lead to path traversal attacks. The best approach is to:

Normalize the constructed path using os.path.normpath or Path.resolve() to eliminate any .. segments.

Verify that the normalized path is within the intended custom_nodes_path directory.

Optionally, use a stricter validation mechanism (e.g., werkzeug.utils.secure_filename) to ensure the node["name"] is a valid filename.

The changes will be made in the toggle_node function in server/api/nodes/nodes.py.

eliteprox

Thank you @ad-astra-video for continuing to work on this, I'd like to merge this within the next week. Looks like there are a few path traversal issues to address. If you can take care of those, will give re-review and merge. We can build additional functionality around node and model installation once this PR is merged

ad-astra-video changed the title ~~feature: add management api for comfystreamComfystream mgmt api~~ feature: add management api for Comfystream mgmt api Mar 31, 2025

ad-astra-video force-pushed the comfystream-mgmt-api branch 2 times, most recently from e711d6d to 9c23759 Compare April 14, 2025 16:59

eliteprox mentioned this pull request May 6, 2025

Add ability to Set Twilio Turn Server in server Settings #165

Closed

ad-astra-video and others added 23 commits May 14, 2025 09:09

initial commit for comfystream api

0ece85d

add GitPython to requirements.txt

465fec4

Add nodes, models and settings api

e42cc31

install nodes in conda environments

bd42ec5

fixes for models download and nodes listing

9d8e95b

reload embedded client

020b9d9

update to install in one env

1171ae7

move adding mgmt api routes before route prefix

678429e

update reload pipeline

e5d99a6

reset webrtc connections on reload

0988908

cleanup

98f5a6c

cleanup

5d46936

update routes

79b10c6

update set twilio account info

c0c3a92

cleanup

0c5a05c

cleanup

1adc535

route update and log lines updates

4cb6b67

add enable/disable and restart comfyui

5825c82

add restart comfyui to comfystream menu

04b07fa

add mgmt api UI to ComfyStream menu in ComfyUI, some fixes/improvements

0ed2303

add mgmt of TURN server credentials

3fc8196

fix update node

7d2a4b1

add log lines on ComfyUI restart

d3993eb

ad-astra-video force-pushed the comfystream-mgmt-api branch from aab2a80 to d3993eb Compare May 14, 2025 22:49

github-advanced-security bot found potential problems May 14, 2025

View reviewed changes

fix dependencies and update import

cca6715

eliteprox requested changes May 19, 2025

View reviewed changes

@@ -79,4 +79,7 @@
-                    # check path is in workspace_dir, raises value error if not
-                    model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+                    # Normalize and validate the path
+                    model_path = model_path.resolve()
+                    root_path = Path(os.path.join(workspace_dir, "models")).resolve()
+                    if not str(model_path).startswith(str(root_path)):
+                        raise ValueError(f"Invalid model path: {model_path} is outside the allowed directory")
                     os.makedirs(model_path.parent, exist_ok=True)
@@ -95,4 +98,7 @@
                     model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path']))
-                    #check path is in workspace_dir, raises value error if not
-                    model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+                    # Normalize and validate the path
+                    model_path = model_path.resolve()
+                    root_path = Path(os.path.join(workspace_dir, "models")).resolve()
+                    if not str(model_path).startswith(str(root_path)):
+                        raise ValueError(f"Invalid model path: {model_path} is outside the allowed directory")

@@ -74,5 +74,8 @@
                     model_path = Path(os.path.join(workspace_dir, "models", model['type'], model_name))
-                    #if specified, use the model path from the model dict (e.g. sd1.5/model.safetensors will put model.safetensors in models/checkpoints/sd1.5)
+                    # if specified, use the model path from the model dict (e.g. sd1.5/model.safetensors will put model.safetensors in models/checkpoints/sd1.5)
                     if 'path' in model:
-                        model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path']))
+                        user_path = os.path.normpath(model['path'])  # Normalize the user-provided path
+                        if user_path.startswith("..") or os.path.isabs(user_path):  # Prevent directory traversal or absolute paths
+                            raise ValueError("Invalid model path: directory traversal or absolute paths are not allowed")
+                        model_path = Path(os.path.join(workspace_dir, "models", model['type'], user_path))
                         logger.info(f"model path: {model_path}")
@@ -94,4 +97,7 @@
                 try:
-                    model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path']))
-                    #check path is in workspace_dir, raises value error if not
+                    user_path = os.path.normpath(model['path'])  # Normalize the user-provided path
+                    if user_path.startswith("..") or os.path.isabs(user_path):  # Prevent directory traversal or absolute paths
+                        raise ValueError("Invalid model path: directory traversal or absolute paths are not allowed")
+                    model_path = Path(os.path.join(workspace_dir, "models", model['type'], user_path))
+                    # check path is in workspace_dir, raises value error if not
                     model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))

@@ -85,3 +85,8 @@
                 except Exception as e:
-                    os.remove(model_path)+".downloading"
+                    try:
+                        # Validate the model_path before removing
+                        validated_path = model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+                        os.remove(validated_path.with_suffix(".downloading"))
+                    except Exception as validation_error:
+                        logger.error(f"Failed to validate or remove path: {validation_error}")
                     raise Exception(f"error downloading model: {e}")

@@ -94,5 +94,7 @@
                 try:
-                    model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path']))
-                    #check path is in workspace_dir, raises value error if not
-                    model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+                    model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path'])).resolve()
+                    base_path = Path(os.path.join(workspace_dir, "models")).resolve()
+                    # Ensure the normalized path is within the intended base directory
+                    if not str(model_path).startswith(str(base_path)):
+                        raise ValueError("Invalid path: Path traversal attempt detected")

@@ -95,6 +95,10 @@
                     model_path = Path(os.path.join(workspace_dir, "models", model['type'], model['path']))
-                    #check path is in workspace_dir, raises value error if not
-                    model_path.resolve().relative_to(Path(os.path.join(workspace_dir, "models")))
+                    # Ensure the resolved path is within the workspace_dir
+                    full_model_path = os.path.realpath(model_path)
+                    base_models_dir = os.path.realpath(os.path.join(workspace_dir, "models"))
+                    if not full_model_path.startswith(base_models_dir):
+                        raise Exception("Invalid model path: Path traversal detected")
-                    os.remove(model_path)
+                    # Safely remove the file
+                    os.remove(full_model_path)
                 except Exception as e:

@@ -122,3 +122,5 @@
-                node_path = custom_nodes_path / node["name"]
+                node_path = (custom_nodes_path / node["name"]).resolve()
+                if not str(node_path).startswith(str(custom_nodes_path)):
+                    raise ValueError(f"Invalid node name: {node['name']}")
                 if not node_path.exists():
@@ -141,3 +143,5 @@
-                node_path = custom_nodes_path / node["name"]
+                node_path = (custom_nodes_path / node["name"]).resolve()
+                if not str(node_path).startswith(str(custom_nodes_path)):
+                    raise ValueError(f"Invalid node name: {node['name']}")
                 is_disabled = False
@@ -147,3 +151,5 @@
                     #try with .disabled
-                    node_path = custom_nodes_path / str(node['name']+".disabled")
+                    node_path = (custom_nodes_path / str(node['name']+".disabled")).resolve()
+                    if not str(node_path).startswith(str(custom_nodes_path)):
+                        raise ValueError(f"Invalid node name: {node['name']}.disabled")
                     logger.info(f"checking if node { node['name'] } is disabled")

@@ -141,20 +141,30 @@
-                node_path = custom_nodes_path / node["name"]
+                # Sanitize and validate the node name
+                node_name = node["name"]
+                if not isinstance(node_name, str) or ".." in node_name or "/" in node_name or "\\" in node_name:
+                    logger.error(f"Invalid node name: {node_name}")
+                    raise ValueError("Invalid node name")
+                node_path = (custom_nodes_path / node_name).resolve()
+                if not str(node_path).startswith(str(custom_nodes_path)):
+                    logger.error(f"Path traversal attempt detected: {node_path}")
+                    raise ValueError("Invalid node path")
                 is_disabled = False
                 #confirm if enabled node exists
-                logger.info(f"toggling node { node['name'] }")
+                logger.info(f"toggling node {node_name}")
                 if not node_path.exists():
                     #try with .disabled
-                    node_path = custom_nodes_path / str(node['name']+".disabled")
-                    logger.info(f"checking if node { node['name'] } is disabled")
+                    node_path = (custom_nodes_path / f"{node_name}.disabled").resolve()
+                    logger.info(f"checking if node {node_name} is disabled")
                     if not node_path.exists():
                         #node does not exist as enabled or disabled
-                        logger.info(f"node { node['name'] }.disabled does not exist")
-                        raise ValueError(f"node { node['name'] } does not exist")
+                        logger.info(f"node {node_name}.disabled does not exist")
+                        raise ValueError(f"node {node_name} does not exist")
                     else:
                         #node is disabled, so we need to enable it
-                        logger.error(f"node { node['name'] } is disabled")
+                        logger.error(f"node {node_name} is disabled")
                         is_disabled = True
                 else:
-                    logger.info(f"node { node['name'] } is enabled")
+                    logger.info(f"node {node_name} is enabled")
@@ -163,4 +173,4 @@
                         #rename the folder to remove .disabled
-                        logger.info(f"enabling node {node['name']}")
-                        new_name = node_path.with_name(node["name"])
+                        logger.info(f"enabling node {node_name}")
+                        new_name = node_path.with_name(node_name)
                         shutil.move(str(node_path), str(new_name))
@@ -168,8 +178,8 @@
                         #rename the folder to add .disabled
-                        logger.info(f"disbling node {node['name']}")
-                        new_name = node_path.with_name(node["name"]+".disabled")
+                        logger.info(f"disabling node {node_name}")
+                        new_name = node_path.with_name(f"{node_name}.disabled")
                         shutil.move(str(node_path), str(new_name))
                 except Exception as e:
-                    logger.error(f"error {action} node {node['name']}: {e}")
-                    raise Exception(f"error {action} node: {e}")
+                    logger.error(f"error toggling node {node_name}: {e}")
+                    raise Exception(f"error toggling node: {e}")

feature: add management api for Comfystream mgmt api #35

Are you sure you want to change the base?

feature: add management api for Comfystream mgmt api #35

Uh oh!

Conversation

ad-astra-video commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ad-astra-video commented Mar 18, 2025

Uh oh!

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

eliteprox left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ad-astra-video commented Mar 18, 2025 •

edited

Loading