Merge lp:~salgado/launchpad/vostok-no-external-redirect into lp:launchpad

[Guilherme Salgado]

Add a safety net so that vostok doesn't redirect users to any host other than vostok.dev

Make sure vostok doesn't redirect users to any host other than vostok.dev

[Curtis Hovey (sinzui)]

We discussed some redirect scenarios on IRC. Changelogs and attachments are not an issue right now. If the librarian needs to be supported, we can add that. What is important is that redirects are locked down to host we trust.

review: Approve (code)

=== modified file 'lib/lp/vostok/publisher.py'
--- lib/lp/vostok/publisher.py	2010-08-06 17:39:52 +0000
+++ lib/lp/vostok/publisher.py	2010-08-16 19:16:50 +0000
@@ -20,7 +20,8 @@
 from canonical.launchpad.webapp import canonical_url, Navigation
 from canonical.launchpad.webapp.publication import LaunchpadBrowserPublication
 from canonical.launchpad.webapp.servers import (
-    LaunchpadBrowserRequest, VirtualHostRequestPublicationFactory)
+    LaunchpadBrowserRequest, LaunchpadBrowserResponse,
+    VirtualHostRequestPublicationFactory)
 from canonical.launchpad.webapp.vhosts import allvhosts
 
 from lp.registry.interfaces.distribution import IDistributionSet
@@ -43,6 +44,26 @@
 class VostokBrowserRequest(VostokRequestMixin, LaunchpadBrowserRequest):
     """Request class for Vostok layer."""
 
+    def _createResponse(self):
+        """As per zope.publisher.browser.BrowserRequest._createResponse"""
+        return VostokBrowserResponse()
+
+
+class VostokBrowserResponse(LaunchpadBrowserResponse):
+
+    def redirect(self, location, status=None, trusted=False,
+                 temporary_if_possible=False):
+        """Override the parent method to make redirects untrusted by default.
+
+        This is so that we don't allow redirects to any hosts other than
+        vostok's.
+        """
+        # Need to call LaunchpadBrowserResponse.redirect() directly because
+        # the temporary_if_possible argument only exists there.
+        LaunchpadBrowserResponse.redirect(
+            self, location, status=status, trusted=trusted,
+            temporary_if_possible=temporary_if_possible)
+
 
 class IVostokRoot(Interface):
     """Marker interface for the root vostok object."""

=== modified file 'lib/lp/vostok/tests/test_publisher.py'
--- lib/lp/vostok/tests/test_publisher.py	2010-08-05 01:08:24 +0000
+++ lib/lp/vostok/tests/test_publisher.py	2010-08-16 19:16:50 +0000
@@ -13,7 +13,8 @@
 from lp.testing import TestCase
 from lp.testing.publication import get_request_and_publication
 
-from lp.vostok.publisher import VostokLayer, VostokRoot
+from lp.vostok.publisher import (
+    VostokBrowserResponse, VostokBrowserRequest, VostokLayer, VostokRoot)
 
 
 class TestRegistration(TestCase):
@@ -38,5 +39,25 @@
         self.assertIsInstance(root, VostokRoot)
 
 
+class TestVostokBrowserRequest(TestCase):
+
+    def test_createResponse(self):
+        request = VostokBrowserRequest(None, {})
+        self.assertIsInstance(
+            request._createResponse(), VostokBrowserResponse)
+
+
+class TestVostokBrowserResponse(TestCase):
+
+    def test_redirect_to_different_host(self):
+        # Unlike Launchpad's BrowserResponse class, VostokBrowserResponse
+        # doesn't allow redirects to any host other than the current one.
+        request = VostokBrowserRequest(None, {})
+        response = request._createResponse()
+        response._request = request
+        self.assertRaises(
+            ValueError, response.redirect, 'http://launchpad.dev')
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)