zipfile.Path regression

Bug report

Bug description:

#122906 introduced a regression with directories that look like Windows drive letters (on Linux):

>>> import io, zipfile
>>> zf = zipfile.ZipFile(io.BytesIO(), "w")
>>> zf.writestr("d:/foo", "bar")
>>> zf.extractall("a")
>>> open("a/d:/foo").read()
'bar'
>>> p = zipfile.Path(zf)
>>> x = p / "d" / "foo"
>>> y = p / "d:" / "foo"
>>> list(p.iterdir())   # before: [Path(None, 'd:/')]
[Path(None, 'd/')]
>>> p.root.namelist()   # before: ['d:/foo', 'd:/']
['d/foo', 'd/']
>>> x.exists()          # before: False
True
>>> y.exists()          # before: True
False
>>> zf.extractall("b")  # before: worked like above
KeyError: "There is no item named 'd/foo' in the archive"
>>> x.read_text()       # before: FileNotFoundError
KeyError: "There is no item named 'd/foo' in the archive"
>>> y.read_text()       # before: worked
FileNotFoundError: ...

This is the result of _sanitize() unconditionally treating a directory that looks like a drive letter as such and removing the colon, regardless of operating system:

cpython/Lib/zipfile/_path/__init__.py

Line 141 in 58fdb16

bare = re.sub('^([A-Z]):', r'\1', name, flags=re.IGNORECASE)

Whereas _extract_member() uses os.path.splitdrive() (which is a no-op on Linux):

cpython/Lib/zipfile/__init__.py

Line 1807 in 58fdb16

arcname = os.path.splitdrive(arcname)[1]

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

Paths like foo//bar and foo\\bar also cause issues:

>>> import io, zipfile
>>> zf = zipfile.ZipFile(io.BytesIO(), "w")
>>> zf.writestr("foo//bar", "text")
>>> zf.namelist()
['foo//bar']
>>> p = zipfile.Path(zf)
>>> p.root.namelist()   # before: ['foo//bar', 'foo/']
['foo/bar', 'foo/']
>>> x = p / "foo//bar"
>>> y = p / "foo" / "bar"
>>> x.exists()          # before: True
False
>>> y.exists()          # before: False
True
>>> x.read_text()       # before: works, returns 'text'
FileNotFoundError: ...
>>> y.read_text()       # before: FileNotFoundError
KeyError: "There is no item named 'foo/bar' in the archive"

>>> import io, zipfile
>>> zf = zipfile.ZipFile(io.BytesIO(), "w")
>>> zf.writestr("foo\\bar", "text")
>>> zf.namelist()
['foo\\bar']
>>> p = zipfile.Path(zf)
>>> p.root.namelist()   # before: ['foo\\bar']
['foo/bar', 'foo/']
>>> x = p / "foo\\bar"
>>> y = p / "foo" / "bar"
>>> x.exists()          # before: True
False
>>> y.exists()          # before: False
True
>>> x.read_text()       # before: works, returns 'text'
FileNotFoundError: ...
>>> y.read_text()       # before: FileNotFoundError
KeyError: "There is no item named 'foo/bar' in the archive"

@jaraco

cc @jaraco

Sanitising is good. But e.g. .open() will not work if it can't map back to the original path to pass that to ZipFile.open(). Though .joinpath() will of course never use // or \\, so opening these files already required constructing the path without using that. Not sure what's best here if opening files with paths affected by sanitising is meant to be supported.

# before
>>> import io, os, zipfile
>>> zf = zipfile.ZipFile(io.BytesIO(), "w")
>>> zf.writestr("foo/bar", "one")
>>> zf.writestr("foo\\bar", "two")
>>> zf.namelist()
['foo/bar', 'foo\\bar']
>>> zf.extractall("a")
>>> os.system("tree a")
a
├── foo
│   └── bar
└── foo\bar
>>> p = zipfile.Path(zf)
>>> p.root.namelist()
['foo/bar', 'foo\\bar', 'foo/']
>>> (p / "foo" / "bar").read_text()
'one'
>>> (p / "foo\\bar").read_text()
'two'

# after
>>> import io, os, zipfile
>>> zf = zipfile.ZipFile(io.BytesIO(), "w")
>>> zf.writestr("foo/bar", "one")
>>> zf.writestr("foo\\bar", "two")
>>> p = zipfile.Path(zf)
>>> zf.namelist()
['foo/bar', 'foo/bar', 'foo/']
>>> zf.extractall("a")
>>> os.system("tree a")
a
└── foo
    └── bar
