From 770b439c8495c3a0b16550c2f04740f31646d66b Mon Sep 17 00:00:00 2001 From: Roald de Vries Date: Thu, 1 Dec 2016 10:36:29 +0100 Subject: WIP: add csrf token to every request --- service/pixelated/resources/__init__.py | 2 +- service/pixelated/resources/inbox_resource.py | 1 - service/pixelated/resources/root_resource.py | 6 +-- service/test/integration/test_delete_mail.py | 5 ++- service/test/integration/test_logout.py | 4 +- .../test/support/integration/app_test_client.py | 4 +- .../test/support/integration/multi_user_client.py | 8 +++- service/test/unit/resources/test_inbox_resource.py | 26 ------------ service/test/unit/resources/test_root_resource.py | 48 ++++++++++++++++------ 9 files changed, 55 insertions(+), 49 deletions(-) diff --git a/service/pixelated/resources/__init__.py b/service/pixelated/resources/__init__.py index 97346a6f..023758de 100644 --- a/service/pixelated/resources/__init__.py +++ b/service/pixelated/resources/__init__.py @@ -66,7 +66,7 @@ class BaseResource(Resource): self._services_factory = services_factory def _add_csrf_cookie(self, request): - csrf_token = hashlib.sha256(os.urandom(CSRF_TOKEN_LENGTH)).hexdigest() + csrf_token = IPixelatedSession(request.getSession()).get_csrf_token() request.addCookie('XSRF-TOKEN', csrf_token) log.debug('XSRF-TOKEN added: %s' % csrf_token) diff --git a/service/pixelated/resources/inbox_resource.py b/service/pixelated/resources/inbox_resource.py index 47a3c072..f759dca9 100644 --- a/service/pixelated/resources/inbox_resource.py +++ b/service/pixelated/resources/inbox_resource.py @@ -53,7 +53,6 @@ class InboxResource(BaseResource): def render_GET(self, request): logger.debug('Inbox rendering GET. %s' % self) - self._add_csrf_cookie(request) if self._is_starting(): logger.debug('Inbox rendering interstitial. %s' % self) return self.interstitial diff --git a/service/pixelated/resources/root_resource.py b/service/pixelated/resources/root_resource.py index 7d5b0b0a..1d32935b 100644 --- a/service/pixelated/resources/root_resource.py +++ b/service/pixelated/resources/root_resource.py @@ -65,6 +65,7 @@ class RootResource(BaseResource): logger.debug('Root in STARTUP mode. %s' % self) def getChildWithDefault(self, path, request): + self._add_csrf_cookie(request) if path == '': return self._redirect_to_login_resource if self._public else self._inbox_resource if self._mode == MODE_STARTUP: @@ -81,7 +82,6 @@ class RootResource(BaseResource): xsrf_token = request.getCookie('XSRF-TOKEN') logger.debug('CSRF token: %s' % xsrf_token) - # TODO: how is comparing the cookie-csrf with the HTTP-header-csrf adding any csrf protection? ajax_request = (request.getHeader('x-requested-with') == 'XMLHttpRequest') if ajax_request: xsrf_header = request.getHeader('x-xsrf-token') @@ -101,7 +101,7 @@ class RootResource(BaseResource): return self.putChildProtected(path, resource) # to be on the safe side def initialize(self, provider=None, disclaimer_banner=None, authenticator=None): - self.putChildProtected('sandbox', SandboxResource(self._static_folder)) + self.putChildPublic('sandbox', SandboxResource(self._static_folder)) self.putChildProtected('keys', KeysResource(self._services_factory)) self.putChildProtected(AttachmentsResource.BASE_URL, AttachmentsResource(self._services_factory)) self.putChildProtected('contacts', ContactsResource(self._services_factory)) @@ -114,7 +114,7 @@ class RootResource(BaseResource): self.putChildProtected('users', UsersResource(self._services_factory)) self.putChildPublic(LoginResource.BASE_URL, LoginResource(self._services_factory, provider, disclaimer_banner=disclaimer_banner, authenticator=authenticator)) - self.putChildProtected(LogoutResource.BASE_URL, LogoutResource(self._services_factory)) + self.putChildPublic(LogoutResource.BASE_URL, LogoutResource(self._services_factory)) self._inbox_resource.initialize() self._mode = MODE_RUNNING diff --git a/service/test/integration/test_delete_mail.py b/service/test/integration/test_delete_mail.py index a912f9f0..6cb9ceb6 100644 --- a/service/test/integration/test_delete_mail.py +++ b/service/test/integration/test_delete_mail.py @@ -15,6 +15,7 @@ # along with Pixelated. If not, see . from twisted.internet import defer from test.support.integration import SoledadTestBase, MailBuilder +from pixelated.resources import IPixelatedSession class DeleteMailTest(SoledadTestBase): @@ -27,7 +28,9 @@ class DeleteMailTest(SoledadTestBase): inbox_mails = yield self.app_test_client.get_mails_by_tag('inbox') self.assertEquals(1, len(inbox_mails)) - yield self.app_test_client.delete_mail(mail.mail_id) + response, first_request = yield self.app_test_client.get('/', as_json=False) + csrftoken = IPixelatedSession(first_request.getSession()).get_csrf_token() + yield self.app_test_client.delete_mail(mail.mail_id, csrf=csrftoken) inbox_mails = yield self.app_test_client.get_mails_by_tag('inbox') self.assertEquals(0, len(inbox_mails)) diff --git a/service/test/integration/test_logout.py b/service/test/integration/test_logout.py index c9d39d17..b4f8ebf3 100644 --- a/service/test/integration/test_logout.py +++ b/service/test/integration/test_logout.py @@ -29,7 +29,8 @@ class MultiUserLogoutTest(MultiUserSoledadTestBase): @defer.inlineCallbacks def test_logout_deletes_services_stop_background_reactor_tasks_and_closes_soledad(self): - response, login_request = yield self.app_test_client.login() + response, first_request = yield self.app_test_client.get('/login', as_json=False) + response, login_request = yield self.app_test_client.login(from_request=first_request) yield response yield self.wait_for_session_user_id_to_finish() @@ -37,6 +38,7 @@ class MultiUserLogoutTest(MultiUserSoledadTestBase): response, request = self.app_test_client.post( "/logout", json.dumps({'csrftoken': [login_request.getCookie('XSRF-TOKEN')]}), + ajax=False, from_request=login_request, as_json=False) yield response diff --git a/service/test/support/integration/app_test_client.py b/service/test/support/integration/app_test_client.py index d52c85c0..ee5a1df2 100644 --- a/service/test/support/integration/app_test_client.py +++ b/service/test/support/integration/app_test_client.py @@ -387,8 +387,8 @@ class AppTestClient(object): return res # TODO: remove - def delete_mail(self, mail_ident): - res, req = self.delete("/mail/%s" % mail_ident) + def delete_mail(self, mail_ident, csrf='token'): + res, req = self.delete("/mail/%s" % mail_ident, csrf=csrf) return res def delete_mails(self, idents): diff --git a/service/test/support/integration/multi_user_client.py b/service/test/support/integration/multi_user_client.py index 82acb210..fe8595fb 100644 --- a/service/test/support/integration/multi_user_client.py +++ b/service/test/support/integration/multi_user_client.py @@ -24,6 +24,7 @@ from pixelated.config.services import ServicesFactory from pixelated.config.sessions import LeapSessionFactory import pixelated.config.services +from pixelated.resources import IPixelatedSession from pixelated.resources.root_resource import RootResource from test.support.integration import AppTestClient from test.support.integration.app_test_client import AppTestAccount, StubSRPChecker @@ -57,7 +58,7 @@ class MultiUserClient(AppTestClient): else: when(Authenticator)._bonafide_auth(username, password).thenRaise(SRPAuthError) - def login(self, username='username', password='password'): + def login(self, username='username', password='password', from_request=None): session = Authentication(username, 'some_user_token', 'some_user_uuid', 'session_id', {'is_admin': False}) leap_session = self._test_account.leap_session leap_session.user_auth = session @@ -76,7 +77,10 @@ class MultiUserClient(AppTestClient): when(leap_session).initial_sync().thenAnswer(lambda: defer.succeed(None)) when(pixelated.config.services).Services(ANY()).thenReturn(self.services) - request = request_mock(path='/login', method="POST", body={'username': username, 'password': password}) + session = from_request.getSession() + csrftoken = IPixelatedSession(session).get_csrf_token() + request = request_mock(path='/login', method="POST", body={'username': username, 'password': password, 'csrftoken': csrftoken}, ajax=False) + request.session = session return self._render(request, as_json=False) def get(self, path, get_args='', as_json=True, from_request=None): diff --git a/service/test/unit/resources/test_inbox_resource.py b/service/test/unit/resources/test_inbox_resource.py index 03fe6f1a..9af355ca 100644 --- a/service/test/unit/resources/test_inbox_resource.py +++ b/service/test/unit/resources/test_inbox_resource.py @@ -44,29 +44,3 @@ class TestInboxResource(unittest.TestCase): d.addCallback(assert_response) return d - - def _test_should_renew_xsrf_cookie(self): - request = DummyRequest(['']) - request.addCookie = MagicMock() - generated_csrf_token = 'csrf_token' - mock_sha = MagicMock() - mock_sha.hexdigest = MagicMock(return_value=generated_csrf_token) - - with patch('hashlib.sha256', return_value=mock_sha): - d = self.web.get(request) - - def assert_csrf_cookie(_): - request.addCookie.assert_called_once_with('XSRF-TOKEN', generated_csrf_token) - - d.addCallback(assert_csrf_cookie) - return d - - # TODO should this be here or just in the root resource test? - def test_should_renew_xsrf_cookie_on_startup_mode(self): - self.inbox_resource._mode = MODE_STARTUP - self._test_should_renew_xsrf_cookie() - - # TODO should this be here or just in the root resource test? - def test_should_renew_xsrf_cookie_on_running_mode(self): - self.inbox_resource._mode = MODE_RUNNING - self._test_should_renew_xsrf_cookie() diff --git a/service/test/unit/resources/test_root_resource.py b/service/test/unit/resources/test_root_resource.py index b674103c..2dfe3e5a 100644 --- a/service/test/unit/resources/test_root_resource.py +++ b/service/test/unit/resources/test_root_resource.py @@ -6,7 +6,7 @@ from mockito import mock, when, any as ANY import pixelated from pixelated.application import UserAgentMode -from pixelated.resources import UnAuthorizedResource +from pixelated.resources import IPixelatedSession, UnAuthorizedResource from pixelated.resources.features_resource import FeaturesResource from pixelated.resources.login_resource import LoginResource from test.unit.resources import DummySite @@ -30,7 +30,7 @@ class TestPublicRootResource(unittest.TestCase): url_fragment, resource_mock = 'some-url-fragment', mock() self.public_root_resource.putChildPublic(url_fragment, resource_mock) request = DummyRequest([url_fragment]) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.public_root_resource, request) self.assertIs(child_resource, resource_mock) @@ -39,7 +39,7 @@ class TestPublicRootResource(unittest.TestCase): url_fragment, resource_mock = 'some-url-fragment', mock() self.public_root_resource.putChildProtected(url_fragment, resource_mock) request = DummyRequest([url_fragment]) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.public_root_resource, request) self.assertIsInstance(child_resource, UnAuthorizedResource) @@ -48,14 +48,14 @@ class TestPublicRootResource(unittest.TestCase): url_fragment, resource_mock = 'some-url-fragment', mock() self.public_root_resource.putChild(url_fragment, resource_mock) request = DummyRequest([url_fragment]) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.public_root_resource, request) self.assertIsInstance(child_resource, UnAuthorizedResource) def test_private_resource_returns_401(self): self.public_root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['mails']) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') d = self.web.get(request) def assert_unauthorized(request): @@ -68,14 +68,14 @@ class TestPublicRootResource(unittest.TestCase): def test_login_url_should_delegate_to_login_resource(self): self.public_root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['login']) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.public_root_resource, request) self.assertIsInstance(child_resource, LoginResource) def test_root_url_should_redirect_to_login_resource(self): self.public_root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['']) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') d = self.web.get(request) def assert_redirect(request): @@ -107,7 +107,7 @@ class TestRootResource(unittest.TestCase): url_fragment, resource_mock = 'some-url-fragment', mock() self.root_resource.putChildProtected(url_fragment, resource_mock) request = DummyRequest([url_fragment]) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.root_resource, request) self.assertIs(child_resource, resource_mock) @@ -116,13 +116,13 @@ class TestRootResource(unittest.TestCase): url_fragment, resource_mock = 'some-url-fragment', mock() self.root_resource.putChild(url_fragment, resource_mock) request = DummyRequest([url_fragment]) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.root_resource, request) self.assertIs(child_resource, resource_mock) def test_root_url_should_delegate_to_inbox(self): request = DummyRequest(['']) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.root_resource, request) self.assertIsInstance(child_resource, InboxResource) @@ -130,13 +130,13 @@ class TestRootResource(unittest.TestCase): def test_login_url_should_delegate_to_login_resource(self, *mocks): self.root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['login']) - request.addCookie = lambda key, value: 'stubbed' + request.addCookie = MagicMock(return_value='stubbed') child_resource = getChildForRequest(self.root_resource, request) self.assertIsInstance(child_resource, LoginResource) def _test_should_renew_xsrf_cookie(self): request = DummyRequest(['']) - request.addCookie = MagicMock() + request.addCookie = MagicMock(return_value='stubbed') generated_csrf_token = 'csrf_token' mock_sha = MagicMock() mock_sha.hexdigest = MagicMock(return_value=generated_csrf_token) @@ -162,6 +162,7 @@ class TestRootResource(unittest.TestCase): self.root_resource._mode = MODE_STARTUP request = DummyRequest(['/child']) + request.addCookie = MagicMock(return_value='stubbed') request.getCookie = MagicMock(return_value='irrelevant -- stubbed') d = self.web.get(request) @@ -182,6 +183,7 @@ class TestRootResource(unittest.TestCase): self.root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['/child']) + request.addCookie = MagicMock(return_value='stubbed') request.method = 'POST' self._mock_ajax_csrf(request, 'stubbed csrf token') @@ -198,6 +200,7 @@ class TestRootResource(unittest.TestCase): def test_GET_should_return_503_for_uninitialized_resource(self): request = DummyRequest(['/sandbox/']) + request.addCookie = MagicMock(return_value='stubbed') request.method = 'GET' request.getCookie = MagicMock(return_value='stubbed csrf token') @@ -215,6 +218,7 @@ class TestRootResource(unittest.TestCase): self.root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['non-existing-child']) + request.addCookie = MagicMock(return_value='stubbed') request.method = 'GET' request.getCookie = MagicMock(return_value='stubbed csrf token') @@ -231,6 +235,7 @@ class TestRootResource(unittest.TestCase): self.root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['non-existing-child']) + request.addCookie = MagicMock(return_value='stubbed') request.method = 'POST' self._mock_ajax_csrf(request, 'stubbed csrf token') request.getCookie = MagicMock(return_value='stubbed csrf token') @@ -246,6 +251,7 @@ class TestRootResource(unittest.TestCase): def test_should_authorize_child_resource_non_ajax_GET_requests(self): request = DummyRequest(['features']) + request.addCookie = MagicMock(return_value='stubbed') request.getCookie = MagicMock(return_value='irrelevant -- stubbed') self.root_resource.putChild('features', FeaturesResource()) @@ -270,6 +276,7 @@ class TestRootResource(unittest.TestCase): mock_content.read = MagicMock(return_value={}) request.content = mock_content + request.addCookie = MagicMock(return_value='stubbed') request.getCookie = MagicMock(return_value='mismatched csrf token') d = self.web.get(request) @@ -286,6 +293,7 @@ class TestRootResource(unittest.TestCase): self.root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['assets', 'dummy.json']) + request.addCookie = MagicMock(return_value='stubbed') d = self.web.get(request) def assert_response(_): @@ -299,6 +307,7 @@ class TestRootResource(unittest.TestCase): self.root_resource.initialize(provider=mock(), authenticator=mock()) request = DummyRequest(['login']) + request.addCookie = MagicMock(return_value='stubbed') d = self.web.get(request) def assert_response(_): @@ -309,6 +318,7 @@ class TestRootResource(unittest.TestCase): def test_root_should_be_handled_by_inbox_resource(self): request = DummyRequest([]) + request.addCookie = MagicMock(return_value='stubbed') request.prepath = [''] request.path = '/' # TODO: setup mocked portal @@ -318,9 +328,23 @@ class TestRootResource(unittest.TestCase): def test_inbox_should_not_be_public(self): request = DummyRequest([]) + request.addCookie = MagicMock(return_value='stubbed') request.prepath = [''] request.path = '/' # TODO: setup mocked portal resource = self.root_resource.getChildWithDefault(request.prepath[-1], request) self.assertIsInstance(resource, InboxResource) + + def test_every_url_should_get_csrftoken_header(self): + # self.root_resource.initialize(provider=mock(), authenticator=mock()) + request = DummyRequest(['any']) + request.addCookie = MagicMock(return_value='stubbed') + d = self.web.get(request) + + def assert_add_cookie_called_for_csrftoken(request): + csrftoken = IPixelatedSession(request.getSession()).get_csrf_token() + self.assertEqual([(('XSRF-TOKEN', csrftoken),)], request.addCookie.call_args_list) + + d.addCallback(assert_add_cookie_called_for_csrftoken) + return d -- cgit v1.2.3