Dnn.Platform icon indicating copy to clipboard operation
Dnn.Platform copied to clipboard

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

Open Mostafa-Moafi opened this issue 1 year ago • 9 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

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?

  • [X] Yes

Would you be interested in sponsoring this enhancement?

  • [ ] Yes

Code of Conduct

  • [X] I agree to follow this project's Code of Conduct

Mostafa-Moafi avatar May 14 '24 06:05 Mostafa-Moafi

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).

bdukes avatar May 14 '24 12:05 bdukes

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

valadas avatar May 14 '24 17:05 valadas

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

Mostafa-Moafi avatar May 14 '24 20:05 Mostafa-Moafi

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....

valadas avatar May 15 '24 03:05 valadas

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

mitchelsellers avatar May 21 '24 19:05 mitchelsellers

@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.).

david-poindexter avatar Jun 23 '24 18:06 david-poindexter

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;

jeremy-farrance avatar Jun 23 '24 19:06 jeremy-farrance

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.

mitchelsellers avatar Jun 24 '24 06:06 mitchelsellers

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

david-poindexter avatar Jun 26 '24 20:06 david-poindexter