Re-evaluate icecave dependency

[W0rma]

#518 added the dependency to icecave/parity.

In composer/composer#12039 (comment) it was recommended to check whether this dependency is really necessary:

At some point we maybe should (re-) evaluate them. Especially the icecave/* seems the odd one out as it is advertised as a A customizable deep comparison library. Which seems unneeded at a quick glance.

I just created this issue to not forget about this evaluation.

[DannyvdSluijs]

Thanks for issue report. Is this something you've already did some investigation into or have some knowledge about?
Or is it purely a reminder at this point?

[W0rma]

It's purely a reminder.
I haven't done any research yet.

[erayd]

@W0rma The dependency on icecave/parity was added in order to fix an issue with deep comparison of objects in the enum & const constraint validation. PHP did not at the time have a native mechanism to do it properly (is that still the case?)

An alternative to the dependency would be to implement proper deep comparison in this library.

[petski]

justinrainbow/json-schema 6.0.0 requires icecave/parity (1.0.0) 
icecave/parity 1.0.0 requires icecave/repr (~1) 

Both icecave/* dependencies were released 10+ years ago. I found out that icecave/repr even has a file on board (see ./vendor/icecave/repr/src/Generator.php:229) that isn't parsable since PHP 8.0. A fix is already present in icecave/repr@9ccbaec (5 years ago) though...

PHPUnit has a (very) good way of comparing "things", and PHPUnit's author has released the compare component under https://github.com/sebastianbergmann/comparator. It may be a good candidate to swap out icecave/* for sebastianbergmann/comparator (?)

[umulmrum]

Sorry for the mere "I agree" comment, but at least I'd like to stress out the security perspective here. Adding a rather unknown and unmaintained library, that adds a transitive dependency that is also unknown and unmaintained, could be interpreted as one step in a supply chain attack. So I'd also be grateful if you could reconsider. Thanks very much!

(please note that this isn't about any concrete mistrust on you or the library authors, more about the general "I have to do with security, so I developed a solid level of paranoia")

[DannyvdSluijs]

@erayd , @W0rma and @umulmrum I'm curious, how do you see replacing the icecave dependency for the sebastian/comparator one. Are you concerns about a new dependency or about the icecave dependency?

PHP version constraints of this library would put us on the 3.0.5 version of sebastian/comparator which is from September 2022

[umulmrum]

@DannyvdSluijs From my point of view it's only about the icecave dependency, not about dependencies in general. Using sebastian/comparator in an old version isn't ideal, but at least Sebastian Bergmann is well-known and trustworthy, and there haven't been security issues in his extremely widespread libs for years. I think he pushes new PHP versions a bit too fast, but if you think your users can handle it, it would be possible to follow that fast-track and remove support for older PHP versions. Other than that I'm not familiar with either lib, sorry. I also can't assess if it is feasible to rebuild the required functionality in this lib.

What's also concerning about the icecave lib is the apparent circumstance of its inclusion. The commit was merged in 2018 but was only included in a new version 6 years later even though there were multiple releases in the meantime. If I planned to build a supply-chain attack, I might also try to make it look like my malicious code was "always there". Again, please take it as general paranoia and not a personal attack.

But this also leads to the question why the change wasn't included earlier from a functional perspective. Was it simply a long-standing issue and people are relieved it's finally solved? Or is it no longer needed?

Thank you for your work!

[DannyvdSluijs]

I've drafted #803 which could use some review.