Merge lp:~benji/launchpad/bug-597324 into lp:launchpad

[Benji York]

This merge proposal covers three relatively small changes to this branch
(r11114-r11118):

- removal an attempted (and ill-fated) work-around for bug 608920
- addition of tests for all existing maybe_block_offsite_form_post code
  paths (in preparation to add another exception)
- addition of referer requirement exception for OpenID callback (fixes
  the error described in
  https://bugs.edge.launchpad.net/launchpad-foundations/+bug/597324/comments/10)

[Māris Fogels (mars)]

Hi Benji,

This change looks good to me. The test suite addition is particularly welcome. That code is absolutely critical judging by the number and detail of the comments, so the additional test coverage counts many many times over. Big +1 for that.

On line 352 of the diff you have a comment that says '# this should not raise an exception', and then on the next line you call self.assertRaises(). The comment probably needs to be deleted.

Otherwise, this change looks good. r=mars

Maris

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-07-24 17:43:17 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-08-17 16:05:20 +0000
@@ -254,15 +254,6 @@
                     'Did not expect multi-valued fields.')
             params[key] = value[0]
 
-        # XXX benji 2010-07-23 bug=608920
-        # The production OpenID provider has some Django middleware that
-        # generates a token used to prevent XSRF attacks and stuffs it into
-        # every form.  Unfortunately that includes forms that have off-site
-        # targets and since our OpenID client verifies that no form values have
-        # been injected as a security precaution, this breaks logging-in in
-        # certain circumstances (see bug 597324).  The best we can do at the
-        # moment is to remove the token before invoking the OpenID library.
-        params.pop('csrfmiddlewaretoken', None)
         return params
 
     def _get_requested_url(self, request):
 
=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2010-07-30 00:06:00 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2010-08-17 16:05:20 +0000
@@ -60,6 +60,90 @@
 
 METHOD_WRAPPER_TYPE = type({}.__setitem__)
 
