This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: zipfile.Path does not work properly with zip archives where paths start with /
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: alanmcintyre, jaraco, serhiy.storchaka, sorrow, twouters
Priority: normal Keywords:

Created on 2020-06-19 12:13 by sorrow, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Messages (12)
msg371880 - (view) Author: sorrow (sorrow) Date: 2020-06-19 12:13
I encountered errors when I had to work with ZPI file where path start with "/"
msg371881 - (view) Author: sorrow (sorrow) Date: 2020-06-19 12:14
>>> import zipfile
>>> import io
>>> data = io.BytesIO()
>>> zf = zipfile.ZipFile(data, 'w')
>>> zf.writestr('/a.txt', 'content of a')
>>> zf.filename = 'abcde.zip'
>>> root = zipfile.Path(zf)
>>> list(root.iterdir())
[]
>>> root.exists()
False
msg371961 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2020-06-20 20:28
Thanks sorrow for filing a report.

I primarily developed this functionality. As I did, I found the 'zip' format to be under-specified, so I used real-world examples as models to infer a spec.

It seems you may have discovered a use-case that violates that expectation, a case where `/a.txt` is identical to `a.txt`.

My instinct is that `zipfile.Path` should support 99% of real-world use-cases and that other use-cases may not be supported or may require additional consideration (wrappers, subclasses) to support.

Can you tell me more about your use-case and why zipp.Path/zipfile.Path should support it? Is this behavior a result of a real-world example (please share details about the origin) or contrived?
msg371982 - (view) Author: sorrow (sorrow) Date: 2020-06-21 05:12
>It seems you may have discovered a use-case that violates that expectation, a case where `/a.txt` is identical to `a.txt`.

The thing is: it's not.

>Can you tell me more about your use-case and why zipp.Path/zipfile.Path should support it?

I received a .zip file and zipfile.Path in my code didn't work with it. I did some testing and discovered that id does not work properly with these archives. It cannot list the contents of such archive.

>Is this behavior a result of a real-world example

Yes, it is.

>(please share details about the origin)

I can't. First, the origin of this archive is not totally clear to me (and I do not want to investigate). And second, I'm under NDA.

