bug: Sorting of bom imports breaks dependency versioning

[herder]

Hi, thanks for a great plugin!

I came across an issue regarding sorting BOM imports in the dependencyManagement section:

Some boms import older versions of libs - for example, Springdoc imports a version of Spring Framework that is susceptible to a CVE. So I manually added the spring-boot-dependencies bom in the dependencyManagement section to ensure that version > 6.1.13 was used.

However, when running Spotless with sort-pom the boms get reordered so that the SpringDoc bom is put before Spring, causing the project to import Spring 6.1.8 again.

I think it would be safer to not touch the order of bom imports to avoid this - possibly putting manual non-bom imports on top, and after that leaving the bom imports in the original order?

I have created a repo to demo the issue here: https://github.com/herder/sortpom-demo

You can reproduce the issue by first running mvn package and inspect the generated jar - it should only contain version 6.1.13 of Spring. Then run mvn spotless:apply clean package - the jar now contains Spring 6.1.8 instead.

[Ekryd]

Ouch! Sounds like a nasty problem. I think that this is one case where I have this disclaimer: Warning. There are two ways to solve the problem.

1. Do not sort dependencies in your project at all
2. Put a Ignore section around the two bom dependencies so that SortPom ignores them during sorting

Building specific code to exclude sorting boms will not change the basic problem: sometimes dependency ordering matter

[herder]

Ah, thanks for the pointer to the ignore feature!

Yeah, I agree: dependency ordering matters sometimes, but I feel this is a kind of footgun that silently can make your app susceptible to CVE:s, even though you thought you fixed them before. This applies to the regular dependencies as well - risky either way :)

But I'm not sure there's ever a reason to change the order in dependencyManagement - at least the bom order? I think the safest route would be to maybe fix formatting, but only sort them if explicitly opting in to doing that?

That would of course change the default behavior that some might have come to depend on, which is always a hard sell...

If this is not a good idea maybe it would make sense to emit some warning if the order was changed, to make sure the user is aware of it?

[Ekryd]

Did you try the ignore section around the bom dependencies? I don't think I can recommend anything else.

[herder]

Did you try the ignore section around the bom dependencies? I don't think I can recommend anything else.

Yep, that works fine - thanks for the pointer! I'm mostly concerned with this being a security issue, since it silently rearranges so that you might not have bumped a library version that you thought you did, so that a CVE is still there.

The ignore section is a great workaround, and I realize that it might be difficult to change the behavior of the plugin, but could it make sense to emit a warning log when the order in the dependencyManagement section is changed?

[Ekryd]

Hmmm, warning for dependencyManagement seems very specific. I wonder if it is better to add a parameter to warn about any rearrangement of dependencies? I also wonder if this should be the responsibility of the plugin. You should have a mechanism to scan for vulnerabilities regardless if the plugin rearranges dependencies or if a developer adds a random dependency manually. Where would you stop to check the warning (if you got one)? If you stop it during a local build, it is easy for a developer to ignore it and if you stop it in a CI pipeline, then the pom-file might already be sorted so it can be missed. I think that you would need a comparison with an older version of the pom to get it right.

[herder]

Hey, sorry for dropping the reply ball on this one... :)

We do have vulnerability scanning for all our services - this is more of a 'warn about the footgun' query in that this makes the dependencies change silently.

I think adding a warning about rearrangement of dependencies could be one way to go. Doing a mvn help:effective-pom before and after would show if anything was changed dependency-wise, but smells a bit hacky to me.

[Stephan202]

@herder consider enabling the dependency convergence check: that forces users to be explicit when two dependencies pull in distinct versions of a transitive dependency.

(The maven-enforcer-plugin has other useful checks as well. For example, you could also require that the transitive dependency ultimately pulled in is the highest version.)

[herder]

@Stephan202 Thanks for the tip (I really like that plugin!), but the dependencyConvergence does not help here, since the dependencies do converge - they just converge on the wrong version :)

[Stephan202]

Hmm that must then be an issue specific to BOMs; not sure I ever specifically tested that case. requireUpperBoundDeps doesn't help either? Unfortunately I don't have other suggestions.

[herder]

Exactly: when you import the boms in the dependencyManagement section it sets the versions of the depencencies managed by those boms, in the order they are first found. That's why this issue changes the version of the spring dependency, which has different versions in the springdoc and spring-boot-dependencies boms that are imported.