replace check_csrf with require_csrf
Closed this issue · 7 comments
The check_csrf view predicate as it currently stands is not considered useful because it behaves as a predicate. Thus if the CSRF check fails the normal scenario is that a PredicateMismatch is raised and results in a 404.
In 1.7 with the introduction of view derivations in #2021 we can add a new require_csrf derivation that properly raises BadCSRFToken which can be caught and handled in an exception view. The check_csrf can be deprecated at this point and potentially removed in the future.
👍
Should require_csrf= with True / False be the only option? The other issue I can think of is wanting to use the deriver on a view that accepts both get and post, at which point we may want to only require csrf on non-get/head requests (post, put, patch, delete, etc). It seems like it might be a little tough to generalize but I think what I said would work. Maybe this could be overridden by require_csrf='always' instead of True or we could just define a few strings instead of true/false. Anyone have some thoughts on this?
Could we allow require_csrf to be a callable that inspects that request and returns True/False? Might be nice to be able to dynamically enable/disable CSRF checks
well... here's what I didn't mention yet that will influence my answer here: Because of issues like this, the view deriver will probably not be added to your app by default but rather just another thing people can use if they want, similar to the default session factories etc. For example:
config.add_view_deriver(`require_csrf`, `pyramid.session.require_csrf_view`)If you want something that can do totally custom logic at the point of view declaration then you probably just want decorator=require_csrf. Passing a callable is better served by the existing decorator deriver. This PR is about a more declarative approach where you define the policy globally and then you can pass options to it per-view. That's the goal, I'm not saying we'll get there but I don't want to add an API with knobs on it if I don't have to. The idea is that you can already just swap out the require_csrf view deriver we're talking about here for a different one that may do a better job with your callable.
I think that the CSRF view derivation that we ship with core should come with seat-belts enabled. If you load it, CSRF checking is mandatory for all POST.
PUT/DELETE are not required because they can't be used cross site without OPTIONS. Anyone that needs more flexibility should adapt the code from core and modify it as necessary and load it as a view derivation.
I am a -1 on knobs.
PUT/DELETE are not required because they can't be used cross site without OPTIONS.
Good point about PUT/DELETE that I hadn't considered. It's easy to forget the exact attack vectors that CSRF is protecting against.
If we require_csrf with True/False it will behave mostly like the existing predicate. If we default to False it will be exactly like the predicate right now, and we could leave it enabled by default and a programmer would have to opt-in per view.