launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-ref-url-encode into lp:launchpad.

Commit message:
Percent-encode reference names in GitRef URLs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1787965 in Launchpad itself: "code.launchpad.net links do not handle pound (#) in git branch name"
  https://bugs.launchpad.net/launchpad/+bug/1787965

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-ref-url-encode/+merge/353464

Conceivably we could do this more generally in lp.services.webapp.metazcml.InterfaceInstanceDispatcher.__getitem__ instead, but that's a rather more sweeping change, and most URL segments we generate are based on IDs or LP-validated names and thus have a restricted enough character set that we don't need to worry about encoding.  Git reference names are unusual in that they allow characters that might need URL-encoding; see git-check-ref-format(1).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-ref-url-encode into lp:launchpad.

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2018-05-16 19:09:04 +0000
+++ lib/lp/code/browser/configure.zcml	2018-08-21 00:01:12 +0000
@@ -913,7 +913,7 @@
         name="+index"/>
     <browser:url
         for="lp.code.interfaces.gitref.IGitRef"
-        path_expression="string:+ref/${name}"
+        path_expression="string:+ref/${url_quoted_name}"
         attribute_to_parent="repository"
         rootsite="code"/>
     <browser:pages

=== modified file 'lib/lp/code/browser/tests/test_gitlisting.py'
--- lib/lp/code/browser/tests/test_gitlisting.py	2018-02-01 18:38:24 +0000
+++ lib/lp/code/browser/tests/test_gitlisting.py	2018-08-21 00:01:12 +0000
@@ -36,7 +36,10 @@
             owner=self.owner, target=self.target, name="foo")
         self.factory.makeGitRefs(
             main_repo,
-            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
+            paths=[
+                "refs/heads/master", "refs/heads/1.0", "refs/heads/with#hash",
+                "refs/tags/1.1",
+                ])
 
         other_repo = self.factory.makeGitRepository(
             owner=self.factory.makePerson(name="contributor"),
@@ -71,11 +74,14 @@
         table = soup.find(
             'div', id='default-repository-branches').find('table')
         self.assertContentEqual(
-            ['1.0', 'master'],
+            ['1.0', 'master', 'with#hash'],
             [link.find(text=True) for link in table.findAll('a')])
         self.assertEndsWith(
             table.find(text="1.0").parent['href'],
             "/~foowner/%s/+git/foo/+ref/1.0" % self.target_path)
+        self.assertEndsWith(
+            table.find(text="with#hash").parent['href'],
+            "/~foowner/%s/+git/foo/+ref/with%%23hash" % self.target_path)
 
         # Other repos are listed.
         table = soup.find(

=== modified file 'lib/lp/code/browser/tests/test_gitref.py'
--- lib/lp/code/browser/tests/test_gitref.py	2017-10-21 18:14:14 +0000
+++ lib/lp/code/browser/tests/test_gitref.py	2018-08-21 00:01:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for GitRefView."""
@@ -28,9 +28,13 @@
     admin_logged_in,
     BrowserTestCase,
     StormStatementRecorder,
+    TestCaseWithFactory,
     )
 from lp.testing.dbuser import dbuser
-from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import (
     extract_text,
@@ -42,6 +46,35 @@
     )
 
 
+class TestGitRefNavigation(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_canonical_url_branch(self):
+        [ref] = self.factory.makeGitRefs(paths=["refs/heads/master"])
+        self.assertEqual(
+            "%s/+ref/master" % canonical_url(ref.repository),
+            canonical_url(ref))
+
+    def test_canonical_url_with_slash(self):
+        [ref] = self.factory.makeGitRefs(paths=["refs/heads/with/slash"])
+        self.assertEqual(
+            "%s/+ref/with/slash" % canonical_url(ref.repository),
+            canonical_url(ref))
+
+    def test_canonical_url_percent_encoded(self):
+        [ref] = self.factory.makeGitRefs(paths=["refs/heads/with#hash"])
+        self.assertEqual(
+            "%s/+ref/with%%23hash" % canonical_url(ref.repository),
+            canonical_url(ref))
+
+    def test_canonical_url_tag(self):
+        [ref] = self.factory.makeGitRefs(paths=["refs/tags/1.0"])
+        self.assertEqual(
+            "%s/+ref/refs/tags/1.0" % canonical_url(ref.repository),
+            canonical_url(ref))
+
+
 class TestGitRefView(BrowserTestCase):
 
     layer = LaunchpadFunctionalLayer

=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py	2018-06-22 10:01:34 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py	2018-08-21 00:01:12 +0000
@@ -76,6 +76,11 @@
         url = "%s/+ref/refs/heads/master" % canonical_url(repository)
         self.assertRaises(NotFound, test_traverse, url)
 
+    def test_traverse_quoted_ref(self):
+        [ref] = self.factory.makeGitRefs(paths=["refs/heads/with#hash"])
+        url = "%s/+ref/with%%23hash" % canonical_url(ref.repository)
+        self.assertEqual(ref, test_traverse(url)[0])
+
 
 class TestGitRepositoryView(BrowserTestCase):
 

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2018-06-15 16:58:14 +0000
+++ lib/lp/code/interfaces/gitref.py	2018-08-21 00:01:12 +0000
@@ -80,6 +80,8 @@
         "A shortened version of the full path to this reference, with any "
         "leading refs/heads/ removed.")
 
+    url_quoted_name = Attribute("The reference name, quoted for use in URLs.")
+
     commit_sha1 = exported(TextLine(
         title=_("Commit SHA-1"), required=True, readonly=True,
         description=_(

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2018-07-12 17:46:32 +0000
+++ lib/lp/code/model/gitref.py	2018-08-21 00:01:12 +0000
@@ -108,6 +108,11 @@
             return self.path
 
     @property
+    def url_quoted_name(self):
+        """See `IGitRef`."""
+        return quote(self.name)
+
+    @property
     def identity(self):
         """See `IGitRef`."""
         return "%s:%s" % (self.repository.shortened_path, self.name)

=== modified file 'lib/lp/testing/publication.py'
--- lib/lp/testing/publication.py	2015-10-26 14:54:43 +0000
+++ lib/lp/testing/publication.py	2018-08-21 00:01:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helpers for testing out publication related code."""
@@ -12,6 +12,7 @@
 
 from cStringIO import StringIO
 
+from six.moves.urllib_parse import unquote
 from zope.app.publication.requestpublicationregistry import factoryRegistry
 from zope.authentication.interfaces import IUnauthenticatedPrincipal
 from zope.component import (
@@ -98,7 +99,7 @@
     request, publication = get_request_and_publication(
         host=url_parts[1], extra_environment={
             'SERVER_URL': server_url,
-            'PATH_INFO': path_info})
+            'PATH_INFO': unquote(path_info)})
 
     request.setPublication(publication)
     # We avoid calling publication.beforePublication because this starts a new

=== modified file 'lib/lp/testing/tests/test_publication.py'
--- lib/lp/testing/tests/test_publication.py	2013-04-10 09:36:25 +0000
+++ lib/lp/testing/tests/test_publication.py	2018-08-21 00:01:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the helpers in `lp.testing.publication`."""
@@ -7,6 +7,7 @@
 
 from lazr.restful import EntryResource
 from lazr.restful.utils import get_current_browser_request
+from six.moves.urllib_parse import quote
 from zope.browserpage.simpleviewclass import simple
 from zope.component import (
     getSiteManager,
@@ -82,6 +83,15 @@
             'https://launchpad.dev/' + product.name)
         self.assertEqual(product, context)
 
+    def test_traverse_quoted(self):
+        # test_traverse decodes percent-encoded segments in URLs when
+        # constructing PATH_INFO so that traversal works.
+        login(ANONYMOUS)
+        product = self.factory.makeProduct(name='foo+bar')
+        context, view, request = test_traverse(
+            'https://launchpad.dev/' + quote(product.name))
+        self.assertEqual(product, context)
+
     def test_request_is_current_during_traversal(self):
         # The request that test_traverse creates is current during
         # traversal in the sense of get_current_browser_request.