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

OF-2722 OF-2902: Remove commons-fileupload #2667

Merged

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Feb 6, 2025

The commons-fileupload dependency that is used by Openfire has a CVE reported against it (rather: against one of its dependencies).

Java/Jetty can nowadays natively process file uploads. There no longer is a need for commons-fileupload.

This commit replaces commons-fileupload, which resolves the outstanding CVE report.

Additionally some plugin-upload properties were slightly refactored.

The commons-fileupload dependency that is used by Openfire has a CVE reported against it (rather: against one of its dependencies).

Java/Jetty can nowadays natively process file uploads. There no longer is a need for commons-fileupload.

This commit replaces commons-fileupload, which resolves the outstanding CVE report.

Additionally some plugin-upload properties were slightly refactored.
By moving JSP code into a servlet, static analyzers are finding more issues. One of the reported issues based on the change in the previous commit is that unsanitized user-provided input is used in the code that implements the downloading of plugins.

This particular code isn't used at all by the admin console. I've removed it in this commit.
Plugin uploads were already added to the audit log. With this change, whenever an admin downloads a plugin, an entry is added to the audit log too.
Copy link
Member

@Fishbowler Fishbowler left a comment

Choose a reason for hiding this comment

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

A few comments / questions.
I don't pretend to understand (remember?) how servlets work, but it all appears rational.

@guusdk guusdk merged commit baa798c into igniterealtime:main Feb 7, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants