[Zope3-dev] Big Problem with FTP in Zope 3.3.0, seems to be the result of using `v = self.get(name, self); if v is self...` in a land where Proxy objects can kill identity comparison

[Jeff Shell]

Hello. I'm filing this in the Collector but wanted to post it as an
email first before I had to wrangle the formatting to work in the
collector (although it might work straight over).

We've encountered a problem with FTP in Zope 3.3. The problem is that
you can CWD into non existant paths. Tools like Fetch, Dreamweaver,
whatever, will test to see if it has to make a directory by trying to
change-dir to that directory, and will make a folder if that fails.
But if the change-dir command works for non-existant containers, then
everything blows up once it starts trying to upload files into said
non-existant containers.

I have a basic (but real) example of this. We have the following basic
folder structure::

    [root]
      - customer
        - winter
          - info
          - services

Following a basic FTP session::

    ftp> cd /
    250 Requested File Action Completed OK
    ftp> cd customer
    250 Requested File Action Completed OK
    ftp> ls
    drwxrwx---   1 na        na                      0 Oct 27 23:23 winter
    226 Transfer Complete.
    ftp> cd winter
    250 Requested File Action Completed OK
    ftp> ls
    drwxr-x---   1 na        na                      0 Oct 19 18:40 info
    drwxr-x---   1 na        na                      0 Oct 27 17:40 services
    226 Transfer Complete.

Everything is OK so far. But lets go back up a level (to 'customer')
and try to get to a non-existant folder named 'bogus'::

    ftp> cd ..
    250 Requested File Action Completed OK
    ftp> pwd
    257 "/customer"
    ftp> ls
    drwxrwx---   1 na        na                      0 Oct 27 23:23 winter
    226 Transfer Complete.
    ftp> cd bogus
    250 Requested File Action Completed OK
    ftp> ls
    drwxr-x---   1 na        na                      0 Jan 01  1970 bogus
    226 Transfer Complete.

Crap!!!!!!!!!!!!

So after digging through the FTP Server code from
twisted.protocols.ftp to zope.app.twisted.ftp.* to
zope.app.publication.ftp to zope.app.ftp.FTPView, I came to this:

In file zope/app/ftp/__init__.py, I put a ``import pdb;
pdb.set_trace()`` call in line 68 (at start of ``def ls(...)``
method). These are some of the results while at that breakpoint. I
found something quite interesting::

    (Pdb) self._dir
    <br.cms.folder.ContentContainerReadDirectory object at 0x2d718f0>

    # Getting a real element of out self._dir
    (Pdb) self._dir['winter']
    <br.cms.site.ContentSection object at 0x3a4e6f0>

    # Getting a bogus element out of self._dir
    (Pdb) self._dir['bogus']
    <br.cms.folder.ContentContainerReadDirectory object at 0x2d718f0>

    # Hmm, those look downright identical
    (Pdb) self._dir['bogus'] is self._dir
    False

    # Oh crap, there's probably proxies.
    (Pdb) from zope import proxy
    (Pdb) proxy.sameProxiedObjects(self._dir, self._dir['bogus'])
    True

Where did this come from?
The FTPView's publishTraverse method is implemented as::

    def publishTraverse(self, request, name):
        return self._dir[name]

The 'CWD' command is implemented in Zope 3 as traversing to the full
path and checking that it can call 'ls' on it::

    ftp> cd customer/bogus

    (Pdb) print self.request
    command:  ls
    filter:   None
    path:     /customer/bogus

The expected behavior is that /customer/bogus should fail during
publishTraverse because 'bogus' is not in 'customer'. Now it should be
noted that self._dir in this instance is a ReadDirectory subclass.
However, it does not override get or getitem (our subclass overrides
the keys method only as a cheap filtering mechanism). So the problem
is happening on ``return self._dir[name]``, which leads to
zope.app.folder.filerepresentation.ReadDirectory::

    def __getitem__(self, key):
        v = self.get(key, self)
        if v is self:
            raise KeyError(key)
        return v

I've been noticing that ``get(key, self)... if v is self`` pattern is
being used more and more in Zope. I'm not sure if this is a good
practice, with all of the proxying that can occur. Proxying seems to
be done at a lower level than I can comprehend. I just know that it
kills identity comparison (even though items look identical when
looking at their default repr() strings).

Notice this similar situation in plain old debugzope, free from
security concerns and whatever else pops up::

    bin> ./debugzope
    >>> from zope.filerepresentation.interfaces import IReadDirectory
    >>> _dir = IReadDirectory(root['customer'])
    >>> _dir
    <br.cms.folder.ContentContainerReadDirectory object at 0x17a8fd0>

    # Successful getitem
    >>> _dir['winter']
    <br.cms.site.ContentSection object at 0x2e1edf0>

    # Failure getitem - the one that succeeds in FTP Land
    >>> _dir['bogus']
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
      File "/opt/local/zope33/lib/python/zope/app/folder/filerepresentation.py",
line 57, in __getitem__
        raise KeyError(key)
    KeyError: 'bogus'

This time we see the expected behavior. Because of this, I spent a lot
more time tracing how FTP works in Zope that I probably needed to
because my first try - "Let's see if IReadDirectory is doing the right
thing by firing up debugzope.." - worked as expected.

Moral of the story: don't use ``query/get (..., default=self)``, ``if
value is self`` paradigm? Be careful of identity comparisons? Is there
a moral?

Is there any reason why the ``_marker = object() .... get(...,
default=_marker)`` paradigm isn't used?

After encountering this, I am rather uneasy about the number of places
in which I see this "self as default marker check" paradigm. My
biggest unease comes from not knowing proxies and when things are
proxied and when they're not and when I have to be careful about them
and when I don't.

Anyways, for our particular FTP problem, I can patch around it since
we have a custom Read Directory adapter anyway.

--
Jeff Shell