Merge lp:~jelmer/bzr/safe-open into lp:bzr

[Jelmer Vernooij]

Import the safe_open code from launchpad.

This code allows the user to open branches while checking all URLs that are
opened with a policy object. This code is generic, and could be useful for
other users than Launchpad too.

[Jelmer Vernooij (jelmer)]

Since this is a fairly isolated piece of code I've put the errors in bzrlib.safe_open. bzrlib.errors is certainly also an option, but that module is already fairly large, and it doesn't seem likely other code will use them.

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2011-12-21 20:43:29 +0000
+++ bzrlib/branch.py	2011-12-22 01:48:24 +0000
@@ -180,8 +180,8 @@
         For instance, if the branch is at URL/.bzr/branch,
         Branch.open(URL) -> a Branch instance.
         """
-        control = controldir.ControlDir.open(base, _unsupported,
-                                     possible_transports=possible_transports)
+        control = controldir.ControlDir.open(base,
+            possible_transports=possible_transports, _unsupported=_unsupported)
         return control.open_branch(unsupported=_unsupported,
             possible_transports=possible_transports)
 
@@ -3008,7 +3008,7 @@
                                                 config=conf)
         if stacked_url is None:
             raise errors.NotStacked(self)
-        return stacked_url
+        return stacked_url.encode('utf-8')
 
     @needs_read_lock
     def get_rev_id(self, revno, history=None):

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/controldir.py	2011-12-22 01:48:24 +0000
@@ -664,17 +664,19 @@
         return klass.open(base, _unsupported=True)
 
     @classmethod
-    def open(klass, base, _unsupported=False, possible_transports=None):
+    def open(klass, base, possible_transports=None, probers=None,
+             _unsupported=False):
         """Open an existing controldir, rooted at 'base' (url).
 
         :param _unsupported: a private parameter to the ControlDir class.
         """
         t = _mod_transport.get_transport(base, possible_transports)
-        return klass.open_from_transport(t, _unsupported=_unsupported)
+        return klass.open_from_transport(t, probers=probers,
+                _unsupported=_unsupported)
 
     @classmethod
     def open_from_transport(klass, transport, _unsupported=False,
-                            _server_formats=True):
+                            probers=None):
         """Open a controldir within a particular directory.
 
         :param transport: Transport containing the controldir.
@@ -686,8 +688,8 @@
         # the redirections.
         base = transport.base
         def find_format(transport):
-            return transport, ControlDirFormat.find_format(
-                transport, _server_formats=_server_formats)
+            return transport, ControlDirFormat.find_format(transport,
+                probers=probers)
 
         def redirected(transport, e, redirection_notice):
             redirected_transport = transport._redirected_to(e.source, e.target)
@@ -1110,22 +1112,24 @@
         return self.get_format_description().rstrip()
 
     @classmethod
+    def all_probers(klass):
+        return klass._probers + klass._server_probers
+
+    @classmethod
     def known_formats(klass):
         """Return all the known formats.
         """
         result = set()
-        for prober_kls in klass._probers + klass._server_probers:
+        for prober_kls in klass.all_probers():
             result.update(prober_kls.known_formats())
         return result
 
     @classmethod
-    def find_format(klass, transport, _server_formats=True):
+    def find_format(klass, transport, probers=None):
         """Return the format present at transport."""
-        if _server_formats:
-            _probers = klass._server_probers + klass._probers
-        else:
-            _probers = klass._probers
-        for prober_kls in _probers:
+        if probers is None:
+            probers = klass.all_probers()
+        for prober_kls in probers:
             prober = prober_kls()
             try:
                 return prober.probe_transport(transport)

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2011-12-18 12:46:49 +0000
+++ bzrlib/remote.py	2011-12-22 01:48:24 +0000
@@ -480,7 +480,7 @@
                 warning('VFS BzrDir access triggered\n%s',
                     ''.join(traceback.format_stack()))
             self._real_bzrdir = _mod_bzrdir.BzrDir.open_from_transport(
-                self.root_transport, _server_formats=False)
+                self.root_transport, probers=[_mod_bzrdir.BzrProber])
             self._format._network_name = \
                 self._real_bzrdir._format.network_name()
 

