Skip to content
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

[JENKINS-69659] Un-inline multiple occurrences of JavaScript in Jelly templates #130

Merged
merged 9 commits into from
Oct 1, 2024
12 changes: 4 additions & 8 deletions src/main/java/hudson/matrix/LabelAxis.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
package hudson.matrix;

import hudson.Extension;
import hudson.Functions;
import jenkins.model.Jenkins;
import hudson.model.labels.LabelAtom;
import org.apache.commons.lang.StringUtils;
Expand Down Expand Up @@ -86,16 +85,13 @@ public boolean isInstantiable() {
return !j.getNodes().isEmpty() || !j.clouds.isEmpty();
}


public String buildLabelCheckBox(LabelAtom la, LabelAxis instance) {
public String buildLabelCheckBox(LabelAtom la) {
final String escapedName = jsStringEscape(htmlAttributeEscape(la.getName()));
final String escapedDescription = jsStringEscape(StringUtils.isEmpty(la.getDescription()) ? "" :
htmlAttributeEscape(la.getDescription()));
return new StringBuilder("\"").append(jsStringEscape("<input type='checkbox' name='values' json='")).append(escapedName).append(jsStringEscape("' "))
.append("\"").append(String.format("+has(\"%s\")+", escapedName)).append("\"")
.append(jsStringEscape("/><label class='attach-previous'>")).append(escapedName).append(" (").append(escapedDescription).append(")</label>\"")
.toString();
// '${h.jsStringEscape('<input type="checkbox" name="values" json="'+h.htmlAttributeEscape(l.name)+'" ')}'+has("${h.jsStringEscape(l.name)}")+'${h.jsStringEscape('/><label class="attach-previous">'+l.name+' ('+l.description+')</label>')}'
return "<input type='checkbox' name='values' json='" +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can revert this if there are objections, this was a refactoring suggested by IDE.

Copy link

Choose a reason for hiding this comment

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

FTR That code was a correction of a previous vulnerability, not sure the current version does not reintroduce issues.

With new YAHOO.widget.HTMLNode I am pretty sure it interprets the values as HTML. Have you tried to reproduce the previous vulnerability or to inject some payloads to understand the behavior? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think lines 89 and 90 are related to the vulnerability fix, unless you mean a different one that I'm not aware of.
I've checked with simple payloads I could think of, and content was escaped properly. I'll give it another shot too to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-checked, it seems fine to me. Tried couple different payloads, couldn't break it. User provided content is escaped. I'd appreciate someone else trying to break it though.

escapedName + "' " + "/><label class='attach-previous'>" + escapedName +
" (" + escapedDescription + ")</label>";
}
}
}
7 changes: 7 additions & 0 deletions src/main/java/hudson/matrix/MatrixProject.java
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.AncestorInPath;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.kohsuke.stapler.TokenList;
Expand Down Expand Up @@ -1125,6 +1127,11 @@
return r;
}

