Skip to content

Support SPDX expressions with operators in allow/deny license lists #916

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

jebeaudet
Copy link
Contributor

@jebeaudet jebeaudet commented Apr 9, 2025

This change updates license validation to support full SPDX expressions (such as 'EPL-1.0 AND LGPL-2.1') in both allow-lists and deny-lists. This enables the action to correctly validate packages that declare multiple licenses using SPDX conjunctions like AND/OR, which are common in the field.

One concrete example is logback-core which is licensed as EPL-1.0 AND LGPL-2.1 AND LGPL-2.1-only source. With the latest version of this action, it is not possible to have a SPDX expression in the allow list which makes this license impossible to accept. Also, declaring the 3 licenses as allowed individually (allow: ['EPL-1.0', 'LGPL-2.1', 'LGPL-2.1-only']) does not work either because satisfiesAny does not consider it as valid.

The new logic uses spdx.satisfies() to evaluate whether a package’s declared license satisfies any expression in the allow/deny list, and comprehensive tests have been added to verify behavior for various SPDX
combinations.

This is a follow up of #915 after we upgraded our internal license check pipeline to the latest version which made me realize this bug/limitation.

fixes #897 #907

This change updates license validation to support full SPDX expressions
(such as 'EPL-1.0 AND LGPL-2.1') in both allow-lists and deny-lists. This
enables the action to correctly validate packages that declare multiple
licenses using SPDX conjunctions like AND/OR, which are common in complex
open-source projects.

Previously, only simple license identifiers were supported, which caused
multi-licensed packages to be improperly flagged as invalid even when
they matched the intent of the allow-list.

The new logic uses `spdx.satisfies()` to evaluate whether a package’s
declared license satisfies any expression in the allow/deny list, and
comprehensive tests have been added to verify behavior for various SPDX
combinations.

This improves compatibility with projects using compound SPDX license
expressions and ensures more accurate license policy enforcement.
@Copilot Copilot AI review requested due to automatic review settings April 9, 2025 16:20
@jebeaudet jebeaudet requested a review from a team as a code owner April 9, 2025 16:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@jebeaudet
Copy link
Contributor Author

I added a small repository to highlight my problem https://github.com/jebeaudet/dep-review-bug-example.

You'll find there one PR that has the 3 licenses of logback-core individually so you can see the failure jebeaudet/dep-review-bug-example#1.

The other PR has the proper SPDX expression in the allow-list so you can see that the dependency-review-action does not allow SPDX expressions in the allow-list jebeaudet/dep-review-bug-example#2. Remember that we don't see the error because exceptions are swallowed by the action here. The exception message that would be printed can be found here.

Copy link

@JPLachance JPLachance left a comment

Choose a reason for hiding this comment

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

From what I understand, this change brings us back to a working implementation and fixes the issues introduced in v4.3.4.

Thanks!!!

@AshelyTC AshelyTC merged commit 5a5d4df into actions:main Apr 15, 2025
6 checks passed
@ahpook
Copy link
Contributor

ahpook commented Apr 18, 2025

@jebeaudet Hi, thank you so much for contributing this change! I'm working on validating it to see if we can make a new release that will hopefully address the problem for everyone.

To make sure I understand correctly: the goal is that an allow-licenses list that contains all three licenses as independent, comma-separated values A, B, C, should permit a package with an spdx expression of A AND B AND C.

I am still seeing a failure though, when using your test repo and running the workflow @ main to get the most recent code - check out:

ahpook/dep-review-bug-example#2

@jebeaudet
Copy link
Contributor Author

jebeaudet commented Apr 19, 2025

@ahpook Thanks for reviewing this. It was not super clear in my description, but A AND B AND C can only be allowed by an entry in the allow-list A AND B AND C. It makes sense when you think about it since a dual/triple licensed project is a combination of many licenses so allowing each license individually is not entirely the same.

You can see how my patches allow this here : jebeaudet/dep-review-bug-example#4

GPT answer on dual MIT AND Apache-2.0 licensed project From a license compliance perspective in a company, is accepting the licenses MIT and Apache-2.0 separately the same as allowing a dual licensed project that would have a spdx MIT AND Apache-2.0?

From a license compliance perspective, there is an important distinction between accepting projects licensed under MIT or Apache-2.0 individually, and accepting a project that is dual-licensed under MIT AND Apache-2.0.

Here’s the breakdown:

  1. Accepting MIT or Apache-2.0 separately

This means:
• Projects licensed under just MIT are OK.
• Projects licensed under just Apache-2.0 are OK.
• You evaluate and approve each license independently.

  1. Dual-licensed MIT AND Apache-2.0

This means:
• You must comply with both the MIT and Apache-2.0 licenses simultaneously.
• You do not get to choose one or the other; both apply.

This is different from:
• MIT OR Apache-2.0, where you choose either one license (more permissive)
• MIT AND Apache-2.0, where both sets of obligations must be met (more restrictive)

Compliance Implications:
• If your compliance policy only allows use of packages licensed under MIT or Apache-2.0 individually, it does not automatically allow a dual-licensed MIT AND Apache-2.0 project.
• For “AND” dual licenses, you have to ensure you can meet the conditions of both licenses:
• E.g., Apache-2.0 includes explicit patent grants and NOTICE file requirements, which MIT does not.
• So the combined license might have more obligations than either one alone.

Summary:

No, accepting MIT and Apache-2.0 separately is not the same as accepting a dual-licensed project with MIT AND Apache-2.0. You need to review and explicitly allow the combination.

@ahpook
Copy link
Contributor

ahpook commented Apr 23, 2025

@jebeaudet i see, thanks for the explanation. i was basing my expectations on the comments from @jcasner in #263:

For a situation like this, the action would parse the license field and use the operator (AND) to check that all 3 licenses are in our allow-list.

But your (and ChatGPT's 😄 ) reasoning makes sense. I'll get back and do more testing on this.

@jebeaudet
Copy link
Contributor Author

Yea, supporting MIT AND Apache-2.0 or MIT + Apache-2.0 individually is not the same, I believe the expectations in the OP of #263 are incorrect. I'll reach out to my legal team here just to confirm but it seems logical like this.

@jebeaudet
Copy link
Contributor Author

I've confirmed with my legal team internally that my answer, and chatgpt's, is correct.

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.

[BUG] validity could not be determined for (MIT OR Apache-2.0) AND Unicode-3.0
4 participants