[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #9259

basil · 2024-05-10T19:44:02Z

JENKINS-73170 Upgrade Commons FileUpload from 1.5 to 2.0.0-M2

Context

JENKINS-73169 describes the long-term goal of removing Commons FileUpload from Stapler and Jenkins core, decreasing API surface area and maintenance burden by eliminating an unneeded library.

This long-term strategy carries a certain amount of risk and labor:

New APIs need to be designed and validated across a variety of consumers (both core consumers and plugin consumers), which is more laborious than a textual package/class rename operation.
New implementations need to be written and tested, ensuring that existing behavior remains the same and no new security issues are introduced. Given the general lack of realistic automated testing for file uploading, there is a high risk of behavioral regression. Even worse is the potential for a security regression that a new implementation brings. Testing all of these scenarios thoroughly will require a large amount of manual testing and is far more laborious than staying with a battle-tested implementation with known (and tested) security modalities.
Implementing this change throughout the ecosystem would require the new APIs to ship in a weekly/LTS release, for plugins to adopt that LTS release, and then for those plugins to be adapted to using the new APIs. The list is:
```
plain-credentials|278830
subversion|86698
uno-choice|37949
google-oauth-plugin|12220
m2release|9733
scriptler|6767
sidebar-link|6678
file-parameters|5507
xcode-plugin|4506
kpp-management-plugin|2262
miniorange-saml-sp|2099
hp-application-automation-tools-plugin|1611
custom-folder-icon|1158
promoted-builds-simple|1038
custom-job-icon|712
jclouds-jenkins|583
patch-parameter|559
svn-partial-release-mgr|517
htmlresource|515
BlameSubversion|405
deployer-framework|271
avatar|214
ios-device-connector|125
pom2config|90
recipe|51
clif-performance-testing|22
```
Our experience has shown that it takes many months for such changes to propagate through the ecosystem and to be adopted by plugins. Upgrading a plugin's baseline, patching it to use new APIs, and getting the result merged and released can be a time-consuming exercise, if not a frustrating one—especially for the long tail of plugins.

Problem

We have a desire to upgrade Spring Security to a recent version in a relatively timely fashion. Recent versions of Spring Security require Java EE 9 or later, while Commons FileUpload 1.x only supports Java EE 8 and earlier, implying in the long term the completion of the abovementioned project as a prerequisite to moving to Java EE 9 or later (or, in the short term, creating a custom fork of Commons FileUpload 1.x with the javax imports changed to jakarta imports—a high-maintenance prospect that would also regress our previous efforts to unfork this library). Yet this long-term solution is laborious and carries a high risk of regression (both functionally and security-wise); moreover, file uploading is not a central feature of the software, and focusing a huge amount of effort on it seems out of place relative to the value to end users it will provide—certainly a higher priority would be to focus on the actual Jakarta EE 9 migration itself, which brings with it the tangible benefits of security updates for Spring Security and thus a clear benefit to Jenkins users.

Solution

A medium-term compromise is possible via Commons FileUpload 2.x, which supports both javax and jakarta imports. Consider that Stapler and Jenkins core provide a number of APIs that work with Commons FileUpload FileItem objects, in contrast to the low-level DiskFileItem objects and the classes that produce them (like DiskFileItemFactory). These APIs somewhat shelter their consumers from low-level implementation details, like the instantiation of a factory against a particular servlet API. The key observation is that the vast majority of plugins (all open-source plugins with more than 3,000 installations, and all but two proprietary plugins) consume these higher-level Jenkins APIs, which still tie us to Commons FileUpload, but not necessarily to low-level implementation details like DiskFileItem; moreover, any cases I found of plugins that directly consume the low-level APIs can be trivially converted to the higher-level Stapler APIs. Unfortunately, the package name has changed between 1.x and 2.x; however, since the FileItem interface is very similar between 1.x and 2.x, and since it is an interface, we can bridge between the two versions somewhat transparently.

The implications are as follows: once a handful of plugins are migrated from direct usages of DiskFileItem to the simpler FileItem wrappers provided by Stapler (which is a trivial and low-risk change, and moreover only needs to be done for 2-4 plugins in the short term), we can upgrade core to FileUpload 2.x (with both javax and jakarta support) maintaining full plugin compatibility (in both production and build/test) with any plugins that consume the 1.x APIs via FileItem (and not through the low-level DiskFileItem). These existing plugins, which link against FileUpload 1.x, will call into Stapler to get their FileItem, which will be a 2.x object wrapped in a 1.x compatibility object, and any methods they call on that 1.x compatibility object will vector back into the 2.x methods at runtime.

In other words, core will be fully running on FileUpload 2.x, with both javax and jakarta support, unblocking the Java EE 9 migration project. Even better, this can be achieved through a fraction of the labor and the risk of the long-term strategy. Commons FileUpload 2.x is largely a package/class rename release, but the code is basically the battle-tested implementation we have always relied on, so the risk of functional and security regression is very low. Rather than having to sweep through dozens of plugins, we'll have to sweep through a handful at most—and the results of that sweep can be released proactively, rather than having to wait for a new API to be available (perhaps even in LTS) in order to consume it. Finally, this gets file uploading out of the way for the time being so that Jenkins contributors can focus on the higher-priority task of dealing with the Jakarta EE 9 migration.

Some might object to the use of a milestone release of Commons FileUpload, and this is a fair point. Yet it is advertised on both the project's home page and their GitHub page; also, it is published to Maven Central (unlike a SNAPSHOT release). It would appear as if the authors are getting close to a release but simply looking for feedback on the API. The Jenkins project could provide a valuable service to the Apache Commons project by confirming that the API is working for our use case. Moreover, I suspect the Spring 5.3.x EOL will likely push the whole Java ecosystem toward Java EE 9+, and I suspect (or at least hope) that this will push Commons FileUpload 2.x toward GA this year. Nevertheless, since it is still in milestone status, I am not recommending that we go ahead and migrate plugins from FileUpload 1.x to 2.x APIs at this time. The 1.x compatibility methods are tried and true, and not subject to change, so it is prudent to remain on them at least until the GA release of 2.x. Moreover, going on a sweep of plugin consumers is exactly the kind of activity that we wanted to avoid doing as part of this exercise; and if we are going to do a sweep of plugin consumers, then we might as well simply implement the long-term solution—something which this medium-term solution in no way precludes, but simply postpones to a later date to focus on more pressing concerns. Finally, with the exception of the FileItem compatibility interface, this is a lateral move in API surface area (removal of 1.x and addition of 2.x) rather than an increase (addition of 2.x to the existing 1.x).

Looking at the bigger picture, shipping this immediately will cross out one major portion of the risk and labor associated with the Jakarta EE 9 upgrade—that which is associated with file uploading. Getting this out into weekly releases early will ensure that we have plenty of time to deal with any issues. The next major change after this will be to require Java 17, at which point we can switch to Jetty 12.x with Jakarta EE 8 and get another big wad of changes out into production. Then, the only remaining part of the problem will be to switch from Jetty 12 (EE 8) and FileUpload 2 (EE 8) to Jetty 12 (EE 9) and FileUpload 2 (EE 9), a much more bounded problem, whose risk will also be a lot lower, since some of its major prerequisites will have already been shipping for weeks earlier. In contrast, trying to ship a FileUpload change, a Jetty upgrade, and a Jakarta migration all in the same release sounds like a scope that is far too great to be practical.

Implementation

Usages in core and Stapler are migrated to 2.x. A full compatibility layer is provided for plugins that use the high-level FileItem API (but not the low-level DiskFileItem API). This compatibilty layer extends to both production as well as compilation/tests.

The compatibility chart is here (there are also two CloudBees plugins, and I will find someone to update those). The only open-source plugins with over 1,000 installations are miniorange-saml-sp and hp-application-automation-tools-plugin—those are the only two I plan to help with or mention in the release notes, but even those are installed on only 0.72% of controllers so I don't think they should block the integration of this PR into the weekly line (this is just too important a milestone and we are just on too tight of a timeline to wait).

Testing done

Full PCT and ATH (both passing)
Manual testing of file uploading in both core (via the upload plugin feature) and two plugins (Scriptler and Sidebar Link) using the compatibility methods
Existing security-related unit tests still pass, as does the unit test I added recently for file upload persistence.

Proposed changelog entries

Upgrade Commons FileUpload from 1.5 to 2.0.0-M2. Users of the miniorange-saml-sp plugin should upgrade to a compatible version in lockstep with upgrading Jenkins core.

Proposed upgrade guidelines

Upgrade Commons FileUpload from 1.5 to 2.0.0-M2. Users of the miniorange-saml-sp plugin should upgrade to a compatible version in lockstep with upgrading Jenkins core.

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.

The Jira issue, if it exists, is well-described.
Options
Successfully updated the issue's project

There was an error updating the issue's project
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.

The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
Options
Successfully updated the issue's project

There was an error updating the issue's project
There is automated testing or an explanation as to why this change has no tests.

There is automated testing or an explanation as to why this change has no tests.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.

New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.

New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).

New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
Options
Successfully updated the issue's project

There was an error updating the issue's project
For dependency updates, there are links to external changelogs and, if possible, full differentials.

For dependency updates, there are links to external changelogs and, if possible, full differentials.
Options
Successfully updated the issue's project

There was an error updating the issue's project
For new APIs and extension points, there is a link to at least one consumer.

For new APIs and extension points, there is a link to at least one consumer.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.

Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.

Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Proper changelog labels are set so that the changelog can be generated automatically.

Proper changelog labels are set so that the changelog can be generated automatically.
Options
Successfully updated the issue's project

There was an error updating the issue's project
If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).

If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
Options
Successfully updated the issue's project

There was an error updating the issue's project
If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

github-actions · 2024-05-12T13:39:46Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

basil · 2024-05-15T00:09:40Z

bom/pom.xml

+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2-core</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2-distribution</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2-jakarta-servlet5</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2-jakarta-servlet6</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2-javax</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-fileupload2-portlet</artifactId>
+ <version>${commons-fileupload2.version}</version>
+ </dependency>


It would be nice if there was a BOM, but I didn't find one, nor did I find an RFE bug asking for one, so I included every package in this library in the Jenkins BOM for completeness.

basil · 2024-05-15T00:11:27Z

core/src/main/java/hudson/model/FileParameterValue.java

@@ -139,10 +150,15 @@ public String getOriginalFileName() {
 return originalFileName;
 }

+ @WithBridgeMethods(value = org.apache.commons.fileupload.FileItem.class, adapterMethod = "fromFileUpload2FileItem")


See the commentary in the Stapler PR for an explanation of how this works.

basil · 2024-05-15T00:12:47Z

core/src/main/java/hudson/model/FileParameterValue.java

- public static final class FileItemImpl implements FileItem {
- private final File file;
+ @Deprecated
+ public static final class FileItemImpl implements org.apache.commons.fileupload.FileItem {


This is basically only consumed by tests, and might even make more sense in the test harness, though I am not tackling that here. However, to retain compatibility at compile time and in tests, it has to retain the old API. I am providing a FileItemImpl2 that implements the new API for use by core itself and implement this in terms of that (through the delegate pattern) in order to reuse code.

basil · 2024-05-15T00:13:09Z

core/src/main/java/hudson/util/MultipartFormDataParser.java

@@ -82,20 +87,20 @@ public MultipartFormDataParser(HttpServletRequest request, int maxParts, long ma
 throw new ServletException("Error creating temporary directory", e);
 }
 tmpDir.deleteOnExit();
- ServletFileUpload upload = new ServletFileUpload(new DiskFileItemFactory(DiskFileItemFactory.DEFAULT_SIZE_THRESHOLD, tmpDir));
+ JavaxServletFileUpload<DiskFileItem, DiskFileItemFactory> upload = new JavaxServletDiskFileUpload(DiskFileItemFactory.builder().setFile(tmpDir).get());


See the explanation for this in the Stapler PR.

daniel-beck · 2024-05-15T15:51:58Z

Reviewed wrt SECURITY-3030 and SECURITY-3073, seems fine.

bridge methods work at runtime but there is still a compilation error. Since I don't want to force plugins to adopt the new API when bumping their core baseline, simpler to avoid bridge methods here.

bom/pom.xml

MarkEWaite

Thanks!

basil · 2024-05-18T00:44:36Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process.

basil added the work-in-progress The PR is under active development, not ready to the final review label May 10, 2024

basil force-pushed the fileupload2 branch from 681658b to 1e61a2e Compare May 10, 2024 20:19

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label May 12, 2024

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label May 12, 2024

basil force-pushed the fileupload2 branch 5 times, most recently from 9454dd8 to 181f931 Compare May 13, 2024 18:48

basil added a commit to basil/bom that referenced this pull request May 13, 2024

Test jenkinsci/jenkins#9259

a6c46b6

basil force-pushed the fileupload2 branch from a8957aa to b69104b Compare May 13, 2024 20:51

basil mentioned this pull request May 14, 2024

Forward compatibility with Commons FileUpload 2.x jenkinsci/plain-credentials-plugin#70

Merged

basil added a commit to basil/bom that referenced this pull request May 14, 2024

Test jenkinsci/jenkins#9259

ef6d13c

basil added a commit to basil/bom that referenced this pull request May 14, 2024

Test jenkinsci/jenkins#9259

bd89984

basil added a commit to basil/bom that referenced this pull request May 14, 2024

Test jenkinsci/jenkins#9259

c8cf0a4

basil force-pushed the fileupload2 branch from b69104b to a7daec3 Compare May 14, 2024 14:33

basil added a commit to basil/bom that referenced this pull request May 14, 2024

Test jenkinsci/jenkins#9259

dfcba30

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

Test jenkinsci/jenkins#9259

8024879

basil added ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-successful This PR has successfully passed the full plugin-compatibility-test suite labels May 14, 2024

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2

29c80f4

basil force-pushed the fileupload2 branch from a7daec3 to 29c80f4 Compare May 14, 2024 23:39

basil mentioned this pull request May 14, 2024

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 jenkinsci/stapler#542

Merged

basil changed the title ~~Upgrade Commons FileUpload from 1.5 to 2.0.0-M2~~ [JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 May 14, 2024

basil added the needs-security-review Awaiting review by a security team member label May 14, 2024

basil marked this pull request as ready for review May 14, 2024 23:59

basil requested review from MarkEWaite, alecharp and a team May 15, 2024 00:00

basil commented May 15, 2024

View reviewed changes

daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels May 15, 2024

basil added 2 commits May 15, 2024 10:41

I tested compilation of a plugin with these changes and found that the

70751a4

bridge methods work at runtime but there is still a compilation error. Since I don't want to force plugins to adopt the new API when bumping their core baseline, simpler to avoid bridge methods here.

fix Javadoc

068d4be

basil commented May 16, 2024

View reviewed changes

bom/pom.xml Outdated Show resolved Hide resolved

basil added 4 commits May 16, 2024 10:27

Update bom/pom.xml

95c6f04

Merge branch 'master' into fileupload2

19e915d

Be defensive

3ca93ac

Be defensive

9f525ff

MarkEWaite approved these changes May 17, 2024

View reviewed changes

basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 18, 2024

NotMyFault approved these changes May 18, 2024

View reviewed changes

basil merged commit 0117bf3 into jenkinsci:master May 18, 2024
17 checks passed

basil deleted the fileupload2 branch May 18, 2024 16:42

basil added a commit to basil/bom that referenced this pull request May 18, 2024

Test jenkinsci/jenkins#9259

e9157f4

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

Test jenkinsci/jenkins#9259

d34a829

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

Test jenkinsci/jenkins#9259

ef3a8d0

strangelookingnerd mentioned this pull request May 21, 2024

Update dependency org.jenkins-ci.main:jenkins-war to v2.459 - autoclosed jenkinsci/custom-folder-icon-plugin#334

Closed

1 task

basil mentioned this pull request May 22, 2024

Upgrade to FileUpload 2 jenkinsci/custom-folder-icon-plugin#335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #9259

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #9259

basil commented May 10, 2024 •

edited

Submitter checklist

Maintainer checklist

github-actions bot commented May 12, 2024

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024

daniel-beck commented May 15, 2024

MarkEWaite left a comment

basil commented May 18, 2024

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #9259

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #9259

Conversation

basil commented May 10, 2024 • edited

Context

Problem

Solution

Implementation

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

github-actions bot commented May 12, 2024

basil May 15, 2024

Choose a reason for hiding this comment

basil May 15, 2024

Choose a reason for hiding this comment

basil May 15, 2024

Choose a reason for hiding this comment

basil May 15, 2024

Choose a reason for hiding this comment

daniel-beck commented May 15, 2024

MarkEWaite left a comment

Choose a reason for hiding this comment

basil commented May 18, 2024

basil commented May 10, 2024 •

edited