Bug - Javalin Security Vulnerabilities #1440
Conversation
PR Code Suggestions ✨No code suggestions found for the PR. |
|
jorg3lopez
requested review from
halprin,
basiliskus,
luis-pabon-tf,
somesylvie,
jbiskie,
tjohnson7021,
pluckyswan,
GilmoreA6 and
jcrichlake
| @@ -23,6 +23,11 @@ dependencies { | |||
| implementation project(':etor') | |||
| testImplementation testFixtures(project(':shared')) | |||
|
|
|||
| // Patch: Add the fixed Jetty version | |||
| // Import the Jetty BOM to pull consistent Jetty versions | |||
| implementation 'org.eclipse.jetty:jetty-bom:12.0.14' | |||
There was a problem hiding this comment.
Javalin pulls a bill of materials so I'm doing the same in order to have all dependencies working correctly
There was a problem hiding this comment.
Just a contextual question since I'm not super familiar with Gradle - how do you test this? Like if you build locally, can you see what version of Jetty is getting used?
There was a problem hiding this comment.
Can we remove org.eclipse.jetty:jetty-bom:12.0.14 once javalin is updated with the patch? Or is the intention to leave it?
There was a problem hiding this comment.
Just a contextual question since I'm not super familiar with Gradle - how do you test this? Like if you build locally, can you see what version of Jetty is getting used?
You can run the following to see how Gradle resolved all the dependencies versions.
./gradlew dependencies
Can we remove
org.eclipse.jetty:jetty-bom:12.0.14once javalin is updated with the patch? Or is the intention to leave it?
Yes, normally you would remove it.
There was a problem hiding this comment.
There are a couple of solutions to this problem.
- Force the Jetty dependency to a newer version without the vulnerability. This is what is basically happening in this PR.
- Ignore this vulnerability in Snyk. Say what?!
I propose we pursue solution 2, but I'm curious what others think.
For the rest of this review, I'll be using two terms: immediate dependency and transitive dependency. In this specific case, our immediate dependency is Javalin and the transitive dependency is Jetty. This is because we directly depend on Javalin (our immediate dependency), while Javalin subsequently depends on Jetty (our transitive dependency).
1. Force upgrade the transitive dependency
Again, this is what we're doing in this PR.
This is normally a valid solution when a vulnerability is disclosed and our immediate dependency hasn't upgraded yet. This is true of our situation: Javalin hasn't released a new version that requires Jetty to be version 11.0.24 or newer. Why 11.0.24? Snyk tells us this vulnerability was fixed in 11.0.24.
When doing this, you need to check whether the immediate dependency works with the force upgraded transitive dependency. That generally is checked by whether a compile works and by a battery of unit and integration tests passing. You can also check the immediate dependency's documentation to see if it mentions anything about compatibility.
In this specific case, Javalin is currently on Jetty 11. They have GitHub issues to explore updating to Jetty versions 12. Because Javalin isn't already on Jetty 12, I suggest that this PR change to force Jetty to a non-vulnerable version of Jetty 11.
Regardless, if we are to pursue this solution, we should put a DevEx/OpEx ticket into the backlog, and prioritize it for when Javalin upgrades to a non-vulnerable version of Jetty. We do not want forced upgrade Gradle lines hanging about any longer than necessary because that can eventually lead to hard-to-debug bugs when Javalin gets out of sync with our forced Jetty version.
2. Ignore the vulnerability
This solution doesn't always apply. The only reason this is acceptable in our case is because Javalin doesn't use the vulnerable class in question. Snyk documents the vulnerable class as ThreadLimitHandler. You can search in IntelliJ in a specific way to see that Javalin doesn't use it. You can also search Javalin's own repository and see it isn't used. Sometimes it isn't this easy to check to see whether our direct dependency is using the vulnerable aspect of the transitive dependency.
Therefore
Therefore, I propose we just ignore this non-applicable vulnerability in Snyk, but I'm curious what others think. There's less code changes, and we don't need to track a DevEx/OpEx ticket to revert this change. I say ignore the vulnerability.
It makes sense to ignore the vulnerability if Javalin is not impacted by it. We'd avoid tracking another issue and having additional future work to revert this change |
|
Closing PR as we have decided to ignore the vulnerabilities at this time. |
Javalin Vulnerabilities
Javalin uses Jetty-server and Snyk notified us of vulnerabilities related to the jetty-server verstion. The dependencies flagged were:
jetty-server and jetty-http
Issue
#1437
Checklist