Decouple CSRF protection from the session machinery #2854

MatthewWilkes · 2016-12-07T10:09:50Z

In #1573 @dstufft makes a number of feature requests about the CSRF implementation of Pyramid, most of which were added in 1.7.

This patch attempts to move the CSRF generation logic out of the session, so new CSRF implementations that do not depend on sessions can be added. It currently does not add any such implementations, the only one shipped is the default which falls back to an implementation of CSRF on the session.

It also introduces new utility functions for get_csrf_token(request) and new_csrf_token(request) in Python, as well as a get_csrf_token() in the template global scope, and repoints documentation to use these rather than explicitly calling the session machinery.

This was written at the DragonSprint by me and @jcerjak, a new contributor to Pyramid.

jcerjak · 2016-12-07T10:44:12Z

pyramid/csrf.py

+        csrf = registry.queryUtility(ICSRF)
+        if csrf is not None:
+            event['get_csrf_token'] = partial(csrf.get_csrf_token, request)
+    except AttributeError:


Is this except needed since request.registry is not always available? It might mask an error if csrf does not implement get_csrf_token (not likely, but still).

This also catches the case when request is None or some weird mock object, like in tests. I've moved the except handler only to cover request.registry and the rest in in an else handler.

The utility promises to implement ICSRF, which guarantees that it has a get_csrf_token method. We don't need to defend against borked implementations.

jcerjak · 2016-12-07T10:56:00Z

pyramid/config/security.py

@@ -218,9 +227,14 @@ def register():
        intr['header'] = header
        intr['safe_methods'] = as_sorted_tuple(safe_methods)
        intr['callback'] = callback
+
+        from pyramid.csrf import csrf_token_template_global
+        from pyramid.events import BeforeRender


Could we move these imports to the top?

tseaver · 2016-12-07T12:38:26Z

pyramid/csrf.py

+)
+from pyramid.interfaces import ICSRF
+from pyramid.settings import aslist
+from zope.interface import implementer


Third-party imports should be prior to "local" imports.

Agreed, moving.

tseaver · 2016-12-07T12:40:25Z

pyramid/csrf.py

+        csrf = registry.queryUtility(ICSRF)
+        if csrf is not None:
+            event['get_csrf_token'] = partial(csrf.get_csrf_token, request)
+    except AttributeError:


The utility promises to implement ICSRF, which guarantees that it has a get_csrf_token method. We don't need to defend against borked implementations.

tseaver · 2016-12-07T12:42:01Z

docs/narr/security.rst

+
+To use CSRF tokens, you must first enable a :term:`session factory` as
+described in :ref:`using_the_default_session_factory` or
+:ref:`using_alternate_session_factories`.


There should be language here which points up the fact that enabling sessions is a requirement of the default ICSRF implementation, rather than a general requirement.

I believe @jcerjak is looking at rewording this, is that the case?

I've fixed it in the code docstrings, but missed it here, fixing.

tseaver · 2016-12-07T12:43:39Z

docs/narr/security.rst

+   token = get_csrf_token(request)
+
+The ``get_csrf_token()`` method accepts a single argument, of the request. It
+returns a CSRF *token* string. If ``get_csrf_token()`` or ``new_csrf_token()``


accepts a single argument, of the request.

The "of" should not be there.

tseaver · 2016-12-07T12:45:31Z

docs/narr/security.rst

+
+.. code-block:: javascript
+
+    var csrfToken = ${get_csrf_token()};


My JS-fu may be weak, but ISTM that the token value needs to be wrapped in quotes here.

Agreed, the same is true of the current documentation, I didn't pick up on it when I changed request.session.get_csrf_token to get_csrf_token.

tseaver · 2016-12-07T12:46:15Z

docs/narr/security.rst

+   from pyramid.csrf import get_csrf_token
+   token = new_csrf_token()
+
+Note, it is not possible to force a new CSRF token from a template. If you want


Use .. note::.

tseaver · 2016-12-07T12:51:37Z

