Merge lp:~mwhudson/launchpad/unicod-branch-names-bug-449528 into lp:launchpad

[Michael Hudson-Doyle]

Fix some OOPSes that can result from attempts to push branches containing non-ASCII names.

[Michael Hudson-Doyle (mwhudson)]

Hi there,

This branch fixes various explosions you can get when trying to use non-ascii characters in branch names. We don't allow this, but we should fail gracefully and this branch tries to get us there.

There are various things that conspire to making this annoying:

 * xmlrpclib papers over the difference between str and unicode to the extent that if you send a utf-8 encoded str containing non-ascii characters, the other side will get a unicode string.
   * in particular this applied to the 'faultString' attribute of Faults.
 * Python exceptions containing non-ascii unicode strings are a bit like dodgy hand grenades, at least in Python 2.5: they tend to blow up if you touch them.
 * There turned out to be bugs in bzr in this area, now fixed in the version we are using.

Anyway, this branch fixes most problems I found. I recommend running make run_all and trying to push branches named things like "bzr+ssh://<email address hidden>/~m%c2%a9rk/gnome-trminal/dd".

This branch has been sitting around in my repo for ages. I'll be happy to get it done!

Cheers,
mwh

[Tim Penhey (thumper)]

Looks good to me. I trust you've tried pushing locally - I'm not going to.

review: Approve

=== modified file 'lib/lp/code/doc/branch-xmlrpc.txt'
--- lib/lp/code/doc/branch-xmlrpc.txt	2009-08-13 15:12:16 +0000
+++ lib/lp/code/doc/branch-xmlrpc.txt	2010-02-22 01:48:21 +0000
@@ -417,8 +417,8 @@
     >>> results = branchset_api.resolve_lp_path('~mark/+junk/doesntexist')
     >>> for url in results['urls']:
     ...     print url
-    bzr+ssh://bazaar.launchpad.dev/~mark/+junk/doesntexist
-    http://bazaar.launchpad.dev/~mark/+junk/doesntexist
+    bzr+ssh://bazaar.launchpad.dev/~mark/%2Bjunk/doesntexist
+    http://bazaar.launchpad.dev/~mark/%2Bjunk/doesntexist
 
 
 But if the project or the user doesn't exist, Launchpad will return a fault:

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-02-16 20:36:58 +0000
+++ lib/lp/code/model/branch.py	2010-02-22 01:48:21 +0000
@@ -1309,7 +1309,7 @@
     accepted_schemes.add('sftp')
     assert scheme in accepted_schemes, "Unknown scheme: %s" % scheme
     host = URI(config.codehosting.supermirror_root).host
-    path = '/' + unique_name
+    path = '/' + urlutils.escape(unique_name)
     if suffix is not None:
         path = os.path.join(path, suffix)
     return str(URI(scheme=scheme, host=host, path=path))

=== modified file 'lib/lp/code/xmlrpc/branch.py'
--- lib/lp/code/xmlrpc/branch.py	2010-02-16 20:36:58 +0000
+++ lib/lp/code/xmlrpc/branch.py	2010-02-22 01:48:21 +0000
@@ -12,6 +12,8 @@
     'PublicCodehostingAPI']
 
 
+from bzrlib import urlutils
+
 from zope.component import getUtility
 from zope.interface import Interface, implements
 
@@ -100,7 +102,7 @@
 
         try:
             unicode_branch_url = branch_url.decode('utf-8')
-            url = IBranch['url'].validate(unicode_branch_url)
+            IBranch['url'].validate(unicode_branch_url)
         except LaunchpadValidationError, exc:
             return faults.InvalidBranchUrl(branch_url, exc)
 
@@ -238,23 +240,24 @@
         # the model code(blech) or some automated way of reraising as faults
         # or using a narrower range of faults (e.g. only one "NoSuch" fault).
         except InvalidProductName, e:
-            raise faults.InvalidProductIdentifier(e.name)
+            raise faults.InvalidProductIdentifier(urlutils.escape(e.name))
         except NoSuchProductSeries, e:
-            raise faults.NoSuchProductSeries(e.name, e.product)
+            raise faults.NoSuchProductSeries(
+                urlutils.escape(e.name), e.product)
         except NoSuchPerson, e:
-            raise faults.NoSuchPersonWithName(e.name)
+            raise faults.NoSuchPersonWithName(urlutils.escape(e.name))
         except NoSuchProduct, e:
-            raise faults.NoSuchProduct(e.name)
+            raise faults.NoSuchProduct(urlutils.escape(e.name))
         except NoSuchDistroSeries, e:
