Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding CSRF form protection via Hapi Crumb plugin #911

Merged
merged 24 commits into from
Jul 31, 2024

Conversation

rvsiyad
Copy link
Contributor

@rvsiyad rvsiyad commented Apr 15, 2024

DEFRA/water-abstraction-team#114

Crumb is used to diminish CSRF attacks using a random unique token that is validated on the server side. Crumb may be used whenever you want to prevent malicious code from executing system commands that are performed by HTTP requests.

After this change, any view that has a form that uses method="POST", will need to have a hidden input field.

  <input type="hidden" name="wrlsCrumb" value="{{wrlsCrumb}}"/>

When the page is requested, the Crumb plugin will generate a token in the onPreResponse event that it will save to a cookie called 'wrlsCrumb'. It also makes it available in the view context, hence our views can reference {{wrlsCrumb}}.

When the user submits the form, the Crumb plugin jumps in again, this time on the onPostAuth event. It compares both values, and if they match, it 'authorises' the request.

If the payload is missing {{wrlsCrumb}}, or the value doesn't match that saved in the cookie (which is secure and unreadable to the client) then it rejects the request. In our service, the user will see a 404, as that is the default for an 'unauthorised' request.

Crumb will be enabled by default for all endpoints to avoid us forgetting to protect an HTML form. However, this means it performs the check for all POST requests. Our service exposes API-only POST endpoints, which do not expect a payload. For this, the route config must include the following to disable Crumb.

 options: {
   plugins: {
     crumb: false
   }
 }

@rvsiyad rvsiyad added the enhancement New feature or request label Apr 15, 2024
@rvsiyad rvsiyad self-assigned this Apr 15, 2024
@rvsiyad rvsiyad force-pushed the hansel-and-grettel-csrf branch from c54cf83 to ccee9c0 Compare April 22, 2024 12:50
@Cruikshanks
Copy link
Member

Just a reminder to my future self. Where we have come to a stop is unit testing. What solution can we implement that will allow us to test a POST request to an endpoint that has CSRF (crumb) enabled on it?

The POST request needs to be able to include a token that crumb will validate successfully. Digging round the future farming repos they solved this by sending a GET request first then extracting the token and adding it to the POST.

There is not much on the interwebs on crumb and testing. What we found suggests disabling crumb when unit testing, though no actual example on how to go about doing this.

There is also the feeling that as we know all the config and credentials used that we should be able to send a POST request with a token that crumb will validate, without having to send a GET request first or disabling it.

rvsiyad and others added 8 commits July 29, 2024 12:29
Crumb is used to diminish CSRF attacks using a random unique token that
is validated on the server side. Crumb may be used whenever you want to
prevent malicious code to execute system commands, that are performed by
HTTP requests.

This PR is focused on adding CSRF Tokens via the Hapi Crumb plugin.
Co-authored-by: Alan Cruikshanks <[email protected]>
We've rebased against `main`` after a long period of time and there have been many updates since we started this change.

This puts back in some changes we following the merge, and adds the crumb input to the additional pages in the journey that didn't exist when we were trialling this.
@Cruikshanks Cruikshanks force-pushed the hansel-and-grettel-csrf branch from 500ce47 to a85d6d5 Compare July 29, 2024 11:38
After much debugging we were able to see that crumb generates a token it then stores in a cookie during the `onPreResponse` hook.

Assuming we have added `<input type="hidden" name="wrlsCrumb" value="{{wrlsCrumb}}"/>` to the form and not disabled crumb in the route options for the endpoint, the cookie is included in the headers and the `wrlsCrumb` in the payload.

Hapi extracts the cookie and makes it available on the request object, from which crumb the plugin can retrieve it. It compares this value to `wrlsCrumb` in the payload.

If they match it deletes `wrlsCrumb` from the payload (hence you don't see it in the controller's version of the payload), else it causes a not authorised (and in our setup this means a 404).

So, as mentioned we saw that a number of the Defra `ffc-*` projects that use crumb share the same util function that fires a GET request, retrieves the crumb token and returns it.

The tests then include this value in the request headers as a cookie and in the payload. Our testing confirmed you got the same result (a validated crumb token) by just doing the latter! There is no need to generate a token using a GET request first.

As we'll need to do this for a number of tests, and knowing we already apply the same auth options across the majority of POST requests, we came up with this helper that we intend to switch the tests to as we update them to work with crumb.
Note - tests still fail on this controller because we are with tests where a form is submitted. We'll need to update the config of POST endpoints that don't expect a form submission to disable crumb to make them work.
@Cruikshanks Cruikshanks marked this pull request as ready for review July 31, 2024 11:28
@Cruikshanks Cruikshanks requested a review from Beckyrose200 July 31, 2024 11:28
Jozzey
Jozzey previously approved these changes Jul 31, 2024
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Spotted a typo

app/plugins/crumb.plugin.js Outdated Show resolved Hide resolved
Co-authored-by: Jason Claxton <[email protected]>
@Cruikshanks Cruikshanks dismissed stale reviews from Jozzey and robertparkinson via 6a03b86 July 31, 2024 14:58
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cruikshanks Cruikshanks merged commit cdc65a5 into main Jul 31, 2024
6 checks passed
@Cruikshanks Cruikshanks deleted the hansel-and-grettel-csrf branch July 31, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants