Skip to content

[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
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.io.Serializable;
import java.util.Objects;

import jenkins.model.Jenkins;
import jenkins.scm.api.SCMHead;
import jenkins.scm.api.SCMNavigator;
import jenkins.scm.api.SCMRevision;
Expand All @@ -53,6 +54,11 @@
* {@link #getObjectUrl()} to point to the pull request on GitHub, the {@link #getObjectDisplayName()} to provide
* the title of the pull request and {@link #getObjectDescription()} to provide the description of the pull request
* </li>
* <li>An external {@link SCMSource} implementation that corresponds to a Gitea repository could use the
* {@link #getObjectUrl()} to point to the Gitea repository URL, the {@link #getObjectDisplayName()} to provide
* the repository name, the {@link #getObjectDescription()} to provide the description of repository, and the
* the optional {@link #getHtmlObjectDescription()} to provide the displayed repository description.
* </li>
* </ul>
*
* @since 2.0
Expand All @@ -70,83 +76,114 @@
private final String objectDescription;
@CheckForNull
private final String objectUrl;
@CheckForNull
private final String htmlObjectDescription;

public ObjectMetadataAction(@CheckForNull String objectDisplayName,
@CheckForNull String objectDescription,
@CheckForNull String objectUrl) {
// [JENKINS-75069] Default htmlObjectDescription set to null in order to keep the initial constructor
// to prevent an interface disruption
this(objectDisplayName, objectDescription, objectUrl, null);
}

public ObjectMetadataAction(@CheckForNull String objectDisplayName,
@CheckForNull String objectDescription,
@CheckForNull String objectUrl,
@CheckForNull String htmlObjectDescription) {
this.objectDisplayName = objectDisplayName;
this.objectDescription = objectDescription;
this.objectUrl = objectUrl;
this.htmlObjectDescription = htmlObjectDescription;
}

/**
* Returns the display name of the object or {@code null}. Consumers should assume the content is plain text that
* needs escaping with {@link Util#escape(String)} when being included in HTML output.
*
* @return the display name of the object or {@code null}
*/
@Exported
@CheckForNull
// note we need to call this getObjectDisplayName() to avoid conflicting with InvisibleAction.getDisplayName()
public String getObjectDisplayName() {
return objectDisplayName;
}

/**
* Returns the description of the object or {@code null}. Consumers should assume the content is plain text that
* needs escaping with {@link Util#escape(String)} when being included in HTML output.
*
* @return the description of the object or {@code null}.
*/
@Exported
@CheckForNull
public String getObjectDescription() {
return objectDescription;
}

/**
* Returns the external url of the object or {@code null} if the object does not have an external url.
*
* @return the display name of the object or {@code null}
*/
@Exported
@CheckForNull
// note we need to call this getObjectDisplayName() to avoid conflicting with InvisibleAction.getUrl()
public String getObjectUrl() {
return objectUrl;
}

/**
* 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()}.
Comment on lines +138 to +142
Copy link
Member

@jtnord jtnord Mar 25, 2025

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

Copy link
Author

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 ?

Copy link
Member

@jtnord jtnord Mar 25, 2025

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.

Copy link
Author

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.

Copy link
Member

@daniel-beck daniel-beck Mar 25, 2025

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.)

Copy link
Author

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 ?

Copy link
Author

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 ?

Copy link
Member

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 :)

Copy link
Author

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.

*
* @return the description of the object for the HTML content or {@code null}.
*/
@Exported
@CheckForNull
public String getHtmlObjectDescription() {
return this.htmlObjectDescription;
}

/**
* {@inheritDoc}
*/
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}

ObjectMetadataAction that = (ObjectMetadataAction) o;

if (!Objects.equals(objectDisplayName, that.objectDisplayName)) {
return false;
}
if (!Objects.equals(objectDescription, that.objectDescription)) {
return false;
}
if (!Objects.equals(htmlObjectDescription, that.htmlObjectDescription)) {
return false;
}
return Objects.equals(objectUrl, that.objectUrl);
}

/**
* {@inheritDoc}
*/
@Override
public int hashCode() {
int result = objectDisplayName != null ? objectDisplayName.hashCode() : 0;
result = 31 * result + (objectDescription != null ? objectDescription.hashCode() : 0);
result = 31 * result + (objectUrl != null ? objectUrl.hashCode() : 0);
result = 31 * result + (htmlObjectDescription != null ? htmlObjectDescription.hashCode() : 0);

Check warning on line 186 in src/main/java/jenkins/scm/api/metadata/ObjectMetadataAction.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 87-186 are not covered by tests
return result;
}

Expand All @@ -159,6 +196,7 @@
"objectDisplayName='" + objectDisplayName + '\'' +
", objectDescription='" + objectDescription + '\'' +
", objectUrl='" + objectUrl + '\'' +
", htmlObjectDescription='" + htmlObjectDescription + '\'' +
'}';
}
}
Loading