@Restricted(NoExternalUse.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

With jenkinsci/jenkins#9150 the method becomes obsolete

public FormValidation doCheckDisplayNameOrNull(@AncestorInPath MatrixProject job, @QueryParameter String value) {

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doCheckDisplayNameOrNull connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in DescriptorImpl#doCheckDisplayNameOrNull
return Jenkins.get().doCheckDisplayName(value, job.getName());
}

/**
* @deprecated as of 1.456
* This was only exposed for Jelly.
Expand Down
41 changes: 9 additions & 32 deletions src/main/resources/hudson/matrix/LabelAxis/config.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -28,39 +28,16 @@ THE SOFTWARE.
</f:entry>
<f:entry title="${%Node/Label}" field="labels">
<div class="yahooTree labelAxis-tree" style="border: 1px solid gray; height: 10em; overflow:auto;" values="${instance.valueStringHtmlEscaped}" />
<script>
Behaviour.specify("DIV.labelAxis-tree", 'LabelAxis', 0, function(e) {
var tree = new YAHOO.widget.TreeView(e);

var labels = new YAHOO.widget.TextNode("${%Labels}", tree.getRoot(), false);
var machines = new YAHOO.widget.TextNode("${%Individual nodes}", tree.getRoot(), false);

var values = (e.getAttribute("values") || "").split("/");
function has(v) {
return values.includes(v) ? 'checked="checked" ' : "";
}
<st:adjunct includes="hudson.matrix.LabelAxis.label-axis-resources"/>
<f:invisibleEntry>
<span class="label-axis-i18n" data-i18n-labels="${%Labels}" data-i18n-individual-nodes="${%Individual nodes}"/>
<div class="label-axis-data-container">
<j:forEach var="l" items="${app.labelAtoms}">
new YAHOO.widget.HTMLNode(<j:out value="${descriptor.buildLabelCheckBox(l,instance)}"/>, ${l.isSelfLabel()?'machines':'labels'}, false);
<span data-label-atom="${l}"
data-label-checkbox="${descriptor.buildLabelCheckBox(l)}"
data-label="${l.isSelfLabel()?'machines':'labels'}"/>
</j:forEach>

tree.draw();
<!--
force the rendering of HTML, so that input fields are there
even when the form is submitted without this tree expanded.
-->
tree.expandAll();
tree.collapseAll();

<!--
cancel the event.

from http://yuilibrary.com/forum/viewtopic.php?f=89&t=8209&p=26239&hilit=HTMLNode#p26239
"To prevent toggling and allow the link to work, add a listener to the clickEvent on that tree and simply return false"
-->
tree.subscribe("clickEvent", function(node) {
return false;
});
});
</script>
</div>
</f:invisibleEntry>
</f:entry>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
Behaviour.specify("DIV.labelAxis-tree", 'LabelAxis', 0, function(e) {
var tree = new YAHOO.widget.TreeView(e);

var i18nContainer = document.querySelector(".label-axis-i18n");
var labels = new YAHOO.widget.TextNode(i18nContainer.getAttribute("data-i18n-labels"), tree.getRoot(), false);
var machines = new YAHOO.widget.TextNode(i18nContainer.getAttribute("data-i18n-individual-nodes"), tree.getRoot(), false);

var values = (e.getAttribute("values") || "").split("/");
function has(v) {
return values.includes(v) ? 'checked="checked" ' : "";
}

var labelAxisDataContainer = document.querySelector(".label-axis-data-container");
labelAxisDataContainer.childNodes.forEach(node => {
var labelCheckbox = node.getAttribute("data-label-checkbox");

var CHECKED_ATTR_INSERT_IDX = "<input ".length;
var output = [labelCheckbox.slice(0, CHECKED_ATTR_INSERT_IDX),
has(node.getAttribute("data-label-atom")), labelCheckbox.slice(CHECKED_ATTR_INSERT_IDX)].join('');
var label = node.getAttribute("data-label");
new YAHOO.widget.HTMLNode(output, label === "machines" ? machines : labels, false);
});

tree.draw();
/*
force the rendering of HTML, so that input fields are there
even when the form is submitted without this tree expanded.
*/
tree.expandAll();
tree.collapseAll();

/*
cancel the event.

from http://yuilibrary.com/forum/viewtopic.php?f=89&t=8209&p=26239&hilit=HTMLNode#p26239
"To prevent toggling and allow the link to work, add a listener to the clickEvent on that tree and simply return false"
*/
tree.subscribe("clickEvent", function(node) {
return false;
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ THE SOFTWARE.
</f:optionalBlock>

<f:entry title="${%Display Name}" field="displayNameOrNull">
<f:textbox checkUrl="'${rootURL}/checkDisplayName?displayName='+encodeURIComponent(this.value)+'&amp;jobName='+encodeURIComponent('${h.jsStringEscape(it.name)}')"/>
<f:textbox/>
</f:entry>
</f:advanced>
</f:section>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
refreshPart('matrix',"./ajaxMatrix");
4 changes: 1 addition & 3 deletions src/main/resources/lib/hudson/matrix-project/matrix.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,7 @@ THE SOFTWARE.
</j:otherwise>
</j:choose>
<j:if test="${ajax==null and attrs.autoRefresh and !h.isAutoRefresh(request)}">
<script defer="defer">
refreshPart('matrix',"./ajaxMatrix");
</script>
<st:adjunct includes="lib.hudson.matrix-project.matrix-resources"/>
</j:if>
</div>
</j:jelly>