← Back to team overview

launchpad-reviewers team mailing list archive

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

 

Benji York has proposed merging lp:~benji/launchpad/bug-597324 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #597324 Login fails when logging in while accessing URL with query parameters 
  https://bugs.launchpad.net/bugs/597324
  #609029 UnicodeDecodeError OOPS on login on edge
  https://bugs.launchpad.net/bugs/609029


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)

-- 
https://code.launchpad.net/~benji/launchpad/bug-597324/+merge/32884
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-597324 into lp:launchpad.
=== 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 15:13:21 +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 15:13:21 +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 appears to be a XSRF 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 15:13:21 +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 15:13:21 +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,97 @@
         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))
+        # this call shouldn't raise an exception
+        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