+OFFSITE_POST_WHITELIST = ('/+storeblob', '/+request-token', '/+access-token',
+    '/+hwdb/+submit', '/+openid')
+
+
+def maybe_block_offsite_form_post(request):
+    """Check if an attempt was made to post a form from a remote site.
+
+    This is a cross-site request forgery (XSRF/CSRF) countermeasure.
+
+    The OffsiteFormPostError exception is raised if the following
+    holds true:
+      1. the request method is POST *AND*
+      2. a. the HTTP referer header is empty *OR*
+         b. the host portion of the referrer is not a registered vhost
+    """
+    if request.method != 'POST':
+        return
+    if (IOAuthSignedRequest.providedBy(request)
+        or not IBrowserRequest.providedBy(request)):
+        # We only want to check for the referrer header if we are
+        # in the middle of a request initiated by a web browser. A
+        # request to the web service (which is necessarily
+        # OAuth-signed) or a request that does not implement
+        # IBrowserRequest (such as an XML-RPC request) can do
+        # without a Referer.
+        return
+    if request['PATH_INFO'] in OFFSITE_POST_WHITELIST:
+        # XXX: jamesh 2007-11-23 bug=124421:
+        # Allow offsite posts to our TestOpenID endpoint.  Ideally we'd
+        # have a better way of marking this URL as allowing offsite
+        # form posts.
+        #
+        # XXX gary 2010-03-09 bug=535122,538097
+        # The one-off exceptions are necessary because existing
+        # non-browser applications make requests to these URLs
+        # without providing a Referer. Apport makes POST requests
+        # to +storeblob without providing a Referer (bug 538097),
+        # and launchpadlib used to make POST requests to
+        # +request-token and +access-token without providing a
+        # Referer.
+        #
+        # XXX Abel Deuring 2010-04-09 bug=550973
+        # The HWDB client "checkbox" accesses /+hwdb/+submit without
+        # a referer. This will change in the version in Ubuntu 10.04,
+        # but Launchpad should support HWDB submissions from older
+        # Ubuntu versions during their support period.
+        #
+        # We'll have to keep an application's one-off exception
+        # until the application has been changed to send a
+        # Referer, and until we have no legacy versions of that
+        # application to support. For instance, we can't get rid
+        # of the apport exception until after Lucid's end-of-life
+        # date. We should be able to get rid of the launchpadlib
+        # exception after Karmic's end-of-life date.
+        return
+    if request['PATH_INFO'].startswith('/+openid-callback'):
+        # If this is a callback from an OpenID provider, we don't require an
+        # on-site referer (because the provider may be off-site).  This
+        # exception was added as a result of bug 597324 (message #10 in
+        # particular).
+        return
+    referrer = request.getHeader('referer') # match HTTP spec misspelling
+    if not referrer:
+        raise NoReferrerError('No value for REFERER header')
+    # XXX: jamesh 2007-04-26 bug=98437:
+    # The Zope testing infrastructure sets a default (incorrect)
+    # referrer value of "localhost" or "localhost:9000" if no
+    # referrer is included in the request.  We let it pass through
+    # here for the benefits of the tests.  Web browsers send full
+    # URLs so this does not open us up to extra XSRF attacks.
+    if referrer in ['localhost', 'localhost:9000']:
+        return
+    # Extract the hostname from the referrer URI
+    try:
+        hostname = URI(referrer).host
+    except InvalidURIError:
+        hostname = None
+    if hostname not in allvhosts.hostnames:
+        raise OffsiteFormPostError(referrer)
+
+
+class ProfilingOops(Exception):
+    """Fake exception used to log OOPS information when profiling pages."""
+
 
 class LoginRoot:
     """Object that provides IPublishTraverse to return only itself.
@@ -191,7 +275,7 @@
         principal = self.getPrincipal(request)
         request.setPrincipal(principal)
         self.maybeRestrictToTeam(request)
-        self.maybeBlockOffsiteFormPost(request)
+        maybe_block_offsite_form_post(request)
         self.maybeNotifyReadOnlyMode(request)
 
     def maybeNotifyReadOnlyMode(self, request):
@@ -295,77 +379,6 @@
             uri = uri.replace(query=query_string)
         return str(uri)
 
-    def maybeBlockOffsiteFormPost(self, request):
-        """Check if an attempt was made to post a form from a remote site.
-
-        The OffsiteFormPostError exception is raised if the following
-        holds true:
-          1. the request method is POST *AND*
-          2. a. the HTTP referer header is empty *OR*
-             b. the host portion of the referrer is not a registered vhost
-        """
-        if request.method != 'POST':
-            return
-        # XXX: jamesh 2007-11-23 bug=124421:
-        # Allow offsite posts to our TestOpenID endpoint.  Ideally we'd
-        # have a better way of marking this URL as allowing offsite
-        # form posts.
-        if request['PATH_INFO'] == '/+openid':
-            return
-        if (IOAuthSignedRequest.providedBy(request)
-            or not IBrowserRequest.providedBy(request)
-            or request['PATH_INFO'] in (
-                '/+storeblob', '/+request-token', '/+access-token',
-                '/+hwdb/+submit')):
-            # We only want to check for the referrer header if we are
-            # in the middle of a request initiated by a web browser. A
-            # request to the web service (which is necessarily
-            # OAuth-signed) or a request that does not implement
-            # IBrowserRequest (such as an XML-RPC request) can do
-            # without a Referer.
-            #
-            # XXX gary 2010-03-09 bug=535122,538097
-            # The one-off exceptions are necessary because existing
-            # non-browser applications make requests to these URLs
-            # without providing a Referer. Apport makes POST requests
-            # to +storeblob without providing a Referer (bug 538097),
-            # and launchpadlib used to make POST requests to
-            # +request-token and +access-token without providing a
-            # Referer.
-            #
-            # XXX Abel Deuring 2010-04-09 bug=550973
-            # The HWDB client "checkbox" accesses /+hwdb/+submit without
-            # a referer. This will change in the version in Ubuntu 10.04,
-            # but Launchpad should support HWDB submissions from older
-            # Ubuntu versions during their support period.
-            #
-            # We'll have to keep an application's one-off exception
-            # until the application has been changed to send a
-            # Referer, and until we have no legacy versions of that
-            # application to support. For instance, we can't get rid
-            # of the apport exception until after Lucid's end-of-life
-            # date. We should be able to get rid of the launchpadlib
-            # exception after Karmic's end-of-life date.
-            return
-        referrer = request.getHeader('referer') # match HTTP spec misspelling
-        if not referrer:
-            raise NoReferrerError('No value for REFERER header')
-        # XXX: jamesh 2007-04-26 bug=98437:
-        # The Zope testing infrastructure sets a default (incorrect)
-        # referrer value of "localhost" or "localhost:9000" if no
-        # referrer is included in the request.  We let it pass through
-        # here for the benefits of the tests.  Web browsers send full
-        # URLs so this does not open us up to extra XSRF attacks.
-        if referrer in ['localhost', 'localhost:9000']:
-            return
-        # Extract the hostname from the referrer URI
-        try:
-            hostname = URI(referrer).host
-        except InvalidURIError:
-            hostname = None
-        if hostname not in allvhosts.hostnames:
-            raise OffsiteFormPostError(referrer)
-
     def constructPageID(self, view, context):
         """Given a view, figure out what its page ID should be.
 
 
=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-08-12 21:26:53 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-08-17 16:05:20 +0000
@@ -219,24 +219,6 @@
         view = OpenIDCallbackView(context=None, request=None)
         self.assertRaises(ValueError, view._gather_params, request)
 
-    def test_csrfmiddlewaretoken_is_ignored(self):
-        # Show that the _gather_params filters out the errant
-        # csrfmiddlewaretoken form field.  See comment in _gather_params for
-        # more info.
-        request = LaunchpadTestRequest(
-            SERVER_URL='http://example.com',
-            QUERY_STRING='foo=bar',
-            form={'starting_url': 'http://launchpad.dev/after-login',
-                'csrfmiddlewaretoken': '12345'},
-            environ={'PATH_INFO': '/'})
-        view = OpenIDCallbackView(context=None, request=None)
-        params = view._gather_params(request)
-        expected_params = {
-            'starting_url': 'http://launchpad.dev/after-login',
-            'foo': 'bar',
-        }
-        self.assertEquals(params, expected_params)
-
     def test_get_requested_url(self):
         # The OpenIDCallbackView needs to pass the currently-being-requested
         # URL to the OpenID library.  OpenIDCallbackView._get_requested_url
 
=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2010-05-27 07:53:38 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2010-08-17 16:05:20 +0000
@@ -10,6 +10,7 @@
 import unittest
 
 from contrib.oauth import OAuthRequest, OAuthSignatureMethod_PLAINTEXT
+from zope.interface import directlyProvides, noLongerProvides
 
 from storm.database import STATE_DISCONNECTED, STATE_RECONNECT
 from storm.exceptions import DisconnectionError
@@ -18,20 +19,25 @@
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.publisher.interfaces import Retry
+from zope.publisher.interfaces.browser import IBrowserRequest
 
 from canonical.config import dbconfig
 from canonical.launchpad.database.emailaddress import EmailAddress
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
-from canonical.launchpad.interfaces.oauth import IOAuthConsumerSet
+from canonical.launchpad.interfaces.oauth import (
+    IOAuthConsumerSet, IOAuthSignedRequest)
 from canonical.launchpad.ftests import ANONYMOUS, login
 from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.tests.readonly import (
     remove_read_only_file, touch_read_only_file)
 import canonical.launchpad.webapp.adapter as dbadapter
+from canonical.launchpad.webapp.vhosts import allvhosts
 from canonical.launchpad.webapp.interfaces import (
-    IStoreSelector, MAIN_STORE, MASTER_FLAVOR, OAuthPermission, SLAVE_FLAVOR)
+    IStoreSelector, MAIN_STORE, MASTER_FLAVOR, OAuthPermission, SLAVE_FLAVOR,
+    OffsiteFormPostError, NoReferrerError)
 from canonical.launchpad.webapp.publication import (
-    is_browser, LaunchpadBrowserPublication)
+    is_browser, LaunchpadBrowserPublication, maybe_block_offsite_form_post,
+    OFFSITE_POST_WHITELIST)
 from canonical.launchpad.webapp.servers import (
     LaunchpadTestRequest, WebServicePublication)
 from canonical.testing import DatabaseFunctionalLayer, FunctionalLayer
