GeezerVison is showing github.com — zipfile.Path regression · Issue #123270 · python/cpython
zipfile.Path regression #123270DescriptionBug 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:
|
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):
|
arcname = os.path.splitdrive(arcname)[1] |
CPython versions tested on:
3.12
Operating systems tested on:
Linux
Linked PRs
Activitychanged the title [-]zipfile.Path regression with directories that look like Windows drive letters (on Linux)[/-] [+]zipfile.Path regression[/+] on Aug 23, 2024obfusk commented on Aug 23, 2024 ContributorAuthor More actions 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 commented on Aug 23, 2024 obfusk commented on Aug 23, 2024 ContributorAuthor More actions 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 commented on Aug 23, 2024 ContributorAuthor More actions # 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 commented on Aug 23, 2024 ContributorAuthor More actions
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 commented on Aug 24, 2024
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 commented on Aug 24, 2024 ContributorAuthor More actions
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 commented on Aug 24, 2024 ContributorAuthor More actions
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 commented on Aug 24, 2024 ContributorAuthor More actions 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 commented on Aug 24, 2024 ContributorAuthor More actions 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'obfusk commented on Aug 24, 2024 ContributorAuthor More actions Meanwhile, as I posted here, the only fix needed for the infinite loop would be the loop condition here:
|
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 = headobfusk commented on Sep 5, 2024 ContributorAuthor More actions
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? jaraco commented on Sep 5, 2024 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. obfusk commented on Sep 5, 2024 ContributorAuthor More actions Thanks. I have too many other projects to work on currently to take this on, but happy to comment and review when I can :) abj101 commented on Sep 7, 2024 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? ZeroIntensity commented on Sep 7, 2024 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. obfusk commented on Sep 7, 2024 ContributorAuthor More actions
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 :) Reacted by Jason R. Coombs
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|