SelectOneMenu: html reserved chars are shown as entities

[NicolaIsotta]

Describe the bug

In the selected value of a p:selectOneMenu, html reserved chars (eg: &, <, >) are shown as entities:

(If itemEscaped="false" chars are not shown at all)

Reproducer

<p:selectOneMenu>
	<f:selectItem itemValue="A" itemLabel="A" />
	<f:selectItem itemValue="B" itemLabel="#{testView.label}"/>
	<f:selectItem itemValue="C" itemLabel="&amp; &gt; &lt;"/>
</p:selectOneMenu>

@Getter
private String label = "<&>";

Expected behavior

No response

PrimeFaces edition

Community

PrimeFaces version

12.0.0

Theme

No response

JSF implementation

Mojarra

JSF version

2.3.18

Java version

11

Browser(s)

No response

[melloware]

Reproducer:
pf-9336.zip

MonkeyPatch:

if (PrimeFaces.widget.SelectOneMenu) {
    PrimeFaces.widget.SelectOneMenu.prototype.renderSelectItem = function(item, isGrouped) {
        var content = "";
        var $item = $(item);
        var label, dataLabel;
        var title = $item.data("title");
        var escape = $item.data("escape");
        var cssClass;

        if (item.tagName === "OPTGROUP") {
            label = $item.attr("label");
            if (escape) {
                label = $("<div>").text(label).html();
            }
            cssClass = "ui-selectonemenu-item-group ui-corner-all";
        } else { //OPTION
            if (escape) {
                label = $item.html();
                if ($item.text() === "&nbsp;") {
                    label = $item.text();
                }
            } else {
                label = $item.text();
            }
            
            cssClass = "ui-selectonemenu-item ui-selectonemenu-list-item ui-corner-all";
            if (isGrouped) {
                cssClass += " ui-selectonemenu-item-group-children";
            }
        }

        dataLabel = this.escapeHTMLIfNecessary(label.replace(/(<([^>]+)>)/gi, ""));

        if ($item.data("noselection-option")) {
            cssClass += " ui-noselection-option";
        }

        content += '<li class="' + cssClass + '" tabindex="-1" role="option"';
        if (title) {
            content += ' title="' + title + '"';
        }
        if ($item.is(':disabled')) {
            content += ' disabled';
        }
        content += ' data-label="' + dataLabel + '"';
        content += '>';
        content += label;
        content += '</li>';

        if (item.tagName === "OPTGROUP") {
            content += this.renderSelectItems($item, true);
        }

        return content;
    };

    PrimeFaces.widget.SelectOneMenu.prototype.escapeHTMLIfNecessary = function(value) {
            return String(value).replace(/[<>"'`=\/]/g, function(s) {
                return PrimeFaces.entityMap[s];
            });
     };
}

[NicolaIsotta]

My example is still broken @melloware
I'm pretty sure there's no need for this:

primefaces/primefaces/src/main/resources/META-INF/resources/primefaces/forms/forms.selectonemenu.js

Lines 1555 to 1557 in d450cb7

	if (escape) {
	dataLabel = PrimeFaces.escapeHTML(label.replace(/(<([^>]+)>)/gi, ""));
	}

The value received from $item.html() is an already escaped value:

primefaces/primefaces/src/main/resources/META-INF/resources/primefaces/forms/forms.selectonemenu.js

Lines 1540 to 1545 in d450cb7

	if (escape) {
	label = $item.html();
	if ($item.text() === " ") {
	label = $item.text();
	}
	}

I removed lines 1555 to 1557 and now it works flawlessly.

[melloware]

Let me look. I had your whole example working yesterday?

[melloware]

@NicolaIsotta did you run my reproducer I attached. it has that patch as well as I properly itemEscaped your items.

<f:selectItem itemValue="A" itemLabel="A"/>
<f:selectItem itemValue="B" itemLabel="#{testView.string}" itemEscaped="false"/>
<f:selectItem itemValue="C" itemLabel="&amp; &gt; &lt;" itemEscaped="false"/>

[NicolaIsotta]

ampersand is not html code, Mojarra's h:selectOneMenu handles it without needing itemEscaped="false".
The problem is that the js at line 1556 escapes the label but it's already escaped.
If you have a itemLabel="&" and try to debug the js, what happens is:

• after line 1541 the value of label is &
• after line 1556 the value of label is &

