dependency-review-action icon indicating copy to clipboard operation
dependency-review-action copied to clipboard

Add Vulnerabilities and license checks

Open tspascoal opened this issue 2 years ago • 5 comments

New Feature

Creates two checks:

  • A check with vulnerabilities added. If the check fails, it adds to the check the list of vulnerabilities found grouped by manifest. The check fails if vulnerable package(s) are added.
  • License check. A license check is created and it lists on the detail incompatible licenses (if any) and the unknown licenses. The check fails if there are incompatible licenses and succeeds otherwise

By separating the failure in two, users can now control if license or vulnerabilities individually prevent the merge from happening.

Checks names are configurable.

Breaking Changes

A breaking change was introduced, the action no longer fails if there are vulnerabilities or license violations. fail-on-violation parameter can be set to true (default value is false) to maintain backward compatibility.

image

There is another potential breaking change, since the workflow now requires checks: write permission, might need to add the new permission explicitly. The dependency review starter workflow will also have to be updated to add this permission

tspascoal avatar Aug 07 '22 11:08 tspascoal

@tspascoal I love the idea of having one check per feature, but the main Action one should never be green if one of the children checks fails. Can we overcome this? If not I don't think we should be spending extra cycles here (it's backwards incompatible and adds extra write security permissions).

febuiles avatar Aug 18 '22 14:08 febuiles

@tspascoal I love the idea of having one check per feature, but the main Action one should never be green if one of the children checks fails. Can we overcome this? If not I don't think we should be spending extra cycles here (it's backwards incompatible and adds extra write security permissions).

@febuiles Not failing the action, allows extra flexibility since users can fine grain when the PR can't be merged by requiring the checks them want (one of them or both).

Backward compatibility (failing the action if one of the checks fails) can be achieved by explicitly setting the parameter fail-on-violation to true (default value is false).

This allows both the extra flexibility or getting the previous behavior with this parameter.

So getting the old behavior by default, is a matter of changing the default value for fail-on-violation or let the user control by enabling if they want it.

tspascoal avatar Aug 18 '22 20:08 tspascoal

Backward compatibility (failing the action if one of the checks fails) can be achieved by explicitly setting the parameter fail-on-violation to true (default value is false). This allows both the extra flexibility or getting the previous behavior with this parameter.

In order for this to be backwards compatible I'd expect it to behave the same way that it has without the user having to modify their YAML. Remember that we pin all release to v2 atm, so we can't release as part of this version because we'll change the behavior for the current users. We can release a v3, but we release major versions only for breaking backwards compatibility!

I don't know how the child checks work, could we default fail-on-violation to true? In that case only users who want this new behavior would be modifying their workflows. We'd be able to switch this behavior once v3 is released if that's the way we want to move forward (still not sure what the right decision is here, would like to hear back from users in case you have feedback!).

febuiles avatar Aug 19 '22 08:08 febuiles

Backward compatibility (failing the action if one of the checks fails) can be achieved by explicitly setting the parameter fail-on-violation to true (default value is false). This allows both the extra flexibility or getting the previous behavior with this parameter.

In order for this to be backwards compatible I'd expect it to behave the same way that it has without the user having to modify their YAML. Remember that we pin all release to v2 atm, so we can't release as part of this version because we'll change the behavior for the current users. We can release a v3, but we release major versions only for breaking backwards compatibility!

Yeah, my intention was to make it a V3, I've added a Upgrading from V2: Breaking changes section to the readme see Since the new permission is potentially a breaking change, might as well introduce a nicer semantic since users (may) have to change the workflow anyway.

I don't know how the child checks work, could we default fail-on-violation to true? In that case only users who want this new behavior would be modifying their workflows. We'd be able to switch this behavior once v3 is released if that's the way we want to move forward (still not sure what the right decision is here, would like to hear back from users in case you have feedback!).

If you feel that this should be a V2 release, we can certainly make fail-on-violation true by default (but users might still have to change the workflow due to permissions), and will need to update the readme to not mention the upgrade from V2.

Let me know which scenario you want to achieve:

  1. V2 with no breaking change on the action failure
  • (fail-on-violation will default to true)
  • No mention on readme to upgrade from V2, but we will still need to document the change of permission
  1. V3 with no breaking change on the action failure (fail-on-violation will default to true)
  • Upgrade from V2 will be tweaked
  1. V3 with breaking change on the action failure (fail-on-violation will default to false)

tspascoal avatar Aug 19 '22 13:08 tspascoal

I keep forgetting about auth, thanks for bringing it up! We can wait until we have the new config file format before revisiting this.

febuiles avatar Aug 19 '22 15:08 febuiles

The latest release of the action (v3) allows users to specify if they want to allow/disallow a specific check (vulns, licenses) in their runs. I know this is not exactly the same functionality this PR provided, but I feel it's close enough to close this for now.

febuiles avatar Nov 11 '22 16:11 febuiles