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

Incomplete escaping of characters in TV (XSS) #15925

Open
Ruslan-Aleev opened this issue Dec 1, 2021 · 8 comments · May be fixed by #15936
Open

Incomplete escaping of characters in TV (XSS) #15925

Ruslan-Aleev opened this issue Dec 1, 2021 · 8 comments · May be fixed by #15936
Labels
bug The issue in the code or project, which should be addressed.

Comments

@Ruslan-Aleev
Copy link
Collaborator

Bug report

Summary

In TV caption you can specify html tags that are not escaped. Maybe for other elements it is relevant too.

Step to reproduce

Add: <script>alert('xss');</script> in TV caption and edit the resource with this TV.

Environment

MODX 2.x, MODX 3.x

@Ruslan-Aleev Ruslan-Aleev added the bug The issue in the code or project, which should be addressed. label Dec 1, 2021
@smg6511
Copy link
Collaborator

smg6511 commented Dec 1, 2021

I'll take a look at this since I've been doing a lot of work in this area. This needs to be accounted for in both the processor and the js.

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 8, 2021

I believe this is intentional, to allow additional context to be added (by admins). I've definitely added images and links in the past, and don't think this necessarily classifies as XSS.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 8, 2021

@Mark-H - Just curious what fields you've added images/links to and whether/where they render in 3.x. I can see the use of links, but am having a hard time imaging an image with the captions or descriptions without it cluttering up the UI. Perhaps we limit to a small set of tags if necessary (and maybe just for the description)? Are the captions/descriptions rendered anywhere other than the elements tree (as a qtip), search results panel, and TVs panel for resources?

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 8, 2021

That was for rendering in the resource TV panel, as in-context help/guidance. As an example, consider a "hero type" TV with a narrow height image in the description to show what the 3 options in a select/radio tv would look like.

For the caption (again, rendering in the resource tv panel) I can also imagine adding just a help link to the caption.

I don't think I have an immediate example at hand as the few sites I'm actively managing these days are not set up for non-technical clients, but I just want my objection that this should not be classified as a XSS vulnerability but a feature noted, before #15936 is considered. #15936 does go beyond just what is reported here and from a cursory review some of that would certainly be good to resolve, but I don't want to be the one to tell people the help text they set up for their clients is no longer possible in 3.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 8, 2021

OK, thanks for clarifying. I'm going to continue this back over on #15936, as the discussion is likely going to lead to changes in that PR.

@Jako
Copy link
Collaborator

Jako commented Dec 8, 2021

@smg6511
Copy link
Collaborator

smg6511 commented Dec 8, 2021

Yes, I saw that. However, it's designed to allow at least some tags, correct? From what I see, all elements' name and description fields (with the exception of TVs) should disallow html, given that where these items render to does not display html (tree qtips and search results).

@Jako
Copy link
Collaborator

Jako commented Dec 8, 2021

If the allowed tags are empty, a default list is used. But you can also just allow an invalid <nothing> tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue in the code or project, which should be addressed.
Projects
None yet
4 participants