fix: disable CSRF protection on @history endpoint

[nileshgulia1]

When accessing endpoint on a successful GET call to @history I get below error/warning.

aborting transaction due to no CSRF protection on url http://localhost:3000/en/page/@history/0

---

📚 Documentation preview 📚: https://plonerestapi--1877.org.readthedocs.build/

[mister-roboto]

@nileshgulia1 thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[davisagli]

We need to understand the context better here. This error only happens if a GET request is trying to write changes to the ZODB. In theory that should not happen if we're just reading the history. So let's check and make sure this doesn't indicate a problem we need to fix. Someone needs to debug in https://github.com/plone/plone.protect/blob/master/plone/protect/auto.py#L283 and check what objects are in registered

[nileshgulia1]

@davisagli Thanks for information. I looked into this and found that plone.protect checks if the parent of current context (which is the obj stored in registered )implements the IPortletAssignment interface here https://github.com/plone/plone.protect/blob/213f1c4da8cc7ce330edcc813868bbcdc2d1eecb/plone/protect/auto.py#L259

I tried implementing in the get @history endpoint, thus instead of providing IDisableCSRFProtection, we could provide on the parent, thus:

See in ef11130

I would like to have your thoughts on this. Please also see https://github.com/plone/plone.protect/blob/213f1c4da8cc7ce330edcc813868bbcdc2d1eecb/plone/protect/auto.py#L246-L250

[ale-rt]

alsoProvides(self.context.aq_parent, IPortletAssignment)

This is a casual comment: I did not fully grasped your issue 🤣
Anyway, it looks to me you either should fix your class in Python code or make an upgrade step.

[nileshgulia1]

alsoProvides(self.context.aq_parent, IPortletAssignment)

This is a casual comment: I did not fully grasped your issue 🤣 Anyway, it looks to me you either should fix your class in Python code or make an upgrade step.

@ale-rt Ignore that line please :)

[ale-rt]

alsoProvides(self.context.aq_parent, IPortletAssignment)

This is a casual comment: I did not fully grasped your issue 🤣 Anyway, it looks to me you either should fix your class in Python code or make an upgrade step.

@ale-rt Ignore that line please :)

I will ignore it when it's gone 🙈

[davisagli]

I looked into this and found that plone.protect checks if the parent of current context (which is the obj stored in registered )implements the IPortletAssignment interface here https://github.com/plone/plone.protect/blob/213f1c4da8cc7ce330edcc813868bbcdc2d1eecb/plone/protect/auto.py#L259
I tried implementing in the get @history endpoint, thus instead of providing IDisableCSRFProtection, we could provide on the parent, thus:

Sorry, this is completely wrong and I don't think you've understood what plone.protect is trying to do yet. The parent of a content item is never a portlet assignment.

Let me try to explain what plone.protect is doing a bit more.

In general, plone.protect tries to stop any changes from being written to the database if the user is logged out and making a GET request, because that is rarely intended and could be a CSRF attack (where an attacker tricked the user into visiting a page which loads the GET request from Plone). There are some exceptions where this is allowed:

1. if the request includes a valid CSRF token, proving that the user already loaded some other page in Plone
2. if the specified objects that were written are considered "safe". This includes image scales and portlet assignments. We consider them safe because they are frequently written as a side effect of anonymous requests, and changing this would have required difficult redesign of these parts of Plone.

My point about this PR is that we haven't yet identified what object is being written in this request. Until we do that, we can't evaluate whether it is safe. So let's please focus on identifying the object instead of bypassing the protections.

My guess is that is some structure that gets created by CMFEditions which is created implicitly if the object doesn't have a history yet. But assuming I'm correct, that should be fixed by either updating CMFEditions to not write the new empty history, or updating plone.protect to consider that object safe, and then we don't need to disable anything here.

Edited to add: Ah, I see you said the object which was modified is the parent of the current context. Hmm. Then we need to see if we can find why that is happening. That doesn't sound like something I would expect from reading the history.

[nileshgulia1]

@davisagli My issue is exactly what you mentioned in last two lines above in your comment. Although plone.protect applies CSRF checks to all requests by default, I'm also aware about the exception cases above and also mentioned along these lines https://github.com/plone/plone.protect/blob/213f1c4da8cc7ce330edcc813868bbcdc2d1eecb/plone/protect/auto.py#L260-L271

Certainly this issue only occurs when requesting a specific version of a current obj. such as http://localhost:3000/en/parent/child/@history/1

plone.restapi/src/plone/restapi/serializer/dxcontent.py

Line 90 in 6e2ab6b

obj = self.getVersion(version)

I will take a step back and debug it this weekend let's see what I can find. Thank you anyway for the explanation!

[nileshgulia1]

Edited to add: Ah, I see you said the object which was modified is the parent of the current context. Hmm. Then we need to see if we can find why that is happening. That doesn't sound like something I would expect from reading the history.

@davisagli This is happening when we are managing the parent "current state" using parent._p_changed. https://github.com/plone/Products.CMFEditions/blob/c9ff225ffb7edefccc98b0c4b3db6a8b2dd0eeb0/Products/CMFEditions/utilities.py#L139

Even if we are restoring the original state above, the plone.protect is marking it as "registered" for writing to db. https://github.com/plone/plone.protect/blob/213f1c4da8cc7ce330edcc813868bbcdc2d1eecb/plone/protect/auto.py#L220

Should we have a transaction savepoint that we can rollback like we did a level before when calling that wrap in Product.CMFEditions itself?

[davisagli]

@nileshgulia1 Good debugging. The problem seems to be that wrap() is marking the object's internal state to indicate that it hasn't changed, but it isn't doing anything to remove the object from _registered_objects on the ZODB connection (obj._p_jar). In other words, setting _p_changed has a side effect of adding to _registered_objects which wrap() isn't accounting for.

I think we should try updating https://github.com/plone/Products.CMFEditions/blob/c9ff225ffb7edefccc98b0c4b3db6a8b2dd0eeb0/Products/CMFEditions/CopyModifyMergeRepositoryTool.py#L518 to move it one line earlier, before the rollback, and see if that helps. I did a quick test locally and that seems to help. Let's make a PR for CMFEditions and see if all tests pass.

[davisagli]

Fixed the root cause in plone/Products.CMFEditions#120 instead.