Only Accept CSRF Tokens in headers or POST bodies

Previously `check_csrf_token` would allow passing in a CSRF token in through a
the URL of a request. However this is a security issue because a CSRF token
must not be allowed to leak, and URLs regularly get copy/pasted or otherwise
end up leaking to the outside world.

6 files changed, 43 insertions, 24 deletions

diff --git a/docs/narr/sessions.rst b/docs/narr/sessions.rst
index d66e86258..ad086268b 100644
--- a/docs/narr/sessions.rst
+++ b/docs/narr/sessions.rst
@@ -391,8 +391,8 @@ will return ``True``, otherwise it will raise ``HTTPBadRequest``. Optionally,
 you can specify ``raises=False`` to have the check return ``False`` instead of
 raising an exception.
 
-By default, it checks for a GET or POST parameter named ``csrf_token`` or a
-header named ``X-CSRF-Token``.
+By default, it checks for a POST parameter named ``csrf_token`` or a header
+named ``X-CSRF-Token``.
 
 .. code-block:: python
 
@@ -430,8 +430,7 @@ If ``require_csrf`` is ``True`` but does not explicitly define a token to
 check, then the token name is pulled from whatever was set in the
 ``pyramid.require_default_csrf`` setting. Finally, if that setting does not
 explicitly define a token, then ``csrf_token`` is the token required. This token
-name will be required in ``request.params`` which is a combination of the
-query string and a submitted form body.
+name will be required in ``request.POST`` which is the submitted form body.
 
 It is always possible to pass the token in the ``X-CSRF-Token`` header as well.
 There is currently no way to define an alternate name for this header without
diff --git a/docs/narr/viewconfig.rst b/docs/narr/viewconfig.rst
index e645185f5..40db5fbeb 100644
--- a/docs/narr/viewconfig.rst
+++ b/docs/narr/viewconfig.rst
@@ -459,7 +459,7 @@ configured view.
   check name.
 
   If CSRF checking is performed, the checked value will be the value of
-  ``request.params[check_name]``.  This value will be compared against the
+  ``request.POST[check_name]``.  This value will be compared against the
   value of ``request.session.get_csrf_token()``, and the check will pass if
   these two values are the same.  If the check passes, the associated view will
   be permitted to execute.  If the check fails, the associated view will not be
diff --git a/pyramid/session.py b/pyramid/session.py
index fd7b5f8d5..6136e26a0 100644
--- a/pyramid/session.py
+++ b/pyramid/session.py
@@ -106,13 +106,14 @@ def check_csrf_token(request,
                      header='X-CSRF-Token',
                      raises=True):
     """ Check the CSRF token in the request's session against the value in
-    ``request.params.get(token)`` or ``request.headers.get(header)``.
-    If a ``token`` keyword is not supplied to this function, the string
-    ``csrf_token`` will be used to look up the token in ``request.params``.
-    If a ``header`` keyword is not supplied to this function, the string
-    ``X-CSRF-Token`` will be used to look up the token in ``request.headers``.
-
-    If the value supplied by param or by header doesn't match the value
+    ``request.POST.get(token)`` (if a POST request) or
+    ``request.headers.get(header)``. If a ``token`` keyword is not supplied to
+    this function, the string ``csrf_token`` will be used to look up the token
+    in ``request.POST``. If a ``header`` keyword is not supplied to this
+    function, the string ``X-CSRF-Token`` will be used to look up the token in
+    ``request.headers``.
+
+    If the value supplied by post or by header doesn't match the value
     supplied by ``request.session.get_csrf_token()``, and ``raises`` is
     ``True``, this function will raise an
     :exc:`pyramid.exceptions.BadCSRFToken` exception.
@@ -128,7 +129,18 @@ def check_csrf_token(request,
 
     .. versionadded:: 1.4a2
     """
