Martin Pool wrote:
> 2009/9/10 Andrew Bennetts <email address hidden>:
[...]
> > I think the way to do this is to add another argument to
> > SmartServerRequest's constructor giving the relative path to use to
[...]
> 
> That seems ok, except a bit specific to ~.  We might want to add
> support for say ~spiv later on.

Well, in the API you can already pass an arbitrary backing_transport to the
server.  It's not particularly hard to implement arbitrary translations by
implementing your own transport.  This is how Launchpad's code hosting works for
instance; it has a server side plugin that adds an "lp-serve" command that uses
a custom backing transport that does translations like ~spiv/foo/bar ->
ab/cd/ef/01.

So features like ~user are already possible, for advanced users anyway.  So I'm
a bit hesitant to add new features to the core based on speculation that it
might be used by someone... perhaps there's a bug report/feature request I'm not
aware of?

A plugin that implements the "expand /~user/ in bzr serve" feature might make a
good example, and be useful to the hypothetical people that want this feature,
so perhaps we should write that?  I guess that deserves its own bug.

> It seems to me that we want to put a limitation on any transport
> constructed in that context, and a good way to do that would be by
> hooking in at the get_transport factory level.  We could pass in a
> transport factory to the request, but don't we already have some kind
> of jail on what kind of requests they can do?

Right.  We have two limitations here already:

  1. The backing_transport, which is a chrooted transport made by cmd_serve
     (although plugins have some control over this).
  2. The per-thread BzrDir.open jail, which prevents accidental opening of
     bzrdirs outside the backing transport, e.g. due to automatic attempts to
     open stacked branches or by hooks from incautious plugins.  (Again, plugins
     may relax the jail if they choose.) 

So, a naïve implementation of /~/ expansion that tries to access /home/spiv when
the backing_transport is chrooted at /srv/public_repo/ would probably trip over
both existing safeguards.

That said, I'd rather a solution that never tried to do something insecure in
the first place. :)

Also, a path that attempts to violate the existing restrictions results in a
different error to a simple “path not found”, IIRC.  I think that's reasonable,
but at the same time I think we would want /~/ to be a simple “path not found”
if it isn't accessible for a given server.  So that's also a reason to only
perform /~/ expansion if we expect it will work.

I did consider implementing this via wrapping the backing transport, rather than
via extending SmartServerRequest.  So then you'd (typically, ignoring wacky
plugins or wacky bzr+http glue) have several layers of transport decorator: 

   homedir-expander(chroot(actual))

Implementing a whole transport decorator seemed likely to be more work than just
extending the logic in SmartServerRequest.translate_client_path, though.

If we already had a path-filtering Transport base class with a single method to
override to vet all paths that got through it, then the effort would probably be
about the same.  (And if we do implement such at thing later, it would be pretty
easy to convert my existing proposal to use that instead while keep
backwards-compat of the API, I think.)

-Andrew.