pyramid/csrf.py

+    registry = request.registry
+    csrf = registry.queryUtility(ICSRF)
+    if csrf is not None:
+        return csrf.get_csrf_token(request)


Shouldn't this helper raise if no utility is registered?

I'm not sure, I think you can make arguments either way. If you've written third part code that's intended to be reusable it means you have to special case when users haven't got CSRF enabled.

Either way, we should document the behavior.

If third-party code registers a view with require_csrf, AFAIK there is no way straightforward way to disable CSRF protection.

As discussed with @MatthewWilkes , if we want to support this use case, we could add a CSRF option for disabling the protection. Or a user could register a dummy ICSRF implementation, which just returns an empty string. But in any case we should raise an error if no ICSRF utility is registered.

If third-party code registers a view with require_csrf, AFAIK there is no way straightforward way to disable CSRF protection.

This is intentional... if someone defines a view that explicitly requires csrf then it will be inaccessible until someone registers a csrf policy for the app. Turning off csrf protection for those views is a non-goal and can already be done just by re-defining it after config.commit() to override the old view.

I'd expect this code to raise if there is no policy. Simply using getUtility should be enough.

tseaver · 2016-12-07T12:51:45Z

pyramid/csrf.py

+    registry = request.registry
+    csrf = registry.queryUtility(ICSRF)
+    if csrf is not None:
+        return csrf.new_csrf_token(request)


Shouldn't this helper raise if no utility is registered?

Yes, it should raise. Using getUtility should be enough.

tseaver · 2016-12-07T12:52:31Z

pyramid/csrf.py

+    if supplied_token == "" and header is not None:
+        supplied_token = request.headers.get(header, "")
+
+    impl = request.registry.getUtility(ICSRF)


Shouldn't this helper raise if no utility is registered?

As this one is getUtility it will raise a ComponentLookupError if it's missing, were you thinking of something a bit more descriptive?

Ah, my eyes read that as queryUtility() like the ones above.

mmerickel

This is on the right track, great work so far!

Could we consider adding an alternate implementation that does not store the CSRF token in the session to prove the viability of this approach? After all, there's no point to this feature if it doesn't change that status quo.

Maybe just a simple CookieCSRF is enough? AFAIK it does not need to be signed, but @bertjwregeer will be happy to correct my ignorance if I'm wrong.

Finally I'd like to think of a better name for the interface than ICSRF such as ICSRFTokenStorage or something. It's a bit weird because the interface itself doesn't control the rest of the default options. Similarly naming it impl and implementation in the code is a bit too generic for me versus storage or policy or something. I'm not quite sure yet.

mmerickel · 2016-12-07T16:27:43Z

docs/whatsnew-1.8.rst

@@ -0,0 +1,40 @@
+What's New in Pyramid 1.8


Please don't add a whatsnew document. We will do it prior to release. Your changes are good I just think it's easier to create that file from scratch just before a major release.

Got it, thanks!

mmerickel · 2016-12-07T16:31:04Z

pyramid/csrf.py

+    registry = request.registry
+    csrf = registry.queryUtility(ICSRF)
+    if csrf is not None:
+        return csrf.get_csrf_token(request)


If third-party code registers a view with require_csrf, AFAIK there is no way straightforward way to disable CSRF protection.

This is intentional... if someone defines a view that explicitly requires csrf then it will be inaccessible until someone registers a csrf policy for the app. Turning off csrf protection for those views is a non-goal and can already be done just by re-defining it after config.commit() to override the old view.

I'd expect this code to raise if there is no policy. Simply using getUtility should be enough.

mmerickel · 2016-12-07T16:31:47Z

pyramid/csrf.py

+    registry = request.registry
+    csrf = registry.queryUtility(ICSRF)
+    if csrf is not None:
+        return csrf.new_csrf_token(request)


Yes, it should raise. Using getUtility should be enough.

mmerickel · 2016-12-07T16:40:13Z