@@ -312,6 +318,96 @@
         self.assertFalse(is_browser(request))
 
 
+class TestBlockingOffsitePosts(TestCase):
+    """We are very particular about what form POSTs we will accept."""
+
+    def test_NoReferrerError(self):
+        # If this request is a POST and there is no referrer, an exception is
+        # raised.
+        request = LaunchpadTestRequest(
+            method='POST', environ=dict(PATH_INFO='/'))
+        self.assertRaises(
+            NoReferrerError, maybe_block_offsite_form_post, request)
+
+    def test_nonPOST_requests(self):
+        # If the request isn't a POST it is always allowed.
+        request = LaunchpadTestRequest(method='SOMETHING')
+        maybe_block_offsite_form_post(request)
+
+    def test_localhost_is_ok(self):
+        # we accept "localhost" and "localhost:9000" as valid referrers.  See
+        # comments in the code as to why and for a related bug report.
+        request = LaunchpadTestRequest(
+            method='POST', environ=dict(PATH_INFO='/', REFERER='localhost'))
+        # this doesn't raise an exception
+        maybe_block_offsite_form_post(request)
+
+    def test_whitelisted_paths(self):
+        # There are a few whitelisted POST targets that don't require the
+        # referrer be LP.  See comments in the code as to why and for related
+        # bug reports.
+        for path in OFFSITE_POST_WHITELIST:
+            request = LaunchpadTestRequest(
+                method='POST', environ=dict(PATH_INFO=path))
+            # this call shouldn't raise an exception
+            maybe_block_offsite_form_post(request)
+
+    def test_OAuth_signed_requests(self):
+        # Requests that are OAuth signed are allowed.
+        request = LaunchpadTestRequest(
+            method='POST', environ=dict(PATH_INFO='/'))
+        directlyProvides(request, IOAuthSignedRequest)
+        # this call shouldn't raise an exception
+        maybe_block_offsite_form_post(request)
+
+    def test_nonbrowser_requests(self):
+        # Requests that are from non-browsers are allowed.
+        class FakeNonBrowserRequest:
+            method = 'SOMETHING'
+
+        # this call shouldn't raise an exception
+        maybe_block_offsite_form_post(FakeNonBrowserRequest)
+
+    def test_onsite_posts(self):
+        # Other than the explicit execptions, all POSTs have to come from a
+        # known LP virtual host.
+        for hostname in allvhosts.hostnames:
+            referer = 'http://' + hostname + '/foo'
+            request = LaunchpadTestRequest(
+                method='POST', environ=dict(PATH_INFO='/', REFERER=referer))
+            # this call shouldn't raise an exception
+            maybe_block_offsite_form_post(request)
+
+    def test_offsite_posts(self):
+        # If a post comes from an unknown host an execption is raised.
+        disallowed_hosts = ['example.com', 'not-subdomain.launchpad.net']
+        for hostname in disallowed_hosts:
+            referer = 'http://' + hostname + '/foo'
+            request = LaunchpadTestRequest(
+                method='POST', environ=dict(PATH_INFO='/', REFERER=referer))
+            self.assertRaises(
+                OffsiteFormPostError, maybe_block_offsite_form_post, request)
+
+    def test_unparsable_referer(self):
+        # If a post has a referer that is unparsable as a URI an exception is
+        # raised.
+        referer = 'this is not a URI'
+        request = LaunchpadTestRequest(
+            method='POST', environ=dict(PATH_INFO='/', REFERER=referer))
+        self.assertRaises(
+            OffsiteFormPostError, maybe_block_offsite_form_post, request)
+
+
+    def test_openid_callback_with_query_string(self):
+        # An OpenId provider (OP) may post to the +openid-callback URL with a
+        # query string and without a referer.  These posts need to be allowed.
+        path_info = u'/+openid-callback?starting_url=...'
+        request = LaunchpadTestRequest(
+            method='POST', environ=dict(PATH_INFO=path_info))
+        # this call shouldn't raise an exception
+        maybe_block_offsite_form_post(request)
+
+
 def test_suite():
     suite = unittest.TestLoader().loadTestsFromName(__name__)
     return suite