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.

Sign up for GitHub

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

Fix Yarn Audit findings bug and patch-bump version from 2.6.0 -> 2.6.1 #70

Merged
merged 2 commits into from
Aug 5, 2019
Merged

Fix Yarn Audit findings bug and patch-bump version from 2.6.0 -> 2.6.1 #70

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f861fa
Fix Yarn Audit findings bug and patch-bump version from 2.6.0 -> 2.6.1
geraldnash-cb Aug 3, 2019
ab31624
Add test case for missing dev key in YarnAudit findings
geraldnash-cb Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ Example CircleCI `config.yml`:
version: 2.1

orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1

workflows:
main:
Expand Down
8 changes: 4 additions & 4 deletions integrations/circleci/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Note: active_scanners and enforced_scanners must be yaml formatted for Salus con
version: 2.1

orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1

workflows:
main:
Expand All @@ -38,7 +38,7 @@ workflows:
version: 2.1

orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1

workflows:
main:
Expand All @@ -53,7 +53,7 @@ workflows:
version: 2.1

orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1

workflows:
main:
Expand All @@ -68,7 +68,7 @@ workflows:
```
version: 2.1
orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1
executors:
salus_2_4_2:
docker:
Expand Down
8 changes: 4 additions & 4 deletions integrations/circleci/orb.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ examples:
usage:
version: 2.1
orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1
workflows:
salus_scan:
jobs:
Expand All @@ -92,7 +92,7 @@ examples:
usage:
version: 2.1
orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1
workflows:
salus_scan:
jobs:
Expand All @@ -103,7 +103,7 @@ examples:
usage:
version: 2.1
orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1
workflows:
salus_scan:
jobs:
Expand All @@ -114,7 +114,7 @@ examples:
usage:
version: 2.1
orbs:
salus: federacy/salus@2.5.1
salus: federacy/salus@2.6.1
executors:
salus_2_4_2:
docker:
Expand Down
2 changes: 1 addition & 1 deletion lib/salus.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
require 'salus/processor'

module Salus
VERSION = '2.6.0'.freeze
VERSION = '2.6.1'.freeze
DEFAULT_REPO_PATH = './repo'.freeze # This is inside the docker container at /home/repo.

SafeYAML::OPTIONS[:default_mode] = :safe
Expand Down
2 changes: 1 addition & 1 deletion lib/salus/scanners/node_audit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def run
# For all advisories,
prod = raw_advisories_for_id.any? do |raw_advisory|
# any there there any instances in the prod dependency tree?
raw_advisory.fetch(:findings).any? { |finding| !finding.fetch(:dev) }
raw_advisory.fetch(:findings).any? { |finding| !finding.fetch(:dev, false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

is the fix to not run development dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, previously this scanner expected the advisories fetched by YarnAudit to have a dev key in each object of their findings array. Some advisories don't have the dev key and cause the scanner to error on this line.

The value of the dev key is typically true or false, and this change treats the value as false if the key is not provided.

TLDR: It treats the finding as a non-dev finding by default if not already specified. @xolaz

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Can you write a quick test for this?

Copy link
Contributor

@xolaz xolaz Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The test can check that our advisory output is as expected even without the dev key. Aka, advisory output reports regardless of whether the dev key exists.

Copy link
Contributor

@xolaz xolaz Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

And what does the dev key mean for NPM/Yarn? That the finding is related to a development package? It looks like this change won't report development dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xolaz I'm unable to find documentation related to the shape of these advisory objects and the meaning of their keys. Comments in the code claim that the key lets us know whether advisories are in some production dependency tree, but it's unclear how the scanner would know that and what the difference between the production dependency tree and trees in other staging environments would be, if any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drafting a test case right now.

Copy link
Contributor Author

@geraldnash-cb geraldnash-cb Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also, I spoke with @nishils and he said that the difference between production and development advisories is that prod advisories are advisories related dependencies and dev advisories are related to devDependencies. So, the dev key lets us know whether the finding is related to a dev dependency. @xolaz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, in essence if we're unsure whether a finding is related to a dev or non-dev dependency (no dev key provided), we assume the worst by treating it as if it's related to a non-dev dependency. @xolaz

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also quickly test this @geraldnash-cb by running yarn audit --json to see what the output looks like.

end

Advisory.new(id, module_name, title, severity, url, prod, excepted)
Expand Down
2 changes: 1 addition & 1 deletion spec/fixtures/integration/expected_report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "2.6.0",
"version": "2.6.1",
"passed": true,
"running_time": 0.0,
"scans": {
Expand Down
2 changes: 1 addition & 1 deletion spec/fixtures/processor/local_uri/expected_report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "2.6.0",
"version": "2.6.1",
"passed": true,
"running_time": 0.0,
"scans": {
Expand Down
2 changes: 1 addition & 1 deletion spec/fixtures/processor/remote_uri/expected_report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "2.6.0",
"version": "2.6.1",
"passed": true,
"running_time": 0.0,
"scans": {
Expand Down