pyramid/csrf.py

+    registry = request.registry
+    csrf = registry.queryUtility(ICSRF)
+    if csrf is not None:
+        return csrf.get_csrf_token(request)


Should these be functions or should we add them to the request as methods, similar to request.has_permission, request.session.get_csrf_token etc? request.get_csrf_token()? request.csrf.get_token()?

I think that new_csrf_token and get_csrf_token should be available in the same place, but I'm a little nervous about having new_csrf_token available in templates, personally. Maybe I'm worrying over nothing, but it feels to me like something that should only be called in Python code.

digitalresistor · 2016-12-07T18:11:07Z

The CSRF cookie doesn't need to be signed.

digitalresistor · 2016-12-07T18:16:11Z

pyramid/csrf.py

+    # should never appear in an URL as doing so is a security issue. We also
+    # explicitly check for request.POST here as we do not support sending form
+    # encoded data over anything but a request.POST.
+    if token is not None:


I would recommend swapping the check for the token in the headers/body. request.POST is more expensive than checking request.headers.

This is unchanged from the previous implementation, but that makes sense. Switching.

Thanks @MatthewWilkes!

MatthewWilkes · 2016-12-08T09:28:28Z

@mmerickel I've pondered a bit about the change to be specifically about storage, and think that it might be worthwhile to have some of the checking in the CSRF implementation too. Specifically, currently it assumes that constant-time string comparison is sufficient, but an implementation might want to do funky stuff like scope tokens (as mentioned in the original ticket).

If we have a hook there then we can allow this to be supported without having to also supply a callback.

MatthewWilkes · 2016-12-08T14:41:59Z

@mmerickel @jcerjak and I worked on this more today and incorporated changes based on your feedback. We have added the checking function I referenced in my previous comment, as I think it gives us a bit more flexibility.

mmerickel

I don't think I like ICSRFPolicy because it's too similar to the current default_csrf_options which I almost named and defined as a policy when I was doing that work, so I think we should avoid that word.

How would scope tokens be done with this new API? I think it's important to at least consider and I don't have a good answer for it right now.

How does this feature influence user apps right now? Does the session factory automatically register a csrf implementation somehow? Or does pyramid's configurator just always enable the SessionCSRF by default, similar to default renderers?

mmerickel · 2016-12-08T19:39:06Z

pyramid/csrf.py

+        return
+    else:
+        csrf = registry.getUtility(ICSRFPolicy)
+        event['get_csrf_token'] = partial(csrf.get_csrf_token, request)


This is code from my own apps that I'll at least paste here incase you like it better.

def includeme(config): # other session setup usually # ... config.add_subscriber(renderer_globals, 'pyramid.interfaces.IBeforeRender') class CSRFToken(object): def __init__(self, request): self.request = request def __json__(self, request): return self.request.session.get_csrf_token() def __html__(self): return self.request.session.get_csrf_token() def renderer_globals(event): request = event['request'] event['csrf_token'] = CSRFToken(request) # allows {{ csrf_token }}

👍 I like this idea!

I think this feels like a little too much magic personally. I can certainly see the simplicity benefits, but the current approach has a couple of minor benefits too:

A function call feels more expensive, which considering setting a CSRF cookie can cause pages to not be trivially cachable may theoretically prevent people adding it 'just in case'

It is closer to the existing idiom, users would change request.session.get_csrf_token() to get_csrf_token(), which may make upgrades easier.

For what it's worth, I don't think either of those two arguments are sufficient to make it the only feasible option, I just thought I'd share my thinking.

I don't think I want to integrate this change at this stage, maybe it would be worth another PR? What do you think @jcerjak?

I like the simplicity, but maybe it's better to be more explicit here. Don't have a strong opinion about it, but I think it would be better if it's a separate PR.

MatthewWilkes · 2016-12-08T23:07:35Z

Will reply to the rest tomorrow, but if you can give us steer on naming that would help. Do you prefer ICSRFStorage?

mmerickel · 2016-12-08T23:48:56Z

Will reply to the rest tomorrow, but if you can give us steer on naming that would help. Do you prefer ICSRFStorage?

Yeah sorry, I know naming is the biggest derailment. Let's go with ICSRFStoragePolicy to disambiguate it from the rest of the csrf settings.

MatthewWilkes · 2016-12-09T10:43:22Z

@mmerickel With regards to scoped tokens, this isn't included in core (and I personally don't think it should be), but it's nice and easy to implement in userspace with these changes. The implementation would be something like:

from functools import wraps
import hmac

from zope.interface import implementer

from pyramid.csrf import ICSRFStoragePolicy, SessionCSRF
from pyramid.compat import bytes_

def csrf_scope(scope):
    def csrf_scope_decorator(func):
        @wraps(func)
        def with_scope(request, *args, **kwargs):
            request.csrf_scope = scope
            return func(request, *args, **kwargs)
        return with_scope
    return csrf_scope_decorator

@implementer(ICSRFStoragePolicy)
class ScopedSessionCSRF(SessionCSRF):

    def _get_scope(self, request):
        return getattr(request, 'csrf_scope', '')

    def _scope_token(self, scope, base_token):
        return hmac.new(bytes_(base_token, 'ascii'), scope).hexdigest()

    def new_csrf_token(self, request):
        base_token = super(ScopedSessionCSRF, self).new_csrf_token(request)
        return self._scope_token(self._get_scope(request), base_token)

    def get_csrf_token(self, request):
        base_token = super(ScopedSessionCSRF, self).get_csrf_token(request)
        return self._scope_token(self._get_scope(request), base_token)

and would be used like:

@view_config(route_name='home', renderer='templates/mytemplate.pt')
@csrf_scope('foobar')
def my_view(request):
    return {'project': 'hacking'}

MatthewWilkes · 2016-12-09T11:00:28Z

@mmerickel WRT user facing changes, that's a good point. I've added a fallback to the check_csrf_token method that ensures that if a user does not configure CSRF the old behaviour of assuming the session will manage it is preserved.

That means that if the user explicitly marks a view as being CSRF checked without calling config.set_default_csrf_options(require_csrf=True) they will get SessionCSRF if there is a session present or an error about no session being configured if not.

mmerickel

I'm thinking pyramid should just register a SessionCSRFStoragePolicy by default in Configurator.__init__ via calling config.set_csrf_storage_policy(SessionCSRFStoragePolicy()). in order to keep bw-compat. This would alleviate the need for the fallback policy creation in each method (check_csrf_token and new_csrf_token). If someone accesses these without a session it'll just raise which is fine.

mmerickel · 2016-12-10T20:26:50Z

docs/narr/security.rst

+   from pyramid.config import Configurator
+
+   config = Configurator()
+   config.set_default_csrf_options(implementation=MyCustomCSRFPolicy())


I'd prefer if this were a separate method for config.set_csrf_storage_policy as CSRF is entirely unusable without it and it would be good if it could be replaced without changing the other options.

mmerickel · 2016-12-10T20:27:43Z

pyramid/csrf.py

+
+
+@implementer(ICSRFStoragePolicy)
+class SessionCSRF(object):


SessionCSRFStoragePolicy

mmerickel · 2016-12-10T20:27:56Z

pyramid/csrf.py

+        )
+
+@implementer(ICSRFStoragePolicy)
+class CookieCSRF(object):


CookieCSRFStoragePolicy

mmerickel · 2016-12-10T20:30:44Z

pyramid/config/security.py

@@ -218,9 +231,12 @@ def register():
        intr['header'] = header
        intr['safe_methods'] = as_sorted_tuple(safe_methods)
        intr['callback'] = callback
+
+        self.add_subscriber(csrf_token_template_global, [BeforeRender])