-    supplied_token = request.params.get(token, request.headers.get(header, ""))
+    # If this is a POST/PUT/etc request, then we'll check the body to see if it
+    # has a token. We explicitly use request.POST here because CSRF tokens
+    # should never appear in an URL as doing so is a security issue. We also
+    # explicitly check for request.POST here as we do not support sending form
+    # encoded data over anything but a request.POST.
+    supplied_token = request.POST.get(token, "")
+
+    # If we were unable to locate a CSRF token in a request body, then we'll
+    # check to see if there are any headers that have a value for us.
+    if supplied_token == "":
+        supplied_token = request.headers.get(header, "")
+
     expected_token = request.session.get_csrf_token()
     if strings_differ(bytes_(expected_token), bytes_(supplied_token)):
         if raises:
diff --git a/pyramid/tests/test_config/test_views.py b/pyramid/tests/test_config/test_views.py
index 0bf0bd0b3..7be72257d 100644
--- a/pyramid/tests/test_config/test_views.py
+++ b/pyramid/tests/test_config/test_views.py
@@ -1502,8 +1502,9 @@ class TestViewsConfigurationMixin(unittest.TestCase):
             self.assertEqual(len(w), 1)
         wrapper = self._getViewCallable(config)
         request = self._makeRequest(config)
+        request.method = "POST"
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params = {'csrf_token': 'foo'}
+        request.POST = {'csrf_token': 'foo'}
         request.headers = {}
         self.assertEqual(wrapper(None, request), 'OK')
 
@@ -1595,7 +1596,7 @@ class TestViewsConfigurationMixin(unittest.TestCase):
         view = self._getViewCallable(config)
         request = self._makeRequest(config)
         request.method = 'POST'
-        request.params = {'st': 'foo'}
+        request.POST = {'st': 'foo'}
         request.headers = {}
         request.session = DummySession({'csrf_token': 'foo'})
         self.assertEqual(view(None, request), 'OK')
@@ -1609,6 +1610,7 @@ class TestViewsConfigurationMixin(unittest.TestCase):
         view = self._getViewCallable(config)
         request = self._makeRequest(config)
         request.method = 'POST'
+        request.POST = {}
         request.headers = {'X-CSRF-Token': 'foo'}
         request.session = DummySession({'csrf_token': 'foo'})
         self.assertEqual(view(None, request), 'OK')
@@ -1622,6 +1624,7 @@ class TestViewsConfigurationMixin(unittest.TestCase):
         view = self._getViewCallable(config)
         request = self._makeRequest(config)
         request.method = 'POST'
+        request.POST = {}
         request.headers = {}
         request.session = DummySession({'csrf_token': 'foo'})
         self.assertRaises(BadCSRFToken, lambda: view(None, request))
diff --git a/pyramid/tests/test_session.py b/pyramid/tests/test_session.py
index 914d28a83..4ec8f94a4 100644
--- a/pyramid/tests/test_session.py
+++ b/pyramid/tests/test_session.py
@@ -666,7 +666,8 @@ class Test_check_csrf_token(unittest.TestCase):
 
     def test_success_token(self):
         request = testing.DummyRequest()
-        request.params['csrf_token'] = request.session.get_csrf_token()
+        request.method = "POST"
+        request.POST = {'csrf_token': request.session.get_csrf_token()}
         self.assertEqual(self._callFUT(request, token='csrf_token'), True)
 
     def test_success_header(self):
@@ -676,7 +677,8 @@ class Test_check_csrf_token(unittest.TestCase):
 
     def test_success_default_token(self):
         request = testing.DummyRequest()
-        request.params['csrf_token'] = request.session.get_csrf_token()
+        request.method = "POST"
+        request.POST = {'csrf_token': request.session.get_csrf_token()}
         self.assertEqual(self._callFUT(request), True)
 
     def test_success_default_header(self):
@@ -698,8 +700,9 @@ class Test_check_csrf_token(unittest.TestCase):
     def test_token_differing_types(self):
         from pyramid.compat import text_
         request = testing.DummyRequest()
+        request.method = "POST"
         request.session['_csrft_'] = text_('foo')
-        request.params['csrf_token'] = b'foo'
+        request.POST = {'csrf_token': b'foo'}
         self.assertEqual(self._callFUT(request, token='csrf_token'), True)
 
 class DummySerializer(object):
diff --git a/pyramid/tests/test_viewderivers.py b/pyramid/tests/test_viewderivers.py
index c8fbe6f36..bd5b3f2de 100644
--- a/pyramid/tests/test_viewderivers.py
+++ b/pyramid/tests/test_viewderivers.py
@@ -1120,6 +1120,7 @@ class TestDeriveView(unittest.TestCase):
             return response
         request = self._makeRequest()
         request.method = 'POST'
+        request.POST = {}
         request.session = DummySession({'csrf_token': 'foo'})
         request.headers = {'X-CSRF-Token': 'foo'}
         view = self.config._derive_view(inner_view, require_csrf=True)
@@ -1133,7 +1134,7 @@ class TestDeriveView(unittest.TestCase):
         request = self._makeRequest()
         request.method = 'POST'
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params['DUMMY'] = 'foo'
+        request.POST = {'DUMMY': 'foo'}
         view = self.config._derive_view(inner_view, require_csrf='DUMMY')
         result = view(None, request)
         self.assertTrue(result is response)
@@ -1154,7 +1155,7 @@ class TestDeriveView(unittest.TestCase):
         request = self._makeRequest()
         request.method = 'POST'
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params['DUMMY'] = 'bar'
+        request.POST = {'DUMMY': 'bar'}
         view = self.config._derive_view(inner_view, require_csrf='DUMMY')
         self.assertRaises(BadCSRFToken, lambda: view(None, request))
 
@@ -1163,6 +1164,7 @@ class TestDeriveView(unittest.TestCase):
         def inner_view(request): pass
         request = self._makeRequest()
         request.method = 'POST'
+        request.POST = {}
         request.session = DummySession({'csrf_token': 'foo'})
         request.headers = {'X-CSRF-Token': 'bar'}
         view = self.config._derive_view(inner_view, require_csrf='DUMMY')
@@ -1175,7 +1177,7 @@ class TestDeriveView(unittest.TestCase):
         request = self._makeRequest()
         request.method = 'POST'
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params['csrf_token'] = 'foo'
+        request.POST = {'csrf_token': 'foo'}
         self.config.add_settings({'pyramid.require_default_csrf': 'yes'})
         view = self.config._derive_view(inner_view)
         result = view(None, request)
@@ -1188,7 +1190,7 @@ class TestDeriveView(unittest.TestCase):
         request = self._makeRequest()
         request.method = 'POST'
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params['DUMMY'] = 'foo'
+        request.POST = {'DUMMY': 'foo'}
         self.config.add_settings({'pyramid.require_default_csrf': 'DUMMY'})
         view = self.config._derive_view(inner_view)
         result = view(None, request)
@@ -1214,7 +1216,7 @@ class TestDeriveView(unittest.TestCase):
         request = self._makeRequest()
         request.method = 'POST'
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params['DUMMY'] = 'foo'
+        request.POST = {'DUMMY': 'foo'}
         self.config.add_settings({'pyramid.require_default_csrf': 'yes'})
         view = self.config._derive_view(inner_view, require_csrf='DUMMY')
         result = view(None, request)
@@ -1227,7 +1229,7 @@ class TestDeriveView(unittest.TestCase):
         request = self._makeRequest()
         request.method = 'POST'
         request.session = DummySession({'csrf_token': 'foo'})
-        request.params['DUMMY'] = 'foo'
+        request.POST = {'DUMMY': 'foo'}
         self.config.add_settings({'pyramid.require_default_csrf': 'DUMMY'})
         view = self.config._derive_view(inner_view, require_csrf=True)
         result = view(None, request)