launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/follow-http-redirects into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #804218 in Launchpad itself: "code importers don't follow http redirects"
  https://bugs.launchpad.net/launchpad/+bug/804218

For more details, see:
https://code.launchpad.net/~jelmer/launchpad/follow-http-redirects/+merge/75561

Make the code imports follow HTTP redirects.

The redirect URLs are checked (using the same mechanism as stacked branches, branch references), to prevent redirects to internal URLs in the data center.
-- 
https://code.launchpad.net/~jelmer/launchpad/follow-http-redirects/+merge/75561
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/follow-http-redirects into lp:launchpad.

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2011-09-13 09:16:46 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2011-09-15 15:15:02 +0000
@@ -27,7 +27,10 @@
     format_registry,
     )
 from bzrlib.errors import NoSuchFile
-from bzrlib.tests import TestCaseWithTransport
+from bzrlib.tests import (
+    TestCaseWithTransport,
+    http_utils,
+    )
 from bzrlib.transport import (
     get_transport,
     get_transport_from_url,
@@ -77,6 +80,7 @@
     AcceptAnythingPolicy,
     BadUrl,
     BlacklistPolicy,
+    BranchOpenPolicy,
     SafeBranchOpener,
     )
 from lp.codehosting.tests.helpers import create_branch_with_one_revision
@@ -1368,3 +1372,55 @@
         self.assertBadUrl("svn+ssh://svn.example.com/bla")
         self.assertGoodUrl("git://git.example.com/repo")
         self.assertGoodUrl("https://hg.example.com/hg/repo/branch";)
+
+
+class RedirectTests(http_utils.TestCaseWithRedirectedWebserver, TestCase):
+
+    layer = ForeignBranchPluginLayer
+
+    def setUp(self):
+        http_utils.TestCaseWithRedirectedWebserver.setUp(self)
+        self.disable_directory_isolation()
+        SafeBranchOpener.install_hook()
+        tree = self.make_branch_and_tree('.')
+        self.revid = tree.commit("A commit")
+        self.bazaar_store = BazaarBranchStore(
+            self.get_transport('bazaar_store'))
+
+    def makeImportWorker(self, opener_policy):
+        """Make a new `ImportWorker`."""
+        source_details = self.factory.makeCodeImportSourceDetails(
+            rcstype='bzr', url=self.get_old_url())
+        return BzrImportWorker(
+            source_details, self.get_transport('import_data'),
+            self.bazaar_store, logging.getLogger(), opener_policy)
+
+    def test_follow_redirect(self):
+        worker = self.makeImportWorker(AcceptAnythingPolicy())
+        self.assertEqual(
+            CodeImportWorkerExitCode.SUCCESS, worker.run())
+        branch_url = self.bazaar_store._getMirrorURL(
+            worker.source_details.branch_id)
+        branch = Branch.open(branch_url)
+        self.assertEquals(self.revid, branch.last_revision())
+
+    def test_redirect_to_forbidden_url(self):
+        class NewUrlBlacklistPolicy(BranchOpenPolicy):
+
+            def __init__(self, new_url):
+                self.new_url = new_url
+
+            def shouldFollowReferences(self):
+                return True
+
+            def checkOneURL(self, url):
+                if url.startswith(self.new_url):
+                    raise BadUrl(url)
+
+            def transformFallbackLocation(self, branch, url):
+                return urlutils.join(branch.base, url), False
+
+        policy = NewUrlBlacklistPolicy(self.get_new_url())
+        worker = self.makeImportWorker(policy)
+        self.assertEqual(
+            CodeImportWorkerExitCode.FAILURE_FORBIDDEN, worker.run())

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2011-09-13 09:16:46 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2011-09-15 15:15:02 +0000
@@ -37,8 +37,12 @@
     NoRepositoryPresent,
     NoSuchFile,
     NotBranchError,
-    )
-from bzrlib.transport import get_transport
+    TooManyRedirections,
+    )
+from bzrlib.transport import (
+    do_catching_redirections,
+    get_transport,
+    )
 from bzrlib.upgrade import upgrade
 import bzrlib.ui
 from bzrlib.urlutils import (
@@ -675,17 +679,28 @@
         :param url: URL to open
         :return: ControlDir instance
         """
+        def redirected(transport, e, redirection_notice):
+            self._opener_policy.checkOneURL(e.target)
+            redirected_transport = transport._redirected_to(e.source, e.target)
+            if redirected_transport is None:
+                raise NotBranchError(e.source)
+            self._logger.info('%s is%s redirected to %s',
+                 transport.base, e.permanently, redirected_transport.base)
+            return redirected_transport
+        def find_format(transport):
+            last_error = None
+            for prober_kls in self.probers:
+                prober = prober_kls()
+                try:
+                    return transport, prober.probe_transport(transport)
+                except NotBranchError, e:
+                    last_error = e
+            else:
+                raise last_error
         transport = get_transport(url)
-        for prober_kls in self.probers:
-            prober = prober_kls()
-            try:
-                format = prober.probe_transport(transport)
-            except NotBranchError:
-                pass
-            else:
-                return format.open(transport)
-        else:
-            raise NotBranchError("Not a valid branch")
+        transport, format = do_catching_redirections(find_format, transport,
+            redirected)
+        return format.open(transport)
 
     def _doImport(self):
         self._logger.info("Starting job.")
@@ -699,6 +714,9 @@
             try:
                 remote_branch = opener.open(
                     self.source_details.url, self._open_dir)
+            except TooManyRedirections:
+                self._logger.info("Too many redirections.")
+                return CodeImportWorkerExitCode.FAILURE_INVALID
             except NotBranchError:
                 self._logger.info("No branch found at remote location.")
                 return CodeImportWorkerExitCode.FAILURE_INVALID

=== modified file 'lib/lp/codehosting/safe_open.py'
--- lib/lp/codehosting/safe_open.py	2011-09-12 11:25:28 +0000
+++ lib/lp/codehosting/safe_open.py	2011-09-15 15:15:02 +0000
@@ -27,7 +27,7 @@
 
 
 # TODO JelmerVernooij 2011-08-06: This module is generic enough to be
-# in bzrlib, and may be of use to others.
+# in bzrlib, and may be of use to others. bug=850843
 
 
 class BadUrl(Exception):