diff --git a/h/security/policy/_api_cookie.py b/h/security/policy/_api_cookie.py index f9076ff4487..3f7dd91c0fd 100644 --- a/h/security/policy/_api_cookie.py +++ b/h/security/policy/_api_cookie.py @@ -1,3 +1,5 @@ +from pyramid.csrf import check_csrf_origin, check_csrf_token +from pyramid.exceptions import BadCSRFOrigin, BadCSRFToken from pyramid.request import Request from pyramid.security import Allowed, Denied from webob.cookies import SignedCookieProfile @@ -28,7 +30,18 @@ def handles(request: Request) -> bool: ) in COOKIE_AUTHENTICATABLE_API_REQUESTS def identity(self, request: Request) -> Identity | None: + try: + check_csrf_origin(request) + except BadCSRFOrigin: + return None + + try: + check_csrf_token(request) + except BadCSRFToken: + return None + self.helper.add_vary_by_cookie(request) + return self.helper.identity(self.cookie, request) def authenticated_userid(self, request: Request) -> str | None: diff --git a/h/static/scripts/group-forms/components/CreateGroupForm.tsx b/h/static/scripts/group-forms/components/CreateGroupForm.tsx index e2c7dd028f6..fa9b2242952 100644 --- a/h/static/scripts/group-forms/components/CreateGroupForm.tsx +++ b/h/static/scripts/group-forms/components/CreateGroupForm.tsx @@ -146,14 +146,14 @@ export default function CreateGroupForm() { setSaving(true); try { - response = (await callAPI( - config.api.createGroup.url, - config.api.createGroup.method, - { + response = (await callAPI(config.api.createGroup.url, { + method: config.api.createGroup.method, + headers: config.api.createGroup.headers, + json: { name, description, }, - )) as CreateGroupAPIResponse; + })) as CreateGroupAPIResponse; } catch (apiError) { setErrorMessage(apiError.message); setSaving(false); diff --git a/h/static/scripts/group-forms/components/test/CreateGroupForm-test.js b/h/static/scripts/group-forms/components/test/CreateGroupForm-test.js index 342cc3aed9d..fd9b6fe22a1 100644 --- a/h/static/scripts/group-forms/components/test/CreateGroupForm-test.js +++ b/h/static/scripts/group-forms/components/test/CreateGroupForm-test.js @@ -8,6 +8,7 @@ const config = { createGroup: { method: 'POST', url: 'https://example.com/api/groups', + headers: { foo: 'bar' }, }, }, }; @@ -209,14 +210,14 @@ describe('CreateGroupForm', () => { await wrapper.find('form[data-testid="form"]').simulate('submit'); assert.isTrue( - fakeCallAPI.calledOnceWithExactly( - config.api.createGroup.url, - config.api.createGroup.method, - { + fakeCallAPI.calledOnceWithExactly(config.api.createGroup.url, { + method: config.api.createGroup.method, + headers: config.api.createGroup.headers, + json: { name, description, }, - ), + }), ); assert.isTrue(fakeSetLocation.calledOnceWithExactly(groupURL)); }); diff --git a/h/static/scripts/group-forms/config.ts b/h/static/scripts/group-forms/config.ts index a0ea13b6dba..771c88a3462 100644 --- a/h/static/scripts/group-forms/config.ts +++ b/h/static/scripts/group-forms/config.ts @@ -1,6 +1,7 @@ export type APIConfig = { method: string; url: string; + headers: object; }; export type ConfigObject = { diff --git a/h/static/scripts/group-forms/utils/api.ts b/h/static/scripts/group-forms/utils/api.ts index c15a4d25fdb..4fee70cc41d 100644 --- a/h/static/scripts/group-forms/utils/api.ts +++ b/h/static/scripts/group-forms/utils/api.ts @@ -45,15 +45,24 @@ export class APIError extends Error { /* Make an API call and return the parsed JSON body or throw APIError. */ export async function callAPI( url: string, - method: string = 'GET', - json: object | undefined, + { + method = 'GET', + json = null, + headers = {}, + }: { + method?: string; + json?: object | null; + headers?: any; + } = {}, ): Promise { + headers['Content-Type'] = 'application/json; charset=UTF-8'; + const options: RequestInit = { - method: method, - headers: { 'Content-Type': 'application/json; charset=UTF-8' }, + method, + headers, }; - if (json !== undefined) { + if (json !== null) { options.body = JSON.stringify(json); } diff --git a/h/static/scripts/group-forms/utils/test/api-test.js b/h/static/scripts/group-forms/utils/test/api-test.js index be1e377c144..a802cc5a2ee 100644 --- a/h/static/scripts/group-forms/utils/test/api-test.js +++ b/h/static/scripts/group-forms/utils/test/api-test.js @@ -37,7 +37,7 @@ describe('callAPI', () => { for (const method of ['GET', 'POST', 'PUT', 'PATCH', 'DELETE']) { context(`when the request method is ${method}`, () => { it('makes a request using the given method', async () => { - await callAPI(url, method); + await callAPI(url, { method }); assert.equal(fakeFetch.lastCall.args[1].method, method); }); @@ -47,11 +47,20 @@ describe('callAPI', () => { it('makes a request with the given JSON body', async () => { const json = { foo: 'bar' }; - await callAPI(url, 'POST', json); + await callAPI(url, { method: 'POST', json }); assert.equal(fakeFetch.lastCall.args[1].body, JSON.stringify(json)); }); + it('makes a request with the given headers', async () => { + const headers = { foo: 'bar' }; + + await callAPI(url, { method: 'POST', headers }); + + headers['Content-Type'] = 'application/json; charset=UTF-8'; + assert.equal(fakeFetch.lastCall.args[1].headers, headers); + }); + it('returns the parsed JSON object', async () => { const result = await callAPI(url); diff --git a/h/templates/groups/create.html.jinja2 b/h/templates/groups/create.html.jinja2 index f2fa60c850d..1d95d03a735 100644 --- a/h/templates/groups/create.html.jinja2 +++ b/h/templates/groups/create.html.jinja2 @@ -10,7 +10,10 @@ "api": { "createGroup": { "method": "POST", - "url": "{{ request.route_url("api.groups") }}" + "url": "{{ request.route_url("api.groups") }}", + "headers": { + "X-CSRF-Token": "{{ get_csrf_token() }}" + } } } } diff --git a/tests/unit/h/conftest.py b/tests/unit/h/conftest.py index 08dfe320048..34d65580237 100644 --- a/tests/unit/h/conftest.py +++ b/tests/unit/h/conftest.py @@ -168,6 +168,8 @@ def pyramid_request(db_session, db_session_replica, fake_feature, pyramid_settin def pyramid_csrf_request(pyramid_request): """Return a dummy Pyramid request object with a valid CSRF token.""" pyramid_request.headers["X-CSRF-Token"] = pyramid_request.session.get_csrf_token() + pyramid_request.referrer = "https://example.com" + pyramid_request.host_port = "80" return pyramid_request diff --git a/tests/unit/h/security/policy/_api_cookie_test.py b/tests/unit/h/security/policy/_api_cookie_test.py index 7843f88e160..7904dddd9b5 100644 --- a/tests/unit/h/security/policy/_api_cookie_test.py +++ b/tests/unit/h/security/policy/_api_cookie_test.py @@ -1,6 +1,7 @@ from unittest.mock import create_autospec, sentinel import pytest +from pyramid.csrf import SessionCSRFStoragePolicy from h.security.policy._api_cookie import APICookiePolicy from h.security.policy.helpers import AuthTicketCookieHelper @@ -17,41 +18,55 @@ class TestAPICookiePolicy: ], ) def test_handles( - self, pyramid_request, route_name, request_method, expected_result + self, pyramid_csrf_request, route_name, request_method, expected_result ): - pyramid_request.matched_route.name = route_name - pyramid_request.method = request_method + pyramid_csrf_request.matched_route.name = route_name + pyramid_csrf_request.method = request_method - assert APICookiePolicy.handles(pyramid_request) == expected_result + assert APICookiePolicy.handles(pyramid_csrf_request) == expected_result - def test_identity(self, api_cookie_policy, helper, pyramid_request): - identity = api_cookie_policy.identity(pyramid_request) + def test_identity(self, api_cookie_policy, helper, pyramid_csrf_request): + identity = api_cookie_policy.identity(pyramid_csrf_request) - helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.identity.assert_called_once_with(sentinel.cookie, pyramid_request) + helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request) + helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request) assert identity == helper.identity.return_value + def test_identity_with_wrong_origin(self, api_cookie_policy, pyramid_csrf_request): + pyramid_csrf_request.referrer = "https://evil.com" + + assert api_cookie_policy.identity(pyramid_csrf_request) is None + + def test_identity_with_wrong_csrf_token( + self, api_cookie_policy, pyramid_csrf_request + ): + del pyramid_csrf_request.headers["X-CSRF-Token"] + + assert api_cookie_policy.identity(pyramid_csrf_request) is None + def test_authenticated_userid( - self, api_cookie_policy, helper, pyramid_request, Identity + self, api_cookie_policy, helper, pyramid_csrf_request, Identity ): - authenticated_userid = api_cookie_policy.authenticated_userid(pyramid_request) + authenticated_userid = api_cookie_policy.authenticated_userid( + pyramid_csrf_request + ) - helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.identity.assert_called_once_with(sentinel.cookie, pyramid_request) + helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request) + helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request) Identity.authenticated_userid.assert_called_once_with( helper.identity.return_value ) assert authenticated_userid == Identity.authenticated_userid.return_value def test_permits( - self, api_cookie_policy, helper, pyramid_request, identity_permits + self, api_cookie_policy, helper, pyramid_csrf_request, identity_permits ): permits = api_cookie_policy.permits( - pyramid_request, sentinel.context, sentinel.permission + pyramid_csrf_request, sentinel.context, sentinel.permission ) - helper.add_vary_by_cookie.assert_called_once_with(pyramid_request) - helper.identity.assert_called_once_with(sentinel.cookie, pyramid_request) + helper.add_vary_by_cookie.assert_called_once_with(pyramid_csrf_request) + helper.identity.assert_called_once_with(sentinel.cookie, pyramid_csrf_request) identity_permits.assert_called_once_with( helper.identity.return_value, sentinel.context, sentinel.permission ) @@ -78,3 +93,9 @@ def identity_permits(mocker): return mocker.patch( "h.security.policy._api_cookie.identity_permits", autospec=True, spec_set=True ) + + +@pytest.fixture(autouse=True) +def pyramid_config(pyramid_config): + pyramid_config.set_csrf_storage_policy(SessionCSRFStoragePolicy()) + return pyramid_config