get: reject copy destinations that resolve above the target

[Nexory]

Summary

get(rpath, lpath, recursive=True) builds each local destination by joining a name from the
source listing onto lpath, then writes it. A name carrying .. segments resolves above that
root, so the copy lands outside the directory the caller asked for. Nothing between
other_paths and get_file compares the result against the root.

A tar or zip whose member list is ["readme.txt", "../escaped.txt"] puts readme.txt in the
destination and escaped.txt one level above it. With more segments the location moves further
out: a member named ../../../../escaped.txt copied into a destination four levels down lands at
the filesystem root. There is no exception, no warning, and the call returns normally, so a caller
has nothing to check.

The async path in AsyncFileSystem._get has the same shape and behaves the same way, which is
the path s3fs, gcsfs, adlfs and HTTPFileSystem take.

This is the same class as #2047, one surface further along. That one hardened DirFileSystem._join
on the source side; the copy path still builds destinations by string arithmetic.

The change

A helper in fsspec/utils.py compares each built destination against the destination root and
raises ValueError if it resolves outside, called from AbstractFileSystem.get and
AsyncFileSystem._get right after other_paths, and only when lpath is a single string. When
the caller passes a list, they named every destination themselves and the guard stays out of the
way.

On the ".." objection from #2047

The concern raised there was that .. is a legitimate path part on many filesystems and only
means something on LocalFileSystem, so _join ended up narrowing its guard to that case. This
check does not need the same narrowing, because it sits on the destination side, and the
destination of get is a real local directory tree by definition. copying.rst states the
intent directly: the copy functions are meant to behave as POSIX cp does.

A .. that stays inside the root is still a plain name. a/b/../inner.txt lands at
dest/a/inner.txt both before and after the change, and there is a test for it.

Testing

Three cases were added to TestAnyArchive, so they run across zip, tar, tar.gz, tar.bz2, tar.xz
and libarchive, plus one in test_asyn_wrapper.py for the async path. Two of them state the
property (nothing above the destination) and the contract (ValueError); the third is the
control that .. inside the destination keeps working.

Measured in a conda environment built from ci/environment-linux.yml with the 3.14 matrix entry
and pip install -e .[test_full], CIRUN=true, so the s3fs, gcsfs, adlfs, pyarrow, pandas, dask,
zarr, fastparquet, kerchunk, panel, paramiko, smbprotocol and libarchive suites all ran:

	without the change	with the change
full suite	5 failed, 1892 passed, 66 skipped, 3 xfailed	5 failed, 1911 passed, 66 skipped, 3 xfailed

The set of failing tests is identical in both runs, so there is no regression. Those five are
test_reference.py parquet cases that already fail on an untouched tree in this environment.

One aside, in case it turns up in a CI run: test_cached.py::test_clear_expired is timing
dependent here. Run on its own it failed once in six attempts with the change and once in six
without it, so it does not look related to this patch, but I would rather mention it than have it
appear unexplained.

Before the change the new cases give 13 failed, 6 passed, the six being the control across the
archive scenarios. The first of them reports the substance rather than a missing exception:

AssertionError: copy wrote /tmp/pytest-of-root/pytest-1/test_get_does_not_write_above_0/escaped.txt,
above /tmp/pytest-of-root/pytest-1/test_get_does_not_write_above_0/dest

ruff check fsspec/ reports the same findings with and without the change.

Neighbours I did not touch

put and copy build their destinations the same way, but their target is a remote filesystem
where .. is an ordinary name part, which is the situation #2047 discussed. They seem to want a
different answer and are left alone here.

ReferenceFileSystem.get overrides get and calls other_paths itself, so this change does not
reach it. Its listing does carry .. keys through expand_path, but in my setup that copy wrote
nothing at all, including the benign file, so I have no working control there and make no claim
about it. Happy to look again if it is worth a separate issue.

I did not add a changelog entry, since recent merged pull requests do not seem to carry one. Say
the word and I will add a line.

[martindurant]

I suppose you should also do get_file() in that case?

[Nexory]

I looked at that before answering, and I think get_file() is the one place where the check has
nothing to work with: the caller passes lpath in, so there is no root to hold it against.
get() is different because it builds the destinations itself by joining names from the source
listing onto lpath, and that join is where a .. can escape.

I checked every get_file and _get_file in the tree rather than assume it. None of them derives
the destination from the source path: AbstractFileSystem.get_file takes lpath as given and only
creates the parent directory before opening it, and AsyncFileSystem._get_file is a bare
NotImplementedError. So a containment check there would need a root passed in as a new argument,
which changes the signature for something the caller already decided.

