Skip to content
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

Fix allow list with multiple license #131

Closed
wants to merge 12 commits into from

Conversation

kachick
Copy link
Contributor

@kachick kachick commented Jun 24, 2022

I have faced an issue to integrate stylelint. It depends meow, meow depends type-fest. Then type-fest has dual license.

Their license is written in SPDX 2.0, the syntax is recommended by npm.

GitHub REST API returns as below.

  {
    "change_type": "added",
    "manifest": "package-lock.json",
    "ecosystem": "npm",
    "name": "type-fest",
    "version": "0.18.1",
    "package_url": "pkg:npm/[email protected]",
    "license": "CC0-1.0 OR MIT OR (CC0-1.0 AND MIT)",
    "source_repository_url": "https://github.com/sindresorhus/type-fest",
    "vulnerabilities": [

    ]
  },
  {
    "change_type": "added",
    "manifest": "package-lock.json",
    "ecosystem": "npm",
    "name": "type-fest",
    "version": "0.8.1",
    "package_url": "pkg:npm/[email protected]",
    "license": "MIT OR (CC0-1.0 AND MIT)",
    "source_repository_url": "https://github.com/sindresorhus/type-fest",
    "vulnerabilities": [

    ]
  },
  {
    "change_type": "added",
    "manifest": "package-lock.json",
    "ecosystem": "npm",
    "name": "type-fest",
    "version": "0.6.0",
    "package_url": "pkg:npm/[email protected]",
    "license": "MIT OR (CC0-1.0 AND MIT)",
    "source_repository_url": "https://github.com/sindresorhus/type-fest",
    "vulnerabilities": [

    ]
  },

That is not simple to parse with current parsing logics.
So I have added the library of https://github.com/jslicense/spdx-satisfies.js.

Although, deny list looks hard to integrate to current spec. (And guessing hard to implement by me 🙇‍♂️ )
So I have added skipping test only for deny list. If it is not good, please remove.

@kachick kachick requested a review from a team as a code owner June 24, 2022 16:17
@kachick
Copy link
Contributor Author

kachick commented Jun 24, 2022

Run eslint with existing config makes many non related alerts. I have updated them. However when the diff is needless, I'll revert them. (Looks CI is not checking them) 🙏

kachick added a commit to kachick/kachick.github.io that referenced this pull request Jun 24, 2022
kachick added a commit to kachick/kachick.github.io that referenced this pull request Jun 24, 2022
* Integrate linter and formatters

* Allow CC

* Is this needed?

* Workaround to point actions/dependency-review-action#131
@@ -26,11 +26,11 @@ test('it defaults to low severity', async () => {

test('it reads custom configs', async () => {
setInput('fail-on-severity', 'critical')
setInput('allow-licenses', ' BSD, GPL 2')
setInput('allow-licenses', ' BSD-2-Clause, GPL-2.0')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spdx-satisfies.js throw errors if given string is invalid in SPDX list.

That might make broken CI for users, however the spec is written in README.md 🤔

# Possible values: Any `spdx_id` value(s) from https://docs.github.com/en/rest/licenses
# allow-licenses: GPL-3.0, BSD-3-Clause, MIT
#
# Possible values: Any `spdx_id` value(s) from https://docs.github.com/en/rest/licenses
# deny-licenses: LGPL-2.0, BSD-2-Clause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added kind error into early stage 6738e7d

@brphelps
Copy link
Contributor

@kachick -- I'm not super familiar with the licensing changes, but it looks like you've grouped together some unrelated changes like adding linting + linter integration. It seems like those changes warrant a PR split!

@kachick
Copy link
Contributor Author

kachick commented Jun 25, 2022

@brphelps Sure! I have updated this PR for focusing multiple license issue.

And extracted CI and lint in following PR kachick#1
I can send it after this PR closed.

@febuiles
Copy link
Contributor

@kachick Thanks a ton for highlighting this issue! I need to find out how the licenses are being handled in the backend, and see if it's expected that they will include SPDX expressions going forward. I'll post an update when I have more info!

kachick added a commit to kachick/wait-other-jobs that referenced this pull request Jul 5, 2022
* Extract dprint workflow for another trigger

* WIP

* Replace ts-node with swc-node

* `rm package-lock.json && npm install`

* Fix prepackage

* Use fork version to avoid false positive with swc dual license

actions/dependency-review-action#131
https://github.com/kachick/wait-other-jobs/runs/7176883355?check_suite_focus=true
https://github.com/swc-project/swc/blob/b79593f7ef7d104c88bf90c212019f473e6c1611/scripts/npm/linux-x64-gnu/package.json#L30

* `npm uninstall @swc-node/register`

* Better...?

* I am nervous

* It did not help me never. Just heavy
@actions actions deleted a comment from Baro1905 Aug 18, 2022
@actions actions deleted a comment from Baro1905 Aug 18, 2022
@febuiles
Copy link
Contributor

@kachick Going forward the API will be using SPDX licenses. I have some ideas about augmenting our current license dataset, but for the time being I think using spdx-satisfies is the proper way to move forward. What conflicts did you encounter when dealing with the denylist?

@kachick
Copy link
Contributor Author

kachick commented Aug 18, 2022

I couldn’t imagine reasonable specs for a denying condition and a given combining condition. 🤔

For example, below patterns.

Allow License Deny License Given License They should be…?
MIT GPL-1.0-or-later GPL-1.0-or-later AND MIT False
MIT GPL-1.0-or-later GPL-1.0-or-later OR MIT Validation error
(Empty list is given) GPL-1.0-or-later GPL-1.0-or-later OR MIT True?

Personally I hope false-positive is the preferable way for this action. Or raising errors for some patterns might be robust even if making many alerts.

@febuiles
Copy link
Contributor

Thanks for bringing up a good point! I will remove the example with an allow list since the action doesn't support both allow/deny: https://github.com/actions/dependency-review-action/#licenses. Using an example from an actual library:

Deny License Given License Result
MIT/X11 OR Apache-2.0 MIT/X11 Invalid license
MIT/X11 MIT/X11 OR Apache-2.0 Valid license

I've thought about this for a while, and the SPDX expression is valid, so we should not be biasing what happens here.

I was initially a bit concerned about SDPX expressions, but I've noticed that they are used more to express "This project uses license X AND Y" than "This project has uses X AND has used Y in the past".

I fetched a full list of unique licenses for all repos: https://gist.github.com/febuiles/985fb5215e72fd196cd9a7ea7fca1a2f. You can see there's a lot of funky stuff in there, but the actual number of repos with those long licenses is not significant.

@kachick what do you make out of all of this? Does it make sense to use spdx-satisfies to cover both allow/deny lists under this lens?

@kachick
Copy link
Contributor Author

kachick commented Oct 25, 2022

Sorry for my late response 🙇‍♂️

Looks updating this issue with #294. And this PR based on outdated code now. So please close this PR for now.

@kachick kachick closed this Oct 25, 2022
@kachick kachick deleted the multiple-licenses branch October 25, 2022 15:35
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.

None yet

3 participants