Skip to content

Add ComfyUI-TJ_NODE to custom-node-list#2924

Open
designloves2 wants to merge 3 commits into
Comfy-Org:mainfrom
designloves2:main
Open

Add ComfyUI-TJ_NODE to custom-node-list#2924
designloves2 wants to merge 3 commits into
Comfy-Org:mainfrom
designloves2:main

Conversation

@designloves2

Copy link
Copy Markdown
Contributor

Hi, I would like to add my new custom node ComfyUI-TJ_NODE to the list. Thank you!

@ltdrdata

ltdrdata commented Jun 4, 2026

Copy link
Copy Markdown
Member
  1. Write-side path traversal — resolve_target_dir() honors .. and absolute paths with no containment, reached by the free STRING input save_path_opt:
  2. Additional write-side path traversal — TJ_SaveAndPreviewImage / TJ_SaveAndPreviewVideo accept a free STRING input path that flows directly into out_dir (absolute path) or os.path.join(output_dir, path) (no .. rejection) → os.makedirs:

Please contain all save targets within folder_paths.get_output_directory() — reject absolute paths and .. sequences, then validate with os.path.realpath + os.path.commonpath.

@designloves2

Copy link
Copy Markdown
Contributor Author

Fixed.

Implemented path traversal protection for all affected save paths in:

  • dynamic_image_batch.py
  • utility_node_tj.py

Changes:

  • reject absolute paths

  • reject .. traversal sequences

  • normalize and validate paths with:

    • os.path.realpath
    • os.path.commonpath
  • contain all save targets inside folder_paths.get_output_directory()

This was implemented as a minimal targeted patch only for save path sanitization.
No wireless/core/frontend behavior was modified.

# Conflicts:
#	custom-node-list.json
@designloves2

Copy link
Copy Markdown
Contributor Author

Merge conflicts have been resolved successfully.

The PR is now cleanly mergeable and all checks are passing.

Thank you for reviewing TJ_NODE!

Copy link
Copy Markdown
Contributor Author

Hi, could you please review this PR again?

I confirmed that the latest ComfyUI-TJ_NODE code now contains the requested path traversal protections:

  • rejects absolute save paths
  • rejects .. traversal sequences
  • validates save targets with realpath / commonpath
  • keeps save targets inside folder_paths.get_output_directory()

The PR is mergeable, but the GitHub Actions workflow appears to be in an action_required state. Could you please approve/re-run the workflow and review it again when you have a chance?

Thank you!

Copy link
Copy Markdown
Contributor Author

Hi @ltdrdata, I pushed an additional security hardening commit to the referenced ComfyUI-TJ_NODE repository:

designloves2/ComfyUI-TJ_NODE@02d2182

This goes beyond the original write-side path traversal fix and tightens the remaining path handling surfaces:

  • save directories are contained with realpath + commonpath
  • absolute paths and leading / / \\ paths are rejected for save/load inputs
  • .. traversal is rejected for save directories and image loader paths
  • filename prefix/suffix values are sanitized so they cannot become path components
  • media preview/path metadata checks now use containment checks instead of raw startswith()

I also verified the updated code with python -m compileall -q core nodes __init__.py and targeted path-safety checks for absolute paths, .. traversal, and filename path separators.

Could you please approve/re-run the workflow and review PR #2924 again when you have a chance? Thank you!

@ltdrdata

ltdrdata commented Jul 4, 2026

Copy link
Copy Markdown
Member

Thanks for the response. I re-verified the current head. The original resolve_target_dir() fix (now in nodes/image/_image_utils.py) is in place and correctly rejects absolute paths and .. for the save_path_opt input — that part is good.

