Comfy-Org / Comfy-Org/comfy-cli

`registry-install`: registry-supplied `version` string is joined unvalidated into the download path

Open
#747 0 comments 0 reactions 1 assignee Claimed by @mattmillerai View on GitHub
bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.