eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
613 stars 145 forks source link

Update `org.eclipse.xtend.lib` dependency to `2.28.0` or later to address CVE-2020-8908 #672

Closed cherylking closed 2 years ago

cherylking commented 2 years ago

Our language server is currently using org.eclipse.lsp4j:0.14.0 which ultimately depends on guava:27.1. The low severity vulnerability CVE-2020-8908 has been detected by our Mend scan and indicates that guava:30.0 or later must be used.

Screen Shot 2022-10-11 at 2 13 52 PM

The org.eclipse.xtend.lib:2.28.0 and org.eclipse.xtext.xbase:2.28.0 update the guava version to 30.1 and would resolve this issue. Please consider updating the lsp4j artifacts to use a minimum of the 2.28.0 version of org.eclipse.xtend.lib. Thank you for any attention to this matter.

jonahgraham commented 2 years ago

Hi @cherylking Thank you for the report. #529 has been trying to break the dependency on xbase.lib for a while. I think that would resolve this issue. However that has been ongoing for a while. It would be great if you are able to help review that change so we can break the dependency altogether.

In the meantime, can you provide a PR to make that update as it looks like you have already done most of the analysis work required to get us there. I think the version is only defined here: https://github.com/eclipse/lsp4j/blob/f84db765ce5f232c899fe48b398a8eb6d67e4c1e/gradle/versions.gradle#L16

We can do a release 0.17.0 if needed.

BTW. a few things about this:

  1. Your report is against 0.14.0, I think the same issue in 0.16.0 that we released last week in https://github.com/eclipse/lsp4j/releases/tag/v0.16.0
  2. The dependency tree you have provided is for the lsp4j.generator, that code isn't needed except to build LSP4J, i.e. it shouldn't be in installed for the user. Even if we finish #529 I think xbase will still be used.
  3. LSP4J allows Guava 30: https://github.com/eclipse/lsp4j/blob/f84db765ce5f232c899fe48b398a8eb6d67e4c1e/gradle/versions.gradle#L17-L18 does xbase.lib have a hard dependency on the version? I didn't think it did and you should be able to use the newer guava with no LSP4J changes. That said, I am much more familiar with the OSGi dependencies than the maven ones, so what works with OSGi may not work with maven.
cherylking commented 2 years ago

@jonahgraham The xbase.lib lists a compile dependency on guava with no version in the pom.xml. It uses a bom for dependency management. That bom lists the following:

      <dependency>
        <groupId>com.google.guava</groupId>
        <artifactId>guava</artifactId>
        <version>27.1-jre</version>
        <scope>compile</scope>
      </dependency>

So it seems like a hard dependency.

To address your feedback above:

  1. Yes, the same issue exists with 0.16.0.
  2. Not sure there is a way to tell Mend that the dependency is not used outside the build.
  3. Are you suggesting I could add something to my pom.xml to update guava to 30.0 or later and that would override what was specified in the transitive dependency? I presume this is not the case since xbase.lib listed a hard dependency on the version.

As for reviewing the existing PR that removes the dependency, I don't have enough familiarity with lsp4j at this point.

Regarding my creating a PR to use org.eclipse.xtend.lib:2.28.0 or later, I presumed that change might not be trivial and could require code changes. Would you like me to go ahead and create a PR with that change and see if there are complications?

nixel2007 commented 2 years ago

Are you suggesting I could add something to my pom.xml to update guava to 30.0 or later and that would override what was specified in the transitive dependency?

Yes, because you can't have 2 different versions of the same dependency (without shadowing or package-rewrite hacks), your concrete version in top pom.xml will overwrite version in xtext bom.

mvn has a goal for dependencies list print (gradle has it either), you should be able to check it after pom.xml change.

jonahgraham commented 2 years ago

If @nixel2007 isn't enough to resolve your issue, then please do go ahead and make the PR. That will run the tests and do some API analysis to make sure everything is working as expected.

szarnekow commented 2 years ago

So it seems like a hard dependency.

Please note that this is only the dependency that we build and test against. From my experience, xbase.lib works fine with a more recent Guava version. You can override the version of that dependency in your local build for the time being.

cherylking commented 2 years ago

@jonahgraham I created a fork of this repo and updated the org.eclipse.xtend.lib dependency to 2.28.0 and the local gradle build failed. Without the change, it was successful. So I don't think it is a straight forward update.

Screen Shot 2022-10-12 at 4 01 43 PM

jonahgraham commented 2 years ago

Thanks @cherylking for the info.

I see that you have updated your code so that you aren't blocked here? https://github.com/OpenLiberty/liberty-language-server/pull/122/

I am not sure when someone on here will get to looking at this. If you do figure it out why it failed, we would be grateful for a PR.

cherylking commented 2 years ago

@jonahgraham I am not blocked currently, but we need to do some testing to make sure my overriding that transitive dependency doesn't break anything. I would like to keep this issue open as it seems like updating to a more recent org.eclipse.xtend.lib dependency would be wise. I am not familiar with the lsp4j code base though, and could not even find the test case that I got a failure on.

Just wanted to post with my findings above so that you are aware it will take some investigation (that I unfortunately do not have time to do myself at this point). Thanks for your guidance and attention to this issue.

cdietrich commented 2 years ago

@oehme any idea?

cdietrich commented 2 years ago

might becaused by this https://github.com/eclipse/lsp4j/blob/5605ca82252833777dbef82ca13a403dbfb9d906/gradle/java-compiler-settings.gradle#L84

configurations.all {
    resolutionStrategy {
        force "com.google.guava:guava:18.0"
    }
}

i also assume that this one is no longer needed https://github.com/eclipse/lsp4j/blob/f84db765ce5f232c899fe48b398a8eb6d67e4c1e/build.gradle#L50

oehme commented 2 years ago

@cdietrich yes, if you wanna upgrade xtend.lib, you need to remove (or update) that force rule for guava

cherylking commented 2 years ago
configurations.all {
  resolutionStrategy {
      force "com.google.guava:guava:18.0"
  }
}

@cdietrich Removing that fixed it. Thanks for the pointer. I'll get a PR created later today. I wasn't sure about the other change you listed. The build was successful without changing that "Tooling" section in build.gradle.