However, the hardening commit you referenced (02d2182, 2026-06-27 18:00) was partially reverted 8 minutes later by 5ea72a5 ("bug fixed."). As a result the code base now has new write- and read-side gaps that weren't in the original review:

  1. filename_prefix traversalnodes/image/save_preview_image.py:59,68 and nodes/video/save_preview_video.py
    The _tj_safe_filename_part sanitizer was removed by 5ea72a5. filename_prefix (user STRING) is now concatenated into the write path unsanitized, so ../../pwn escapes _tj_safe_output_dir.

  2. filename_suffix traversalnodes/image/save_subsequent.py:49-55
    Same pattern — user filename_suffix STRING is concatenated into final_dir / file_name after 5ea72a5 removed the sanitizer.

  3. commonpathstr.startswith regression (prefix confusion)core/tj_api.py:31 _safe_resolve and core/tj_api.py:140 delete_files
    5ea72a5 replaced os.path.commonpath containment with str.startswith. That lets /comfy/output_evil/malware.dll pass a check against base /comfy/output. delete_files reaches an unrestricted delete sink.

  4. Read-side traversal via absolute path trustnodes/utility/_utility_utils.py:251 shutil.copy2(media_path, tmp_path)
    _tj_resolve_media_path after 5ea72a5 accepts absolute paths (os.path.isabs(cand) → return cand), reaching shutil.copy2 on attacker-controlled paths (e.g. /etc/passwd).

Please restore the sanitizers and containment helpers from 02d2182 (safe_filename_part / _tj_safe_filename_part / _tj_is_relative_to / commonpath-based containment) and apply them to all the sinks above. Prefer Path(base).resolve() + Path(target).resolve().is_relative_to(Path(base).resolve()) for containment, and reject inputs whose Path(...).is_absolute() before joining.

I'll re-evaluate once the sanitizers are back in place across the affected files.

@designloves2

Copy link
Copy Markdown
Contributor Author

Thank you for the detailed re-review. All four issues have been addressed in commit 959858f.

1. filename_prefix / filename_suffix traversal
Added _tj_safe_filename_part() to both nodes/utility/_utility_utils.py and nodes/image/_image_utils.py. It strips all / \ characters and leading . sequences before the value is concatenated into a file path. Applied to:

  • nodes/image/save_preview_image.pyfilename_prefix
  • nodes/video/save_preview_video.pyfilename_prefix
  • nodes/image/save_subsequent.pyfilename_suffix

2. str.startswithos.path.commonpath containment
Replaced both startswith checks in core/tj_api.py:

  • _safe_resolve() — now uses os.path.commonpath([real_base, target]) == real_base
  • delete_files route — same pattern with a try/ValueError guard for cross-drive paths on Windows

3. Read-side traversal via absolute path trust
_tj_resolve_media_path() in nodes/utility/_utility_utils.py no longer returns arbitrary absolute paths. An absolute path is only accepted if os.path.commonpath([known_root, realpath(cand)]) == known_root for one of the three ComfyUI directories (input / output / temp). Paths outside those roots fall through to the relative-path search instead.

4. _tj_media_meta_for_path str.startswith regression
Replaced the three media_real.startswith(...) guards with a local _in() helper that uses os.path.commonpath, consistent with the fix in (2).

@designloves2

Copy link
Copy Markdown
Contributor Author

Hi, thank you again for the thorough and patient review — the pointers were extremely helpful. I've pushed a new commit (32849bb) to main that restores the sanitizers/containment helpers and, while I was at it, closed several sibling gaps I found by auditing every write/read sink in the same style. Summary below.

Your five items — all addressed

  • resolve_target_dir() — rejects absolute paths and .., then validates with realpath + commonpath (nodes/image/_image_utils.py).
  • filename_prefix_tj_safe_filename_part restored on the save-preview image/video sinks (nodes/image/save_preview_image.py, nodes/video/save_preview_video.py).
  • filename_suffix — sanitizer restored (nodes/image/save_subsequent.py).
  • commonpath regression — str.startswith reverted back to os.path.commonpath containment in both _safe_resolve and delete_files (core/tj_api.py).
  • Read-side absolute-path trust — _tj_resolve_media_path now only accepts absolute paths that resolve inside the known ComfyUI roots (nodes/utility/_utility_utils.py).

Additional gaps I found and fixed in the same pass

  • nodes/image/save_eclipse_subsequent.pyfilename_suffix was concatenated unsanitized here (the sibling of save_subsequent.py); it now runs through _tj_safe_filename_part.
  • core/tj_api.py download_url — the fetch had no URL scheme validation, allowing file:// local reads and SSRF to internal hosts. It now allows only http/https and rejects hosts that resolve to private/loopback/link-local/reserved addresses.
  • nodes/image/multi_image_loader.py_resolve_path trusted arbitrary absolute paths and ..; it now contains every path within the input/output/temp roots via commonpath (the legitimate relative input/, output/, download/ prefixes still work unchanged).
  • nodes/utility/smart_show.py — both the file input and the media-path resolver now enforce known-root containment (replacing str.startswith with commonpath), and no longer copy arbitrary external files into the temp directory.