-            raise faults.NoSuchDistroSeries(e.name)
+            raise faults.NoSuchDistroSeries(urlutils.escape(e.name))
         except NoSuchSourcePackageName, e:
-            raise faults.NoSuchSourcePackageName(e.name)
+            raise faults.NoSuchSourcePackageName(urlutils.escape(e.name))
         except NoLinkedBranch, e:
             raise faults.NoLinkedBranch(e.component)
         except CannotHaveLinkedBranch, e:
             raise faults.CannotHaveLinkedBranch(e.component)
         except InvalidNamespace, e:
-            raise faults.InvalidBranchUniqueName(e.name)
+            raise faults.InvalidBranchUniqueName(urlutils.escape(e.name))
         return self._getResultDict(branch, suffix, supported_schemes)
 
     def resolve_lp_path(self, path):

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2009-10-19 05:40:52 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2010-02-22 01:48:21 +0000
@@ -12,6 +12,7 @@
 
 
 import datetime
+import urllib
 
 import pytz
 
@@ -232,7 +233,7 @@
         def create_branch(requester):
             if not branch_path.startswith('/'):
                 return faults.InvalidPath(branch_path)
-            escaped_path = unescape(branch_path.strip('/')).encode('utf-8')
+            escaped_path = unescape(branch_path.strip('/'))
             try:
                 namespace_name, branch_name = split_unique_name(escaped_path)
             except ValueError:
@@ -254,7 +255,12 @@
             try:
                 branch = namespace.createBranch(
                     BranchType.HOSTED, branch_name, requester)
-            except (BranchCreationException, LaunchpadValidationError), e:
+            except LaunchpadValidationError, e:
+                msg = e.args[0]
+                if isinstance(msg, unicode):
+                    msg = msg.encode('utf-8')
+                return faults.PermissionDenied(msg)
+            except BranchCreationException, e:
                 return faults.PermissionDenied(str(e))
             else:
                 return branch.id
@@ -294,8 +300,7 @@
     def _serializeControlDirectory(self, requester, product_path,
                                    trailing_path):
         try:
-            namespace = lookup_branch_namespace(
-                unescape(product_path).encode('utf-8'))
+            namespace = lookup_branch_namespace(product_path)
         except (InvalidNamespace, NotFoundError):
             return
         if not ('.bzr' == trailing_path or trailing_path.startswith('.bzr/')):
@@ -321,9 +326,9 @@
                 return faults.InvalidPath(path)
             stripped_path = path.strip('/')
             for first, second in iter_split(stripped_path, '/'):
+                first = unescape(first)
                 # Is it a branch?
-                branch = getUtility(IBranchLookup).getByUniqueName(
-                    unescape(first).encode('utf-8'))
+                branch = getUtility(IBranchLookup).getByUniqueName(first)
                 if branch is not None:
                     branch = self._serializeBranch(requester, branch, second)
                     if branch is None:

=== modified file 'lib/lp/code/xmlrpc/tests/test_branch.py'
--- lib/lp/code/xmlrpc/tests/test_branch.py	2009-11-12 04:28:28 +0000
+++ lib/lp/code/xmlrpc/tests/test_branch.py	2010-02-22 01:48:21 +0000
@@ -11,6 +11,8 @@
 import unittest
 import xmlrpclib
 
+from bzrlib import urlutils
+
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
@@ -23,6 +25,9 @@
 from canonical.testing import DatabaseFunctionalLayer
 
 
+NON_ASCII_NAME = u'nam\N{LATIN SMALL LETTER E WITH ACUTE}'
+
+
 class TestExpandURL(TestCaseWithFactory):
     """Test the way that URLs are expanded."""
 
@@ -54,9 +59,7 @@
         return branch
 
     def assertResolves(self, lp_url_path, unique_name):
-        """Assert that the given lp URL path expands to the unique name of
-        'branch'.
-        """
+        """Assert that `lp_url_path` path expands to `unique_name`."""
         results = self.api.resolve_lp_path(lp_url_path)
         # This improves the error message if results happens to be a fault.
         if isinstance(results, faults.LaunchpadFault):
@@ -110,7 +113,7 @@
         self.assertResolves(
             self.product.name, trunk_series.branch.unique_name)
 
-    def test_productDoesntExist(self):
+    def test_product_doesnt_exist(self):
         # Return a NoSuchProduct fault if the product doesn't exist.
         self.assertFault(
             'doesntexist', faults.NoSuchProduct('doesntexist'))
@@ -141,11 +144,19 @@
     def test_invalid_product_name(self):
         # If we get a string that cannot be a name for a product where we
         # expect the name of a product, we should error appropriately.
