JENKINS-55710, #28 - Fix icon upload iframe in global config #36
Conversation
|
I did not build this locally. I intend to let https://ci.jenkins.io/job/Plugins/job/sidebar-link-plugin/job/PR-36/ build it, and then download the hpi artifact, compare to a previous version, and verify that nothing unexpected changed in the |
|
Diff from https://ci.jenkins.io/job/Plugins/job/sidebar-link-plugin/job/master/28/ shows:
Looking good enough so far. |
|
I installed this, and the generated HTML now has |
|
Judging from https://github.com/jenkinsci/jenkins/blob/8a29e8858b49da45c0071f690d4ceebe8db2f18f/core/src/main/java/hudson/Plugin.java#L68-L71, the generated |
|
The Promoted Builds (Simple) plugin has a similar iframe, and that one loads fine. |
|
PromotedBuildsSimplePlugin extends Plugin, but SidebarLinkPlugin extends GlobalConfiguration: That was changed in #25. Perhaps that causes the |
|
I looked at several plugins that support CasC. In them, the classes that have configurable properties typically extend GlobalConfiguration. None of them extends Plugin. I don't know whether CasC supports configuring actual plugin classes. The Plugin() constructor is deprecated. The comment there claims that DummyImpl "will still route the URL space", but this is apparently implemented in Plugin.doDynamic, which seems to support only static (possibly localized) files, rather than Jelly views like Perhaps the file upload feature has to be moved from SidebarLinkPlugin to a new class that implements RootAction (javadoc). |
|
Ohh, actually …no, that doesn't work. the HTML form appears all right, but the upload POST fails with "HTTP ERROR 403 No valid crumb was included in the request". |
|
Can someone test whether the Promoted Builds (Simple) plugin has the same problem with the anti-CSRF crumb? It's too late for me to continue on this tonight. |
|
Did you try to change parent class and test result? |
|
The CSRF crumb error is JENKINS-55710.
Not going to try because it would require using the deprecated Plugin() constructor. |
|
I wonder if it would be better to delete the file upload feature altogether. Jenkins administrators presumably have other ways to copy files to the userContent directory. The plugin does not offer a full-featured file manager anyway; you cannot even list or delete files. After all, #28 is a complaint about a blocked iframe, not about the inability to upload icons. Even so, I think the resource filtering should still be disabled. None of the existing resources benefits from the filtering, and it seems pretty risky to use on jelly files. |
Because SidebarLinkPlugin nowadays extends GlobalConfiguration
rather than Plugin, Jenkins maps ${rootURL}/plugin/sidebar-link/*
only to static resources and not to Jelly views like startUpload.jelly.
Form validation generates URLs that use "descriptorByName",
so let's use that for the upload iframe as well.
(Descriptor.getDescriptorUrl would return that string,
but the risk of bugs seems lower if I just hardcode it.)
layout.jelly will add data-crumb-header and data-crumb-value attributes to the head element, and include behavior.js, which should then add the anti-CSRF crumb as a hidden field to the HTML form. Fix JENKINS-55710
|
No, it still doesn't work. The upload POST now fails with status 404. The POST request has four copies of
|
|
When I upload a plugin hpi file to the Plugin Manager, the POST request to Aha! |
action="upload" in startUpload.jelly attempted to call the SidebarLinkPlugin.doUpload method, but that was renamed to doUploadLinkImage in commit 3ef9756 during the CasC work, so we need to use action="uploadLinkImage".
KalleOlaviNiemitalo
changed the title
KalleOlaviNiemitalo
changed the title
KalleOlaviNiemitalo
changed the title
KalleOlaviNiemitalo
changed the title
|
I tested that the file upload in the global configuration now works; the uploaded file is created in the |
|
@damianszczepanik do you need anything else from me here? |
|
How about compatibility with last release ? Does it break configuration when users upgrade plugin after releasing this change? |
|
No, this does not break compatibility. This does not change any Java code, so the settings saved using this version can be loaded using Sidebar Link 2.0.0 and vice versa. Commits b0fac40 and 2df9a3c together make the iframe point to
Commit e92770a makes the icon upload HTML page reference the usual JavaScript files of Jenkins. This change depends on JENKINS-34670 and thus requires Jenkins 2.53 or later; that's OK because pom.xml already declares Jenkins 2.263.4. The commit also adds an HTML title element that is localizable but does not have any translations yet. Commit 4f35c1a changes the URL to which the icon upload HTML page posts the form data. I believe the new URL would have worked in Sidebar Link 2.0.0 and later if there had been a link to it, and I don't expect people to bookmark this URL anyway. |
|
Thanks @KalleOlaviNiemitalo for supporting this change! |
The icon upload feature of this plugin did not work, for several reasons:
${url}withhttps://github.com/jenkinsci/sidebar-link-plugininSidebarLinkPlugin/config.jellyat build time, so the iframe pointed to GitHub and not to the Jenkins controller, and was blocked by Web browsers.This was reported in Blocked iframe #28.
✅ Fix by disabling resource filtering.
${rootURL}/plugin/sidebar-link/startUploadtostartUpload.jellybecause [JENKINS-63149] - Adding JCasC support #25 made SidebarLinkPlugin extend GlobalConfiguration rather than Plugin.✅ Fix by using
${rootURL}/descriptorByName/hudson.plugins.sidebar_link.SidebarLinkPlugin/startUploadinstead.This was reported in JENKINS-55710.
✅ Fix by replacing
<l:ajax>with<l:layout>, which adds the anti-CSRF crumb values and scripts.startUpload.jellyhadaction="upload"but the method in SidebarLinkPlugin was renamed fromdoUploadtodoUploadLinkImagein [JENKINS-63149] - Adding JCasC support #25.✅ Fix by using
action="uploadLinkImage".Fix #28 and JENKINS-55710