Discard allow list entries that are not SPDX IDs #932
Conversation
The allow-licenses list is expected (and documented) to be a list of SPDX license IDs (LicenseRefs are also valid). If someone puts an expression in the list (e.g. "GPL-3.0-only OR MIT"), it should be discarded so that the whole list does not become invalid. Fixes #907
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the allow list only contains simple SPDX IDs by discarding SPDX expressions with logical operators.
- Filter out allow-list entries containing
ANDorOR - Preserve
denylogic unchanged - Add a test to verify that expressions in the allow list do not trigger forbidden changes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/licenses.ts | Filter out AND/OR expressions from the allow list |
| tests/licenses.test.ts | Add test confirming expressions are ignored |
Comments suppressed due to low confidence (2)
tests/licenses.test.ts:293
- [nitpick] The test name is ambiguous about the behavior. Rename it to something like 'ignores license expressions in the allow list' to clearly state the intended outcome.
test('it does not fail if there is a license expression in the allow list', async () => {
tests/licenses.test.ts:293
- Add a test case for entries containing 'WITH' (e.g., 'GPL-2.0-only WITH Classpath-exception-2.0') to ensure all non-simple SPDX expressions are filtered out.
test('it does not fail if there is a license expression in the allow list', async () => {
| // or OR because the list should be simple license IDs and | ||
| // not expressions. | ||
| allow = allow?.filter(license => { | ||
| return !license.includes(' AND ') && !license.includes(' OR ') |
There was a problem hiding this comment.
The filter only excludes 'AND' and 'OR' expressions but ignores other expression syntax like 'WITH' or parentheses. Consider using a stricter check (e.g., a regex for simple IDs or spdx.isValid on each entry) to discard all non-simple SPDX IDs.
| return !license.includes(' AND ') && !license.includes(' OR ') | |
| return spdx.isValid(license) |
There was a problem hiding this comment.
I'm not sure if the library we're using supports WITH, but technically WITH does not represent a complex expression but rather a single license with an exception (eg GPL-2.0 WITH Classpath). It's better to potentially allow WITH.
I don't think Copilot's suggestion above works because spdx.isValid will allow license expressions and not just IDs.
Purpose
The allow-licenses list is expected (and documented) to be a list of
SPDX license IDs (LicenseRefs are also valid). If someone puts an
expression in the list (e.g. "GPL-3.0-only OR MIT"), it should be
discarded so that the whole list does not become invalid.
Related Issues
Closes #907