Skip to content

Add "ComfyUI CoolB Nodes" to custom nodes list - #3292

Open
CoolBreeze164 wants to merge 1 commit into
Comfy-Org:mainfrom
CoolBreeze164:main
Open

CoolBreeze164 wants to merge 1 commit into
Comfy-Org:mainfrom
CoolBreeze164:main

Conversation

@CoolBreeze164

Copy link
Copy Markdown

Adds a pack of utility nodes for ComfyUI featuring saving/loading speakers with transcripts (useful alongside TTS packs), execution flow control (muting/bypassing nodes and groups on trigger to actively change the workflow state from run to run), some useful text file utilities (including save/load text file) and data type conversion.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: cd381d72-30db-49a1-8a88-259bfc6d6d94

📥 Commits

Reviewing files that changed from the base of the PR and between 21ab2b7 and 1568ef3.

📒 Files selected for processing (1)
  • custom-node-list.json

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The custom-node list adds ComfyUI CoolB Nodes with git-clone installation, its GitHub repository, author information, and a description of its utility nodes.

Changes

Custom Node Registration

Layer / File(s) Summary
Add custom-node entry
custom-node-list.json
Adds the ComfyUI-CoolB-Nodes entry with git-clone installation, GitHub source, author information, and utility-node capabilities.

Suggested reviewers: ltdrdata

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 1568e

The manifest entry satisfies the documented registration and git-clone integration contracts, so it is ready to merge.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Warning

⚠️ This pull request shows signs of AI-generated slop (description_diff_mismatch). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from ltdrdata September 16, 2026 22:49
@ltdrdata

Copy link
Copy Markdown
Member

Thanks for the contribution! I took a look at ComfyUI-CoolB-Nodes and ran into a couple of issues I'd like to see addressed before this can be merged.

  1. In TextFileWriter.save_file (nodes.py:1520), the file path comes from a node widget, which any workflow can set, and it's joined to the ComfyUI root with os.path.join and no containment check — so an absolute or ../ path writes attacker-controlled content anywhere on the host.
  2. In TextFileReader.read_file (nodes.py:1450), the file path is again taken from a node widget with no containment check, and the file's contents are returned as node output — so any path outside the intended folder can be read back.

Both stem from the same broken-containment issue at the path-construction layer. Please resolve each of these against a fixed base directory: use realpath and check with commonpath that the result stays inside that base, and reject absolute paths and any .. segments.

I'll re-evaluate once these are addressed.

@CoolBreeze164

Copy link
Copy Markdown
Author

Thank you for your review! I'm completely new to all this coding stuff, this was my first repo. You were right, after testing, the original writer and reader could indeed operate outside comfy root folder. I added a fix using Qwen:

  • Absolute paths (/etc/passwd, C:\Windows\..., \Windows\..., C:foo, UNC) are rejected outright, so os.path.join can never drop the base.
  • .. segments are rejected up front, even ones that would resolve back inside the base.
  • realpath + commonpath is the final proof: realpath collapses symlinks (so a symlink inside output/ pointing outside is caught), and commonpath compares whole segments, avoiding the startswith("/comfy/out") vs /comfy/outside prefix bug. Cross-drive paths on Windows raise ValueError from commonpath, which is treated as an escape.
  • os.makedirs now runs only after validation, so the writer can no longer create attacker-chosen directory trees outside the root.
  • Both nodes keep their existing graceful behavior — violations surface as Error: ... node output instead of crashing the prompt.
  • The increment branch only ever mutates the filename inside the already-validated directory, but it's re-checked with _assert_within_base anyway.

After testing the fix it seems to resolve the issues. If only comfy had text managers natively :(
Btw i'm planning to add another bigger pack soon. It would allow to make telegram bots inside comfy. Hopefully it won't have any issues :p

@ltdrdata

Copy link
Copy Markdown
Member

Thanks for the quick fix, and nice work — especially for a first repo! I re-checked the updated code, and the path handling itself is solid now: absolute paths and ".." are rejected, and the realpath + commonpath check keeps both nodes inside their base directory.

The remaining issue is which directory that base is. Both nodes use the whole ComfyUI root (nodes.py:1494 for the reader, nodes.py:1541 for the writer), and that is still too wide:

  1. Save Text File (nodes.py:1539) can create or overwrite any file under the ComfyUI root, including Python files under custom_nodes/ that would run on the next start. Please set the base to a dedicated folder instead, e.g. the ComfyUI output directory (folder_paths.get_output_directory()) or a subfolder of it, and keep your existing containment check against that base.

  2. Load Text File (nodes.py:1492) can read any file under the ComfyUI root and return its contents, which includes files in user/ that may hold settings or keys. Please confine it the same way, to the input or output directory (folder_paths.get_input_directory() / get_output_directory()).

Your _safe_resolve_under_base helper can stay exactly as it is — only the base_dir it is given needs to change.

I'll re-evaluate once these are addressed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants