l3nz / cli-matic

Compact, hands-free [sub]command line parsing library for Clojure.
Eclipse Public License 2.0
360 stars 28 forks source link

cli-matic can enable global spec instrumentation under inappropriate circumstances #160

Closed psagers closed 1 year ago

psagers commented 1 year ago

Problem

cli-matic enables global instrumentation when orchestra is in the classpath. This means that if any project dependency causes orchestra to get pulled in for any reason, the entire project will be instrumented, even in a production environment.

I discovered this when I updated a library dependency that happened to pull in orchestra. Regardless of whether that's a good idea, it caused unexpected breaking behavior. It's actually a good thing that some library threw an error, as otherwise it would have been easy to inadvertently end up deploying with instrumentation.

Repro

Just add orchestra as a direct or transitive dependency in any project with cli-matic.

Expected vs actual behavior

The expected behavior is that spec/orchestra instrumentation is never enabled for a project unless the top-level project explicitly authorizes it. In particular, I would suggest:

  1. Instrumentation should never be enabled in production.
  2. Any assumption about what is or is not on the classpath in production is fragile.
  3. Ergo, under no circumstances should a library implicitly enable instrumentation based on the classpath.

Version / Platform

cli-matic 0.5.4 clojure 1.11.1 Java 17

l3nz commented 1 year ago

I believe this is an intended behavior; the big question is why some other library is pulling in Orchestra as a dep....

psagers commented 1 year ago

It's obviously intended, I was suggesting that it's a misfeature. The other project removed orchestra from their normal runtime dependencies, as that's also unwise.

If this were a matter of security, then a minor convenience feature that converted someone else's relatively innocuous error into a catastrophic failure would be difficult to justify (defense in depth). As a mere matter of performance, it's perhaps a little easier to brush off.