Implement endpoint csrf protection #2057

martijn-tao · 2019-03-25T15:08:32Z

This PR now contains all the work for:
https://oat-sa.atlassian.net/browse/TAO-7304
https://oat-sa.atlassian.net/browse/TAO-7305
https://oat-sa.atlassian.net/browse/TAO-7306

Description

7304 (Backend part):

Added a new Token model that provides a token object
Token service can now generate a pool of tokens that will be used by the frontend
Validation of an endpoint is done by adding a try { $this->validateCsrf(); } catch(\common_exception_Unauthorized $e) { ... } block
CSRF tokens in forms use a hidden element. This gets added by passing [FormContainer::CSRF_PROTECTION_OPTION => true] to the form options.
Since the csrf for forms is added as an element, validation is part of the form validation, meaning that a simple $form->isValid() check offers protection.
A detailed log entry will be added if CSRF validation fails.

7305 (Frontend part):

added core/request module, mixing old functionality of core/dataProvider/request and qtiServiceProxy
converted core/dataProvider/request to thin wrapper returning the response body
converted taoQtiTest/runner/proxy/qtiServiceProxy to work with this
converted core/tokenHandler to manage a store of multiple tokens (fully async, Promise-based)
added core/tokenStore to encapsulate the store functionality (fully async, Promise-based)
used the core/store backend to ensure a single source of truth for the token pool
async unit tests for all these modules

7306 (Implementation in TAO):

Backend token pool changed to allow Instance Forms to keep their old method of protection (single token rendered by PHP as hidden input field)
Removed cookie-based token implementation, and switched to new core/request with tokens, for the following endpoints:

tao/Users/delete
tao/Users/lockUser
tao/Users/unlockUser
tao/RdfController/addInstance
tao/RdfController/deleteAll
tao/RdfController/deleteClass
tao/RdfController/deleteResource
taoGroups/Groups/addSubClass
taoGroups/Groups/cloneInstance
taoGroups/Groups/delete (deleteResource)
taoTestTaker/TestTaker/delete (deleteResource)
taoItems/QtiCreator/createItem

A more comprehensive view of protected forms and endpoints is being documented here:
https://docs.google.com/spreadsheets/d/1TaXQDKcemsIjbWPzCvzyLsU1_EyH8MJ3vKW_-r4iZz0

To test:

Checkout all branches
Use TAO BackOffice extensively, creating and deleting Items, Groups, Test-Takers, Users, etc.
Try to re-run the same requests your browser is making, with old headers or removed headers
Run some TAO deliveries

Related PRs:

…aviour based on token pool

codeclimate

The PR diff size of 6147 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 6278 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 6278 lines exceeds the maximum allowed for the inline comments feature.

krampstudio · 2019-04-29T13:31:26Z

Any reason why the HTTP response has a 412 status when the token is invalid ? I'd have expected a 403

oatymart · 2019-04-29T14:13:45Z

Any reason why the HTTP response has a 412 status when the token is invalid ? I'd have expected a 403

@martijn-tao @siwane could you comment? I didn't catch the exact details of why we changed it.

krampstudio · 2019-04-30T15:30:24Z

scripts/update/Updater.php

+
+            /** @var TokenStore $tokenStore */
+            $tokenStore = $tokenService->getOption(TokenService::OPTION_STORE);
+            $tokenStore->removeTokens();


for my own understanding, what the purpose of removing the tokens during an update ?

krampstudio

Still very minor issues regarding the source code, but the overall looks very good !

I've made some testing of the back-office and the test runner which looks working as expected.
I've only encountered one issue on taoDacSimple, which is most likely related to the call of the request itself (so the error belong to that extension) : oat-sa/extension-tao-dac-simple#87 (review)

views/js/layout/actions/common.js

…ttps://github.com/oat-sa/tao-core into feature/TAO-7306-Implement-endpoint-CSRF-protection

krampstudio · 2019-05-02T07:51:38Z

And by the way the status code 403 vs 412 should also sorted out before merging

codeclimate

The PR diff size of 6257 lines exceeds the maximum allowed for the inline comments feature.

krampstudio

Code is ok now from my perspective.
I didn't found any other issue

…ttps://github.com/oat-sa/tao-core into feature/TAO-7306-Implement-endpoint-CSRF-protection

martijn-tao · 2019-05-02T11:12:34Z

And by the way the status code 403 vs 412 should also sorted out before merging

@krampstudio Done, Changed the status to 403

codeclimate

The PR diff size of 6257 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 6257 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 6257 lines exceeds the maximum allowed for the inline comments feature.

oatymart and others added 30 commits January 11, 2019 14:48

Multi-token handler, hook tokenHandler into requests

a3c8186

Extra methods for token queue & cookies

e59fa7e

Wrapped request in function to allow queueing; logic to determine beh…

323f005

…aviour based on token pool

Adapting request module to work with QtiServiceProxy

675eb24