=== added file 'bzrlib/safe_open.py'
--- bzrlib/safe_open.py	1970-01-01 00:00:00 +0000
+++ bzrlib/safe_open.py	2011-12-22 01:48:24 +0000
@@ -0,0 +1,307 @@
+# Copyright (C) 2011 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Safe branch opening."""
+
+import threading
+
+from bzrlib import (
+    errors,
+    urlutils,
+    )
+from bzrlib.branch import Branch
+from bzrlib.controldir import (
+    ControlDir,
+    )
+
+
+class BadUrl(errors.BzrError):
+
+    _fmt = "Tried to access a branch from bad URL %(url)s."
+
+
+class BranchReferenceForbidden(errors.BzrError):
+
+    _fmt = ("Trying to mirror a branch reference and the branch type "
+            "does not allow references.")
+
+
+class BranchLoopError(errors.BzrError):
+    """Encountered a branch cycle.
+
+    A URL may point to a branch reference or it may point to a stacked branch.
+    In either case, it's possible for there to be a cycle in these references,
+    and this exception is raised when we detect such a cycle.
+    """
+
+    _fmt = "Encountered a branch cycle"""
+
+
+class BranchOpenPolicy(object):
+    """Policy on how to open branches.
+
+    In particular, a policy determines which branches are safe to open by
+    checking their URLs and deciding whether or not to follow branch
+    references.
+    """
+
+    def should_follow_references(self):
+        """Whether we traverse references when mirroring.
+
+        Subclasses must override this method.
+
+        If we encounter a branch reference and this returns false, an error is
+        raised.
+
+        :returns: A boolean to indicate whether to follow a branch reference.
+        """
+        raise NotImplementedError(self.should_follow_references)
+
+    def transform_fallback_location(self, branch, url):
+        """Validate, maybe modify, 'url' to be used as a stacked-on location.
+
+        :param branch:  The branch that is being opened.
+        :param url: The URL that the branch provides for its stacked-on
+            location.
+        :return: (new_url, check) where 'new_url' is the URL of the branch to
+            actually open and 'check' is true if 'new_url' needs to be
+            validated by check_and_follow_branch_reference.
+        """
+        raise NotImplementedError(self.transform_fallback_location)
+
+    def check_one_url(self, url):
+        """Check the safety of the source URL.
+
+        Subclasses must override this method.
+
+        :param url: The source URL to check.
+        :raise BadUrl: subclasses are expected to raise this or a subclass
+            when it finds a URL it deems to be unsafe.
+        """
+        raise NotImplementedError(self.check_one_url)
+
+
+class BlacklistPolicy(BranchOpenPolicy):
+    """Branch policy that forbids certain URLs."""
+
+    def __init__(self, should_follow_references, unsafe_urls=None):
+        if unsafe_urls is None:
+            unsafe_urls = set()
+        self._unsafe_urls = unsafe_urls
+        self._should_follow_references = should_follow_references
+
+    def should_follow_references(self):
+        return self._should_follow_references
+
+    def check_one_url(self, url):
+        if url in self._unsafe_urls:
+            raise BadUrl(url)
+
+    def transform_fallback_location(self, branch, url):
+        """See `BranchOpenPolicy.transform_fallback_location`.
+
+        This class is not used for testing our smarter stacking features so we
+        just do the simplest thing: return the URL that would be used anyway
+        and don't check it.
+        """
+        return urlutils.join(branch.base, url), False
+
+
+class AcceptAnythingPolicy(BlacklistPolicy):
+    """Accept anything, to make testing easier."""
+
+    def __init__(self):
+        super(AcceptAnythingPolicy, self).__init__(True, set())
+
+
+class WhitelistPolicy(BranchOpenPolicy):
+    """Branch policy that only allows certain URLs."""
+
+    def __init__(self, should_follow_references, allowed_urls=None,
+                 check=False):
+        if allowed_urls is None:
+            allowed_urls = []
+        self.allowed_urls = set(url.rstrip('/') for url in allowed_urls)
+        self.check = check
+
+    def should_follow_references(self):
+        return self._should_follow_references
+
+    def check_one_url(self, url):
+        if url.rstrip('/') not in self.allowed_urls:
+            raise BadUrl(url)
+
+    def transform_fallback_location(self, branch, url):
+        """See `BranchOpenPolicy.transform_fallback_location`.
+
+        Here we return the URL that would be used anyway and optionally check
+        it.
+        """
+        return urlutils.join(branch.base, url), self.check
+
+
+class SingleSchemePolicy(BranchOpenPolicy):
+    """Branch open policy that rejects URLs not on the given scheme."""
+
+    def __init__(self, allowed_scheme):
+        self.allowed_scheme = allowed_scheme
+
+    def should_follow_references(self):
+        return True
+
+    def transform_fallback_location(self, branch, url):
+        return urlutils.join(branch.base, url), True
+
+    def check_one_url(self, url):
+        """Check that `url` is safe to open."""
+        if urlutils.URL.from_string(str(url)).scheme != self.allowed_scheme:
+            raise BadUrl(url)
+
+
+class SafeBranchOpener(object):
+    """Safe branch opener.
+
+    All locations that are opened (stacked-on branches, references) are
+    checked against a policy object.
+
+    The policy object is expected to have the following methods:
+    * check_one_url 
+    * should_follow_references
+    * transform_fallback_location
+    """
+
+    _threading_data = threading.local()
+
+    def __init__(self, policy, probers=None):
+        """Create a new SafeBranchOpener.
+
+        :param policy: The opener policy to use.
+        :param probers: Optional list of probers to allow.
+            Defaults to local and remote bzr probers.
+        """
+        self.policy = policy
+        self._seen_urls = set()
+        self.probers = probers
+
+    @classmethod
+    def install_hook(cls):
+        """Install the ``transform_fallback_location`` hook.
+
+        This is done at module import time, but transform_fallback_locationHook
+        doesn't do anything unless the `_active_openers` threading.Local
+        object has a 'opener' attribute in this thread.
+
+        This is in a module-level function rather than performed at module
+        level so that it can be called in setUp for testing `SafeBranchOpener`
+        as bzrlib.tests.TestCase.setUp clears hooks.
+        """
+        Branch.hooks.install_named_hook(
+            'transform_fallback_location',
+            cls.transform_fallback_locationHook,
+            'SafeBranchOpener.transform_fallback_locationHook')
+
+    def check_and_follow_branch_reference(self, url):
+        """Check URL (and possibly the referenced URL) for safety.
+
+        This method checks that `url` passes the policy's `check_one_url`
+        method, and if `url` refers to a branch reference, it checks whether
+        references are allowed and whether the reference's URL passes muster
+        also -- recursively, until a real branch is found.
+
+        :param url: URL to check
+        :raise BranchLoopError: If the branch references form a loop.
+        :raise BranchReferenceForbidden: If this opener forbids branch
+            references.
+        """
+        while True:
+            if url in self._seen_urls:
+                raise BranchLoopError()
+            self._seen_urls.add(url)
+            self.policy.check_one_url(url)
+            next_url = self.follow_reference(url)
+            if next_url is None:
+                return url
+            url = next_url
+            if not self.policy.should_follow_references():
+                raise BranchReferenceForbidden(url)
+
+    @classmethod
+    def transform_fallback_locationHook(cls, branch, url):
+        """Installed as the 'transform_fallback_location' Branch hook.
+
+        This method calls `transform_fallback_location` on the policy object and
+        either returns the url it provides or passes it back to
+        check_and_follow_branch_reference.
+        """
+        try:
+            opener = getattr(cls._threading_data, "opener")
+        except AttributeError:
+            return url
+        new_url, check = opener.policy.transform_fallback_location(branch, url)
+        if check:
+            return opener.check_and_follow_branch_reference(new_url)
+        else:
+            return new_url
+
+    def run_with_transform_fallback_location_hook_installed(
+            self, callable, *args, **kw):
+        assert (self.transform_fallback_locationHook in
+                Branch.hooks['transform_fallback_location'])
+        self._threading_data.opener = self
+        try:
+            return callable(*args, **kw)
+        finally:
+            del self._threading_data.opener
+            # We reset _seen_urls here to avoid multiple calls to open giving
+            # spurious loop exceptions.
+            self._seen_urls = set()
+
+    def follow_reference(self, url):
+        """Get the branch-reference value at the specified url.
+
+        This exists as a separate method only to be overriden in unit tests.
+        """
+        bzrdir = ControlDir.open(url, probers=self.probers)
+        return bzrdir.get_branch_reference()
+
+    def open(self, url):
+        """Open the Bazaar branch at url, first checking for safety.
+
+        What safety means is defined by a subclasses `follow_reference` and
+        `check_one_url` methods.
+        """
+        if type(url) != str:
+            raise TypeError
+
+        url = self.check_and_follow_branch_reference(url)
+
+        def open_branch(url):
+            dir = ControlDir.open(url, probers=self.probers)
+            return dir.open_branch()
+        return self.run_with_transform_fallback_location_hook_installed(
+            open_branch, url)
+
+
+def safe_open(allowed_scheme, url):
+    """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
+
+    :raises BadUrl: An attempt was made to open a URL that was not on
+        `allowed_scheme`.
+    """
+    return SafeBranchOpener(SingleSchemePolicy(allowed_scheme)).open(url)
+
+
+SafeBranchOpener.install_hook()

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-12-18 12:46:49 +0000
+++ bzrlib/tests/__init__.py	2011-12-22 01:48:24 +0000
@@ -4046,6 +4046,7 @@
         'bzrlib.tests.test_revisiontree',
         'bzrlib.tests.test_rio',
         'bzrlib.tests.test_rules',
