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

require_csrf to replace check_csrf #2413

Merged
merged 5 commits into from Apr 13, 2016

Conversation

Projects
None yet
3 participants
@mmerickel
Member

mmerickel commented Mar 22, 2016

Fixes #2367

  • Merge #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 #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.

@mmerickel mmerickel changed the title from require csrf to require_csrf to replace check_csrf Mar 22, 2016

@@ -1660,18 +1665,19 @@ token unless ``disable_csrf=True`` is passed to the view:
from pyramid.response import Response
from pyramid.session import check_csrf_token
def require_csrf_view(view, info):
def csrf_view(view, info):

This comment has been minimized.

@bertjwregeer

bertjwregeer Mar 22, 2016

Member

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

This comment has been minimized.

Member

mmerickel commented Apr 10, 2016

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.
@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

  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

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

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.

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

That sounds good to me :-).

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

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

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

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

If CSRF checks fail then a :class:`pyramid.exceptions.BadCSRFToken` exception
will be raised. This exception may be caught and handled by an
:term:`exception view` but, by default, will result in a ``400 Bad Request``
resposne being sent to the client.

This comment has been minimized.

@bertjwregeer

bertjwregeer Apr 11, 2016

Member

typo on response

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

Besides the typo, this looks great :-)

mmerickel added some commits Mar 22, 2016

rewrite csrf checks to support a global setting to turn it on
- only check csrf on POST
- support "pyramid.require_default_csrf" setting
- support "require_csrf=True" to fallback to the global setting to
  determine the token name

@bertjwregeer bertjwregeer added this to the 1.7 milestone Apr 11, 2016

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

👍 this looks fantastic! Thanks @mmerickel!

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

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

This comment has been minimized.

Member

mmerickel commented Apr 11, 2016

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.

@stevepiercy stevepiercy referenced this pull request Apr 11, 2016

Closed

update Pyramid Request Processing diagram #2473

2 of 2 tasks complete
@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 11, 2016

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

This comment has been minimized.

Member

stevepiercy commented Apr 11, 2016

@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

This comment has been minimized.

Member

mmerickel commented Apr 12, 2016

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.

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 12, 2016

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

@mmerickel

This comment has been minimized.

Member

mmerickel commented Apr 12, 2016

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.

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 12, 2016

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

This comment has been minimized.

Member

mmerickel commented Apr 12, 2016

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.

@bertjwregeer

This comment has been minimized.

Member

bertjwregeer commented Apr 12, 2016

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

@bertjwregeer bertjwregeer merged commit d26e3af into Pylons:master Apr 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stevepiercy added a commit to stevepiercy/pyramid that referenced this pull request Apr 13, 2016

@stephanarts stephanarts referenced this pull request Jun 2, 2016

Closed

Monitor Session expires #24

@pyup-bot pyup-bot referenced this pull request Jun 30, 2017

Merged

Initial Update #100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment