[MERGE][1.0][Bug 124089] Add root_client_path parameter to SmartWSGIApp, and use it to translate paths received from clients to local paths.

Martin gzlist at googlemail.com
Wed Dec 12 10:33:00 GMT 2007


Okay, I updated my bzr to trunk and merged this branch, so I can
review this for smart-smuggled-over-http, but what I say may be wrong
for smart-elsewhere.

On 07/12/2007, John Arbash Meinel <john at arbash-meinel.com> wrote:
> John Arbash Meinel has voted tweak.
> Status is now: Conditionally approved
> Comment:
> +
> +    def translate_client_path(self, client_path):
> +        """Translate a path received from a network client into a local
> +        relpath.
> +
> +        All paths received from the client *must* be translated.
> +
> +        :returns: a relpath that may be used with
> self._backing_transport
> +        """
> +        if not client_path.startswith('/'):
> +            client_path = '/' + client_path
> +        if client_path.startswith(self._root_client_path):
> +            path = client_path[len(self._root_client_path):]
> +            relpath = urlutils.joinpath('/', path)
> +            assert relpath.startswith('/')
> +            return '.' + relpath
> +        else:
> +            raise errors.PathNotChild(client_path,
> self._root_client_path)
> +
>
> ^- Does this work for Windows paths? The:
> If not path.startswith('/'):
>    path = '/' + path
>
> seems like it would break for "C:/..."

Using '/' is fine for the http case, because client_path is given in
context of server URL layout, for which path.sep is always forward
slash. For the general case, (not os.path.isabs(client_path)) would
do.

However, I think the function is wrong. If you've *not* got an abs
path, you can't just stick a slash on the front then carry on
regardless. Also, making paths that were absolute into relative does
not work for the bzr+http case.

For example, requests are issued in the both these manners:
    urlopen("http://localhost/bazaar/bzr.dev/.bzr/smart",
        "BzrDir.open\x01/bazaar/bzr.dev/")
    urlopen("http://localhost/bazaar/bzr.dev/.bzr/smart",
        "get\x01.bzr/branch-format")
RelpathSetter takes the given server URL and pulls "/bazaar/" off the
front, and ".bzr/smart" off the back, leaving "bzr.dev/".
For the relative path, the right thing to do is just path.join(chroot,
"bzr.dev/", ".bzr/branch-format") - instead, the current code does
("/" + ".bzr/branch-format").startswith("/bazaar/") - and throws an
exception.
For the abs path, the current code does the equiv. of
path.join(chroot, "bzr.dev/", "./" +
"/bazaar/bzr.dev/".split("/bazaar/",0)[1]) - which ends up duplicating
the "bzr.dev" step.

The right thing to do, from the smart-over-http perspective, is leave
relative paths well alone, and strip root_client_path off the front of
abs paths, but leave the path absolute.

With the attached smart_changes.patch applied, branch over bzr+http
now works for me (though see below) - however selftest still isn't
passing here, or even completing, so I can't tell if it breaks other
uses of the smart server.

> Same thing for all of the places where you are doing:
> -    def __init__(self, sock, backing_transport):
> +    def __init__(self, sock, backing_transport, root_client_path='/'):
>
>
> Actually, now that I think about it, I'm a little concerned about using
> a default path of '/'. Since if someone forgets to pass it from a
> function to another one, then we end up exposing everything from '/'
> when it was meant to be restricted elsewhere.

It's fine for chroot-ed case.

> So what about defaulting to None, and using a DeprecationWarning if it
> is not supplied.

But this might be a good idea if over smart-server uses don't chroot.

> ...
> Also, the same issue about forcing paths to start with '/' causing
> problems on Windows. I do understand forcing them to end with a '/'.

Is this to make it safe to join a '.bzr/branch-format' etc on the end
later? The smart server generally confuses me.

> ...
> Also, as you mentioned, the documentation http_smart_server.txt needs to
> be updated.

I could cover this... where's the current copy under
<http://doc.bazaar-vcs.org/bzr.dev/> though?

---

While checking this, I ran into two different additional issues, that
are probably worth mentioning.

Firstly, I couldn't actually branch bzr.dev through
bzr+http://localhost/ because it choaked with a (10055, 'No buffer
space available') socket error on the 70MB pack file. I see that
Alexander has also run into this recently
<https://lists.ubuntu.com/archives/bazaar/2007q4/035539.html> - is
there a launchpad bug number? I attach server access log, client
.bzr.log and hacked server log (with
<https://bugs.launchpad.net/bzr/+bug/119330/comments/2> that patch)
for the failed bzr.dev branch and succeeded trivial branch.

Secondly, I anticipated there might be an issue with shared
repositories, as bzr used to climb up the dir structure on the server
internally (past the chroot, even), and this patch looked like it
might stop that. However, it seems now that even if the base repo dir
is exposed, bzr+http will fail on repo-less branch - does not even
attempt to get the parent dir from the outside.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smart_changes.patch
Type: application/octet-stream
Size: 1403 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071212/49722dd3/attachment-0001.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr_branch_stst_success.7z
Type: application/7z
Size: 3754 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071212/49722dd3/attachment-0002.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr_branch_bzr.dev_fail.7z
Type: application/7z
Size: 235410 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071212/49722dd3/attachment-0003.bin 


More information about the bazaar mailing list