[BUG] Release 4.3.4 breaking change with SPDX expressions

[jtomkiew-mng]

Describe the bug
Release 4.3.4 changes how license list is parsed and having SPDX expression like Apache-2.0 AND MIT now causes all license checks to fail. But also does not seem to work as intended (qustion mark?).

To Reproduce

1. specify Apache-2.0 AND MIT expression in the allow-licenses input, like so:

allow-licenses: >-
  Apache-2.0,
  Apache-2.0 AND MIT,
  MIT

2. have project with package using Apache-2.0 AND MIT license expression (like morelinq 4.2.0)
3. (optional) have other packages using Apache-2.0 or MIT licenses
4. run the action in versions 4.3.3 and 4.3.4

On version 4.3.3 it passes, on version 4.3.4 all packages fail with Incompatible License, and if Apache-2.0 AND MIT is removed from the list, only morelinq fails as incompatible.

Expected behavior
Apache-2.0 AND MIT should be able to pass.

Screenshots
v4.3.4 with Apache-2.0 AND MIT in allow-licenses input:

v4.3.4 without Apache-2.0 AND MIT:

v4.3.3 with Apache-2.0 AND MIT (no screenshot as it passes)

Action version
4.3.4

Examples
Project file:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <IsPackable>false</IsPackable>
        <IsTestProject>true</IsTestProject>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0"/>
        <PackageReference Include="xunit" Version="2.5.3"/>
        <PackageReference Include="xunit.runner.visualstudio" Version="2.5.3"/>
        <PackageReference Include="morelinq" Version="4.2.0"/>
    </ItemGroup>

</Project>

Action:

- name: Dependency Review
  uses: actions/dependency-review-action@v4.3.4
  with:
    license-check: true
    allow-licenses: >-
      Apache-2.0,
      Apache-2.0 AND MIT,
      MIT

Additional context
None.

[kade-robertson]

This also broke our usage of this action -- our allow-licenses is fairly simple (truncated but follows this pattern:

allow-licenses: (Apache-2.0 OR ... OR MIT OR ... OR MPL-2.0)

On v4.3.3 this was working fine, but now this fails on a license that should pass the check:

Edit: Here's a repo with a minimal reproduction of the issue, showing the same rule working on 4.3.3 but failing on 4.3.4: https://github.com/kade-robertson/dra-repro

[jonjanego]

Thanks for the report. We'll take a look.

[juxtin]

Hi folks, this was not intended to be a breaking change at all, so I do apologize for that.

I've spent some time investigating this and it appears that our new underlying SPDX parser does work a little bit differently here. When parsing allow-licenses and deny-licenses, it expects a simple list of license identifiers—so rather than MIT OR Apache-2.0, it expects just MIT, Apache-2.0. This is also technically what our documentation states, although clearly we've been looser about that in previous versions.

I think we can try to support more detailed expressions here in the future, but for now I believe the solution is to use only an array of license identifiers in allow-licenses and deny-licenses.

[lucacome]

@juxtin it seems like it's also failing with a single license

  The following dependencies have incompatible licenses:
  .github/workflows/dependency-review.yml » actions/dependency-review-action@5a2ce3f5b92ee19cbb1541a4984c76d921601d7c – License: MIT
  Error: Dependency review detected incompatible licenses.

when we have MIT in the allowed licenses
https://github.com/nginxinc/k8s-common/blob/main/dependency-review-config.yml#L8

[jtomkiew-mng]

I've spent some time investigating this and it appears that our new underlying SPDX parser does work a little bit differently here. When parsing allow-licenses and deny-licenses, it expects a simple list of license identifiers—so rather than MIT OR Apache-2.0, it expects just MIT, Apache-2.0. This is also technically what our documentation states, although clearly we've been looser about that in previous versions.

I think we can try to support more detailed expressions here in the future, but for now I believe the solution is to use only an array of license identifiers in allow-licenses and deny-licenses.

Using expressions directly in the allow-licenses input was just a workaround until support was implemented, we expected it to change at some point, so no worries here.

From the changelog of 4.3.4 I understood it that basic expressions like Apache-2.0 AND MIT should now work if allow-licenses contains correct licences, but as mentioned in the original post this exact case does not seem to work: morelinq 4.2.0 with Apache-2.0 AND MIT is rejected as invalid despite allow-licenses having both Apache-2.0 and MIT entries.

Is there a limitation somewhere on why it does not work with 4.3.4 or did I missunderstood something along the way?

[juxtin]

@lucacome I believe the problem is not just limited to the individual expression, but it may break license matching altogether. We may be able to improve our config validation to surface this in a clearer way.

In the meantime, does that still work when you remove lines like these that still have expressions?

[juxtin]

@jtomkiew-mng

From the changelog of 4.3.4 I understood it that basic expressions like Apache-2.0 AND MIT should now work if allow-licenses contains correct licences, but as mentioned in the original post this exact case does not seem to work: morelinq 4.2.0 with Apache-2.0 AND MIT is rejected as invalid despite allow-licenses having both Apache-2.0 and MIT entries.

The PR you're referring to was originally intended to fix just that kind of behavior by switching to a library that had better support for SPDX expression parsing. However, this didn't give us the kinds of easy wins that we'd hoped. Instead, I tried to make it clear in the changelog that this was mainly a refactor and wasn't expected to address many, if any, of the outstanding issues around improper handling of SPDX expressions.

That said, those issues are still on our radar and we do plan to address them in a future update.

[lucacome]

@juxtin removing those lines resolved the problem for us, thanks!