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

Make HTML in node titles optional #171

Closed
Blinnikov opened this Issue Mar 31, 2014 · 9 comments

Comments

5 participants
@Blinnikov

Blinnikov commented Mar 31, 2014

Please consider using of innerText for <span> tag when constructing node's title to prevent HTML injection. For now if I have title of my node looking something like this: <hi>
then I will not see this title in the browser. It will be interpreted as html-tag.

The possible solution is to change line 2804

nodeTitle = "<span " + role + " class='fancytree-title'" + id + tooltip + tabindex + ">" + node.title + "</span>";

to something like this:

var span = document.createElement('span');
span.className = 'fancytree-title';
if (aria) {
    span.setAttribute('role', 'treeitem');
    span.setAttribute('id', 'ftal_' + node.key);
}
if (node.tooltip) {
    span.setAttribute('title', node.tooltip.replace(/\"/g, "&quot;"));
}
span.innerText = node.title;
nodeTitle = span.outerHTML;

@mar10 mar10 added the waiting label Apr 1, 2014

@mar10

This comment has been minimized.

Owner

mar10 commented Apr 1, 2014

I would consider html markup inside a node title a feature, not a bug ;-)
What are your concerns?

(Is there some text missing in your message?):

For now if I have title of my node looking something like this:
then I will

@mar10 mar10 added this to the 2.0.0-9 milestone Apr 1, 2014

@Blinnikov

This comment has been minimized.

Blinnikov commented Apr 1, 2014

Corrected the message. I forgot to escape html.

If you allow node's title to contain html markup it is direct way to html injection. It is possible to set title as <script>alert('Hi');</script> and you will see alert message in your browser. I think this ability should be prohibited. The other possible solution I see - to have the option to allow setting title as innerText or innerHtml of the span element.

@mar10

This comment has been minimized.

Owner

mar10 commented Apr 1, 2014

Would a new method node.setTitleSafe() help, or are you concerned about an untrusted Ajax source? Then an option {allowHtmlTitles: true} could be a solution.

What scenario are you thinking about, and can you give some references what danger you see?
I still think it's an important feature to allow HTML markup in the titles, and I am not planning to disallow this altogether.

@Blinnikov

This comment has been minimized.

Blinnikov commented Apr 1, 2014

I agree that having ability to set HTML in the node is good. But when it used properly.
I think {allowHtmlTitles: true} option would be great.

In our current project we allow end users to create items that lately will be shown in the tree structure. In this case if user creates item with closing tag in title (like </li> or </ul>) then layout will be broken. In other case user can insert malicious js-code into title. This code can redirect me to some other page that looks like mine, moreover it can chage tree structure, add fake nodes or remove existing ones. Maybe a lot of examples.

@mar10 mar10 changed the title from Setting node's title executes HTML to Make HTML in node titles optional Apr 2, 2014

@mar10 mar10 modified the milestones: 9.0.0 Backlog, 2.0.0-9 Apr 2, 2014

@mar10 mar10 removed the waiting label Apr 4, 2014

@mar10 mar10 added the enhancement label May 1, 2014

@mar10 mar10 modified the milestones: 2.1 Backlog, 9.0.0 Backlog May 1, 2014

@markbernard

This comment has been minimized.

Contributor

markbernard commented May 3, 2014

You are getting the user input so sanitize it before applying it.

@mar10

This comment has been minimized.

Owner

mar10 commented Aug 20, 2014

I would agree with @markbernard: The programmer is responsible to escape the HTML, before setting it as title. Closing this for now.

@mar10 mar10 closed this Aug 20, 2014

@albinohrn

This comment has been minimized.

albinohrn commented Oct 24, 2014

Hi!

I think that you should reconsider escaping the title upon rendering.

Raw HTML in titles can be seen as a feature in some cases but now it's not always treated as raw HTML. When I look at the edit extension I can see that HTML in node titles are not allowed. When added back they are always escaped.

To make the behaviour more flexible and explicit I would suggest an option, escapeTitle. When enabled the title would be escaped using FT.escapeHtml() before it is rendered. The benefit of escaping upon rendering vs a node.setTitleSafe() is that you still have the correct value in the model.

With the escapeTitle option you can decide if you should render your titles as raw HTML or not. The option would default to false.

If you are interested I could send you a pull-request where I have added the option.

/Albin

@mar10

This comment has been minimized.

Owner

mar10 commented Apr 8, 2016

Due to multiple requests, going to re-consider this

@mar10 mar10 reopened this Apr 8, 2016

@mar10 mar10 closed this in 32ad6df Apr 16, 2016

@pmeijer

This comment has been minimized.

pmeijer commented Feb 10, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment