🔏 feat: add csrf protection

[adamkdean]

This introduces CSRF protection.

Closes #59.

About

Cross-Site Request Forgery (CSRF) is an attack that forces an end user to execute unwanted actions on a web application in which they're currently authenticated. CSRF attacks specifically target state-changing requests, not theft of data, since the attacker has no way to see the response to the forged request. More about CSRF.

This feature allows developers to use a per-request CSRF token which will be injected into the view model, and ensures that all POST requests supply a correct CSRF token. Without a correct token, with CSRF enabled, users will be greeted with a 403.

This feature uses csurf.

Usage

To enable CSRF, set the security.csrf config option in your config/config.{env}.json file:

"security": {
  "csrf": true
}

Once enabled, the variable csrfToken will be injected into the viewModel. You will need to add this to any forms which perform a POST using the field name _csrf, like so:

<form action="/" method="post">
  <input type="text" name="test_input_safe">
  <input type="hidden" name="_csrf" value="{csrfToken}">
  <input type="submit" value="Submit form">
</form>

If the CSRF token provided is incorrect, or one isn't provided, then a 403 forbidden error will occur.

A working example can be found here: dadi-web-csrf-test.

[adamkdean]

@jimlambie do you know why these tests failed? Am I a bad person or is it an unrelated issue?

[mingard]

@adamkdean something funny going on with the doc param in the config schema e.g.

  csrf: {
      doc:
        'If true, a CSRF token will be provided, and all form submissions must include this as _csrf',
      format: '*',
      default: false
    }

[adamkdean]

@mingard oh wow, just seen this now in the diff, that's weird, looks like something has changed all the docs? I'll fix it tomorrow and tidy it up. Good spot!

[jimlambie]

Actually, @adamkdean, I think the issue is due to an incorrect version of sinon being installed. Please change in the package file:

from "sinon": "latest",
to "sinon": "2.x.x",

[abovedave]

@adamkdean no need to be alarmed, it's prettier (trimming the line length)

[adamkdean]

@abovebored @jimlambie ready for review

[jimlambie]

This looks good so far, but it'd be great to see some tests to show that it works as expected. We can write these for you, if we know what the expected behaviour is - does it added headers? What's the expected response if a token doesn't exist? What's the expected response if a token is invalid?

[adamkdean]

@jimlambie my understanding of csurf's implementation is that if it's used (csrf enabled in web config), then any POST requests must provide a valid token under the name _csrf, and if it either isn't present or is incorrect, then a 403 is generated, which currently gets picked up by web for the custom error page. If you could knock up a couple of tests for this, that'd be great!

Also, probably worth testing that if csrf is enabled, that the csrfToken is present on the view model.

[jimlambie]

@adamkdean I have added 4 tests in test/unit/controller.js, however the final test (HTTP 200 with valid token) is failing. Could you please have a look, as perhaps I misunderstand the implementation.

Adding the following to node_modules/csrf/index.js L142 gives me different token values:

console.log(token, expected)

[adamkdean]

FYI the issue was that the secret is stored in a cookie, and needs to be passed back to the host with the next request, though if sessions are enabled, they are used instead.

[adamkdean]

This time we're gonna get ✅ , I can feel it.

[eduardoboucas]

Bonus points (and a beer) are granted if you replace var with const/let as you go. ⭐️ 🍺

[adamkdean]

@eduardoboucas I wasn't sure if we were doing that or if the plan was to stick with the existing style. Maybe it'd be worth tidying up the dependencies at least.

[eduardoboucas]

🦄

[abovedave]

We've enabled this by default and also made a change so the error stack (for this and all other errors) are hidden when the environment is set to production mode 🕵🏻

[eduardoboucas]

We've enabled this by default and also made a change so the error stack (for this and all other errors) are hidden when the environment is set to production mode 🕵🏻

Does making this enabled by default introduce a breaking change?

[jimlambie]

An excellent point. Perhaps something we can wait to introduce in the next major release?

[eduardoboucas]

If I understand correctly, if this is disabled in the config then everything works exactly as before, right? If that's the case, perhaps we can introduce the feature but make it disabled by default and revisit when we release the next major.

[adamkdean]

@eduardoboucas yes, you're correct. When disabled, everything will work as expected, but if enabled by default, all existing forms will break. Definitely requires a major bump.