fix(hardening): address PR #157 review findings
Critical fixes: - node_runtime.py: change outer handler in _load_module() from except Exception to except BaseException with sys.modules.pop() cleanup, preventing non-Exception BaseExceptions (SystemExit, KeyboardInterrupt) from violating the no-raise contract - app.py: remove incorrect reassignment of base_node_class_mappings after custom node import; keep it as pre-custom-node baseline so WorkflowPlanner correctly treats extras/custom nodes as NODE_CLASS_MAPPINGS dict lookups instead of direct imports - Regenerate stale test fixtures (text-to-image.py, upscale-model-loader.py) from current source: updated _load_module() with exec failure cleanup pattern, 3-marker structural verification, and try/finally sys.path restoration in import_custom_nodes() Suggestions addressed: - node_runtime.py: _is_comfyui_directory() now checks 3 structural markers (nodes.py, main.py, comfy/) instead of just nodes.py - generator/render.py: added None guard for getattr on generated_helpers.__all__ items with log warning for missing helpers - test_import_path_resolution.py: fixed test_sys_path_restored_on_crash to target the server module specifically after sys.path manipulation, so the try/finally restoration path is actually exercised Nits addressed: - .gitignore: added missing trailing newline New test coverage (tests/test_app_base_mappings.py): - base_node_class_mappings unchanged after custom node import - deep copy independence of base_node_class_mappings - render output contains hardened _load_module with except BaseException pattern
This commit is contained in:
@@ -9,10 +9,18 @@ log = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _is_comfyui_directory(path: str) -> bool:
|
||||
"""Verify a directory has ComfyUI structural markers (nodes.py)."""
|
||||
"""Verify a directory has ComfyUI structural markers.
|
||||
|
||||
Checks for nodes.py, main.py, and the comfy/ subdirectory to raise
|
||||
the bar against spoofing via a directory with only a single marker file.
|
||||
"""
|
||||
if not os.path.isdir(path):
|
||||
return False
|
||||
return os.path.isfile(os.path.join(path, "nodes.py"))
|
||||
return (
|
||||
os.path.isfile(os.path.join(path, "nodes.py"))
|
||||
and os.path.isfile(os.path.join(path, "main.py"))
|
||||
and os.path.isdir(os.path.join(path, "comfy"))
|
||||
)
|
||||
|
||||
|
||||
def _load_module(module_name: str, filepath: str) -> Any:
|
||||
@@ -43,8 +51,9 @@ def _load_module(module_name: str, filepath: str) -> Any:
|
||||
sys.modules.pop(module_name, None)
|
||||
raise
|
||||
return mod
|
||||
except Exception as e:
|
||||
except BaseException as e:
|
||||
log.debug("Failed to load %s from %s: %s", module_name, filepath, e)
|
||||
sys.modules.pop(module_name, None) # Also clean up on edge-case exceptions
|
||||
return None
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user