search

jakartaee / faces

Jakarta Faces
Other
109 stars 55 forks source link

Character Escape Bug With Programmatic Views #1796

Open volosied opened 1 year ago

volosied commented 1 year ago

This this a problem sprung from #1581 and seen in MyFaces.

The special characters, such as less-than, greater-than (< >), are escaped in MyFaces, but not in Mojarra.

If they are escaped, then the <html> tag will be visible on the rendered page. By default MyFaces escapes these characters, and the RenderKit Documentation specifics that escape should be enabled by default (i.e. true) for component family jakarta.faces.Output and renderer jakarta.faces.Text. These are what UIOutput is registered with. (Maybe this issue could be a bug in Mojarra?)

See escape attribute - https://jakarta.ee/specifications/faces/4.0/renderkitdoc/HTML_BASIC/jakarta.faces.Outputjakarta.faces.Text.html

For a concrete example, the hello facelet from the TCK would render this output in MyFaces: 

&lt;html xmlns=&quot;http://www.w3.org/1999/xhtml&quot;&gt;
<body>
<form id="form" name="form" method="post" action="[/test-faces40-javaPage/hello.xhtml](view-source:http://localhost:9080/test-faces40-javaPage/hello.xhtml)" enctype="application/x-www-form-urlencoded"><span id="form:message"></span><input id="form:button" name="form:button" type="submit" value="Do action" /><input type="hidden" name="form_SUBMIT" value="1" /><input type="hidden" name="jakarta.faces.ViewState" id="j_id__v_0:jakarta.faces.ViewState:1" value="YTk0M2Q0MjBiYTcxOWY2NjAwMDAwMDAx" autocomplete="off" />
</form>
</body>
&lt;/html&gt;
image
volosied commented 5 months ago

@BalusC Any comment here?

BalusC commented 5 months ago

I haven't specced/implemented this so I don't know offhand.

@arjantijms can you take a look? perhaps there was an oversight in spec/impl in this area? I can at earliest take a look the weekend of 14-15 due to vacaciones next weekend.

BalusC commented 5 months ago

for component family jakarta.faces.Output and renderer jakarta.faces.Text. These are what UIOutput is registered with.

We may have hit a hole in the spec. These are with jakarta.faces.component.html.HtmlOutputText in mind. When I use new HtmlOutputText() instead of new UIOutput() then it gets properly escaped by Mojarra's HTML renderer.

The UIOutput javadoc doesn't say any word about escaping: https://jakarta.ee/specifications/platform/10/apidocs/jakarta/faces/component/uioutput

The HtmlOutputText javadoc does mention it: https://jakarta.ee/specifications/platform/10/apidocs/jakarta/faces/component/html/htmloutputtext

The RenderKit doc is for the HTML renderer and the attribute list only matches HtmlOutputText while UIOutput is a superclass: https://jakarta.ee/specifications/faces/4.0/renderkitdoc/html_basic/jakarta.faces.outputjakarta.faces.text

However, the UIOutput itself is indeed also of component family jakarta.faces.Output and renderer jakarta.faces.Text.

@arjantijms, WDYT? Is MyFaces correct or is the RenderKit doc wrong? I personally have the impression that UIXyz superclasses don't have any association with HTML rendering, those are only the HtmlXyz subclasses. But, strictly speaking, if we follow the spec in its current form letter by letter, MyFaces is correct.

BalusC commented 2 months ago

@arjantijms @mnriem any feedback?

Summarized: Is it expected that var output = new UIOutput() has a default of escape="true"? Or is that only applicable to var output = new HtmlOutputText()?

mnriem commented 2 months ago

As far as I recall UIOuput or any of the UIxxx classes are not part of the HTML_BASIC RenderKit specification. So if an implementation is using UIOutput directly as part of any RenderKit then the behavior of it is not formally specified nor was it ever intended to be. So any rendering is out of scope for any direct UIxxxx instance and as such no expectations regarding rendering can or should be assumed.

arjantijms commented 2 months ago

Indeed, UIOutput is supposed to not know anything about escaping. Escaping only comes into play for subclasses that know about (in this case) HTML. Perhaps we need to pull up setEscape to UIOutput and specify it as applying escaping for whatever render target is currently used by the view, but that does open up some other concerns.

In the TCK test it's of course explicitely intended that escape is off, otherwise, indeed, would not render as it should. The test clearly does not expect that escaped output is sent to the client here.

BalusC commented 2 months ago

So any rendering is out of scope for any direct UIxxxx instance and as such no expectations regarding rendering can or should be assumed.

So, the spec needs adjustment/clarification in this regard? E.g. "when component type equals component family then disregard any defaults implied by render kit" or so?

In the TCK test it's of course explicitely intended that escape is off, otherwise, indeed, would not render as it should. The test clearly does not expect that escaped output is sent to the client here.

Clearly. But the TCK test uses UIOutput instead of HtmlOutputText. So either the TCK or spec definitely needs adjustment.

Currently, UIOutput returns jakarta.faces.Output component family and jakarta.faces.Text renderer type, which is in the eyes of the render kit exactly the same as HtmlOutputText. The HtmlOutputText has only an explicitly set component type while in UIOutput the component type is exactly the same as component family, like as all other UIxxx classes. We could probably use this distinction to clarify matters in the spec.

[update] oh there's no API available which returns the component type used to create the component. I mixed COMPONENT_TYPE constant with getComponentType(). So, how would the render kit know at all? Sniffing getClass() and see if it matches jakarta.faces.component.UIxxx? How about instances wrapped in proxies?

mnriem commented 2 months ago

The specification should probably clarify that a UIxxx class is to be used as a super class only and is NOT to be used directly for rendering and as such the component family and renderer type really are only defaults and only carry real meaning for a RenderKit specific sub class. In essence what and how anything is rendered is up to the available RenderKits and for the fHTML_BASIC RenderKit what is supported should be clear and crisp. Thoughts? @arjantijms

BalusC commented 2 months ago

Makes sense. But there exist exceptions such as UIParameter, UISelectItem, UISelectItemGroup, UIViewAction and UIViewParameter which should also be covered in the explanation (their common denominator is the absence of a COMPONENT_TYPE constant and a default renderer type of null).

And the spec should state that all components having a COMPONENT_TYPE constant should be created via Application#createComponent() in order to get the proper implementation. But this in turn makes me wonder why these haven't been abstract classes from the beginning on?