+        'bzrlib.tests.test_safe_open',
         'bzrlib.tests.test_sampler',
         'bzrlib.tests.test_scenarios',
         'bzrlib.tests.test_script',

=== added file 'bzrlib/tests/test_safe_open.py'
--- bzrlib/tests/test_safe_open.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/test_safe_open.py	2011-12-22 01:48:24 +0000
@@ -0,0 +1,356 @@
+# Copyright (C) 2011 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Tests for the safe branch open code."""
+
+from bzrlib import urlutils
+from bzrlib.branch import (
+    Branch,
+    BranchReferenceFormat,
+    )
+from bzrlib.bzrdir import (
+    BzrDir,
+    BzrProber,
+    )
+from bzrlib.controldir import ControlDirFormat
+from bzrlib.errors import NotBranchError
+from bzrlib.safe_open import (
+    BadUrl,
+    BlacklistPolicy,
+    BranchLoopError,
+    BranchReferenceForbidden,
+    safe_open,
+    SafeBranchOpener,
+    WhitelistPolicy,
+    )
+from bzrlib.tests import (
+    TestCase,
+    TestCaseWithTransport,
+    )
+from bzrlib.transport import chroot
+
+
+class TestSafeBranchOpenerCheckAndFollowBranchReference(TestCase):
+    """Unit tests for `SafeBranchOpener.check_and_follow_branch_reference`."""
+
+    def setUp(self):
+        super(TestSafeBranchOpenerCheckAndFollowBranchReference, self).setUp()
+        SafeBranchOpener.install_hook()
+
+    class StubbedSafeBranchOpener(SafeBranchOpener):
+        """SafeBranchOpener that provides canned answers.
+
+        We implement the methods we need to to be able to control all the
+        inputs to the `follow_reference` method, which is what is
+        being tested in this class.
+        """
+
+        def __init__(self, references, policy):
+            parent_cls = TestSafeBranchOpenerCheckAndFollowBranchReference
+            super(parent_cls.StubbedSafeBranchOpener, self).__init__(policy)
+            self._reference_values = {}
+            for i in range(len(references) - 1):
+                self._reference_values[references[i]] = references[i + 1]
+            self.follow_reference_calls = []
+
+        def follow_reference(self, url):
+            self.follow_reference_calls.append(url)
+            return self._reference_values[url]
+
+    def make_branch_opener(self, should_follow_references, references,
+                         unsafe_urls=None):
+        policy = BlacklistPolicy(should_follow_references, unsafe_urls)
+        opener = self.StubbedSafeBranchOpener(references, policy)
+        return opener
+
+    def test_check_initial_url(self):
+        # check_and_follow_branch_reference rejects all URLs that are not allowed.
+        opener = self.make_branch_opener(None, [], set(['a']))
+        self.assertRaises(
+            BadUrl, opener.check_and_follow_branch_reference, 'a')
+
+    def test_not_reference(self):
+        # When branch references are forbidden, check_and_follow_branch_reference
+        # does not raise on non-references.
+        opener = self.make_branch_opener(False, ['a', None])
+        self.assertEquals(
+            'a', opener.check_and_follow_branch_reference('a'))
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def test_branch_reference_forbidden(self):
+        # check_and_follow_branch_reference raises BranchReferenceForbidden if
+        # branch references are forbidden and the source URL points to a
+        # branch reference.
+        opener = self.make_branch_opener(False, ['a', 'b'])
+        self.assertRaises(
+            BranchReferenceForbidden,
+            opener.check_and_follow_branch_reference, 'a')
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def test_allowed_reference(self):
+        # check_and_follow_branch_reference does not raise if following references
+        # is allowed and the source URL points to a branch reference to a
+        # permitted location.
+        opener = self.make_branch_opener(True, ['a', 'b', None])
+        self.assertEquals(
+            'b', opener.check_and_follow_branch_reference('a'))
+        self.assertEquals(['a', 'b'], opener.follow_reference_calls)
+
+    def test_check_referenced_urls(self):
+        # check_and_follow_branch_reference checks if the URL a reference points
+        # to is safe.
+        opener = self.make_branch_opener(
+            True, ['a', 'b', None], unsafe_urls=set('b'))
+        self.assertRaises(
+            BadUrl, opener.check_and_follow_branch_reference, 'a')
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def test_self_referencing_branch(self):
+        # check_and_follow_branch_reference raises BranchReferenceLoopError if
+        # following references is allowed and the source url points to a
+        # self-referencing branch reference.
+        opener = self.make_branch_opener(True, ['a', 'a'])
+        self.assertRaises(
+            BranchLoopError, opener.check_and_follow_branch_reference, 'a')
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def test_branch_reference_loop(self):
+        # check_and_follow_branch_reference raises BranchReferenceLoopError if
+        # following references is allowed and the source url points to a loop
+        # of branch references.
+        references = ['a', 'b', 'a']
+        opener = self.make_branch_opener(True, references)
+        self.assertRaises(
+            BranchLoopError, opener.check_and_follow_branch_reference, 'a')
+        self.assertEquals(['a', 'b'], opener.follow_reference_calls)
+
+
+class TrackingProber(BzrProber):
+    """Subclass of BzrProber which tracks URLs it has been asked to open."""
+
+    seen_urls = []
+
+    @classmethod
+    def probe_transport(klass, transport):
+        klass.seen_urls.append(transport.base)
+        return BzrProber.probe_transport(transport)
+
+
+class TestSafeBranchOpenerStacking(TestCaseWithTransport):
+
+    def setUp(self):
+        super(TestSafeBranchOpenerStacking, self).setUp()
+        SafeBranchOpener.install_hook()
+
+    def make_branch_opener(self, allowed_urls, probers=None):
+        policy = WhitelistPolicy(True, allowed_urls, True)
+        return SafeBranchOpener(policy, probers)
+
+    def test_probers(self):
+        # Only the specified probers should be used
+        b = self.make_branch('branch')
+        opener = self.make_branch_opener([b.base], probers=[])
+        self.assertRaises(NotBranchError, opener.open, b.base)
+        opener = self.make_branch_opener([b.base], probers=[BzrProber])
+        self.assertEquals(b.base, opener.open(b.base).base)
+
+    def test_default_probers(self):
+        # If no probers are specified to the constructor
+        # of SafeBranchOpener, then a safe set will be used,
+        # rather than all probers registered in bzr.
+        self.addCleanup(ControlDirFormat.unregister_prober, TrackingProber)
+        ControlDirFormat.register_prober(TrackingProber)
+        # Open a location without any branches, so that all probers are
+        # tried.
+        # First, check that the TrackingProber tracks correctly.
+        TrackingProber.seen_urls = []
+        opener = self.make_branch_opener(["."], probers=[TrackingProber])
+        self.assertRaises(NotBranchError, opener.open, ".")
+        self.assertEquals(1, len(TrackingProber.seen_urls))
+        TrackingProber.seen_urls = []
+        # And make sure it's registered in such a way that BzrDir.open would
+        # use it.
+        self.assertRaises(NotBranchError, BzrDir.open, ".")
+        self.assertEquals(1, len(TrackingProber.seen_urls))
+
+    def test_allowed_url(self):
+        # the opener does not raise an exception for branches stacked on
+        # branches with allowed URLs.
+        stacked_on_branch = self.make_branch('base-branch', format='1.6')
+        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_branch.set_stacked_on_url(stacked_on_branch.base)
+        opener = self.make_branch_opener(
+            [stacked_branch.base, stacked_on_branch.base])
+        # This doesn't raise an exception.
+        opener.open(stacked_branch.base)
+
+    def test_nstackable_repository(self):
+        # treats branches with UnstackableRepositoryFormats as
+        # being not stacked.
+        branch = self.make_branch('unstacked', format='knit')
+        opener = self.make_branch_opener([branch.base])
+        # This doesn't raise an exception.
+        opener.open(branch.base)
+
+    def test_allowed_relative_url(self):
+        # passes on absolute urls to check_one_url, even if the
+        # value of stacked_on_location in the config is set to a relative URL.
+        stacked_on_branch = self.make_branch('base-branch', format='1.6')
+        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_branch.set_stacked_on_url('../base-branch')
+        opener = self.make_branch_opener(
+            [stacked_branch.base, stacked_on_branch.base])
+        # Note that stacked_on_branch.base is not '../base-branch', it's an
+        # absolute URL.
+        self.assertNotEqual('../base-branch', stacked_on_branch.base)
+        # This doesn't raise an exception.
+        opener.open(stacked_branch.base)
+
+    def test_allowed_relative_nested(self):
+        # Relative URLs are resolved relative to the stacked branch.
+        self.get_transport().mkdir('subdir')
+        a = self.make_branch('subdir/a', format='1.6')
+        b = self.make_branch('b', format='1.6')
+        b.set_stacked_on_url('../subdir/a')
+        c = self.make_branch('subdir/c', format='1.6')
+        c.set_stacked_on_url('../../b')
+        opener = self.make_branch_opener([c.base, b.base, a.base])
+        # This doesn't raise an exception.
+        opener.open(c.base)
+
+    def test_forbidden_url(self):
+        # raises a BadUrl exception if a branch is stacked on a
+        # branch with a forbidden URL.
+        stacked_on_branch = self.make_branch('base-branch', format='1.6')
+        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_branch.set_stacked_on_url(stacked_on_branch.base)
+        opener = self.make_branch_opener([stacked_branch.base])
+        self.assertRaises(BadUrl, opener.open, stacked_branch.base)
+
+    def test_forbidden_url_nested(self):
+        # raises a BadUrl exception if a branch is stacked on a
+        # branch that is in turn stacked on a branch with a forbidden URL.
+        a = self.make_branch('a', format='1.6')
+        b = self.make_branch('b', format='1.6')
+        b.set_stacked_on_url(a.base)
+        c = self.make_branch('c', format='1.6')
+        c.set_stacked_on_url(b.base)
+        opener = self.make_branch_opener([c.base, b.base])
+        self.assertRaises(BadUrl, opener.open, c.base)
+
+    def test_self_stacked_branch(self):
+        # raises StackingLoopError if a branch is stacked on
+        # itself. This avoids infinite recursion errors.
+        a = self.make_branch('a', format='1.6')
+        # Bazaar 1.17 and up make it harder to create branches like this.
+        # It's still worth testing that we don't blow up in the face of them,
+        # so we grovel around a bit to create one anyway.
+        a.get_config().set_user_option('stacked_on_location', a.base)
+        opener = self.make_branch_opener([a.base])
+        self.assertRaises(BranchLoopError, opener.open, a.base)
+
+    def test_loop_stacked_branch(self):
+        # raises StackingLoopError if a branch is stacked in such
+        # a way so that it is ultimately stacked on itself. e.g. a stacked on
+        # b stacked on a.
+        a = self.make_branch('a', format='1.6')
+        b = self.make_branch('b', format='1.6')
+        a.set_stacked_on_url(b.base)
+        b.set_stacked_on_url(a.base)
+        opener = self.make_branch_opener([a.base, b.base])
+        self.assertRaises(BranchLoopError, opener.open, a.base)
+        self.assertRaises(BranchLoopError, opener.open, b.base)
+
+    def test_custom_opener(self):
+        # A custom function for opening a control dir can be specified.
+        a = self.make_branch('a', format='2a')
+        b = self.make_branch('b', format='2a')
+        b.set_stacked_on_url(a.base)
+
+        TrackingProber.seen_urls = []
+        opener = self.make_branch_opener(
+            [a.base, b.base], probers=[TrackingProber])
+        opener.open(b.base)
+        self.assertEquals(
+            set(TrackingProber.seen_urls), set([b.base, a.base]))
+
+    def test_custom_opener_with_branch_reference(self):
+        # A custom function for opening a control dir can be specified.
+        a = self.make_branch('a', format='2a')
+        b_dir = self.make_bzrdir('b')
+        b = BranchReferenceFormat().initialize(b_dir, target_branch=a)
+        TrackingProber.seen_urls = []
+        opener = self.make_branch_opener(
+            [a.base, b.base], probers=[TrackingProber])
+        opener.open(b.base)
+        self.assertEquals(
+            set(TrackingProber.seen_urls), set([b.base, a.base]))
+
+
+class TestSafeOpen(TestCaseWithTransport):
+    """Tests for `safe_open`."""
+
+    def setUp(self):
+        super(TestSafeOpen, self).setUp()
+        SafeBranchOpener.install_hook()
+
+    def test_hook_does_not_interfere(self):
+        # The transform_fallback_location hook does not interfere with regular
+        # stacked branch access outside of safe_open.
+        self.make_branch('stacked')
+        self.make_branch('stacked-on')
+        Branch.open('stacked').set_stacked_on_url('../stacked-on')
+        Branch.open('stacked')
+
+    def get_chrooted_scheme(self, relpath):
+        """Create a server that is chrooted to `relpath`.
+
+        :return: ``(scheme, get_url)`` where ``scheme`` is the scheme of the
+            chroot server and ``get_url`` returns URLs on said server.
+        """
+        transport = self.get_transport(relpath)
+        chroot_server = chroot.ChrootServer(transport)
+        chroot_server.start_server()
+        self.addCleanup(chroot_server.stop_server)
+
+        def get_url(relpath):
+            return chroot_server.get_url() + relpath
+
+        return urlutils.URL.from_string(chroot_server.get_url()).scheme, get_url
+
+    def test_stacked_within_scheme(self):
+        # A branch that is stacked on a URL of the same scheme is safe to
+        # open.
+        self.get_transport().mkdir('inside')
+        self.make_branch('inside/stacked')
+        self.make_branch('inside/stacked-on')
+        scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
+        Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
+            get_chrooted_url('stacked-on'))
+        safe_open(scheme, get_chrooted_url('stacked'))
+
+    def test_stacked_outside_scheme(self):
+        # A branch that is stacked on a URL that is not of the same scheme is
+        # not safe to open.
+        self.get_transport().mkdir('inside')
+        self.get_transport().mkdir('outside')
+        self.make_branch('inside/stacked')
+        self.make_branch('outside/stacked-on')
+        scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
+        Branch.open(get_chrooted_url('stacked')).set_stacked_on_url(
+            self.get_url('outside/stacked-on'))
+        self.assertRaises(
+            BadUrl, safe_open, scheme, get_chrooted_url('stacked'))

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2011-12-19 17:39:35 +0000
+++ bzrlib/workingtree.py	2011-12-22 01:48:24 +0000
@@ -267,8 +267,8 @@
         """
         if path is None:
             path = osutils.getcwd()
-        control = controldir.ControlDir.open(path, _unsupported)
-        return control.open_workingtree(_unsupported)
+        control = controldir.ControlDir.open(path, _unsupported=_unsupported)
+        return control.open_workingtree(_unsupported=_unsupported)
 
     @staticmethod
     def open_containing(path=None):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-21 21:31:03 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-22 01:48:24 +0000
@@ -123,6 +123,8 @@
 .. Major internal changes, unlikely to be visible to users or plugin 
    developers, but interesting for bzr developers.
 
+* Add new module ``bzrlib.safe_open``. (Jelmer Vernooij, #850843)
+
 * ``bzrlib.urlutils`` now includes ``quote`` and ``unquote`` functions,
   rather than importing them from ``urllib``. This prevents loading
   of the ``socket``, ``ssl`` and ``urllib`` modules for