So there's a double escape of the value, one of the two can be safely removed.

[melloware]

OK 2 different issues.
First issue:

<f:selectItem itemValue="B" itemLabel="#{testView.string}" itemEscaped="false"/>

Was wrong without my fix above because data-label="" so when you selected it the value was blank. So that was the fix I made above for that specific scenario and now the data-label="<&>" which is correct and when you select it is correct.

Second Issue:

<f:selectItem itemValue="C" itemLabel="&amp;" itemEscaped="true"/>
<f:selectItem itemValue="D" itemLabel="&amp;" itemEscaped="false"/>

Renders in the dropdown like this:

However selecting the first value with itemEscaped="true" does this:

Which is correct because as you mentioned it is NOT HTML so it renders the raw text.

Selecting the itemEscaped="false" does this:

Which is correct because you said false which means you want to render HTML so it converts & to &.

So I guess I am still unclear of your 3 scenarios above exactly what is wrong?

[NicolaIsotta]

I'll try to be clearer with an example.
This is the xhtml:

<p:selectOneMenu>
	<f:selectItem itemValue="A" itemLabel="#{testView.ampersand}" />
</p:selectOneMenu>
<h:selectOneMenu>
	<f:selectItem itemValue="A" itemLabel="#{testView.ampersand}" />
</h:selectOneMenu>

And this is the java:

@Getter
@Setter
private String ampersand = "&";

The rendered output, with both Mojarra and MyFaces, is:


(The primefaces menu could show "&" at first, just trigger selection to show the wrong value)

I expect primefaces to show "&" as both the dropdown item and the current value, just like the standard h:selectOneMenu. It does not make any sense to show two different values.

In my message #9336 (comment) I wrote a possible fix, let me know if I need to open a new issue and/or a pull request.

[melloware]

OK that but your ORIGINAL issue was <f:selectItem itemValue="B" itemLabel="#{testView.label}"/> with

@Getter
private String label = "<&>";

are you saying removing the following fixes it:

if (escape) {  
     dataLabel = PrimeFaces.escapeHTML(label.replace(/(<([^>]+)>)/gi, "")); 
 } 

Let me submit a PR and let the Integration tests run.

[melloware]

OK two Integration test failures:

SelectOneMenu002Test.testEscape:89 expected: <German Cars> but was: <German Cars">German Cars>
SelectOneMenu011Test.testBasic:45 expected: <true> but was: <false>

[melloware]

OK so here is the input.

 SelectItemGroup g1 = new SelectItemGroup("<span style=\"color:darkgreen\">German</span> Cars");
g1.setEscape(false);

[melloware]

With the PR included this is what happens..

[melloware]

I think the major difference between h:selectOneMenu and PF is you can't format HTML in the h:selectOne it just renders a browser <select> element where ours allows custom styling etc so we have to defend against XSS.

[NicolaIsotta]

I saw. I'll try find another solution.

[melloware]

I will close for now as it works as designed but let me know if you come up with a solution or you can just leave yours MonkeyPatched to leave that code out.

[melloware]

@josefplch I just wrote an Integration test.

I added GitHub "9336" Quoted to the SelectItems...

new SelectItem("GitHub \"9336\" Quoted", "GitHub \"9336\" Quoted")

Then in my Test I check the value...

 Assertions.assertEquals("GitHub \"9336\" Quoted", options.get(8).getText());

And the UI:

And the Generated HTML:

<li class="ui-selectonemenu-item ui-selectonemenu-list-item ui-corner-all ui-selectonemenu-item-group-children" tabindex="-1" role="option" 
data-label="GitHub &quot;9336&quot; Quoted" aria-selected="false" id="form:selectonemenu_8">GitHub "9336" Quoted</li>

So everything is passing right now with NO changes. What am I doing differently than you?

[josefplch]

@melloware Well, that’s weird… Without my modification, with the same SelectItem definition, in both Chrome and Firefox, I get:

<li class="ui-selectonemenu-item ui-selectonemenu-list-item ui-corner-all ui-selectonemenu-item-group-children" tabindex="-1" role="option"
data-label="GitHub " 9336"="" quoted"="" aria-selected="true" id="form:selectonemenu_1">GitHub "9336" Quoted</li>

