zipfile.Path regression

[obfusk]

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

• gh-123270: Replaced SanitizedNames with a more surgical fix. #123354
• [3.13] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123410
• [3.12] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123411
• [3.11] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123425
• [3.10] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123426
• [3.9] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123432
• [3.8] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123433

[obfusk]

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"

[ZeroIntensity]

cc @jaraco

[obfusk]

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.

[obfusk]

# 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: ...

[obfusk]

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).

[jaraco]

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?

[obfusk]

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.

[obfusk]

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.

[obfusk]

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"

[obfusk]

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'