-
Notifications
You must be signed in to change notification settings - Fork 97
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
Bump commons-text version to 1.10.0 to address CVE-2022-42889 #170
Conversation
Hi @michael-o, I'd like you to take a look at this PR when you have a moment. TY! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property does not make sense for a singe use case.
Thanks for your feedback @michael-o |
did you check if the plugin is really affected but the issue? |
If there is only the slightest doubt, one would want to upgrade, don't you agree? |
sure no worries it's a good idea. |
Hi! We're using the maven-javadoc-plugin at our company, and our parent company's IT department is complaining about "dangerous" code that is present on our build server. Turns out, they want to eradicate all uses and presence of commons-text version 1.9 and below. They are probably overreacting more than slightly, but it would save me, my team and our department a lot of headache if the maven-javadoc-plugin could upgrade to version 1.10.0 @michael-o Are you really adamant about not wanting a property for a single version? @sman-81 gave some context for his choice, are you able to agree with him on this? Thanks in advance :) |
I won't object, it is not a blocker. |
LMK how the title should be rephrased and I will happily do so. |
Not due to merge conflicts. Change is binary-compatible. Contributing can at times feel a little like an uphill battle :) |
It seems like this PR still isn't merged. @michael-o Could you please approve this PR? I've now got a solutions architect breathing down my neck, so thanks in advance :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree with @olamy, we are not affected here. The CVE does not apply:
$ grep -r commons.text .
./pom.xml: <artifactId>commons-text</artifactId>
./src/main/java/org/apache/maven/plugins/javadoc/AbstractFixJavadocMojo.java:import org.apache.commons.text.StringEscapeUtils;
@Neutius Please show us how the plugin here is affected by the commons-text CVE. Thanks |
Why do you keep on going on about this @olamy? I've offered to rename this PR to a title of your liking. You have not responded to the offer. Let me know how the title should be rephrased and I will happily do so.
@michael-o and as a conclusion the plugin won't be upgraded? |
No, I did not say that. I approved the PR because I don't see a reason not to merge it, but the abstract provided by the PR does not apply here. So any other committer is free to merge. Those who absolutely need this to happen also this is a non-issue are free to get in touch privately with a committer to sponsor a release. |
read my comment here #170 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the PR to something like "Bump commons-text version to 1.10.0" and let the show go on.
Superseded by #174 |
From checklist: "Trivial changes do not require a JIRA issue" |
@cstamas Shouldn't you have given me time to react to your comment rather than merging my change under your own pr and your own name? |
I submitted this PR as I wanted to contribute (as I have before). I find this change quite valuable, as small as it is. The experience on this PR, the wait times, the nitpicking and mocking of outside contributors feels very discouraging. @Neutius Hope you will have a new version soon with this issue fixed. |
One of the core issues with this PR was that you tried to sell as a security issue which it was not. It was a mere dependency upgrade to shut off stupid, superficial scanners. |
Sorry for my comment. I have to say it was a bit sarcastic to point to our procedures. (nothing related to you but that's another problem ;) ) But hey at the end of the day you ticked those checkboxes whereas you didn't create a jira.... |
This trivial PR opened since Oct 14 was "parked" for two obvious (and explained) reasons: the wrong intent ("get rid of CVE..." without any assessment or proof that without this PR plugin is affected by it), and, a technical issue (a single used property for a "minor" dependency). The order of things was that I approved the PR to get it moving, just to figure out I disagree with adding a version property for this single dependency (as it was pointed out on Oct 27) and not fixed since. Hence, after I approved the PR, I would have to start nagging you to rework this trivial PR. Not to mention the pain of going thru creating JIRA account for you (latest ASF infra changes: JIRA is not self signup anymore), and so on. IMHO, trivial PRs are like fixing a typo in a log message, but changing (even minor) of dependency IMHO should be present in release notes, hence JIRA would be needed (this is strictly my personal opinion) Sorry for hijacking the change. |
Hi, this is a tiny PR that addresses a potentially severe issue:
https://www.cvedetails.com/cve-details.php?t=1&cve_id=CVE-2022-42889
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles
,where you replace
MJAVADOC-XXX
with the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verify -Prun-its
to make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.