>>> (p / "foo" / "bar").read_text()
'one'
>>> (p / "foo\\bar").read_text()
FileNotFoundError: ...

Sanitising is good. But e.g. .open() will not work if it can't map back to the original path to pass that to ZipFile.open(). Though .joinpath() will of course never use // or \\, so opening these files already required constructing the path without using that. Not sure what's best here if opening files with paths affected by sanitising is meant to be supported.

Keeping a dict mapping sanitised paths back to the originals might be a good choice. Though .namelist() will still be "wrong" for the ZipFile (e.g. when calling .extractall()). And it doesn't handle "duplicates" (see above).

Not sure what's best here if opening files with paths affected by sanitising is meant to be supported.

My expectation was that paths affected by sanitizing would not be supported.

regression with directories that look like Windows drive letters (on Linux)

My goal is to provide a (perhaps constrained) interface that's portable and provides consistent behavior independent of platform.

I'll have to think about this some more

Paths like foo//bar and foo\\bar also cause issues:

I was not expecting that foo\bar or d:/foo were in use in zip files.

Can you say more about the use-cases that are affected by these paths? What causes these zip files to exist? What does the author of such a zip file expect to happen when consumed by a zipfile client?

I was not expecting that foo\bar or d:/foo were in use in zip files.

Are these file names common? Maybe not. But on Linux these are perfectly acceptable file names. And whilst they may indeed cause portability issues for Windows users and I would thus avoid them when creating ZIP files for use on Windows, the ZIP format also considers them perfectly acceptable.

I myself have quite a few music files with odd file names; song titles can be quite varied. And whilst I've sometimes run into problems extracting those files on other operating systems I have never had issues creating or listing ZIP files.

File names like foo/..//./bar can be considered malformed and would not be generated by simply creating a ZIP file from an existing directory tree. But d:/foo\\bar is not malformed, just unusual (and perhaps not portable).

$ mkdir d:
$ echo "Hi!" >> d:/foo\\bar
$ zip -r foo.zip d:
  adding: d:/ (stored 0%)
  adding: d:/foo\bar (stored 0%)
$ unzip -l foo.zip
Archive:  foo.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  2024-08-24 04:04   d:/
        4  2024-08-24 04:04   d:/foo\bar
---------                     -------
        4                     2 files
$ python3 -mzipfile -l foo.zip
File Name                                             Modified             Size
d:/                                            2024-08-24 04:04:20            0
d:/foo\bar                                     2024-08-24 04:04:20            4
$ zipinfo.py -e foo.zip
Archive:  foo.zip
Zip file size: 308 bytes, number of entries: 2
drwx------  3.0 unx        0 bx        0 stor 2024-08-24 04:04:20 00000000 d:/
-rw-------  3.0 unx        4 tx        4 stor 2024-08-24 04:04:20 54a8a39c d:/foo\bar
2 files, 4 bytes uncompressed, 4 bytes compressed:  0.0%

Tools like zip and unzip and my own zipinfo.py handle them just fine. So I would expect the same from Python. And indeed zipfile.ZipFile handles them just fine. But zipfile.Path now rejects them.

I generally expect to be able to take any directory tree and turn it into a ZIP file without having to worry about file names containing colons or backslashes. Or at the most that I might need to be careful when extracting them on Windows. I would not expect to have problems merely listing the contents, especially when not even using Windows.

So it is very surprising when zipfile.Path effectively rejects perfectly valid ZIP files just because they have "unsupported" file names, especially since it didn't do so before. I would perhaps expect this kind of backwards-incompatible change to happen in a major version, given a good reason, but not in a security patch. And for it to be clearly documented if the interface is constrained like this. It also fails in unexpected ways, instead of providing a clear error message that makes it clear the problem is an unsupported file name.

My expectation was that paths affected by sanitizing would not be supported.

And my expectation would be that such paths would be either explicitly rejected as malformed if they cannot reasonably be supported. Or when they can be, handled gracefully with a portable interface that maps sanitised names providing a consistent interface that can reasonably be supported onto the underlying file names instead of simply causing attempts to read files to fail because the underlying file name does not match the sanitised one. And before this change, d:/foo\\bar worked just fine.

