Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Character Escape Bug With Programmatic Views #1796

Open
volosied opened this issue Feb 27, 2023 · 9 comments
Open

Character Escape Bug With Programmatic Views #1796

volosied opened this issue Feb 27, 2023 · 9 comments

Comments

@volosied
Copy link
Contributor

volosied commented Feb 27, 2023 •
edited
Loading

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
Copy link
Contributor Author

volosied commented Jun 4, 2024

@BalusC Any comment here?

@BalusC
Copy link
Member

BalusC commented Jun 4, 2024 •
edited
Loading

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.

This was referenced Jun 4, 2024
Closed
Open
@BalusC
Copy link
Member

BalusC commented Jun 15, 2024 •
edited
Loading

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
Copy link
Member

BalusC commented Sep 17, 2024

@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
Copy link
Contributor

mnriem commented Sep 18, 2024

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
Copy link
Contributor

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
Copy link
Member

BalusC commented Sep 21, 2024 •
edited
Loading

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
Copy link
Contributor

mnriem commented Sep 23, 2024

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
Copy link
Member

BalusC commented Sep 24, 2024 •
edited
Loading

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@BalusC @arjantijms @volosied @mnriem