dnnsoftware / Dnn.Platform

DNN (formerly DotNetNuke) is the leading open source web content management platform (CMS) in the Microsoft ecosystem.
https://dnncommunity.org/
MIT License
1.01k stars 745 forks source link

[Enhancement]: Prevent writing invalid module title (such as HTML, CSS, JS) #6035

Open Mostafa-Moafi opened 1 month ago

Mostafa-Moafi commented 1 month ago

Is there an existing issue for this?

Description of problem

I think that we shouldn't set HTML or JS in the module title and should prevent writing invalid code.

Description of solution

It's very simple to solve it. we should use WebUtility.HtmlEncode() when update module title.

Description of alternatives considered

No response

Anything else?

No response

Do you be plan to contribute code for this enhancement?

Would you be interested in sponsoring this enhancement?

Code of Conduct

bdukes commented 1 month ago

I agree in principle, however, we have many years of folks relying on being able to put HTML into the module title, so I fear that this change could break existing workflows for some folks. If we decide encoding is a better default, I would want to put that change behind some kind of flag, so that it only affects new sites (or at least allows old sites to opt into the previous behavior).

valadas commented 1 month ago

I agree, I have seen multiple implementations where html was used in titles. Not saying this change is wrong, but it has that implication of breaking some existing sites

Mostafa-Moafi commented 1 month ago

@bdukes @valadas ok, I understood. What can I do about it?

valadas commented 1 month ago

Hmm, I am thinking we could make it an opt-in feature (web.config, host setting, portal setting) that way the new behavior could be ON for new installs and OFF on upgrades... I think I would prefer a site setting with a UI toggle in a perfect world, that way we can even make it off upon upgrade and one could just toggle it back on only if needed....

mitchelsellers commented 1 month ago

I too woudl say that this needs to at a minimum be a setting, but I think it should be allowed most likely by default due to content. I will try to have this as topic of discussion at the next TAG/Approvers meetings

david-poindexter commented 1 week ago

@dnnsoftware/approvers should we bring this back up for discussion? I may have missed the meeting when this was discussed, so forgive me if we already have a path forward. I just noticed the PR was still pending approvals (I'm guessing because of missing setting, etc.).

jeremy-farrance commented 1 week ago

Could this be an opt-in feature on install in DNN 9 and then in DNN its on by default for new installs? And for upgrades, keep the current setting.

Since I don't see any examples above, and it might help to understand the needs and usefulness, here are some typical examples that we use often and encourage clients to use:

Add a FontAwesome icon: <i class="fa-solid fa-cake-candles"></i>Picnic on the Lake Birthday Event

Highlight words using CSS: Have you heard about the all new <span>Profile Pins</span> in the Member Profile Settings?

HTML mark (highlight) part of the title: Advantages of <mark>Hiring</mark> a Financial Advisor

Put a word in quotes: Favorite Characters from &quot;Movie Title (1999)&quot;

mitchelsellers commented 1 week ago

This was missed during the approvers meeting, so we have not yet fully talked about it.

I personally don't see a reason to blockade it, in any capacity given the expected level of trust with the users that edit and the extensive number of reasons that you would need/want people to be able to do it.

david-poindexter commented 1 week ago

I agree as long as it is controllable by a flag/setting of some sort.