CSP compatibility improvements #533

zbynek · 2024-05-26T20:14:23Z

Somewhat related to https://issues.jenkins.io/browse/JENKINS-73166 which showed that geval is used by this plugin.

This PR reduces usages of inline JS to improve CSP compatibility.

Testing done

Manually checked that the affected functionality is still OK:

certificate can be uploaded, validated and updated
new domain creation: submit button deactivates for empty name, auto-focus works
same for domain configuration
help for "include user credentials" working (in a job parameter)

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

zbynek · 2024-05-30T21:09:35Z

...ces/com/cloudbees/plugins/credentials/CredentialsStoreAction/CredentialsWrapper/update.jelly

@@ -49,10 +49,6 @@
          </f:bottomButtonBar>
        </f:form>
      </j:scope>
-      <script>


The sticky footer no longer needs realigning explicitly from JS.

zbynek · 2024-05-30T21:09:53Z

.../com/cloudbees/plugins/credentials/CredentialsStoreAction/DomainWrapper/newCredentials.jelly

@@ -72,10 +72,6 @@
          <f:submit value="${%Create}"/>
        </f:bottomButtonBar>
      </f:form>
-      <script>


The sticky footer no longer needs realigning explicitly from JS.

zbynek · 2024-05-30T21:10:54Z

...urces/com/cloudbees/plugins/credentials/CredentialsStoreAction/DomainWrapper/configure.jelly

@@ -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"/>


Moved to formBehaviour.js

zbynek · 2024-05-30T21:12:20Z

...urces/com/cloudbees/plugins/credentials/CredentialsStoreAction/DomainWrapper/configure.jelly

@@ -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">


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

zbynek · 2024-05-30T21:19:27Z

...s/plugins/credentials/impl/CertificateCredentialsImpl/UploadedKeyStoreSource/configUpload.js

+  }
+
+  // workaround for JENKINS-19124
+  // without this script, the password changes will be not trigger the check on the uploadedKeystore


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.

@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.

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.

zbynek · 2024-05-30T23:07:39Z

...s/plugins/credentials/impl/CertificateCredentialsImpl/UploadedKeyStoreSource/configUpload.js

@@ -0,0 +1,57 @@
+// multiple objects named "password" in the form =>
+// extend findNearBy to allow selecting by id
+if (!findNearBy.patched) {


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?

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

...s/plugins/credentials/impl/CertificateCredentialsImpl/UploadedKeyStoreSource/configUpload.js

into csp

The YUI library is old and no longer maintained. Add a user experimental flag to disable YUI. It's disabled by default. When enabling all the YUI related js libraries and css classes are not loaded. Following PR are required to get Jenkins to not show any errors eventually jenkinsci#7569 Some plugins that use YUI (not complete): credentials ldap global-build-stats build-monitor categorized-view Plugins that make use of makeButton (not complete) credentials (fixed with jenkinsci/credentials-plugin#533) openid acceptance-test-harness

* experimental flag to run Jenkins without YUI The YUI library is old and no longer maintained. Add a user experimental flag to disable YUI. It's disabled by default. When enabling all the YUI related js libraries and css classes are not loaded. Following PR are required to get Jenkins to not show any errors eventually #7569 Some plugins that use YUI (not complete): credentials ldap global-build-stats build-monitor categorized-view Plugins that make use of makeButton (not complete) credentials (fixed with jenkinsci/credentials-plugin#533) openid acceptance-test-harness * fix typo Co-authored-by: Jan Faracik <[email protected]> * add license and restrict class --------- Co-authored-by: Jan Faracik <[email protected]>

zbynek · 2024-08-02T23:41:22Z

@mawinter69 note that this is only partial fix for removing YUI from this plugin, there will still one YUI button and one popup menu left.

mawinter69 · 2024-08-02T23:55:19Z

I know #551 will do the rest

jtnord · 2024-09-27T12:49:56Z

...s/plugins/credentials/impl/CertificateCredentialsImpl/UploadedKeyStoreSource/configUpload.js

+    }
+  }
+
+  // workaround for JENKINS-65616


JENKINS-65616 aka Form validation not working with different "level/layer" of fields

jtnord

seems reasonable pending review from core-security-review requested by Basil.

basil · 2024-10-01T21:50:39Z

Passing ATH: jenkinsci/acceptance-test-harness#1750

basil · 2024-10-01T22:18:05Z

Passing PCT: jenkinsci/bom#3653

zbynek requested a review from a team as a code owner May 26, 2024 20:14

zbynek marked this pull request as draft May 26, 2024 20:14

zbynek force-pushed the csp branch from a7262e2 to 0b6ccce Compare May 30, 2024 21:05

zbynek marked this pull request as ready for review May 30, 2024 21:05

zbynek commented May 30, 2024

View reviewed changes

zbynek force-pushed the csp branch 3 times, most recently from 8a4ee3f to da5c849 Compare May 30, 2024 22:08

zbynek commented May 30, 2024

View reviewed changes

CSP compatibility improvements

122e2b2

zbynek force-pushed the csp branch from da5c849 to 122e2b2 Compare June 1, 2024 12:57

jtnord reviewed Jul 12, 2024

View reviewed changes

...s/plugins/credentials/impl/CertificateCredentialsImpl/UploadedKeyStoreSource/configUpload.js Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/jenkinsci/credentials-plugin

25c1294

into csp

mawinter69 mentioned this pull request Jul 20, 2024

Use jenkins-button over yui-button in 'New domain' and 'Update domain' screens #550

Closed

6 tasks

mawinter69 mentioned this pull request Jul 20, 2024

experimental flag to run Jenkins without YUI jenkinsci/jenkins#9489

Merged

Update workaround comment

5eff7dc

basil requested a review from a team August 22, 2024 20:22

Merge branch 'master' into csp

1a36798

timja requested a review from jtnord September 26, 2024 21:24

jtnord reviewed Sep 27, 2024

View reviewed changes

Merge branch 'master' into csp

e9d929a

basil added the enhancement label Oct 1, 2024

basil added a commit to basil/acceptance-test-harness that referenced this pull request Oct 1, 2024

Test jenkinsci/credentials-plugin#533

73e4822

basil mentioned this pull request Oct 1, 2024

Test jenkinsci/credentials-plugin#533 jenkinsci/acceptance-test-harness#1750

Closed

basil added a commit to basil/bom that referenced this pull request Oct 1, 2024

Test jenkinsci/credentials-plugin#533

7758bdf

basil mentioned this pull request Oct 1, 2024

Test jenkinsci/credentials-plugin#533 jenkinsci/bom#3653

Closed

basil merged commit a435002 into jenkinsci:master Oct 1, 2024
17 checks passed

zbynek deleted the csp branch October 2, 2024 04:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSP compatibility improvements #533

CSP compatibility improvements #533

zbynek commented May 26, 2024 •

edited

Loading

zbynek May 30, 2024

zbynek May 30, 2024

zbynek May 30, 2024

zbynek May 30, 2024

zbynek May 30, 2024

jtnord Jul 12, 2024

zbynek Jul 29, 2024

zbynek May 30, 2024

basil Sep 26, 2024

zbynek commented Aug 2, 2024

mawinter69 commented Aug 2, 2024

jtnord Sep 27, 2024 •

edited

Loading

jtnord left a comment

basil commented Oct 1, 2024

basil commented Oct 1, 2024

CSP compatibility improvements #533

CSP compatibility improvements #533

Conversation

zbynek commented May 26, 2024 • edited Loading

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbynek commented Aug 2, 2024

mawinter69 commented Aug 2, 2024

jtnord Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

basil commented Oct 1, 2024

basil commented Oct 1, 2024

zbynek commented May 26, 2024 •

edited

Loading

jtnord Sep 27, 2024 •

edited

Loading