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

CSP compatibility improvements #533

Merged
merged 5 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -587,11 +587,11 @@ public String getDisplayName() {
@Restricted(NoExternalUse.class)
@RequirePOST
public FormValidation doCheckUploadedKeystore(@QueryParameter String value,
@QueryParameter String uploadedCertFile,
@QueryParameter String certificateBase64,
@QueryParameter String password) {
// Priority for the file, to cover the (re-)upload cases
if (StringUtils.isNotEmpty(uploadedCertFile)) {
byte[] uploadedCertFileBytes = Base64.getDecoder().decode(uploadedCertFile.getBytes(StandardCharsets.UTF_8));
if (StringUtils.isNotEmpty(certificateBase64)) {
byte[] uploadedCertFileBytes = Base64.getDecoder().decode(certificateBase64.getBytes(StandardCharsets.UTF_8));
return validateCertificateKeystore("PKCS12", uploadedCertFileBytes, password);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,6 @@
</f:bottomButtonBar>
</f:form>
</j:scope>
<script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sticky footer no longer needs realigning explicitly from JS.

// TODO remove this JENKINS-24662 workaround when baseline core has fix for root cause
window.setTimeout(function(){layoutUpdateCallback.call();}, 1000);
</script>
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
<j:otherwise>
<f:form action="configSubmit" method="POST" name="config">
<f:entry title="${%Name}" help="/plugin/credentials/help/domain/name.html">
<f:textbox field="name" id="name" onchange="updateSave(this.form)" onkeyup="updateSave(this.form)"/>
<f:textbox field="name" clazz="required-for-submit"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to formBehaviour.js

</f:entry>
<f:entry title="${%Description}" help="/plugin/credentials/help/domain/description.html">
<f:textarea field="description"/>
Expand All @@ -54,28 +54,14 @@
items="${instance.specifications}"/>
</f:entry>
<f:bottomButtonBar>
<input type="submit" name="Submit" value="${%Save}" id="save" class="submit-button primary" />
<button formnovalidate="formNoValidate" id="save" name="Submit" class="jenkins-button jenkins-button--primary">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to standard button to avoid the need for makeButton JS function.

${%Save}
</button>
</f:bottomButtonBar>
</f:form>
</j:otherwise>
</j:choose>
<script><![CDATA[
var saveButton = makeButton(document.getElementById('save'), null);
function updateSave(form) {
function state() {
return (document.getElementById('name').value.length === 0);
}
saveButton.set('disabled', state(), false);
}
updateSave(saveButton.getForm());
window.setTimeout(function () {
// TODO remove this JENKINS-24662 workaround when baseline core has fix for root cause
layoutUpdateCallback.call();
}, 1000);
]]></script>
<st:adjunct includes="com.cloudbees.plugins.credentials.common.formBehaviour"/>
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@
<f:submit value="${%Create}"/>
</f:bottomButtonBar>
</f:form>
<script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sticky footer no longer needs realigning explicitly from JS.

// TODO remove this JENKINS-24662 workaround when baseline core has fix for root cause
window.setTimeout(function(){layoutUpdateCallback.call();}, 1000);
</script>
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@
<j:set var="instance" value="${null}"/>
<f:form action="createDomain" method="POST" name="newDomain">
<f:entry title="${%Domain Name}" help="/plugin/credentials/help/domain/name.html">
<f:textbox id="name" field="name" onchange="updateOk(this.form)" onkeyup="updateOk(this.form)"/>
<script>document.getElementById('name').focus();</script>
<f:textbox field="name" clazz="autofocus required-for-submit"/>
</f:entry>
<f:entry title="${%Description}" help="/plugin/credentials/help/domain/description.html">
<f:textarea name="description"/>
Expand All @@ -43,22 +42,12 @@
items="${null}"/>
</f:entry>
<f:bottomButtonBar>
<input type="submit" name="Submit" value="${%Create}" id="ok" class="submit-button primary" />
<button type="submit" name="Submit" id="ok" class="jenkins-button jenkins-button--primary">
${%Create}
</button>
</f:bottomButtonBar>
</f:form>
<script><![CDATA[
var okButton = makeButton(document.getElementById('ok'), null);
function updateOk(form) {
function state() {
return (document.getElementById('name').value.length === 0);
}
okButton.set('disabled', state(), false);
}
updateOk(okButton.getForm());
]]></script>
<st:adjunct includes="com.cloudbees.plugins.credentials.common.formBehaviour"/>
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@
<f:apply/>
</f:bottomButtonBar>
</f:form>
<script>
// TODO remove this JENKINS-24662 workaround when baseline core has fix for root cause
window.setTimeout(function(){layoutUpdateCallback.call();}, 1000);
</script>
</l:main-panel>
</l:layout>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
Behaviour.specify(".required-for-submit", 'required-for-submit', -99, function(requiredField) {
const saveButton = requiredField.closest("form").querySelector('[name="Submit"]');
function updateSave() {
const state = requiredField.value.length === 0;
saveButton.disabled = state;
}
requiredField.addEventListener('input', updateSave);
updateSave(saveButton);
});

Behaviour.specify(".autofocus", "autofocus", 0, function(el) {
el.focus();
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
~ THE SOFTWARE.
-->

<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<j:set var="divId" value="${h.generateId()}"/>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler">
<j:set var="fileId" value="${h.generateId()}"/>
<f:entry field="uploadedKeystore">
<!--
Expand All @@ -35,74 +34,15 @@
-->

<!-- $$ => $ after jelly interpretation -->
<f:textbox id="${divId}" style="display:none" default="${descriptor.DEFAULT_VALUE}" checkMethod="post"
checkUrl="'${rootURL}/descriptorByName/com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl$$UploadedKeyStoreSource/checkUploadedKeystore?'+fillValues_${divId}(this)"
<f:textbox id="${fileId}-textbox" style="display:none" default="${descriptor.DEFAULT_VALUE}" checkMethod="post"
checkDependsOn="certificateBase64"
checkUrl="${rootURL}/descriptorByName/com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl$$UploadedKeyStoreSource/checkUploadedKeystore"
/>
<input type="hidden" disabled="disabled" name="certificateBase64"/>
</f:entry>
<f:entry field="uploadedCertFile">
<!-- TODO switch to f:file after baseline includes https://github.com/jenkinsci/jenkins/pull/7452 -->
<input id="${fileId}" type="file" name="uploadedCertFile" class="jenkins-file-upload" jsonAware="true" />
<input id="${fileId}" type="file" name="uploadedCertFile" class="jenkins-file-upload certificate-file-upload" jsonAware="true" />
</f:entry>
<script><![CDATA[
var uploadedCertFile_${fileId} = '';
(function(){
// Adding a onChange method on the file input to retrieve the value of the file content in a variable
var uploadedCertFileInput = window.document.getElementById("${fileId}");
var _onchange = uploadedCertFileInput.onchange;
if(typeof _onchange === "function") {
uploadedCertFileInput.onchange = function() { fileOnChange(this); _onchange.call(this); }
} else {
uploadedCertFileInput.onchange = fileOnChange.bind(uploadedCertFileInput);
}
function fileOnChange() {
try { // inspired by https://stackoverflow.com/a/754398
var uploadedCertFileInputFile = uploadedCertFileInput.files[0];
var reader = new FileReader();
reader.onload = function (evt) {
uploadedCertFile_${fileId} = btoa(evt.target.result);
var uploadedKeystore = document.getElementById("${divId}");
uploadedKeystore.onchange(uploadedKeystore);
}
reader.onerror = function (evt) {
if (window.console !== null) {
console.warn("Error during loading uploadedCertFile content", evt);
}
uploadedCertFile_${fileId} = '';
}
reader.readAsBinaryString(uploadedCertFileInputFile);
}
catch(e){
if (window.console !== null) {
console.warn("Unable to retrieve uploadedCertFile content");
}
}
}
})();
function fillValues_${divId}(el) {
var value = el.value;
var password = findNextFormItem(el, 'password').value;
var uploadedCertFile = uploadedCertFile_${fileId};
return "value="+encodeURIComponent(value)+"&password="+encodeURIComponent(password)+"&uploadedCertFile="+encodeURIComponent(uploadedCertFile);
}
// workaround for JENKINS-19124
// without this script, the password changes will be not trigger the check on the uploadedKeystore
window.setTimeout(function(){
var r = window.document.getElementById("${divId}");
var p = findNextFormItem(r, 'password');
if (p) {
var _onchange = p.onchange;
var _field = r;
if(typeof _onchange=="function") {
p.onchange = function() { _field.onchange(_field); _onchange.call(this); }
} else {
p.onchange = function() { _field.onchange(_field); };
}
}
r = null;
p = null;
}, 500);
]]></script>
<st:adjunct includes="com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl.UploadedKeyStoreSource.configUpload"/>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// multiple objects named "password" in the form =>
// extend findNearBy to allow selecting by id
if (!findNearBy.patched) {
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 allowing IDs or CSS selectors in checkDependsOn instead of just names would help with ambiguities. Should be probably done in core, but for now can be patched in plugins like this.

@daniel-beck do you have a better idea?

Copy link
Member

Choose a reason for hiding this comment

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

@jenkinsci/core-security-review Any objection to this technique?

var oldNearBy = findNearBy;
findNearBy = function(el, name) {
return name.charAt(0) == '#' ? document.querySelector(name.split('/')[0]) : oldNearBy(el, name);
}
findNearBy.patched = true;
}

Behaviour.specify(".certificate-file-upload", 'certificate-file-upload', -99, function(uploadedCertFileInput) {
// Adding a onChange method on the file input to retrieve the value of the file content in a variable
var fileId = uploadedCertFileInput.id;
var _onchange = uploadedCertFileInput.onchange;
if (typeof _onchange === "function") {
uploadedCertFileInput.onchange = function() { fileOnChange(this); _onchange.call(this); }
} else {
uploadedCertFileInput.onchange = fileOnChange.bind(uploadedCertFileInput);
}
const base64field = uploadedCertFileInput.closest('.radioBlock-container').querySelector('[name="certificateBase64"]');
function fileOnChange() {
try { // inspired by https://stackoverflow.com/a/754398
var uploadedCertFileInputFile = uploadedCertFileInput.files[0];
var reader = new FileReader();
reader.onload = function (evt) {
base64field.value = btoa(evt.target.result);
var uploadedKeystore = document.getElementById(fileId + "-textbox");
uploadedKeystore.onchange(uploadedKeystore);
}
reader.onerror = function (evt) {
if (window.console !== null) {
console.warn("Error during loading uploadedCertFile content", evt);
}
uploadedCertFile[fileId] = '';
}

reader.readAsBinaryString(uploadedCertFileInputFile);
}
catch(e){
if (window.console !== null) {
console.warn("Unable to retrieve uploadedCertFile content");
}
}
}

// workaround for JENKINS-19124
zbynek marked this conversation as resolved.
Show resolved Hide resolved
// without this script, the password changes will be not trigger the check on the uploadedKeystore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code changed significantly but the reasoning is still the same: I couldn't find a way how to make password textfield trigger validation of the cert field, since there are multiple fields named password in the form and they are on higher level than the cert upload field.

Copy link
Member

Choose a reason for hiding this comment

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

@RealativePath(..) is the way. There is a bug https://issues.jenkins.io/browse/JENKINS-65616 that causes this to work for some validation triggers, but not others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The relative path can't be just password because the field is on a higher level. But the relative path ../password matches the wrong password.

var r = window.document.getElementById(fileId + "-textbox");
var p = findNextFormItem(r, 'password');
if (p) {
const dependsOn = r.getAttribute('checkDependsOn');
if (!dependsOn.includes(p.id)) {
r.setAttribute('checkDependsOn', dependsOn + ' #' + p.id + "/password");
}
}
});

Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
<f:hetero-radio field="keyStoreSource" descriptors="${descriptor.getPropertyType('keyStoreSource').applicableDescriptors}"/>
</f:entry>
<f:entry title="${%Password}" field="password">
<f:password value="${instance==null || instance.passwordEmpty ? '' : instance.password}"/>
<f:password value="${instance==null || instance.passwordEmpty ? '' : instance.password}" id="${h.generateId()}"/>
</f:entry>
<st:include page="id-and-description" class="${descriptor.clazz}"/>
</j:jelly>
22 changes: 3 additions & 19 deletions src/main/resources/lib/credentials/select.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,10 @@
</span>
<span class="help-btn"><t:help href="#" /></span>
</div>
<div class="help-content" hidden="hidden">
<div class="help">
<p>${%includeUserCredentialsHelp}</p>
<div class="from-plugin">
(from <a href="https://wiki.jenkins.io/display/JENKINS/Credentials+Plugin">Credentials Plugin</a>)
(from <a href="https://plugins.jenkins.io/credentials">Credentials Plugin</a>)
</div>
</div>
<div class="warning user-credentials-caution" hidden="hidden">
Expand Down Expand Up @@ -165,21 +165,5 @@
</div>
</j:if>
</div>
<script>
// HACK: can be removed once base version of Jenkins has fix of https://issues.jenkins-ci.org/browse/JENKINS-26578
// conditionally include the adjunct resources as AdjunctManager doesn't do this correctly for CSS
if (!document.getElementById('lib.credentials.select.select')) {
var link = document.createElement('link');
link.id = 'lib.credentials.select.select';
link.rel = 'stylesheet';
link.type = 'text/css';
link.href = "${request.contextPath+'/'+app.getAdjuncts(null).rootURL+'/lib/credentials/select/select.css'}";
link.media = 'all';
document.getElementsByTagName('head')[0].appendChild(link);
var script = document.createElement('script');
script.type = 'text/javascript';
script.src = "${request.contextPath+'/'+app.getAdjuncts(null).rootURL+'/lib/credentials/select/select.js'}";
document.getElementsByTagName('body')[0].appendChild(script);
}
</script>
<st:adjunct includes="lib.credentials.select.select"/>
</j:jelly>
Loading
Loading