The label logged via JavaScript is GitHub "9336" Quoted, so the extra characters are added by browser when trying to fix the HTML structure.

I use PrimeFaces 12.0.0, running it with Java 11.0.17+8, Mojarra 2.3.18.

[melloware]

Oh yeah I am running 13.0.0-SNAPSHOT in my Integration Tests but it has the above MonkeyPatch included which is why I am confused you needed to add. var dataLabel = escape ? label.replaceAll('"', '"') : PrimeFaces.escapeHTML(label); as mine test is working without that?

[melloware]

Can you try this MonkeyPatch which is exactly the code my Integration Test is using?

if (PrimeFaces.widget.SelectOneMenu) {
    PrimeFaces.widget.SelectOneMenu.prototype.renderSelectItem = function(item, isGrouped) {
        var content = "";
        var $item = $(item);
        var label;
        var title = $item.data("title");
        var escape = $item.data("escape");
        var cssClass;

        if (item.tagName === "OPTGROUP") {
            label = $item.attr("label");
            if (escape) {
                label = $("<div>").text(label).html();
            }
            cssClass = "ui-selectonemenu-item-group ui-corner-all";
        } else { //OPTION
            if (escape) {
                label = $item.html();
                if ($item.text() === "&nbsp;") {
                    label = $item.text();
                }
            } else {
                label = $item.text();
            }
            cssClass = "ui-selectonemenu-item ui-selectonemenu-list-item ui-corner-all";
            if (isGrouped) {
                cssClass += " ui-selectonemenu-item-group-children";
            }
        }

        dataLabel = PrimeFaces.escapeHTML(label.replace(/(<([^>]+)>)/gi, ""));
        if (escape) {
            dataLabel = dataLabel.replace("&amp;", "&");
        }

        if ($item.data("noselection-option")) {
            cssClass += " ui-noselection-option";
        }

        content += '<li class="' + cssClass + '" tabindex="-1" role="option"';
        if (title) {
            content += ' title="' + title + '"';
        }
        if ($item.is(':disabled')) {
            content += ' disabled';
        }
        content += ' data-label="' + dataLabel + '"';
        content += '>';
        content += label;
        content += '</li>';

        if (item.tagName === "OPTGROUP") {
            content += this.renderSelectItems($item, true);
        }

        return content;
    }
}

[josefplch]

Sure. It handles the quotes correctly, but after click, the label for new SelectItem(1, "< <i>italics</i>"), rendered in dropdown as < <i>italics</i>, becomes < &lt;i&gt;italics&lt;/i&gt; – that is, the original issue remains.

[melloware]

But i believe that is correct behavior. If you want it like that don't you need escape="false" else we must escape it properly.

[josefplch]

When you show <i>italics</i>, it already is escaped. The unescaped version would be italics. Showing <i>italics</i> means double escaping which I definitely consider a bug. Generally, I don’t want the label to change when clicked.

[melloware]

@josefplch i think I got it if you want to try this one... I updated my integration tests with your new scenario.

if (PrimeFaces.widget.SelectOneMenu) {
    PrimeFaces.widget.SelectOneMenu.prototype.renderSelectItem = function(item, isGrouped) {
        var content = "";
        var $item = $(item);
        var label, dataLabel;
        var title = $item.data("title");
        var escape = $item.data("escape");
        var cssClass;

        if (item.tagName === "OPTGROUP") {
            label = $item.attr("label");
            if (escape) {
                label = $("<div>").text(label).html();
            }
            cssClass = "ui-selectonemenu-item-group ui-corner-all";
        } else { //OPTION
            if (escape) {
                label = $item.html();
                if ($item.text() === "&nbsp;") {
                    label = $item.text();
                }
            } else {
                label = $item.text();
            }
            
            cssClass = "ui-selectonemenu-item ui-selectonemenu-list-item ui-corner-all";
            if (isGrouped) {
                cssClass += " ui-selectonemenu-item-group-children";
            }
        }

        dataLabel = this.escapeHTMLIfNecessary(label.replace(/(<([^>]+)>)/gi, ""));

        if ($item.data("noselection-option")) {
            cssClass += " ui-noselection-option";
        }

        content += '<li class="' + cssClass + '" tabindex="-1" role="option"';
        if (title) {
            content += ' title="' + title + '"';
        }
        if ($item.is(':disabled')) {
            content += ' disabled';
        }
        content += ' data-label="' + dataLabel + '"';
        content += '>';
        content += label;
        content += '</li>';

        if (item.tagName === "OPTGROUP") {
            content += this.renderSelectItems($item, true);
        }

        return content;
    };

    PrimeFaces.widget.SelectOneMenu.prototype.escapeHTMLIfNecessary = function(value) {
            return String(value).replace(/[<>"'`=\/]/g, function(s) {
                return PrimeFaces.entityMap[s];
            });
     };
}