Currently Pyramid has no precedent for automatically registering a BeforeRender subscriber and I think we should keep it that way. If you really want to inject it we could a) do it in the current renderer bindings (pyramid_jinja2, pyramid_mako, pyramid_chameleon) or b) do it in the pyramid.renderer.RendererHelper as a system value.

mmerickel · 2016-12-10T20:31:59Z

pyramid/csrf.py

+        self.secure = secure
+        self.httponly = httponly
+        self.domain = domain
+        self.path = path


This implementation should be using the webob.cookies.CookieProfile from webob in order to set the cookies on potentially multiple domains similar to how SignedCookieSessionFactory and AuthTktAuthenticationPolicy operate.

…from the session machinery. Adds configuration of this to the csrf_options configurator commands. Make the default implementation a fallback to the old one. Documentation patches for new best practices given updates CSRF implementation.

… review regarding naming of variables and code cleanup.

…configure a session and set explicit checks would see an exception

… implemenations based on feedback, split CSRF implementation and option configuration and make the csrf token function exposed as a system default rather than a renderer event.

MatthewWilkes · 2017-04-12T11:19:21Z

@mmerickel I've looked at this again today, as I now have a client who wants CSRF protection without sessions. I've pushed changes to address your comments, let me know when you've had a chance to take a look?

digitalresistor

👍 liking this a lot :-D

digitalresistor · 2017-04-12T12:14:23Z

pyramid/csrf.py

+       considered valid. It must be passed in either the request body or
+       a header.
+
+    .. versionchanged:: 1.8a1


We are going to need to change these to Pyramid 1.9

mmerickel

@MatthewWilkes this is really great stuff! I think one more round with very minor tweaks and we're good to go.

mmerickel · 2017-04-13T02:42:45Z

docs/narr/sessions.rst

@@ -320,179 +319,3 @@ flash storage.
 .. index::


I think these index directives need to be moved.

mmerickel · 2017-04-13T03:58:38Z

pyramid/config/security.py

+        def register():
+            self.registry.registerUtility(policy, ICSRFStoragePolicy)
+
+        self.action(ICSRFStoragePolicy, register, order=PHASE1_CONFIG)


I'm pretty sure the PHASE1_CONFIG is unnecessary here. It's used in set_default_csrf_options because it needs to be configured prior to add_view running the csrf_view view deriver. I think the storage policy is only used at request-time though so can be left at the default config phase.

If you think it should be PHASE1_CONFIG then it should be added to http://docs.pylonsproject.org/projects/pyramid/en/master/narr/extconfig.html#ordering-actions

I had added it to extconfig, but you're right. Removing it from here and extconfig.

mmerickel · 2017-04-13T04:01:05Z

pyramid/csrf.py

+        # This means explicit validation has been asked for without configuring
+        # the CSRF implementation. Fall back to SessionCSRFStoragePolicy as that is the
+        # default
+        policy = SessionCSRFStoragePolicy()


This is probably unnecessary now. Just use getUtility instead an exception if there's no policy. Thoughts?

Yep, thanks. I was missing a call in the testing setup that this was hiding. Fixed.

- Renamed `SessionCSRFStoragePolicy` to `LegacySessionCSRFStoragePolicy` for the version that uses the legacy `ISession.get_csrf_token` and `ISession.new_csrf_token` apis and set that as the default. - Added new `SessionCSRFStoragePolicy` that stores data in the session similar to how the `SessionAuthenticationPolicy` works. - `CookieCSRFStoragePolicy` did not properly return the newly generated token from `get_csrf_token` after calling `new_csrf_token`. It needed to cache the new value since the response callback does not affect the current request. - `CookieCSRFStoragePolicy` was not forwarding the `domain` value to the `CookieProfile` causing that setting to be ignored. - Removed `check_csrf_token` from the `ICSRFStoragePolicy` interface to simplify implementations of storage policies. - Added an introspectable item for the configured storage policy so that it appears on the debugtoolbar. - Added a change note on `ISession` that it no longer required the csrf methods.

mmerickel

I went through this tonight and found several minor issues.