Trying requesting tokens from a mock endpoint

63a5a62

Split tokenStore off tokenHandler; Token expiry

5ae13ae

Added pruning function

c4ab195

Full tests for tokenStore

c6d5480

Full tests for tokenStore (missing files)

47d88b8

Tests for tokenHandler using mockjax

fcf0a5a

Improvements to tokenStore & tokenHandler tests

9f11283

Moved logic to new core/request module

b2d9f49

Now fetching tokens directly from module config

421a298

Allow configuration of pool size from outside tokenHandler

9234db0

Allow ajax timeout to come from client config or TR config

9ffcd02

Allow legacy initialToken string parameter

664ce43

Merge branch 'develop' into feature/TAO-7305_frontend-xsrf-token-handler

cfc54db

More tests & improvements to latest tests

3afa76b

Fix to make tokenStore work with localStorage & indexeddb

c70e4d9

Bugfix in enforceMaxSize

a4888ee

Changes to ClientConfig data structure

b3e14a4

Cleaning up some logging & dead code

220e927

Fixed 204 behaviour in test

8d88fff

Rewrote tokenStore methods & tests in async style

55d1867

Rewrote core/request tests in fully async style

c758f8d

Store ClientConfig access flag in a memory store

3e2e191

Fix for getToken clientConfig bug

19034c1

Cleaned up some logging, added some back

8d626bb

Moved failed response processing to core/request

5a497c3

WIP - Implement CSRF token generation and validation

d8360dc

Refactored frontend unit tests

72aa85d

codeclimate bot reviewed Apr 25, 2019

View reviewed changes

oatymart added 2 commits April 26, 2019 16:15

Fix bug in core/request unit test

b38ddea

Replace ajaxMock with $.mockjax in poll test

989de3e

codeclimate bot reviewed Apr 29, 2019

View reviewed changes

oatymart requested a review from krampstudio April 29, 2019 07:15

MErge develop

05b9962

codeclimate bot reviewed Apr 29, 2019

View reviewed changes

krampstudio reviewed Apr 30, 2019

View reviewed changes

krampstudio suggested changes Apr 30, 2019

View reviewed changes

views/js/layout/actions/common.js Outdated Show resolved Hide resolved

views/js/layout/actions/common.js Outdated Show resolved Hide resolved

views/js/layout/actions/common.js Outdated Show resolved Hide resolved

oatymart added 2 commits May 2, 2019 08:52

Revert new UI messages, remove useless catch blocks

6e843c8

Merge branch 'feature/TAO-7306-Implement-endpoint-CSRF-protection' of h…

310676d

…ttps://github.com/oat-sa/tao-core into feature/TAO-7306-Implement-endpoint-CSRF-protection

codeclimate bot reviewed May 2, 2019

View reviewed changes

krampstudio approved these changes May 2, 2019

View reviewed changes

martijn-tao added 3 commits May 2, 2019 13:06

Changed CSRF failure HTTP response to 403

58b9a8e

Merge branch 'feature/TAO-7306-Implement-endpoint-CSRF-protection' of h…

27d9cd3

…ttps://github.com/oat-sa/tao-core into feature/TAO-7306-Implement-endpoint-CSRF-protection

Changed param type to follow method signature

a232581

codeclimate bot reviewed May 2, 2019

View reviewed changes

412 code to 403 for frontend

a0dae38

codeclimate bot reviewed May 2, 2019

View reviewed changes

siwane approved these changes May 2, 2019

View reviewed changes

Merge develop

861edf9

codeclimate bot reviewed May 2, 2019

View reviewed changes

siwane marked this pull request as ready for review May 2, 2019 12:50

siwane merged commit d657a6c into develop May 2, 2019

siwane deleted the feature/TAO-7306-Implement-endpoint-CSRF-protection branch May 2, 2019 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement endpoint csrf protection #2057

Implement endpoint csrf protection #2057

martijn-tao commented Mar 25, 2019 •

edited by siwane

Loading

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot left a comment

krampstudio commented Apr 29, 2019

oatymart commented Apr 29, 2019

krampstudio Apr 30, 2019

krampstudio left a comment

krampstudio commented May 2, 2019

codeclimate bot left a comment

krampstudio left a comment

martijn-tao commented May 2, 2019

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot left a comment

Implement endpoint csrf protection #2057

Implement endpoint csrf protection #2057

Conversation

martijn-tao commented Mar 25, 2019 • edited by siwane Loading

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

krampstudio commented Apr 29, 2019

oatymart commented Apr 29, 2019

krampstudio Apr 30, 2019

Choose a reason for hiding this comment

krampstudio left a comment

Choose a reason for hiding this comment

krampstudio commented May 2, 2019

codeclimate bot left a comment

Choose a reason for hiding this comment

krampstudio left a comment

Choose a reason for hiding this comment

martijn-tao commented May 2, 2019

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

martijn-tao commented Mar 25, 2019 •

edited by siwane

Loading