[JENKINS-75069] Add HTML description for HTML display in ObjectMetadataAction class #314
Conversation
| * Returns the description of the object or {@code null} for the HTML content. Consumers should assume the content | ||
| * is valid HTML or plain text that needs to be formatted with {@link Jenkins#getMarkupFormatter()} | ||
| * or escaped with {@link Util#escape(String)} when being included in HTML output. | ||
| * Note: If this value is {@code null}, Consumers should get the default object description with | ||
| * {@link #getObjectDescription()}. |
There was a problem hiding this comment.
How will consumer know which is the correct case for the format? a wrong assumption will likely be a security issue.
The markup formater may not be one for HTML (it could be ascidoc, so if something returns HTML and you run it via an ADOC processor then you will likely get garbage out
There was a problem hiding this comment.
I understand your meaning. Maybe my comment is misunderstandable. What I mean is this field may be whatever you want, but should be used for the HTML display. I use the Jenkins markup formatter in the Branch API to render a nice description field. But maybe I shouldn't say it may be used with the MarkupFormatter ?
There was a problem hiding this comment.
The use of a markup formatter for things that do not come from a user should be discouraged as things get out of sync (it was a bad historic pattern in some plugins/core that we should not build on).
If this is set by some code system (e.g. prettifying some info from an external system) then in my PoV what sets this should be sanitizing it before it gets here.
Adding @daniel-beck as he has looked at similar (safe formatting of HTML) elsewhere (and in core) IIRC and may have some valuable insights here.
But additionally I am not sure where this is expected to be used. Taking arbitrary HTML form external systems could completely break the Jenkins UX.
There was a problem hiding this comment.
I understand well better now. The issue appears in the Gitea plugin in the description column of an organization. It authorizes HTML content and you may want to display HTML as is. But for now, it is simply escaped (for security reason) and displays it as plain text. And, for example, when you select the organization, there is a description below, formatted as HTML, when the description in Gitea is written using HTML tags.
Anyway, it is a pleasure to discuss these stuff with you because I have not all the historicals and don't know all the troubles you encountered.
I am open to search for another solution and attend your guidance.
There was a problem hiding this comment.
Using the configured markup formatter is wrong for the reasons mentioned.
In the past I've recommended a dependency on the specific "Safe HTML" formatter as sort of an API handling HTML sanitization, which would sidestep Markdown and similar incompatible formats being configured, but that caused trouble when the rules for the formatter changed in a way that didn't work for the use case.
The only reasonable option seems to be using an HTML sanitizer like https://github.com/OWASP/java-html-sanitizer/ directly, defining what subset of HTML we're OK with, and enforcing that.
(This is not an opinion on the specific API change here. That looks rather awkward, but I don't know enough about this plugin to really evaluate.)
There was a problem hiding this comment.
OK. If I understand well, I may propose here the use of this formatter in my getHtmlObjectDescription to sanitize the content with authorized elements, attributes... Then this method would return a safe value. Do you agree with that ?
There was a problem hiding this comment.
I will propose to use this formatter directly in my getHtmlObjectDescription, configuring it with authorized tags. Il will need to modify the pom. Do you agree ?
There was a problem hiding this comment.
FTR I am not a maintainer, so am unable to provide feedback on those questions. I can only recommend options and explain why, whether you and/or maintainers agree is a different story :)
There was a problem hiding this comment.
I think your feedback takes sense, so I will propose a solution based on our discussion and create a new pull request :) It may be refused, but if I don't test, I will never know I guess.
Testing done
This issue JENKINS-75069 intially causes the issue JENKINS-73226 in the Gitea plugin. Its goal is about adding an HTML description method (
getHtmlObjectDescription) inObjectMetadataActionclass to provide a proper way to display the content as an HTML one. A new constructor is provided, but the older one remains. The default default behavior also remains (no interface breakdown). It offers the possibility to display the value using safeHTML.This solution has been discussed through a refused pull request with one of the maintainers of Branch API.
Submitter checklist