Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL-147 Add suppression for core.async nvd FP #222

Merged
merged 5 commits into from
Apr 25, 2022
Merged

SQL-147 Add suppression for core.async nvd FP #222

merged 5 commits into from
Apr 25, 2022

Conversation

milt
Copy link
Member

@milt milt commented Apr 25, 2022

Per jeremylong/DependencyCheck#4384, we have a false positive on nvd scan. Add suppression file to clear core.async, at least until it is resolved.

Note that this should follow the merge + new version resulting from yetanalytics/actions#9

@milt milt marked this pull request as ready for review April 25, 2022 15:46
@milt milt requested a review from kelvinqian00 April 25, 2022 15:46
Makefile Outdated
@@ -76,7 +76,7 @@ bench-async:
# Vulnerability check

target/nvd:
clojure -Xnvd check :classpath '"'"$$(clojure -Spath -A:db-h2:db-sqlite:db-postgres)"'"'
clojure -Xnvd check :classpath '"'"$$(clojure -Spath -A:db-h2:db-sqlite:db-postgres)"'"' :config-filename '"nvd_config.json"'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like nvd_config.json (and nvd_suppression.xml) could go in the dev_resources folder.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Unless the :nvd alias requires that the files be at the top level)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't require that to my knowledge, but by convention I'd expect to see config & scan exception stuff like this at the top level

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a convention?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That dev-time config goes in the root of the repo? Yeah, definitely. See deps.edn, Makefile, Dockerfile, etc... If you are for some reason compelled to put them in a directory, make it a new (possibly hidden) one, as config is not really a "resource". this is where you see things like .git, .github, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case you could create an .nvd directory to put all the NVD stuff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,3 @@
{
"nvd": {"suppression-file": ".nvd/suppression.xml"}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kelvinqian00 this is a good example of why we put config in the root, file paths in config files. This works (because the .nvd/ added), but is immediately more confusing because it knows nothing of the relative path in the dir. In this case everything works so let's just go with it.

@milt milt merged commit ab44971 into main Apr 25, 2022
@milt milt deleted the SQL-147 branch April 25, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants