launchpad-reviewers team mailing list archive

[Guilherme Salgado <salgado@xxxxxxxxxxxxx>]

Guilherme Salgado has proposed merging lp:~salgado/launchpad/vostok-no-external-redirect into lp:launchpad/devel with lp:~salgado/launchpad/vostok-traverse-package as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Make sure vostok doesn't redirect users to any host other than vostok.dev
-- 
https://code.launchpad.net/~salgado/launchpad/vostok-no-external-redirect/+merge/32623
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~salgado/launchpad/vostok-no-external-redirect into lp:launchpad/devel.

=== 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-13 20:12:48 +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-13 20:12:48 +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__)