-
Notifications
You must be signed in to change notification settings - Fork 85
[JENKINS-75069] Add HTML description for HTML display in ObjectMetadataAction class #314
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
Open
GGY-AH
wants to merge
2
commits into
jenkinsci:master
Choose a base branch
from
GGY-AH:JENKINS-75069
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.