This worked perfectly fine before. Now it raises FileNotFoundError.

>>> zf = zipfile.ZipFile("foo.zip", "r")
>>> zf.namelist()
['d:/', 'd:/foo\\bar']
>>> p = zipfile.Path(zf)
>>> (p / "d:").is_dir()
True
>>> (p / "d:" / "foo\\bar").read_text()
'Hi!\n'

And if d:/foo\\bar is not considered a valid path by zipfile.Path, I'd expect an error. Instead it happily tells me there is a file named d/foo/bar. Except when I try reading I get an error that says there is no such file:

>>> (p / "d" / "foo" / "bar").is_file()
True
>>> (p / "d" / "foo" / "bar").read_text()
KeyError: "There is no item named 'd/foo/bar' in the archive"

Note that it's not just d:/foo that's no longer allowed, e.g. V: The New Mythology Suite.flac is now also a forbidden file name (though extractall() would remove the "V: " -- but only on Windows, and only when extracting, not listing files).

Meanwhile, this seems incorrect:

>>> zipfile._path.SanitizedNames._sanitize("d:/foo")
'd/foo'
>>> zipfile._path.SanitizedNames._sanitize("/d:/foo")
'd:/foo'

Meanwhile, as I posted here, the only fix needed for the infinite loop would be the loop condition here:

cpython/Lib/zipfile/_path/__init__.py

Lines 53 to 55 in 52caaef

    
           while path and path != posixpath.sep: 
        
               yield path 
        
               path, tail = posixpath.split(path)

This should work:

    while path and path != posixpath.sep * len(path):
        yield path
        path, tail = posixpath.split(path)

Or this:

    while path:
        yield path
        head, tail = posixpath.split(path)
        if head == path:
            break
        path = head

[3.10] gh-123270: Replaced SanitizedNames with a more surgical fix. (G…

Advertise that zipfile.Path does not do any name sanitization and it's the responsibility of the caller to check the inputs, etc.

I think documenting the caveats would be good.

zipfile.Path could provide its own traversal that could offer some safety checks.

Something like that might be nice. Though perhaps it could be more generic instead of being part of zipfile.Path, even if zipfile.Path would then perhaps provide an extraction API using that?

I opened two issues to track. I'm not planning on driving them, but I'm happy to guide and advise. Feel free to take one or both on, or help drive the conversation toward something that's acceptable. Thanks.

Thanks. I have too many other projects to work on currently to take this on, but happy to comment and review when I can :)

Hi,

I'm new to contributing in general and have some experience in programming. I wanted to start by helping with documentation. How can I get started on this?

I don't think there's anything here that needs documentation updates, since it's a bugfix. You should take a look at the devguide.

I don't think there's anything here that needs documentation updates, since it's a bugfix.

But #123726 is about updating the documentation to explain the caveats regarding name sanitation as a result of the discussion here :)

chore(deps): Bump zipp from 3.20.0 to 3.20.2 (#1996)

Bump zipp from 3.20.0 to 3.20.2 (#566)

Bump the dependabot group with 4 updates (#312)

chore(deps): Bump zipp from 3.20.0 to 3.21.0 in /experimenter/tests (#…

Bump the dependabot group across 1 directory with 7 updates (#65)

Bump the pip group with 4 updates (#220)

Bump the dependabot group across 1 directory with 7 updates (#65)

Navigation Menu

Repository navigation

zipfile.Path regression #123270

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

43 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	while path and path != posixpath.sep:
	yield path
	path, tail = posixpath.split(path)

zipfile.Path regression #123270

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

obfusk commented on Aug 23, 2024

ZeroIntensity commented on Aug 23, 2024

obfusk commented on Aug 23, 2024

obfusk commented on Aug 23, 2024

obfusk commented on Aug 23, 2024

jaraco commented on Aug 24, 2024

obfusk commented on Aug 24, 2024

obfusk commented on Aug 24, 2024

obfusk commented on Aug 24, 2024

obfusk commented on Aug 24, 2024

obfusk commented on Aug 24, 2024

43 remaining items

obfusk commented on Sep 5, 2024

jaraco commented on Sep 5, 2024

obfusk commented on Sep 5, 2024

abj101 commented on Sep 7, 2024

ZeroIntensity commented on Sep 7, 2024

obfusk commented on Sep 7, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions