folder_contents escapes "&" wrong in content titles (to "&")

[laulaz]

What I did:

I added a content with a "&" in its name : Foo & Bar.

What I expect to happen:

See the right content title when exploring through folder_contents.

What actually happened:

I see the html code & instead of & in content title.

What version of Plone/ Addons I am using:

buildout.coredev, 6.0 branch It was working correctly on Plone 5.2.

[ldaverio]

My 2 cents, based on Plone instances I can play with:

• Plone 5.2.4 / py27 doesn't seem to have the problem
• Plone 5.2.5 / py39 does have it

After migrating from 5.2.5 to 5.2.7, we ran into a similar problem in the menu bar, but it went away for some reason (maybe some cached templates were refreshed).

But my guess, at the moment, is that the problem in folder_contents appeared with Plone 5.2.5. Or with Python 3. I need to explore both hypotheses.

[mauritsvanrees]

I confirm this is a problem in latest 5.2 and 6.0 on Python 2 and 3. It is caused by a security fix for this vulnerability. The problem should be caused since Plone 5.2.5 by this plone.app.content PR.

I thought I had checked this and that it worked fine. So now I wonder if I overlooked something, or if I thought: yes, this breaks the folder contents a bit, but at least I know it is safe.

I expect this is tricky to fix without reintroducing this security vulnerability, either in this spot or somewhere else.

[frapell]

@mauritsvanrees I don't think the proper fix for this would be in @@getVocabulary, this should be handled by whoever consumes the returned JSON.

See for instance the example I put in https://github.com/plone/plone.app.content/issues/244 The titles for items are plain wrong, like an empty string instead of the actual correct title of <input> or <span>Title<p></p></span> instead of the correct <span>Title<p>

Making sure there is no XSS should be handled by the pat-structure pattern when presenting the information, would you agree? By the way, do you have a title example I can use where an XSS would be triggered in folder_contents if I remove these lines? https://github.com/plone/plone.app.content/blob/60c7c3798b37a3473978187f6506420ca658a495/plone/app/content/browser/vocabulary.py#L240-L241

[frapell]

Just to be clear... removing those 2 lines does fix this issue... Now, I don't know if I am introducing some XSS vulnerability, cannot find an example on my own

[frapell]

@mauritsvanrees Any thoughts on this issue?

[mauritsvanrees]

Just to be clear... removing those 2 lines does fix this issue... Now, I don't know if I am introducing some XSS vulnerability, cannot find an example on my own

What we want to prevent is that a title like this pops up an alert:

Bad title <script>alert('very bad')</script>

It could show up when viewing the page, when searching for it, when shown in the global sections, when in folder contents if contained by another folder, or when in folder contents if the title is of the current folder.

In other words: it is tricky to be sure that it both works safely and shows nicely in all places.

[yurj]

https://github.com/plone/mockup/tree/master/src/pat/structure/templates

I think the templates should decode the title (& -> &). These are tinymce templates, but don't know if it is possible and how it works...

[frapell]

@mauritsvanrees Yeah, agreed that should be prevented, however fixing it in a way that the API response returns a wrong title is a mistake IMHO... I added a document with the title you suggested, and when I see it there is no alert and it shows up correct in navigation, breadcrumbs and the view template for the item itself as expected

Folder contents though shows up wrong

Removing those 2 lines in plone.app.content

diff --git a/plone/app/content/browser/vocabulary.py b/plone/app/content/browser/vocabulary.py
index 7db38fc..3052fe9 100644
--- a/plone/app/content/browser/vocabulary.py
+++ b/plone/app/content/browser/vocabulary.py
@@ -237,8 +237,6 @@ class BaseVocabularyView(BrowserView):
                         val = val[len(base_path) :]
                     if key not in translate_ignored and isinstance(val, str):
                         val = translate(_(safe_text(val)), context=self.request)
-                    if isinstance(val, (bytes, str)):
-                        val = transform.scrub_html(val)
                     item[key] = val
                     if key == "getMimeIcon":
                         item[key] = None

Fixes the issue and no alert is shown

If XSS is possible somewhere else, it should be fixed there, not by having the API call return invalid data...

[frapell]

@mauritsvanrees I noticed that the test you created was related to some bad actor using the fullname for XSS, I tried that as well, but cannot trigger the issue...

I believe if someone wants to be called Bobby tables, they should be able to! :P

[ewohnlich]

My issue is on this line https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/vocabulary.py#L265 - I just have a SimpleVocabulary so no attributes. The vocabulary term is "Pediatric Blood & Cancer" and I am using plone.app.z3cform.widget.AjaxSelectFieldWidget. So what happens is that BaseVocabularyView is calling scrub_html on the value but then AjaxSelectFieldWidget expects plain text, not HTML, so it's escaped again. This is only a problem in plone.app.content 3.8.8, 3.8.7 does not escape those vocab values. I don't understand enough about why this change was made, so I'll just say the issue is that it is now producing HTML instead of plain text but at least some consumers of this data are expecting (and thus escaping) plain text.

[mauritsvanrees]

Fixed by https://github.com/plone/plone.app.content/pull/250