SessionCSRFStoragePolicy should really just use the session as a storage mechanism. I added LegacySessionCSRFStoragePolicy for the version that uses the legacy ISession.get_csrf_token and ISession.new_csrf_token apis and set that as the default. I also updated SessionCSRFStoragePolicy to just store data in the session similar to how the SessionAuthenticationPolicy works.
CookieCSRFStoragePolicy did not properly return the newly generated token from get_csrf_token after calling new_csrf_token. It needs to cache the new value since the response callback does not affect the current request.
CookieCSRFStoragePolicy was not forwarding the domain value to the CookieProfile causing that setting to be ignored.
I removed check_csrf_token from the ICSRFStoragePolicy interface because I can't see the point to making it pluggable. On top of that it makes implementation of storage policies more difficult.
Added an introspectable item for the configured storage policy so that it appears on the debugtoolbar.
Added a change note on ISession that it no longer required the csrf methods.
Restored (but deprecated) shims in pyramid.session for check_csrf_origin and check_csrf_token.

- Renamed `SessionCSRFStoragePolicy` to `LegacySessionCSRFStoragePolicy` for the version that uses the legacy `ISession.get_csrf_token` and `ISession.new_csrf_token` apis and set that as the default. - Added new `SessionCSRFStoragePolicy` that stores data in the session similar to how the `SessionAuthenticationPolicy` works. - `CookieCSRFStoragePolicy` did not properly return the newly generated token from `get_csrf_token` after calling `new_csrf_token`. It needed to cache the new value since the response callback does not affect the current request. - `CookieCSRFStoragePolicy` was not forwarding the `domain` value to the `CookieProfile` causing that setting to be ignored. - Removed `check_csrf_token` from the `ICSRFStoragePolicy` interface to simplify implementations of storage policies. - Added an introspectable item for the configured storage policy so that it appears on the debugtoolbar. - Added a change note on `ISession` that it no longer required the csrf methods.

- Renamed `SessionCSRFStoragePolicy` to `LegacySessionCSRFStoragePolicy` for the version that uses the legacy `ISession.get_csrf_token` and `ISession.new_csrf_token` apis and set that as the default. - Added new `SessionCSRFStoragePolicy` that stores data in the session similar to how the `SessionAuthenticationPolicy` works. - `CookieCSRFStoragePolicy` did not properly return the newly generated token from `get_csrf_token` after calling `new_csrf_token`. It needed to cache the new value since the response callback does not affect the current request. - `CookieCSRFStoragePolicy` was not forwarding the `domain` value to the `CookieProfile` causing that setting to be ignored. - Removed `check_csrf_token` from the `ICSRFStoragePolicy` interface to simplify implementations of storage policies. - Added an introspectable item for the configured storage policy so that it appears on the debugtoolbar. - Added a change note on `ISession` that it no longer required the csrf methods. - Leave deprecated shims in ``pyramid.session`` for ``check_csrf_origin`` and ``check_csrf_token``.

typo is fixed (function is removed entirely) in #3019

mmerickel · 2017-04-29T07:09:49Z

This PR will closed and show as merged automatically when #3019 is merged.

MatthewWilkes · 2017-04-29T14:55:24Z

@mmerickel Please don't remove the check_csrf_token method, it is very useful to have it pluggable as that allows for keyring based or fully-stateless CSRF policies.

For example, consider this CSRF policy:

implementer(ICSRFStoragePolicy)
class HMACCSRFStoragePolicy(object):
    """ An alternative CSRF implementation that uses short-lived HMAC keyed on
    the user.
    """

    def __init__(self, key):
        self.key = key

    def new_csrf_token(self, request):
        user = request.authenticated_userid
        generated = int(time.time())
        digest = hmac.new(self.key, 'csrf-{}-{}'.format(user, generated), hashlib.sha1).hexdigest()
        return '{}:{}:{}'.format(user, generated, digest)

    def get_csrf_token(self, request):
        return self.new_csrf_token(request)

    def check_csrf_token(self, request, supplied_token):
        user, generated, digest = supplied_token.split(':')
        if user != request.authenticated_userid:
            return False
        if int(generated) < (time.time() - 300):
            return False
        expected = hmac.new(self.key, 'csrf-{}-{}'.format(user, generated), hashlib.sha1).hexdigest()
        return hmac.compare_digest(expected, digest)