I provided minimal example where archive created with zipfile.ZipFile itself reproduces this behaviour. Just prerpend all paths with / an it does not work.
msg371998 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2020-06-21 14:23
I created [jaraco/zipp#56](https://github.com/jaraco/zipp/issues/56) to track the issue in the backport.
msg372000 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2020-06-21 15:37
>>It seems you may have discovered a use-case that violates that expectation, a case where `/a.txt` is identical to `a.txt`.

> The thing is: it's not.

I think maybe you misunderstood. I mean that the zipfile you have seems to be treating `/a.txt` as a file `a.txt` at the root of the zipfile, identical to another zipfile that has an item named `a.txt`.

I'm not saying that zipfile.Path handles that situation; your example clearly contradicts that notion.

> I provided minimal example where archive created with zipfile.ZipFile itself reproduces this behaviour. Just prerpend all paths with / an it does not work.

Thank you. I'm grateful for the minimal example. What I'm trying to assess here is the impact - how common is this use-case and should it be supported. One option here might be to document the library as not supporting files whose names begin with a leading slash.

Digging into [the spec](https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT), Section 4.4.17.1 explicitly states:

> The path stored MUST NOT contain a drive or device letter, or a leading slash.

It appears the file your client has sent and the minimal example you've generated represents an invalid zip file.

In [this branch](https://github.com/jaraco/zipp/tree/bugfix/bpo-41035), I started exploring what it would take to support this format. Unfortunately, just patching the namelist was not enough. Supporting this change interacts with behaviors across a number of methods, so would add substantial complexity to the implementation. It becomes inelegant to manage the position in the file (`.at` property) when there's ambiguity about the underlying format. It opens up lots of questions, like:

- should `at` include the leading slash?
- should the class support zip files with mixed leading and non-leading slashes?
- at what point does `Path` become aware of the format used?
- are there emergent performance concerns?

In other words, the design relies heavily on the assumption that there's one way to store a file and two ways to store a directory (explicitly and implicitly).

Based on these findings, I'm disinclined to support the format in the canonical Path implementation.

What I recommend is that you develop a subclass of zipfile.Path that supports the abnormal format, use that for your work, and publish it (perhaps here, perhaps as a library) for others with the same problem to use. If enough people report it having usefulness, then I'd definitely consider incorporating it into the library, either as a separate implementation or perhaps integrating it (especially if that can be done without substantially complicating the canonical implementation).

Alternately, ask if the client can generate valid zip files. I'm eager to hear your thoughts in light of my work. Can we close this as invalid?
msg372003 - (view) Author: sorrow (sorrow) Date: 2020-06-21 16:41
>how common is this use-case

I don't know about that. I just know that I have to make my program work with these files. Asking the clients to stop using this (presumably incorrect) format (or the program that makes it) is not an option.

>It appears the file your client has sent and the minimal example you've generated represents an invalid zip file.

Well, I didn't know that. But it doesn't change the matter for me.

>What I recommend is that you develop a subclass of zipfile.Path that supports the abnormal format, use that for your work, and publish it (perhaps here, perhaps as a library) for others with the same problem to use.

I guess I'll have to go this way.

>It opens up lots of questions, like:

I'll share my considerations about how I see it should be implemented.

>- should `at` include the leading slash?

I think it doesn't matter, because `at` property is not part of `Path` public AP. Ideally, the files with leading slashes and without them should be treated exactly the same (as if they didn't have the leading slash). So, the only thing I'm concerned with is `at` argument of `Path.__init__`. It shouldn't require leading slash in any case.

>- should the class support zip files with mixed leading and non-leading slashes?

No, this is definitely an error and shouldn't work.

>- at what point does `Path` become aware of the format used?

When the class is created? 

>- are there emergent performance concerns?

For me - no, there aren't. I don't know how this kind of questions is resolved in python community though.

>Can we close this as invalid?

I guess you can.
msg372004 - (view) Author: sorrow (sorrow) Date: 2020-06-21 16:43
>`Path` public AP.

API of course
msg372005 - (view) Author: sorrow (sorrow) Date: 2020-06-21 16:45
>When the class is created? 

I mean the class instance
msg372013 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2020-06-21 19:25
Yes, I generally agree with your assessment. Let me know if you have any questions about the implementation as you're exploring a solution.
msg372107 - (view) Author: sorrow (sorrow) Date: 2020-06-22 18:39
Here's what I came up with:

```python
class ZipPath(zipfile.Path):
def __init__(self, root, at=""):
    super().__init__(root, at)
    if not at.startswith("/") and self.root.namelist()[0].startswith("/"):
        self.at = f"/{at}"

def __repr__(self):
    return (
        f"{self.__class__.__name__}({self.root.filename!r}, "
        f"{self.at.lstrip('/')!r})"
    )

def __str__(self):
    return posixpath.join(self.root.filename, self.at.lstrip("/"))

def _is_child(self, path):
    return posixpath.dirname(path.at.strip("/")) == self.at.strip("/")

def _next(self, at):
    return self.__class__(self.root, at)
```

Pretty simple. The main things are going on in `__init__` and `_is_child` methods. These changes are enough for `iteritems()` to work. I decided to include the leading slash in the `at`, but strip it for the outside world (__str__ and __repr__). Also, I had to override the `_next` method because it makes `Path` objects no matter what class it's called from.
msg372108 - (view) Author: sorrow (sorrow) Date: 2020-06-22 18:47
>`iteritems()`

I meant `iterdir()` of course.
History
Date User Action Args
2022-04-11 14:59:32adminsetgithub: 85207
2020-06-22 18:47:08sorrowsetmessages: + msg372108
2020-06-22 18:39:36sorrowsetmessages: + msg372107
2020-06-21 19:26:04jaracosetstatus: open -> closed
resolution: not a bug
stage: resolved
2020-06-21 19:25:40jaracosetmessages: + msg372013
2020-06-21 16:45:48sorrowsetmessages: + msg372005
2020-06-21 16:43:54sorrowsetmessages: + msg372004
2020-06-21 16:41:30sorrowsetmessages: + msg372003
2020-06-21 15:37:03jaracosetmessages: + msg372000
2020-06-21 14:23:17jaracosetmessages: + msg371998
2020-06-21 05:12:41sorrowsetmessages: + msg371982
2020-06-20 20:28:16jaracosetmessages: + msg371961
2020-06-20 17:59:45brett.cannonsetnosy: + twouters, alanmcintyre, serhiy.storchaka
2020-06-20 17:59:31brett.cannonsetnosy: + jaraco
2020-06-19 12:14:31sorrowsetmessages: + msg371881
2020-06-19 12:13:56sorrowcreate