HEAD requests in presents of browser:defaultView gives AttributeError in publisher

Bug #180155 reported by Martijn Faassen
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Silva
Fix Released
Medium
Unassigned
Zope 2
Invalid
Low
Unassigned

Bug Description

Let's define a default view for a normal Zope folder, like with the following ZCML:

  <browser:page
    name="index.html"
    for="OFS.interfaces.IFolder"
    template="test.pt"
    permission="zope.Public"
    />

  <browser:defaultView
    for="OFS.interfaces.IFolder"
    name="index.html"
    />

When you now issue a HEAD request against a folder, like so:

curl -X HEAD --head http://localhost:8080/foo

(it's important to issue this request against something else than the top folder, as the top folder does have correct behavior)

there will be an error in the ZPublisher:

Traceback (innermost last):
  Module ZPublisher.Publish, line 110, in publish
  Module ZPublisher.BaseRequest, line 530, in traverse
AttributeError: has_key

in the following piece of code:

        if (no_acquire_flag and
            hasattr(parents[1], 'aq_base') and
            not hasattr(parents[1],'__bobo_traverse__')):
            if not (hasattr(parents[1].aq_base, entry_name) or
                    parents[1].aq_base.has_key(entry_name)):
                raise AttributeError, entry_name

There'll be an AttributeError because parents[1].aq_base.has_key has no method 'has_key'.
parents[1].aq_base in this context is Folder. entry name will be @@index.html.

Apparently HEAD requests don't trigger the right code-path in this tortuous pile of ZPublisher code anymore.
This can be traced back to the browserDefault() method of DefaultPublishTraverse in the same module.

    def browserDefault(self, request):
        if hasattr(self.context, '__browser_default__'):
            return self.context.__browser_default__(request)
        # Zope 3.2 still uses IDefaultView name when it
        # registeres default views, even though it's
        # deprecated. So we handle that here:
        default_name = queryDefaultViewName(self.context, request)
        if default_name is not None:
            # Adding '@@' here forces this to be a view.
            # A neater solution might be desireable.
            return self.context, ('@@' + default_name,)
        return self.context, ()

if default_name is not None, the failing behavior is triggered. The problem seems to go away if I change the method to this to bail out early if we're not dealing with GET or POST:

    def browserDefault(self, request):
        if hasattr(self.context, '__browser_default__'):
            return self.context.__browser_default__(request)
        method=req_method=request.get('REQUEST_METHOD', 'GET').upper()
        if method not in ('GET', 'POST'):
            return self.context, ()
        # Zope 3.2 still uses IDefaultView name when it
        # registeres default views, even though it's
        # deprecated. So we handle that here:
        default_name = queryDefaultViewName(self.context, request)
        if default_name is not None:
            # Adding '@@' here forces this to be a view.
            # A neater solution might be desireable.
            return self.context, ('@@' + default_name,)
        return self.context, ()

This may be a fix, but there may be a better fix...

Revision history for this message
Jasper Op de Coul (jasper-infrae) wrote :

Jasper and Martijn here. We've been reviewing this more in depth and have concluded
that the patched described above isn't the right solution. It still fails for HEAD requests
issued against a Five-view directly for instance, it just happens to work for default views.

We dug into this problem some more, and we think a better solution would be to add
a HEAD method to Zope 2's BrowserView base class (Products.Five.BrowserView).

This default HEAD method should then delegate the request to the content object, which
(in the Resource base class of SimpleItem) defines HEAD. People should also be allow
to override it in the view simply.

In order to define a working HEAD request we have to please Zope security, and we do this in the following hackish way, delegating it to the content-object security (approach thanks to Daniel Nouri):

from AccessControl.ZopeSecurityPolicy import getRoles

class MyRolesProxy:
    def __init__(self, method):
        self.method = method

    def rolesForPermissionOn(self, value):
        context = value.im_self.aq_inner.aq_parent
        return getRoles(context, self.method,
                        getattr(context, self.method),
                        None)

then we define a HEAD method on the view which delegates everything back to
the context:

    def HEAD(self, REQUEST, RESPONSE=None):
        """Proxy HTTP HEAD requests.
        Docstring needs to exist to please Zope security.
        """
        # A bit hackish, but it works.
        context = self.context_or_default()
        func = context.HEAD
        getSecurityManager().validate(None, context, 'HEAD', func) if RESPONSE is None:
            return func(REQUEST)
        return func(REQUEST, RESPONSE)

Finally we make sure that this HEAD method also delegates security to the context:

    HEAD.__roles__ = MyRolesProxy('HEAD')

We think we should be doing this for other HTTP methods too (HEAD, PUT, OPTIONS, etc, basically anything non GET/POST).

An other approach to accomplish the same result would be to modify the ZPublisher so that this delegation happens there if the relevant method cannot be found. To find how to adjust the ZPublisher to this effect is a challenge, however. :)

Revision history for this message
Tres Seaver (tseaver) wrote : Re: [Bug 180155] Re: HEAD requests in presents of browser:defaultView gives AttributeError in publisher

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jasper Op de Coul wrote:
> Jasper and Martijn here. We've been reviewing this more in depth and have concluded
> that the patched described above isn't the right solution. It still fails for HEAD requests
> issued against a Five-view directly for instance, it just happens to work for default views.
>
> We dug into this problem some more, and we think a better solution would be to add
> a HEAD method to Zope 2's BrowserView base class (Products.Five.BrowserView).
>
> This default HEAD method should then delegate the request to the content object, which
> (in the Resource base class of SimpleItem) defines HEAD. People should also be allow
> to override it in the view simply.

<snip what="implementation details"/>

> We think we should be doing this for other HTTP methods too (HEAD, PUT,
> OPTIONS, etc, basically anything non GET/POST).
>
> An other approach to accomplish the same result would be to modify the
> ZPublisher so that this delegation happens there if the relevant method
> cannot be found. To find how to adjust the ZPublisher to this effect is
> a challenge, however. :)

+1 to fixing this in the view. We've "blown up" the Zope2 publisher in
repoze, largely because touching it inside Zope2 is no fun. ;)

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHhlVE+gerLs4ltQ4RApZVAJ4/64STUZjohH7bGCUDNcEsKkbARACgsvTv
9D/7sxgCvA+XeddOkbMXDKA=
=SFu8
-----END PGP SIGNATURE-----

Revision history for this message
Richard Waid (richard-iopen) wrote :

What's the status of this issue?

It doesn't *seem* like it has been implemented (I can't find it implemented anywhere, including the Zope2 trunk).

Just banging up against it at the moment :)

Many thanks,

Richard Waid
Technical Lead
http://onlinegroups.net

Revision history for this message
Martin Aspeli (optilude) wrote :

It's not implemented (at least not in Zope 2.12) and this is still a problem.

If you register a defaultView on a folder, for example, it's impossible to access the folder via WebDAV because of this error. It tries to traverse to the default view when doing the initial PROPFIND request, causing an error.

I actually think the correct solution should be to not traverse to the defaultView for WebDAV requests. It's not quite clear to me whether it should happen on HEAD requests always, or only if the "maybe WebDAV client" flag is set.

Changed in zope2:
importance: Undecided → Low
status: New → Confirmed
Revision history for this message
Wichert Akkerman (wichert) wrote :

This is becomingproblematic with IE9, which seems to issue HEAD requests before GET requests in some situations (possibly when having network tracing enabled).

Revision history for this message
Wichert Akkerman (wichert) wrote :

I am wondering if a much simpler solution to check for acquisition would be something like this:

  if no_acquire_flag and IAcquirer.providedBy(parents[1]):
      if aq_base(getattr(parents[1], entry_name, None)) is aq_base(object):
          raise AttributeError(entry_name)

the idea here being that we need to test if fetching entry_name from the grandparent results in the exact same object as we already have then acquisition must be in play. Are there any situations that would be missed by this simpler version?

Revision history for this message
Malthe Borch (mborch) wrote :

I have fixed this in trunk and 2.13 branch, simply by using the in-operator instead of has_key, which isn't always implemented.

That said, the code should probably be revisited altogether.

Revision history for this message
yuppie (yuppie3) wrote :
Revision history for this message
Sylvain Viollon (thefunny) wrote :

All views in Silva 3.0 are based on Grok views, and have a custom implementation for HEAD, implemented with publishTraverse/browserDefault that doesn't exhibit this bug. This is tested and released in 3.0b1.

Changed in silva:
importance: Undecided → Medium
milestone: none → 3.0
status: New → Fix Released
Revision history for this message
Colin Watson (cjwatson) wrote :

The zope2 project on Launchpad has been archived at the request of the Zope developers (see https://answers.launchpad.net/launchpad/+question/683589 and https://answers.launchpad.net/launchpad/+question/685285). If this bug is still relevant, please refile it at https://github.com/zopefoundation/zope2.

Changed in zope2:
status: Confirmed → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.