Skip to content

[SECURITY-2220] Escape plot description#72

Merged
ericbn merged 1 commit into
jenkinsci:masterfrom
offa:SECURITY-2220-escape_description
Jul 12, 2022
Merged

[SECURITY-2220] Escape plot description#72
ericbn merged 1 commit into
jenkinsci:masterfrom
offa:SECURITY-2220-escape_description

Conversation

@offa

@offa offa commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

Naive approach to address SECURITY-2220.

<j:out … /> seems to ignore escape-by-default, using a simple <span>…</span> instead escapes the value.

@Wadeck @daniel-beck

What has been done

  1. Escaping added to prevent a security issue

How to test

  1. Use Plot description that needs escaping

Checklist

  • Git commits follow best practices
  • Build passes in Jenkins
  • Appropriate tests or explanation to why this change has no tests
  • Pull Request is marked with appropriate label (see .github/release-drafter.yml)
  • JIRA issue is well described (problem explanation, steps to reproduce, screenshots)
  • For dependency updates: links to external changelogs and, if possible, full diffs

@Wadeck Wadeck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not manually tested but that's the kind of correction that was expected 👍

@offa

offa commented Jul 12, 2022

Copy link
Copy Markdown
Contributor Author

Now, how to continue from here?

@Wadeck

Wadeck commented Jul 12, 2022

Copy link
Copy Markdown

Manually tested:

Screenshot_2022-07-12_092713_001

It's working as expected, great job.


Next step:

@ericbn
ericbn merged commit 4b681af into jenkinsci:master Jul 12, 2022
<j:if test="${it.getPlotDescription(index) != null}">
<j:if test="${!it.getPlotDescription(index).isEmpty()}">
<b>Description</b>: <j:out value="${it.getPlotDescription(index)}"/>
<b>Description</b>: <span>${it.getPlotDescription(index)}</span>

@daniel-beck daniel-beck Jul 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't even need the span.

              <b>Description</b>: ${it.getPlotDescription(index)}

would do it.

Note that j:out specifically disables the effect of escape-by-default and prints it verbatim. (Its counterpart st:out always escapes what's printed, luckily there's no chance of anyone ever confusing the two 😫 .)

@offa
offa deleted the SECURITY-2220-escape_description branch July 12, 2022 12:27
@offa

offa commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

@ericbn could you consider a new release so we can install a fixed version please?

@offa

offa commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

@ericbn @timja please create a new release, the security issue is still open for Jenkins instances around.

@timja

timja commented Jul 28, 2022

Copy link
Copy Markdown
Member

I am not a maintainer of plot

@ericbn

ericbn commented Jul 29, 2022

Copy link
Copy Markdown
Member

Version 2.1.11 released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants