Skip to content
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

Include protect.js for Plone 4? And Plone 5? #42

Closed
mauritsvanrees opened this issue Apr 25, 2016 · 11 comments
Closed

Include protect.js for Plone 4? And Plone 5? #42

mauritsvanrees opened this issue Apr 25, 2016 · 11 comments

Comments

@mauritsvanrees
Copy link
Member

Basically, see plone/plone4.csrffixes#19
And possibly related thread on community.plone.org: https://community.plone.org/t/plone-protect-and-ajax-post-requests/2010

In short:
Plone 4.3.9 has all fixes from plone4.csrffixes included, except the plone.protect pin and the transform which adds protect.js and blesses a few read-on-writes. These read-on-writes are presumably already blessed by plone.protect 3.x or have been fixed in their respective packages. But the javascript still seems needed for best experience.

Should we add this script on plone.protect master and perhaps include it only when the Plone version is 4.x? My guess is that adding it to the auto transform would work and is not too invasive.
Or is this something we should do in for example CMFPlone?

@mauritsvanrees
Copy link
Member Author

In plone/buildout.coredev#176 (comment) @idgserpro asks a similar question, but slight broader. Basically: should we include the protect.js file in all Plone versions, so also in Plone 5?
@vangheem, do you know this?

@mauritsvanrees mauritsvanrees changed the title Include protect.js for Plone 4? Include protect.js for Plone 4? And Plone 5? May 12, 2016
@idgserpro
Copy link

We vote for adding protect.js to Plone 5 as well. All add-ons that need to be both compatible to Plone 4 and 5 will benefit from this. Somewhat related: https://community.plone.org/t/the-incredible-and-sad-tale-of-innocent-plone-5-and-its-heartless-add-ons/1437

@hvelarde FYI

@vangheem
Copy link
Member

I'm indifferent to adding it to plone 5. I personally think people should know what they are doing about CSRF and get ugly errors when they write unsafe things; however, it will make things easier...

If added to plone 5, some of the plone 4 specific bits should be removed.

@idgserpro
Copy link

idgserpro commented May 12, 2016

We already have some "magic" happening in plone.protect transforms, automatically adding tokens to our forms. The idea is to have this same ease of use but now in AJAX calls.

Imagine you have a Plone 4 addon, using plone4.csrffixes. You have 50 ajax calls. You want to make it compatible with Plone 5. You have two options:

  • Add _authenticator variables across all of your ajax calls, getting its value from an input in a form;
  • Copy protect.js from plone4.csrffixes.

Our opinion is: if you don't need to explicitly add tokens to forms, you shouldn't need this in AJAX calls either. This is consistent with plone.protect README as well:

Automatic CSRF Protection

Since version 3, plone.protect provides automatic CSRF protection. It does this by automatically including the auth token to all internal forms when the user requesting the page is logged in.

@vangheem
Copy link
Member

Alright, as I said, I'm indifferent.

idgserpro added a commit to collective/collective.cover that referenced this issue May 13, 2016
In essence, this is needed for Plone 5 only. Plone 4 installations that
have plone4.csrffixes and plone.protect >= 3.0.x don't need it.

Check

plone/plone.protect#42

for mode details.
@idgserpro
Copy link

idgserpro commented May 18, 2016

@mauritsvanrees Could you please add protect.js in plone.protect?

@mauritsvanrees
Copy link
Member Author

See pull request #49 which finally passes the tests.

@idgserpro
Copy link

@mauritsvanrees fantastic! You are thinking pin plone.protect 3 in Plone 4.3? Or will we still depend of plone4.csrffixes?

@mauritsvanrees
Copy link
Member Author

No, plone.protect will stay at 2.x. Moving to 3.x causes too many test failures. See also pull request #48.

mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jun 10, 2016
Branch: refs/heads/master
Date: 2016-06-08T02:35:56+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.protect@d6c2207

Added protect.js from plone4.csrffixes.

This adds an `X-CSRF-TOKEN` header to ajax requests.

Fixes plone/plone.protect#42

Files changed:
A plone/protect/protect.js
M CHANGES.rst
M plone/protect/auto.py
M plone/protect/configure.zcml
Repository: plone.protect
Branch: refs/heads/master
Date: 2016-06-10T09:13:01+02:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.protect@b8c9557

Merge pull request #49 from plone/maurits-add-protect-js

Added protect.js from plone4.csrffixes.

Files changed:
A plone/protect/protect.js
M CHANGES.rst
M plone/protect/auto.py
M plone/protect/configure.zcml
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jun 10, 2016
Branch: refs/heads/master
Date: 2016-06-08T02:35:56+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.protect@d6c2207

Added protect.js from plone4.csrffixes.

This adds an `X-CSRF-TOKEN` header to ajax requests.

Fixes plone/plone.protect#42

Files changed:
A plone/protect/protect.js
M CHANGES.rst
M plone/protect/auto.py
M plone/protect/configure.zcml
Repository: plone.protect
Branch: refs/heads/master
Date: 2016-06-10T09:13:01+02:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.protect@b8c9557

Merge pull request #49 from plone/maurits-add-protect-js

Added protect.js from plone4.csrffixes.

Files changed:
A plone/protect/protect.js
M CHANGES.rst
M plone/protect/auto.py
M plone/protect/configure.zcml
@frisi
Copy link
Contributor

frisi commented Aug 24, 2016

@mauritsvanrees this change breaks plone sites that follow the instructions on https://plone.org/security/hotfix/20151006 as both packages register protect.js

zope.configuration.config.ConfigurationConflictError: Conflicting configuration actions
  For: ('resource', u'protect.js', <InterfaceClass zope.publisher.interfaces.browser.IBrowserRequest>, <InterfaceClass zope.publisher.interfaces.browser.IDefaultBrowserLayer>)
    File "plone.protect-3.0.19-py2.7.egg/plone/protect/configure.zcml", line 38.4-41.10
          <browser:resource
              name="protect.js"
              file="protect.js"
              />
    File "plone4.csrffixes-1.0.9-py2.7.egg/plone4/csrffixes/configure.zcml", line 16.2-19.6
        <browser:resource
          name="protect.js"
          file="protect.js"
          />

until today i thought i'd need to add plone4.csrffixes to my instance eggs if i want to activate the hotfix https://plone.org/security/hotfix/20151006

what's the official way to get around the above error?
we either need to update the documentation that we need to pin plone.protect = 3.0.18
or that plone4.csrfprotect is not needed anymore (did not check if the package actually does additional stuff and is still needed)

mauritsvanrees added a commit to plone/plone4.csrffixes that referenced this issue Aug 24, 2016
This adds `protect.js`, so we do not have to do this anymore.
See issue
plone/plone.protect#42
@mauritsvanrees
Copy link
Member Author

True, this gives a problem. This pull request should fix it: plone/plone4.csrffixes#22
After this is merged, and a new release of plone4.csrffixes is made, you can use the two together again.
Workaround for now would be to keep using plone.protect 3.0.18.

plone.protect 3.x is still missing a few fixes, see #48

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

No branches or pull requests

4 participants