[JENKINS-74996] Get the current markup formatter to display the descr…

[GGY-AH]

The description colum (DescriptionColumn Java class) gets the current markup formatter to display the description column text as HTML when safeHTML is activated, instead of a simple escape.

Testing done

This issue causes the issue JENKINS-73226 in the Gitea plugin. It has been tested with both a local Jenkins and Gitea instance. In Gitea, an organization test is created with a repository dummy-test. The repository is visible in the Jenkins Gitea organization. Several tests have been performed:

• With valid HTML tags (p, i, ...): The description is HTML formatted when safeHTML is activated, plain otherwise,
• With invalid tags (toto, titi, ...): Tags are removed when safeHTML is activated, plain otherwise,
• With forbidden tag (script): The content is removed when safeHTML is activated, plain otherwise.

The screenshot below shows the result as example:

Submitter checklist

• [ x] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• [ x] Ensure that the pull request title represents the desired changelog entry
• [ x] Please describe what you did
• [x ] Link to relevant issues in GitHub or Jira
• [x ] Link to relevant pull requests, esp. upstream and downstream changes
• [x ] Ensure you have provided tests - that demonstrates feature works or fixes the issue

[GGY-AH]

Another solution would be to add a boolean (default false) in ObjectMetadataAction in the SCM Api to display as formatted text if true (and wouldnt change the initial behavior otherwise), but it would involve 3 packages to modify.

[jglick]

This does not smell right. The markup formatter is global and is intended for text entered by users inside Jenkins, which this is not.

https://javadoc.jenkins.io/plugin/scm-api/jenkins/scm/api/metadata/ObjectMetadataAction.html#getObjectDescription() clearly says

Consumers should assume the content is plain text

From the issue description, it seems that the Gitea plugin is loading HTML from Gitea and then incorrectly returning that raw markup from a Jenkins API method which is expecting plain text. If so, the correct fix would be in that branch source, to strip markup.

[GGY-AH]

From my analyze, the ObjectMetadataAction is instanciated in the Gitea plugin to display this description, and this last is used in Branch API to display this field. We wish to display the text formatted as HTML, not only strip markup. That is why I used this formatter getter. I don't know if I am clear enough... I apologize.

A solution may be to create a Class implementing an Action which would be the same as ObjectMetadataAction in Gitea plugin or in SCM API, and use it in DescriptionColumn (dependency to Gitea plugin or SCM API, but I didn't check the dependencies tree yet). Do you agree ? Or did you have in mind to override the DescriptionColumn behavior in Gitea plugin ?

[jglick]

We wish to display the text formatted as HTML, not only strip markup.

Then you would need to define a new API method (could be added to ObjectMetadataAction with use of default) dedicated to that purpose in scm-api.

Use of MarkupFormatter would still not really be right; it is OK for the method to declare that it returns (specifically) HTML, and for branch-api to render that result raw, but then it would be the obligation of any implementation of that method (in the Gitea plugin) to ensure that all HTML thus produced has been sanitized to prevent XSS and similar attacks.

[GGY-AH]

Use of MarkupFormatter would still not really be right; it is OK for the method to declare that it returns (specifically) HTML, and for branch-api to render that result raw, but then it would be the obligation of any implementation of that method (in the Gitea plugin) to ensure that all HTML thus produced has been sanitized to prevent XSS and similar attacks.

I hope I understand well. I had something like that in mind as a second proposal (see comment under the initial one). My solution would be:

• Add a boolean flag (default false) for the description field in ObjectMetadataAction in SCM API telling if the field is to format (true) or not (false).
• In the Description Column In Branch API, if this flag is true, I use the Markup formatter. Otherwise, I let the initial behavior (escape).
• In the Gitea plugin, I set the boolean to true to mark the description field to format.

I don't want to let Branch API display RAW data because I guess that other plugins may wish to treat the description field value as raw data when incoming.
Do you validate this approach ? It is my first time participating to this kind of public project, so I want to do it properly. I hope I am clear enough... If not, I apologize once again...

[jglick]

Add a boolean flag (default false) for the description field in ObjectMetadataAction in SCM API telling if the field is to format (true) or not (false).

That is one option, but it puts the burden on the caller (branch-api) to pay attention to this flag. I more had in mind a separate method for an HTML description, with appropriate fallback behavior for existing implementations and existing callers. Util.isOverridden is helpful here though it takes some concentration to understand how to use it; unit tests are recommended! See jenkinsci/lib-access-modifier#28 but one idiom is to throw AbstractMethodError if an implementation class neglected to offer either variant; for example:

interface X { // v1
  int m();
}

⇒

interface X {{ /v2
  default int m() {
    if (Util.isOverridden(X.class, getClass(), "m2")) {
      return Integer.parseInt(m2());
    } else {
      throw new AbstractMethodError("implement either m() or m2()");
    }
  }
  default String m2() {
    if (Util.isOverridden(X.class, getClass(), "m")) {
      return Integer.toString(m());
    } else {
      throw new AbstractMethodError("implement either m() or m2()");
    }
  }
}

It may be simpler to just deprecate the existing interface and define a new ObjectMetadataAction2. Callers such as branch-api would then need to check for either. Other impls like github-branch-source could continue to use the original interface indefinitely, and at some point switch to the new interface and just call Util.escape themselves (though the method name would need to clearly indicate an HTML return value and the Javadoc would need to be very emphatic that you must escape user-controlled inputs in order to avoid XSS).

In the Description Column In Branch API, if this flag is true, I use the Markup formatter.

But again, the markup formatter is not the right choice for this, because the markup formatter is intended solely to format user-entered text inside Jenkins textareas such as project descriptions, and could be in any format (plain text, simple HTML, Markdown, AsciiDoc, …). For purposes of this issue, the format coming from Gitea is known to be HTML.

[GGY-AH]

I understand now. I had a problem with the default because ObjectMetadataAction is a standard class declared as ExportedBean, not an interface. This is the reason why I wished to put the flag instead...

[jglick]

a standard class […] not an interface

The same options are available for abstract classes.

[GGY-AH]

I think I understand what you mean. I need to create an Abstract class DefaultMetadataAction with the default behavior of the original ObjectMetadataAction class (all the getters + toString because it will be the same). The v2 of ObjectMetadataAction will only implement the abstract method getCustomObjectDescription return objectDescription which will be declared and getObjectDescription will return getCustomObjectDescription. I will have another class FormattedObjectMetadataAction which will implement getCustomObjectDescription method and return the formatted description. In the DescriptionColumn, I will have to add the instanceof check of the received object and then, if it is FormattedObjectMetadataAction, will display the description as raw text.

[jglick]

Ah I see that https://github.com/jenkinsci/scm-api-plugin/blob/a02381dd8a83b2bc23d5d0126d13c793502fda51/src/main/java/jenkins/scm/api/metadata/ObjectMetadataAction.java#L61 is actually a concrete class; it is just up to the branch source to create an instance via published constructor. I guess the simplest change would be to add a new constructor (or a setter) to add a field objectDescriptionHtml or similar, with the default just being null (no distinct HTML). branch-api would then need to check for the new optional field, falling back to escaping objectDescription as it does now.

[GGY-AH]

That is what I tried to explain. I wasn't clear at all. I m sorry. So I would set the value Html if required, and else I do the same as for now. Great.