Where the guard genuinely does not reach is one level up, in the classes that override get()
itself:

• ReferenceFileSystem.get (reference.py) builds its destinations with other_paths and never
  goes through AbstractFileSystem.get, so it is not covered. I mentioned it in the description:
  its listing does carry .. keys, but in my setup that copy wrote nothing at all, not even the
  benign file, so I have no working control there and did not want to patch blind.
• DirFileSystem only forwards to the wrapped filesystem, so it is covered by the check in the
  wrapped get().

If you would rather have one place that covers get, put and copy together, the natural spot
is other_paths itself, since all three build their destinations there. I did not do that because
put and copy write to a remote filesystem where .. is an ordinary name part, which is the
distinction you drew on #2047, and other_paths cannot tell the two apart from where it sits.
Happy to go that way if you want it, but it seemed like your call rather than mine.

The branch rebases cleanly onto current master, which has moved four commits since I opened this,
two of them touching files here. Say the word and I will push the rebase.

[fallenmi]

Approved after an independent exact-head check.

I reproduced the defect on the exact PR base 669de5e: a tar listing with readme.txt and ../escaped.txt completed normally and wrote escaped.txt above the requested destination. On exact head bb4d3e0, the same call raises ValueError before either file is copied.

The new generated archive cases passed across the available zip/tar variants, and the complete touched archive plus async-wrapper test files finished with 112 passed and 20 optional-backend skips. I also merged this exact head locally into current master 9b7cd48; it merged cleanly, repeated the same 112/20 result, and fsspec/tests/test_utils.py passed 98/98. A direct containment matrix covered equality, nested paths, an internal .., sibling-prefix collisions, parent escapes, and filesystem root.

The guard is placed at the right boundary: after source-controlled names become local destinations and before directory creation or copying. Explicit destination lists remain caller-owned.

Disclosure: OpenAI Codex assisted with this independent review and the account owner authorized its publication.

[martindurant]

Should use pytest.raises like the other tests, no?

[Nexory]

Done. The two tests had the same fixture and the same archive, so I folded them
into one rather than leave a pytest.raises next to a try/except for the same
call. The remaining test asserts both the raise and that nothing was written
above the destination:

with scenario.provider(data) as archive:
    fs = fsspec.filesystem(scenario.protocol, fo=archive)
    with pytest.raises(ValueError, match="outside the destination"):
        fs.get("*", str(dest), recursive=True)

assert not outside.exists(), f"copy wrote {outside}, above {dest}"

The except ValueError: pass was there from before the guard existed, when the
call still completed and only the filesystem check could tell the difference.

While checking that the suite would actually notice a broken guard, I found that
it would not. check_contained had no direct test, only cover through fs.get(),
and its own edge cases were unguarded: dropping the separator from the prefix, so
that a sibling directory whose name starts with the destination counts as inside,
passes the entire suite. That is a one-character change away.

So there is a second commit with a small table over equality, nested paths, an
internal .., a parent escape, and that sibling case. With the separator removed
it now fails on exactly the sibling row and nothing else.

test_archive.py, test_asyn_wrapper.py and test_utils.py together: 191
passed, 19 skipped. Lint unchanged against the baseline.

The rebase offer stands: the branch still applies cleanly to current master, and
I will push it if you want the PR on top of it.

[Nexory]

A correction to my last comment. When I wrote that the branch still applied
cleanly, that had stopped being true by the time it posted: master moved while I
was writing. The conflict was one import in fsspec/tests/test_utils.py, where
#2091 and my new test each added one.

I have merged current master into the branch rather than rebasing it, so the
commit that was reviewed stays in the history under its own hash. The resolution
keeps both imports. Say the word if you would rather have a linear branch and I
will rebase instead.

Measured on the merged branch and on an untouched checkout of master, same image
and same run: the suite gives 1630 passed, 164 skipped and 2 xfailed with the
change, and 1612 passed, 162 skipped and 2 xfailed without it, both exit 0. The
difference is the tests this PR adds.

One note on lint, because it briefly looked as though this change had introduced
two findings in fsspec/implementations/sftp.py. It had not. That file is not in
this diff. Both findings already existed and only moved from lines 159 and 166 to
176 and 183 when #2104 inserted code above them, while the baseline I compared
against still came from the older tree. Against a baseline taken on current
master the count is 9 before and 9 after, with nothing new.