-        invalid_name = '+' + self.factory.getUniqueString()
+        invalid_name = '_' + self.factory.getUniqueString()
         self.assertFault(
             invalid_name,
             faults.InvalidProductIdentifier(invalid_name))
 
+    def test_invalid_product_name_non_ascii(self):
+        # lp:<non-ascii-string> returns InvalidProductIdentifier with the name
+        # escaped.
+        invalid_name = '_' + NON_ASCII_NAME
+        self.assertFault(
+            invalid_name,
+            faults.InvalidProductIdentifier(urlutils.escape(invalid_name)))
+
     def test_product_and_series(self):
         # lp:product/series expands to the branch associated with the product
         # series 'series' on 'product'.
@@ -184,6 +195,14 @@
             '%s/%s' % (self.product.name, "doesntexist"),
             faults.NoSuchProductSeries("doesntexist", self.product))
 
+    def test_no_such_product_series_non_ascii(self):
+        # lp:product/<non-ascii-string> returns NoSuchProductSeries with the
+        # name escaped.
+        self.assertFault(
+            '%s/%s' % (self.product.name, NON_ASCII_NAME),
+            faults.NoSuchProductSeries(
+                urlutils.escape(NON_ASCII_NAME), self.product))
+
     def test_no_such_distro_series(self):
         # Return a NoSuchDistroSeries fault if there is no series of the given
         # name on that distribution.
@@ -192,6 +211,14 @@
             '%s/doesntexist/whocares' % distro.name,
             faults.NoSuchDistroSeries("doesntexist"))
 
+    def test_no_such_distro_series_non_ascii(self):
+        # lp:distro/<non-ascii-string>/whatever returns NoSuchDistroSeries
+        # with the name escaped.
+        distro = self.factory.makeDistribution()
+        self.assertFault(
+            '%s/%s/whocares' % (distro.name, NON_ASCII_NAME),
+            faults.NoSuchDistroSeries(urlutils.escape(NON_ASCII_NAME)))
+
     def test_no_such_source_package(self):
         # Return a NoSuchSourcePackageName fault if there is no source package
         # of the given name.
@@ -201,6 +228,16 @@
             '%s/%s/doesntexist' % (distribution.name, distroseries.name),
             faults.NoSuchSourcePackageName('doesntexist'))
 
+    def test_no_such_source_package_non_ascii(self):
+        # lp:distro/series/<non-ascii-name> returns NoSuchSourcePackageName
+        # with the name escaped.
+        distroseries = self.factory.makeDistroRelease()
+        distribution = distroseries.distribution
+        self.assertFault(
+            '%s/%s/%s' % (
+                distribution.name, distroseries.name, NON_ASCII_NAME),
+            faults.NoSuchSourcePackageName(urlutils.escape(NON_ASCII_NAME)))
+
     def test_no_linked_branch_for_source_package(self):
         # Return a NoLinkedBranch fault if there's no linked branch for the
         # sourcepackage.
@@ -234,13 +271,23 @@
             owner.name, self.product.name)
         self.assertResolves(nonexistent_branch, nonexistent_branch)
 
+    def test_no_such_branch_product_non_ascii(self):
+        # A path to a branch that contains non ascii characters will never
+        # find a branch, but it still resolves rather than erroring.
+        owner = self.factory.makePerson()
+        nonexistent_branch = u'~%s/%s/%s' % (
+            owner.name, self.product.name, NON_ASCII_NAME)
+        self.assertResolves(
+            nonexistent_branch, urlutils.escape(nonexistent_branch))
+
     def test_no_such_branch_personal(self):
         # Resolve paths to junk branches.
         # This test added to make sure we don't raise a fault when looking for
         # the '+junk' project, which doesn't actually exist.
         owner = self.factory.makePerson()
         nonexistent_branch = '~%s/+junk/doesntexist' % owner.name
-        self.assertResolves(nonexistent_branch, nonexistent_branch)
+        self.assertResolves(
+            nonexistent_branch, urlutils.escape(nonexistent_branch))
 
     def test_no_such_branch_package(self):
         # Resolve paths to package branches even if there's no branch of that
@@ -269,6 +316,16 @@
             nonexistent_owner_branch,
             faults.NoSuchPersonWithName('doesntexist'))
 
+    def test_resolve_branch_with_no_such_owner_non_ascii(self):
+        # lp:~<non-ascii-string>/product/name returns NoSuchPersonWithName
+        # with the name escaped.
+        nonexistent_owner_branch = u"~%s/%s/%s" % (
+            NON_ASCII_NAME, self.factory.getUniqueString(),
+            self.factory.getUniqueString())
+        self.assertFault(
+            nonexistent_owner_branch,
+            faults.NoSuchPersonWithName(urlutils.escape(NON_ASCII_NAME)))
+
     def test_too_many_segments(self):
         # If we have more segments than are necessary to refer to a branch,
         # then attach these segments to the resolved url.

