launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00635
[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
