rm-hull / nvd-clojure

National Vulnerability Database dependency checker for Clojure projects
MIT License
275 stars 36 forks source link

Fix NVD API token bug #174

Closed kelvinqian00 closed 7 months ago

kelvinqian00 commented 8 months ago

Address the issue #173, in which the NVD_API_TOKEN variable does not work when the :nvd-api :key value is present but nil (which is the case with the default generated config file). I also verified that passing in the API key via the config file and as an env var both work by doing a local make install followed by a scan via clojure tools.

Side note: For some reason when I run the integration tests locally they fail on the dogfooding config steps, even before I made any changes. It may be something funky with my local environment.

vemv commented 8 months ago

Hi!

Thanks for the PR. I disagree on the change - if a nil key is present, it should be honored, which here means failing fast and clearly.

So, users either set it to a valid value, or remove it, setting the NVD_API_TOKEN env var.

I'd suggest undoing the design change and fixing the bug(s) only.

I enabled GH actions now.

Cheers - V

kelvinqian00 commented 8 months ago

Hi @vemv,

Thanks for the clarification on your intent. Unfortunately I think your goal of failing fast on nil values of :key is problematic, since the scan automatically generates an nvd-clojure.edn file with {:key nil}. Thus, the user has a few choices:

The point is, it is problematic in contexts where users cannot easily modify or remove the nvd-clojure.edn config file.

vemv commented 7 months ago

Hi @kelvinqian00, sorry for the delay, I've had limited bandwith in the last few weeks.

I would not want to try forcing a specific direction - it's not a good use of our time.

I'll fix this directly in another branch asap.

(Bear in mind that there are workarounds that can be used as of today)

My sincere thanks for giving this a shot and trying to improve things!