=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2009-10-19 05:40:52 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-02-22 01:48:21 +0000
@@ -676,13 +676,27 @@
         owner = self.factory.makePerson()
         product = self.factory.makeProduct()
         invalid_name = 'invalid name!'
-        message = ("Invalid branch name %r. %s"
+        message = ("Invalid branch name '%s'. %s"
                    % (invalid_name, BRANCH_NAME_VALIDATION_ERROR_MESSAGE))
         fault = self.branchfs.createBranch(
             owner.id, escape(
                 '/~%s/%s/%s' % (owner.name, product.name, invalid_name)))
         self.assertFaultEqual(faults.PermissionDenied(message), fault)
 
+    def test_createBranch_unicode_name(self):
+        # Creating a branch with an invalid name fails.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        invalid_name = u'invalid\N{LATIN SMALL LETTER E WITH ACUTE}'
+        message = ("Invalid branch name '%s'. %s"
+                   % (invalid_name.encode('utf-8'),
+                      str(BRANCH_NAME_VALIDATION_ERROR_MESSAGE)))
+        fault = self.branchfs.createBranch(
+            owner.id, escape(
+                '/~%s/%s/%s' % (owner.name, product.name, invalid_name)))
+        self.assertFaultEqual(
+            faults.PermissionDenied(message), fault)
+
     def test_createBranch_bad_user(self):
         # Creating a branch under a non-existent user fails.
         owner = self.factory.makePerson()
@@ -920,6 +934,13 @@
         path = '/~%s/%s/no-such-branch' % (requester.name, product.name)
         self.assertNotFound(requester, path)
 
+    def test_translatePath_no_such_branch_non_ascii(self):
+        requester = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        path = u'/~%s/%s/non-asci\N{LATIN SMALL LETTER I WITH DIAERESIS}' % (
+            requester.name, product.name)
+        self.assertNotFound(requester, escape(path))
+
     def test_translatePath_private_branch(self):
         requester = self.factory.makePerson()
         branch = removeSecurityProxy(

=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2010-01-06 12:09:46 +0000
+++ lib/lp/codehosting/inmemory.py	2010-02-22 01:48:21 +0000
@@ -555,7 +555,7 @@
     def createBranch(self, requester_id, branch_path):
         if not branch_path.startswith('/'):
             return faults.InvalidPath(branch_path)
-        escaped_path = unescape(branch_path.strip('/')).encode('utf-8')
+        escaped_path = unescape(branch_path.strip('/'))
         try:
             namespace_path, branch_name = escaped_path.rsplit('/', 1)
         except ValueError:
@@ -565,7 +565,7 @@
         owner = self._person_set.getByName(data['person'])
         if owner is None:
             return faults.NotFound(
-                "User/team %r does not exist." % (data['person'],))
+                "User/team '%s' does not exist." % (data['person'],))
         registrant = self._person_set.get(requester_id)
         # The real code consults the branch creation policy of the product. We
         # don't need to do so here, since the tests above this layer never
@@ -583,7 +583,7 @@
             product = self._product_set.getByName(data['product'])
             if product is None:
                 return faults.NotFound(
-                    "Project %r does not exist." % (data['product'],))
+                    "Project '%s' does not exist." % (data['product'],))
         elif data['distribution'] is not None:
             distro = self._distribution_set.getByName(data['distribution'])
             if distro is None:
@@ -612,7 +612,10 @@
                 sourcepackage=sourcepackage, registrant=registrant,
                 branch_type=BranchType.HOSTED).id
         except LaunchpadValidationError, e:
-            return faults.PermissionDenied(str(e))
+            msg = e.args[0]
+            if isinstance(msg, unicode):
+                msg = msg.encode('utf-8')
+            return faults.PermissionDenied(msg)
 
     def requestMirror(self, requester_id, branch_id):
         self._branch_set.get(branch_id).requestMirror()

=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2010-01-20 02:15:27 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2010-02-22 01:48:21 +0000
@@ -612,7 +612,10 @@
             # parent directories", which is just misleading.
             fault = trap_fault(
                 failure, faults.NotFound, faults.PermissionDenied)
-            raise PermissionDenied(virtual_url_fragment, fault.faultString)
+            faultString = fault.faultString
+            if isinstance(faultString, unicode):
+                faultString = faultString.encode('utf-8')
+            raise PermissionDenied(virtual_url_fragment, faultString)
 
         return deferred.addErrback(translate_fault)