fix: match forbidden licenses by family, not exact string - #24
Merged
Conversation
The check compared each forbidden entry against the found-license array with `jq 'index($license)'`, which is exact element equality. With the org's `FORBIDDEN_LICENSES="GPL;AGPL"` that blocked only a license literally named "GPL" — no real dependency is. Every actual SPDX identifier passed: GPL-3.0-only, AGPL-3.0, and any composite such as "Apache-2.0 AND GPL-2.0". Both validators had it. An entry now names a family: it matches an identifier that equals it or continues it after `-`, `.` or `+`, case-insensitively, after the SPDX expression is split on AND / OR / WITH and parentheses. Family, not substring, so "GPL" does not silently start blocking LGPL and AGPL — those are different licenses, and a project that listed GPL has not thereby decided about them. Callers wanting them list them. test/forbidden-matching_test.sh extracts the block from the shipped action.yml rather than restating it, so it cannot keep passing after the code it covers has changed. It fails 18 of its 28 cases against the exact-match implementation.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Hinne1
marked this pull request as ready for review
August 3, 2026 08:57
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Both validators compared each forbidden entry against the found-license array with:
index()on an array is exact element equality. With the org-wideFORBIDDEN_LICENSES="GPL;AGPL", that blocks only a license literally namedGPL— which no real dependency carries. Measured against the current implementation:Every real SPDX identifier sails through. The gate has been effectively inert for its configured value.
Found while auditing warden and council for commercial redistribution — those closures happen to be clean (all permissive, verified independently), so nothing has slipped past. But the gate would not have caught it if something had.
The fix
An entry now names a family: it matches an identifier that equals it, or continues it after
-,.or+, case-insensitively — after the SPDX expression is split onAND/OR/WITHand parentheses, since a found value is an expression rather than a bare identifier.Family, not substring, deliberately.
GPLdoes not start blockingLGPL-3.0andAGPL-3.0— those are different licenses that merely contain the same letters, and a project that listedGPLhas not thereby made a decision about them. Callers wanting them list them (GPL;LGPL;AGPL). This is the one judgement call in the change and the easiest to reverse if the org wants the broader reading.The identifiers the READMEs document (
GPL-3.0;AGPL-3.0;SSPL-1.0) keep working, and now also match the modern-only/-or-laterspellings they predate.Both validators
npm-license-validatorcarried the identical block and the identical bug. Fixed in the same way.Test
The repo had no tests.
test/forbidden-matching_test.shextracts the block from the shippedaction.ymlrather than restating the logic, so it cannot keep passing after the code it claims to cover has changed. Wired intolint.ymlas aforbidden-license matchingjob.Verified it is a real regression test, not a vacuous one:
mainCases cover: family matching, LGPL/AGPL non-matching, composite
AND/OR/WITHexpressions, the documented full identifiers, case-insensitivity, the permissive-set and empty-list pass paths, and multi-family reporting.Also verified locally:
shellcheck --severity=infoclean on the new script, and the repo's own run-block shellcheck passes on both patchedaction.ymlfiles.Note
Repos relying on this gate have been getting less protection than the configuration implied. Worth a re-run on enrolled repos once this merges — a previously green License Check is not evidence of a clean closure.