Some Settings In External Configuration Overridden By Inline Defaults

[AlexWilson-GIS]

The Problem

I discovered that the warn-only parameter is not settable in an external configuration file by first seeing it get ignored when set only there, then seeing it honored when it was set inline. I believe that this is the source of the issue:

The default value for warn-only is set here:

dependency-review-action/action.yml

Lines 64 to 67 in 40eb2b8

	warn-only:
	description: When set to `true` this action will always complete with success, overriding the `fail-on-severity` parameter.
	required: false
	default: false

This means that this parameter, along with every other parameter that has a default value defined in action.yml, will have an inline setting when the code is merging configurations. However, the way that they are merged is problematic:

dependency-review-action/src/config.ts

Lines 19 to 22 in 40eb2b8

	return ConfigurationOptionsSchema.parse({
	...externalConfig,
	...inlineConfig
	})

As documented in MDN, whenever two objects are merged using the spread operator, fields in a later object will override fields from an earlier object with the same key. This makes warn-only and other settings with default values un-configurable from an external file. This is a problem if you want to distribute one configuration file across PR checks in many repositories, including some owned by others that are subscribing to your settings.

Suggested Fixes

1. Flip the order in which the settings are merged with the spread operator so that parameters set in the external configuration file override inline settings.
2. Add another boolean parameter that dictates if the inline settings override the external settings, with a default value of false in order to fix the bug.
3. Simply remove the default values from all of the parameters except one. They are all denoted as not required in action.yml:
   

   dependency-review-action/action.yml

   Lines 7 to 10 in 40eb2b8

   	repo-token:
   	description: Token for the repository. Can be passed in using `{{ secrets.GITHUB_TOKEN }}`.
   	required: false
   	default: ${{ github.token }}

   
   

   dependency-review-action/action.yml

   Lines 56 to 67 in 40eb2b8

   	retry-on-snapshot-warnings:
   	description: Whether to retry on snapshot warnings
   	required: false
   	default: false
   	retry-on-snapshot-warnings-timeout:
   	description: Number of seconds to wait before stopping snapshot retries.
   	required: false
   	default: 120
   	warn-only:
   	description: When set to `true` this action will always complete with success, overriding the `fail-on-severity` parameter.
   	required: false
   	default: false

   
   ...and they are all pulled into the action using functions that denote they are optional:
   

   dependency-review-action/src/config.ts

   Lines 44 to 50 in 40eb2b8

   	const retry_on_snapshot_warnings = getOptionalBoolean(
   	'retry-on-snapshot-warnings'
   	)
   	const retry_on_snapshot_warnings_timeout = getOptionalNumber(
   	'retry-on-snapshot-warnings-timeout'
   	)
   	const warn_only = getOptionalBoolean('warn-only')

   
   The one exception to this is repo-token:
   

   dependency-review-action/src/dependency-graph.ts

   Line 13 in 40eb2b8

   githubUtils.getOctokitOptions(core.getInput('repo-token', {required: true}))

   
   ...however, tokens shouldn't be hardcoded into config files anyway, so that's not an issue like the others.
4. Move the setting of each default value into the underlying code after checking to see if the parameter in question is not set in the inline configuration or an external settings file. This is the only suggested solution that isn't a breaking change, but it also makes the code more messy, since there are now three places that parameters could be set.

[febuiles]

@AlexWilson-GIS thank you for the very detailed report. Can you test the Action with the branch remove-warn-default and see if it fixes the issue?

    uses: actions/dependency-review-action@remove-warn-default

Pull request up here: #722

[AlexWilson-GIS]

Unfortunately, it appears to still be failing.

[febuiles]

@AlexWilson-GIS thanks for the update, we'll try to go through it this week and see what's up. If possible, please leave the reproduction repo up!

[febuiles]

@AlexWilson-GIS I think I understand what the problem is now: If you take a look at the raw log for your PR, the error you are running into is Error: Dependency review detected denied packages.. The denylist contains the faulty log4j package, so it'll fail regardless of warn_only or fail_on_severity.

I created this PR with the same pom.xml you are using, but without providing a denylist for log4j. You can see it has been marked as a successful run. If you dig into the logs however, you can see the vulnerable package:

Vulnerabilities
  pom.xml » org.apache.logging.log4j:[email protected] – Improper Input Validation and Injection in Apache Log4j2 (moderate severity)
    ↪ https://github.com/advisories/GHSA-8489-44mv-ggj8
  Warning: Dependency review detected vulnerable packages.

These are two different issues:

1. There is a problem in the latest release and in main with the handling of external config files (and other options). This PR provides a fix for that.
2. warn_only does not work in conjunction with deny_list. This is not a bug, but we might want to reconsider this interaction. The reasons for this behavior are historical, not technical.

I'm marking this as closed and will be merging the PR/doing a new release in the morning. If you feel we should change the behavior of warn_only to take deny_list into account (understandable!) please open a new issue (cc @jonjanego).

[AlexWilson-GIS]

Sounds good, thanks. I will open a new issue to request that.