`tar.extractall()` runs with the extraction filter disabled on both paths

[christian-byrne]

What — extract_tarball() calls tar.extractall(filter=None) on the non-progress path, and on the progress path passes a custom _filter that returns tinfo unmodified. Both bypass Python's data_filter, so a tarball member with an absolute path or ../ traversal is written wherever it points.

Why it matters here — This is the CVE-2007-4559 class. It matters here because StandalonePython.FromTarball(fpath) accepts an arbitrary caller-supplied path, and FromDistro() downloads from a configurable asset_url_prefix rather than a pinned source. The default path is a trusted HTTPS distro, which is why this is recommended and not critical — but the entry point is general.

Worth being clear: this was a knowing tradeoff, not an oversight. The code carries # TODO: ideally we'd use data_filter here, but it's busted: https://github.com/python/cpython/issues/107845. The ask is to recheck whether that upstream bug is still open on the Python versions comfy-cli now supports, not to fix a mistake.

Evidence

$ ruff check --select S202 --output-format concise comfy_cli
comfy_cli/utils.py:174:13: S202 Uses of `tarfile.extractall()`
comfy_cli/utils.py:195:13: S202 Uses of `tarfile.extractall()`

$ sed -n '173,175p;184,195p' comfy_cli/utils.py
        with tarfile.open(inPath) as tar:
            tar.extractall(filter=None)
...
        def _filter(tinfo: tarfile.TarInfo, _path: PathLike):
            ...
            # TODO: ideally we'd use data_filter here, but it's busted: cpython#107845
            # return tarfile.data_filter(tinfo, _path)
            return tinfo

$ grep -n 'extract_tarball' comfy_cli/standalone.py
128:        extract_tarball(inPath=fpath, outPath=rpath, show_progress=show_progress)

Fix

Recheck cpython#107845 against the minimum supported Python. If it is fixed, switch both paths to data_filter. If not, keep the progress callback for UI but have _filter delegate to tarfile.data_filter(tinfo, _path) for the safety decision — the progress bar and the filter are separable concerns, and the current code conflates them.

---

Found by repo-audit during repo improvement sweep 2026-08-17. Parent: #723

[christian-byrne]

Re-reviewing this function for a cross-repo security sweep, and there is a second primitive in extract_tarball that this issue does not cover and that #734 does not fix.

comfy_cli/utils.py:162-172:

with tarfile.open(inPath) as tar:
    info = tar.next()
    old_name = info.name.split("/")[0]     # attacker-controlled tar header
extractPath = inPath.with_name(old_name)
shutil.rmtree(extractPath, ignore_errors=True)

Path.with_name("..") is legal — only names containing a separator raise, and ".." does not. Path("/w/downloads/py.tgz").with_name("..") is /w/downloads/.., which resolves to /w. So a tarball whose first member is named ../anything makes old_name == ".." and the CLI rmtrees the parent of the download directory.

The important part: this delete is derived from the header, and it happens before extractall. The extraction filter never gets a vote. I ran the same repro against origin/main and against c0fe495 (the head of #734):

                    first member 'python/bin/python3'   first member '../evil'
origin/main         workspace intact                    workspace after: []
c0fe495 (PR #734)   workspace intact                    workspace after: []

In the attack row comfy.settings.json, my_models/sd_xl.safetensors and downloads/ are all gone — on both branches. (extract_tarball then raises FileNotFoundError on the move, well after the damage.) The control row is untouched in both, so this is the tar header steering the delete, not the test.

#734 is correct about what it fixes — I verified separately that filter="data" raises OutsideDestinationError where both of origin/main's forms silently write outside the destination. It just closes the write primitive and leaves the delete primitive open. If #734 merges and auto-closes this issue, half the function stays exposed.

Two other things worth folding into the scope while it's open, both of which raise how reachable this is:

1. The download host is remote-controlled, not pinned. comfy_cli/standalone.py:82-88 fetches https://raw.githubusercontent.com/astral-sh/python-build-standalone/latest-release/latest-release.json and takes both tag and asset_url_prefix out of that JSON; :97-99 then builds the URL as f"{asset_url_prefix.rstrip('/')}/{fname.lstrip('/')}". So whoever controls that JSON chooses the host the tarball comes from. The tag from the same JSON is also interpolated into fname, which is the local filename download_url writes — so a tag containing / or .. steers the write path too. The issue text above says "downloads from a configurable asset_url_prefix"; it's worth being precise that in the default tag == "latest" path it is not configured locally at all, it is fetched.

2. The tarball is never verified. _resolve_python_version (standalone.py:30-59) downloads SHA256SUMS from the release and uses it only as a regex source to find patch version numbers. The digest it contains is never compared against the tarball that gets extracted. The checksum file is already being fetched — comparing it is nearly free and would be the real defense here, independent of the filter.

Suggested fix for the delete primitive: don't derive a filesystem path from the header at all. Extract into a fresh scoped temp directory (extractall(path=...), which neither call currently passes — today extraction lands in os.getcwd()), then move the single top-level entry into outPath. If the header-derived name has to stay, reject any old_name that is not a single inert component (., .., absolute, or containing a separator) before it reaches rmtree.

Happy to extend #734 to cover this rather than opening a separate issue — flagging here first so the scope decision is explicit rather than implied by whatever merges.