require_csrf to replace check_csrf

[mmerickel]

Fixes #2367

• Merge configurable view deriver #2021 and rebase this PR on that.
• Deprecate check_csrf.
• Fix any other docs and docstrings referencing check_csrf to point to require_csrf instead.
  • Exclude docs/whatsnew-*
  • docs/api/session.rst
  • docs/narr/hooks.rst
  • docs/narr/sessions.rst
  • docs/narr/viewconfig.rst
  • pyramid/session.py
  • pyramid/view.py
  • pyramid/config/predicates.py
  • pyramid/config/views.py
  • pyramid/tests/tests_session.py
  • pyramid/tests/test_predicates.py
• Update the request processing diagram to show the csrf checks just below authorization in the view pipeline. See update Pyramid Request Processing diagram #2473.
• Update scaffolds to set pyramid.require_default_csrf = yes in development.ini and production.ini. See branch https://github.com/Pylons/pyramid/tree/docs/easy-install-to-pip.2104.

[digitalresistor]

This should probably match the actual csrf_view we ship more closely (related to the val stuff), and add just the check for request.method == POST.

[mmerickel]

I've rebased this PR on master.

Two remaining questions on the impl:

1. Should I place the csrf_view above or below the secured_view? Right now I have it above but I'm not so sure anymore.
2. Should I take this approach of leaving it entirely opt-in on a per-view basis? My alternative right now is to add a setting such as pyramid.require_default_csrf = yes. If someone sets it then they will get the example code I put in the docs where it's on by default and only on POST. We could then add this setting to the scaffolds so that it's on by default for new projects.

[digitalresistor]

1. I would argue that it should be below secured_view. If you don't have permission to the view in the first place, raising an not authorized makes more sense than raising BadCSRFToken. They couldn't submit to that view in the first place anyway.
2. I like the opt-in with the setting to enable it by default, so long as on a particular view a user can turn it off. Then again, I am biased because it means I won't need to write my own that does it ;-)

[mmerickel]

I will reverse the order. I'm working on implementing this feature now... it touches several more systems than the simple implementation but I think it'll be worth it. I'm adding pyramid.require_default_csrf which will turn on CSRF checking for an app's POST requests. It can be overridden by a local call to add_view(require_csrf=...).

The only quirk I've added is that if you set require_csrf=True and pyramid.require_default_csrf=foo then the token used will be foo instead of csrf_token. This means if the view didn't specify a token then it'll fallback to whatever was in require_default_csrf. If that value is truthy then it'll fallback to csrf_token.

[digitalresistor]

That sounds good to me :-).

[mmerickel]

Is there any interest in discussing how to enable/disable this feature based on whether authorization is done via headers or cookies? As I understand it, CSRF is only required when auth is via cookies that a browser will send automatically. I'm not too worried about this but I wanted to just hypothesize how an app could turn this on or off on a per-request basis. Like maybe someone could override this thing with one that supports a per-request setting to turn it off that they could set in their auth policy or something.

[mmerickel]

Okay, I've done that work. I'd appreciate some review of the implementation.

[digitalresistor]

typo on response

[digitalresistor]

Besides the typo, this looks great :-)

[digitalresistor]

👍 this looks fantastic! Thanks @mmerickel!

[mmerickel]

Okay so the only point remaining is to fix the scaffolds which I'm afraid to do while @stevepiercy is messing with the easy_install vs pip stuff. I need some guidance on when that might be done so we can add it. We just need to slip in a little pyramid.require_default_csrf = yes into the ini files. No other changes or docs required.

[mmerickel]

We just need to slip in a little pyramid.require_default_csrf = yes into the ini files. No other changes or docs required.

Well that's not quite right. If we do this we also need to setup the scaffolds to configure a session factory. I'm fine with that but it's a new thing. The session will require adding another secret to the settings at least, as well as a bit of code. I think that's fine for the alchemy and zodb scaffolds... I'm a little more wary of adding it to the starter scaffold which may not want a session.

I could also update the view deriver to ignore the checks if there's no session factory.

[digitalresistor]

The session will require adding another secret to the settings at least, as well as a bit of code.

Do we currently have any "secrets" in the settings? I am now worried that if we add this secret, and keep it the default, that people will end up running their applications using the config file and be vulnerable to remote code execution...

[stevepiercy]

@mmerickel I'm down to the last file to update, so today.

I've moved HACKING.txt and Pyramid's setup.py into a separate issue #2474, too, since that is outside of docs scope and deserves special attention from developers working on the core.

[mmerickel]

Do we currently have any "secrets" in the settings?

Yes, the alchemy tutorial added the auth policy secret to the settings as the tutorial was previously using a hard-coded string which was even worse.

[digitalresistor]

Hmmm, okay, guess adding one more secret isn't that big of a deal then.

[mmerickel]

The bigger issue is like I said it requires adding a session to the app which means invalidating it on boundaries as well as possibly explaining such things. On the bonus side it means we could possibly add flash messages to the tutorial in the future to make it nicer. I had previously envisioned adding this stuff in a new chapter but the invalidation might just need to go earlier.

The alternative is to leave this stuff out of the scaffolds for this release and stew on whether to update the scaffolds / tutorials.

[digitalresistor]

I hadn't thought of the fact that we'd have to add session to the basic scaffolds, not sure it is worth adding that by default especially since a lot of people prefer to use pyramid_beaker for instance to store data server side versus using cookie sessions.

Could we possibly leave it in the .ini commented out with a comment that you'll need to enable a session before enabling the feature?

[mmerickel]

I think we should punt that change for a later version and just ship this feature as something people can turn on. We should consider (I already wanted to do this but it was getting out of scope) adding a session chapter to the tutorial at which point it will be more clear.

[digitalresistor]

Could you merge master into this please? Then I'll click the merge button.