fix(utils): apply the stdlib data filter on both tarball extraction paths

[christian-byrne]

Closes #725

The problem

extract_tarball() bypassed Python's tarball extraction filter on both of its paths:

• non-progress path: tar.extractall(filter=None)
• progress path: a custom _filter that updated the progress bar and then return tinfo unmodified

So a member named ../evil.txt or /etc/evil was written wherever it pointed — the CVE-2007-4559 class. It is reachable from caller-supplied input: StandalonePython.FromTarball(fpath) takes an arbitrary path, and FromDistro() downloads from a configurable asset_url_prefix.

The crux: is cpython#107845 still a problem?

The filter was disabled deliberately, not by accident. The code carried:

# TODO: ideally we'd use data_filter here, but it's busted: https://github.com/python/cpython/issues/107845

That TODO is now stale. What the bug actually was:

data_filter resolved a symlink's target against the destination root rather than against the directory containing the link. TarInfo.linkname is relative to the link's own directory for symlinks but relative to the archive root for hardlinks, and PEP 706's original implementation used the hardlink rule for both. Result: LinkOutsideDestinationError raised on perfectly valid archives.

Two things follow, and both matter here:

1. It was a false-rejection bug, never an escape. The buggy target was realpath(dest/linkname); the correct one is realpath(dest/dirname(name)/linkname). dirname(name) is always inside dest (the member name itself is checked earlier), so the buggy computation starts strictly shallower and can only reject more than the correct one — never accept a link that actually escapes. Upstream carries no security label and no CVE; the NEWS entry says it "will no longer reject some valid tarballs".
2. It is fixed on every version we support. Fixed by cpython#107846 and backported to every live branch, all released 2023-08-24:

Branch	First fixed release
3.10	3.10.13
3.11	3.11.5
3.12	3.12.0rc2 → all 3.12.x finals are clean
3.13+	clean from 3.13.0a1

The affected set was exactly the releases that introduced the filter: 3.8.17, 3.9.17, 3.10.12, 3.11.4, plus 3.12.0b1–rc1.

requires-python = ">=3.10", and CI runs 3.10 (pytest/build) and 3.12 (mac/windows/GPU). Intersecting that with the affected set leaves exactly 3.10.12 and 3.11.4 — two patch releases superseded three years ago. And since extractall(filter=...) does not exist at all before 3.10.12, the existing code already could not run below that line, so that two-release window is the entire residual exposure. On those two the worst case is a loud LinkOutsideDestinationError, not a silent escape — it fails closed.

So: use data_filter directly, no version gate. The pypa/build-style version gate is the wrong trade here, because its fallback branch reverts to unfiltered extraction on precisely those versions — reintroducing the hole this PR closes, to dodge a bug that only ever produced a loud error.

The fix

The progress bar and the safety decision were conflated in one callback. They are separable concerns:

• non-progress path → tar.extractall(filter="data")
• progress path → tar.extractall(members=_reporting_members(tar), filter="data"), where the generator drives the two progress bars while yielding members

members drives the UI; filter stays the literal "data" filter. Using the string literal rather than filter=tarfile.data_filter also clears ruff's S202 — ruff check --select S202 comfy_cli now passes clean across the package, which the callable form would not have.

Tests

TestExtractTarballFiltering in tests/comfy_cli/test_utils.py, parametrised over both show_progress values so neither path can regress. The malicious tarball is built in-test; no binary fixture.

• test_rejects_path_traversal_member — a tarball carrying ../evil.txt must not write outside the extraction directory, and must be rejected.
• test_allows_internal_symlinks — the control. python-build-standalone tarballs (the only thing StandalonePython extracts) are full of relative symlinks like bin/python3 -> python3.12. Those stay inside the destination and must still extract. This is the case cpython#107845 would have broken.

Verified the regression test genuinely fails against the unfixed code, on both paths:

FAILED tests/comfy_cli/test_utils.py::TestExtractTarballFiltering::test_rejects_path_traversal_member[False]
FAILED tests/comfy_cli/test_utils.py::TestExtractTarballFiltering::test_rejects_path_traversal_member[True]

E  AssertionError: traversal member escaped the extraction directory:
E    /tmp/pytest-of-c_byrne/pytest-76/test_rejects_path_traversal_me1/evil.txt
E  assert not True

With the fix, tests/comfy_cli/test_utils.py is 8 passed, and tests/comfy_cli/test_standalone.py (the caller) is 11 passed / 3 skipped.

Notes for the reviewer

• feat(workflow): agent-first workflow editing — CRDT edit primitives, recipes, offline catalog, cloud run association #511 also touches comfy_cli/utils.py, but only to make requests a lazy import inside download_url. It does not touch extract_tarball, so these should not conflict.
• Scope note: filter="data" is not an absolute guarantee on an unpatched interpreter — there is a separate, later cluster of filter-bypass bugs (cpython#135034, CVE-2025-4517 / 4330 / 4138 / 4435, fixed June 2025). That is an argument for keeping interpreters current, not against this change: filtered extraction is strictly better than the filter=None it replaces.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 42 minutes

Limit details: You’ve used all 3 included reviews currently available. Your 76 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1392cace-cafe-4fee-9cfe-2cd1198d7d41

📥 Commits

Reviewing files that changed from the base of the PR and between 0a6cb6e and c0fe495.

📒 Files selected for processing (2)

• comfy_cli/utils.py
• tests/comfy_cli/test_utils.py

---

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

[christian-byrne]

Verified this branch does what it says, and found one thing it does not cover — details and repro in #725 (comment).

What this branch fixes, confirmed. Ran all three filter forms against a tarball with a ../../ESCAPED.txt member:

origin/main  filter=None            (utils.py:177)     -> extracted; escaped to /tmp/tar-.../ESCAPED.txt
origin/main  identity callable      (utils.py:187-195) -> extracted; escaped to /tmp/tar-.../ESCAPED.txt
c0fe495      filter="data"                             -> REFUSED: OutsideDestinationError

So both paths really were open and both are really closed. The commit message's read of cpython#107845 matches mine — false-rejection, never an escape, fixed in 3.10.13 / 3.11.5 / 3.12.0rc2.

What it does not cover. utils.py:162-172 derives an rmtree target from the first tar member's name, before extractall runs:

old_name = info.name.split("/")[0]
extractPath = inPath.with_name(old_name)     # with_name("..") is legal
shutil.rmtree(extractPath, ignore_errors=True)

A first member named ../evil gives old_name == "..", and the CLI recursively deletes the parent of the download directory. Same repro on both branches:

                    'python/bin/python3'   '../evil'
origin/main         workspace intact       workspace after: []
c0fe495 (this PR)   workspace intact       workspace after: []

The filter can't help here — the delete is header-derived and happens first.

Since this PR closes #725, merging as-is would retire the ticket with the delete primitive still live. Two options: extend this branch to also scope the extraction (extractall(path=...) into a temp dir, then move the top-level entry — note neither call passes path= today, so extraction currently lands in os.getcwd()), or explicitly de-scope it and drop the auto-close so #725 stays open. Either is fine, but it should be a decision rather than a side effect.

Also worth knowing while this is open, from the same review: the download host is not pinned (standalone.py:82-88 takes both tag and asset_url_prefix out of a fetched latest-release.json), and the tarball is never checksummed even though SHA256SUMS is already downloaded two functions away (standalone.py:30-59 parses it only for version numbers). Both are in the #725 comment.

[christian-byrne]

Re-verified the above independently against origin/main and pull/734/head (c0fe4956dd). The
region is byte-identical on both — this PR adds filter="data" to the extractall calls, and the
rmtree happens before either of them:

old_name = info.name.split("/")[0]
extractPath = inPath.with_name(old_name)
shutil.rmtree(extractPath, ignore_errors=True)   # <-- filter never gets a vote

One detail worth adding, because the obvious way to check this gives the wrong answer. On 3.12.3:

'..'       -> /home/u/dl/..
'.'        -> RAISES ValueError Invalid name '.'
'../evil'  -> RAISES ValueError Invalid name '../evil'

with_name rejects a traversal — so trying with_name("../evil"), seeing the ValueError, and
concluding the report is a false positive is a very easy mistake to make. .. is the single string
that gets through, and split("/")[0] is precisely what turns the rejected form into the accepted
one. A member named ../evil yields old_name = "..", and rmtree then takes the parent of the
download directory.

Not asking for scope creep on this PR — just flagging that merging it as-is would auto-close #725
with the rmtree path still live, so the ticket would stop tracking the part that deletes files.
Either extend the fix to validate old_name (reject anything that isn't a plain single component),
or land this and leave #725 open against the remainder.

[christian-byrne]

Following up on the earlier comment about the rmtree half — adding only the thing it didn't have: a runnable test, and a measured red/green.

The two tests this PR adds both build their tarball with payload/keep.txt first:

_write_member(tar, "payload/keep.txt", b"benign")
_write_member(tar, "../evil.txt", b"pwned")

So tar.next() returns payload/keep.txt, old_name is "payload", and extractPath is benign. The tests exercise the write primitive that filter="data" closes, and never reach the delete primitive — which runs off the first header, before extractall, where the filter has no vote.

Making the malicious member first is the whole difference. Measured with the same harness on three variants:

utils.py from	attack case	control (benign first member)
origin/main @ 3ff9f55	2 failed	2 passed
this PR's head @ c0fe495	2 failed	2 passed
this PR's head + the 2-line guard below	2 passed	2 passed

Failure message on both unfixed refs, for both show_progress values:

E       AssertionError: workspace settings were deleted by a tar header

The control passing on every row is what rules out a broken harness.

The guard that turns it green, at comfy_cli/utils.py immediately before extractPath = inPath.with_name(old_name):

if old_name in ("", ".", "..") or "/" in old_name or "\\" in old_name:
    raise ValueError(f"refusing to extract tarball with unsafe top-level member name: {old_name!r}")

Drop-in test, written against this PR's own file so it can be appended to TestExtractTarballFiltering:

@pytest.mark.parametrize("show_progress", [False, True])
def test_first_member_name_cannot_select_an_rmtree_target(self, tmp_path, monkeypatch, show_progress):
    """The first tar header must not be able to pick what gets deleted.

    extractPath is derived from tar.next().name and rmtree'd BEFORE extraction,
    and Path.with_name("..") is legal, so a first member named "../evil"
    resolves the delete target to the parent of the download directory.
    """
    workspace = tmp_path / "workspace"
    downloads = workspace / "downloads"
    downloads.mkdir(parents=True)
    (workspace / "comfy.settings.json").write_text('{"real": "settings"}')

    tarball = downloads / "python.tgz"
    with tarfile.open(tarball, "w:gz") as tar:
        _write_member(tar, "../evil", b"x")          # FIRST — this is the point
        _write_member(tar, "payload/keep.txt", b"x")

    monkeypatch.chdir(downloads)
    with patch("comfy_cli.utils.Live"):
        try:
            extract_tarball(tarball, downloads / "out", show_progress=show_progress)
        except Exception:
            pass  # the delete already happened; how extraction ends is not the point

    assert (workspace / "comfy.settings.json").exists(), "workspace settings were deleted by a tar header"

Filing note: this came out of a closed-bug regression audit whose lens was "closed bugs whose regression test tests the wrong half". This PR is the cleanest live example of that shape, which is why it got a test rather than another issue. No objection to the PR's actual change — filter="data" is correct and measurably closes the write primitive; the only ask is that it not auto-close #725 while the delete primitive is still reachable.