search

plone / plone.app.content

Various views for Plone, such as folder_contents, as well as general content infrastructure, such as base classes and name choosers.
https://pypi.org/project/plone.app.content/
7 stars 32 forks source link

Fix escaped html entities in vocabulary items #273

Closed petschki closed 9 months ago

petschki commented 11 months ago

see https://github.com/plone/Products.CMFPlone/issues/3874

I tried to change the parser here https://github.com/plone/Products.PortalTransforms/blob/master/Products/PortalTransforms/transforms/safe_html.py#L187 to html.XHTMLParser(encoding="utf-8", resolve_entities=False, recover=True) but this removes all entities in the string and leads to many testing failures.

Note that this only affects @@getVocabulary BrowserView when no attributes are passed in the request and for this special & case. Wonder if there is a more generic solution for more cases out there? Update: I think its ok to unescape all entities.

mister-roboto commented 11 months ago

@petschki 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!

petschki commented 11 months ago

@jenkins-plone-org please run jobs

mauritsvanrees commented 11 months ago

Seems okay, but I am investigating possible security implications of this PR, so have reverted it to Draft status for now. We have had a few security fixes in this area the past few years, so it is tricky to be sure if something is safe.

jensens commented 9 months ago

Looks good to me. It solves escaping, scrubbing is still in there. I do not see how this could touch any security relevant as it delivers vocabs as JSON to the browser. I am not sure if the id needs this.

petschki commented 9 months ago

@jenkins-plone-org please run jobs

petschki commented 9 months ago

@mauritsvanrees I'd like to merge this. Do you still have security concerns?