For non-security experts reading this, I'm not claiming that the above is a good CSRF implementation, however I do think that it demonstrates that there is a value in allowing the CSRF implementation to decide what's valid and what isn't.

If you're concerned about boilerplate (although, personally, I think it's minimal), maybe we could make a mixin available?

digitalresistor · 2017-04-29T16:57:28Z

@MatthewWilkes get_csrf_token has access to the request object. You will have to manually pull the existing token out of the appropriate header/POST data or wherever, but it is still possible to do what you want to do.

MatthewWilkes · 2017-04-29T23:02:00Z

This is true, good point.

mmerickel · 2017-04-30T03:22:47Z

@MatthewWilkes thank you very much for the alternative example. I definitely had not considered this stateless use case. It appears that the main downside of supporting that use case without a pluggable check_csrf_token is that the policy is now responsible for parsing the header/body itself to find the supplied token. This feels vaguely similar to unauthenticated_userid versus authenticated_userid in authentication policies. Perhaps I can add another API to get the "unauthenticated" csrf token which can be used if necessary. I'm thinking find_csrf_token or get_unverified_csrf_token.

EDIT: I thought about it a bit longer and I can't actually provide this get_unverified_csrf_token API because it is not standardized where to find the tokens in the request. It can be different based on csrf defaults and for other reasons like the fact that check_csrf_token allows the user to specify their own key/header. I will bring back policy.check_csrf_token and try to be happy that we have the most over-engineered csrf protection in the business.

MatthewWilkes · 2017-04-30T08:25:18Z

Thanks for coming to that decision, it occured to me this morning that Bert's workaround has a fatal flaw. Any submission that includes a CSRF check would cause get_card_token to return the supplied token, making it very easy to hit the short timeout. Imagine a form that takes 3 minutes to fill in, once you've submitted that the resultant page render will use a token with only 2 minutes of validity, so a multipage form would quickly fail.

As to being the most overengineered in the business, I have one word for you, Plone.

