Comfy-Org / Comfy-Org/comfy-cli
`registry-install`: registry-supplied `version` string is joined unvalidated into the download path
- Dominant language
- Python
- Stars
- 968
- Forks
- 151
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 77
Description
**What** — `comfy node registry-install` builds the archive's download destination out of the **registry-supplied `version` string**, with no validation:
`comfy_cli/command/custom_nodes/command.py:1410`
```python
local_filename = node_specific_path / f"{node_id}-{node_version.version}.zip"
...
download_file(node_version.download_url, local_filename) # :1413
```
`node_version.version` is `api_node_version["version"]` taken verbatim from the registry response (`comfy_cli/registry/api.py:259`) — mandatory field, no validation anywhere between the HTTP response and the path join. `pathlib.__truediv__` does not sanitize, and `download_file` does `local_filepath.parent.mkdir(parents=True, exist_ok=True)` (`comfy_cli/file_utils.py:837`) before writing, so it will happily create the traversed directory chain first.
A `version` of `1.0.0/../../../../some/other/dir/x` therefore writes attacker-supplied bytes (the body of `downloadUrl`) outside `custom_nodes/`.
**Scope, stated honestly** — the last path component is always `…{version}.zip`, so the written file's *name* still ends in `.zip`. This is an arbitrary-**directory** write of a `.zip`-suffixed file with fully attacker-chosen content, not an arbitrary-filename overwrite. That rules out the obvious "clobber a dotfile" escalation, and is why I'd call this moderate rather than high. It still lets a publisher plant content anywhere the user can write, overwrite an existing `.zip`, and fill a disk. The cleanup `os.remove(local_filename)` at `:1428` only runs when extraction returns normally.
For the avoidance of doubt about the *other* two values in that line: `node_id` is the user's own CLI argument (`registry_install(node_id: str, …)` at `:1341`), not registry-supplied, and `node_specific_path` derives from it — so this is specifically about `version`. And the extraction itself is fine: `extract_package_as_zip` uses `zipfile.extractall`, which sanitizes member names and never materializes symlinks (I tested `../..`, an absolute member, and an `S_IFLNK` member on CPython 3.12.3 — all three land inside the destination, the symlink as a regular file). The bug is where the *archive file itself* is written, not where it is unpacked.
**Attacker position** — the registry API (`api.comfy.org`), or more plausibly a **node publisher** who can put an arbitrary string in `project.version` in their `pyproject.toml` at publish time (`comfy_cli/registry/api.py:121` sends it). Whether that is exploitable end-to-end depends on whether the registry backend enforces semver on `version`, which I could not check from the CLI repo — so treat this as "the client should not be relying on that", not as a demonstrated live exploit.
**The fix already exists in this repo.** `comfy_cli/command/models/models.py:130-157` `_reject_unsafe_component` does exactly this check, and its docstring describes exactly this situation:
> `local_filename` and `basemodel` are joined into the destination path, and both can come straight off the CivitAI API response … which is remote input, accepted without a prompt in non-interactive runs. `pathlib`/`os.path.join` do not sanitize `..` or an absolute component, so an unvalidated value writes outside the workspace.
It rejects `.`, `..`, absolute paths, drive letters, `/` and `\`. Applying it to `node_version.version` (and cheaply to `node_id`) before `command.py:1410` closes this with no new code.
**Assignee note** — `git log -S` puts the original line at `6b4786c` (James Kwon, 2024-05-11, *"Implementation for installing node from registry (#41)"*), but his last commit to this repo was 2024-07-22. Assigning to @mattmillerai instead, who wrote `_reject_unsafe_component` — the exact guard this path is missing — and is the current active owner of both `custom_nodes/command.py` and `registry/api.py`. Reassign if that's wrong.
---
Found during a cross-repo security review of `comfy-cli`, 2026-08-21. Related: #746, #725.
Contributor guide
Assessment
This issue has not been assessed yet.