All containment now uses Path.resolve()/realpath + commonpath, and any input whose path is absolute or contains .. is rejected before joining. I verified that the legitimate relative-path flows still work and that traversal / file:// / internal-host attempts are blocked.

Whenever you have time, I'd be grateful for another look. Thank you for helping make this package safer.

@ltdrdata

Copy link
Copy Markdown
Member

Thanks for the thorough hardening — I re-verified the current head, and the four write-side path-traversal gaps I flagged earlier (filename_prefix, filename_suffix, the tj_api.py commonpath containment + delete_files, and the read-side absolute-path trust in _tj_resolve_media_path) are all correctly fixed now.

One reachable issue remains — read-side arbitrary file read via the Save & Preview Video node:

  • nodes/video/save_preview_video.py declares "video": (any_type,), so a plain STRING can be connected to that input (it is not restricted to a VIDEO object).
  • _tj_find_video_path() (nodes/utility/_utility_utils.py) takes the isinstance(obj, str) branch and passes the value straight to _tj_resolve_media_path().
  • _tj_resolve_media_path()'s relative fall-through (os.path.join(root, cand) for root in input/output/temp) does not reject .., so ../../../../etc/passwd resolves to a real out-of-root path.
  • _tj_media_meta_for_path() then treats it as external media and shutil.copy2()s it into the temp dir — disclosing an arbitrary file the ComfyUI process can read (.env, ssh keys, etc.).

Please apply the same containment you added on the write side to this read path: reject .. and absolute paths in _tj_resolve_media_path()'s relative branch, and validate the resolved path with os.path.realpath + os.path.commonpath against the input/output/temp roots before returning. The any_type video input should be treated as untrusted STRING.

I'll re-evaluate once the read path is contained.

designloves2 added a commit to designloves2/ComfyUI-TJ_NODE that referenced this pull request Jul 16, 2026
Addresses ComfyUI-Manager review (Comfy-Org/ComfyUI-Manager#2924):
arbitrary file read via any_type `video` STRING input.

- _tj_resolve_media_path(): reject `..` segments and out-of-root absolute
  paths; validate final path with realpath + commonpath against the
  input/output/temp roots. Treats the any_type video input as untrusted
  STRING, so ../../../../etc/passwd no longer escapes to _tj_media_meta_for_path's
  external-copy branch.
- Bump registry to 2.3.4.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@designloves2

Copy link
Copy Markdown
Contributor Author

Thanks again for the careful review — the remaining read-side arbitrary file read via Save & Preview Video is now fixed at head (commit 1cb8012).

Changes in nodes/utility/_utility_utils.py_tj_resolve_media_path():

  • The any_type video input is now treated as an untrusted STRING.
  • Relative branch rejects any .. path segment outright (covers ../../../../etc/passwd, ..\..\, input/../secret, etc.).
  • Out-of-root absolute paths are rejected.
  • Every returned path is validated with os.path.realpath + os.path.commonpath against the input/output/temp roots, so nothing outside those roots can be returned (also blocks symlink-escape). Because the resolver can no longer yield an external path, _tj_media_meta_for_path()'s external-copy (shutil.copy2) branch is unreachable from this input.

Verified with unit tests:

  • Legit paths (clip.mp4, video/sub.mp4, basename fallback, in-root absolute) resolve correctly.
  • Traversal/abs-escape inputs (../secret, ..\..\secret, out-of-root absolute, input/../secret) all return None.

Ready for re-evaluation whenever you have a moment — thanks!

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dbcc920d-ea87-4019-923b-86826d2e5ab5

📥 Commits

Reviewing files that changed from the base of the PR and between bd88cff and d5989d8.

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

📝 Walkthrough

Walkthrough

Adds a ComfyUI-TJ_NODE entry to custom-node-list.json, including GitHub metadata, git-clone installation configuration, and a description of its image batch and filename fixes.

Changes

Custom node registration

Layer / File(s) Summary
Add custom node catalog entry
custom-node-list.json
Registers ComfyUI-TJ_NODE with author, repository, files URL, git-clone installation type, and fix description metadata.

Possibly related PRs

Suggested reviewers: ltdrdata, azornes, anonymzx

🚥 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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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