Decouple CSRF protection from the session machinery (replaced #2854)

MatthewWilkes force-pushed the csrf_session_decouple branch from f1fe60c to e4f3399 Compare December 7, 2016 10:27

jcerjak reviewed Dec 7, 2016

View reviewed changes

MatthewWilkes force-pushed the csrf_session_decouple branch from e4f3399 to 3066e55 Compare December 7, 2016 12:37

tseaver reviewed Dec 7, 2016

View reviewed changes

MatthewWilkes force-pushed the csrf_session_decouple branch from c29afe9 to 052aa49 Compare December 7, 2016 14:24

mmerickel requested changes Dec 7, 2016

View reviewed changes

digitalresistor reviewed Dec 7, 2016

View reviewed changes

MatthewWilkes force-pushed the csrf_session_decouple branch from 5f9dd69 to 197a609 Compare December 8, 2016 14:40

MatthewWilkes force-pushed the csrf_session_decouple branch from 197a609 to 028132b Compare December 8, 2016 16:54

mmerickel reviewed Dec 8, 2016

View reviewed changes

mmerickel requested changes Dec 10, 2016

View reviewed changes

mmerickel added the needs-work label Dec 14, 2016

MatthewWilkes and others added 6 commits April 12, 2017 12:13

Fix tests and documentation in various places, and feedback following…

313c251

… review regarding naming of variables and code cleanup.

add to contributors list

8f60e2c

Rename implementation to ICSRFStoragePolicy

fe0d223

Fix a bug where people that didn't configure CSRF protection but did …

f6d63a4

…configure a session and set explicit checks would see an exception

Use the webob CookieProfile in the Cookie implementation, rename some…

7c0f098

… implemenations based on feedback, split CSRF implementation and option configuration and make the csrf token function exposed as a system default rather than a renderer event.

MatthewWilkes force-pushed the csrf_session_decouple branch from a3e3f70 to 7c0f098 Compare April 12, 2017 11:14

digitalresistor approved these changes Apr 12, 2017

View reviewed changes

mmerickel requested changes Apr 13, 2017

View reviewed changes

mmerickel mentioned this pull request Apr 29, 2017

Decouple CSRF protection from the session machinery (replaced #2854) #3019

Merged

mmerickel approved these changes Apr 29, 2017

View reviewed changes

mmerickel removed the needs-work label Apr 29, 2017

mmerickel merged commit 4b3603a into Pylons:master Apr 30, 2017

mmerickel added a commit that referenced this pull request Apr 30, 2017

Merge pull request #3019 from mmerickel/fixes/csrf-decoupling-2854

80973f8

Decouple CSRF protection from the session machinery (replaced #2854)

mmerickel mentioned this pull request Apr 30, 2017

Improve CSRF Support for Safety, REST, and Decoupling #1573

Closed

This was referenced Jun 26, 2017

Scheduled weekly dependency update for week 26 openstax/cnx-deploy#337

Closed

Update pyramid to 1.9 knowsuchagency/ninjadog#2

Merged

Update pyramid to 1.9 nicfit/MishMash#137

Closed

Initial Update mozilla-services/shavar#100

Merged

pyup-bot mentioned this pull request Jul 3, 2017

Scheduled weekly dependency update for week 27 openstax/cnx-deploy#341

Closed

This was referenced Jul 10, 2017

Scheduled weekly dependency update for week 28 openstax/cnx-deploy#342

Closed

Update pyramid to 1.9.1 nicfit/MishMash#146

Merged

Scheduled weekly dependency update for week 29 openstax/cnx-deploy#344

Closed

pyup-bot mentioned this pull request Jul 24, 2017

Scheduled weekly dependency update for week 30 openstax/cnx-deploy#346

Closed

This was referenced Sep 29, 2017

Initial Update openstax/cnx-deploy#385

Merged

Update pyramid to 1.9.1 OnroerendErfgoed/pyramid_skosprovider#37

Closed

Update pyramid to 1.9.1 OnroerendErfgoed/pyramid_urireferencer#27

Merged

pyup-bot mentioned this pull request Dec 12, 2017

Pin pyramid to latest version 1.9.1 openstax/cnx-press#6

Merged

pyup-bot mentioned this pull request Jun 30, 2020

Pin pyramid to latest version 1.10.4 camptocamp/c2cgeoportal#6622

Closed


		.. code-block:: javascript

		var csrfToken = ${get_csrf_token()};

Decouple CSRF protection from the session machinery #2854

Decouple CSRF protection from the session machinery #2854

Conversation

MatthewWilkes commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmerickel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalresistor commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewWilkes commented Dec 8, 2016

MatthewWilkes commented Dec 8, 2016

mmerickel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewWilkes commented Dec 8, 2016

mmerickel commented Dec 8, 2016

MatthewWilkes commented Dec 9, 2016 • edited Loading

MatthewWilkes commented Dec 9, 2016

mmerickel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewWilkes commented Apr 12, 2017

digitalresistor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmerickel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmerickel left a comment • edited Loading

Choose a reason for hiding this comment

mmerickel commented Apr 29, 2017

MatthewWilkes commented Apr 29, 2017

digitalresistor commented Apr 29, 2017

MatthewWilkes commented Apr 29, 2017

mmerickel commented Apr 30, 2017 • edited Loading

MatthewWilkes commented Apr 30, 2017

MatthewWilkes commented Dec 9, 2016 •

edited

Loading

mmerickel left a comment •

edited

Loading

mmerickel commented Apr 30, 2017 •

edited

Loading