clj-holmes / clj-watson

clojure deps SCA
Eclipse Public License 2.0
77 stars 8 forks source link

chore: Address little hack we added to quiet clj-holmes #81

Open lread opened 1 month ago

lread commented 1 month ago

Background

While working on PR #80 checks initially failed clj-holmes checks (not clj-holmes/watson clj-holmes/clj-holmes!).

It turns out clj-holmes is a bit overzealous on what it considers a clojure spec. It was convinced the map that describes our cli arguments was a spec, it saw :require true and assumed that we meant to type :required true.

Minimal example that triggers the clj-holmes finding:

(def foo {:bingo
          {:require true}})

The finding looks like this:

|                   :filename |               :name |                                                        :message | :severity | :lines |
|-----------------------------+---------------------+-----------------------------------------------------------------+-----------+--------|
| src/clj_watson/cli_spec.clj | Schema require typo | Typo on schema declaration using :require instead of :required. |     error |   [16] |

This would be very helpful if this we actually a clojure spec, but it is not.

We tried to reduce clj-holmes to only look at security related issues, and this worked locally via

clj-holmes scan -p . --rule-tags security

Alas there is to facility pass rule-tags to the clj-holmes github action we are using.

The Hack

There might be ways to suppress clj-holmes false positives but I didn't see a way after reading the docs. For now I've appeased clj-holmes by using :require :yes instead of :require true.

Options

Option 1: Do nothing

Leave the hack in.

Option 2: Wait for a fix from clj-holmes

  1. Ideally, this would be a fix for the false positive.
  2. Less ideally, a way to specify rule tags on the clj-holmes github action.

Option 3: Use clj-holmes without the github action

This would allow us to do 2.2 ourselves.

Option 4: Disable clj-holmes

I like that a security-related project is being scanned by a security tool, so I'd rather not.

Proposal

Let's wait a bit and see what happens with clj-holmes. I'd rather option 2.1, but 2.2 or 3 would be ok as well.

lread commented 1 month ago

Sean raised an issue over at clj-holmes: https://github.com/clj-holmes/clj-holmes-rules/issues/41 And another for clj-holmes-action: https://github.com/clj-holmes/clj-holmes-action/issues/10