Add target attribute to badges and summaries

[chonton]

Add target attribute to AddBadge. Target is the anchor target, allowing badge to link to specified frame; end user does not need to context click to open link in new tab.

Testing done

Unit tests and hpi:run manual testing to verify link with target works as expected

Submitter checklist

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

[strangelookingnerd]

@chonton First of all thanks for the contribution. I’ll review the changes and come back to you. Already looks good, I just want to verify if it is breaking compatibility with consumers of the plugin.

[strangelookingnerd]

@chonton Changes look good so far. I had to retain the old constructors to keep compatibility with upstream groovy-postbuild-plugin.

One thing I'm a little worried about is that the target is unsanitized user input and we not using rel="noopener noreferrer" either. @jenkinsci/badge-plugin-developers any thoughts on that?

[chonton]

Anchor target=_blank implies rel=noopener by default

Unsanitized input is a concern. How are cssClass and style sanitized?

[strangelookingnerd]

Anchor target=_blank implies rel=noopener by default

Unsanitized input is a concern. How are cssClass and style sanitized?

Fair point. I did some more testing and did not find any issues with it so far.

One thing I would like to also add is the support for target in summary links. I created jenkinsci/jenkins#10508 for that. This is no show-stopper for this PR but unless you require this feature urgently I'd like to include it.

[chonton]

My use case is more urgent for AddBadge target, and I can wait. I was dismayed that hudson/summary did not have "target" support and did not look further. Many thanks for knowing (or finding) that fix. I suspect that adding target to summary won't break this plugin, even if the jenkins instance is not updated.

[strangelookingnerd]

After discussion with @daniel-beck in jenkinsci/jenkins#10508 I came to the conclusion that it would be better (= safer & less error prone) to not allow setting the target attribute directly. We should instead have a new flag newWindow that will cause links to have target="_blank" in conjunction with rel="noopener noreferrer" set.

@chonton Is this fine for your requirement / use case or do you actually need to set the target to be something other than _blank?

[chonton]

I intend to have a target per badge type. For the github link, target would be 'github'. For the jira link, target would be 'jira'. For the 'sonarqube' link, target would be 'sonarqube'. For the 'snyk' link, target would be 'snyk'.

What is the supported set of browsers? Should this plugin support hopelessly outdated browsers that have many other security flaws? Browsers since ~2021 have automatically defaulted rel="noopener" behavior for target="_blank". The firefox hack requiring rel="norefer" has likewise been fixed in that timeframe.

[strangelookingnerd]

I intend to have a target per badge type. For the github link, target would be 'github'. For the jira link, target would be 'jira'. For the 'sonarqube' link, target would be 'sonarqube'. For the 'snyk' link, target would be 'snyk'.

Okay, I see. Thanks for the clarification. For badges that should be possible as the implementation does not rely on something from jenkins-core. For the summaries I may need to compromise. The decision there is not mine make. I'm mostly interested in having a simple implementation in the plugin that will not introduce any security issues.

[mPokornyETM]

Mya you pls paste screenshot before and after your changes.
src/main/resources/com/jenkinsci/plugins/badge/action/BadgeSummaryAction/summary.jelly

[strangelookingnerd]

Mya you pls paste screenshot before and after your changes. src/main/resources/com/jenkinsci/plugins/badge/action/BadgeSummaryAction/summary.jelly

As a matter of fact there is no difference here. Visuals are identical as the changes I made in summary.jelly pull the implementation of jenkins-core or rather the <summary> tag into our plugin.

[mPokornyETM]

Mya you pls paste screenshot before and after your changes. src/main/resources/com/jenkinsci/plugins/badge/action/BadgeSummaryAction/summary.jelly

As a matter of fact there is no difference here. Visuals are identical as the changes I made in summary.jelly pull the implementation of jenkins-core or rather the <summary> tag into our plugin.

I see and undestand that. But exactly therefore I want to see, that everything is fine. Just for validation. My aproval is done

[daniel-beck]

@mPokornyETM Could you hold off releasing this for a bit? I want to check something.

[strangelookingnerd]

@daniel-beck Just ping me whenever you are done checking something 😄 I wanted to include something else in the next release anyway that is lying in my workspace for some time now.

[daniel-beck]

Related to our core conversaion, I had to check what Cross-Origin-Opener-Policy does again, given that this PR allows setting target 😬

AFAIUI as a result of that option this PR will not create problems if a lowly Job/Configure'r defines badges, as they won't be able to mess with other users' browsers, so should be fine. If you know more, now would be a great time to let me know 😉

[strangelookingnerd]

Related to our core conversaion, I had to check what Cross-Origin-Opener-Policy does again, given that this PR allows setting target 😬

AFAIUI as a result of that option this PR will not create problems if a lowly Job/Configure'r defines badges, as they won't be able to mess with other users' browsers, so should be fine. If you know more, now would be a great time to let me know 😉

That pretty much confirms what I verified on my end as well. As #244 (comment) points out, all supported browsers already provide sufficient measures to mitigate any harmful configuration. I further found no way to mess up the Jenkins UI with any misconfiguration.

What's left are the points raised in jenkinsci/jenkins#10508 (comment). Leaving the value of a target up to the user should not cause any harm, it may just be less guided (e.g. setting target="_blank" controlled by a boolean). However I think that users working with pipelines and this plugin should be well versed enough to achieve the desired goal without that guidance.