[josefplch]

@melloware Results for the updated MonkeyPatch:

new SelectItem(1, "#1 < <i>italics</i>", null, false, true)
→ option: #1 < <i>italics</i>, label: #1 < <i>italics</i>

new SelectItem(2, "#2 < <i>italics</i>", null, false, false)
→ option: #2 < <i>italics</i>, label: #2 < italics

I think these two items should behave the same way – keep <i>italics</i> in the label (which they do with my MonkeyPatch modification). It is not important whether the label is escaped by Java or manually, is it? On the other hand, it keeps the behaviour of PrimeFaces 11.

[melloware]

I think it does. Can you post your monkey patch I can run it against the existing integration tests which are all passing.

[josefplch]

Sure.

if (PrimeFaces.widget.SelectOneMenu) {
    PrimeFaces.widget.SelectOneMenu.prototype.renderSelectItem = function(item, isGrouped) {
        var content = "";
        var $item = $(item);
        var label;
        var title = $item.data("title");
        var escape = $item.data("escape");
        var cssClass;
        
        if (item.tagName === "OPTGROUP") {
            label = $item.attr("label");
            if (escape) {
                label = $("<div>").text(label).html();
            }
            cssClass = "ui-selectonemenu-item-group ui-corner-all";
        } else { //OPTION
            if (escape) {
                label = $item.html();
                if ($item.text() === "&nbsp;") {
                    label = $item.text();
                }
            } else {
                label = $item.text();
            }
            cssClass = "ui-selectonemenu-item ui-selectonemenu-list-item ui-corner-all";
            if (isGrouped) {
                cssClass += " ui-selectonemenu-item-group-children";
            }
        }

        // The original MonkeyPatch used: "var dataLabel = label;" which allowed HTML injection.
        var dataLabel = escape ? label.replaceAll('"', '&quot;') : PrimeFaces.escapeHTML(label);
        
        if ($item.data("noselection-option")) {
            cssClass += " ui-noselection-option";
        }

        content += '<li class="' + cssClass + '" tabindex="-1" role="option"';
        if (title) {
            content += ' title="' + title + '"';
        }
        if ($item.is(':disabled')) {
            content += ' disabled';
        }
        content += ' data-label="' + dataLabel + '"';
        content += '>';
        content += label;
        content += '</li>';

        if (item.tagName === "OPTGROUP") {
            content += this.renderSelectItems($item, true);
        }

        return content;
    }
}

[melloware]

It looks like the Integration tests are passing but your fix has 1 failing scenario that mine does not.

Here is a runnable reproducer:
pf-9336.zip

From your Stack Overflow pos your scenario 4:

new SelectItem("< <i>italics</i>", "< <i>italics</i>", "", false, true)

Is broken with your fix and will not select the item. Where with mine the item is selected properly.

It looks properly in the dropdown but the value is NOT selectable.

[melloware]

If I change your monkeypatch to my escapeIfNecessary it all works including the scenario above

var dataLabel = escape ? label.replaceAll('"', '"') : this.escapeHTMLIfNecessary(label);

[josefplch]

@melloware Thanks for your effort, but for some reason, my fix works in my product and all the items are both rendered properly and selectable as expected. It passed both our automatic and manual tests. Unfortunately, I don’t have any more time to explore the issue, so I will just stick with my solution; I hope I have at least contributed to the extension of officially tested cases.

[koan00]

Does this also effects version 11? I just got a user report of this issue on a build running version 11.0.8. The selection list shows "This & That" but the selected value shows "This & That"

[melloware]

Yep you can use this MonkeyPatch for your 11.0.8 release as well.

[koan00]

So to track this for the version 11, I assume just create a new issue and link it to this one?

[melloware]

No this is community tracker only. For 11 you would need to report through your PRO account.

See: https://github.com/primefaces/primefaces#community--elite--pro